Skip to content

Simplify registration of Jackson mixin types #30152

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

Closed
wants to merge 1 commit into from

Conversation

terminux
Copy link
Contributor

Hi, this PR basically solves #25920, but there are still some issues to consider:

@Bean
public MappingJackson2HttpMessageConverter jackson2HttpMessageConverter() {
    return new MappingJackson2HttpMessageConverter(new ObjectMapper());
}

Closes gh-25920

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 10, 2022
@mhalbritter mhalbritter self-requested a review March 10, 2022 07:39
@mhalbritter
Copy link
Contributor

mhalbritter commented Mar 10, 2022

Hi, thanks for your contribution!

Mix-in classes are mostly abstract classes or interfaces.

When adding a @Scope("prototype") to the @JsonMixin, Spring will not eagerly instantiate the bean. If you then change the addJsonMixinBeans to this:

	private void addJsonMixinBeans(ListableBeanFactory beanFactory) {
		String[] names = beanFactory.getBeanNamesForAnnotation(JsonMixin.class);
		for (String name: names) {
			Class<?> type = beanFactory.getType(name);
			addJsonMixinBean(type);
		}
	}

then Spring will not try to instantiate the bean. You're only interested in the type of the bean, anyway. This way abstract beans and interfaces work.

But I'm not sure if that solution isn't too hacky. @wilkinsona what do you think?

If you want to play around with it, the changes are on this branch.

if the user customizes the MappingJackson2HttpMessageConverter, @JsonMixin may not work

That's the same as with @JsonComponent, right? Should be no problem, we just need to document it here.

@terminux
Copy link
Contributor Author

Thank you very much for your guidance @mhalbritter . Adding @Scope("prototype") to @JsonMixin is indeed a good option, at least better than having the user add the @Lookup . I've updated the PR.

@wilkinsona
Copy link
Member

I don't think @JsonMixin should be annotated with @Component at all. We don't want the mixins to be beans, prototypes or otherwise. Instead, I would use a custom sub-class of ClassPathScanningCandidateComponentProvider that overrides isCandidateComponent to find the @JsonMixin-annotated classes. They can then be registered with the JsonMixinModule by extracting information from the bean definitions that ClassPathScanningCandidateComponentProvider returns. The latter part of this is similar to what EntityScanner already does.

@terminux
Copy link
Contributor Author

Thank you so much for your reply @wilkinsona, very good suggestion. I will adjust the way to find mix-in classes based on your suggestion.

@mhalbritter mhalbritter added the status: waiting-for-feedback We need additional information before we can continue label Mar 10, 2022
@terminux
Copy link
Contributor Author

Hi, I updated the PR and it is ready to be reviewed again.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 18, 2022
@mhalbritter mhalbritter removed their request for review March 18, 2022 08:45
@wilkinsona
Copy link
Member

This is looking really good now, @terminux. Thank you. The only thing I'm not completely sure about is the need for @JsonMixinScan. I'm in two minds about whether or not it needs to be separately configurable, of it we should just use the "main" packages for scanning for @JsonMixin-annotated types. I'll flag this for a forthcoming team meeting so that we can discuss it.

@wilkinsona wilkinsona added type: enhancement A general enhancement for: team-meeting An issue we'd like to discuss as a team to make progress and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Mar 18, 2022
@wilkinsona wilkinsona added this to the 2.7.x milestone Mar 18, 2022
@philwebb
Copy link
Member

We discussed this today on our team call and we'd like to proceed but without the @JsonMixinScan annotation. We take that on as part of the merge. Thanks @terminux.

@philwebb philwebb added for: merge-with-amendments Needs some changes when we merge and removed for: team-meeting An issue we'd like to discuss as a team to make progress labels Mar 23, 2022
@wilkinsona wilkinsona self-assigned this Mar 25, 2022
@wilkinsona wilkinsona modified the milestones: 2.7.x, 2.7.0-RC1 Mar 25, 2022
wilkinsona pushed a commit that referenced this pull request Mar 25, 2022
wilkinsona added a commit that referenced this pull request Mar 25, 2022
@wilkinsona
Copy link
Member

Thanks very much for the contribution, @terminux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify registration of Jackson mixin types
5 participants