Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transforms

src/main/java/graphql/schema/GraphQLTypeUtil.java

@@ -196,12 +196,7 @@ public static <T extends GraphQLType> 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 extends GraphQLType> 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);
+        }
     }
 
 

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<String, GraphQLNamedType> result = new LinkedHashMap<>();
+    private final Map<String, GraphQLNamedType> fieldActualTypes = new LinkedHashMap<>();
 
     public GraphQLTypeCollectingVisitor() {
     }
@@ -86,10 +90,14 @@ public TraversalControl visitGraphQLUnionType(GraphQLUnionType node, TraverserCo
 
     @Override
     public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition node, TraverserContext<GraphQLSchemaElement> 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<String, GraphQLName
         if (existingType != null) {
             // type references are ok
             if (!(existingType instanceof GraphQLTypeReference || type instanceof GraphQLTypeReference))
-                // object comparison here is deliberate
+            // object comparison here is deliberate
+            {
                 if (existingType != type) {
                     throw new AssertException(format("All types within a GraphQL schema must have unique names. No two provided types may have the same name.\n" +
                                     "No provided type may have a name which conflicts with any built in types (including Scalar and Introspection types).\n" +
                                     "You have redefined the type '%s' from being a '%s' to a '%s'",
                             type.getName(), existingType.getClass().getSimpleName(), type.getClass().getSimpleName()));
                 }
+            }
         }
     }
 
     public ImmutableMap<String, GraphQLNamedType> getResult() {
-        return ImmutableMap.copyOf(new TreeMap<>(result));
+        Map<String, GraphQLNamedType> 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<String, GraphQLNamedType> fixFieldDanglingTypes(Map<String, GraphQLNamedType> visitedTypes) {
+        for (GraphQLNamedType fieldPointerType : fieldActualTypes.values()) {
+            String typeName = fieldPointerType.getName();
+            visitedTypes.putIfAbsent(typeName, fieldPointerType);
+        }
+        return visitedTypes;
     }
 }

src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy

@@ -7,9 +7,12 @@ 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
+import java.util.stream.Collectors
 
 import static graphql.Scalars.GraphQLString
 import static graphql.StarWarsSchema.characterInterface
@@ -18,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 {
 
@@ -187,7 +191,6 @@ class GraphQLSchemaTest extends Specification {
         type Dog implements Pet {
             name : String
         }
-
         type Cat implements Pet {
             name : String
         }
@@ -214,6 +217,124 @@ 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<GraphQLFieldDefinition> 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 "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<GraphQLSchemaElement> 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<GraphQLFieldDefinition> 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 "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 = '''
@@ -324,4 +445,5 @@ class GraphQLSchemaTest extends Specification {
         thrown(AssertException)
 
     }
+
 }

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