From fac9e56c9aef455ae90af6884ce34e1469a8d010 Mon Sep 17 00:00:00 2001
From: Ian Smith Botsford <issmith@amazon.com>
Date: Thu, 29 Jul 2021 18:10:34 +0000
Subject: [PATCH 1/2] Flag enum insertions as errors

When calculating Smithy diffs, enum value insertions should be flagged
as errors because they can cause backwards incompatibility concerns when
enum ordinals are used for iterating, serialization, etc. Enum values
appended to the end of the existing set of values should not be errors.
---
 .../diff/evaluators/ChangedEnumTrait.java     | 20 +++++--
 .../diff/evaluators/ChangedEnumTraitTest.java | 60 ++++++++++++++++++-
 2 files changed, 75 insertions(+), 5 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 8ccc9ad9f0c..0ff85339b4c 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
@@ -29,8 +29,9 @@
 import software.amazon.smithy.utils.Pair;
 
 /**
- * Emits a NOTE when a new enum value is added, emits an ERROR when an
- * enum value is removed, and emits an ERROR when an enum name changes.
+ * Emits a NOTE when a new enum value is appended, emits an ERROR when an
+ * enum value is removed, emits an ERROR when an enum name changes, and
+ * emits an ERROR when a new enum value is inserted.
  */
 public final class ChangedEnumTrait extends AbstractDiffEvaluator {
     @Override
@@ -46,6 +47,7 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
         EnumTrait oldTrait = trait.getLeft();
         EnumTrait newTrait = trait.getRight();
         List<ValidationEvent> events = new ArrayList<>();
+        int oldEndPosition = oldTrait.getValues().size() - 1;
 
         for (EnumDefinition definition : oldTrait.getValues()) {
             Optional<EnumDefinition> maybeNewValue = newTrait.getValues().stream()
@@ -55,6 +57,7 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
             if (!maybeNewValue.isPresent()) {
                 events.add(error(change.getNewShape(), String.format(
                         "Enum value `%s` was removed", definition.getValue())));
+                oldEndPosition--;
             } else {
                 EnumDefinition newValue = maybeNewValue.get();
                 if (!newValue.getName().equals(definition.getName())) {
@@ -67,11 +70,20 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
             }
         }
 
+        int newPosition = 0;
         for (EnumDefinition definition : newTrait.getValues()) {
             if (!oldTrait.getEnumDefinitionValues().contains(definition.getValue())) {
-                events.add(note(change.getNewShape(), String.format(
-                        "Enum value `%s` was added", definition.getValue())));
+                if (newPosition <= oldEndPosition) {
+                    events.add(error(change.getNewShape(), String.format(
+                            "Enum value `%s` was inserted. This can cause compatibility issues when ordinal values are "
+                                    + "used for iteration, serialization, etc.", definition.getValue())));
+                } else {
+                    events.add(note(change.getNewShape(), String.format(
+                            "Enum value `%s` was appended", definition.getValue())));
+                }
             }
+
+            newPosition++;
         }
 
         return events;
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 e75690686ce..3dba083834d 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
@@ -17,6 +17,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.stringContainsInOrder;
 
 import java.util.List;
 import org.junit.jupiter.api.Test;
@@ -30,7 +31,7 @@
 
 public class ChangedEnumTraitTest {
     @Test
-    public void detectsAddedEnums() {
+    public void detectsAppendedEnums() {
         StringShape s1 = StringShape.builder()
                 .id("foo.baz#Baz")
                 .addTrait(EnumTrait.builder()
@@ -96,4 +97,61 @@ public void detectsRenamedEnums() {
         assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").size(), equalTo(1));
         assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
     }
+
+    @Test
+    public void detectsInsertedEnums() {
+        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("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").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedEnumTrait").get(0).getSeverity(), equalTo(Severity.ERROR));
+    }
+
+    @Test
+    public void detectsAppendedEnumsAfterRemovedEnums() {
+        StringShape s1 = StringShape.builder()
+                .id("foo.baz#Baz")
+                .addTrait(EnumTrait.builder()
+                        .addEnum(EnumDefinition.builder().value("old1").build())
+                        .addEnum(EnumDefinition.builder().value("old2").build())
+                        .addEnum(EnumDefinition.builder().value("old3").build())
+                        .build())
+                .build();
+        StringShape s2 = StringShape.builder()
+                .id("foo.baz#Baz")
+                .addTrait(EnumTrait.builder()
+                        .addEnum(EnumDefinition.builder().value("old1").build())
+                        .addEnum(EnumDefinition.builder().value("old3").build())
+                        .addEnum(EnumDefinition.builder().value("new1").build())
+                        .build())
+                .build();
+        Model modelA = Model.assembler().addShape(s1).assemble().unwrap();
+        Model modelB = Model.assembler().addShape(s2).assemble().unwrap();
+        List<ValidationEvent> allEvents = ModelDiff.compare(modelA, modelB);
+
+        List<ValidationEvent> changeEvents = TestHelper.findEvents(allEvents, "ChangedEnumTrait");
+        assertThat(changeEvents.size(), equalTo(2));
+
+        ValidationEvent removedEvent = changeEvents.get(0);
+        assertThat(removedEvent.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(removedEvent.getMessage(), stringContainsInOrder("Enum value `old2` was removed"));
+
+        ValidationEvent appendedEvent = changeEvents.get(1);
+        assertThat(appendedEvent.getSeverity(), equalTo(Severity.NOTE));
+        assertThat(appendedEvent.getMessage(), stringContainsInOrder("Enum value `new1` was appended"));
+    }
 }

From c3fb83e0393de7199cbcb225519ffd0762c48700 Mon Sep 17 00:00:00 2001
From: Ian Smith Botsford <issmith@amazon.com>
Date: Fri, 30 Jul 2021 21:30:11 +0000
Subject: [PATCH 2/2] Clarify meaning of 'inserted' to be 'inserted before the
 end of the list of existing values'

---
 .../amazon/smithy/diff/evaluators/ChangedEnumTrait.java   | 8 +++++---
 1 file changed, 5 insertions(+), 3 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 0ff85339b4c..6bffe92ef85 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
@@ -31,7 +31,8 @@
 /**
  * Emits a NOTE when a new enum value is appended, emits an ERROR when an
  * enum value is removed, emits an ERROR when an enum name changes, and
- * emits an ERROR when a new enum value is inserted.
+ * emits an ERROR when a new enum value is inserted before the end of the
+ * list of existing values.
  */
 public final class ChangedEnumTrait extends AbstractDiffEvaluator {
     @Override
@@ -75,8 +76,9 @@ private List<ValidationEvent> validateEnum(ChangedShape<Shape> change, Pair<Enum
             if (!oldTrait.getEnumDefinitionValues().contains(definition.getValue())) {
                 if (newPosition <= oldEndPosition) {
                     events.add(error(change.getNewShape(), String.format(
-                            "Enum value `%s` was inserted. This can cause compatibility issues when ordinal values are "
-                                    + "used for iteration, serialization, etc.", definition.getValue())));
+                            "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())));
                 } else {
                     events.add(note(change.getNewShape(), String.format(
                             "Enum value `%s` was appended", definition.getValue())));