Support custom json marshaller for unframed gRPC error #5555

sato9818 · 2024-04-02T08:57:51Z

Motivation:

A user can't use custom JSON marshaller for unframed grpc error.

Modifications:

Implement UnframedGrpcErrorHandlerBuilder which takes a custom json marshaller

Result:

Closes Support custom JSON marshaller for error details in unframed grpc error #4723.
A user can use their custom JSON marshaller for unframed grpc error.

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

codecov · 2024-04-02T09:31:50Z

Codecov Report

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

Project coverage is 74.05%. Comparing base (b8eb810) to head (9c3e53e).
Report is 154 commits behind head on main.

❗ Current head 9c3e53e differs from pull request most recent head 09f782f. Consider uploading reports for the commit 09f782f to get more accurate results

Files	Patch %	Lines
...a/server/grpc/UnframedGrpcErrorHandlerBuilder.java	0.00%	40 Missing ⚠️
...ria/server/grpc/UnframedGrpcErrorResponseType.java	0.00%	3 Missing ⚠️
.../armeria/server/grpc/UnframedGrpcErrorHandler.java	0.00%	2 Missing ⚠️
...armeria/server/grpc/UnframedGrpcErrorHandlers.java	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5555      +/-   ##
============================================
+ Coverage     73.95%   74.05%   +0.09%     
- Complexity    20115    20982     +867     
============================================
  Files          1730     1821      +91     
  Lines         74161    77352    +3191     
  Branches       9465     9882     +417     
============================================
+ Hits          54847    57280    +2433     
- Misses        14837    15421     +584     
- Partials       4477     4651     +174

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

github-actions · 2024-04-02T10:02:31Z

