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

feat: explicitEmptyCollections V2, for more json (or less) #1239

Merged

Conversation

ThijsBroersen
Copy link
Contributor

@ThijsBroersen ThijsBroersen commented Jan 23, 2025

A retry of the explicitEmptyCollections feature. The defaults should be exactly as the current release (decoding missing collections will fail).

To tap in on the feature use annotation @jsonExplicitEmptyCollections(false) on fields or case classes or use the JsonCodecConfiguration.explicitEmptyCollections = false config.
After this, all collections and case classes which have empty values will be omitted from the json output and decoding will fill in the missing values with empty collections.

To implement this while keeping performance I introduced three new decoder traits (scope private[json]), to allow to build an efficient decoder. (CollectionJsonDecoder, OptionJsonDecoder, MappedJsonDecoder).

@ThijsBroersen ThijsBroersen marked this pull request as ready for review January 23, 2025 21:55
@ThijsBroersen ThijsBroersen requested a review from a team as a code owner January 23, 2025 21:55
@ThijsBroersen ThijsBroersen changed the title feat: explicitEmptyCollections for more json feat: explicitEmptyCollections V2, for more json Jan 23, 2025
@ThijsBroersen ThijsBroersen changed the title feat: explicitEmptyCollections V2, for more json feat: explicitEmptyCollections V2, for more json (or less) Jan 23, 2025
@ThijsBroersen ThijsBroersen force-pushed the feat/explicit-empty-collections-v2 branch from f3b2f44 to dff02e9 Compare January 24, 2025 16:31
Copy link
Contributor

@987Nabil 987Nabil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThijsBroersen I wrote this before in one of the related issues, but I guess you missed it.
Please make it possible to configure the de-/encoding separately. Users might want to explicitly serialize [], but be graceful when receiving messages with the collection field missing.

@ThijsBroersen
Copy link
Contributor Author

ThijsBroersen commented Jan 24, 2025

@ThijsBroersen I wrote this before in one of the related issues, but I guess you missed it. Please make it possible to configure the de-/encoding separately. Users might want to explicitly serialize [], but be graceful when receiving messages with the collection field missing.

@987Nabil if you refer to #1235 (comment) that is not how I read it. The goals was to have to feature 100% opt-in. Unlike explicitNulls, the explicitEmptyCollections will apply strict encoding and decoding when the feature is used. So by default everything is as before.

If you say you want to have it configurable separately, how would that config look like? That would need a redesign, adjustment of logic, adjustment of tests.
Would case class jsonExplicitEmptyCollections(enabled: Boolean = true) transform into case class jsonExplicitEmptyCollections(whenDecoding: Boolean = true, whenEncoding: Boolean = true) or another annotation?
What about the JsonCodecConfiguration, use two field? explicitEmptyCollectionsForDecoding: Boolean = true, explicitEmptyCollectionsForEncoding: Boolean = true or explicitEmptyCollections: ExplicitEmptyCollectionsConfig?

@987Nabil
Copy link
Contributor

@ThijsBroersen I do not care that much about the details of the implementation. It is just that these are two different things and she be handled as such im the config. Try it out, see what fits

@ThijsBroersen
Copy link
Contributor Author

@987Nabil done

@987Nabil
Copy link
Contributor

@ThijsBroersen maybe shorter names for the booleans? They might show up in user land since it is booleans. Maybe just remove the when? Or just encode and decode?

@plokhotnyuk plokhotnyuk merged commit 8cde6d6 into zio:series/2.x Jan 26, 2025
31 checks passed
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.

3 participants