RFC: Assert subscription field is not introspection.

[benjie]

This PR implements the spec changes in the following GraphQL Spec RFC:

graphql/graphql-spec#776

[benjie]

This is very precise wording; if we were to use one non-introspection field or a single non-introspection field here then it wouldn't place limits on the introspection fields we added, only the non-introspection fields.

[benjie]

I've updated the PR to overhaul the subscription validation rule to walk fragments too (see #2715).

[IvanGoncharov]

@benjie You need to handle recursive fragments:

subscription {
  ...A
}

fragment A {
  ...A
}

Even thought it's invalid, validation shouldn't be stuck on it

[benjie]

@IvanGoncharov I've handled that by tracking visitedFragmentNames; but I've added a test with your operation to show this is the case for completeness 👍

[IvanGoncharov]

Tests look great 👍
Implementation is also looking correct.
I just like to play with the source code of the rule to see if it can be simplified.
Will try to do this on weekends.

[leebyron]

Happy to also see this validation bug being fixed - fields are collected before asserting a single selection

[leebyron]

Any reason why this can't be a more typical CollectFields() implementation? I'm surprised there isn't something to be reused from another validation rule.

[benjie]

The closest I could find was _collectFieldsAndFragmentNames from src/validation/rules/OverlappingFieldsCanBeMergedRule.js but it doesn't recurse into named fragments. We also can't use collectFields from execution because it requires an execution context.

My algorithm is more efficient than recursing over calls to _collectFieldsAndFragmentNames because it has no need to reference the underlying types - there's no need for calls to parentType.getFields()/typeFromAST/isObjectType/etc since all we want to know is:

1. is there any __introspection fields?
2. is there more than one field?

The actual type they're defined on (or even whether it exists or not!) is irrelevant.

[benjie]

I suppose that could potentially be untrue; e.g.

type Query implements Node {...}
type Subscription implements Node {...}

subscription ShouldBeFine($bool: Boolean!) {
  ... on Node {
    ... on Query { __typename id firstName }
    ... on Subscription {
      mySubscriptionField @include(if: $bool)
      myOtherSubscriptionField @skip(if: $bool)
    }
  }
}

in this case a runtime collectFields would return only mySubscriptionField or myOtherSubscription field (depending on the value of $bool) because the Query selections would be scoped out as incompatible types and the @include/@skip would ensure only one field was valid. This is problematic though, since in validate we don't have access to the runtime variables so we cannot determine which of the fields will/won't be included.

In the spec edit I wrote this leveraging the CollectFields call which was already present, which already relies on varibleValues; but GraphQL.js allows validation without variables so... is this a fundamental issue in the way GraphQL.js is built? Personally I think we should edit the spec to reflect GraphQL.js' behaviour - validating the query without access to the variables seems desirable and so having the validation rule for "single root field" depend on CollectFields which depends on variableValues is undesirable.

[benjie]

Reading more carefully the spec explicitly makes variableValues an empty object, so in this case the operation would be valid; however so would the following:

type Query implements Node {...}
type Subscription implements Node {...}

subscription ShouldBeFine($bool: Boolean!) {
  ... on Node {
    ... on Query { __typename id firstName }
    ... on Subscription {
      mySubscriptionField1 @include(if: $bool)
      mySubscriptionField2 @include(if: $bool)
      mySubscriptionField3 @skip(if: $bool)
    }
  }
}

In this case the CollectFields algorithm would result in only mySubscriptionField3 being considered; the operation would pass validation, but then if $bool was set to true at runtime there'd be two selections and the operation would not work (it would raise a request error in step 5 of CreateSourceEventStream).

This is definitely a spec bug. I'm going to write up a proper fix.

[benjie]

(After more thought, I'm not sure it's a bug so much as an "unexpected solution"; but I've filed a potential spec edit anyway: graphql/graphql-spec#860)

Given the above discussion I've rewritten the solution to use CollectFields so that it can take into account the fragment compatibility and the @include/@skip directives in the same way that the GraphQL spec would.

[benjie]

To address @leebyron's feedback I've rewritten the implementation to more closely follow the spec - it calls into collectFields (and to do so it sets up a fake execution context, similar to how the spec creates the empty variableValues map). This allows the factoring in of @skip / @include and fragment traversal as the spec currently states (previously this implementation did not fully consider these concerns, being overly strict and rejecting more operations than necessary).

This is the only place in Section 5 of the GraphQL spec that utilises CollectFields, so it's not unexpected that the code in GraphQL.js to set this up is a little gross with a bunch of fake things. I think this is prefereable to re-implementing the algorithms locally.

[IvanGoncharov]

Generally looks good 👍
Not really happy with the fact that we create fake execution context but it's the only way to use collectFields at the moment.
Added FIXME to show that we plan to change that in the future since collectFields doesn't really need the whole execution context.