From 3c595cdd1149e4c8389aca6f95d7ded87b044429 Mon Sep 17 00:00:00 2001
From: rchache <105247787+rchache@users.noreply.github.com>
Date: Wed, 24 May 2023 16:38:38 -0400
Subject: [PATCH] Emit specific ids for ChangedEnumTrait ERROR events (#1787)

* Emit id 'ChangedEnumTrait.Inserted' for inserted enum diff events

* Changed .Inserted to .OrderChanged, and added subparts for .Removed and .NameChanged
---
 .../diff/evaluators/ChangedEnumTrait.java     | 47 ++++++++++++++-----
 .../diff/evaluators/ChangedEnumTraitTest.java | 39 +++++++++------
 2 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedEnumTrait.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedEnumTrait.java
index 231f02db5f0..318a00a2f03 100644
--- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedEnumTrait.java
+++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedEnumTrait.java
@@ -27,6 +27,7 @@
 import software.amazon.smithy.model.traits.EnumDefinition;
 import software.amazon.smithy.model.traits.EnumTrait;
 import software.amazon.smithy.model.traits.synthetic.SyntheticEnumTrait;
+import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidationEvent;
 import software.amazon.smithy.utils.OptionalUtils;
 import software.amazon.smithy.utils.Pair;
@@ -38,6 +39,10 @@
  * list of existing values.
  */
 public final class ChangedEnumTrait extends AbstractDiffEvaluator {
+    private static final String ORDER_CHANGED = ".OrderChanged";
+    private static final String NAME_CHANGED = ".NameChanged";
+    private static final String REMOVED = ".Removed";
+
     @Override
     public List<ValidationEvent> evaluate(Differences differences) {
         return differences.changedShapes()
@@ -74,17 +79,30 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
                     .findFirst();
 
             if (!maybeNewValue.isPresent()) {
-                events.add(error(change.getNewShape(), String.format(
-                        "Enum value `%s` was removed", definition.getValue())));
+                events.add(
+                        ValidationEvent.builder()
+                                .severity(Severity.ERROR)
+                                .message(String.format("Enum value `%s` was removed", definition.getValue()))
+                                .shape(change.getNewShape())
+                                .id(getEventId() + REMOVED)
+                                .build()
+                );
                 oldEndPosition--;
             } else {
                 EnumDefinition newValue = maybeNewValue.get();
                 if (!newValue.getName().equals(definition.getName())) {
-                    events.add(error(change.getNewShape(), String.format(
-                            "Enum `name` changed from `%s` to `%s` for the `%s` value",
-                            definition.getName().orElse(null),
-                            newValue.getName().orElse(null),
-                            definition.getValue())));
+                    events.add(
+                            ValidationEvent.builder()
+                                    .severity(Severity.ERROR)
+                                    .message(String.format(
+                                            "Enum `name` changed from `%s` to `%s` for the `%s` value",
+                                            definition.getName().orElse(null),
+                                            newValue.getName().orElse(null),
+                                            definition.getValue()))
+                                    .shape(change.getNewShape())
+                                    .id(getEventId() + NAME_CHANGED)
+                                    .build()
+                    );
                 }
             }
         }
@@ -93,10 +111,17 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
         for (EnumDefinition definition : newTrait.getValues()) {
             if (!oldTrait.getEnumDefinitionValues().contains(definition.getValue())) {
                 if (newPosition <= oldEndPosition) {
-                    events.add(error(change.getNewShape(), String.format(
-                            "Enum value `%s` was inserted before the end of the list of existing values. This can "
-                                    + "cause compatibility issues when ordinal values are used for iteration, "
-                                    + "serialization, etc.", definition.getValue())));
+                    events.add(
+                            ValidationEvent.builder()
+                                    .severity(Severity.ERROR)
+                                    .message(String.format(
+                                    "Enum value `%s` was inserted before the end of the list of existing values. This "
+                                            + "can cause compatibility issues when ordinal values are used for "
+                                            + "iteration, serialization, etc.", definition.getValue()))
+                                    .shape(change.getNewShape())
+                                    .id(getEventId() + ORDER_CHANGED)
+                                    .build()
+                    );
                 } else {
                     events.add(note(change.getNewShape(), String.format(
                             "Enum value `%s` was appended", definition.getValue())));
diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedEnumTraitTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedEnumTraitTest.java
index 93b7aa062ec..76f6d60647e 100644
--- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedEnumTraitTest.java
+++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedEnumTraitTest.java
@@ -16,10 +16,13 @@
 package software.amazon.smithy.diff.evaluators;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.stringContainsInOrder;
 
 import java.util.List;
+
+import org.hamcrest.core.Every;
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.diff.ModelDiff;
 import software.amazon.smithy.model.Model;
@@ -74,7 +77,8 @@ public void detectsAppendedEnumsEnumTraitNoNameToEnumShape() {
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(2));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(0).getSeverity(),
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
                 equalTo(Severity.ERROR));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(1).getSeverity(),
                 equalTo(Severity.NOTE));
@@ -124,7 +128,7 @@ public void detectsRemovedEnums() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
         assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
     }
 
@@ -149,6 +153,8 @@ public void detectsRemovedEnumsEnumTraitNoNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
     }
@@ -176,8 +182,8 @@ public void detectsRemovedEnumsEnumTraitWithNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
     }
 
@@ -199,7 +205,7 @@ public void detectsRenamedEnums() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
         assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
     }
 
@@ -221,8 +227,8 @@ public void detectsRenamedEnumsEnumTraitNoNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(0).getSeverity(),
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
                 equalTo(Severity.ERROR));
     }
 
@@ -245,8 +251,8 @@ public void detectsRenamedEnumsEnumTraitWithNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(0).getSeverity(),
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
                 equalTo(Severity.ERROR));
     }
 
@@ -269,8 +275,8 @@ public void detectsInsertedEnums() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(0).getSeverity(), equalTo(Severity.ERROR));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").get(0).getSeverity(), equalTo(Severity.ERROR));
     }
 
     @Test
@@ -293,6 +299,7 @@ public void detectsInsertedEnumsEnumTraitNoNameToEnumShape() {
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(2));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").size(), equalTo(1));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
     }
@@ -317,8 +324,8 @@ public void detectsInsertedEnumsEnumTraitWithNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
     }
 
@@ -346,6 +353,7 @@ public void detectsAppendedEnumsAfterRemovedEnums() {
 
         List<ValidationEvent> changeEvents = TestHelper.findEvents(allEvents, "ChangedEnumTrait");
         assertThat(changeEvents.size(), equalTo(2));
+        assertThat(TestHelper.findEvents(changeEvents, "ChangedEnumTrait.Removed").size(), equalTo(1));
 
         ValidationEvent removedEvent = changeEvents.get(0);
         assertThat(removedEvent.getSeverity(), equalTo(Severity.ERROR));
@@ -383,6 +391,8 @@ public void detectsAppendedEnumsAfterRemovedEnumsEnumTraitNoNameToEnumShape() {
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(4));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").size(), equalTo(2));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").subList(0, 3).stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").subList(3, 4).stream()
@@ -419,7 +429,8 @@ public void detectsAppendedEnumsAfterRemovedEnumsEnumTraitWithNameToEnumShape()
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(2));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").subList(0, 1).stream()
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").subList(1, 2).stream()
                 .allMatch(e -> e.getSeverity() == Severity.NOTE), equalTo(true));