Skip to content

Edge case with GraphQLTypeReference and Schema Transforms #2905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xuorig wants to merge 1 commit into from
Closed

Edge case with GraphQLTypeReference and Schema Transforms #2905

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion src/test/groovy/graphql/schema/GraphQLSchemaTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -187,7 +188,6 @@ class GraphQLSchemaTest extends Specification {
type Dog implements Pet {
name : String
}

type Cat implements Pet {
name : String
}
Expand All @@ -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
}
Copy link
Member

@bbakerman bbakerman Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments - you have a transient Query.a -> type A -> A.a -> type A - while the schema is being built type references are indeed used HOWEVER once the schema is fully built (a late step) then all type references are removed and replaced with actual object pointers. This means that the runtime types are in fact NOT Immutable but are mutable by design.

So you mentioned GraphQLTypeReferences but these would not exist by the time def schema = TestUtil.schema(sdl) returned

Copy link
Author

@xuorig xuorig Jul 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the problem is that during build and buildImpl the schema is partially built at first, and the transformed roots do indeed contain GraphQLTypeReferences.

I just saw you confirmed the bug with transformSchema as well, I should have mentioned this hits the same problem as well yeah 👍


type A {
b: B
}

type B {
a: A
b: B
}
'''

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

@andimarek andimarek Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b 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")
Copy link
Member

@bbakerman bbakerman Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect this to pass since the schema pointers are Query.a -> type A -> A.b -> type B. So this really feels like a bug. Thanks for reporting

We will need to do some more investigation on why

Copy link
Member

@bbakerman bbakerman Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started to look at this and I can indeed see it blowing up with something to do with GraphQLTypeReferences which is interesting

However it might be that this form of "schema editing" is not the right way. I need to look further.

GraphQLSchema newSchema = SchemaTransformer.transformSchema(schema, visitor); is the best way to change a schema - it handles all of the invariants etc... versus directly editing the runtime graphql types.

For example it will fix up references and so on correctly.

As part of debugging this, I will write another test that shows how to do what you want (in filtering certain fields)

See https://www.graphql-java.com/documentation/schema and the section Changing Schema

Copy link
Member

@bbakerman bbakerman Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this also fails - wow this is a real bug

See #2906 which is this PR plus another test


}

def "cheap transform without types transformation works"() {

def sdl = '''
Expand Down