New spec validation: Subscriptions root field must not contain @skip nor @include on root selection set #3871
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dondonz
commented
Mar 29, 2025
| @@ -27,7 +27,7 @@ | |||
| import graphql.validation.rules.PossibleFragmentSpreads; | |||
| import graphql.validation.rules.ProvidedNonNullArguments; | |||
| import graphql.validation.rules.ScalarLeaves; | |||
| import graphql.validation.rules.SubscriptionUniqueRootField; | |||
| import graphql.validation.rules.SubscriptionRootField; | |||
There was a problem hiding this comment.
I changed this rule name to be more generic, reflecting that it does 2 checks on the root field
dondonz
commented
Mar 29, 2025
| @@ -39,16 +47,24 @@ public void checkOperationDefinition(OperationDefinition operationDef) { | |||
|
|
|||
| GraphQLObjectType subscriptionType = getValidationContext().getSchema().getSubscriptionType(); | |||
|
|
|||
| // This coercion takes into account default values for variables | |||
There was a problem hiding this comment.
This is fixing an edge case - this check previously didn't take into account default variable values because there was previously no coercion.
That caused my tests to fail (see later) because I had set a default boolean value for include and skip.
dondonz
commented
Mar 29, 2025
| .objectType(subscriptionType) | ||
| .graphQLContext(getValidationContext().getGraphQLContext()) | ||
| .build(); | ||
|
|
||
| MergedSelectionSet fields = fieldCollector.collectFields(collectorParameters, operationDef.getSelectionSet()); | ||
| List<Selection> subscriptionSelections = operationDef.getSelectionSet().getSelections(); |
dondonz
commented
Mar 29, 2025
|
|
||
| private boolean hasSkipOrIncludeDirectives(MergedField field) { | ||
| List<Directive> directives = field.getSingleField().getDirectives(); | ||
| for (Directive directive : directives) { |
There was a problem hiding this comment.
Given recent performance improvements I am using plain for loops
dondonz
commented
Mar 29, 2025
| @@ -68,9 +68,8 @@ ScalarLeaves.subselectionOnLeaf=Validation error ({0}) : Subselection not allowe | |||
| ScalarLeaves.subselectionRequired=Validation error ({0}) : Subselection required for type ''{1}'' of field ''{2}'' | |||
| # | |||
| SubscriptionUniqueRootField.multipleRootFields=Validation error ({0}) : Subscription operation ''{1}'' must have exactly one root field | |||
| SubscriptionUniqueRootField.multipleRootFieldsWithFragment=Validation error ({0}) : Subscription operation ''{1}'' must have exactly one root field with fragments | |||
Test Results 312 files 312 suites 45s ⏱️ Results for commit 0219c47. ♻️ This comment has been updated with latest results. |
dondonz
commented
Mar 29, 2025
| SubscriptionIntrospectionRootField.introspectionRootField=Validierungsfehler ({0}) : Subscription operation ''{1}'' root field ''{2}'' kann kein introspection field sein | ||
| SubscriptionIntrospectionRootField.introspectionRootFieldWithFragment=Validierungsfehler ({0}) : Subscription operation ''{1}'' fragment root field ''{2}'' kann kein introspection field sein | ||
| SubscriptionRootField.forbidSkipAndIncludeOnSubscriptionRoot=Validierungsfehler ({0}) : Subscription operation ''{1}'' root field ''{2}'' darf weder @skip noch @include directive in top level selection | ||
| # |
dondonz
commented
Mar 29, 2025
| def "valid subscription with @skip and @include directives on subfields"() { | ||
| given: | ||
| def query = """ | ||
| subscription MySubscription(\$bool: Boolean = true) { |
There was a problem hiding this comment.
This is what I mean by default values. When an empty coerced variables map is passed, this test fails
dondonz
commented
Mar 29, 2025
| SubscriptionIntrospectionRootField.introspectionRootField=Validatiefout ({0}) : Subscription operation ''{1}'' root field ''{2}'' kan geen introspectieveld zijn | ||
| SubscriptionIntrospectionRootField.introspectionRootFieldWithFragment=Validatiefout ({0}) : Subscription operation ''{1}'' fragment root field ''{2}'' kan geen introspectieveld zijn | ||
| SubscriptionRootField.forbidSkipAndIncludeOnSubscriptionRoot=Validation error ({0}) : Subscription operation ''{1}'' root field ''{2}'' must not use @skip nor @include directives in top level selection |
bbakerman
approved these changes
Mar 31, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3868 and implements graphql/graphql-spec#860
We had an existing validation for subscription root fields. This adds on a new requirement that subscription root fields can't contain the
@includenor@skipdirectives. See the spec link for more.This could be considered a "breaking change" but it is more correctly a new validation requirement.