GeezerVison is showing github.com — Fix overlapping fields null type regression from 072165b by andimarek · Pull Request #4291 · graphql-java/graphql-java
Conversation
andimarek
commented
Mar 4, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Summary
Supersedes #4289 (which had an incorrect fix and a failing test).
Fixes a regression introduced in commit 072165b ("Clean up OperationValidator") where the null-handling logic in requireSameOutputTypeShape() was incorrectly simplified, causing the benchmarkDeepAbstractConcrete JMH benchmark to crash with spurious FieldsConflict errors.
Root cause
In OperationValidator.requireSameOutputTypeShape(), the null check was simplified from a skip-on-null to an error-on-null:
// Before (072165b) - always errors when typeB is null
if (typeB == null) {
return mkNotSameTypeError(path, fields, typeA, typeB);
}The benchmarkDeepAbstractConcrete benchmark uses fragments on Whatever (a type that doesn't exist in the schema). This produces null graphQLType entries alongside non-null entries (e.g., Abstract from interface inline fragments). The broken code reports 'field/field/field' : returns different types 'Abstract' and 'null', which causes the benchmark's assertTrue(errors.size() == 0) assertion to fail.
Fix
When either type is null (meaning the parent type was unresolvable), skip the comparison entirely:
if (typeA == null || typeB == null) {
continue;
}A null type means "we couldn't resolve this field's type" — it doesn't mean "this field has a conflicting type". Comparing a known type against an unknown type is meaningless, so we skip it.
Test plan
- New test reproduces the exact
benchmarkDeepAbstractConcrete scenario and passes
- All 33 tests in
OverlappingFieldsCanBeMergedTest pass
- Full test suite passes with no regressions
https://claude.ai/code/session_01MZkhqChmheW6d46H4p8T7Y
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The requireSameOutputTypeShape() null check in OperationValidator was
incorrectly simplified in commit 072165b. When a FieldAndType set
contains both null-typed entries (from unresolvable parent types) and
non-null entries (from interface inline fragments), the refactored code
reports a spurious FieldsConflict error instead of skipping the
comparison. This breaks the benchmarkDeepAbstractConcrete JMH benchmark.
https://claude.ai/code/session_01XYg9Dqneb941aStgKaZ1Em
Restore the original null-handling logic that was incorrectly simplified
in 072165b. When both typeA and typeB are null (e.g., from fragments on
unresolvable types), they should be treated as compatible rather than
producing a spurious FieldsConflict error.
https://claude.ai/code/session_01XYg9Dqneb941aStgKaZ1Em
…regression
The original test used mixed null and non-null field types which correctly
produces a FieldsConflict error. The fix in requireSameOutputTypeShape handles
the case where both typeA and typeB are null (treating them as compatible).
Updated the test to exercise this specific both-null scenario using inline
fragments on an unknown type.
https://claude.ai/code/session_01MZkhqChmheW6d46H4p8T7Y
Test Results
336 files +1 336 suites +1 5m 8s ⏱️ +4s
5 389 tests +9 5 380 ✅ +9 9 💤 ±0 0 ❌ ±0
5 478 runs +9 5 469 ✅ +9 9 💤 ±0 0 ❌ ±0
Results for commit fa6c75a. ± Comparison against base commit 5b63591.
This pull request removes 196 and adds 181 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@53f4c1e6 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@74174a23 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@6342d610 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@12e0f1cb 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@4a163575 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@7e642b88 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@2af69643 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@658255aa 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@508a65bf 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@5fa23c 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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The previous fix only handled the both-null case, but the actual
benchmarkDeepAbstractConcrete scenario has mixed null and non-null types:
fragments on "Whatever" (unresolvable) produce null-typed fields alongside
Abstract-typed fields from interface inline fragments. A null type means the
parent was unresolvable, so we should skip the comparison entirely rather
than report a spurious conflict.
This changes the null check from reporting an error to simply continuing,
which matches the semantic that null = unknown/unresolvable = skip.
The test now reproduces the exact benchmark scenario that was crashing.
https://claude.ai/code/session_01MZkhqChmheW6d46H4p8T7Y
These tests exercise the same validation paths as
OverlappingFieldValidationPerformance and OverlappingFieldValidationBenchmark
but assert zero errors instead of measuring performance. This ensures the
overlapping fields rule does not produce false positives (e.g. from null
type handling in abstract/concrete type scenarios).
https://claude.ai/code/session_01MZkhqChmheW6d46H4p8T7Y
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|