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: Assert subscription field is not introspection. #2861

Merged
merged 15 commits into from
Jun 3, 2021

Conversation

benjie
Copy link
Member

@benjie benjie commented Nov 25, 2020

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

graphql/graphql-spec#776

@benjie benjie force-pushed the subscription-root-introspection-ban branch from 15eded0 to ba1a314 Compare November 25, 2020 17:45
@@ -6,9 +6,10 @@ import type { OperationDefinitionNode } from '../../language/ast';
import type { ASTValidationContext } from '../ValidationContext';

/**
* Subscriptions must only include one field.
* Subscriptions must only include a non-introspection field.
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 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
Copy link
Member Author

benjie commented Dec 5, 2020

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

@IvanGoncharov
Copy link
Member

@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
Copy link
Member Author

benjie commented Jan 7, 2021

@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 👍

Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

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.

@IvanGoncharov IvanGoncharov added this to the 16.0.0-alpha.1 milestone Apr 1, 2021
*/
export function SingleFieldSubscriptionsRule(
context: ASTValidationContext,
): ASTVisitor {
return {
OperationDefinition(node: OperationDefinitionNode) {
if (node.operation === 'subscription') {
if (node.selectionSet.selections.length !== 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* Walks the selection set and returns a list of selections where extra fields
* were selected, and selections where introspection fields were selected.
*/
function walkSubscriptionSelectionSet(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@benjie benjie Apr 25, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@benjie benjie Apr 25, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

(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.

src/validation/rules/SingleFieldSubscriptionsRule.js Outdated Show resolved Hide resolved
@benjie
Copy link
Member Author

benjie commented Apr 25, 2021

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.

@benjie benjie force-pushed the subscription-root-introspection-ban branch from 8177dbf to 9f718b8 Compare May 27, 2021 12:52
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

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.

@IvanGoncharov IvanGoncharov merged commit d5eac89 into main Jun 3, 2021
@IvanGoncharov IvanGoncharov deleted the subscription-root-introspection-ban branch June 3, 2021 17:51
@IvanGoncharov IvanGoncharov added PR: bug fix 🐞 requires increase of "patch" version number spec RFC Implementation of a proposed change to the GraphQL specification labels Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants