Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

throw ISE when gRPC HTTP JSON transcoding is enabled but serialization format JSON is missing #5224

Merged
merged 4 commits into from Oct 18, 2023
Merged

throw ISE when gRPC HTTP JSON transcoding is enabled but serialization format JSON is missing #5224

merged 4 commits into from Oct 18, 2023

Conversation

Dogacel
Copy link
Contributor

@Dogacel Dogacel commented Oct 7, 2023

Motivation:

Explain why you're making this change and what problem you're trying to solve.

Modifications:

  • Throw ISE in GrpcServiceBuilder.build() if transcoding is enabled but serialization format JSON is not set.

Result:

The detailed description on why this is an annoying issue is discussed in the issue.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2023 •
edited

🔍 Build Scan® (commit: 97aa819)

Job name Status Build Scan®
build-windows-latest-jdk-19 ✅ https://ge.armeria.dev/s/yyotk4im5yej6
build-self-hosted-jdk-8 ✅ https://ge.armeria.dev/s/ptd3ig4nuha74
build-self-hosted-jdk-19-snapshot-blockhound ✅ https://ge.armeria.dev/s/p27pl5uaoehic
build-self-hosted-jdk-17-min-java-17-coverage ✅ https://ge.armeria.dev/s/5nzobqnktqba6
build-self-hosted-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/i2d3fz5in7foi
build-self-hosted-jdk-17-leak ✅ https://ge.armeria.dev/s/l27ybmkhwk5om
build-self-hosted-jdk-11 ✅ https://ge.armeria.dev/s/qhwuakni7yvce
build-macos-12-jdk-19 ✅ https://ge.armeria.dev/s/7d3symurfssoq

minwoox
minwoox approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!
Please fix the lint error as well. 😎

Error: eckstyle] [ERROR] /home/ec2-user/actions-runner/_work/armeria/armeria/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java:982:13: '&&' should be on the previous line. [OperatorWrap]

grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java Outdated
* <p>Limitations:
* <p><b>Limitations:</b>
* Make sure {@code GrpcSerializationFormats.JSON} is supported by the service by calling
* {@code this#supportedSerializationFormats(GrpcSerializationFormats.JSON)}. Otherwise, service builder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding this sentence at last instead of containing it in this section?

     * <p>{@link GrpcSerializationFormats#JSON} must be supported via
     * {@link #supportedSerializationFormats(SerializationFormat...)} to enable HTTP/JSON transcoding.

@Dogacel
Copy link
Contributor Author

Dogacel commented Oct 10, 2023

Looks great, thanks! Please fix the lint error as well. 😎

Error: eckstyle] [ERROR] /home/ec2-user/actions-runner/_work/armeria/armeria/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java:982:13: '&&' should be on the previous line. [OperatorWrap]

Oops, did not realize that because build scans looked good 😓

Double checked with ./gradlew :core:lint and now looks good 👍

@minwoox minwoox added the defect label Oct 11, 2023
@minwoox minwoox added this to the 1.26.0 milestone Oct 11, 2023
jrhee17
jrhee17 approved these changes Oct 13, 2023
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Dogacel 👍 🙇 👍

ikhoon
ikhoon approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Dogacel! 🙇‍♂️

grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java Outdated
@@ -651,6 +651,10 @@ public GrpcServiceBuilder unframedGrpcErrorHandler(UnframedGrpcErrorHandler unfr
* </li>
* </ul>
*
* <p>{@link GrpcSerializationFormats#JSON} must be supported via
* {@link #supportedSerializationFormats(SerializationFormat...)} to enable HTTP/JSON transcoding.
Copy link
Contributor

@ikhoon ikhoon Oct 17, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence sounds like users must always set GrpcSerializationFormats#JSON but they don't need it when using the default GrpcSerializationFormats which is all GrpcSerializationFormats#values().
How about?

When custom {@link #supportedSerializationFormats(SerializationFormat...)} are used,
{@link GrpcSerializationFormats#JSON} must be included to enable HTTP/JSON transcoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 👍

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f93ccca) 74.21% compared to head (97aa819) 74.08%.
Report is 7 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #5224      +/-   ##
============================================
- Coverage     74.21%   74.08%   -0.13%     
- Complexity    19858    19964     +106     
============================================
  Files          1705     1715      +10     
  Lines         73185    73557     +372     
  Branches       9357     9365       +8     
============================================
+ Hits          54311    54494     +183     
- Misses        14436    14633     +197     
+ Partials       4438     4430       -8
Files Coverage Δ
...necorp/armeria/server/grpc/GrpcServiceBuilder.java 73.57% <100.00%> (+0.62%) ⬆️

... and 49 files with indirect coverage changes

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

@ikhoon ikhoon merged commit 7efd95b into line:main Oct 18, 2023
15 checks passed
Bue-von-hon pushed a commit to Bue-von-hon/armeria that referenced this pull request Nov 10, 2023
@Dogacel @Bue-von-hon
throw ISE when gRPC HTTP JSON transcoding is enabled but serializatio…
614753c 
…n format JSON is missing (line#5224)

Modifications:

- Throw ISE in `GrpcServiceBuilder.build()` if transcoding is enabled but serialization format `JSON` is not set.

Result:

- Closes line#5176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikhoon ikhoon ikhoon approved these changes

@jrhee17 jrhee17 jrhee17 approved these changes

@minwoox minwoox minwoox approved these changes

@trustin trustin Awaiting requested review from trustin trustin is a code owner

Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
1.26.0
Development

Successfully merging this pull request may close these issues.

gRPC Http-Json transcoding throws 500 when supported serialization format does not include JSON.
4 participants
@Dogacel @ikhoon @jrhee17 @minwoox