Skip to content
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

Reject directives within fields arg of @key, @provides and @requires #1975

Merged
merged 3 commits into from
Jul 14, 2022

Conversation

pcmanus
Copy link
Contributor

@pcmanus pcmanus commented Jul 12, 2022

Noticed that we accepting (parsing) directive applications with a @key, @provide or @requires, but were silently ignoring it. In other words:

type T @key(fields: "x ... @skip(if: true) { y }") {
  x: Int
  y: Int
}

was accepted, but the @skip was completely ignored. Of course, it's unclear why would anyone write this, but it still imo qualify as a bug that we silently ignore this since the code essentially do not do what is written. It's certainly an oversight.

More generally, as we don't anything with any such directive applications, and for user/custom directives, there is no way to provide any processing whatsoever, so overall, I think we should just reject any directive applications to avoid any risk of confusion, and this is what the patch does.

I'll further note that:

  1. the example above with @skip is a bit arbitrary when you need a hard-coded true or false for the if: why would you ever write this? But the context here is actually Deliver: @defer support in the query planner #1921: someone could want to try to see what a @defer does within a fields value, and we should be clear it just doesn't work.
  2. I'm not trying to say that it will never make sense to allow directives in such values. There absolutely could be future use case for it, but we can just relax this validation when that happens. The point is that we shouldn't accept something we don't actually support.
  3. one could make the argument that, insofar as this reject things that were not rejected before, this could be considered as a backward incompatible change. My argument here is that the existing behaviour is imo a bug and so I have no qualm putting this in a minor release (I mean, fixing bugs always modify behaviour after all). But of course, we'll still put a word about it in the changelog.

@netlify
Copy link

netlify bot commented Jul 12, 2022

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit e7aded5

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 12, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

@benweatherman benweatherman left a comment

Choose a reason for hiding this comment

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

🐐

Sylvain Lebresne added 3 commits July 14, 2022 15:40
…ovides`

Syntactically, we were allowing directive applications within the
`fields` argument of `@key`/`@requires`/`@provides` directives, but
this was potentially confusing since those applications were competely
ignored. This means something like a `@skip` or `@include` (or, when
supported, `@defer`) would silently not take effect, and for custom
directives, there is no way to have those processed in any way.

To avoid any risk of misguiding something, this patch reject any
directive applications with that `fields` argument. Of course, this does
not prevent us from later support a specific set of directives that make
sense if we so wish.
The validation of the `fields` argument of
`@key`/`@provides`/`@requires` was always stopping on the first
encountered error, but noticed a trivial opportunity to collect
more errors in at some of the cases, which is implemented by
this commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants