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.

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

Merged
merged 17 commits into from
Jul 10, 2024

Conversation

cowtowncoder
Copy link
Member

@cowtowncoder cowtowncoder commented Jul 6, 2024

Implements #4584.

@cowtowncoder cowtowncoder marked this pull request as ready for review July 10, 2024 02:56
@cowtowncoder
Copy link
Member Author

cowtowncoder commented Jul 10, 2024

Implementation by other modules would be along lines of (see PrimaryCreatorDetection4584Test for bigger examples):

public class PrimaryDetectingIntrospector extends AnnotationIntrospector
{
        @Override
        public PotentialCreator findDefaultCreator(MapperConfig<?> config,
                AnnotatedClass valueClass,
                List<PotentialCreator> declaredConstructors,
                List<PotentialCreator> declaredFactories)
        {
           if (valueClass.getRawType() == MyClass.class) {
              // Only detect constructors (for example)
              for (PotentialCreator ctor : declaredConstructors) {
                  // use whatever logic to see if this should match (annotations, types etc)
                if (ctor.paramCount() == 1) {
                    // important: indicate mode (DELEGATING or PROPERTIES)
                   // (strictly speaking only required for 1-param case but good practice):
                   ctor.overrideMode(JsonCreator.Mode.PROPERTIES);
                   return ctor;
                }
              }
           }
           return null;
        }
}

@cowtowncoder
Copy link
Member Author

@JooHyukKim @k163377 Here's complete implementation for review; will merge soon.

@cowtowncoder cowtowncoder changed the title Fixes #4584: add AnnotationIntrospector method for canonical Creator discovery Fixes #4584: add AnnotationIntrospector method for primary Creator discovery Jul 10, 2024
@JooHyukKim
Copy link
Member

@JooHyukKim @k163377 Here's complete implementation for review; will merge soon.

I will try to review today 👍🏼

@cowtowncoder
Copy link
Member Author

@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 master tonight.

@cowtowncoder cowtowncoder merged commit 6e68d81 into 2.18 Jul 10, 2024
9 checks passed
@cowtowncoder cowtowncoder deleted the tatu/2.18/4584-canonical-creator-ext-point branch July 10, 2024 04:05
@JooHyukKim
Copy link
Member

@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 master tonight.

Makes sense, I've been watching this PR for a couple of days and LGTM so far.
I will probably leave a few comments around JavaDoc.

Comment on lines +1400 to +1402
/**
* 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
Copy link
Member

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.

Copy link
Member Author

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,
Copy link
Contributor

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?

Copy link
Member Author

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)

Copy link
Contributor

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.

Copy link
Member Author

@cowtowncoder cowtowncoder Jul 11, 2024

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 !

cowtowncoder added a commit that referenced this pull request Jul 11, 2024
@cowtowncoder cowtowncoder changed the title Fixes #4584: add AnnotationIntrospector method for primary Creator discovery Fixes #4584: add AnnotationIntrospector method for default Creator discovery (findDefaultCreator()) Jul 11, 2024
@cowtowncoder
Copy link
Member Author

Ok, tweaked a bit: lmk for further suggestions.

@k163377
Copy link
Contributor

k163377 commented Jul 13, 2024

@cowtowncoder
Deployment failing?
I tried to reflect it in kotlin-module, but the name was still findPrimary....
https://github.com/FasterXML/jackson-databind/actions/runs/9884061392

@JooHyukKim
Copy link
Member

Could be just temporary error. It might simply succeed by re-running.
@cowtowncoder Could you try re-running above build?

@cowtowncoder
Copy link
Member Author

@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.

@cowtowncoder
Copy link
Member Author

@k163377 Should work better now

@k163377
Copy link
Contributor

k163377 commented Jul 15, 2024

I submitted a problem I found when implementing to kotlin-module.
#4620

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants