Skip to content

Commit 6c2cdb0

Browse files
andimarekandimarek
authored
Merge pull request #4213 from graphql-java/type-collecting-indirect-fix
Fix type collecting for indirect references
2 parents e860aff + fc78f2f commit 6c2cdb0

2 files changed

Lines changed: 298 additions & 2 deletions

File tree

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

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,13 @@
1818
import graphql.schema.GraphQLTypeReference;
1919
import graphql.schema.GraphQLTypeVisitorStub;
2020
import graphql.schema.GraphQLUnionType;
21+
import graphql.schema.SchemaTraverser;
2122
import graphql.util.TraversalControl;
2223
import graphql.util.TraverserContext;
2324

25+
import java.util.ArrayList;
2426
import java.util.LinkedHashMap;
27+
import java.util.List;
2528
import java.util.Map;
2629
import java.util.TreeMap;
2730
import java.util.function.Supplier;
@@ -175,17 +178,42 @@ public ImmutableMap<String, GraphQLNamedType> getResult() {
175178
* <p>
176179
* The fix: During traversal, we also capture types directly from fields/arguments/inputs
177180
* (in {@link #indirectStrongReferences}). After traversal, we merge any types that were captured
178-
* this way but weren't found through normal traversal.
181+
* this way but weren't found through normal traversal. Additionally, we traverse each newly
182+
* discovered indirect strong reference to collect any types it references, recursively handling
183+
* cases where indirect strong references are nested within other indirect strong references.
184+
* <p>
185+
* We reuse the same visitor instance to ensure duplicate type detection works correctly
186+
* across all traversals.
179187
*
180188
* @param visitedTypes the types collected through normal traversal
181189
*
182190
* @return the fixed map including any dangling replaced types
183191
*/
184192
private Map<String, GraphQLNamedType> fixDanglingReplacedTypes(Map<String, GraphQLNamedType> visitedTypes) {
193+
// Collect indirect strong references that are not yet in the visited types
194+
List<GraphQLNamedType> newlyDiscoveredTypes = new ArrayList<>();
185195
for (GraphQLNamedType indirectStrongReference : indirectStrongReferences.values()) {
186196
String typeName = indirectStrongReference.getName();
187-
visitedTypes.putIfAbsent(typeName, indirectStrongReference);
197+
if (!visitedTypes.containsKey(typeName)) {
198+
visitedTypes.put(typeName, indirectStrongReference);
199+
newlyDiscoveredTypes.add(indirectStrongReference);
200+
}
201+
}
202+
203+
// For each newly discovered type, traverse it to collect any types it references
204+
// We reuse this visitor instance to ensure duplicate type detection works correctly
205+
if (!newlyDiscoveredTypes.isEmpty()) {
206+
// Clear indirect strong references before traversing to capture new ones
207+
indirectStrongReferences.clear();
208+
209+
SchemaTraverser traverser = new SchemaTraverser(
210+
schemaElement -> schemaElement.getChildrenWithTypeReferences().getChildrenAsList());
211+
traverser.depthFirst(this, newlyDiscoveredTypes);
212+
213+
// Recursively fix any newly discovered indirect strong references
214+
fixDanglingReplacedTypes(visitedTypes);
188215
}
216+
189217
return visitedTypes;
190218
}
191219
}

‎src/test/groovy/graphql/schema/SchemaTransformerTest.groovy‎

Lines changed: 268 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package graphql.schema
22

33

4+
import graphql.AssertException
45
import graphql.GraphQL
56
import graphql.Scalars
67
import graphql.TestUtil
@@ -1196,4 +1197,271 @@ type Rental {
11961197
id: ID
11971198
}""".trim()
11981199
}
1200+
1201+
def "indirect type references should have their children collected"() {
1202+
given:
1203+
// Bar is referenced by Foo.bar directly
1204+
def bar = newObject()
1205+
.name("Bar")
1206+
.field(newFieldDefinition()
1207+
.name("id")
1208+
.type(Scalars.GraphQLID)
1209+
.build())
1210+
.build()
1211+
1212+
// Foo references Bar directly via Foo.bar field
1213+
def foo = newObject()
1214+
.name("Foo")
1215+
.field(newFieldDefinition()
1216+
.name("bar")
1217+
.type(bar) // Direct reference to Bar
1218+
.build())
1219+
.build()
1220+
1221+
// Query.foo1 references Foo via type reference (indirect)
1222+
// Query.foo2 references Foo directly (strong reference)
1223+
def query = newObject()
1224+
.name("Query")
1225+
.field(newFieldDefinition()
1226+
.name("foo1")
1227+
.type(typeRef("Foo")) // Indirect reference via typeRef
1228+
.build())
1229+
.field(newFieldDefinition()
1230+
.name("foo2")
1231+
.type(foo) // Direct reference to Foo
1232+
.build())
1233+
.build()
1234+
1235+
def schema = newSchema()
1236+
.query(query)
1237+
.build()
1238+
1239+
// Visitor that removes Query.foo2
1240+
def visitor = new GraphQLTypeVisitorStub() {
1241+
@Override
1242+
TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext<GraphQLSchemaElement> context) {
1243+
if (node.name == "foo2") {
1244+
return deleteNode(context)
1245+
}
1246+
return TraversalControl.CONTINUE
1247+
}
1248+
}
1249+
1250+
when:
1251+
def newSchema = SchemaTransformer.transformSchemaWithDeletes(schema, visitor)
1252+
1253+
then: "Query.foo2 should be removed"
1254+
(newSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("foo2") == null
1255+
1256+
and: "Query.foo1 should still exist"
1257+
(newSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("foo1") != null
1258+
1259+
and: "Foo should still exist (reachable via Query.foo1)"
1260+
newSchema.getType("Foo") != null
1261+
1262+
and: "Bar should still exist (reachable via Query.foo1 -> Foo -> bar)"
1263+
newSchema.getType("Bar") != null
1264+
}
1265+
1266+
def "nested indirect type references requiring multiple traversals should have their children collected"() {
1267+
given:
1268+
// Create a deeply nested structure where each level has indirect references:
1269+
// Query.level1 -> Level1 (via typeRef) -> Level2 (direct) -> Level3 (via typeRef) -> Level4 (direct) -> Leaf (direct)
1270+
// Query.directRef -> Level1 (direct) - this is the only direct path
1271+
// When we remove Query.directRef, the nested traversals should still find all types
1272+
1273+
def leaf = newObject()
1274+
.name("Leaf")
1275+
.field(newFieldDefinition()
1276+
.name("value")
1277+
.type(Scalars.GraphQLString)
1278+
.build())
1279+
.build()
1280+
1281+
def level4 = newObject()
1282+
.name("Level4")
1283+
.field(newFieldDefinition()
1284+
.name("leaf")
1285+
.type(leaf) // Direct reference to Leaf
1286+
.build())
1287+
.build()
1288+
1289+
def level3 = newObject()
1290+
.name("Level3")
1291+
.field(newFieldDefinition()
1292+
.name("level4")
1293+
.type(level4) // Direct reference to Level4
1294+
.build())
1295+
.build()
1296+
1297+
def level2 = newObject()
1298+
.name("Level2")
1299+
.field(newFieldDefinition()
1300+
.name("level3")
1301+
.type(typeRef("Level3")) // Indirect reference via typeRef
1302+
.build())
1303+
.field(newFieldDefinition()
1304+
.name("level3Direct")
1305+
.type(level3) // Direct reference to Level3 (needed for schema build)
1306+
.build())
1307+
.build()
1308+
1309+
def level1 = newObject()
1310+
.name("Level1")
1311+
.field(newFieldDefinition()
1312+
.name("level2")
1313+
.type(level2) // Direct reference to Level2
1314+
.build())
1315+
.build()
1316+
1317+
def query = newObject()
1318+
.name("Query")
1319+
.field(newFieldDefinition()
1320+
.name("level1Indirect")
1321+
.type(typeRef("Level1")) // Indirect reference via typeRef
1322+
.build())
1323+
.field(newFieldDefinition()
1324+
.name("level1Direct")
1325+
.type(level1) // Direct reference to Level1
1326+
.build())
1327+
.build()
1328+
1329+
def schema = newSchema()
1330+
.query(query)
1331+
.build()
1332+
1333+
// Visitor that removes Query.level1Direct and Level2.level3Direct
1334+
// This leaves only indirect paths: Query.level1Indirect -> Level1 -> Level2.level3 -> Level3 -> Level4 -> Leaf
1335+
def visitor = new GraphQLTypeVisitorStub() {
1336+
@Override
1337+
TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext<GraphQLSchemaElement> context) {
1338+
if (node.name == "level1Direct" || node.name == "level3Direct") {
1339+
return deleteNode(context)
1340+
}
1341+
return TraversalControl.CONTINUE
1342+
}
1343+
}
1344+
1345+
when:
1346+
def newSchema = SchemaTransformer.transformSchemaWithDeletes(schema, visitor)
1347+
1348+
then: "Direct fields should be removed"
1349+
(newSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("level1Direct") == null
1350+
(newSchema.getType("Level2") as GraphQLObjectType).getFieldDefinition("level3Direct") == null
1351+
1352+
and: "Indirect fields should still exist"
1353+
(newSchema.getType("Query") as GraphQLObjectType).getFieldDefinition("level1Indirect") != null
1354+
(newSchema.getType("Level2") as GraphQLObjectType).getFieldDefinition("level3") != null
1355+
1356+
and: "All types in the chain should still exist (discovered through nested indirect reference traversal)"
1357+
newSchema.getType("Level1") != null
1358+
newSchema.getType("Level2") != null
1359+
newSchema.getType("Level3") != null
1360+
newSchema.getType("Level4") != null
1361+
newSchema.getType("Leaf") != null
1362+
}
1363+
1364+
def "redefined types are caught when introduced during transformation and discovered through indirect references"() {
1365+
given:
1366+
// Build a valid schema where:
1367+
// - Query.fooIndirect -> Foo (via typeRef)
1368+
// - Query.fooDirect -> Foo (direct) - will be removed during transformation
1369+
// - Foo.targetType -> TargetType (direct) - will be REPLACED during transformation
1370+
// - Query.existingType -> ExistingType (direct) - already in schema
1371+
//
1372+
// During transformation, we will:
1373+
// 1. Remove Query.fooDirect (so Foo is only reachable via indirect reference)
1374+
// 2. Replace TargetType with a NEW object also named "ExistingType" (introduces duplicate)
1375+
//
1376+
// When fixDanglingReplacedTypes traverses from Foo (indirect reference),
1377+
// it should discover the replaced type and detect the duplicate with ExistingType
1378+
1379+
def targetType = newObject()
1380+
.name("TargetType")
1381+
.field(newFieldDefinition()
1382+
.name("id")
1383+
.type(Scalars.GraphQLID)
1384+
.build())
1385+
.build()
1386+
1387+
def existingType = newObject()
1388+
.name("ExistingType")
1389+
.field(newFieldDefinition()
1390+
.name("name")
1391+
.type(Scalars.GraphQLString)
1392+
.build())
1393+
.build()
1394+
1395+
def foo = newObject()
1396+
.name("Foo")
1397+
.field(newFieldDefinition()
1398+
.name("targetType")
1399+
.type(targetType)
1400+
.build())
1401+
.build()
1402+
1403+
def query = newObject()
1404+
.name("Query")
1405+
.field(newFieldDefinition()
1406+
.name("fooIndirect")
1407+
.type(typeRef("Foo")) // Indirect reference
1408+
.build())
1409+
.field(newFieldDefinition()
1410+
.name("fooDirect")
1411+
.type(foo) // Direct reference - will be removed
1412+
.build())
1413+
.field(newFieldDefinition()
1414+
.name("existingType")
1415+
.type(existingType) // Direct reference to ExistingType
1416+
.build())
1417+
.build()
1418+
1419+
def schema = newSchema()
1420+
.query(query)
1421+
.build()
1422+
1423+
// Create a duplicate type with the same name as ExistingType but different instance
1424+
def duplicateExistingType = newObject()
1425+
.name("ExistingType")
1426+
.field(newFieldDefinition()
1427+
.name("differentField") // Different field makes it a different object
1428+
.type(Scalars.GraphQLInt)
1429+
.build())
1430+
.build()
1431+
1432+
// Visitor that:
1433+
// 1. Removes Query.fooDirect (so Foo is only reachable via indirect reference)
1434+
// 2. Replaces TargetType with duplicateExistingType (introduces a duplicate "ExistingType")
1435+
def visitor = new GraphQLTypeVisitorStub() {
1436+
@Override
1437+
TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext<GraphQLSchemaElement> context) {
1438+
if (node.name == "fooDirect") {
1439+
return deleteNode(context)
1440+
}
1441+
return TraversalControl.CONTINUE
1442+
}
1443+
1444+
@Override
1445+
TraversalControl visitGraphQLObjectType(GraphQLObjectType node, TraverserContext<GraphQLSchemaElement> context) {
1446+
if (node.name == "TargetType") {
1447+
// Replace TargetType with a type named "ExistingType" (duplicate!)
1448+
return changeNode(context, duplicateExistingType)
1449+
}
1450+
return TraversalControl.CONTINUE
1451+
}
1452+
}
1453+
1454+
when:
1455+
// This should fail because:
1456+
// 1. After removing fooDirect, Foo is only reachable via fooIndirect (typeRef)
1457+
// 2. fixDanglingReplacedTypes traverses from Foo
1458+
// 3. It discovers the replaced type (now named "ExistingType")
1459+
// 4. This conflicts with the already-collected ExistingType from Query.existingType
1460+
SchemaTransformer.transformSchemaWithDeletes(schema, visitor)
1461+
1462+
then:
1463+
def e = thrown(AssertException)
1464+
e.getMessage().contains("All types within a GraphQL schema must have unique names")
1465+
e.getMessage().contains("ExistingType")
1466+
}
11991467
}

0 commit comments

Comments
 (0)