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 +``` 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 41d669f4ed..96761b7d11 100644 --- a/src/main/java/graphql/schema/SchemaTransformer.java +++ b/src/main/java/graphql/schema/SchemaTransformer.java @@ -4,6 +4,8 @@ import com.google.common.collect.Multimap; 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; @@ -16,6 +18,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; @@ -102,6 +105,59 @@ 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 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 dependencies. + * + * @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 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 dependencies. + * + * @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) { + SchemaTransformer schemaTransformer = new SchemaTransformer(); + return (GraphQLSchema) schemaTransformer.transformImpl(schema, null, visitor, postTransformation, true); + } + + /** * Transforms a {@link GraphQLSchemaElement} and returns a new element. * @@ -117,40 +173,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); } } @@ -158,7 +219,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); } @@ -181,10 +242,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<>(); @@ -219,7 +285,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); } } } @@ -278,7 +344,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 { @@ -338,7 +404,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; } @@ -383,7 +450,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) @@ -431,7 +498,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"); @@ -508,6 +576,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); } @@ -528,6 +599,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"; @@ -539,13 +611,15 @@ private static class DummyRoot implements GraphQLSchemaElement { GraphQLObjectType mutation; GraphQLObjectType subscription; GraphQLObjectType introspectionSchemaType; - Set additionalTypes; + 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; @@ -555,6 +629,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 (ScalarInfo.isGraphqlSpecifiedScalar(typeName)) { + continue; + } + extraTypes.add(typeMap.get(typeName)); + } + } } DummyRoot(GraphQLSchemaElement schemaElement) { @@ -585,6 +687,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); @@ -608,6 +711,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; } @@ -616,7 +721,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/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/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()) { diff --git a/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java b/src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java index 945966c00f..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)); @@ -112,7 +113,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/schema/SchemaTransformerTest.groovy b/src/test/groovy/graphql/schema/SchemaTransformerTest.groovy index 9513ed39fc..41cf3ad9f6 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.GraphQL import graphql.Scalars import graphql.TestUtil @@ -1056,14 +1057,106 @@ 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() + } + + /** + * 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 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): + *
    + *
  • {@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 would NOT be visited (only referenced by name)
    6. + *
  6. {@code Customer.payment} field is visited → has @remove → DELETED → {@code Payment} NOT visited
    7. + *
  7. {@code Inventory} would NOT be visited (only reachable through Payment)
    8. + *
+ *

+ *

The Fix

+ * {@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 - circular references with deletes - 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() { @@ -1085,7 +1178,8 @@ type Query { } } when: - def newSchema = SchemaTransformer.transformSchema(schema, visitor) + // 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) diff --git a/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy b/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy index 25a6331e02..2d168a6de7 100644 --- a/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy +++ b/src/test/groovy/graphql/schema/transform/FieldVisibilitySchemaTransformationTest.groovy @@ -1346,5 +1346,74 @@ class FieldVisibilitySchemaTransformationTest extends Specification { (restrictedSchema.getType("Input") as GraphQLInputObjectType).getFieldDefinition("toDelete") == null } + /** + * 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 - fixed"() { + 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: + GraphQLSchema restrictedSchema = visibilitySchemaTransformation.apply(schema) + + 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 + } } 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);