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

ClassDiscriminatorMode in 3rd party formats (move to kxs-core, or rename w/ Json prefix) #2604

Open
BenWoodworth opened this issue Mar 18, 2024 · 3 comments
Labels

Comments

@BenWoodworth
Copy link
Contributor

What is your use-case and why do you need this feature?

I have a 3rd party format (knbt)[https://github.com/BenWoodworth/knbt] and a request in slack to implement a class discriminator mode similar to JSON's.

If ClassDiscriminatorMode were in kotlinx-serialization-core, then I could use the existing enum (and maybe throw an error during configuration if a specific mode)

It's awkward/confusing for me to add a similarly named enum into my library, notably if my library and kxs-json are both used in the same project (which is the situation for the person requesting the discriminator mode in my library)

Describe the solution you'd like

I couldn't find any discussion about why ClassDiscriminatorMode was added or the decisions behind it, but I think making it available as kotlinx.serialization.ClassDiscriminatorMode in kxs-core instead of in the json package would be convenient for other format libraries that want to add the functionality. (And since it's still marked as experimental in kxs-json, I think this might still be an option).

That could also make it more standard, though I can see issues if kxs wants to add a new mode to the enum later and libraries don't update their code to handle it.

Otherwise, maybe renaming it to be JsonClassDiscriminatorMode, Json-prefixed like other json-specific features, might be another option. Though I'm leaning towards moving this to core as a standard configuration option that formats could support

@sandwwraith
Copy link
Member

There are several reasons for this enum to be json-specific. First, other formats in serialization repo do not place class discriminator inside objects at all; they use 'default polymorphism', which is represented as [type, object] array. Second, not all enum entries are meaningful for all formats. While NEVER surely can be used in every format, there is also ALL_JSON_OBJECTS entry (which already contains JSON in its name). I'm not sure it will entirely fit other possible formats. For example, if you use array polymorphism, you can even make primitives polymorphic — ["kotlin.Int", 3], and they're obviously neither Json objects nor "[StructureKind.CLASS] or [StructureKind.OBJECT]".

To sum up, to me, it looks like only NEVER mode is actually meaningful for all formats universally. So, probably, you can get away with adding writeClassDiscriminator: Boolean flag for your format.

However, renaming ClassDiscriminatorMode to Json(Class)DiscriminatorMode indeed makes sense if you want to implement a similar enum for your format. We definitely should consider this.

@pdvrieze
Copy link
Contributor

As an example, XML has a completely different way of dealing with polymorphism. It supports the [type, object] mode, it supports using the type derived tag name as "discriminator" (this implies that this type can only map to a single child - at least until I add support for order-sensitive decoding), and finally it allows using an attribute to indicate the type (by default xsi:type). It is significant here that the xml format, by default, ignores the property name for child tags (and uses type names instead).
As to renaming, it makes some sense, but may also be a compatibility issue.

@BenWoodworth
Copy link
Contributor Author

Thanks for the insight! It does sound like renaming would be the way to go, if changing anything at all.

In my mind I was thinking that a discriminator (and its mode) could work for any format, basically encoding the Kotlin classes as if there was an extra property added in the first place. Having the classes translate all the same. That, and ALL_CLASSES being a better name for the mode (or just ALL, since classes might be implied). Then for serialization configured without discriminators (array polymorphism, no support at all, ...) could disregard the ClassDiscriminatorMode enum entirely.

Even if that's all true though, and any discriminator-supporting format could implement these options, there might be additional modes that a format might want to add. And @pdvrieze, similar to what you mentioned, encoding the discriminator as an attribute instead of an entry (or in other formats, perhaps as metadata instead of an additional field). Thanks for bringing up the XML example :)

My format is very similar to JSON in structure, so for me, I'll probably include my own NbtClassDiscriminatorMode to loosely mirror JSON's. And if we're open to renaming it to JsonClassDiscriminatorMode, I could also play around with doing so in a binary compatible and auto-migratable way, and open up a PR if I'm getting anywhere with it. Let me know!

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

No branches or pull requests

3 participants