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 not working for mutation response #994

Closed
palhoye opened this issue Jun 7, 2024 · 15 comments
Closed

@BatchMapping not working for mutation response #994

palhoye opened this issue Jun 7, 2024 · 15 comments
Assignees
Labels
for: external-project Needs a change in external project

Comments

@palhoye
Copy link

palhoye commented Jun 7, 2024

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.

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

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

@palhoye
Copy link
Author

palhoye commented Jun 30, 2024

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

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 rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Jul 3, 2024
@spring-projects-issues
Copy link
Collaborator

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 spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Jul 10, 2024
@spring-projects-issues
Copy link
Collaborator

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 spring-projects-issues added status: waiting-for-triage An issue we've not yet triaged and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Jul 17, 2024
@rstoyanchev rstoyanchev removed the status: waiting-for-triage An issue we've not yet triaged label Jul 18, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 18, 2024
@MateusDadalto
Copy link

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

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

rstoyanchev commented Sep 4, 2024

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

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

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Sep 4, 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 labels Sep 4, 2024
@palhoye
Copy link
Author

palhoye commented Sep 4, 2024

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

rstoyanchev commented Sep 4, 2024

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 rstoyanchev reopened this Sep 4, 2024
@rstoyanchev rstoyanchev added type: documentation A documentation task and removed for: external-project Needs a change in external project labels Sep 4, 2024
@rstoyanchev rstoyanchev added this to the 1.2.9 milestone Sep 4, 2024
@rstoyanchev rstoyanchev changed the title @BatchMapping not working for mutation response Update documentation with advice on batch loading for mutations Sep 4, 2024
@palhoye
Copy link
Author

palhoye commented Sep 4, 2024

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

@rstoyanchev rstoyanchev removed this from the 1.2.9 milestone Sep 5, 2024
@rstoyanchev rstoyanchev added this to the 1.3.3 milestone Sep 5, 2024
@rstoyanchev rstoyanchev self-assigned this Sep 5, 2024
@rstoyanchev
Copy link
Contributor

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 rstoyanchev modified the milestones: 1.3.3, 1.x Backlog Sep 12, 2024
@rstoyanchev rstoyanchev added the status: blocked An issue that's blocked on an external project change label Sep 12, 2024
@palhoye
Copy link
Author

palhoye commented Sep 12, 2024

Understand.

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

@rstoyanchev rstoyanchev changed the title Update documentation with advice on batch loading for mutations @BatchMapping not working for mutation response Sep 23, 2024
@rstoyanchev rstoyanchev removed the type: documentation A documentation task label Sep 23, 2024
@rstoyanchev
Copy link
Contributor

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

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Sep 23, 2024
@rstoyanchev rstoyanchev removed this from the 1.x Backlog milestone Sep 23, 2024
@rstoyanchev rstoyanchev added for: external-project Needs a change in external project and removed status: blocked An issue that's blocked on an external project change labels Sep 23, 2024
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

4 participants