Skip to content

Conversation

@MatejNedic
Copy link
Member

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions github-actions bot added component: s3 S3 integration related issue component: secrets-manager Secrets Manager integration related issue component: sns SNS integration related issue component: sqs SQS integration related issue component: core Core functionality related issue labels Dec 11, 2025
@MatejNedic MatejNedic marked this pull request as ready for review December 17, 2025 15:05
@maciejwalkowiak
Copy link
Contributor

In general great job @MatejNedic! Left two comments plus all new public classes need proper Javadocs

Copy link
Contributor

@tomazfernandes tomazfernandes left a 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 {
Copy link
Contributor

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:

https://github.com/spring-projects/spring-kafka/blob/fcca99f6ae3d4f370817cfd1e24f2ae5db93efbf/spring-kafka/src/main/java/org/springframework/kafka/support/converter/MessagingMessageConverter.java#L93-L105

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?

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

@MatejNedic MatejNedic Dec 23, 2025

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?

Copy link
Contributor

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.

@github-actions github-actions bot added the type: documentation Documentation or Samples related issue label Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: core Core functionality related issue component: s3 S3 integration related issue component: secrets-manager Secrets Manager integration related issue component: sns SNS integration related issue component: sqs SQS integration related issue type: documentation Documentation or Samples related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants