Skip to content

Commit 12781ad

Browse files
committed
Adress feedback comments:
- Make it clearer that the hierarchy from the sealed class must only be concrete/sealed. Any open descendants fail. - Revert back to the list of subclasses to be a list, rather than a map. - Fix some documentation/exception text errors. - Make the test for open descendants more robust/comprehensive.
1 parent ea9e850 commit 12781ad

File tree

3 files changed

+56
-18
lines changed

3 files changed

+56
-18
lines changed

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

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,20 +20,20 @@ public class PolymorphicModuleBuilder<in Base : Any> @PublishedApi internal cons
2020
private val baseClass: KClass<Base>,
2121
private val baseSerializer: KSerializer<Base>? = null
2222
) {
23-
private val subclasses: MutableMap<KClass<out Base>, KSerializer<out Base>> = mutableMapOf()
23+
private val subclasses: MutableList<Pair<KClass<out Base>, KSerializer<out Base>>> = mutableListOf()
2424
private var defaultSerializerProvider: ((Base) -> SerializationStrategy<Base>?)? = null
2525
private var defaultDeserializerProvider: ((String?) -> DeserializationStrategy<Base>?)? = null
2626

2727

2828
/**
2929
* Registers the child serializers for the sealed [subclass] [serializer] in the resulting module under
30-
* the [base class][Base]. Please note that type `T` must be sealed, if not a runtime error will
30+
* the [base class][Base]. Please note that type `T` has to be sealed and have a standard serializer, if not a runtime error will
3131
* be thrown at registration time. This function is a convenience function for the version that
3232
* receives a serializer.
3333
*
3434
*
3535
* Example:
36-
* ```kotlin
36+
* ```
3737
* interface Base
3838
*
3939
* @Serializable
@@ -75,20 +75,21 @@ public class PolymorphicModuleBuilder<in Base : Any> @PublishedApi internal cons
7575
* }
7676
* ```
7777
*
78-
* It is an error if th
78+
* Note that if Sub1 is itself open polymorphic this is an error.
79+
*
7980
*/
8081
@ExperimentalSerializationApi
8182
public fun <T: Base> subclassesOfSealed(serializer: KSerializer<T>) {
8283
// Note that the parameter type is `KSerializer` as `SealedClassSerializer` is an internal type
8384
// not available to users
8485
require(serializer is SealedClassSerializer) {
85-
"subClassesOf only supports automatic adding of subclasses of sealed types."
86+
"subclassesOfSealed only supports automatic adding of subclasses of sealed types with standard serializers."
8687
}
8788
for ((subsubclass, subserializer) in serializer.class2Serializer.entries) {
8889
// This error would be caught by the Json format in its validation, but this is format specific
8990
require (subserializer.descriptor.kind != PolymorphicKind.OPEN) {
90-
"It is not possible to register subclasses of sealed types when those subclasses " +
91-
"themselves are (open) polymorphic, as this would represent an incomplete hierarchy"
91+
"It is not possible to register subclasses (${serializer.descriptor.serialName}) of sealed types when those subclasses " +
92+
"themselves are (open) polymorphic, as this would represent an incomplete hierarchy."
9293
}
9394
@Suppress("UNCHECKED_CAST")
9495
// We don't know the type here, but it matches if correct in the sealed serializer.
@@ -100,7 +101,7 @@ public class PolymorphicModuleBuilder<in Base : Any> @PublishedApi internal cons
100101
* Registers a [subclass] [serializer] in the resulting module under the [base class][Base].
101102
*/
102103
public fun <T : Base> subclass(subclass: KClass<T>, serializer: KSerializer<T>) {
103-
subclasses[subclass] = serializer
104+
subclasses.add(subclass to serializer)
104105
}
105106

106107
/**

docs/polymorphism.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,7 @@ A sealed parent interface or class can be used to directly register all its chil
419419
This will expose all children that would be available when serializing the parent directly, but now as sealed. Please
420420
note that this is will remain open serialization, and the sealed parent serializer will not be used in serialization.
421421
In addition, it is not valid if the hierarchy contains open (not sealed) polymorphic children (this will result in
422-
an error at runtime).
422+
an error at runtime). In other words all children/descendants must be either concrete or sealed.
423423

424424
<!--- TEST LINES_START -->
425425

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

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ class PolymorphicSealedChildTest {
1515
@Serializable
1616
data class FooHolder(
1717
val someMetadata: Int,
18-
val payload: List<@Polymorphic FooBase>
18+
val payload: List<FooBase>
1919
)
2020

2121
@Serializable
@@ -29,7 +29,15 @@ class PolymorphicSealedChildTest {
2929
data class Bar(val bar: Int) : Foo()
3030
@Serializable
3131
@SerialName("Baz")
32-
data class Baz(val baz: String) : Foo()
32+
sealed class Baz : Foo()
33+
34+
@Serializable
35+
@SerialName("BazC1")
36+
data class SealedBazChild1(val baz: String): Baz()
37+
@Serializable
38+
@SerialName("BazC2")
39+
data class SealedBazChild2(val baz: String): Baz()
40+
3341
}
3442

3543
@Serializable
@@ -39,8 +47,10 @@ class PolymorphicSealedChildTest {
3947
@Serializable
4048
abstract class Baz: FooOpen()
4149
@Serializable
50+
@SerialName("BazOC1")
4251
data class BazChild1(val baz: String) : Baz()
4352
@Serializable
53+
@SerialName("BazOC2")
4454
data class BazChild2(val baz: String) : Baz()
4555
}
4656

@@ -52,30 +62,57 @@ class PolymorphicSealedChildTest {
5262

5363
val json = Json { serializersModule = sealedModule }
5464

65+
/**
66+
* Test that attempting to register a grandchild (any descendant of the sealed intermediary)
67+
* fails with an exception
68+
*/
5569
@Test
5670
fun testOpenGrandchildIsInvalid() {
57-
assertFailsWith<IllegalArgumentException> {
58-
SerializersModule {
59-
polymorphic(FooBase::class) {
60-
subclassesOfSealed<FooOpen>()
71+
72+
lateinit var openJson: Json
73+
val e = assertFailsWith<IllegalArgumentException> {
74+
openJson = Json {
75+
serializersModule = SerializersModule {
76+
polymorphic(FooBase::class) {
77+
subclassesOfSealed<FooOpen>()
78+
fail("This code should be unreachable as the previous operation fails")
79+
subclass(FooOpen.BazChild1::class)
80+
subclass(FooOpen.BazChild2::class)
81+
}
6182
}
6283
}
84+
85+
fail("Unreachable code that would represent the usage if valid (which is isn't per policy)")
86+
assertStringFormAndRestored(
87+
"""{"someMetadata":43,"payload":[{"type":"BazOC1","baz":"aaa"}]}""",
88+
FooHolder(43,listOf(FooOpen.BazChild1("aaa"))),
89+
FooHolder.serializer(),
90+
openJson
91+
)
6392
}
93+
assertContains(assertNotNull(e.message), "FooOpen", )
94+
assertContains(assertNotNull(e.message), "incomplete hierarchy", )
6495
}
6596

97+
/**
98+
* This tests both a directly sealed child (Bar) and a (sealed) grandchild (SealedBazChild1)
99+
* Nesting sealed hierarchies is valid (and flattened by the plugin).
100+
*/
66101
@Test
67102
fun testSaveSealedClassesList() {
68103
assertStringFormAndRestored(
69104
"""{"someMetadata":42,"payload":[
70105
|{"type":"Bar","bar":1},
71-
|{"type":"Baz","baz":"2"}]}""".trimMargin().replace("\n", ""),
72-
FooHolder(42, listOf(Foo.Bar(1), Foo.Baz("2"))),
106+
|{"type":"BazC1","baz":"2"}]}""".trimMargin().replace("\n", ""),
107+
FooHolder(42, listOf(Foo.Bar(1), Foo.SealedBazChild1("2"))),
73108
FooHolder.serializer(),
74109
json,
75-
printResult = true
76110
)
77111
}
78112

113+
/**
114+
* Test that a simple direct descendant of the serialized base type can be serialized.
115+
*/
79116
@Test
80117
fun testCanSerializeSealedClassPolymorphicallyOnTopLevel() {
81118
assertStringFormAndRestored(

0 commit comments

Comments
 (0)