From df710b1175158a760e89436fba60c2cfafeedb2a Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Thu, 18 Jan 2024 11:26:01 -0500 Subject: [PATCH 1/6] Check references via idRef in UnreferencedShapes Applications of traits with `@idRef` members don't create relationships between shapes, so removing unreferenced shapes from the following: ``` service FooService { ... operations: [GetFoo] } operation GetFoo { output := { foo: Foo } } @trait @idRef(failWhenMissing: true, selector: "*") string myIdRef @myIdRef(Referenced) structure Foo {} stucture Referenced {} ``` will remove `Referenced` from the model. This commit changes UnreferencedShapes so that `Referenced` is considered connected to `FooService` because it is referenced by an `@idRef` on a shape connected to the service. Determining which shapes are connected via id ref is done by the new IdRefShapeReferences class, which uses the following process: 1. Get all trait definition shapes. 2. Find all paths from those trait definition shapes to shapes with the idRef trait. Each path found says that applications of the trait definition may have a shape reference. 3. For each path, get all applications of the trait on connected shapes and search the node value using the path to the shape that had idRef. 4. For each search result, exclude all connected shapes from being unreferenced. Other changes: - A few test cases that expected to receive an UnreferencedShape event were updated - An `asShapeId` method was added to StringNode --- .../additionalschemas-conflict.errors | 1 - .../errorfiles/deconflict-by-excluding.errors | 1 - .../invalid-additional-schemas-shape.errors | 1 - .../model/neighbor/IdRefShapeReferences.java | 104 ++++++++++ .../smithy/model/neighbor/NodeQuery.java | 76 +++++++ .../model/neighbor/UnreferencedShapes.java | 7 + .../amazon/smithy/model/node/StringNode.java | 13 ++ .../neighbor/UnreferencedShapesTest.java | 25 +++ .../neighbor/through-idref-unconnected.smithy | 12 ++ .../model/neighbor/through-idref.smithy | 186 ++++++++++++++++++ 10 files changed, 423 insertions(+), 3 deletions(-) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref-unconnected.smithy create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy diff --git a/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/additionalschemas-conflict.errors b/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/additionalschemas-conflict.errors index 9e324e1b417..471020f98a4 100644 --- a/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/additionalschemas-conflict.errors +++ b/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/additionalschemas-conflict.errors @@ -1,3 +1,2 @@ [ERROR] smithy.example#AdditionalSchemasConflictResource: The `bar` property of the generated `AdditionalSchemasConflictResource` CloudFormation resource targets multiple shapes: [smithy.api#Boolean, smithy.api#String]. Reusing member names that target different shapes can cause confusion for users of the API. This target discrepancy must either be resolved in the model or one of the members must be excluded from the conversion. | CfnResourceProperty -[NOTE] smithy.example#AdditionalSchemasConflictProperties: The structure shape is not connected to from any service shape. | UnreferencedShape [WARNING] smithy.example#AdditionalSchemasConflictResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait diff --git a/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/deconflict-by-excluding.errors b/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/deconflict-by-excluding.errors index da36a2fc359..4bda42c1381 100644 --- a/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/deconflict-by-excluding.errors +++ b/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/deconflict-by-excluding.errors @@ -1,3 +1,2 @@ -[NOTE] smithy.example#AdditionalSchemasDeconflictedProperties: The structure shape is not connected to from any service shape. | UnreferencedShape [WARNING] smithy.example#CreateAdditionalSchemasDeconflictedResourceRequest$bar: This shape applies a trait that is unstable: aws.cloudformation#cfnExcludeProperty | UnstableTrait [WARNING] smithy.example#AdditionalSchemasDeconflictedResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait diff --git a/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/invalid-additional-schemas-shape.errors b/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/invalid-additional-schemas-shape.errors index 0d7f38476a7..3fe72f1e10c 100644 --- a/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/invalid-additional-schemas-shape.errors +++ b/smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/invalid-additional-schemas-shape.errors @@ -1,3 +1,2 @@ [ERROR] smithy.example#InvalidAdditionalSchemasShapeResource: Error validating trait `aws.cloudformation#cfnResource`.additionalSchemas.0: Shape ID `smithy.example#ListShape` does not match selector `structure` | TraitValue -[NOTE] smithy.example#ListShape: The list shape is not connected to from any service shape. | UnreferencedShape [WARNING] smithy.example#InvalidAdditionalSchemasShapeResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java new file mode 100644 index 00000000000..d33df7ec1ff --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java @@ -0,0 +1,104 @@ +/* + * Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.model.neighbor; + +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.node.Node; +import software.amazon.smithy.model.node.StringNode; +import software.amazon.smithy.model.selector.PathFinder; +import software.amazon.smithy.model.selector.Selector; +import software.amazon.smithy.model.shapes.MemberShape; +import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.traits.IdRefTrait; +import software.amazon.smithy.model.traits.Trait; +import software.amazon.smithy.model.traits.TraitDefinition; + +/** + * Finds shapes referenced by an {@link IdRefTrait} from within a trait + * value. + * + *

