DictionaryResolverType should use return GraphQLObjectType from Schema rather than cached map

[timward60]

Fixes #592

Checklist

• Pull requests follows the contribution guide
• New or modified functionality is covered by tests

Description

The DictionaryResolverType was returning GraphQL Schema Objects from a cache, however if the shema is transformed by SchemaTransformer, these GraphQL Schema Objects instances can change.

As such we should resolve the GraphQLObjectType from the schema object rather than a local cache.

[oryan-block]

@timward60 this looks good. Could you come up with a test scenario for this?

I'm a little confused as to how you ran into this because the type resolvers run during makeExecutableSchema before which there is no schema to transform.

[timward60]

@timward60 this looks good. Could you come up with a test scenario for this?

Trying to wrap my head around how to best test this. The existing test validate there are no regressions. I could try coding up a integration test that post-transforms the schema, but it seems a bit heavy. I can try to code up some simpler unit tests. Any suggestions on what approach to take?

I'm a little confused as to how you ran into this because the type resolvers run during makeExecutableSchema before which there is no schema to transform.

var baseGraphQLSchema = schemaParser.makeExecutableSchema();
var federatedGraphQLSchema = Federation.transform(baseGraphQLSchema)
              .fetchEntities(federationDataFetcher.orElseThrow())
              .resolveEntityType(federationTypeResolver.orElseThrow())
              .build();

Basically the transforms occur after make executable schema. In addition to the example above using the Federation transform from GraphQL-Java-Federation, we have another transform that deletes fields which ends up mutating the GraphQL schema objects using the TreeTransformerUtil.* utilities.

For example:

final class VersionTransformer extends GraphQLTypeVisitorStub {
  ...
  @Override
  public TraversalControl visitGraphQLFieldDefinition(
      GraphQLFieldDefinition node, TraverserContext<GraphQLSchemaElement> context) {
    return visit(node, node, context);
  }
  ...
  private TraversalControl visit(
      GraphQLSchemaElement element,
      GraphQLDirectiveContainer directiveContainer,
      TraverserContext<GraphQLSchemaElement> context) {
    var optionalVersion = getVersion(directiveContainer);
    if (optionalVersion.isEmpty()) {
      return visitGraphQLType(element, context);
    }

    var fieldVersion = optionalVersion.get();
    if (fieldVersion.compareTo(version) > 0) {
      return TreeTransformerUtil.deleteNode(context);
    }

    return visitGraphQLType(element, context);
  }
  ...
}

var baseGraphQLSchema = schemaParser.makeExecutableSchema();
var transformedGraphQLSchema = new SchemaTransformer().transform(baseGraphQLSchema, new VersionTransformer());

[oryan-block]

A test that builds a simple schema, transforms it (deletes a type?) and tries to run a query that would fail before the fix would be good.

Basically the transforms occur after make executable schema.

If that's true then using the cached map should be ok because it's done before the transformer is executed. So I'm still not sure how does transforming the schema interferes with the dictionary resolver?

[timward60]

A test that builds a simple schema, transforms it (deletes a type?) and tries to run a query that would fail before the fix would be good.

I'll try my best, but the GraphQL-Java transformer logic is quite complex, trying to determine how to replicate this without bringing in our entire schema has been challenging.

If that's true then using the cached map should be ok because it's done before the transformer is executed. So I'm still not sure how does transforming the schema interferes with the dictionary resolver?

That is the problem, the transformer results in a new instances of certain GraphQL types, which means the CacheMap is pointing to an old unreferenced instance from the pre-transformed schema. The GraphQL schema instance maintains the correct mapping. The consequence of not doing this is that later on when the GraphQL-Java is processing queries they use equality checks for GraphQL types which fail due to the cache map returning an orphaned GraphQL type instance.

[timward60]

@oryan-block Added a test that reproduces the issue before my fix (private branch), let me know if there is anything else you need me to address?

[oryan-block]

Thanks @timward60 I'll take a look soon.

[oryan-block]

Good stuff!

Just for my own clarity: the problem here was that the type resolvers are created and stored in the code registry before the transformer runs. These resolvers are then used when trying to run queries and they have an outdated reference to the schema objects so nothing is found.