Skip to content

Carry the notification subject on SNS batch entries - #1704

Open
anjeongkyun wants to merge 1 commit into
awspring:mainfrom
anjeongkyun:fix/sns-batch-subject-header
Open

Carry the notification subject on SNS batch entries#1704
anjeongkyun wants to merge 1 commit into
awspring:mainfrom
anjeongkyun:fix/sns-batch-subject-header

Conversation

@anjeongkyun

Copy link
Copy Markdown

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

SnsHeaderConverterUtil.isSkipHeader leaves NOTIFICATION_SUBJECT_HEADER out of the message attributes, because the subject belongs on the request rather than in the attribute map:

private static boolean isSkipHeader(String headerName) {
    return NOTIFICATION_SUBJECT_HEADER.equals(headerName) || MESSAGE_GROUP_ID_HEADER.equals(headerName)
            || MESSAGE_DEDUPLICATION_ID_HEADER.equals(headerName);
}

Two of the three publish paths then set it:

  • TopicMessageChannel.sendInternal calls .subject(findNotificationSubject(message))
  • DefaultSnsPublishMessageConverter.populateHeaders sets publishRequest::subject

DefaultSnsMessageConverter (batch) never mentions the subject. PublishBatchRequestEntry has a subject field, so nothing fails - the header is excluded from the attributes and never put on the entry, and the subject is silently gone. The FIFO headers next to it are already handled in all three paths, which is what made the gap visible.

So the same message sent through SnsTemplate reaches subscribers with a subject, and through SnsBatchTemplate without one. Email subscribers get an empty subject line.

The change follows DefaultSnsPublishMessageConverter, the closest sibling, and reads the header as a String rather than calling toString() on it the way TopicMessageChannel does.

This PR also carries a one-line fix in the same subpackage: SequentialBatchExecutionStrategy.send asserts topicArn twice and never checks entries.

Assert.notNull(topicArn, "topicArn is required");
Assert.notNull(topicArn, "entries are required");

Happy to split that into its own PR if you would rather keep this one to the subject.

💡 Motivation and Context

Both came in with the batch template in #1574 and neither is covered by a test today.

💚 How did you test it?

Added setsNotificationSubject to DefaultSnsMessageConverterTest, which checks the subject lands on the entry and does not leak into the message attributes. It fails on current main and passes with the change. Extended leavesOptionalHeadersNullWhenAbsent to cover the subject alongside the FIFO headers it already asserts.

spring-cloud-aws-sns passes in full (119 tests).

📝 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

The SNS reference docs describe the payload types the batch template supports and do not enumerate headers, so there is nothing there to correct.

🔮 Next steps

While reading this code I noticed SnsAsyncTopicArnResolver.resolveTopicArn calls .join() when it is given a topic name rather than an ARN, which blocks the caller of SnsAsyncTemplate. TopicArnResolver is a synchronous interface, so changing that is an API question rather than a fix. I can open an issue if it is worth discussing.

The subject header is skipped when building message attributes, on the
assumption that it is set on the request instead. The single publish and async
paths do set it; the batch converter never did, so the subject was dropped.

Also fixes a duplicated assertion in SequentialBatchExecutionStrategy that
checked topicArn twice and left entries unvalidated.
@github-actions github-actions Bot added the component: sns SNS integration related issue label Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: sns SNS integration related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant