spring-projects / spring-graphql

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

@BatchMapping not working for mutation response #994

Closed palhoye closed 2 months ago

palhoye commented 5 months ago

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.

rstoyanchev commented 5 months ago

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

palhoye commented 5 months ago

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
}
rstoyanchev commented 4 months ago

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.

spring-projects-issues commented 4 months ago

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 commented 4 months ago

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.

MateusDadalto commented 4 months ago

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 commented 4 months ago

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

rstoyanchev commented 2 months ago

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 commented 2 months ago

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

palhoye commented 2 months ago

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.

rstoyanchev commented 2 months ago

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.

palhoye commented 2 months ago

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

rstoyanchev commented 2 months ago

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.

palhoye commented 2 months ago

Understand.

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

rstoyanchev commented 2 months ago

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.