[abhiramshaji-cyber]

Running a diff against a stack whose resources lost their metadata crashed with an assertion error, and now the diff prints and ignores that metadata as intended.

What this code is for

fullDiff in packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts compares the deployed template with the new one, and when a change set is available it hands every changed resource to TemplateAndChangeSetDiffMerger.overrideDiffResourceChangeImpactWithChangeSetChangeImpact. That method corrects each difference with what the change set says. For the resource Metadata section it replaces the difference with an equal pair, so metadata changes never show up in cdk diff or toolkit.diff().

Before this PR

A user runs cdk diff --path-metadata false --asset-metadata false against a stack that was deployed with metadata, as @andresliszt described.

1. The deployed Account resource has Metadata: { 'aws:cdk:path': ... }, and the new template has no Metadata on it.
2. The template diff records an Other difference named Metadata with oldValue set and newValue undefined.
3. overrideDiffResourceChangeImpactWithChangeSetChangeImpact reaches case 'Metadata' and calls new types.Difference(value.newValue, value.newValue).
4. Both arguments are undefined, so the Difference constructor throws AssertionError [ERR_ASSERTION]: oldValue and newValue are both undefined!.
5. The whole diff fails and nothing is printed.

After this PR

Same user, same command.

1. Same as above through step 2.
2. case 'Metadata' now picks value.oldValue when value.isRemoval is true, and value.newValue otherwise.
3. It calls new types.Difference(ignoredMetadata, ignoredMetadata) with the deployed metadata on both sides, so the difference is not counted.
4. The diff prints, without a metadata line, the same as when metadata only changed.
5. Metadata that was added, or changed on both sides, still uses value.newValue exactly as before.

The diagram

flowchart LR

classDef box fill:#eef1ff,stroke:#4c5bd4,color:#111827
classDef bad fill:#ffd6d6,stroke:#cc0000,color:#111827,stroke-width:3px
classDef good fill:#c8f0c8,stroke:#008800,color:#111827,stroke-width:3px

subgraph BEFORE["BEFORE: diff crashes, 0 lines printed"]
direction TB
  B1["cdk diff with --path-metadata false"] --> B2["fullDiff with a change set"]
  B2 --> B3["case Metadata in the merger"]
  B3 --> B4{"Difference(newValue, newValue)"}
  B4 -->|"newValue undefined"| B5["AssertionError, diff fails"]
  B4 -->|"newValue present"| B6["metadata ignored"]
end

subgraph AFTER["AFTER: diff prints, metadata ignored"]
direction TB
  A1["cdk diff with --path-metadata false"] --> A2["fullDiff with a change set"]
  A2 --> A3["case Metadata in the merger"]
  A3 --> A4{"THE CHANGE: oldValue when isRemoval"}
  A4 -->|"metadata removed"| A5["equal pair, diff prints"]
  A4 -->|"metadata added or changed"| A6["newValue as before"]
end

B1 ~~~ A1

class B1,B2,B3,B6 box
class A1,A2,A3,A6 box
class B4,B5 bad
class A4,A5 good

linkStyle 3 stroke:#cc0000,stroke-width:5px
linkStyle 8 stroke:#008800,stroke-width:5px

style BEFORE fill:#fff5f5,stroke:#cc0000,color:#111827
style AFTER fill:#f3fff3,stroke:#008800,color:#111827

Loading

The change

• In template-and-changeset-diff-merger.ts, case 'Metadata' now builds the pair from value.isRemoval ? value.oldValue : value.newValue. That is the whole fix. Removed, added and changed metadata all become an equal pair, and none of them can reach the constructor with two undefined values. I chose isRemoval over ?? so a Metadata: null in the new template is still kept as null.
• A new test.each in template-and-changeset-diff-merger.test.ts covers metadata that was removed and metadata that was added. The removed case fails on main with the same assertion as the issue, and both pass with this change. I also checked, outside the suite, null against absent in both directions and absent on both sides; all give zero differences.

Thank you @jakajancar for the detailed report and the logs, which showed exactly which differences reached the merger. @pahud pointed straight at TemplateAndChangeSetDiffMerger, @coderbyheart found that DiffMethod.TemplateOnly() avoids it, and @andresliszt noticed that the metadata flags were the trigger, which is what led me to this line.

