Add HttpResponseBuilder to fluently build `HttpResponse` by ta7uw · Pull Request #3941 · line/armeria

ta7uw · 2021-11-22T10:05:10Z

Motivation:

It would be convenient to add HttpResponseBuilder as like HttpRequestBuilder

Modifications:

Add HttpResponseBuilder to build HttpResponse
Add HttpResponse#builder to get a builder instance

Result:

Closes Provide a way to fluently build an HttpResponse #3398
Fluently build HttpResponse by using this builder as like HttpRequestBuilder

HttpResponse.builder()
            .ok()
            .content(content)
            .build();

ikhoon · 2021-11-23T05:50:03Z

+    /**
+     * Sets the content as UTF_8 for this response.
+     */
+    public HttpResponseBuilder content(String content) {
+        requireNonNull(content, "content");
+        return content(MediaType.PLAIN_TEXT_UTF_8, content);
+    }
+
+    /**
+     * Sets the content for this response.
+     */
+    public HttpResponseBuilder content(MediaType contentType, CharSequence content) {
+        requireNonNull(contentType, "contentType");
+        requireNonNull(content, "content");
+        return content(contentType, HttpData.of(contentType.charset(StandardCharsets.UTF_8), content));
+    }
+
+    /**
+     * Sets the content for this response.
+     */
+    public HttpResponseBuilder content(MediaType contentType, String content) {
+        requireNonNull(contentType, "contentType");
+        requireNonNull(content, "content");
+        return content(contentType, HttpData.of(contentType.charset(StandardCharsets.UTF_8), content));
+    }


We may extract the common part between AbstractHttpRequestBuilder and HttpResponseBuilder to AbstractHttpMessageBuilder.

Would be useful if we add contentJson(Object content)?

We may extract the common part between AbstractHttpRequestBuilder and HttpResponseBuilder to AbstractHttpMessageBuilder.

Should I address it in this PR?

Would be useful if we add contentJson(Object content)?

It seems to be useful.
I will add contentJson(Object content).

How about also adding the content method that takes Publisher as an argument?

public HttpResponseBuilder content(Publisher<? extends HttpData> content) {...}

Should I address it in this PR?

I hope so. 😀 Because duplicate code is caused by this new feature.

How about also adding the content method that takes Publisher as an argument?

It seems to be nice.

…ich is converted into JSON by ObjectMapper

…sponseBuilder to AbstractHttpMessageBuilder.

codecov · 2021-11-24T09:07:04Z

Codecov Report

Merging #3941 (e979549) into master (3ec1e4d) will increase coverage by 0.03%.
The diff coverage is 75.80%.

@@             Coverage Diff              @@
##             master    #3941      +/-   ##
============================================
+ Coverage     73.25%   73.28%   +0.03%     
- Complexity    15646    15691      +45     
============================================
  Files          1368     1370       +2     
  Lines         60025    60102      +77     
  Branches       7621     7625       +4     
============================================
+ Hits          43972    44047      +75     
+ Misses        12200    12196       -4     
- Partials       3853     3859       +6

Impacted Files	Coverage Δ
...rp/armeria/client/WebClientRequestPreparation.java	`65.21% <0.00%> (-2.97%)`	⬇️
...om/linecorp/armeria/common/HttpRequestBuilder.java	`87.09% <0.00%> (-9.34%)`	⬇️
...m/linecorp/armeria/common/HttpResponseBuilder.java	`76.74% <76.74%> (ø)`
...orp/armeria/common/AbstractHttpMessageBuilder.java	`78.72% <78.72%> (ø)`
...orp/armeria/common/AbstractHttpRequestBuilder.java	`91.47% <85.18%> (-0.36%)`	⬇️
...java/com/linecorp/armeria/common/HttpResponse.java	`91.45% <100.00%> (+0.04%)`	⬆️
.../linecorp/armeria/client/DefaultClientFactory.java	`72.91% <0.00%> (-2.09%)`	⬇️
.../com/linecorp/armeria/server/docs/ServiceInfo.java	`78.94% <0.00%> (-1.76%)`	⬇️
...rp/armeria/common/stream/DefaultStreamMessage.java	`86.55% <0.00%> (-1.08%)`	⬇️
...armeria/client/HttpClientPipelineConfigurator.java	`77.19% <0.00%> (-0.98%)`	⬇️
... and 12 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 3ec1e4d...e979549. Read the comment docs.

