Fix json parsing of nullable/empty fields (#2968)

getsentry · Oct 11, 2023 · 2989106 · 2989106
1 parent 7e47940
commit 2989106
Show file tree

Hide file tree

Showing 19 changed files with 114 additions and 45 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,14 @@
   - [changelog](https://github.com/getsentry/sentry-native/blob/master/CHANGELOG.md#066)
   - [diff](https://github.com/getsentry/sentry-native/compare/0.6.5...0.6.6)
 
+### Fixes
+
+- Fix json parsing of nullable/empty fields for Hybrid SDKs ([#2968](https://github.com/getsentry/sentry-java/pull/2968))
+  - (Internal) Rename `nextList` to `nextListOrNull` to actually match what the method does
+  - (Hybrid) Check if there's any object in a collection before trying to parse it (which prevents the "Failed to deserilize object in list" log message)
+  - (Hybrid) If a date can't be parsed as an ISO timestamp, attempts to parse it as millis silently, without printing a log message
+  - (Hybrid) If `op` is not defined as part of `SpanContext`, fallback to an empty string, because the filed is optional in the spec
+
 ## 6.30.0
 
 ### Features

diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api
@@ -770,7 +770,7 @@ public final class io/sentry/JsonObjectReader : io/sentry/vendor/gson/stream/Jso
 	public fun nextFloat ()Ljava/lang/Float;
 	public fun nextFloatOrNull ()Ljava/lang/Float;
 	public fun nextIntegerOrNull ()Ljava/lang/Integer;
-	public fun nextList (Lio/sentry/ILogger;Lio/sentry/JsonDeserializer;)Ljava/util/List;
+	public fun nextListOrNull (Lio/sentry/ILogger;Lio/sentry/JsonDeserializer;)Ljava/util/List;
 	public fun nextLongOrNull ()Ljava/lang/Long;
 	public fun nextMapOrNull (Lio/sentry/ILogger;Lio/sentry/JsonDeserializer;)Ljava/util/Map;
 	public fun nextObjectOrNull ()Ljava/lang/Object;

diff --git a/sentry/src/main/java/io/sentry/JsonObjectReader.java b/sentry/src/main/java/io/sentry/JsonObjectReader.java
@@ -81,21 +81,23 @@ public void nextUnknown(ILogger logger, Map<String, Object> unknown, String name
     }
   }
 
-  public <T> @Nullable List<T> nextList(
+  public <T> @Nullable List<T> nextListOrNull(
       @NotNull ILogger logger, @NotNull JsonDeserializer<T> deserializer) throws IOException {
     if (peek() == JsonToken.NULL) {
       nextNull();
       return null;
     }
     beginArray();
     List<T> list = new ArrayList<>();
-    do {
-      try {
-        list.add(deserializer.deserialize(this, logger));
-      } catch (Exception e) {
-        logger.log(SentryLevel.ERROR, "Failed to deserialize object in list.", e);
-      }
-    } while (peek() == JsonToken.BEGIN_OBJECT);
+    if (hasNext()) {
+      do {
+        try {
+          list.add(deserializer.deserialize(this, logger));
+        } catch (Exception e) {
+          logger.log(SentryLevel.WARNING, "Failed to deserialize object in list.", e);
+        }
+      } while (peek() == JsonToken.BEGIN_OBJECT);
+    }
     endArray();
     return list;
   }
@@ -108,14 +110,16 @@ public void nextUnknown(ILogger logger, Map<String, Object> unknown, String name
     }
     beginObject();
     Map<String, T> map = new HashMap<>();
-    do {
-      try {
-        String key = nextName();
-        map.put(key, deserializer.deserialize(this, logger));
-      } catch (Exception e) {
-        logger.log(SentryLevel.ERROR, "Failed to deserialize object in map.", e);
-      }
-    } while (peek() == JsonToken.BEGIN_OBJECT || peek() == JsonToken.NAME);
+    if (hasNext()) {
+      do {
+        try {
+          String key = nextName();
+          map.put(key, deserializer.deserialize(this, logger));
+        } catch (Exception e) {
+          logger.log(SentryLevel.WARNING, "Failed to deserialize object in map.", e);
+        }
+      } while (peek() == JsonToken.BEGIN_OBJECT || peek() == JsonToken.NAME);
+    }
 
     endObject();
     return map;
@@ -144,16 +148,12 @@ public void nextUnknown(ILogger logger, Map<String, Object> unknown, String name
     }
     try {
       return DateUtils.getDateTime(dateString);
-    } catch (Exception e) {
-      logger.log(
-          SentryLevel.DEBUG,
-          "Error when deserializing UTC timestamp format, it might be millis timestamp format.",
-          e);
-    }
-    try {
-      return DateUtils.getDateTimeWithMillisPrecision(dateString);
-    } catch (Exception e) {
-      logger.log(SentryLevel.ERROR, "Error when deserializing millis timestamp format.", e);
+    } catch (Exception ignored) {
+      try {
+        return DateUtils.getDateTimeWithMillisPrecision(dateString);
+      } catch (Exception e) {
+        logger.log(SentryLevel.ERROR, "Error when deserializing millis timestamp format.", e);
+      }
     }
     return null;
   }

diff --git a/sentry/src/main/java/io/sentry/JsonSerializer.java b/sentry/src/main/java/io/sentry/JsonSerializer.java
@@ -134,7 +134,7 @@ public JsonSerializer(@NotNull SentryOptions options) {
           return (T) jsonObjectReader.nextObjectOrNull();
         }
 
-        return (T) jsonObjectReader.nextList(options.getLogger(), elementDeserializer);
+        return (T) jsonObjectReader.nextListOrNull(options.getLogger(), elementDeserializer);
       } else {
         return (T) jsonObjectReader.nextObjectOrNull();
       }

diff --git a/sentry/src/main/java/io/sentry/ProfilingTraceData.java b/sentry/src/main/java/io/sentry/ProfilingTraceData.java
@@ -556,7 +556,7 @@ public static final class Deserializer implements JsonDeserializer<ProfilingTrac
             break;
           case JsonKeys.TRANSACTION_LIST:
             List<ProfilingTransactionData> transactions =
-                reader.nextList(logger, new ProfilingTransactionData.Deserializer());
+                reader.nextListOrNull(logger, new ProfilingTransactionData.Deserializer());
             if (transactions != null) {
               data.transactions.addAll(transactions);
             }

diff --git a/sentry/src/main/java/io/sentry/SentryBaseEvent.java b/sentry/src/main/java/io/sentry/SentryBaseEvent.java
@@ -435,7 +435,7 @@ public boolean deserializeValue(
           baseEvent.dist = reader.nextStringOrNull();
           return true;
         case JsonKeys.BREADCRUMBS:
-          baseEvent.breadcrumbs = reader.nextList(logger, new Breadcrumb.Deserializer());
+          baseEvent.breadcrumbs = reader.nextListOrNull(logger, new Breadcrumb.Deserializer());
           return true;
         case JsonKeys.DEBUG_META:
           baseEvent.debugMeta = reader.nextOrNull(logger, new DebugMeta.Deserializer());

diff --git a/sentry/src/main/java/io/sentry/SentryEvent.java b/sentry/src/main/java/io/sentry/SentryEvent.java
@@ -338,14 +338,15 @@ public static final class Deserializer implements JsonDeserializer<SentryEvent>
             reader.beginObject();
             reader.nextName(); // SentryValues.JsonKeys.VALUES
             event.threads =
-                new SentryValues<>(reader.nextList(logger, new SentryThread.Deserializer()));
+                new SentryValues<>(reader.nextListOrNull(logger, new SentryThread.Deserializer()));
             reader.endObject();
             break;
           case JsonKeys.EXCEPTION:
             reader.beginObject();
             reader.nextName(); // SentryValues.JsonKeys.VALUES
             event.exception =
-                new SentryValues<>(reader.nextList(logger, new SentryException.Deserializer()));
+                new SentryValues<>(
+                    reader.nextListOrNull(logger, new SentryException.Deserializer()));
             reader.endObject();
             break;
           case JsonKeys.LEVEL:

diff --git a/sentry/src/main/java/io/sentry/SpanContext.java b/sentry/src/main/java/io/sentry/SpanContext.java
@@ -358,10 +358,13 @@ public static final class Deserializer implements JsonDeserializer<SpanContext>
       }
 
       if (op == null) {
-        String message = "Missing required field \"" + JsonKeys.OP + "\"";
-        Exception exception = new IllegalStateException(message);
-        logger.log(SentryLevel.ERROR, message, exception);
-        throw exception;
+        /*
+         This is the case for hybrid SDKs. In fact, 'op' field is not required as part of the
+         trace context, but we utilise this class heavily also for transactions and spans, so it
+         would be a lot of changes to make it optional and we just duct-tape it here.
+         See doc https://develop.sentry.dev/sdk/event-payloads/contexts/#trace-context
+        */
+        op = "";
       }
 
       SpanContext spanContext = new SpanContext(traceId, spanId, op, parentSpanId, null);

diff --git a/sentry/src/main/java/io/sentry/clientreport/ClientReport.java b/sentry/src/main/java/io/sentry/clientreport/ClientReport.java
@@ -89,7 +89,7 @@ public static final class Deserializer implements JsonDeserializer<ClientReport>
             break;
           case JsonKeys.DISCARDED_EVENTS:
             List<DiscardedEvent> deserializedDiscardedEvents =
-                reader.nextList(logger, new DiscardedEvent.Deserializer());
+                reader.nextListOrNull(logger, new DiscardedEvent.Deserializer());
             discardedEvents.addAll(deserializedDiscardedEvents);
             break;
           default:

diff --git a/sentry/src/main/java/io/sentry/profilemeasurements/ProfileMeasurement.java b/sentry/src/main/java/io/sentry/profilemeasurements/ProfileMeasurement.java
@@ -134,7 +134,7 @@ public static final class Deserializer implements JsonDeserializer<ProfileMeasur
             break;
           case JsonKeys.VALUES:
             List<ProfileMeasurementValue> values =
-                reader.nextList(logger, new ProfileMeasurementValue.Deserializer());
+                reader.nextListOrNull(logger, new ProfileMeasurementValue.Deserializer());
             if (values != null) {
               data.values = values;
             }

diff --git a/sentry/src/main/java/io/sentry/protocol/DebugMeta.java b/sentry/src/main/java/io/sentry/protocol/DebugMeta.java
@@ -109,7 +109,7 @@ public static final class Deserializer implements JsonDeserializer<DebugMeta> {
             debugMeta.sdkInfo = reader.nextOrNull(logger, new SdkInfo.Deserializer());
             break;
           case JsonKeys.IMAGES:
-            debugMeta.images = reader.nextList(logger, new DebugImage.Deserializer());
+            debugMeta.images = reader.nextListOrNull(logger, new DebugImage.Deserializer());
             break;
           default:
             if (unknown == null) {

diff --git a/sentry/src/main/java/io/sentry/protocol/SdkVersion.java b/sentry/src/main/java/io/sentry/protocol/SdkVersion.java
@@ -245,7 +245,7 @@ public static final class Deserializer implements JsonDeserializer<SdkVersion> {
             break;
           case JsonKeys.PACKAGES:
             List<SentryPackage> deserializedPackages =
-                reader.nextList(logger, new SentryPackage.Deserializer());
+                reader.nextListOrNull(logger, new SentryPackage.Deserializer());
             if (deserializedPackages != null) {
               packages.addAll(deserializedPackages);
             }

diff --git a/sentry/src/main/java/io/sentry/protocol/SentryStackTrace.java b/sentry/src/main/java/io/sentry/protocol/SentryStackTrace.java
@@ -162,7 +162,8 @@ public static final class Deserializer implements JsonDeserializer<SentryStackTr
         final String nextName = reader.nextName();
         switch (nextName) {
           case JsonKeys.FRAMES:
-            sentryStackTrace.frames = reader.nextList(logger, new SentryStackFrame.Deserializer());
+            sentryStackTrace.frames =
+                reader.nextListOrNull(logger, new SentryStackFrame.Deserializer());
             break;
           case JsonKeys.REGISTERS:
             sentryStackTrace.registers =

diff --git a/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java b/sentry/src/main/java/io/sentry/protocol/SentryTransaction.java
@@ -281,7 +281,7 @@ public static final class Deserializer implements JsonDeserializer<SentryTransac
             break;
           case JsonKeys.SPANS:
             List<SentrySpan> deserializedSpans =
-                reader.nextList(logger, new SentrySpan.Deserializer());
+                reader.nextListOrNull(logger, new SentrySpan.Deserializer());
             if (deserializedSpans != null) {
               transaction.spans.addAll(deserializedSpans);
             }

diff --git a/sentry/src/main/java/io/sentry/protocol/ViewHierarchy.java b/sentry/src/main/java/io/sentry/protocol/ViewHierarchy.java
@@ -88,7 +88,7 @@ public static final class Deserializer implements JsonDeserializer<ViewHierarchy
             renderingSystem = reader.nextStringOrNull();
             break;
           case JsonKeys.WINDOWS:
-            windows = reader.nextList(logger, new ViewHierarchyNode.Deserializer());
+            windows = reader.nextListOrNull(logger, new ViewHierarchyNode.Deserializer());
             break;
           default:
             if (unknown == null) {

diff --git a/sentry/src/main/java/io/sentry/protocol/ViewHierarchyNode.java b/sentry/src/main/java/io/sentry/protocol/ViewHierarchyNode.java
@@ -244,7 +244,7 @@ public static final class Deserializer implements JsonDeserializer<ViewHierarchy
             node.alpha = reader.nextDoubleOrNull();
             break;
           case JsonKeys.CHILDREN:
-            node.children = reader.nextList(logger, this);
+            node.children = reader.nextListOrNull(logger, this);
             break;
           default:
             if (unknown == null) {

diff --git a/sentry/src/test/java/io/sentry/JsonObjectReaderTest.kt b/sentry/src/test/java/io/sentry/JsonObjectReaderTest.kt
@@ -1,7 +1,10 @@
 package io.sentry
 
 import org.junit.Test
+import org.mockito.kotlin.any
 import org.mockito.kotlin.mock
+import org.mockito.kotlin.never
+import org.mockito.kotlin.verify
 import java.io.StringReader
 import kotlin.test.assertEquals
 import kotlin.test.assertNull
@@ -142,10 +145,24 @@ class JsonObjectReaderTest {
             Deserializable("foo", "bar"),
             Deserializable("fooo", "baar")
         )
-        val actual = reader.nextList(logger, Deserializable.Deserializer())
+        val actual = reader.nextListOrNull(logger, Deserializable.Deserializer())
         assertEquals(expected, actual)
     }
 
+    @Test
+    fun `returns empty list for empty list`() {
+        val jsonString = "{\"deserializable\": []}"
+        val reader = fixture.getSut(jsonString)
+        val logger = mock<ILogger>()
+        reader.beginObject()
+        reader.nextName()
+
+        val expected = emptyList<Deserializable>()
+        val actual = reader.nextListOrNull(logger, Deserializable.Deserializer())
+        assertEquals(expected, actual)
+        verify(fixture.logger, never()).log(any(), any(), any<Throwable>())
+    }
+
     // nextMap
 
     @Test
@@ -175,6 +192,19 @@ class JsonObjectReaderTest {
         assertEquals(expected, actual)
     }
 
+    @Test
+    fun `returns empty map`() {
+        val jsonString = "{\"deserializable\": {}}"
+        val reader = fixture.getSut(jsonString)
+        reader.beginObject()
+        reader.nextName()
+
+        val expected = mapOf<String, Deserializable>()
+        val actual = reader.nextMapOrNull(fixture.logger, Deserializable.Deserializer())
+        assertEquals(expected, actual)
+        verify(fixture.logger, never()).log(any(), any(), any<Throwable>())
+    }
+
     // nextDateOrNull
 
     @Test
@@ -211,6 +241,7 @@ class JsonObjectReaderTest {
         val expected = DateUtils.getDateTimeWithMillisPrecision(dateTimestampFormat)
         val actual = reader.nextDateOrNull(fixture.logger)
         assertEquals(expected, actual)
+        verify(fixture.logger, never()).log(any(), any(), any<Throwable>())
     }
 
     @Test
@@ -224,6 +255,7 @@ class JsonObjectReaderTest {
         val expected = DateUtils.getDateTimeWithMillisPrecision(dateTimestampWithMillis)
         val actual = reader.nextDateOrNull(fixture.logger)
         assertEquals(expected, actual)
+        verify(fixture.logger, never()).log(any(), any(), any<Throwable>())
     }
 
     // nextTimeZoneOrNull

diff --git a/sentry/src/test/java/io/sentry/protocol/SpanContextSerializationTest.kt b/sentry/src/test/java/io/sentry/protocol/SpanContextSerializationTest.kt
@@ -57,6 +57,16 @@ class SpanContextSerializationTest {
         assertEquals(expectedJson, actualJson)
     }
 
+    @Test
+    fun deserializeNullOp() {
+        val expectedJson = sanitizedFile("json/span_context_null_op.json")
+        val actual = deserialize(expectedJson)
+        assertNull(actual.sampled)
+        assertNull(actual.profileSampled)
+        assertNotNull(actual.tags)
+        assertEquals("", actual.operation)
+    }
+
     // Helper
 
     private fun sanitizedFile(path: String): String {

diff --git a/sentry/src/test/resources/json/span_context_null_op.json b/sentry/src/test/resources/json/span_context_null_op.json
@@ -0,0 +1,14 @@
+{
+  "trace_id": "afcb46b1140ade5187c4bbb5daa804df",
+  "span_id": "bf6b582d-8ce3-412b-a334-f4c5539b9602",
+  "parent_span_id": "c7500f2a-d4e6-4f5f-a0f4-6bb67e98d5a2",
+  "description": "c204b6c7-9753-4d45-927d-b19789bfc9a5",
+  "status": "resource_exhausted",
+  "origin": "auto.test.unit.spancontext",
+  "tags":
+  {
+    "2a5fa3f5-7b87-487f-aaa5-84567aa73642": "4781d51a-c5af-47f2-a4ed-f030c9b3e194",
+    "29106d7d-7fa4-444f-9d34-b9d7510c69ab": "218c23ea-694a-497e-bf6d-e5f26f1ad7bd",
+    "ba9ce913-269f-4c03-882d-8ca5e6991b14": "35a74e90-8db8-4610-a411-872cbc1030ac"
+  }
+}