From 0464f19224dbd66988920d7eac2f35e0fd7401a6 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Mon, 19 Jan 2026 16:57:36 +1000 Subject: [PATCH 1/8] add specific transform method for deletion cases --- .../graphql/schema/SchemaTransformer.java | 68 +++++++++ .../schema/SchemaTransformerTest.groovy | 130 +++++++++++++++++- 2 files changed, 193 insertions(+), 5 deletions(-) diff --git a/src/main/java/graphql/schema/SchemaTransformer.java b/src/main/java/graphql/schema/SchemaTransformer.java index 41d669f4ed..13f8991302 100644 --- a/src/main/java/graphql/schema/SchemaTransformer.java +++ b/src/main/java/graphql/schema/SchemaTransformer.java @@ -4,6 +4,7 @@ import com.google.common.collect.Multimap; import graphql.PublicApi; import graphql.collect.ImmutableKit; +import graphql.introspection.Introspection; import graphql.util.Breadcrumb; import graphql.util.NodeAdapter; import graphql.util.NodeLocation; @@ -102,6 +103,73 @@ public static GraphQLSchema transformSchema(GraphQLSchema schema, GraphQLTypeVis return schemaTransformer.transform(schema, visitor, postTransformation); } + /** + * Transforms a GraphQLSchema with support for delete operations. + *

+ * When a visitor uses {@link GraphQLTypeVisitor#deleteNode(TraverserContext)} to delete schema elements, + * the traversal does not continue to the children of deleted nodes. This can cause issues when types + * are only reachable through fields that get deleted, as those types won't be visited and transformed. + *

+ * This method ensures all types in the schema are visited by adding them to the schema's additional types + * before transformation. This guarantees that even types only reachable through deleted fields will be + * properly visited and transformed. + *

+ * Use this method instead of {@link #transformSchema(GraphQLSchema, GraphQLTypeVisitor)} when your + * visitor deletes fields or types that may reference other types via circular references. + * + * @param schema the schema to transform + * @param visitor the visitor call back + * + * @return a new GraphQLSchema instance. + * + * @see GraphQLTypeVisitor#deleteNode(TraverserContext) + */ + public static GraphQLSchema transformSchemaWithDeletes(GraphQLSchema schema, GraphQLTypeVisitor visitor) { + return transformSchemaWithDeletes(schema, visitor, null); + } + + /** + * Transforms a GraphQLSchema with support for delete operations. + *

+ * When a visitor uses {@link GraphQLTypeVisitor#deleteNode(TraverserContext)} to delete schema elements, + * the traversal does not continue to the children of deleted nodes. This can cause issues when types + * are only reachable through fields that get deleted, as those types won't be visited and transformed. + *

+ * This method ensures all types in the schema are visited by adding them to the schema's additional types + * before transformation. This guarantees that even types only reachable through deleted fields will be + * properly visited and transformed. + *

+ * Use this method instead of {@link #transformSchema(GraphQLSchema, GraphQLTypeVisitor, Consumer)} when your + * visitor deletes fields or types that may reference other types via circular references. + * + * @param schema the schema to transform + * @param visitor the visitor call back + * @param postTransformation a callback that can be used as a final step to the schema (can be null) + * + * @return a new GraphQLSchema instance. + * + * @see GraphQLTypeVisitor#deleteNode(TraverserContext) + */ + public static GraphQLSchema transformSchemaWithDeletes(GraphQLSchema schema, GraphQLTypeVisitor visitor, Consumer postTransformation) { + // Add all types to additionalTypes to ensure they are all visited during transformation. + // This is necessary because when a node is deleted, its children are not traversed. + // Types that are only reachable through deleted fields would otherwise not be visited. + GraphQLSchema schemaWithAllTypes = schema.transform(builder -> { + for (GraphQLNamedType type : schema.getTypeMap().values()) { + if (!isRootType(schema, type) && !Introspection.isIntrospectionTypes(type)) { + builder.additionalType(type); + } + } + }); + return transformSchema(schemaWithAllTypes, visitor, postTransformation); + } + + private static boolean isRootType(GraphQLSchema schema, GraphQLNamedType type) { + return type == schema.getQueryType() + || type == schema.getMutationType() + || type == schema.getSubscriptionType(); + } + /** * Transforms a {@link GraphQLSchemaElement} and returns a new element. * diff --git a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy index 9513ed39fc..e325abb505 100644 --- a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy +++ b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy @@ -1,5 +1,6 @@ package graphql.schema +import graphql.AssertException import graphql.GraphQL import graphql.Scalars import graphql.TestUtil @@ -1056,14 +1057,75 @@ type Query { """ def schema = TestUtil.schema(sdl) - schema = schema.transform { builder -> - for (def type : schema.getTypeMap().values()) { - if (type != schema.getQueryType() && type != schema.getMutationType() && type != schema.getSubscriptionType()) { - builder.additionalType(type) + + def visitor = new GraphQLTypeVisitorStub() { + + @Override + TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { + if (node.hasAppliedDirective("remove")) { + return deleteNode(context) + } + return TraversalControl.CONTINUE + } + + @Override + TraversalControl visitGraphQLObjectType(GraphQLObjectType node, TraverserContext context) { + if (node.getFields().stream().allMatch(field -> field.hasAppliedDirective("remove"))) { + return deleteNode(context) } + + return TraversalControl.CONTINUE } } + when: + def newSchema = SchemaTransformer.transformSchemaWithDeletes(schema, visitor) + def printer = new SchemaPrinter(SchemaPrinter.Options.defaultOptions().includeDirectives(false)) + def newSdl = printer.print(newSchema) + then: + newSdl.trim() == """type Customer { + rental: Rental +} + +type Query { + customer: Customer +} + +type Rental { + id: ID +}""".trim() + } + + def "issue 4133 simplified reproduction - demonstrates the bug"() { + def sdl = """ + directive @remove on FIELD_DEFINITION + + type Query { + rental: Rental @remove + customer: Customer + } + + type Customer { + rental: Rental + payment: Payment @remove + } + + type Rental { + id: ID + customer: Customer @remove + } + + type Payment { + inventory: Inventory @remove + } + + type Inventory { + payment: Payment @remove + } + """ + + def schema = TestUtil.schema(sdl) + // NO WORKAROUND - this should fail with regular transformSchema def visitor = new GraphQLTypeVisitorStub() { @@ -1085,7 +1147,65 @@ type Query { } } when: - def newSchema = SchemaTransformer.transformSchema(schema, visitor) + SchemaTransformer.transformSchema(schema, visitor) + + then: + def e = thrown(AssertException) + e.message.contains("not found in schema") + } + + def "issue 4133 simplified - fixed with transformSchemaWithDeletes"() { + def sdl = """ + directive @remove on FIELD_DEFINITION + + type Query { + rental: Rental @remove + customer: Customer + } + + type Customer { + rental: Rental + payment: Payment @remove + } + + type Rental { + id: ID + customer: Customer @remove + } + + type Payment { + inventory: Inventory @remove + } + + type Inventory { + payment: Payment @remove + } + """ + + def schema = TestUtil.schema(sdl) + + def visitor = new GraphQLTypeVisitorStub() { + + @Override + TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { + if (node.hasAppliedDirective("remove")) { + return deleteNode(context) + } + return TraversalControl.CONTINUE + } + + @Override + TraversalControl visitGraphQLObjectType(GraphQLObjectType node, TraverserContext context) { + if (node.getFields().stream().allMatch(field -> field.hasAppliedDirective("remove"))) { + return deleteNode(context) + } + + return TraversalControl.CONTINUE + } + } + when: + // Use the new transformSchemaWithDeletes method - this should work! + def newSchema = SchemaTransformer.transformSchemaWithDeletes(schema, visitor) def printer = new SchemaPrinter(SchemaPrinter.Options.defaultOptions().includeDirectives(false)) def newSdl = printer.print(newSchema) From bfd8789950239c2e939562b2651a8a85e9f6903e Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Mon, 19 Jan 2026 17:02:23 +1000 Subject: [PATCH 2/8] explanation --- .../schema/SchemaTransformerTest.groovy | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy index e325abb505..ad18969fa3 100644 --- a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy +++ b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy @@ -1096,6 +1096,46 @@ type Rental { }""".trim() } + /** + * This test demonstrates a bug in SchemaTransformer when deleting nodes that form circular references. + * + *

The Problem

+ * When a node is deleted via {@code deleteNode(context)}, the traversal does NOT continue to visit + * the children of that deleted node (see {@code TraverserState.pushAll()}). This becomes problematic + * when combined with how GraphQL-Java handles circular type references using {@code GraphQLTypeReference}. + * + *

How GraphQLTypeReference Creates Asymmetry

+ * In circular references, one direction uses the actual type object, while the other uses a + * {@code GraphQLTypeReference} (a placeholder resolved during schema building): + * + * + *

Traversal in This Test

+ *
    + *
  1. {@code Query.rental} field is visited → has @remove → DELETED → children NOT traversed
    2. + *
  2. {@code Rental} type is NOT visited via this path (it's a child of the deleted field)
    3. + *
  3. {@code Query.customer} field is visited → {@code Customer} type IS visited
    4. + *
  4. {@code Customer.rental} field is visited → but its type is {@code GraphQLTypeReference("Rental")}
    5. + *
  5. The actual {@code Rental} GraphQLObjectType is NEVER visited (only referenced by name)
    6. + *
  6. {@code Customer.payment} field is visited → has @remove → DELETED → {@code Payment} NOT visited
    7. + *
  7. {@code Inventory} is NOT visited (only reachable through Payment)
    8. + *
+ * + *

The Error

+ * Since {@code Rental} is never visited, it retains its original {@code customer} field pointing to the + * ORIGINAL (unmodified) {@code Customer} object. This original Customer still has the {@code payment} field, + * which references {@code Payment}, which has {@code inventory: GraphQLTypeReference("Inventory")}. + * + * During schema rebuild, when resolving this TypeReference, {@code Inventory} is not found in the + * schema's type map (it was never visited/transformed), causing: + * {@code AssertException: type Inventory not found in schema} + * + *

The Fix

+ * Use {@code SchemaTransformer.transformSchemaWithDeletes()} which ensures all types are visited + * by adding them to additionalTypes before transformation. + */ def "issue 4133 simplified reproduction - demonstrates the bug"() { def sdl = """ directive @remove on FIELD_DEFINITION @@ -1125,7 +1165,6 @@ type Rental { """ def schema = TestUtil.schema(sdl) - // NO WORKAROUND - this should fail with regular transformSchema def visitor = new GraphQLTypeVisitorStub() { From 49dca7e874bd838af92c062787cecc1b80f2ccfa Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Mon, 19 Jan 2026 17:38:32 +1000 Subject: [PATCH 3/8] Test to showcase bug --- ...dVisibilitySchemaTransformationTest.groovy | 122 ++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy b/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy index 25a6331e02..2ddce60c0f 100644 --- a/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy +++ b/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy @@ -1,5 +1,6 @@ package graphql.schema.transform +import graphql.AssertException import graphql.Scalars import graphql.TestUtil import graphql.schema.GraphQLAppliedDirective @@ -1346,5 +1347,126 @@ class FieldVisibilitySchemaTransformationTest extends Specification { (restrictedSchema.getType("Input") as GraphQLInputObjectType).getFieldDefinition("toDelete") == null } + /** + * This test reproduces the issue 4133 bug pattern with FieldVisibilitySchemaTransformation. + * + * The problem occurs when: + * 1. A field is marked @private and gets deleted + * 2. The traversal doesn't continue to children of deleted nodes + * 3. Types only reachable through deleted fields are not visited + * 4. Those unvisited types may have circular references using GraphQLTypeReference + * 5. During schema rebuild, stale object references point to types with unresolvable TypeReferences + * + * Schema structure: + * - Query.rental @private -> Rental (not visited because parent field deleted) + * - Query.customer -> Customer (visited) + * - Customer.rental -> TypeReference("Rental") (placeholder, doesn't cause Rental to be visited) + * - Rental.customer -> Customer (actual object reference to ORIGINAL unmodified Customer) + * - Customer.payment @private -> Payment (deleted, Payment not visited) + * - Payment.inventory -> TypeReference("Inventory") + * - Inventory not in schema -> ERROR during type reference resolution + * + * WORKAROUND: Add all types to additionalTypes before applying the transformation. + * This ensures all types are visited during the transformation. + */ + def "issue 4133 - circular references with private fields - demonstrates the bug"() { + given: + GraphQLSchema schema = TestUtil.schema(""" + + directive @private on FIELD_DEFINITION + + type Query { + rental: Rental @private + customer: Customer + } + + type Customer { + rental: Rental + payment: Payment @private + } + + type Rental { + id: ID + customer: Customer @private + } + + type Payment { + inventory: Inventory @private + } + + type Inventory { + payment: Payment @private + } + """) + + when: + visibilitySchemaTransformation.apply(schema) + + then: + // This demonstrates the bug - without the workaround, we get "type not found" error + def e = thrown(AssertException) + e.message.contains("not found in schema") + } + + /** + * This test shows the workaround for issue 4133 - add all types to additionalTypes + * before applying the transformation to ensure all types are visited. + */ + def "issue 4133 - circular references with private fields - workaround with additionalTypes"() { + given: + GraphQLSchema schema = TestUtil.schema(""" + + directive @private on FIELD_DEFINITION + + type Query { + rental: Rental @private + customer: Customer + } + + type Customer { + rental: Rental + payment: Payment @private + } + + type Rental { + id: ID + customer: Customer @private + } + + type Payment { + inventory: Inventory @private + } + + type Inventory { + payment: Payment @private + } + """) + + // WORKAROUND: Add all types to additionalTypes to ensure they are all visited + def patchedSchema = schema.transform { builder -> + schema.typeMap.each { entry -> + def type = entry.value + if (type != schema.queryType && type != schema.mutationType && type != schema.subscriptionType) { + builder.additionalType(type) + } + } + } + + when: + GraphQLSchema restrictedSchema = visibilitySchemaTransformation.apply(patchedSchema) + + then: + // Query should only have customer field (rental is private) + (restrictedSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("rental") == null + (restrictedSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("customer") != null + + // Customer should only have rental field (payment is private) + (restrictedSchema.getType("Customer") as GraphQLObjectType).getFieldDefinition("rental") != null + (restrictedSchema.getType("Customer") as GraphQLObjectType).getFieldDefinition("payment") == null + + // Rental should only have id field (customer is private) + (restrictedSchema.getType("Rental") as GraphQLObjectType).getFieldDefinition("id") != null + (restrictedSchema.getType("Rental") as GraphQLObjectType).getFieldDefinition("customer") == null + } } From 44aeed6c93423ba274df4b90ae8f2c386f8fdb94 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Mon, 19 Jan 2026 18:48:44 +1000 Subject: [PATCH 4/8] Refactor additionalTypes to use GraphQLNamedType Change GraphQLSchema.additionalTypes from Set to Set for improved type safety. All types that can be added as additional types are named types, so this makes the API more precise. --- src/main/java/graphql/schema/GraphQLSchema.java | 14 +++++++------- .../java/graphql/schema/SchemaTransformer.java | 2 +- .../java/graphql/schema/idl/SchemaGenerator.java | 3 ++- .../graphql/schema/idl/SchemaGeneratorHelper.java | 13 +++++++------ .../FieldVisibilitySchemaTransformation.java | 2 +- .../graphql/validation/SpecValidationSchema.java | 3 ++- 6 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/main/java/graphql/schema/GraphQLSchema.java b/src/main/java/graphql/schema/GraphQLSchema.java index 92ff53835c..a53f58165a 100644 --- a/src/main/java/graphql/schema/GraphQLSchema.java +++ b/src/main/java/graphql/schema/GraphQLSchema.java @@ -52,7 +52,7 @@ public class GraphQLSchema { private final GraphQLObjectType mutationType; private final GraphQLObjectType subscriptionType; private final GraphQLObjectType introspectionSchemaType; - private final ImmutableSet additionalTypes; + private final ImmutableSet additionalTypes; private final GraphQLFieldDefinition introspectionSchemaField; private final GraphQLFieldDefinition introspectionTypeField; // we don't allow modification of "__typename" - it's a scalar @@ -275,10 +275,10 @@ public GraphQLObjectType getIntrospectionSchemaType() { * * @return an immutable set of types that were explicitly added as additional types * - * @see Builder#additionalType(GraphQLType) + * @see Builder#additionalType(GraphQLNamedType) * @see Builder#additionalTypes(Set) */ - public Set getAdditionalTypes() { + public Set getAdditionalTypes() { return additionalTypes; } @@ -745,7 +745,7 @@ public static class Builder { private final Set additionalDirectives = new LinkedHashSet<>( asList(Directives.IncludeDirective, Directives.SkipDirective) ); - private final Set additionalTypes = new LinkedHashSet<>(); + private final Set additionalTypes = new LinkedHashSet<>(); private final List schemaDirectives = new ArrayList<>(); private final List schemaAppliedDirectives = new ArrayList<>(); @@ -808,7 +808,7 @@ public Builder codeRegistry(GraphQLCodeRegistry codeRegistry) { * * @see GraphQLSchema#getAdditionalTypes() */ - public Builder additionalTypes(Set additionalTypes) { + public Builder additionalTypes(Set additionalTypes) { this.additionalTypes.addAll(additionalTypes); return this; } @@ -831,7 +831,7 @@ public Builder additionalTypes(Set additionalTypes) { * @see GraphQLSchema#getAdditionalTypes() * @see #additionalTypes(Set) */ - public Builder additionalType(GraphQLType additionalType) { + public Builder additionalType(GraphQLNamedType additionalType) { this.additionalTypes.add(additionalType); return this; } @@ -844,7 +844,7 @@ public Builder additionalType(GraphQLType additionalType) { * * @return this builder * - * @see #additionalType(GraphQLType) + * @see #additionalType(GraphQLNamedType) * @see #additionalTypes(Set) */ public Builder clearAdditionalTypes() { diff --git a/src/main/java/graphql/schema/SchemaTransformer.java b/src/main/java/graphql/schema/SchemaTransformer.java index 13f8991302..8879874baf 100644 --- a/src/main/java/graphql/schema/SchemaTransformer.java +++ b/src/main/java/graphql/schema/SchemaTransformer.java @@ -607,7 +607,7 @@ private static class DummyRoot implements GraphQLSchemaElement { GraphQLObjectType mutation; GraphQLObjectType subscription; GraphQLObjectType introspectionSchemaType; - Set additionalTypes; + Set additionalTypes; Set directives; Set schemaDirectives; Set schemaAppliedDirectives; diff --git a/src/main/java/graphql/schema/idl/SchemaGenerator.java b/src/main/java/graphql/schema/idl/SchemaGenerator.java index c53ef08b8f..05aeaf1dc5 100644 --- a/src/main/java/graphql/schema/idl/SchemaGenerator.java +++ b/src/main/java/graphql/schema/idl/SchemaGenerator.java @@ -5,6 +5,7 @@ import graphql.language.OperationTypeDefinition; import graphql.schema.GraphQLCodeRegistry; import graphql.schema.GraphQLDirective; +import graphql.schema.GraphQLNamedType; import graphql.schema.GraphQLSchema; import graphql.schema.GraphQLType; import graphql.schema.idl.errors.SchemaProblem; @@ -130,7 +131,7 @@ private GraphQLSchema makeExecutableSchemaImpl(ImmutableTypeDefinitionRegistry t schemaGeneratorHelper.buildOperations(buildCtx, schemaBuilder); - Set additionalTypes = schemaGeneratorHelper.buildAdditionalTypes(buildCtx); + Set additionalTypes = schemaGeneratorHelper.buildAdditionalTypes(buildCtx); schemaBuilder.additionalTypes(additionalTypes); buildCtx.getCodeRegistry().fieldVisibility(buildCtx.getWiring().getFieldVisibility()); diff --git a/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java b/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java index c6368da666..00951ce91c 100644 --- a/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java +++ b/src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java @@ -48,6 +48,7 @@ import graphql.schema.GraphQLInterfaceType; import graphql.schema.GraphQLNamedInputType; import graphql.schema.GraphQLNamedOutputType; +import graphql.schema.GraphQLNamedType; import graphql.schema.GraphQLObjectType; import graphql.schema.GraphQLOutputType; import graphql.schema.GraphQLScalarType; @@ -115,8 +116,8 @@ static class BuildContext { private final RuntimeWiring wiring; private final Deque typeStack = new ArrayDeque<>(); - private final Map outputGTypes = new LinkedHashMap<>(); - private final Map inputGTypes = new LinkedHashMap<>(); + private final Map outputGTypes = new LinkedHashMap<>(); + private final Map inputGTypes = new LinkedHashMap<>(); private final Set directives = new LinkedHashSet<>(); private final GraphQLCodeRegistry.Builder codeRegistry; public final Map operationTypeDefs; @@ -173,7 +174,7 @@ void putOutputType(GraphQLNamedOutputType outputType) { outputGTypes.put(outputType.getName(), outputType); // certain types can be both input and output types, for example enums and scalars if (outputType instanceof GraphQLInputType) { - inputGTypes.put(outputType.getName(), (GraphQLInputType) outputType); + inputGTypes.put(outputType.getName(), (GraphQLNamedInputType) outputType); } } @@ -181,7 +182,7 @@ void putInputType(GraphQLNamedInputType inputType) { inputGTypes.put(inputType.getName(), inputType); // certain types can be both input and output types, for example enums and scalars if (inputType instanceof GraphQLOutputType) { - outputGTypes.put(inputType.getName(), (GraphQLOutputType) inputType); + outputGTypes.put(inputType.getName(), (GraphQLNamedOutputType) inputType); } } @@ -996,12 +997,12 @@ List unionTypeExtensions(UnionTypeDefinition typeD * * @return the additional types not referenced from the top level operations */ - Set buildAdditionalTypes(BuildContext buildCtx) { + Set buildAdditionalTypes(BuildContext buildCtx) { TypeDefinitionRegistry typeRegistry = buildCtx.getTypeRegistry(); Set detachedTypeNames = getDetachedTypeNames(buildCtx); - Set additionalTypes = new LinkedHashSet<>(); + Set additionalTypes = new LinkedHashSet<>(); // recursively record detached types on the ctx and add them to the additionalTypes set typeRegistry.types().values().stream() .filter(typeDefinition -> detachedTypeNames.contains(typeDefinition.getName())) diff --git a/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java b/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java index 945966c00f..69f5ab34b9 100644 --- a/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java +++ b/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java @@ -112,7 +112,7 @@ private Function> getChildrenFn private GraphQLSchema removeUnreferencedTypes(Set markedForRemovalTypes, GraphQLSchema connectedSchema) { GraphQLSchema withoutAdditionalTypes = connectedSchema.transform(builder -> { - Set additionalTypes = new HashSet<>(connectedSchema.getAdditionalTypes()); + Set additionalTypes = new HashSet<>(connectedSchema.getAdditionalTypes()); additionalTypes.removeAll(markedForRemovalTypes); builder.clearAdditionalTypes(); builder.additionalTypes(additionalTypes); diff --git a/src/test/groovy/graphql/validation/SpecValidationSchema.java b/src/test/groovy/graphql/validation/SpecValidationSchema.java index aa244a6a11..adbbb924d5 100644 --- a/src/test/groovy/graphql/validation/SpecValidationSchema.java +++ b/src/test/groovy/graphql/validation/SpecValidationSchema.java @@ -9,6 +9,7 @@ import graphql.schema.GraphQLInputObjectField; import graphql.schema.GraphQLInputObjectType; import graphql.schema.GraphQLInterfaceType; +import graphql.schema.GraphQLNamedType; import graphql.schema.GraphQLObjectType; import graphql.schema.GraphQLSchema; import graphql.schema.GraphQLType; @@ -236,7 +237,7 @@ public class SpecValidationSchema { .type(inputDogType))) .build(); - public static final Set specValidationDictionary = new HashSet() {{ + public static final Set specValidationDictionary = new HashSet() {{ add(dogCommand); add(catCommand); add(sentient); From fe82d4fdeec3cc3db695c24dffd9586eadb76754 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Tue, 20 Jan 2026 06:54:55 +1000 Subject: [PATCH 5/8] simplify --- .../impl/GraphQLTypeCollectingVisitor.java | 93 ++++++++++--------- 1 file changed, 49 insertions(+), 44 deletions(-) diff --git a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java index e6ceb76e3c..8712d0eed9 100644 --- a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java +++ b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java @@ -30,6 +30,23 @@ import static graphql.util.TraversalControl.CONTINUE; import static java.lang.String.format; +/** + * A visitor that collects all {@link GraphQLNamedType}s during schema traversal. + *

+ * This visitor must be used with a traverser that calls {@code getChildrenWithTypeReferences()} + * to get children (see {@link SchemaUtil#visitPartiallySchema}). This means that when a field, + * argument, or input field references a type via {@link GraphQLTypeReference}, the traverser + * will see the type reference as a child, not the actual type it points to. Type references + * themselves are not collected - only concrete type instances are stored in the result map. + *

+ * Because type references are not followed, this visitor also tracks "indirect strong references" + * - types that are directly referenced (not via type reference) by fields, arguments, and input + * fields. This handles edge cases where schema transformations replace type references with + * actual types, which would otherwise be missed during traversal. + * + * @see SchemaUtil#visitPartiallySchema + * @see #fixDanglingReplacedTypes + */ @Internal public class GraphQLTypeCollectingVisitor extends GraphQLTypeVisitorStub { @@ -55,31 +72,22 @@ public TraversalControl visitGraphQLScalarType(GraphQLScalarType node, Traverser @Override public TraversalControl visitGraphQLObjectType(GraphQLObjectType node, TraverserContext context) { - if (isNotTypeReference(node.getName())) { - assertTypeUniqueness(node, result); - } else { - save(node.getName(), node); - } + assertTypeUniqueness(node, result); + save(node.getName(), node); return CONTINUE; } @Override public TraversalControl visitGraphQLInputObjectType(GraphQLInputObjectType node, TraverserContext context) { - if (isNotTypeReference(node.getName())) { - assertTypeUniqueness(node, result); - } else { - save(node.getName(), node); - } + assertTypeUniqueness(node, result); + save(node.getName(), node); return CONTINUE; } @Override public TraversalControl visitGraphQLInterfaceType(GraphQLInterfaceType node, TraverserContext context) { - if (isNotTypeReference(node.getName())) { - assertTypeUniqueness(node, result); - } else { - save(node.getName(), node); - } + assertTypeUniqueness(node, result); + save(node.getName(), node); return CONTINUE; } @@ -114,43 +122,25 @@ public TraversalControl visitGraphQLAppliedDirectiveArgument(GraphQLAppliedDirec return CONTINUE; } - private void saveIndirectStrongReference(Supplier typeSupplier) { + private void saveIndirectStrongReference(Supplier typeSupplier) { GraphQLNamedType type = unwrapAllAs(typeSupplier.get()); if (!(type instanceof GraphQLTypeReference)) { indirectStrongReferences.put(type.getName(), type); } } - - private boolean isNotTypeReference(String name) { - return result.containsKey(name) && !(result.get(name) instanceof GraphQLTypeReference); - } - private void save(String name, GraphQLNamedType type) { result.put(name, type); } - - /* - From https://spec.graphql.org/October2021/#sec-Type-System - - All types within a GraphQL schema must have unique names. No two provided types may have the same name. - No provided type may have a name which conflicts with any built in types (including Scalar and Introspection types). - - Enforcing this helps avoid problems later down the track fo example https://github.com/graphql-java/graphql-java/issues/373 - */ private void assertTypeUniqueness(GraphQLNamedType type, Map result) { - GraphQLType existingType = result.get(type.getName()); - // do we have an existing definition + GraphQLNamedType existingType = result.get(type.getName()); if (existingType != null) { - // type references are ok - if (!(existingType instanceof GraphQLTypeReference || type instanceof GraphQLTypeReference)) { - assertUniqueTypeObjects(type, existingType); - } + assertUniqueTypeObjects(type, existingType); } } - private void assertUniqueTypeObjects(GraphQLNamedType type, GraphQLType existingType) { + private void assertUniqueTypeObjects(GraphQLNamedType type, GraphQLNamedType existingType) { // object comparison here is deliberate if (existingType != type) { throw new AssertException(format("All types within a GraphQL schema must have unique names. No two provided types may have the same name.\n" + @@ -166,15 +156,30 @@ public ImmutableMap getResult() { } /** - * It's possible for certain schema edits to create a situation where a field / arg / input field had a type reference, then - * it got replaced with an actual strong reference and then the schema gets edited such that the only reference - * to that type is the replaced strong reference. This edge case means that the replaced reference can be - * missed if it's the only way to get to that type because this visitor asks for the children as original type, - * e.g. the type reference and not the replaced reference. + * Fixes an edge case where types might be missed during traversal due to replaced type references. + *

+ * The problem: During schema construction or transformation, a field's type might initially + * be a {@link GraphQLTypeReference} (a placeholder). Later, this reference gets replaced with the + * actual type instance. However, the schema traverser uses {@code getChildrenWithTypeReferences()} + * to discover children, which returns the original type references, not the replaced strong references. + *

+ * The scenario: + *

    + *
  1. Field {@code foo} has type reference to {@code Bar}
    2. + *
  2. During schema editing, the reference is replaced with the actual {@code Bar} type
    3. + *
  3. Further edits remove all other paths to {@code Bar}
    4. + *
  4. Now the only way to reach {@code Bar} is via the replaced reference in {@code foo}
    5. + *
  5. But the traverser still sees the original type reference as the child, so {@code Bar} + * is never visited as a child node
    6. + *
+ *

+ * 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. * - * @param visitedTypes the types collected by this visitor + * @param visitedTypes the types collected through normal traversal * - * @return a fixed up map where the only + * @return the fixed map including any dangling replaced types */ private Map fixDanglingReplacedTypes(Map visitedTypes) { for (GraphQLNamedType indirectStrongReference : indirectStrongReferences.values()) { From 7c5574709c788cd44307103f5e374d2bc6dd22d7 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Tue, 20 Jan 2026 08:04:32 +1000 Subject: [PATCH 6/8] handle deletion case much better on the schema transformer level --- .../graphql/schema/SchemaTransformer.java | 128 ++++++++++++------ .../FieldVisibilitySchemaTransformation.java | 3 +- .../schema/SchemaTransformerTest.groovy | 93 ++----------- ...dVisibilitySchemaTransformationTest.groovy | 99 ++++---------- 4 files changed, 125 insertions(+), 198 deletions(-) diff --git a/src/main/java/graphql/schema/SchemaTransformer.java b/src/main/java/graphql/schema/SchemaTransformer.java index 8879874baf..a25b916f5f 100644 --- a/src/main/java/graphql/schema/SchemaTransformer.java +++ b/src/main/java/graphql/schema/SchemaTransformer.java @@ -17,6 +17,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -110,12 +111,12 @@ public static GraphQLSchema transformSchema(GraphQLSchema schema, GraphQLTypeVis * the traversal does not continue to the children of deleted nodes. This can cause issues when types * are only reachable through fields that get deleted, as those types won't be visited and transformed. *

- * This method ensures all types in the schema are visited by adding them to the schema's additional types - * before transformation. This guarantees that even types only reachable through deleted fields will be + * This method ensures all types in the schema are visited by temporarily adding them as extra root types + * during transformation. This guarantees that even types only reachable through deleted fields will be * properly visited and transformed. *

* Use this method instead of {@link #transformSchema(GraphQLSchema, GraphQLTypeVisitor)} when your - * visitor deletes fields or types that may reference other types via circular references. + * visitor deletes fields or types that may reference other types via circular dependencies. * * @param schema the schema to transform * @param visitor the visitor call back @@ -135,12 +136,12 @@ public static GraphQLSchema transformSchemaWithDeletes(GraphQLSchema schema, Gra * the traversal does not continue to the children of deleted nodes. This can cause issues when types * are only reachable through fields that get deleted, as those types won't be visited and transformed. *

- * This method ensures all types in the schema are visited by adding them to the schema's additional types - * before transformation. This guarantees that even types only reachable through deleted fields will be + * This method ensures all types in the schema are visited by temporarily adding them as extra root types + * during transformation. This guarantees that even types only reachable through deleted fields will be * properly visited and transformed. *

* Use this method instead of {@link #transformSchema(GraphQLSchema, GraphQLTypeVisitor, Consumer)} when your - * visitor deletes fields or types that may reference other types via circular references. + * visitor deletes fields or types that may reference other types via circular dependencies. * * @param schema the schema to transform * @param visitor the visitor call back @@ -151,24 +152,10 @@ public static GraphQLSchema transformSchemaWithDeletes(GraphQLSchema schema, Gra * @see GraphQLTypeVisitor#deleteNode(TraverserContext) */ public static GraphQLSchema transformSchemaWithDeletes(GraphQLSchema schema, GraphQLTypeVisitor visitor, Consumer postTransformation) { - // Add all types to additionalTypes to ensure they are all visited during transformation. - // This is necessary because when a node is deleted, its children are not traversed. - // Types that are only reachable through deleted fields would otherwise not be visited. - GraphQLSchema schemaWithAllTypes = schema.transform(builder -> { - for (GraphQLNamedType type : schema.getTypeMap().values()) { - if (!isRootType(schema, type) && !Introspection.isIntrospectionTypes(type)) { - builder.additionalType(type); - } - } - }); - return transformSchema(schemaWithAllTypes, visitor, postTransformation); + SchemaTransformer schemaTransformer = new SchemaTransformer(); + return (GraphQLSchema) schemaTransformer.transformImpl(schema, null, visitor, postTransformation, true); } - private static boolean isRootType(GraphQLSchema schema, GraphQLNamedType type) { - return type == schema.getQueryType() - || type == schema.getMutationType() - || type == schema.getSubscriptionType(); - } /** * Transforms a {@link GraphQLSchemaElement} and returns a new element. @@ -185,40 +172,45 @@ public static T transformSchema(final T schemaE } public GraphQLSchema transform(final GraphQLSchema schema, GraphQLTypeVisitor visitor) { - return (GraphQLSchema) transformImpl(schema, null, visitor, null); + return (GraphQLSchema) transformImpl(schema, null, visitor, null, false); } public GraphQLSchema transform(final GraphQLSchema schema, GraphQLTypeVisitor visitor, Consumer postTransformation) { - return (GraphQLSchema) transformImpl(schema, null, visitor, postTransformation); + return (GraphQLSchema) transformImpl(schema, null, visitor, postTransformation, false); } public T transform(final T schemaElement, GraphQLTypeVisitor visitor) { //noinspection unchecked - return (T) transformImpl(null, schemaElement, visitor, null); + return (T) transformImpl(null, schemaElement, visitor, null, false); } - private Object transformImpl(final GraphQLSchema schema, GraphQLSchemaElement schemaElement, GraphQLTypeVisitor visitor, Consumer postTransformation) { + private Object transformImpl(final GraphQLSchema schema, + GraphQLSchemaElement schemaElement, + GraphQLTypeVisitor visitor, + Consumer postTransformation, + boolean ensureAllTypesAreVisited) { DummyRoot dummyRoot; GraphQLCodeRegistry.Builder codeRegistry = null; if (schema != null) { - dummyRoot = new DummyRoot(schema); + dummyRoot = new DummyRoot(schema, ensureAllTypesAreVisited); codeRegistry = GraphQLCodeRegistry.newCodeRegistry(schema.getCodeRegistry()); } else { dummyRoot = new DummyRoot(schemaElement); } - final Map changedTypes = new LinkedHashMap<>(); + final Map typesWhereNameIsChanged = new LinkedHashMap<>(); + final Set allChangedNamedTypes = new LinkedHashSet<>(); final Map typeReferences = new LinkedHashMap<>(); // first pass - general transformation - boolean schemaChanged = traverseAndTransform(dummyRoot, changedTypes, typeReferences, visitor, codeRegistry, schema); + boolean schemaChanged = traverseAndTransform(dummyRoot, typesWhereNameIsChanged, allChangedNamedTypes, typeReferences, visitor, codeRegistry, schema); // if we have changed any named elements AND we have type references referring to them then // we need to make a second pass to replace these type references to the new names - if (!changedTypes.isEmpty()) { - boolean hasTypeRefsForChangedTypes = changedTypes.keySet().stream().anyMatch(typeReferences::containsKey); + if (!typesWhereNameIsChanged.isEmpty()) { + boolean hasTypeRefsForChangedTypes = typesWhereNameIsChanged.keySet().stream().anyMatch(typeReferences::containsKey); if (hasTypeRefsForChangedTypes) { - replaceTypeReferences(dummyRoot, schema, codeRegistry, changedTypes); + replaceTypeReferences(dummyRoot, schema, codeRegistry, typesWhereNameIsChanged); } } @@ -226,7 +218,7 @@ private Object transformImpl(final GraphQLSchema schema, GraphQLSchemaElement sc GraphQLSchema graphQLSchema = schema; if (schemaChanged || codeRegistry.hasChanged()) { - graphQLSchema = dummyRoot.rebuildSchema(codeRegistry); + graphQLSchema = dummyRoot.rebuildSchema(codeRegistry, allChangedNamedTypes); if (postTransformation != null) { graphQLSchema = graphQLSchema.transform(postTransformation); } @@ -249,10 +241,15 @@ public TraversalControl visitGraphQLTypeReference(GraphQLTypeReference typeRef, return CONTINUE; } }; - traverseAndTransform(dummyRoot, new HashMap<>(), new HashMap<>(), typeRefVisitor, codeRegistry, schema); + traverseAndTransform(dummyRoot, new HashMap<>(), new HashSet<>(), new HashMap<>(), typeRefVisitor, codeRegistry, schema); } - private boolean traverseAndTransform(DummyRoot dummyRoot, Map changedTypes, Map typeReferences, GraphQLTypeVisitor visitor, GraphQLCodeRegistry.Builder codeRegistry, GraphQLSchema schema) { + private boolean traverseAndTransform(DummyRoot dummyRoot, + Map typesWhereNameIsChanged, + Set allChangedNamedTypes, + Map typeReferences, + GraphQLTypeVisitor visitor, GraphQLCodeRegistry.Builder codeRegistry, + GraphQLSchema schema) { List> zippers = new LinkedList<>(); Map> zipperByNodeAfterTraversing = new LinkedHashMap<>(); Map> zipperByOriginalNode = new LinkedHashMap<>(); @@ -287,7 +284,7 @@ public TraversalControl enter(TraverserContext context) { GraphQLNamedType originalNamedType = (GraphQLNamedType) context.originalThisNode(); GraphQLNamedType changedNamedType = (GraphQLNamedType) context.thisNode(); if (!originalNamedType.getName().equals(changedNamedType.getName())) { - changedTypes.put(originalNamedType.getName(), changedNamedType); + typesWhereNameIsChanged.put(originalNamedType.getName(), changedNamedType); } } } @@ -346,7 +343,7 @@ public TraversalControl backRef(TraverserContext context) List> stronglyConnectedTopologicallySorted = getStronglyConnectedComponentsTopologicallySorted(reverseDependencies, typeRefReverseDependencies); - return zipUpToDummyRoot(zippers, stronglyConnectedTopologicallySorted, breadcrumbsByZipper, zipperByNodeAfterTraversing); + return zipUpToDummyRoot(zippers, stronglyConnectedTopologicallySorted, breadcrumbsByZipper, zipperByNodeAfterTraversing, allChangedNamedTypes); } private static class RelevantZippersAndBreadcrumbs { @@ -406,7 +403,8 @@ public void updateZipper(NodeZipper currentZipper, private boolean zipUpToDummyRoot(List> zippers, List> stronglyConnectedTopologicallySorted, Map, List>>> breadcrumbsByZipper, - Map> nodeToZipper) { + Map> nodeToZipper, + Set allChangedNamedTypes) { if (zippers.size() == 0) { return false; } @@ -451,7 +449,7 @@ private boolean zipUpToDummyRoot(List> zippers, if (zipperWithSameParent.size() == 0) { continue; } - NodeZipper newZipper = moveUp(element, zipperWithSameParent); + NodeZipper newZipper = moveUp(element, zipperWithSameParent, allChangedNamedTypes); if (element instanceof DummyRoot) { // this means we have updated the dummy root and we are done (dummy root is a special as it gets updated in place, see Implementation of DummyRoot) @@ -499,7 +497,8 @@ private Map, Breadcrumb> private NodeZipper moveUp( GraphQLSchemaElement parent, - Map, Breadcrumb> sameParentsZipper) { + Map, Breadcrumb> sameParentsZipper, + Set allChangedNamedTypes) { Set> sameParent = sameParentsZipper.keySet(); assertNotEmpty(sameParent, "expected at least one zipper"); @@ -576,6 +575,9 @@ private NodeZipper moveUp( } else { newBreadcrumbs = ImmutableKit.emptyList(); } + if (newNode instanceof GraphQLNamedType) { + allChangedNamedTypes.add(((GraphQLNamedType) newNode).getName()); + } return new NodeZipper<>(newNode, newBreadcrumbs, SCHEMA_ELEMENT_ADAPTER); } @@ -596,6 +598,7 @@ private static class DummyRoot implements GraphQLSchemaElement { static final String MUTATION = "mutation"; static final String SUBSCRIPTION = "subscription"; static final String ADD_TYPES = "addTypes"; + static final String EXTRA_TYPES = "extraTypes"; static final String DIRECTIVES = "directives"; static final String SCHEMA_DIRECTIVES = "schemaDirectives"; static final String SCHEMA_APPLIED_DIRECTIVES = "schemaAppliedDirectives"; @@ -608,12 +611,14 @@ private static class DummyRoot implements GraphQLSchemaElement { GraphQLObjectType subscription; GraphQLObjectType introspectionSchemaType; Set additionalTypes; + Set extraTypes; Set directives; Set schemaDirectives; Set schemaAppliedDirectives; GraphQLSchemaElement schemaElement; + boolean ensureAllTypesAreVisited; - DummyRoot(GraphQLSchema schema) { + DummyRoot(GraphQLSchema schema, boolean ensureAllTypesAreVisited) { this.schema = schema; query = schema.getQueryType(); mutation = schema.isSupportingMutations() ? schema.getMutationType() : null; @@ -623,6 +628,34 @@ private static class DummyRoot implements GraphQLSchemaElement { schemaAppliedDirectives = new LinkedHashSet<>(schema.getSchemaAppliedDirectives()); directives = new LinkedHashSet<>(schema.getDirectives()); introspectionSchemaType = schema.getIntrospectionSchemaType(); + + extraTypes = new LinkedHashSet<>(); + if (ensureAllTypesAreVisited) { + // add all names types which are not already directly referenced into extra types, + // hence making sure even if elements are deleted, all named types are visited + Map typeMap = schema.getTypeMap(); + for (String typeName : typeMap.keySet()) { + if (Introspection.isIntrospectionTypes(typeName)) { + continue; + } + if (typeName.equals(query.getName())) { + continue; + } + if (mutation != null && typeName.equals(mutation.getName())) { + continue; + } + if (subscription != null && typeName.equals(subscription.getName())) { + continue; + } + if (additionalTypes.contains(typeMap.get(typeName))) { + continue; + } + if (typeMap.get(typeName) instanceof GraphQLScalarType) { + continue; + } + extraTypes.add(typeMap.get(typeName)); + } + } } DummyRoot(GraphQLSchemaElement schemaElement) { @@ -653,6 +686,7 @@ public SchemaElementChildrenContainer getChildrenWithTypeReferences() { builder.child(SUBSCRIPTION, subscription); } builder.children(ADD_TYPES, additionalTypes); + builder.children(EXTRA_TYPES, extraTypes); builder.children(DIRECTIVES, directives); builder.children(SCHEMA_DIRECTIVES, schemaDirectives); builder.children(SCHEMA_APPLIED_DIRECTIVES, schemaAppliedDirectives); @@ -676,6 +710,8 @@ public GraphQLSchemaElement withNewChildren(SchemaElementChildrenContainer newCh directives = new LinkedHashSet<>(newChildren.getChildren(DIRECTIVES)); schemaDirectives = new LinkedHashSet<>(newChildren.getChildren(SCHEMA_DIRECTIVES)); schemaAppliedDirectives = new LinkedHashSet<>(newChildren.getChildren(SCHEMA_APPLIED_DIRECTIVES)); + // if extra types + extraTypes = new LinkedHashSet<>(newChildren.getChildren(EXTRA_TYPES)); return this; } @@ -684,7 +720,15 @@ public TraversalControl accept(TraverserContext context, G return assertShouldNeverHappen(); } - public GraphQLSchema rebuildSchema(GraphQLCodeRegistry.Builder codeRegistry) { + public GraphQLSchema rebuildSchema(GraphQLCodeRegistry.Builder codeRegistry, Set changedNamedTypes) { + // if an extra type was changed, we are adding the type to the additional types + // to ensure it is correctly discovered, because it might not be directly reachable (any more) + // this is a special handling for deletion case. See SchemaTransformerTest for more info. + for (GraphQLNamedType extraType : extraTypes) { + if (changedNamedTypes.contains(extraType.getName())) { + this.additionalTypes.add(extraType); + } + } return GraphQLSchema.newSchema() .query(this.query) .mutation(this.mutation) diff --git a/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java b/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java index 69f5ab34b9..cf6f1c739d 100644 --- a/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java +++ b/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java @@ -34,6 +34,7 @@ import java.util.stream.Stream; import static graphql.schema.SchemaTransformer.transformSchema; +import static graphql.schema.SchemaTransformer.transformSchemaWithDeletes; /** * Transforms a schema by applying a visibility predicate to every field. @@ -75,7 +76,7 @@ public final GraphQLSchema apply(GraphQLSchema schema) { new SchemaTraverser(getChildrenFn(schema)).depthFirst(new TypeObservingVisitor(observedBeforeTransform), getRootTypes(schema)); // remove fields - GraphQLSchema interimSchema = transformSchema(schema, + GraphQLSchema interimSchema = transformSchemaWithDeletes(schema, new FieldRemovalVisitor(visibleFieldPredicate, markedForRemovalTypes)); new SchemaTraverser(getChildrenFn(interimSchema)).depthFirst(new TypeObservingVisitor(observedAfterTransform), getRootTypes(interimSchema)); diff --git a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy index ad18969fa3..41cf3ad9f6 100644 --- a/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy +++ b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy @@ -1,6 +1,6 @@ package graphql.schema -import graphql.AssertException + import graphql.GraphQL import graphql.Scalars import graphql.TestUtil @@ -1097,13 +1097,13 @@ type Rental { } /** - * This test demonstrates a bug in SchemaTransformer when deleting nodes that form circular references. - * - *

The Problem

+ * This test verifies the fix for issue 4133: deleting nodes with circular references. + *

+ *

The Problem (Fixed)

* When a node is deleted via {@code deleteNode(context)}, the traversal does NOT continue to visit - * the children of that deleted node (see {@code TraverserState.pushAll()}). This becomes problematic + * the children of that deleted node (see {@code TraverserState.pushAll()}). This was problematic * when combined with how GraphQL-Java handles circular type references using {@code GraphQLTypeReference}. - * + *

*

How GraphQLTypeReference Creates Asymmetry

* In circular references, one direction uses the actual type object, while the other uses a * {@code GraphQLTypeReference} (a placeholder resolved during schema building): @@ -1111,89 +1111,24 @@ type Rental { *
  • {@code Customer.rental} → {@code GraphQLTypeReference("Rental")} (placeholder)
    • *
  • {@code Rental.customer} → {@code Customer} (actual object reference)
    • * - * + *

    *

    Traversal in This Test

    *
      *
    1. {@code Query.rental} field is visited → has @remove → DELETED → children NOT traversed
      2. *
    2. {@code Rental} type is NOT visited via this path (it's a child of the deleted field)
      3. *
    3. {@code Query.customer} field is visited → {@code Customer} type IS visited
      4. *
    4. {@code Customer.rental} field is visited → but its type is {@code GraphQLTypeReference("Rental")}
      5. - *
    5. The actual {@code Rental} GraphQLObjectType is NEVER visited (only referenced by name)
      6. + *
    6. The actual {@code Rental} GraphQLObjectType would NOT be visited (only referenced by name)
      7. *
    7. {@code Customer.payment} field is visited → has @remove → DELETED → {@code Payment} NOT visited
      8. - *
    8. {@code Inventory} is NOT visited (only reachable through Payment)
      9. + *
    9. {@code Inventory} would NOT be visited (only reachable through Payment)
      10. *
    - * - *

    The Error

    - * Since {@code Rental} is never visited, it retains its original {@code customer} field pointing to the - * ORIGINAL (unmodified) {@code Customer} object. This original Customer still has the {@code payment} field, - * which references {@code Payment}, which has {@code inventory: GraphQLTypeReference("Inventory")}. - * - * During schema rebuild, when resolving this TypeReference, {@code Inventory} is not found in the - * schema's type map (it was never visited/transformed), causing: - * {@code AssertException: type Inventory not found in schema} - * + *

    *

    The Fix

    - * Use {@code SchemaTransformer.transformSchemaWithDeletes()} which ensures all types are visited - * by adding them to additionalTypes before transformation. + * {@code SchemaTransformer.transformSchemaWithDeletes()} ensures all types are visited by temporarily + * adding them as extra root types during transformation. Types that are modified are then included + * in the rebuilt schema's additionalTypes, ensuring type references are properly resolved. */ - def "issue 4133 simplified reproduction - demonstrates the bug"() { - def sdl = """ - directive @remove on FIELD_DEFINITION - - type Query { - rental: Rental @remove - customer: Customer - } - - type Customer { - rental: Rental - payment: Payment @remove - } - - type Rental { - id: ID - customer: Customer @remove - } - - type Payment { - inventory: Inventory @remove - } - - type Inventory { - payment: Payment @remove - } - """ - - def schema = TestUtil.schema(sdl) - - def visitor = new GraphQLTypeVisitorStub() { - - @Override - TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { - if (node.hasAppliedDirective("remove")) { - return deleteNode(context) - } - return TraversalControl.CONTINUE - } - - @Override - TraversalControl visitGraphQLObjectType(GraphQLObjectType node, TraverserContext context) { - if (node.getFields().stream().allMatch(field -> field.hasAppliedDirective("remove"))) { - return deleteNode(context) - } - - return TraversalControl.CONTINUE - } - } - when: - SchemaTransformer.transformSchema(schema, visitor) - - then: - def e = thrown(AssertException) - e.message.contains("not found in schema") - } - - def "issue 4133 simplified - fixed with transformSchemaWithDeletes"() { + def "issue 4133 - circular references with deletes - fixed with transformSchemaWithDeletes"() { def sdl = """ directive @remove on FIELD_DEFINITION diff --git a/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy b/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy index 2ddce60c0f..2d168a6de7 100644 --- a/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy +++ b/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy @@ -1,6 +1,5 @@ package graphql.schema.transform -import graphql.AssertException import graphql.Scalars import graphql.TestUtil import graphql.schema.GraphQLAppliedDirective @@ -1348,28 +1347,29 @@ class FieldVisibilitySchemaTransformationTest extends Specification { } /** - * This test reproduces the issue 4133 bug pattern with FieldVisibilitySchemaTransformation. - * - * The problem occurs when: - * 1. A field is marked @private and gets deleted - * 2. The traversal doesn't continue to children of deleted nodes - * 3. Types only reachable through deleted fields are not visited - * 4. Those unvisited types may have circular references using GraphQLTypeReference - * 5. During schema rebuild, stale object references point to types with unresolvable TypeReferences - * - * Schema structure: - * - Query.rental @private -> Rental (not visited because parent field deleted) - * - Query.customer -> Customer (visited) - * - Customer.rental -> TypeReference("Rental") (placeholder, doesn't cause Rental to be visited) - * - Rental.customer -> Customer (actual object reference to ORIGINAL unmodified Customer) - * - Customer.payment @private -> Payment (deleted, Payment not visited) - * - Payment.inventory -> TypeReference("Inventory") - * - Inventory not in schema -> ERROR during type reference resolution - * - * WORKAROUND: Add all types to additionalTypes before applying the transformation. - * This ensures all types are visited during the transformation. + * This test verifies the fix for issue 4133 with FieldVisibilitySchemaTransformation. + *

    + *

    The Problem (Fixed)

    + * When a field is marked @private and deleted, the traversal doesn't continue to children of + * deleted nodes. Types only reachable through deleted fields were not being visited, and those + * unvisited types with circular references using GraphQLTypeReference would cause errors during + * schema rebuild. + *

    + *

    Schema Structure

    + *
      + *
    • {@code Query.rental @private} → Rental (would not be visited because parent field deleted)
      • + *
    • {@code Query.customer} → Customer (visited)
      • + *
    • {@code Customer.rental} → TypeReference("Rental") (placeholder, doesn't cause Rental to be visited)
      • + *
    • {@code Rental.customer} → Customer (actual object reference)
      • + *
    • {@code Customer.payment @private} → Payment (deleted, Payment would not be visited)
      • + *
    • {@code Payment.inventory} → TypeReference("Inventory")
      • + *
    + *

    + *

    The Fix

    + * {@code FieldVisibilitySchemaTransformation} now uses {@code SchemaTransformer.transformSchemaWithDeletes()} + * which ensures all types are visited by temporarily adding them as extra root types during transformation. */ - def "issue 4133 - circular references with private fields - demonstrates the bug"() { + def "issue 4133 - circular references with private fields - fixed"() { given: GraphQLSchema schema = TestUtil.schema(""" @@ -1400,60 +1400,7 @@ class FieldVisibilitySchemaTransformationTest extends Specification { """) when: - visibilitySchemaTransformation.apply(schema) - - then: - // This demonstrates the bug - without the workaround, we get "type not found" error - def e = thrown(AssertException) - e.message.contains("not found in schema") - } - - /** - * This test shows the workaround for issue 4133 - add all types to additionalTypes - * before applying the transformation to ensure all types are visited. - */ - def "issue 4133 - circular references with private fields - workaround with additionalTypes"() { - given: - GraphQLSchema schema = TestUtil.schema(""" - - directive @private on FIELD_DEFINITION - - type Query { - rental: Rental @private - customer: Customer - } - - type Customer { - rental: Rental - payment: Payment @private - } - - type Rental { - id: ID - customer: Customer @private - } - - type Payment { - inventory: Inventory @private - } - - type Inventory { - payment: Payment @private - } - """) - - // WORKAROUND: Add all types to additionalTypes to ensure they are all visited - def patchedSchema = schema.transform { builder -> - schema.typeMap.each { entry -> - def type = entry.value - if (type != schema.queryType && type != schema.mutationType && type != schema.subscriptionType) { - builder.additionalType(type) - } - } - } - - when: - GraphQLSchema restrictedSchema = visibilitySchemaTransformation.apply(patchedSchema) + GraphQLSchema restrictedSchema = visibilitySchemaTransformation.apply(schema) then: // Query should only have customer field (rental is private) From 7553d762bb53eba902180816169d87792fdb3d2b Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Tue, 20 Jan 2026 08:18:51 +1000 Subject: [PATCH 7/8] handle deletion case much better on the schema transformer level --- src/main/java/graphql/schema/SchemaTransformer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/graphql/schema/SchemaTransformer.java b/src/main/java/graphql/schema/SchemaTransformer.java index a25b916f5f..96761b7d11 100644 --- a/src/main/java/graphql/schema/SchemaTransformer.java +++ b/src/main/java/graphql/schema/SchemaTransformer.java @@ -5,6 +5,7 @@ import graphql.PublicApi; import graphql.collect.ImmutableKit; import graphql.introspection.Introspection; +import graphql.schema.idl.ScalarInfo; import graphql.util.Breadcrumb; import graphql.util.NodeAdapter; import graphql.util.NodeLocation; @@ -650,7 +651,7 @@ private static class DummyRoot implements GraphQLSchemaElement { if (additionalTypes.contains(typeMap.get(typeName))) { continue; } - if (typeMap.get(typeName) instanceof GraphQLScalarType) { + if (ScalarInfo.isGraphqlSpecifiedScalar(typeName)) { continue; } extraTypes.add(typeMap.get(typeName)); From 6b77ff8ab366a02a5b4710a7bdbcd768dfbe3c2a Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Tue, 20 Jan 2026 08:25:08 +1000 Subject: [PATCH 8/8] add AGENTS.md --- AGENTS.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 AGENTS.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000000..50afcbce83 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,11 @@ +# AI Agent Context for graphql-java + +This file provides context for AI assistants working with this codebase. + +## Test Execution + +When running tests, exclude the Java version-specific test tasks to avoid failures: + +```bash +./gradlew test -x testWithJava17 -x testWithJava11 -x testng +```