Skip to content

Commit c225c59

Browse files
authored
Merge pull request #4209 from graphql-java/schema-transform-delete-add-types
Fix schema transformation and Field Visibility for complex deletion cases
2 parents 2766087 + 6b77ff8 commit c225c59

File tree

10 files changed

+383
-87
lines changed

10 files changed

+383
-87
lines changed

AGENTS.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# AI Agent Context for graphql-java
2+
3+
This file provides context for AI assistants working with this codebase.
4+
5+
## Test Execution
6+
7+
When running tests, exclude the Java version-specific test tasks to avoid failures:
8+
9+
```bash
10+
./gradlew test -x testWithJava17 -x testWithJava11 -x testng
11+
```

src/main/java/graphql/schema/GraphQLSchema.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public class GraphQLSchema {
5252
private final GraphQLObjectType mutationType;
5353
private final GraphQLObjectType subscriptionType;
5454
private final GraphQLObjectType introspectionSchemaType;
55-
private final ImmutableSet<GraphQLType> additionalTypes;
55+
private final ImmutableSet<GraphQLNamedType> additionalTypes;
5656
private final GraphQLFieldDefinition introspectionSchemaField;
5757
private final GraphQLFieldDefinition introspectionTypeField;
5858
// we don't allow modification of "__typename" - it's a scalar
@@ -275,10 +275,10 @@ public GraphQLObjectType getIntrospectionSchemaType() {
275275
*
276276
* @return an immutable set of types that were explicitly added as additional types
277277
*
278-
* @see Builder#additionalType(GraphQLType)
278+
* @see Builder#additionalType(GraphQLNamedType)
279279
* @see Builder#additionalTypes(Set)
280280
*/
281-
public Set<GraphQLType> getAdditionalTypes() {
281+
public Set<GraphQLNamedType> getAdditionalTypes() {
282282
return additionalTypes;
283283
}
284284

@@ -745,7 +745,7 @@ public static class Builder {
745745
private final Set<GraphQLDirective> additionalDirectives = new LinkedHashSet<>(
746746
asList(Directives.IncludeDirective, Directives.SkipDirective)
747747
);
748-
private final Set<GraphQLType> additionalTypes = new LinkedHashSet<>();
748+
private final Set<GraphQLNamedType> additionalTypes = new LinkedHashSet<>();
749749
private final List<GraphQLDirective> schemaDirectives = new ArrayList<>();
750750
private final List<GraphQLAppliedDirective> schemaAppliedDirectives = new ArrayList<>();
751751

@@ -808,7 +808,7 @@ public Builder codeRegistry(GraphQLCodeRegistry codeRegistry) {
808808
*
809809
* @see GraphQLSchema#getAdditionalTypes()
810810
*/
811-
public Builder additionalTypes(Set<GraphQLType> additionalTypes) {
811+
public Builder additionalTypes(Set<? extends GraphQLNamedType> additionalTypes) {
812812
this.additionalTypes.addAll(additionalTypes);
813813
return this;
814814
}
@@ -831,7 +831,7 @@ public Builder additionalTypes(Set<GraphQLType> additionalTypes) {
831831
* @see GraphQLSchema#getAdditionalTypes()
832832
* @see #additionalTypes(Set)
833833
*/
834-
public Builder additionalType(GraphQLType additionalType) {
834+
public Builder additionalType(GraphQLNamedType additionalType) {
835835
this.additionalTypes.add(additionalType);
836836
return this;
837837
}
@@ -844,7 +844,7 @@ public Builder additionalType(GraphQLType additionalType) {
844844
*
845845
* @return this builder
846846
*
847-
* @see #additionalType(GraphQLType)
847+
* @see #additionalType(GraphQLNamedType)
848848
* @see #additionalTypes(Set)
849849
*/
850850
public Builder clearAdditionalTypes() {

src/main/java/graphql/schema/SchemaTransformer.java

Lines changed: 134 additions & 21 deletions
Large diffs are not rendered by default.

src/main/java/graphql/schema/idl/SchemaGenerator.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import graphql.language.OperationTypeDefinition;
66
import graphql.schema.GraphQLCodeRegistry;
77
import graphql.schema.GraphQLDirective;
8+
import graphql.schema.GraphQLNamedType;
89
import graphql.schema.GraphQLSchema;
910
import graphql.schema.GraphQLType;
1011
import graphql.schema.idl.errors.SchemaProblem;
@@ -130,7 +131,7 @@ private GraphQLSchema makeExecutableSchemaImpl(ImmutableTypeDefinitionRegistry t
130131

131132
schemaGeneratorHelper.buildOperations(buildCtx, schemaBuilder);
132133

133-
Set<GraphQLType> additionalTypes = schemaGeneratorHelper.buildAdditionalTypes(buildCtx);
134+
Set<GraphQLNamedType> additionalTypes = schemaGeneratorHelper.buildAdditionalTypes(buildCtx);
134135
schemaBuilder.additionalTypes(additionalTypes);
135136

136137
buildCtx.getCodeRegistry().fieldVisibility(buildCtx.getWiring().getFieldVisibility());

src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import graphql.schema.GraphQLInterfaceType;
4949
import graphql.schema.GraphQLNamedInputType;
5050
import graphql.schema.GraphQLNamedOutputType;
51+
import graphql.schema.GraphQLNamedType;
5152
import graphql.schema.GraphQLObjectType;
5253
import graphql.schema.GraphQLOutputType;
5354
import graphql.schema.GraphQLScalarType;
@@ -115,8 +116,8 @@ static class BuildContext {
115116
private final RuntimeWiring wiring;
116117
private final Deque<String> typeStack = new ArrayDeque<>();
117118

118-
private final Map<String, GraphQLOutputType> outputGTypes = new LinkedHashMap<>();
119-
private final Map<String, GraphQLInputType> inputGTypes = new LinkedHashMap<>();
119+
private final Map<String, GraphQLNamedOutputType> outputGTypes = new LinkedHashMap<>();
120+
private final Map<String, GraphQLNamedInputType> inputGTypes = new LinkedHashMap<>();
120121
private final Set<GraphQLDirective> directives = new LinkedHashSet<>();
121122
private final GraphQLCodeRegistry.Builder codeRegistry;
122123
public final Map<String, OperationTypeDefinition> operationTypeDefs;
@@ -173,15 +174,15 @@ void putOutputType(GraphQLNamedOutputType outputType) {
173174
outputGTypes.put(outputType.getName(), outputType);
174175
// certain types can be both input and output types, for example enums and scalars
175176
if (outputType instanceof GraphQLInputType) {
176-
inputGTypes.put(outputType.getName(), (GraphQLInputType) outputType);
177+
inputGTypes.put(outputType.getName(), (GraphQLNamedInputType) outputType);
177178
}
178179
}
179180

180181
void putInputType(GraphQLNamedInputType inputType) {
181182
inputGTypes.put(inputType.getName(), inputType);
182183
// certain types can be both input and output types, for example enums and scalars
183184
if (inputType instanceof GraphQLOutputType) {
184-
outputGTypes.put(inputType.getName(), (GraphQLOutputType) inputType);
185+
outputGTypes.put(inputType.getName(), (GraphQLNamedOutputType) inputType);
185186
}
186187
}
187188

@@ -996,12 +997,12 @@ List<UnionTypeExtensionDefinition> unionTypeExtensions(UnionTypeDefinition typeD
996997
*
997998
* @return the additional types not referenced from the top level operations
998999
*/
999-
Set<GraphQLType> buildAdditionalTypes(BuildContext buildCtx) {
1000+
Set<GraphQLNamedType> buildAdditionalTypes(BuildContext buildCtx) {
10001001
TypeDefinitionRegistry typeRegistry = buildCtx.getTypeRegistry();
10011002

10021003
Set<String> detachedTypeNames = getDetachedTypeNames(buildCtx);
10031004

1004-
Set<GraphQLType> additionalTypes = new LinkedHashSet<>();
1005+
Set<GraphQLNamedType> additionalTypes = new LinkedHashSet<>();
10051006
// recursively record detached types on the ctx and add them to the additionalTypes set
10061007
typeRegistry.types().values().stream()
10071008
.filter(typeDefinition -> detachedTypeNames.contains(typeDefinition.getName()))

src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java

Lines changed: 49 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,23 @@
3030
import static graphql.util.TraversalControl.CONTINUE;
3131
import static java.lang.String.format;
3232

33+
/**
34+
* A visitor that collects all {@link GraphQLNamedType}s during schema traversal.
35+
* <p>
36+
* This visitor must be used with a traverser that calls {@code getChildrenWithTypeReferences()}
37+
* to get children (see {@link SchemaUtil#visitPartiallySchema}). This means that when a field,
38+
* argument, or input field references a type via {@link GraphQLTypeReference}, the traverser
39+
* will see the type reference as a child, not the actual type it points to. Type references
40+
* themselves are not collected - only concrete type instances are stored in the result map.
41+
* <p>
42+
* Because type references are not followed, this visitor also tracks "indirect strong references"
43+
* - types that are directly referenced (not via type reference) by fields, arguments, and input
44+
* fields. This handles edge cases where schema transformations replace type references with
45+
* actual types, which would otherwise be missed during traversal.
46+
*
47+
* @see SchemaUtil#visitPartiallySchema
48+
* @see #fixDanglingReplacedTypes
49+
*/
3350
@Internal
3451
public class GraphQLTypeCollectingVisitor extends GraphQLTypeVisitorStub {
3552

@@ -55,31 +72,22 @@ public TraversalControl visitGraphQLScalarType(GraphQLScalarType node, Traverser
5572

5673
@Override
5774
public TraversalControl visitGraphQLObjectType(GraphQLObjectType node, TraverserContext<GraphQLSchemaElement> context) {
58-
if (isNotTypeReference(node.getName())) {
59-
assertTypeUniqueness(node, result);
60-
} else {
61-
save(node.getName(), node);
62-
}
75+
assertTypeUniqueness(node, result);
76+
save(node.getName(), node);
6377
return CONTINUE;
6478
}
6579

6680
@Override
6781
public TraversalControl visitGraphQLInputObjectType(GraphQLInputObjectType node, TraverserContext<GraphQLSchemaElement> context) {
68-
if (isNotTypeReference(node.getName())) {
69-
assertTypeUniqueness(node, result);
70-
} else {
71-
save(node.getName(), node);
72-
}
82+
assertTypeUniqueness(node, result);
83+
save(node.getName(), node);
7384
return CONTINUE;
7485
}
7586

7687
@Override
7788
public TraversalControl visitGraphQLInterfaceType(GraphQLInterfaceType node, TraverserContext<GraphQLSchemaElement> context) {
78-
if (isNotTypeReference(node.getName())) {
79-
assertTypeUniqueness(node, result);
80-
} else {
81-
save(node.getName(), node);
82-
}
89+
assertTypeUniqueness(node, result);
90+
save(node.getName(), node);
8391
return CONTINUE;
8492
}
8593

@@ -114,43 +122,25 @@ public TraversalControl visitGraphQLAppliedDirectiveArgument(GraphQLAppliedDirec
114122
return CONTINUE;
115123
}
116124

117-
private <T> void saveIndirectStrongReference(Supplier<GraphQLType> typeSupplier) {
125+
private void saveIndirectStrongReference(Supplier<GraphQLType> typeSupplier) {
118126
GraphQLNamedType type = unwrapAllAs(typeSupplier.get());
119127
if (!(type instanceof GraphQLTypeReference)) {
120128
indirectStrongReferences.put(type.getName(), type);
121129
}
122130
}
123131

124-
125-
private boolean isNotTypeReference(String name) {
126-
return result.containsKey(name) && !(result.get(name) instanceof GraphQLTypeReference);
127-
}
128-
129132
private void save(String name, GraphQLNamedType type) {
130133
result.put(name, type);
131134
}
132135

133-
134-
/*
135-
From https://spec.graphql.org/October2021/#sec-Type-System
136-
137-
All types within a GraphQL schema must have unique names. No two provided types may have the same name.
138-
No provided type may have a name which conflicts with any built in types (including Scalar and Introspection types).
139-
140-
Enforcing this helps avoid problems later down the track fo example https://github.com/graphql-java/graphql-java/issues/373
141-
*/
142136
private void assertTypeUniqueness(GraphQLNamedType type, Map<String, GraphQLNamedType> result) {
143-
GraphQLType existingType = result.get(type.getName());
144-
// do we have an existing definition
137+
GraphQLNamedType existingType = result.get(type.getName());
145138
if (existingType != null) {
146-
// type references are ok
147-
if (!(existingType instanceof GraphQLTypeReference || type instanceof GraphQLTypeReference)) {
148-
assertUniqueTypeObjects(type, existingType);
149-
}
139+
assertUniqueTypeObjects(type, existingType);
150140
}
151141
}
152142

153-
private void assertUniqueTypeObjects(GraphQLNamedType type, GraphQLType existingType) {
143+
private void assertUniqueTypeObjects(GraphQLNamedType type, GraphQLNamedType existingType) {
154144
// object comparison here is deliberate
155145
if (existingType != type) {
156146
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<String, GraphQLNamedType> getResult() {
166156
}
167157

168158
/**
169-
* It's possible for certain schema edits to create a situation where a field / arg / input field had a type reference, then
170-
* it got replaced with an actual strong reference and then the schema gets edited such that the only reference
171-
* to that type is the replaced strong reference. This edge case means that the replaced reference can be
172-
* missed if it's the only way to get to that type because this visitor asks for the children as original type,
173-
* e.g. the type reference and not the replaced reference.
159+
* Fixes an edge case where types might be missed during traversal due to replaced type references.
160+
* <p>
161+
* The problem: During schema construction or transformation, a field's type might initially
162+
* be a {@link GraphQLTypeReference} (a placeholder). Later, this reference gets replaced with the
163+
* actual type instance. However, the schema traverser uses {@code getChildrenWithTypeReferences()}
164+
* to discover children, which returns the original type references, not the replaced strong references.
165+
* <p>
166+
* The scenario:
167+
* <ol>
168+
* <li>Field {@code foo} has type reference to {@code Bar}</li>
169+
* <li>During schema editing, the reference is replaced with the actual {@code Bar} type</li>
170+
* <li>Further edits remove all other paths to {@code Bar}</li>
171+
* <li>Now the only way to reach {@code Bar} is via the replaced reference in {@code foo}</li>
172+
* <li>But the traverser still sees the original type reference as the child, so {@code Bar}
173+
* is never visited as a child node</li>
174+
* </ol>
175+
* <p>
176+
* The fix: During traversal, we also capture types directly from fields/arguments/inputs
177+
* (in {@link #indirectStrongReferences}). After traversal, we merge any types that were captured
178+
* this way but weren't found through normal traversal.
174179
*
175-
* @param visitedTypes the types collected by this visitor
180+
* @param visitedTypes the types collected through normal traversal
176181
*
177-
* @return a fixed up map where the only
182+
* @return the fixed map including any dangling replaced types
178183
*/
179184
private Map<String, GraphQLNamedType> fixDanglingReplacedTypes(Map<String, GraphQLNamedType> visitedTypes) {
180185
for (GraphQLNamedType indirectStrongReference : indirectStrongReferences.values()) {

src/main/java/graphql/schema/transform/FieldVisibilitySchemaTransformation.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.stream.Stream;
3535

3636
import static graphql.schema.SchemaTransformer.transformSchema;
37+
import static graphql.schema.SchemaTransformer.transformSchemaWithDeletes;
3738

3839
/**
3940
* Transforms a schema by applying a visibility predicate to every field.
@@ -75,7 +76,7 @@ public final GraphQLSchema apply(GraphQLSchema schema) {
7576
new SchemaTraverser(getChildrenFn(schema)).depthFirst(new TypeObservingVisitor(observedBeforeTransform), getRootTypes(schema));
7677

7778
// remove fields
78-
GraphQLSchema interimSchema = transformSchema(schema,
79+
GraphQLSchema interimSchema = transformSchemaWithDeletes(schema,
7980
new FieldRemovalVisitor(visibleFieldPredicate, markedForRemovalTypes));
8081

8182
new SchemaTraverser(getChildrenFn(interimSchema)).depthFirst(new TypeObservingVisitor(observedAfterTransform), getRootTypes(interimSchema));
@@ -112,7 +113,7 @@ private Function<GraphQLSchemaElement, List<GraphQLSchemaElement>> getChildrenFn
112113

113114
private GraphQLSchema removeUnreferencedTypes(Set<GraphQLType> markedForRemovalTypes, GraphQLSchema connectedSchema) {
114115
GraphQLSchema withoutAdditionalTypes = connectedSchema.transform(builder -> {
115-
Set<GraphQLType> additionalTypes = new HashSet<>(connectedSchema.getAdditionalTypes());
116+
Set<GraphQLNamedType> additionalTypes = new HashSet<>(connectedSchema.getAdditionalTypes());
116117
additionalTypes.removeAll(markedForRemovalTypes);
117118
builder.clearAdditionalTypes();
118119
builder.additionalTypes(additionalTypes);

0 commit comments

Comments
 (0)