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

Add HttpRequestBuilder #3110

Merged
merged 16 commits into from Nov 26, 2020
Merged

Add HttpRequestBuilder #3110

merged 16 commits into from Nov 26, 2020

Conversation

tumile
Copy link
Contributor

@tumile tumile commented Oct 13, 2020

Motivation:

Modifications:

  • Add HttpRequest.builder() for fluent construction of HttpRequest
  • Add WebClient.prepare() for fluent construction and execution of HttpRequest

Result:

WebClient client = ...;

// Creates a POST HttpRequest to "/foo?q=bar" 
// with headers "cookie: name=value" and "authorization: value" and a JSON body.
// Using HttpRequest.builder():
client.execute(
    HttpRequest.builder()
               .post("/{resource}")
               .pathParam("resource", "foo")
               .queryParam("q", "bar")
               .cookie(Cookie.of("name", "value"))
               .header("authorization", "value")
               .content(MediaType.JSON, "{\"foo\":\"bar\"}"));

// Using WebClient.prepare():
client.prepare()
      .post("/{resource}")
      .pathParam("resource", "foo")
      .queryParam("q", "bar")
      .cookie(Cookie.of("name", "value"))
      .header("authorization", "value")
      .content(MediaType.JSON, "{\"foo\":\"bar\"}")
      .execute();

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #3110 (ba35648) into master (454a22c) will increase coverage by 0.14%.
The diff coverage is 78.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3110      +/-   ##
============================================
+ Coverage     73.90%   74.04%   +0.14%     
- Complexity    12305    12711     +406     
============================================
  Files          1064     1104      +40     
  Lines         46844    48208    +1364     
  Branches       5949     6125     +176     
============================================
+ Hits          34619    35697    +1078     
- Misses         9201     9378     +177     
- Partials       3024     3133     +109     
Impacted Files Coverage Δ Complexity Δ
...rp/armeria/client/WebClientRequestPreparation.java 27.58% <27.58%> (ø) 6.00 <6.00> (?)
...orp/armeria/common/AbstractHttpRequestBuilder.java 86.88% <86.88%> (ø) 47.00 <47.00> (?)
...om/linecorp/armeria/common/HttpRequestBuilder.java 96.29% <96.29%> (ø) 26.00 <26.00> (?)
...in/java/com/linecorp/armeria/client/WebClient.java 68.88% <100.00%> (+0.70%) 23.00 <1.00> (+1.00)
.../java/com/linecorp/armeria/common/HttpRequest.java 80.64% <100.00%> (+2.38%) 31.00 <1.00> (+2.00)
.../com/linecorp/armeria/server/grpc/GrpcService.java 28.57% <0.00%> (-71.43%) 2.00% <0.00%> (ø%)
...c/main/java/com/linecorp/armeria/server/Route.java 66.66% <0.00%> (-33.34%) 2.00% <0.00%> (ø%)
...linecorp/armeria/common/DefaultRequestHeaders.java 70.58% <0.00%> (-11.77%) 9.00% <0.00%> (-1.00%)
...a/com/linecorp/armeria/server/CompositeRouter.java 46.87% <0.00%> (-9.38%) 6.00% <0.00%> (-1.00%)
...ria/server/metric/PrometheusExpositionService.java 81.25% <0.00%> (-8.75%) 6.00% <0.00%> (+3.00%) ⬇️
... and 185 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 454a22c...2453262. Read the comment docs.

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thank you so much for volunteering on this issue. This is gonna be a great addition to Armeria API!

@tumile
Copy link
Contributor Author

tumile commented Oct 19, 2020

@ikhoon
Copy link
Contributor

ikhoon commented Oct 19, 2020

Somehow AppVeyor sometimes just hangs on a test and times out/doesn't run at all.
ci.appveyor.com/project/line/armeria/builds/35808547/job/82961887f2xb9vxw
ci.appveyor.com/project/line/armeria/builds/35809237/job/5myrrqwtokx8k9ag

Hmm, let me rerun it.

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Mostly minor ones. 👍

@minwoox minwoox added this to the 1.3.0 milestone Nov 3, 2020
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Excellent job, @tumile. This is a great usability improvement in Armeria's HTTP client. 🙇 Just a few nits

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Excellent work! I'm getting really curious about what your next improvement will be 💡

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @tumile 🙇‍♂️
Left a minor comment.

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Excellent work! 👍

@trustin trustin merged commit e8ef252 into line:master Nov 26, 2020
@trustin
Copy link
Member

trustin commented Nov 26, 2020

🎉🎉🎉

@tumile
Copy link
Contributor Author

tumile commented Nov 26, 2020

Thanks for reviewing 🙇 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants