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

@BatchMapping not working for mutation response

Open palhoye opened this issue 1 year ago • 3 comments

Spring Boot 3.3.0 with Spring GraphQL v1.3.0

I have a GraphQL query and mutation that returns the same response. The response is complex with nested GraphQL types and @BatchMapping at several layers.

When I execute a mutation that will return a large number of rows in the response, it breaks. Upon investigation I see that fields marked in the controller as @BatchMapping, are not batching but running individually for each item.

When I execute a query with the exact same fragment as response and same data it works fine and I see that the fields marked as @BatchMapping works as expected by batching the data.

palhoye avatar Jun 07 '24 20:06 palhoye

I see no reason why a mutation should work differently. Is it possible to provide a basic sample?

rstoyanchev avatar Jun 26 '24 11:06 rstoyanchev

I see no reason why a mutation should work differently. Is it possible to provide a basic sample?

Agree, it is surprising.

I'm not able to share a code example right now, but here is a shortened and obfuscated version of the GraphQL which demonstrates the issue:

Note:

  1. We have a mutation and a query with the same response.
  2. When the mutation is executed, the response dataloader is not invoked as expected and each item under elementItems is run as a separate batch with one element in each batch.
  3. When the query is executed, the response dataloader is invoked as expected and all elementItems are run with a single dataloader in one batch.
mutation MutationExample($id: ID!, $stage: Stage!) {
    updateExample(input: {idToUpdate: $id, stage: stage}) {
        ...myFields
    }
}

query QueryExample($id: ID!) {
    ...myFields
}

fragment myFields on TypeElement {
    id
    stage
    myFieldSelection {
        numberOfDistinctElements
        elementItems {
            id
            item {
                id
                name
                owner {
                    ...userProfile
                }
            }
        }
    }
}

fragment userProfile on User {
    id
    email
    displayName
    photoUrl
    isActive
}

palhoye avatar Jun 30 '24 14:06 palhoye

Thanks for the extra context, but I'm afraid it doesn't help enough. An issue of this kind is likely something less than obvious and could also be something in GraphQL Java. The best way to move forward is to provide a small sample (e.g. via https://start.spring.io) that demonstrates the issue.

rstoyanchev avatar Jul 03 '24 16:07 rstoyanchev

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

spring-projects-issues avatar Jul 10 '24 16:07 spring-projects-issues

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

spring-projects-issues avatar Jul 17 '24 16:07 spring-projects-issues

Hi, I also stumbled upon this bug in my project. Created a basic example here @rstoyanchev if you want to check

https://github.com/MateusDadalto/spring-batch-mapping-error

MateusDadalto avatar Jul 25 '24 22:07 MateusDadalto

I was thinking about my scenario here and this only emerge in a very convoluted mutation result of a janky data structure... For me at least the fix is reviewing the mutation and the models and doing something that makes more sense

MateusDadalto avatar Jul 26 '24 00:07 MateusDadalto

Thank you for the sample. I've been able to have a closer look and found this.

This DataLoader documentation page says that it works with AsyncExecutionStrategy only, but the strategy used for mutations is AsyncSerialExecutionStrategy. Accordingly for the mutation case, I see FallbackDataLoaderDispatchStrategy being used, which is a very simple class that calls dispatchAll after every DataFetcher call. So this seems to be expected behavior that should be possible to demonstrate without Spring.

It's worth asking in https://github.com/graphql-java/java-dataloader/discussions to see if there is any further advice on this, and if you do please link to this issue so others can find it.

rstoyanchev avatar Sep 04 '24 11:09 rstoyanchev

Closing for now, but we can re-open if necessary.

rstoyanchev avatar Sep 04 '24 11:09 rstoyanchev

Thanks @rstoyanchev. This clearly explains the current behavior and I truly appreciate the link to the docs and the code.

Besides from being clear from dataloader doc you shared and the code in line 293 for GraphQL.java, it'd be good to document that dataloader does not support mutations since this is not evident.

Again, many thanks.

palhoye avatar Sep 04 '24 11:09 palhoye

Fair enough, I'll re-open for a documentation update. However, it would be useful to confirm with the Dataloader team first if there isn't anything more to this.

rstoyanchev avatar Sep 04 '24 11:09 rstoyanchev

I added a discussion thread with the dataloader team here: Dataloader support for mutations

palhoye avatar Sep 04 '24 11:09 palhoye

I'm putting this on hold until we hear from the GraphQL Java team. It's clear that the combination doesn't work out of the box, but I'm not sure if it is intentional or not given that mutation execution should work like query execution.

rstoyanchev avatar Sep 12 '24 13:09 rstoyanchev

Understand.

Great if you and others can upvote the discussion thread to encourage a response: Dataloader support for mutations

palhoye avatar Sep 12 '24 13:09 palhoye

I've created https://github.com/graphql-java/graphql-java/issues/3711 and will close this for now, but we can re-open if necessary.

rstoyanchev avatar Sep 23 '24 16:09 rstoyanchev