support excluding directives in FederationSdlPrinter #79

[ankit-joinwal]

This is an extension for #53

Use Case:
There are some server side directives(_mappedType, _mappedInputField and _mappedOperation) added to original schema by graphql-spqr library which do not have directive definitions exposed in the schema.

grapqhl-spqr is correct in not exposing the server side directive definitions as per Graph Spec.

These directives break the Apollo Gateway while discovering the schema of the service.

As part of this fix, i have added support for excluding such directives.

Related leangen/graphql-spqr#290

[apollo-cla]

@ankit-joinwal: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[sachindshinde]

@ankit-joinwal
Thanks for the contribution! Some questions about the specific problem being solved:

There are some server side directives(_mappedType, _mappedInputField and _mappedOperation) added to original schema by graphql-spqr library which do not have directive definitions.

To make sure I understand, it sounds like you're saying that SQPR is creating a graphql-java GraphQLSchema object that contains directive usages (GraphQLDirectives), without corresponding directive definitions (DirectiveDefinitions/GraphQLDirectives).

This creates a particular issue, namely that the corresponding schema for that GraphQLSchema object is invalid as per spec. Specifically, Section 3 of the spec describes validation of a GraphQL schema:

A GraphQL service’s collective type system capabilities are referred to as that service’s “schema”. A schema is defined in terms of the types and directives it supports as well as the root operation types for each kind of operation: query, mutation, and subscription; this determines the place in the type system where those operations begin.

A GraphQL schema must itself be internally valid. This section describes the rules for this validation process where relevant.

And Section 3.13 of the spec covers directives in a schema:

A GraphQL schema describes directives which are used to annotate various parts of a GraphQL document as an indicator that they should be evaluated differently by a validator, executor, or client tool such as a code generator.
...
Directives must only be used in the locations they are declared to belong in.

That last part would presumably imply that directives must have definitions (otherwise they would have no locations).

It's clear at least that this section covers the usage of type-system directives, as it later mentions:

Directives can also be used to annotate the type system definition language as well, which can be a useful tool for supplying additional metadata in order to generate GraphQL execution services, produce client generated runtime code, or many other useful extensions of the GraphQL semantics.

Lee Byron, when asked about whether directive definitions need to exist for directives in schema SDL and whether locations need to match, makes this comment:

one question I have regarding IDL and directives capture is whether the directive location will be enforced

Absolutely it will be enforced. Not doing so invites accidental error.

That would be very precise. BUT a mistake in my opinion. The use of directives for metadata is really powerful but requiring schema consumers to name where the metadata is valid is TOO PRECISE.

I'm not following why being too precise is a bad thing, or a low value thing. Not having this precision would allow people to supply directives in a way that was unexpected or unsupported by the underlying tools that will read those directives. At best this requires GraphQL library authors to essentially reproduce this sort of error checking ad-hoc, at worst this would result in GraphQL users suffering from silent errors where a misspelling or misplacement causes them to accidentally break their services.

So from this, it looks like the schema for the GraphQLSchema object is not valid GraphQL. When federation-jvm prints the schema for this object and exposes it to Apollo Gateway, the gateway will fail composition during validation.

It's worth noting that creating a GraphQLSchema object via SchemaGenerator().makeExecutableSchema() specifically does not allow the usage of GraphQL directives without definitions. This used to be permissible in graphql-java v14.1 via the enforceSchemaDirectives() option, but this option was removed in graphql-java v15 in this commit. So while GraphQLSchema currently allows this, this might be only for strictly backwards-compatibility reasons.

One way to produce a valid schema here is to provide a directive definition (DirectiveDefinition/GraphQLDirective) via e.g. GraphQLSchema.Builder.additionalDirectives(). Note that if using DirectiveDefinition, you'll need to convert the DirectiveDefinition to a GraphQLDirective via something like SchemaGeneratorHelper.buildDirectiveFromDefinition(). You can also directly create a GraphQLDirective that represents a definition.

grapqhl-spqr is correct in not exposing the server side directive definitions as per Graph Spec.

Could you point to the specific portion of GraphQL spec you're talking about? I'm assuming you're talking about schema introspection here, so the relevant part of spec is Section 4.5.6:

The __Directive type represents a Directive that a server supports.

Fields

• name must return a String
• description may return a String or null
• locations returns a List of __DirectiveLocation representing the valid locations this directive may be placed.
• args returns a List of __InputValue representing the arguments this directive accepts.

This section though doesn't talk about restricting which directives are visible via introspection to just those that are "server-side" (by which, I presume you mean directives that are only valid on TypeSystemDirectiveLocations). It's worth noting that both Apollo Server and graphql-java expose "server-side" directives via introspection, so there may be a misunderstanding here.

Another point worth noting is that federation doesn't use introspection to communicate schemas with Apollo Gateway, but instead exposes the schema by adding a new GraphQL field to the implementing service, specifically _service { sdl }. This field is for use solely by Apollo Gateway, and isn't exposed to clients.

As part of this fix, i have added support for excluding such directives.

It's worth noting here that "server-side" directives aren't exposed to clients by Apollo Gateway. Specifically, gateway's composition algorithm strips them out of the composed schema (in the Apollo docs, "server-side" directives are called type-system directives, to align more closely with spec language). This means that clients won't see such directive usages or definitions.

Another relevant detail is that the Apollo federation programming model is predicated around implementing services remaining internal/not publicly accessible. This is because the _entities field (another GraphQL field added to implementing services by federation) is a powerful field that, while critical for the operation of Apollo Gateway, creates security concerns when made publicly accessible.

Given this, my recommendation would be to add GraphQL definitions (DirectiveDefinitions/GraphQLDirectives) for _mappedType, _mappedInputField and _mappedOperation to the GraphQLSchema. This would allow for the GraphQLSchema object to have a valid schema, and for gateway validation to succeed. These directive usages and their definitions will not appear in the schema seen by clients.

[ankit-joinwal]

Hi @sachindshinde

Thanks for the detailed response.

Could you point to the specific portion of GraphQL spec you're talking about? I'm assuming you're talking about schema introspection here, so the relevant part of spec is Section 4.5.6:

Yes i was talking about introspection.

Two things from my side:

1. Because federation-jvm uses SDL instead of introspection from the GraphQL service, even though SPQR removes those directives from introspection we still are not able to get it work with federation-jvm. Adding these directive definitions to schema is something that SPQR is not willing to do. Please refer to Directives _mappedType, _mappedInputField and _mappedOperation not in schema leangen/graphql-spqr#290.
   Now, while this Pull request primarily solves the use case of SPQR, do you agree that it can also help in general, to allow excluding certain directives from schema that gets printed?
   Some standard directive definitions are already being excluded as part of graphql-java-support: Remove federation directive and type definitions from SDL and use stricter SchemaGenerator checks #53.

2. We want to achieve federation along with Code first approach (provided as of now with SPQR efficiently). We dont want to loose the best of both worlds.

It's worth noting here that "server-side" directives aren't exposed to clients by Apollo Gateway. Specifically, gateway's composition algorithm strips them out of the composed schema (in the Apollo docs, "server-side" directives are called type-system directives, to align more closely with spec language). This means that clients won't see such directive usages or definitions.

From SPQR standpoint, not every SPQR service will be exposed via Apollo Gateway so it makes sense for SPQR to have that protection for server side directives.

In your view, does this Pull request violate any specification or principles of federation-jvm?

[sachindshinde]

@ankit-joinwal
Thanks for the quick response!

Yes i was talking about introspection.
...
Because federation-jvm uses SDL instead of introspection from the GraphQL service, even though SPQR removes those directives from introspection we still are not able to get it work with federation-jvm.

