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

Don't replace nullable absent properties with null #1856

Closed
wants to merge 6 commits into from

Conversation

ZacSweers
Copy link
Collaborator

@ZacSweers ZacSweers commented Jun 19, 2024

This was always supposed to be a feature of moshi but not actually enforced in kotlin reflect or code gen. This fixes that in both with a regression test. Now nullable properties either need to explicitly declare a default value or explicitly have a null present in the JSON.

This uses a couple clever tricks with kotlin contracts to avoid the extra runtime intrinsic checks.

It's worth mentioning that while this was always how this was advertised to work, this is likely going to be a fairly invasive change to people that were implicitly relying on the old behavior.

@ZacSweers ZacSweers marked this pull request as ready for review June 19, 2024 18:38
@ZacSweers ZacSweers changed the title Don't replace nullable absent types with null Don't replace nullable absent properties with null Jun 23, 2024
@swankjesse
Copy link
Member

I don’t think this behavior is consistent with .serializeNulls(false) . . . JSON is generally pretty lenient between absent and null, and I think making Moshi strict is going to be more pain than it’s worth?

@ZacSweers
Copy link
Collaborator Author

Agreed 👍

@ZacSweers ZacSweers closed this Jul 2, 2024
@ZacSweers ZacSweers deleted the z/actualNullAbsence branch July 2, 2024 19:48
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