Skip to content

feat(gateway): Implement @core v0.2, including "for" and core__Purpose support #957

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Aug 18, 2021

Conversation

trevor-scheer
Copy link
Contributor

@trevor-scheer trevor-scheer commented Aug 11, 2021

This PR updates the gateway to @core v0.2 🎉

This includes implementing support for the optional for argument in the @core directive. This argument allows a core directive usage to specify how a gateway should behave in the case that its feature is unsupported. This is important for various use cases - in particular, execution and security concerns.

the gateway will throw UnsupportedFeature on schema update IF:

  1. it sees a core feature specified which it doesn't implement AND
  2. that core feature provides the for argument (only valid options are currently EXECUTION and SECURITY

additionally, if the supergraph specifies core v0.1, and contains core directives which use for:, the gateway will report ForUnsupported.

TODOs

Closes https://github.com/apollographql/polaris-planning/issues/109

* Permit all core features for core v0.1
* Enforce "for" requirements on core features for core v0.2
@trevor-scheer trevor-scheer self-assigned this Aug 12, 2021
Copy link
Contributor

@queerviolet queerviolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good! my only question is about the behavior we want to have if we see for: something in a core v0.1 document. currently, we explicitly do not honor it and fail open, which seems… maybe not great, since we understand the likely intent, and the intent was to protect the schema from being served by us dunderheads who don't understand how the @auth directive works (or whatever)

three options:

  1. hard fail if we see a for: in a core v0.1 doc. this will teach people (mostly us, probably ;) ) to update the feature in their docs / generators
  2. warn if we see a for: in a core v0.1 doc, but honor its intention (i.e. ensure the feature is supported). this will annoy people into eventually updating the feature, and meanwhile mostly do what they want
  3. do not warn if we see for: in a core v0.1 doc and honor its intention

i think (1) is a bit harsh and (3) is a bit loose, so i lean towards (2). wyt?

@queerviolet queerviolet enabled auto-merge August 18, 2021 23:50
@queerviolet queerviolet merged commit f83e11e into main Aug 18, 2021
@queerviolet queerviolet deleted the trevor/implement-for branch August 18, 2021 23:55
abernix added a commit that referenced this pull request Aug 26, 2021
abernix added a commit that referenced this pull request Aug 26, 2021
abernix added a commit that referenced this pull request Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants