Allow specifying additional HttpRules for gRPC/JSON transcoding #6567
Conversation
0x1306e6d
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
📝 WalkthroughWalkthroughAdds programmatic HTTP/JSON transcoding controls: ignoreProtoHttpRule, additionalHttpRules, and conflictStrategy; moves transcoding spec generation into a new HttpJsonTranscodingServiceBuilder; introduces builder copy semantics and updates tests to cover rules and conflict strategies. Changes
Sequence Diagram(s)sequenceDiagram
participant GrpcSB as GrpcServiceBuilder
participant OptBuilder as HttpJsonTranscodingOptionsBuilder
participant OptDef as DefaultHttpJsonTranscodingOptions
participant SvcBuilder as HttpJsonTranscodingServiceBuilder
participant SvcDef as HttpJsonTranscodingService
GrpcSB->>OptBuilder: new(existingOptions) (copy constructor)
OptBuilder-->>OptBuilder: copy fields (ignoreProtoHttpRule, additionalHttpRules, conflictStrategy, ...)
GrpcSB->>OptBuilder: additionalHttpRules(...) / ignoreProtoHttpRule(flag) / conflictStrategy(...)
GrpcSB->>OptBuilder: build()
OptBuilder->>OptDef: new(ignoreProtoHttpRule, additionalHttpRules, conflictStrategy, matchRules, errorHandler)
OptDef-->>GrpcSB: options
GrpcSB->>SvcBuilder: HttpJsonTranscodingServiceBuilder.of(grpcService, options)
SvcBuilder->>SvcBuilder: scan services/methods, register proto and additional rules
SvcBuilder->>SvcBuilder: resolve conflicts via conflictStrategy
SvcBuilder->>SvcDef: new(delegate, routeAndSpecs, options)
SvcDef-->>GrpcSB: transcoding service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.java (1)
65-65: Consider using UPPER_SNAKE_CASE for the constant name.Java convention for static final constants is to use UPPER_SNAKE_CASE. Consider renaming
testBytesValuetoTEST_BYTES_VALUE.🔎 Proposed fix
- static final String testBytesValue = "abc123!?$*&()'-=@~"; + static final String TEST_BYTES_VALUE = "abc123!?$*&()'-=@~";This would also require updating the reference at line 192:
assertThat(request.getBytesVal().getValue().toStringUtf8()).isEqualTo(TEST_BYTES_VALUE);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultHttpJsonTranscodingOptions.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptions.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilder.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingServiceBuilder.javagrpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingHttpRuleTest.javagrpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTest.javagrpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilderTest.javait/grpc/protobuf4/build.gradle
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingHttpRuleTest.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilderTest.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptions.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingServiceBuilder.javagrpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultHttpJsonTranscodingOptions.javagrpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTest.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilder.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java
🧬 Code graph analysis (3)
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingHttpRuleTest.java (1)
junit5/src/main/java/com/linecorp/armeria/testing/junit5/server/ServerExtension.java (1)
ServerExtension(49-405)
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTest.java (2)
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.java (1)
HttpJsonTranscodingTestService(63-302)grpc/src/test/java/com/linecorp/armeria/it/grpc/TranscodingVerbTest.java (1)
HttpJsonTranscodingTestService(38-69)
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java (1)
grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingServiceBuilder.java (1)
HttpJsonTranscodingServiceBuilder(76-456)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: Kubernetes Chaos test
- GitHub Check: flaky-tests
- GitHub Check: lint
- GitHub Check: site
- GitHub Check: Summary
🔇 Additional comments (18)
it/grpc/protobuf4/build.gradle (1)
30-30: LGTM!The additional include pattern correctly ensures
HttpJsonTranscodingTestService.javais copied alongside the test files for the protobuf4 integration tests.grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptions.java (1)
47-60: LGTM!The new API methods are well-documented with clear descriptions of their purpose and default behavior. Since the interface is already annotated with
@UnstableApi, the new methods don't require additional annotations. Based on coding guidelines.grpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilderTest.java (1)
73-95: LGTM!The test comprehensively validates the copy constructor behavior, ensuring all fields (including the new
useHttpAnnotationsandadditionalHttpRules) are correctly copied from an existing options instance.grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTest.java (1)
19-19: LGTM!Clean refactoring that imports
testBytesValuefrom the extractedHttpJsonTranscodingTestServiceclass, enabling reuse of the test service across multiple test classes.grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcServiceBuilder.java (1)
1073-1080: LGTM!The refactoring correctly uses the new builder-based construction:
HttpJsonTranscodingOptionsBuilder(this.httpJsonTranscodingOptions)properly copies all existing options before applying the error handler override.HttpJsonTranscodingServiceBuilder.of(...).build()aligns with the new service construction pathway.This maintains backward compatibility while enabling the new programmatic HTTP rules functionality.
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingHttpRuleTest.java (1)
36-111: LGTM!Excellent test coverage for the new HTTP/JSON transcoding rules functionality:
serverMixedRulesvalidates coexistence of annotation-based and programmatic rulesserverOnlyAdditionalRulesvalidates that annotation-based rules can be disabled- All three test methods clearly verify the expected behavior with appropriate assertions
grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingServiceBuilder.java (3)
391-410: Consider clarifying theordervariable scope.The
ordervariable is local to thehttpRulemethod, so each call tohttpRule()resetsorderto 0. This means differentHttpRuleinstances will have overlapping order values. Iforderis intended to provide a unique ordering across all specs, consider making it an instance field.However, if the current behavior is intentional (i.e., order is only meaningful within the context of a single
HttpRuleand its additional bindings), this is fine as-is.
76-131: LGTM!The builder is well-structured with a clean factory method that:
- Collects gRPC methods from the delegate service
- Conditionally processes
google.api.httpannotations based onuseHttpAnnotations()- Processes additional HTTP rules from
additionalHttpRules()The class being package-private is appropriate for internal implementation details.
435-455: Good optimization onRecursiveTypeException.Overriding
fillInStackTrace()to returnthisis a good performance optimization since this exception is used for control flow during field building rather than error reporting.grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultHttpJsonTranscodingOptions.java (2)
30-53: LGTM!The new fields and accessors are implemented correctly:
- Fields are properly initialized in the constructor
- Accessor methods correctly implement the interface contract
- The implementation follows the existing patterns in the class
74-82: LGTM!The
equals()andhashCode()implementations correctly include the newuseHttpAnnotationsandadditionalHttpRulesfields, maintaining the contract between the two methods.grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.java (1)
63-302: LGTM!The test service implementation is well-structured with comprehensive coverage of various transcoding scenarios. All RPC methods follow the standard gRPC asynchronous pattern correctly.
grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilder.java (3)
51-70: LGTM! Well-designed copy constructor and field additions.The copy constructor properly transfers all configuration from an existing
HttpJsonTranscodingOptions, and the fields are appropriately initialized. This enables theGrpcServiceBuilderto modify options while preserving existing settings.
100-130: LGTM! Clean API for programmatic HTTP rule configuration.The new methods follow the existing builder patterns in this class:
useHttpAnnotations(boolean)provides control over annotation-based rule extractionadditionalHttpRules(...)overloads handle both varargs andIterableinputs consistentlyThe Javadoc clearly explains that additional rules are processed regardless of the
useHttpAnnotations()setting.
146-165: LGTM!The
build()method correctly integrates the newuseHttpAnnotationsandadditionalHttpRulesparameters into theDefaultHttpJsonTranscodingOptionsconstructor.grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java (3)
195-218: LGTM! Appropriate visibility change for builder pattern.The constructor visibility change from private to package-private aligns with the refactoring to use
HttpJsonTranscodingServiceBuilderfor instantiation. The constructor logic remains unchanged and properly initializes the routes with correct ordering.
407-408: LGTM!Adding the
@Nullableannotation correctly documents thatrequest.contentType()may return null, improving null-safety documentation.
579-631: LGTM! Visibility changes support the builder pattern refactoring.The package-private visibility for
TranscodingSpecandFieldconstructors enablesHttpJsonTranscodingServiceBuilderto construct these objects while keeping them internal to the package.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6567 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| matchRule, queryParamMatchRules); | ||
| }).collect(toImmutableList()); | ||
|
|
||
| if (routeAndSpecs.containsKey(route)) { |
There was a problem hiding this comment.
If there are duplicate routes, would it be more flexible to let users choose one instead of simply rejecting the later one? Because some users may want to override the predefined HttpRule in proto files.
There was a problem hiding this comment.
I agree with this - it may be a good idea to collect/merge all of the HttpRule#selector -> HttpRule mappings before starting to create the GrpcService.
I'm also unsure if the useHttpAnnotations knob will be needed if overriding is possible.
There was a problem hiding this comment.
Great.
We can add a selector that chooses between the option rule and the additional rule:
builder.httpRuleSelector((selector, option, additional) -> { // called when a conflicting selector is found
return additional;
});Alternatively, we could introduce a simple enum such as FIRST_WINS or LAST_WINS.
The original intention of useHttpAnnotations was to give users full control over the proto definition in cases where they do not maintain the proto and do not want to accidentally expose unexpected endpoints. The main purpose, therefore, was to clear HTTP rules parsed from the proto.
There was a problem hiding this comment.
We can add a selector that chooses between the option rule and the additional rule:
A dynamic selector sounds better to me.
The main purpose, therefore, was to clear HTTP rules parsed from the proto.
If so, I don't think useHttpAnnotations() clearly conveys its intent. Would it make sense to rename it to ignoreProtoHttpRule?
There was a problem hiding this comment.
Thank you for the ideas.
I have added HttpJsonTranscodingConflictStrategy to resolve conflicts when multiple HttpRules are registered for the same method selector.
Since HttpRules are converted into Routes, it is theoretically possible for different gRPC methods to be configured to use the same Route. Because standard conflicts are now handled by the selector-based resolution, I considered these route-level collisions (different gRPC methods, same path) to be abnormal, so they will now raise an exception. I am unsure whether this becomes a breaking change or a bug fix.
The default conflict strategy is currently first-wins to maintain backward compatibility. However, I noticed that google/api/http.proto states: "All service configuration rules follow 'last one wins' order." Therefore, I suggest changing the default strategy to last-wins when Armeria reaches version 2.0.
| final HttpJsonTranscodingServiceBuilder builder = new HttpJsonTranscodingServiceBuilder(delegate, | ||
| methods, | ||
| options); | ||
| if (options.useHttpAnnotations()) { |
There was a problem hiding this comment.
My understanding is that useHttpAnnotations(false) completely ignores HttpRule in proto files but instead only uses additionalHttpRules(). Could you add a description of when useHttpAnnotations(false) is useful in the Javadoc?
| public Map<String, GrpcMethod> methods() { | ||
| return methods; | ||
| } |
There was a problem hiding this comment.
nit; it seems like this can be removed
| public Map<String, GrpcMethod> methods() { | |
| return methods; | |
| } | |
| public Map<String, GrpcMethod> methods() { | |
| return methods; | |
| } |
| matchRule, queryParamMatchRules); | ||
| }).collect(toImmutableList()); | ||
|
|
||
| if (routeAndSpecs.containsKey(route)) { |
There was a problem hiding this comment.
I agree with this - it may be a good idea to collect/merge all of the HttpRule#selector -> HttpRule mappings before starting to create the GrpcService.
I'm also unsure if the useHttpAnnotations knob will be needed if overriding is possible.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.java (1)
65-65: Consider makingtestBytesValueprivate.The constant is only used within this class (line 192), so it could be declared
privatefor better encapsulation.♻️ Suggested improvement
- static final String testBytesValue = "abc123!?$*&()'-=@~"; + private static final String testBytesValue = "abc123!?$*&()'-=@~";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultHttpJsonTranscodingOptions.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingConflictStrategy.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptions.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilder.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingServiceBuilder.javagrpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingHttpRuleTest.javagrpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingConflictStrategyTest.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilderTest.javagrpc/src/test/proto/testing/grpc/transcoding.protoit/grpc/protobuf4/build.gradle
🚧 Files skipped from review as they are similar to previous changes (2)
- it/grpc/protobuf4/build.gradle
- grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingHttpRuleTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingConflictStrategy.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingServiceBuilder.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptions.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingConflictStrategyTest.javagrpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilderTest.javagrpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultHttpJsonTranscodingOptions.javagrpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilder.java
🧬 Code graph analysis (1)
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.java (1)
grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTest.java (1)
HttpJsonTranscodingTestService(120-359)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: flaky-tests
- GitHub Check: lint
- GitHub Check: site
- GitHub Check: Kubernetes Chaos test
- GitHub Check: Summary
🔇 Additional comments (15)
grpc/src/test/proto/testing/grpc/transcoding.proto (1)
252-260: LGTM! Test RPC method for additional bindings.The new
AdditionalBindingRPC method with multiple HTTP bindings (POST and PUT) provides good test coverage for the additional HTTP rules and conflict resolution features introduced in this PR.grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTestService.java (1)
67-301: LGTM! Comprehensive test service implementation.The test service provides thorough coverage of various transcoding scenarios including path parameters, query parameters, request/response body mappings, wrapper types, and special protobuf types (Timestamp, Duration, FieldMask, Struct, Any, etc.).
grpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilderTest.java (1)
73-95: LGTM! Comprehensive test for copy constructor.The test thoroughly verifies that the
HttpJsonTranscodingOptionsBuildercopy constructor correctly preserves all fields including the newly addedignoreProtoHttpRule,additionalHttpRules, and the existingqueryParamMatchRulesanderrorHandler.grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptions.java (1)
47-87: LGTM! Well-designed API extension with excellent documentation.The new methods provide flexible configuration for HTTP/JSON transcoding:
ignoreProtoHttpRule()enables strict control over exposed endpointsadditionalHttpRules()supports programmatic route configurationconflictStrategy()handles rule conflicts predictablyThe javadoc clearly explains the behavior, use cases, and important limitations (e.g., route conflicts between different methods are not resolved by the conflict strategy).
grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingConflictStrategy.java (1)
23-60: LGTM! Clean and well-documented strategy interface.The functional interface provides a clear extension point for conflict resolution with sensible built-in strategies (
firstWins()as default,lastWins()as alternative). The javadoc effectively distinguishes between selector-level conflicts (handled by this strategy) and route-level conflicts (which remain errors).grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingServiceBuilder.java (4)
75-130: Well-structured builder initialization.The factory method correctly handles both proto annotation-based rules and programmatic additional rules. The conditional extraction of proto HTTP rules based on
ignoreProtoHttpRule()provides the flexibility described in the PR objectives.
369-388: Excellent defensive validation for conflict resolution.The check at lines 382-386 ensures that custom conflict strategies cannot return arbitrary
HttpRuleinstances, which prevents potential security or correctness issues. This is a well-designed safeguard.
335-348: Clear duplicate route detection.The validation prevents ambiguous route mappings with an informative error message. This will help developers quickly identify configuration issues.
152-250: Robust recursive type handling.The field mapping logic correctly handles:
- Multiple match rule strategies (ORIGINAL_FIELD, LOWER_CAMEL_CASE, JSON_NAME)
- Recursive message types via
RecursiveTypeExceptioncontrol flow- Field name conflicts via
buildKeepingLast()The implementation is thorough and well-commented.
grpc/src/test/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingConflictStrategyTest.java (1)
1-172: Comprehensive test coverage for conflict resolution.The test suite effectively validates:
- Both
firstWins()andlastWins()strategies work as expected- Additional bindings are properly removed when a primary rule is replaced
- Cross-method route conflicts are detected and rejected
- Invalid strategies that return unexpected rules are caught
This provides strong confidence in the conflict resolution implementation.
grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingService.java (1)
195-197: Appropriate visibility adjustments for builder pattern.The visibility changes from
privateto package-private are well-justified:
- Enables
HttpJsonTranscodingServiceBuilderto construct instances- Maintains encapsulation by limiting visibility to the package
- Uses
@VisibleForTestingappropriately for test-only accessorsThese changes align well with the builder-based refactoring.
Also applies to: 272-275, 597-621, 633-641
grpc/src/main/java/com/linecorp/armeria/server/grpc/DefaultHttpJsonTranscodingOptions.java (1)
30-61: Complete and correct implementation of new fields.All new fields (
ignoreProtoHttpRule,additionalHttpRules,conflictStrategy) are properly:
- Initialized in the constructor
- Exposed via accessor methods
- Included in
equals,hashCode, andtoStringThe immutable options implementation is consistent and well-structured.
Also applies to: 82-100
grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingOptionsBuilder.java (3)
65-74: Well-designed copy constructor.The copy constructor enables creating a builder from existing
HttpJsonTranscodingOptions, which is useful for modifying configurations. The implementation correctly delegates to builder methods, maintaining consistency.
104-164: Excellent documentation and API design.The new builder methods are well-documented with:
- Clear explanations of each option's purpose and behavior
- Use case examples (e.g., allowlist approach, third-party proto files)
- Interaction details between options
The API design is intuitive with both varargs and
Iterableoverloads foradditionalHttpRules.
180-200: Build method correctly integrates new fields.The updated
build()method properly collects all new configuration options and passes them toDefaultHttpJsonTranscodingOptions. The implementation maintains backward compatibility while supporting the new features.
ikhoon
left a comment
ikhoon
left a comment
There was a problem hiding this comment.
Overall looks good. Left some comments about the API design and the default behavior.
|
|
||
| private boolean ignoreProtoHttpRule; | ||
|
|
||
| private HttpJsonTranscodingConflictStrategy conflictStrategy = firstWins(); |
There was a problem hiding this comment.
I'm not sure whether firstWins() is a sensible default. Rather than silently ignoring duplicate rules, how about adding strict() method that rejects duplicate rules? I think strict() would be a more reasonable default.
| * This is the default behavior. | ||
| */ | ||
| static HttpJsonTranscodingConflictStrategy firstWins() { | ||
| return (selector, oldRule, newRule) -> oldRule; |
There was a problem hiding this comment.
Would you add a debug message explaining which HttpRule is ignored and why?
| * Returns a strategy that always replaces the existing {@link HttpRule} with the new one. | ||
| */ | ||
| static HttpJsonTranscodingConflictStrategy lastWins() { | ||
| return (selector, oldRule, newRule) -> newRule; |
| * occurred, the {@link HttpRule} that was registered first (oldRule), and the {@link HttpRule} | ||
| * that is being added (newRule). | ||
| */ | ||
| HttpRule resolve(String selector, HttpRule oldRule, HttpRule newRule); |
There was a problem hiding this comment.
What do you think of exposing a gRPC method descriptor instead of selector, so we don't have to explain what it is.
4 participants
Motivation:
HTTP/JSON transcoding for gRPC services in Armeria currently relies solely on
google.api.httpannotations in proto files. This requires modifying proto files to configure HTTP endpoints, which may not be desirable or possible in some scenarios (e.g., using third-party proto definitions or managing transcoding rules separately from service definitions).Modifications:
useHttpAnnotations()toHttpJsonTranscodingOptionsto control whether HTTP/JSON transcoding rules are extracted fromgoogle.api.httpannotations in proto descriptors. Defaults totrueto maintain backward compatibility.additionalHttpRules()toHttpJsonTranscodingOptionsto allow programmatic specification ofHttpRules without modifying proto files. These rules are processed regardless of theuseHttpAnnotations()setting, allowing annotation-based and programmatic rules to coexist.HttpJsonTranscodingServiceintoHttpJsonTranscodingServiceBuilder.Result: