Don't cache idRef provider, add test case for enum

Also makes most NodeQuery queries static instances, and moves some stuff around in tests.
milesziemer · Jan 23, 2024 · 8fbb1d3 · 8fbb1d3
1 parent 505285e
commit 8fbb1d3
Show file tree

Hide file tree

Showing 7 changed files with 84 additions and 55 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
@@ -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
@@ -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<Query> 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
@@ -55,7 +55,9 @@ public UnreferencedShapes(Predicate<Shape> keepFilter) {
      * @return Returns the unreferenced shapes.
      */
     public Set<Shape> 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<Shape> 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
@@ -97,18 +97,14 @@ public void canGetIdRefRelationshipsThroughTraitDefs() {
         List<Relationship> 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<Relationship> 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<Relationship> 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<Relationship> 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
@@ -13,29 +13,37 @@ public class NodeQueryTest {
     @Test
     public void noQueriesGivesNoResults() {
         Node node = Node.from("{}");
+
         List<Node> result = new NodeQuery().execute(node);
+
         assertThat(result, hasSize(0));
     }
 
     @Test
     public void self() {
         Node node = Node.from("{}");
+
         List<Node> result = new NodeQuery().self().execute(node);
+
         assertThat(result, containsInAnyOrder(node));
     }
 
     @Test
     public void selfCanBeAppliedMultipleTimes() {
         Node node = Node.from("{}");
+
         List<Node> 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<Node> 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<Node> 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<Node> 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<Node> result = new NodeQuery().anyMemberName().execute(node);
+
         assertThat(result, containsInAnyOrder(key1, key2));
     }
 
     @Test
     public void memberGivesNoResultsOnNonObjectNode() {
         Node node = Node.from("[{\"foo\": 0}]");
+
         List<Node> result = new NodeQuery().member("foo").execute(node);
+
         assertThat(result, hasSize(0));
     }
 
     @Test
     public void memberGivesNoResultsIfMemberNameNotFound() {
         Node node = Node.from("{\"a\": 0, \"b\": 0}");
+
         List<Node> result = new NodeQuery().member("foo").execute(node);
+
         assertThat(result, hasSize(0));
     }
 
     @Test
     public void anyMemberGivesNoResultsOnNonObjectNode() {
         Node node = Node.from("[{\"foo\": 0}]");
+
         List<Node> result = new NodeQuery().anyMember().execute(node);
+
         assertThat(result, hasSize(0));
     }
 
     @Test
     public void anyMemberGivesNoResultsOnEmptyObjectNode() {
         Node node = Node.from("{}");
+
         List<Node> result = new NodeQuery().anyMember().execute(node);
+
         assertThat(result, hasSize(0));
     }
 
     @Test
     public void anyElementGivesNoResultsOnNonArrayNode() {
         Node node = Node.from("{\"foo\": [0]}");
+
         List<Node> result = new NodeQuery().anyElement().execute(node);
+
         assertThat(result, hasSize(0));
     }
 
     @Test
     public void anyElementGivesNoResultsOnEmptyArrayNode() {
         Node node = Node.from("[]");
+
         List<Node> result = new NodeQuery().anyElement().execute(node);
+
         assertThat(result, hasSize(0));
     }
 
     @Test
     public void anyMemberNameGivesNoResultsOnNonObjectNode() {
         Node node = Node.from("1");
+
         List<Node> result = new NodeQuery().anyMemberName().execute(node);
+
         assertThat(result, hasSize(0));
     }
 
     @Test
     public void anyMemberNameGivesNoResultsOnEmptyObject() {
         Node node = Node.from("{}");
+
         List<Node> 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
@@ -89,6 +89,7 @@ public void checksShapeReferencesThroughIdRef() {
                 .unwrap();
 
         Set<Shape> shapes = new UnreferencedShapes().compute(m);
+
         assertThat(shapes, empty());
     }
 
@@ -100,6 +101,7 @@ public void doesNotCheckShapeReferencesThroughIdRefOnUnconnectedShapes() {
                 .unwrap();
 
         Set<ShapeId> 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.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,19 @@ 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 {}
+