Edge case with GraphQLTypeReference and Schema Transforms #2905
Conversation
I havent look at your repro yet so I wont say its a bug quite yet depending on how you are doing the delete. But in general we should handle this |
| # 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 | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 👍
| }) | ||
|
|
||
| then: | ||
| transformed.containsType("B") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
And this also fails - wow this is a real bug
See #2906 which is this PR plus another test
|
|
||
| when: | ||
| def schema = TestUtil.schema(sdl) | ||
| // When field `a` is filtered out |
|
see #2906 for the fix and most of this repro |
3 participants
👋 Hello
I've hit a pretty interesting edge case with
GraphQLSchema#transform. We use it to do some schema filtering and noticed a failure when combined with recursive types using GraphQLTypeReference.When a part of the schema holding the "true" reference to a GraphQL object type is removed, if there are
GraphQLTypeReferences remaining in the rest of the schema, they won't be able to be resolved.The original type won't be in the type map anymore because the new type map is regenerated from the trasnformed schema tree (https://github.com/graphql-java/graphql-java/blob/master/src/main/java/graphql/schema/GraphQLSchema.java#L916)
I've included a test that reproduces the issue. It's possible to mitigate the issue by re-injecting those types through
additionalTypes, but I'd like your thoughts if this would deserve handling at the graphql-java level, or you think it's best left to the user to avoid / mitigate the issue from happening. Happy to contribute a fix if this is worthwhile, but feel free to close if this is a no-fix.