DataLoader/BatchLoader causing unexpected NPEs

[iuliiasobolevska]

Describe the bug

Hi Folks,

The 3060a30 commit added JSpecify's @NonNull annotation on the load method key argument but missed adding it on some methods that call the annotated ones, e.g.

    public CompletableFuture<V> load(K key) {
        return load(key, null);
    }

https://github.com/graphql-java/java-dataloader/blame/master/src/main/java/org/dataloader/DataLoader.java#L172-L174

As a result, IntelliJ IDE doesn't provide any hinting on passing a nullable argument to the dataLoader.load(key) method.

Additional context: Some applications got caught by surprise since they picked up OSS Spring Boot 3.5 via automated dependency updates, which upgraded spring-graphql and, as a result, java-dataloader. Thanks to adding JSpecify nullity annotations, these old bugs are getting surfaced, but, unfortunately, this happens at runtime, and the tooling (e.g. IntelliJ IDE) doesn't help much. In one case, the key was derived from a several chained calls pulling data from the DgsDataFetchingEnvironment.

Overall, org.dataloader.DataLoader only has one method that takes key annotated as @NonNull. While it's self-explanatory that null should be passed in all others, it would still be great to explicitly annotate all of them to make it easier to identify and catch issues.

[bbakerman]

We are new to JSpecify and hence cant claim to be experts on it. We are however embracing this in this repo and graphql-java say

Overall, org.dataloader.DataLoader only has one method that takes key annotated as @nonnull. While it's self-explanatory that null should be passed in all others, it would still be great to explicitly annotate all of them to make it easier to identify and catch issues.

We don't want to explicitly makes things as @NonNull - the @NullMarked annotation is used to say "everything is non null unless we say other wise".

@PublicApi
@NullMarked
public class DataLoader<K, V extends @Nullable Object> {

https://jspecify.dev/docs/user-guide/#nullmarked

It would be annoying to have to annotate each and every type usage in your Java code with either @Nullable or @NonNull to avoid unspecified nullness (especially once you add generics!).

So JSpecify gives you the @NullMarked annotation. When you apply @NullMarked to a module, package, class, or method, it means that unannotated types in that scope are treated as if they were annotated with @NonNull.

ps... I think it would have been better called @NonNullMarked but thats just like my opinion man.

Thanks to adding JSpecify nullity annotations, these old bugs are getting surfaced, but, unfortunately, this happens at runtime,

Can you please expand more here - what is happening at runtime? Is something in your system enforcing non null ability at runtime?? Or is it blowing up in the way it would have before the new annotations?

This is of great interest to us if we are some how having a runtime effect via these annotatons.

[iuliiasobolevska]

I think my original description was more of a symptom than the root cause. We see many breakages for different services, all manifesting as unexpected NPEs, and each has a different reason (some need the service code fixed to better handle nulls, some are still under investigation).

My current thoughts:

1. JSpecify annotations are breaking Kotlin code because it's technically public API contract change. Details in issue Isaiah logged: @NonNull changes in DataLoader APIs breaks downstream Kotlin apps. #195
2. https://github.com/graphql-java/java-dataloader/blob/master/src/main/java/org/dataloader/DataLoader.java#L226-L228 nonNull check specifically throws some NPEs. From a consumer perspective, a more gradual rollout would have been preferred (with nullity being marked via JSpecify, then in the next major release, add nonNull check).
3. More importantly, the BatchLoader contract may have gotten broken because of these changes. The batch function should return the same number of elements as the original keys. In case of errors, it's common to return null: https://github.com/graphql-java/java-dataloader/blob/master/src/main/java/org/dataloader/BatchLoader.java#L66. But with @NullMarked it doesn't seem like V (value) can be null anymore: https://github.com/graphql-java/java-dataloader/blob/master/src/main/java/org/dataloader/BatchLoader.java#L79C1-L80.

[bbakerman]

Thanks for the report back - we appreciate this

We do want to move to a JSpecified non nullness world so we are on this journey progressively.

My reply to you thoughts are as follows

1 JSpecify annotations are breaking Kotlin code because it's technically public API contract change.
Yeah we got caught out by this because of the semantic differences between Java and Kotlin - we have added Kotlin to our integration tests to help ensure a better situation ongoing.

2 From a consumer perspective, a more gradual rollout would have been preferred (with nullity being marked via JSpecify, then in the next major release, add nonNull check).

Sorry I disagree here - the adding of the runtime nonNull was in a major release AND it was something that should have been done from the start - the key was always meant to be provided - it never made sense to be nullable

3 More importantly, the BatchLoader contract may have gotten broken because of these changes. The batch function should return the same number of elements as the original keys. In case of errors, it's common to return null:

Yeah we need to fix this - this is something we missed and perhaps its Kotlin enforced while not Java enforced.

[bbakerman]

regarding

3 More importantly, the BatchLoader contract may have gotten broken because of these changes. The batch function should return the same number of elements as the original keys. In case of errors, it's common to return null:

I checked on the latest master (with one with Kotlin test support) and Kotlin seems happy here

        val batchLoadFunction: (List<String>) -> CompletionStage<List<String?>> =
            { keys -> completedFuture(keys.toList()) }
        val dataLoader: DataLoader<String, String?> = DataLoaderFactory.newDataLoader(batchLoadFunction)

So you are able to put null values into the load function

I think this is because the latest master has the fix

@PublicApi
@NullMarked
public class DataLoader<K, V extends @Nullable Object> {

The V extends @Nullable Object is how you tell JSpecify (and Kotlin by extension it seems) that the V can be nullable in an otherwise non null file.

I think we could improve BatchLoader to have the same but Kotlin doesnt seem the care from what I can see, at least not when the V of the DataLoader is transferred via DataLoaderFactory

[iuliiasobolevska]

I think this is because the latest master has the fix

1. can you please share a snapshot number from the latest main so I can validate on our side?
2. do you happen to know the date of the next release?

[bbakerman]

#207 is the PR that helps expand the semantics of nullable to the BatchLoader interfaces. Its more explicit however it seems Kotlin didn't strictly need it in a compile sense.

[bbakerman]

can you please share a snapshot number from the latest main so I can validate on our side?

https://repo1.maven.org/maven2/com/graphql-java/java-dataloader/ has all the versions

https://repo1.maven.org/maven2/com/graphql-java/java-dataloader/0.0.0-2025-06-22T00-24-37-10731f8/ is the latest as I write this.

do you happen to know the date of the next release?

not sure here - we havent planned that far ahead yet to give a date

[iuliiasobolevska]

Hi, we can probably close this one as 5.0.1 addressed the BatchLoader issue and Kotlin projects will just have to fix nullity issues. Appreciate your help and guidance!

[dondonz]

Thanks for reporting and the feedback