Begin supporting executable directives in federation #3464

jbaxleyiii · 2019-11-01T21:46:40Z

The purpose of this PR is to introduce support for ExecutableDirectives. This doesn't require any changes from the gateway, only in how we perform composition and validation.

Composition now adds all custom directives to the final schema

Note: since we're dealing with n implementations of the same directive, composition arbitrarily chooses the first one it sees. This should be fine based on our new validations.

Additional validations:
- EXECUTABLE_DIRECTIVES_ONLY: Custom directives must be executable only. Directives that use aTypeSystemDirectiveLocation are disallowed and will trigger validation errors
- EXECUTABLE_DIRECTIVES_EVERYWHERE: Custom directives must be implemented within every service, even if it's just a no-op. The directive must exist in the SDL query for every service { _service { sdl } }
- EXECUTABLE_DIRECTIVES_IDENTICAL: Custom directives must be identical across all services. This means that the locations of the directive must be exactly the same from one service to the next.

Preserved commentary from @jbaxleyiii:

It removes extra filtering we currently do to allow any directives that were part of the service definitions to be maintained in the final schema. The final implementation is a model where if an executable directive is present, all services must both contain and exactly match the ones found in other services. This is because executable directives can't be limited to specific types/fields, but must be usable on any location they are defined. Long term we may be able to move support for common executable directives to the gateway but custom ones will always require them to be present in all services (even as a no-op)

jbaxleyiii

@trevor-scheer this looks really great. I love how nicely this all came together. I left a couple small comments and the only addition I'd like to see (aside from docs ;) ) is a test to make sure that validation works for arguments on a directive as well.

Oh and I'd like to see some tests using an executable directive, especially accross service boundaries. We need to make sure that the query planner keeps them in the correct location, keeps variables for them as needed, etc

Really excited to see this land

packages/apollo-federation/src/composition/compose.ts

...deration/src/composition/validate/postComposition/__tests__/executableDirectivesOnly.test.ts

...on/src/composition/validate/postComposition/__tests__/executableDirectivesEverywhere.test.ts

...apollo-federation/src/composition/validate/postComposition/executableDirectivesEverywhere.ts

.../apollo-federation/src/composition/validate/postComposition/executableDirectivesIdentical.ts

This commit is the very start of executable directives in federation. It removes extra filtering we currently do to allow any directives that were part of the service definitions to be maintained in the final schema. The final implementation is a model where if an executable directive is present, *all* services must both contain and exactly match the ones found in other services. This is because executable directives can't be limited to *specific* types/fields, but must be usable on any location they are defined. Long term we may be able to move support for common executable directives to the gateway but custom ones will always require them to be present in all services (even as a no-op)

Validators may need knowledge of the whole serviceList in order to perform certain validations. This change extends the API to call post-composition validators with this additional information. * Update current validators * Update tests * Create PostCompositionValidator type

This commit appends all of the DirectiveDefinitionNodes to the composed schema (at their respective GraphQLDirective) to be inspected by validators. These definitions are indexed by the service that they belong to in order to provide useful validation messages.

…hin EVERY service

…ices

Previously, we added ALL definitions of a single directive to the schema which resulted in a schema validation error. This only adds them once. Added tests to reproduce the bug and confirm the fix.

Custom executable directives should be validated as equal only if they have the same locations and same arguments.

These tests verify that the query plan includes custom directives in requests to underlying services. They also ensure that validation errors are as expected when services define custom directives in an invalid way.

Scope "in all services" validator to executable directives only.

docs/source/federation/errors.md

docs/source/federation/implementing.md

