Skip to content

Consolidate validation rules into single OperationValidator#4228

Open
andimarek wants to merge 8 commits intomasterfrom
validation-refactor
Open

Consolidate validation rules into single OperationValidator#4228
andimarek wants to merge 8 commits intomasterfrom
validation-refactor

Conversation

@andimarek
Copy link
Member

@andimarek andimarek commented Jan 27, 2026 •
edited
Loading

Breaking Change

The rule filtering predicate type has changed from Predicate<Class<?>> to Predicate<OperationValidationRule> in the following public APIs:

  • Validator.validateDocument(schema, document, rulePredicate, locale)
  • ParseAndValidate.parseAndValidate(...) overloads accepting a predicate
  • GraphQL internal validation predicate hint

Code that filters validation rules by class reference (e.g., rule -> rule != NoUnusedFragments.class) must migrate to the new OperationValidationRule enum (e.g., rule -> rule != OperationValidationRule.NO_UNUSED_FRAGMENTS).

Additionally, the following @Internal classes have been removed:

  • AbstractRule
  • RulesVisitor
  • All 31 individual rule classes in graphql.validation.rules.* (except VariablesTypesMatcher)

Summary

  • Consolidate 31 separate validation rule classes, AbstractRule, and RulesVisitor into a single OperationValidator class that implements DocumentVisitor directly
  • Introduce OperationValidationRule enum (@PublicApi) with one value per validation rule for type-safe rule filtering
  • Simplify Validator to create a single OperationValidator instead of 31 rule instances
  • Net deletion of ~1,400 lines of code (63 files changed, +2,247 / -3,644)
  • All existing validation behavior and test coverage preserved

Performance

JMH benchmark results comparing master vs this branch (ValidatorBenchmark, avg ms/op, lower is better):

Benchmark master this branch Change
largeSchema1 0.019 ± 0.001 ms 0.014 ± 0.001 ms -26%
largeSchema4 8.826 ± 0.685 ms 7.675 ± 0.153 ms -13%
manyFragments 5.880 ± 0.044 ms 5.738 ± 0.044 ms -2.4%

Environment: JDK 25.0.1 (OpenJDK, Amazon Corretto), 2 forks, 2 warmup iterations (5s), 3 measurement iterations (10s).

The single-traversal approach avoids redundant AST walks that the 31 separate rule classes were performing, resulting in consistent improvements across all scenarios.

Test plan

  • Full test suite passes (./gradlew test -x testWithJava11 -x testWithJava17 -x testWithJava21)
  • All validation tests pass (./gradlew test --tests "graphql.validation.*")
  • JSpecify annotation check passes
  • JMH benchmarks updated and compile
  • CI pipeline passes

🤖 Generated with Claude Code

@andimarek @claude
Consolidate 31 validation rule classes into single OperationValidator
8884d2d 
Replace the AbstractRule + RulesVisitor + 31 individual rule class
architecture with a single OperationValidator class that implements
DocumentVisitor directly.

BREAKING CHANGE: The rule filtering predicate type changes from
Predicate<Class<?>> to Predicate<OperationValidationRule> in
Validator, ParseAndValidate, and GraphQL. Code that filters
validation rules by class reference must migrate to the new
OperationValidationRule enum.

- Create OperationValidationRule enum with 31 values (one per rule)
- Create OperationValidator implementing DocumentVisitor with all
  validation logic consolidated from the 31 rule classes
- Simplify Validator to create OperationValidator directly
- Update ParseAndValidate and GraphQL predicate types
- Delete AbstractRule, RulesVisitor, and all 31 rule classes
  (keep VariablesTypesMatcher as utility)
- Convert all test files from mock-based rule instantiation to
  integration tests or OperationValidator with rule predicates
- Update JMH benchmarks

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@andimarek andimarek added the breaking change requires a new major version to be relased label Jan 27, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 27, 2026 •
edited
Loading

Test Results

  335 files  ±0    335 suites  ±0   5m 5s ⏱️ ±0s
