spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.54k stars 306 forks source link

`@BatchMapping` is broken for subscriptions #1019

Closed yassenb closed 4 months ago

yassenb commented 4 months ago

Please find a very minimal project that reproduces the problem: https://github.com/Havelock-JSC/playground

This was broken in Spring Boot 3.3.0. When there is a GraphQL @SubscriptionMapping that returns an object that has some of its requested fields resolved with a @BatchMapping it hangs / doesn't return any results. If either

Not entirely sure where the underlying problem is but I would presume it's not in Spring Boot but in Spring GraphQL 1.3.0

rstoyanchev commented 4 months ago

Thanks for the report and repro.

I haven't narrowed the exact cause, but if I downgrade GraphQL Java from 22.0 to 21.5, it works, so I suspect an issue in GraphQ Java. Possibly related to https://github.com/graphql-java/graphql-java/pull/3574.

/cc @bbakerman, @dondonz

FYI, to pin the GraphQL Java version, I added the following to build.gradle.kts:

configurations.all {
    resolutionStrategy.eachDependency {
        if (requested.group == "com.graphql-java" && requested.name == "graphql-java") {
            useVersion("21.5")
        }
    }
}

I've also tried the latest 0.0.0-2024-07-03T07-03-29-f5c340a version, but no difference.

bbakerman commented 4 months ago

https://github.com/graphql-java/graphql-java/pull/3574 hasnt been released yet so I it cant be that.

It must be some other change

rstoyanchev commented 4 months ago

Must be something else then. I experimented with specific versions before 22. The last build that works is 0.0.0-2024-02-18T22-33-40-06b6844, so the issue starts at 0.0.0-2024-02-19T06-04-20-4cfbb35 and after. There are still quite a number of commits on Feb 18-19, but hopefully that narrows it down a bit.

pdavidson commented 4 months ago

Spent quite a bit debugging this - when I manually dispatch the data loader, then it kicks off the batch loader.

    @SchemaMapping
    fun users(
        conversation: Conversation,
        getUsersForMessagesByIds: DataLoader<String, User>
    ): CompletableFuture<List<User>> {
        return getUsersForMessagesByIds.loadMany(conversation.userIds.toList()).also { getUsersForMessagesByIds.dispatch() }
    }
rstoyanchev commented 4 months ago

@bbakerman would it help to have the issue re-created in the graphql-java issue tracker? As the issue is demonstrated by just switching the GraphQL Java versions mentioned above, it does look like a recent regression in the GraphQL Java engine.

dondonz commented 4 months ago

@rstoyanchev Sure I created an issue https://github.com/graphql-java/graphql-java/issues/3662

It is on the graphql-java team to investigate this one further

rstoyanchev commented 4 months ago

Thank you @dondonz. I'm closing this for now. We can re-open if there is anything further to be done here.

bbakerman commented 4 months ago

Just musing here - I don't have a cause of fix BUT I am not sure DataLoader ever worked with subscriptions.

The DataLoader support code tracks every field level and dispatches when the fields in a level is all dispatched as ready to run.

But with subscriptions we cant know when a level if ready to go since it comes in via a reactive stream on objects and the code that tracks does not work

We actually check this in code

        if (executionStrategy instanceof AsyncExecutionStrategy) {
            return new PerLevelDataLoaderDispatchStrategy(executionContext);
        } else {
            return new FallbackDataLoaderDispatchStrategy(executionContext);
        }

The old code in previous versions did this as well (well something similar). So I am not sure how this every worked with @BatchMapping and subscriptions and if it did was it accidentally working some how?

bbakerman commented 4 months ago

https://github.com/graphql-java/graphql-java/pull/3673 is a fix for this problem.

rstoyanchev commented 3 months ago

Thanks for finding the root cause.

dondonz commented 3 months ago

FYI @rstoyanchev we're planning on a new release GraphQL Java very soon (in the next few days hopefully), this fix will be included in the next release

rstoyanchev commented 3 months ago

Sounds good. Spring Boot 3.3.3 should pick that up on August 22, but in the mean time @yassenb you can test by overriding the GraphQL Java version in your application.

dondonz commented 3 months ago

Confirming @bbakerman's fix works.

I gave the playground reproduction repo a try with the master build version of GraphQL Java that contains @bbakerman's fix 0.0.0-2024-07-29T06-38-57-20f51e3, and the subscription field with @BatchMapping now completes

yassenb commented 3 months ago

I can confirm that with the newly released graphql-java 22.2 it's working. Thank you all for your work!