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

Change MapperFeature.SORT_PROPERTIES_ALPHABETICALLY default to true (3.x) #4572

Conversation

JooHyukKim
Copy link
Member

As described in JSTEP-2,

(`false` really doesn't make much sense since it is unstable, and arbitrary based on JVM/JDK)

false doesn't make sense.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 9, 2024

My one possible concern here, that I hadn't thought of before is handling of Records -- I think the default for Records should remain declaration order, unless explicitly reordered.
Not sure if this changes that aspect or not; and if so, would we need more settings, I don't know.

What may be relevant is that with Records "Creator-properties first" rule might keep ordering as expected (unless MapperFeature.SORT_CREATOR_PROPERTIES_FIRST disabled)?\

EDIT: looking at SORT_CREATOR_PROPERTIES_FIRST Javadocs it does suggest precedence over alphabetic-sorting. So that should be fine.

@cowtowncoder cowtowncoder changed the title [Jackson 3] Change default SORT_PROPERTIES_ALPHABETICALLY to true Change MapperFeature.SORT_PROPERTIES_ALPHABETICALLY default to true (3.x) Jun 14, 2024
@cowtowncoder cowtowncoder added the 3.x Issues to be only tackled for Jackson 3.x, not 2.x label Jun 14, 2024
@@ -150,7 +150,7 @@ public BeanForStrictOrdering(@JsonProperty("c") int c, @JsonProperty("a") int a)

@Test
public void testImplicitOrderByCreator() throws Exception {
assertEquals("{\"c\":1,\"a\":2,\"b\":0}",
assertEquals("{\"a\":2,\"c\":1,\"b\":0}",
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this seems wrong: since Constructor is:

        @JsonCreator public BeanWithCreator(@JsonProperty("c") int c, @JsonProperty("a") int a)

order should remain "c", "a" (creator parameters), "b", given that MapperFeature.SORT_CREATOR_PROPERTIES_FIRST is left enabled (default state).

So something is wrong... not sure if same is true for 2.x.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhhhh. No, actually, I am wrong. Code in POJOPropertiesCollector says:

protected void _sortProperties(Map<String, POJOPropertyBuilder> props) {
....
        // Third by sorting Creator properties before other unordered properties
        // (unless strict ordering is requested)
        if ((_creatorProperties != null)
                && (!sortAlpha || _config.isEnabled(MapperFeature.SORT_CREATOR_PROPERTIES_FIRST))) {
            /* As per [databind#311], this is bit delicate; but if alphabetic ordering
             * is mandated, at least ensure creator properties are in alphabetic
             * order. Related question of creator vs non-creator is punted for now,
             * so creator properties still fully predate non-creator ones.
             */

that is, ALL Creator properties should be sorted before all non-Creator properties (unless explicit order dictates otherwise).

So in that sense this works as expected.

Copy link
Member

Choose a reason for hiding this comment

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

However: this brings up another question, mentioned elsewhere: should ordering of Record properties be ordered by declaration order because it:

  1. Is actually well-defined and stable
  2. May well be what users expect, as the default choice

I think I better file a separate issue for that; not strictly related to this PR, but something worth considering.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't had time to look at the sorting functionality wrt creator-properties yet, but my first intuition was wondering...

how is `record` related to creators?

But it seems like record constructors , by its nature, is considered creators or similar. I think I do need to (and will) learn more about records.

Thank you for the explanation tho @cowtowncoder !

Copy link
Member

Choose a reason for hiding this comment

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

So, Records are handled quite similar to POJOs with 2.18, in that their Constructors are selected either from explicit annotation, or, if none, by selection so-called "Canonical" constructor (one that has all record fields in order).

But ordering of properties is then done separately across set of properties, not considering Creator (constructor or factory method).
(and of course actual Creator is not used for serialization anyway, nor are CreatorProperty instances -- but POJOPropertiesCollector has information on whether logical property has a creator-parameter binding).

I think sorting is handled by POJOPropertiesCollector and could keep creator-bound properties in order they were added (which is declaration order).

I hope 2.18 code, esp. in POJOPropertiesCollector, is easier to follow through than before refactoring. There is still some later processing in BasicDeserializerFactory, but more of logic is now in PPC.

@cowtowncoder cowtowncoder added this to the 3.0.0 milestone Jun 15, 2024
@cowtowncoder cowtowncoder merged commit 4b6c307 into FasterXML:master Jun 15, 2024
8 checks passed
@JooHyukKim JooHyukKim deleted the Change-defaults-for-sort-properties-alphabetically branch June 16, 2024 02:07
@pjfanning
Copy link
Member

pjfanning commented Jun 18, 2024

I would argue strongly that this is not going to be popular. In the Scala module, I have no problem getting the fields/properties in a determined order. Sorting alphabetically will be very strange for any Scala user - they will use lots of alternate libs - none of which will mess with the property order. The property order will be based on the defined order of the fields in the Scala class.

@cowtowncoder
Copy link
Member

Let's instead take this to #4580: behavior itself (wrt sorting) has already existed for the long time and this particular issue is only about default setting -- using consistent ordering, instead of (ignoring Case classes etc. other Constructor-bound properties for a moment) possibly arbitrary order in which Methods and Fields are exposed by Reflection.

But the question of specific status of Creator properties is what determines specific behavior that matters here, wrt Case classes (and Records, Kotlin/Lombok Data classes).

@cowtowncoder
Copy link
Member

I am also open to other ways of dealing with the problem of Java classes exposing Fields, Methods, in arbitrary order (esp. considering sub-classing). F.ex allowing modules to indicate different default ordering for types.

Also note this is just for 3.0, no plans to do anything similar with 2.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues to be only tackled for Jackson 3.x, not 2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants