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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jun 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f879990
Change default to true
JooHyukKim Jun 9, 2024
8995e93
Fix failing record tests
JooHyukKim Jun 9, 2024
354820b
Merge branch 'master' into Change-defaults-for-sort-properties-alphab…
cowtowncoder Jun 14, 2024
c0e7cea
Add release note, javadoc default comment
cowtowncoder Jun 14, 2024
bd055ce
Merge branch 'master' into Change-defaults-for-sort-properties-alphab…
cowtowncoder Jun 14, 2024
b9044bd
Merge branch 'master' into Change-defaults-for-sort-properties-alphab…
cowtowncoder Jun 14, 2024
2d3f4a8
Merge branch 'master' into Change-defaults-for-sort-properties-alphab…
cowtowncoder Jun 15, 2024
c66d2fa
Merge branch 'Change-defaults-for-sort-properties-alphabetically' of …
cowtowncoder Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/main/java/tools/jackson/databind/MapperFeature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,11 @@ public enum MapperFeature
*<p>
* Note: does <b>not</b> apply to {@link java.util.Map} serialization (since
* entries are not considered Bean/POJO properties.
*
*<p>
* Feature is disabled by default.
* Feature is enabled by default.
cowtowncoder marked this conversation as resolved.
Show resolved Hide resolved
*/
SORT_PROPERTIES_ALPHABETICALLY(false),
SORT_PROPERTIES_ALPHABETICALLY(true),

/**
* Feature that defines whether Creator properties (ones passed through
Expand Down
2 changes: 1 addition & 1 deletion src/test-jdk17/java/tools/jackson/databind/records/RecordCreatorSerialization4452Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public CreatorTestObject(
public void testPlain() throws Exception
{
String result = OBJECT_MAPPER.writeValueAsString(new PlainTestObject("test", 1));
assertEquals("{\"strField\":\"test\",\"intField\":1}", result);
assertEquals("{\"intField\":1,\"strField\":\"test\"}", result);
}

@Test
Expand Down
2 changes: 1 addition & 1 deletion src/test-jdk17/java/tools/jackson/databind/records/RecordExplicitCreatorsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ public void testDeserializeUsingCanonicalConstructor_WhenJsonPropertyConstructor
} catch (DatabindException e) {
verifyException(e, "Unrecognized property \"id\"");
verifyException(e, "RecordWithTwoJsonPropertyWithoutJsonCreator");
verifyException(e, "2 known properties: \"the_id\", \"the_email\"");
verifyException(e, "2 known properties: \"the_email\", \"the_id\"");
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/test-jdk17/java/tools/jackson/databind/records/RecordSerializationOrderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public void testSerializationOrder() throws Exception {
NestedRecordTwo nestedRecordTwo = new NestedRecordTwo("2", "111110");
NestedRecordOne nestedRecordOne = new NestedRecordOne("1", "[email protected]", nestedRecordTwo);
final String output = MAPPER.writeValueAsString(nestedRecordOne);
final String expected = "{\"id\":\"1\",\"email\":\"[email protected]\",\"nestedRecordTwo\":{\"id\":\"2\",\"passport\":\"111110\"}}";
final String expected = "{\"email\":\"[email protected]\",\"id\":\"1\",\"nestedRecordTwo\":{\"id\":\"2\",\"passport\":\"111110\"}}";
assertEquals(expected, output);
}

Expand All @@ -49,7 +49,7 @@ public void testSerializationOrderWithJsonProperty() throws Exception {
NestedRecordOneWithJsonProperty nestedRecordOne =
new NestedRecordOneWithJsonProperty("1", "[email protected]", nestedRecordTwo);
final String output = MAPPER.writeValueAsString(nestedRecordOne);
final String expected = "{\"id\":\"1\",\"email\":\"[email protected]\",\"nestedProperty\":{\"id\":\"2\",\"passport\":\"111110\"}}";
final String expected = "{\"email\":\"[email protected]\",\"id\":\"1\",\"nestedProperty\":{\"id\":\"2\",\"passport\":\"111110\"}}";
assertEquals(expected, output);
}

Expand Down
2 changes: 1 addition & 1 deletion src/test-jdk17/java/tools/jackson/databind/records/RecordWithView4085Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public record Record4085(int total, @JsonView(View4085Field.class) int current)
public void testRecordWithView4085() throws Exception
{
final Record4085 input = new Record4085(1, 2);
final String EXP = a2q("{'total':1,'current':2}");
final String EXP = a2q("{'current':2,'total':1}");
final ObjectWriter w = newJsonMapper().writer();

// by default, all properties included, without view
Expand Down
10 changes: 5 additions & 5 deletions src/test/java/tools/jackson/databind/ObjectMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,15 @@ public void testConfigForPropertySorting() throws Exception
{
ObjectMapper m = newJsonMapper();

// sort-alphabetically is disabled by default:
assertFalse(m.isEnabled(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY));
// sort-alphabetically is enabled by default:
assertTrue(m.isEnabled(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY));
assertTrue(m.isEnabled(MapperFeature.SORT_CREATOR_PROPERTIES_FIRST));
SerializationConfig sc = m.serializationConfig();
assertFalse(sc.isEnabled(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY));
assertFalse(sc.shouldSortPropertiesAlphabetically());
assertTrue(sc.isEnabled(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY));
assertTrue(sc.shouldSortPropertiesAlphabetically());
assertTrue(sc.isEnabled(MapperFeature.SORT_CREATOR_PROPERTIES_FIRST));
DeserializationConfig dc = m.deserializationConfig();
assertFalse(dc.shouldSortPropertiesAlphabetically());
assertTrue(dc.shouldSortPropertiesAlphabetically());
assertTrue(dc.isEnabled(MapperFeature.SORT_CREATOR_PROPERTIES_FIRST));

// but when enabled, should be visible:
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/tools/jackson/databind/jsontype/ExistingPropertyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,16 +267,16 @@ static class DefaultShape3271 extends Shape3271 {}
private static final String fruitListJson = "[" + pinguoJson + "," + mandarinJson + "]";

private static final Cat beelzebub = new Cat("Beelzebub", "tabby");
private static final String beelzebubJson = "{\"name\":\"Beelzebub\",\"furColor\":\"tabby\",\"type\":\"kitty\"}";
private static final String beelzebubJson = "{\"furColor\":\"tabby\",\"name\":\"Beelzebub\",\"type\":\"kitty\"}";
private static final Dog rover = new Dog("Rover", 42);
private static final String roverJson = "{\"name\":\"Rover\",\"boneCount\":42,\"type\":\"doggie\"}";
private static final String roverJson = "{\"boneCount\":42,\"name\":\"Rover\",\"type\":\"doggie\"}";
private static final AnimalWrapper beelzebubWrapper = new AnimalWrapper(beelzebub);
private static final String beelzebubWrapperJson = "{\"animal\":" + beelzebubJson + "}";
private static final List<Animal> animalList = Arrays.asList(beelzebub, rover);
private static final String animalListJson = "[" + beelzebubJson + "," + roverJson + "]";

private static final Camry camry = new Camry("Sweet Ride", "candy-apple-red");
private static final String camryJson = "{\"name\":\"Sweet Ride\",\"exteriorColor\":\"candy-apple-red\",\"type\":\"camry\"}";
private static final String camryJson = "{\"exteriorColor\":\"candy-apple-red\",\"name\":\"Sweet Ride\",\"type\":\"camry\"}";
private static final Accord accord = new Accord("Road Rage", 6);
private static final String accordJson = "{\"name\":\"Road Rage\",\"speakerCount\":6,\"type\":\"accord\"}";
private static final CarWrapper camryWrapper = new CarWrapper(camry);
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/tools/jackson/databind/jsontype/TestPolymorphicDeduction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ static enum Enum3711 { A, B }
/**********************************************************
*/

private static final String deadCatJson = a2q("{'name':'Felix','causeOfDeath':'entropy'}");
private static final String liveCatJson = a2q("{'name':'Felix','angry':true}");
private static final String deadCatJson = a2q("{'causeOfDeath':'entropy','name':'Felix'}");
private static final String liveCatJson = a2q("{'angry':true,'name':'Felix'}");
private static final String luckyCatJson = a2q("{'name':'Felix','angry':true,'lives':8}");
private static final String ambiguousCatJson = a2q("{'name':'Felix','age':2}");
private static final String fleabagJson = a2q("{}");
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/tools/jackson/databind/jsontype/TestTypeNames.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public void testSerialization() throws Exception
// static type on serialization. If we had root static types,
// could use those; but at the moment root type is dynamic

assertEquals("[{\"doggy\":{\"name\":\"Spot\",\"ageInYears\":3}}]",
assertEquals("[{\"doggy\":{\"ageInYears\":3,\"name\":\"Spot\"}}]",
MAPPER.writeValueAsString(new Animal[] { new Dog("Spot", 3) }));
assertEquals("[{\"MaineCoon\":{\"name\":\"Belzebub\",\"purrs\":true}}]",
MAPPER.writeValueAsString(new Animal[] { new MaineCoon("Belzebub", true)}));
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/tools/jackson/databind/ser/SerializationOrderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

MAPPER.writeValueAsString(new BeanWithCreator(1, 2)));
}

Expand Down