-
-
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
Fix unintended change of explicit creators priority (including primary constructor of record
class )
#4731
base: 2.18
Are you sure you want to change the base?
Conversation
record
class )record
class )
@@ -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 |
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 am not sure I follow this comment... does _addExplicitDelegatingCreator override possible formerly added ones or... ?
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.
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.
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.
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. :)
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.
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.
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.
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.
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.
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.
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.
Forgot to mention where to exactly use the "default-ness"? Possibly within CreatorCollector.verifyNonDup()
when CreatorCollector.addStringCreator()
is called.
fixes #4724