search

graphql-rust / juniper

GraphQL server library for Rust
Other
5.7k stars 423 forks source link

`Request body consumed multiple times` with juniper_warp #844

Open crocme10 opened 3 years ago

crocme10 commented 3 years ago

Hello,

I've updated a server I had running, with more recent versions of juniper and juniper_warp. I ran into an error, which I have been able to reproduce in the following minimal test case:

Running the starwars example found in the juniper_warp folder, and running a simple hero query against the interface will result in an error Request body consumed multiple times.

I've created a repo which contains the example from juniper_warp, as well as the fixtures so that the example is self-contained. You should just do the following:

git clone https://github.com/crocme10/test-juniper
cd test-juniper
cargo build --release
RUST_LOG=debug ./target/release/test-juniper&
curl -X POST "http://localhost:8080/graphql" -H 'Content-Type: application/json' --data-binary @payload.json
Request body consumed multiple times⏎

I would not expect any error.

This occur on a Linux machine, with rust 1.48

Thank you for your help

LegNeato commented 3 years ago

Thanks for the report and the repro! I'll take a look tomorrow

crocme10 commented 3 years ago

Hello, the issue is still present with updated depencies (0.15.2).

crocme10 commented 3 years ago

Hello, so I looked a bit into this...

If I change the function make_graphql_filter function by removing the support for the post_json_filter, I don't have the error anymore. So I suspect the regression, if there is one, must have been introduced by PR 654. My understanding is that warp cannot have support for both json and graphql... This is probably a very summary judgement because I haven't looked into the details of the PR.

This means, somewhere around here

This does not work:

get_filter.or(post_json_filter).unify().or(post_graphql_filter).unify().boxed()

This works (no consumed multiple times error):

get_filter.or(post_graphql_filter).unify().boxed()

Of course this change is probably not desirable cause the json is there for a reason and that would remove useful functionality, but that might put someone on the right track...

LegNeato commented 3 years ago

Can this still be reproduced on master?