`juniper_warp::make_graphql_filter` improperly returns 4xx on some `context_extractor` 5xx errors about juniper HOT 5 CLOSED

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

from juniper.

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

from juniper.

@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.

from juniper.

@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).

from juniper.

@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.

from juniper.