Support `ResponseEntity` in annotated service instead of `HttpResult` #5586

moromin · 2024-04-09T03:38:35Z

Motivation:

We have HttpResult<T> and ResponseEntity<T>, both of them support automatic JSON deserialization.
However, we can't use HttpResult for clients because it is located in com.linecorp.armeria.server.annotation.

So, we should support ResponseEntity in that package so that users are not confused as to which one to use, and so that it can be used commonly between server and client.

Modifications:

Add logic to check if result type is ResponseEntity.
Create ResponseEntityUtil to build response headers as well as HttpResult.
Deprecate HttpResult.
Tests
- Add ResponseEntity cases for the cases that verify HttpResult behavior itself.
- Replace for the cases that use HttpResult to verify other behaviors.

Result:

Closes Support ResponseEntity in annotated services. #4910

github-actions · 2024-04-09T03:48:38Z

🔍 Build Scan® (commit: `b1a5c37`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/4r4bc42bx2wdy
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/opbkdxbzda3te
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/as62s5su7rosw
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/p5mnycgal7xms
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/pzd26hj4yyivu
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/d6qprjlmrivsc
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/i5i7mjfap5dfc
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/fxj3i2d6lgsne

codecov · 2024-04-09T05:57:38Z

Codecov Report

Attention: Patch coverage is 96.55172% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 74.13%. Comparing base (b8eb810) to head (4094889).
Report is 160 commits behind head on main.

❗ Current head 4094889 differs from pull request most recent head b1a5c37. Consider uploading reports for the commit b1a5c37 to get more accurate results

Files	Patch %	Lines
...annotation/CompositeResponseConverterFunction.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5586      +/-   ##
============================================
+ Coverage     73.95%   74.13%   +0.18%     
- Complexity    20115    21020     +905     
============================================
  Files          1730     1820      +90     
  Lines         74161    77443    +3282     
  Branches       9465     9901     +436     
============================================
+ Hits          54847    57415    +2568     
- Misses        14837    15378     +541     
- Partials       4477     4650     +173

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

trustin

Very nice work, @moromin! Left some minor comments 🙇

core/src/main/java/com/linecorp/armeria/common/ResponseEntityUtil.java

core/src/main/java/com/linecorp/armeria/server/annotation/ResponseConverterFunction.java

core/src/test/java/com/linecorp/armeria/common/ResponseEntityUtilTest.java

trustin · 2024-04-09T06:18:27Z

...a/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceResponseConverterTest.java

+                @Get("/expect-unauthorized")
+                public ResponseEntity<HttpResponse> expectUnauthorized() {
+                    // Will send '401 Unauthorized' because the content of ResponseEntity is HttpResponse.
+                    return ResponseEntity.of(
+                            ResponseHeaders.of(HttpStatus.OK),
+                            HttpResponse.of(HttpStatus.UNAUTHORIZED));
+                }


Should we raise an exception to avoid this confusing behavior? What do you think?

There is also warning, but I think the exception is more kind.
If we do, I think we could throw the exception here with the warning.

armeria/core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedService.java

Lines 210 to 213 in a13d582

if (HttpResponse.class.isAssignableFrom(clazz) ||

AggregatedHttpResponse.class.isAssignableFrom(clazz)) {

logger.warn("{} in the return type '{}' may take precedence over {}.",

clazz.getSimpleName(), returnType, HttpResult.class.getSimpleName());

Ahh, I see. We already log a warning message. Let's leave it as it is for now given that it may break an existing application. 🤔

trustin

One last comment 😄

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedService.java

trustin

👏👏👏

ikhoon · 2024-04-11T08:48:11Z

Regarding the test failure of build-self-hosted-unsafe-jdk-17-leak, the test was skipped but the test report had running logs for the test class.
https://ge.armeria.dev/s/3apwtzpy5yhzu/tests/overview?class=com.linecorp.armeria.client.kubernetes.ArmeriaHttpInterceptorTest&task-path=:kubernetes:shadedTest

Maybe Gradle's build result had not been properly cleared somehow.

ikhoon

Looks great! I hope to see you often even after Armeria Sprint. 😆

ikhoon · 2024-04-11T08:20:38Z

core/src/main/java/com/linecorp/armeria/internal/common/ResponseEntityUtil.java

+        final ResponseHeadersBuilder builder = result.headers().toBuilder();
+
+        if (builder.status().isContentAlwaysEmpty()) {
+            return builder.build();
+        }
+        if (builder.contentType() != null) {
+            return builder.build();
+        }


Micro optimization) Should we lazily create ResponseHeadersBuilder?

ResponseHeaders headers = result.headers(); if (builder.status().isContentAlwaysEmpty()) { return headers; } if (builder.contentType() != null) { return headers; } ....

jrhee17

Left one nit but looks good overall! 👍

jrhee17 · 2024-04-16T01:03:55Z

core/src/main/java/com/linecorp/armeria/internal/common/ResponseEntityUtil.java

+/**
+ * Utilities for {@link ResponseEntity}.
+ */
+public final class ResponseEntityUtil {


nit; if this class is moved to com.linecorp.armeria.internal.server.annotation, it can be package private

Suggested change

public final class ResponseEntityUtil {

final class ResponseEntityUtil {

jrhee17 · 2024-04-16T01:04:03Z

core/src/main/java/com/linecorp/armeria/internal/common/ResponseEntityUtil.java

+    /**
+     * Build {@link ResponseHeaders} from the given {@link ServiceRequestContext} and {@link ResponseEntity}.
+     */
+    public static ResponseHeaders buildResponseHeaders(ServiceRequestContext ctx, ResponseEntity<?> result) {


Suggested change

public static ResponseHeaders buildResponseHeaders(ServiceRequestContext ctx, ResponseEntity<?> result) {

static ResponseHeaders buildResponseHeaders(ServiceRequestContext ctx, ResponseEntity<?> result) {

jrhee17

👍 👍 👍

minwoox

Thanks, @moromin! 👍

@description

* upstream/main: Allow `{Service,Server}ErrorHandler.renderStatus()` to access `ServiceRequestContext` (line#5634) Bump com.gradle.enterprise from 3.16.2 to 3.17 (line#5559) Bump com.gradle.common-custom-user-data-gradle-plugin from 2.0 to 2.0.1 (line#5618) Add SchemeAndAuthority class which is in charge of authority normalization (line#5561) [Issue-5581] Change into transitive dependency(netty.transport.native.unix.common) (line#5623) Make @description annotation at super class/interface work (line#5562) Support `ResponseEntity` in annotated service instead of `HttpResult` (line#5586) Update public suffix list (line#5625) Wait until buffers are released in `ExceedingServiceMaxContentLengthTest.maxContentLength` (line#5600) Add the release note for 1.28.2 (line#5615) Fixed bug where a request timeout is scheduled after receiving the entire body (line#5614) Support custom json marshaller for unframed gRPC error (line#5555) Update public suffix list (line#5612)

…line#5586) Motivation: We have `HttpResult<T>` and `ResponseEntity<T>`, both of them support automatic JSON deserialization. However, we can't use `HttpResult` for clients because it is located in `com.linecorp.armeria.server.annotation`. So, we should support `ResponseEntity` in that package so that users are not confused as to which one to use, and so that it can be used commonly between server and client. Modifications: - Add logic to check if result type is `ResponseEntity`. - Create `ResponseEntityUtil` to build response headers as well as `HttpResult`. - Deprecate `HttpResult`. - Tests - Add `ResponseEntity` cases for the cases that verify `HttpResult` behavior itself. - Replace for the cases that use `HttpResult` to verify other behaviors. Result: - Closes line#4910

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin April 9, 2024 03:38

trustin added new feature deprecation labels Apr 9, 2024

trustin reviewed Apr 9, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedService.java Outdated Show resolved Hide resolved

trustin approved these changes Apr 9, 2024

View reviewed changes

ikhoon approved these changes Apr 11, 2024

View reviewed changes

moromin added 17 commits April 11, 2024 19:25

wip: check and convert ResponseEntity only in AnnotatedService

60f3065

feat: add factory method which is passed only content

525a617

docs: add ResponseEntity trailers description

0616a77

feat: add response entity util to build headers

199ca81

test: replace or add test cases for ResponseEntity

fe0de49

test: add test cases for other modules

0caade5

docs: deprecate HttpResult

bb69612

style: revert wildcard imports

5a4e47f

style: fix lint errors

7df576a

docs: add description to utility class

d3412db

test: add utility test

c5d76d5

docs: simplify the description of converter function

2713348

refactor: move util to internal.common package

ac9fd5f

test: build real context

c85f89b

style: fix lint errors

1a881f1

refactor: defer calling getSimpleName() until output warning

6932b62

docs: replace HttpResult with ResponseEntity in docs

2406abe

refactor: defer creating builder

cf0e9ca

moromin force-pushed the support_response_entity branch from 338849a to cf0e9ca Compare April 11, 2024 10:25

jrhee17 reviewed Apr 16, 2024

View reviewed changes

jrhee17 added this to the 1.29.0 milestone Apr 16, 2024

refactor: move ResponseEntityUtil to server.annotation package

b1a5c37

jrhee17 approved these changes Apr 17, 2024

View reviewed changes

minwoox approved these changes Apr 17, 2024

View reviewed changes

ikhoon merged commit a8dea21 into line:main Apr 22, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `ResponseEntity` in annotated service instead of `HttpResult` #5586

Support `ResponseEntity` in annotated service instead of `HttpResult` #5586

moromin commented Apr 9, 2024

github-actions bot commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading

trustin left a comment

trustin Apr 9, 2024

moromin Apr 9, 2024

trustin Apr 9, 2024

trustin left a comment

trustin left a comment

ikhoon commented Apr 11, 2024

ikhoon left a comment

ikhoon Apr 11, 2024

jrhee17 left a comment

jrhee17 Apr 16, 2024

jrhee17 Apr 16, 2024

jrhee17 left a comment

minwoox left a comment

	if (HttpResponse.class.isAssignableFrom(clazz) \|\|
	AggregatedHttpResponse.class.isAssignableFrom(clazz)) {
	logger.warn("{} in the return type '{}' may take precedence over {}.",
	clazz.getSimpleName(), returnType, HttpResult.class.getSimpleName());

	public final class ResponseEntityUtil {
	final class ResponseEntityUtil {

	public static ResponseHeaders buildResponseHeaders(ServiceRequestContext ctx, ResponseEntity<?> result) {
	static ResponseHeaders buildResponseHeaders(ServiceRequestContext ctx, ResponseEntity<?> result) {

Support ResponseEntity in annotated service instead of HttpResult #5586

Support ResponseEntity in annotated service instead of HttpResult #5586

Conversation

moromin commented Apr 9, 2024

github-actions bot commented Apr 9, 2024 • edited Loading

🔍 Build Scan® (commit: b1a5c37)

codecov bot commented Apr 9, 2024 • edited Loading

Codecov Report

trustin left a comment

Choose a reason for hiding this comment

trustin Apr 9, 2024

Choose a reason for hiding this comment

moromin Apr 9, 2024

Choose a reason for hiding this comment

trustin Apr 9, 2024

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon commented Apr 11, 2024

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon Apr 11, 2024

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Apr 16, 2024

Choose a reason for hiding this comment

jrhee17 Apr 16, 2024

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Support `ResponseEntity` in annotated service instead of `HttpResult` #5586

Support `ResponseEntity` in annotated service instead of `HttpResult` #5586

github-actions bot commented Apr 9, 2024 •

edited

Loading

🔍 Build Scan® (commit: `b1a5c37`)

codecov bot commented Apr 9, 2024 •

edited

Loading