Consolidate validation rules into single OperationValidator and improve performance by andimarek · Pull Request #4228 · graphql-java/graphql-java

andimarek · 2026-01-27T23:41:51Z

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

Benchmark Results

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

Benchmark	master	this branch	Improvement
largeSchema1	0.020 ± 0.001 ms	0.012 ± 0.001 ms	40% faster
largeSchema4	8.448 ± 0.169 ms	6.936 ± 0.208 ms	18% faster
manyFragments	5.989 ± 0.127 ms	5.155 ± 0.099 ms	14% faster

Environment: JDK 25.0.1 (OpenJDK, Amazon Corretto), 2 forks, 2 warmup iterations (5s), 3 measurement iterations (10s). Both runs performed back-to-back on the same machine.

How the performance gains are achieved

The improvements come from two sources:

1. Single-traversal architecture — The consolidation itself eliminates redundant AST walks that 31 separate rule classes were performing, resulting in fewer visitor dispatches and better cache locality.

2. Targeted hot-path optimizations in OperationValidator — Async-profiler showed 46-56% of validation CPU time was spent in HashMap/LinkedHashMap operations. The following changes address this:

Linear scan instead of argumentMap() — Fields/directives typically have 1-5 arguments. Replaced LinkedHashMap creation + lookup with a simple for loop (findArgumentByName), eliminating the most frequent short-lived map allocation.
Single-pass groupByCommonParents() — Replaced two filterSet calls (each allocating an ImmutableSet) plus groupingBy (allocating a LinkedHashMap + ImmutableList.Builder per group) with a single loop that partitions abstract vs concrete types in-place.
Pre-sized maps and computeIfAbsent — Pre-sized LinkedHashMap in overlappingFieldsImpl using selection count. Replaced containsKey + put + get triple-lookup with single computeIfAbsent in overlappingFields_collectFieldsForField, cutting HashMap resize events.
HashSet for fragment tracking — Changed allUsedFragments from ArrayList<String> (O(n) contains) to HashSet<String> (O(1)), and combined contains + add into a single add() call using its return value.
ArrayList replaces LinkedList in buildTransitiveSpreads — Eliminated per-element LinkedList$Node allocations. Switched from add(0, child) / peekFirst() to add(child) / get(size()-1).
isRuleEnabled short-circuit — Added a boolean allRulesEnabled flag computed once in the constructor. When all rules are enabled (the common case), isRuleEnabled() returns true immediately without predicate dispatch, avoiding lambda allocation on every call.

Test Coverage

Summary

	master	this PR	delta
Validation tests	316	311	-5
Total project tests	5,366	5,361	-5

The net -5 comes from two test files:

ArgumentsOfCorrectTypeTest: 21 → 17 (-4)
FieldsOnCorrectTypeTest: 3 → 2 (-1)

All removed tests were mock-based unit tests that tested internal rule classes via mocked ValidationContext. They have been accounted for as detailed below.

ArgumentsOfCorrectTypeTest (-4 net)

9 mock-based tests removed; 5 integration tests added.

Replaced (5 → 5):

Removed (master)	Replaced by (this PR)
`"invalid input object type results in error"`	`"invalid input object field type results in error"`
`"invalid list object type results in error"`	`"invalid list of input objects results in error"`
`"invalid list inside object type results in error"`	`"invalid nested list inside input object results in error"`
`"invalid list simple type results in error"`	`"invalid simple list type results in error"`
`"type null fields results in error"`	`"null value for non-null field in input object results in error"`

Each replacement tests the same scenario (same GraphQL input pattern, same expected WrongType error) but uses Validator.validateDocument() against a real schema instead of calling checkArgument() on a mocked rule.

Already covered by retained "with message" variants (3 tests):

Removed	Retained test covering same scenario
`"type missing fields results in error"`	`"type missing fields results in error with message"` — same query, stricter message assertion
`"type not object results in error"`	`"invalid not object type results in error with message"` — same query, stricter message assertion
`"type with extra fields results in error"`	`"type with extra fields results in error with message"` — same query, stricter message assertion

Architecture-specific, not applicable (1 test):

Removed	Why no gap
`"current null argument from context is no error"`	Tests null-guard via mocked `ValidationContext.getArgument()` returning null. Guard retained in `OperationValidator`, but unreachable through public `Validator.validateDocument()` API.

FieldsOnCorrectTypeTest (-1 net)

3 mock-based tests removed; 2 integration tests added.

Replaced (2 → 2):

Removed (master)	Replaced by (this PR)
`"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"`

Architecture-specific, not applicable (1 test):

Removed	Why no gap
`"should results in no error when parent type is null"`	Tests null-guard via mocked context. Guard retained in `OperationValidator`, but unreachable through public API.

Conclusion

All 10 removed tests are accounted for: 7 replaced with integration tests covering the same scenarios via the public API, 3 covered by existing retained tests with stricter assertions, and 2 architecture-specific (null-guards on mocked internals not exercisable through the public API). No validation behavior is left untested.

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

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 <[email protected]>

github-actions · 2026-01-27T23:44:00Z

Test Results

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

Results for commit 051f73d. ± Comparison against base commit bd87652.

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.

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 <[email protected]>

…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 <[email protected]>

- 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 <[email protected]>

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 <[email protected]>

…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 <[email protected]>

- 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 <[email protected]>

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.

Replace HashMap/LinkedHashMap allocations with linear scans for small argument lists, single-pass partitioning in groupByCommonParents, pre-sized maps, computeIfAbsent, HashSet for fragment tracking, and ArrayList over LinkedList. Short-circuit isRuleEnabled when all rules are enabled. ~12% improvement on ValidatorBenchmark (largeSchema4: 7.86→6.94ms, manyFragments: 5.83→5.16ms). Co-Authored-By: Claude Opus 4.6 <[email protected]>

Copilot

Pull request overview

This PR consolidates 31 separate validation rule classes into a single OperationValidator class, introducing a new @PublicApi enum OperationValidationRule for type-safe rule filtering. This is a significant refactoring that:

Removes ~1,400 lines of code while preserving all validation behavior
Changes the rule filtering API from Predicate<Class<?>> to Predicate<OperationValidationRule>
Achieves 14-40% performance improvement via single-traversal architecture and targeted optimizations
Maintains comprehensive test coverage with integration tests replacing mock-based unit tests

Changes:

Introduces OperationValidationRule enum with 31 values (one per validation rule)
Consolidates all validation logic into single OperationValidator class
Updates test files from graphql.validation.rules package to graphql.validation package
Adds JSpec null safety annotations to ValidationContext and TraversalContext
Updates ParseAndValidate, GraphQL, and Validator APIs to use new enum-based predicate

Reviewed changes

Copilot reviewed 85 out of 85 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`OperationValidationRule.java`	New @publicapi enum defining all validation rules with comprehensive javadoc
`Validator.java`	Simplified to create single OperationValidator instead of 31 rule instances
`ValidationContext.java`	Added @NullMarked and @nullable annotations for null safety
`TraversalContext.java`	Added @NullMarked and null safety annotations, defensive null checks
`VariablesTypesMatcher.java`	Package moved from rules to validation
`ParseAndValidate.java`	Updated predicate type and added @nonnull annotations
`GraphQL.java`	Updated predicate type for validation rule filtering
Test files	Migrated from rules package, mock tests replaced with integration tests
Benchmark files	Updated to use new OperationValidator API

Copilot · 2026-02-15T11:16:39Z

src/main/java/graphql/validation/TraversalContext.java

        GraphQLInputType inputType = null;
        GraphQLInputObjectField inputField = null;
-        if (objectType instanceof GraphQLInputObjectType) {
+        if (objectType instanceof GraphQLInputObjectType && schema.getCodeRegistry() != null) {


The null check schema.getCodeRegistry() != null is added but CodeRegistry is never null in a properly constructed GraphQLSchema. This check appears unnecessary and may indicate defensive programming for an impossible case. Consider removing this check or documenting why it's needed.

Suggested change

if (objectType instanceof GraphQLInputObjectType && schema.getCodeRegistry() != null) {

if (objectType instanceof GraphQLInputObjectType) {

Copilot · 2026-02-15T11:16:40Z

src/main/java/graphql/validation/TraversalContext.java

            return schema.getIntrospectionTypenameFieldDefinition();
        }
-        if (parentType instanceof GraphQLFieldsContainer) {
+        if (parentType instanceof GraphQLFieldsContainer && schema.getCodeRegistry() != null) {


The null check schema.getCodeRegistry() != null is added but CodeRegistry is never null in a properly constructed GraphQLSchema. This check appears unnecessary and may indicate defensive programming for an impossible case. Consider removing this check or documenting why it's needed.

Suggested change

if (parentType instanceof GraphQLFieldsContainer && schema.getCodeRegistry() != null) {

if (parentType instanceof GraphQLFieldsContainer) {

GraphQLSchema.getCodeRegistry() is no longer @nullable, so these guards are unnecessary. Co-Authored-By: Claude Opus 4.6 <[email protected]>

andimarek added the breaking change requires a new major version to be relased label Jan 27, 2026

andimarek and others added 8 commits January 28, 2026 10:10

andimarek requested a review from Copilot February 15, 2026 11:13

Copilot started reviewing on behalf of andimarek February 15, 2026 11:13 View session

Copilot AI reviewed Feb 15, 2026

View reviewed changes

andimarek changed the title ~~Consolidate validation rules into single OperationValidator~~ Consolidate validation rules into single OperationValidator and improve performance Feb 15, 2026

andimarek and others added 2 commits February 15, 2026 21:32

Merge branch 'master' into validation-refactor

c0b4b1c

Remove unnecessary null checks on getCodeRegistry()

051f73d

GraphQLSchema.getCodeRegistry() is no longer @nullable, so these guards are unnecessary. Co-Authored-By: Claude Opus 4.6 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate validation rules into single OperationValidator and improve performance#4228

Consolidate validation rules into single OperationValidator and improve performance#4228
andimarek wants to merge 11 commits intomasterfrom
validation-refactor

andimarek commented Jan 27, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Jan 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 15, 2026

Uh oh!

Copilot AI Feb 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	if (objectType instanceof GraphQLInputObjectType && schema.getCodeRegistry() != null) {
	if (objectType instanceof GraphQLInputObjectType) {

	if (parentType instanceof GraphQLFieldsContainer && schema.getCodeRegistry() != null) {
	if (parentType instanceof GraphQLFieldsContainer) {

Conversation

andimarek commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Change

Summary

Performance

Benchmark Results

How the performance gains are achieved

Test Coverage

Summary

ArgumentsOfCorrectTypeTest (-4 net)

FieldsOnCorrectTypeTest (-1 net)

Conclusion

Test plan

Uh oh!

github-actions bot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

andimarek commented Jan 27, 2026 •

edited

Loading

github-actions bot commented Jan 27, 2026 •

edited

Loading