Skip to content

Commit 644dcd8

Browse files
adinaueradinauer
committed
review feedback
1 parent 2c7271a commit 644dcd8

File tree

6 files changed

+69
-18
lines changed

6 files changed

+69
-18
lines changed

‎sentry-android-core/api/sentry-android-core.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,7 @@ public final class io/sentry/android/core/cache/AndroidEnvelopeCache : io/sentry
483483
public static fun hasStartupCrashMarker (Lio/sentry/SentryOptions;)Z
484484
public static fun lastReportedAnr (Lio/sentry/SentryOptions;)Ljava/lang/Long;
485485
public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
486+
public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
486487
}
487488

488489
public class io/sentry/android/core/performance/ActivityLifecycleCallbacksAdapter : android/app/Application$ActivityLifecycleCallbacks {

‎sentry-android-core/src/main/java/io/sentry/android/core/cache/AndroidEnvelopeCache.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,15 @@ public AndroidEnvelopeCache(final @NotNull SentryAndroidOptions options) {
5050
@SuppressWarnings("deprecation")
5151
@Override
5252
public void store(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
53+
storeInternalAndroid(envelope, hint);
54+
}
55+
56+
@Override
57+
public boolean storeEnvelope(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
58+
return storeInternalAndroid(envelope, hint);
59+
}
60+
61+
private boolean storeInternalAndroid(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
5362
super.store(envelope, hint);
5463

5564
final SentryAndroidOptions options = (SentryAndroidOptions) this.options;
@@ -84,6 +93,7 @@ public void store(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {
8493

8594
writeLastReportedAnrMarker(timestamp);
8695
});
96+
return true;
8797
}
8898

8999
@TestOnly

‎sentry/api/sentry.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4354,6 +4354,7 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache {
43544354
public static fun getPreviousSessionFile (Ljava/lang/String;)Ljava/io/File;
43554355
public fun iterator ()Ljava/util/Iterator;
43564356
public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
4357+
public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z
43574358
public fun waitPreviousSessionFlush ()Z
43584359
}
43594360

‎sentry/src/main/java/io/sentry/cache/EnvelopeCache.java‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,15 @@ public EnvelopeCache(
9696
@SuppressWarnings("deprecation")
9797
@Override
9898
public void store(final @NotNull SentryEnvelope envelope, final @NotNull Hint hint) {
99+
storeInternal(envelope, hint);
100+
}
101+
102+
@Override
103+
public boolean storeEnvelope(final @NotNull SentryEnvelope envelope, final @NotNull Hint hint) {
104+
return storeInternal(envelope, hint);
105+
}
106+
107+
private boolean storeInternal(final @NotNull SentryEnvelope envelope, final @NotNull Hint hint) {
99108
Objects.requireNonNull(envelope, "Envelope is required.");
100109

101110
rotateCacheIfNeeded(allEnvelopeFiles());
@@ -172,19 +181,20 @@ public void store(final @NotNull SentryEnvelope envelope, final @NotNull Hint hi
172181
WARNING,
173182
"Not adding Envelope to offline storage because it already exists: %s",
174183
envelopeFile.getAbsolutePath());
175-
return;
184+
return true;
176185
} else {
177186
options
178187
.getLogger()
179188
.log(DEBUG, "Adding Envelope to offline storage: %s", envelopeFile.getAbsolutePath());
180189
}
181190

182-
writeEnvelopeToDisk(envelopeFile, envelope);
191+
final boolean didWriteToDisk = writeEnvelopeToDisk(envelopeFile, envelope);
183192

184193
// write file to the disk when its about to crash so crashedLastRun can be marked on restart
185194
if (HintUtils.hasType(hint, UncaughtExceptionHandlerIntegration.UncaughtExceptionHint.class)) {
186195
writeCrashMarkerFile();
187196
}
197+
return didWriteToDisk;
188198
}
189199

190200
/**
@@ -296,7 +306,7 @@ private void updateCurrentSession(
296306
}
297307
}
298308

299-
private void writeEnvelopeToDisk(
309+
private boolean writeEnvelopeToDisk(
300310
final @NotNull File file, final @NotNull SentryEnvelope envelope) {
301311
if (file.exists()) {
302312
options
@@ -313,7 +323,9 @@ private void writeEnvelopeToDisk(
313323
options
314324
.getLogger()
315325
.log(ERROR, e, "Error writing Envelope %s to offline storage", file.getAbsolutePath());
326+
return false;
316327
}
328+
return true;
317329
}
318330

319331
private void writeSessionToDisk(final @NotNull File file, final @NotNull Session session) {

‎sentry/src/test/java/io/sentry/cache/EnvelopeCacheTest.kt‎

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package io.sentry.cache
33
import io.sentry.DateUtils
44
import io.sentry.Hint
55
import io.sentry.ILogger
6+
import io.sentry.ISerializer
67
import io.sentry.NoOpLogger
78
import io.sentry.SentryCrashLastRunState
89
import io.sentry.SentryEnvelope
@@ -31,20 +32,25 @@ import kotlin.test.assertEquals
3132
import kotlin.test.assertFalse
3233
import kotlin.test.assertNotNull
3334
import kotlin.test.assertTrue
35+
import org.mockito.kotlin.any
3436
import org.mockito.kotlin.mock
37+
import org.mockito.kotlin.same
38+
import org.mockito.kotlin.whenever
3539

3640
class EnvelopeCacheTest {
3741
private class Fixture {
3842
val dir: Path = Files.createTempDirectory("sentry-session-cache-test")
3943
val options = SentryOptions()
4044
val logger = mock<ILogger>()
4145

42-
fun getSUT(): EnvelopeCache {
46+
fun getSUT(optionsCallback: ((SentryOptions) -> Unit)? = null): EnvelopeCache {
4347
options.cacheDirPath = dir.toAbsolutePath().toFile().absolutePath
4448

4549
options.setLogger(logger)
4650
options.setDebug(true)
4751

52+
optionsCallback?.invoke(options)
53+
4854
return EnvelopeCache.create(options) as EnvelopeCache
4955
}
5056
}
@@ -90,13 +96,15 @@ class EnvelopeCacheTest {
9096
val envelope = SentryEnvelope.from(fixture.options.serializer, createSession(), null)
9197

9298
val hints = HintUtils.createWithTypeCheckHint(SessionStartHint())
93-
cache.store(envelope, hints)
99+
val didStore = cache.storeEnvelope(envelope, hints)
94100

95101
val currentFile =
96102
File(fixture.options.cacheDirPath!!, "$PREFIX_CURRENT_SESSION_FILE$SUFFIX_SESSION_FILE")
97103
assertTrue(currentFile.exists())
98104

99105
file.deleteRecursively()
106+
107+
assertTrue(didStore)
100108
}
101109

102110
@Test
@@ -108,7 +116,7 @@ class EnvelopeCacheTest {
108116
val envelope = SentryEnvelope.from(fixture.options.serializer, createSession(), null)
109117

110118
val hints = HintUtils.createWithTypeCheckHint(SessionStartHint())
111-
cache.store(envelope, hints)
119+
val didStore = cache.storeEnvelope(envelope, hints)
112120

113121
val currentFile =
114122
File(fixture.options.cacheDirPath!!, "$PREFIX_CURRENT_SESSION_FILE$SUFFIX_SESSION_FILE")
@@ -119,6 +127,7 @@ class EnvelopeCacheTest {
119127
assertFalse(currentFile.exists())
120128

121129
file.deleteRecursively()
130+
assertTrue(didStore)
122131
}
123132

124133
@Test
@@ -130,7 +139,7 @@ class EnvelopeCacheTest {
130139
val envelope = SentryEnvelope.from(fixture.options.serializer, createSession(), null)
131140

132141
val hints = HintUtils.createWithTypeCheckHint(SessionStartHint())
133-
cache.store(envelope, hints)
142+
val didStore = cache.storeEnvelope(envelope, hints)
134143

135144
val currentFile =
136145
File(fixture.options.cacheDirPath!!, "$PREFIX_CURRENT_SESSION_FILE$SUFFIX_SESSION_FILE")
@@ -146,6 +155,7 @@ class EnvelopeCacheTest {
146155
currentFile.delete()
147156

148157
file.deleteRecursively()
158+
assertTrue(didStore)
149159
}
150160

151161
@Test
@@ -160,7 +170,7 @@ class EnvelopeCacheTest {
160170
val envelope = SentryEnvelope.from(fixture.options.serializer, createSession(), null)
161171

162172
val hints = HintUtils.createWithTypeCheckHint(SessionStartHint())
163-
cache.store(envelope, hints)
173+
val didStore = cache.storeEnvelope(envelope, hints)
164174

165175
val newEnvelope = SentryEnvelope.from(fixture.options.serializer, createSession(), null)
166176

@@ -172,6 +182,7 @@ class EnvelopeCacheTest {
172182

173183
// passing empty string since readCrashedLastRun is already set
174184
assertTrue(SentryCrashLastRunState.getInstance().isCrashedLastRun("", false)!!)
185+
assertTrue(didStore)
175186
}
176187

177188
@Test
@@ -185,11 +196,12 @@ class EnvelopeCacheTest {
185196
val envelope = SentryEnvelope.from(fixture.options.serializer, createSession(), null)
186197

187198
val hints = HintUtils.createWithTypeCheckHint(SessionStartHint())
188-
cache.store(envelope, hints)
199+
val didStore = cache.storeEnvelope(envelope, hints)
189200

190201
// passing empty string since readCrashedLastRun is already set
191202
assertTrue(SentryCrashLastRunState.getInstance().isCrashedLastRun("", false)!!)
192203
assertFalse(markerFile.exists())
204+
assertTrue(didStore)
193205
}
194206

195207
@Test
@@ -203,9 +215,10 @@ class EnvelopeCacheTest {
203215

204216
val hints =
205217
HintUtils.createWithTypeCheckHint(UncaughtExceptionHint(0, NoOpLogger.getInstance()))
206-
cache.store(envelope, hints)
218+
val didStore = cache.storeEnvelope(envelope, hints)
207219

208220
assertTrue(markerFile.exists())
221+
assertTrue(didStore)
209222
}
210223

211224
@Test
@@ -214,7 +227,7 @@ class EnvelopeCacheTest {
214227

215228
val envelope = SentryEnvelope.from(fixture.options.serializer, SentryEvent(), null)
216229
val hints = HintUtils.createWithTypeCheckHint(SessionStartHint())
217-
cache.store(envelope, hints)
230+
cache.storeEnvelope(envelope, hints)
218231

219232
assertTrue(cache.waitPreviousSessionFlush())
220233
}
@@ -232,7 +245,7 @@ class EnvelopeCacheTest {
232245

233246
val envelope = SentryEnvelope.from(fixture.options.serializer, SentryEvent(), null)
234247
val hints = HintUtils.createWithTypeCheckHint(SessionStartHint())
235-
cache.store(envelope, hints)
248+
cache.storeEnvelope(envelope, hints)
236249

237250
assertTrue(previousSessionFile.exists())
238251
val persistedSession =
@@ -261,7 +274,7 @@ class EnvelopeCacheTest {
261274
override fun timestamp(): Long? = null
262275
}
263276
val hints = HintUtils.createWithTypeCheckHint(abnormalHint)
264-
cache.store(envelope, hints)
277+
cache.storeEnvelope(envelope, hints)
265278

266279
val updatedSession =
267280
fixture.options.serializer.deserialize(
@@ -293,7 +306,7 @@ class EnvelopeCacheTest {
293306
override fun timestamp(): Long = sessionExitedWithAbnormal
294307
}
295308
val hints = HintUtils.createWithTypeCheckHint(abnormalHint)
296-
cache.store(envelope, hints)
309+
cache.storeEnvelope(envelope, hints)
297310

298311
val updatedSession =
299312
fixture.options.serializer.deserialize(
@@ -323,7 +336,7 @@ class EnvelopeCacheTest {
323336
override fun timestamp(): Long = sessionExitedWithAbnormal
324337
}
325338
val hints = HintUtils.createWithTypeCheckHint(abnormalHint)
326-
cache.store(envelope, hints)
339+
cache.storeEnvelope(envelope, hints)
327340

328341
val updatedSession =
329342
fixture.options.serializer.deserialize(
@@ -334,6 +347,20 @@ class EnvelopeCacheTest {
334347
assertEquals(null, updatedSession.abnormalMechanism)
335348
}
336349

350+
@Test
351+
fun `failing to store returns false`() {
352+
val serializer = mock<ISerializer>()
353+
val envelope = SentryEnvelope.from(SentryOptions.empty().serializer, createSession(), null)
354+
355+
whenever(serializer.serialize(same(envelope), any())).thenThrow(RuntimeException("forced ex"))
356+
357+
val cache = fixture.getSUT { options -> options.setSerializer(serializer) }
358+
359+
val didStore = cache.storeEnvelope(envelope, Hint())
360+
361+
assertFalse(didStore)
362+
}
363+
337364
private fun createSession(started: Date? = null): Session =
338365
Session(
339366
Ok,

‎sentry/src/test/java/io/sentry/transport/AsyncHttpTransportTest.kt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ class AsyncHttpTransportTest {
8585
val order = inOrder(fixture.connection, fixture.sentryOptions.envelopeDiskCache)
8686

8787
// because storeBeforeSend is enabled by default
88-
order.verify(fixture.sentryOptions.envelopeDiskCache).store(eq(envelope), anyOrNull())
88+
order.verify(fixture.sentryOptions.envelopeDiskCache).storeEnvelope(eq(envelope), anyOrNull())
8989

9090
order.verify(fixture.connection).send(eq(envelope))
9191
order.verify(fixture.sentryOptions.envelopeDiskCache).discard(eq(envelope))
@@ -102,7 +102,7 @@ class AsyncHttpTransportTest {
102102
fixture.getSUT().send(envelope)
103103

104104
// then
105-
verify(fixture.sentryOptions.envelopeDiskCache).store(eq(envelope), anyOrNull())
105+
verify(fixture.sentryOptions.envelopeDiskCache).storeEnvelope(eq(envelope), anyOrNull())
106106
verify(fixture.rateLimiter).filter(eq(envelope), anyOrNull())
107107
}
108108

@@ -125,7 +125,7 @@ class AsyncHttpTransportTest {
125125
val order = inOrder(fixture.connection, fixture.sentryOptions.envelopeDiskCache)
126126

127127
// because storeBeforeSend is enabled by default
128-
order.verify(fixture.sentryOptions.envelopeDiskCache).store(eq(envelope), anyOrNull())
128+
order.verify(fixture.sentryOptions.envelopeDiskCache).storeEnvelope(eq(envelope), anyOrNull())
129129

130130
order.verify(fixture.connection).send(eq(envelope))
131131
verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any())

0 commit comments

Comments
 (0)