-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Change MapperFeature.SORT_PROPERTIES_ALPHABETICALLY
default to true (3.x)
#4572
Conversation
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. What may be relevant is that with Records "Creator-properties first" rule might keep ordering as expected (unless EDIT: looking at |
SORT_PROPERTIES_ALPHABETICALLY
to trueMapperFeature.SORT_PROPERTIES_ALPHABETICALLY
default to true (3.x)
@@ -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}", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Is actually well-defined and stable
- 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.
There was a problem hiding this comment.
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 !
There was a problem hiding this comment.
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.
…github.com:JooHyukKim/jackson-databind into Change-defaults-for-sort-properties-alphabetically
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. |
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). |
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. |
As described in JSTEP-2,
false doesn't make sense.