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

[benjie]

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.

:rotating_light: 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]

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.

[benjie]

@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