🔍 Build Scan® (commit: `09f782f`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/46qgxqrgv4p4i
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/f4tjzlpqwkvd4
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/cqdl25toic56u
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/fpwjrwi7d4cfq
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/z3eic2evoxzzm
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/wysabrwveiq6w
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/v44oraw4pwwsg
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/bv7czjpqelcnw

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

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

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

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

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

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

…functions

trustin

Mostly minor things - thanks for your patience!

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

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

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

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

…pcErrorHandlerBuilder.java

trustin

Left some nits, but it looks good! 👍

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

…r-builder

…of github.com:sato9818/armeria into feature/implement-unframed-grpc-error-handler-builder

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

ikhoon

Looks nice and useful! Thanks, @sato9818. 🙇‍♂️🙇‍♂️

ikhoon · 2024-04-12T05:30:49Z

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

@@ -110,7 +114,8 @@ static UnframedGrpcErrorHandler of(UnframedGrpcStatusMappingFunction statusMappi
     * @param statusMappingFunction The function which maps the {@link Throwable} or gRPC {@link Status} code
     *                              to an {@link HttpStatus} code.
     */
-    static UnframedGrpcErrorHandler ofJson(UnframedGrpcStatusMappingFunction statusMappingFunction) {
+    static UnframedGrpcErrorHandler ofJson(
+            UnframedGrpcStatusMappingFunction statusMappingFunction, MessageMarshaller jsonMarshaller) {


Optional) This class becomes larger and more complex compared to its initial implementation. I think it's time to refactor. Are you interested in sending a follow/up PR to extract the code into TextUnframedGrpcErrorHandler and JsonUnframedGrpcErrorHandler?

ikhoon · 2024-04-12T05:33:18Z

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

+/**
+ * Constructs a {@link UnframedGrpcErrorHandler} to handle unframed gRPC errors.
+ */
+public final class UnframedGrpcErrorHandlerBuilder {


Suggested change

public final class UnframedGrpcErrorHandlerBuilder {

@UnstableApi

public final class UnframedGrpcErrorHandlerBuilder {

When should we use @Unstable generally?
I'm just curious.

We add @UnstableApi for new public APIs. It is best not to produce any breaking changes. However, it is difficult to make an API stable from the start. @UnstableApi is a marker that the API can be changed in a minor update.

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

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

ikhoon

Still LGTM! 🙇‍♂️

minwoox

Thanks!

@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)

Motivation: Refactor `UnframedGrpcErrorHandlers`. #5555 (comment) Modifications: - Create `TextUnframedGrpcErrorHandler` for a plaintext format response. - Create `JsonUnframedGrpcErrorHandler` for a JSON format response. - Create `DefaultUnframedGrpcErrorHandler` for a default error handler. Result: - Seperate `UnframedGrpcErrorHandlers` into three classes, `TextUnframedGrpcErrorHandler`, `JsonUnframedGrpcErrorHandler` and `DefaultUnframedGrpcErrorHandler`. --------- Co-authored-by: minwoox <songmw725@gmail.com> Co-authored-by: jrhee17 <guins_j@guins.org>

Motivation: - A user can't use custom JSON marshaller for unframed grpc error. Modifications: - Implement `UnframedGrpcErrorHandlerBuilder` which takes a custom json marshaller Result: - Closes line#4723. - A user can use their custom JSON marshaller for unframed grpc error.

Motivation: Refactor `UnframedGrpcErrorHandlers`. line#5555 (comment) Modifications: - Create `TextUnframedGrpcErrorHandler` for a plaintext format response. - Create `JsonUnframedGrpcErrorHandler` for a JSON format response. - Create `DefaultUnframedGrpcErrorHandler` for a default error handler. Result: - Seperate `UnframedGrpcErrorHandlers` into three classes, `TextUnframedGrpcErrorHandler`, `JsonUnframedGrpcErrorHandler` and `DefaultUnframedGrpcErrorHandler`. --------- Co-authored-by: minwoox <songmw725@gmail.com> Co-authored-by: jrhee17 <guins_j@guins.org>

sato9818 added 2 commits April 2, 2024 17:45

Implement draft UnframedGrpcErrorHandlerBuilder

f6e581c

Add copyright

383d8b5

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin April 2, 2024 08:58

sato9818 changed the title ~~Support custom json marshaller for unframed gRPC error~~ [WIP] Support custom json marshaller for unframed gRPC error Apr 2, 2024

sato9818 commented Apr 2, 2024

View reviewed changes

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

ikhoon added new feature sprint Issues for OSS Sprint participants labels Apr 2, 2024

trustin reviewed Apr 2, 2024

View reviewed changes

Refactor UnframedGrpcErrorHandlerBuilder

9c3e53e

trustin reviewed Apr 5, 2024

View reviewed changes

Fix UnframedGrpcErrorHandlerBuilder and add tests and javadocs

6671ea9

sato9818 changed the title ~~[WIP] Support custom json marshaller for unframed gRPC error~~ Support custom json marshaller for unframed gRPC error Apr 10, 2024

sato9818 marked this pull request as ready for review April 10, 2024 02:13

sato9818 requested a review from trustin April 10, 2024 02:13

Update javadoc on registerMarshalledMessages

b3c3cad

trustin reviewed Apr 10, 2024

View reviewed changes

Address reviews

194ab48

trustin reviewed Apr 11, 2024

View reviewed changes

Add extra tests and seperate registerMarshalledMessages fun into two …

a5043b2

…functions

sato9818 requested a review from trustin April 11, 2024 09:40

trustin reviewed Apr 11, 2024

View reviewed changes

Fix format and address review

bb3d77f

trustin reviewed Apr 12, 2024

View reviewed changes

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

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

b6b1144

…pcErrorHandlerBuilder.java

trustin approved these changes Apr 12, 2024

View reviewed changes

trustin and others added 4 commits April 12, 2024 10:54

Merge branch 'main' into feature/implement-unframed-grpc-error-handle…

fd9e00a

…r-builder

Fix boolean assertion

0791ca1

Merge branch 'feature/implement-unframed-grpc-error-handler-builder' …

0acbcce

…of github.com:sato9818/armeria into feature/implement-unframed-grpc-error-handler-builder

Fix test

8f9da64

trustin reviewed Apr 12, 2024

View reviewed changes

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

Remove requireNonNull from test

b284399

ikhoon approved these changes Apr 12, 2024

View reviewed changes

Add @unstable

09f782f

sato9818 requested a review from ikhoon April 15, 2024 02:08

ikhoon approved these changes Apr 17, 2024

View reviewed changes

minwoox approved these changes Apr 17, 2024

View reviewed changes

ikhoon merged commit 1d268aa into line:main Apr 17, 2024
16 checks passed

sato9818 mentioned this pull request Apr 17, 2024

Refactor UnframedGrpcErrorHandlers #5617

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom json marshaller for unframed gRPC error #5555

Support custom json marshaller for unframed gRPC error #5555

sato9818 commented Apr 2, 2024

codecov bot commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

trustin left a comment

trustin left a comment

ikhoon left a comment

ikhoon Apr 12, 2024

sato9818 Apr 12, 2024

ikhoon Apr 12, 2024

sato9818 Apr 12, 2024

ikhoon Apr 17, 2024

ikhoon left a comment

minwoox left a comment

	public final class UnframedGrpcErrorHandlerBuilder {
	@UnstableApi
	public final class UnframedGrpcErrorHandlerBuilder {

Support custom json marshaller for unframed gRPC error #5555

Support custom json marshaller for unframed gRPC error #5555

Conversation

sato9818 commented Apr 2, 2024

codecov bot commented Apr 2, 2024 • edited Loading

Codecov Report

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

🔍 Build Scan® (commit: 09f782f)

trustin left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon Apr 12, 2024

Choose a reason for hiding this comment

sato9818 Apr 12, 2024

Choose a reason for hiding this comment

ikhoon Apr 12, 2024

Choose a reason for hiding this comment

sato9818 Apr 12, 2024

Choose a reason for hiding this comment

ikhoon Apr 17, 2024

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

🔍 Build Scan® (commit: `09f782f`)