GeezerVison is showing github.com — Add JSpecify annotations to language package nodes - redo (12 more) by dondonz · Pull Request #4257 · graphql-java/graphql-java
Conversation
dondonz
commented
Feb 22, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Summary
Adds JSpecify nullability annotations (@NullMarked, @NullUnmarked, @Nullable) to the following graphql.language classes and interfaces, removing each from the JSpecifyAnnotationsCheck exemption list:
NodeTraverserNonNullTypeObjectFieldObjectTypeDefinition / ObjectTypeExtensionDefinitionOperationDefinitionOperationTypeDefinitionPrettyAstPrinterSDLDefinition / SDLExtensionDefinitionSDLNamedDefinition / TypeDefinition
Key decisions
Key interesting point - NamedNode.getName() is @Nullable
OperationDefinition can be anonymous (e.g. { user { name } }) — the GraphQL spec makes the operation name optional. Pushing @Nullable onto NamedNode.getName() is the truthful representation of the interface contract.
TypeDefinition.getName() overrides as non-null
TypeDefinition extends both SDLNamedDefinition (non-null getName()) and NamedNode (@Nullable getName()). SDL type definitions always have a name, so TypeDefinition explicitly overrides getName() to return non-null. This prevents @Nullable from cascading into callers like TypeDefinitionRegistry that work with concrete schema types.
OperationDefinition.selectionSet is non-null
Legacy convenience constructors that passed null for selectionSet were deleted (they were only used in one test). The field, constructor, and getSelectionSet() are all non-null, matching the GraphQL spec requirement that every operation has a selection set.
OperationDefinition.operation is @Nullable
The shorthand query form (e.g. { user { name } }) omits the query keyword, so getOperation() can legitimately return null.
Builder inner classes are @NullUnmarked
All builder static inner classes are annotated @NullUnmarked rather than annotating each field individually.
PrettyAstPrinter private helpers
node(@Nullable Class startClass), isEmpty(@Nullable ...), nvl(@Nullable ...), block(... @Nullable String separatorSingleLine, @Nullable String whenEmpty), and spaced(@Nullable String... args) / join(String, @Nullable String...) are nullable because null is actually passed at the call sites or because the method bodies already handle null gracefully.
Cascading fixes
OperationValidator: getName() is now @Nullable on OperationDefinition, so error message construction uses a pre-checked local variable or Objects.toString(x, "") fallback for the anonymous operation case.TypeDefinitionRegistry: two Assert.assertNotNull(t.getName(), ...) workarounds are removed now that TypeDefinition.getName() is non-null.
🤖 Generated with Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Test Results
335 files ±0 335 suites ±0 5m 8s ⏱️ +3s
5 376 tests ±0 5 367 ✅ ±0 9 💤 ±0 0 ❌ ±0
5 465 runs ±0 5 456 ✅ ±0 9 💤 ±0 0 ❌ ±0
Results for commit 1063251. ± Comparison against base commit 50c22c1.
This pull request removes 196 and adds 172 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@296e281a 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@59cda16e 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@5dd903be 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@70887727 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@56da7487 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@599e4d41 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@fab35b1 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@5c77ba8f 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@4a734c04 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@10a0fe30 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.
|
This is a redo of a previous PR that went off the rails
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
- **Do not remove interfaces** from public classes (e.g., if a class implements `NamedNode`, it must continue to do so).
|
|
|
- **Be extremely careful when changing methods to return `@Nullable`**. If an interface contract (or widespread ecosystem usage) expects a non-null return value, changing it to `@Nullable` is a breaking change that will cause compilation errors or `NullPointerException`s for callers. For example, if a method returned `null` implicitly but its interface requires non-null, you must honor the non-null contract (e.g., returning an empty string or default value instead of `null`).
|
|
|
- **Do not change the binary signature** of methods or constructors in a way that breaks backwards compatibility.
|
|
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous attempt randomly deleted a constructor parameter which caused that PR to go off the rails. Let's fix that here
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
* @return the name of this node, or null if this node is anonymous (e.g. an anonymous operation definition)
|
|
|
*/
|
|
|
String getName();
|
|
|
@Nullable String getName();
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be nullable because an operation definition might not have a name
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
this.selectionSet = selectionSet;
|
|
|
}
|
|
|
|
|
|
public OperationDefinition(String name,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These constructors date back to 2018, no longer needed with the builders we have today.
I am removing these lines because these parameters should be not-nullable.
I would usually first deprecate the methods, then remove, but if I keep them here with a deprecated annotation, the nullability annotations will not be correct.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically a breaking change
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
dondonz
changed the title
Add JSpecify annotations to language package nodes (batch 3)
Add JSpecify annotations to language package nodes - redo
Feb 22, 2026
|
|
OperationDefinition.Operation op = operationDefinition.getOperation() != null
|
|
|
? operationDefinition.getOperation()
|
|
|
: OperationDefinition.Operation.QUERY;
|
|
|
switch (op) {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some follow on changes after adding nullability annotations
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
def operationDefinition = OperationDefinition.newOperationDefinition()
|
|
|
.name("q")
|
|
|
.selectionSet(SelectionSet.newSelectionSet().selection(new Field("f")).build())
|
|
|
.build()
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor test change after selection set not nullable
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
dondonz
changed the title
Add JSpecify annotations to language package nodes - redo
Add JSpecify annotations to language package nodes - redo (12 more)
Feb 28, 2026
|
|
@Override
|
|
|
public ObjectField deepCopy() {
|
|
|
return new ObjectField(name, deepCopy(this.value), getSourceLocation(), getComments(), getIgnoredChars(), getAdditionalData());
|
|
|
return new ObjectField(name, assertNotNull(deepCopy(this.value), "value deepCopy should not return null"), getSourceLocation(), getComments(), getIgnoredChars(), getAdditionalData());
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used to think assertion messages matter but they dont. A stacktrace would lead to this code location - so less code can be better code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert is a special one to satisfy the NullAway check
The problem is that deepCopy is a generic utility and I can't guarantee that always returns not-nullable result
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
public OperationDefinition(String name) {
|
|
|
this(name, null, emptyList(), emptyList(), null, null, emptyList(), IgnoredChars.EMPTY, emptyMap());
|
|
|
}
|
|
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough - cruft must be removed at some point
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
}
|
|
|
|
|
|
public Operation getOperation() {
|
|
|
public @Nullable Operation getOperation() {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ummmmmm can this be null??? if its not specified surely is defaults to QUERY??
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I agree, should have defaulted to query instead
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does in the parser as well
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Per the GraphQL spec and graphql-js reference implementation, the
operation type is always known — shorthand queries are QUERY. Default
the Builder to Operation.QUERY and remove null checks in callers.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Test Report
Test Results
| Java Version |
Total |
Passed |
Failed |
Errors |
Skipped |
| Java 11 |
5671 (±0) |
5614 (±0) |
0 (±0) |
0 (±0) |
57 (±0) |
| Java 17 |
5671 (±0) |
5613 (±0) |
0 (±0) |
0 (±0) |
58 (±0) |
| Java 21 |
5671 (±0) |
5613 (±0) |
0 (±0) |
0 (±0) |
58 (±0) |
| Java 25 |
5671 (±0) |
5613 (±0) |
0 (±0) |
0 (±0) |
58 (±0) |
| jcstress |
32 (±0) |
32 (±0) |
0 (±0) |
0 (±0) |
0 (±0) |
| Total |
22716 (±0) |
22485 (±0) |
0 (±0) |
0 (±0) |
231 (±0) |
Code Coverage (Java 25)
| Metric |
Covered |
Missed |
Coverage |
vs Master |
| Lines |
28699 |
3123 |
90.2% |
±0.0% |
| Branches |
8333 |
1507 |
84.7% |
±0.0% |
| Methods |
7680 |
1223 |
86.3% |
±0.0% |
Changed Class Coverage (4 classes)
| Class |
Line |
Branch |
Method |
| g.e.i.d.ExhaustedDataLoaderDispatchStrategy |
+1.2% 🟢 |
+7.7% 🟢 |
±0.0% |
| g.l.AstSorter |
±0.0% |
+2.9% 🟢 |
±0.0% |
| g.l.OperationDefinition |
+3.0% 🟢 |
±0.0% |
+3.3% 🟢 |
g.l.OperationDefinition
$Builder |
+0.3% 🟢 |
±0.0% |
±0.0% |
Full HTML report: build artifact jacoco-html-report
Updated: 2026-03-08 00:51:01 UTC
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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
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.
|