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

Add KeyableFormat for Map json serialization (#125). #331

Open
wants to merge 2 commits into
base: release/1.3.x
Choose a base branch
from

Conversation

janjaali
Copy link

This PR aims both to resolve an unclear behavior of the mapFormat method and improve the extensibility of this method. The mapFormat method provides a RootJsonFormat to serialize scala.Predef#Maps as JsObjects and vice versa.

Uncertain behavior

The signature of the mapFormat method incorrectly implies that all types of keys are applicable to serialize a Map as a JsObject as long as these keys are serializable as JsValue. Thus trying to serialize a Map with a key which is not serialized as JsString results in a SerializationException.

This PR will modify the method signature such that an implicit KeyableFormat instead of a JsonFormat is required. A KeyableFormat implies that a key must be serializable as a String - the only valid key format in JSON - and it can be deserialized back from a String. This change will improve the type-safety of the method and prevent surprising SerializationExceptions while serializing a Map without a valid key format.

Extensibility

The newly introduced KeyableFormat trait is extendable and enables user to provide their own format to serialize their keys. A simple and probably most generous KeyableFormat for String is provided in the KeyableFormats object.

Is this is a breaking change?

I would argue partially: The signature of an implicit method was changed by replacing the implicit JsonFormat argument for the key to an implicit KeyableFormat and the DefaultJsonProtocol provides the default StringKeyableFormat. Thus most users may not be affected by this change. Nevertheless it is a change in a public API and should be considered as breaking change.

@janjaali
Copy link
Author

janjaali commented Jun 4, 2020

Anybody some spare time to have a look at this one?

@jrudolph
Copy link
Member

Thanks for the suggestion. I agree it would be better if we could improve type-safety for maps but at this point we cannot make an incompatible change like this. If we want to do it, it needs to be in a way that allows us to keep the existing signature (and maybe deprecate that one or remove the implicit status).

@janjaali
Copy link
Author

janjaali commented Mar 8, 2021

So, if I get you right, you are suggesting to drop the implicit status from the existing signature (did so in the PR) and remain with the new signature?

As mentioned in the description, the change of the "implicit signature" probably won't affect users since the only currently supported JsonFormat for keys is JsonString, otherwise the serialization just fails. This implicit constraint is caught by the implicit StringKeyableFormat that was added to the DefaultJsonProtocol.

Maybe it's not a PR for this release train but for the next :)

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

Successfully merging this pull request may close these issues.

2 participants