SchemaParser should not return appliedDirectives under directives

[xcq1]

Description

I'm seeing unexpected behavior from SchemaParser that prevents me to properly upgrade to the newest versions of graphql-java 19.2 & graphql-java-kickstart 15.
I'm using GraphQL Federation via com.apollographql.federation:federation-graphql-java-support:2.2.0 which includes directives such as @extends or @key in the schema. Starting with graphql-java-tools 13.0.2, these are not only reported as appliedDirectives, but as directives as well. This causes the validation to fail and the server to not start, because they are not defined in the same schema:

# ... stack trace ...
Caused by: graphql.schema.validation.InvalidSchemaException: invalid schema:
A definition for directive 'extends' could not be found
A definition for directive 'key' could not be found
A definition for directive 'external' could not be found
# ...

I believe this is incorrect with regards to the docs in https://github.com/graphql-java/graphql-java/blob/master/src/main/java/graphql/schema/GraphQLDirective.java#L28-L32. From my limited knowledge, I think this states that the field directives should only contain directive definitions and no longer directive applications, however I'm not entirely sure whether I'm reporting this at the right place.

It works correctly with either

• graphql-java-kickstart 14 that depends on com.graphql-java-kickstart:graphql-java-tools 13.0.0
• graphql-java-kickstart 15 and downgrading com.graphql-java-kickstart:graphql-java-tools to 13.0.1

Expected behavior

Parsing such a schema:

type Query @extends {
  dummy(): String
}

with schemaParser.parseSchemaObjects() should return empty query.directivesHolder.allDirectivesByName == [] and query.directivesHolder.allAppliedDirectivesByName == ["extends" to ...]

Actual behavior

Parsing with schemaParser.parseSchemaObjects() returns query.directivesHolder.allDirectivesByName == ["extends" to ...].

Steps to reproduce the bug

See SDL example above. In case this is insufficient, I can also provide a minimal repro project.

[foxylion]

Are there any updates on this?

[mmuth]

Still strugging with this one. Having to build workarounds :(

[oryan-block]

@xcq1 can you check if #763 resolved this?

[xcq1]

@oryan-block I tried to use 13.1.1-SNAPSHOT in the same project and I'm now getting this exception

Caused by: graphql.kickstart.tools.SchemaError: Found applied directive key without corresponding directive definition.
	at graphql.kickstart.tools.SchemaParser.buildAppliedDirectives(SchemaParser.kt:355)
	at graphql.kickstart.tools.SchemaParser.createObject(SchemaParser.kt:127)
	at graphql.kickstart.tools.SchemaParser.parseSchemaObjects(SchemaParser.kt:84)

according to the debugger for Directive{name='key', arguments=[Argument{name='fields', value=StringValue{value='id'}}]}. The exception is correct in so far that @key is not defined in the schema, only used in applied form like so:

type Resource @extends @key(fields: "id") {
...
}

I tried to poke around a bit more, the code to setup the schema would call Federation.transform which I think would add the directive definitions, but can only be called once a GraphQLSchema is built.: (Based on the docs of https://github.com/apollographql/federation-jvm/tree/main/graphql-java-support#creating-federated-schemas)

    fun schemaTransformer(schemaParser: SchemaParser): SchemaTransformer {
        val (query, mutation, subscription, dictionary, directives, codeRegistryBuilder) = schemaParser.parseSchemaObjects() // new exception happens here
        val queryTypeIsEmpty = query.fieldDefinitions.isEmpty()
        val newQuery = if (queryTypeIsEmpty) query.transform { graphQLObjectTypeBuilder: GraphQLObjectType.Builder ->
            graphQLObjectTypeBuilder.field(
                GraphQLFieldDefinition.newFieldDefinition()
                    .name("_dummy")
                    .type(Scalars.GraphQLString)
                    .build()
            )
        } else query
        val graphQLSchema = GraphQLSchema.newSchema()
            .query(newQuery)
            .mutation(mutation)
            .subscription(subscription)
            .additionalTypes(dictionary)
            .additionalDirectives(directives)
            .codeRegistry(codeRegistryBuilder.build())
            .build()
        return Federation.transform(graphQLSchema, queryTypeIsEmpty)
    }

So the new exception kind of makes sense here, but for this case it's a bit less ideal that the parser already performs semantic checks. However, frankly I don't have the overview which GQL libraries are at play here, which one is buggy or if there's a better way to sidestep the behavior. I can just tell you the existing code still works with 13.0.1.

[oryan-block]

@xcq1 sorry for the late reply but are you not able to add these definitions to your schema? I'm not familiar with how Apollo works or intended to work.

One option, if this is really required, would be to add a schema option that will "inject" these definitions if turned on.

[xcq1]

@oryan-block
Well I don't really want to define them in every service, but I tried the suggestion to see the results. So the first part of the federation stuff looks like this:

directive @external on FIELD_DEFINITION
directive @requires(fields: _FieldSet!) on FIELD_DEFINITION
directive @provides(fields: _FieldSet!) on FIELD_DEFINITION
directive @key(fields: _FieldSet!) on OBJECT | INTERFACE

# this is an optional directive discussed below
directive @extends on OBJECT | INTERFACE

scalar _FieldSet

This now gives me the following error:
graphql.kickstart.tools.SchemaError: Expected type '_FieldSet' to be a GraphQLInputType, but it wasn't! Was a type only permitted for object types incorrectly used as an input type, or vice-versa?

As far as I can see all of this would be added by the call to Federation.transform() which is not reached because the parser complains about the unknown applied directives first, before they can be defined. Feels like sort of a hen/egg problem.

If you think this is an issue on the part of apollographql/federation-jvm (e.g. because they should implement this differently), I can also report it there.

[oryan-block]

@xcq1 If you look at the test here, as long as you pass the scalar type to the parser it doesn't have a problem.

I don't know anything about Federation.transform() but it does sound like some of it has to happen before you try to build the schema. Maybe that's something you can ask them to help with?