When you say "removes those directives from introspection", I'm guessing the reason for this is because the GraphQLSchema object created by SPQR does not include those directive definitions, so that this results in introspection not seeing such definitions and not exposing them at __schema { directives { ... } }.

While this is a clever way to avoid exposing those directives via introspection, it results in a GraphQLSchema object with a spec-invalid schema unfortunately.

Now, while this Pull request primarily solves the use case of SPQR, do you agree that it can also help in general, to allow excluding certain directives from schema that gets printed?

The user benefit is unclear. Type-system directive usages and definitions are already removed by gateway, and adding another place to do so adds a layer of complexity for little value other than supporting spec-invalid schemas (which we generally don't want to encourage), for which it looks like graphql-java may not support in the future (given that they've already made it impossible to do so via SchemaGenerator.makeExecutableSchema()).

To be specific, developers would need to consider which directives to remove and why, and keep track of which directives those are while developing and debugging. This also creates a divergence between what schema is being executed by the server, and which schema is presented to gateway. For users tracking their schema in Apollo Studio (e.g. for managed federation), this difference would appear there as well and hinder observability.

Some standard directive definitions are already being excluded as part of #53.

That PR doesn't exclude both usages and definitions, but instead just definitions. That is because Apollo Gateway will add these definitions itself, and validation will still fail if usages don't match those definitions. (As an aside, this is a bug in Apollo Gateway that we intend to eventually fix; we'd like to eventually arrive at a world where _service { sdl } is a spec-valid schema without any modifications needed by gateway, and where we don't need FederationSdlPrinter.)

We want to achieve federation along with Code first approach (provided as of now with SPQR efficiently). We dont want to loose the best of both worlds.

Could you elaborate here? I'm not sure in what way SPQR hiding certain directives from introspection facilitates an efficient code-first approach, especially given it involves the use of a spec-invalid schema. Producing spec-invalid schemas hurts interoperability with other libraries, which is what we're seeing here firsthand.

From SPQR standpoint, not every SPQR service will be exposed via Apollo Gateway so it makes sense for SPQR to have that protection for server side directives.

Two things here:

• I'm not sure what you're protecting server-side/type-system directives against. To be clear, is the main concern around SQPR directive definitions appearing in introspection? What would happen if those directive definitions appeared in introspection?
• This protection is done in such a manner that it violates GraphQL spec, in two separate and distinct ways. It violates Section 3.13 in that it creates a schema containing directive usages without corresponding definitions. It violates Section 4.5.6 in that this section doesn't allow for directive definitions to be arbitrarily omitted.

If there is a need to keep these directive usages and definitions private, I would recommend finding an alternative means to embedding the metadata so that the library may remain spec-compliant while maintaining privacy.

In your view, does this Pull request violate any specification or principles of federation-jvm?

