RemovedShape events from ERROR -> NOTE if the shape is safe to remove…

… from a model.
rchache · Nov 9, 2023 · 88238e5 · 88238e5
1 parent 0ba148d
commit 88238e5
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 5 deletions.
diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedShape.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedShape.java
@@ -19,19 +19,23 @@
 import java.util.stream.Collectors;
 import software.amazon.smithy.diff.Differences;
 import software.amazon.smithy.model.shapes.Shape;
+import software.amazon.smithy.model.traits.EnumTrait;
 import software.amazon.smithy.model.traits.PrivateTrait;
 import software.amazon.smithy.model.validation.ValidationEvent;
 
 /**
- * Creates an ERROR event when a non-private shape is removed.
+ * Creates an ERROR event when a non-private non-scalar shape is removed.
+ * Creates a NOTE event when a non-private scalar shape is removed.
  */
 public final class RemovedShape extends AbstractDiffEvaluator {
     @Override
     public List<ValidationEvent> evaluate(Differences differences) {
         return differences.removedShapes()
                 .filter(shape -> !shape.hasTrait(PrivateTrait.class))
                 .filter(shape -> !isMemberOfRemovedShape(shape, differences))
-                .map(shape -> error(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId())))
+                .map(shape -> isScalarType(shape)
+                        ? note(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId()))
+                        : error(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId())))
                 .collect(Collectors.toList());
     }
 
@@ -40,4 +44,19 @@ private boolean isMemberOfRemovedShape(Shape shape, Differences differences) {
                 .filter(member -> !differences.getNewModel().getShapeIds().contains(member.getContainer()))
                 .isPresent();
     }
+
+    private boolean isScalarType(Shape shape) {
+        return shape.isIntegerShape()
+            || shape.isBigDecimalShape()
+            || shape.isBigIntegerShape()
+            || shape.isBlobShape()
+            || shape.isBooleanShape()
+            || shape.isByteShape()
+            || shape.isDoubleShape()
+            || shape.isFloatShape()
+            || shape.isShortShape()
+            || shape.isTimestampShape()
+            || shape.isLongShape()
+            || (shape.isStringShape() && !shape.hasTrait(EnumTrait.class));
+    }
 }
diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/ModelDiffTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/ModelDiffTest.java
@@ -11,6 +11,7 @@
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.shapes.StringShape;
+import software.amazon.smithy.model.shapes.StructureShape;
 import software.amazon.smithy.model.traits.SensitiveTrait;
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidatedResult;
@@ -77,7 +78,7 @@ public void testsEquality() {
     @Test
     public void findsBreakingChanges() {
         Model oldModel = Model.builder()
-                .addShape(StringShape.builder().id("smithy.example#Str").build())
+                .addShape(StructureShape.builder().id("smithy.example#Str").build())
                 .build();
         Model newModel = Model.builder().build();
         ModelDiff.Result result = ModelDiff.builder().oldModel(oldModel).newModel(newModel).compare();

diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedShapeTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedShapeTest.java
@@ -23,19 +23,33 @@
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.diff.ModelDiff;
 import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.shapes.BigDecimalShape;
+import software.amazon.smithy.model.shapes.BigIntegerShape;
+import software.amazon.smithy.model.shapes.BlobShape;
+import software.amazon.smithy.model.shapes.BooleanShape;
+import software.amazon.smithy.model.shapes.ByteShape;
+import software.amazon.smithy.model.shapes.DoubleShape;
+import software.amazon.smithy.model.shapes.FloatShape;
+import software.amazon.smithy.model.shapes.IntegerShape;
 import software.amazon.smithy.model.shapes.ListShape;
+import software.amazon.smithy.model.shapes.LongShape;
 import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.Shape;
+import software.amazon.smithy.model.shapes.ShortShape;
 import software.amazon.smithy.model.shapes.StringShape;
+import software.amazon.smithy.model.shapes.StructureShape;
+import software.amazon.smithy.model.shapes.TimestampShape;
+import software.amazon.smithy.model.traits.EnumDefinition;
+import software.amazon.smithy.model.traits.EnumTrait;
 import software.amazon.smithy.model.traits.PrivateTrait;
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidationEvent;
 
 public class RemovedShapeTest {
     @Test
     public void detectsShapeRemoval() {
-        Shape shapeA1 = StringShape.builder().id("foo.baz#Baz").build();
-        Shape shapeB1 = StringShape.builder().id("foo.baz#Bam").build();
+        Shape shapeA1 = StructureShape.builder().id("foo.baz#Baz").build();
+        Shape shapeB1 = StructureShape.builder().id("foo.baz#Bam").build();
         Model modelA = Model.assembler().addShapes(shapeA1, shapeB1).assemble().unwrap();
         Model modelB = Model.assembler().addShapes(shapeB1).assemble().unwrap();
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
@@ -45,6 +59,46 @@ public void detectsShapeRemoval() {
         assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
     }
 
+    @Test
+    public void emitsNotesForScalarShapes() {
+        Shape[] scalarShapes = new Shape[] {
+                IntegerShape.builder().id("foo.baz#BazOne").build(),
+                BigDecimalShape.builder().id("foo.baz#BazTwo").build(),
+                BigIntegerShape.builder().id("foo.baz#BazThree").build(),
+                BlobShape.builder().id("foo.baz#BazFour").build(),
+                BooleanShape.builder().id("foo.baz#BazFive").build(),
+                ByteShape.builder().id("foo.baz#BazSix").build(),
+                DoubleShape.builder().id("foo.baz#BazSeven").build(),
+                FloatShape.builder().id("foo.baz#BazEight").build(),
+                ShortShape.builder().id("foo.baz#BazNine").build(),
+                TimestampShape.builder().id("foo.baz#BazTen").build(),
+                LongShape.builder().id("foo.baz#BazEleven").build(),
+                StringShape.builder().id("foo.baz#BazTwelve").build()
+        };
+        Model modelA = Model.assembler().addShapes(scalarShapes).assemble().unwrap();
+        Model modelB = Model.assembler().assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "RemovedShape").size(), equalTo(12));
+        assertThat("Scalar removals should be NOTE severity",
+                events.stream().allMatch(event -> Severity.NOTE.equals(event.getSeverity())));
+    }
+
+    @Test
+    public void emitsErrorForEnumString() {
+        Shape shapeA1 = StringShape.builder()
+                .id("foo.baz#Baz")
+                .addTrait(EnumTrait.builder().addEnum(EnumDefinition.builder().value("val").build()).build())
+                .build();
+        Model modelA = Model.assembler().addShapes(shapeA1).assemble().unwrap();
+        Model modelB = Model.assembler().addShapes().assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "RemovedShape").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, shapeA1.getId()).size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
+    }
+
     @Test
     public void doesNotEmitForPrivateShapes() {
         Shape shape = StringShape.builder().id("foo.baz#Baz").addTrait(new PrivateTrait()).build();