Add test for synthetic box traits on mixins

This commit also fixes an issue where we would sometimes add
empty apply statements to the JSON AST because a mixin introduces
no traits. The AST parser now handles this without failing, and
the model serializer no longer writes out empty apply blocks.

Author: mtdowling

smithy-model/src/main/java/software/amazon/smithy/model/loader/AstModelLoader.java

@@ -194,7 +194,7 @@ private LoadOperation.DefineShape loadShape(ShapeId id, String type, ObjectNode
                 return loadOperation(id, value);
             case "apply":
                 LoaderUtils.checkForAdditionalProperties(value, id, APPLY_PROPERTIES).ifPresent(this::emit);
-                applyTraits(id, value.expectObjectMember(TRAITS));
+                value.getObjectMember(TRAITS).ifPresent(traits -> applyTraits(id, traits));
                 return null;
             default:
                 throw new SourceException("Invalid shape `type`: " + type, value);

smithy-model/src/main/java/software/amazon/smithy/model/shapes/ModelSerializer.java

@@ -17,6 +17,7 @@
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -123,18 +124,20 @@ public ObjectNode serialize(Model model) {
             if (!shape.isMemberShape() && shapeFilter.test(shape)) {
                 Node value = shape.accept(shapeSerializer);
                 shapes.put(Node.from(shape.getId().toString()), value);
-                // Add any necessary apply statements to inherited mixin members that added traits.
-                // Apply statements are used here instead of redefining members on structures because
-                // apply statements are more resilient to change over time if the shapes targeted by
+                // Add any necessary apply statements to inherited mixin members that added traits, but only if there
+                // are actually traits to serialize. Apply statements are used here instead of redefining members on
+                // structures because apply statements are more resilient to change over time if the shapes targeted by
                 // an inherited member changes.
                 if (!shapeSerializer.mixinMemberTraits.isEmpty()) {
                     for (MemberShape member : shapeSerializer.mixinMemberTraits) {
-                        ObjectNode.Builder applyBuilder = Node.objectNodeBuilder();
-                        applyBuilder.withMember("type", "apply");
-                        shapes.put(
-                            Node.from(member.getId().toString()),
-                            serializeTraits(applyBuilder, member.getIntroducedTraits().values()).build()
-                        );
+                        Map<StringNode, Node> introducedTraits = createIntroducedTraitsMap(
+                                member.getIntroducedTraits().values());
+                        if (!introducedTraits.isEmpty()) {
+                            ObjectNode.Builder applyBuilder = Node.objectNodeBuilder();
+                            applyBuilder.withMember("type", "apply");
+                            ObjectNode traits = serializeTraits(applyBuilder, introducedTraits).build();
+                            shapes.put(Node.from(member.getId().toString()), traits);
+                        }
                     }
                 }
             }
@@ -258,20 +261,29 @@ public ModelSerializer build() {
     }
 
     private ObjectNode.Builder serializeTraits(ObjectNode.Builder builder, Collection<Trait> traits) {
+        return serializeTraits(builder, createIntroducedTraitsMap(traits));
+    }
+
+    private ObjectNode.Builder serializeTraits(ObjectNode.Builder builder, Map<StringNode, Node> traits) {
         if (!traits.isEmpty()) {
+            builder.withMember("traits", new ObjectNode(traits, SourceLocation.none()));
+        }
+
+        return builder;
+    }
+
+    private Map<StringNode, Node> createIntroducedTraitsMap(Collection<Trait> traits) {
+        if (traits.isEmpty()) {
+            return Collections.emptyMap();
+        } else {
             Map<StringNode, Node> traitsToAdd = new TreeMap<>();
             for (Trait trait : traits) {
                 if (traitFilter.test(trait)) {
                     traitsToAdd.put(Node.from(trait.toShapeId().toString()), trait.toNode());
                 }
             }
-
-            if (!traitsToAdd.isEmpty()) {
-                builder.withMember("traits", new ObjectNode(traitsToAdd, SourceLocation.none()));
-            }
+            return traitsToAdd;
         }
-
-        return builder;
     }
 
     private final class ShapeSerializer extends ShapeVisitor.Default<Node> {

smithy-model/src/test/java/software/amazon/smithy/model/loader/AstModelLoaderTest.java

@@ -15,15 +15,14 @@
 
 package software.amazon.smithy.model.loader;
 
-import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import org.junit.jupiter.api.Test;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidatedResult;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 public class AstModelLoaderTest {
     @Test
     public void failsToLoadPropertiesFromV1() {
@@ -34,4 +33,14 @@ public void failsToLoadPropertiesFromV1() {
         assertTrue(model.getValidationEvents(Severity.ERROR).get(0).getMessage()
                 .contains("Resource properties can only be used with Smithy version 2 or later."));
     }
+
+    @Test
+    public void doesNotFailOnEmptyApply() {
+        // Empty apply statements are pointless but shouldn't break the loader.
+        Model.assembler()
+                .addImport(getClass().getResource("ast-empty-apply-1.json"))
+                .addImport(getClass().getResource("ast-empty-apply-2.json"))
+                .assemble()
+                .unwrap();
+    }
 }

smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java

@@ -55,6 +55,7 @@
 import software.amazon.smithy.model.node.ObjectNode;
 import software.amazon.smithy.model.node.StringNode;
 import software.amazon.smithy.model.shapes.MemberShape;
+import software.amazon.smithy.model.shapes.ModelSerializer;
 import software.amazon.smithy.model.shapes.ServiceShape;
 import software.amazon.smithy.model.shapes.SetShape;
 import software.amazon.smithy.model.shapes.Shape;
@@ -1079,4 +1080,32 @@ public void syntheticBoxingResultsInSameModelBetween1and2() {
         assertThat(model1, equalTo(model2));
         assertThat(model1, equalTo(model3));
     }
+
+    @Test
+    public void appliesBoxTraitsToMixinsToo() {
+        Model model1 = Model.assembler()
+                .addImport(getClass().getResource("synthetic-boxing-mixins.smithy"))
+                .assemble()
+                .unwrap();
+
+        // MixedInteger and MixinInteger have synthetic box traits.
+        assertThat(model1.expectShape(ShapeId.from("smithy.example#MixinInteger")).hasTrait(BoxTrait.class), is(true));
+        assertThat(model1.expectShape(ShapeId.from("smithy.example#MixedInteger")).hasTrait(BoxTrait.class), is(true));
+
+        // MixinStruct$bar and MixedStruct$bar have synthetic box traits.
+        StructureShape mixinStruct = model1.expectShape(ShapeId.from("smithy.example#MixinStruct"),
+                                                        StructureShape.class);
+        StructureShape mixedStruct = model1.expectShape(ShapeId.from("smithy.example#MixedStruct"),
+                                                        StructureShape.class);
+        assertThat(mixinStruct.getAllMembers().get("bar").hasTrait(BoxTrait.class), is(true));
+        assertThat(mixinStruct.getAllMembers().get("bar").hasTrait(DefaultTrait.class), is(true));
+        assertThat(mixedStruct.getAllMembers().get("bar").hasTrait(BoxTrait.class), is(true));
+        assertThat(mixedStruct.getAllMembers().get("bar").hasTrait(DefaultTrait.class), is(true));
+
+        // Now ensure round-tripping results in the same model.
+        Node serialized = ModelSerializer.builder().build().serialize(model1);
+        Model model2 = Model.assembler().addDocumentNode(serialized).assemble().unwrap();
+
+        assertThat(model1, equalTo(model2));
+    }
 }

smithy-model/src/test/resources/software/amazon/smithy/model/loader/ast-empty-apply-1.json

@@ -0,0 +1,8 @@
+{
+    "smithy": "2.0",
+    "shapes": {
+        "smithy.example#Foo": {
+            "type": "string"
+        }
+    }
+}

smithy-model/src/test/resources/software/amazon/smithy/model/loader/ast-empty-apply-2.json

@@ -0,0 +1,8 @@
+{
+    "smithy": "2.0",
+    "shapes": {
+        "smithy.example#Foo": {
+            "type": "apply"
+        }
+    }
+}

smithy-model/src/test/resources/software/amazon/smithy/model/loader/synthetic-boxing-mixins.smithy

@@ -0,0 +1,14 @@
+$version: "2.0"
+namespace smithy.example
+
+@mixin
+integer MixinInteger
+
+integer MixedInteger with [MixinInteger]
+
+@mixin
+structure MixinStruct {
+    bar: MixedInteger = null
+}
+
+structure MixedStruct with [MixinStruct] {}

smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/mixins/no-empty-apply-types.json

@@ -0,0 +1,28 @@
+{
+    "smithy": "2.0",
+    "shapes": {
+        "smithy.example#MixedStruct": {
+            "type": "structure",
+            "mixins": [
+                {
+                    "target": "smithy.example#MixinStruct"
+                }
+            ],
+            "members": {}
+        },
+        "smithy.example#MixinStruct": {
+            "type": "structure",
+            "members": {
+                "bar": {
+                    "target": "smithy.api#PrimitiveInteger",
+                    "traits": {
+                        "smithy.api#default": null
+                    }
+                }
+            },
+            "traits": {
+                "smithy.api#mixin": {}
+            }
+        }
+    }
+}

smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/mixins/no-empty-apply-types.smithy

@@ -0,0 +1,9 @@
+$version: "2.0"
+namespace smithy.example
+
+@mixin
+structure MixinStruct {
+    bar: PrimitiveInteger = null
+}
+
+structure MixedStruct with [MixinStruct] {}