From fc78f2f3884c8eebe30f294ad049ecab82c202f0 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Fri, 23 Jan 2026 06:24:12 +1000 Subject: [PATCH] Fix GraphQLTypeCollectingVisitor to traverse children of indirect strong references The fixDanglingReplacedTypes method now recursively traverses types discovered through indirect strong references (types referenced via replaced type references). Previously, when an indirect strong reference was found, it was added to the result but its children were not traversed, causing types referenced by the indirect reference to be missing from the schema. The fix: - For each newly discovered indirect strong reference, start a new traversal - Reuse the same visitor instance to ensure duplicate type detection works - Clear indirectStrongReferences before each traversal to capture new ones - Recursively call fixDanglingReplacedTypes to handle nested indirect references Added tests: - indirect type references should have their children collected - nested indirect type references requiring multiple traversals - redefined types are caught when introduced during transformation --- .../impl/GraphQLTypeCollectingVisitor.java | 32 ++- .../schema/SchemaTransformerTest.groovy | 268 ++++++++++++++++++ 2 files changed, 298 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java index 8712d0eed9..0ce7026426 100644 --- a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java +++ b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java @@ -18,10 +18,13 @@ import graphql.schema.GraphQLTypeReference; import graphql.schema.GraphQLTypeVisitorStub; import graphql.schema.GraphQLUnionType; +import graphql.schema.SchemaTraverser; import graphql.util.TraversalControl; import graphql.util.TraverserContext; +import java.util.ArrayList; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.TreeMap; import java.util.function.Supplier; @@ -175,17 +178,42 @@ public ImmutableMap getResult() { *

* The fix: During traversal, we also capture types directly from fields/arguments/inputs * (in {@link #indirectStrongReferences}). After traversal, we merge any types that were captured - * this way but weren't found through normal traversal. + * this way but weren't found through normal traversal. Additionally, we traverse each newly + * discovered indirect strong reference to collect any types it references, recursively handling + * cases where indirect strong references are nested within other indirect strong references. + *

+ * We reuse the same visitor instance to ensure duplicate type detection works correctly + * across all traversals. * * @param visitedTypes the types collected through normal traversal * * @return the fixed map including any dangling replaced types */ private Map fixDanglingReplacedTypes(Map visitedTypes) { + // Collect indirect strong references that are not yet in the visited types + List newlyDiscoveredTypes = new ArrayList<>(); for (GraphQLNamedType indirectStrongReference : indirectStrongReferences.values()) { String typeName = indirectStrongReference.getName(); - visitedTypes.putIfAbsent(typeName, indirectStrongReference); + if (!visitedTypes.containsKey(typeName)) { + visitedTypes.put(typeName, indirectStrongReference); + newlyDiscoveredTypes.add(indirectStrongReference); + } + } + + // For each newly discovered type, traverse it to collect any types it references + // We reuse this visitor instance to ensure duplicate type detection works correctly + if (!newlyDiscoveredTypes.isEmpty()) { + // Clear indirect strong references before traversing to capture new ones + indirectStrongReferences.clear(); + + SchemaTraverser traverser = new SchemaTraverser( + schemaElement -> schemaElement.getChildrenWithTypeReferences().getChildrenAsList()); + traverser.depthFirst(this, newlyDiscoveredTypes); + + // Recursively fix any newly discovered indirect strong references + fixDanglingReplacedTypes(visitedTypes); } + return visitedTypes; } } diff --git a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy index 41cf3ad9f6..ebe47ad2c4 100644 --- a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy +++ b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy @@ -1,6 +1,7 @@ package graphql.schema +import graphql.AssertException import graphql.GraphQL import graphql.Scalars import graphql.TestUtil @@ -1196,4 +1197,271 @@ type Rental { id: ID }""".trim() } + + def "indirect type references should have their children collected"() { + given: + // Bar is referenced by Foo.bar directly + def bar = newObject() + .name("Bar") + .field(newFieldDefinition() + .name("id") + .type(Scalars.GraphQLID) + .build()) + .build() + + // Foo references Bar directly via Foo.bar field + def foo = newObject() + .name("Foo") + .field(newFieldDefinition() + .name("bar") + .type(bar) // Direct reference to Bar + .build()) + .build() + + // Query.foo1 references Foo via type reference (indirect) + // Query.foo2 references Foo directly (strong reference) + def query = newObject() + .name("Query") + .field(newFieldDefinition() + .name("foo1") + .type(typeRef("Foo")) // Indirect reference via typeRef + .build()) + .field(newFieldDefinition() + .name("foo2") + .type(foo) // Direct reference to Foo + .build()) + .build() + + def schema = newSchema() + .query(query) + .build() + + // Visitor that removes Query.foo2 + def visitor = new GraphQLTypeVisitorStub() { + @Override + TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { + if (node.name == "foo2") { + return deleteNode(context) + } + return TraversalControl.CONTINUE + } + } + + when: + def newSchema = SchemaTransformer.transformSchemaWithDeletes(schema, visitor) + + then: "Query.foo2 should be removed" + (newSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("foo2") == null + + and: "Query.foo1 should still exist" + (newSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("foo1") != null + + and: "Foo should still exist (reachable via Query.foo1)" + newSchema.getType("Foo") != null + + and: "Bar should still exist (reachable via Query.foo1 -> Foo -> bar)" + newSchema.getType("Bar") != null + } + + def "nested indirect type references requiring multiple traversals should have their children collected"() { + given: + // Create a deeply nested structure where each level has indirect references: + // Query.level1 -> Level1 (via typeRef) -> Level2 (direct) -> Level3 (via typeRef) -> Level4 (direct) -> Leaf (direct) + // Query.directRef -> Level1 (direct) - this is the only direct path + // When we remove Query.directRef, the nested traversals should still find all types + + def leaf = newObject() + .name("Leaf") + .field(newFieldDefinition() + .name("value") + .type(Scalars.GraphQLString) + .build()) + .build() + + def level4 = newObject() + .name("Level4") + .field(newFieldDefinition() + .name("leaf") + .type(leaf) // Direct reference to Leaf + .build()) + .build() + + def level3 = newObject() + .name("Level3") + .field(newFieldDefinition() + .name("level4") + .type(level4) // Direct reference to Level4 + .build()) + .build() + + def level2 = newObject() + .name("Level2") + .field(newFieldDefinition() + .name("level3") + .type(typeRef("Level3")) // Indirect reference via typeRef + .build()) + .field(newFieldDefinition() + .name("level3Direct") + .type(level3) // Direct reference to Level3 (needed for schema build) + .build()) + .build() + + def level1 = newObject() + .name("Level1") + .field(newFieldDefinition() + .name("level2") + .type(level2) // Direct reference to Level2 + .build()) + .build() + + def query = newObject() + .name("Query") + .field(newFieldDefinition() + .name("level1Indirect") + .type(typeRef("Level1")) // Indirect reference via typeRef + .build()) + .field(newFieldDefinition() + .name("level1Direct") + .type(level1) // Direct reference to Level1 + .build()) + .build() + + def schema = newSchema() + .query(query) + .build() + + // Visitor that removes Query.level1Direct and Level2.level3Direct + // This leaves only indirect paths: Query.level1Indirect -> Level1 -> Level2.level3 -> Level3 -> Level4 -> Leaf + def visitor = new GraphQLTypeVisitorStub() { + @Override + TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { + if (node.name == "level1Direct" || node.name == "level3Direct") { + return deleteNode(context) + } + return TraversalControl.CONTINUE + } + } + + when: + def newSchema = SchemaTransformer.transformSchemaWithDeletes(schema, visitor) + + then: "Direct fields should be removed" + (newSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("level1Direct") == null + (newSchema.getType("Level2") as GraphQLObjectType).getFieldDefinition("level3Direct") == null + + and: "Indirect fields should still exist" + (newSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("level1Indirect") != null + (newSchema.getType("Level2") as GraphQLObjectType).getFieldDefinition("level3") != null + + and: "All types in the chain should still exist (discovered through nested indirect reference traversal)" + newSchema.getType("Level1") != null + newSchema.getType("Level2") != null + newSchema.getType("Level3") != null + newSchema.getType("Level4") != null + newSchema.getType("Leaf") != null + } + + def "redefined types are caught when introduced during transformation and discovered through indirect references"() { + given: + // Build a valid schema where: + // - Query.fooIndirect -> Foo (via typeRef) + // - Query.fooDirect -> Foo (direct) - will be removed during transformation + // - Foo.targetType -> TargetType (direct) - will be REPLACED during transformation + // - Query.existingType -> ExistingType (direct) - already in schema + // + // During transformation, we will: + // 1. Remove Query.fooDirect (so Foo is only reachable via indirect reference) + // 2. Replace TargetType with a NEW object also named "ExistingType" (introduces duplicate) + // + // When fixDanglingReplacedTypes traverses from Foo (indirect reference), + // it should discover the replaced type and detect the duplicate with ExistingType + + def targetType = newObject() + .name("TargetType") + .field(newFieldDefinition() + .name("id") + .type(Scalars.GraphQLID) + .build()) + .build() + + def existingType = newObject() + .name("ExistingType") + .field(newFieldDefinition() + .name("name") + .type(Scalars.GraphQLString) + .build()) + .build() + + def foo = newObject() + .name("Foo") + .field(newFieldDefinition() + .name("targetType") + .type(targetType) + .build()) + .build() + + def query = newObject() + .name("Query") + .field(newFieldDefinition() + .name("fooIndirect") + .type(typeRef("Foo")) // Indirect reference + .build()) + .field(newFieldDefinition() + .name("fooDirect") + .type(foo) // Direct reference - will be removed + .build()) + .field(newFieldDefinition() + .name("existingType") + .type(existingType) // Direct reference to ExistingType + .build()) + .build() + + def schema = newSchema() + .query(query) + .build() + + // Create a duplicate type with the same name as ExistingType but different instance + def duplicateExistingType = newObject() + .name("ExistingType") + .field(newFieldDefinition() + .name("differentField") // Different field makes it a different object + .type(Scalars.GraphQLInt) + .build()) + .build() + + // Visitor that: + // 1. Removes Query.fooDirect (so Foo is only reachable via indirect reference) + // 2. Replaces TargetType with duplicateExistingType (introduces a duplicate "ExistingType") + def visitor = new GraphQLTypeVisitorStub() { + @Override + TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { + if (node.name == "fooDirect") { + return deleteNode(context) + } + return TraversalControl.CONTINUE + } + + @Override + TraversalControl visitGraphQLObjectType(GraphQLObjectType node, TraverserContext context) { + if (node.name == "TargetType") { + // Replace TargetType with a type named "ExistingType" (duplicate!) + return changeNode(context, duplicateExistingType) + } + return TraversalControl.CONTINUE + } + } + + when: + // This should fail because: + // 1. After removing fooDirect, Foo is only reachable via fooIndirect (typeRef) + // 2. fixDanglingReplacedTypes traverses from Foo + // 3. It discovers the replaced type (now named "ExistingType") + // 4. This conflicts with the already-collected ExistingType from Query.existingType + SchemaTransformer.transformSchemaWithDeletes(schema, visitor) + + then: + def e = thrown(AssertException) + e.getMessage().contains("All types within a GraphQL schema must have unique names") + e.getMessage().contains("ExistingType") + } }