Skip to content

Commit 6ffda99

Browse files
committed
Correctly handle serial name conflict for different classes
in SerializersModule.overwriteWith. Previous KClass should be removed from the map as well to avoid creating module with asymmetric mappings. Fixes #2820
1 parent 38977b3 commit 6ffda99

File tree

3 files changed

+46
-26
lines changed

3 files changed

+46
-26
lines changed

core/commonMain/src/kotlinx/serialization/modules/SerializersModuleBuilders.kt

Lines changed: 17 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -192,39 +192,30 @@ public class SerializersModuleBuilder @PublishedApi internal constructor() : Ser
192192
concreteSerializer: KSerializer<Sub>,
193193
allowOverwrite: Boolean = false
194194
) {
195-
// Check for overwrite
196195
val name = concreteSerializer.descriptor.serialName
197196
val baseClassSerializers = polyBase2Serializers.getOrPut(baseClass, ::hashMapOf)
198-
val previousSerializer = baseClassSerializers[concreteClass]
199197
val names = polyBase2NamedSerializers.getOrPut(baseClass, ::hashMapOf)
200-
if (allowOverwrite) {
201-
// Remove previous serializers from name mapping
202-
if (previousSerializer != null) {
203-
names.remove(previousSerializer.descriptor.serialName)
204-
}
205-
// Update mappings
206-
baseClassSerializers[concreteClass] = concreteSerializer
207-
names[name] = concreteSerializer
208-
return
209-
}
210-
// Overwrite prohibited
211-
if (previousSerializer != null) {
212-
if (previousSerializer != concreteSerializer) {
213-
throw SerializerAlreadyRegisteredException(baseClass, concreteClass)
214-
} else {
215-
// Cleanup name mapping
216-
names.remove(previousSerializer.descriptor.serialName)
217-
}
198+
199+
// Check KClass conflict
200+
val previousSerializer = baseClassSerializers[concreteClass]
201+
if (previousSerializer != null && previousSerializer != concreteSerializer) {
202+
if (allowOverwrite) names.remove(previousSerializer.descriptor.serialName)
203+
else throw SerializerAlreadyRegisteredException(baseClass, concreteClass)
218204
}
205+
206+
// Check SerialName conflict
219207
val previousByName = names[name]
220-
if (previousByName != null) {
221-
val conflictingClass = polyBase2Serializers[baseClass]!!.asSequence().find { it.value === previousByName }
222-
throw IllegalArgumentException(
223-
"Multiple polymorphic serializers for base class '$baseClass' " +
224-
"have the same serial name '$name': '$concreteClass' and '$conflictingClass'"
208+
if (previousByName != null && previousByName != concreteSerializer) {
209+
val previousClass = baseClassSerializers.asSequence().find { it.value === previousByName }?.key
210+
?: error("Name $name is registered in the module but no Kotlin class is associated with it.")
211+
212+
if (allowOverwrite) baseClassSerializers.remove(previousClass)
213+
else throw IllegalArgumentException(
214+
"Multiple polymorphic serializers in a scope of '$baseClass' " +
215+
"have the same serial name '$name': $concreteSerializer for '$concreteClass' and $previousByName for '$previousClass'"
225216
)
226217
}
227-
// Overwrite if no conflicts
218+
228219
baseClassSerializers[concreteClass] = concreteSerializer
229220
names[name] = concreteSerializer
230221
}

core/commonTest/src/kotlinx/serialization/modules/ModuleBuildersTest.kt

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import kotlinx.serialization.*
1010
import kotlinx.serialization.builtins.*
1111
import kotlinx.serialization.descriptors.*
1212
import kotlinx.serialization.encoding.*
13+
import kotlinx.serialization.test.assertFailsWithMessage
1314
import kotlin.reflect.*
1415
import kotlin.test.*
1516

@@ -176,6 +177,10 @@ class ModuleBuildersTest {
176177
@SerialName("C")
177178
class C
178179

180+
@Serializable
181+
@SerialName("C")
182+
class C2
183+
179184
@Serializer(forClass = C::class)
180185
object CSerializer : KSerializer<C> {
181186
override val descriptor: SerialDescriptor = buildSerialDescriptor("AnotherName", StructureKind.OBJECT)
@@ -206,6 +211,26 @@ class ModuleBuildersTest {
206211
assertNull(result.getPolymorphic(Any::class, serializedClassName = "AnotherName"))
207212
}
208213

214+
@Test
215+
fun testOverwriteWithDifferentClass() {
216+
val c1 = SerializersModule {
217+
polymorphic<Any>(Any::class) {
218+
subclass(C::class)
219+
}
220+
}
221+
val c2 = SerializersModule {
222+
polymorphic<Any>(Any::class) {
223+
subclass(C2::class)
224+
}
225+
}
226+
assertFailsWithMessage<IllegalArgumentException>("Multiple polymorphic serializers in a scope of 'class kotlin.Any' have the same serial name 'C'") { c1 + c2 }
227+
val module = c1 overwriteWith c2
228+
// C should not be registered at all, C2 should be registered both under "C" and C2::class
229+
assertEquals(C2.serializer(), module.getPolymorphic(Any::class, serializedClassName = "C"))
230+
assertNull(module.getPolymorphic(Any::class, C()))
231+
assertEquals(C2.serializer(), module.getPolymorphic(Any::class, C2()))
232+
}
233+
209234
@Test
210235
fun testOverwriteWithSameSerialName() {
211236
val m1 = SerializersModule {

core/commonTest/src/kotlinx/serialization/test/TestHelpers.kt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,7 @@ inline fun jvmOnly(test: () -> Unit) {
3838
if (isJvm()) test()
3939
}
4040

41+
inline fun <reified T : Throwable> assertFailsWithMessage(message: String, block: () -> Unit) {
42+
val exception = assertFailsWith(T::class, null, block)
43+
assertTrue(exception.message!!.contains(message), "Expected message '${exception.message}' to contain substring '$message'")
44+
}

0 commit comments

Comments
 (0)