Add JSpecify annotations to language package nodes - redo (12 more) by dondonz · Pull Request #4257 · graphql-java/graphql-java · GitHub

Summary

Adds JSpecify nullability annotations (@NullMarked, @NullUnmarked, @Nullable) to the following graphql.language classes and interfaces, removing each from the JSpecifyAnnotationsCheck exemption list:

NodeTraverser
NonNullType
ObjectField
ObjectTypeDefinition / ObjectTypeExtensionDefinition
OperationDefinition
OperationTypeDefinition
PrettyAstPrinter
SDLDefinition / SDLExtensionDefinition
SDLNamedDefinition / 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

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.

This is a redo of a previous PR that went off the rails

Previous attempt randomly deleted a constructor parameter which caused that PR to go off the rails. Let's fix that here

This has to be nullable because an operation definition might not have a name

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.

This is technically a breaking change

Some follow on changes after adding nullability annotations

Minor test change after selection set not nullable

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

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

Fair enough - cruft must be removed at some point

Ummmmmm can this be null??? if its not specified surely is defaults to QUERY??

Yep I agree, should have defaulted to query instead

It does in the parser as well

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 <noreply@anthropic.com>

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

dondonz added 6 commits February 22, 2026 10:27

Add corrections to prompt

b261a43

Add first lot of changes

4df4205

Add operation definition

8fb836e

Add pretty ast printer

298fea2

Override name as non null

82712c7

Add operation validator

7a9e810

dondonz commented Feb 22, 2026

View reviewed changes

dondonz added the breaking change requires a new major version to be relased label Feb 22, 2026

dondonz changed the title Add JSpecify annotations to language package nodes (batch 3) Add JSpecify annotations to language package nodes - redo Feb 22, 2026

dondonz marked this pull request as draft February 22, 2026 05:09

dondonz added 3 commits February 22, 2026 16:38

Adjust AST Printer

dd49296

Account for a nullable op name

9e31132

Fix test for non-null selection set

1063251

dondonz commented Feb 22, 2026

View reviewed changes

dondonz marked this pull request as ready for review February 22, 2026 05:52

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

bbakerman reviewed Mar 7, 2026

View reviewed changes

bbakerman approved these changes Mar 7, 2026

View reviewed changes

bbakerman added this to the 26.x is a breaking change release (other fixes as well) milestone Mar 7, 2026

dondonz merged commit 53c0c82 into master Mar 21, 2026
11 checks passed

github-actions Bot mentioned this pull request Mar 22, 2026

CI Failure DoctorCI Failure - Run #23398087408 - JSpecifyAnnotationsCheck exemption list conflict #4351

Closed

1 task

+                   * @return the name of this node, or null if this node is anonymous (e.g. an anonymous operation definition)
                    */
-                  String getName();
+                  @Nullable String getName();

+                      OperationDefinition.Operation op = operationDefinition.getOperation() != null
+                              ? operationDefinition.getOperation()
+                              : OperationDefinition.Operation.QUERY;
+                      switch (op) {

+                  def operationDefinition = OperationDefinition.newOperationDefinition()
+                          .name("q")
+                          .selectionSet(SelectionSet.newSelectionSet().selection(new Field("f")).build())
+                          .build()

                   @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());

+              - **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.

                       this.selectionSet = selectionSet;
                   }
-                  public OperationDefinition(String name,

-                  public OperationDefinition(String name) {
-                      this(name, null, emptyList(), emptyList(), null, null, emptyList(), IgnoredChars.EMPTY, emptyMap());
-                  }

                   }
-                  public Operation getOperation() {
+                  public @Nullable Operation getOperation() {

Add JSpecify annotations to language package nodes - redo (12 more) by dondonz · Pull Request #4257 · graphql-java/graphql-java · GitHub

Uh oh!

Add JSpecify annotations to language package nodes - redo (12 more) - #4257

Add JSpecify annotations to language package nodes - redo (12 more)#4257dondonz merged 10 commits intomastergraphql-java/graphql-java:masterfrom add-jspecify-redographql-java/graphql-java:add-jspecify-redoCopy head branch name to clipboard

Conversation

dondonz commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key decisions

Cascading fixes

Uh oh!

github-actions Bot commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

dondonz commented Feb 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 8, 2026

Test Report

Test Results

Code Coverage (Java 25)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add JSpecify annotations to language package nodes - redo (12 more)#4257
dondonz merged 10 commits into
masterfrom
add-jspecify-redo

dondonz commented Feb 22, 2026 •
edited
Loading

github-actions Bot commented Feb 22, 2026 •
edited
Loading