From 9cd28ff9053a59683363bb045a9342fa710268d3 Mon Sep 17 00:00:00 2001 From: Marc-Andre Giroux Date: Fri, 29 Jul 2022 17:34:01 -0400 Subject: [PATCH 1/7] add a test to reproduce GraphQLTypeReference issue in transforms --- .../graphql/schema/GraphQLSchemaTest.groovy | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy index 40a0a3fd1f..a7e955f6c6 100644 --- a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy @@ -10,6 +10,7 @@ import graphql.schema.idl.TypeRuntimeWiring import spock.lang.Specification import java.util.function.UnaryOperator +import java.util.stream.Collectors import static graphql.Scalars.GraphQLString import static graphql.StarWarsSchema.characterInterface @@ -187,7 +188,6 @@ class GraphQLSchemaTest extends Specification { type Dog implements Pet { name : String } - type Cat implements Pet { name : String } @@ -214,6 +214,46 @@ class GraphQLSchemaTest extends Specification { dogType.getName() == "Dog" } + def "issue with type references when original type is transformed away"() { + def sdl = ''' + type Query { + # The b fields leads to the addition of the B type (actual definition) + b: B + # When we filter out the `b` field, we can still access B through A + # however they are GraphQLTypeReferences and not an actual GraphQL Object + a: A + } + + type A { + b: B + } + + type B { + a: A + b: B + } + ''' + + when: + def schema = TestUtil.schema(sdl) + // When field `a` is filtered out + List fields = schema.queryType.fieldDefinitions.stream().filter({ + it.name == "a" + }).collect(Collectors.toList()) + // And we transform the schema's query root, the schema building + // will throw because type B won't be in the type map anymore, since + // there are no more actual B object types in the schema tree. + def transformed = schema.transform({ + it.query(schema.queryType.transform({ + it.replaceFields(fields) + })) + }) + + then: + transformed.containsType("B") + + } + def "cheap transform without types transformation works"() { def sdl = ''' From 9ccbe94e6f19df675922210946764ae799e88665 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Sun, 31 Jul 2022 20:53:01 +1000 Subject: [PATCH 2/7] Added extra test for schema transform --- .../graphql/schema/GraphQLSchemaTest.groovy | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy index a7e955f6c6..a9c19f3a06 100644 --- a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy @@ -7,6 +7,8 @@ import graphql.GraphQL import graphql.TestUtil import graphql.schema.idl.RuntimeWiring import graphql.schema.idl.TypeRuntimeWiring +import graphql.util.TraversalControl +import graphql.util.TraverserContext import spock.lang.Specification import java.util.function.UnaryOperator @@ -254,6 +256,55 @@ class GraphQLSchemaTest extends Specification { } + def "can change types via SchemaTransformer and visitor"() { + def sdl = ''' + type Query { + # The b fields leads to the addition of the B type (actual definition) + b: B + # When we filter out the `b` field, we can still access B through A + # however they are GraphQLTypeReferences and not an actual GraphQL Object + a: A + } + + type A { + b: B + } + + type B { + a: A + b: B + } + ''' + + when: + def schema = TestUtil.schema(sdl) + + GraphQLTypeVisitorStub visitor = new GraphQLTypeVisitorStub() { + @Override + TraversalControl visitGraphQLObjectType(GraphQLObjectType objectType, TraverserContext context) { + GraphQLCodeRegistry.Builder codeRegistry = context.getVarFromParents(GraphQLCodeRegistry.Builder.class); + // we need to change __XXX introspection types to have directive extensions + if (objectType.getName().equals("Query")) { + def queryType = objectType + List fields = queryType.fieldDefinitions.stream().filter({ + it.name == "a" + }).collect(Collectors.toList()) + + GraphQLObjectType newObjectType = queryType.transform({ + it.replaceFields(fields) + }) + + return changeNode(context, newObjectType); + } + return TraversalControl.CONTINUE + } + } + GraphQLSchema transformed = SchemaTransformer.transformSchema(schema, visitor) + + then: + transformed.containsType("B") + } + def "cheap transform without types transformation works"() { def sdl = ''' From 3de7dc45f4288cfe9260d6e2b6a2b5ce5de72a52 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 2 Aug 2022 12:38:48 +1000 Subject: [PATCH 3/7] Fixes the bug where dangling actual references are not used during schema rebuild --- .../java/graphql/schema/GraphQLTypeUtil.java | 18 ++++++---- .../impl/GraphQLTypeCollectingVisitor.java | 35 +++++++++++++++++-- .../graphql/schema/GraphQLSchemaTest.groovy | 31 ++++++++++++++++ .../graphql/schema/GraphQLTypeUtilTest.groovy | 33 +++++++++++++++++ 4 files changed, 107 insertions(+), 10 deletions(-) diff --git a/src/main/java/graphql/schema/GraphQLTypeUtil.java b/src/main/java/graphql/schema/GraphQLTypeUtil.java index bdd01fc760..1306ce2448 100644 --- a/src/main/java/graphql/schema/GraphQLTypeUtil.java +++ b/src/main/java/graphql/schema/GraphQLTypeUtil.java @@ -196,12 +196,7 @@ public static T unwrapOneAs(GraphQLType type) { * @return the underlying type */ public static GraphQLUnmodifiedType unwrapAll(GraphQLType type) { - while (true) { - if (isNotWrapped(type)) { - return (GraphQLUnmodifiedType) type; - } - type = unwrapOne(type); - } + return unwrapAllAs(type); } /** @@ -215,7 +210,16 @@ public static GraphQLUnmodifiedType unwrapAll(GraphQLType type) { */ public static T unwrapAllAs(GraphQLType type) { //noinspection unchecked - return (T) unwrapAll(type); + return (T) unwrapAllImpl(type); + } + + private static GraphQLType unwrapAllImpl(GraphQLType type) { + while (true) { + if (isNotWrapped(type)) { + return type; + } + type = unwrapOne(type); + } } diff --git a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java index 4ac7333fe3..755dd2edc0 100644 --- a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java +++ b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java @@ -22,12 +22,16 @@ import java.util.Map; import java.util.TreeMap; +import static graphql.schema.GraphQLTypeUtil.isNotWrapped; +import static graphql.schema.GraphQLTypeUtil.unwrapAllAs; +import static graphql.schema.GraphQLTypeUtil.unwrapOne; import static java.lang.String.format; @Internal public class GraphQLTypeCollectingVisitor extends GraphQLTypeVisitorStub { private final Map result = new LinkedHashMap<>(); + private final Map fieldActualTypes = new LinkedHashMap<>(); public GraphQLTypeCollectingVisitor() { } @@ -86,10 +90,14 @@ public TraversalControl visitGraphQLUnionType(GraphQLUnionType node, TraverserCo @Override public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { - + GraphQLNamedType type = unwrapAllAs(node.getType()); + if (! (type instanceof GraphQLTypeReference)) { + fieldActualTypes.put(type.getName(), type); + } return super.visitGraphQLFieldDefinition(node, context); } + private boolean isNotTypeReference(String name) { return result.containsKey(name) && !(result.get(name) instanceof GraphQLTypeReference); } @@ -113,17 +121,38 @@ private void assertTypeUniqueness(GraphQLNamedType type, Map getResult() { - return ImmutableMap.copyOf(new TreeMap<>(result)); + Map types = new TreeMap<>(fixFieldDanglingTypes(result)); + return ImmutableMap.copyOf(types); + } + + /** + * It's possible for certain field edits to create a situation where a field had a type reference, then + * it got replaced with an actual reference and then the schema gets edited such that the only reference + * to that type is the replaced field reference. This edge case means that the replaced reference can be + * missed if it's the only way to get to that type. + * + * @param visitedTypes the types collected by this visitor + * + * @return a fixed up map where the only + */ + private Map fixFieldDanglingTypes(Map visitedTypes) { + for (GraphQLNamedType fieldPointerType : fieldActualTypes.values()) { + String typeName = fieldPointerType.getName(); + visitedTypes.putIfAbsent(typeName, fieldPointerType); + } + return visitedTypes; } } diff --git a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy index a9c19f3a06..1aa192a901 100644 --- a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy @@ -21,6 +21,7 @@ import static graphql.StarWarsSchema.humanType import static graphql.StarWarsSchema.starWarsSchema import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition import static graphql.schema.GraphQLObjectType.newObject +import static graphql.schema.GraphQLTypeReference.typeRef class GraphQLSchemaTest extends Specification { @@ -305,6 +306,35 @@ class GraphQLSchemaTest extends Specification { transformed.containsType("B") } + def "fields edited from type references should still built valid schemas"() { + def typeB = newObject().name("B") + .field(newFieldDefinition().name("a").type(typeRef("A"))) + .field(newFieldDefinition().name("b").type(typeRef("B"))) + .build() + + + def typeAFieldB = newFieldDefinition().name("b").type(typeRef("B")).build() + // at the line above typeB is never strongly referenced + // and this simulates an edit situation that wont happen with direct java declaration + // but where the type reference is replaced to an actual but its the ONLY direct reference + // to that type`` + typeAFieldB.replaceType(typeB) + + def typeA = newObject().name("A") + .field(typeAFieldB) + .build() + + def queryType = newObject().name("Query") + .field(newFieldDefinition().name("a").type(typeA)) + .build() + + when: + def schema = GraphQLSchema.newSchema().query(queryType).build() + then: + schema.getType("A") != null + schema.getType("B") != null + } + def "cheap transform without types transformation works"() { def sdl = ''' @@ -415,4 +445,5 @@ class GraphQLSchemaTest extends Specification { thrown(AssertException) } + } diff --git a/src/test/groovy/graphql/schema/GraphQLTypeUtilTest.groovy b/src/test/groovy/graphql/schema/GraphQLTypeUtilTest.groovy index 416f2107d7..39763a0b0d 100644 --- a/src/test/groovy/graphql/schema/GraphQLTypeUtilTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLTypeUtilTest.groovy @@ -6,6 +6,7 @@ import static graphql.Scalars.GraphQLString import static graphql.schema.GraphQLList.list import static graphql.schema.GraphQLNonNull.nonNull import static graphql.schema.GraphQLObjectType.newObject +import static graphql.schema.GraphQLTypeReference.* class GraphQLTypeUtilTest extends Specification { @@ -127,6 +128,38 @@ class GraphQLTypeUtilTest extends Specification { } + def "unwrapAllAs tests"() { + def type = list(nonNull(list(nonNull(GraphQLString)))) + def typeRef = list(nonNull(list(nonNull(typeRef("A"))))) + + when: + type = GraphQLTypeUtil.unwrapAllAs(type) + + then: + type == GraphQLString + + when: + type = GraphQLTypeUtil.unwrapAllAs(type) + + then: + type == GraphQLString + + when: + typeRef = GraphQLTypeUtil.unwrapAllAs(typeRef) + + then: + typeRef instanceof GraphQLTypeReference + (typeRef as GraphQLTypeReference).name == "A" + + when: + typeRef = GraphQLTypeUtil.unwrapAllAs(typeRef) + + then: + typeRef instanceof GraphQLTypeReference + (typeRef as GraphQLTypeReference).name == "A" + + } + def "isLeaf tests"() { when: def type = GraphQLString From c5dc1ca69580c5080699d36862ab18bbdb0c9c93 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 2 Aug 2022 12:56:11 +1000 Subject: [PATCH 4/7] Extra test to ensure that field referenced types are still ok --- .../impl/GraphQLTypeCollectingVisitor.java | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java index 755dd2edc0..f0d1451d79 100644 --- a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java +++ b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java @@ -22,9 +22,7 @@ import java.util.Map; import java.util.TreeMap; -import static graphql.schema.GraphQLTypeUtil.isNotWrapped; import static graphql.schema.GraphQLTypeUtil.unwrapAllAs; -import static graphql.schema.GraphQLTypeUtil.unwrapOne; import static java.lang.String.format; @Internal @@ -91,7 +89,7 @@ public TraversalControl visitGraphQLUnionType(GraphQLUnionType node, TraverserCo @Override public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { GraphQLNamedType type = unwrapAllAs(node.getType()); - if (! (type instanceof GraphQLTypeReference)) { + if (!(type instanceof GraphQLTypeReference)) { fieldActualTypes.put(type.getName(), type); } return super.visitGraphQLFieldDefinition(node, context); @@ -120,19 +118,22 @@ private void assertTypeUniqueness(GraphQLNamedType type, Map getResult() { Map types = new TreeMap<>(fixFieldDanglingTypes(result)); return ImmutableMap.copyOf(types); @@ -151,7 +152,13 @@ public ImmutableMap getResult() { private Map fixFieldDanglingTypes(Map visitedTypes) { for (GraphQLNamedType fieldPointerType : fieldActualTypes.values()) { String typeName = fieldPointerType.getName(); - visitedTypes.putIfAbsent(typeName, fieldPointerType); + GraphQLNamedType visitedType = visitedTypes.get(typeName); + if (visitedType == null) { + visitedTypes.put(typeName, fieldPointerType); + } else { + // if we have this type (by name) already then it better be the same object + assertUniqueTypeObjects(visitedType, fieldPointerType); + } } return visitedTypes; } From f58324c828b516058d83fccf5de810169f3fd1bd Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 2 Aug 2022 15:58:17 +1000 Subject: [PATCH 5/7] Got carried away and got too precise. A changed type should not be referenced --- .../graphql/schema/impl/GraphQLTypeCollectingVisitor.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java index f0d1451d79..bd6c6982bd 100644 --- a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java +++ b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java @@ -152,13 +152,7 @@ public ImmutableMap getResult() { private Map fixFieldDanglingTypes(Map visitedTypes) { for (GraphQLNamedType fieldPointerType : fieldActualTypes.values()) { String typeName = fieldPointerType.getName(); - GraphQLNamedType visitedType = visitedTypes.get(typeName); - if (visitedType == null) { - visitedTypes.put(typeName, fieldPointerType); - } else { - // if we have this type (by name) already then it better be the same object - assertUniqueTypeObjects(visitedType, fieldPointerType); - } + visitedTypes.putIfAbsent(typeName, fieldPointerType); } return visitedTypes; } From 31dd5a32ef732c5f283f44496ccf4d28dfaace64 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 2 Aug 2022 19:35:50 +1000 Subject: [PATCH 6/7] Updated to include other types that have replaceable types --- .../impl/GraphQLTypeCollectingVisitor.java | 81 ++++++++++++++----- .../graphql/schema/GraphQLSchemaTest.groovy | 53 +++++++++--- 2 files changed, 103 insertions(+), 31 deletions(-) diff --git a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java index bd6c6982bd..d68f4931bf 100644 --- a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java +++ b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java @@ -3,11 +3,16 @@ import com.google.common.collect.ImmutableMap; import graphql.AssertException; import graphql.Internal; +import graphql.schema.GraphQLAppliedDirectiveArgument; +import graphql.schema.GraphQLArgument; import graphql.schema.GraphQLEnumType; import graphql.schema.GraphQLFieldDefinition; +import graphql.schema.GraphQLInputObjectField; import graphql.schema.GraphQLInputObjectType; import graphql.schema.GraphQLInterfaceType; +import graphql.schema.GraphQLList; import graphql.schema.GraphQLNamedType; +import graphql.schema.GraphQLNonNull; import graphql.schema.GraphQLObjectType; import graphql.schema.GraphQLScalarType; import graphql.schema.GraphQLSchemaElement; @@ -21,15 +26,17 @@ import java.util.LinkedHashMap; import java.util.Map; import java.util.TreeMap; +import java.util.function.Supplier; import static graphql.schema.GraphQLTypeUtil.unwrapAllAs; +import static graphql.util.TraversalControl.CONTINUE; import static java.lang.String.format; @Internal public class GraphQLTypeCollectingVisitor extends GraphQLTypeVisitorStub { private final Map result = new LinkedHashMap<>(); - private final Map fieldActualTypes = new LinkedHashMap<>(); + private final Map indirectStrongReferences = new LinkedHashMap<>(); public GraphQLTypeCollectingVisitor() { } @@ -38,14 +45,14 @@ public GraphQLTypeCollectingVisitor() { public TraversalControl visitGraphQLEnumType(GraphQLEnumType node, TraverserContext context) { assertTypeUniqueness(node, result); save(node.getName(), node); - return super.visitGraphQLEnumType(node, context); + return CONTINUE; } @Override public TraversalControl visitGraphQLScalarType(GraphQLScalarType node, TraverserContext context) { assertTypeUniqueness(node, result); save(node.getName(), node); - return super.visitGraphQLScalarType(node, context); + return CONTINUE; } @Override @@ -55,7 +62,7 @@ public TraversalControl visitGraphQLObjectType(GraphQLObjectType node, Traverser } else { save(node.getName(), node); } - return super.visitGraphQLObjectType(node, context); + return CONTINUE; } @Override @@ -65,7 +72,7 @@ public TraversalControl visitGraphQLInputObjectType(GraphQLInputObjectType node, } else { save(node.getName(), node); } - return super.visitGraphQLInputObjectType(node, context); + return CONTINUE; } @Override @@ -75,24 +82,57 @@ public TraversalControl visitGraphQLInterfaceType(GraphQLInterfaceType node, Tra } else { save(node.getName(), node); } - - return super.visitGraphQLInterfaceType(node, context); + return CONTINUE; } @Override public TraversalControl visitGraphQLUnionType(GraphQLUnionType node, TraverserContext context) { assertTypeUniqueness(node, result); save(node.getName(), node); - return super.visitGraphQLUnionType(node, context); + return CONTINUE; } @Override public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { - GraphQLNamedType type = unwrapAllAs(node.getType()); + saveIndirectStrongReference(node::getType); + return CONTINUE; + } + + @Override + public TraversalControl visitGraphQLInputObjectField(GraphQLInputObjectField node, TraverserContext context) { + saveIndirectStrongReference(node::getType); + return CONTINUE; + } + + @Override + public TraversalControl visitGraphQLArgument(GraphQLArgument node, TraverserContext context) { + saveIndirectStrongReference(node::getType); + return CONTINUE; + } + + @Override + public TraversalControl visitGraphQLAppliedDirectiveArgument(GraphQLAppliedDirectiveArgument node, TraverserContext context) { + saveIndirectStrongReference(node::getType); + return CONTINUE; + } + + @Override + public TraversalControl visitGraphQLList(GraphQLList node, TraverserContext context) { + saveIndirectStrongReference(node::getWrappedType); + return CONTINUE; + } + + @Override + public TraversalControl visitGraphQLNonNull(GraphQLNonNull node, TraverserContext context) { + saveIndirectStrongReference(node::getWrappedType); + return CONTINUE; + } + + private void saveIndirectStrongReference(Supplier typeSupplier) { + GraphQLNamedType type = unwrapAllAs(typeSupplier.get()); if (!(type instanceof GraphQLTypeReference)) { - fieldActualTypes.put(type.getName(), type); + indirectStrongReferences.put(type.getName(), type); } - return super.visitGraphQLFieldDefinition(node, context); } @@ -135,24 +175,25 @@ private void assertUniqueTypeObjects(GraphQLNamedType type, GraphQLType existing } public ImmutableMap getResult() { - Map types = new TreeMap<>(fixFieldDanglingTypes(result)); + Map types = new TreeMap<>(fixDanglingReplacedTypes(result)); return ImmutableMap.copyOf(types); } /** - * It's possible for certain field edits to create a situation where a field had a type reference, then - * it got replaced with an actual reference and then the schema gets edited such that the only reference - * to that type is the replaced field reference. This edge case means that the replaced reference can be - * missed if it's the only way to get to that type. + * 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. * * @param visitedTypes the types collected by this visitor * * @return a fixed up map where the only */ - private Map fixFieldDanglingTypes(Map visitedTypes) { - for (GraphQLNamedType fieldPointerType : fieldActualTypes.values()) { - String typeName = fieldPointerType.getName(); - visitedTypes.putIfAbsent(typeName, fieldPointerType); + private Map fixDanglingReplacedTypes(Map visitedTypes) { + for (GraphQLNamedType indirectStrongReference : indirectStrongReferences.values()) { + String typeName = indirectStrongReference.getName(); + visitedTypes.putIfAbsent(typeName, indirectStrongReference); } return visitedTypes; } diff --git a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy index 1aa192a901..11d7383de9 100644 --- a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy @@ -12,16 +12,19 @@ import graphql.util.TraverserContext import spock.lang.Specification import java.util.function.UnaryOperator -import java.util.stream.Collectors import static graphql.Scalars.GraphQLString import static graphql.StarWarsSchema.characterInterface import static graphql.StarWarsSchema.droidType import static graphql.StarWarsSchema.humanType import static graphql.StarWarsSchema.starWarsSchema +import static graphql.schema.GraphQLArgument.newArgument import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition +import static graphql.schema.GraphQLInputObjectField.newInputObjectField +import static graphql.schema.GraphQLInputObjectType.newInputObject import static graphql.schema.GraphQLObjectType.newObject import static graphql.schema.GraphQLTypeReference.typeRef +import static java.util.stream.Collectors.toList class GraphQLSchemaTest extends Specification { @@ -239,10 +242,10 @@ class GraphQLSchemaTest extends Specification { when: def schema = TestUtil.schema(sdl) - // When field `a` is filtered out + // When field `b` is filtered out List fields = schema.queryType.fieldDefinitions.stream().filter({ it.name == "a" - }).collect(Collectors.toList()) + }).collect(toList()) // And we transform the schema's query root, the schema building // will throw because type B won't be in the type map anymore, since // there are no more actual B object types in the schema tree. @@ -260,10 +263,7 @@ class GraphQLSchemaTest extends Specification { def "can change types via SchemaTransformer and visitor"() { def sdl = ''' type Query { - # The b fields leads to the addition of the B type (actual definition) b: B - # When we filter out the `b` field, we can still access B through A - # however they are GraphQLTypeReferences and not an actual GraphQL Object a: A } @@ -283,15 +283,13 @@ class GraphQLSchemaTest extends Specification { GraphQLTypeVisitorStub visitor = new GraphQLTypeVisitorStub() { @Override TraversalControl visitGraphQLObjectType(GraphQLObjectType objectType, TraverserContext context) { - GraphQLCodeRegistry.Builder codeRegistry = context.getVarFromParents(GraphQLCodeRegistry.Builder.class); - // we need to change __XXX introspection types to have directive extensions - if (objectType.getName().equals("Query")) { + if (objectType.getName() == "Query") { def queryType = objectType List fields = queryType.fieldDefinitions.stream().filter({ it.name == "a" - }).collect(Collectors.toList()) + }).collect(toList()) - GraphQLObjectType newObjectType = queryType.transform({ + GraphQLObjectType newObjectType = queryType.transform({ it.replaceFields(fields) }) @@ -324,8 +322,38 @@ class GraphQLSchemaTest extends Specification { .field(typeAFieldB) .build() + // + // the same pattern above applies ot other replaceable types like arguments and input fields + def inputTypeY = newInputObject().name("InputTypeY") + .field(newInputObjectField().name("in2").type(GraphQLString)) + .build() + + def inputFieldOfY = newInputObjectField().name("inY").type(typeRef("InputTypeY")).build() + // only reference to InputTypeY + inputFieldOfY.replaceType(inputTypeY) + + def inputTypeZ = newInputObject().name("InputTypeZ") + .field(inputFieldOfY) + .build() + + def inputTypeX = newInputObject().name("InputTypeX") + .field(newInputObjectField().name("inX").type(GraphQLString)) + .build() + + GraphQLArgument argOfX = newArgument().name("argOfX").type(typeRef("InputTypeX")).build() + // only reference to InputTypeX + argOfX.replaceType(inputTypeX) + + GraphQLArgument argOfZ = newArgument().name("argOfZ").type(inputTypeZ).build() + + def typeC = newObject().name("C") + .field(newFieldDefinition().name("f1").type(GraphQLString).argument(argOfX)) + .field(newFieldDefinition().name("f2").type(GraphQLString).argument(argOfZ)) + .build() + def queryType = newObject().name("Query") .field(newFieldDefinition().name("a").type(typeA)) + .field(newFieldDefinition().name("c").type(typeC)) .build() when: @@ -333,6 +361,9 @@ class GraphQLSchemaTest extends Specification { then: schema.getType("A") != null schema.getType("B") != null + schema.getType("InputTypeX") != null + schema.getType("InputTypeY") != null + schema.getType("InputTypeZ") != null } def "cheap transform without types transformation works"() { From 355aab1a48a81d77310bca98a8ae747d5b7197a6 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Wed, 3 Aug 2022 20:43:51 +1000 Subject: [PATCH 7/7] Added non null and list wrapper tests --- .../impl/GraphQLTypeCollectingVisitor.java | 12 ------ .../graphql/schema/GraphQLSchemaTest.groovy | 40 ++++++++++++++++++- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java index d68f4931bf..7da9ab1f88 100644 --- a/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java +++ b/src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java @@ -116,18 +116,6 @@ public TraversalControl visitGraphQLAppliedDirectiveArgument(GraphQLAppliedDirec return CONTINUE; } - @Override - public TraversalControl visitGraphQLList(GraphQLList node, TraverserContext context) { - saveIndirectStrongReference(node::getWrappedType); - return CONTINUE; - } - - @Override - public TraversalControl visitGraphQLNonNull(GraphQLNonNull node, TraverserContext context) { - saveIndirectStrongReference(node::getWrappedType); - return CONTINUE; - } - private void saveIndirectStrongReference(Supplier typeSupplier) { GraphQLNamedType type = unwrapAllAs(typeSupplier.get()); if (!(type instanceof GraphQLTypeReference)) { diff --git a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy index 11d7383de9..d112f40fc8 100644 --- a/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy @@ -22,6 +22,7 @@ import static graphql.schema.GraphQLArgument.newArgument import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition import static graphql.schema.GraphQLInputObjectField.newInputObjectField import static graphql.schema.GraphQLInputObjectType.newInputObject +import static graphql.schema.GraphQLNonNull.nonNull import static graphql.schema.GraphQLObjectType.newObject import static graphql.schema.GraphQLTypeReference.typeRef import static java.util.stream.Collectors.toList @@ -298,10 +299,10 @@ class GraphQLSchemaTest extends Specification { return TraversalControl.CONTINUE } } - GraphQLSchema transformed = SchemaTransformer.transformSchema(schema, visitor) + GraphQLSchema transformedSchema = SchemaTransformer.transformSchema(schema, visitor) then: - transformed.containsType("B") + transformedSchema.containsType("B") } def "fields edited from type references should still built valid schemas"() { @@ -366,6 +367,41 @@ class GraphQLSchemaTest extends Specification { schema.getType("InputTypeZ") != null } + def "list and non nulls work when direct references are edited"() { + + def typeX = newObject().name("TypeX") + .field(newFieldDefinition().name("f1").type(GraphQLString)) + .build() + def queryType = newObject().name("Query") + .field(newFieldDefinition().name("direct").type(typeX)) + .field(newFieldDefinition().name("indirectNonNull").type(nonNull(typeRef("TypeX")))) + .field(newFieldDefinition().name("indirectList").type(GraphQLList.list(nonNull(typeRef("TypeX"))))) + .build() + + when: + def schema = GraphQLSchema.newSchema().query(queryType).build() + then: + schema.getType("TypeX") != null + + // now edit away the actual strong reference + when: + GraphQLTypeVisitor visitor = new GraphQLTypeVisitorStub() { + + @Override + TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext context) { + if (node.getName() == "direct") { + return deleteNode(context) + } + return TraversalControl.CONTINUE + } + } + + GraphQLSchema transformedSchema = SchemaTransformer.transformSchema(schema, visitor) + + then: + transformedSchema.getType("TypeX") != null + } + def "cheap transform without types transformation works"() { def sdl = '''