Fix overlapping fields null type regression from 072165b#4289
Closed
andimarek wants to merge 2 commits into
Closed
Fix overlapping fields null type regression from 072165b#4289andimarek wants to merge 2 commits into
andimarek wants to merge 2 commits into
Conversation
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
Contributor
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
3 tasks
Member
Author
|
Superseded by #4291 which fixes the failing test. The original test used mixed null/non-null field types which is a legitimate conflict. The new PR corrects the test to use a both-null scenario that properly exercises the regression. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
requireSameOutputTypeShape()was incorrectly simplifiedFieldsConflicterrors like'field/field/field' : returns different types 'Abstract' and 'null'benchmarkDeepAbstractConcreteJMH benchmark inOverlappingFieldValidationPerformancewas crashing due to this bugRoot cause
In
OperationValidator.requireSameOutputTypeShape(), the null check was incorrectly simplified from:to:
The old code treated two null types as compatible (skip comparison). The new code unconditionally reported a conflict when
typeBis null, even whentypeAis also null or when null simply means the parent type was unresolvable.Fix
Restored the original null-handling logic: when both types are null, they are treated as compatible and we
continueto the next field. Only when one is null and the other isn't do we report a conflict.Test plan
https://claude.ai/code/session_01XYg9Dqneb941aStgKaZ1Em