Skip to content

Commit 2f0dc5b

Browse files
dondonzdondonz
authored
Merge pull request #4184 from graphql-java/infer-nullity-3
JSpecify big wave 2
2 parents 8e1c98a + d83f175 commit 2f0dc5b

77 files changed

Lines changed: 447 additions & 253 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.claude/commands/jspecify-annotate.md‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
The task is to annotate public API classes (marked with `@PublicAPI`) with JSpecify nullability annotations.
2+
3+
Note that JSpecify is already used in this repository so it's already imported.
4+
5+
If you see a builder static class, you can label it `@NullUnmarked` and not need to do anymore for this static class in terms of annotations.
6+
7+
Analyze this Java class and add JSpecify annotations based on:
8+
1. Set the class to be `@NullMarked`
9+
2. Remove all the redundant `@NonNull` annotations that IntelliJ added
10+
3. Check Javadoc @param tags mentioning "null", "nullable", "may be null"
11+
4. Check Javadoc @return tags mentioning "null", "optional", "if available"
12+
5. Method implementations that return null or check for null
13+
6. GraphQL specification details (see details below)
14+
15+
## GraphQL Specification Compliance
16+
This is a GraphQL implementation. When determining nullability, consult the GraphQL specification (https://spec.graphql.org/draft/) for the relevant concept. Key principles:
17+
18+
The spec defines which elements are required (non-null) vs optional (nullable). Look for keywords like "MUST" to indicate when an element is required, and conditional words such as "IF" to indicate when an element is optional.
19+
20+
If a class implements or represents a GraphQL specification concept, prioritize the spec's nullability requirements over what IntelliJ inferred.
21+
22+
## How to validate
23+
Finally, please check all this works by running the NullAway compile check.
24+
25+
If you find NullAway errors, try and make the smallest possible change to fix them. If you must, you can use assertNotNull. Make sure to include a message as well.
26+
27+
## Formatting Guidelines
28+
29+
Do not make spacing or formatting changes. Avoid adjusting whitespace, line breaks, or other formatting when editing code. These changes make diffs messy and harder to review. Only make the minimal changes necessary to accomplish the task.
30+
31+
## Cleaning up
32+
Finally, can you remove this class from the JSpecifyAnnotationsCheck as an exemption
33+
34+
You do not need to run the JSpecifyAnnotationsCheck. Removing the completed class is enough.
35+
36+
Remember to delete all unused imports when you're done from the class you've just annotated.
37+
38+
## Generics Annotations
39+
40+
When annotating generic types and methods, follow these JSpecify rules:
41+
42+
### Type Parameter Bounds
43+
44+
The bound on a type parameter determines whether nullable type arguments are allowed:
45+
46+
| Declaration | Allows `@Nullable` type argument? |
47+
|-------------|----------------------------------|
48+
| `<T>` | ❌ No — `Box<@Nullable String>` is illegal |
49+
| `<T extends @Nullable Object>` | ✅ Yes — `Box<@Nullable String>` is legal |
50+
51+
**When to use `<T extends @Nullable Object>`:**
52+
- When callers genuinely need to parameterize with nullable types
53+
- Example: `DataFetcherResult<T extends @Nullable Object>` — data fetchers may return nullable types
54+
55+
**When to keep `<T>`:**
56+
- When the type parameter represents a concrete non-null object
57+
- Even if some methods return `@Nullable T` (meaning "can be null even if T is non-null")
58+
- Example: `Edge<T>` with `@Nullable T getNode()` — node may be null, but T represents the object type
59+

‎src/main/java/graphql/AssertException.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
package graphql;
22

33

4+
import org.jspecify.annotations.NullMarked;
5+
46
@PublicApi
7+
@NullMarked
58
public class AssertException extends GraphQLException {
69

710
public AssertException(String message) {

‎src/main/java/graphql/Directives.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import graphql.language.DirectiveDefinition;
77
import graphql.language.StringValue;
88
import graphql.schema.GraphQLDirective;
9+
import org.jspecify.annotations.NullMarked;
910

1011
import java.util.Collections;
1112
import java.util.LinkedHashMap;
@@ -39,6 +40,7 @@
3940
* The directives that are understood by graphql-java
4041
*/
4142
@PublicApi
43+
@NullMarked
4244
public class Directives {
4345

4446
private static final String DEPRECATED = "deprecated";

‎src/main/java/graphql/ErrorClassification.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package graphql;
22

3+
import org.jspecify.annotations.NullMarked;
4+
35
/**
46
* Errors in graphql-java can have a classification to help with the processing
57
* of errors. Custom {@link graphql.GraphQLError} implementations could use
@@ -8,6 +10,7 @@
810
* graphql-java ships with a standard set of error classifications via {@link graphql.ErrorType}
911
*/
1012
@PublicApi
13+
@NullMarked
1114
public interface ErrorClassification {
1215

1316
/**

‎src/main/java/graphql/ErrorType.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
package graphql;
22

33

4+
import org.jspecify.annotations.NullMarked;
5+
46
/**
57
* All the errors in graphql belong to one of these categories
68
*/
79
@PublicApi
10+
@NullMarked
811
public enum ErrorType implements ErrorClassification {
912
InvalidSyntax,
1013
ValidationError,

‎src/main/java/graphql/ExceptionWhileDataFetching.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33

44
import graphql.execution.ResultPath;
55
import graphql.language.SourceLocation;
6+
import org.jspecify.annotations.NullMarked;
7+
import org.jspecify.annotations.Nullable;
68

79
import java.util.Collections;
810
import java.util.LinkedHashMap;
@@ -16,13 +18,14 @@
1618
* This graphql error will be used if a runtime exception is encountered while a data fetcher is invoked
1719
*/
1820
@PublicApi
21+
@NullMarked
1922
public class ExceptionWhileDataFetching implements GraphQLError {
2023

2124
private final String message;
2225
private final List<Object> path;
2326
private final Throwable exception;
2427
private final List<SourceLocation> locations;
25-
private final Map<String, Object> extensions;
28+
private final @Nullable Map<String, Object> extensions;
2629

2730
public ExceptionWhileDataFetching(ResultPath path, Throwable exception, SourceLocation sourceLocation) {
2831
this.path = assertNotNull(path).toList();
@@ -41,7 +44,7 @@ private String mkMessage(ResultPath path, Throwable exception) {
4144
* exception into the ExceptionWhileDataFetching error and hence have custom "extension attributes"
4245
* per error message.
4346
*/
44-
private Map<String, Object> mkExtensions(Throwable exception) {
47+
private @Nullable Map<String, Object> mkExtensions(Throwable exception) {
4548
Map<String, Object> extensions = null;
4649
if (exception instanceof GraphQLError) {
4750
Map<String, Object> map = ((GraphQLError) exception).getExtensions();
@@ -73,7 +76,7 @@ public List<Object> getPath() {
7376
}
7477

7578
@Override
76-
public Map<String, Object> getExtensions() {
79+
public @Nullable Map<String, Object> getExtensions() {
7780
return extensions;
7881
}
7982

‎src/main/java/graphql/ExecutionInput.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,9 @@
2525
public class ExecutionInput {
2626
private final String query;
2727
private final String operationName;
28-
private final Object context;
28+
private final @Nullable Object context;
2929
private final GraphQLContext graphQLContext;
30-
private final Object localContext;
30+
private final @Nullable Object localContext;
3131
private final Object root;
3232
private final RawVariables rawVariables;
3333
private final Map<String, Object> extensions;

‎src/main/java/graphql/ExecutionResult.java‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
package graphql;
22

33

4+
import org.jspecify.annotations.NullMarked;
5+
import org.jspecify.annotations.NullUnmarked;
6+
import org.jspecify.annotations.Nullable;
7+
48
import java.util.List;
59
import java.util.Map;
610
import java.util.function.Consumer;
@@ -9,6 +13,7 @@
913
* This simple value class represents the result of performing a graphql query.
1014
*/
1115
@PublicApi
16+
@NullMarked
1217
@SuppressWarnings("TypeParameterUnusedInFormals")
1318
public interface ExecutionResult {
1419

@@ -22,16 +27,16 @@ public interface ExecutionResult {
2227
*
2328
* @return the data in the result or null if there is none
2429
*/
25-
<T> T getData();
30+
<T> @Nullable T getData();
2631

2732
/**
2833
* The graphql specification specifies:
29-
*
34+
* <p>
3035
* "If an error was encountered before execution begins, the data entry should not be present in the result.
3136
* If an error was encountered during the execution that prevented a valid response, the data entry in the response should be null."
32-
*
37+
* <p>
3338
* This allows to distinguish between the cases where {@link #getData()} returns null.
34-
*
39+
* <p>
3540
* See : <a href="https://graphql.github.io/graphql-spec/June2018/#sec-Data">https://graphql.github.io/graphql-spec/June2018/#sec-Data</a>
3641
*
3742
* @return <code>true</code> if the entry "data" should be present in the result
@@ -42,14 +47,14 @@ public interface ExecutionResult {
4247
/**
4348
* @return a map of extensions or null if there are none
4449
*/
45-
Map<Object, Object> getExtensions();
50+
@Nullable Map<Object, Object> getExtensions();
4651

4752

4853
/**
4954
* The graphql specification says that result of a call should be a map that follows certain rules on what items
5055
* should be present. Certain JSON serializers may or may interpret {@link ExecutionResult} to spec, so this method
5156
* is provided to produce a map that strictly follows the specification.
52-
*
57+
* <p>
5358
* See : <a href="https://spec.graphql.org/October2021/#sec-Response-Format">https://spec.graphql.org/October2021/#sec-Response-Format</a>
5459
*
5560
* @return a map of the result that strictly follows the spec
@@ -88,6 +93,7 @@ static Builder<?> newExecutionResult() {
8893
return ExecutionResultImpl.newExecutionResult();
8994
}
9095

96+
@NullUnmarked
9197
interface Builder<B extends Builder<B>> {
9298

9399
/**

‎src/main/java/graphql/GraphQL.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -559,14 +559,14 @@ private PreparsedDocumentEntry parseAndValidate(AtomicReference<ExecutionInput>
559559

560560
ParseAndValidateResult parseResult = parse(executionInput, graphQLSchema, instrumentationState);
561561
if (parseResult.isFailure()) {
562-
return new PreparsedDocumentEntry(parseResult.getSyntaxException().toInvalidSyntaxError());
562+
return new PreparsedDocumentEntry(assertNotNull(parseResult.getSyntaxException(), "Parse result syntax exception cannot be null when failed").toInvalidSyntaxError());
563563
} else {
564564
final Document document = parseResult.getDocument();
565565
// they may have changed the document and the variables via instrumentation so update the reference to it
566566
executionInput = executionInput.transform(builder -> builder.variables(parseResult.getVariables()));
567567
executionInputRef.set(executionInput);
568568

569-
final List<ValidationError> errors = validate(executionInput, document, graphQLSchema, instrumentationState);
569+
final List<ValidationError> errors = validate(executionInput, assertNotNull(document, "Document cannot be null when parse succeeded"), graphQLSchema, instrumentationState);
570570
if (!errors.isEmpty()) {
571571
return new PreparsedDocumentEntry(document, errors);
572572
}
@@ -599,7 +599,7 @@ private List<ValidationError> validate(ExecutionInput executionInput, Document d
599599
validationCtx.onDispatched();
600600

601601
Predicate<Class<?>> validationRulePredicate = executionInput.getGraphQLContext().getOrDefault(ParseAndValidate.INTERNAL_VALIDATION_PREDICATE_HINT, r -> true);
602-
Locale locale = executionInput.getLocale() != null ? executionInput.getLocale() : Locale.getDefault();
602+
Locale locale = executionInput.getLocale();
603603
List<ValidationError> validationErrors = ParseAndValidate.validate(graphQLSchema, document, validationRulePredicate, locale);
604604

605605
validationCtx.onCompleted(validationErrors, null);

‎src/main/java/graphql/GraphQLContext.java‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
package graphql;
22

3+
import org.jspecify.annotations.NullMarked;
4+
import org.jspecify.annotations.NullUnmarked;
5+
import org.jspecify.annotations.Nullable;
6+
37
import java.util.Map;
48
import java.util.Objects;
59
import java.util.Optional;
@@ -32,12 +36,13 @@
3236
* You can set this up via {@link ExecutionInput#getGraphQLContext()}
3337
*
3438
* All keys and values in the context MUST be non null.
35-
*
39+
* <p>
3640
* The class is mutable via a thread safe implementation but it is recommended to try to use this class in an immutable way if you can.
3741
*/
3842
@PublicApi
3943
@ThreadSafe
4044
@SuppressWarnings("unchecked")
45+
@NullMarked
4146
public class GraphQLContext {
4247

4348
private final ConcurrentMap<Object, Object> map;
@@ -66,7 +71,7 @@ public GraphQLContext delete(Object key) {
6671
*
6772
* @return a value or null
6873
*/
69-
public <T> T get(Object key) {
74+
public <T> @Nullable T get(Object key) {
7075
return (T) map.get(assertNotNull(key));
7176
}
7277

@@ -210,7 +215,7 @@ public GraphQLContext putAll(Consumer<GraphQLContext.Builder> contextBuilderCons
210215
*
211216
* @return the new value associated with the specified key, or null if none
212217
*/
213-
public <T> T compute(Object key, BiFunction<Object, ? super T, ? extends T> remappingFunction) {
218+
public <T> @Nullable T compute(Object key, BiFunction<Object, ? super T, ? extends T> remappingFunction) {
214219
assertNotNull(remappingFunction);
215220
return (T) map.compute(assertNotNull(key), (k, v) -> remappingFunction.apply(k, (T) v));
216221
}
@@ -226,7 +231,7 @@ public <T> T compute(Object key, BiFunction<Object, ? super T, ? extends T> rema
226231
* @return the current (existing or computed) value associated with the specified key, or null if the computed value is null
227232
*/
228233

229-
public <T> T computeIfAbsent(Object key, Function<Object, ? extends T> mappingFunction) {
234+
public <T> @Nullable T computeIfAbsent(Object key, Function<Object, ? extends T> mappingFunction) {
230235
return (T) map.computeIfAbsent(assertNotNull(key), assertNotNull(mappingFunction));
231236
}
232237

@@ -241,7 +246,7 @@ public <T> T computeIfAbsent(Object key, Function<Object, ? extends T> mappingFu
241246
* @return the new value associated with the specified key, or null if none
242247
*/
243248

244-
public <T> T computeIfPresent(Object key, BiFunction<Object, ? super T, ? extends T> remappingFunction) {
249+
public <T> @Nullable T computeIfPresent(Object key, BiFunction<Object, ? super T, ? extends T> remappingFunction) {
245250
assertNotNull(remappingFunction);
246251
return (T) map.computeIfPresent(assertNotNull(key), (k, v) -> remappingFunction.apply(k, (T) v));
247252
}
@@ -254,7 +259,7 @@ public Stream<Map.Entry<Object, Object>> stream() {
254259
}
255260

256261
@Override
257-
public boolean equals(Object o) {
262+
public boolean equals(@Nullable Object o) {
258263
if (this == o) {
259264
return true;
260265
}
@@ -315,6 +320,7 @@ public static Builder newContext() {
315320
return new Builder();
316321
}
317322

323+
@NullUnmarked
318324
public static class Builder {
319325
private final ConcurrentMap<Object, Object> map = new ConcurrentHashMap<>();
320326

0 commit comments

Comments
 (0)