Repository navigation
Conversation
MatejNedic
left a comment
There was a problem hiding this comment.
Hey @azunox ,
Thanks on PR! I see few things I personally would do differently.
@tomazfernandes what do you think?
| private static final TypeReference<Map<String, SnsNotification.MessageAttribute>> SNS_MESSAGE_ATTRIBUTES_TYPE = new TypeReference<>() { | ||
| }; | ||
|
|
||
| private final JsonMapper jsonMapper = new JsonMapper(); |
There was a problem hiding this comment.
JsonMapper should be the same one that is passed in autoconfiguration.
| accessor.copyHeadersIfAbsent(getMessageAttributesAsHeaders(source)); | ||
| accessor.copyHeadersIfAbsent(createDefaultHeaders(source)); | ||
| accessor.copyHeadersIfAbsent(createAdditionalHeaders(source)); | ||
| accessor.copyHeadersIfAbsent(getSnsMessageAttributesAsHeaders(source)); |
There was a problem hiding this comment.
This runs on every SQS message, whether it's an SNS notification or not. SqsHeaderMapper is the default mapper for all SQS traffic, so this adds an SNS-envelope check (and a JSON parse on match) to the hot path for everyone, even deployments that never receive SNS messages.
Custom SnsAwareSqsHeaderMapper should be implemented and wired via existing hooks such as AbstractMessagingMessageConverter.setHeaderMapper and AbstractMessagingMessageConverter.configureHeaderMapper.
Container options can be used then to configure this custom mapper for Queues which will have messages sent via SnsTemplate. This makes it cheaper for performance.
|
Hello! Thanks for the review, @MatejNedic I agree that SNS-specific parsing should not run for all SQS messages. I propose moving the logic into an opt-in SnsAwareSqsHeaderMapper that receives the application's configured JsonMapper, with focused tests and a configuration example for queues receiving SNS notifications. does this approach look right to you before I update the PR? |
|
Hey @azunox, thanks for the PR, and thanks @MatejNedic for the suggestions. I'll take a closer look shortly. |
|
Hey @azunox and @MatejNedic, agreed on the new Mapper opt-in direction. This can be a Jackson 3-only feature since Jackson 2 support is deprecated, let's have that explicit in the docs. |
Address review comment 3937155533 by moving SNS envelope parsing out of the default SQS mapper and covering explicit converter configuration. Signed-off-by: 김우성 <azunox9@gmail.com>
Address review comment 3937126374 with constructor injection, parser configuration coverage, and a dedicated listener factory example. Signed-off-by: 김우성 <azunox9@gmail.com>
Address Tomaz Fernandes comment 5751349643 by documenting the Jackson 3-only feature and deprecated Jackson 2 support. Signed-off-by: 김우성 <azunox9@gmail.com>
|
Thanks for confirming the direction, @tomazfernandes, and thanks for the suggestions, @MatejNedic.
The changes are split into three commits: Focused tests cover attribute type conversion, SQS header precedence, preservation of additional headers and message IDs, non-SNS and malformed bodies, opt-in converter configuration, and use of the supplied mapper's parsing configuration. Validation: ./mvnw -B -pl spring-cloud-aws-sqs -am \
-Dtest=SqsHeaderMapperTests,SnsAwareSqsHeaderMapperTests \
-Dsurefire.failIfNoSpecifiedTests=false testAll 44 tests passed, with no failures, errors, or skipped tests. The build's Spotless checks also passed. The full repository and LocalStack integration test suites were not run. Please let me know if you'd like any further adjustments. |
📢 Type of change
📜 Description
Maps SNS
MessageAttributesembedded in a non-raw SNS notification envelope to Spring Message headers when the notification is received through SQS.The change:
MessageAttributes.💡 Motivation and Context
When raw message delivery is disabled for an SNS-to-SQS subscription, SNS message attributes are stored inside the SNS JSON envelope rather than as native SQS message attributes.
SqsHeaderMapperpreviously mapped only native SQS message attributes. Consequently, attributes published throughSnsTemplatewere unavailable fromMessage#getHeaders()or an@Headersargument in an@SqsListener.This change promotes the nested SNS attributes to Spring Message headers while retaining native SQS attributes as the authoritative value in case of a name collision.
Closes #1111
💚 How did you test it?
Added focused
SqsHeaderMapperTestscovering:MessageAttributesfound in non-SNS JSON payloads.Executed:
./mvnw -pl spring-cloud-aws-sqs -am -Dtest=SqsHeaderMapperTests -Dsurefire.failIfNoSpecifiedTests=false testResult: 33 tests run, 0 failures, 0 errors, 0 skipped. The full repository and LocalStack integration test suites were not executed.
📝 Checklist
🔮 Next steps
Run the complete CI and integration test suites through the repository pull request workflow.