Kotlin / kotlinx.serialization

Kotlin multiplatform / multi-format serialization
Apache License 2.0
5.33k stars 618 forks source link

Should `Json.configuration` really be internal? #1323

Closed Whathecode closed 3 years ago

Whathecode commented 3 years ago

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

I know my use case for wanting to access Json.configuration is highly specific, and I will detail it next. But, even not considering my specific use case, from a design perspective, why is configuration internal?

It is an immutable parameter, which is fully initialized by the caller. The caller is fully aware of this configuration, there is logic coupling. Why not simply allow the caller to see what exact configuration was specified? To me, the JsonConf abstraction lives on the same level as Json. I do not see what is gained by encapsulating access to it.

Describe the solution you'd like

I want to access Json.configuration, i.e., for it to be public.

My use case in more detail

I have a custom serializer implementation: UnknownPolymophicSerializer.

A serializer for polymorph objects of type [P] which wraps extending types unknown at runtime as instances of type [W].

It currently is hardcoded to only work for JSON serialization using a class descriptor configuration, given the way it achieves this functionality (JSON parsing and substring manipulations).

When deserializing a polymorphic type, I need to extract the class name from the class descriptor field so that I can verify whether or not a polymorphic serializer is registered for that specific class name. I thus need to know the configured class descriptor from JsonConf.

I could access it easy enough if it were public, and I can access it easy enough using full kotlin.reflect. However, I do not want to pull in a full reflection library, and all of a sudden lose support for this feature on JS platforms which do not support full kotlin.reflect simply for one line of code.

I've had this custom serializer for quite some time, and its functionality seems to overlap with a feature which has since been introduced in kotlinx.serialization: Default polymorphic type handler for deserialization. I have already looked at this shortly, but at a glance it seems my implementation does a bit more; I remove the wrapper upon serialization. I still need to look into more detail whether I could use it to my advantage to get rid off, or simplify my UnknownPolymorphicSerializer.

Regardless, that is the current use case. :) A one-liner could have fixed a current bug report I received if it were public: https://github.com/cph-cachet/carp.core-kotlin/issues/226

Whathecode commented 3 years ago

I found this workaround for now, further showing that these fields are public really ...

val jsonToGetConfigurationFor: Json = decoder.json
var extractedClassDiscriminator: String?
Json( jsonToGetConfigurationFor ) { extractedClassDiscriminator = classDiscriminator }
sandwwraith commented 3 years ago

We've hid it because JsonConfiguration is pretty meaningless without Json itself: configuring and copying Json objects can be done easier without creating configuration instance. Moreover, since we can introduce new flags in configuration, exposing config as a public type means that we should provide all possible constructors for stable public ABI — this also increases maintenance cost.

For 'specific' cases, approach in your workaround is perfectly legit — there's nothing wrong in reading those values that way

Whathecode commented 3 years ago

We've hid it because JsonConfiguration is pretty meaningless without Json itself

Sure. I don't see how that contradicts what I said, though. 🤔 I want to access configuration through Json.

exposing config as a public type means that we should provide all possible constructors for stable public ABI

I'm not asking for the constructor to be public. I simply need read access to the configuration of Json in my serializer.

When you introduce new config options, or rename them, or change their behavior, they still need to be exposed through JsonBuilder. My point is this API is already public. The builder pattern doesn't encapsulate that. The builder pattern here mostly seems useful to have a flexible, easy to extend/maintain initializer while keeping JsonConfig immutable. But, I don't see how it hides JsonConfig or its internals in any way.

your workaround is perfectly legit

I need to do a full clone of Json on every decode call. Not that it's a bottleneck in any way (premature optimization and all that ...), but from a conceptual point of view, this simply does not make sense. If the configuration options are public through a builder, why not just make them public (readable) directly?

Whathecode commented 3 years ago

Similar request here: https://github.com/Kotlin/kotlinx.serialization/issues/832

The configuration is inherently public, hence these requests. You can hide it, but it doesn't decouple it.

Whathecode commented 3 years ago

Solved by https://github.com/Kotlin/kotlinx.serialization/pull/1409