Skip to content

Commit 8d747c1

Browse files
tinnoutinnou
authored
Fix nested detached types were not added to additionalTypes (#2157)
1 parent 0d4492c commit 8d747c1

4 files changed

Lines changed: 294 additions & 33 deletions

File tree

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

Lines changed: 64 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
import java.util.ArrayList;
7474
import java.util.Arrays;
7575
import java.util.Deque;
76+
import java.util.HashSet;
7677
import java.util.LinkedHashMap;
7778
import java.util.LinkedHashSet;
7879
import java.util.List;
@@ -1149,31 +1150,73 @@ List<UnionTypeExtensionDefinition> unionTypeExtensions(UnionTypeDefinition typeD
11491150
* @return the additional types not referenced from the top level operations
11501151
*/
11511152
Set<GraphQLType> buildAdditionalTypes(BuildContext buildCtx) {
1152-
Set<GraphQLType> additionalTypes = new LinkedHashSet<>();
11531153
TypeDefinitionRegistry typeRegistry = buildCtx.getTypeRegistry();
1154-
typeRegistry.types().values().forEach(typeDefinition -> {
1155-
TypeName typeName = TypeName.newTypeName().name(typeDefinition.getName()).build();
1156-
if (typeDefinition instanceof InputObjectTypeDefinition) {
1157-
if (buildCtx.hasInputType(typeDefinition) == null) {
1158-
additionalTypes.add(buildInputType(buildCtx, typeName));
1159-
}
1160-
} else {
1161-
if (buildCtx.hasOutputType(typeDefinition) == null) {
1162-
additionalTypes.add(buildOutputType(buildCtx, typeName));
1163-
}
1164-
}
1165-
});
1166-
typeRegistry.scalars().values().forEach(scalarTypeDefinition -> {
1167-
if (ScalarInfo.isGraphqlSpecifiedScalar(scalarTypeDefinition.getName())) {
1168-
return;
1169-
}
1170-
if (buildCtx.hasInputType(scalarTypeDefinition) == null && buildCtx.hasOutputType(scalarTypeDefinition) == null) {
1171-
additionalTypes.add(buildScalar(buildCtx, scalarTypeDefinition));
1172-
}
1173-
});
1154+
1155+
Set<String> detachedTypeNames = getDetachedTypeNames(buildCtx);
1156+
1157+
Set<GraphQLType> additionalTypes = new LinkedHashSet<>();
1158+
// recursively record detached types on the ctx and add them to the additionalTypes set
1159+
typeRegistry.types().values().stream()
1160+
.filter(typeDefinition -> detachedTypeNames.contains(typeDefinition.getName()))
1161+
.forEach(typeDefinition -> {
1162+
TypeName typeName = TypeName.newTypeName().name(typeDefinition.getName()).build();
1163+
1164+
if (typeDefinition instanceof InputObjectTypeDefinition) {
1165+
if (buildCtx.hasInputType(typeDefinition) == null) {
1166+
buildCtx.putInputType((GraphQLNamedInputType) buildInputType(buildCtx, typeName));
1167+
}
1168+
additionalTypes.add(buildCtx.inputGTypes.get(typeDefinition.getName()));
1169+
} else {
1170+
if (buildCtx.hasOutputType(typeDefinition) == null) {
1171+
buildCtx.putOutputType(buildOutputType(buildCtx, typeName));
1172+
}
1173+
additionalTypes.add(buildCtx.outputGTypes.get(typeDefinition.getName()));
1174+
}
1175+
});
1176+
1177+
typeRegistry.scalars().values().stream()
1178+
.filter(typeDefinition -> detachedTypeNames.contains(typeDefinition.getName()))
1179+
.forEach(scalarTypeDefinition -> {
1180+
if (ScalarInfo.isGraphqlSpecifiedScalar(scalarTypeDefinition.getName())) {
1181+
return;
1182+
}
1183+
1184+
if (buildCtx.hasInputType(scalarTypeDefinition) == null && buildCtx.hasOutputType(scalarTypeDefinition) == null) {
1185+
buildCtx.putOutputType(buildScalar(buildCtx, scalarTypeDefinition));
1186+
}
1187+
if (buildCtx.hasInputType(scalarTypeDefinition) != null) {
1188+
additionalTypes.add(buildCtx.inputGTypes.get(scalarTypeDefinition.getName()));
1189+
} else if (buildCtx.hasOutputType(scalarTypeDefinition) != null) {
1190+
additionalTypes.add(buildCtx.outputGTypes.get(scalarTypeDefinition.getName()));
1191+
}
1192+
});
1193+
11741194
return additionalTypes;
11751195
}
11761196

1197+
/**
1198+
* Detached types (or additional types) are all types that
1199+
* are not connected to the root operations types.
1200+
*
1201+
* @param buildCtx buildCtx
1202+
* @return detached type names
1203+
*/
1204+
private Set<String> getDetachedTypeNames(BuildContext buildCtx) {
1205+
TypeDefinitionRegistry typeRegistry = buildCtx.getTypeRegistry();
1206+
// connected types are all types that have a path that connects them back to the root operation types.
1207+
Set<String> connectedTypes = new HashSet<>(buildCtx.inputGTypes.keySet());
1208+
connectedTypes.addAll(buildCtx.outputGTypes.keySet());
1209+
1210+
Set<String> allTypeNames = new HashSet<>(typeRegistry.types().keySet());
1211+
Set<String> scalars = new HashSet<>(typeRegistry.scalars().keySet());
1212+
allTypeNames.addAll(scalars);
1213+
1214+
// detached types are all types minus the connected types.
1215+
Set<String> detachedTypeNames = new HashSet<>(allTypeNames);
1216+
detachedTypeNames.removeAll(connectedTypes);
1217+
return detachedTypeNames;
1218+
}
1219+
11771220
Set<GraphQLDirective> buildAdditionalDirectives(BuildContext buildCtx) {
11781221
Set<GraphQLDirective> additionalDirectives = new LinkedHashSet<>();
11791222
TypeDefinitionRegistry typeRegistry = buildCtx.getTypeRegistry();

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

Lines changed: 58 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import graphql.schema.GraphQLInputObjectField;
77
import graphql.schema.GraphQLInterfaceType;
88
import graphql.schema.GraphQLNamedSchemaElement;
9+
import graphql.schema.GraphQLNamedType;
910
import graphql.schema.GraphQLObjectType;
1011
import graphql.schema.GraphQLSchema;
1112
import graphql.schema.GraphQLSchemaElement;
@@ -52,7 +53,7 @@ public FieldVisibilitySchemaTransformation(VisibleFieldPredicate visibleFieldPre
5253
public final GraphQLSchema apply(GraphQLSchema schema) {
5354
Set<GraphQLType> observedBeforeTransform = new HashSet<>();
5455
Set<GraphQLType> observedAfterTransform = new HashSet<>();
55-
Set<GraphQLType> removedTypes = new HashSet<>();
56+
Set<GraphQLType> markedForRemovalTypes = new HashSet<>();
5657

5758
// query, mutation, and subscription types should not be removed
5859
final Set<String> protectedTypeNames = getRootTypes(schema).stream()
@@ -65,20 +66,46 @@ public final GraphQLSchema apply(GraphQLSchema schema) {
6566

6667
// remove fields
6768
GraphQLSchema interimSchema = transformSchema(schema,
68-
new FieldRemovalVisitor(visibleFieldPredicate, removedTypes));
69+
new FieldRemovalVisitor(visibleFieldPredicate, markedForRemovalTypes));
6970

7071
new SchemaTraverser().depthFirst(new TypeObservingVisitor(observedAfterTransform, interimSchema), getRootTypes(interimSchema));
7172

72-
// remove types that are not used
73-
GraphQLSchema finalSchema = transformSchema(interimSchema,
74-
new TypeVisibilityVisitor(protectedTypeNames, observedBeforeTransform, observedAfterTransform,
75-
removedTypes));
73+
// remove types that are not used after removing fields - (connected schema only)
74+
GraphQLSchema connectedSchema = transformSchema(interimSchema,
75+
new TypeVisibilityVisitor(protectedTypeNames, observedBeforeTransform, observedAfterTransform));
76+
77+
// ensure markedForRemovalTypes are not referenced by other schema elements, and delete from the schema
78+
// the ones that aren't.
79+
GraphQLSchema finalSchema = removeUnreferencedTypes(markedForRemovalTypes, connectedSchema);
7680

7781
afterTransformationHook.run();
7882

7983
return finalSchema;
8084
}
8185

86+
private GraphQLSchema removeUnreferencedTypes(Set<GraphQLType> markedForRemovalTypes, GraphQLSchema connectedSchema) {
87+
GraphQLSchema withoutAdditionalTypes = connectedSchema.transform(builder -> {
88+
Set<GraphQLType> additionalTypes = new HashSet<>(connectedSchema.getAdditionalTypes());
89+
additionalTypes.removeAll(markedForRemovalTypes);
90+
builder.clearAdditionalTypes();
91+
builder.additionalTypes(additionalTypes);
92+
});
93+
94+
// remove from markedForRemovalTypes any type that might still be referenced by other schema elements
95+
transformSchema(withoutAdditionalTypes, new AdditionalTypeVisibilityVisitor(markedForRemovalTypes));
96+
97+
// finally remove the types on the schema we are certain aren't referenced by any other node.
98+
return transformSchema(connectedSchema, new GraphQLTypeVisitorStub() {
99+
@Override
100+
protected TraversalControl visitGraphQLType(GraphQLSchemaElement node, TraverserContext<GraphQLSchemaElement> context) {
101+
if (node instanceof GraphQLType && markedForRemovalTypes.contains(node)) {
102+
return deleteNode(context);
103+
}
104+
return super.visitGraphQLType(node, context);
105+
}
106+
});
107+
}
108+
82109
private static class TypeObservingVisitor extends GraphQLTypeVisitorStub {
83110

84111
private final Set<GraphQLType> observedTypes;
@@ -151,16 +178,13 @@ private static class TypeVisibilityVisitor extends GraphQLTypeVisitorStub {
151178
private final Set<String> protectedTypeNames;
152179
private final Set<GraphQLType> observedBeforeTransform;
153180
private final Set<GraphQLType> observedAfterTransform;
154-
private final Set<GraphQLType> removedTypes;
155181

156182
private TypeVisibilityVisitor(Set<String> protectedTypeNames,
157183
Set<GraphQLType> observedTypes,
158-
Set<GraphQLType> observedAfterTransform,
159-
Set<GraphQLType> removedTypes) {
184+
Set<GraphQLType> observedAfterTransform) {
160185
this.protectedTypeNames = protectedTypeNames;
161186
this.observedBeforeTransform = observedTypes;
162187
this.observedAfterTransform = observedAfterTransform;
163-
this.removedTypes = removedTypes;
164188
}
165189

166190
@Override
@@ -186,6 +210,30 @@ public TraversalControl visitGraphQLType(GraphQLSchemaElement node,
186210
}
187211
}
188212

213+
private static class AdditionalTypeVisibilityVisitor extends GraphQLTypeVisitorStub {
214+
215+
private final Set<GraphQLType> markedForRemovalTypes;
216+
217+
private AdditionalTypeVisibilityVisitor(Set<GraphQLType> markedForRemovalTypes) {
218+
this.markedForRemovalTypes = markedForRemovalTypes;
219+
}
220+
221+
@Override
222+
public TraversalControl visitGraphQLType(GraphQLSchemaElement node,
223+
TraverserContext<GraphQLSchemaElement> context) {
224+
225+
if (node instanceof GraphQLNamedType) {
226+
GraphQLNamedType namedType = (GraphQLNamedType) node;
227+
// we encountered a node referencing one of the marked types, so it should not be removed.
228+
if (markedForRemovalTypes.contains(node)) {
229+
markedForRemovalTypes.remove(namedType);
230+
}
231+
}
232+
233+
return TraversalControl.CONTINUE;
234+
}
235+
}
236+
189237
private List<GraphQLObjectType> getRootTypes(GraphQLSchema schema) {
190238
return Stream.of(
191239
schema.getQueryType(),

‎src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import graphql.schema.GraphQLFieldsContainer
1212
import graphql.schema.GraphQLInputObjectType
1313
import graphql.schema.GraphQLInterfaceType
1414
import graphql.schema.GraphQLList
15+
import graphql.schema.GraphQLNamedType
1516
import graphql.schema.GraphQLNonNull
1617
import graphql.schema.GraphQLObjectType
1718
import graphql.schema.GraphQLScalarType
@@ -1147,6 +1148,103 @@ class SchemaGeneratorTest extends Specification {
11471148
schema.getType("UnReferencedD") instanceof GraphQLUnionType
11481149
}
11491150

1151+
def "nested additional types should be part of the additional types, not the schema types"() {
1152+
def spec = """
1153+
type Query {
1154+
fieldA : ReferencedA
1155+
}
1156+
1157+
type ReferencedA {
1158+
field : String
1159+
}
1160+
1161+
type UnReferencedA {
1162+
field : UnReferencedNestedE
1163+
}
1164+
1165+
input UnReferencedB {
1166+
field : UnReferencedNestedF
1167+
}
1168+
1169+
type UnReferencedNestedE {
1170+
field: String
1171+
field2: UnReferencedScalarB
1172+
}
1173+
1174+
input UnReferencedNestedF {
1175+
field: String
1176+
}
1177+
1178+
interface UnReferencedC {
1179+
field : UnReferencedNestedE
1180+
}
1181+
1182+
union UnReferencedD = ReferencedA
1183+
1184+
scalar UnReferencedScalarA
1185+
1186+
scalar UnReferencedScalarB
1187+
"""
1188+
1189+
def schema = schema(spec)
1190+
1191+
expect: "all types to be registered"
1192+
schema.getType("ReferencedA") instanceof GraphQLObjectType
1193+
schema.getType("UnReferencedA") instanceof GraphQLObjectType
1194+
schema.getType("UnReferencedB") instanceof GraphQLInputObjectType
1195+
schema.getType("UnReferencedC") instanceof GraphQLInterfaceType
1196+
schema.getType("UnReferencedD") instanceof GraphQLUnionType
1197+
schema.getType("UnReferencedNestedE") instanceof GraphQLObjectType
1198+
schema.getType("UnReferencedNestedF") instanceof GraphQLInputObjectType
1199+
1200+
1201+
and: "unreferenced types should all be additional types"
1202+
1203+
def namedTypes = schema.getAdditionalTypes() as Set<GraphQLNamedType>
1204+
namedTypes.name.toSet() == ["UnReferencedA",
1205+
"UnReferencedB",
1206+
"UnReferencedC",
1207+
"UnReferencedD",
1208+
"UnReferencedNestedE",
1209+
"UnReferencedNestedF",
1210+
"UnReferencedScalarA",
1211+
"UnReferencedScalarB"].toSet()
1212+
}
1213+
1214+
1215+
def "nested additional types recursive"() {
1216+
def spec = """
1217+
type Query {
1218+
fieldA : ReferencedA
1219+
}
1220+
1221+
type ReferencedA {
1222+
field : String
1223+
}
1224+
1225+
type UnReferencedA {
1226+
field : UnReferencedNestedB
1227+
}
1228+
1229+
type UnReferencedNestedB {
1230+
field: UnReferencedNestedB
1231+
}
1232+
"""
1233+
1234+
def schema = schema(spec)
1235+
1236+
expect: "all types to be registered"
1237+
schema.getType("ReferencedA") instanceof GraphQLObjectType
1238+
schema.getType("UnReferencedA") instanceof GraphQLObjectType
1239+
schema.getType("UnReferencedNestedB") instanceof GraphQLObjectType
1240+
1241+
and: "unreferenced types should all be additional types"
1242+
1243+
def namedTypes = schema.getAdditionalTypes() as Set<GraphQLNamedType>
1244+
namedTypes.name.toSet() == ["UnReferencedA", "UnReferencedNestedB"].toSet()
1245+
}
1246+
1247+
11501248
def "scalar default value is parsed"() {
11511249
def spec = """
11521250
type Query {

0 commit comments

Comments
 (0)