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

Auto-configured JacksonJsonpMapper is conditional on an ObjectMapper bean but does not use such a bean #36109

Closed
wants to merge 1 commit into from

Conversation

Pengfei-Lu
Copy link
Contributor

@Pengfei-Lu Pengfei-Lu commented Jun 29, 2023

Spring Boot version

3.1.1

Bug description

The autoconfigured JacksonJsonpMapper bean used to depend on a global ObjectMapper bean before SpringBoot version v3.0.3.

After this commit introduced by SpringBoot version v3.0.3, the global ObjectMapper bean is not required when creating JacksonJsonpMapper bean.

However, the JacksonJsonpMapper bean creation condition @ConditionalOnBean(ObjectMapper.class) is not changed, resulting in JacksonJsonpMapper bean can be created only when ObjectMapper bean is present instead of the ObjectMapper class.

Bug resolving solution

Change @ConditionalOnBean(ObjectMapper.class) to @ConditionalOnClass(ObjectMapper.class)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 29, 2023
@wilkinsona wilkinsona changed the title Fix JacksonJsonpMapper bean autoconfigure condition bug Auto-configured JacksonJsonpMapper is conditional on an ObjectMapper bean but does not use such a bean Jun 29, 2023
@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 29, 2023
@wilkinsona wilkinsona added this to the 3.1.x milestone Jun 29, 2023
@Pengfei-Lu
Copy link
Contributor Author

Pengfei-Lu commented Jun 29, 2023

I don't figure it out how to mock the "ObjectMapper class is not loaded" senario, thus deleting the two failed withoutJsonbOrJacksonShouldDefineSimpleMapper & withJsonbShouldDefineJsonbMapper tests cases.

Is there any better solution?

@wilkinsona
Copy link
Member

You should be able to use contextRunner.withClassLoader(new FilteredClassLoader(ObjectMapper.class)).

@Pengfei-Lu
Copy link
Contributor Author

You should be able to use contextRunner.withClassLoader(new FilteredClassLoader(ObjectMapper.class)).

It works! Please review the code.

Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, @Pengfei-Lu. I think ElasticsearchClientAutoConfiguration could also be updated as it no longer needs to be ordered after JacksonAutoConfiguration. Could you do that as well please?

@Pengfei-Lu Pengfei-Lu requested a review from wilkinsona June 30, 2023 01:10
…bean but does not use such a bean.

Signed-off-by: Pengfei-Lu <[email protected]>
@Pengfei-Lu
Copy link
Contributor Author

Thanks for the updates, @Pengfei-Lu. I think ElasticsearchClientAutoConfiguration could also be updated as it no longer needs to be ordered after JacksonAutoConfiguration. Could you do that as well please?

Done. Please check.

@Pengfei-Lu
Copy link
Contributor Author

@wilkinsona If there is any update, please let me know, thanks!

@wilkinsona
Copy link
Member

Thanks very much for the PR, @Pengfei-Lu.

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

Successfully merging this pull request may close these issues.

3 participants