GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Consolidate validation rules into single OperationValidator and improve performance by andimarek · Pull Request #4228 · graphql-java/graphql-java Conversation
|
| 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.
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.
| 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.
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. |
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. |
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.
./gradlew test -x testWithJava11 -x testWithJava17 -x testWithJava21)./gradlew test --tests "graphql.validation.*")🤖 Generated with Claude Code
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
There was an error while loading. Please reload this page.
Test Results 335 files ±0 335 suites ±0 5m 9s ⏱️ +5s Results for commit 072165b. ± Comparison against base commit bd87652. This pull request removes 449 and adds 430 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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:
Predicate<Class<?>> to Predicate<OperationValidationRule>Changes:
OperationValidationRule enum with 31 values (one per validation rule)OperationValidator classgraphql.validation.rules package to graphql.validation packageValidationContext and TraversalContextParseAndValidate, GraphQL, and Validator APIs to use new enum-based predicateCopilot reviewed 85 out of 85 changed files in this pull request and generated 2 comments.
| 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| GraphQLInputType inputType = null; | ||
| GraphQLInputObjectField inputField = null; | ||
| if (objectType instanceof GraphQLInputObjectType) { | ||
| if (objectType instanceof GraphQLInputObjectType && schema.getCodeRegistry() != null) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
| if (objectType instanceof GraphQLInputObjectType && schema.getCodeRegistry() != null) { | |
| if (objectType instanceof GraphQLInputObjectType) { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return schema.getIntrospectionTypenameFieldDefinition(); | ||
| } | ||
| if (parentType instanceof GraphQLFieldsContainer) { | ||
| if (parentType instanceof GraphQLFieldsContainer && schema.getCodeRegistry() != null) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
| if (parentType instanceof GraphQLFieldsContainer && schema.getCodeRegistry() != null) { | |
| if (parentType instanceof GraphQLFieldsContainer) { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
GraphQLSchema.getCodeRegistry() is no longer @nullable, so these guards are unnecessary. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Keep the original behavior of passing locale through as-is from ExecutionInput.getLocale() without falling back to Locale.getDefault(). Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Remove unused import (GraphQLCodeRegistry) - Remove unused method parameters (checkSelectionSet, leaveOperationDefinition, leaveSelectionSet, documentFinished, validateUniqueArgumentNames_directive) - Remove always-false null check (validationContext in isExperimentalApiKeyEnabled) - Remove redundant @SuppressWarnings("deprecation") - Remove always-false value == null check in validateProvidedNonNullArguments - Simplify typeA/typeB null check (typeA never null in context) - Rename sameType -> notSameType, doTypesOverlap -> typesDoNotOverlap (always inverted) - Extract common validateUniqueArgumentNames to deduplicate field/directive variants - Convert indexed for-loop to enhanced for-each - Simplify ifArgumentMightBeFalse return - Remove dead query-type branch (schema always has query type) - Add null safety for getSourceLocation() and StringValue.getValue() - Add @nullable to GraphQLSchema.getDirective() (can return null per javadoc) Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.