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

[theoforger]

@jcamiel Hello! I was looking into this:

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

Since we are changing the signature, how do we want to handle implicit content type?

hurl/packages/hurl/src/http/client.rs

Lines 547 to 549 in 1025eae

	if !request_spec.headers.contains_key(CONTENT_TYPE) {
	if let Some(s) = &request_spec.implicit_content_type {
	list.append(&format!("{}: {s}", CONTENT_TYPE))?;

Here we only append the implicit content type if it exists, but if the method takes a HeaderVec instead, this field wouldn't be accessible.

We could simply add another parameter to this method, something like:

fn set_headers(
        &mut self,
        headers_spec: &HeaderVec,
        implicit_content_type: Option<String>,
        options: &ClientOptions,
    ) -> Result<(), HttpError> {
...

What do you think? Is there a better solution?

[jcamiel]

Yes, you're totally right, we have to pass the (potential) implicit content type as parameter now!

What you propose is totally fine, maybe the type of the parameter can be Option<&str> instead of Option<String> (the same way we accept &str instead of String when passing parameter as reference - we don't need to pass ownership of implicit content type to the set_headers method)

[theoforger]

@jcamiel I just submit another PR since we're going to split this one in half. I'll do the rest of the work here 😄

[theoforger]

@jcamiel I'm working on the second half. One slight issue:

To aggregate the headers, the new method takes a &[&str]:

hurl/packages/hurl/src/http/headers_helper.rs

Line 68 in 28d5717

pub fn aggregate_raw_headers(&self, raw_headers: &[&str]) -> HeaderVec {

However, in ClientOptions, the headers are defined as Vec<String>:

hurl/packages/hurl/src/cli/options/mod.rs

Line 64 in c0a5126

pub headers: Vec<String>,

We could map it to a Vec<&str>, but wouldn't it make more sense for the aggregation method to simply take a &[String] instead?

Appreciate the guidance 🙏

[jcamiel]

Hi,
Good question, I can't give you definitive answer on it, I just can give you what I feel is idiomatic in Rust (maybe wrongly !)

• a field in a struct of type Vec<String> (vs Vec<&str> or even &[&str])is simpler: we don't have to deal with lifetime, if this is less performant, it remains acceptable in our case. So ClienOptions is OK as it,
• as argument, I feel that &[&str] is more idiomatic than &[String]. To produce the latter you'll have to clone each string from the ClientOptions struct imo. In the unit test, we can work with array of &str (&["User-Agent: hurl/6.1.0", "Invalid-Header"]) which is super readable and convenient. (By the way, we could add another unit test that aggregate some headers with the same name but different values to check that we get all the headers)

It's just a feeling so don't take this as authoritative. We can go with the initial proposition. You could ask also this question on the Rust forum https://users.rust-lang.org I would be curious of the answer!

[theoforger]

To produce the latter you'll have to clone each string from the ClientOptions struct imo

If my understanding is correct, with a &[String] parameter, when we call the method like so:

request_spec.headers.aggregate_raw_headers(&options.headers);

&Vec<String> is automatically dereferenced to a slice (&[String]), so theoretically no clone should be created here.

Inside the aggregate_raw_headers method, Header::parse only borrows the string, so no clones of String are created here either.

Correct me if I'm wrong though. I'm still learning Rust 😆

[jcamiel]

Yes it's correct. Still, I still prefer to have &[&str] (once again still a feeling). Unless we have to make a too convoluted /complex code of course. In fact, I prefer to see a method with this signature:

fn method(param: &[&str]) vs fn method(param: &[String]) and adapt the calling code accordingly.

What could be tried, but not very found of, also is to make a generic method that can both accept &[&str] and &[String] with a trait AsRef<str>. I haven't the syntax in mind it should be something like:

fn method<T>(&self, headers: &[T]) where T: AsRef<str> (99% sure the syntax is not good...)

[theoforger]

I still prefer to have &[&str] (once again still a feeling)

No problem! Totally understandable 👍

fn method(param: &[&str]) vs fn method(param: &[String]) and adapt the calling code accordingly.

Just to clarify, are you suggesting that we make two separate methods with different parameter types?

[jcamiel]

No 😅 I mean: choose only one preferred method/ API signature and adapt the client call code to this API... (not sure if it's clearer)

[theoforger]

Ahh I see. In that case I'll simply go with &[&str]. Thanks!

[theoforger]

@jcamiel Hello! I made some more progress 😄. Let me know if anything needs to be changed. (Sorry I had to push it twice. Messed up the rebase earlier)

we could add another unit test that aggregate some headers with the same name but different values to check that we get all the headers

Btw, should I add it to this branch or make another PR for this?

[jcamiel]

Good job @theoforger I will look at it this week-end. For the units test, you can make another independent PR, as it's much more simple and with not a lot of code, I will merge it asap (the current PR needs a little more time to review 😅). Once again thanks for the PRs

[jcamiel]

Just some changes and we've almost done this one @theoforger . Could you please squash the new commits after the modification ? Thanks !

[jcamiel]

Well done @theoforger it's a good new feature! I will create an issue to add an integration test for it, fell free to work on it, if you want of course!

[jcamiel]

/accept

[hurl-bot]

🕗 /accept is running, please wait for completion.

[hurl-bot]

✅ Pull request merged with fast forward by jcamiel..

# List of commits merged from theoforger/hurl/2144-header-option branch into Orange-OpenSource/hurl/master branch:

• 3971e91 Add a new option --header

[jcamiel]

Issue for integration test => #3504

[theoforger]

@jcamiel Thanks for the approval! It's been such a pleasure working with you🙏

I might as well take the integration test issue 😆. And there's also the header option from inside the hurl file right? Which one should I work on first?

[jcamiel]

The integration test might be a good one so as --header is completely done. And after the header option in [Options] section. I think both will be easier than this one!