Serialization of Set as Collection does not work

[sschuberth]

Describe the bug

I'm using Retrofit with kotlinx.serialization and the following code:

@Serializable
data class PackagesWrapper(
    val purls: Collection<String>
)

@POST("api/packages/bulk_search")
suspend fun getPackageVulnerabilities(@Body packageUrls: PackagesWrapper): List<PackageVulnerabilities>

When calling service.getPackageVulnerabilities(PackagesWrapper(packageMap.keys)) where keys is a Set, I get

java.lang.ClassCastException: class java.util.LinkedHashMap$LinkedKeySet cannot be cast to class java.util.List (java.util.LinkedHashMap$LinkedKeySet and java.util.List are in module java.base of loader 'bootstrap')

To Reproduce

Checkout https://github.com/oss-review-toolkit/ort/tree/vc2kxs-bug and run ./gradlew :advisor:test --tests org.ossreviewtoolkit.advisor.advisors.VulnerableCodeTest.

Expected behavior

As a Set is a Collection that can be serialized to a JSON array, I'd expected this to just work. However, ArrayListSerializer seems to hard-code in ListLikeSerializer<E, List<E>, ArrayList<E>>(element) that the "list like" collection needs to be a List.

Environment

• Kotlin version: 1.4.32
• Library version: 1.1.0
• Kotlin platforms: JVM
• Gradle version: 7.0
• IDE version (if bug is related to the IDE): IntellijIDEA 2021.1
• Other relevant context: Windows 10 64-bit

[sandwwraith]

Serializer is determined by the static type without looking at the actual instance. For now, we do not have special collection serializer, so we use ArrayListSerializer instead (

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

Line 102 in 83d0faa

Collection::class, List::class, MutableList::class, ArrayList::class -> ArrayListSerializer(serializers[0])

). It is possible to support sets in there, but it still would be unclear how to deserailize a collection if not to the list.

I wonder why you need exactly Collection static type and why you can't replace it with val purls: Collection<String>? Such an approach may lead to incorrect behavior: if you deserialize your definition, you will still get List instead of Set in the purls, because Collection type allows it.

[sschuberth]

I wonder why you need exactly Collection static type and why you can't replace it with val purls: Collection<String>?

I'm confused, I already am using val purls: Collection<String>. Did you mean val purls: List<String>? I'm using Collection<String> for convenience as I sometimes want to pass in a Set and sometimes a List, and want to avoid an additional toList() conversion.

if you deserialize your definition, you will still get List instead of Set in the purls, because Collection type allows it.

Which would be fine with me. But actually in this case, the class only ever gets serialized, but never deserialized.

[sandwwraith]

Oh sorry, I think I've got distracted and mistyped. I meant

I wonder why you need exactly Collection static type and why you can't replace it with val purls: Set?

Use-case ' a Set and sometimes a List, and want to avoid an additional toList() conversion' looks reasonable

[sschuberth]

Serializer is determined by the static type without looking at the actual instance.

Also, to me that's exactly an argument for the ArrayListSerializer to actually be able to serialize generic collections, without assuming them to be Lists.

[sschuberth]

Any idea how to start addressing this @sandwwraith? Would the right approach be to try generalizing ArrayListSerializer to take a Collection instead of a List, or should there be a new SetListSerializer (or something like this)?

[sandwwraith]

I'd say we need to add new CollectionSerializer: ListLikeSerializer with override fun Collection<E>.collectionSize(): Int = size and override fun Collection<E>.collectionIterator and make it a base class for ArrayListSerializer and set serializers (and remove collectionSize methods from them). Then any of list or set serializers would be able to serialize any Collection properly, deserializing it back to serializer's respective builder

[sschuberth]

Thanks for the hints! I tried to come up with something here, but unfortunately I'm running into a compile error:

e: kotlinx.serialization\core\commonMain\src\kotlinx\serialization\builtins\BuiltinSerializers.kt: (176, 5): Type mismatch: inferred type is ArrayListSerializer<T> but KSerializer<List<T>> was expected

Anymore hints?

[sandwwraith]

@sschuberth Try

internal abstract class CollectionSerializer<E, C: Collection<E>, B>(element: KSerializer<E>) : ListLikeSerializer<E, C, B>(element) {
    override fun C.collectionSize(): Int = size
    override fun C.collectionIterator(): Iterator<E> = iterator()
}

internal class ArrayListSerializer<E>(element: KSerializer<E>) : CollectionSerializer<E, List<E>, ArrayList<E>>(element)

[sschuberth]

Thanks @sandwwraith, that worked. Do you have one more hint for me in which test class(es) to add a test for this?

[sandwwraith]

I think there should be various tests for serializing sets as lists and vice versa. At least, add your use-case (with Collection static type)

[sschuberth]

I filed a draft PR to move forward with this, in the hope that tests would be executed as part of PR checks, but unfortunately no tests seem to be run as part of the PR 😢