Skip to content

fix(notifications): dedupe SNS/SQS policy statements by Sid (#801) - #802

Merged
zirkelc merged 4 commits into
masterfrom
fix/801_notifications-duplicate-sid
Aug 3, 2026
Merged

fix(notifications): dedupe SNS/SQS policy statements by Sid (#801)#802
zirkelc merged 4 commits into
masterfrom
fix/801_notifications-duplicate-sid

Conversation

@VirtueMe

Copy link
Copy Markdown
Collaborator

Closes #801

Summary

Test plan

  • npm test — 618 passing
  • npm run lint — clean
  • Packaged notifications fixture → one merged policy per target, 1 statement with a unique Sid each

🤖 Generated with Claude Code

- mergeResourcePolicies now dedupes statements by Sid via a
  mergeStatements helper, and folds the SNS/SQS branches into one
  table-driven loop
- Two machines sharing a topic/queue emit byte-identical statements
  with the same Sid; concatenating them made AWS reject the template
  with "Every policy statement must have a unique ID"
- Add unit tests asserting Sid uniqueness for shared SNS/SQS targets;
  correct two pre-existing count assertions (10->5) that encoded the bug
- Replace the notifications fixture's statement-count checks with
  Sid-uniqueness guards (LocalStack accepts duplicate Sids, so only the
  template assertion catches this)

Part of #801

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@VirtueMe
VirtueMe requested a review from zirkelc July 31, 2026 11:37
@pkg-pr-new

pkg-pr-new Bot commented Jul 31, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/serverless-operations/serverless-step-functions@802

commit: 33c7a40

Comment on lines +222 to +223
'AWS::SNS::TopicPolicy': { map: new Map(), keyOf: (r) => JSON.stringify(r.Properties.Topics) },
'AWS::SQS::QueuePolicy': { map: new Map(), keyOf: (r) => JSON.stringify(r.Properties.Queues) },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A stable key might require a sort first, otherwise the keys follow insertion order.

@copilot wdyt?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call — I updated this in 010fcd6 by normalizing the merge key with a sorted target list before JSON.stringify, so equivalent topic/queue sets produce a stable key regardless of insertion order.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good call!

Copilot AI requested a review from zirkelc August 3, 2026 06:48
}

function policyTargetKey(targets) {
return JSON.stringify(_.sortBy([].concat(targets), (target) => JSON.stringify(target)));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot that still does not sort the keys, right?

here is what I meant:

function stringifyJSON(obj) {
  return JSON.stringify(obj, (key, value) =>
    value && typeof value === 'object'
      ? Object.keys(value)
          .sort()
          .reduce<{ [key: string]: unknown }>((result, k) => {
            result[k] = value[k];
            return result;
          }, {})
      : value,
  );
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — I updated this in 3f21642 to canonicalize objects recursively (sorted keys) before stringifying, and then sort the target list by that canonical string.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot why is better in your solution than using the the replace arg of JSON.stringify?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing fundamentally better—both approaches are valid. I used a separate canonicalize+stringify helper mainly for readability/reuse and explicit recursion in tests/debugging. If you prefer, I can switch to the JSON.stringify replacer form for the same behavior.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot yes do that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 33c7a40 to use the JSON.stringify replacer form for stable key serialization (with sorted object keys) in the policy merge key path.

@zirkelc
zirkelc merged commit f1a5f47 into master Aug 3, 2026
4 checks passed
@zirkelc
zirkelc deleted the fix/801_notifications-duplicate-sid branch August 3, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notifications: merged SNS/SQS policy has duplicate Sids → "Every policy statement must have a unique ID" (regression from #747)

3 participants