fix(sns): support SubscriptionConfirmation and UnsubscribeConfirmation message types#1102
fix(sns): support SubscriptionConfirmation and UnsubscribeConfirmation message types#1102
Conversation
…n message types The SnsMessage and SnsMessageObj structs were failing to deserialize SubscriptionConfirmation and UnsubscribeConfirmation SNS messages because: 1. `unsubscribe_url` was required (String) but these message types don't have it 2. `subscribe_url` and `token` fields were missing entirely This fix: - Makes `unsubscribe_url` optional (Option<String>) since it's only present in Notification messages - Adds `subscribe_url` field (Option<String>) for confirmation messages - Adds `token` field (Option<String>) for confirmation messages All fields use #[serde(default)] to handle missing fields gracefully. Fixes aws#966 Signed-off-by: abhu85 <151518127+abhu85@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You are correct to state that
I didn't realize that it was possible to dispatch the (un)subscribe notifications to an lambda event sink in general. Could you share a little about your use case? Anyway - Rather than this breaking change, how about annotating the current type with a note that it is only for Notifications. And then add a separate type for the (un)subscription confirmations? We could also have a |
|
Thanks for the feedback @jlizen! You're absolutely right about avoiding breaking changes. Use CaseWe're building Lambda functions that handle SNS subscription confirmations directly (e.g., when subscribing HTTP(S) endpoints to SNS topics programmatically). The Lambda needs to parse the subscription confirmation message to extract the Proposed Non-Breaking ApproachI agree with your suggestion. Here's what I propose: Option 1: Separate Types (Recommended)// Keep existing for notifications (no changes)
pub struct SnsMessage { ... }
// New type for confirmations
pub struct SnsSubscriptionConfirmation {
pub message: String,
pub subscribe_url: Option<String>,
pub unsubscribe_url: Option<String>,
pub token: String,
// ... other common fields
}
// Optional: Enum wrapper
#[non_exhaustive]
pub enum SnsEvent {
Notification(SnsMessage),
SubscriptionConfirmation(SnsSubscriptionConfirmation),
UnsubscribeConfirmation(SnsSubscriptionConfirmation),
}Option 2: Separate ModuleMove confirmation types to a new Would you prefer Option 1 with the enum wrapper, or would you like me to explore a different approach? I'm happy to revise the PR based on your preferred design. Also, should I add documentation explaining which message types map to which structs based on the |
|
I prefer #1. No need to separate out to a new module. I don't think we need the enum wrapper unless you have a usage for it. It sounds like you don't. I think lambdas handling subscription events will generally be fairly distinct from ones handling notification events.
Is this with the enum? If you do need the enum, then yes you should document semantics. Otherwise, I think we just could use some clearer docs on the For the enum, do keep in mind that you will need to handle getting serde to distinguish the two, so your example wouldn't work exactly as written (no serde handling). I would probably not bother unless you have a need for it (ie, you are receiving a mixed stream of event types). |
|
Thanks for the clarification @jlizen! I've drafted an implementation following your guidance. Here's the plan: Proposed Implementation1. Keep
|
|
Overall approach seems fine.
That's fine
Follow existing conventions. I believe that is only to export the module, which we already have for sns.
I probably would prefer
I think we probably already have a test covering this, but if not, this would be good too. |
Add a separate SnsSubscriptionMessage struct for handling SubscriptionConfirmation
and UnsubscribeConfirmation SNS message types. This is a non-breaking change that
keeps the existing SnsMessage struct unchanged.
Changes:
- Add SnsSubscriptionMessage struct with subscribe_url (Option) and token fields
- Add documentation to SnsMessage/SnsMessageObj clarifying they are for Notification only
- Add test fixtures for both SubscriptionConfirmation and UnsubscribeConfirmation
- Add tests verifying deserialization of both confirmation types
The new struct distinguishes confirmation types by:
- sns_message_type field ("SubscriptionConfirmation" or "UnsubscribeConfirmation")
- subscribe_url: Some(url) for SubscriptionConfirmation, None for UnsubscribeConfirmation
Fixes aws#966
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@jlizen I've implemented the changes based on your feedback! Summary of Changes (commit 86cac61)1. New
|
jlizen
left a comment
There was a problem hiding this comment.
LGTM overall, one small change.
lambda-events/src/event/sns/mod.rs
Outdated
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```ignore |
There was a problem hiding this comment.
This doesn't need to be ignore, does it? It should compile as-is?
There was a problem hiding this comment.
You're right! Fixed in 9a9a3c5 - removed the ignore attribute. Doc tests confirm it compiles correctly.
jlizen
left a comment
There was a problem hiding this comment.
whoops, meant to request the one change
The doc example compiles as-is, so the ignore attribute is unnecessary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Add a separate
SnsSubscriptionMessagestruct for handling SNS SubscriptionConfirmation and UnsubscribeConfirmation message types. This is a non-breaking change that keeps the existingSnsMessagestruct unchanged.Fixes #966
Changes
1. New
SnsSubscriptionMessageStruct2. Documentation Updates
SnsMessageandSnsMessageObjclarifying they are for Notification messages onlySnsSubscriptionMessagefor handling confirmation types3. Test Coverage
my_example_sns_subscription_confirmation- verifies SubscriptionConfirmation deserializationmy_example_sns_unsubscribe_confirmation- verifies UnsubscribeConfirmation deserializationUsage Example
Breaking Changes
None. This PR:
SnsMessageorSnsMessageObjstructsTest Results
Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com