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 support with accept: multipart/mixed header rather than accept: text/event-stream #3335

Closed
giulio-opal opened this issue Oct 18, 2024 · 11 comments
Labels
apollo federation Related to Apollo federation SSE Server-Sent Events websocket

Comments

@giulio-opal
Copy link
Contributor

What happened?

I'm using Apollo GraphQL on the frontend, which automatically passes accept: multipart/mixed as header when the query contains a @defer directive. Unfortunately this causes the gqlgen server to expire the context and actually not return the deferred fields.

If I were to use accept: text/event-stream as header, the gqlgen server actually handles the context correctly and returns the deferred data.

Example:

 ▲ ~/work/gqlgen-todos curl 'http://localhost:8080/query' -H 'Connection: keep-alive' -H 'accept: text/event-stream' -H 'content-type: application/json; charset=utf-8' --data-raw '{"query":"query test { todos { ... on Todo @defer { user { id } } } }"}'
:

event: next
data: {"data":{"todos":[{"user":null},{"user":null}]},"hasNext":true}

event: next
data: {"data":{"user":{"id":"1"}},"path":["todos",1],"hasNext":true}

event: next
data: {"data":{"user":{"id":"1"}},"path":["todos",0],"hasNext":false}

event: complete

 ▲ ~/work/gqlgen-todos curl 'http://localhost:8080/query' -H 'Connection: keep-alive' -H 'accept: multipart/mixed' -H 'content-type: application/json; charset=utf-8' --data-raw '{"query":"query test { todos { ... on Todo @defer { user { id } } } }"}'
{"data":{"todos":[{"user":null},{"user":null}]},"hasNext":true}%

What did you expect?

Context to not expire and subsequent multipart responses returned to the frontend.

Is there an additional configuration that needs to be set up in the gqlgen server or in Gin that I might be missing?

Minimal graphql.schema and models to reproduce

Example repo based off the official documentation with gin: https://github.com/giulio-opal/defer-gqlgen-todo-example

It also contains a custom playground (GraphiQL) based off #2771 (comment)

versions

  • go run github.com/99designs/gqlgen version? v0.17.49+
  • go version? 1.22.0
@tlbonum
Copy link

tlbonum commented Oct 19, 2024

I'm experiencing the same issue as mentioned (but using echo instead of gin). An interesting observation is that if I send the query through an apollo router, it works. I know it's not a solution to the problem posted, just thought it would be worth mentioning.

@giulio-opal
Copy link
Contributor Author

I'm experiencing the same issue as mentioned (but using echo instead of gin). An interesting observation is that if I send the query through an apollo router, it works. I know it's not a solution to the problem posted, just thought it would be worth mentioning.

Unfortunately we don't use Apollo Router, but it's interesting nonetheless.

I've actually managed to implement my own transport for multipart/mixed which does work with Apollo Client. I'll publish a PR in case.

@giulio-opal
Copy link
Contributor Author

giulio-opal commented Oct 28, 2024

@StevenACoffman Hi Steven, it seems that my branch might have an issue upon further testing. With Apollo Client 3.11.8, if the server returns only one response, even with hasNext: false in its payload, it thinks that the communication is not over and leaves the network status as "1" (loading).

It's unclear to me from the defer spec, what's the expected response in case of a single response payload, so I can't determine if it's an issue with my code or Apollo Client.

For the time being there are some potential fixes:

  • Send an empty payload with "data": null, "hasNext": false if there was only one response
    This might be the preferred approach, in some cases, judging by how multipart/mixed payloads are handled by this code.
  • If the first response has hasNext: false, then downgrade the response header to Content-type: application/json, so potentially don't set any header until this line has returned.

The former is a little simpler to implement, but maybe the latter is the expected behavior. Either one should work with Apollo Client though. What is your thought? I can put together a PR based on your feedback.

Actually upon even further testing, it seems that just sending the boundary as last message, with no payload, eg.

--graphql
Content-Type: application/json

<EOF>

seems to fix any hanging issue.

@StevenACoffman StevenACoffman added websocket federation Related to Apollo federation apollo labels Oct 29, 2024
@StevenACoffman
Copy link
Collaborator

Yeah, I kind of prefer the first option based on what the Apollo code looks like. I love a PR!

@giulio-opal
Copy link
Contributor Author

Yeah, I kind of prefer the first option based on what the Apollo code looks like. I love a PR!

Thanks for the quick feedback! Another thing that I'm going to add is batching, so basically "group by" 1ms the deferred responses into one array of incremental: []. This way the Apollo Client feels more responsive and fewer payloads are sent through, especially when the deferred field is a value in an array.

This type of batching should be handled higher in the stack, though. Since the defer spec is very much in flight, and there might be other clients depending on the SSE implementation of defer, I'm going to hold off refactoring the rest of the code at this point.

@StevenACoffman StevenACoffman added the SSE Server-Sent Events label Oct 30, 2024
@StevenACoffman
Copy link
Collaborator

@giulio-opal Also, I'm curious what you (or anyone) use as an alternative to Apollo Router?

At work, we built our own closed-source GraphQL gateway but it still uses the Apollo Query Planner for our safelisted queries. Our GraphQL gateway is not a secret, but it's so bespoke, that it is not worth it to anyone else to open-source it.

@argoyle
Copy link

argoyle commented Oct 30, 2024

@giulio-opal Also, I'm curious what you (or anyone) use as an alternative to Apollo Router?

At work, we built our own closed-source GraphQL gateway but it still uses the Apollo Query Planner for our safelisted queries. Our GraphQL gateway is not a secret, but it's so bespoke, that it is not worth it to anyone else to open-source it.

I'm using a variant of the federation gateway example from here: https://github.com/wundergraph/graphql-go-tools/tree/master/examples/federation
Haven't tried it with @defer though. This is more or less what Wundergraph bases their Cosmo-router on.

@giulio-opal
Copy link
Contributor Author

We don't use Apollo Router as we don't have a need for federation yet.

I updated my previous PR with the improvements that I mentioned above: #3357
I did eventually find this doc specifying that you do need a final delimiter after the last payload: https://github.com/graphql/graphql-over-http/blob/main/rfcs/IncrementalDelivery.md#content-type-multipartmixed

Apollo Server seems to be doing the same as well: https://github.com/apollographql/apollo-server/blob/90832fd2b1e4be9f3fb4baf704f348f8975fd0ea/packages/server/src/runHttpQuery.ts#L319-L340

@giulio-opal
Copy link
Contributor Author

@StevenACoffman thank you for merging all my PRs! Any chance that we could do a minor release?

@StevenACoffman
Copy link
Collaborator

@giulio-opal Do you think that this issue can be closed? I was waiting to see if you had any remaining work in follow-up PRs before cutting a release.

@giulio-opal
Copy link
Contributor Author

I think I'm pretty happy with the new changes I've made, maybe only a further improvement could be moving the "incremental" logic higher in the stack, and potentially implement the full 2023 spec graphql/defer-stream-wg#69 but honestly I don't think Apollo Server does that right now either. In regards to this issue, it can be closed, now gqlgen does support "multipart/mixed" as request and we're in a much better state than we were 2 weeks ago :)

I'll open more tickets and/or submit more PRs if I find issues or I end up making further improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apollo federation Related to Apollo federation SSE Server-Sent Events websocket
Projects
None yet
Development

No branches or pull requests

4 participants