Prevent incorrect merge of nested maps in metadata

CHANGELOG.md

@@ -7,6 +7,9 @@
 * Prevent ConcurrentModificationException thrown from Metadata class
   [#935](https://github.com/bugsnag/bugsnag-android/pull/935)
 
+* Prevent incorrect merge of nested maps in metadata
+  [#936](https://github.com/bugsnag/bugsnag-android/pull/936)
+
 ## 5.1.0 (2020-09-08)
 
 ### Enhancements

bugsnag-android-core/src/main/java/com/bugsnag/android/Metadata.kt

@@ -42,11 +42,14 @@ internal data class Metadata @JvmOverloads constructor(
         }
     }
 
-    private fun insertValue(map: MutableMap<String, Any>, key: String, value: Any) {
-        var obj = value
-
-        if (obj is MutableMap<*, *> && map.isNotEmpty()) {
-            obj = mergeMaps(listOf(map as Map<String, Any>, value as Map<String, Any>))
+    private fun insertValue(map: MutableMap<String, Any>, key: String, newValue: Any) {
+        var obj = newValue
+
+        // only merge if both the existing and new value are maps
+        val existingValue = map[key]
+        if (obj is MutableMap<*, *> && existingValue is MutableMap<*, *>) {
+            val maps = listOf(existingValue as Map<String, Any>, newValue as Map<String, Any>)
+            obj = mergeMaps(maps)
         }
         map[key] = obj
     }

bugsnag-android-core/src/test/java/com/bugsnag/android/MetadataMergeTest.kt

@@ -0,0 +1,98 @@
+package com.bugsnag.android
+
+import com.bugsnag.android.Metadata.Companion.merge
+import org.junit.Assert.assertEquals
+import org.junit.Test
+import java.util.HashMap
+
+class MetadataMergeTest {
+
+    @Test
+    fun testBasicMerge() {
+        val base = Metadata()
+        base.addMetadata("example", "name", "bob")
+        base.addMetadata("example", "awesome", false)
+
+        val overrides = Metadata()
+        overrides.addMetadata("example", "age", 30)
+        overrides.addMetadata("example", "awesome", true)
+
+        val merged = merge(base, overrides)
+        val tab = merged.getMetadata("example")
+        assertEquals("bob", tab!!["name"])
+        assertEquals(30, tab["age"])
+        assertEquals(true, tab["awesome"])
+    }
+
+    @Test
+    fun testDeepMerge() {
+        val baseMap = mapOf(Pair("key", "fromBase"))
+        val base = Metadata()
+        base.addMetadata("example", "map", baseMap)
+
+        val overridesMap = mapOf(Pair("key", "fromOverrides"))
+        val overrides = Metadata()
+        overrides.addMetadata("example", "map", overridesMap)
+
+        val merged = merge(base, overrides)
+        val tab = merged.getMetadata("example")
+        val mergedMap = tab!!["map"] as Map<*, *>?
+        assertEquals("fromOverrides", mergedMap!!["key"])
+    }
+
+    @Test
+    fun testNestedMapMerge() {
+        val metadata = Metadata()
+        val alphaMap = mapOf("alphaKey" to "alphaValue")
+        val betaMap = mapOf("betaKey" to "betaValue")
+        val gammaMap = mapOf("gammaKey" to "gammaValue")
+
+        metadata.addMetadata("mytab",
+            mapOf("alpha" to alphaMap)
+        )
+        metadata.addMetadata("mytab",
+            mapOf("beta" to betaMap)
+        )
+        metadata.addMetadata("mytab",
+            mapOf("gamma" to gammaMap)
+        )
+        assertEquals(alphaMap, metadata.getMetadata("mytab", "alpha"))
+        assertEquals(betaMap, metadata.getMetadata("mytab", "beta"))
+        assertEquals(gammaMap, metadata.getMetadata("mytab", "gamma"))
+    }
+
+    @Test
+    fun testNestedMapMergeOverride() {
+        val metadata = Metadata()
+        val baseMap = mapOf(
+            "alpha" to 1,
+            "beta" to 2
+        )
+        metadata.addMetadata("mytab", mapOf("custom" to baseMap))
+        assertEquals(baseMap, metadata.getMetadata("mytab", "custom"))
+
+        val overrideMap = mapOf("alpha" to 5)
+        metadata.addMetadata("mytab", mapOf("custom" to overrideMap))
+        val expected = mapOf(
+            "alpha" to 5,
+            "beta" to 2
+        )
+        assertEquals(expected, metadata.getMetadata("mytab", "custom"))
+    }
+
+    @Test
+    fun testConcurrentMapMerge() {
+        merge(generateMetadata(), generateMetadata())
+    }
+
+    /**
+     * Generates a metadata object with a tab value containing a map with a null entry
+     */
+    private fun generateMetadata(): Metadata {
+        val metadata = Metadata()
+        val nestedMap: MutableMap<Any, Any?> = HashMap()
+        metadata.addMetadata("foo", "bar", nestedMap)
+        nestedMap["whoops"] = null
+        return metadata
+    }
+}

bugsnag-android-core/src/test/resources/meta_data_serialization_1.json

@@ -13,18 +13,6 @@
       "World"
     ],
     "map": {
-      "boolean": true,
-      "string": "value",
-      "array": [
-        "a",
-        "b"
-      ],
-      "double": 123.45,
-      "integer": 123,
-      "collection": [
-        "Hello",
-        "World"
-      ],
       "key": "value"
     }
   }