Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experimental UuidSerializer breaks projects with custom UUID serializer #2799

Closed
nielsvanvelzen opened this issue Aug 28, 2024 · 11 comments
Closed
Assignees

Comments

@nielsvanvelzen
Copy link

Describe the bug

A project using a custom UUID serializer with the name of the serial descriptor set to UUID (casing does not matter) will break with the recent 1.7.2 release. Even though the new serializer is not used, the serialization library will throw the following error:

The name of serial descriptor should uniquely identify associated serializer.
For serial name UUID there already exist UuidSerializer.
Please refer to SerialDescriptor documentation for additional information.
java.lang.IllegalArgumentException: The name of serial descriptor should uniquely identify associated serializer.
For serial name UUID there already exist UuidSerializer.
Please refer to SerialDescriptor documentation for additional information.

While I would argue this is fine to happen in a 1.8 release, it's not in a patch release. In this specific case, the serializer is added in an SDK that is used by multiple apps. If one of those apps were to update to kotlinx.serialization 1.7.3 they would also be affected by this issue.

To Reproduce

Expected behavior

In any 1.7.z release a custom serializer should just work.

Environment

  • Kotlin version: 2.0.20
  • Library version: 1.7.2
  • Kotlin platforms: JVM / Android
  • Gradle version: 8.5.2
  • IDE version (if bug is related to the IDE) IntellijIDEA 2024.2.0.2
@nielsvanvelzen
Copy link
Author

I forgot to mention, our current UUID serializer is for the java.util.UUID class. So it's not the same type that the serializer is made for. In the linked repository we have a typealias for that so it may not be obvious from reading the code.

@sandwwraith
Copy link
Member

It wasn't our intention to break users' serializers, especially those that work with java.util.UUID. It's just a bug, and even patch releases may contain them :(

I still want to point out that providing a library with a serializer that is named just "UUID" maybe not reliable since any of your clients can declare a serializer with the same short name, which may possibly lead to bugs

@sandwwraith sandwwraith self-assigned this Aug 29, 2024
sandwwraith added a commit that referenced this issue Aug 29, 2024
in PrimitiveSerialDescriptor constructor.

By doing so, we allow users to declare their own serializers with short names like "Uuid" or "Duration", even though it is not recommended — because they may have been declared before library updates.

Fixes #2799
@nielsvanvelzen
Copy link
Author

Thanks for picking it up! Regarding the names, I wasn't that familiar with the serialization library when I first wrote that uuid serializer. The documentation example also uses a simple name (just "Color") so I never thought much about it.

I'll update our serializers so they're prefixes to avoid any future problems. Thanks for the tip!

@qwwdfsad
Copy link
Collaborator

@sandwwraith could you please fix the example from the guide as well?

@chrisjenx
Copy link

Just ran into this too, downgraded back to 1.7.1

@chrisjenx
Copy link

I still want to point out that providing a library with a serializer that is named just "UUID" maybe not reliable since any of your clients can declare a serializer with the same short name, which may possibly lead to bugs

Wait so I can write a serialize with the same name as another one to override a library one? And it's case insensitive? That seems like a little bit of an oversight? They should at least be package name sensitive?

@chrisjenx
Copy link

Oh for reference my issue is similar but different, I just get UUIDSerializer class not found so not sure what happens but I guess the 1.7.2 version nukes my custom one so then it can't be found anymore?

@lukaszkalnik
Copy link
Contributor

lukaszkalnik commented Sep 13, 2024

@sandwwraith we have run into the same issue, and our serializer class has a custom name. Serializer class name that conflict are:

UUIDSerializer
MscoUUIDSerializer

Both of them cause conflict with the built-in serializer in 1.7.2

Caused by: java.lang.IllegalArgumentException: The name of serial descriptor should uniquely identify associated serializer.
For serial name UUID there already exist UuidSerializer.
Please refer to SerialDescriptor documentation for additional information.
at kotlinx.serialization.internal.PrimitivesKt.checkName(Primitives.kt:83)
at kotlinx.serialization.internal.PrimitivesKt.PrimitiveDescriptorSafe(Primitives.kt:73)
at kotlinx.serialization.descriptors.SerialDescriptorsKt.PrimitiveSerialDescriptor(SerialDescriptors.kt:91)
at com.myapplication.serializer.MscoUUIDSerializer.<clinit>(MscoUUIDSerializer.kt

We are also deserializing the java.util.UUID. And what seems to be in conflict here, ist the name of the class to be serialized (in this case UUID), not the name of the serializer itself.

@lukaszkalnik
Copy link
Contributor

Ah ok, I see: we have to change the name in the
PrimitiveSerialDescriptor("UUID", PrimitiveKind.STRING)
to something else than just UUID.

@pdvrieze
Copy link
Contributor

Ah ok, I see: we have to change the name in the
PrimitiveSerialDescriptor("UUID", PrimitiveKind.STRING)
to something else than just UUID.

Yes, the name is used by formats for caching (and resolving polymorphic children), and thus needs to be unique.

@chrisjenx
Copy link

I feel dumb as the error is pretty clear, but it only just clicked it's the string name of the descriptor not the object SerializerName..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants