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

@defer directive should work with mutations #2099

Closed
alessbell opened this issue Nov 14, 2022 · 8 comments · Fixed by #2102 or #2109
Closed

@defer directive should work with mutations #2099

alessbell opened this issue Nov 14, 2022 · 8 comments · Fixed by #2102 or #2109
Assignees

Comments

@alessbell
Copy link
Contributor

Describe the bug
Mutations should also support @defer. Given the following mutation:

  mutation ($userId: ID!, $paymentInformation: PaymentInformationInput!) {
    makePayment(userId: $userId, paymentInformation: $paymentInformation) {
      id
      ... @defer {
        paymentStatus {
          __typename
          id
          ... on PaymentSuccess {
            billedAmount
          }
          ... on PaymentFailed {
            reason
          }
        }
      }
    }
  }

when following the reproduction steps below, the router returns the error cannot query field 'makePayment' on type 'Query'. The same mutation returns a multipart response when used with Apollo Server 4.

I've tried several different syntaxes using @defer with mutations in the router but always receive an error.

To Reproduce
Steps to reproduce the behavior:

  1. Checkout chore: test mutation with defer client-router-e2e-tests#15
  2. Follow the README instructions to bring up the router and start the front-end dev server
  3. Visit http://localhost:3000/purchase, open the network tab and click the "Make Payment" button
  4. Observe the error cannot query field 'makePayment' on type 'Query'

Expected behavior
A multipart response with the fragment marked with @defer arriving after the initial chunk.

@jpvajda
Copy link
Contributor

jpvajda commented Nov 15, 2022

@abernix it looks like this issue has a Draft PR, is the plan to deliver this improvement for the upcoming defer GA? We have a related issue on the client to consider for this change. apollographql/apollo-client#10272 cc @alessbell

@abernix
Copy link
Member

abernix commented Nov 16, 2022

@jpvajda My current understanding is that this issue is actually about more than just mutations — it's actually a more general bug in the execution of the query plan that is being encountered here. There's a Router-based bug here an I think we may update the title of this issue to reflect more precisely what it is pending a bit more understanding.

The solution here will require that we add a bit of information to our query plans or derive the information details another way (ourselves) from the schema/operation. @Geal is working on a solution to that.

Our intention is to ship this as a bug fix as soon as possible, whenever that is. Do you expect to require some matching changes on Client-side for this specifically? My surface-level assessment is that the issue you linked to is not necessarily directly connected to this and perhaps still something that needs fixing, but that you encountered a different error when trying to reproduce this with the Router. Does that track with you/others?

@alessbell
Copy link
Contributor Author

@abernix thanks for that additional context, and yes that absolutely tracks (insofar as the client bug will be fixed in a patch release independent of this issue).

@Geal
Copy link
Contributor

Geal commented Nov 17, 2022

I think we could merge #2102 first, which fixes this issue, then move on to the larger change in #2109

@abernix
Copy link
Member

abernix commented Nov 18, 2022

That sounds good to me, @Geal

Geal pushed a commit that referenced this issue Nov 24, 2022
Fix #2099

to validate the primary part of a request with `@defer`, we reconstruct
a partial query by walking through the query plan. The code responsible
for that was not detecting mutations.

while the PR in the current state fixes the issue, I'd like to refactor
a bit so that we do not special case query and mutation (which will be
annoying if we add subscription at some point), and moving the query
reconstruction elsewhere, to work directly on the query instead of the
query plan
garypen pushed a commit that referenced this issue Nov 30, 2022
Fix #2099

to validate the primary part of a request with `@defer`, we reconstruct
a partial query by walking through the query plan. The code responsible
for that was not detecting mutations.

while the PR in the current state fixes the issue, I'd like to refactor
a bit so that we do not special case query and mutation (which will be
annoying if we add subscription at some point), and moving the query
reconstruction elsewhere, to work directly on the query instead of the
query plan
@BrynCooke BrynCooke modified the milestones: v1-NEXT, v1.5.0 Dec 2, 2022
@garypen garypen added this to the v1.5.0 milestone Dec 5, 2022
@BrynCooke BrynCooke modified the milestone: v1.5.0 Dec 5, 2022
Geal pushed a commit that referenced this issue Dec 12, 2022
Fix #2105 
Fix #2099 

When we are using `@defer`, response formatting must apply on a subset
of the query (primary or deferred), that is reconstructed from
information provided by the query planner: a path into the response and
a subselection. Previously, the query planner did not add fragments to
that path, which resulted in reconstruction issues for queries like:

```graphql
query {
  me {
    ... on User {
      id
      fullName
      memberships {
        permission
        account {
          ... on Account @defer {
            name
          }
        }
      }
    }
  }
}
```

we would get a deferred part with the path `/me/memberships/account` and
the subselection `{ name }`. The path does not give enough information
to reconstruct the query when `me` returns an interface: we cannot know
which type is used.

The query planner 2.2.2 introduces a fragment in the path to solve that,
like `/me/... on User/memberships/account`. It also removes the
'flatten' element from deferred nodes paths, so the code has to be
adapted a bit.
@dkogithub
Copy link

Dear Apollo Team, we just started using apollo http link to handle @defer, I'm a bit confused how it works with mutations, as I understood everything get updated when final patch is came, which is a bit breaking the whole idea of @defer, I found this fix which is introduced it apollographql/apollo-client#10368.

Is there any plan to get updated UI patch by patch, to be able to get achievements from defer directive or maybe you can suggest any workaround?

The situation is next - we have a configurator with deferred prices fields and we would like to update UI and give access to user to do the next configuration action before prices come. Pricing is quite complicated and slow in some cases, we would like to do not block the user.

Can you please advice?

@alessbell
Copy link
Contributor Author

Hi @dkogithub 👋

The client.mutate API in Apollo Client was designed to resolve with a single result, so when adding basic @defer support to client.mutate in that PR you referenced above which shipped at a patch version, breaking changes were off the table. This meant we'd have to collect the chunks and resolve them once at the end; as you correctly note, this approach doesn't leverage the power of @defer.

In sum, this has everything to do with Apollo Client and nothing to do with the Router :) I've gone ahead and created a feature request you can subscribe to since this will require a more fundamental change to the client.mutate API.

Let me know if you have any other questions over on that issue, thanks!

@dkogithub
Copy link

Hi @alessbell, many thanks for the your quick response and clarification! Glad to hear it's already planned to do, I will follow the feature then, thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants