diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/AbstractShapeBuilder.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/AbstractShapeBuilder.java index 0e8ad65f24d..f65ef21748a 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/AbstractShapeBuilder.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/AbstractShapeBuilder.java @@ -321,6 +321,25 @@ public B flattenMixins() { return (B) this; } + /** + * Adds a mixin reference to the shape without triggering member recalculation. + * + *

This is used during mixin flattening to re-add interface mixin references + * after members/traits have already been flattened. Unlike {@link #addMixin(Shape)}, + * this method does not trigger {@code NamedMemberUtils.cleanMixins()} in subclasses. + * + * @param shape Mixin shape to add as a reference. + * @return Returns the builder. + */ + @SuppressWarnings("unchecked") + public B addMixinRef(Shape shape) { + if (mixins == null) { + mixins = new LinkedHashMap<>(); + } + mixins.put(shape.getId(), shape); + return (B) this; + } + Map getMixins() { return mixins == null ? Collections.emptyMap() : mixins; } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/traits/MixinTrait.java b/smithy-model/src/main/java/software/amazon/smithy/model/traits/MixinTrait.java index aad516feb10..1b04707ec70 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/traits/MixinTrait.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/traits/MixinTrait.java @@ -12,6 +12,8 @@ import java.util.Map; import java.util.Set; import software.amazon.smithy.model.node.Node; +import software.amazon.smithy.model.node.ObjectNode; +import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.utils.SetUtils; import software.amazon.smithy.utils.ToSmithyBuilder; @@ -20,10 +22,12 @@ public final class MixinTrait extends AbstractTrait implements ToSmithyBuilder localTraits; + private final boolean isInterface; private MixinTrait(Builder builder) { super(ID, builder.sourceLocation); this.localTraits = SetUtils.orderedCopyOf(builder.localTraits); + this.isInterface = builder.isInterface; } /** @@ -35,31 +39,53 @@ public Set getLocalTraits() { return localTraits; } + /** + * Gets whether this mixin should generate a Java interface. + * + * @return returns true if this mixin should generate a Java interface. + */ + public boolean isInterface() { + return isInterface; + } + + /** + * Checks if the given shape is a mixin with {@code interface = true}. + * + * @param shape Shape to check. + * @return returns true if the shape has a mixin trait with interface set to true. + */ + public static boolean isInterfaceMixin(Shape shape) { + return shape.getTrait(MixinTrait.class).map(MixinTrait::isInterface).orElse(false); + } + @Override protected Node createNode() { + ObjectNode.Builder builder = Node.objectNodeBuilder().sourceLocation(getSourceLocation()); + // smithy.api#mixin is always present, so no need to serialize it. - if (localTraits.size() <= 1) { - return Node.objectNode(); + if (localTraits.size() > 1) { + List nonImplicitValues = new ArrayList<>(); + for (ShapeId trait : localTraits) { + if (!trait.equals(ID)) { + nonImplicitValues.add(Node.from(trait.toString())); + } + } + builder.withMember("localTraits", Node.fromNodes(nonImplicitValues)); } - List nonImplicitValues = new ArrayList<>(); - for (ShapeId trait : localTraits) { - if (!trait.equals(ID)) { - nonImplicitValues.add(Node.from(trait.toString())); - } + if (isInterface) { + builder.withMember("interface", Node.from(true)); } - return Node.objectNodeBuilder() - .sourceLocation(getSourceLocation()) - .withMember("localTraits", Node.fromNodes(nonImplicitValues)) - .build(); + return builder.build(); } @Override public Builder toBuilder() { return builder() .sourceLocation(getSourceLocation()) - .localTraits(localTraits); + .localTraits(localTraits) + .isInterface(isInterface); } /** @@ -104,6 +130,7 @@ public static Builder builder() { */ public static final class Builder extends AbstractTraitBuilder { private final Set localTraits = new LinkedHashSet<>(); + private boolean isInterface; @Override public MixinTrait build() { @@ -122,6 +149,11 @@ public Builder addLocalTrait(ShapeId trait) { localTraits.add(trait); return this; } + + public Builder isInterface(boolean isInterface) { + this.isInterface = isInterface; + return this; + } } public static final class Provider implements TraitService { @@ -133,7 +165,9 @@ public ShapeId getShapeId() { @Override public MixinTrait createTrait(ShapeId target, Node value) { Builder builder = builder().sourceLocation(value); - value.expectObjectNode().getArrayMember("localTraits", ShapeId::fromNode, builder::localTraits); + ObjectNode objectNode = value.expectObjectNode(); + objectNode.getArrayMember("localTraits", ShapeId::fromNode, builder::localTraits); + objectNode.getBooleanMember("interface", builder::isInterface); return builder.build(); } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/transform/FlattenAndRemoveMixins.java b/smithy-model/src/main/java/software/amazon/smithy/model/transform/FlattenAndRemoveMixins.java index c08703b54c0..7a9191eb7e3 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/transform/FlattenAndRemoveMixins.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/transform/FlattenAndRemoveMixins.java @@ -5,37 +5,128 @@ package software.amazon.smithy.model.transform; import java.util.ArrayList; +import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; +import java.util.Set; import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.AbstractShapeBuilder; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.traits.MixinTrait; /** * Flattens mixins out of the model. + * + *

Interface mixins (those with {@code interface = true}) are preserved in the model + * and their references are maintained on shapes that use them, while non-interface + * mixins are removed as before. */ final class FlattenAndRemoveMixins { Model transform(ModelTransformer transformer, Model model) { + Set interfaceMixinIds = new HashSet<>(); + for (Shape shape : model.toSet()) { + if (MixinTrait.isInterfaceMixin(shape)) { + interfaceMixinIds.add(shape.getId()); + } + } + List updatedShapes = new ArrayList<>(); - List toRemove = new ArrayList<>(); + List toRemove = new ArrayList<>(); for (Shape shape : model.toSet()) { - if (shape.hasTrait(MixinTrait.ID)) { - toRemove.add(shape.getId()); + if (shape.hasTrait(MixinTrait.ID) && !interfaceMixinIds.contains(shape.getId())) { + // Non-interface mixin: remove from model + toRemove.add(shape); } else if (!shape.getMixins().isEmpty()) { - updatedShapes.add(Shape.shapeToBuilder(shape).flattenMixins().build()); + // Shape has mixins (either an interface mixin with parents, or a concrete shape). + // Flatten all mixins, then re-add references to effective interface mixins. + Set effectiveInterfaceMixins = collectEffectiveInterfaceMixins( + shape, + model, + interfaceMixinIds); + if (effectiveInterfaceMixins.isEmpty() && !shape.hasTrait(MixinTrait.ID)) { + // Simple case: no interface mixins involved, just flatten everything + updatedShapes.add(Shape.shapeToBuilder(shape).flattenMixins().build()); + } else if (!effectiveInterfaceMixins.isEmpty()) { + updatedShapes.add(flattenAndPreserveInterfaceRefs( + shape, + model, + effectiveInterfaceMixins)); + } } } if (!updatedShapes.isEmpty() || !toRemove.isEmpty()) { Model.Builder builder = model.toBuilder(); updatedShapes.forEach(builder::addShape); - // Don't use the removeShapes transform because that would further mutate shapes and remove the things - // that were just flattened into the shapes. It's safe to just remove mixin shapes here. - toRemove.forEach(builder::removeShape); + toRemove.forEach(s -> builder.removeShape(s.getId())); model = builder.build(); } - return model; } + + /** + * Collect the interface mixin IDs that should be preserved as references on this shape. + * Direct interface mixins are kept as-is; non-interface mixins are walked to find + * transitive interface ancestors. + */ + private Set collectEffectiveInterfaceMixins( + Shape shape, + Model model, + Set interfaceMixinIds + ) { + Set result = new LinkedHashSet<>(); + for (ShapeId mixinId : shape.getMixins()) { + if (interfaceMixinIds.contains(mixinId)) { + result.add(mixinId); + } else { + collectTransitiveInterfaceMixins(model, model.expectShape(mixinId), interfaceMixinIds, result); + } + } + return result; + } + + /** + * Flatten a shape's mixins and re-add references to the given interface mixins. + * Traits that will be re-inherited from the interface mixins are removed from the + * builder's introduced traits so they don't appear double-counted. + */ + private Shape flattenAndPreserveInterfaceRefs( + Shape shape, + Model model, + Set interfaceMixinIds + ) { + AbstractShapeBuilder builder = Shape.shapeToBuilder(shape); + Set introducedTraitIds = shape.getIntroducedTraits().keySet(); + builder.flattenMixins(); + + // Remove traits that will be re-inherited, then re-add the mixin refs + for (ShapeId ifaceId : interfaceMixinIds) { + Shape ifaceMixin = model.expectShape(ifaceId); + for (ShapeId traitId : MixinTrait.getNonLocalTraitsFromMap(ifaceMixin.getAllTraits()).keySet()) { + if (!introducedTraitIds.contains(traitId)) { + builder.removeTrait(traitId); + } + } + builder.addMixinRef(ifaceMixin); + } + + return builder.build(); + } + + private void collectTransitiveInterfaceMixins( + Model model, + Shape shape, + Set interfaceMixinIds, + Set result + ) { + for (ShapeId parentId : shape.getMixins()) { + if (interfaceMixinIds.contains(parentId)) { + result.add(parentId); + } else { + collectTransitiveInterfaceMixins(model, model.expectShape(parentId), interfaceMixinIds, result); + } + } + } } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/MixinTraitValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/MixinTraitValidator.java new file mode 100644 index 00000000000..193d6c3da91 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/MixinTraitValidator.java @@ -0,0 +1,32 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ +package software.amazon.smithy.model.validation.validators; + +import java.util.ArrayList; +import java.util.List; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.traits.MixinTrait; +import software.amazon.smithy.model.validation.AbstractValidator; +import software.amazon.smithy.model.validation.ValidationEvent; + +/** + * Validates constraints on the mixin trait's properties. + */ +public final class MixinTraitValidator extends AbstractValidator { + @Override + public List validate(Model model) { + List events = new ArrayList<>(); + for (Shape shape : model.getShapesWithTrait(MixinTrait.class)) { + MixinTrait trait = shape.expectTrait(MixinTrait.class); + if (trait.isInterface() && shape.isUnionShape()) { + events.add(error(shape, + trait, + "The `interface` property of the `@mixin` trait cannot be used on union shapes.")); + } + } + return events; + } +} diff --git a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator index 4b76f019461..cf0c3ef42bb 100644 --- a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator +++ b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator @@ -26,6 +26,7 @@ software.amazon.smithy.model.validation.validators.IdempotencyTokenIgnoredValida software.amazon.smithy.model.validation.validators.JsonNameValidator software.amazon.smithy.model.validation.validators.LengthTraitValidator software.amazon.smithy.model.validation.validators.MediaTypeValidator +software.amazon.smithy.model.validation.validators.MixinTraitValidator software.amazon.smithy.model.validation.validators.MemberShouldReferenceResourceValidator software.amazon.smithy.model.validation.validators.NoInlineDocumentSupportValidator software.amazon.smithy.model.validation.validators.OperationValidator diff --git a/smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy b/smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy index a91842c7bc9..17bba8ecee8 100644 --- a/smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy +++ b/smithy-model/src/main/resources/software/amazon/smithy/model/loader/prelude.smithy @@ -1190,6 +1190,7 @@ structure unitType {} @trait(selector: ":not(member)") structure mixin { localTraits: LocalMixinTraitList + interface: Boolean } @private diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/traits/MixinTraitTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/traits/MixinTraitTest.java index efa6bbf827b..b713a737fd8 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/traits/MixinTraitTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/traits/MixinTraitTest.java @@ -9,6 +9,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.instanceOf; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Optional; @@ -16,6 +17,7 @@ import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.shapes.StructureShape; public class MixinTraitTest { @@ -37,6 +39,9 @@ public void loadsEmptyTrait() { // But doesn't serialize it since it's implied. assertThat(mixinTrait.toNode(), equalTo(node)); + + // Interface defaults to false + assertFalse(mixinTrait.isInterface()); } @Test @@ -57,4 +62,55 @@ public void retainsMixinLocalTraits() { assertThat(rebuilt, equalTo(trait)); } + + @Test + public void isInterfaceRoundTripsViaSerde() { + MixinTrait trait = MixinTrait.builder().isInterface(true).build(); + Node node = trait.toNode(); + + // Verify the node has the interface member set to true + assertTrue(node.expectObjectNode().getMember("interface").isPresent()); + assertTrue(node.expectObjectNode().getMember("interface").get().expectBooleanNode().getValue()); + + // Verify it can be deserialized back + TraitFactory provider = TraitFactory.createServiceFactory(); + Optional deserialized = provider.createTrait( + ShapeId.from("smithy.api#mixin"), + ShapeId.from("ns.qux#foo"), + node); + assertTrue(deserialized.isPresent()); + MixinTrait deserializedMixin = (MixinTrait) deserialized.get(); + assertTrue(deserializedMixin.isInterface()); + } + + @Test + public void isInterfaceFalseDoesNotSerialize() { + MixinTrait trait = MixinTrait.builder().isInterface(false).build(); + Node node = trait.toNode(); + + // When interface is false, it should not appear in serialized form + assertFalse(node.expectObjectNode().getMember("interface").isPresent()); + } + + @Test + public void isInterfaceMixinStaticHelper() { + MixinTrait interfaceMixin = MixinTrait.builder().isInterface(true).build(); + MixinTrait normalMixin = MixinTrait.builder().build(); + + StructureShape ifaceShape = StructureShape.builder() + .id("ns.foo#IfaceMixin") + .addTrait(interfaceMixin) + .build(); + StructureShape normalShape = StructureShape.builder() + .id("ns.foo#NormalMixin") + .addTrait(normalMixin) + .build(); + StructureShape noMixin = StructureShape.builder() + .id("ns.foo#NoMixin") + .build(); + + assertTrue(MixinTrait.isInterfaceMixin(ifaceShape)); + assertFalse(MixinTrait.isInterfaceMixin(normalShape)); + assertFalse(MixinTrait.isInterfaceMixin(noMixin)); + } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/transform/FlattenAndRemoveInterfaceMixinsTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/transform/FlattenAndRemoveInterfaceMixinsTest.java new file mode 100644 index 00000000000..c54dfbf3a33 --- /dev/null +++ b/smithy-model/src/test/java/software/amazon/smithy/model/transform/FlattenAndRemoveInterfaceMixinsTest.java @@ -0,0 +1,109 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ +package software.amazon.smithy.model.transform; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Set; +import org.junit.jupiter.api.Test; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.shapes.StructureShape; +import software.amazon.smithy.model.traits.MixinTrait; + +public class FlattenAndRemoveInterfaceMixinsTest { + + @Test + void interfaceMixinsArePreservedInModel() { + Model before = Model.assembler() + .addImport(getClass().getResource("flatten-interface-mixins.smithy")) + .assemble() + .unwrap(); + Model result = ModelTransformer.create().flattenAndRemoveMixins(before); + + // Interface mixin should still be in model + assertTrue(result.getShape(ShapeId.from("smithy.example#HasName")).isPresent()); + assertTrue(MixinTrait.isInterfaceMixin(result.expectShape(ShapeId.from("smithy.example#HasName")))); + + // Non-interface mixin should be removed + assertFalse(result.getShape(ShapeId.from("smithy.example#NormalMixin")).isPresent()); + } + + @Test + void concreteShapeGetsInterfaceMixinRef() { + Model before = Model.assembler() + .addImport(getClass().getResource("flatten-interface-mixins.smithy")) + .assemble() + .unwrap(); + Model result = ModelTransformer.create().flattenAndRemoveMixins(before); + + var concrete = result.expectShape(ShapeId.from("smithy.example#Concrete"), StructureShape.class); + // Should have the interface mixin ref + assertTrue(concrete.getMixins().contains(ShapeId.from("smithy.example#HasName"))); + // Should have flattened members from both mixins + assertTrue(concrete.getMember("name").isPresent()); + assertTrue(concrete.getMember("age").isPresent()); + assertTrue(concrete.getMember("id").isPresent()); + } + + @Test + void transitiveInterfaceMixinDiscovery() { + Model before = Model.assembler() + .addImport(getClass().getResource("flatten-interface-mixins.smithy")) + .assemble() + .unwrap(); + Model result = ModelTransformer.create().flattenAndRemoveMixins(before); + + // TransitiveUser uses NonInterfaceChild which extends HasName (interface). + // NonInterfaceChild should be removed, but TransitiveUser should get HasName ref. + var transitiveUser = result.expectShape(ShapeId.from("smithy.example#TransitiveUser"), StructureShape.class); + assertTrue(transitiveUser.getMixins().contains(ShapeId.from("smithy.example#HasName"))); + assertFalse(result.getShape(ShapeId.from("smithy.example#NonInterfaceChild")).isPresent()); + // Members from the removed NonInterfaceChild and its parent HasName should be flattened in + assertTrue(transitiveUser.getMember("name").isPresent()); + assertTrue(transitiveUser.getMember("extra").isPresent()); + assertTrue(transitiveUser.getMember("userId").isPresent()); + } + + @Test + void interfaceMixinHierarchyPreserved() { + Model before = Model.assembler() + .addImport(getClass().getResource("flatten-interface-mixins.smithy")) + .assemble() + .unwrap(); + Model result = ModelTransformer.create().flattenAndRemoveMixins(before); + + // HasFullName extends HasName (both interface mixins) + var hasFullName = result.expectShape(ShapeId.from("smithy.example#HasFullName"), StructureShape.class); + assertTrue(MixinTrait.isInterfaceMixin(hasFullName)); + // HasFullName should still reference HasName as mixin + assertTrue(hasFullName.getMixins().contains(ShapeId.from("smithy.example#HasName"))); + // HasFullName should have members from HasName flattened in + assertTrue(hasFullName.getMember("name").isPresent()); + assertTrue(hasFullName.getMember("lastName").isPresent()); + + // FullNameUser should implement HasFullName (not HasName directly, since hierarchy handles it) + var fullNameUser = result.expectShape(ShapeId.from("smithy.example#FullNameUser"), StructureShape.class); + assertTrue(fullNameUser.getMixins().contains(ShapeId.from("smithy.example#HasFullName"))); + } + + @Test + void diamondInterfaceMixins() { + Model before = Model.assembler() + .addImport(getClass().getResource("flatten-interface-mixins.smithy")) + .assemble() + .unwrap(); + Model result = ModelTransformer.create().flattenAndRemoveMixins(before); + + // DiamondUser uses both HasFullName and HasAge (both interface), which both extend HasName + var diamondUser = result.expectShape(ShapeId.from("smithy.example#DiamondUser"), StructureShape.class); + Set mixinRefs = diamondUser.getMixins(); + assertTrue(mixinRefs.contains(ShapeId.from("smithy.example#HasFullName"))); + assertTrue(mixinRefs.contains(ShapeId.from("smithy.example#HasAge"))); + // HasName should NOT appear as a direct ref — it's reachable via HasFullName and HasAge + assertFalse(mixinRefs.contains(ShapeId.from("smithy.example#HasName"))); + } +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/transform/flatten-interface-mixins.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/transform/flatten-interface-mixins.smithy new file mode 100644 index 00000000000..41f73245287 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/transform/flatten-interface-mixins.smithy @@ -0,0 +1,52 @@ +$version: "2" +namespace smithy.example + +// Simple interface mixin +@mixin(interface: true) +structure HasName { + name: String +} + +// Normal (non-interface) mixin +@mixin +structure NormalMixin { + age: Integer +} + +// Concrete structure using both +structure Concrete with [HasName, NormalMixin] { + id: String +} + +// Non-interface mixin that extends an interface mixin (transitive case) +@mixin +structure NonInterfaceChild with [HasName] { + extra: String +} + +// Uses non-interface child, should get HasName transitively +structure TransitiveUser with [NonInterfaceChild] { + userId: String +} + +// Interface mixin hierarchy: HasFullName extends HasName (both interface) +@mixin(interface: true) +structure HasFullName with [HasName] { + lastName: String +} + +// Concrete using the interface hierarchy +structure FullNameUser with [HasFullName] { + email: String +} + +// Another interface mixin extending HasName for diamond test +@mixin(interface: true) +structure HasAge with [HasName] { + personAge: Integer +} + +// Diamond: uses both HasFullName and HasAge, which both extend HasName +structure DiamondUser with [HasFullName, HasAge] { + employeeId: String +}