…tives. > Note: This is not the solution, just a test that demonstrates the failure! Recently, the beginnings of [Executable directive] support were brought into federation through #3464 and its follow-up, #3536. The implementation of executable directives currently in place within federation requires that federated directives be declared identically across all implementing services, even if their implementation is different (or a no-op). This is working as intended! However, [Type system directives] need some additional pruning from the composed schema. Specifically, while type system directive declarations don't need to be declared identically across implementing services, their _definitions_ are currently (intentionally!) removed from the composed schema. That would be fine, but the _usages_ of those directives are currently being left in-tact, which results in validation errors within composed schemas since the directives, while still defined in the implementing services, have been removed. It's certainly important that the implementing services maintain those type system directives in order to act upon them within the services themselves, I don't believe those directives need to be preserved in the composed schema that runs at the gateway and is used to generate the query plan. This commit is mostly documenting a reproduction of an issue that was brought to my attention and introduces what I believe is a test case to build a solution against. [Type system directive]: https://graphql.github.io/graphql-spec/draft/#TypeSystemDirectiveLocation [Executable directive]: https://graphql.github.io/graphql-spec/draft/#ExecutableDirectiveLocation

> Note: This is not the solution, just a test that demonstrates the failure! Recently, the beginnings of [Executable directive] support were brought into federation through #3464 and its follow-up, #3536. The implementation of executable directives currently in place within federation requires that federated directives be declared identically across all implementing services, even if their implementation is different (or a no-op). This is working as intended! However, [Type system directives] need some additional pruning from the composed schema. Specifically, while type system directive declarations don't need to be declared identically across implementing services, their _definitions_ are currently (intentionally!) removed from the composed schema. That would be fine, but the _usages_ of those directives are currently being left in-tact, which results in validation errors within composed schemas since the directives, while still defined in the implementing services, have been removed. It's certainly important that the implementing services maintain those type system directives in order to act upon them within the services themselves, I don't believe those directives need to be preserved in the composed schema that runs at the gateway and is used to generate the query plan. This commit is mostly documenting a reproduction of an issue that was brought to my attention and introduces what I believe is a test case to build a solution against. [Type system directive]: https://graphql.github.io/graphql-spec/draft/#TypeSystemDirectiveLocation [Executable directive]: https://graphql.github.io/graphql-spec/draft/#ExecutableDirectiveLocation

> Note: This is not the solution, just a test that demonstrates the failure! Recently, the beginnings of [Executable directive] support were brought into federation through apollographql#3464 and its follow-up, apollographql#3536. The implementation of executable directives currently in place within federation requires that federated directives be declared identically across all implementing services, even if their implementation is different (or a no-op). This is working as intended! However, [Type system directives] need some additional pruning from the composed schema. Specifically, while type system directive declarations don't need to be declared identically across implementing services, their _definitions_ are currently (intentionally!) removed from the composed schema. That would be fine, but the _usages_ of those directives are currently being left in-tact, which results in validation errors within composed schemas since the directives, while still defined in the implementing services, have been removed. It's certainly important that the implementing services maintain those type system directives in order to act upon them within the services themselves, I don't believe those directives need to be preserved in the composed schema that runs at the gateway and is used to generate the query plan. This commit is mostly documenting a reproduction of an issue that was brought to my attention and introduces what I believe is a test case to build a solution against. [Type system directive]: https://graphql.github.io/graphql-spec/draft/#TypeSystemDirectiveLocation [Executable directive]: https://graphql.github.io/graphql-spec/draft/#ExecutableDirectiveLocation

> Note: This is not the solution, just a test that demonstrates the failure! Recently, the beginnings of [Executable directive] support were brought into federation through #3464 and its follow-up, #3536. The implementation of executable directives currently in place within federation requires that federated directives be declared identically across all implementing services, even if their implementation is different (or a no-op). This is working as intended! However, [Type system directives] need some additional pruning from the composed schema. Specifically, while type system directive declarations don't need to be declared identically across implementing services, their _definitions_ are currently (intentionally!) removed from the composed schema. That would be fine, but the _usages_ of those directives are currently being left in-tact, which results in validation errors within composed schemas since the directives, while still defined in the implementing services, have been removed. It's certainly important that the implementing services maintain those type system directives in order to act upon them within the services themselves, I don't believe those directives need to be preserved in the composed schema that runs at the gateway and is used to generate the query plan. This commit is mostly documenting a reproduction of an issue that was brought to my attention and introduces what I believe is a test case to build a solution against. [Type system directive]: https://graphql.github.io/graphql-spec/draft/#TypeSystemDirectiveLocation [Executable directive]: https://graphql.github.io/graphql-spec/draft/#ExecutableDirectiveLocation

