Add curl -H/--header option to globally add headers to all requests

[theoforger]

Description

This PR implements the curl option --header. It is the first part of #2144. For now this is a cli-only option.

What's happening in hurl/src/http/client.rs?

In curl, --header is able to override internal headers added by curl. For example:

# This will replace the user agent header set by curl
curl -H "User-Agent: some-value" https://example.com

Originally, at packages/hurl/src/http/client.rs#L534, the function set_headers pushes all headers into a list before applying them at the end. However, curl::easy::List doesn't seem to provide an easy way to override its items.

In this PR, I decide to store the headers in a Vec<String> instead. This way it's easier to iterate through them. Then the to_list function is called at the end to convert the vector to a curl::easy::List.

Do let me know if you prefer handing this a different way 🙏

[jcamiel]

Hi @theoforger

Thanks for the PR, I think we should change some things:

client.rs

1. What we want is --header to be able to add additional user header.

For example with this file test.hurl:

GET http://localhost:8000/hello

Running :

$ hurl --header "foo:toto" --header "bar:tutu" test.hurl --verbose
...
> GET /hello HTTP/1.1
> Host: localhost:8000
> Accept: */*
> User-Agent: hurl/6.0.0-SNAPSHOT
> foo: toto
> bar: tutu
...

If we define header in test.hurl

GET http://localhost:8000/hello
baz: titi

$ hurl --header "foo:toto" --header "bar:tutu" test.hurl --verbose
...
> GET /hello HTTP/1.1
> Host: localhost:8000
> Accept: */*
> User-Agent: hurl/6.0.0-SNAPSHOT
> foo: toto
> bar: tutu
> baz: titi
...

Headers defined in --header should be added to the headers defined in the file.

A particular case is if we use the same header name:

GET http://localhost:8000/hello
foo: titi

