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

Fix unintended change of explicit creators priority (including primary constructor of record class ) #4731

Closed
wants to merge 3 commits into from
Closed

Fix unintended change of explicit creators priority (including primary constructor of record class ) #4731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfbb2f7
fix 4724
JooHyukKim Oct 3, 2024
46187a2
fix typo
JooHyukKim Oct 3, 2024
758dec3
Merge branch '2.18' into fix-4724
cowtowncoder Oct 9, 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: 4 additions & 1 deletion src/main/java/com/fasterxml/jackson/databind/deser/BasicDeserializerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,10 @@ private boolean _addExplicitDelegatingCreators(DeserializationContext ctxt,
final AnnotationIntrospector intr = ctxt.getAnnotationIntrospector();
boolean added = false;

for (PotentialCreator ctor : potentials) {
// [databind#4724] since 2.18.1 Let's iterate creators in reverse because collection of creators
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I follow this comment... does _addExplicitDelegatingCreator override possible formerly added ones or... ?

Copy link
Member Author

@JooHyukKim JooHyukKim Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

does _addExplicitDelegatingCreator override possible formerly added ones

Yeah, via BasicDeserializer._handleSingleArgumentCreator() -> CreatorCollector.addStringCreator().
Due to both constructor and @JsonCreator-annotated method being single-arg with same type.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmmh. I would have thought a conflict should arise if multiple explicit creators of same type exist. I'll need to think this through; I am not comfortable with change I don't understand. :)

Copy link
Member Author

@JooHyukKim JooHyukKim Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

if multiple explicit creators of same type exist

This may be a problem as well. There seems to be no forcing deduplication.

Or could also be wrong with current issue where record classs primary constructor and creator method are being considered. Ideally we should just know which one to choose.

Copy link
Member

Choose a reason for hiding this comment

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

There is intended logic; but precedence only works within category... so it might not be considering choices as being similar enough to collide.

But the basic rule is that an explicitly annotated Creator does override (and not conflict) default one; both Record case and POJOs. Default one does similarly have precedence over implicit ones; implicit ones do not conflict (if multiple exist, none chosen).
But explicitly annotated constructors should conflict -- except there's Properties- vs Delegating which are not conflicting.

I'll need to check out this example again to see if I agree with intended resolution.

Copy link
Member Author

@JooHyukKim JooHyukKim Oct 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The basic rule is that an explicitly annotated Creator overrides the default one, for both Records and POJOs.

After reviewing your explanation, I think we could benefit from introducing the concept of "default-ness" to creators. Currently, the primary constructor of a record class is treated as just another "explicitDelegatingConstructor." At the point of setting the creator, we can’t use default-ness to select the right one.

By using "default-ness," we could establish a clearer ordering without relying on for-loop direction, as we are now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to mention where to exactly use the "default-ness"? Possibly within CreatorCollector.verifyNonDup() when CreatorCollector.addStringCreator() is called.

Copy link
Member

Choose a reason for hiding this comment

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

Quick note: I think the term should be "default creator" but in some places went with "Primary Creator", like in POJOPropertiesCollector._addCreators(). But I will change that now.

The default constructor of the Record class is absolutely not explicit one since there is no annotation. So if there are true explicit creators, it should be ignored.
Note, too, that POJOs (and Kotlin/Scala equivalents) can introduce Primary creators as well; via `AnnotationIntrospector.

Conversely tho it has higher precedence than implicit ones: so three levels really:

  1. Explicit creators -- annotations (or anything AnnotationIntrospector indicates)
  2. Default Creator
  3. Implicit Creators

Copy link
Member

Choose a reason for hiding this comment

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

So, basically "default-ness" is not carried along BasicDeserializerFactory and it should not need to.
But it is considered during POJOPropertiesCollector._addCreators().

It looks like somehow default constructor is found as-is, but also as explicit one.
This latter part seems odd... I'll see if PotentialCreator might be constructed incorrectly or something.

Copy link
Member

Choose a reason for hiding this comment

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

One other note: "default creator" (or "default constructor") can be confusing wrt JsonCreator.Mode -- DEFAULT for that is completely unrelated (rather that DEFAULT basically means mode is unspecified/undefined).

// via POJOPropertiesCollector is done starting with ones with higher priority.
for (int i = potentials.size(); --i >= 0; ) {
PotentialCreator ctor = potentials.get(i);
added |= _addExplicitDelegatingCreator(ctxt, beanDesc, creators,
CreatorCandidate.construct(intr, ctor.creator(), null));
}
Expand Down
1 change: 0 additions & 1 deletion ...dk17/java/com/fasterxml/jackson/databind/tofix/RecordJsonCreatorAndJsonValue4724Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ public String toString() {
}
}

@JacksonTestFailureExpected
@Test
void deserialization() throws Exception {
newJsonMapper().readValue("\"\"", Something.class);
Expand Down