Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #4808 - Review HttpClient Request header APIs. #4845

Merged

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented May 4, 2020

Introduced:

  • Request Request.headers(Consumer<HttpFields.Mutable>).
    This allows applications to modify the headers, and chain calls.
    It also delegates the precise semantic of put/add/remove/clear to HttpFields, so there is no API duplication.

  • HttpRequest.header(HttpField) to efficiently add fields while normalizing the request (only used in implementation).

  • HttpResponse.header(HttpField) to efficiently add fields while parsing the response (only used in implementation).
    This pairs with HttpResponse.trailer(HttpField).

  • HttpResponse.headers(Consumer<HttpFields.Mutable>) to modify the fields after they have been populated (only used in tests).

Removed:

  • Request.[set,add,put,remove], replaced by headers(Consumer<HttpFields.Mutable>).

Deprecated:

  • Request.header(String, String)
  • Request.header(HttpHeader, String)
    Both replaced by headers(Consumer<HttpFields.Mutable>) with clearer semantic for add/put/remove.

All the rest is code cleanup to remove the usage of the deprecated header() methods.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

Introduced:
* Request Request.headers(Consumer<HttpFields.Mutable>).
This allows applications to modify the headers, and chain calls.
It also delegates the precise semantic of put/add/remove/clear to HttpFields, so there is no API duplication.
* HttpRequest.header(HttpField) to efficiently add fields while normalizing the request (only used in implementation).

* HttpResponse.header(HttpField) to efficiently add fields while parsing the response (only used in implementation).
This pairs with HttpResponse.trailer(HttpField).
* HttpResponse.headers(Consumer<HttpFields.Mutable>) to modify the fields after they have been populated (only used in tests).

Removed:
* Request.[set,add,put,remove], replaced by headers(Consumer<HttpFields.Mutable>).

Deprecated:
* Request.header(String, String)
* Request.header(HttpHeader, String)
Both replaced by headers(Consumer<HttpFields.Mutable>) with clearer semantic for add/put/remove.

All the rest is code cleanup to remove the usage of the deprecated header() methods.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested review from gregw and lorban May 4, 2020 21:17
Fixed test failures.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the overall idea of concentrating all the HTTP field modification API into a single class, but it should be pushed to its full extent then: only have one single way to mutate HTTP fields: the lambda-taking mutating method headers().

This would have the extra bonus of making it cheap to extend the API with nice helper method (addIfAbsent, addIfNotNull...) that would make calling the lambda-taking mutating method very elegant to use.

@joakime joakime linked an issue May 6, 2020 that may be closed by this pull request
@joakime
Copy link
Contributor

joakime commented May 6, 2020

Is the new headers() lambda applied after the Request.Content based content-types are applied?
Or in fact after any "default" headers are added?

That would allow someone to remove/change a header that is added by other processes in the request before it's sent.

@joakime
Copy link
Contributor

joakime commented May 6, 2020

Will you update the client documentation in a future commit?

@sbordet
Copy link
Contributor Author

sbordet commented May 6, 2020

@joakime the lambda is invoke immediately.
What client documentation needs to be updated? The code snippets already are.

@sbordet sbordet requested a review from lorban May 8, 2020 15:20
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Updates after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet merged commit 0018c29 into jetty-10.0.x May 8, 2020
@sbordet sbordet deleted the jetty-10.0.x-4808-review_httpclient_header_api branch May 8, 2020 15:52
@joakime joakime linked an issue May 8, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review HttpFields API consistency Review HttpClient Request header APIs
3 participants