Add support for alternative protobuf content types #4364

mscheong01 · 2022-07-22T07:15:47Z

Motivation:
Armeria UnframedGrpcService doesn't support alternative protobuf content types

Modifications:

add application/x-protobuf and application/x-google-protobuf to MediaType
add isProtobuf() function
identify Protobuf contentType in UnframedGrpcService using isProtobuf()
respond with request-given protobuf content type for deframed response

Result:

Closes Support protobuf Content-Type not limited to "application/protobuf" in UnframedGrpcService #4355
Armeria UnframedGrpcService now supports application/x-protobuf and application/x-google-protobuf media types.

mscheong01

PR is not yet ready, but I wanted some help along the way

mscheong01 · 2022-07-22T07:17:41Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/AbstractUnframedGrpcService.java

-            if (grpcMediaType.is(GrpcSerializationFormats.PROTO.mediaType())) {
-                unframedHeaders.contentType(MediaType.PROTOBUF);
-            } else if (grpcMediaType.is(GrpcSerializationFormats.JSON.mediaType())) {
-                unframedHeaders.contentType(MediaType.JSON_UTF_8);
-            }
+            unframedHeaders.contentType(originalContentType);


for requests that use x-protobuf or x-google-protobuf, my guess is that they would expect the server to respond with the same content type. Would using the passed down originalContentType be safe? Also, should accept header be checked for the response protobuf content type?

Also, should there be an else clause here? (for when grpcMediaType is null)

Would using the passed down originalContentType be safe? Also, should accept header be checked for the response protobuf content type?

If a suitable accept-header is specified, it makes sense to respect the value over content-type.

Also, should there be an else clause here? (for when grpcMediaType is null)

It looks like defensive code.
A normal response including an OK status should be returned with a proper content-type.

armeria/grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java

Lines 181 to 187 in 49054ad

defaultHeaders = supportedSerializationFormats

.stream()