Given the above (namely that there's not a clear user benefit), I don't see the need for the added complexity and mental overhead for developers. If SPQR does not wish to add directive definitions to its GraphQLSchema object, the end user can do it themselves, specifically using GraphQLSchema.Builder.newSchema(spqrSchema).additionalDirectives(...).build().

[ankit-joinwal]

Thanks @sachindshinde for clarifying on the questions.

Could you elaborate here? I'm not sure in what way SPQR hiding certain directives from introspection facilitates an efficient code-first approach, especially given it involves the use of a spec-invalid schema. Producing spec-invalid schemas hurts interoperability with other libraries, which is what we're seeing here firsthand.

What I meant by efficient code-first approach is that SPQR provides annotations which help a lot in developing GraphQL services quickly. More info here

I'm not sure what you're protecting server-side/type-system directives against. To be clear, is the main concern around SQPR directive definitions appearing in introspection? What would happen if those directive definitions appeared in introspection?

This needs to be answered by the developer of SPQR library . I can only relay information what is present in leangen/graphql-spqr#290

If SPQR does not wish to add directive definitions to its GraphQLSchema object, the end user can do it themselves, specifically using GraphQLSchema.Builder.newSchema(spqrSchema).additionalDirectives(...).build().

I do not understand this. The directives and their definition have been defined in SPQR library. How can we add directive definitions for the directives already defined and part of SPQR library. Can you elaborate on this?
Consider an example below

type Product @_mappedType(type : "__internal__") @key(fields : "upc") {
  name: String @_mappedOperation(operation : "__internal__")
  price: Int @_mappedOperation(operation : "__internal__")
  upc: String @_mappedOperation(operation : "__internal__")
}

The @_mappedType and @_mappedOperation have been added by SPQR library and their definitions reside inside SPQR library.

I understand that this Pull Request may not be merged which leaves us blocked in using federation with SPQR.

[sachindshinde]

@ankit-joinwal

What I meant by efficient code-first approach is that SPQR provides annotations which help a lot in developing GraphQL services quickly.

Ah, you mean Java annotations. I don't think that part would need to change for SPQR to generate spec-valid schemas.

I do not understand this. The directives and their definition have been defined in SPQR library. How can we add directive definitions for the directives already defined and part of SPQR library. Can you elaborate on this?
...
The @_mappedType and @_mappedOperation have been added by SPQR library and their definitions reside inside SPQR library.

Let's look at the example in their README.md:

UserService userService = new UserService();
GraphQLSchema schema = new GraphQLSchemaGenerator()
    .withBasePackages("io.leangen")
    .withOperationsFromSingleton(userService)
    .generate();
GraphQL graphQL = new GraphQL.Builder(schema)
    .build();

You'll need to modify the GraphQLSchema object generated by GraphQLSchemaGenerator(), specifically adding the directive definitions.

How the directive usages are generated is described in Directives.java. Looking at that file, you'll need to create the following GraphQLDirectives in your code:

// UNREPRESENTABLE scalar
GraphQLScalarType unrepresentableScalar = (GraphQLScalarType) schema.getType("UNREPRESENTABLE");

// _mappedType directive definition
GraphQLDirective mappedTypeDirective = GraphQLDirective.newDirective()
        .name("_mappedType")
        .description("")
        .validLocation(Introspection.DirectiveLocation.OBJECT)
        .argument(GraphQLArgument.newArgument()
                .name("type")
                .description("")
                .type(unrepresentableScalar)
                .build()
        )
        .build();

// _mappedOperation directive definition
GraphQLDirective mappedOperationDirective = GraphQLDirective.newDirective()
        .name("_mappedOperation")
        .description("")
        .validLocation(Introspection.DirectiveLocation.FIELD_DEFINITION)
        .argument(GraphQLArgument.newArgument()
                .name("operation")
                .description("")
                .type(unrepresentableScalar)
                .build()
        )
        .build();

// _mappedInputField directive definition
GraphQLDirective mappedInputFieldDirective = GraphQLDirective.newDirective()
        .name("_mappedInputField")
        .description("")
        .validLocation(Introspection.DirectiveLocation.INPUT_FIELD_DEFINITION)
        .argument(GraphQLArgument.newArgument()
                .name("inputField")
                .description("")
                .type(unrepresentableScalar)
                .build()
        )
        .build();

// Add new definitions to schema
GraphQLSchema newSchema = GraphQLSchema.newSchema(schema)
        .additionalDirective(mappedTypeDirective)
        .additionalDirective(mappedOperationDirective)
        .additionalDirective(mappedInputFieldDirective)
        .build();

The newSchema object will then have the directive definitions. (You should be able to pass this to Federation.transform().)

[ankit-joinwal]

Thanks @sachindshinde for patiently working with me on the resolution. I have tried above code and it works well with Gateway. So, even though i am not sure if this is the correct way of doing things with SPQR (means providing the directive definitions for SPQR directives in our code) , i think this will unblock us.

[sachindshinde]

@ankit-joinwal Good to hear that the code works with gateway and that you're unblocked, thanks for explaining the problem in detail!