Disallow repeated keys in CBOR (WIP, need to add config switches)

Fixes Kotlin#2662 by adding
a `visitKey` method to `CompositeDecoder`; map and set serializers should
call this so that decoders have an opportunity to throw an error when a
duplicate key is detected.

Also fixes a typo in an unrelated method docstring.

Author: timmc

core/commonMain/src/kotlinx/serialization/SerializationExceptions.kt

@@ -133,3 +133,10 @@ internal constructor(message: String?) : SerializationException(message) {
     // This constructor is used by the generated serializers
     constructor(index: Int) : this("An unknown field for index $index")
 }
+
+/**
+ * Thrown when a map deserializer encounters a repeated map key (and configuration disallows this.)
+ */
+@ExperimentalSerializationApi
+public class DuplicateMapKeyException(public val key: Any?) :
+    SerializationException("Duplicate keys not allowed in maps. Key appeared twice: $key")

core/commonMain/src/kotlinx/serialization/encoding/Decoding.kt

@@ -275,7 +275,7 @@ internal inline fun <T : Any> Decoder.decodeIfNullable(deserializer: Deserializa
  * [CompositeDecoder] is a part of decoding process that is bound to a particular structured part of
  * the serialized form, described by the serial descriptor passed to [Decoder.beginStructure].
  *
- * Typically, for unordered data, [CompositeDecoder] is used by a serializer withing a [decodeElementIndex]-based
+ * Typically, for unordered data, [CompositeDecoder] is used by a serializer within a [decodeElementIndex]-based
  * loop that decodes all the required data one-by-one in any order and then terminates by calling [endStructure].
  * Please refer to [decodeElementIndex] for example of such loop.
  *
@@ -558,6 +558,17 @@ public interface CompositeDecoder {
         deserializer: DeserializationStrategy<T?>,
         previousValue: T? = null
     ): T?
+
+    /**
+     * Called after a key has been read.
+     *
+     * This could be a map or set key, or anything otherwise intended to be
+     * distinct within the collection under normal circumstances.
+     *
+     * Implementations might use this as a hook for throwing an exception when
+     * duplicate keys are encountered.
+     */
+    public fun visitKey(key: Any?) { }
 }
 
 /**

core/commonMain/src/kotlinx/serialization/internal/CollectionSerializers.kt

@@ -98,6 +98,7 @@ public sealed class MapLikeSerializer<Key, Value, Collection, Builder : MutableM
 
     final override fun readElement(decoder: CompositeDecoder, index: Int, builder: Builder, checkIndex: Boolean) {
         val key: Key = decoder.decodeSerializableElement(descriptor, index, keySerializer)
+        decoder.visitKey(key)
         val vIndex = if (checkIndex) {
             decoder.decodeElementIndex(descriptor).also {
                 require(it == index + 1) { "Value must follow key in a map, index for key: $index, returned index for value: $it" }

formats/cbor/commonMain/src/kotlinx/serialization/cbor/Cbor.kt

@@ -32,13 +32,14 @@ import kotlinx.serialization.modules.*
 public sealed class Cbor(
     internal val encodeDefaults: Boolean,
     internal val ignoreUnknownKeys: Boolean,
+    internal val allowDuplicateKeys: Boolean,
     override val serializersModule: SerializersModule
 ) : BinaryFormat {
 
     /**
      * The default instance of [Cbor]
      */
-    public companion object Default : Cbor(false, false, EmptySerializersModule())
+    public companion object Default : Cbor(false, false, true, EmptySerializersModule())
 
     override fun <T> encodeToByteArray(serializer: SerializationStrategy<T>, value: T): ByteArray {
         val output = ByteArrayOutput()
@@ -55,8 +56,11 @@ public sealed class Cbor(
 }
 
 @OptIn(ExperimentalSerializationApi::class)
-private class CborImpl(encodeDefaults: Boolean, ignoreUnknownKeys: Boolean, serializersModule: SerializersModule) :
-    Cbor(encodeDefaults, ignoreUnknownKeys, serializersModule)
+private class CborImpl(
+    encodeDefaults: Boolean, ignoreUnknownKeys: Boolean, allowDuplicateKeys: Boolean,
+    serializersModule: SerializersModule,
+) :
+    Cbor(encodeDefaults, ignoreUnknownKeys, allowDuplicateKeys, serializersModule)
 
 /**
  * Creates an instance of [Cbor] configured from the optionally given [Cbor instance][from]
@@ -66,7 +70,7 @@ private class CborImpl(encodeDefaults: Boolean, ignoreUnknownKeys: Boolean, seri
 public fun Cbor(from: Cbor = Cbor, builderAction: CborBuilder.() -> Unit): Cbor {
     val builder = CborBuilder(from)
     builder.builderAction()
-    return CborImpl(builder.encodeDefaults, builder.ignoreUnknownKeys, builder.serializersModule)
+    return CborImpl(builder.encodeDefaults, builder.ignoreUnknownKeys, builder.allowDuplicateKeys, builder.serializersModule)
 }
 
 /**
@@ -87,6 +91,14 @@ public class CborBuilder internal constructor(cbor: Cbor) {
      */
     public var ignoreUnknownKeys: Boolean = cbor.ignoreUnknownKeys
 
+    /**
+     * Specifies whether it is an error to read a map with duplicate keys.
+     *
+     * If this is set to false, decoding a map with two keys that compare as equal
+     * will cause a [DuplicateMapKeyException] error to be thrown.
+     */
+    public var allowDuplicateKeys: Boolean = cbor.allowDuplicateKeys
+
     /**
      * Module with contextual and polymorphic serializers to be used in the resulting [Cbor] instance.
      */

formats/cbor/commonMain/src/kotlinx/serialization/cbor/internal/Encoding.kt

@@ -197,8 +197,21 @@ internal class CborEncoder(private val output: ByteArrayOutput) {
     }
 }
 
-private class CborMapReader(cbor: Cbor, decoder: CborDecoder) : CborListReader(cbor, decoder) {
+private class CborMapReader(val cbor: Cbor, decoder: CborDecoder) : CborListReader(cbor, decoder) {
+    /** Keys that have been seen so far while reading this map. */
+    private val seenKeys = mutableSetOf<Any?>()
+
     override fun skipBeginToken() = setSize(decoder.startMap() * 2)
+
+    override fun visitKey(key: Any?) {
+        if (cbor.allowDuplicateKeys)
+            return
+
+        val added = seenKeys.add(key)
+        if (!added) {
+            throw DuplicateMapKeyException(key)
+        }
+    }
 }
 
 private open class CborListReader(cbor: Cbor, decoder: CborDecoder) : CborReader(cbor, decoder) {

formats/cbor/commonTest/src/kotlinx/serialization/cbor/CborStrictModeTest.kt

@@ -0,0 +1,21 @@
+package kotlinx.serialization.cbor
+
+import kotlinx.serialization.assertFailsWithMessage
+import kotlinx.serialization.decodeFromByteArray
+import kotlinx.serialization.HexConverter
+import kotlinx.serialization.DuplicateMapKeyException
+import kotlin.test.Test
+import kotlin.test.assertEquals
+
+class CborStrictModeTest {
+    private val strict = Cbor { allowDuplicateKeys = false }
+
+    /** Duplicate keys are rejected in generic maps. */
+    @Test
+    fun testDuplicateKeysInMap() {
+        val duplicateKeys = HexConverter.parseHexBinary("A2617805617806")
+        assertFailsWithMessage<DuplicateMapKeyException>("Duplicate keys not allowed") {
+            strict.decodeFromByteArray<Map<String, Long>>(duplicateKeys)
+        }
+    }
+}