I would really value your view on one choice, and I am happy to change it:

• This keeps the current shape and rewrites the Metadata difference as an equal pair. Another option is to drop the Metadata entry from the resource entirely. Would you prefer that? I kept the current shape to stay as close as possible to what is there today.

What changes for a user

• cdk diff and toolkit.diff() no longer crash when the new template drops resource metadata, for example with --path-metadata false or --asset-metadata false.
• Metadata differences stay hidden, as they already were for changed metadata.
• Property changes, replacements and resource additions or removals are reported exactly as before.
• The template only diff, without a change set, is not touched.

Fixes #390

Checklist

• This change contains a major version upgrade for a dependency and I confirm all breaking changes are addressed
  • Release notes for the new version:

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[abhiramshaji-cyber]

I only noticed afterwards that @fukunaga-kengo opened #1741 for this in July, which marked the existing Metadata difference as unchanged instead of building a new pair. That is a nice approach too, and credit to them for it. Would you prefer that direction? I am very happy to switch, or to step aside if they want to reopen theirs.

[fukunaga-kengo]

Thanks for the mention and the kind words! I've reopened #1741 and will keep it up to date.
Your extra test case for added metadata is a nice touch — happy to fold that into #1741 with
credit if you'd like. Thanks again for digging into this.

[mrgrain]

@abhiramshaji-cyber @fukunaga-kengo Thanks for both of your fixes! I think each PR is missing a test case:

• fix(cloudformation-diff): handle removed resource metadata #1741 is missing the remove/add
• fix(cloudformation-diff): diff crashes on removed resource metadata #2106 is not testing the complete absence of the Metadata key

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.08%. Comparing base (95683e3) to head (188d3c4).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2106   +/-   ##
=======================================
  Coverage   92.08%   92.08%           
=======================================
  Files          83       83           
  Lines       13246    13246           
  Branches     1886     1885    -1     
=======================================
  Hits        12197    12197           
  Misses       1011     1011           
  Partials       38       38           

Flag	Coverage Δ
suite.unit	92.08% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[maintainer-for-aws]

Automated review

A maintainer will still review this — treat the notes below as a starting point.

📌 Commit 188d3c4

This PR fixes a crash in cloudformation-diff where cdk diff / toolkit.diff() aborts with an "oldValue and newValue are both undefined" assertion error when diffing against a deployed stack whose resources previously carried CDK metadata that the new template no longer includes. The fix is in the change-set diff-merge path (template-and-changeset-diff-merger.ts): the Metadata suppression branch now selects the defined side (oldValue on removal, otherwise newValue) before building the equal Difference pair, so the constructor never receives two undefined values while metadata still collapses to a non-counted, hidden change.

The change is clean and correctly scoped. The removal, addition, and changed-on-both-sides cases all yield a defined value, and an explicit Metadata: null in the new template is preserved rather than silently replaced. A new test.each covers both the regressing removed case and the added case, asserting the concrete effect. No issues found on this change.

🔴 0 blocking · 🟡 0 recommended · ⚪ 0 optional

---

Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews.

[mrgrain]

@fukunaga-kengo I'm leaning towards #1741 once additional tests are added.

[abhiramshaji-cyber]

Thanks @mrgrain for the review, and thanks @fukunaga-kengo for getting there first with #1741. I'm happy to close this one in favour of #1741.

To help, here is a test for #1741 that drops the Metadata key entirely, covering both removal and addition. On #1741 it passes, and with the fix reverted the removed case fails with the same assertion as #390. Please take it as is or reshape it however you like.

  test.each([
    ['removed', true, false],
    ['added', false, true],
  ])('metadata key that was %s is obscured from the diff', (_, oldHasMetadata, newHasMetadata) => {
    const template = (hasMetadata: boolean) => ({
      Resources: {
        BucketResource: {
          Type: 'AWS::S3::Bucket',
          Properties: { BucketName: 'magic-bucket' },
          ...(hasMetadata ? { Metadata: { 'aws:cdk:path': '/foo/BucketResource' } } : {}),
        },
      },
    });

    const differences = fullDiff(template(oldHasMetadata), template(newHasMetadata), {});
    expect(differences.differenceCount).toBe(0);
  });

@mrgrain, should I close this now, or once #1741 lands?

[mrgrain]

Closed in favor of #1741