5 366 tests  - 5  5 358 ✅  - 4  8 💤  - 1  0 ❌ ±0 
5 455 runs   - 5  5 447 ✅  - 4  8 💤  - 1  0 ❌ ±0 

Results for commit 23dd3db. ± Comparison against base commit 2f0dc5b.

This pull request removes 449 and adds 420 tests. Note that renamed tests count towards both. 
	?

	, expected: combo-\"\\\b\f\n\r\t, #4]
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
…

graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure23@1e60b459 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure24@7c0777b5 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure25@29ebbdf4 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure20@6ba060f3 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure21@411a5965 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure22@18a25bbd delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure3@5a0bef24 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure4@17092fff delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure5@22c75c01 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of error args with non null does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_6prov0_closure6@63d5874f delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
…

♻️ This comment has been updated with latest results.

@andimarek @claude
Restore 10 test coverage gaps lost during validation refactor
b3e3f3e 
Add integration tests replacing removed mock-based tests:
- Per-operation fragment visiting (RulesVisitorTest)
- Invalid input object field, list, nested list, and simple list types (ArgumentsOfCorrectTypeTest)
- Null value for non-null input object field (ArgumentsOfCorrectTypeTest)
- Unknown type on inline fragment not triggering composite type error (FragmentsOnCompositeTypeTest)
- Directive argument scope isolation (KnownArgumentNamesTest)
- Defaulted non-null field and directive arguments (ProvidedNonNullArgumentsTest)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@andimarek
Copy link
Member Author

andimarek commented Jan 28, 2026

Test Coverage Analysis: All 15 Removed Tests Accounted For

The consolidation commit removed 15 mock-based tests. The latest commit restores 10 as integration tests. Here's the full accounting:

Restored (10 tests)

# File Removed Test Restored As
1 RulesVisitorTest "RulesVisitor visits fragment definition with isVisitFragmentSpread rules once per operation" "OperationValidator visits fragment definitions per-operation for fragment-spread rules"
2 ArgumentsOfCorrectTypeTest "invalid input object type results in error" "invalid input object field type results in error"
3 ArgumentsOfCorrectTypeTest "invalid list object type results in error" "invalid list of input objects results in error"
4 ArgumentsOfCorrectTypeTest "invalid list inside object type results in error" "invalid nested list inside input object results in error"
5 ArgumentsOfCorrectTypeTest "invalid list simple type results in error" "invalid simple list type results in error"
6 ArgumentsOfCorrectTypeTest "type null fields results in error" "null value for non-null field in input object results in error"
7 FragmentsOnCompositeTypeTest "should results in no error" (StrangeType case) "unknown type on inline fragment should not trigger composite type error"
8 KnownArgumentNamesTest "directive argument not validated against field arguments" "directive argument not validated against field arguments"
9 ProvidedNonNullArgumentsTest "not provided and but defaulted non null field argument" "not provided but defaulted non null field argument is not an error"
10 ProvidedNonNullArgumentsTest "not provided but defaulted directive argument" "not provided but defaulted directive argument is not an error"

Already covered by existing integration tests (3 tests — no gap)

# Removed Test Existing Equivalent
11 "type missing fields results in error" "type missing fields results in error with message" — same scenario (myObject: { id: "1" } missing required name)
12 "type not object results in error" "invalid not object type results in error with message" — same scenario (myObject: 1)
13 "type with extra fields results in error" "type with extra fields results in error with message" — same scenario (extraField: "ShouldNotBeHere")

Not applicable to new architecture (2 tests — no gap)

# Removed Test Why no gap
14 "current null argument from context is no error" Defensive null-safety check for the old mock ValidationContext.getArgument() returning null. In OperationValidator, argument definitions are resolved directly from the schema — this null state can't occur in practice.
15 "should results in no error when parent type is null" Defensive null-guard for ValidationContext.getParentType() returning null. In OperationValidator, parent type tracking is managed internally and is always set during traversal.

15 removed, 15 accounted for. No remaining coverage gaps.

@andimarek
Copy link
Member Author

andimarek commented Jan 28, 2026

Correction: Full Accounting of All 23 Removed Tests

The previous comment only covered 15 tests. Here's the complete breakdown of all 23 mock-based tests removed in the consolidation commit.

Refactored in the same commit (8 tests — renamed/rewritten as integration tests)

Removed Replaced By
"all directive arguments are provided" "all directive arguments are provided results in no error"
"all field arguments are provided" "all field arguments are provided results in no error"
"default value has wrong type" "default value has wrong type" (same name, rewritten as integration test)
"known directive argument" "known directive argument results in no error"
"not provided and not defaulted non null field argument" "not provided non null field argument results in error"
"not provided not defaulted directive argument" "not provided not defaulted directive argument results in error"
"should add error to collector when field definition is null" "should add error when field is undefined on type"
"should results in no error when field definition is filled" "should result in no error when field is defined"

Partially refactored (1 parameterized test — 2 of 3 cases replaced)

Removed Replaced By
"should results in no error" (case: no type condition) "should result in no error for inline fragment without type condition"
"should results in no error" (case: composite type) "should result in no error for inline fragment with composite type condition"
"should results in no error" (case: StrangeType) Not replaced → Gap 7, now restored

Restored by latest commit (10 test scenarios)

# File Removed Test Restored As
1 RulesVisitorTest "RulesVisitor visits fragment definition with isVisitFragmentSpread rules once per operation" "OperationValidator visits fragment definitions per-operation for fragment-spread rules"
2 ArgumentsOfCorrectTypeTest "invalid input object type results in error" "invalid input object field type results in error"
3 ArgumentsOfCorrectTypeTest "invalid list object type results in error" "invalid list of input objects results in error"
4 ArgumentsOfCorrectTypeTest "invalid list inside object type results in error" "invalid nested list inside input object results in error"
5 ArgumentsOfCorrectTypeTest "invalid list simple type results in error" "invalid simple list type results in error"
6 ArgumentsOfCorrectTypeTest "type null fields results in error" "null value for non-null field in input object results in error"
7 FragmentsOnCompositeTypeTest "should results in no error" (StrangeType case) "unknown type on inline fragment should not trigger composite type error"
8 KnownArgumentNamesTest "directive argument not validated against field arguments" "directive argument not validated against field arguments"
9 ProvidedNonNullArgumentsTest "not provided and but defaulted non null field argument" "not provided but defaulted non null field argument is not an error"
10 ProvidedNonNullArgumentsTest "not provided but defaulted directive argument" "not provided but defaulted directive argument is not an error"

Already covered by existing integration tests (3 tests — no gap)

# Removed Test Existing Equivalent
11 "type missing fields results in error" "type missing fields results in error with message" — same scenario
12 "type not object results in error" "invalid not object type results in error with message" — same scenario
13 "type with extra fields results in error" "type with extra fields results in error with message" — same scenario

Not applicable to new architecture (2 tests — no gap)

# Removed Test Why no gap
14 "current null argument from context is no error" Defensive null-safety check for old mock ValidationContext.getArgument(). In OperationValidator, argument definitions are resolved directly from the schema — this null state can't occur.
15 "should results in no error when parent type is null" Defensive null-guard for old mock ValidationContext.getParentType(). In OperationValidator, parent type tracking is managed internally and always set during traversal.

Summary

Category Count
Refactored in consolidation commit 8 + 2 cases of parameterized test
Restored by latest commit 10 (including 1 missing case from parameterized test)
Already covered by existing tests 3
Architecture-specific, not applicable 2
Total 23

23 removed, 23 accounted for. No remaining coverage gaps.

andimarek and others added 6 commits January 28, 2026 10:58
@andimarek @claude
Move VariablesTypesMatcher and all validation tests from rules sub-pa…
1f6d39d 
…ckage to validation package

