Fix 1-2 command, add tests, reduce diff noise

smithy-lang · Sep 11, 2022 · 1f0da40 · 1f0da40
1 parent 0bf9630
commit 1f0da40
Show file tree

Hide file tree

Showing 26 changed files with 282 additions and 94 deletions.
diff --git a/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java b/smithy-cli/src/main/java/software/amazon/smithy/cli/commands/Upgrade1to2Command.java
@@ -48,7 +48,6 @@
 import software.amazon.smithy.model.loader.ModelAssembler;
 import software.amazon.smithy.model.loader.Prelude;
 import software.amazon.smithy.model.shapes.MemberShape;
-import software.amazon.smithy.model.shapes.NumberShape;
 import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeType;
 import software.amazon.smithy.model.shapes.ShapeVisitor;
@@ -272,6 +271,8 @@ String getModelString() {
         protected Void getDefault(Shape shape) {
             if (shape.hasTrait(BoxTrait.class)) {
                 writer.eraseTrait(shape, shape.expectTrait(BoxTrait.class));
+            } else if (hasSyntheticDefault(shape)) {
+                addDefault(shape, shape.getType());
             }
             // Handle members in reverse definition order.
             shape.members().stream()
@@ -282,31 +283,15 @@ protected Void getDefault(Shape shape) {
 
         private void handleMemberShape(MemberShape shape) {
             if (hasSyntheticDefault(shape)) {
-                SourceLocation memberLocation = shape.getSourceLocation();
-                String padding = "";
-                if (memberLocation.getColumn() > 1) {
-                    padding = StringUtils.repeat(' ', memberLocation.getColumn() - 1);
-                }
-                Shape target = completeModel.expectShape(shape.getTarget());
-                String defaultValue = "";
-                if (target.isBooleanShape()) {
-                    defaultValue = "false";
-                } else if (target instanceof NumberShape) {
-                    defaultValue = "0";
-                } else if (target.isBlobShape() || target.isStringShape()) {
-                    defaultValue = "\"\"";
-                } else {
-                    throw new UnsupportedOperationException("Unexpected default: " + target);
-                }
-                writer.insertLine(shape.getSourceLocation().getLine(), padding + "@default(" + defaultValue + ")");
+                addDefault(shape, completeModel.expectShape(shape.getTarget()).getType());
             }
 
             if (shape.hasTrait(BoxTrait.class)) {
                 writer.eraseTrait(shape, shape.expectTrait(BoxTrait.class));
             }
         }
 
-        private boolean hasSyntheticDefault(MemberShape shape) {
+        private boolean hasSyntheticDefault(Shape shape) {
             Optional<SourceLocation> defaultLocation = shape.getTrait(DefaultTrait.class)
                     .map(Trait::getSourceLocation);
             // When Smithy injects the default trait, it sets the source
@@ -316,6 +301,40 @@ private boolean hasSyntheticDefault(MemberShape shape) {
             return defaultLocation.filter(location -> shape.getSourceLocation().equals(location)).isPresent();
         }
 
+        private void addDefault(Shape shape, ShapeType targetType) {
+            SourceLocation memberLocation = shape.getSourceLocation();
+            String padding = "";
+            if (memberLocation.getColumn() > 1) {
+                padding = StringUtils.repeat(' ', memberLocation.getColumn() - 1);
+            }
+            String defaultValue = "";
+            // Boxed members get a null default.
+            if (shape.hasTrait(BoxTrait.class)) {
+                defaultValue = "null";
+            } else {
+                switch (targetType) {
+                    case BOOLEAN:
+                        defaultValue = "false";
+                        break;
+                    case BYTE:
+                    case SHORT:
+                    case INTEGER:
+                    case LONG:
+                    case FLOAT:
+                    case DOUBLE:
+                        defaultValue = "0";
+                        break;
+                    case BLOB:
+                    case STRING:
+                        defaultValue = "\"\"";
+                        break;
+                    default:
+                        throw new UnsupportedOperationException("Unexpected default: " + targetType);
+                }
+            }
+            writer.insertLine(shape.getSourceLocation().getLine(), padding + "@default(" + defaultValue + ")");
+        }
+
         @Override
         public Void memberShape(MemberShape shape) {
             // members are handled from their containers so that they can

diff --git a/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/Upgrade1to2CommandTest.java b/smithy-cli/src/test/java/software/amazon/smithy/cli/commands/Upgrade1to2CommandTest.java
@@ -31,6 +31,7 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
@@ -51,7 +52,10 @@ public void testUpgrade(Path initialPath, String name) {
         Model model = Model.assembler().addImport(initialPath).assemble().unwrap();
         String actual = new Upgrade1to2Command("smithy").upgradeFile(model, initialPath);
         String expected = IoUtils.readUtf8File(expectedPath);
-        assertThat(actual, equalTo(expected));
+
+        if (!actual.equals(expected)) {
+            Assertions.fail("Expected models to be equal:\n\nActual:\n\n" + actual + "\n\nExpected:\n\n" + expected);
+        }
     }
 
     public static Stream<Arguments> source() throws Exception {

diff --git a/...hy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v1.smithy b/...hy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v1.smithy
@@ -0,0 +1,39 @@
+$version: "1.0"
+
+namespace com.example
+
+@box
+integer BoxedInteger
+
+integer NonBoxedInteger
+
+structure StructureWithOptionalString {
+    boxedTarget: BoxedInteger,
+
+    @box
+    boxedMember: NonBoxedInteger,
+}
+
+union BoxyUnion {
+    boxedTarget: BoxedInteger,
+
+    @box
+    boxedMember: NonBoxedInteger,
+}
+
+list BadSparseList {
+    @box
+    member: NonBoxedInteger,
+}
+
+set BadSparseSet {
+    @box
+    member: NonBoxedInteger,
+}
+
+map BadSparseMap {
+    key: String,
+
+    @box
+    value: NonBoxedInteger,
+}
diff --git a/...hy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v2.smithy b/...hy-cli/src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/box.v2.smithy
@@ -0,0 +1,35 @@
+$version: "2.0"
+
+namespace com.example
+
+integer BoxedInteger
+
+@default(0)
+integer NonBoxedInteger
+
+structure StructureWithOptionalString {
+    boxedTarget: BoxedInteger,
+
+    @default(null)
+    boxedMember: NonBoxedInteger,
+}
+
+union BoxyUnion {
+    boxedTarget: BoxedInteger,
+
+    boxedMember: NonBoxedInteger,
+}
+
+list BadSparseList {
+    member: NonBoxedInteger,
+}
+
+set BadSparseSet {
+    member: NonBoxedInteger,
+}
+
+map BadSparseMap {
+    key: String,
+
+    value: NonBoxedInteger,
+}
diff --git a/.../src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/primitive.v2.smithy b/.../src/test/resources/software/amazon/smithy/cli/commands/upgrade/cases/primitive.v2.smithy
@@ -25,5 +25,6 @@ structure PrimitiveBearer {
     @default(0)
     handlesRequired: PrimitiveLong,
 
+    @default(null)
     handlesBox: PrimitiveByte,
 }
diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedDefault.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedDefault.java
@@ -77,7 +77,7 @@ private void validateChange(
                 }
             }
         } else if (!oldTrait.toNode().equals(newTrait.toNode())) {
-            if (change.getNewShape().asMemberShape().isPresent()) {
+            if (change.getNewShape().isMemberShape()) {
                 evaluateChangedTrait(model, change.getNewShape().asMemberShape().get(), oldTrait, newTrait, events);
             } else {
                 events.add(error(change.getNewShape(), newTrait,

diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedDefaultTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedDefaultTest.java
@@ -1,6 +1,7 @@
 package software.amazon.smithy.diff.evaluators;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 
 import java.util.List;
@@ -114,6 +115,30 @@ public void errorWhenDefaultIsAddedToMemberWithNoAddedDefault() {
         assertThat(TestHelper.findEvents(events, "ChangedNullability").size(), equalTo(1));
     }
 
+    @Test
+    public void updateModelWithAddedDefault() {
+        String originalModel =
+                "$version: \"2\"\n"
+                + "namespace smithy.example\n"
+                + "structure Foo {\n"
+                + "    @required\n"
+                + "    bar: Integer\n"
+                + "}\n";
+        String updatedModel =
+                "$version: \"2\"\n"
+                + "namespace smithy.example\n"
+                + "structure Foo {\n"
+                + "    @addedDefault\n"
+                + "    bar: Integer = 1\n"
+                + "}\n";
+        Model modelA = Model.assembler().addUnparsedModel("test.smithy", originalModel).assemble().unwrap();
+        Model modelB = Model.assembler().addUnparsedModel("test.smithy", updatedModel).assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedDefault"), empty());
+        assertThat(TestHelper.findEvents(events, "ChangedNullability"), empty());
+    }
+
     @Test
     public void errorWhenDefaultChangesFromZeroToNonZeroValue() {
         String originalModel =
@@ -179,4 +204,48 @@ public void addingTheDefaultTraitToNullableMemberEmitsNoEvents() {
         assertThat(TestHelper.findEvents(events, "ChangedDefault").size(), equalTo(0));
         assertThat(TestHelper.findEvents(events, "ChangedNullability").size(), equalTo(0));
     }
+
+    @Test
+    public void changingFromNullDefaultToOneIsBreaking() {
+        String originalModel =
+                "$version: \"2\"\n"
+                + "namespace smithy.example\n"
+                + "structure Foo {\n"
+                + "    bar: Integer = null\n"
+                + "}\n";
+        String updatedModel =
+                "$version: \"2\"\n"
+                + "namespace smithy.example\n"
+                + "structure Foo {\n"
+                + "    bar: Integer = 1\n"
+                + "}\n";
+        Model modelA = Model.assembler().addUnparsedModel("test.smithy", originalModel).assemble().unwrap();
+        Model modelB = Model.assembler().addUnparsedModel("test.smithy", updatedModel).assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedDefault").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedNullability").size(), equalTo(1));
+    }
+
+    @Test
+    public void changingFromNullDefaultToZeroIsBreaking() {
+        String originalModel =
+                "$version: \"2\"\n"
+                + "namespace smithy.example\n"
+                + "structure Foo {\n"
+                + "    bar: Integer = null\n"
+                + "}\n";
+        String updatedModel =
+                "$version: \"2\"\n"
+                + "namespace smithy.example\n"
+                + "structure Foo {\n"
+                + "    bar: Integer = 0\n"
+                + "}\n";
+        Model modelA = Model.assembler().addUnparsedModel("test.smithy", originalModel).assemble().unwrap();
+        Model modelB = Model.assembler().addUnparsedModel("test.smithy", updatedModel).assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedDefault").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedNullability").size(), equalTo(1));
+    }
 }
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderShapeMap.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderShapeMap.java
@@ -363,7 +363,7 @@ private Shape buildShape(
     ) {
         try {
             AbstractShapeBuilder<?, ?> builder = defineShape.builder();
-            ModelUpgrader.patchShapeBeforeBuilding(defineShape, builder, events);
+            ModelInteropTransformer.patchShapeBeforeBuilding(defineShape, builder, events);
 
             for (MemberShape.Builder memberBuilder : defineShape.memberBuilders().values()) {
                 for (ShapeModifier modifier : defineShape.modifiers()) {

diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelAssembler.java
@@ -555,8 +555,8 @@ public ValidatedResult<Model> assemble() {
         }
 
         // Do the 1.0 -> 2.0 transform before full-model validation.
-        ValidatedResult<Model> transformed = new ModelUpgrader(processedModel, events, processor::getShapeVersion)
-                .transform();
+        ValidatedResult<Model> transformed = new ModelInteropTransformer(processedModel, events,
+                                                                         processor::getShapeVersion).transform();
 
         if (disableValidation
                 || !transformed.getResult().isPresent()

diff --git a/...on/smithy/model/loader/ModelUpgrader.java → ...model/loader/ModelInteropTransformer.java b/...on/smithy/model/loader/ModelUpgrader.java → ...model/loader/ModelInteropTransformer.java
@@ -19,7 +19,6 @@
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
-import java.util.Set;
 import java.util.function.Function;
 import software.amazon.smithy.model.FromSourceLocation;
 import software.amazon.smithy.model.Model;
@@ -44,15 +43,21 @@
 import software.amazon.smithy.model.transform.ModelTransformer;
 import software.amazon.smithy.model.validation.ValidatedResult;
 import software.amazon.smithy.model.validation.ValidationEvent;
-import software.amazon.smithy.utils.SetUtils;
 
 /**
- * Upgrades Smithy models from IDL v1 to IDL v2, specifically taking into
- * account the removal of the box trait, the change in default value
- * semantics of numbers and booleans, and the @default trait.
+ * Ensures interoperability between IDL 1 and IDL 2 models by adding default traits and synthetic box traits where
+ * needed to normalize across versions.
+ *
+ * <p>For 1 to 2 interop, this class adds default traits. If a root level v1 shape is not boxed and supports the box
+ * trait, a default trait is added set to the zero value of the type. If a v1 member is marked with the box trait,
+ * then a default trait is added to the member set to null. If a member is targets a streaming blob and is not
+ * required, the default trait is added set to an empty string.
+ *
+ * <p>For 2 to 1 interop, if a v2 member has a default set to null a synthetic box trait is added. If a root level
+ * does not have the default trait and the type supports the box trait, a synthetic box trait is added.
  */
 @SuppressWarnings("deprecation")
-final class ModelUpgrader {
+final class ModelInteropTransformer {
 
     /** Shape types in Smithy 1.0 that had a default value. */
     private static final EnumSet<ShapeType> HAD_DEFAULT_VALUE_IN_1_0 = EnumSet.of(
@@ -64,20 +69,12 @@ final class ModelUpgrader {
             ShapeType.DOUBLE,
             ShapeType.BOOLEAN);
 
-    private static final Set<ShapeType> SUPPORTS_RANGE_TRAIT_AND_ZERO_VALUE = SetUtils.of(
-            ShapeType.BYTE,
-            ShapeType.SHORT,
-            ShapeType.INTEGER,
-            ShapeType.LONG,
-            ShapeType.FLOAT,
-            ShapeType.DOUBLE);
-
     private final Model model;
     private final List<ValidationEvent> events;
     private final Function<Shape, Version> fileToVersion;
     private final List<Shape> shapeUpgrades = new ArrayList<>();
 
-    ModelUpgrader(Model model, List<ValidationEvent> events, Function<Shape, Version> fileToVersion) {
+    ModelInteropTransformer(Model model, List<ValidationEvent> events, Function<Shape, Version> fileToVersion) {
         this.model = model;
         this.events = events;
         this.fileToVersion = fileToVersion;
@@ -123,24 +120,23 @@ private void upgradeV1Member(MemberShape member, Shape target) {
         } else if (member.hasTrait(BoxTrait.class)) {
             // Add a default trait to the member set to null to indicate it was boxed in v1.
             MemberShape.Builder builder = member.toBuilder();
-            builder.addTrait(new DefaultTrait(Node.nullNode()));
+            builder.addTrait(new DefaultTrait(new NullNode(member.getSourceLocation())));
             shapeUpgrades.add(builder.build());
         }
     }
 
     private boolean shouldV1MemberHaveDefaultTrait(MemberShape member, Shape target) {
         // Only when the targeted shape had a default value by default in v1 or if
-        // the member has the http payload trait and targets a streaming blob, which
-        // implies a default in 2.0
-        return (HAD_DEFAULT_VALUE_IN_1_0.contains(target.getType()) || isDefaultPayload(member, target))
+        // the member targets a streaming blob, which implies a default in 2.0
+        return (HAD_DEFAULT_VALUE_IN_1_0.contains(target.getType()) || streamingBlobNeedsDefault(member, target))
             // Don't re-add the @default trait
             && !member.hasTrait(DefaultTrait.ID)
             // Don't add a @default trait if the member or target are considered boxed in v1.
             && memberAndTargetAreNotAlreadyExplicitlyBoxed(member, target);
     }
 
-    private boolean isDefaultPayload(MemberShape member, Shape target) {
-        // httpPayload requires that the member is required or default.
+    private boolean streamingBlobNeedsDefault(MemberShape member, Shape target) {
+        // Streaming blobs require that the member is required or default.
         // No need to add the default trait if the member is required.
         if (member.isRequired()) {
             return false;