Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SentryFileWriter and SentryFileOutputStream append overwrites file contents #2304

Merged
merged 4 commits into from Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -7,6 +7,7 @@
- Ensure potential callback exceptions are caught #2123 ([#2291](https://github.com/getsentry/sentry-java/pull/2291))
- Remove verbose FrameMetricsAggregator failure logging ([#2293](https://github.com/getsentry/sentry-java/pull/2293))
- Ignore broken regex for tracePropagationTarget ([#2288](https://github.com/getsentry/sentry-java/pull/2288))
- Fix `SentryFileWriter`/`SentryFileOutputStream` append overwrites file contents ([#2304](https://github.com/getsentry/sentry-java/pull/2304))

### Features

Expand Down
Expand Up @@ -100,13 +100,13 @@ class SendCachedEnvelopeIntegrationTest {

@Test
fun `when synchronous send times out, continues the task on a background thread`() {
val sut = fixture.getSut(hasStartupCrashMarker = true, delaySend = 50)
fixture.options.startupCrashFlushTimeoutMillis = 10
val sut = fixture.getSut(hasStartupCrashMarker = true, delaySend = 1000)
fixture.options.startupCrashFlushTimeoutMillis = 100

sut.register(fixture.hub, fixture.options)

// first wait until synchronous send times out and check that the logger was hit in the catch block
await.atLeast(11, MILLISECONDS)
await.atLeast(500, MILLISECONDS)
verify(fixture.logger).log(
eq(DEBUG),
eq("Synchronous send timed out, continuing in the background.")
Expand Down
Expand Up @@ -24,7 +24,7 @@ public final class SentryFileOutputStream extends FileOutputStream {
private final @NotNull FileIOSpanManager spanManager;

public SentryFileOutputStream(final @Nullable String name) throws FileNotFoundException {
this(name != null ? new File(name) : null, HubAdapter.getInstance());
this(name != null ? new File(name) : null, false, HubAdapter.getInstance());
}

public SentryFileOutputStream(final @Nullable String name, final boolean append)
Expand All @@ -33,7 +33,7 @@ public SentryFileOutputStream(final @Nullable String name, final boolean append)
}

public SentryFileOutputStream(final @Nullable File file) throws FileNotFoundException {
this(file, HubAdapter.getInstance());
this(file, false, HubAdapter.getInstance());
}

public SentryFileOutputStream(final @Nullable File file, final boolean append)
Expand All @@ -45,9 +45,9 @@ public SentryFileOutputStream(final @NotNull FileDescriptor fdObj) {
this(init(fdObj, null, HubAdapter.getInstance()), fdObj);
}

SentryFileOutputStream(final @Nullable File file, final @NotNull IHub hub)
SentryFileOutputStream(final @Nullable File file, final boolean append, final @NotNull IHub hub)
throws FileNotFoundException {
this(init(file, false, null, hub));
this(init(file, append, null, hub));
}

private SentryFileOutputStream(
Expand All @@ -72,7 +72,7 @@ private static FileOutputStreamInitData init(
throws FileNotFoundException {
final ISpan span = FileIOSpanManager.startSpan(hub, "file.write");
if (delegate == null) {
delegate = new FileOutputStream(file);
delegate = new FileOutputStream(file, append);
}
return new FileOutputStreamInitData(
file, append, span, delegate, hub.getOptions().isSendDefaultPii());
Expand Down
Expand Up @@ -30,7 +30,8 @@ public SentryFileWriter(final @NotNull FileDescriptor fd) {
super(new SentryFileOutputStream(fd));
}

SentryFileWriter(final @NotNull File file, final @NotNull IHub hub) throws FileNotFoundException {
super(new SentryFileOutputStream(file, hub));
SentryFileWriter(final @NotNull File file, final boolean append, final @NotNull IHub hub)
throws FileNotFoundException {
super(new SentryFileOutputStream(file, append, hub));
}
}
Expand Up @@ -22,13 +22,14 @@ class SentryFileOutputStreamTest {
internal fun getSut(
tmpFile: File? = null,
activeTransaction: Boolean = true,
append: Boolean = false
): SentryFileOutputStream {
whenever(hub.options).thenReturn(SentryOptions())
sentryTracer = SentryTracer(TransactionContext("name", "op"), hub)
if (activeTransaction) {
whenever(hub.span).thenReturn(sentryTracer)
}
return SentryFileOutputStream(tmpFile, hub)
return SentryFileOutputStream(tmpFile, append, hub)
}
}

Expand Down
Expand Up @@ -21,13 +21,14 @@ class SentryFileWriterTest {
internal fun getSut(
tmpFile: File,
activeTransaction: Boolean = true,
append: Boolean = false
): SentryFileWriter {
whenever(hub.options).thenReturn(SentryOptions())
sentryTracer = SentryTracer(TransactionContext("name", "op"), hub)
if (activeTransaction) {
whenever(hub.span).thenReturn(sentryTracer)
}
return SentryFileWriter(tmpFile, hub)
return SentryFileWriter(tmpFile, append, hub)
}
}

Expand All @@ -36,7 +37,7 @@ class SentryFileWriterTest {

private val fixture = Fixture()

private val tmpFile: File get() = tmpDir.newFile("test.txt")
private val tmpFile: File by lazy { tmpDir.newFile("test.txt") }

@Test
fun `captures a span`() {
Expand All @@ -53,4 +54,20 @@ class SentryFileWriterTest {
assertEquals(fileIOSpan.isFinished, true)
assertEquals(fileIOSpan.status, OK)
}

@Test
fun `append works`() {
val writer1 = fixture.getSut(tmpFile, append = true)
writer1.use {
it.append("test")
}

// second writer should not overwrite the file contents, but append to the existing content
val writer2 = fixture.getSut(tmpFile, append = true)
writer2.use {
it.append("test2")
}

assertEquals("testtest2", tmpFile.readText())
}
}