implement map_response for deferred responses in rhai

[Geal]

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

[garypen]

This looks great and it's nice to see the rhai binding catching up with the defer enhancements.

I have some questions about how to use this. It seems like an implementation of RouterService or ExecutionService has to decide to either map_response() or map_deferred_response(). I think it would be nice to actually "hide" this decision from rhai scripters if possible, so that map_deferred_response is implicitly chosen for Router and Execution services that call map_response and not possible to be chosen for QueryPlanning and Subgraph Services.

(I'm aware we can't force this kind of decision on rust plugin users, so we have to expose the complexity there, but it would be really nice to hide this from rhai scripters.)

Once this is resolved, we need to think about how best to document this new capability in the router doc pages for rhai.

[Geal]

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

[Geal]

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?

[garypen]

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

[Geal]

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?

[garypen]

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

[Geal]

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