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

Multipart/mixed incremental delivery transport missing boundary #3381

Closed
phughes-scwx opened this issue Nov 21, 2024 · 1 comment
Closed

Comments

@phughes-scwx
Copy link
Contributor

phughes-scwx commented Nov 21, 2024

What happened?

When the multipart response aggregator introduced in #3357 aggregates the initial response with a deferred response in the same cycle, it fails to write the boundary between them.

Using a separate resolver for user in Todo for the init schema, when you send the curl command:

curl --location 'http://localhost:8080/query' \
  --header 'Content-Type: application/json' 
  --header 'Accept: multipart/mixed' \
  --data '{"query":"query { todos { id ... on Todo @defer { user { id } } } }"}'

Note: the ... on Todo @defer { ... } syntax instead of ... @defer { ... }: this is necessary due to a separate bug in field collection identified in Issue #1972. I have a separate PR to address that bug that I will submit after this bug is fixed.

... you receive:

--graphql
Content-Type: application/json

{"data":{"todos":[{"id":"1","user":null}]},"hasNext":true}Content-Type: application/json

{"incremental":[{"data":{"user":{"id":"1"}},"path":["todos",0],"hasNext":false}],"hasNext":false}
--graphql--

What did you expect?

--graphql
Content-Type: application/json

{"data":{"todos":[{"id":"1","user":null}]},"hasNext":true}
--graphql
Content-Type: application/json

{"incremental":[{"data":{"user":{"id":"1"}},"path":["todos",0],"hasNext":false}],"hasNext":false}
--graphql--

Minimal graphql.schema and models to reproduce

This is simple to generate, but takes some setup: you can take the gqlgen init output, but add a separate resolver for a field like user on Todo, start a server with the line below, and then query with the above curl command, it returns the response.

srv.AddTransport(transport.MultipartMixed{
	Boundary:        "graphql",
	DeliveryTimeout: time.Millisecond * 100, // Actually trigger aggregation.
})

But the easier way to do this is to make a small change to the DeliveryTimeout in the test here.

Versions

  • go run github.com/99designs/gqlgen version: v0.17.56-dev
  • go version: go version go1.22.5 darwin/arm64

I have already written a test that surfaces this bug and a solution, and will submit a PR and link.

@giulio-opal
Copy link
Contributor

@StevenACoffman sorry about this issue, I just noticed this myself as well. Thanks @phughes-scwx for fixing it! I was about to make a similar fix, by changing the if len(a.deferResponses) > 0 to an if else, but this works too.

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

No branches or pull requests

2 participants