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

Annotations in mix-ins added to ObjectMapper are not considered by the generator #411

Open
bmaizel opened this issue Nov 17, 2023 · 7 comments
Labels
help wanted Extra attention is needed

Comments

@bmaizel
Copy link

bmaizel commented Nov 17, 2023

I expected, that if I add a mix-in (with ObjectMapper.addMixIn() or ObjectMapper.setMixInResolver()) to the ObjectMapper instance and supply it to the SchemaGeneratorConfigBuilder constructor, it will be considered by the generator. (Yes, I know that this expectation is not supported by the documentation :)

It partially works. Mix-ins are considered during introspection, so, per example, adding @JsonProperty annotation via mix-in can make the annotated field visible to the generator. But the annotation access methods in the FieldScope and MethodScope do not consider mix-ins. So adding @JsonProperty("changedName") to the mix-in will not affect the property name in the generated schema.

I believe, it would be good to support mix-ins in the FieldScope, MethodScope and TypeScope.

For a while I can live with a workaround, which is hacky and shaky, and I would like to remove it:

SchemaGeneratorConfigBuilder configBuilder = new SchemaGeneratorConfigBuilder(
        objectMapper,
        schemaVersion,
        OptionPreset.PLAIN_JSON)
        .with(builder -> {
            builder.forFields().withPropertyNameOverrideResolver(fieldScope -> mixIn(fieldScope));
        })
        .with(jacksonModule)
        .with(javaxValidationModule)
        .with(jakartaValidationModule);
private String mixIn(FieldScope fieldScope) {
    Class<?> mixIn = objectMapper.findMixInClassFor(fieldScope.getDeclaringType().getErasedType());
    if(mixIn != null) {
        try {
            Field declaredField = mixIn.getDeclaredField(fieldScope.getDeclaredName());
            Annotation[] declaredAnnotations = declaredField.getDeclaredAnnotations();
            for(Annotation a : declaredAnnotations) {
                fieldScope.getMember().getAnnotations().add(a);
            }
        } catch (NoSuchFieldException e) {
            // NOOP
        }
    }
    return null;
}
@CarstenWickner
Copy link
Member

Hi @bmaizel,

Three thoughts on this:

  1. Supporting MixIns would be nice, as they are a standard Jackson feature.
  2. I'm glad you found a workaround for now. That looks awfully Jackson-specific though and would therefore need to be considered carefully (and allowed to be toggled on/off). Doing this in the schema generator is not the ideal place to start.
  3. Since the ResolvedField you're adding the annotation one yourself is returned by the classmate library, which happens to be maintained by the same person as the jackson libraries, it'd be a good idea to approach them for suggestions how to consider things like MixIns in classmate. If that requires changes inside the schema generator, I'm open to that.

Will you approach Tatu through https://github.com/FasterXML/java-classmate?

@CarstenWickner CarstenWickner added the help wanted Extra attention is needed label Nov 18, 2023
@bmaizel
Copy link
Author

bmaizel commented Nov 19, 2023

Hi @CarstenWickner,

I could do it, but you can do it better (please!). You definitely have much deeper understanding of the problem, and your code is going to use the (new) functionality, so better you start the discussion. Of course I will follow, and I will try to help if I see how.

@CarstenWickner
Copy link
Member

Hi @bmaizel,

I had a brief look into the classmate code and figured that this is the place, where one would have to make a change to get mix-ins to work:

return this.memberResolver.resolve(resolvedType, this.annotationConfig, null);

The third parameter on the classmate MemberResolver.resolve() method is of type AnnotationOverrides.
Currently, I'm always sending null. But providing an AnnotationOverrides instance would allow mix-ins to be respected.

Perhaps creating a subtype of the AnnotationOverrides, which delegates to the ObjectMapper's findMixInClassFor() method, would do the trick.
I've never used mix-ins myself, so I feel a little out of my depth here. Feel free to give it a shot and create a PR to move this along.

@CarstenWickner
Copy link
Member

I've raised the question now: FasterXML/java-classmate#81
Let's see, what comes out of it.

@bmaizel
Copy link
Author

bmaizel commented Nov 22, 2023

Subscribed.
BTW, don't forget, that mix-ins can extend not only annotations on the class members, but also on the class itself.

@CarstenWickner
Copy link
Member

One challenge at a time. 😆
Feel free to provide PRs to extend the mix-in support going forward. 😉

@bmaizel
Copy link
Author

bmaizel commented Nov 23, 2023

I have a workaround 😆
It seams to be more complicated then I thought. I will try to find time for it next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Development

No branches or pull requests

2 participants