Support RxJava-wrapped `HttpResult` response in annotated services #5386

KarboniteKream · 2024-01-17T01:25:47Z

Motivation:

When returning Single<HttpResult<T>> from an annotated service, the response is always serialized as {}, since we currently just try to serialize HttpResult with Jackson. Instead, we should convert it to HttpResponse and only do serialization on HttpResult.content().

Modifications:

Create HttpResultUtil with the logic needed to build HttpResponse headers from HttpResult
Apply the same HttpResult conversion logic from AnnotatedService to CompositeResponseConverterFunction

Result:

Closes Annotated services do not work with Single<HttpResult<T>> #5380
Annotated services support HttpResult wrapped in RxJava objects

KarboniteKream · 2024-01-17T01:26:44Z

...java/com/linecorp/armeria/internal/server/annotation/CompositeResponseConverterFunction.java

@@ -62,6 +63,12 @@ public HttpResponse convertResponse(ServiceRequestContext ctx,
        if (result instanceof HttpResponse) {
            return (HttpResponse) result;
        }
+        if (result instanceof HttpResult) {


I'm not very familiar with the converter setup, so I'm not 100% sure if this is the only guaranteed entrypoint to the converter chain where we might receive an HttpResult.

I think this is the place to put logic because we don't support the conversion recursively.

KarboniteKream · 2024-01-17T01:27:42Z

...s/annotated-http-service/src/main/java/example/armeria/server/annotated/ReactiveService.java

+import io.reactivex.rxjava3.core.Single;
+
+@ProducesJson
+public class ReactiveService {


Temporary service for testing purposes. I still need to test Mono, but I want to figure out how to apply the converters.

Let's just leave it as TODO.
I thought we already had something like ReactorResponseConverterFuction but we don't have.
We can implement it later when we need it.

I see! I've just tested an API that returns Mono and it indeed seems like that's not implemented yet.

github-actions · 2024-01-17T01:32:17Z

🔍 Build Scan® (commit: `ea25d7c`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/xtzklh3fiz7wg
build-self-hosted-jdk-8	✅	https://ge.armeria.dev/s/rf3e2mff2wi6m
build-self-hosted-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/kus6zz2byxqwy
build-self-hosted-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/2awvg3lntp5rm
build-self-hosted-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/7t44imgu4widg
build-self-hosted-jdk-17-leak	✅	https://ge.armeria.dev/s/rtrm2sdrw6bme
build-self-hosted-jdk-11	✅	https://ge.armeria.dev/s/z4ok62x5lqnhq
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/wb4zgg4j6orpa

minwoox

Apologize for the late review. The changes look good to me. 👍

minwoox · 2024-01-22T01:24:57Z

...java/com/linecorp/armeria/internal/server/annotation/CompositeResponseConverterFunction.java

@@ -62,6 +63,12 @@ public HttpResponse convertResponse(ServiceRequestContext ctx,
        if (result instanceof HttpResponse) {
            return (HttpResponse) result;
        }
+        if (result instanceof HttpResult) {


I think this is the place to put logic because we don't support the conversion recursively.

minwoox · 2024-01-22T01:25:54Z

...s/annotated-http-service/src/main/java/example/armeria/server/annotated/ReactiveService.java

+import io.reactivex.rxjava3.core.Single;
+
+@ProducesJson
+public class ReactiveService {


Let's just leave it as TODO.
I thought we already had something like ReactorResponseConverterFuction but we don't have.
We can implement it later when we need it.

KarboniteKream · 2024-01-22T03:08:26Z

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

+                final AnnotatedService service = ctx.config().service().as(AnnotatedService.class);
+                if (service != null) {
+                    builder.status(service.defaultStatus());
+                } else {
+                    builder.status(HttpStatus.OK);
+                }


The only thing that changed from the original implementation in AnnotatedService

KarboniteKream · 2024-01-22T05:35:35Z

Seems like adding mockito-inline breaks KeepAliveHandlerTest 🤔 Let me find a different approach.

minwoox

Could you also add tests that check if the following types are converted correctly?
CompletableFuture<HttpResult> and Single<HttpResult>

minwoox

Great!!! Thanks a lot, @KarboniteKream! 😄

jrhee17

Nice! Thanks for reporting/fixing this issue @KarboniteKream 🙇 👍 🙇

jrhee17 · 2024-01-23T09:09:47Z

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

@@ -398,7 +402,7 @@ private HttpResponse convertResponse(ServiceRequestContext ctx, @Nullable Object

        if (result instanceof HttpResult) {


note; Perhaps in the future we may just remove this block since CompositeResponseConverterFunction has this logic already.

I understand this can't be removed this iteration due to breaking changes though

I've tried that but it fails depends on the implementation:

armeria/kotlin/src/test/kotlin/com/linecorp/armeria/server/kotlin/SuspendingAnnotatedServiceTest.kt

Lines 374 to 377 in d34cee7

when (result) {

is Bar -> responseConverter.convertResponse(ctx, headers, "hello, bar!", trailers)

else -> ResponseConverterFunction.fallthrough()

}

Thanks, I also checked that the content unwrapping logic is the reason for the failure

result = httpResult.content();

ikhoon

Clean fix! ❤️💯

KarboniteKream commented Jan 17, 2024

View reviewed changes

KarboniteKream force-pushed the fix/http-result-rxjava branch from b4d8cfb to 449361d Compare January 17, 2024 01:28

minwoox reviewed Jan 22, 2024

View reviewed changes

KarboniteKream changed the title ~~Support wrapped HttpResult response in annotated services~~ Support RxJava-wrapped HttpResult response in annotated services Jan 22, 2024

KarboniteKream commented Jan 22, 2024

View reviewed changes

KarboniteKream added 2 commits January 22, 2024 12:10

Support wrapped HttpResult response in annotated services

3a734fc

Remove reproducible

2d0547b

KarboniteKream force-pushed the fix/http-result-rxjava branch from d5a44e9 to 3d790ea Compare January 22, 2024 03:10

KarboniteKream marked this pull request as ready for review January 22, 2024 03:10

KarboniteKream requested review from ikhoon, jrhee17 and trustin as code owners January 22, 2024 03:10

Add tests

a5dc644

KarboniteKream force-pushed the fix/http-result-rxjava branch from 3d790ea to a5dc644 Compare January 22, 2024 05:11

Remove mockito-inline

ea25d7c

KarboniteKream force-pushed the fix/http-result-rxjava branch from 04e49d6 to ea25d7c Compare January 22, 2024 06:17

minwoox reviewed Jan 22, 2024

View reviewed changes

Add missing tests

4693039

minwoox added this to the 1.27.0 milestone Jan 22, 2024

minwoox added the improvement label Jan 22, 2024

minwoox approved these changes Jan 22, 2024

View reviewed changes

jrhee17 approved these changes Jan 23, 2024

View reviewed changes

ikhoon approved these changes Jan 24, 2024

View reviewed changes

ikhoon merged commit c20e058 into line:main Jan 24, 2024
13 checks passed

KarboniteKream deleted the fix/http-result-rxjava branch January 24, 2024 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support RxJava-wrapped `HttpResult` response in annotated services #5386

Support RxJava-wrapped `HttpResult` response in annotated services #5386

KarboniteKream commented Jan 17, 2024 •

edited

Loading

KarboniteKream Jan 17, 2024

minwoox Jan 22, 2024

KarboniteKream Jan 17, 2024

minwoox Jan 22, 2024

KarboniteKream Jan 22, 2024

github-actions bot commented Jan 17, 2024 •

edited

Loading

minwoox left a comment

minwoox Jan 22, 2024

minwoox Jan 22, 2024

KarboniteKream Jan 22, 2024

KarboniteKream commented Jan 22, 2024

minwoox left a comment

minwoox left a comment

jrhee17 left a comment

jrhee17 Jan 23, 2024

minwoox Jan 23, 2024 •

edited

Loading

jrhee17 Jan 24, 2024

ikhoon left a comment

		@@ -398,7 +402,7 @@ private HttpResponse convertResponse(ServiceRequestContext ctx, @Nullable Object

		if (result instanceof HttpResult) {

	when (result) {
	is Bar -> responseConverter.convertResponse(ctx, headers, "hello, bar!", trailers)
	else -> ResponseConverterFunction.fallthrough()
	}

Support RxJava-wrapped HttpResult response in annotated services #5386

Support RxJava-wrapped HttpResult response in annotated services #5386

Conversation

KarboniteKream commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2024 • edited Loading

🔍 Build Scan® (commit: ea25d7c)

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarboniteKream commented Jan 22, 2024

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Support RxJava-wrapped `HttpResult` response in annotated services #5386

Support RxJava-wrapped `HttpResult` response in annotated services #5386

KarboniteKream commented Jan 17, 2024 •

edited

Loading

github-actions bot commented Jan 17, 2024 •

edited

Loading

🔍 Build Scan® (commit: `ea25d7c`)

minwoox Jan 23, 2024 •

edited

Loading