minwoox

Basically, looks good! 😄

ikhoon · 2021-11-29T06:34:25Z

+     */
+    public HttpResponseBuilder headers(Iterable<? extends Entry<? extends CharSequence, String>> headers) {
+        requireNonNull(headers, "headers");
+        responseHeadersBuilder.set(headers);


I think it is a bug to use set here. #3932 (comment)
Could you use add for header and headers instead.

minwoox

Thanks! I think we are almost there. 😄

minwoox · 2021-11-29T12:40:30Z

+@SuppressWarnings("checkstyle:OverloadMethodsDeclarationOrder")
+abstract class AbstractHttpMessageBuilder {
+
+    private final HttpHeadersBuilder httpHeaders = HttpHeaders.builder();


How about removing this and adding/setting directly to RequestHeadersBuilder and ResponseHeadersBuilder?

abstract class AbstractHttpMessageBuilder { abstract HttpHeadersBuilder headersBuilder(); AbstractHttpMessageBuilder header(CharSequence name, Object value) { headersBuilder().addObject(requireNonNull(name, "name"), requireNonNull(value, "value")); return this; } } public abstract class AbstractHttpRequestBuilder extends AbstractHttpMessageBuilder { private final RequestHeadersBuilder requestHeadersBuilder = RequestHeaders.builder(); @override HttpHeadersBuilder headersBuilder() { return requestHeadersBuilder; } } public final class HttpResponseBuilder extends AbstractHttpMessageBuilder { private final ResponseHeadersBuilder responseHeadersBuilder = ResponseHeaders.builder(); @override HttpHeadersBuilder headersBuilder() { return responseHeadersBuilder; } }

It seems good to me.

minwoox · 2021-11-29T12:44:50Z

+    @Nullable
+    private HttpHeadersBuilder httpTrailers;
+
+    protected final HttpHeadersBuilder httpHeaders() {


I think we can remove all protected and public modifiers in this class.

minwoox · 2021-11-29T12:50:44Z

+        if (httpTrailers == null) {
+            httpTrailers = HttpHeaders.builder();
+        }
+        httpTrailers.set(trailers);


Suggested change

httpTrailers.set(trailers);

httpTrailers.add(trailers);

Like we do in https://github.com/line/armeria/pull/3941/files#diff-d6ae049d812a9ab0b5ab421d60f14009c4418adff5ee0b597a1eac361c350f75R76?

minwoox · 2021-11-29T13:02:11Z

+                return HttpResponse.of(responseHeaders, content, trailers.build());
+            }
+        } else {
+            return HttpResponse.of(responseHeaders, publisher);


I think we should also contain trailers if it's not null as we do in the request builder.

…Builder

…ractHttpMessageBuilder

minwoox

I guess this is the final round. 😄

ikhoon · 2021-11-30T09:54:27Z

Added defect label because this PR also fixes #3932 (comment)

ikhoon

Almost there! Left only minor nits.

jrhee17

The changes look really nice 👍 LGTM apart from minor comments. Thank you! 🙇

jrhee17 · 2021-12-03T14:59:34Z

+    }
+
+    AbstractHttpMessageBuilder contentJson(Object content) {
+        requireNonNull(content, "content");


nit; should we also reject setting the content if publisher has been set already?
Otherwise, a user may set both contentJson and publisher, and it will be ambiguous which content will be applied.

checkState(publisher == null, "publisher has been set already");

or we can also modify to reuse content(MediaType contentType, HttpData content)

i.e.

AbstractHttpMessageBuilder contentJson(Object content) { requireNonNull(content, "content"); try { return content(MediaType.JSON, HttpData.wrap(JacksonUtil.writeValueAsBytes(content))); } catch (JsonProcessingException e) { throw new IllegalArgumentException("failed to serialize " + content, e); } }

jrhee17 · 2021-12-04T04:44:16Z

+     * Sets 200 OK to the status of this response.
+     */
+    public HttpResponseBuilder ok() {
+        status(HttpStatus.OK);


nit+optional; could just return as-is

Suggested change

status(HttpStatus.OK);

return status(HttpStatus.OK);

jrhee17 · 2021-12-04T06:02:09Z

+package com.linecorp.armeria.common;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;


Prefer to use import static org.assertj.core.api.Assertions.assertThat; so that we can take full advantage of the assertj API

Co-authored-by: jrhee17 <[email protected]>

ikhoon

Looking good. Left only minor nits.

ikhoon · 2021-12-06T09:36:02Z

+    }
+
+    /**
+     * Sets a header for this response. For example:


ikhoon · 2021-12-06T09:36:14Z

+    }
+
+    /**
+     * Sets multiple headers for this response. For example:


minwoox

Great job! Thanks, @ta7uw! 😄

ikhoon · 2021-12-08T05:54:26Z

Revert? A publisher will be sent with chunked transfer encoding.

Had a chat internally, my previous comment does not comply with Armeria's policy.
If users can predict the total size of a Publisher, we need to respect it.
Please revive if (publisher() == null) { condition.
Sorry for the back and forth. 😅

ikhoon · 2021-12-08T03:30:17Z

+    AbstractHttpMessageBuilder contentJson(Object content) {
+        requireNonNull(content, "content");
+        checkState(publisher == null, "publisher has been set already");
+        headersBuilder().contentType(MediaType.JSON);


Could be removed now?

Suggested change

headersBuilder().contentType(MediaType.JSON);

ikhoon · 2021-12-08T03:42:02Z

                                      StreamMessage.concat(publisher, StreamMessage.of(httpTrailers.build())));
            }
        }
+        final HttpData content = content();


Though, it is not related to this PR, could we delegate the following code to HttpRequest.of()?

HttpData content = firstNonNull(content, HttpData.empty()); HttpHeaders httpTrailers; if (httpTrailersBuiler != null) { httpTrailers = httpTrailersBuiler.build(); } else { httpTrailers = HttpHeaders.of(); } return HttpRequest.of(requestHeaders, content, httpTrailers);

ikhoon

Thanks a lot, @ta7uw! 🎉🚀
Left a minor comment. 🙇‍♂️

minwoox · 2021-12-13T13:55:54Z

Now I can fluently build my response. 🤣
Great work, @ta7uw! 🎉 🎉 🎉
And thanks, reviewers!

…s)` (#4727) **Related Issue** #3959 ### Motivation: ``` We have PrependingPublisher to publish ResponseHeaders and body in a StreamMessage. Similarly, we can add AppendingPublisher to publish body and HTTP trailers more efficiently. ``` - On #3941, we added `PrependingPublisher` to publish `ResponseHeaders` and body in a `StreamMessage` - So it's nice to add `SurroundingPublisher` to publish body and `HTTP trailers` too! ### Modifications: - Add `SurroundingPublisher` to append trailers - Add `HttpResponse.of(header, pub, trailers)` that using `SurroundingPublisher` ### Result: - You can now publish body and `HTTP trailers` easily by using `SurroundingPublisher` - Close #3959 issue --------- Co-authored-by: Ikhun Um <[email protected]> Co-authored-by: Ikhun Um <[email protected]>

Add HttpResponseBuilder

4052623

ta7uw requested review from ikhoon, jrhee17, minwoox and trustin as code owners November 22, 2021 10:05

minwoox added the new feature label Nov 23, 2021

ikhoon reviewed Nov 23, 2021

View reviewed changes

ta7uw added 5 commits November 24, 2021 11:49

Add method to set content using Publisher to HttpResponseBuilder

9d7d097

Add HttpResponseBuilder#contentJson to set content using an object wh…

cbc51b4

…ich is converted into JSON by ObjectMapper

Extract the common part between AbstractHttpRequestBuilder and HttpRe…

1e9298b

…sponseBuilder to AbstractHttpMessageBuilder.

Polish 1e9298b

1e09229

Fix style for checkstyle task

0d9d801

ikhoon reviewed Nov 25, 2021

View reviewed changes

ta7uw added 3 commits November 26, 2021 14:35

Remove get prefix from getters

c269579

Polish AbstractHttpMessageBuilder

105ebfa

Add SuppressWarnings to AbstractHttpMessageBuilder

5f523a7

minwoox reviewed Nov 29, 2021

View reviewed changes

ikhoon reviewed Nov 29, 2021

View reviewed changes

ta7uw added 4 commits November 29, 2021 18:06

Fix reviewed points

98adc2a

Use add instead of set

cd468b0

Move trailers to AbstractHttpMessageBuilder

0fd3c0b

Move headers to AbstractHttpMessageBuilder

6dfac2a

minwoox reviewed Nov 29, 2021

View reviewed changes

ta7uw added 3 commits November 30, 2021 11:16

Remove protected and public access modifiers from AbstractHttpMessage…

1b554c9

…Builder

Fix reviewed points

3229a1a

Define abstract method to get request/response header builder in Abst…

4c91f2e

…ractHttpMessageBuilder

minwoox reviewed Nov 30, 2021

View reviewed changes

Comment thread core/src/main/java/com/linecorp/armeria/common/AbstractHttpMessageBuilder.java Outdated

Comment thread core/src/main/java/com/linecorp/armeria/common/HttpResponseBuilder.java Outdated

Comment thread core/src/test/java/com/linecorp/armeria/common/HttpResponseBuilderTest.java Outdated

ta7uw added 2 commits November 30, 2021 15:46

Add the detail message for failure of serialization

198907a

Remove unnecessary method call

9fe46fd

Remove redundant assertions

3314260

ikhoon added the defect label Nov 30, 2021

ikhoon reviewed Nov 30, 2021

View reviewed changes

Fix reviewed points

4035bf5

ikhoon added this to the 1.14.0 milestone Dec 2, 2021

jrhee17 approved these changes Dec 4, 2021

View reviewed changes

ta7uw and others added 5 commits December 6, 2021 13:16

Update Javadoc

c091fb4

Co-authored-by: jrhee17 <[email protected]>

Rename parameter

e2949d0

Co-authored-by: jrhee17 <[email protected]>

Use containsExactly

18044e6

Co-authored-by: jrhee17 <[email protected]>

Update Javadoc

dc9fadb

Co-authored-by: jrhee17 <[email protected]>

Fix reviewed points

328e534

ikhoon reviewed Dec 6, 2021

View reviewed changes

ta7uw added 2 commits December 8, 2021 11:39

Update Javadoc

012b579

Content-Length header is removed

743c23d

minwoox approved these changes Dec 8, 2021

View reviewed changes

Comment thread core/src/main/java/com/linecorp/armeria/common/AbstractHttpMessageBuilder.java Outdated

ikhoon reviewed Dec 8, 2021

View reviewed changes

Fix reviewed points

d946096

ta7uw requested a review from ikhoon December 10, 2021 02:10

ikhoon approved these changes Dec 10, 2021

View reviewed changes

Comment thread core/src/main/java/com/linecorp/armeria/common/HttpResponseBuilder.java

Comment thread core/src/main/java/com/linecorp/armeria/common/HttpResponse.java

ta7uw and others added 2 commits December 10, 2021 12:02

Add @UnstableApi

0b72e42

Merge branch 'master' into issue-3398

e979549

minwoox merged commit 5c521cc into line:master Dec 13, 2021

ikhoon mentioned this pull request Jan 24, 2022

HttpRequestBuilder.add(name, value) behaves differently from Request.header(name, value) #3932

Closed

ta7uw deleted the issue-3398 branch May 27, 2022 15:05

injae-kim mentioned this pull request Mar 6, 2023

Add SurroundingPublisher and HttpResponse.of(headers, pub, trailers) #4727

Merged

Conversation

ta7uw commented Nov 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ta7uw Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minwoox Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minwoox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ikhoon commented Nov 30, 2021

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrhee17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ta7uw Nov 23, 2021 •

edited

Loading

codecov Bot commented Nov 24, 2021 •

edited

Loading

minwoox Nov 29, 2021 •

edited

Loading