spring-graphql icon indicating copy to clipboard operation
spring-graphql copied to clipboard

`@BatchMapping` is broken for subscriptions

Open yassenb opened this issue 1 year ago • 3 comments

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

  • the version is reverted to 3.2.7
  • @QueryMapping is used to resolve the field instead of a @BatchMapping
  • a @QueryMapping is used to fetch an object instead of a @SubscriptionMapping it works.

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

yassenb avatar Jul 03 '24 11:07 yassenb

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.

rstoyanchev avatar Jul 03 '24 14:07 rstoyanchev

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

bbakerman avatar Jul 03 '24 22:07 bbakerman

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.

rstoyanchev avatar Jul 04 '24 12:07 rstoyanchev

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() }
    }

pdavidson avatar Jul 05 '24 15:07 pdavidson

@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.

rstoyanchev avatar Jul 11 '24 11:07 rstoyanchev

@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

dondonz avatar Jul 12 '24 05:07 dondonz

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

rstoyanchev avatar Jul 12 '24 07:07 rstoyanchev

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 avatar Jul 15 '24 00:07 bbakerman

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

bbakerman avatar Jul 28 '24 23:07 bbakerman

Thanks for finding the root cause.

rstoyanchev avatar Jul 29 '24 06:07 rstoyanchev

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

dondonz avatar Jul 29 '24 07:07 dondonz

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.

rstoyanchev avatar Jul 29 '24 10:07 rstoyanchev

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

dondonz avatar Jul 29 '24 22:07 dondonz

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

yassenb avatar Aug 12 '24 14:08 yassenb