This works by finding all paths from {@link TraitDefinition} shapes + * to shapes with {@link IdRefTrait}, then using those paths to search + * the node values of each application of the trait to extract the {@link ShapeId} + * value. + */ +final class IdRefShapeReferences { + private static final Selector WITH_ID_REF = Selector.parse("[trait|idRef]"); + private final Model model; + private final Set found = new HashSet<>(); + + IdRefShapeReferences(Model model) { + this.model = model; + } + + Set compute(Set connected) { + PathFinder finder = PathFinder.create(model); + for (Shape traitDef : model.getShapesWithTrait(TraitDefinition.class)) { + if (traitDef.hasTrait(IdRefTrait.class)) { + // PathFinder doesn't handle the case where the trait def has the idRef + NodeQuery query = new NodeQuery().self(); + addReferences(traitDef, query, connected); + continue; + } + List paths = finder.search(traitDef, WITH_ID_REF); + if (!paths.isEmpty()) { + for (PathFinder.Path path : paths) { + NodeQuery query = toNodeQuery(path); + addReferences(traitDef, query, connected); + } + } + } + return found; + } + + private void addReferences(Shape traitDef, NodeQuery query, Set connected) { + model.getShapesWithTrait(traitDef.getId()).stream() + .filter(connected::contains) + .forEach(shape -> { + Trait trait = shape.findTrait(traitDef.getId()).get(); // We already know the shape has the trait. + Node node = trait.toNode(); + query.execute(node).forEach(n -> + // Invalid shape ids are handled by the idRef trait validator, so ignore them here. + n.asStringNode().flatMap(StringNode::asShapeId).ifPresent(found::add) + ); + }); + } + + private static NodeQuery toNodeQuery(PathFinder.Path path) { + NodeQuery query = new NodeQuery(); + // The path goes from trait definition -> shape with idRef + for (Relationship relationship : path) { + if (!relationship.getNeighborShape().isPresent()) { + break; + } + switch (relationship.getRelationshipType()) { + case MEMBER_TARGET: + case MAP_KEY: + case TRAIT: + case MIXIN: + default: + break; + case MAP_VALUE: + query.anyMember(); + break; + case LIST_MEMBER: + case SET_MEMBER: + query.anyElement(); + break; + case UNION_MEMBER: + case STRUCTURE_MEMBER: + MemberShape member = (MemberShape) relationship.getNeighborShape().get(); + query.member(member.getMemberName()); + break; + } + } + return query; + } +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java new file mode 100644 index 00000000000..c429839c958 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java @@ -0,0 +1,76 @@ +/* + * Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.model.neighbor; + +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; +import software.amazon.smithy.model.node.Node; +import software.amazon.smithy.utils.ListUtils; + +/** + * Searches {@link Node}s to find matching children. Each search + * condition is executed on the result of the previous search, + * and the results are aggregated. + */ +final class NodeQuery { + private final List queries = new ArrayList<>(); + + NodeQuery() { + } + + NodeQuery self() { + queries.add(ListUtils::of); + return this; + } + + NodeQuery member(String name) { + queries.add(node -> { + if (node == null || !node.isObjectNode()) { + return ListUtils.of(); + } + return node.expectObjectNode().getMember(name).map(ListUtils::of).orElse(ListUtils.of()); + }); + return this; + } + + NodeQuery anyMember() { + queries.add(node -> { + if (node == null || !node.isObjectNode()) { + return ListUtils.of(); + } + return ListUtils.copyOf(node.expectObjectNode().getMembers().values()); + }); + return this; + } + + NodeQuery anyElement() { + queries.add(node -> { + if (node == null || !node.isArrayNode()) { + return ListUtils.of(); + } + return node.expectArrayNode().getElements(); + }); + return this; + } + + List execute(Node node) { + if (queries.isEmpty()) { + return ListUtils.of(); + } + + List previousResult = ListUtils.of(node); + for (Query query : queries) { + previousResult = previousResult.stream().flatMap(n -> query.run(n).stream()).collect(Collectors.toList()); + } + return previousResult; + } + + @FunctionalInterface + interface Query { + List run(Node node); + } +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java index 2de3ce2b592..a5c96d8a546 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java @@ -22,6 +22,7 @@ import software.amazon.smithy.model.knowledge.NeighborProviderIndex; import software.amazon.smithy.model.loader.Prelude; import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.traits.TraitDefinition; import software.amazon.smithy.utils.FunctionalUtils; @@ -66,6 +67,12 @@ public Set compute(Model model) { connected.addAll(shapeWalker.walkShapes(trait)); } + for (ShapeId referencedThroughIdRef : new IdRefShapeReferences(model).compute(connected)) { + // Referenced shapes may not exist in the model, but we don't want to throw. + model.getShape(referencedThroughIdRef) + .ifPresent(shape -> connected.addAll(shapeWalker.walkShapes(shape))); + } + // Any shape that wasn't identified as connected to a service is considered unreferenced. Set result = new HashSet<>(); for (Shape shape : model.toSet()) { diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/node/StringNode.java b/smithy-model/src/main/java/software/amazon/smithy/model/node/StringNode.java index c7775108715..81b001dccbc 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/node/StringNode.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/node/StringNode.java @@ -168,6 +168,19 @@ public ShapeId expectShapeId() { } } + /** + * Gets the value of the string as a ShapeId if it is a valid Shape ID. + * + * @return Returns the optional Shape ID. + */ + public Optional asShapeId() { + try { + return Optional.of(ShapeId.from(getValue())); + } catch (ShapeIdSyntaxException e) { + return Optional.empty(); + } + } + @Override public boolean equals(Object other) { return other instanceof StringNode && value.equals(((StringNode) other).getValue()); diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java index 41e4d75897f..d456b87ee42 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java @@ -80,4 +80,29 @@ private Model createPrivateShapeModel(ShapeId id) { .assemble() .unwrap(); } + + @Test + public void checksShapeReferencesThroughIdRef() { + Model m = Model.assembler() + .addImport(getClass().getResource("through-idref.smithy")) + .assemble() + .unwrap(); + + Set shapes = new UnreferencedShapes().compute(m); + assertThat(shapes, empty()); + } + + @Test + public void doesNotCheckShapeReferencesThroughIdRefOnUnconnectedTraits() { + Model m = Model.assembler() + .addImport(getClass().getResource("through-idref-unconnected.smithy")) + .assemble() + .unwrap(); + + Set ids = new UnreferencedShapes().compute(m).stream().map(Shape::getId).collect(Collectors.toSet()); + assertThat(ids, containsInAnyOrder( + ShapeId.from("com.foo#WithTrait"), + ShapeId.from("com.foo#Referenced") + )); + } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref-unconnected.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref-unconnected.smithy new file mode 100644 index 00000000000..77346b7f393 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref-unconnected.smithy @@ -0,0 +1,12 @@ +$version: "2.0" + +namespace com.foo + +@trait +@idRef(failWhenMissing: true, selector: "*") +string unconnected + +@unconnected(Referenced) +structure WithTrait {} + +structure Referenced {} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy new file mode 100644 index 00000000000..c52f2c521f8 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy @@ -0,0 +1,186 @@ +$version: "2.0" + +namespace com.foo + +service FooService { + version: "2024-01-18" + operations: [GetFoo] +} + +operation GetFoo { + input := { + one: One + two: Two + three: Three + four: Four + five: Five + six: Six + seven: Seven + eight: Eight + nine: Nine + ten: Ten + } +} + +// -- +@trait +structure withIdRefOnMember { + @idRef(failWhenMissing: true, selector: "*") + ref: String +} + +@withIdRefOnMember(ref: Ref1) +structure One {} + +structure Ref1 {} + +// -- +@trait +structure withIdRefOnMemberTarget { + ref: OnTarget +} + +@idRef(failWhenMissing: true, selector: "*") +string OnTarget + +@withIdRefOnMemberTarget(ref: Ref2) +structure Two {} + +structure Ref2 {} + +// -- +@trait +structure withIdRefOnNestedStructureMember { + struct: Nested +} + +structure Nested { + @idRef(failWhenMissing: true, selector: "*") + member: String +} + +@withIdRefOnNestedStructureMember( + struct: { + member: Ref3 + } +) +structure Three {} + +structure Ref3 {} + + +// -- +@trait +list withIdRefOnListMemberTarget { + member: OnListMemberTarget +} + +@idRef(failWhenMissing: true, selector: "*") +string OnListMemberTarget + +@withIdRefOnListMemberTarget([ + Ref4 +]) +structure Four {} + +structure Ref4 {} + +// -- +@trait +@idRef(failWhenMissing: true, selector: "*") +string withIdRefOnSelf + +@withIdRefOnSelf(Ref5) +structure Five {} + +structure Ref5 {} + +// -- +@withIdRefOnSelf(Ref6) +structure Six {} + +structure Ref6 { + ref: RefByRef6 +} + +structure RefByRef6 {} + +// -- +@trait +list withIdRefOnNestedStruct { + member: Nested +} + +@withIdRefOnNestedStruct([ + { + member: Ref7 + } +]) +structure Seven {} + +structure Ref7 {} + +// -- +@trait +structure withIdRefThroughMixin with [ThroughMixin] {} + +@mixin +structure ThroughMixin { + @idRef(failWhenMissing: true, selector: "*") + ref: String +} + +@withIdRefThroughMixin(ref: Ref8) +structure Eight {} + +structure Ref8 {} + +// -- +@trait +map withIdRefOnMapValue { + key: String + value: OnMap +} + +@idRef(failWhenMissing: true, selector: "*") +string OnMap + +@withIdRefOnMapValue({ + foo: Ref9 +}) +structure Nine {} + +structure Ref9 {} + +// -- +@trait +map withIdRefOnNestedMapValue { + key: String + value: Nested +} + +@withIdRefOnNestedMapValue({ + foo: { + member: Ref10 + } +}) +structure Ten {} + +structure Ref10 {} + +// -- +// NOTE: This actually doesn't work because map key of 'Ref11' it says isn't a valid +// shape id. + +//@trait +//map withIdRefOnMapKey { +// key: OnMap +// value: String +//} + +//@withIdRefOnMapKey({ +// Ref11: "foo" +//}) +//structure Eleven {} + +//structure Ref11 {} From e536ea2ef7d3fe011fd32206a072c4e683c83dd8 Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Fri, 19 Jan 2024 12:55:41 -0500 Subject: [PATCH 2/6] Update docs, use Stream in NodeQuery --- .../model/neighbor/IdRefShapeReferences.java | 11 +++++---- .../smithy/model/neighbor/NodeQuery.java | 23 ++++++++++--------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java index d33df7ec1ff..4941d2b52ed 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java @@ -21,13 +21,14 @@ import software.amazon.smithy.model.traits.TraitDefinition; /** - * Finds shapes referenced by an {@link IdRefTrait} from within a trait - * value. + * Finds all shapes referenced by {@link IdRefTrait} from within trait + * values. * *

This works by finding all paths from {@link TraitDefinition} shapes * to shapes with {@link IdRefTrait}, then using those paths to search * the node values of each application of the trait to extract the {@link ShapeId} - * value. + * value. Because we don't have a fixed set of traits known to potentially have + * idRef members, this has to be done dynamically. */ final class IdRefShapeReferences { private static final Selector WITH_ID_REF = Selector.parse("[trait|idRef]"); @@ -50,7 +51,7 @@ Set compute(Set connected) { List paths = finder.search(traitDef, WITH_ID_REF); if (!paths.isEmpty()) { for (PathFinder.Path path : paths) { - NodeQuery query = toNodeQuery(path); + NodeQuery query = buildNodeQuery(path); addReferences(traitDef, query, connected); } } @@ -71,7 +72,7 @@ private void addReferences(Shape traitDef, NodeQuery query, Set connected }); } - private static NodeQuery toNodeQuery(PathFinder.Path path) { + private static NodeQuery buildNodeQuery(PathFinder.Path path) { NodeQuery query = new NodeQuery(); // The path goes from trait definition -> shape with idRef for (Relationship relationship : path) { diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java index c429839c958..842616161b0 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java @@ -8,6 +8,7 @@ import java.util.ArrayList; import java.util.List; import java.util.stream.Collectors; +import java.util.stream.Stream; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.utils.ListUtils; @@ -23,16 +24,16 @@ final class NodeQuery { } NodeQuery self() { - queries.add(ListUtils::of); + queries.add(Stream::of); return this; } NodeQuery member(String name) { queries.add(node -> { if (node == null || !node.isObjectNode()) { - return ListUtils.of(); + return Stream.empty(); } - return node.expectObjectNode().getMember(name).map(ListUtils::of).orElse(ListUtils.of()); + return node.expectObjectNode().getMember(name).map(Stream::of).orElse(Stream.empty()); }); return this; } @@ -40,9 +41,9 @@ NodeQuery member(String name) { NodeQuery anyMember() { queries.add(node -> { if (node == null || !node.isObjectNode()) { - return ListUtils.of(); + return Stream.empty(); } - return ListUtils.copyOf(node.expectObjectNode().getMembers().values()); + return node.expectObjectNode().getMembers().values().stream(); }); return this; } @@ -50,9 +51,9 @@ NodeQuery anyMember() { NodeQuery anyElement() { queries.add(node -> { if (node == null || !node.isArrayNode()) { - return ListUtils.of(); + return Stream.empty(); } - return node.expectArrayNode().getElements(); + return node.expectArrayNode().getElements().stream(); }); return this; } @@ -62,15 +63,15 @@ List execute(Node node) { return ListUtils.of(); } - List previousResult = ListUtils.of(node); + Stream previousResult = Stream.of(node); for (Query query : queries) { - previousResult = previousResult.stream().flatMap(n -> query.run(n).stream()).collect(Collectors.toList()); + previousResult = previousResult.flatMap(query::run); } - return previousResult; + return previousResult.collect(Collectors.toList()); } @FunctionalInterface interface Query { - List run(Node node); + Stream run(Node node); } } From b8a7aada99689e98deab0e487a66f7065ec413dd Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Fri, 19 Jan 2024 14:57:53 -0500 Subject: [PATCH 3/6] Handle map keys with idRef --- .../model/neighbor/IdRefShapeReferences.java | 8 ++-- .../smithy/model/neighbor/NodeQuery.java | 12 +++++- .../model/neighbor/through-idref.smithy | 38 +++++++++---------- 3 files changed, 33 insertions(+), 25 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java index 4941d2b52ed..dea9ad707d0 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java @@ -80,11 +80,8 @@ private static NodeQuery buildNodeQuery(PathFinder.Path path) { break; } switch (relationship.getRelationshipType()) { - case MEMBER_TARGET: case MAP_KEY: - case TRAIT: - case MIXIN: - default: + query.anyMemberName(); break; case MAP_VALUE: query.anyMember(); @@ -98,6 +95,9 @@ private static NodeQuery buildNodeQuery(PathFinder.Path path) { MemberShape member = (MemberShape) relationship.getNeighborShape().get(); query.member(member.getMemberName()); break; + default: + // Other relationship types don't produce meaningful edges to search the node. + break; } } return query; diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java index 842616161b0..b96055d6126 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java @@ -58,6 +58,16 @@ NodeQuery anyElement() { return this; } + NodeQuery anyMemberName() { + queries.add(node -> { + if (node == null || !node.isObjectNode()) { + return Stream.empty(); + } + return node.expectObjectNode().getMembers().keySet().stream(); + }); + return this; + } + List execute(Node node) { if (queries.isEmpty()) { return ListUtils.of(); @@ -72,6 +82,6 @@ List execute(Node node) { @FunctionalInterface interface Query { - Stream run(Node node); + Stream run(Node node); } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy index c52f2c521f8..f73bd85e7bb 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy @@ -19,13 +19,14 @@ operation GetFoo { eight: Eight nine: Nine ten: Ten + eleven: Eleven } } // -- @trait structure withIdRefOnMember { - @idRef(failWhenMissing: true, selector: "*") + @idRef(failWhenMissing: true) ref: String } @@ -40,7 +41,7 @@ structure withIdRefOnMemberTarget { ref: OnTarget } -@idRef(failWhenMissing: true, selector: "*") +@idRef(failWhenMissing: true) string OnTarget @withIdRefOnMemberTarget(ref: Ref2) @@ -55,7 +56,7 @@ structure withIdRefOnNestedStructureMember { } structure Nested { - @idRef(failWhenMissing: true, selector: "*") + @idRef(failWhenMissing: true) member: String } @@ -75,7 +76,7 @@ list withIdRefOnListMemberTarget { member: OnListMemberTarget } -@idRef(failWhenMissing: true, selector: "*") +@idRef(failWhenMissing: true) string OnListMemberTarget @withIdRefOnListMemberTarget([ @@ -87,7 +88,7 @@ structure Ref4 {} // -- @trait -@idRef(failWhenMissing: true, selector: "*") +@idRef(failWhenMissing: true) string withIdRefOnSelf @withIdRefOnSelf(Ref5) @@ -126,7 +127,7 @@ structure withIdRefThroughMixin with [ThroughMixin] {} @mixin structure ThroughMixin { - @idRef(failWhenMissing: true, selector: "*") + @idRef(failWhenMissing: true) ref: String } @@ -142,7 +143,7 @@ map withIdRefOnMapValue { value: OnMap } -@idRef(failWhenMissing: true, selector: "*") +@idRef(failWhenMissing: true) string OnMap @withIdRefOnMapValue({ @@ -169,18 +170,15 @@ structure Ten {} structure Ref10 {} // -- -// NOTE: This actually doesn't work because map key of 'Ref11' it says isn't a valid -// shape id. - -//@trait -//map withIdRefOnMapKey { -// key: OnMap -// value: String -//} +@trait +map withIdRefOnMapKey { + key: OnMap + value: String +} -//@withIdRefOnMapKey({ -// Ref11: "foo" -//}) -//structure Eleven {} +@withIdRefOnMapKey({ + "com.foo#Ref11": "foo" +}) +structure Eleven {} -//structure Ref11 {} +structure Ref11 {} From 505285e021375444773935f225fa651cdd7440aa Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Mon, 22 Jan 2024 14:42:36 -0500 Subject: [PATCH 4/6] Refactor to create an ID_REF relationship type Previous impl wouldn't consider shapes referenced through multiple levels of idRef as connected, since idRef'd shapes could themselves be connected to other shapes that have idRef references. Refactoring to a custom NeighborProvider means the shape walker just traverses those relationships in a single pass. --- .../knowledge/NeighborProviderIndex.java | 23 +++ ...nces.java => IdRefShapeRelationships.java} | 42 ++--- .../model/neighbor/NeighborProvider.java | 24 +++ .../model/neighbor/RelationshipType.java | 34 +++- .../model/neighbor/UnreferencedShapes.java | 13 +- .../model/neighbor/NeighborProviderTest.java | 96 +++++++++++ .../smithy/model/neighbor/NodeQueryTest.java | 152 ++++++++++++++++++ .../neighbor/UnreferencedShapesTest.java | 9 +- .../idref-neighbors-in-trait-def.smithy | 34 ++++ .../idref-neighbors-multiple-levels.smithy | 32 ++++ .../idref-neighbors-unconnected.smithy | 32 ++++ ...gh-idref.smithy => idref-neighbors.smithy} | 36 +++++ .../neighbor/through-idref-unconnected.smithy | 12 -- 13 files changed, 493 insertions(+), 46 deletions(-) rename smithy-model/src/main/java/software/amazon/smithy/model/neighbor/{IdRefShapeReferences.java => IdRefShapeRelationships.java} (71%) create mode 100644 smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-in-trait-def.smithy create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-multiple-levels.smithy create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-unconnected.smithy rename smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/{through-idref.smithy => idref-neighbors.smithy} (83%) delete mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref-unconnected.smithy diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NeighborProviderIndex.java b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NeighborProviderIndex.java index 40901019732..acc7a58eaac 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NeighborProviderIndex.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NeighborProviderIndex.java @@ -32,6 +32,7 @@ public final class NeighborProviderIndex implements KnowledgeIndex { private volatile NeighborProvider reversed; private volatile NeighborProvider providerWithTraits; private volatile NeighborProvider reversedWithTraits; + private volatile NeighborProvider providerWithIdRefs; public NeighborProviderIndex(Model model) { provider = NeighborProvider.precomputed(model); @@ -76,6 +77,28 @@ public NeighborProvider getProviderWithTraitRelationships() { return result; } + /** + * Gets the neighbor provider that includes idRef relationships. + * + * @return Returns the provider. + */ + public NeighborProvider getProviderWithIdRefRelationships() { + NeighborProvider result = providerWithIdRefs; + + if (result == null) { + Model model = getOrThrowModel(); + synchronized (this) { + result = providerWithIdRefs; + if (result == null) { + providerWithIdRefs = result = NeighborProvider.cached( + NeighborProvider.withIdRefRelationships(model, provider)); + } + } + } + + return result; + } + /** * Gets a reversed, bottom up neighbor provider. * diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeRelationships.java similarity index 71% rename from smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java rename to smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeRelationships.java index dea9ad707d0..b864cfdba38 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeReferences.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeRelationships.java @@ -5,8 +5,10 @@ package software.amazon.smithy.model.neighbor; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.node.Node; @@ -21,8 +23,7 @@ import software.amazon.smithy.model.traits.TraitDefinition; /** - * Finds all shapes referenced by {@link IdRefTrait} from within trait - * values. + * Finds all {@link RelationshipType#ID_REF} relationships in a model. * *

This works by finding all paths from {@link TraitDefinition} shapes * to shapes with {@link IdRefTrait}, then using those paths to search @@ -30,46 +31,47 @@ * value. Because we don't have a fixed set of traits known to potentially have * idRef members, this has to be done dynamically. */ -final class IdRefShapeReferences { +final class IdRefShapeRelationships { private static final Selector WITH_ID_REF = Selector.parse("[trait|idRef]"); private final Model model; - private final Set found = new HashSet<>(); + private final Map> relationships = new HashMap<>(); - IdRefShapeReferences(Model model) { + IdRefShapeRelationships(Model model) { this.model = model; } - Set compute(Set connected) { + Map> getRelationships() { PathFinder finder = PathFinder.create(model); for (Shape traitDef : model.getShapesWithTrait(TraitDefinition.class)) { if (traitDef.hasTrait(IdRefTrait.class)) { // PathFinder doesn't handle the case where the trait def has the idRef NodeQuery query = new NodeQuery().self(); - addReferences(traitDef, query, connected); + addRelationships(traitDef, query); continue; } List paths = finder.search(traitDef, WITH_ID_REF); if (!paths.isEmpty()) { for (PathFinder.Path path : paths) { NodeQuery query = buildNodeQuery(path); - addReferences(traitDef, query, connected); + addRelationships(traitDef, query); } } } - return found; + return relationships; } - private void addReferences(Shape traitDef, NodeQuery query, Set connected) { - model.getShapesWithTrait(traitDef.getId()).stream() - .filter(connected::contains) - .forEach(shape -> { - Trait trait = shape.findTrait(traitDef.getId()).get(); // We already know the shape has the trait. - Node node = trait.toNode(); - query.execute(node).forEach(n -> - // Invalid shape ids are handled by the idRef trait validator, so ignore them here. - n.asStringNode().flatMap(StringNode::asShapeId).ifPresent(found::add) - ); - }); + private void addRelationships(Shape traitDef, NodeQuery query) { + model.getShapesWithTrait(traitDef.getId()).forEach(shape -> { + Trait trait = shape.findTrait(traitDef.getId()).get(); // We already know the shape has the trait. + Node node = trait.toNode(); + // Invalid shape ids are handled by the idRef trait validator, so ignore them here. + query.execute(node).forEach(n -> n.asStringNode() + .flatMap(StringNode::asShapeId) + .flatMap(model::getShape) + .map(referenced -> Relationship.create(shape, RelationshipType.ID_REF, referenced)) + .ifPresent(rel -> relationships + .computeIfAbsent(rel.getShape().getId(), id -> new HashSet<>()).add(rel))); + }); } private static NodeQuery buildNodeQuery(PathFinder.Path path) { diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborProvider.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborProvider.java index 76978d85c55..00f8dedf367 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborProvider.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NeighborProvider.java @@ -26,6 +26,7 @@ import software.amazon.smithy.model.Model; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.traits.IdRefTrait; import software.amazon.smithy.utils.ListUtils; /** @@ -91,6 +92,29 @@ static NeighborProvider withTraitRelationships(Model model, NeighborProvider nei }; } + /** + * Creates a NeighborProvider that includes {@link RelationshipType#ID_REF} + * relationships. + * + * @param model Model to use to look up shapes referenced by {@link IdRefTrait}. + * @param neighborProvider Provider to wrap. + * @return Returns the created neighbor provider. + */ + static NeighborProvider withIdRefRelationships(Model model, NeighborProvider neighborProvider) { + Map> idRefRelationships = new IdRefShapeRelationships(model).getRelationships(); + return shape -> { + List relationships = neighborProvider.getNeighbors(shape); + + if (!idRefRelationships.containsKey(shape.getId())) { + return relationships; + } + + relationships = new ArrayList<>(relationships); + relationships.addAll(idRefRelationships.get(shape.getId())); + return relationships; + }; + } + /** * Creates a NeighborProvider that precomputes the neighbors of a model. * diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/RelationshipType.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/RelationshipType.java index 97aaceb5b0d..633154676aa 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/RelationshipType.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/RelationshipType.java @@ -26,8 +26,10 @@ import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.ResourceShape; import software.amazon.smithy.model.shapes.SetShape; +import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.shapes.UnionShape; +import software.amazon.smithy.model.traits.IdRefTrait; import software.amazon.smithy.model.traits.TraitDefinition; import software.amazon.smithy.utils.SmithyInternalApi; @@ -214,7 +216,37 @@ public enum RelationshipType { * Relationship that exists between a structure or union and a mixin applied * to the shape. */ - MIXIN("mixin", RelationshipDirection.DIRECTED); + MIXIN("mixin", RelationshipDirection.DIRECTED), + + /** + * Relationships that exist between a shape and another shape referenced by an + * {@link IdRefTrait}. + * + *

This relationship is formed by applying a trait with a value containing a + * reference to another {@link ShapeId}. For + * example: + * 

+     * {@code
+     * @trait
+     * structure myRef {
+     *     @idRef
+     *     shape: String
+     * }
+     *
+     * // @myRef trait applied, and the value references the shape `Referenced`
+     * @myRef(shape: Referenced)
+     * structure WithMyRef {}
+     *
+     * string Referenced
+     * }
+     *
+ * + *

This kind of relationship is not returned by default from a + * {@link NeighborProvider}. You must explicitly wrap a {@link NeighborProvider} + * with {@link NeighborProvider#withIdRefRelationships(Model, NeighborProvider)} + * in order to yield idRef relationships. + */ + ID_REF(null, RelationshipDirection.DIRECTED); private String selectorLabel; private RelationshipDirection direction; diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java index a5c96d8a546..3455d116435 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java @@ -22,13 +22,14 @@ import software.amazon.smithy.model.knowledge.NeighborProviderIndex; import software.amazon.smithy.model.loader.Prelude; import software.amazon.smithy.model.shapes.Shape; -import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.traits.TraitDefinition; import software.amazon.smithy.utils.FunctionalUtils; /** * Finds shapes that are not connected to a service shape, are not trait - * definitions, and are not referenced by trait definitions. + * definitions, are not referenced by trait definitions, and are not + * referenced in trait values through + * {@link software.amazon.smithy.model.traits.IdRefTrait}. * *

Prelude shapes are never considered unreferenced. */ @@ -54,7 +55,7 @@ public UnreferencedShapes(Predicate keepFilter) { * @return Returns the unreferenced shapes. */ public Set compute(Model model) { - Walker shapeWalker = new Walker(NeighborProviderIndex.of(model).getProvider()); + Walker shapeWalker = new Walker(NeighborProviderIndex.of(model).getProviderWithIdRefRelationships()); // Find all shapes connected to any service shape. Set connected = new HashSet<>(); @@ -67,12 +68,6 @@ public Set compute(Model model) { connected.addAll(shapeWalker.walkShapes(trait)); } - for (ShapeId referencedThroughIdRef : new IdRefShapeReferences(model).compute(connected)) { - // Referenced shapes may not exist in the model, but we don't want to throw. - model.getShape(referencedThroughIdRef) - .ifPresent(shape -> connected.addAll(shapeWalker.walkShapes(shape))); - } - // Any shape that wasn't identified as connected to a service is considered unreferenced. Set result = new HashSet<>(); for (Shape shape : model.toSet()) { diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborProviderTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborProviderTest.java index eedc0d27da7..9bb9c49e6df 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborProviderTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborProviderTest.java @@ -1,13 +1,18 @@ package software.amazon.smithy.model.neighbor; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import java.util.List; +import java.util.stream.Collectors; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.StringShape; import software.amazon.smithy.model.traits.SensitiveTrait; @@ -42,4 +47,95 @@ public void canGetTraitRelationshipsFromShapeWithNoTraits() { assertThat(relationships, empty()); } + + @ParameterizedTest + @CsvSource({ + "One,Ref1", + "Two,Ref2", + "Three,Ref3", + "Four,Ref4", + "Five,Ref5", + "Six,Ref6", + "Seven,Ref7", + "Eight,Ref8", + "Nine,Ref9", + "Ten,Ref10", + "Eleven,Ref11", + "Twelve,Ref12", + "Thirteen,Ref13", + "Fourteen,Ref14" + }) + public void canGetIdRefRelationships(String shapeName, String referencedShapeName) { + Model model = Model.assembler() + .addImport(getClass().getResource("idref-neighbors.smithy")) + .assemble() + .unwrap(); + NeighborProvider provider = NeighborProvider.of(model); + provider = NeighborProvider.withIdRefRelationships(model, provider); + + Shape shape = model.expectShape(ShapeId.fromParts("com.foo", shapeName)); + Shape ref = model.expectShape(ShapeId.fromParts("com.foo", referencedShapeName)); + List relationships = provider.getNeighbors(shape).stream() + .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) + .collect(Collectors.toList()); + + assertThat(relationships, containsInAnyOrder( + equalTo(Relationship.create(shape, RelationshipType.ID_REF, ref)))); + } + + @Test + public void canGetIdRefRelationshipsThroughTraitDefs() { + Model model = Model.assembler() + .addImport(getClass().getResource("idref-neighbors-in-trait-def.smithy")) + .assemble() + .unwrap(); + NeighborProvider provider = NeighborProvider.of(model); + provider = NeighborProvider.withIdRefRelationships(model, provider); + + Shape shape = model.expectShape(ShapeId.from("com.foo#WithRefStructTrait")); + Shape ref = model.expectShape(ShapeId.from("com.foo#OtherReferenced")); + List relationships = provider.getNeighbors(shape).stream() + .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) + .collect(Collectors.toList()); + + assertThat(relationships, containsInAnyOrder( + equalTo(Relationship.create(shape, RelationshipType.ID_REF, ref)))); + + Shape shape1 = model.expectShape(ShapeId.from("com.foo#refStruct$other")); + Shape ref1 = model.expectShape(ShapeId.from("com.foo#ReferencedInTraitDef")); + List relationships1 = provider.getNeighbors(shape1).stream() + .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) + .collect(Collectors.toList()); + + assertThat(relationships1, containsInAnyOrder( + equalTo(Relationship.create(shape1, RelationshipType.ID_REF, ref1)))); + } + + @Test + public void canGetIdRefRelationshipsThroughMultipleLevelsOfIdRef() { + Model model = Model.assembler() + .addImport(getClass().getResource("idref-neighbors-multiple-levels.smithy")) + .assemble() + .unwrap(); + NeighborProvider provider = NeighborProvider.of(model); + provider = NeighborProvider.withIdRefRelationships(model, provider); + + Shape shape = model.expectShape(ShapeId.from("com.foo#WithIdRef")); + Shape ref = model.expectShape(ShapeId.from("com.foo#Referenced")); + List relationships = provider.getNeighbors(shape).stream() + .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) + .collect(Collectors.toList()); + + assertThat(relationships, containsInAnyOrder( + Relationship.create(shape, RelationshipType.ID_REF, ref))); + + Shape shape1 = model.expectShape(ShapeId.from("com.foo#ConnectedThroughReferenced")); + Shape ref1 = model.expectShape(ShapeId.from("com.foo#AnotherReferenced")); + List relationships1 = provider.getNeighbors(shape1).stream() + .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) + .collect(Collectors.toList()); + + assertThat(relationships1, containsInAnyOrder( + Relationship.create(shape1, RelationshipType.ID_REF, ref1))); + } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java new file mode 100644 index 00000000000..180ad55811d --- /dev/null +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java @@ -0,0 +1,152 @@ +package software.amazon.smithy.model.neighbor; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.hasSize; + +import java.util.List; +import org.junit.jupiter.api.Test; +import software.amazon.smithy.model.node.Node; +import software.amazon.smithy.model.node.StringNode; + +public class NodeQueryTest { + @Test + public void noQueriesGivesNoResults() { + Node node = Node.from("{}"); + List result = new NodeQuery().execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void self() { + Node node = Node.from("{}"); + List result = new NodeQuery().self().execute(node); + assertThat(result, containsInAnyOrder(node)); + } + + @Test + public void selfCanBeAppliedMultipleTimes() { + Node node = Node.from("{}"); + List result = new NodeQuery().self().self().self().execute(node); + assertThat(result, containsInAnyOrder(node)); + } + + @Test + public void member() { + Node member = StringNode.from("bar"); + Node node = Node.objectNode().withMember("foo", member); + List result = new NodeQuery().member("foo").execute(node); + assertThat(result, containsInAnyOrder(member)); + } + + @Test + public void anyMember() { + Node member1 = StringNode.from("member-one"); + Node member2 = StringNode.from("member-two"); + Node node = Node.objectNode().withMember("one", member1).withMember("two", member2); + List result = new NodeQuery().anyMember().execute(node); + assertThat(result, containsInAnyOrder(member1, member2)); + } + + @Test + public void anyElement() { + Node element1 = StringNode.from("element-one"); + Node element2 = StringNode.from("element-two"); + Node node = Node.arrayNode(element1, element2); + List result = new NodeQuery().anyElement().execute(node); + assertThat(result, containsInAnyOrder(element1, element2)); + } + + @Test + public void anyMemberName() { + StringNode key1 = StringNode.from("one"); + StringNode key2 = StringNode.from("two"); + Node member1 = StringNode.from("member-one"); + Node member2 = StringNode.from("member-two"); + Node node = Node.objectNode().withMember(key1, member1).withMember(key2, member2); + List result = new NodeQuery().anyMemberName().execute(node); + assertThat(result, containsInAnyOrder(key1, key2)); + } + + @Test + public void memberGivesNoResultsOnNonObjectNode() { + Node node = Node.from("[{\"foo\": 0}]"); + List result = new NodeQuery().member("foo").execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void memberGivesNoResultsIfMemberNameNotFound() { + Node node = Node.from("{\"a\": 0, \"b\": 0}"); + List result = new NodeQuery().member("foo").execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void anyMemberGivesNoResultsOnNonObjectNode() { + Node node = Node.from("[{\"foo\": 0}]"); + List result = new NodeQuery().anyMember().execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void anyMemberGivesNoResultsOnEmptyObjectNode() { + Node node = Node.from("{}"); + List result = new NodeQuery().anyMember().execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void anyElementGivesNoResultsOnNonArrayNode() { + Node node = Node.from("{\"foo\": [0]}"); + List result = new NodeQuery().anyElement().execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void anyElementGivesNoResultsOnEmptyArrayNode() { + Node node = Node.from("[]"); + List result = new NodeQuery().anyElement().execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void anyMemberNameGivesNoResultsOnNonObjectNode() { + Node node = Node.from("1"); + List result = new NodeQuery().anyMemberName().execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void anyMemberNameGivesNoResultsOnEmptyObject() { + Node node = Node.from("{}"); + List result = new NodeQuery().anyMemberName().execute(node); + assertThat(result, hasSize(0)); + } + + @Test + public void eachQueryExecuteOnResultOfPreviousQuery() { + Node element1 = Node.from(0); + Node element2 = Node.from("{}"); + Node element3 = Node.from("element3"); + Node obj = Node.objectNode().withMember("foo", Node.objectNode() + .withMember("arr1", Node.arrayNode(element1)) + .withMember("arr2", Node.arrayNode(element2)) + .withMember("arr3", Node.arrayNode(element3))); + Node node = Node.arrayNode(obj, obj); + + List result = new NodeQuery() + .anyElement() + .member("foo") + .anyMember() + .anyElement() + .execute(node); + assertThat(result, containsInAnyOrder( + element1, + element2, + element3, + element1, + element2, + element3)); + } +} diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java index d456b87ee42..382baf327b9 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java @@ -84,7 +84,7 @@ private Model createPrivateShapeModel(ShapeId id) { @Test public void checksShapeReferencesThroughIdRef() { Model m = Model.assembler() - .addImport(getClass().getResource("through-idref.smithy")) + .addImport(getClass().getResource("idref-neighbors.smithy")) .assemble() .unwrap(); @@ -93,16 +93,17 @@ public void checksShapeReferencesThroughIdRef() { } @Test - public void doesNotCheckShapeReferencesThroughIdRefOnUnconnectedTraits() { + public void doesNotCheckShapeReferencesThroughIdRefOnUnconnectedShapes() { Model m = Model.assembler() - .addImport(getClass().getResource("through-idref-unconnected.smithy")) + .addImport(getClass().getResource("idref-neighbors-unconnected.smithy")) .assemble() .unwrap(); Set ids = new UnreferencedShapes().compute(m).stream().map(Shape::getId).collect(Collectors.toSet()); assertThat(ids, containsInAnyOrder( ShapeId.from("com.foo#WithTrait"), - ShapeId.from("com.foo#Referenced") + ShapeId.from("com.foo#Referenced"), + ShapeId.from("com.foo#Unconnected") )); } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-in-trait-def.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-in-trait-def.smithy new file mode 100644 index 00000000000..59c2303047f --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-in-trait-def.smithy @@ -0,0 +1,34 @@ +$version: "2.0" + +namespace com.foo + +service FooService { + version: "2024-01-22" + operations: [GetFoo] +} + +operation GetFoo { + input := { + withRefStructTrait: WithRefStructTrait + } +} + +@trait +@idRef(failWhenMissing: true) +string ref + +@trait +structure refStruct { + @ref(ReferencedInTraitDef) + other: String + + @idRef(failWhenMissing: true) + ref: String +} + +string ReferencedInTraitDef + +@refStruct(other: "foo", ref: OtherReferenced) +structure WithRefStructTrait {} + +string OtherReferenced diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-multiple-levels.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-multiple-levels.smithy new file mode 100644 index 00000000000..211cf736d59 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-multiple-levels.smithy @@ -0,0 +1,32 @@ +$version: "2.0" + +namespace com.foo + +service FooService { + version: "2024-01-22" + operations: [GetFoo] +} + +operation GetFoo { + input := { + withIdRef: WithIdRef + } +} + +@trait +@idRef(failWhenMissing: true) +string ref + +@ref(Referenced) +structure WithIdRef {} + +structure Referenced { + connectedThroughReferenced: ConnectedThroughReferenced +} + +// Only connected through `Referenced`, which itself is only +// connected via idRef. +@ref(AnotherReferenced) +structure ConnectedThroughReferenced {} + +string AnotherReferenced diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-unconnected.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-unconnected.smithy new file mode 100644 index 00000000000..790028fe45b --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-unconnected.smithy @@ -0,0 +1,32 @@ +$version: "2.0" + +namespace com.foo + +service FooService { + version: "2024-01-22" + operations: [GetFoo] +} + +operation GetFoo { + input := { + withReferencedByUnconnected: WithReferencedByUnconnected + } +} + +@trait +@idRef(failWhenMissing: true) +string ref + +@ref(Referenced) +structure WithTrait {} + +structure Referenced {} + +@ref(ReferencedByUnconnected) +structure WithReferencedByUnconnected {} + +string ReferencedByUnconnected + +structure Unconnected { + ref: ReferencedByUnconnected +} \ No newline at end of file diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors.smithy similarity index 83% rename from smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy rename to smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors.smithy index f73bd85e7bb..f5b9e119f52 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors.smithy @@ -20,6 +20,9 @@ operation GetFoo { nine: Nine ten: Ten eleven: Eleven + twelve: Twelve + thirteen: Thirteen + fourteen: Fourteen } } @@ -182,3 +185,36 @@ map withIdRefOnMapKey { structure Eleven {} structure Ref11 {} + +// -- +@trait +@idRef(failWhenMissing: true) +string ref + +@ref(Ref12) +structure Twelve {} + +structure Ref12 { + connectedToRef13: ConnectedToRef13 +} + +structure ConnectedToRef13 { + ref13: Ref13 +} + +@ref(Ref13) +structure Thirteen {} + +structure Ref13 { + connectedToRef14: ConnectedToRef14 +} + +structure ConnectedToRef14 { + ref14: Ref14 +} + +@ref(Ref14) +structure Fourteen {} + +string Ref14 + diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref-unconnected.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref-unconnected.smithy deleted file mode 100644 index 77346b7f393..00000000000 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/through-idref-unconnected.smithy +++ /dev/null @@ -1,12 +0,0 @@ -$version: "2.0" - -namespace com.foo - -@trait -@idRef(failWhenMissing: true, selector: "*") -string unconnected - -@unconnected(Referenced) -structure WithTrait {} - -structure Referenced {} From 67b47d31035006500dda594d0ea122b27ae3229e Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Tue, 23 Jan 2024 17:12:24 -0500 Subject: [PATCH 5/6] Don't cache idRef provider, add test case for enum Also makes most NodeQuery queries static instances, and moves some stuff around in tests. --- .../knowledge/NeighborProviderIndex.java | 23 ---------- .../smithy/model/neighbor/NodeQuery.java | 46 +++++++++++-------- .../model/neighbor/UnreferencedShapes.java | 4 +- .../model/neighbor/NeighborProviderTest.java | 16 ++----- .../smithy/model/neighbor/NodeQueryTest.java | 31 +++++++++++++ .../neighbor/UnreferencedShapesTest.java | 2 + .../idref-neighbors-unconnected.smithy | 2 +- .../model/neighbor/idref-neighbors.smithy | 16 +++++++ 8 files changed, 84 insertions(+), 56 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NeighborProviderIndex.java b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NeighborProviderIndex.java index acc7a58eaac..40901019732 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NeighborProviderIndex.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NeighborProviderIndex.java @@ -32,7 +32,6 @@ public final class NeighborProviderIndex implements KnowledgeIndex { private volatile NeighborProvider reversed; private volatile NeighborProvider providerWithTraits; private volatile NeighborProvider reversedWithTraits; - private volatile NeighborProvider providerWithIdRefs; public NeighborProviderIndex(Model model) { provider = NeighborProvider.precomputed(model); @@ -77,28 +76,6 @@ public NeighborProvider getProviderWithTraitRelationships() { return result; } - /** - * Gets the neighbor provider that includes idRef relationships. - * - * @return Returns the provider. - */ - public NeighborProvider getProviderWithIdRefRelationships() { - NeighborProvider result = providerWithIdRefs; - - if (result == null) { - Model model = getOrThrowModel(); - synchronized (this) { - result = providerWithIdRefs; - if (result == null) { - providerWithIdRefs = result = NeighborProvider.cached( - NeighborProvider.withIdRefRelationships(model, provider)); - } - } - } - - return result; - } - /** * Gets a reversed, bottom up neighbor provider. * diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java index b96055d6126..97ba84d3bad 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java @@ -18,13 +18,36 @@ * and the results are aggregated. */ final class NodeQuery { + private static final Query SELF = Stream::of; + + private static final Query ANY_MEMBER = (node) -> { + if (node == null || !node.isObjectNode()) { + return Stream.empty(); + } + return node.expectObjectNode().getMembers().values().stream(); + }; + + private static final Query ANY_ELEMENT = (node) -> { + if (node == null || !node.isArrayNode()) { + return Stream.empty(); + } + return node.expectArrayNode().getElements().stream(); + }; + + private static final Query ANY_MEMBER_NAME = (node) -> { + if (node == null || !node.isObjectNode()) { + return Stream.empty(); + } + return node.expectObjectNode().getMembers().keySet().stream(); + }; + private final List queries = new ArrayList<>(); NodeQuery() { } NodeQuery self() { - queries.add(Stream::of); + queries.add(SELF); return this; } @@ -39,32 +62,17 @@ NodeQuery member(String name) { } NodeQuery anyMember() { - queries.add(node -> { - if (node == null || !node.isObjectNode()) { - return Stream.empty(); - } - return node.expectObjectNode().getMembers().values().stream(); - }); + queries.add(ANY_MEMBER); return this; } NodeQuery anyElement() { - queries.add(node -> { - if (node == null || !node.isArrayNode()) { - return Stream.empty(); - } - return node.expectArrayNode().getElements().stream(); - }); + queries.add(ANY_ELEMENT); return this; } NodeQuery anyMemberName() { - queries.add(node -> { - if (node == null || !node.isObjectNode()) { - return Stream.empty(); - } - return node.expectObjectNode().getMembers().keySet().stream(); - }); + queries.add(ANY_MEMBER_NAME); return this; } diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java index 3455d116435..e412f885a48 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/UnreferencedShapes.java @@ -55,7 +55,9 @@ public UnreferencedShapes(Predicate keepFilter) { * @return Returns the unreferenced shapes. */ public Set compute(Model model) { - Walker shapeWalker = new Walker(NeighborProviderIndex.of(model).getProviderWithIdRefRelationships()); + NeighborProvider baseProvider = NeighborProviderIndex.of(model).getProvider(); + NeighborProvider providerWithIdRefRelationships = NeighborProvider.withIdRefRelationships(model, baseProvider); + Walker shapeWalker = new Walker(providerWithIdRefRelationships); // Find all shapes connected to any service shape. Set connected = new HashSet<>(); diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborProviderTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborProviderTest.java index 9bb9c49e6df..15162e5a240 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborProviderTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NeighborProviderTest.java @@ -97,18 +97,14 @@ public void canGetIdRefRelationshipsThroughTraitDefs() { List relationships = provider.getNeighbors(shape).stream() .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) .collect(Collectors.toList()); - - assertThat(relationships, containsInAnyOrder( - equalTo(Relationship.create(shape, RelationshipType.ID_REF, ref)))); - Shape shape1 = model.expectShape(ShapeId.from("com.foo#refStruct$other")); Shape ref1 = model.expectShape(ShapeId.from("com.foo#ReferencedInTraitDef")); List relationships1 = provider.getNeighbors(shape1).stream() .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) .collect(Collectors.toList()); - assertThat(relationships1, containsInAnyOrder( - equalTo(Relationship.create(shape1, RelationshipType.ID_REF, ref1)))); + assertThat(relationships, containsInAnyOrder(Relationship.create(shape, RelationshipType.ID_REF, ref))); + assertThat(relationships1, containsInAnyOrder(Relationship.create(shape1, RelationshipType.ID_REF, ref1))); } @Test @@ -125,17 +121,13 @@ public void canGetIdRefRelationshipsThroughMultipleLevelsOfIdRef() { List relationships = provider.getNeighbors(shape).stream() .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) .collect(Collectors.toList()); - - assertThat(relationships, containsInAnyOrder( - Relationship.create(shape, RelationshipType.ID_REF, ref))); - Shape shape1 = model.expectShape(ShapeId.from("com.foo#ConnectedThroughReferenced")); Shape ref1 = model.expectShape(ShapeId.from("com.foo#AnotherReferenced")); List relationships1 = provider.getNeighbors(shape1).stream() .filter(relationship -> relationship.getRelationshipType().equals(RelationshipType.ID_REF)) .collect(Collectors.toList()); - assertThat(relationships1, containsInAnyOrder( - Relationship.create(shape1, RelationshipType.ID_REF, ref1))); + assertThat(relationships, containsInAnyOrder(Relationship.create(shape, RelationshipType.ID_REF, ref))); + assertThat(relationships1, containsInAnyOrder(Relationship.create(shape1, RelationshipType.ID_REF, ref1))); } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java index 180ad55811d..cdfcb03caf2 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java @@ -13,21 +13,27 @@ public class NodeQueryTest { @Test public void noQueriesGivesNoResults() { Node node = Node.from("{}"); + List result = new NodeQuery().execute(node); + assertThat(result, hasSize(0)); } @Test public void self() { Node node = Node.from("{}"); + List result = new NodeQuery().self().execute(node); + assertThat(result, containsInAnyOrder(node)); } @Test public void selfCanBeAppliedMultipleTimes() { Node node = Node.from("{}"); + List result = new NodeQuery().self().self().self().execute(node); + assertThat(result, containsInAnyOrder(node)); } @@ -35,7 +41,9 @@ public void selfCanBeAppliedMultipleTimes() { public void member() { Node member = StringNode.from("bar"); Node node = Node.objectNode().withMember("foo", member); + List result = new NodeQuery().member("foo").execute(node); + assertThat(result, containsInAnyOrder(member)); } @@ -44,7 +52,9 @@ public void anyMember() { Node member1 = StringNode.from("member-one"); Node member2 = StringNode.from("member-two"); Node node = Node.objectNode().withMember("one", member1).withMember("two", member2); + List result = new NodeQuery().anyMember().execute(node); + assertThat(result, containsInAnyOrder(member1, member2)); } @@ -53,7 +63,9 @@ public void anyElement() { Node element1 = StringNode.from("element-one"); Node element2 = StringNode.from("element-two"); Node node = Node.arrayNode(element1, element2); + List result = new NodeQuery().anyElement().execute(node); + assertThat(result, containsInAnyOrder(element1, element2)); } @@ -64,63 +76,81 @@ public void anyMemberName() { Node member1 = StringNode.from("member-one"); Node member2 = StringNode.from("member-two"); Node node = Node.objectNode().withMember(key1, member1).withMember(key2, member2); + List result = new NodeQuery().anyMemberName().execute(node); + assertThat(result, containsInAnyOrder(key1, key2)); } @Test public void memberGivesNoResultsOnNonObjectNode() { Node node = Node.from("[{\"foo\": 0}]"); + List result = new NodeQuery().member("foo").execute(node); + assertThat(result, hasSize(0)); } @Test public void memberGivesNoResultsIfMemberNameNotFound() { Node node = Node.from("{\"a\": 0, \"b\": 0}"); + List result = new NodeQuery().member("foo").execute(node); + assertThat(result, hasSize(0)); } @Test public void anyMemberGivesNoResultsOnNonObjectNode() { Node node = Node.from("[{\"foo\": 0}]"); + List result = new NodeQuery().anyMember().execute(node); + assertThat(result, hasSize(0)); } @Test public void anyMemberGivesNoResultsOnEmptyObjectNode() { Node node = Node.from("{}"); + List result = new NodeQuery().anyMember().execute(node); + assertThat(result, hasSize(0)); } @Test public void anyElementGivesNoResultsOnNonArrayNode() { Node node = Node.from("{\"foo\": [0]}"); + List result = new NodeQuery().anyElement().execute(node); + assertThat(result, hasSize(0)); } @Test public void anyElementGivesNoResultsOnEmptyArrayNode() { Node node = Node.from("[]"); + List result = new NodeQuery().anyElement().execute(node); + assertThat(result, hasSize(0)); } @Test public void anyMemberNameGivesNoResultsOnNonObjectNode() { Node node = Node.from("1"); + List result = new NodeQuery().anyMemberName().execute(node); + assertThat(result, hasSize(0)); } @Test public void anyMemberNameGivesNoResultsOnEmptyObject() { Node node = Node.from("{}"); + List result = new NodeQuery().anyMemberName().execute(node); + assertThat(result, hasSize(0)); } @@ -141,6 +171,7 @@ public void eachQueryExecuteOnResultOfPreviousQuery() { .anyMember() .anyElement() .execute(node); + assertThat(result, containsInAnyOrder( element1, element2, diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java index 382baf327b9..7164cedd623 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/UnreferencedShapesTest.java @@ -89,6 +89,7 @@ public void checksShapeReferencesThroughIdRef() { .unwrap(); Set shapes = new UnreferencedShapes().compute(m); + assertThat(shapes, empty()); } @@ -100,6 +101,7 @@ public void doesNotCheckShapeReferencesThroughIdRefOnUnconnectedShapes() { .unwrap(); Set ids = new UnreferencedShapes().compute(m).stream().map(Shape::getId).collect(Collectors.toSet()); + assertThat(ids, containsInAnyOrder( ShapeId.from("com.foo#WithTrait"), ShapeId.from("com.foo#Referenced"), diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-unconnected.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-unconnected.smithy index 790028fe45b..1d1e2443427 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-unconnected.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors-unconnected.smithy @@ -29,4 +29,4 @@ string ReferencedByUnconnected structure Unconnected { ref: ReferencedByUnconnected -} \ No newline at end of file +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors.smithy index f5b9e119f52..ab45d60fd9d 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/neighbor/idref-neighbors.smithy @@ -23,6 +23,7 @@ operation GetFoo { twelve: Twelve thirteen: Thirteen fourteen: Fourteen + fifteen: Fifteen } } @@ -218,3 +219,18 @@ structure Fourteen {} string Ref14 +// -- +@trait +structure withIdRefOnEnum { + refEnum: RefEnum +} + +@idRef(failWhenMissing: true) +enum RefEnum { + REF15 = "com.foo#Ref15" +} + +@withIdRefOnEnum(refEnum: "com.foo#Ref15") +structure Fifteen {} + +structure Ref15 {} From e95797890ee50cdd85895c68e626cb58a4ec707c Mon Sep 17 00:00:00 2001 From: Miles Ziemer Date: Tue, 23 Jan 2024 18:37:02 -0500 Subject: [PATCH 6/6] Use queue instead of streams in NodeQuery Changes NodeQuery to use a single instance of a Queue for keeping track of and aggregating query results to avoid extra allocations within queries and when joining the results. --- .../neighbor/IdRefShapeRelationships.java | 1 + .../smithy/model/neighbor/NodeQuery.java | 51 +++++++++++-------- .../smithy/model/neighbor/NodeQueryTest.java | 34 ++++++------- 3 files changed, 47 insertions(+), 39 deletions(-) diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeRelationships.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeRelationships.java index b864cfdba38..7466afb6174 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeRelationships.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/IdRefShapeRelationships.java @@ -33,6 +33,7 @@ */ final class IdRefShapeRelationships { private static final Selector WITH_ID_REF = Selector.parse("[trait|idRef]"); + private final Model model; private final Map> relationships = new HashMap<>(); diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java index 97ba84d3bad..c01738d1850 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/NodeQuery.java @@ -5,12 +5,12 @@ package software.amazon.smithy.model.neighbor; +import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Collection; import java.util.List; -import java.util.stream.Collectors; -import java.util.stream.Stream; +import java.util.Queue; import software.amazon.smithy.model.node.Node; -import software.amazon.smithy.utils.ListUtils; /** * Searches {@link Node}s to find matching children. Each search @@ -18,27 +18,27 @@ * and the results are aggregated. */ final class NodeQuery { - private static final Query SELF = Stream::of; + private static final Query SELF = (node, result) -> result.add(node); - private static final Query ANY_MEMBER = (node) -> { + private static final Query ANY_MEMBER = (node, result) -> { if (node == null || !node.isObjectNode()) { - return Stream.empty(); + return; } - return node.expectObjectNode().getMembers().values().stream(); + result.addAll(node.expectObjectNode().getMembers().values()); }; - private static final Query ANY_ELEMENT = (node) -> { + private static final Query ANY_ELEMENT = (node, result) -> { if (node == null || !node.isArrayNode()) { - return Stream.empty(); + return; } - return node.expectArrayNode().getElements().stream(); + result.addAll(node.expectArrayNode().getElements()); }; - private static final Query ANY_MEMBER_NAME = (node) -> { + private static final Query ANY_MEMBER_NAME = (node, result) -> { if (node == null || !node.isObjectNode()) { - return Stream.empty(); + return; } - return node.expectObjectNode().getMembers().keySet().stream(); + result.addAll(node.expectObjectNode().getMembers().keySet()); }; private final List queries = new ArrayList<>(); @@ -52,11 +52,11 @@ NodeQuery self() { } NodeQuery member(String name) { - queries.add(node -> { + queries.add((node, result) -> { if (node == null || !node.isObjectNode()) { - return Stream.empty(); + return; } - return node.expectObjectNode().getMember(name).map(Stream::of).orElse(Stream.empty()); + node.expectObjectNode().getMember(name).ifPresent(result::add); }); return this; } @@ -76,20 +76,27 @@ NodeQuery anyMemberName() { return this; } - List execute(Node node) { + Collection execute(Node node) { + Queue previousResult = new ArrayDeque<>(); + if (queries.isEmpty()) { - return ListUtils.of(); + return previousResult; } - Stream previousResult = Stream.of(node); + previousResult.add(node); for (Query query : queries) { - previousResult = previousResult.flatMap(query::run); + // Each time a query runs, it adds to the queue, but we only want it to + // run on the nodes added by the previous query. + for (int i = previousResult.size(); i > 0; i--) { + query.run(previousResult.poll(), previousResult); + } } - return previousResult.collect(Collectors.toList()); + + return previousResult; } @FunctionalInterface interface Query { - Stream run(Node node); + void run(Node node, Queue result); } } diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java index cdfcb03caf2..51ab639251f 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/neighbor/NodeQueryTest.java @@ -4,7 +4,7 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.hasSize; -import java.util.List; +import java.util.Collection; import org.junit.jupiter.api.Test; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.node.StringNode; @@ -14,7 +14,7 @@ public class NodeQueryTest { public void noQueriesGivesNoResults() { Node node = Node.from("{}"); - List result = new NodeQuery().execute(node); + Collection result = new NodeQuery().execute(node); assertThat(result, hasSize(0)); } @@ -23,7 +23,7 @@ public void noQueriesGivesNoResults() { public void self() { Node node = Node.from("{}"); - List result = new NodeQuery().self().execute(node); + Collection result = new NodeQuery().self().execute(node); assertThat(result, containsInAnyOrder(node)); } @@ -32,7 +32,7 @@ public void self() { public void selfCanBeAppliedMultipleTimes() { Node node = Node.from("{}"); - List result = new NodeQuery().self().self().self().execute(node); + Collection result = new NodeQuery().self().self().self().execute(node); assertThat(result, containsInAnyOrder(node)); } @@ -42,7 +42,7 @@ public void member() { Node member = StringNode.from("bar"); Node node = Node.objectNode().withMember("foo", member); - List result = new NodeQuery().member("foo").execute(node); + Collection result = new NodeQuery().member("foo").execute(node); assertThat(result, containsInAnyOrder(member)); } @@ -53,7 +53,7 @@ public void anyMember() { Node member2 = StringNode.from("member-two"); Node node = Node.objectNode().withMember("one", member1).withMember("two", member2); - List result = new NodeQuery().anyMember().execute(node); + Collection result = new NodeQuery().anyMember().execute(node); assertThat(result, containsInAnyOrder(member1, member2)); } @@ -64,7 +64,7 @@ public void anyElement() { Node element2 = StringNode.from("element-two"); Node node = Node.arrayNode(element1, element2); - List result = new NodeQuery().anyElement().execute(node); + Collection result = new NodeQuery().anyElement().execute(node); assertThat(result, containsInAnyOrder(element1, element2)); } @@ -77,7 +77,7 @@ public void anyMemberName() { Node member2 = StringNode.from("member-two"); Node node = Node.objectNode().withMember(key1, member1).withMember(key2, member2); - List result = new NodeQuery().anyMemberName().execute(node); + Collection result = new NodeQuery().anyMemberName().execute(node); assertThat(result, containsInAnyOrder(key1, key2)); } @@ -86,7 +86,7 @@ public void anyMemberName() { public void memberGivesNoResultsOnNonObjectNode() { Node node = Node.from("[{\"foo\": 0}]"); - List result = new NodeQuery().member("foo").execute(node); + Collection result = new NodeQuery().member("foo").execute(node); assertThat(result, hasSize(0)); } @@ -95,7 +95,7 @@ public void memberGivesNoResultsOnNonObjectNode() { public void memberGivesNoResultsIfMemberNameNotFound() { Node node = Node.from("{\"a\": 0, \"b\": 0}"); - List result = new NodeQuery().member("foo").execute(node); + Collection result = new NodeQuery().member("foo").execute(node); assertThat(result, hasSize(0)); } @@ -104,7 +104,7 @@ public void memberGivesNoResultsIfMemberNameNotFound() { public void anyMemberGivesNoResultsOnNonObjectNode() { Node node = Node.from("[{\"foo\": 0}]"); - List result = new NodeQuery().anyMember().execute(node); + Collection result = new NodeQuery().anyMember().execute(node); assertThat(result, hasSize(0)); } @@ -113,7 +113,7 @@ public void anyMemberGivesNoResultsOnNonObjectNode() { public void anyMemberGivesNoResultsOnEmptyObjectNode() { Node node = Node.from("{}"); - List result = new NodeQuery().anyMember().execute(node); + Collection result = new NodeQuery().anyMember().execute(node); assertThat(result, hasSize(0)); } @@ -122,7 +122,7 @@ public void anyMemberGivesNoResultsOnEmptyObjectNode() { public void anyElementGivesNoResultsOnNonArrayNode() { Node node = Node.from("{\"foo\": [0]}"); - List result = new NodeQuery().anyElement().execute(node); + Collection result = new NodeQuery().anyElement().execute(node); assertThat(result, hasSize(0)); } @@ -131,7 +131,7 @@ public void anyElementGivesNoResultsOnNonArrayNode() { public void anyElementGivesNoResultsOnEmptyArrayNode() { Node node = Node.from("[]"); - List result = new NodeQuery().anyElement().execute(node); + Collection result = new NodeQuery().anyElement().execute(node); assertThat(result, hasSize(0)); } @@ -140,7 +140,7 @@ public void anyElementGivesNoResultsOnEmptyArrayNode() { public void anyMemberNameGivesNoResultsOnNonObjectNode() { Node node = Node.from("1"); - List result = new NodeQuery().anyMemberName().execute(node); + Collection result = new NodeQuery().anyMemberName().execute(node); assertThat(result, hasSize(0)); } @@ -149,7 +149,7 @@ public void anyMemberNameGivesNoResultsOnNonObjectNode() { public void anyMemberNameGivesNoResultsOnEmptyObject() { Node node = Node.from("{}"); - List result = new NodeQuery().anyMemberName().execute(node); + Collection result = new NodeQuery().anyMemberName().execute(node); assertThat(result, hasSize(0)); } @@ -165,7 +165,7 @@ public void eachQueryExecuteOnResultOfPreviousQuery() { .withMember("arr3", Node.arrayNode(element3))); Node node = Node.arrayNode(obj, obj); - List result = new NodeQuery() + Collection result = new NodeQuery() .anyElement() .member("foo") .anyMember()