fix: prevent setting duplicate externalId types (#446)

* chore: fix indentation of MessageUtilsTest

* fix: merging of externalIds

Currently, even if the externalId.type is same they are not merged. Ideally they should be merged in such cases.

* test: add test cases related to the merging of the externalId option

* test: refactor test method names

android/src/main/java/com/rudderstack/android/internal/plugins/ExtractStatePlugin.kt

@@ -103,8 +103,8 @@ internal class ExtractStatePlugin : Plugin {
     }
 
     private fun appendContextForIdentify(messageContext: MessageContext) : MessageContext{
-       return _analytics?.contextState?.value?.let {
-           messageContext optAddContext it
+       return _analytics?.contextState?.value?.let { savedContext ->
+           messageContext optAddContext savedContext
        }?: messageContext
     }
 

core/src/main/java/com/rudderstack/core/RudderOption.kt

@@ -14,6 +14,9 @@
 
 package com.rudderstack.core
 
+private const val TYPE = "type"
+private const val ID = "id"
+
 /**
  * Can be set as global or customised options for each message.
  * If no customised option is set for a message, global options will be used.
@@ -25,25 +28,40 @@ package com.rudderstack.core
  * message will be delivered to all destinations added to Analytics.
  * @property customContexts Custom context elements that are going to be sent with message
  */
-class RudderOption{
+class RudderOption {
     val integrations: Map<String, Boolean>
         get() = _integrations
-    private val _integrations = mutableMapOf<String,Boolean>()
+    private val _integrations = mutableMapOf<String, Boolean>()
     val customContexts: Map<String, Any>
         get() = _customContexts
     private val _customContexts = mutableMapOf<String, Any>()
 
     val externalIds: List<Map<String, String>>
         get() = _externalIds
-    private val _externalIds= mutableListOf<Map<String, String>>()
+    private val _externalIds = mutableListOf<Map<String, String>>()
 
     fun putExternalId(type: String, id: String): RudderOption {
-        _externalIds.add(
-            mapOf("type" to type,
-                "id" to id)
-        )
+        val existingExternalIdIndex =
+            _externalIds.indexOfFirst { it[TYPE]?.equals(type, ignoreCase = true) == true }
+
+        if (existingExternalIdIndex != -1) {
+            // If the type exists, update the id
+            _externalIds[existingExternalIdIndex] =
+                _externalIds[existingExternalIdIndex].toMutableMap().apply {
+                    this[ID] = id
+                }
+        } else {
+            // If the type does not exist, add a new externalId
+            _externalIds.add(
+                mapOf(
+                    TYPE to type,
+                    ID to id
+                )
+            )
+        }
         return this
     }
+
     fun putIntegration(destinationKey: String, enabled: Boolean): RudderOption {
         _integrations[destinationKey] = enabled
         return this
@@ -71,4 +89,4 @@ class RudderOption{
         return externalIds.hashCode() + integrations.hashCode() + customContexts.hashCode()
     }
 
-}
+}

core/src/test/java/com/rudderstack/core/RudderOptionTest.kt

@@ -0,0 +1,51 @@
+package com.rudderstack.core
+
+import junit.framework.TestCase.assertEquals
+import org.junit.Test
+
+class RudderOptionTest {
+    @Test
+    fun `given all the externalIds are of different type, when externalIds are passed, they should be added to the list`() {
+        val rudderOption = RudderOption().apply {
+            putExternalId("brazeExternalID", "12345")
+            putExternalId("amplitudeExternalID", "67890")
+        }
+
+        val expectedExternalIds = listOf(
+            mapOf("type" to "brazeExternalID", "id" to "12345"),
+            mapOf("type" to "amplitudeExternalID", "id" to "67890"),
+        )
+
+        assertEquals(expectedExternalIds, rudderOption.externalIds)
+    }
+
+    @Test
+    fun `given few externalIds have same type but different Ids, when externalIds are passed, they should be merged in the list`() {
+        val rudderOption = RudderOption().apply {
+            putExternalId("brazeExternalID", "12345")
+            putExternalId("brazeExternalID", "67890")
+            putExternalId("amplitudeExternalID", "67890")
+        }
+
+        val expectedExternalIds = listOf(
+            mapOf("type" to "brazeExternalID", "id" to "67890"),
+            mapOf("type" to "amplitudeExternalID", "id" to "67890"),
+        )
+
+        assertEquals(expectedExternalIds, rudderOption.externalIds)
+    }
+
+    @Test
+    fun `given few externalIds have same type and Ids, when externalIds are passed, they should be merged in the list`() {
+        val rudderOption = RudderOption().apply {
+            putExternalId("brazeExternalID", "12345")
+            putExternalId("brazeExternalID", "12345")
+        }
+
+        val expectedExternalIds = listOf(
+            mapOf("type" to "brazeExternalID", "id" to "12345"),
+        )
+
+        assertEquals(expectedExternalIds, rudderOption.externalIds)
+    }
+}

models/src/main/java/com/rudderstack/models/Extensions.kt

@@ -52,8 +52,14 @@ infix fun MessageContext?.optAddContext (context: MessageContext?): MessageConte
     val newCustomContexts = context.customContexts?.let {
         (it - (this.customContexts?.keys ?: setOf()).toSet()) optAdd this.customContexts
     } ?: customContexts
-    val newExternalIds = context.externalIds?.let {
-        it + (this.externalIds?: emptyList())
+    val newExternalIds = context.externalIds?.let { savedExternalIds ->
+        val currentExternalIds = this.externalIds ?: emptyList()
+        val filteredSavedExternalIds = savedExternalIds.filter { savedExternalId ->
+            currentExternalIds.none { currentExternalId ->
+                currentExternalId["type"] == savedExternalId["type"]
+            }
+        }
+        currentExternalIds + filteredSavedExternalIds
     } ?: externalIds
 
     createContext(newTraits, newExternalIds, newCustomContexts).let {
@@ -94,4 +100,4 @@ infix fun <K, V> Iterable<Map<K, V>>.inWrtKeys(item: Map<K, V>): Boolean {
             return true
     }
     return false
-}
+}

models/src/test/java/com/rudderstack/models/MessageUtilsTest.kt

@@ -1,37 +1,36 @@
 package com.rudderstack.models
 
-import org.hamcrest.MatcherAssert
 import org.hamcrest.MatcherAssert.assertThat
-import org.hamcrest.Matchers
 import org.hamcrest.Matchers.allOf
 import org.hamcrest.Matchers.hasEntry
 import org.hamcrest.Matchers.hasItems
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNull
 import org.junit.Test
 
-class MessageUtilsTest{
+class MessageUtilsTest {
     @Test
-    fun `optAdd with both contexts null`() {
+    fun `given both the contexts are null, when optAddContext is called, then it should return null`() {
         val context1: MessageContext? = null
         val context2: MessageContext? = null
 
-        val result = context1 optAddContext  context2
+        val result = context1 optAddContext context2
 
         assertNull(result)
     }
 
     @Test
-    fun `optAdd with first context null`() {
+    fun `given first context is null, when optAddContext is called, then it should return second context`() {
         val context1: MessageContext? = null
         val context2: MessageContext = mapOf("key" to "value")
 
-        val result = context1 optAddContext  context2
+        val result = context1 optAddContext context2
 
         assertEquals(context2, result)
     }
+
     @Test
-    fun `optAdd with second context null`() {
+    fun `given second context is null, when optAddContext is called, then it should return first context`() {
         val context1: MessageContext = mapOf("key" to "value")
         val context2: MessageContext? = null
 
@@ -41,64 +40,87 @@ class MessageUtilsTest{
     }
 
     @Test
-    fun `optAdd with overlapping traits`() {
+    fun `given there are overlapping keys in both contexts, when optAddContext is called, then it should return the merged context while prioritizing the key-value pair from the first context`() {
         val context1: MessageContext = createContext(
             mapOf("trait1" to "value1", "common" to "value1")
         )
         val context2: MessageContext = createContext(
             mapOf("trait2" to "value2", "common" to "value2")
         )
-        val result = context1 optAddContext  context2
-        println(result)
-        assertThat(result?.traits, allOf(
-            hasEntry("trait1",  "value1"),
-            hasEntry("trait2",  "value2"),
-            hasEntry("common",  "value1"),
-        ))
+
+        val result = context1 optAddContext context2
+
+        assertThat(
+            result?.traits, allOf(
+                hasEntry("trait1", "value1"),
+                hasEntry("trait2", "value2"),
+                hasEntry("common", "value1"),
+            )
+        )
     }
 
     @Test
-    fun `optAdd with non-overlapping customContexts`() {
+    fun `given there are no overlapping keys in both contexts, when optAddContext is called, then it should return the merged context`() {
         val context1: MessageContext = mapOf(
             Constants.CUSTOM_CONTEXT_MAP_ID to mapOf("context1" to "value1")
         )
         val context2: MessageContext = mapOf(
             Constants.CUSTOM_CONTEXT_MAP_ID to mapOf("context2" to "value2")
         )
 
-        val result = context1 optAddContext  context2
+        val result = context1 optAddContext context2
 
-        assertThat(result?.customContexts, allOf(
-            hasEntry("context1", "value1"),
-            hasEntry("context2", "value2"),
-        ))
+        assertThat(
+            result?.customContexts, allOf(
+                hasEntry("context1", "value1"),
+                hasEntry("context2", "value2"),
+            )
+        )
     }
 
     @Test
-    fun `optAdd with overlapping externalIds`() {
-        val context1: MessageContext = createContext(
-            externalIds =  listOf(mapOf("id1" to "value1"))
+    fun `given there are overlapping externalIds in both contexts, when optAddContext is called, then it should return the merged context while prioritizing the key-value pair from the first context`() {
+        val currentEventContext: MessageContext = createContext(
+            externalIds = listOf(
+                mapOf("type" to "brazeExternalID", "id" to "braze-67890-override"),
+                mapOf("type" to "amplitudeExternalID", "id" to "amp-5678-override"),
+                mapOf("type" to "firebaseExternalID", "id" to "fire-67890"),
+            )
         )
-        val context2: MessageContext = createContext(
-            externalIds =  listOf(mapOf("id2" to "value2"), mapOf("id1" to "value3"))
+        val savedContext: MessageContext = createContext(
+            externalIds = listOf(
+                mapOf("type" to "brazeExternalID", "id" to "braze-1234"),
+                mapOf("type" to "amplitudeExternalID", "id" to "amp-5678"),
+                mapOf("type" to "adobeExternalID", "id" to "fire-67890"),
+            )
         )
 
-        val result = context1 optAddContext  context2
+        val result = currentEventContext optAddContext savedContext
 
-        assertThat(result?.externalIds, hasItems(mapOf("id1" to "value1"), mapOf("id2" to "value2"), mapOf("id2" to "value2")))
+        assertThat(
+            result?.externalIds,
+            hasItems(
+                mapOf("type" to "brazeExternalID", "id" to "braze-67890-override"),
+                mapOf("type" to "amplitudeExternalID", "id" to "amp-5678-override"),
+                mapOf("type" to "adobeExternalID", "id" to "fire-67890"),
+                mapOf("type" to "firebaseExternalID", "id" to "fire-67890"),
+            )
+        )
     }
 
     @Test
-    fun `optAdd with extra keys`() {
+    fun `given there are overlapping extra keys in both contexts, when optAddContext is called, then it should return the merged context while prioritizing the key-value pair from the first context`() {
         val context1: MessageContext = mapOf("extra1" to "value1", "common" to "value1")
         val context2: MessageContext = mapOf("extra2" to "value2", "common" to "value2")
 
-        val result = context1 optAddContext  context2
+        val result = context1 optAddContext context2
 
-        assertThat(result, allOf(
-            hasEntry("extra1", "value1"),
-            hasEntry("extra2", "value2"),
-            hasEntry("common", "value1"),
-        ))
+        assertThat(
+            result, allOf(
+                hasEntry("extra1", "value1"),
+                hasEntry("extra2", "value2"),
+                hasEntry("common", "value1"),
+            )
+        )
     }
-}
+}