Skip to content

Commit 3860962

Browse files
alfalf
committed
Make sure SchemaFeatureFilter visits all nodes
This works around graphql-java/graphql-java#4133. Since we were relying on not visiting all nodes for pruning the schema, I added some extra code to remove unreachable types. This uncovered a bug in our existing test, so I fixed the test. During my changes here I triggered some strage behaviour related to running the Federation transforms multiple times when `removeFederationDefinitions` is false. Because of this I made the code skip this step if the _Entity union type already exists. Finally I added another test to verify that we're removing dangling union type members.
1 parent d322f72 commit 3860962

6 files changed

Lines changed: 101 additions & 10 deletions

File tree

‎graphitron-schema-transform/src/main/java/no/fellesstudentsystem/schema_transformer/SchemaTransformer.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
import com.apollographql.federation.graphqljava.Federation;
44
import com.apollographql.federation.graphqljava.directives.LinkDirectiveProcessor;
55
import graphql.language.ScalarTypeDefinition;
6+
import graphql.language.TypeName;
7+
import graphql.language.UnionTypeDefinition;
68
import graphql.schema.GraphQLSchema;
79
import graphql.schema.GraphQLType;
810
import graphql.schema.idl.*;
@@ -108,13 +110,18 @@ public static GraphQLSchema assembleSchema(TypeDefinitionRegistry typeDefinition
108110
});
109111
});
110112

111-
if (federationIsImported(typeDefinitionRegistry)) {
113+
if (federationIsImported(typeDefinitionRegistry) && !federationAlreadyAssembled(typeDefinitionRegistry)) {
112114
return assembleSchemaWithFederation(typeDefinitionRegistry, runtimeWiring);
113115
} else {
114116
return new SchemaGenerator().makeExecutableSchema(typeDefinitionRegistry, runtimeWiring);
115117
}
116118
}
117119

120+
private static boolean federationAlreadyAssembled(TypeDefinitionRegistry typeDefinitionRegistry) {
121+
var _entity = typeDefinitionRegistry.getType("_Entity", UnionTypeDefinition.class);
122+
return _entity.isPresent();
123+
}
124+
118125
private static boolean federationIsImported(TypeDefinitionRegistry typeDefinitionRegistry) {
119126
return LinkDirectiveProcessor.loadFederationImportedDefinitions(typeDefinitionRegistry) != null;
120127
}

‎graphitron-schema-transform/src/main/java/no/fellesstudentsystem/schema_transformer/transform/SchemaFeatureFilter.java‎

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
package no.fellesstudentsystem.schema_transformer.transform;
22

33
import graphql.language.DirectivesContainer;
4+
import graphql.language.UnionTypeDefinition;
45
import graphql.schema.*;
56
import graphql.util.TraversalControl;
67
import graphql.util.TraverserContext;
78

8-
import java.util.List;
9-
import java.util.Set;
9+
import java.util.*;
1010
import java.util.stream.Collectors;
1111
import java.util.stream.Stream;
1212

@@ -120,7 +120,55 @@ private TraversalControl visitFieldElement(GraphQLNamedSchemaElement node, Trave
120120
}
121121
};
122122

