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

[RFC] Prevent @skip and @include on root subscription selection set #860

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Apr 25, 2021

Following on from #776 and based on the discussion in the GraphQL.js pull request I'm still uncomfortable with the single root field validation rule for subscription operations.

Currently the "single root field" algorithm sets variableValues to be the empty set and then calls CollectFields. To see the issue with this, consider the following subscription operation:

type Subscription {
  mySubscriptionField1: Int
  mySubscriptionField2: Int
  mySubscriptionField3: Int
}

subscription TwoFieldsByDefault($bool: Boolean = true) {
  mySubscriptionField1 @skip(if: $bool)
  mySubscriptionField2 @include(if: $bool)
  mySubscriptionField3 @include(if: $bool)
}

A call to CollectFields passing an empty map for {variableValues} will result in all selections with @skip directives being considered, and all selections with the @include directive being skipped. So according to the current validation rule, this operation is valid - it only has one field mySubscriptionField1 (which is not an introspection field) in the root selection set.

However, if you pass no variables at runtime, the runtime CollectFields() called during CreateSourceEventStream for the subscription operation will result in groupedFieldSet containing two selections (mySubscriptionField2 and mySubscriptionField3). This will result in a request error being raised: If groupedFieldSet does not have exactly one entry, raise a request error.

This catches the invalid operation at runtime rather than validation time, giving a false sense of security about the validity of a GraphQL operation that may fail by default.

No other validation rule in the entire of Section 5 references variableValues or calls CollectFields; but we already know that the root subscription selection set is very special (it's been discussed many times at the GraphQL Spec WG).

This PR proposes that @skip and @include are forbidden on root subscription selection sets.

🚨 This is a breaking change since previously valid operations such as the following will now be marked as invalid:

subscription ThisIsFineDotJpg($bool: Boolean = true) {
  mySubscriptionField1 @skip(if: $bool)
  mySubscriptionField2 @include(if: $bool)
}

@leebyron
Copy link
Collaborator

This is probably the right direction - I need some time to read and review (and this is definitely worth a WG discussion).

Though one point of clarification for the process we're going through for anyone following along:

We should first remove ambiguity and ensure subscription root selection sets are behaving as originally designed, even if that means further restricting behavior to do so (eg #776). It's good to be inspired to further improve from there, but these restrictions are a good thing not just because they remove the ambiguity, but because in doing so they carve out design space for proposals like this one.

Copy link
Collaborator

@leebyron leebyron left a comment

Choose a reason for hiding this comment

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

Agreed that this is a restriction worth adding, and so this can be RFC1.

I do think we should treat this as a separate RFC from #776 since that was already approved and merged, and thus should be a stacked PR.

Comment on lines 494 to 496
* If {variableValues} is {null}:
* {selection} must not provide the `@skip` directive.
* {selection} must not provide the `@include` directive.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is breaking. We already allow @skip(if: $var) with no variables provided to result in not skipping.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only breaking in the way that it was intended to be breaking; I checked every call to CollectFields when I proposed the change and in all cases variableValues was non-null. The new null was introduced by me in this one specific location (only intended to be called from validation).

Comment on lines 482 to 488
When {CollectFields()} is used during validation (see for example the
[single root field](#sec-Single-root-field) subscription operation validation
rule), the runtime value for {variableValues} will not be available - in this
case we set {variableValues} to {null} and forbid the use of the `@skip` and
`@include` directives. During execution, {variableValues} will always be
non-null.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is pretty confusing and feels like a hack. I'd rather see validation self-contained and not require the edge case within execution that's potentially not used by execution.

* Let {subscriptionType} be the root Subscription type in {schema}.
* Let {selectionSet} be the top level selection set on {subscription}.
* Let {groupedFieldSet} be the result of
{CollectFields(subscriptionType, selectionSet, null)}.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be scary, but I think the right thing to do would be to no longer rely on CollectFields, which is intended for runtime (hence the requirement to have variables)

You could probably get hand-wavy with the spec text and just say something about "recursively follow fragment spreads", but I think even if you were to copy and pair-down the CollectFields algorithm to only look for one field (encountering none with @skip/@include along the way) that it might not be too burdensome.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was your suggestion to use CollectFields() that made me realise the issue with @skip/@include for subscriptions. I'll add a similar algorithm.

@leebyron leebyron added 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) and removed 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) labels May 13, 2021
@benjie benjie force-pushed the subscription-single-root-field branch from 27408f8 to 5e742bb Compare May 26, 2021 14:47
@benjie
Copy link
Member Author

benjie commented May 26, 2021

@leebyron I've added a new CollectSubscriptionFields() algorithm which is basically a copy of CollectFields() but with the no @skip/@include constraint. I did consider trying to implement a more efficient algorithm that just scans for/returns the single field but decided to stick with CollectFields-esque for three reasons:

  1. maintenance is easier because the algorithms are virtually identical
  2. the extra fields get collected so that the GraphQL library can raise the relevant errors for them
  3. consistency with the existing algorithm eases understanding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants