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

Coprocessor with streaming responses panics when supergraph_response is configured #4013

Closed
BrynCooke opened this issue Oct 11, 2023 · 3 comments · Fixed by #4014
Closed
Assignees

Comments

@BrynCooke
Copy link
Contributor

BrynCooke commented Oct 11, 2023

The logic for supporting deferred responses can't work.

The issues is here:

let payload = Externalizable::router_builder()
.stage(PipelineStep::SupergraphResponse)
.

This will always panic as the wrong builder is used for the stage

Reproduction
Configure coprocessor.supergraph.response in router.yaml, for instance:

coprocessor:
  supergraph: 
    response:
      body: true

Send a query to the router that uses @defer
Router will panic.

Mitigation
Anyone who is using a coprocessor with coprocessor.supergraph.response configured should explicitly disable defer and not use subscriptions.
OR
Disable coprocessor.supergraph.response

@BrynCooke BrynCooke self-assigned this Oct 11, 2023
BrynCooke added a commit that referenced this issue Oct 11, 2023
…4014)

The logic for supporting deferred responses can't work.

The issues is here:
[router/apollo-router/src/plugins/coprocessor/supergraph.rs](https://github.com/apollographql/router/blob/cf4a79a126132eb30e368bdf2277a89278ae2a9b/apollo-router/src/plugins/coprocessor/supergraph.rs#L431-L432)

This will always panic as the wrong builder is used for the stage.

Fixes #4013

<!-- start metadata -->
---

**Checklist**

Complete the checklist (and note appropriate exceptions) before the PR
is marked ready-for-review.

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]: It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]: Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]: Tick whichever testing boxes are applicable. If you are adding
Manual Tests, please document the manual testing (extensively) in the
Exceptions.

Co-authored-by: bryn <[email protected]>
@BrynCooke BrynCooke changed the title Coprocessor with streaming responses doesn't work Coprocessor with streaming responses panics Oct 13, 2023
@garypen
Copy link
Contributor

garypen commented Oct 13, 2023

I think your description is wrong in the issue. It's supergraph logic that is mistakenly using a router builder, so the impact is on the new supergraph coprocessor.

It should be:

Reproduction
Configure coprocessor.supergraph_response
Send a query to the router that uses @defer
Router will panic.

Mitigation
Anyone who is using a coprocessor with supergraph_response configured should explicitly disable defer and not use subscriptions.
OR
Disable coprocessor supergraph_response

@BrynCooke BrynCooke changed the title Coprocessor with streaming responses panics Coprocessor with streaming responses panics when supergraph_response is configured Oct 13, 2023
@BrynCooke
Copy link
Contributor Author

You are right, I've updated it.

@o0Ignition0o
Copy link
Contributor

o0Ignition0o commented Oct 16, 2023

#3647 is when the bug was introduced, so router [1.31.0..=1.32.0] are affected

This was referenced Oct 17, 2023
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

Successfully merging a pull request may close this issue.

3 participants