-
-
Notifications
You must be signed in to change notification settings - Fork 358
Introduce jackson 3 support #1539
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
base: main
Are you sure you want to change the base?
Conversation
spring-cloud-aws-sns/src/main/java/io/awspring/cloud/sns/core/JsonStringEncoderDelegator.java
Outdated
Show resolved
Hide resolved
|
In general great job @MatejNedic! Left two comments plus all new public classes need proper Javadocs |
tomazfernandes
left a comment
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.
Hey @MatejNedic , overall this looks good on the SQS side. I left a couple of questions, mostly around simplifying version detection.
I also think we could prefix the previous classes "Jackson2" rather than Legacy so it's more precise, but that's just a nit.
Let me know what you think.
|
|
||
| import org.springframework.messaging.converter.MessageConverter; | ||
|
|
||
| public abstract class AbstractMessageConverterFactory { |
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’m curious: could we use direct instantiation based on classpath detection instead of introducing this factory abstraction?
For instance, Spring Kafka’s MessagingMessageConverter handles Jackson version selection via classpath detection:
Since we already have JacksonPresent too, could we apply the same pattern here so version selection is automatic and users don’t have to manually choose? If there’s a downside or constraint in our case, what is it?
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 was thinking that people can extend AbstractMessageConverterFactory and do their own implementation of MessageConverter. Also, it is much more readable when I refactored to this way considering deletions and so on.
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.
Hey @MatejNedic, I understand what you're aiming at, and we already support users providing their own MessageConverters through the SqsListenerConfigurer.
As far as I can tell, the only place we're using the create() method from AbstractMessageConverterFactory is in AbstractListenerAnnotationBeanPostProcessor.java:345.
I think we could just detect the Jackson version on the classpath there and decide what converter to use automatically.
This might eliminate the need for AbstractMessageConverterFactory, JacksonJsonMessageConverterFactory, and LegacyJackson2MessageConverterFactory, and make overall converter configuration simpler, while still preserving the customization options for users.
What do you think?
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.
Hey @tomazfernandes , I was thinking about it but I came up with following scenario.
What if you have Jackson 3 and Jackson 2 on classpath. Since this is messaging I want to keep Jackson 2 for SQS integration. currently if we put JacksonPresent users are not able to change this behaviour.
Notice inside of SqsListenerAnnotationBeanPostProcessor we are also based on instace getting ObjectMapper or JsonMapper. I want to allow users to control this. I don't also want to polute EndpointRegistar with ObjectMapper and JsonMapper (we had objectMapper there). So for me best way was to do a wrapper and then extract either Jackson 2 or Jackson 3 based on whichever Bean users defined. This allows users themselves to have on Classpath Jackson 3 and 2 and opt in for 2.
if (wrapper instanceof JacksonJsonMessageConverterFactory) {
argumentResolvers.add(new NotificationMessageArgumentResolver(messageConverter,
((JacksonJsonMessageConverterFactory) wrapper).getJsonMapperWrapper().getJsonMapper()));
argumentResolvers.add(new NotificationSubjectArgumentResolver(
((JacksonJsonMessageConverterFactory) wrapper).getJsonMapperWrapper().getJsonMapper()));
argumentResolvers.add(new SnsNotificationArgumentResolver(messageConverter,
((JacksonJsonMessageConverterFactory) wrapper).getJsonMapperWrapper().getJsonMapper()));
}
else if (wrapper instanceof LegacyJackson2MessageConverterFactory) {
argumentResolvers.add(new LegacyJackson2NotificationMessageArgumentResolver(messageConverter,
((LegacyJackson2MessageConverterFactory) wrapper).getObjectMapper()));
argumentResolvers.add(new LegacyJackson2NotificationSubjectArgumentResolver(
((LegacyJackson2MessageConverterFactory) wrapper).getObjectMapper()));
argumentResolvers.add(new LegacyJackson2SnsNotificationArgumentResolver(messageConverter,
((LegacyJackson2MessageConverterFactory) wrapper).getObjectMapper()));
}
@tomazfernandes Wdyt?
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 want to allow users to control this.
If the goal is to enable users to force Jackson 2 over Jackson 3 when both are present, a more consistent approach would be exposing this as an option on the EndpointRegistrar, so users can explicitly toggle it via SqsListenerConfigurer. For example JacksonVersion {AUTO, JACKSON_2, JACKSON_3} or something similar. We can also expose the same option as an auto-configuration property if we’d like.
This way we rely on the existing configuration path instead of introducing a new one, and we don’t require users to declare a bean just to flip a configuration toggle. It also makes the selection explicit user intent and keeps the wiring simpler.
If it helps unblock the release we can make this PR focused on the default behavior only (no factory, detect Jackson 2 vs 3, if both are present default to Jackson 3), and add the explicit opt-in toggle in a separate PR.
Let me know your thoughts.
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.
Hey @MatejNedic, can you share the exact 3.x scenario you're referring to, ideally a snippet?
Thanks.
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 can completely rework this for RC1.
@MatejNedic, appreciate you reworking this for RC1.
For the set later behavior with ObjectMapper, we can keep the 3.x wiring, with both Jackson 2's ObjectMapper (when present) alongside Jackson's 3 JsonMapper in the EndpointRegistrar, and build the appropriate converter.
Let me know if you run into any issues.
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.
@MatejNedic, after looking more closely at the classpath and classloading implications, I think this path could introduce CNFE/classpath edge cases across certain dependency combinations, including native/AOT setups.
I also think the current implementation has similar risk: some code paths (for example, instanceof checks against Jackson-specific impl classes) may trigger eager loading/linking of those impls, which can fail when the relevant Jackson dependency is not present.
If timeline allows, I’d prefer releasing this as an M2 so we have room to iterate and gather feedback before RC/GA. If RC1 is the priority, I’m ok shipping as-is and then reassessing for an RC2 if needed.
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.
@tomazfernandes let's release it as RC1. I believe this solution is good as it is even for GA - the mentioned factory class should be deprecated from the start as this is anyway only intermediary solution for backward compatibility with Jackson 2.
Of course, if we have ideas and capacity to implement a better one, we could do it, but seems like time flies faster than we are able to catch up.
Are you able to give more specific places in code that can introduce CNFE? (I don't see it but i might have missed it).
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.
@maciejwalkowiak agreed on RC1.
While Matej's refactor addressed the instanceof CNFE concerns, I've made some review comments that point out a few remaining points.
the mentioned factory class should be deprecated from the start as this is anyway only intermediary solution for backward compatibility with Jackson 2
Agreed. Let's reflect that in the code by marking it as deprecated and renaming it to something more migration-scoped to make the long-term direction clearer.
...ws-autoconfigure/src/main/java/io/awspring/cloud/autoconfigure/sqs/SqsAutoConfiguration.java
Outdated
Show resolved
Hide resolved
tomazfernandes
left a comment
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.
Left a few minor nits.
.../main/java/io/awspring/cloud/sqs/annotation/AbstractListenerAnnotationBeanPostProcessor.java
Outdated
Show resolved
Hide resolved
...src/main/java/io/awspring/cloud/sqs/support/converter/AbstractMessagingMessageConverter.java
Outdated
Show resolved
Hide resolved
...ing/cloud/sqs/support/resolver/legacy/LegacyJackson2NotificationMessageArgumentResolver.java
Show resolved
Hide resolved
tomazfernandes
left a comment
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.
Nice work with the refactor, @MatejNedic! I left some follow-ups. Once those are addressed, I’ll do a final pass and we should be good to go.
| sqsConverter.setObjectMapper(om); | ||
| private void setMapperToConverter(MessagingMessageConverter<?> messagingMessageConverter, | ||
| AbstractMessageConverterFactory factory) { | ||
| if (messagingMessageConverter instanceof LegacyJackson2SqsMessagingMessageConverter sqsConverter) { |
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.
This branch can still fail with NoClassDefFoundError in Jackson3-only setups (or AOT/native) because the instanceof LegacyJackson2SqsMessagingMessageConverter may trigger resolution/linking of the Jackson2 converter type when Jackson2 isn’t on the runtime classpath.
I suggest guarding before the instanceof:
if (JacksonPresent.isJackson2Present()
&& messagingMessageConverter instanceof LegacyJackson2SqsMessagingMessageConverter sqsConverter) {
sqsConverter.setObjectMapper(((LegacyJackson2MessageConverterFactory) factory).getObjectMapper());
}Also, let's create a similar path for Jackson 3 (see comment for the converter below).
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 thought of better way.
Once I push refactor lets check together design and see if you like it :)
| // Object Mapper for SqsListener annotations handler method | ||
| if (registrar.getObjectMapper() == null && objectMapper != null) { | ||
| registrar.setObjectMapper(objectMapper); | ||
| if (wrapper != null) { |
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.
We should also check if the registrar doesn't have a user-defined wrapper before assigning this one to it.
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 have on purpose made interface public. I wanted users to be able to extend and implement their own implementation. If they want to use different converter or something like that.
| ObjectProvider<SqsListenerObservation.Convention> observationConventionProvider, | ||
| ObjectProvider<MessageInterceptor<Object>> interceptors, ObjectProvider<ObjectMapper> objectMapperProvider, | ||
| ObjectProvider<MessageInterceptor<Object>> interceptors, | ||
| ObjectProvider<AbstractMessageConverterFactory> objectMapperProvider, |
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.
Variable name is inconsistent since this is not an ObjectMapper anymore.
| }); | ||
| } | ||
|
|
||
| @Disabled("We should see what we want with this test!") |
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.
Not sure I understand the question - why did you need to disable it?
Why not have a JsonMapperConfiguration?
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.
Since in configuresFactoryComponentsAndOptions we were checking if user provided objectMapper was set.
Issue is in Spring Boot 4 Jackson 3 MessageConverter cannot accept JsonMapper after it was created. Only through the constructor. So I disabled the test.
| messageConverters.add(new StringMessageConverter()); | ||
| messageConverters.add(new SimpleMessageConverter()); | ||
| messageConverters.add(createDefaultMappingJackson2MessageConverter(this.endpointRegistrar.getObjectMapper())); | ||
| if (endpointRegistrar.getAbstractMessageConverterFactory() != null) { |
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.
SQS needs to work standalone without Spring Boot and autoconfiguration, so we can't rely on having a wrapper here to offer a working OOTB solution.
My suggestion is we branch here based on JacksonPresent and instantiate the corresponding default wrapper if the registrar doesn't have one.
| * @param messageConverterConfigurer a {@link LegacyJackson2SqsMessagingMessageConverter} consumer. | ||
| * @return the builder. | ||
| */ | ||
| SqsTemplateBuilder configureLegacyJackson2Converter( |
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.
See comment about the javadoc above.
| * @author Matej Nedic | ||
| * @since 4.0.0 | ||
| */ | ||
| public interface AbstractMessageConverterFactory { |
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.
Since this is now an interface and does more than create a MessageConverter, the Abstract / Factory naming is misleading.
Since this is an interim Jackson2 compatibility surface, please rename it to something Jackson and migration-scoped (e.g. JacksonMigrationHelper) and mark it @deprecated(forRemoval=true) with javadoc like “helper to assist with Jackson 3 migration, will be removed”.
| private String typeHeader = SqsHeaders.SQS_DEFAULT_TYPE_HEADER; | ||
|
|
||
| private MessageConverter payloadMessageConverter; | ||
| public MessageConverter payloadMessageConverter; |
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.
Please move this back to private and pass it through the constructor instead of setting it directly in the subclass.
| } | ||
|
|
||
| private SimpleClassMatchingMessageConverter createClassMatchingMessageConverter() { | ||
| public SimpleClassMatchingMessageConverter createClassMatchingMessageConverter() { |
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.
The logic for createDefaultCompositeMessageConverter should remain in the AbstractMessagingMessageConverter.
Each subclass can have a "createDefaultJacksonMessageConverter" implementation, and either have the logic for creating the specific Jackson converter there, or use a specific JacksonAbstractMessageConverterFactory for the corresponding Jackson version.
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 want to reuse them because they are used in composite. I am not seeing the difference tbh.
I refactored mb this will make more sense please check.
| * Set the {@link ObjectMapper} instance to be used for converting the {@link Message} instances payloads. | ||
| * @param objectMapper the object mapper instance. | ||
| */ | ||
| public void setObjectMapper(ObjectMapper objectMapper) { |
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.
With this diff we're removing the ability for the user to set their own mapper in the converter through direct configuration.
I suggest that instead we make it so that if the user sets a JsonMapper, we'll use that to create a new JacksonJsonMessageConverter with the default settings, and then follow a similar path to the current one.
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.
User should pass objectMapper through the factory. Since now Factory objectMapper is used to be set later on MessageConverter.
Thing is I don't want in this class anything Jackson 2 or Jackson 3 specific. That is why I went with Factroy approach to completely remove any Jackson 2 or 3 classes from here.
In JacksonJsonMessageConverter for Jacskon 3 we cannot set JsonMapper after Converter is created that is why I moved it to factory creation as well so JsonMapper user passed is respected.
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.
User should pass objectMapper through the factory. Since now Factory objectMapper is used to be set later on MessageConverter.
This is not true for user-instantiated converters, users need a way to manually set it.
Thing is I don't want in this class anything Jackson 2 or Jackson 3 specific.
We can move this method (and the one for the JsonMapper) to the respective subclasses.
|
@tomazfernandes Thanks on review will apply the changes today! Please check the comments I left. |
tomazfernandes
left a comment
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.
Thanks Matej. I’ll take a look.
SQS acceptance criteria for this PR:
• CNFE/classpath risks are mitigated
• No new long-term supported user-facing APIs or extension points are introduced as part of the migration
• No loss of existing user functionality
tomazfernandes
left a comment
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.
@MatejNedic, requesting a couple more changes.
Since this is a migration-scoped PR, let’s not introduce new long-term user-facing APIs or extension points. For SQS, we don’t plan to support JacksonMessageConverterFactory long term, so it should be scoped as migration-only.
So, please:
• rename it to something migration-scoped, and
• annotate it with @deprecated(forRemoval = true) plus migration-only Javadoc.
Once updated, I’ll take another look.
| * @author Matej Nedic | ||
| * @since 4.0.0 | ||
| */ | ||
| public interface JacksonMessageConverterFactory { |
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.
As previously aligned, for SQS, we don’t intend to support this factory type as a long-term API. This should be a migration-only hook.
Please rename it to something migration-specific, annotate it with @deprecated(forRemoval = true), and add migration-only Javadoc.
| // Object Mapper for SqsListener annotations handler method | ||
| if (registrar.getObjectMapper() == null && objectMapper != null) { | ||
| registrar.setObjectMapper(objectMapper); | ||
| if (factory != null) { |
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.
We should check if the user has set a wrapper to the registrar already before overriding it.
| return new SqsMessagingMessageConverter(); | ||
| } | ||
|
|
||
| private static LegacyJackson2SqsMessagingMessageConverter createDefaultLegacyJackson2MessageConverter() { |
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.
Not sure what you mean, can you be more specific about how this enum would work?
| } | ||
|
|
||
| @Override | ||
| public SqsTemplateBuilder configureLegacyJackson2Converter( |
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 don't see how an enum could solve this public API issue, can you be more specific?
If nothing comes to mind, my suggestions stands - we can add a guard clause and document it.
| * Set the {@link ObjectMapper} instance to be used for converting the {@link Message} instances payloads. | ||
| * @param objectMapper the object mapper instance. | ||
| */ | ||
| public void setObjectMapper(ObjectMapper objectMapper) { |
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.
User should pass objectMapper through the factory. Since now Factory objectMapper is used to be set later on MessageConverter.
This is not true for user-instantiated converters, users need a way to manually set it.
Thing is I don't want in this class anything Jackson 2 or Jackson 3 specific.
We can move this method (and the one for the JsonMapper) to the respective subclasses.
| } | ||
|
|
||
| private SimpleClassMatchingMessageConverter createClassMatchingMessageConverter() { | ||
| public static SimpleClassMatchingMessageConverter createClassMatchingMessageConverter() { |
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.
Please move this method back to private so it doesn't create a public-facing API.
| } | ||
|
|
||
| private StringMessageConverter createStringMessageConverter() { | ||
| public static StringMessageConverter createStringMessageConverter() { |
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.
Please move this method back to private so it doesn't create a public-facing API.
📢 Type of change
📜 Description
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
🔮 Next steps