lychee icon indicating copy to clipboard operation
lychee copied to clipboard

Response chain

Open thomas-zahner opened this issue 1 year ago • 1 comments

The Client currently has a plugin_request_chain but no plugin_response_chain. It might make sense to add a response chain.

In the following snippet we can see how the request chain is traversed and we get the status. handle_github is the perfect candidate for an element in the internal default response chain.

let status = ClientRequestChains::new(vec![&self.plugin_request_chain, default_chain])
    .traverse(request)
    .await;

self.handle_github(status, uri).await

The question is how we define the ResponseChain type. Do we want to include the response body? This way plugin users could handle complicated use cases based on the response body.

pub type ResponseChain = Chain<Status, Status>;

or

pub type ResponseChain = Chain<(Body, Status), Status>;

thomas-zahner avatar May 15 '24 15:05 thomas-zahner

Yes, I like the symmetry between plugin_request_chain and plugin_response_chain. Regarding the proposed ResponseChain type, I believe including the response body would offer more versatility:

pub type ResponseChain = Chain<(Body, Status), Status>;

This approach would allow plugin users to handle complex scenarios based on both the status and the response body. It provides maximum flexibility while still allowing simpler plugins to ignore the body if not needed.

Some potential benefits of a response chain:

  1. Custom error handling or retries based on specific response contents
  2. Response transformation or sanitization
  3. Logging or metrics collection on responses

However, we should consider the performance impact of passing large response bodies through the chain.

Before proceeding, it might be helpful to gather more use cases from the community to ensure this addition would be broadly useful and prototype the change to assess its impact on performance and existing code

Would you like to proceed with prototyping this feature?

mre avatar Oct 08 '24 22:10 mre