-
-
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
Fixes #4584: add AnnotationIntrospector method for default Creator discovery (findDefaultCreator()
)
#4615
Fixes #4584: add AnnotationIntrospector method for default Creator discovery (findDefaultCreator()
)
#4615
Conversation
…s (without annotations)
…om:FasterXML/jackson-databind into tatu/2.18/4584-canonical-creator-ext-point
Implementation by other modules would be along lines of (see
|
@JooHyukKim @k163377 Here's complete implementation for review; will merge soon. |
I will try to review today 👍🏼 |
@JooHyukKim Sounds good - I think I'll merge this and address anything you may find with a follow-up. This so I can merge to |
Makes sense, I've been watching this PR for a couple of days and LGTM so far. |
/** | ||
* Method called to check if introspector is able to detect so-called Primary | ||
* Creator: Creator to select for use when no explicit annotation is found |
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.
/**
* Method called to detect so-called <b>Primary Creator</b> (via {@link #findCreatorAnnotation}).
* Primary Creator is the Creator to select for use when no explicit annotation is found.
* .... // rest omitted
@cowtowncoder I was thinking maybe the first part of the JavaDoc could be this. But other than that, seems good.
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.
Hmmm. Reference to findCreatorAnnotation()
seems misplaced since potential creators includes all constructors, most applicable static factory methods, not just ones annotated.
(in fact, the main point is to allow selecting non-annotated creators).
@@ -735,9 +735,23 @@ public JsonCreator.Mode findCreatorAnnotation(MapperConfig<?> config, Annotated | |||
return (mode == null) ? _secondary.findCreatorAnnotation(config, a) : mode; | |||
} | |||
|
|||
@Override | |||
public PotentialCreator findPrimaryCreator(MapperConfig<?> config, |
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 familiar with English, but is the name of this method Primary
OK?
Personally, it seemed like a function to get a creator with a higher priority than the setting by annotation such as JsonCreator
.
How about a name like findImplicitCreator
?
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.
It's up to debate, I'm sure -- at first I was considering canonical.
But I see the challenge wrt precedence.
Instead of implicit (which is used as sort of fallback already, which is why I hesitate with it), we could consider "default"?
findDefaultCreator()
? (and similar naming)
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.
findDefaultCreator
I can think of findFallbackConstructor
, but findDefaultCreator
would also be good.
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 I'll go with "default" since that to me conveys it better. But as long as descriptions define logic either would work.
(note: cannot use "constructor" since method can return constructor OR factory method (hence, creator to cover both types))
Thank you @k163377 !
findDefaultCreator()
)
Ok, tweaked a bit: lmk for further suggestions. |
@cowtowncoder |
Could be just temporary error. It might simply succeed by re-running. |
@k163377 @JooHyukKim There is something odd about CI, seems to fail the first snapshot deployment for merged PRs. No idea why. Same does not occur for merges from earlier branches I think (and PRs will not try to deploy so won't fail there). Something to do with access, I suppose, but these are branches I create and merge and my credentials should be sufficient. I re-ran it, and as usual that succeeds. Need to keep an eye on this going forward but not sure how to resolve it. |
@k163377 Should work better now |
I submitted a problem I found when implementing to |
Implements #4584.