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 JSON Decode Ordering Parity #1411

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hardwil
Copy link

@hardwil hardwil commented Oct 27, 2021

This change is replacing LinkedHashMap with TreeMap in KotlinJsonAdapter to match the ordering in the ClassJsonAdapter, so that both Java and Kotlin instances will have ordering parity.

When migrating code from Java to Kotlin, I found that some tests failed due to entry ordering changes between the respective adapters.

# Conflicts:
#	kotlin/reflect/src/main/java/com/squareup/moshi/kotlin/reflect/KotlinJsonAdapter.kt
@ZacSweers
Copy link
Collaborator

At a glance, it feels like ClassJsonAdapter is the one that should be fixed to preserve insertion order. We also preserve insertion order in the standard map adapter.

Curious for thoughts from @swankjesse and @JakeWharton on this

@swankjesse
Copy link
Member

I don't like that ClassJsonAdapter needs to sort fields either; I think we do that for determinism across JVMs. Retaining field order seems like the nicest user interface.

Also note that fixing this would be very invasive to anyone upgrading.

@swankjesse
Copy link
Member

Original PR that sorted Java classes:
#19

@ZacSweers
Copy link
Collaborator

ZacSweers commented Dec 12, 2021

Do we try to offer an system prop opt-out temporarily to ease migration (kinda like what coroutines does)?

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Let's do this in ClassJsonAdapter as well and add a system property to restore the previous behavior as an escape hatch for people that need it. @hardwil would you be up for doing that? If not I can continue this work too

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.

None yet

3 participants