Fix a bug when gRPC transcoding is used with parameterized paths #5679

jrhee17 · 2024-05-16T02:46:20Z

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.

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

Line 385 in 1737482

return 'p' + StringUtil.toString(pathVarIndex);

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.

github-actions · 2024-05-16T02:58:50Z

🔍 Build Scan® (commit: `75323fc`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/6ck2skuc56zio
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/jkorgw2zsnvzc
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/ydb3se7735piy
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/swpskvezcmjng
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/e2xpic6fw7fpg
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/yyello6wnxlde
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/7zs5fcst2soyu
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/h4ryrwnsc3y54

minwoox · 2024-05-16T07:25:29Z

Could you check the failure? 🤔
https://github.com/line/armeria/actions/runs/9105745527/job/25031842664?pr=5679#step:11:1667

minwoox · 2024-05-17T05:30:11Z

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

@@ -501,7 +501,8 @@ private HttpJsonTranscodingService(GrpcService delegate,
    }

    @Override
-    public HttpEndpointSpecification httpEndpointSpecification(Route route) {
+    public HttpEndpointSpecification
+    httpEndpointSpecification(Route route) {


minwoox · 2024-05-17T05:36:55Z

grpc/src/test/java/com/linecorp/armeria/it/grpc/HttpJsonTranscodingTest.java

@@ -174,7 +175,7 @@ public void updateMessageV1(UpdateMessageRequestV1 request, StreamObserver<Messa
        @Override
        public void updateMessageV2(Message request, StreamObserver<Message> responseObserver) {
            final ServiceRequestContext ctx = ServiceRequestContext.current();
-            final String messageId = Optional.ofNullable(ctx.pathParam("message_id")).orElse("no_id");
+            final String messageId = Optional.ofNullable(ctx.pathParam("p0")).orElse("no_id");


It seems like a breaking behavior.
What do you think of other approaches? e.g. prohibiting p0...pn as a parameter name or using different parameter names that a user wouldn't use(Still need a validation though).

Good point. I've updated so that we prepend a '@' in front of generated path parameters. Let me know if this doesn't make sense.

That's a good idea. I like it. 👍

This reverts commit 7ec8469.

This reverts commit 3a6c4f6.

This reverts commit 1643c08.

This reverts commit fd378f6.

minwoox

Thanks a lot, @jrhee17 👍 👍 👍

minwoox

Thanks a lot, @jrhee17 👍 👍 👍

ikhoon

Looks good!

minwoox · 2024-06-04T00:56:30Z

@jrhee17 👍 👍 👍

…rancoding (#5729) Motivation: PR #5679 allows groups name in a regex path pattern to start with `@`. However, group names of a regular expression must start with a letter. https://docs.oracle.com/javase/7/docs/api/java/util/regex/Pattern.html#groupname Modifications: - Use `p\d` as a group name for `PathMappingType.REGEX` - `@p\d` will stay unchanged for `PathMappingType.PARAMETERIZED` Result: Fix a regression caused by #5679

…e#5679) Motivation: This bug was found while analyzing the code for line#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.

…rancoding (line#5729) Motivation: PR line#5679 allows groups name in a regex path pattern to start with `@`. However, group names of a regular expression must start with a letter. https://docs.oracle.com/javase/7/docs/api/java/util/regex/Pattern.html#groupname Modifications: - Use `p\d` as a group name for `PathMappingType.REGEX` - `@p\d` will stay unchanged for `PathMappingType.PARAMETERIZED` Result: Fix a regression caused by line#5679

minimal fix

fd378f6

jrhee17 added the defect label May 16, 2024

jrhee17 requested review from ikhoon, minwoox and trustin as code owners May 16, 2024 02:46

jrhee17 added this to the 1.29.0 milestone May 16, 2024

lint

1643c08

jrhee17 added 2 commits May 16, 2024 22:20

fix failing tests

3a6c4f6

more failing tests

7ec8469

minwoox reviewed May 17, 2024

View reviewed changes

jrhee17 added 6 commits May 22, 2024 16:47

Revert "more failing tests"

f5ae2c8

This reverts commit 7ec8469.

Revert "fix failing tests"

66693a9

This reverts commit 3a6c4f6.

Revert "lint"

16b93bf

This reverts commit 1643c08.

Revert "minimal fix"

85e431f

This reverts commit fd378f6.

use a different prefix

e0203e5

address comments by @minwoox

75323fc

jrhee17 added the breaking change label May 22, 2024

minwoox approved these changes May 22, 2024

View reviewed changes

ikhoon approved these changes Jun 3, 2024

View reviewed changes

minwoox merged commit baa6829 into line:main Jun 4, 2024
15 checks passed

ikhoon mentioned this pull request Jun 4, 2024

Fix a regession where an invalid path pattern is generated for gRPC trancoding #5729

Merged

ikhoon added a commit to ikhoon/armeria that referenced this pull request Jun 5, 2024

revert test code added by line#5679

5b236a4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug when gRPC transcoding is used with parameterized paths #5679

Fix a bug when gRPC transcoding is used with parameterized paths #5679

jrhee17 commented May 16, 2024 •

edited

github-actions bot commented May 16, 2024 •

edited

minwoox commented May 16, 2024

minwoox May 17, 2024

minwoox May 17, 2024

jrhee17 May 22, 2024

minwoox May 22, 2024

minwoox left a comment

minwoox left a comment

ikhoon left a comment

minwoox commented Jun 4, 2024

Fix a bug when gRPC transcoding is used with parameterized paths #5679

Fix a bug when gRPC transcoding is used with parameterized paths #5679

Conversation

jrhee17 commented May 16, 2024 • edited

github-actions bot commented May 16, 2024 • edited

🔍 Build Scan® (commit: 75323fc)

minwoox commented May 16, 2024

minwoox May 17, 2024

Choose a reason for hiding this comment

minwoox May 17, 2024

Choose a reason for hiding this comment

jrhee17 May 22, 2024

Choose a reason for hiding this comment

minwoox May 22, 2024

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Jun 4, 2024

jrhee17 commented May 16, 2024 •

edited

github-actions bot commented May 16, 2024 •

edited

🔍 Build Scan® (commit: `75323fc`)