-
Notifications
You must be signed in to change notification settings - Fork 899
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix a bug when gRPC transcoding is used with parameterized paths (#5679)
Motivation: This bug was found while analyzing the code for #5676. Our gRPC-transcoding path parser contains logic which uses the parameter name when constructing the mapped path. While doing so, we use an arbitrary name prefixed with a 'p' if we are unable to determine a suitable parameter name. https://github.com/line/armeria/blob/1737482b82255d2ff4728927bdf91631bf1b23e9/grpc/src/main/java/com/linecorp/armeria/server/grpc/HttpJsonTranscodingPathParser.java#L385 This can be problematic since a path may be constructed in a different way from what the user expected. e.g. Assume we use the following path pattern: - path: `/v1/conflict/{id=hello/*}/{p0}/test` The above path will currently be mapped to: - `/v1/conflict/hello/:p0/:p0/test`. However, there is no guarantee that `id` and `p0` should be equal, and hence should be built to the following path: - `/v1/conflict/hello/:p0/:p1/test`. I propose that we prepend a '@' to the generated fields. The reasoning for this is: - Protobuf doesn't allow field names other than letters, digits, underscores in identifiers. - https://protobuf.dev/reference/protobuf/proto3-spec/#identifiers - https://protobuf.dev/reference/protobuf/proto2-spec/#identifiers - '@' is a valid character in a path segment, so it stays the same after `RequestTarget#forServer` is called. This is useful when displaying the paths in `DocService` (called by `MethodInfo`). - https://datatracker.ietf.org/doc/html/rfc3986#section-3.3 As long as the above is satisfied, I think any character is fine. I actually believe any `sub-delims` character can be used, so let me know if any character should be preferred more. > sub-delims = "!" / "$" / "&" / "'" / "(" / ")" / "*" / "+" / "," / ";" / "=" Modifications: - Modified to prepend a '@' character in front of generated fields. Result: - There is no longer a bug when a user uses field `p{\d}` for gRPC transcoding. <!-- Visit this URL to learn more about how to write a pull request description: https://armeria.dev/community/developer-guide#how-to-write-pull-request-description -->
- Loading branch information
Showing
6 changed files
with
37 additions
and
10 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters