router

router copied to clipboard

Fix #1219 Fix #1409

we need a specific function to map over deferred responses, so we can make the distinction with the first one, where we have access to headers

@Geal your pull request is missing a changelog!

it would be possible to do everything in the same response, but then we need to address the issue of header edition: headers can only be modified when looking at the first response, not deferred ones (otherwise we would have to buffer the entire http response).

Another thing I worry about here is how to link deferred responses. It looks like map_response and map_deferred_response are called independently, so there's no way to know in which serie of responses it is executing, is that correct? I guess that's the same for map_request vs map_response as well, so if we want to keep common info between responses we have to use the context?

Ideally I'd like an API like this:

fn router_service(service) {                                                
    let f = |response| {                                        
        let start = apollo_start.elapsed;                                                   
        let duration = apollo_start.elapsed - start;            
        print(`response processing took: ${duration}`);                          
        print(response.body.errors); 

         let g = |response| {                                        
                let start = apollo_start.elapsed;                       
                let duration = apollo_start.elapsed - start;            
                print(`deferred response processing took: ${duration}`);               
                print(response.body.errors);                            
         }; 
         response.map_deferred(g);                                      
    };                                                                             
    service.map_response(f);                                                            

But I am not sure it is achievable

ok, now it is a bit nicer with the same method used everywhere. Accessing the headers from a deferred response will throw:

fn supergraph_service(service) {                                 
    let f = |response| {                                         
        let start = apollo_start.elapsed;                        
        let duration = apollo_start.elapsed - start;             
        print(`response processing took: ${duration}`);          
        try {                                                    
          print(`headers: ${response.headers}`);                 
        } catch(err) {                                           
          print(`cannot access headers from a deferred response`)
        }                                                        
                                                                 
        print(response.body.errors);                             
    };                                                           
    service.map_response(f);                                     
}                                                                

should I add a is_deferred method to the response object?

Talked about this with @SimonSapin during our API:1.0 review and he raised the point that it will be difficult for a programmer to deal with runtime errors due to handling of deferred responses. It's difficult to know what the "best" solution is here, because it's clearly a compromise whatever we do. However, your is_deferred() suggestion is definitely pointing in another direction. Rather than making rhai scripters understand about defer/not defer, would it be better to have a function which does something like: headers_are_available()? So, the example would become:

fn supergraph_service(service) {                                 
    let f = |response| {                                         
        let start = apollo_start.elapsed;                        
        let duration = apollo_start.elapsed - start;             
        print(`response processing took: ${duration}`); 
        if response.headers_are_available() {
          print(`headers: ${response.headers}`);                 
        } else {
          print(`cannot access headers from a deferred response`)
        }                                                                 
        print(response.body.errors);                             
    };                                                           
    service.map_response(f);                                     
}                                                                

I think it's more idiomatic rhai to do what your example does and catch the exception rather than have the headers_are_available() method, but I'm bringing it up for discussion since I know Simon didn't like the fact that dealing with runtime errors was a surprise and a bit of a pain.

I think with documentation and judicious use of try...catch, the best outcome is available, since even if we added headers_are_available() we'd still have to throw an error if they weren't...

ok so the headers_are_available method is there, but maybe the name should be more explicit about its usage? Somethings like response_headers_available because the headers would obviously be available in the request?

ok so the headers_are_available method is there, but maybe the name should be more explicit about its usage? Somethings like response_headers_available because the headers would obviously be available in the request?

I think the name is clear enough already since it's a method on a Response, so you already know you are dealing with a response.

I still think it would be better to not have this function and just handle the exception like you did in your original example. @SimonSapin could you look at the examples and offer up an opinion?

BTW: I imagine for your example that won't work right now since we haven't added support for accessing headers on a DeferredResponse. We'll need something like:

    #[rhai_fn(get = "headers", pure, return_raw)]
    pub(crate) fn get_originating_headers_execution_deferred_response(
        _obj: &mut SharedMut<execution::DeferredResponse>,
    ) -> Result<HeaderMap, Box<EvalAltResult>> {
        Err("cannot access headers on a deferred response.".into())
    }

and the same again for router::DeferredResponse. And the same pairing for setting of headers...

The example works because it throws due to a missing function instead of an explicit error