Additional EmbraceUrlConnectionDelegate unit tests and bug fixes

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/internal/network/http/EmbraceHttpUrlConnection.java

@@ -73,8 +73,6 @@ interface EmbraceHttpUrlConnection {
     @Nullable
     InputStream getErrorStream();
 
-    boolean shouldInterceptHeaderRetrieval(@Nullable String key);
-
     @Nullable
     String getHeaderField(int n);
 

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/internal/network/http/EmbraceHttpUrlConnectionOverride.java

@@ -28,7 +28,7 @@ public String getHeaderByName(@NonNull String name) {
     public String getOverriddenURL(@NonNull String pathOverride) {
         try {
             return new URL(connection.getURL().getProtocol(), connection.getURL().getHost(),
-                connection.getURL().getPort(), pathOverride).toString();
+                connection.getURL().getPort(), pathOverride + "?" + connection.getURL().getQuery()).toString();
         } catch (MalformedURLException e) {
             InternalStaticEmbraceLogger.logError("Failed to override path of " +
                 connection.getURL() + " with " + pathOverride);

embrace-android-sdk/src/main/java/io/embrace/android/embracesdk/internal/network/http/EmbraceUrlConnectionDelegate.java

@@ -63,12 +63,12 @@
     /**
      * The content encoding HTTP header.
      */
-    private static final String CONTENT_ENCODING = "Content-Encoding";
+    static final String CONTENT_ENCODING = "Content-Encoding";
 
     /**
      * The content length HTTP header.
      */
-    private static final String CONTENT_LENGTH = "Content-Length";
+    static final String CONTENT_LENGTH = "Content-Length";
 
     /**
      * Reference to the wrapped connection.
@@ -140,8 +140,7 @@
     @Nullable
     private volatile String traceparent = null;
 
-    @Nullable
-    private volatile byte[] responseBody = null;
+    private final boolean isSDKStarted;
 
     /**
      * Wraps an existing {@link HttpURLConnection} with the Embrace network logic.
@@ -160,6 +159,7 @@
         this.embrace = embrace;
         this.createdTime = embrace.getInternalInterface().getSdkCurrentTime();
         this.callId = UUID.randomUUID().toString();
+        this.isSDKStarted = embrace.isStarted();
     }
 
     @Override
@@ -169,21 +169,23 @@
 
     @Override
     public void connect() throws IOException {
-        identifyTraceId();
-        try {
-            if (embrace.getInternalInterface().isNetworkSpanForwardingEnabled()) {
-                traceparent = connection.getRequestProperty(TRACEPARENT_HEADER_NAME);
+        if (isSDKStarted) {
+            identifyTraceId();
+            try {
+                if (embrace.getInternalInterface().isNetworkSpanForwardingEnabled()) {
+                    traceparent = connection.getRequestProperty(TRACEPARENT_HEADER_NAME);
+                }
+            } catch (Exception e) {
+                // Ignore traceparent if there was a problem obtaining it
             }
-        } catch (Exception e) {
-            // Ignore traceparent if there was a problem obtaining it
         }
         this.connection.connect();
     }
 
     @Override
     public void disconnect() {
         // The network call must be logged before we close the transport
-        internalLogNetworkCall(this.createdTime);
+        internalLogNetworkCall(createdTime);
         this.connection.disconnect();
     }
 
@@ -287,8 +289,7 @@
         return getWrappedInputStream(this.connection.getErrorStream());
     }
 
-    @Override
-    public boolean shouldInterceptHeaderRetrieval(@Nullable String key) {
+    private boolean shouldInterceptHeaderRetrieval(@Nullable String key) {
         return shouldUncompressGzip() && key != null && (key.equalsIgnoreCase(CONTENT_ENCODING) || key.equalsIgnoreCase(CONTENT_LENGTH));
     }
 
@@ -358,7 +359,7 @@
     @Nullable
     public Map<String, List<String>> getHeaderFields() {
         final long startTime = embrace.getInternalInterface().getSdkCurrentTime();
-        cacheResponseData();
+        cacheNetworkCallData();
         internalLogNetworkCall(startTime);
         return headerFields.get();
     }
@@ -377,7 +378,7 @@
         }
 
         R result = action.invoke();
-        cacheResponseData();
+        cacheNetworkCallData();
         internalLogNetworkCall(startTime);
         return result;
     }
@@ -473,7 +474,7 @@
     public int getResponseCode() {
         identifyTraceId();
         long startTime = embrace.getInternalInterface().getSdkCurrentTime();
-        cacheResponseData();
+        cacheNetworkCallData();
         internalLogNetworkCall(startTime);
         return responseCode.get();
     }
@@ -484,7 +485,7 @@
         identifyTraceId();
         long startTime = embrace.getInternalInterface().getSdkCurrentTime();
         String responseMsg = this.connection.getResponseMessage();
-        cacheResponseData();
+        cacheNetworkCallData();
         internalLogNetworkCall(startTime);
         return responseMsg;
     }
@@ -547,7 +548,9 @@
      * ignored.
      */
     synchronized void internalLogNetworkCall(long startTime) {
-        internalLogNetworkCall(startTime, embrace.getInternalInterface().getSdkCurrentTime(), false, null);
+        if (isSDKStarted) {
+            internalLogNetworkCall(startTime, embrace.getInternalInterface().getSdkCurrentTime(), false, null);
+        }
     }
 
     /**
@@ -658,8 +661,7 @@
             hasNetworkCaptureRules(),
             (bytesCount, responseBody) -> {
                 if (this.startTime != null && this.endTime != null) {
-                    this.responseBody = responseBody;
-                    cacheResponseData();
+                    cacheNetworkCallData(responseBody);
                     internalLogNetworkCall(
                         this.startTime,
                         this.endTime,
@@ -695,7 +697,7 @@
     }
 
     private void identifyTraceId() {
-        if (traceId == null) {
+        if (isSDKStarted && traceId == null) {
             try {
                 traceId = getRequestProperty(embrace.getTraceIdHeader());
             } catch (Exception e) {
@@ -808,13 +810,13 @@
                 countingInputStream(new BufferedInputStream(connectionInputStream)) : connectionInputStream;
         }
 
-        cacheResponseData();
+        cacheNetworkCallData();
         internalLogNetworkCall(startTime);
         return in;
     }
 
     private boolean hasNetworkCaptureRules() {
-        if (this.connection.getURL() == null) {
+        if (!isSDKStarted || this.connection.getURL() == null) {
             return false;
         }
         String url = this.connection.getURL().toString();
@@ -823,11 +825,17 @@
         return embrace.getInternalInterface().shouldCaptureNetworkBody(url, method);
     }
 
+    private void cacheNetworkCallData() {
+        if (isSDKStarted) {
+            cacheNetworkCallData(null);
+        }
+    }
+
     /**
      * Cache values from response at the first point of availability so that we won't try to retrieve these values when the response
      * is not available.
      */
-    private void cacheResponseData() {
+    private void cacheNetworkCallData(@Nullable byte[] responseBody) {
         if (headerFields.get() == null) {
             synchronized (headerFields) {
                 if (headerFields.get() == null) {
@@ -874,26 +882,42 @@
             }
         }
 
-        if (shouldCaptureNetworkData() && networkCaptureData.get() == null) {
+        if (shouldCaptureNetworkData()) {
             // If we don't have network capture rules, it's unnecessary to save these values
             synchronized (networkCaptureData) {
-                if (shouldCaptureNetworkData() && networkCaptureData.get() == null) {
+                if (shouldCaptureNetworkData()) {
                     try {
-                        Map<String, String> requestHeaders = this.requestHeaders;
-                        String requestQueryParams = connection.getURL().getQuery();
-                        byte[] requestBody = this.outputStream != null ? this.outputStream.getRequestBody() : null;
-                        Map<String, String> responseHeaders = getProcessedHeaders(headerFields.get());
-
-                        networkCaptureData.set(
-                            new NetworkCaptureData(
-                                requestHeaders,
-                                requestQueryParams,
-                                requestBody,
-                                responseHeaders,
-                                responseBody,
-                                null
-                            )
-                        );
+                        NetworkCaptureData existingData = networkCaptureData.get();
+                        if (existingData == null) {
+                            Map<String, String> requestHeaders = this.requestHeaders;
+                            String requestQueryParams = connection.getURL().getQuery();
+                            byte[] requestBody = this.outputStream != null ? this.outputStream.getRequestBody() : null;
+                            Map<String, String> responseHeaders = getProcessedHeaders(headerFields.get());
+
+                            networkCaptureData.set(
+                                new NetworkCaptureData(
+                                    requestHeaders,
+                                    requestQueryParams,
+                                    requestBody,
+                                    responseHeaders,
+                                    responseBody,
+                                    null
+                                )
+                            );
+                        } else if (responseBody != null) {
+                            // Update the response body field in the cached networkCaptureData object if a subsequent call
+                            // is update to update the network logging with this data.
+                            networkCaptureData.set(
+                                new NetworkCaptureData(
+                                    existingData.getRequestHeaders(),
+                                    existingData.getRequestQueryParams(),
+                                    existingData.getCapturedRequestBody(),
+                                    existingData.getResponseHeaders(),
+                                    responseBody,
+                                    null
+                                )
+                            );
+                        }
                     } catch (Exception e) {
                         lastConnectionAccessException = e;
                     }
@@ -903,6 +927,7 @@
     }
 
     private boolean shouldCaptureNetworkData() {
-        return hasNetworkCaptureRules() && (enableWrapIoStreams || inputStreamAccessException != null);
+        return (hasNetworkCaptureRules() && (enableWrapIoStreams || inputStreamAccessException != null)) &&
+            (networkCaptureData.get() == null || networkCaptureData.get().getCapturedResponseBody() == null);
     }
 }

embrace-android-sdk/src/test/java/io/embrace/android/embracesdk/internal/network/http/EmbraceHttpPathOverrideTest.kt

@@ -0,0 +1,33 @@
+package io.embrace.android.embracesdk.internal.network.http
+
+import io.mockk.every
+import io.mockk.mockk
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+internal class EmbraceHttpPathOverrideTest {
+
+    @Test
+    fun `check override path validity`() {
+        val request: HttpPathOverrideRequest = mockk(relaxed = true)
+        every { request.urlString } answers { defaultUrl }
+        every { request.getOverriddenURL(customPath) } answers { customUrl }
+        every { request.getOverriddenURL("/error") } answers { throw RuntimeException() }
+
+        assertEquals(defaultUrl, EmbraceHttpPathOverride.getURLString(request, null))
+        assertEquals(defaultUrl, EmbraceHttpPathOverride.getURLString(request, ""))
+        assertEquals(defaultUrl, EmbraceHttpPathOverride.getURLString(request, "/a".repeat(1025)))
+        assertEquals(defaultUrl, EmbraceHttpPathOverride.getURLString(request, "/屈福特"))
+        assertEquals(defaultUrl, EmbraceHttpPathOverride.getURLString(request, "watford"))
+        assertEquals(defaultUrl, EmbraceHttpPathOverride.getURLString(request, "/custom#"))
+        assertEquals(defaultUrl, EmbraceHttpPathOverride.getURLString(request, ""))
+        assertEquals(defaultUrl, EmbraceHttpPathOverride.getURLString(request, "/error"))
+        assertEquals(customUrl, EmbraceHttpPathOverride.getURLString(request, customPath))
+    }
+
+    companion object {
+        private const val defaultUrl = "https://embrace.io/default-path"
+        private const val customPath = "/custom-path"
+        private const val customUrl = "https://embrace.io$customPath"
+    }
+}