Address PR review

CHANGELOG.md

@@ -16,7 +16,9 @@ Breaking changes:
 - Automatic user interaction tracking: every click now starts a new automatic transaction ([#2891](https://github.com/getsentry/sentry-java/pull/2891))
   - Previously performing a click on the same UI widget twice would keep the existing transaction running, the new behavior now better aligns with other SDKs
 - Android only: If global hub mode is enabled, Sentry.getSpan() returns the root span instead of the latest span ([#2855](https://github.com/getsentry/sentry-java/pull/2855))
-- Android only: Observe network state to upload any unsent envelopes ([#2910](https://github.com/getsentry/sentry-java/pull/2910))
+- Observe network state to upload any unsent envelopes ([#2910](https://github.com/getsentry/sentry-java/pull/2910))
+  - Android: it works out-of-the-box as part of the default `SendCachedEnvelopeIntegration`
+  - JVM: you'd have to install `SendCachedEnvelopeFireAndForgetIntegration` as mentioned in https://docs.sentry.io/platforms/java/configuration/#configuring-offline-caching and provide your own implementation of `IConnectionStatusProvider` via `SentryOptions`
 - Do not try to send and drop cached envelopes when rate-limiting is active ([#2937](https://github.com/getsentry/sentry-java/pull/2937))
 
 ### Fixes

sentry-android-core/src/main/java/io/sentry/android/core/SendCachedEnvelopeIntegration.java

@@ -41,8 +41,7 @@ public SendCachedEnvelopeIntegration(
 
   @Override
   public void register(@NotNull IHub hub, @NotNull SentryOptions options) {
-    Objects.requireNonNull(hub, "Hub is required");
-    this.hub = hub;
+    this.hub = Objects.requireNonNull(hub, "Hub is required");
     this.options =
         Objects.requireNonNull(
             (options instanceof SentryAndroidOptions) ? (SentryAndroidOptions) options : null,
@@ -70,7 +69,8 @@ public void close() throws IOException {
   }
 
   @Override
-  public void onConnectionStatusChanged(IConnectionStatusProvider.ConnectionStatus status) {
+  public void onConnectionStatusChanged(
+      final @NotNull IConnectionStatusProvider.ConnectionStatus status) {
     if (hub != null && options != null) {
       sendCachedEnvelopes(hub, options);
     }

sentry-android-core/src/main/java/io/sentry/android/core/internal/util/AndroidConnectionStatusProvider.java

@@ -64,7 +64,7 @@ public AndroidConnectionStatusProvider(
 
   @SuppressLint("NewApi") // we have an if-check for that down below
   @Override
-  public boolean addConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) {
+  public boolean addConnectionStatusObserver(final @NotNull IConnectionStatusObserver observer) {
     if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) {
       logger.log(SentryLevel.DEBUG, "addConnectionStatusObserver requires Android 5+.");
       return false;
@@ -98,7 +98,7 @@ public void onUnavailable() {
   }
 
   @Override
-  public void removeConnectionStatusObserver(@NotNull IConnectionStatusObserver observer) {
+  public void removeConnectionStatusObserver(final @NotNull IConnectionStatusObserver observer) {
     final @Nullable ConnectivityManager.NetworkCallback callback =
         registeredCallbacks.remove(observer);
     if (callback != null) {

sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidTest.kt

@@ -445,7 +445,6 @@ class SentryAndroidTest {
     private class CustomEnvelopCache : IEnvelopeCache {
         override fun iterator(): MutableIterator<SentryEnvelope> = TODO()
         override fun store(envelope: SentryEnvelope, hint: Hint) = Unit
-        override fun discard(envelope: SentryEnvelope, hint: Hint) = Unit
-        override fun containsFile(file: File): Boolean = false
+        override fun discard(envelope: SentryEnvelope) = Unit
     }
 }

sentry/api/sentry.api

@@ -2554,9 +2554,8 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache {
 	public static final field SUFFIX_ENVELOPE_FILE Ljava/lang/String;
 	protected static final field UTF_8 Ljava/nio/charset/Charset;
 	public fun <init> (Lio/sentry/SentryOptions;Ljava/lang/String;I)V
-	public fun containsFile (Ljava/io/File;)Z
 	public static fun create (Lio/sentry/SentryOptions;)Lio/sentry/cache/IEnvelopeCache;
-	public fun discard (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
+	public fun discard (Lio/sentry/SentryEnvelope;)V
 	public fun flushPreviousSession ()V
 	public static fun getCurrentSessionFile (Ljava/lang/String;)Ljava/io/File;
 	public static fun getPreviousSessionFile (Ljava/lang/String;)Ljava/io/File;
@@ -2566,8 +2565,7 @@ public class io/sentry/cache/EnvelopeCache : io/sentry/cache/IEnvelopeCache {
 }
 
 public abstract interface class io/sentry/cache/IEnvelopeCache : java/lang/Iterable {
-	public abstract fun containsFile (Ljava/io/File;)Z
-	public abstract fun discard (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
+	public abstract fun discard (Lio/sentry/SentryEnvelope;)V
 	public fun store (Lio/sentry/SentryEnvelope;)V
 	public abstract fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
 }
@@ -4177,8 +4175,7 @@ public abstract interface class io/sentry/transport/ITransportGate {
 
 public final class io/sentry/transport/NoOpEnvelopeCache : io/sentry/cache/IEnvelopeCache {
 	public fun <init> ()V
-	public fun containsFile (Ljava/io/File;)Z
-	public fun discard (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V
+	public fun discard (Lio/sentry/SentryEnvelope;)V
 	public static fun getInstance ()Lio/sentry/transport/NoOpEnvelopeCache;
 	public fun iterator ()Ljava/util/Iterator;
 	public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V

sentry/src/main/java/io/sentry/IConnectionStatusProvider.java

@@ -20,7 +20,7 @@ interface IConnectionStatusObserver {
      *
      * @param status the new connection status
      */
-    void onConnectionStatusChanged(ConnectionStatus status);
+    void onConnectionStatusChanged(@NotNull ConnectionStatus status);
   }
 
   /**

sentry/src/main/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegration.java

@@ -64,11 +64,8 @@ public SendCachedEnvelopeFireAndForgetIntegration(
 
   @Override
   public void register(final @NotNull IHub hub, final @NotNull SentryOptions options) {
-    Objects.requireNonNull(hub, "Hub is required");
-    Objects.requireNonNull(options, "SentryOptions is required");
-
-    this.hub = hub;
-    this.options = options;
+    this.hub = Objects.requireNonNull(hub, "Hub is required");
+    this.options = Objects.requireNonNull(options, "SentryOptions is required");
 
     final String cachedDir = options.getCacheDirPath();
     if (!factory.hasValidPath(cachedDir, options.getLogger())) {
@@ -97,14 +94,16 @@ public void close() throws IOException {
   }
 
   @Override
-  public void onConnectionStatusChanged(IConnectionStatusProvider.ConnectionStatus status) {
+  public void onConnectionStatusChanged(
+      final @NotNull IConnectionStatusProvider.ConnectionStatus status) {
     if (hub != null && options != null) {
       sendCachedEnvelopes(hub, options);
     }
   }
 
   @SuppressWarnings({"FutureReturnValueIgnored", "NullAway"})
-  private synchronized void sendCachedEnvelopes(@NotNull IHub hub, @NotNull SentryOptions options) {
+  private synchronized void sendCachedEnvelopes(
+      final @NotNull IHub hub, final @NotNull SentryOptions options) {
 
     // skip run only if we're certainly disconnected
     if (connectionStatusProvider != null

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

@@ -335,7 +335,7 @@ private void writeSessionToDisk(final @NotNull File file, final @NotNull Session
   }
 
   @Override
-  public void discard(final @NotNull SentryEnvelope envelope, final @NotNull Hint hint) {
+  public void discard(final @NotNull SentryEnvelope envelope) {
     Objects.requireNonNull(envelope, "Envelope is required.");
 
     final File envelopeFile = getEnvelopeFile(envelope);
@@ -416,11 +416,6 @@ public void discard(final @NotNull SentryEnvelope envelope, final @NotNull Hint
     return ret.iterator();
   }
 
-  @Override
-  public synchronized boolean containsFile(@NotNull File file) {
-    return fileNameMap.containsValue(file.getAbsolutePath());
-  }
-
   private @NotNull File[] allEnvelopeFiles() {
     if (isDirectoryValid()) {
       // lets filter the session.json here

sentry/src/main/java/io/sentry/cache/IEnvelopeCache.java

@@ -2,7 +2,6 @@
 
 import io.sentry.Hint;
 import io.sentry.SentryEnvelope;
-import java.io.File;
 import org.jetbrains.annotations.NotNull;
 
 public interface IEnvelopeCache extends Iterable<SentryEnvelope> {
@@ -13,7 +12,5 @@ default void store(@NotNull SentryEnvelope envelope) {
     store(envelope, new Hint());
   }
 
-  void discard(@NotNull SentryEnvelope envelope, @NotNull Hint hint);
-
-  boolean containsFile(@NotNull File file);
+  void discard(@NotNull SentryEnvelope envelope);
 }

sentry/src/main/java/io/sentry/hints/Enqueable.java

@@ -1,5 +1,6 @@
 package io.sentry.hints;
 
+/** Marker interface for envelopes to notify when they are submitted to the http transport queue */
 public interface Enqueable {
   void markEnqueued();
 }

sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java

@@ -84,7 +84,7 @@ public void send(final @NotNull SentryEnvelope envelope, final @NotNull Hint hin
 
     if (filteredEnvelope == null) {
       if (cached) {
-        envelopeCache.discard(envelope, hint);
+        envelopeCache.discard(envelope);
       }
     } else {
       SentryEnvelope envelopeThatMayIncludeClientReport;
@@ -268,7 +268,7 @@ public void run() {
 
           result = connection.send(envelopeWithClientReport);
           if (result.isSuccess()) {
-            envelopeCache.discard(envelope, hint);
+            envelopeCache.discard(envelope);
           } else {
             final String message =
                 "The transport failed to send the envelope with response code "

sentry/src/main/java/io/sentry/transport/NoOpEnvelopeCache.java

@@ -3,7 +3,6 @@
 import io.sentry.Hint;
 import io.sentry.SentryEnvelope;
 import io.sentry.cache.IEnvelopeCache;
-import java.io.File;
 import java.util.Collections;
 import java.util.Iterator;
 import org.jetbrains.annotations.NotNull;
@@ -19,16 +18,11 @@ public static NoOpEnvelopeCache getInstance() {
   public void store(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {}
 
   @Override
-  public void discard(@NotNull SentryEnvelope envelope, @NotNull Hint hint) {}
+  public void discard(@NotNull SentryEnvelope envelope) {}
 
   @NotNull
   @Override
   public Iterator<SentryEnvelope> iterator() {
     return Collections.emptyIterator();
   }
-
-  @Override
-  public boolean containsFile(@NotNull File file) {
-    return false;
-  }
 }

sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt

@@ -3,6 +3,7 @@ package io.sentry
 import io.sentry.SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget
 import io.sentry.protocol.SdkVersion
 import io.sentry.test.ImmediateExecutorService
+import io.sentry.transport.RateLimiter
 import org.mockito.kotlin.any
 import org.mockito.kotlin.eq
 import org.mockito.kotlin.mock
@@ -174,6 +175,20 @@ class SendCachedEnvelopeFireAndForgetIntegrationTest {
         verify(fixture.sender, times(3)).send()
     }
 
+    @Test
+    fun `when rate limiter is active, does not send envelopes`() {
+        val sut = fixture.getSut()
+        val rateLimiter = mock<RateLimiter> {
+            whenever(mock.isActiveForCategory(any())).thenReturn(true)
+        }
+        whenever(fixture.hub.rateLimiter).thenReturn(rateLimiter)
+
+        sut.register(fixture.hub, fixture.options)
+
+        // no factory call should be done if there's rate limiting active
+        verify(fixture.sender, never()).send()
+    }
+
     private class CustomFactory : SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForgetFactory {
         override fun create(hub: IHub, options: SentryOptions): SendCachedEnvelopeFireAndForgetIntegration.SendFireAndForget? {
             return null

sentry/src/test/java/io/sentry/SentryTest.kt

@@ -890,8 +890,7 @@ class SentryTest {
     private class CustomEnvelopCache : IEnvelopeCache {
         override fun iterator(): MutableIterator<SentryEnvelope> = TODO()
         override fun store(envelope: SentryEnvelope, hint: Hint) = Unit
-        override fun discard(envelope: SentryEnvelope, hint: Hint) = Unit
-        override fun containsFile(file: File): Boolean = false
+        override fun discard(envelope: SentryEnvelope) = Unit
     }
 
     private fun getTempPath(): String {

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

@@ -1,7 +1,6 @@
 package io.sentry.cache
 
 import io.sentry.DateUtils
-import io.sentry.Hint
 import io.sentry.ILogger
 import io.sentry.NoOpLogger
 import io.sentry.SentryCrashLastRunState
@@ -75,7 +74,7 @@ class EnvelopeCacheTest {
     fun `tolerates discarding unknown envelope`() {
         val cache = fixture.getSUT()
 
-        cache.discard(SentryEnvelope.from(fixture.options.serializer, createSession(), null), Hint())
+        cache.discard(SentryEnvelope.from(fixture.options.serializer, createSession(), null))
 
         // no exception thrown
     }

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

@@ -2,7 +2,6 @@ package io.sentry.transport
 
 import io.sentry.CachedEvent
 import io.sentry.Hint
-import io.sentry.Sentry
 import io.sentry.SentryEnvelope
 import io.sentry.SentryEnvelopeHeader
 import io.sentry.SentryEnvelopeItem
@@ -82,7 +81,7 @@ class AsyncHttpTransportTest {
         order.verify(fixture.sentryOptions.envelopeDiskCache).store(eq(envelope), anyOrNull())
 
         order.verify(fixture.connection).send(eq(envelope))
-        order.verify(fixture.sentryOptions.envelopeDiskCache).discard(eq(envelope), any())
+        order.verify(fixture.sentryOptions.envelopeDiskCache).discard(eq(envelope))
     }
 
     @Test
@@ -122,7 +121,7 @@ class AsyncHttpTransportTest {
         order.verify(fixture.sentryOptions.envelopeDiskCache).store(eq(envelope), anyOrNull())
 
         order.verify(fixture.connection).send(eq(envelope))
-        verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any(), any())
+        verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any())
     }
 
     @Test
@@ -143,7 +142,7 @@ class AsyncHttpTransportTest {
         // then
         val order = inOrder(fixture.connection, fixture.sentryOptions.envelopeDiskCache)
         order.verify(fixture.connection).send(eq(envelope))
-        verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any(), any())
+        verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any())
     }
 
     @Test
@@ -158,10 +157,6 @@ class AsyncHttpTransportTest {
 
         // then
         verify(fixture.executor, never()).submit(any())
-
-        Sentry.withScope {
-            val installationId = it.user?.id
-        }
     }
 
     @Test
@@ -202,7 +197,7 @@ class AsyncHttpTransportTest {
         fixture.getSUT().send(envelope, hints)
 
         // then
-        verify(fixture.sentryOptions.envelopeDiskCache).discard(any(), any())
+        verify(fixture.sentryOptions.envelopeDiskCache).discard(any())
     }
 
     @Test
@@ -215,7 +210,7 @@ class AsyncHttpTransportTest {
         fixture.getSUT().send(envelope)
 
         // then
-        verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any(), any())
+        verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any())
     }
 
     @Test
@@ -260,7 +255,7 @@ class AsyncHttpTransportTest {
         fixture.getSUT().send(envelope, hints)
 
         // then
-        verify(fixture.sentryOptions.envelopeDiskCache).discard(any(), any())
+        verify(fixture.sentryOptions.envelopeDiskCache).discard(any())
     }
 
     @Test
@@ -274,7 +269,7 @@ class AsyncHttpTransportTest {
         fixture.getSUT().send(envelope)
 
         // then
-        verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any(), any())
+        verify(fixture.sentryOptions.envelopeDiskCache, never()).discard(any())
     }
 
     @Test