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.

Sign up for GitHub

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

Jump to bottom

bug: incoming http POST queries are flagged as GET queries by the router #865

Closed
neslinesli93 opened this issue Apr 15, 2022 · 2 comments · Fixed by #873
Closed

bug: incoming http POST queries are flagged as GET queries by the router #865

neslinesli93 opened this issue Apr 15, 2022 · 2 comments · Fixed by #873
Assignees
@o0Ignition0o
Labels
raised by user

Comments

@neslinesli93
Copy link
Contributor

Describe the bug
After trying the router binary built from master, it looks like every operation performed on subgraphs uses a GET request. This is not a problem for queries, but it's a problem for mutations since our graphql server that implements a subgraph rejects mutation done with a GET rather than a POST. Maybe this started happening after the switch to axum?

To Reproduce
Steps to reproduce the behavior:

  1. Perform a mutation to the router
  2. The router returns 405 if the subgraph dealing with it has no support for mutation done with GET requests

Expected behavior
The router should work with both graphql queries and mutations
@Geal
Copy link
Contributor

Geal commented Apr 15, 2022

thanks for the report, this is a regression, we'll fix that soon

@o0Ignition0o o0Ignition0o self-assigned this Apr 19, 2022
o0Ignition0o added a commit that referenced this issue Apr 19, 2022
@o0Ignition0o
fix: Add a test that makes sure the fetch requests are POST
2b9ad50 
fixes #865

When writing a unit test to assert subgraph requests are POST, I noticed the test would never fail. this is because withf only acts as a request router, and panicking (or in our case asserting) within wouldnt cause the router to fail.

This commit rewrites a couple of subgraph tests so they properly notify whether they completed successfully or failed.
@o0Ignition0o o0Ignition0o mentioned this issue Apr 19, 2022
Merged
o0Ignition0o added a commit that referenced this issue Apr 20, 2022
@o0Ignition0o
fix: Correctly flag incoming POST requests.
3b2157e 
fixes #865

A regression happened during our switch to Axum which would mark incoming POST requests as GET, thus triggering our forbid_http_mutations layer, and preventing mutations on the router.
This PR fixes this, and introduces some tests. More integration or axum oriented tests should follow.

When writing a unit test to assert subgraph requests are POST, I noticed the test would never fail. this is because withf only acts as a request router, and panicking (or in our case asserting) within wouldnt cause the router to fail.

This commit rewrites a couple of subgraph tests so they properly notify whether they completed successfully or failed.
@o0Ignition0o o0Ignition0o changed the title Router performs GET requests to subgraphs bug: incoming http POST queries are flagged as GET queries by the router Apr 20, 2022
@o0Ignition0o
Copy link
Contributor

Ok this was a tricky one, but I've found and fixed it this pull request.

We will thus merge it and cut a new release, hopefully tomorrow!

TLDR:
Incoming HTTP POST queries would be extracted as HTTP GET queries in our axum service handler, which would trigger the forbid_http_get_mutations plugin.

o0Ignition0o added a commit that referenced this issue Apr 21, 2022
@o0Ignition0o
fix: Correctly flag incoming POST requests. (#873)
91733b5 
fixes #865

A regression happened during our switch to Axum which would mark incoming POST requests as GET, thus triggering our forbid_http_mutations layer, and preventing mutations on the router.
This PR fixes this, and introduces some tests. More integration or axum oriented tests should follow.

When writing a unit test to assert subgraph requests are POST, I noticed the test would never fail. this is because withf only acts as a request router, and panicking (or in our case asserting) within wouldnt cause the router to fail.

This commit rewrites a couple of subgraph tests so they properly notify whether they completed successfully or failed.
@BrynCooke BrynCooke mentioned this issue Apr 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@o0Ignition0o o0Ignition0o

Labels
raised by user
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Correctly flag incoming POST requests. apollographql/router
3 participants
@Geal @neslinesli93 @o0Ignition0o