The rules sub-package is no longer needed since all 31 rule classes
were consolidated into OperationValidator. Move the remaining utility
class (VariablesTypesMatcher) and all 32 test files into the parent
graphql.validation package, then delete the empty rules directories.

Also remove stale JSpecifyAnnotationsCheck entries for deleted rule
classes and update comments referencing the old package.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@andimarek @cursoragent
Improve OperationValidator documentation and naming
142bad7 
- Rename methods for clarity:
  - shouldRunNonFragmentSpreadChecks() -> shouldRunDocumentLevelRules()
  - shouldRunFragmentSpreadChecks() -> shouldRunOperationScopedRules()
  - fragmentSpreadVisitDepth -> fragmentRetraversalDepth

- Add comprehensive class Javadoc explaining:
  - Two independent state variables and their three valid combinations
  - Visual matrices showing when each rule category runs
  - Step-by-step traversal example with state at each step

- Add detailed Javadoc to OperationValidationRule enum categorizing
  all rules by their traversal behavior

- Simplify code by removing duplicate checks and redundant comments

Co-authored-by: Cursor <cursoragent@cursor.com>
@andimarek @cursoragent
Remove redundant shouldRunDocumentLevelRules() checks
72b2828 
These checks are always true in methods that are only called at
document level (depth=0):
- checkDocument(): Document node visited once at start
- checkOperationDefinition(): Operations are never inside fragments
- checkVariableDefinition(): Variable definitions only exist in operations
- documentFinished(): Called when leaving Document

Co-authored-by: Cursor <cursoragent@cursor.com>
@andimarek @cursoragent
Add JSpecify @NullMarked annotations to ValidationContext and Operati…
e0c5f0b 
…onValidator

ValidationContext:
- Add @NullMarked at class level
- Add @nullable to methods that can return null based on traversal state:
  getFragment, getParentType, getInputType, getDefaultValue, getFieldDef,
  getDirective, getArgument, getOutputType, getQueryPath

OperationValidator:
- Add @NullMarked at class level
- Add @nullable to methods that can return null:
  getQueryPath, requireSameNameAndArguments, findArgumentByName,
  requireSameOutputTypeShape
- Add @nullable to parameters that can be null:
  addError (SourceLocation), mkNotSameTypeError (typeA, typeB),
  overlappingFieldsImpl, overlappingFields_collectFields,
  overlappingFields_collectFieldsForInlineFragment,
  overlappingFields_collectFieldsForField, sameType, sameArguments
- Add @nullable to fields that can be null:
  variableDefinitionMap, FieldAndType.graphQLType

Co-authored-by: Cursor <cursoragent@cursor.com>
@andimarek @cursoragent
Add JSpecify @NullMarked annotations to TraversalContext
b65e8c3 
- Add @NullMarked at class level
- Add @nullable to fields: directive, argument
- Add @nullable to public methods that can return null:
  getOutputType, getParentType, getInputType, getDefaultValue,
  getFieldDef, getQueryPath, getDirective, getArgument
- Add @nullable to private methods that can return null:
  lastElement, find, getFieldDef(schema, parentType, field),
  getNullableType
- Add @nullable to parameters that accept null:
  addOutputType, addParentType, addInputType, addDefaultValue,
  addFieldDef, getNullableType

Co-authored-by: Cursor <cursoragent@cursor.com>
@andimarek
Merge origin/master into validation-refactor
23dd3db 
Fix NullAway errors from master's @NullMarked additions to
GraphQLSchema and GraphQLTypeUtil. Add null checks in
OperationValidator and TraversalContext for nullable
getCodeRegistry(), unwrapAll(), and type comparison methods.

Update JSpecifyAnnotationsCheck exemption list to match master's
annotated classes, removing deleted DeferDirective rule entries.
Add @NullMarked to ParseAndValidate to match master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

breaking change requires a new major version to be relased

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@andimarek