From d5026ea90ebb12b71f4053661fe65b8a77bf5dbc Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Mon, 1 Aug 2022 13:54:55 -0700 Subject: [PATCH] Add missing mixin relationships Mixin relationships can exist on any shape but we only created a relationship for structure and union. This also attempts to ensure that the list of relationships is created with sufficient capacity to avoiding needing to resize the array as rels are computed and added. --- .../model/neighbor/NeighborVisitor.java | 77 ++++++++++++++----- .../validators/TargetValidator.java | 1 + .../model/neighbor/NeighborVisitorTest.java | 21 +++++ 3 files changed, 78 insertions(+), 21 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborVisitor.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborVisitor.java index 5097a633490..8108ac92685 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborVisitor.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborVisitor.java @@ -16,6 +16,7 @@ package software.amazon.smithy.model.neighbor; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.EntityShape; @@ -33,7 +34,6 @@ import software.amazon.smithy.model.shapes.ShapeVisitor; import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.shapes.UnionShape; -import software.amazon.smithy.utils.ListUtils; /** * Finds all neighbors of a shape, returning them as a list of @@ -59,12 +59,32 @@ public List getNeighbors(Shape shape) { @Override public List getDefault(Shape shape) { - return ListUtils.of(); + return shape.getMixins().isEmpty() + ? Collections.emptyList() + : initializeRelationships(shape, 0); + } + + private List initializeRelationships(Shape shape, int knownMemberCount) { + if (shape.isMemberShape()) { + // Members have mixins but shouldn't contribute a relationship. + return new ArrayList<>(knownMemberCount); + } else { + knownMemberCount += shape.getMixins().size(); + List result = new ArrayList<>(knownMemberCount); + for (ShapeId mixin : shape.getMixins()) { + result.add(relationship(shape, RelationshipType.MIXIN, mixin)); + } + return result; + } } @Override public List serviceShape(ServiceShape shape) { - List result = new ArrayList<>(); + int operationAndResourceRelationships = 2 * (shape.getAllOperations().size() + shape.getResources().size()); + int errorSize = shape.getErrors().size(); + int neededSize = operationAndResourceRelationships + errorSize; + List result = initializeRelationships(shape, neededSize); + // Add OPERATION from service -> operation. Add BINDING from operation -> service. for (ShapeId operation : shape.getOperations()) { addBinding(result, shape, operation, RelationshipType.OPERATION); @@ -92,7 +112,12 @@ private void addBound(List result, Shape container, ShapeId bindin @Override public List resourceShape(ResourceShape shape) { - List result = new ArrayList<>(); + // This is a rough estimate for the number of needed relationships. + int operationAndResourceRelationships = 2 * (shape.getAllOperations().size() + shape.getResources().size()); + int identifierSize = shape.getIdentifiers().size(); + int neededSize = (operationAndResourceRelationships * 2) + identifierSize; + List result = initializeRelationships(shape, neededSize); + // Add IDENTIFIER relationships. shape.getIdentifiers().forEach((k, v) -> result.add(relationship(shape, RelationshipType.IDENTIFIER, v))); // Add RESOURCE from resourceA -> resourceB and BOUND from resourceB -> resourceA @@ -154,10 +179,22 @@ private void addServiceAndResourceBindings(List result, ResourceSh @Override public List operationShape(OperationShape shape) { - List result = new ArrayList<>(); + ShapeId input = shape.getInput().orElse(null); + ShapeId output = shape.getOutput().orElse(null); + + // Calculate the number of relationships up front. + int assumedRelationshipCount = shape.getErrors().size() + + (input == null ? 0 : 1) + + (output == null ? 0 : 1); + List result = initializeRelationships(shape, assumedRelationshipCount); - shape.getInput().ifPresent(input -> result.add(relationship(shape, RelationshipType.INPUT, input))); - shape.getOutput().ifPresent(output -> result.add(relationship(shape, RelationshipType.OUTPUT, output))); + if (input != null) { + result.add(relationship(shape, RelationshipType.INPUT, input)); + } + + if (output != null) { + result.add(relationship(shape, RelationshipType.OUTPUT, output)); + } for (ShapeId errorId : shape.getErrors()) { result.add(relationship(shape, RelationshipType.ERROR, errorId)); @@ -167,7 +204,7 @@ public List operationShape(OperationShape shape) { @Override public List memberShape(MemberShape shape) { - List result = new ArrayList<>(2); + List result = initializeRelationships(shape, 2); result.add(relationship(shape, RelationshipType.MEMBER_CONTAINER, shape.getContainer())); result.add(relationship(shape, RelationshipType.MEMBER_TARGET, shape.getTarget())); return result; @@ -175,7 +212,7 @@ public List memberShape(MemberShape shape) { @Override public List enumShape(EnumShape shape) { - List result = new ArrayList<>(); + List result = initializeRelationships(shape, shape.getAllMembers().size()); for (MemberShape member : shape.getAllMembers().values()) { result.add(relationship(shape, RelationshipType.ENUM_MEMBER, member)); } @@ -184,7 +221,7 @@ public List enumShape(EnumShape shape) { @Override public List intEnumShape(IntEnumShape shape) { - List result = new ArrayList<>(); + List result = initializeRelationships(shape, shape.getAllMembers().size()); for (MemberShape member : shape.getAllMembers().values()) { result.add(relationship(shape, RelationshipType.INT_ENUM_MEMBER, member)); } @@ -193,17 +230,21 @@ public List intEnumShape(IntEnumShape shape) { @Override public List listShape(ListShape shape) { - return ListUtils.of(relationship(shape, RelationshipType.LIST_MEMBER, shape.getMember())); + List result = initializeRelationships(shape, 1); + result.add(relationship(shape, RelationshipType.LIST_MEMBER, shape.getMember())); + return result; } @Override public List setShape(SetShape shape) { - return ListUtils.of(relationship(shape, RelationshipType.SET_MEMBER, shape.getMember())); + List result = initializeRelationships(shape, 1); + result.add(relationship(shape, RelationshipType.SET_MEMBER, shape.getMember())); + return result; } @Override public List mapShape(MapShape shape) { - List result = new ArrayList<>(2); + List result = initializeRelationships(shape, 2); result.add(relationship(shape, RelationshipType.MAP_KEY, shape.getKey())); result.add(relationship(shape, RelationshipType.MAP_VALUE, shape.getValue())); return result; @@ -211,10 +252,7 @@ public List mapShape(MapShape shape) { @Override public List structureShape(StructureShape shape) { - List result = new ArrayList<>(); - for (ShapeId mixin : shape.getMixins()) { - result.add(relationship(shape, RelationshipType.MIXIN, mixin)); - } + List result = initializeRelationships(shape, shape.getAllMembers().size()); for (MemberShape member : shape.getAllMembers().values()) { result.add(Relationship.create(shape, RelationshipType.STRUCTURE_MEMBER, member)); } @@ -223,10 +261,7 @@ public List structureShape(StructureShape shape) { @Override public List unionShape(UnionShape shape) { - List result = new ArrayList<>(); - for (ShapeId mixin : shape.getMixins()) { - result.add(relationship(shape, RelationshipType.MIXIN, mixin)); - } + List result = initializeRelationships(shape, shape.getAllMembers().size()); for (MemberShape member : shape.getAllMembers().values()) { result.add(Relationship.create(shape, RelationshipType.UNION_MEMBER, member)); } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java index 156db099dd8..0fe1ac4b96e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java @@ -102,6 +102,7 @@ private Optional validateTarget(Model model, Shape shape, Shape return Optional.of(error(shape, format( "Members cannot target %s shapes, but found %s", target.getType(), target))); } + return Optional.empty(); case MAP_KEY: return target.asMemberShape().flatMap(m -> validateMapKey(shape, m.getTarget(), model)); case RESOURCE: diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborVisitorTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborVisitorTest.java index 4ecaccfbcf5..f34f99fa8b3 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborVisitorTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborVisitorTest.java @@ -21,6 +21,7 @@ import static org.hamcrest.Matchers.empty; import java.util.List; +import java.util.stream.Collectors; import org.junit.jupiter.api.Test; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.BlobShape; @@ -58,6 +59,26 @@ public void blobShape() { assertThat(relationships, empty()); } + @Test + public void findsMixinsOnThingsOtherThanStructAndUnion() { + Shape blobMixin = BlobShape.builder().id("smithy.example#BlobMixin") + .addTrait(MixinTrait.builder().build()) + .build(); + Shape shape = BlobShape.builder() + .id("ns.foo#name") + .addMixin(blobMixin) + .build(); + Model model = Model.builder().addShapes(shape, blobMixin).build(); + NeighborVisitor neighborVisitor = new NeighborVisitor(model); + List relationships = shape.accept(neighborVisitor); + + List types = relationships.stream() + .map(Relationship::getRelationshipType) + .collect(Collectors.toList()); + + assertThat(types, contains(RelationshipType.MIXIN)); + } + @Test public void booleanShape() { Shape shape = BooleanShape.builder().id("ns.foo#name").build();