Skip to content

Forbid @skip and @include on subscription root selections#4396

Open
andimarek wants to merge 1 commit into
masterfrom
fix-3868-subscription-root-skip-include
Open

Forbid @skip and @include on subscription root selections#4396
andimarek wants to merge 1 commit into
masterfrom
fix-3868-subscription-root-skip-include

Conversation

@andimarek
Copy link
Copy Markdown
Member

@andimarek andimarek commented May 20, 2026

Summary

Fixes #3868.

This implements the merged spec change from graphql/graphql-spec#860: subscription root selections must not use @skip or @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 @skip or @include for this validation rule. It also does not introduce a separate subscription-only document traversal. Instead, OperationValidator now 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:

  • depth-1 subscription fields are collected as root fields
  • root-level @skip / @include directive AST nodes are collected syntactically
  • fragment definitions are interpreted from each operation spread site using the existing fragment retraversal context
  • fragment summaries are cached only to cover the existing optimization where a fragment already visited elsewhere in the operation is not traversed again

There is no ValuesResolver.coerceVariableValues call, no empty runtime variable map passed into execution collection, no execution FieldCollector, 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:

  • root subscription fields with @skip / @include using variables
  • literal @skip(if: true), @skip(if: false), @include(if: true), and @include(if: false)
  • one validation error containing both directive locations when both directives appear at the subscription root
  • anonymous subscriptions
  • root fragment spreads and inline fragments
  • root fields reached through fragments
  • recursive fragments
  • cached fragment-summary behavior when a fragment is first traversed below a root field and later spread at the subscription root
  • subfield directives remaining valid
  • query and mutation root directives remaining valid
  • aliased introspection root field validation
  • interface and union type-condition cases for subscription root inline fragments

Added JMH coverage for:

  • complex subscription validation with many root fragments and nested fragments
  • existing general validation benchmark harness now returns validation results instead of discarding them and no longer depends on execution success during setup

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

Local 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).*'
  • same command against origin/master in a temporary worktree with only the JMH benchmark patch applied

Results, ms/op lower is better:

Benchmark master this PR delta
SubscriptionValidationBenchmark.complexSubscription 0.215 0.228 +5.7%
ValidatorBenchmark.largeSchema1 0.012 0.012 -0.5%
ValidatorBenchmark.largeSchema4 4.224 4.280 +1.3%
ValidatorBenchmark.manyFragments 1.363 1.380 +1.2%

Repeated 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'
Benchmark master this PR delta
SubscriptionValidationBenchmark.complexSubscription 0.216 +/- 0.002 0.226 +/- 0.002 +4.7%

Interpretation: 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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 11 5891 (+26 🟢) 5835 (+26 🟢) 0 (±0) 0 (±0) 56 (±0)
Java 17 5891 (+26 🟢) 5834 (+26 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 21 5891 (+26 🟢) 5834 (+26 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 25 5891 (+26 🟢) 5834 (+26 🟢) 0 (±0) 0 (±0) 57 (±0)
jcstress 32 (±0) 32 (±0) 0 (±0) 0 (±0) 0 (±0)
Total 23596 (+104 🟢) 23369 (+104 🟢) 0 (±0) 0 (±0) 227 (±0)

Code Coverage (Java 25)

Metric Covered Missed Coverage vs Master
Lines 29809 3127 90.5% +0.1% 🟢
Branches 8833 1531 85.2% +0.2% 🟢
Methods 7938 1208 86.8% ±0.0%

Changed Class Coverage (3 classes)

Class Line Branch Method
g.e.MergedSelectionSet +8.3% 🟢 ±0.0% +11.1% 🟢
g.v.OperationValidator +0.4% 🟢 +1.0% 🟢 +0.2% 🟢
g.v.ValidationError
$Builder
±0.0% +50.0% 🟢 ±0.0%

Full HTML report: build artifact jacoco-html-report

Updated: 2026-05-21 00:11:21 UTC

@andimarek andimarek force-pushed the fix-3868-subscription-root-skip-include branch 2 times, most recently from b4e60e2 to a1879bd Compare May 20, 2026 23:09
@andimarek andimarek force-pushed the fix-3868-subscription-root-skip-include branch from a1879bd to 4e79ffd Compare May 21, 2026 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tracking spec change: Prevent @skip and @include on root subscription selection set

1 participant