Provide a way to encode and decode Protobuf in annotated services #3124

ikhoon · 2020-10-21T08:43:11Z

Motivation:

Protocol Buffers is a universal serialization protocol. The usage is not limit to gRPC.
It could be used an HTTP payload for REST API.
It is also useful when building an HTTP proxy server in front of a gRPC server.

Modifications:

Add RequestConverterFunctionProvider that can load a custom RequestConverterFunction using SPI
Add @ConsumesProtobuf and @ProducesProtobuf annotations
Protobuf
- Create armeria-protobuf module to use Protobuf in annotated services
- Add ProtobufRequestConverterFunction that decodes Protocol Buffers or JSON body into Protobuf Message type
- Add ProtobufResponseConverterFunction that encodes Protobuf Message(s) into Protocol Buffers or JSON
- Add ProtobufRequestConverterFunctionProvider and ProtobufResponseConverterFunctionProvider to support Protobuf converters automatically via SPI
ScalaPB
- Create armeria-scalapb_2.12 and armeria-scalapb_2.13 module to use ScalaPB in annotated services
- Add ScalaPbRequestConverterFunction that decodes Protocol Buffers or JSON body into ScalaPB GenreatedMessage type
- Add ScalaPbResponseConverterFunction that encodes ScalaPB GenerratedMesage(s) into Protocol Buffers or JSON
- Add ScalaPbRequestConverterFunctionProvider and ScalaPbResponseConverterFunctionProvider to support ScalaPB converters automatically via SPI
These converters are applied only when the content-type is:
- Protobuf - application/protobuf, application/x-protobuf or application/octet-stream
- JSON - application/json; charset=utf-8 or subtype ends with +json

Result:

You can now use Protobuf's Message and ScalaPB's GeneratedMessage as a request/response object in annotated service.
Closes Add Protocol Buffers codec for annoated service #3088

Motivation: Protocol Buffers is an universal serialization protocol. The usage is not limit to gRPC. It could be used an HTTP payload for REST API. It is also useful when building an HTTP proxy server in front of a gRPC server. Modifications: - Add ProtobufRequestConverterFunction that decodes Protocol Buffers or JSON body into Protobuf Message type - Add ProtobufResponseConverterFunction that encodes Protobuf Message(s) into Protocol Buffers or JSON body - These converters are applied only when the content type is: - `application/protobuf` - `application/json; charset=utf-8` - Or subtype contains `protobuf` because there is no standard for protobuf content type. See https://stackoverflow.com/questions/30505408/what-is-the-correct-protobuf-content-type Result: - You can now encode and decode Protobuf Message using annotated service automatically. - Closes line#3088

ikhoon · 2020-10-21T08:43:54Z

/cc @dpsoft

codecov · 2020-10-21T09:11:40Z

Codecov Report

Merging #3124 (11b4702) into master (bd7fe49) will increase coverage by 0.02%.
The diff coverage is 74.80%.

@@             Coverage Diff              @@
##             master    #3124      +/-   ##
============================================
+ Coverage     73.92%   73.95%   +0.02%     
- Complexity    12417    12587     +170     
============================================
  Files          1080     1098      +18     
  Lines         47380    47955     +575     
  Branches       5999     6094      +95     
============================================
+ Hits          35027    35463     +436     
- Misses         9269     9365      +96     
- Partials       3084     3127      +43

Impacted Files	Coverage Δ	Complexity Δ
...a/com/linecorp/armeria/client/retry/RetryRule.java	`69.69% <ø> (ø)`	`16.00 <0.00> (ø)`
.../annotation/ByteArrayRequestConverterFunction.java	`71.42% <ø> (ø)`	`3.00 <0.00> (ø)`
...annotation/ByteArrayResponseConverterFunction.java	`75.86% <ø> (ø)`	`11.00 <0.00> (ø)`
...er/annotation/JacksonRequestConverterFunction.java	`87.17% <ø> (ø)`	`15.00 <0.00> (ø)`
...r/annotation/JacksonResponseConverterFunction.java	`83.87% <ø> (ø)`	`13.00 <0.00> (ø)`
...ver/annotation/StringRequestConverterFunction.java	`75.00% <ø> (ø)`	`4.00 <0.00> (ø)`
...er/annotation/StringResponseConverterFunction.java	`78.12% <ø> (ø)`	`14.00 <0.00> (ø)`
...in/example/armeria/server/annotated/kotlin/Main.kt	`70.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../example/armeria/contextpropagation/kotlin/Main.kt	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...2/ObservableResponseConverterFunctionProvider.java	`84.00% <ø> (ø)`	`15.00 <0.00> (ø)`
... and 91 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 bd7fe49...11b4702. Read the comment docs.

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

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

minwoox · 2020-10-26T11:28:32Z

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

+ * However a <a href="https://tools.ietf.org/html/rfc7159#section-5">JSON array</a> body
+ * could be converted into a {@link Collection} type. e.g, {@code List<Message>} and {@code Set<Message>}.
+ */
+public final class ProtobufRequestConverterFunction implements RequestConverterFunction {


Shouldn't we make a separate module so that the user doesn't have to bring gRPC dependencies to use this?

I thought this feature would be interesting to some users who want to gRPC service and client already. Do we need armeria-protobuf? 🤔

I'm not sure. 🤣 @anuraaga Do you have any suggestions?

Sorry for late reply - it does seem strange to have this in gRPC package since it seems to have nothing to do with gRPC. Perhaps core with compileOnly dependency on protobuf so it's next to the other annotated service functionality? not sure

Yeah, the code is too small to create a seperate module. Let me move this code to core and add @UnstableApi in case we need armeria-protobuf later.

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

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

grpc/build.gradle

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

...rc/main/java/com/linecorp/armeria/server/grpc/ProtobufResponseConverterFunctionProvider.java

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

…dule

minwoox

Just nits.
Great job, @ikhoon!

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

...buf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufRequestConverterFunction.java

...uf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufResponseConverterFunction.java

site/src/pages/docs/setup.mdx

...uf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufResponseConverterFunction.java

...buf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufRequestConverterFunction.java

trustin · 2020-11-09T06:58:38Z

...main/java/com/linecorp/armeria/server/protobuf/ProtobufRequestConverterFunctionProvider.java

+    }
+
+    private static boolean isProtobufMessage(Class<?> clazz) {
+        return Message.class.isAssignableFrom(clazz);


Message or MessageLite?

Message class extends MessageLite. When building(deserializing) a message from protobuf, Message class is required to get Builder class. However, when serializing a message to protobuf, MessageLite is enough.
This inconsistency is able to confuse users. Let me just use Message instead of MessageLite.

MessageLite is very rare in servers btw (though I remember an issue filed by someone that was actually using it don't remember the reasoning)

I remember an issue filed by someone that was actually using it don't remember the reasoning

Maybe #3085? He said it was caused by wrong dependencies.

...uf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufResponseConverterFunction.java

...c/main/java/com/linecorp/armeria/internal/server/annotation/CompositeConverterFunctions.java

trustin · 2020-11-16T08:16:26Z

gradle.properties

+scalaVersions=2.13.3
+defaultScalaVersions=2.13.2


Question: Why these properties have two different versions?

Oops... it was cruft. Actually, I removed gradle-scala-multiversion-plugin and add scalapb_2.12 and scalapb_2.13 because it is difficult to integrate gradle-scala-multiversion-plugin with Armeria's gradle scripts.
And we need a lot of workarounds and some of them are flaky. 😅

gradle/scripts/lib/java-publish.gradle

gradle/scripts/lib/java-shade.gradle

...uf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufResponseConverterFunction.java

protobuf/src/main/java/com/linecorp/armeria/server/protobuf/package-info.java

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

...apb/src/main/scala/com/linecorp/armeria/server/scalapb/ScalaPbRequestConverterFunction.scala

...src/test/scala/com/linecorp/armeria/server/scalapb/ScalaPbRequestConverterFunctionTest.scala

ikhoon · 2020-11-17T01:36:37Z

Finally, all builds are passed. 😆

minwoox

Still LGTM 👍
(Do not know much about the Scala part 🤣 )

minwoox · 2020-11-19T01:26:17Z

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

@@ -0,0 +1,46 @@
+/*
+ * Copyright 2018 LINE Corporation


minwoox · 2020-11-19T01:32:10Z

scalapb/scalapb_2.13/build.gradle

+    implementation "com.thesamet.scalapb:scalapb-runtime-grpc_2.13"
+}
+
+


nit: two empty lines

minwoox · 2020-11-19T01:32:32Z

scalapb/scalapb_2.13/build.gradle

+
+// Run `scalafmt` to automatically format scala code from source sets
+// https://github.com/alenkacz/gradle-scalafmt#tasks
+project.ext.getLintTask().dependsOn tasks.checkScalafmt


trustin

How will we have to expose Scaladoc in our web site by the way?

...uf/src/main/java/com/linecorp/armeria/server/protobuf/ProtobufResponseConverterFunction.java

....13/src/main/scala/com/linecorp/armeria/server/scalapb/ScalaPbRequestConverterFunction.scala

site/src/pages/docs/setup.mdx

ikhoon · 2020-11-19T04:14:51Z

How will we have to expose Scaladoc in our web site by the way?

How about adding a dedicated documentation page for ScalaPB? The page could have a simple introduction and a Scaladoc link.
I think we can add the page after migrating ScalaPB plugins from examples/examples/grpc-scala.

trustin

Great work, @ikhoon. ScalaPB users will love this.

trustin · 2020-11-19T23:13:22Z

@ikhoon Could you update the PR description?

ikhoon · 2020-11-20T02:18:29Z

@ikhoon Could you update the PR description?

Sure! The PR description is updated.

ikhoon added the new feature label Oct 21, 2020

ikhoon requested review from minwoox and trustin as code owners October 21, 2020 08:43

checkstyle

ca3b157

Merge branch 'master' into protobuf-response-converter

5577893

ikhoon commented Oct 23, 2020

View reviewed changes

grpc/src/main/java/com/linecorp/armeria/server/grpc/ProtobufRequestConverterFunction.java Outdated Show resolved Hide resolved

Use MethodHandle

07fca0c

ikhoon force-pushed the protobuf-response-converter branch from 324c2d6 to 07fca0c Compare October 23, 2020 09:58

ikhoon added 2 commits October 23, 2020 19:02

Update error message

e529336

Clean up

ea9688b

minwoox reviewed Oct 26, 2020

View reviewed changes

Add RequestConverterFunctionProvider

3158769

trustin reviewed Nov 1, 2020

View reviewed changes

ikhoon added 2 commits November 3, 2020 22:46

Address comments by @minwoox and @trustin / Add 'armeria-protobuf' mo…

545a442

…dule

Typo

1a3aad6

minwoox added this to the 1.3.0 milestone Nov 6, 2020

minwoox approved these changes Nov 6, 2020

View reviewed changes

trustin reviewed Nov 9, 2020

View reviewed changes

ikhoon added 3 commits November 9, 2020 18:33

Merge branch 'master' into protobuf-response-converter

e9353b5

Address comments by @minwoox and @trustin

2030ba5

Add request and response converter for ScalaPB

f5ef663

ikhoon requested a review from minwoox November 11, 2020 03:13

ikhoon added 3 commits November 11, 2020 15:49

Fix shaded test error

9509056

Use defaultScalaVersions

8359b18

Fix compile error with Scala 2.12

84c1a22

trustin reviewed Nov 16, 2020

View reviewed changes

Remove scala multi version plugin and split project

c9c9f2e

ikhoon added 4 commits November 16, 2020 22:04

Use 'api' for scalapb.json4s

69064fd

Exclude from jacoco

2645935

Address comments by @trustin

89b9220

Clean up

4f28b18

ikhoon added 3 commits November 17, 2020 11:30

Clean up dependencies

3136e41

Misc

2218a31

checkstyle

4e8efa0

minwoox approved these changes Nov 19, 2020

View reviewed changes

Address comments by @minwoox

e924fd0

trustin reviewed Nov 19, 2020

View reviewed changes

Address comments by @trustin / Fix Scaladoc indent

11b4702

trustin approved these changes Nov 19, 2020

View reviewed changes

trustin merged commit 0605acd into line:master Nov 20, 2020

ikhoon deleted the protobuf-response-converter branch December 8, 2020 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to encode and decode Protobuf in annotated services #3124

Provide a way to encode and decode Protobuf in annotated services #3124

ikhoon commented Oct 21, 2020 •

edited

Loading

ikhoon commented Oct 21, 2020

codecov bot commented Oct 21, 2020 •

edited

Loading

minwoox Oct 26, 2020

ikhoon Oct 26, 2020

minwoox Oct 26, 2020

anuraaga Nov 3, 2020

ikhoon Nov 3, 2020

minwoox left a comment

trustin Nov 9, 2020

ikhoon Nov 9, 2020 •

edited

Loading

anuraaga Nov 11, 2020

ikhoon Nov 11, 2020

trustin Nov 16, 2020

ikhoon Nov 16, 2020

ikhoon commented Nov 17, 2020

minwoox left a comment

minwoox Nov 19, 2020

ikhoon Nov 19, 2020

minwoox Nov 19, 2020

minwoox Nov 19, 2020

trustin left a comment

ikhoon commented Nov 19, 2020 •

edited

Loading

trustin left a comment

trustin commented Nov 19, 2020

ikhoon commented Nov 20, 2020

		implementation "com.thesamet.scalapb:scalapb-runtime-grpc_2.13"
		}

		scalaVersions=2.13.3
		defaultScalaVersions=2.13.2

Provide a way to encode and decode Protobuf in annotated services #3124

Provide a way to encode and decode Protobuf in annotated services #3124

Conversation

ikhoon commented Oct 21, 2020 • edited Loading

ikhoon commented Oct 21, 2020

codecov bot commented Oct 21, 2020 • edited Loading

Codecov Report

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Nov 9, 2020 • 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

ikhoon commented Nov 17, 2020

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

trustin left a comment

Choose a reason for hiding this comment

ikhoon commented Nov 19, 2020 • edited Loading

trustin left a comment

Choose a reason for hiding this comment

trustin commented Nov 19, 2020

ikhoon commented Nov 20, 2020

ikhoon commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading

ikhoon Nov 9, 2020 •

edited

Loading

ikhoon commented Nov 19, 2020 •

edited

Loading