Add delegating response converter function provider #4336

resquivel-squareup · 2022-07-06T04:44:50Z

Potentially resolves issue 4301

Motivation:

Fix bug with Protobuf response converter. The fix now allows us to use a custom converter, while still preserving desired behaviour in other areas.

Please see the discussion issue 4301 for more information.

Modifications:

Take functionality from AnnotatedService into its own ResponseConverterFunctionSelector class. This isolates the behaviour of selecting response converters and lets us test its functionality in isolation.
Tests regarding desired order of converter usage
Test to ensure the bug with the Protobuf response converter is fixed
Split existing ResponseConverterFunctionProvider functionality into two separate interfaces --- ResponseConverterFunctionProvider and DelegatingResponseConverterFunctionProvider. These interfaces have different expectations in terms of parameters and usage of passed-in parameters
Several later commits involve resolving merge conflicts which were introduced because there were changes in the master branch to the code which we had pulled out into ResponseConverterFunctionSelector in this branch

Result:

Closes Unable to use custom JsonFormat.Printer for ProtobufResponseConverterFunction #4301.
You will now be able to use a custom JsonFormat.Printer for ProtobufResponseConverterFunction
You will have to update any custom SPI implementations to conform to either ResponseConverterFunctionProvider or DelegatingResponseConverterFunctionProvider

…akes it easier to test its behaviour

…ConverterFunctionProvider

…our from DelegatingResponseConverterFunctionProvider

CLAassistant · 2022-07-06T04:44:55Z

All committers have signed the CLA.

…Function for clarity

…-converter-function-provider * origin/master: Update the project version to 1.17.1-SNAPSHOT Add the release note for 1.17.0 (line#4330) Send 400 Bad Request when HTTP2 cleartext upgrade contains invalid headers (line#4224) Fix the test failure of `GrpcServiceLogNameTest.logName()` (line#4334) Support `ExchangeType` for the response of annotated service (line#4177) Recover HttpResponse according to a specific exception class (line#4283)

…thod argument

…unrelated tests to fail

codecov · 2022-07-08T05:03:14Z

Codecov Report

Merging #4336 (f4161d6) into master (3e85b61) will increase coverage by 0.01%.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##             master    #4336      +/-   ##
============================================
+ Coverage     73.81%   73.82%   +0.01%     
- Complexity    17740    17752      +12     
============================================
  Files          1510     1512       +2     
  Lines         66244    66252       +8     
  Branches       8322     8320       -2     
============================================
+ Hits          48896    48911      +15     
- Misses        13336    13342       +6     
+ Partials       4012     3999      -13

Impacted Files	Coverage Δ
...nnotation/AggregatedResponseConverterFunction.java	`70.00% <70.00%> (ø)`
...a/internal/server/annotation/AnnotatedService.java	`88.00% <100.00%> (+0.91%)`	⬆️
...rver/annotation/ResponseConverterFunctionUtil.java	`100.00% <100.00%> (ø)`
...on/kotlin/FlowResponseConverterFunctionProvider.kt	`72.72% <100.00%> (ø)`
...buf/ProtobufResponseConverterFunctionProvider.java	`83.33% <100.00%> (-0.88%)`	⬇️
...2/ObservableResponseConverterFunctionProvider.java	`90.00% <100.00%> (ø)`
...3/ObservableResponseConverterFunctionProvider.java	`84.00% <100.00%> (ø)`
...m/linecorp/armeria/client/DecodedHttpResponse.java	`85.00% <0.00%> (-5.00%)`	⬇️
...rnal/common/GracefulConnectionShutdownHandler.java	`70.21% <0.00%> (-4.26%)`	⬇️
...ecorp/armeria/server/grpc/StreamingServerCall.java	`82.02% <0.00%> (-2.25%)`	⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

minwoox

Thanks! We are going in the right direction!
Just left a few minor comments. 😉

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

minwoox · 2022-07-14T02:35:00Z

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

+                                new CompositeResponseConverterFunction(backingConverters)))
+                        .build());
+
+        for (final DelegatingResponseConverterFunctionProvider provider : delegatingResponseConverterFunctionProviders) {


Could you fix the lint?

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

...java/com/linecorp/armeria/server/annotation/DelegatingResponseConverterFunctionProvider.java

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

...ain/java/com/linecorp/armeria/server/protobuf/ProtobufResponseConverterFunctionProvider.java

minwoox · 2022-07-14T02:47:17Z

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

+                );
+
+        final HttpResponse response = converterFunction.convertResponse(ctx, null,
+                                                                        new TestClassWithDelegatingResponseConverterProvider(),


Could you check the lint of test files? 😄

Done 👍 Let me know if there are still issues, checkStyle seems to have passed

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

jrhee17

I wasn't able to review the test files yet, but the changes look good to me 👍
Left some minor comments 🙏

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

...java/com/linecorp/armeria/server/annotation/DelegatingResponseConverterFunctionProvider.java

…atic block

…unctionUtil

…OSS linter

minwoox

Looks great. 😄
Left only minor comments.

...main/java/com/linecorp/armeria/internal/server/annotation/ResponseConverterFunctionUtil.java

minwoox · 2022-07-29T07:02:52Z

.../src/test/java/com/linecorp/armeria/internal/server/annotation/TestSpiConverterProvider.java

+import com.linecorp.armeria.server.annotation.ResponseConverterFunctionProvider;
+
+/**
+ * For use with ResponseConverterFunctionSelectorTest.


minwoox · 2022-07-29T07:03:51Z

.../src/test/java/com/linecorp/armeria/internal/server/annotation/TestSpiConverterProvider.java

+        }
+    }
+
+    private Class<?> toClass(Type type) {


ditto. Let's reduce the cognitive load.

Otherwise, we can just replace this with ClassUtil.typeToClass().

minwoox · 2022-07-29T07:08:09Z

...java/com/linecorp/armeria/server/protobuf/ProtobufResponseAnnotatedServiceJsonArrayTest.java

+    @Test
+    void returnEmptyJsonArrayGivenCustomJsonPrinter() {
+        final AggregatedHttpResponse response = client.get("/json").aggregate().join();
+        assertThat(Objects.requireNonNull(Objects.requireNonNull(response.headers().contentType()))


Objects.requireNonNull is used twice for the same value.
Let's use AssertJ API.

final MediaType mediaType = response.headers().contentType(); assertThat(mediaType).isNotNull(); assertThat(mediaType).isSameAs(MediaType.JSON_UTF_8);

We can use MediaType.isJson() which is a shortcut for that.

minwoox · 2022-07-29T07:29:10Z

...src/test/java/com/linecorp/armeria/server/protobuf/ProtobufResponseAnnotatedServiceTest.java

@@ -218,8 +218,8 @@ void protobufJsonResponse(String contentType) throws InvalidProtocolBufferExcept
    void protobufStreamResponse(String path) {
        final AggregatedHttpResponse response = client.get(path).aggregate().join();
        assertThat(response.status()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
-        assertThat(cause).isInstanceOf(IllegalArgumentException.class)
-                         .hasMessageContaining("Cannot convert a");
+        assertThat(cause).isInstanceOf(IllegalStateException.class)


Note for reviewers.
The exception is used to be raised by ProtobufResponseConverterFunction. https://github.com/line/armeria/blob/master/protobuf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufResponseConverterFunction.java#L243-L244`
The exception is now wrapped by the CompositeResponseConverterFunction.
https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/internal/server/annotation/CompositeResponseConverterFunction.java#L71-L75

minwoox · 2022-07-29T07:31:12Z

...ain/scala/com/linecorp/armeria/server/scalapb/ScalaPbResponseConverterFunctionProvider.scala

@@ -17,15 +17,15 @@
 package com.linecorp.armeria.server.scalapb

 import com.linecorp.armeria.common.annotation.{Nullable, UnstableApi}
-import com.linecorp.armeria.server.annotation.{ResponseConverterFunction, ResponseConverterFunctionProvider}
+import com.linecorp.armeria.server.annotation.{ResponseConverterFunction, DelegatingResponseConverterFunctionProvider}


We could probably do
import com.linecorp.armeria.server.annotation._ to fix the lint failure. 😉

Co-authored-by: minux <songmw725@gmail.com>

minwoox

Thanks! I added more minor comments. 😄

...main/java/com/linecorp/armeria/internal/server/annotation/ResponseConverterFunctionUtil.java

...java/com/linecorp/armeria/server/annotation/DelegatingResponseConverterFunctionProvider.java

.../src/main/java/com/linecorp/armeria/server/annotation/ResponseConverterFunctionProvider.java

…tion/ResponseConverterFunctionUtil.java Co-authored-by: minux <songmw725@gmail.com>

…gatingResponseConverterFunctionProvider.java Co-authored-by: minux <songmw725@gmail.com>

ikhoon · 2022-08-16T08:52:16Z

@resquivel-squareup I fixed the code commented by @minwoox and slightly cleaned it up.

ikhoon

Thanks a lot, @resquivel-squareup 🙇‍♂️💯

minwoox · 2022-08-24T02:24:21Z

@resquivel-squareup
Would you mind signing the CLA? 😄
#4336 (comment)

resquivel-squareup · 2022-09-02T08:32:12Z

Thanks a lot, @resquivel-squareup 🙇‍♂️💯

Thanks for the added fixes, too! I've been busy recently and so haven't been able to add the fixes 😅

I've signed the CLA @minwoox. Apologies for the delay!

minwoox · 2022-09-02T08:44:49Z

Please, don't worry about it. Thanks for signing the CLA. 😄

minwoox

Awesome. Thanks! 😄

...main/java/com/linecorp/armeria/internal/server/annotation/ResponseConverterFunctionUtil.java

jrhee17

Looks great! Thanks @resquivel-squareup 🙇 👍 🙇

minwoox · 2022-09-05T13:39:33Z

Congratulations @resquivel-squareup! 🎉 🎉 🎉
Hope to see your next PR again soon. 🤣

Potentially resolves issue [4301](line#4301 (comment)) Motivation: Fix bug with Protobuf response converter. The fix now allows us to use a custom converter, while still preserving desired behaviour in other areas. Please see the discussion issue 4301 for more information. Modifications: - Take functionality from `AnnotatedService` into its own `ResponseConverterFunctionSelector` class. This isolates the behaviour of selecting response converters and lets us test its functionality in isolation. - Tests regarding desired order of converter usage - Test to ensure the bug with the Protobuf response converter is fixed - Split existing `ResponseConverterFunctionProvider` functionality into two separate interfaces --- `ResponseConverterFunctionProvider` and `DelegatingResponseConverterFunctionProvider`. These interfaces have different expectations in terms of parameters and usage of passed-in parameters - Several later commits involve resolving merge conflicts which were introduced because there were changes in the master branch to the code which we had pulled out into `ResponseConverterFunctionSelector` in this branch Result: - Closes line#4301. - You will now be able to use a custom JsonFormat.Printer for ProtobufResponseConverterFunction - You will have to update any custom SPI implementations to conform to either `ResponseConverterFunctionProvider` or `DelegatingResponseConverterFunctionProvider`

resquivel-squareup added 4 commits July 5, 2022 17:04

Move response converter function selection into its own class; This m…

8cef36c

…akes it easier to test its behaviour

Test to ensure converters passed in are prioritised over SPI converters

33893a0

[4301] Rename ResponseConverterFunctionProvider to DelegatingResponse…

f4d63bd

…ConverterFunctionProvider

[4301] Create ResponseConverterFunctionProvider with different behavi…

aa97c98

…our from DelegatingResponseConverterFunctionProvider

resquivel-squareup changed the title ~~Delegating response converter function provider~~ Work-in-progress - Delegating response converter function provider Jul 6, 2022

[4301] Protobuf json array test

7b5d5d3

resquivel-squareup mentioned this pull request Jul 6, 2022

Unable to use custom JsonFormat.Printer for ProtobufResponseConverterFunction #4301

Closed

resquivel-squareup added 6 commits July 7, 2022 15:35

[4301] Rename createResponseConverterFunction to newResponseConverter…

1c3617b

…Function for clarity

[4301] Fix up issues introduced by merge conflicts

1ae94ae

[4301] Remove now-unused methods from ResponseConverterFunctionSelector

420ecb5

[4301] Update/fix ResponseConverterFunctionSelectorTest to use new me…

17f9653

…thod argument

[4301] Fix issue with test provider implementation which was causing …

8a9a4d2

…unrelated tests to fail

resquivel-squareup marked this pull request as ready for review July 11, 2022 05:22

resquivel-squareup requested review from ikhoon, jrhee17, minwoox and trustin as code owners July 11, 2022 05:22

ikhoon added the defect label Jul 13, 2022

resquivel-squareup changed the title ~~Work-in-progress - Delegating response converter function provider~~ Delegating response converter function provider Jul 14, 2022

jrhee17 added this to the 1.18.0 milestone Jul 14, 2022

minwoox reviewed Jul 14, 2022

View reviewed changes

jrhee17 reviewed Jul 14, 2022

View reviewed changes

.../java/com/linecorp/armeria/internal/server/annotation/ResponseConverterFunctionSelector.java Outdated Show resolved Hide resolved

...java/com/linecorp/armeria/server/annotation/DelegatingResponseConverterFunctionProvider.java Show resolved Hide resolved

resquivel-squareup added 5 commits July 22, 2022 11:09

[4301] Fix copyright in ResponseConverterFunctionSelector

a18d9cb

[4301] Reformat using LINE OSS formatter in IDE

6a6e397

[4301] ResponseConverterFunctionSelector - move static field above st…

849f6b3

…atic block

[4301] ResponseConverterFunctionSelector - move private constructor

a733f23

[4301] Rename ResponseConverterFunctionSelector to ResponseConverterF…

3344c0c

…unctionUtil

resquivel-squareup added 4 commits July 22, 2022 13:41

[4301] Add copyright to test files

3bd2c79

[4301] Reformat ProtobufResponseConverterFunctionProvider using LINE …

bb8e1ae

…OSS linter

[4301] Fix checkstyle issues

a0a4b53

[4301] Fix further checkstyle issue

b6055b8

minwoox reviewed Jul 29, 2022

View reviewed changes

minwoox modified the milestones: 1.18.0, 1.19.0 Aug 1, 2022

Apply suggestions from code review

69e45a1

Co-authored-by: minux <songmw725@gmail.com>

minwoox reviewed Aug 12, 2022

View reviewed changes

ikhoon and others added 5 commits August 16, 2022 16:21

Update core/src/main/java/com/linecorp/armeria/internal/server/annota…

60392e8

…tion/ResponseConverterFunctionUtil.java Co-authored-by: minux <songmw725@gmail.com>

Update core/src/main/java/com/linecorp/armeria/internal/server/annota…

a2e3604

…tion/ResponseConverterFunctionUtil.java Co-authored-by: minux <songmw725@gmail.com>

Update core/src/main/java/com/linecorp/armeria/server/annotation/Dele…

eee1a4a

…gatingResponseConverterFunctionProvider.java Co-authored-by: minux <songmw725@gmail.com>

Clean up

d601238

Fix import

9e33c13

ikhoon approved these changes Aug 16, 2022

View reviewed changes

minwoox mentioned this pull request Aug 23, 2022

DO-NOT-MERGE [4301] Return provided converter in Protobuf Response Converter Function #4312

Closed

ks-yim mentioned this pull request Sep 4, 2022

Fix AnnotatedService behavior with void return types #4398

Merged

minwoox approved these changes Sep 5, 2022

View reviewed changes

...main/java/com/linecorp/armeria/internal/server/annotation/ResponseConverterFunctionUtil.java Outdated Show resolved Hide resolved

Merge branch 'master' into pr-4336.20220905143815

f4161d6

jrhee17 approved these changes Sep 5, 2022

View reviewed changes

minwoox changed the title ~~Delegating response converter function provider~~ Add delegating response converter function provider Sep 5, 2022

minwoox merged commit 70d410d into line:master Sep 5, 2022

ikhoon added the breaking change label Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add delegating response converter function provider #4336

Add delegating response converter function provider #4336

resquivel-squareup commented Jul 6, 2022 •

edited by minwoox

Loading

CLAassistant commented Jul 6, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading

minwoox left a comment

minwoox Jul 14, 2022

resquivel-squareup Jul 22, 2022

minwoox Jul 14, 2022

resquivel-squareup Jul 22, 2022

jrhee17 left a comment

minwoox left a comment

minwoox Jul 29, 2022

minwoox Jul 29, 2022

ikhoon Aug 16, 2022

minwoox Jul 29, 2022

ikhoon Aug 16, 2022

minwoox Jul 29, 2022 •

edited

Loading

minwoox Jul 29, 2022

minwoox left a comment

ikhoon commented Aug 16, 2022

ikhoon left a comment

minwoox commented Aug 24, 2022

resquivel-squareup commented Sep 2, 2022

minwoox commented Sep 2, 2022

minwoox left a comment

jrhee17 left a comment

minwoox commented Sep 5, 2022

Add delegating response converter function provider #4336

Add delegating response converter function provider #4336

Conversation

resquivel-squareup commented Jul 6, 2022 • edited by minwoox Loading

CLAassistant commented Jul 6, 2022 • edited Loading

codecov bot commented Jul 8, 2022 • edited Loading

Codecov Report

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

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

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

minwoox Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon commented Aug 16, 2022

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Aug 24, 2022

resquivel-squareup commented Sep 2, 2022

minwoox commented Sep 2, 2022

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox commented Sep 5, 2022

resquivel-squareup commented Jul 6, 2022 •

edited by minwoox

Loading

CLAassistant commented Jul 6, 2022 •

edited

Loading

codecov bot commented Jul 8, 2022 •

edited

Loading

minwoox Jul 29, 2022 •

edited

Loading