Fix a bug when gRPC transcoding is used with parameterized paths (lin…

…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.

<!--
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
-->

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

@@ -382,7 +382,7 @@ public String pathVariable(PathMappingType type) {
                 if (parentFieldPath != null) {
                     return parentFieldPath;
                 } else {
-                    return 'p' + StringUtil.toString(pathVarIndex);
+                    return "@p" + StringUtil.toString(pathVarIndex);
                 }
             } else {
                 return StringUtil.toString(pathVarIndex);

grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePluginTest.java

@@ -348,7 +348,7 @@ void httpEndpoint() {
 
         // Expected generated routes. See 'transcoding.proto' file.
         final List<Route> routes = ImmutableList.of(
-                Route.builder().methods(HttpMethod.GET).path("/v1/messages/:p0").build(),
+                Route.builder().methods(HttpMethod.GET).path("/v1/messages/:@p0").build(),
                 Route.builder().methods(HttpMethod.GET).path("/v2/messages/{message_id}").build(),
                 Route.builder().methods(HttpMethod.GET).path("/v3/messages/{message_id}").build(),
                 Route.builder().methods(HttpMethod.PATCH).path("/v1/messages/{message_id}").build(),
@@ -379,7 +379,7 @@ void httpEndpoint() {
                                                    .findFirst().get();
         assertThat(getMessageV1.httpMethod()).isEqualTo(HttpMethod.GET);
         assertThat(getMessageV1.endpoints()).containsAll(ImmutableSet.of(
-                EndpointInfo.builder(virtualHostNamePattern, "/v1/messages/:p0")
+                EndpointInfo.builder(virtualHostNamePattern, "/v1/messages/:@p0")
                             .availableMimeTypes(MediaType.JSON_UTF_8).build()));
         assertThat(getMessageV1.parameters()).containsAll(ImmutableList.of(
                 FieldInfo.builder("name", TypeSignature.ofBase(JavaType.STRING.name()))

grpc/src/test/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePrefixTest.java

@@ -78,7 +78,7 @@ void prefixTest() throws Exception {
                 "/route/innerPrefix/GetMessageV1");
         getMessageV1 = findMethod(httpMethods, "GetMessageV1");
         assertThat(pathMapping(getMessageV1)).containsExactlyInAnyOrder(
-                "/v1/messages/:p0", "/outerPrefix/v1/messages/:p0");
+                "/v1/messages/:@p0", "/outerPrefix/v1/messages/:@p0");
 
         JsonNode getMessageV2 = findMethod(methods, "GetMessageV2");
         assertThat(pathMapping(getMessageV2)).containsExactlyInAnyOrder(

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

@@ -76,6 +76,7 @@
 import io.grpc.stub.StreamObserver;
 import testing.grpc.HttpJsonTranscodingTestServiceGrpc.HttpJsonTranscodingTestServiceBlockingStub;
 import testing.grpc.HttpJsonTranscodingTestServiceGrpc.HttpJsonTranscodingTestServiceImplBase;
+import testing.grpc.Transcoding.ConflictMessage;
 import testing.grpc.Transcoding.EchoAnyRequest;
 import testing.grpc.Transcoding.EchoAnyResponse;
 import testing.grpc.Transcoding.EchoFieldMaskRequest;
@@ -333,6 +334,13 @@ public void echoNestedMessageField(EchoNestedMessageRequest request,
                     .onNext(EchoNestedMessageResponse.newBuilder().setNested(request.getNested()).build());
             responseObserver.onCompleted();
         }
+
+        @Override
+        public void paramNameConflict(ConflictMessage request,
+                                      StreamObserver<ConflictMessage> responseObserver) {
+            responseObserver.onNext(request);
+            responseObserver.onCompleted();
+        }
     }
 
     @RegisterExtension
@@ -947,8 +955,8 @@ void shouldBeIntegratedWithDocService() throws JsonProcessingException {
 
         final JsonNode getMessageV1 = findMethod(methods, "GetMessageV1");
         assertThat(getMessageV1.get("httpMethod").asText()).isEqualTo("GET");
-        assertThat(pathMapping(getMessageV1)).containsExactlyInAnyOrder("/v1/messages/:p0",
-                                                                        "/foo/v1/messages/:p0");
+        assertThat(pathMapping(getMessageV1)).containsExactlyInAnyOrder("/v1/messages/:@p0",
+                                                                        "/foo/v1/messages/:@p0");
 
         final JsonNode getMessageV2 = findMethod(methods, "GetMessageV2");
         assertThat(getMessageV2.get("httpMethod").asText()).isEqualTo("GET");
@@ -1079,6 +1087,14 @@ void shouldAcceptNestedMessageTypeFields() throws JsonProcessingException {
         assertThat(nested.get("name").asText()).isEqualTo("Armeria");
     }
 
+    @Test
+    void conflictingParamNamesReturnsCorrectly() {
+        final AggregatedHttpResponse res = server.blockingWebClient().get("/v1/conflict/hello/1/a/test");
+        assertThat(res.status().code()).isEqualTo(200);
+        assertThatJson(res.contentUtf8()).node("id").isEqualTo("hello/1");
+        assertThatJson(res.contentUtf8()).node("p0").isEqualTo("a");
+    }
+
     public static JsonNode findMethod(JsonNode methods, String name) {
         return StreamSupport.stream(methods.spliterator(), false)
                             .filter(node -> node.get("name").asText().equals(name))

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

@@ -89,19 +89,19 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
                               ImmutableMap.of("user_id", "1", "message_id", "2"),
                               ImmutableMap.of("user_id", "1", "message_id", "2")),
                     arguments("/v1/{name=messages/*}",
-                              "/v1/messages/:p0",
+                              "/v1/messages/:@p0",
                               PathMappingType.PARAMETERIZED,
-                              ImmutableMap.of("p0", "1"),
+                              ImmutableMap.of("@p0", "1"),
                               ImmutableMap.of("name", "messages/1")),
                     arguments("/v1/{name=messages/{name2=*}}",
                               "/v1/messages/:name2",
                               PathMappingType.PARAMETERIZED,
                               ImmutableMap.of("name2", "1"),
                               ImmutableMap.of("name", "messages/1", "name2", "1")),
                     arguments("/v1/{name=messages/{name2=*/foo/{name3=*}}}",
-                              "/v1/messages/:p0/foo/:name3",
+                              "/v1/messages/:@p0/foo/:name3",
                               PathMappingType.PARAMETERIZED,
-                              ImmutableMap.of("p0", "1", "name3", "2"),
+                              ImmutableMap.of("@p0", "1", "name3", "2"),
                               ImmutableMap.of("name", "messages/1/foo/2", "name2", "1/foo/2", "name3", "2")),
                     arguments("/v1/messages/{message_id}:verb",
                               "/v1/messages/:message_id:verb",

grpc/src/test/proto/testing/grpc/transcoding.proto

@@ -224,6 +224,12 @@ service HttpJsonTranscodingTestService {
       }
     };
   }
+
+  rpc ParamNameConflict(ConflictMessage) returns (ConflictMessage) {
+    option(google.api.http) = {
+      get: "/v1/conflict/{id=hello/*}/{p0}/test"
+    };
+  }
 }
 
 message GetMessageRequestV1 {
@@ -407,3 +413,8 @@ message EchoNestedMessageRequest {
 message EchoNestedMessageResponse {
   TopLevelMessage.NestedMessage nested = 1;
 }
+
+message ConflictMessage {
+  string p0 = 1;
+  string id = 2;
+}