Class discriminator conflict check rework

formats/json-tests/commonTest/src/kotlinx/serialization/features/DefaultPolymorphicSerializerTest.kt

@@ -6,6 +6,7 @@ package kotlinx.serialization.features
 import kotlinx.serialization.*
 import kotlinx.serialization.json.*
 import kotlinx.serialization.modules.*
+import kotlinx.serialization.test.assertFailsWithMessage
 import kotlin.test.*
 
 class DefaultPolymorphicSerializerTest : JsonTestBase() {
@@ -33,4 +34,17 @@ class DefaultPolymorphicSerializerTest : JsonTestBase() {
             json.decodeFromString<Project>(""" {"type":"unknown","name":"example"}""", it))
     }
 
+    @Test
+    fun defaultSerializerConflictWithDiscriminatorNotAllowed() {
+        @Suppress("UNCHECKED_CAST") val module = SerializersModule {
+            polymorphicDefaultSerializer(Project::class) {
+                DefaultProject.serializer() as KSerializer<Project>
+            }
+        }
+        val j = Json { serializersModule = module }
+        assertFailsWithMessage<SerializationException>("Class 'kotlinx.serialization.features.DefaultPolymorphicSerializerTest.DefaultProject' cannot be serialized as base class 'kotlinx.serialization.Polymorphic<Project>' because it has property name that conflicts with JSON class discriminator 'type'.") {
+            j.encodeToString<Project>(DefaultProject("example", "custom"))
+        }
+    }
+
 }

formats/json-tests/commonTest/src/kotlinx/serialization/features/JsonClassDiscriminatorTest.kt

@@ -8,6 +8,7 @@ import kotlinx.serialization.*
 import kotlinx.serialization.builtins.*
 import kotlinx.serialization.json.*
 import kotlinx.serialization.modules.*
+import kotlinx.serialization.test.assertFailsWithMessage
 import kotlin.test.*
 
 class JsonClassDiscriminatorTest : JsonTestBase() {
@@ -110,4 +111,21 @@ class JsonClassDiscriminatorTest : JsonTestBase() {
             json
         )
     }
+
+    @Serializable
+    @JsonClassDiscriminator("type2")
+    @SerialName("Foo")
+    sealed interface Foo
+
+    @Serializable
+    @SerialName("FooImpl")
+    data class FooImpl(val type2: String) : Foo
+
+    @Test
+    fun testCannotHaveConflictWithJsonClassDiscriminator() {
+        assertFailsWithMessage<SerializationException>("Class 'FooImpl' cannot be serialized as base class 'Foo' because it has property name that conflicts with JSON class discriminator 'type2'") {
+            Json.encodeToString<Foo>( FooImpl("foo"))
+        }
+    }
+
 }

formats/json-tests/commonTest/src/kotlinx/serialization/features/JsonNamingStrategyTest.kt

@@ -182,6 +182,11 @@ class JsonNamingStrategyTest : JsonTestBase() {
         val json = Json(jsonWithNaming) {
             ignoreUnknownKeys = true
         }
+        parametrizedTest { mode ->
+            assertFailsWithMessage<SerializationException>("The transformed name 'test_case' for property test_case already exists") {
+                json.encodeToString(CollisionCheckPrimary("a", "b"))
+            }
+        }
         parametrizedTest { mode ->
             assertFailsWithMessage<SerializationException>("The suggested name 'test_case' for property test_case is already one of the names for property testCase") {
                 json.decodeFromString<CollisionCheckPrimary>("""{"test_case":"a"}""", mode)
@@ -209,6 +214,7 @@ class JsonNamingStrategyTest : JsonTestBase() {
     }
 
     @Serializable
+    @SerialName("SealedBase")
     sealed interface SealedBase {
         @Serializable
         @JsonClassDiscriminator("typeSub")
@@ -239,4 +245,28 @@ class JsonNamingStrategyTest : JsonTestBase() {
             json
         )
     }
+
+    @Test
+    fun testClashWithDiscriminator() {
+        val correctJson = Json(jsonWithNaming) {
+            classDiscriminator = "test_base"
+        }
+        val holder = Holder(SealedBase.SealedSub2(), SealedBase.SealedMid.SealedSub1)
+
+        // Should pass because same name is only on different levels
+        assertJsonFormAndRestored(
+            Holder.serializer(),
+            holder,
+            """{"test_base":{"test_base":"SealedSub2","test_case":0},"test_mid":{"typeSub":"SealedSub1"}}""",
+            correctJson
+        )
+
+        val incorrectJson = Json(jsonWithNaming) {
+            classDiscriminator = "test_case"
+        }
+
+        assertFailsWithMessage<SerializationException>("Class 'SealedSub2' cannot be serialized as base class 'SealedBase' because it has property name that conflicts with JSON class discriminator 'test_case'.") {
+            incorrectJson.encodeToString<Holder>(holder)
+        }
+    }
 }

formats/json-tests/commonTest/src/kotlinx/serialization/json/polymorphic/JsonClassDiscriminatorModeBaseTest.kt

@@ -80,7 +80,7 @@ abstract class JsonClassDiscriminatorModeBaseTest(
     @SerialName("mixed")
     data class MixedPolyAndRegular(val sb: SealedBase, val sc: SealedContainer, val i: Inner)
 
-    private inline fun <reified T> doTest(expected: String, obj: T) {
+    internal inline fun <reified T> doTest(expected: String, obj: T) {
         parametrizedTest { mode ->
             val serialized = json.encodeToString(serializer<T>(), obj, mode)
             assertEquals(expected, serialized, "Failed with mode = $mode")
@@ -150,4 +150,8 @@ abstract class JsonClassDiscriminatorModeBaseTest(
         val nm = NullableMixed(null, null)
        doTest(expected, nm)
     }
+
+    @Serializable
+    @SerialName("Conflict")
+    class Conflict(val type: String)
 }

formats/json-tests/commonTest/src/kotlinx/serialization/json/polymorphic/JsonClassDiscriminatorModeTest.kt

@@ -7,6 +7,7 @@ package kotlinx.serialization.json.polymorphic
 import kotlinx.serialization.*
 import kotlinx.serialization.json.*
 import kotlinx.serialization.modules.*
+import kotlinx.serialization.test.assertFailsWithMessage
 import kotlin.test.*
 
 class ClassDiscriminatorModeAllObjectsTest :
@@ -46,6 +47,13 @@ class ClassDiscriminatorModeAllObjectsTest :
     @Test
     fun testNullable() = testNullable("""{"type":"NullableMixed","sb":null,"sc":null}""")
 
+    @Test
+    fun testConflictWithDiscriminator() {
+        assertFailsWithMessage<SerializationException>("Class 'Conflict' cannot be serialized in ALL_JSON_OBJECTS class discriminator mode because it has property name that conflicts with JSON class discriminator 'type'") {
+            json.encodeToString(Conflict("foo"))
+        }
+    }
+
 }
 
 class ClassDiscriminatorModeNoneTest :
@@ -83,6 +91,11 @@ class ClassDiscriminatorModeNoneTest :
     @Test
     fun testNullable() = testNullable("""{"sb":null,"sc":null}""")
 
+    @Test
+    fun testConflictWithDiscriminator() {
+        doTest("""{"type":"foo"}""", Conflict("foo"))
+    }
+
     interface CommandType
 
     @Serializable // For Kotlin/JS

formats/json-tests/commonTest/src/kotlinx/serialization/modules/SerialNameCollisionInSealedClassesTest.kt

@@ -24,8 +24,8 @@ class SerialNameCollisionInSealedClassesTest {
 
     @Test
     fun testCollisionWithDiscriminator() {
-        assertFailsWith<IllegalStateException> { Json("type").encodeToString(Base.serializer(), Base.Child("a")) }
-        assertFailsWith<IllegalStateException> { Json("type2").encodeToString(Base.serializer(), Base.Child("a")) }
+        assertFailsWith<SerializationException> { Json("type").encodeToString(Base.serializer(), Base.Child("a")) }
+        assertFailsWith<SerializationException> { Json("type2").encodeToString(Base.serializer(), Base.Child("a")) }
         Json("f").encodeToString(Base.serializer(), Base.Child("a"))
     }
 

formats/json-tests/commonTest/src/kotlinx/serialization/modules/SerialNameCollisionTest.kt

@@ -6,6 +6,7 @@ package kotlinx.serialization.modules
 
 import kotlinx.serialization.*
 import kotlinx.serialization.json.*
+import kotlinx.serialization.test.assertFailsWithMessage
 import kotlin.test.*
 
 private const val prefix = "kotlinx.serialization.modules.SerialNameCollisionTest"
@@ -34,7 +35,6 @@ class SerialNameCollisionTest {
         classDiscriminator = discriminator
         this.useArrayPolymorphism = useArrayPolymorphism
         serializersModule = context
-
     }
 
     @Test
@@ -45,9 +45,15 @@ class SerialNameCollisionTest {
             }
         }
 
-        assertFailsWith<IllegalArgumentException> { Json("type", module) }
-        assertFailsWith<IllegalArgumentException> { Json("type2", module) }
-        Json("type3", module) // OK
+        assertFailsWithMessage<SerializationException>("Class 'kotlinx.serialization.modules.SerialNameCollisionTest.Derived' cannot be serialized as base class 'kotlinx.serialization.Polymorphic<Base>' because it has property name that conflicts with JSON class discriminator 'type'.") {
+            Json("type", module).encodeToString<Base>(Derived("foo", "bar"))
+        }
+        assertFailsWithMessage<SerializationException>("Class 'kotlinx.serialization.modules.SerialNameCollisionTest.Derived' cannot be serialized as base class 'kotlinx.serialization.Polymorphic<Base>' because it has property name that conflicts with JSON class discriminator 'type2'.") {
+            Json("type2", module).encodeToString<Base>(Derived("foo", "bar"))
+        }
+        assertEquals("{\"type3\":\"kotlinx.serialization.modules.SerialNameCollisionTest.Derived\",\"type\":\"foo\",\"type2\":\"bar\"}",
+            Json("type3", module).encodeToString<Base>(Derived("foo", "bar"))
+        )
     }
 
     @Test
@@ -68,10 +74,18 @@ class SerialNameCollisionTest {
             }
         }
 
-        assertFailsWith<IllegalArgumentException> { Json("type", module) }
-        assertFailsWith<IllegalArgumentException> { Json("type2", module) }
-        assertFailsWith<IllegalArgumentException> { Json("t3", module) }
-        Json("t4", module) // OK
+        assertFailsWithMessage<SerializationException>("Class 'kotlinx.serialization.modules.SerialNameCollisionTest.DerivedCustomized' cannot be serialized as base class 'kotlinx.serialization.Polymorphic<Base>' because it has property name that conflicts with JSON class discriminator 'type'.") {
+            Json("type", module).encodeToString<Base>(DerivedCustomized("foo", "bar", "t3"))
+        }
+        assertFailsWithMessage<SerializationException>("Class 'kotlinx.serialization.modules.SerialNameCollisionTest.DerivedCustomized' cannot be serialized as base class 'kotlinx.serialization.Polymorphic<Base>' because it has property name that conflicts with JSON class discriminator 'type2'.") {
+            Json("type2", module).encodeToString<Base>(DerivedCustomized("foo", "bar", "t3"))
+        }
+        assertFailsWithMessage<SerializationException>("Class 'kotlinx.serialization.modules.SerialNameCollisionTest.DerivedCustomized' cannot be serialized as base class 'kotlinx.serialization.Polymorphic<Base>' because it has property name that conflicts with JSON class discriminator 't3'.") {
+            Json("t3", module).encodeToString<Base>(DerivedCustomized("foo", "bar", "t3"))
+        }
+        assertEquals("{\"t4\":\"kotlinx.serialization.modules.SerialNameCollisionTest.DerivedCustomized\",\"type\":\"foo\",\"type2\":\"bar\",\"t3\":\"t3\"}",
+        Json("t4", module).encodeToString<Base>(DerivedCustomized("foo", "bar", "t3"))
+        )
 
     }
 

formats/json/commonMain/src/kotlinx/serialization/json/internal/JsonNamesMap.kt

@@ -8,8 +8,8 @@ package kotlinx.serialization.json.internal
 import kotlinx.serialization.*
 import kotlinx.serialization.descriptors.*
 import kotlinx.serialization.encoding.*
+import kotlinx.serialization.internal.jsonCachedSerialNames
 import kotlinx.serialization.json.*
-import kotlin.native.concurrent.*
 
 internal val JsonDeserializationNamesKey = DescriptorSchemaCache.Key<Map<String, Int>>()
 
@@ -19,7 +19,7 @@ private fun SerialDescriptor.buildDeserializationNamesMap(json: Json): Map<Strin
     fun MutableMap<String, Int>.putOrThrow(name: String, index: Int) {
         val entity = if (kind == SerialKind.ENUM) "enum value" else "property"
         if (name in this) {
-            throw JsonException(
+            throw JsonDecodingException(
                 "The suggested name '$name' for $entity ${getElementName(index)} is already one of the names for $entity " +
                         "${getElementName(getValue(name))} in ${this@buildDeserializationNamesMap}"
             )
@@ -55,9 +55,15 @@ internal fun Json.deserializationNamesMap(descriptor: SerialDescriptor): Map<Str
 
 internal fun SerialDescriptor.serializationNamesIndices(json: Json, strategy: JsonNamingStrategy): Array<String> =
     json.schemaCache.getOrPut(this, JsonSerializationNamesKey) {
+        val trackingSet = mutableSetOf<String>()
         Array(elementsCount) { i ->
             val baseName = getElementName(i)
-            strategy.serialNameForJson(this, i, baseName)
+            val name = strategy.serialNameForJson(this, i, baseName)
+            if (!trackingSet.add(name)) throw JsonException(
+                "The transformed name '$name' for property $baseName already exists " +
+                    "in ${this@serializationNamesIndices}"
+            )
+            name
         }
     }
 
@@ -66,6 +72,12 @@ internal fun SerialDescriptor.getJsonElementName(json: Json, index: Int): String
     return if (strategy == null) getElementName(index) else serializationNamesIndices(json, strategy)[index]
 }
 
+// Emits only names used for encoding, i.e. from naming strategy, but not from @JsonNames
+internal fun SerialDescriptor.getJsonEncodedNames(json: Json): Set<String> {
+    val strategy = namingStrategy(json)
+    return if (strategy == null) jsonCachedSerialNames() else serializationNamesIndices(json, strategy).toSet()
+}
+
 internal fun SerialDescriptor.namingStrategy(json: Json) =
     if (kind == StructureKind.CLASS) json.configuration.namingStrategy else null
 
@@ -79,7 +91,6 @@ private fun Json.decodeCaseInsensitive(descriptor: SerialDescriptor) =
  * Serves same purpose as [SerialDescriptor.getElementIndex] but respects [JsonNames] annotation
  * and [JsonConfiguration] settings.
  */
-@OptIn(ExperimentalSerializationApi::class)
 internal fun SerialDescriptor.getJsonNameIndex(json: Json, name: String): Int {
     if (json.decodeCaseInsensitive(this)) {
         return getJsonNameIndexSlowPath(json, name.lowercase())

formats/json/commonMain/src/kotlinx/serialization/json/internal/JsonSerializersModuleValidator.kt

@@ -15,7 +15,6 @@ internal class JsonSerializersModuleValidator(
     configuration: JsonConfiguration,
 ) : SerializersModuleCollector {
 
-    private val discriminator: String = configuration.classDiscriminator
     private val useArrayPolymorphism: Boolean = configuration.useArrayPolymorphism
     private val isDiscriminatorRequired = configuration.classDiscriminatorMode != ClassDiscriminatorMode.NONE
 
@@ -33,10 +32,6 @@ internal class JsonSerializersModuleValidator(
     ) {
         val descriptor = actualSerializer.descriptor
         checkKind(descriptor, actualClass)
-        if (!useArrayPolymorphism && isDiscriminatorRequired) {
-            // Collisions with "type" can happen only for JSON polymorphism
-            checkDiscriminatorCollisions(descriptor, actualClass)
-        }
     }
 
     private fun checkKind(descriptor: SerialDescriptor, actualClass: KClass<*>) {
@@ -62,23 +57,6 @@ internal class JsonSerializersModuleValidator(
         }
     }
 
-    private fun checkDiscriminatorCollisions(
-        descriptor: SerialDescriptor,
-        actualClass: KClass<*>
-    ) {
-        for (i in 0 until descriptor.elementsCount) {
-            val name = descriptor.getElementName(i)
-            if (name == discriminator) {
-                throw IllegalArgumentException(
-                    "Polymorphic serializer for $actualClass has property '$name' that conflicts " +
-                            "with JSON class discriminator. You can either change class discriminator in JsonConfiguration, " +
-                            "rename property with @SerialName annotation " +
-                            "or fall back to array polymorphism"
-                )
-            }
-        }
-    }
-
     override fun <Base : Any> polymorphicDefaultSerializer(
         baseClass: KClass<Base>,
         defaultSerializerProvider: (value: Base) -> SerializationStrategy<Base>?

formats/json/commonMain/src/kotlinx/serialization/json/internal/Polymorphic.kt

@@ -9,8 +9,6 @@ import kotlinx.serialization.*
 import kotlinx.serialization.descriptors.*
 import kotlinx.serialization.internal.*
 import kotlinx.serialization.json.*
-import kotlinx.serialization.modules.*
-import kotlin.jvm.*
 
 @Suppress("UNCHECKED_CAST")
 internal inline fun <T> JsonEncoder.encodePolymorphically(
@@ -37,32 +35,35 @@ internal inline fun <T> JsonEncoder.encodePolymorphically(
         val casted = serializer as AbstractPolymorphicSerializer<Any>
         requireNotNull(value) { "Value for serializer ${serializer.descriptor} should always be non-null. Please report issue to the kotlinx.serialization tracker." }
         val actual = casted.findPolymorphicSerializer(this, value)
-        if (baseClassDiscriminator != null) {
-            validateIfSealed(serializer, actual, baseClassDiscriminator)
-            checkKind(actual.descriptor.kind)
-        }
         actual as SerializationStrategy<T>
     } else serializer
 
-    if (baseClassDiscriminator != null) ifPolymorphic(baseClassDiscriminator, actualSerializer.descriptor.serialName)
+    if (baseClassDiscriminator != null) {
+        json.checkEncodingConflicts(serializer, actualSerializer, baseClassDiscriminator)
+        checkKind(actualSerializer.descriptor.kind)
+        ifPolymorphic(baseClassDiscriminator, actualSerializer.descriptor.serialName)
+    }
     actualSerializer.serialize(this, value)
 }
 
-private fun validateIfSealed(
+private fun Json.checkEncodingConflicts(
     serializer: SerializationStrategy<*>,
     actualSerializer: SerializationStrategy<*>,
     classDiscriminator: String
 ) {
-    if (serializer !is SealedClassSerializer<*>) return
-    @Suppress("DEPRECATION_ERROR")
-    if (classDiscriminator in actualSerializer.descriptor.jsonCachedSerialNames()) {
+    if (classDiscriminator in actualSerializer.descriptor.getJsonEncodedNames(this)) {
         val baseName = serializer.descriptor.serialName
         val actualName = actualSerializer.descriptor.serialName
-        error(
-            "Sealed class '$actualName' cannot be serialized as base class '$baseName' because" +
+
+        val text = when {
+            configuration.classDiscriminatorMode == ClassDiscriminatorMode.ALL_JSON_OBJECTS && baseName == actualName -> "in ALL_JSON_OBJECTS class discriminator mode"
+            else -> "as base class '$baseName'"
+        }
+        throw JsonEncodingException(
+            "Class '$actualName' cannot be serialized $text because" +
                     " it has property name that conflicts with JSON class discriminator '$classDiscriminator'. " +
-                    "You can either change class discriminator in JsonConfiguration, " +
-                    "rename property with @SerialName annotation or fall back to array polymorphism"
+                    "You can either change class discriminator in JsonConfiguration, or " +
+                    "rename property with @SerialName annotation."
         )
     }
 }