Forbid @skip and @include on subscription root selections#4396
Open
andimarek wants to merge 1 commit into
Open
Forbid @skip and @include on subscription root selections#4396andimarek wants to merge 1 commit into
andimarek wants to merge 1 commit into
Conversation
Contributor
Test ReportTest Results
Code Coverage (Java 25)
Changed Class Coverage (3 classes)
|
b4e60e2 to
a1879bd
Compare
a1879bd to
4e79ffd
Compare
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
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.
Summary
Fixes #3868.
This implements the merged spec change from graphql/graphql-spec#860: subscription root selections must not use
@skipor@include. This aligns GraphQL Java with graphql-js graphql/graphql-js#3974.Why this is different from the reverted implementation
The previous GraphQL Java implementation in #3871 was reverted by #3917 because it tried to coerce variables during validation and then used the execution
FieldCollector. That made validation depend on runtime-style variable handling, and it failed when validation did not have the complete execution input variables available.This implementation avoids that problem by not evaluating
@skipor@includefor this validation rule. It also does not introduce a separate subscription-only document traversal. Instead,OperationValidatornow reuses its existing operation-scoped traversal and fragment retraversal, the same path already used for operation-scoped rules such as variable usage, defer validation, and complexity tracking.The rule keeps small operation-local state while the existing traversal runs:
@skip/@includedirective AST nodes are collected syntacticallyThere is no
ValuesResolver.coerceVariableValuescall, no empty runtime variable map passed into execution collection, no executionFieldCollector, and no custom conditional runtime decision involved.That matches the reason the spec changed: validation must be able to determine subscription root fields without access to runtime variable values. It also catches cases the reverted implementation could miss, such as
@skip(if: true)or@include(if: false)on root subscription selections.Tests
Added Spock coverage for:
@skip/@includeusing variables@skip(if: true),@skip(if: false),@include(if: true), and@include(if: false)Added JMH coverage for:
Local verification:
./gradlew test --tests graphql.validation.SubscriptionUniqueRootFieldTest --tests graphql.execution.MergedSelectionSetTest./gradlew test --tests 'graphql.validation.*' --tests graphql.execution.MergedSelectionSetTest./gradlew jmhClasses test --tests graphql.archunit.JMHForkArchRuleTestLocal JMH Comparison
Ran on the same machine with JDK 25.0.2:
java -jar build/libs/*-jmh.jar -wi 2 -i 5 -w 2s -r 2s -f 1 -rf json -rff /tmp/graphql-java-pr-validation-jmh.json 'benchmark.(ValidatorBenchmark|SubscriptionValidationBenchmark).*'origin/masterin a temporary worktree with only the JMH benchmark patch appliedResults, ms/op lower is better:
SubscriptionValidationBenchmark.complexSubscriptionValidatorBenchmark.largeSchema1ValidatorBenchmark.largeSchema4ValidatorBenchmark.manyFragmentsRepeated the subscription benchmark with more samples:
java -jar build/libs/*-jmh.jar -wi 3 -i 8 -w 2s -r 2s -f 2 -rf json -rff /tmp/graphql-java-pr-subscription-jmh.json 'benchmark.SubscriptionValidationBenchmark.complexSubscription'SubscriptionValidationBenchmark.complexSubscriptionInterpretation: I do not see a general validation regression in the large-schema and many-fragment validation benchmarks. The synthetic complex subscription benchmark shows a small, measurable cost of about 0.010 ms/op for the new rule logic, which is the expected cost of tracking subscription root selections syntactically during the existing validation traversal instead of using execution field collection.