Customizable `Response`s for middlewares

kade-robertson commented 1 year ago

Hello!

This is partially related to https://github.com/algesten/ureq/issues/363 in terms of my personal use case, but I wanted this to be more of a generic request.

Currently, it isn't really possible to construct anything but a basic, text-only Response object (as in, one not generated by making a request). I've run into this limitation recently on one of my projects -- I've implementing a caching middleware here as a very basic replacement for the functionality I can get from something like http-cache-reqwest. This generally works okay for UTF-8 data such as serialized JSON responses like I primarily deal with, but falls apart when downloading images. I ended up having to add an escape hatch and a custom caching wrapper around my request to deal with that particular instance.

The current "faking" mechanism is almost what I'd expect, but the missing piece here is accepting a &[u8] content body and (possibly) a more structured way of specifying headers like Content-Type.

Maybe this doesn't make sense to overload Response::new with and rather a ResponseBuilder or some .set_body(&mut self)-like method would be the way to go.
Maybe Response and Request can #[derive(Serialize, Deserialize)]?

This might be leaning towards making this project more complex then it aims to be, but I think given that a middleware trait already exists, it makes sense to flesh this out a bit more than it is currently.

kade-robertson commented 1 year ago

Looking at this a bit more, maybe what makes the most sense is using http::Response as the tool for constructing the response, and have this crate's Response implement From<http::Response<T>>.

It seems reasonably thorough and provides the level of customization I'd be looking for I think
Relatively lightweight in terms of build time impact
Doesn't pull in too many additional dependencies
MSRV of the current version (0.2.8) is below what seems to be the MSRV of this version (1.53 or 1.56 based on the MSRV issue here)
Reasonably easy to put behind a feature if this isn't desired to be included by default

algesten commented 1 year ago

Hi! Thanks for thinking about this!

I think an optional dependency on the http crate would be good. Something that both allows conversion in both directions for both Request and Response (if possible, I haven't thought about this deeply. I.e. we would provide:

impl<T: what bound?!> From<http::Request<T>> for Request
impl From<Request> for http::Request<something good>
impl<T: what bound?!> From<http::Response<T>> for Response
impl From<Response> for http::Response<something good>

I guess the main question is what T and something good is.

kade-robertson commented 1 year ago

Looking at how other libraries do this:

reqwest implements impl<T: Into<Body>> From<http::Response<T>> for Response, where their body is more or less formatted how they want, and their body implements impl From<hyper::Body> for Body since their response is essentially a wrapper on hyper's response. They also implement some convenience conversions from Bytes, Vec<u8>, &'static [u8], String, &'static str, and files for their body.
surf is in a bit of an easier scenario, where their Response is a wrapper around http_client::Response. You can craft a custom surf response with Response::new(my_http_client_response). Given that surf is entirely based on using the http-rs project's various crates I don't think their implementations necessarily match what would work for this repo.

Based on how this project works (at least from what I can gather, I might be missing something), Stream is the closest thing that could be used as a reasonable bound, i.e. impl<T: Into<Stream>> From<http::Response<T>> for Response, but I think the issue there is the stream isn't really just the body -- it's the entire response. So the way to do this with only new, feature-gated code would be to have something like an impl<T: AsRef<[u8]>> From<http::Response<T>> for Stream (this may be challenging), and then have impl From<Stream> for Response just call .do_from_stream().

That doesn't feel particularly great to me but it avoids messing with the structure of the repo too much. Might also just be overthinking this. Given the reader for the body is fairly generic, this could be as simple as:

impl<T: Into<http::Body>> From<http::Response<T>> for Response {
    fn from from(r: http::Response<T>) -> Response {
        let code = r.status();
        let body = r.take_body();
        Response {
            url: "https://example.com".to_owned(),
            status_line: format!("{} {}", code, code.canonical_reason()),
            index: ResponseStatusIndex { http_version: 0, response_code: 0 },
            headers: r.iter().map(|h| h.into()),
            reader: body.into_reader(),
            remote_addr: () // make something up?,
            history: vec![]
        }
    }
}

The type for the reader http::Body creates is a bit different than the reader Response expects (Box<dyn AsyncBufRead + Unpin + Send + Sync + 'static>). A similar approach would probably work for Request but I haven't looked at that one as much.

In terms of the reverse direction, http::Body can be converted to/from a reader, string, or bytes, so it should be pretty compatible with this crates' body::Payload, leading to something like impl From<Request> for http::Request<http::Body>

algesten commented 1 year ago

I'm with you overall. The one thing I would want to look at closer is to not expose any of the internal types like Stream publicly. Generally I prefer to keep the API surface the same size as now.

If we need new exposed types, I rather wrap things like Stream in some level of abstraction outwards.

kade-robertson commented 1 year ago

I've created https://github.com/algesten/ureq/pull/591 as a preliminary look at what this might look like.

algesten commented 2 months ago

Closing since we're moving to ureq 3.x. The new version is based on the http crate which probably helps here.

algesten / ureq

Customizable `Response`s for middlewares #589