Repository: smithy-lang/smithy
Pull request #2037 (2023-11-09 21:11:11)

Scalar RemovedShape events from ERROR -> NOTE
*Description of changes:*
For scalar shapes (and non-enum strings), removing them is safe and therefore can be lowered from an ERROR to a NOTE

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-aws-endpoints/src/main/java/software/amazon/smithy/rulesengine/aws/validators/AwsSpecialCaseEndpointValidator.java b/smithy-aws-endpoints/src/main/java/software/amazon/smithy/rulesengine/aws/validators/AwsSpecialCaseEndpointValidator.java
index a5591c87a..5294b04f6 100644
--- a/smithy-aws-endpoints/src/main/java/software/amazon/smithy/rulesengine/aws/validators/AwsSpecialCaseEndpointValidator.java
+++ b/smithy-aws-endpoints/src/main/java/software/amazon/smithy/rulesengine/aws/validators/AwsSpecialCaseEndpointValidator.java
@@ -115,7 +115,7 @@ public final class AwsSpecialCaseEndpointValidator extends AbstractValidator {
                     "InvalidEndpointPatternScheme"));
         }
 
-        if (!isValidURL(endpoint)) {
+        if (!isValidUrl(endpoint)) {
             events.add(error(
                     serviceShape, location,
                     String.format("Endpoint `%s` should be a valid URL.",
@@ -126,16 +126,14 @@ public final class AwsSpecialCaseEndpointValidator extends AbstractValidator {
         return events;
     }
 
-    private boolean isValidURL(String endpointPattern) {
+    private boolean isValidUrl(String endpointPattern) {
         String url = endpointPattern
                 .replace("{", "")
                 .replace("}", "");
         try {
             new URL(url).toURI();
             return true;
-        } catch (MalformedURLException e) {
-            return false;
-        } catch (URISyntaxException e) {
+        } catch (MalformedURLException | URISyntaxException e) {
             return false;
         }
     }
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
index c0abb8a9d..8b4913433 100644
--- 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,11 +19,15 @@ import java.util.List;
 import java.util.stream.Collectors;
 import software.amazon.smithy.diff.Differences;
 import software.amazon.smithy.model.shapes.Shape;
+import software.amazon.smithy.model.shapes.ShapeType;
+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;
 
 /**
- * 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 WARNING event when a non-private scalar shape is removed.
  */
 public final class RemovedShape extends AbstractDiffEvaluator {
     @Override
@@ -31,7 +35,15 @@ public final class RemovedShape extends AbstractDiffEvaluator {
         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 -> isInconsequentialType(shape)
+                        ? ValidationEvent.builder()
+                            .severity(Severity.WARNING)
+                            .message(String.format("Removed %s `%s`", shape.getType(), shape.getId()))
+                            .shapeId(shape.getId())
+                            .id(getEventId() + ".ScalarShape")
+                            .sourceLocation(shape.getSourceLocation())
+                            .build()
+                        : error(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId())))
                 .collect(Collectors.toList());
     }
 
@@ -40,4 +52,20 @@ public final class RemovedShape extends AbstractDiffEvaluator {
                 .filter(member -> !differences.getNewModel().getShapeIds().contains(member.getContainer()))
                 .isPresent();
     }
+
+    private boolean isInconsequentialType(Shape shape) {
+        ShapeType shapeType = shape.getType();
+        return shapeType == ShapeType.BIG_DECIMAL
+                || shapeType == ShapeType.BIG_INTEGER
+                || shapeType == ShapeType.BLOB
+                || shapeType == ShapeType.BOOLEAN
+                || shapeType == ShapeType.BYTE
+                || shapeType == ShapeType.DOUBLE
+                || shapeType == ShapeType.FLOAT
+                || shapeType == ShapeType.SHORT
+                || shapeType == ShapeType.TIMESTAMP
+                || shapeType == ShapeType.LONG
+                || ((shapeType == ShapeType.STRING) && !shape.hasTrait(EnumTrait.class))
+                || (shapeType == ShapeType.INTEGER);
+    }
 }


Test patch:
diff --git a/smithy-aws-protocol-tests/model/awsJson1_1/main.smithy b/smithy-aws-protocol-tests/model/awsJson1_1/main.smithy
index 19e660cc3..380b2ab8a 100644
--- a/smithy-aws-protocol-tests/model/awsJson1_1/main.smithy
+++ b/smithy-aws-protocol-tests/model/awsJson1_1/main.smithy
@@ -14,7 +14,7 @@ use smithy.test#httpResponseTests
     cloudFormationName: "JsonProtocol",
     cloudTrailEventSource: "jsonprotocol.amazonaws.com",
 )
-@sigv4(name: "foo")
+@sigv4(name: "jsonprotocol")
 @awsJson1_1
 @title("Sample Json 1.1 Protocol Service")
 service JsonProtocol {
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
index cbc22a416..307dacbe9 100644
--- 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 org.junit.jupiter.api.Test;
 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 class ModelDiffTest {
     @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
index 7cae1ff63..04b187e6c 100644
--- 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,10 +23,26 @@ import java.util.List;
 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.EnumShape;
+import software.amazon.smithy.model.shapes.FloatShape;
+import software.amazon.smithy.model.shapes.IntEnumShape;
+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;
@@ -34,8 +50,8 @@ 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 +61,61 @@ public class RemovedShapeTest {
         assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1));
     }
 
+    @Test
+    public void emitsWarningsForScalarShapes() {
+        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.ScalarShape").size(), equalTo(12));
+        assertThat("Scalar removals should be WARNING severity",
+                events.stream().allMatch(event -> Severity.WARNING.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 emitsErrorForIntEnum() {
+        Shape shapeA1 = IntEnumShape.builder()
+                .id("foo.baz#Baz")
+                .addMember("FOO", 1)
+                .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();
diff --git a/smithy-openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/protocols/AwsRestJson1ProtocolTest.java b/smithy-openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/protocols/AwsRestJson1ProtocolTest.java
index 4b588f984..35209a359 100644
--- a/smithy-openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/protocols/AwsRestJson1ProtocolTest.java
+++ b/smithy-openapi/src/test/java/software/amazon/smithy/openapi/fromsmithy/protocols/AwsRestJson1ProtocolTest.java
@@ -215,6 +215,7 @@ public class AwsRestJson1ProtocolTest {
 
     @ParameterizedTest
     @MethodSource("protocolHeaderCases")
+    @SuppressWarnings("unchecked")
     public void assertProtocolHeaders(
             String operationId,
             Set<String> expectedRequestHeaders,