Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transforms #2906

bbakerman · 2022-07-31T10:54:32Z

This is the same code as #2905 but with an extra test for SchemaTransformer which also fails

bbakerman · 2022-08-01T23:45:48Z

I debugged this further. The reason this blows up (even for SchemaTransformer changes along with direct ones) is this

The first step of the schema build code calls this

            SchemaUtil.visitPartiallySchema(partiallyBuiltSchema, codeRegistryVisitor, typeCollectingVisitor);
            ImmutableMap<String, GraphQLNamedType> allTypes = typeCollectingVisitor.getResult();

At this point the map is meant to contain a map of name -> ACTUAL types (and not type references)

Later the type reference replacement code finds all type references and expects to find the actual type for them in the map above and its does not and asserts.

However this particular edit means that the schema DOES have a field pointer to a actual type but the GraphQLTypeCollectingVisitor never grabs it because it only visits types via direct declaration and not via field declaration.

This makes sense in SDL generation because its impossible to create a situation where the actual type is not declared but code declaration can make this happen as demonstrated in this PR test case.

bbakerman · 2022-08-02T01:17:42Z

Ok debugging more - this is more about the Traverser related code

In this "edit case" we have a field b that at some point earlier had a type reference to B in the field def. On the original schema build this gets resolved to an actual type. So now the field reference has originalType=typeRef("B") and also replacedType=actualTypeB

Then the code does and edit and this gets rid of the direct reference to type B (which came via another field chain) but this field b still has originalType=typeRef("B") andreplacedType=actualTypeB.

So Type B is still around in some form

The graphql.schema.impl.GraphQLTypeCollectingVisitor#GraphQLTypeCollectingVisitor setup code does this

        traverser = new SchemaTraverser(schemaElement -> schemaElement.getChildrenWithTypeReferences().getChildrenAsList());

and the callback to the GraphqlFieldDefinition is getChildrenWithTypeReferences which is code as

        return SchemaElementChildrenContainer.newSchemaElementChildrenContainer()
                .child(CHILD_TYPE, originalType)
                .children(CHILD_ARGUMENTS, arguments)
                .children(CHILD_DIRECTIVES, directivesHolder.getDirectives())
                .children(CHILD_APPLIED_DIRECTIVES, directivesHolder.getAppliedDirectives())
                .build();

So while the replace type is in place - its never used.

The question is - should it be?

bbakerman · 2022-08-02T01:30:49Z

The code rightly wants a DAG - if we change it, it starts to fail with

Internal error: should never happen: This schema is not forming an Directed Acyclic Graph

The challenge here is this. The edit caused the field to be the only reference to the type B BUT the traverser in the schema build uses getChildrenWithTypeReferences and this missed this type.

It's as if the specific use case GraphQLTypeCollectingVisitor needs to grab dangling field references and do a fix up if they are missing - eg type referenced but not in the schema in some other way

…hema rebuild

bbakerman · 2022-08-02T02:47:55Z

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

-            }
-            type = unwrapOne(type);
-        }
+        return unwrapAllAs(type);


This was probably a mistake to have the well named unwrapAll return GraphQLUnmodifiedType which excludes GraphqlTypeReferences - but API is API so leave it as is

I did change how it works internally here

bbakerman · 2022-08-02T02:48:12Z

src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java

+        GraphQLNamedType type = unwrapAllAs(node.getType());
+        if (! (type instanceof GraphQLTypeReference)) {
+            fieldActualTypes.put(type.getName(), type);
+        }


It only puts in fields that unwrap to be an actual type

bbakerman · 2022-08-02T02:49:27Z

src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java

+    private Map<String, GraphQLNamedType> fixFieldDanglingTypes(Map<String, GraphQLNamedType> visitedTypes) {
+        for (GraphQLNamedType fieldPointerType : fieldActualTypes.values()) {
+            String typeName = fieldPointerType.getName();
+            visitedTypes.putIfAbsent(typeName, fieldPointerType);


if we have a type pointed to by a field AND it's not the the list of visited types, we add it since the field is the only pointer to it say

bbakerman · 2022-08-02T02:58:36Z

src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java

+            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()));


made a common method here

bbakerman · 2022-08-02T02:59:23Z

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

+
+        then:
+        transformed.containsType("B")
+


Andres original test that was failing

bbakerman · 2022-08-02T02:59:45Z

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

+        GraphQLSchema transformed = SchemaTransformer.transformSchema(schema, visitor)
+
+        then:
+        transformed.containsType("B")


My SchemaTransformer test that was failing and now passing

bbakerman · 2022-08-02T03:00:07Z

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

+        then:
+        schema.getType("A") != null
+        schema.getType("B") != null
+    }


Direct object test case (no SDL) that was failing and is now passing

bbakerman · 2022-08-02T03:00:30Z

src/test/groovy/graphql/schema/GraphQLTypeUtilTest.groovy

+        then:
+        typeRef instanceof GraphQLTypeReference
+        (typeRef as GraphQLTypeReference).name == "A"
+


Extra tests as unwrapAllAs was not previously covered

andimarek · 2022-08-02T04:18:51Z

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

+
+        when:
+        def schema = TestUtil.schema(sdl)
+        // When field `a` is filtered out


b is filtered out

…ferenced

bbakerman · 2022-08-02T09:40:23Z

src/main/java/graphql/schema/impl/GraphQLTypeCollectingVisitor.java

    }

    @Override
    public TraversalControl visitGraphQLScalarType(GraphQLScalarType node, TraverserContext<GraphQLSchemaElement> context) {
        assertTypeUniqueness(node, result);
        save(node.getName(), node);
-        return super.visitGraphQLScalarType(node, context);
+        return CONTINUE;


all of this super code boiled down to CONTINUE!

xuorig

Appreciate the fix! Very similar solution to the workaround I implemented but much much nicer now that it is built in in the type collector. Thanks 🙇

Marc-Andre Giroux and others added 2 commits July 29, 2022 17:34

add a test to reproduce GraphQLTypeReference issue in transforms

9cd28ff

Added extra test for schema transform

9ccbe94

bbakerman mentioned this pull request Jul 31, 2022

Edge case with GraphQLTypeReference and Schema Transforms #2905

Closed

Fixes the bug where dangling actual references are not used during sc…

3de7dc4

…hema rebuild

bbakerman commented Aug 2, 2022

View reviewed changes

Extra test to ensure that field referenced types are still ok

c5dc1ca

bbakerman commented Aug 2, 2022

View reviewed changes

bbakerman changed the title ~~Xuorig repro PR - Edge case with GraphQLTypeReference and Schema Transforms~~ Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transforms Aug 2, 2022

andimarek reviewed Aug 2, 2022

View reviewed changes

bbakerman added 2 commits August 2, 2022 15:58

Got carried away and got too precise. A changed type should not be re…

f58324c

…ferenced

Updated to include other types that have replaceable types

31dd5a3

bbakerman added this to the 19.1 milestone Aug 2, 2022

bbakerman commented Aug 2, 2022

View reviewed changes

xuorig reviewed Aug 2, 2022

View reviewed changes

Added non null and list wrapper tests

355aab1

bbakerman merged commit fb507ad into master Aug 11, 2022

yeikel mentioned this pull request Aug 12, 2022

Bump com.graphql-java:graphql-java from 19.0 to 19.1 vert-x3/vertx-web#2252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transforms #2906

Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transforms #2906

bbakerman commented Jul 31, 2022

bbakerman commented Aug 1, 2022

bbakerman commented Aug 2, 2022

bbakerman commented Aug 2, 2022

bbakerman Aug 2, 2022

bbakerman Aug 2, 2022

bbakerman Aug 2, 2022

bbakerman Aug 2, 2022

bbakerman Aug 2, 2022

bbakerman Aug 2, 2022

bbakerman Aug 2, 2022

bbakerman Aug 2, 2022

andimarek Aug 2, 2022

bbakerman Aug 2, 2022

bbakerman Aug 2, 2022

xuorig left a comment

Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transforms #2906

Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transforms #2906

Conversation

bbakerman commented Jul 31, 2022

bbakerman commented Aug 1, 2022

bbakerman commented Aug 2, 2022

bbakerman commented Aug 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuorig left a comment

Choose a reason for hiding this comment