diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/AstModelLoader.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/AstModelLoader.java index ec0be663496..3f0f70c1168 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/AstModelLoader.java +++ b/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); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/ModelSerializer.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/ModelSerializer.java index bd0742591f0..42dd5e891f2 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/ModelSerializer.java +++ b/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 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 traits) { + return serializeTraits(builder, createIntroducedTraitsMap(traits)); + } + + private ObjectNode.Builder serializeTraits(ObjectNode.Builder builder, Map traits) { if (!traits.isEmpty()) { + builder.withMember("traits", new ObjectNode(traits, SourceLocation.none())); + } + + return builder; + } + + private Map createIntroducedTraitsMap(Collection traits) { + if (traits.isEmpty()) { + return Collections.emptyMap(); + } else { Map 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 { diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/AstModelLoaderTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/AstModelLoaderTest.java index bba04542343..561f0238072 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/AstModelLoaderTest.java +++ b/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(); + } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java index 3c887675d48..0db515a4c0c 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java +++ b/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)); + } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/ast-empty-apply-1.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/ast-empty-apply-1.json new file mode 100644 index 00000000000..39ee34c5898 --- /dev/null +++ b/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" + } + } +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/ast-empty-apply-2.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/ast-empty-apply-2.json new file mode 100644 index 00000000000..9276bb3e9b0 --- /dev/null +++ b/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" + } + } +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/synthetic-boxing-mixins.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/synthetic-boxing-mixins.smithy new file mode 100644 index 00000000000..41d2eb63b26 --- /dev/null +++ b/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] {} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/mixins/no-empty-apply-types.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/mixins/no-empty-apply-types.json new file mode 100644 index 00000000000..7eb2cfc7f27 --- /dev/null +++ b/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": {} + } + } + } +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/mixins/no-empty-apply-types.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/mixins/no-empty-apply-types.smithy new file mode 100644 index 00000000000..20623d71e30 --- /dev/null +++ b/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] {}