Uses enum index instead of enum value, fixed bug when multiple enums …

…were inserted
smithy-lang · Jun 8, 2023 · cbe85ab · cbe85ab
1 parent 7e37611
commit cbe85ab
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 31 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
@@ -73,7 +73,8 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
         List<ValidationEvent> events = new ArrayList<>();
         int oldEndPosition = oldTrait.getValues().size() - 1;
 
-        for (EnumDefinition definition : oldTrait.getValues()) {
+        for (int enumIndex = 0; enumIndex < oldTrait.getValues().size(); enumIndex++) {
+            EnumDefinition definition = oldTrait.getValues().get(enumIndex);
             Optional<EnumDefinition> maybeNewValue = newTrait.getValues().stream()
                     .filter(d -> d.getValue().equals(definition.getValue()))
                     .findFirst();
@@ -84,7 +85,7 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
                                 .severity(Severity.ERROR)
                                 .message(String.format("Enum value `%s` was removed", definition.getValue()))
                                 .shape(change.getNewShape())
-                                .id(getEventId() + REMOVED + definition.getValue())
+                                .id(getEventId() + REMOVED + enumIndex)
                                 .build()
                 );
                 oldEndPosition--;
@@ -100,7 +101,7 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
                                             newValue.getName().orElse(null),
                                             definition.getValue()))
                                     .shape(change.getNewShape())
-                                    .id(getEventId() + NAME_CHANGED + definition.getValue())
+                                    .id(getEventId() + NAME_CHANGED + enumIndex)
                                     .build()
                     );
                 }
@@ -119,9 +120,10 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
                                             + "can cause compatibility issues when ordinal values are used for "
                                             + "iteration, serialization, etc.", definition.getValue()))
                                     .shape(change.getNewShape())
-                                    .id(getEventId() + ORDER_CHANGED + definition.getValue())
+                                    .id(getEventId() + ORDER_CHANGED + newPosition)
                                     .build()
                     );
+                    oldEndPosition++;
                 } 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
@@ -77,8 +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.NameChanged").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged").get(0).getSeverity(),
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").get(0).getSeverity(),
                 equalTo(Severity.ERROR));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(1).getSeverity(),
                 equalTo(Severity.NOTE));
@@ -116,6 +116,7 @@ public void detectsRemovedEnums() {
                 .addTrait(EnumTrait.builder()
                         .addEnum(EnumDefinition.builder().value("foo").build())
                         .addEnum(EnumDefinition.builder().value("baz").build())
+                        .addEnum(EnumDefinition.builder().value("bat").build())
                         .build())
                 .build();
         StringShape s2 = StringShape.builder()
@@ -128,8 +129,9 @@ public void detectsRemovedEnums() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.baz").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.2").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(2));
     }
 
     @Test
@@ -153,8 +155,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.baz").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
     }
@@ -182,8 +184,8 @@ public void detectsRemovedEnumsEnumTraitWithNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.baz").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.baz").stream()
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
     }
 
@@ -192,21 +194,24 @@ public void detectsRenamedEnums() {
         StringShape s1 = StringShape.builder()
                 .id("foo.baz#Baz")
                 .addTrait(EnumTrait.builder()
-                        .addEnum(EnumDefinition.builder().value("foo").name("OLD").build())
+                        .addEnum(EnumDefinition.builder().value("foo").name("OLD1").build())
+                        .addEnum(EnumDefinition.builder().value("baz").name("OLD2").build())
                         .build())
                 .build();
         StringShape s2 = StringShape.builder()
                 .id("foo.baz#Baz")
                 .addTrait(EnumTrait.builder()
-                        .addEnum(EnumDefinition.builder().value("foo").name("NEW").build())
+                        .addEnum(EnumDefinition.builder().value("foo").name("NEW1").build())
+                        .addEnum(EnumDefinition.builder().value("baz").name("NEW2").build())
                         .build())
                 .build();
         Model modelA = Model.assembler().addShape(s1).assemble().unwrap();
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.1").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(2));
     }
 
     @Test
@@ -227,8 +232,8 @@ public void detectsRenamedEnumsEnumTraitNoNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").get(0).getSeverity(),
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").get(0).getSeverity(),
                 equalTo(Severity.ERROR));
     }
 
@@ -251,8 +256,8 @@ public void detectsRenamedEnumsEnumTraitWithNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.foo").get(0).getSeverity(),
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").get(0).getSeverity(),
                 equalTo(Severity.ERROR));
     }
 
@@ -268,15 +273,43 @@ public void detectsInsertedEnums() {
                 .id("foo.baz#Baz")
                 .addTrait(EnumTrait.builder()
                         .addEnum(EnumDefinition.builder().value("baz").build())
+                        .addEnum(EnumDefinition.builder().value("bat").build())
                         .addEnum(EnumDefinition.builder().value("foo").build())
                         .build())
                 .build();
         Model modelA = Model.assembler().addShape(s1).assemble().unwrap();
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.baz").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.baz").get(0).getSeverity(), equalTo(Severity.ERROR));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.1").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(2));
+    }
+
+    @Test
+    public void detectsInsertedEnumsBeforeAppendedEnums() {
+        StringShape s1 = StringShape.builder()
+                .id("foo.baz#Baz")
+                .addTrait(EnumTrait.builder()
+                        .addEnum(EnumDefinition.builder().value("foo").build())
+                        .build())
+                .build();
+        StringShape s2 = StringShape.builder()
+                .id("foo.baz#Baz")
+                .addTrait(EnumTrait.builder()
+                        .addEnum(EnumDefinition.builder().value("baz").build())
+                        .addEnum(EnumDefinition.builder().value("bat").build())
+                        .addEnum(EnumDefinition.builder().value("foo").build())
+                        .addEnum(EnumDefinition.builder().value("bar").build())
+                        .build())
+                .build();
+        Model modelA = Model.assembler().addShape(s1).assemble().unwrap();
+        Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.1").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(2));
     }
 
     @Test
@@ -299,7 +332,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.baz").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").size(), equalTo(1));
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
     }
@@ -324,8 +357,8 @@ public void detectsInsertedEnumsEnumTraitWithNameToEnumShape() {
         Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.baz").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.baz").stream()
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.OrderChanged.0").stream()
                 .allMatch(e -> e.getSeverity() == Severity.ERROR), equalTo(true));
     }
 
@@ -353,7 +386,7 @@ public void detectsAppendedEnumsAfterRemovedEnums() {
 
         List<ValidationEvent> changeEvents = TestHelper.findEvents(allEvents, "ChangedEnumTrait");
         assertThat(changeEvents.size(), equalTo(2));
-        assertThat(TestHelper.findEvents(changeEvents, "ChangedEnumTrait.Removed.old2").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(changeEvents, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
 
         ValidationEvent removedEvent = changeEvents.get(0);
         assertThat(removedEvent.getSeverity(), equalTo(Severity.ERROR));
@@ -391,9 +424,9 @@ public void detectsAppendedEnumsAfterRemovedEnumsEnumTraitNoNameToEnumShape() {
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(4));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.old1").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.old2").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.old3").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.0").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.NameChanged.2").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()
@@ -430,8 +463,8 @@ public void detectsAppendedEnumsAfterRemovedEnumsEnumTraitWithNameToEnumShape()
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(2));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.old2").size(), equalTo(1));
-        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.old2").stream()
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait.Removed.1").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));