Default Content-Type header to application/json if omitted

[michaelsproul]

Description

It would make our HTTP API slightly more ergonomic to use via curl if it didn't error when the Content-Type header is omitted.

This seems to be acceptable according to the HTTP spec:

https://stackoverflow.com/questions/15860742/is-content-type-mandatory-in-http-post-request

I think using application/json as the default for most endpoints is OK, as that's usually what one is munging around on the CLI. If the user is sending binary they should be savvy enough to set the header (we could maybe log a warning if the error message doesn't already make it clear enough that Lighthouse attempted to parse JSON data).

[eserilev]

I'd like to work on this

[eserilev]

EDIT
I think a solution for this issue specifically is to go endpoint by endpoint and make sure we arent rejecting when content-type json isn't included

When we include the filter .and(warp::body::json()) the filter checks for Content-Type json in the header. If the header doesn't exist, it rejects the request. A small tweak to this filter will resolve this issue.

I put together a big write up on what I had done so far down below, but I think I may have gone down a wrong path here.

---

I was under the impression that I'd be able to simply use some warp filter to append a Content-Type header if none existed. However this is not necessarily the case. Firstly, request headers in warp are immutable, so in order to append a new header, I would need to clone the existing headers, append a new header to the cloned list and then replace the existing request headers with the "new" headers.

I tried doing something like this

fn default_content_type(log: Logger)->  impl Filter<Extract = (HeaderMap,), Error = Infallible> + Copy {
    warp::header::headers_cloned()
        .map(|mut headers:HeaderMap| {
            if headers.get("Content-Type").is_none() {
                headers.insert("Content-Type", HeaderValue::from_static("application/json"));
                println!("{:?}", headers);
            }
            headers
        })
}

and then updating the route filters

 let routes = warp::get()
        .and(default_content_type(log.clone))
        .and( the GET route filters)
        ...
        ...
         .uor(
            warp::post().and(default_content_type(log.clone))
            .and(the POST route filters)

however the return type of default_content_type doesn't play nice with some of the code downstream. Once we start getting into the with statements toward the end of the route filters I get the error message

the trait `Reply` is not implemented for `(HeaderMap, warp::generic::Either<(warp::http::Response<warp::hyper::Body>,), (impl Reply,)>)`

If I update default_content_type to

fn default_content_type(log: Logger)->  impl Filter<Extract = (), Error = Infallible> + Copy {
    warp::header::headers_cloned()
        .map(|mut headers:HeaderMap| {
            if headers.get("Content-Type").is_none() {
                headers.insert("Content-Type", HeaderValue::from_static("application/json"));
                println!("{:?}", headers);
            }
        })
}

things work fine, however the updated header doesn't actually get passed downstream, so this becomes a useless filter. I am beginning to think that warp filters weren't really built to update request headers like this. Theres another option here to completely rebuild the request from scratch and append the missing header, this sounds a lot like a proxy server.

If we were to implement some sort of proxy server, I dont think warp filters are the right tool for the job. There is a crate I came across called tower: https://docs.rs/tower/latest/tower/ that seems to be the right tool for this. It is compatible with warp and would allow us to easily build middleware/proxy server. We could use the proxy server to check for the existence of the Content-Type header, if it doesn't exist we can add the Content-Type header and then pass the request along to the beacon api. A proxy server might also offer some additional benefits as an intermediary layer between the external world and the beacon api. This is a relatively big change to the underlying beacon api infra, so the proxy server would probably need to provide more utility than just making curl requests easier for users. Otherwise it seems like a big change for such a small use case.

I've spent a good amount of time on this. I could be completely missing the boat here, maybe my warp filter is just not set up correctly and a small code change could resolve this entirely. But this is the conclusion I came to based on the work I've done so far.

[michaelsproul]

There is potentially another option here to go endpoint by endpoint and alway make sure were defaulting to JSON even if the content-type header isnt included. I think most endpoints already do this?

Yeah I think this might be the most viable approach. First step would be identifying which endpoints don't work without a content-type provided

[eserilev]

Yeah I think this might be the most viable approach. First step would be identifying which endpoints don't work without a content-type provided

added some edits to my initial comments, I think I know what to do now