Repository: smithy-lang/smithy
Pull request #2249 (2024-04-19 00:04:39)

Fix ModifiedTrait for traits with breaking change rules
## Overview

Fix ModifiedTrait for traits with breaking change rules.

Before this change, if a trait had breaking change rules defined, the diff strategy in ModifiedTrait would eventually resolve the trait as unknown and produce a ModifiedTrait warning on any change.

- Missing registering traits with breaking change rules: https://github.com/smithy-lang/smithy/blob/7c6228f351d85dab343af614181bbaa41893737e/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java#L148-L151
- Results in treating traits with breaking change rules as unknown and warn on any change: https://github.com/smithy-lang/smithy/blob/7c6228f351d85dab343af614181bbaa41893737e/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java#L117-L119

## Testing

Tested with `@jsonName`:

Model A:

```smithy
$version: "2.0"

namespace ns.foo

structure Test {
    @jsonName("a")
    member: String
}
```

Model B:

```smithy
$version: "2.0"

namespace ns.foo

structure Test {
    @jsonName("b")
    member: String
}
```

Diff events before the change (see duplicate ModifiedTrait event):

```log
[ERROR] ns.foo#Test$member: Changed trait `smithy.api#jsonName` from `a` to `b` | TraitBreakingChange.Update.smithy.api#jsonName
[WARNING] ns.foo#Test$member: Changed trait `smithy.api#jsonName` from `a` to `b` | ModifiedTrait.Update.smithy.api#jsonName
```

Diff events after the change (no duplicate ModifiedTrait event):

```log
[ERROR] ns.foo#Test$member: Changed trait `smithy.api#jsonName` from `a` to `b` | TraitBreakingChange.Update.smithy.api#jsonName
```
---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java
index ec41e2667..611d1b1ae 100644
--- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java
+++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ModifiedTrait.java
@@ -145,8 +145,10 @@ public final class ModifiedTrait extends AbstractDiffEvaluator {
             List<DiffStrategy> strategies = createStrategiesForShape(shape, true);
             if (!strategies.isEmpty()) {
                 result.put(shape.getId(), strategies);
-            } else if (definition.getBreakingChanges().isEmpty()) {
-                // Avoid duplicate validation events; only perform the default validation when there are no diff rules.
+            } else if (!definition.getBreakingChanges().isEmpty()) {
+                // Avoid duplicate validation events; delegate emitting events to TraitBreakingChange.
+                result.put(shape.getId(), Collections.emptyList());
+            } else {
                 result.put(shape.getId(), DEFAULT_STRATEGIES);
             }
         }


Test patch:
diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java
index 5e0aa9f6c..ebbac8d48 100644
--- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java
+++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ModifiedTraitTest.java
@@ -36,6 +36,7 @@ import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.shapes.StringShape;
 import software.amazon.smithy.model.traits.DynamicTrait;
+import software.amazon.smithy.model.traits.JsonNameTrait;
 import software.amazon.smithy.model.traits.RequiredTrait;
 import software.amazon.smithy.model.traits.TagsTrait;
 import software.amazon.smithy.model.traits.TraitDefinition;
@@ -366,4 +367,23 @@ public class ModifiedTraitTest {
 
         assertThat(TestHelper.findEvents(ModelDiff.compare(oldModel, newModel), "ModifiedTrait"), empty());
     }
+
+    @Test
+    public void letsTraitBreakingChangeHandleDiffEvents() {
+        String originalModel =
+            "$version: \"2.0\"\n"
+          + "namespace smithy.example\n"
+          + "structure Test {\n"
+          + "   @jsonName(\"a\")\n"
+          + "   member: String\n"
+          + "}";
+        Model oldModel = Model.assembler().addUnparsedModel("foo.smithy", originalModel).assemble().unwrap();
+        Model newModel = ModelTransformer.create().replaceShapes(oldModel, ListUtils.of(
+                Shape.shapeToBuilder(oldModel.expectShape(ShapeId.from("smithy.example#Test$member")))
+                        .removeTrait(JsonNameTrait.ID)
+                        .build()));
+
+        assertThat(TestHelper.findEvents(ModelDiff.compare(oldModel, newModel), "ModifiedTrait"), empty());
+        assertThat(TestHelper.findEvents(ModelDiff.compare(oldModel, newModel), "TraitBreakingChange"), hasSize(1));
+    }
 }
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/trait-modified-all-severities-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/trait-modified-all-severities-a.smithy
index 495e4b8ff..d1673fc35 100644
--- a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/trait-modified-all-severities-a.smithy
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/trait-modified-all-severities-a.smithy
@@ -56,4 +56,7 @@ structure aTrait {
 
     @tags(["diff.warning.const"])
     l: String,
+
+    @tags(["diff.contents"])
+    m: String,
 }
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/trait-modified-all-severities-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/trait-modified-all-severities-b.smithy
index 339dc7a5a..a73fe9dc7 100644
--- a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/trait-modified-all-severities-b.smithy
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/trait-modified-all-severities-b.smithy
@@ -56,4 +56,7 @@ structure aTrait {
 
     @tags(["diff.warning.const"])
     l: String,
+
+    @tags(["diff.contents"])
+    m: String,
 }