123-
return SchemaTransformer.transformSchema(schema, visitorFields); // Seems the objects get removed automatically when no fields point to them.
123+
// Make sure we always visit every type
124+
var schemaCopy = schema;
125+
schema = schema.transform(builder -> {
126+
for (var type : schemaCopy.getTypeMap().values()) {
127+
if (type != schemaCopy.getQueryType() && type != schemaCopy.getMutationType() && type != schemaCopy.getSubscriptionType()) {
128+
builder.additionalType(type);
129+
}
130+
}
131+
});
132+
133+
var prunedSchema = SchemaTransformer.transformSchema(schema, visitorFields); // Seems the objects get removed automatically when no fields point to them.
134+
return removeUnreachableTypes(prunedSchema);
135+
}
136+
137+
private GraphQLSchema removeUnreachableTypes(GraphQLSchema schema) {
138+
var reachableTypes = findReachableTypes(schema);
139+
return schema.transform(builder -> builder.clearAdditionalTypes().additionalTypes(reachableTypes));
140+
}
141+
142+
private Set<GraphQLType> findReachableTypes(GraphQLSchema schema) {
143+
Set<String> reachableNames = new HashSet<>();
144+
Set<GraphQLType> reachableTypes = new HashSet<>();
145+
146+
var schemaRoots = Stream.of(schema.getQueryType(), schema.getMutationType(), schema.getSubscriptionType())
147+
.filter(Objects::nonNull).toList();
148+
Queue<GraphQLType> toVisit = new LinkedList<>(schemaRoots);
149+
150+
while (!toVisit.isEmpty()) {
151+
GraphQLType type = GraphQLTypeUtil.unwrapAll(toVisit.poll());
152+
if (type instanceof GraphQLNamedType namedType) {
153+
var alreadyVisited = reachableNames.add(namedType.getName());
154+
if (alreadyVisited) {
155+
continue;
156+
}
157+
158+
reachableTypes.add(namedType);
159+
160+
var children = schema.getTypeMap().get(namedType.getName()).getChildren();
161+
for (var child : children) {
162+
if (child instanceof GraphQLNamedType namedChild) {
163+
if (!reachableNames.contains(namedChild.getName())) {
164+
toVisit.add(namedChild);
165+
}
166+
}
167+
}
168+
}
169+
}
170+
171+
return reachableTypes;
124172
}
125173

126174
private boolean containerIsEmpty(GraphQLInputObjectType node) {

‎graphitron-schema-transform/src/test/java/no/fellesstudentsystem/schema_transformer/transform/SchemaFeatureFilterTest.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,13 @@ void filter_shouldRemoveUnusedTypes() {
8282
testSchema(new SchemaFeatureFilter(Set.of()).getFilteredGraphQLSchema(schema), expected);
8383
}
8484

85+
@Test
86+
void filter_shouldRemoveDanglingUnionMembers() {
87+
var schema = createSchema("removesDanglingUnionMembers/test.graphql");
88+
var expected = createSchema("removesDanglingUnionMembers/expected.graphql");
89+
testSchema(new SchemaFeatureFilter(Set.of()).getFilteredGraphQLSchema(schema), expected);
90+
}
91+
8592
@Test
8693
void filter_shouldRemoveEmptyTypes() {
8794
var schema = createSchema("removesEmptyTypes/test.graphql");

‎graphitron-schema-transform/src/test/resources/transform/removesDanglingUnionMembers/expected.graphql‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
type Query {
2+
q1: U0
3+
}
4+
5+
union U0 = T1
6+
7+
type T1 {
8+
f0: [String]
9+
}

‎graphitron-schema-transform/src/test/resources/transform/removesDanglingUnionMembers/test.graphql‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
type Query {
2+
q1: U0
3+
}
4+
5+
type T0 {
6+
f0: [String] @feature(flags: "F0")
7+
u1: U1 @feature(flags: "F0")
8+
}
9+
10+
type T1 {
11+
f0: [String]
12+
}
13+
14+
union U0 = T0 | T1
15+
16+
union U1 = UgyldigId
17+
18+
interface Error {
19+
path: [String!]! @feature(flags: "F0")
20+
message: String! @feature(flags: "F0")
21+
}
22+
23+
type UgyldigId implements Error {
24+
path: [String!]! @feature(flags: "F0")
25+
message: String! @feature(flags: "F0")
26+
}

‎graphitron-schema-transform/src/test/resources/transform/removesUnusedTypes/expected.graphql‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,3 @@
11
type Query {
22
q1: Boolean
33
}
4-
5-
union U0 = T1
6-
7-
type T1 {
8-
f0: [String]
9-
}

0 commit comments

Comments
 (0)