Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@BatchMapping is broken for subscriptions #1019

Closed
yassenb opened this issue Jul 3, 2024 · 14 comments
Closed

@BatchMapping is broken for subscriptions #1019

yassenb opened this issue Jul 3, 2024 · 14 comments
Labels
for: external-project Needs a change in external project

Comments

@yassenb
Copy link

yassenb commented Jul 3, 2024

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

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 3, 2024
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jul 3, 2024

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 graphql-java/graphql-java#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 rstoyanchev added status: blocked An issue that's blocked on an external project change labels Jul 3, 2024
@bbakerman
Copy link

graphql-java/graphql-java#3574 hasnt been released yet so I it cant be that.

It must be some other change

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jul 4, 2024

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
Copy link

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
Copy link
Contributor

@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
Copy link

dondonz commented Jul 12, 2024

@rstoyanchev Sure I created an issue graphql-java/graphql-java#3662

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

@rstoyanchev
Copy link
Contributor

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

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Jul 12, 2024
@rstoyanchev rstoyanchev added for: external-project Needs a change in external project and removed status: waiting-for-triage An issue we've not yet triaged status: blocked An issue that's blocked on an external project change labels Jul 12, 2024
@bbakerman
Copy link

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
Copy link

graphql-java/graphql-java#3673 is a fix for this problem.

@rstoyanchev
Copy link
Contributor

Thanks for finding the root cause.

@dondonz
Copy link

dondonz commented Jul 29, 2024

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
Copy link
Contributor

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
Copy link

dondonz commented Jul 29, 2024

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
Copy link
Author

yassenb commented Aug 12, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project Needs a change in external project
Projects
None yet
Development

No branches or pull requests

6 participants