* tests: Show composition failures w/ undeclared type-system directives. > Note: This is not the solution, just a test that demonstrates the failure! Recently, the beginnings of [Executable directive] support were brought into federation through #3464 and its follow-up, #3536. The implementation of executable directives currently in place within federation requires that federated directives be declared identically across all implementing services, even if their implementation is different (or a no-op). This is working as intended! However, [Type system directives] need some additional pruning from the composed schema. Specifically, while type system directive declarations don't need to be declared identically across implementing services, their _definitions_ are currently (intentionally!) removed from the composed schema. That would be fine, but the _usages_ of those directives are currently being left in-tact, which results in validation errors within composed schemas since the directives, while still defined in the implementing services, have been removed. It's certainly important that the implementing services maintain those type system directives in order to act upon them within the services themselves, I don't believe those directives need to be preserved in the composed schema that runs at the gateway and is used to generate the query plan. This commit is mostly documenting a reproduction of an issue that was brought to my attention and introduces what I believe is a test case to build a solution against. [Type system directive]: https://graphql.github.io/graphql-spec/draft/#TypeSystemDirectiveLocation [Executable directive]: https://graphql.github.io/graphql-spec/draft/#ExecutableDirectiveLocation * Remove directives from field definition in composition * Expand functionality to all custom type system directives * Update stripping mechanism to include all TypeSystemDirectives * Jest auto-prettification of snapshots * Rename var in composition to match implementation * Add changelog entry for PR #3736 * nit: Line-length. It's bad in this file altogether, but broad re-formatting of this file won't do anything better than stomp on history. Since this line is already changing, it's worth a fix in the same commit. * Change `definition` to be a `const`-ant rather than `let`. The overall height of this block seems to be a ripe opportunity to try to lock down the re-assignment of this objects' reference, even if it wasn't introduced by this PRs intended change. * Add a comment about what `null` return does on visits. * Add a comment about why we're stripping type system directives. Co-authored-by: Jesse Rosenberger <git@jro.cc> Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>

…pollo-server#3464) This commit is the very start of executable directives in federation. It removes extra filtering we currently do to allow any directives that were part of the service definitions to be maintained in the final schema. The final implementation is a model where if an executable directive is present, *all* services must both contain and exactly match the ones found in other services. This is because executable directives can't be limited to *specific* types/fields, but must be usable on any location they are defined. Long term we may be able to move support for common executable directives to the gateway but custom ones will always require them to be present in all services (even as a no-op) * Update post-composition validator API Validators may need knowledge of the whole serviceList in order to perform certain validations. This change extends the API to call post-composition validators with this additional information. * Update current validators * Update tests * Create PostCompositionValidator type * Add directive metadata to composed schema This commit appends all of the DirectiveDefinitionNodes to the composed schema (at their respective GraphQLDirective) to be inspected by validators. These definitions are indexed by the service that they belong to in order to provide useful validation messages. * Add validator for permitting only custom ExecutableDirectives * Add validator for the enforcing the presence of custom directives within EVERY service * Add validator for enforcing custom directive identicality across services * Rename DefinitionsMap and ExtensionsMap * Add integration tests for custom directives These tests verify that the query plan includes custom directives in requests to underlying services. They also ensure that validation errors are as expected when services define custom directives in an invalid way. Apollo-Orig-Commit-AS: apollographql/apollo-server@e3b118b

…ollo-server#3736) * tests: Show composition failures w/ undeclared type-system directives. > Note: This is not the solution, just a test that demonstrates the failure! Recently, the beginnings of [Executable directive] support were brought into federation through apollographql/apollo-server#3464 and its follow-up, apollographql/apollo-server#3536. The implementation of executable directives currently in place within federation requires that federated directives be declared identically across all implementing services, even if their implementation is different (or a no-op). This is working as intended! However, [Type system directives] need some additional pruning from the composed schema. Specifically, while type system directive declarations don't need to be declared identically across implementing services, their _definitions_ are currently (intentionally!) removed from the composed schema. That would be fine, but the _usages_ of those directives are currently being left in-tact, which results in validation errors within composed schemas since the directives, while still defined in the implementing services, have been removed. It's certainly important that the implementing services maintain those type system directives in order to act upon them within the services themselves, I don't believe those directives need to be preserved in the composed schema that runs at the gateway and is used to generate the query plan. This commit is mostly documenting a reproduction of an issue that was brought to my attention and introduces what I believe is a test case to build a solution against. [Type system directive]: https://graphql.github.io/graphql-spec/draft/#TypeSystemDirectiveLocation [Executable directive]: https://graphql.github.io/graphql-spec/draft/#ExecutableDirectiveLocation * Remove directives from field definition in composition * Expand functionality to all custom type system directives * Update stripping mechanism to include all TypeSystemDirectives * Jest auto-prettification of snapshots * Rename var in composition to match implementation * Add changelog entry for PR apollographql/apollo-server#3736 * nit: Line-length. It's bad in this file altogether, but broad re-formatting of this file won't do anything better than stomp on history. Since this line is already changing, it's worth a fix in the same commit. * Change `definition` to be a `const`-ant rather than `let`. The overall height of this block seems to be a ripe opportunity to try to lock down the re-assignment of this objects' reference, even if it wasn't introduced by this PRs intended change. * Add a comment about what `null` return does on visits. * Add a comment about why we're stripping type system directives. Co-authored-by: Jesse Rosenberger <git@jro.cc> Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com> Apollo-Orig-Commit-AS: apollographql/apollo-server@db71535

jbaxleyiii requested a review from trevor-scheer November 1, 2019 21:46

trevor-scheer requested a review from abernix November 6, 2019 23:01

jbaxleyiii commented Nov 7, 2019

View reviewed changes

James Baxley and others added 13 commits November 11, 2019 14:48

Fix typings

a2ddaac

Small changes

a2ebb33

Add validator for permitting only custom ExecutableDirectives

e482c17

Add validator for the enforcing the presence of custom directives wit…

c36446d

…hin EVERY service

Add validator for enforcing custom directive identicality across serv…

4b93ca0

…ices

Only add a directive to the schema once

bcfec29

Previously, we added ALL definitions of a single directive to the schema which resulted in a schema validation error. This only adds them once. Added tests to reproduce the bug and confirm the fix.

Rename DefinitionsMap and ExtensionsMap

3093741

No need to spread here at all

4ec8b60

Validate against directive arguments

2e53294

Custom executable directives should be validated as equal only if they have the same locations and same arguments.

Add integration tests for custom directives

c6910ab

These tests verify that the query plan includes custom directives in requests to underlying services. They also ensure that validation errors are as expected when services define custom directives in an invalid way.

trevor-scheer force-pushed the executable-directives branch from 0829357 to c6910ab Compare November 11, 2019 22:49

Include directives with args in integration tests

1cda9d8

trevor-scheer marked this pull request as ready for review November 11, 2019 22:59

Add docs for new directive-related validation errors

e89471e

jbaxleyiii requested a review from StephenBarlow as a code owner November 12, 2019 17:18

trevor-scheer added 7 commits November 12, 2019 09:19

Fix MD link

dea92c2

Merge branch 'master' into executable-directives

a740c23

Merge branch 'master' into executable-directives

36cab27

Rename validator and respective error code

fbae698

Scope "in all services" validator to executable directives only.

Preserve the federation object via spread

eddf47b

Add documentation about implementing custom directives

4876f93

Update changelog

b3b9fc2