.map(format -> {

final ResponseHeadersBuilder builder =

ResponseHeaders

.builder(HttpStatus.OK)

.contentType(format.mediaType())

ikhoon · 2022-07-25T03:46:44Z

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

@@ -1052,6 +1054,20 @@ public boolean isJson() {
        return is(JSON) || subtype().endsWith("+json");
    }

+    /**
+     * Returns {@code true} when the subtype is in [{@link MediaType#PROTOBUF}, {@link MediaType#X_PROTOBUF}, {@link MediaType#X_GOOGLE_PROTOBUF}].


We prefer to list elements using and or or.

Suggested change

* Returns {@code true} when the subtype is in [{@link MediaType#PROTOBUF}, {@link MediaType#X_PROTOBUF}, {@link MediaType#X_GOOGLE_PROTOBUF}].

* Returns {@code true} when the subtype is one of {@link MediaType#PROTOBUF}, {@link MediaType#X_PROTOBUF}, and {@link MediaType#X_GOOGLE_PROTOBUF}.

mscheong01 · 2022-07-27T14:35:24Z

Some fixes are made based on review & test breaks
I wondered if there should be an accept-header checking logic in httpjsontranscodingservice too but concluded that that sort of validation should be placed where it could be used more generally on all http req/resps if needed.
If the fix looks ok, I'll start on writing tests someday soon.

codecov · 2022-07-27T15:42:13Z

Codecov Report

Merging #4364 (2b82ff4) into master (e2cc62a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #4364      +/-   ##
============================================
- Coverage     73.84%   73.83%   -0.02%     
- Complexity    17702    17721      +19     
============================================
  Files          1505     1509       +4     
  Lines         66105    66201      +96     
  Branches       8309     8311       +2     
============================================
+ Hits          48816    48880      +64     
- Misses        13289    13320      +31     
- Partials       4000     4001       +1

Impacted Files	Coverage Δ
...in/java/com/linecorp/armeria/common/MediaType.java	`97.06% <100.00%> (+0.02%)`	⬆️
...meria/server/grpc/AbstractUnframedGrpcService.java	`83.90% <100.00%> (+4.59%)`	⬆️
...rmeria/server/grpc/HttpJsonTranscodingService.java	`80.09% <100.00%> (+0.04%)`	⬆️
...ecorp/armeria/server/grpc/UnframedGrpcService.java	`82.50% <100.00%> (-0.43%)`	⬇️
...ria/internal/client/dns/DelegatingDnsResolver.java	`81.25% <0.00%> (-18.75%)`	⬇️
...ecorp/armeria/server/LengthBasedServiceNaming.java	`75.00% <0.00%> (-16.67%)`	⬇️
...rmeria/internal/client/dns/CachingDnsResolver.java	`78.37% <0.00%> (-8.11%)`	⬇️
...a/internal/client/dns/SearchDomainDnsResolver.java	`87.14% <0.00%> (-5.72%)`	⬇️
...eria/internal/common/stream/StreamMessageUtil.java	`61.11% <0.00%> (-5.56%)`	⬇️
...rnal/common/stream/AbstractFixedStreamMessage.java	`89.23% <0.00%> (-3.08%)`	⬇️
... and 25 more

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

jrhee17

Changes look good to me so far 👍

If the fix looks ok, I'll start on writing tests someday soon.

Please do so 🙏

jrhee17 · 2022-08-12T01:23:50Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/AbstractUnframedGrpcService.java

@@ -210,14 +212,15 @@ static void deframeAndRespond(ServiceRequestContext ctx,
        }

        final MediaType grpcMediaType = grpcResponse.contentType();
+        requireNonNull(grpcMediaType);


We shouldn't throw exceptions in this method since the exception isn't propagated to the response.

res.completeExceptionally(new NullPointerException("MediaType is undefined")); return;

done fixing in addition to closing pooled object before the return

jrhee17 · 2022-08-12T01:26:50Z

grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGrpcService.java

-                    frameAndServe(unwrap(), ctx, grpcHeaders.build(),
-                                  clientRequest.content(), responseFuture, null);
+                    frameAndServe(unwrap(), ctx, grpcHeaders.build(), clientRequest.content(),
+                            responseFuture, null, responseContentType);


Incorrect indentation - can you check that your IDE is correctly formatted?
https://armeria.dev/community/developer-guide#setting-up-your-ide

CLAassistant · 2022-08-22T14:59:09Z

All committers have signed the CLA.

…m:j-min5u/armeria into support-additional-protobuf-content-types

ikhoon

Thanks, @j-min5u! 😀
Left minor comments for testing.

grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGrpcService.java

...src/test/java/com/linecorp/armeria/server/grpc/UnframedGrpcServiceResponseMediaTypeTest.java

grpc/src/test/java/com/linecorp/armeria/server/grpc/UnframedGrpcServiceTest.java

…pcService.java

…m:j-min5u/armeria into support-additional-protobuf-content-types

mscheong01 · 2022-08-23T14:04:08Z

some fixes were added based on review.
Plus, I missed this

If a suitable accept-header is specified, it makes sense to respect the value over content-type.

should I do it? I'd have to pass the request acceptheaders to deframeAndRespond, then set response contentType to that included in acceptHeader if the request contentType isn't in it. Seems like a pretty extreme case though 🤔

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

...src/test/java/com/linecorp/armeria/server/grpc/UnframedGrpcServiceResponseMediaTypeTest.java

minwoox · 2022-08-24T03:52:02Z

should I do it? I'd have to pass the request acceptheaders to deframeAndRespond, then set response contentType to that included in acceptHeader if the request contentType isn't in it. Seems like a pretty extreme case though 🤔

Here's what I thought:

For HttpJsonTranscodingService, we always set GrpcSerializationFormats.JSON and I think we should do that because we generate the content regardless of the accept header.
For UnframedGrpcService, how about just setting the same content type from the request headers?
This is the link from OTLP and it says that The server MUST use the same “Content-Type” in the response as it received in the request. With the current approach in this PR, if the client forgets to set the accept header, the server violates the rule.

So, we could remove these lines completely and just pass the contentType to the frameAndServe method.

mscheong01 · 2022-08-25T09:23:21Z

So, we could remove these lines completely and just pass the contentType to the frameAndServe method.

I actually forgot about writing these lines 😅 removed them in commit a498c67

minwoox

Sorry for the late review. Left minor comments. 😉

grpc/src/main/java/com/linecorp/armeria/server/grpc/AbstractUnframedGrpcService.java

minwoox

👍 👍 👍
Thanks a lot for fixing this bug, @j-min5u! 😄

jrhee17

Looks good to me! Thanks @j-min5u ! 🙇 👍 🙇

minwoox · 2022-09-05T14:10:19Z

Our new hero, @j-min5u! 😆
What's your next PR? 😆

mscheong01 · 2022-09-06T01:19:01Z

@minwoox
Ha ha, I'm flattered 😄 But I'm going to military training in about a week, so nothing soon I'm afraid,,

minwoox · 2022-09-07T14:15:49Z

Please, don't worry. 😄 Take care of yourself and see you later. 😄

Motivation: Armeria `UnframedGrpcService` doesn't support alternative protobuf content types Modifications: - add `application/x-protobuf` and `application/x-google-protobuf` to MediaType - add isProtobuf() function - identify Protobuf contentType in UnframedGrpcService using isProtobuf() - respond with request-given protobuf content type for deframed response Result: - Closes line#4355 - Armeria `UnframedGrpcService` now supports `application/x-protobuf` and `application/x-google-protobuf` media types.

Motivation: Wrong response contenttype served for http json transcoding endpoints as result of #4364 Modifications: - since AbstractUnframedGrpcService$frameAndServe uses the provided responseContentType parameter directly as the resulting http contenttype, application/json should be provided in HttpJsonTranscodingService instead of application/grpc+json - alternative would be to respond with the request contenttype ex) JSON -> JSON / JSON_UTF_8 -> JSON_UTF_8 Result: - Closes #4473

mscheong01 requested review from ikhoon, jrhee17, minwoox and trustin as code owners July 22, 2022 07:15

mscheong01 commented Jul 22, 2022

View reviewed changes

mscheong01 changed the title ~~add support for additional protobuf content types~~ Add support for alternative protobuf content types Jul 22, 2022

ikhoon reviewed Jul 25, 2022

View reviewed changes

ikhoon added the defect label Jul 27, 2022

ikhoon modified the milestones: 1.19.0, 1.18.0 Jul 27, 2022

jrhee17 modified the milestones: 1.18.0, 1.19.0 Jul 28, 2022

jrhee17 reviewed Aug 12, 2022

View reviewed changes

mscheong01 added 8 commits August 23, 2022 00:00

add support for additional protobuf content types

851aae4

add to MediaTypeNames

8960e69

fixes baed on review @ikhoon

f3d8bf7

Merge branch 'support-additional-protobuf-content-types' of github.co…

98fea98

…m:j-min5u/armeria into support-additional-protobuf-content-types

add testcases for UnframedGrpcService response mediaType

ada1ef7

close pooled object and completeExceptionally for null grpcMediaType

4f14929

add license

fda8690

fix style

239f63e

mscheong01 force-pushed the support-additional-protobuf-content-types branch from 8f9b775 to 239f63e Compare August 22, 2022 15:01

fix indent

efc2525

ikhoon approved these changes Aug 23, 2022

View reviewed changes

ikhoon and others added 2 commits August 23, 2022 18:45

Update grpc/src/main/java/com/linecorp/armeria/server/grpc/UnframedGr…

575f321

…pcService.java

code review @ikhoon

4dd3040

Merge branch 'support-additional-protobuf-content-types' of github.co…

6195f8f

…m:j-min5u/armeria into support-additional-protobuf-content-types

minwoox reviewed Aug 24, 2022

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/MediaType.java Outdated Show resolved Hide resolved

...src/test/java/com/linecorp/armeria/server/grpc/UnframedGrpcServiceResponseMediaTypeTest.java Outdated Show resolved Hide resolved

code review @minwoox

a498c67

fix test break

ddb59b7

minwoox reviewed Aug 30, 2022

View reviewed changes

code review @minwoox

2b82ff4

minwoox approved these changes Aug 31, 2022

View reviewed changes

jrhee17 approved these changes Sep 5, 2022

View reviewed changes

minwoox merged commit e7da834 into line:master Sep 5, 2022

ikhoon mentioned this pull request Oct 10, 2022

HTTP/JSON gRPC transcoding services does not return application/json but applicaiton/grpc+json #4473

Closed

mscheong01 mentioned this pull request Oct 11, 2022

Respond with application/json for httpjson transcoding endpoints #4474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for alternative protobuf content types #4364

Add support for alternative protobuf content types #4364

mscheong01 commented Jul 22, 2022 •

edited by minwoox

mscheong01 left a comment

mscheong01 Jul 22, 2022

mscheong01 Jul 22, 2022

ikhoon Jul 25, 2022

ikhoon Jul 25, 2022

mscheong01 commented Jul 27, 2022

codecov bot commented Jul 27, 2022 •

edited

jrhee17 left a comment

jrhee17 Aug 12, 2022

mscheong01 Aug 22, 2022

jrhee17 Aug 12, 2022

CLAassistant commented Aug 22, 2022 •

edited

ikhoon left a comment

mscheong01 commented Aug 23, 2022

minwoox commented Aug 24, 2022

mscheong01 commented Aug 25, 2022

minwoox left a comment

minwoox left a comment

jrhee17 left a comment

minwoox commented Sep 5, 2022

mscheong01 commented Sep 6, 2022

minwoox commented Sep 7, 2022

	defaultHeaders = supportedSerializationFormats
	.stream()
	.map(format -> {
	final ResponseHeadersBuilder builder =
	ResponseHeaders
	.builder(HttpStatus.OK)
	.contentType(format.mediaType())

	* Returns {@code true} when the subtype is in [{@link MediaType#PROTOBUF}, {@link MediaType#X_PROTOBUF}, {@link MediaType#X_GOOGLE_PROTOBUF}].
	* Returns {@code true} when the subtype is one of {@link MediaType#PROTOBUF}, {@link MediaType#X_PROTOBUF}, and {@link MediaType#X_GOOGLE_PROTOBUF}.

Add support for alternative protobuf content types #4364

Add support for alternative protobuf content types #4364

Conversation

mscheong01 commented Jul 22, 2022 • edited by minwoox

mscheong01 left a comment

Choose a reason for hiding this comment

mscheong01 Jul 22, 2022

Choose a reason for hiding this comment

mscheong01 Jul 22, 2022

Choose a reason for hiding this comment

ikhoon Jul 25, 2022

Choose a reason for hiding this comment

ikhoon Jul 25, 2022

Choose a reason for hiding this comment

mscheong01 commented Jul 27, 2022

codecov bot commented Jul 27, 2022 • edited

Codecov Report

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Aug 12, 2022

Choose a reason for hiding this comment

mscheong01 Aug 22, 2022

Choose a reason for hiding this comment

jrhee17 Aug 12, 2022

Choose a reason for hiding this comment

CLAassistant commented Aug 22, 2022 • edited

ikhoon left a comment

Choose a reason for hiding this comment

mscheong01 commented Aug 23, 2022

minwoox commented Aug 24, 2022

mscheong01 commented Aug 25, 2022

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

minwoox commented Sep 5, 2022

mscheong01 commented Sep 6, 2022

minwoox commented Sep 7, 2022

mscheong01 commented Jul 22, 2022 •

edited by minwoox

codecov bot commented Jul 27, 2022 •

edited

CLAassistant commented Aug 22, 2022 •

edited