juniper_warp::make_graphql_filter improperly returns 4xx on some context_extractor 5xx errors

[scottlamb]

Describe the bug
with juniper_warp = "0.7.0", this sequence can happen:

1. request comes in with valid application/json body.
2. post_json_filter runs; context_extractor fails for some transient reason (database error in my case), returns some Rejection that should produce a 5xx error.
3. then post_graphql_filter runs. context_extractor happens to succeed this time. post_graphql_filter returns a 4xx rejection about a parse error because the body's actually in json format.

To Reproduce
Steps to reproduce the behavior:
(code example preferred)

failing unit test diff

diff --git a/juniper_warp/src/lib.rs b/juniper_warp/src/lib.rs
index 7b524d2a..7567062f 100644
--- a/juniper_warp/src/lib.rs
+++ b/juniper_warp/src/lib.rs
@@ -449,7 +449,7 @@ pub mod subscriptions {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use warp::{http, test::request};
+    use warp::{http, test::request, reject::Reject};
 
     #[test]
     fn graphiql_response_does_not_panic() {
@@ -642,6 +642,52 @@ mod tests {
 
         assert!(result.is_err());
     }
+
+    #[tokio::test]
+    async fn context_extractor_failure() {
+        use juniper::{
+            tests::fixtures::starwars::schema::{Database, Query},
+            EmptyMutation, EmptySubscription, RootNode,
+        };
+
+        type Schema =
+            juniper::RootNode<'static, Query, EmptyMutation<Database>, EmptySubscription<Database>>;
+
+        let schema: Schema = RootNode::new(
+            Query,
+            EmptyMutation::<Database>::new(),
+            EmptySubscription::<Database>::new(),
+        );
+
+        let called = Arc::new(std::sync::atomic::AtomicBool::new(false));
+        #[derive(Debug)]
+        struct MyRejection;
+        impl Reject for MyRejection {}
+        let context_extractor: warp::filters::BoxedFilter<(Database,)> = warp::any().and_then(move || {
+            std::future::ready(if called.swap(true, std::sync::atomic::Ordering::Relaxed) {
+                println!("second call");
+                Ok(Database::new())
+            } else {
+                println!("first call");
+                Err(warp::reject::custom(MyRejection))
+            })
+        }).boxed();
+        let filter = warp::path("graphql").and(make_graphql_filter(schema, context_extractor));
+        let response = request()
+            .method("POST")
+            .path("/graphql")
+            .header("accept", "application/json")
+            .header("content-type", "application/json")
+            .body(
+                r##"[
+                     { "variables": null, "query": "{ hero(episode: NEW_HOPE) { name } }" },
+                     { "variables": null, "query": "{ hero(episode: EMPIRE) { id name } }" }
+                 ]"##,
+            )
+            .reply(&filter)
+            .await;
+        assert_eq!(response.status(), http::StatusCode::INTERNAL_SERVER_ERROR);
+    }
 }
 
 #[cfg(test)]

Expected behavior
It should run my context_extractor only once per request. Any Rejections returned from the context_extractor should be returned faithfully from the combined filter returned from make_graphql_filter, rather than effectively turning a 5xx error into a 4xx error.

Additional context
Add any other context about the problem here.

[scottlamb]

I'm having trouble understanding warp's filter model; possibly my context_extractor is supposed to do something special to say that its error is terminal? but make_graphql_filter's behavior seems suboptimal at least:

• it's not great to call context_extractor twice at least from a efficiency/load perspective, and
• it's not great to try parsing an application/json body as graphql, and then to report that error instead of the one from the first context_extractor call

[LegNeato]

I personally haven't used warp so I will let other maintainers or community members chime in on this.

[tyranron]

@scottlamb sorry for responding late on this.

but make_graphql_filter's behavior seems suboptimal at least:

• it's not great to call context_extractor twice at least from a efficiency/load perspective, and
• it's not great to try parsing an application/json body as graphql, and then to report that error instead of the one from the first context_extractor call

This behavior is definitely wrong. Especially, considering possible non-idempotent mutation semantics.

It seems that we're missing reject in our .or() filter chains, so failing one branch switches to another one without rejection as desired.

[tyranron]

@scottlamb upon further investigation, it seems that everything is relatively fine in juniper_warp, but a big misunderstanding of warp filters has place. See seanmonstar/warp#388 (comment) for the main insight:

Rejections are meant to say a Filter couldn't fulfill its preconditions, but maybe another Filter can. If a filter is otherwise fully matched, and an error occurs in your business logic, it's probably not correct to reject with the error. In that case, you'd want to construct a Reply that describes your error.

In the todos example, once the request has gotten through the filters and arrived to the handler function, it chooses to return responses with the correct status codes instead of rejecting.

It may be worth adding some pattern to ease that?

So, every time we return Err(warp::reject::something()), we don't actually say "abort this request completely", we do say "reject this branch of filters and try another one, if any", and thus, we do trigger, another .or() branch.

This exactly what happens in your example:

let context_extractor: warp::filters::BoxedFilter<(Database,)> = warp::any().and_then(move || {
    std::future::ready(if called.swap(true, std::sync::atomic::Ordering::Relaxed) {
        println!("second call");
        Ok(Database::new())
    } else {
        println!("first call");
        Err(warp::reject::custom(MyRejection))
    })
}).boxed();

Here, the post_graphql_filter runs after trying the branch with post_json_filter exactly because the context_extractor filter rejects this branch and switches to another one. Nothing in juniper_warp triggers it.

To avoid switching to another .or() branch, we should use the .recover() filter, where do the inspection whether the Rejection contains our cause, and if so, return a Reply immediately, or reject into other branches.

However, we cannot write such .recover() filter inside juniper_warp, because we cannot know the exact user-defined error being used. That's why, it should be specified from the user-side, along with the context_extractor (something like this):

let context_extractor: warp::filters::BoxedFilter<(Database,)> = warp::any()
.and_then(move || {
    std::future::ready(if called.swap(true, std::sync::atomic::Ordering::Relaxed) {
        println!("second call");
        Ok(Database::new())
    } else {
        println!("first call");
        Err(warp::reject::custom(MyRejection))
    })
})
.recover(|rejection| {
    // `MyRejection` should impl `warp::Reply`
    // and `Clone`, because `find` returns `Option<&T>` only =(
    rejection.find::<MyRejection>().cloned().ok_or(rejection) 
})
.boxed();

In #1222 I'll describe this clearly in the docs and will provide an example.

Also, regarding rejects inside make_graphql_filter, it seems that some other cases should be handled too (like failing body() or query() filters).

[tyranron]

@scottlamb so, in the end, I've ended up rewriting juniper_warp a bit in the way that make_graphql_filter() executes context_extractor only once. However, Rejections, returned by context_extractor, still should be handled outside, to avoid branching into other Filters. I've added an example demonstrating this.