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

MapperFeature.SORT_PROPERTIES_ALPHABETICALLY is not respected for records #3900

Closed
agavrilov76 opened this issue Apr 27, 2023 · 10 comments
Closed
Labels
will-not-fix Closed as either non-issue or something not planned to be worked on

Comments

@agavrilov76
Copy link
Contributor

MapperFeature.SORT_PROPERTIES_ALPHABETICALLY is no longer respected by record method based JSON properties

Version information
2.15

To Reproduce

    final var mapper =
        Jackson.mapperBuilder()
            .enable(SerializationFeature.INDENT_OUTPUT)
            .enable(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY)
            .build();

    record R(String c, String b, String e) {

      @JsonProperty
      String a() {
        return "4";
      }
    }

    System.out.println(mapper.writeValueAsString(new R("1", "2", "3")));

Output:

  {
    "b" : "2",
    "c" : "1",
    "e" : "3",
    "a" : "4"
  }

The property 'a' in the example below comes at the last place:

Expected behavior
All record properties must be serialized the natural order

@yihtserns
Copy link
Contributor

yihtserns commented Apr 27, 2023

Fix/workaround

Disable MapperFeature.SORT_CREATOR_PROPERTIES_FIRST:

Jackson.mapperBuilder()
    .enable(SerializationFeature.INDENT_OUTPUT)
    .enable(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY)
    .disable(MapperFeature.SORT_CREATOR_PROPERTIES_FIRST)
    .build();

Explanation

Previously, Records deserialization is specially handled, resulting in some behavioural differences (mostly missing behaviours) compared to POJO deserialization.

#2992 #3724 was done to address those differences, to close the gap between Records vs POJO de/serialization. Because of that, this Record class:

record R(String c, String b, String e) {

  @JsonProperty
  String a() {
    return "4";
  }
}

...behaves similarly to:

public class R {
    private String c;
    private String b;
    private String e;
    
    @ConstructorProperties({"c", "b", "e"})
    public R(String c, String b, String e) {
        this.c = c;
        this.b = b;
        this.e = e;
    }

    public String getC() { 
        return this.c;
    }
    
    public String getB() {
        return this.b;
    }
    
    public String getE() {
        return this.e;
    }
    
    @JsonProperty
    public String a() {
        return "4";
    }
}

...resulting in this issue.

I'm really sorry about the inconvenience this caused you. Reminds me of an adage that one does not simply fix a bug - some "bugs" may be relied on as features. 😬

@JooHyukKim
Copy link
Member

oh no, @yihtserns 👍🏻 It's already good you feel responsible. I don't think you should be sorry because, the change thoroughly discussed (I really did read most part). I could have written more tests against record types.

@cowtowncoder
Copy link
Member

As unfortunate as unplanned behavior changes are, I will consider this "works as intended", so that one needs to disable:

MapperFeature.SORT_CREATOR_PROPERTIES_FIRST

to force sorting of all properties.

@cowtowncoder cowtowncoder added will-not-fix Closed as either non-issue or something not planned to be worked on 2.15 and removed to-evaluate Issue that has been received but not yet evaluated labels May 3, 2023
@agavrilov76
Copy link
Contributor Author

@cowtowncoder Can you consider disabling SORT_CREATOR_PROPERTIES_FIRST by default by any chance?

I think the fact that enabling one single SORT_PROPERTIES_ALPHABETICALLY feature doesn't automatically work for all the properties is not quite expected behavior. Not sure how many people would use SORT_PROPERTIES_ALPHABETICALLY with SORT_CREATOR_PROPERTIES_FIRST set as the default.

Just for the context, my use case is testing. I compare generated JSON documents with data files stored in a canonical form sorting properties and map entries.

@JooHyukKim
Copy link
Member

JooHyukKim commented May 5, 2023

I think the fact that enabling one single SORT_PROPERTIES_ALPHABETICALLY feature doesn't automatically work for all the properties is not quite expected behavior.

I cannot disagree... 👀

In fact, from a user's perspective,

  • SORT_PROPERTIES_ALPHABETICALLY intuitively sounds like superset of all(if not most) property sorting
  • SORT_CREATOR_PROPERTIES_FIRST sounds like partial sorting

Maybe you can file an issue regarding suggestion, @agavrilov76 ?

@yawkat
Copy link
Member

yawkat commented May 5, 2023

the problem with disabling SORT_CREATOR_PROPERTIES_FIRST is that putting creator properties first is more performant for deserialization. since all creator properties are required to create the instance, all properties before the last creator property in the input need to be buffered. SORT_CREATOR_PROPERTIES_FIRST ensures that there is as little buffering as possible.

@JooHyukKim
Copy link
Member

JooHyukKim commented May 5, 2023

the problem with disabling SORT_CREATOR_PROPERTIES_FIRST is that putting creator properties first is more performant for deserialization.

This is quite informant... putting a side the changing of default configuration, let's add this to the JavaDoc itself.
Thank you for your explanation! 🙏🏼🙏🏼

Explanation of @yawkat added to PR (see)

@cowtowncoder
Copy link
Member

As per @yawkat's pointing it out, there are non-trivial performance implications.
Changing default setting would also change observed sorting order of existing users and could break some usage (ideally shoudn't as ordering of properties is conceptually undefined for JSON but... in practice...).
So I don't think I'd want to change defaults for 2.x.
This could be changed for 3.0 however; if so, please file a new issue.

@yawkat
Copy link
Member

yawkat commented May 16, 2023

I just saw that if neither SORT_CREATOR_PROPERTIES_FIRST or SORT_PROPERTIES_ALPHABETICALLY is set, jackson will still put creator properties first. So it may be safe to disable SORT_CREATOR_PROPERTIES_FIRST by default since SORT_PROPERTIES_ALPHABETICALLY is also off by default, so with default settings the output would be unchanged.

@cowtowncoder
Copy link
Member

Ok but what is the specific upside of doing that? Would it make enabling SORT_PROPERTIES_ALPHABETICALLY work more logically wrt user expectations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
will-not-fix Closed as either non-issue or something not planned to be worked on
Projects
None yet
Development

No branches or pull requests

5 participants