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..7da9ab1f88 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,13 +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 indirectStrongReferences = new LinkedHashMap<>(); public GraphQLTypeCollectingVisitor() { } @@ -36,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 @@ -53,7 +62,7 @@ public TraversalControl visitGraphQLObjectType(GraphQLObjectType node, Traverser } else { save(node.getName(), node); } - return super.visitGraphQLObjectType(node, context); + return CONTINUE; } @Override @@ -63,7 +72,7 @@ public TraversalControl visitGraphQLInputObjectType(GraphQLInputObjectType node, } else { save(node.getName(), node); } - return super.visitGraphQLInputObjectType(node, context); + return CONTINUE; } @Override @@ -73,23 +82,48 @@ 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) { + saveIndirectStrongReference(node::getType); + return CONTINUE; + } + + @Override + public TraversalControl visitGraphQLInputObjectField(GraphQLInputObjectField node, TraverserContext context) { + saveIndirectStrongReference(node::getType); + return CONTINUE; + } - return super.visitGraphQLFieldDefinition(node, context); + @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; + } + + private void saveIndirectStrongReference(Supplier typeSupplier) { + GraphQLNamedType type = unwrapAllAs(typeSupplier.get()); + if (!(type instanceof GraphQLTypeReference)) { + indirectStrongReferences.put(type.getName(), type); + } + } + + private boolean isNotTypeReference(String name) { return result.containsKey(name) && !(result.get(name) instanceof GraphQLTypeReference); } @@ -112,18 +146,43 @@ private void assertTypeUniqueness(GraphQLNamedType type, Map getResult() { - return ImmutableMap.copyOf(new TreeMap<>(result)); + Map types = new TreeMap<>(fixDanglingReplacedTypes(result)); + return ImmutableMap.copyOf(types); + } + + /** + * 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 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 40a0a3fd1f..d112f40fc8 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 @@ -16,8 +18,14 @@ 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.GraphQLNonNull.nonNull import static graphql.schema.GraphQLObjectType.newObject +import static graphql.schema.GraphQLTypeReference.typeRef +import static java.util.stream.Collectors.toList class GraphQLSchemaTest extends Specification { @@ -187,7 +195,6 @@ class GraphQLSchemaTest extends Specification { type Dog implements Pet { name : String } - type Cat implements Pet { name : String } @@ -214,6 +221,187 @@ 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 `b` is filtered out + List fields = schema.queryType.fieldDefinitions.stream().filter({ + it.name == "a" + }).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. + def transformed = schema.transform({ + it.query(schema.queryType.transform({ + it.replaceFields(fields) + })) + }) + + then: + transformed.containsType("B") + + } + + def "can change types via SchemaTransformer and visitor"() { + def sdl = ''' + type Query { + b: B + 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) { + if (objectType.getName() == "Query") { + def queryType = objectType + List fields = queryType.fieldDefinitions.stream().filter({ + it.name == "a" + }).collect(toList()) + + GraphQLObjectType newObjectType = queryType.transform({ + it.replaceFields(fields) + }) + + return changeNode(context, newObjectType); + } + return TraversalControl.CONTINUE + } + } + GraphQLSchema transformedSchema = SchemaTransformer.transformSchema(schema, visitor) + + then: + transformedSchema.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() + + // + // 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: + def schema = GraphQLSchema.newSchema().query(queryType).build() + then: + schema.getType("A") != null + schema.getType("B") != null + schema.getType("InputTypeX") != null + schema.getType("InputTypeY") != null + 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 = ''' @@ -324,4 +512,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