$ hurl --header "foo:toto" --header "foo:tutu" test.hurl --verbose
...
> GET /hello HTTP/1.1
> Host: localhost:8000
> Accept: */*
> User-Agent: hurl/6.0.0-SNAPSHOT
> foo: toto
> foo: tutu
> foo: titi
...

Even if the name is similar, we add user headers from the file and from the --header options.

2. In client.rs

I think there is subtil bug in the new code.

For instance we have this code:

fn set_headers(
        &mut self,
        request_spec: &RequestSpec,
        options: &ClientOptions,
    ) -> Result<(), HttpError> {
    // ...

        // If request has no Content-Type header, we set it if the content type has been set
        // implicitly on this request.
        if !request_spec.headers.contains_key(CONTENT_TYPE) {
            if let Some(s) = &request_spec.implicit_content_type {
                headers.push(format!("{}: {s}", CONTENT_TYPE));
            } else {
                // We remove default Content-Type headers added by curl because we want
                // to explicitly manage this header.
                // For instance, with --data option, curl will send a 'Content-type: application/x-www-form-urlencoded'
                // header.
                headers.push(format!("{}:", CONTENT_TYPE));
            }
        }

We set an implicit content type given the request headers. Now, with --header the request headers are not only from request_spec but also come from --header. If --headerdefine a CONTENT_TYPE header, we don't want to add implicit content type header. (same problem with if !request_spec.headers.contains_key(EXPECT) && options.aws_sigv4.is_none()etc...)

Trying to make less modification and and avoiding regression, what we could do is:

1. Change the signature of method set_headers

From:

fn set_headers(
        &mut self,
        request_spec: &RequestSpec,
        options: &ClientOptions,
    ) -> Result<(), HttpError>

to

fn set_headers(
        &mut self,
        headers_spec: &HeaderVec,
        options: &ClientOptions,
    ) -> Result<(), HttpError>

We previously take headers from a RequestSpec (the file), now we take headers from a HeaderVecthat will be the aggregation of file headers and --header options.

In the set_headers method, the only change we have to do is replace request_spec.headers to headers_spec

2. Make a method of the module header_helpers.rs with this signature

impl HeaderVec {
    fn with_raw_headers(&self, options_headers: &[&str]) -> HeaderVec {
        // ...
    }
    
}

This method takes a list of "raw" headers (maybe find a better name) i.e: vec!["foo:toto","bar:tutu"]. It returns a new HeaderVec. Initially I thought it should return a Result<HeaderVec, RunnerError> but I don't think it's necessarly. fn with_raw_headers can't fail. I was thinking on the case where the options headers are not well formatted (like --header "foo") but curl doesn't fail in this case, it filters this kind of value.

With this new builder method with_raw_headers, we can make unit tests in header_helpers.rs module. This is a "pure" method easy to unit test: given a HeaderVec and a list of headers in input, we want a HeadeVec as output.

3. Given this new method on HeaderVec, we can modify the following code in client.rs:

        self.set_body(request_spec_body)?;
        self.set_headers(request_spec, options)?;
        if let Some(aws_sigv4) = &options.aws_sigv4 {
            // ...
        }

To become:

        self.set_body(request_spec_body)?;
        let headers_spec = request_spec.headers.with_raw_headers(options.headers):
        self.set_headers(headers_spec, options)?;
        if let Some(aws_sigv4) = &options.aws_sigv4 {
            // ...
        }

And that should bee good!

The idea is really to be able to easily unit test the aggregation of headers from file and header from option.

curl_cmd.rs

The command line construction regarding headers should be done only here: let mut params = headers_params(request_spec);

Instead of this, we should call let mut params = headers_params(request_spec, options.headers); and try to reuse with_raw_headers method.

integration test

For the integation tests that are failing, you should look at hurl --help to see where is added -H, --header <HEADER.

I think we could split this in 2 PRs, it could be easier to review:

1. a first PR with the with_raw_headers method on header_helpers.rs along with unit tests (the method would be marker with `#[allow(dead_code)])
2. a second PR with the options stuff with --header and the use of with_raw_headers etc...

[theoforger]

@jcamiel Wow! Really appreciate the detailed response 🙏

One more question regarding the behavior. I understand that --header shouldn't override any user-defined header from the hurl file. But let's say we run with:

# test.hurl
GET http://localhost:8000/hello

$ hurl --header "User-Agent: some-text" test.hurl --verbose

Would we get:

...
> GET /hello HTTP/1.1
> Host: localhost:8000
> Accept: */*
> User-Agent: some-text #------------ Override (this is the behavior with `curl --header`)
...

or:

...
> GET /hello HTTP/1.1
> Host: localhost:8000
> Accept: */*
> User-Agent: hurl/6.0.0-SNAPSHOT
> User-Agent: some-text #----------------- Append
...

[jcamiel]

I think if we modify the code from

        if !request_spec.headers.contains_key(USER_AGENT) {
            let user_agent = match options.user_agent {
                Some(ref u) => u.clone(),
                None => format!("hurl/{}", clap::crate_version!()),
            };
            headers.push(format!("{}: {user_agent}", USER_AGENT));
        }

to

        if !headers_spec.contains_key(USER_AGENT) {
            let user_agent = match options.user_agent {
                Some(ref u) => u.clone(),
                None => format!("hurl/{}", clap::crate_version!()),
            };
            headers.push(format!("{}: {user_agent}", USER_AGENT));
        }

we will add a User-Agent only if there is no User-Agent header in --header nor in the file.

So we want:

$ hurl --header "User-Agent: some-text" test.hurl --verbose

...
> GET /hello HTTP/1.1
> Host: localhost:8000
> Accept: */*
> User-Agent: some-text #------------ Override (this is the behavior with `curl --header`)
...

[jcamiel]

/keepopen

[jcamiel]

Hi @theoforger just to warn you, we're doing a 6.0.0 release now; no rush to code/develop the PRs, we'll put this feature in the next release (should be 6.1.0)

[theoforger]

@jcamiel Thanks for letting me know! 🙏 I'll update you when I have something. Just been busy with school lately haha