Review HttpClient Request header APIs

[sbordet]

Jetty version
10.0.x

Description
In light of #4773 and #4777, the client Request.header() API should be reviewed.

Currently, there is only one API: header(String, String).
This has the problem that there is no semantic for add/put (i.e. add the header, or replace the header), there is no semantic for remove (you have to pass a null value), and it's not optimal if you already have the pre-computed HttpField.

So perhaps the API should just expose a HttpFields.Mutable to let application have all the needed semantic (without repeating them in Request) - however this has the problem that it will break the API fluency (since a Request.headers() method would return HttpFields.Mutable, not Request so fluent chaining won't be possible).

@gregw thoughts?

[sbordet]

See also #577.

[joakime]

I was initially a bit nervous at the removal of some APIs, like Request.header(String, String) as it's one of the most commonly used API methods.

But the replacement just feels and looks better.

Old example

Request request = client.newRequest(uri)
    .path("/")
    .method(method);
if (clientClose)
    request.header(HttpHeader.CONNECTION, "close");
else if (serverClose)
    request.header("X-Close", "true");

New way

Request request = client.newRequest(uri)
    .path("/")
    .method(method)
    .headers((fields) -> {
        if (clientClose)
            fields.put(HttpHeader.CONNECTION, "close");
        else if (serverClose)
            fields.put("X-Close", "true");
    });

[chernser]

New API is counterintuitive and doesn't fit overall style. Then why query parameters still param(name, value) ?

[sbordet]

Why it is counterintuitive? It gives you full access to the HttpFields API without duplicating all the semantic.

Then why query parameters still param(name, value) ?

Because the values require special encoding that 99.9% of the developers forget, so Jetty does it for them.

[chernser]

Why it is counterintuitive? It gives you full access to the HttpFields API without duplicating all the semantic.
I agree that previous API was may confusing, but part of it was quite useful.

It is counterintuitive because:

• it doesn't follow the same pattern with other setters.
• Consumer<> is usually used when consumer should process immutable object, Supplier<> is the closer alternative here.
• Signature Consumer<Mutable> is not obvious - what is Mutable?

I'm would like has simple header(String name, String value) additionally to that so code would be more a chain like. Here is example:

 client.newRequest("http://localhost:8080")
  .method("GET") 
  .param("search", textSearch) 
  .header("X-app-log-metrics", "true")
  .header("X-app-client-id", clientId)
  .send()

vs

 client.newRequest("http://localhost:8080")
  .method("GET") 
  .param("search", textSearch) 
  .headers( (headers) -> {
       headers.put("X-app-log-metrics", "true")
       headers.put("X-app-client-id", clientId)
  })
  .send()

I think it is longer and more text to read. It "breaks" chain of common methods.

Don't get me wrong - I just would like an old way to be here too.

Because the values require special encoding that 99.9% of the developers forget, so Jetty does it for them.
Nothing stops to use Consumer<Params> and make params do the encoding. No-one I think would expect that client send it in as is.
Btw, if parameters have similar method than users may just pass method reference to, for example, add common parameters and headers. Please consider next example:

 
     client.newRequest("http://localhost:8080")
  .method("GET") 
  .param("search", textSearch) 
  .headers(CommonHeaders::add)
  .params(context::qparams)
  .send()

[sbordet]

I'm would like has simple header(String name, String value) additionally to that so code would be more a chain like.

Read the first comment. header(String, String) is misleading.

We cannot support only super-simple examples, we have to support all use cases.

I don't understand your last example -- things seems more complicated and less clear just to be "chain like".

The goal is to provide clear and precise functionalities for all use cases, with little code duplication.

In your last example, it is not clear what happens to param("search", textSearch) if params(context::qparams) also provides a "search" parameter.
Is it replaced? Added? If added, what is the order? Throws an exception due to duplicate?

You may have a point about param(String, String) (same problems of header(String, String)) and to use Fields instead.
If you want to pursue that, please open a new issue.