Support colons in path for server-side #5676

jrhee17 · 2024-05-13T16:17:34Z

Motivation:

Currently, gRPC verb suffix is supported by introducing a VerbSuffixPathMapping.
There were several issues with this approach:

VerbSuffixPathMapping was applied inconsistently.
- For instance, RouteBuilder path(String pathPattern) applies VerbSuffixPathMapping, but RouteBuilder path(String prefix, String pathPattern) doesn't.
In the context of ExactPathMapping, VerbSuffixPathMapping was acting as a workaround to support colons in the path. Additionally the support for colons is incomplete since only the last colon is correctly handled.
- Another side-effect of this is route collisions are incorrectly reported since the verb suffix isn't taken into account.
Continued support of VerbSuffixPathMapping makes supporting colons natively difficult.
Misc) A path such ending with an escape character failed to start up the server: /path\\

The original motivation for #5613 was that the following patterns weren't supported.

/path/{param}:verb
/path/literal:verb

I propose that the first case be handled via using REGEX PathMapping types, and the second case be handled by natively supporting colons in our path parameters.
Note that colons are supported per the rfc3986, and we already have an issue #4577.

Supporting /path/{param}:verb

I propose that we support this simply by introducing a new PathMappingType.REGEX. We can simply check if the last PathSegment is a VerbPathSegment. If it is a VerbPathSegment, then we can just use PathMappingType.REGEX.

Supporting /path/literal:verb

Internally, we represent colons as parameterized variables both in ParameterizedPathMapping and RoutingTrieBuilder. This makes it difficult to support colons, so I propose that we modify the internal representation to a different character (\0). This character isn't a valid path character per rfc3986, so I believe the impact is minimal.

One side effect of this approach is that ParameterizedPathMapping#paths will return null character delimited paths.
e.g. /v0/:/path -> /v0/\0/path
Having said this, I believe the normal user path doesn't really use this value so it shouldn't matter.

Supporting colons in general

When one calls RouteBuilder#path(String), we determine whether to use ParameterizedPathMapping or ExactPathMapping depending on whether a colon is used.

armeria/core/src/main/java/com/linecorp/armeria/server/RouteBuilder.java

Line 546 in 8ab4284

if (!pathPattern.contains("{") && !pathPattern.contains(":")) {

If the colon does not start the segment (e.g. /a:b), it is trivial to assume that the colon should be matched exactly.
If the colon does start the segment (e.g. /:param), it is ambiguous whether the user intended this to be a literal or parameter.

For this case, I propose the following:

If a colon is used as-is, /:param, then the segment will be used to represent a parameter
If a colon is escaped, /\\:param, then the segment will be treated as a literal

Optimization) ExactPathMapping is more performant than ParameterizedPathMapping because a simple equality check is done. I propose as an optimization we modify the condition to check if /: is contained instead of :. As a downside of this approach, the colon escape logic needs to also be added to ExactPathMapping. This can be undone if this logic seems too complicated though.

Modifications:

When VerbPathSegment is used, use RegexPathMapping for gRPC transcoding.
Removed VerbSuffixPathMapping and related changes in RoutingTrieBuilder, RouteBuilder, and ParameterizedPathMapping
Modified RoutingTrieBuilder and ParameterizedPathMapping to use \0 instead of ':' to represent parameterized path segments. This allows us to use ':' in most PathMapping types.
Relaxed ParameterizedPathMapping to act like ExactPathMapping when a path segment isn't capturing a path parameter.
Replace /\\: to /: in ParameterizedPathMapping and ExactPathMapping to give users an option to use the first colon as a literal.

Result:

Exact paths with colons cannot be routed to correctly #4577 is closed
Logic related to gRPC verb suffixes is generalized and cleaned up

github-actions · 2024-05-13T17:01:56Z

🔍 Build Scan® (commit: `0fce731`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/tquuh3pkbrug4
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/v644i3mfb3ops
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/b372qn6vmhuy4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/uoe3q4nzpiyva
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/z74tvnupis2nq
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/4vedoij4oauqo
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/cll3xdpqrrpga
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/3jju4a76x5slm

jrhee17 · 2024-05-16T07:37:59Z

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

@@ -74,13 +74,13 @@ service HttpJsonTranscodingTestService {

  rpc EchoTimestampAndDuration(EchoTimestampAndDurationRequest) returns (EchoTimestampAndDurationResponse) {
    option (google.api.http) = {
-      get: "/v1/echo/{timestamp}/{duration}"
+      get: "/v1/echo/timestamp/{timestamp}/{duration}"


These paths can be a source of flakiness since it clashes with the following path:

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

Line 174 in e2d93c2

get: "/v1/echo/response_body/value"

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

Line 181 in e2d93c2

get: "/v1/echo/response_body/repeated"

jrhee17 · 2024-05-16T07:40:11Z

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

    };
  }

  rpc EchoTimestamp(EchoTimestampRequest) returns (EchoTimestampResponse) {
    option (google.api.http) = {
-      post: "/v1/echo/{timestamp}:get",
+      post: "/v1/echo/timestamp/{timestamp}:get",


Ditto

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

Line 83 in 8a94bcc

post: "/v1/echo/{timestamp}:get",

jrhee17 · 2024-05-16T07:46:50Z

grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java

-                    sortedEndpoints.stream().map(httpEndpoint -> httpEndpoint.spec().route().patternString())
+                    sortedEndpoints.stream()
+                                   .map(httpEndpoint -> httpEndpoint.spec().route())
+                                   .filter(route -> route.pathType() != RoutePathType.REGEX)


This change is necessary because GrpcDocServicePluginTest.servicesTest fails.

When the path type is regex, a question mark is included in the path. e.g. /(?<p0>[^/])/hello

However, the regex path is incorrectly cleansed in MethodInfo.

armeria/core/src/main/java/com/linecorp/armeria/server/docs/MethodInfo.java

Line 150 in 8ab4284

final RequestTarget reqTarget = RequestTarget.forServer(path);

Since this automatic behavior is done for gRPC only, I've added a logic to filter out this case.

Can't we create the method info with the examplepath as is?
I'm a bit worried about the situation that all paths that have verb suffixes wouldn't have the example paths. 🤔

I see, I guess I didn't think example paths were too important.
Having said this, I do think it's possible to replace regex patterns with the capturing group name at the cost of extra complexity. Pushed a commit to address this.

minwoox

Looks straightforward! Left small suggestions. 👍

core/src/main/java/com/linecorp/armeria/server/RouteBuilder.java

core/src/main/java/com/linecorp/armeria/server/RoutingTrieBuilder.java

minwoox · 2024-05-17T05:25:17Z

grpc/src/main/java/com/linecorp/armeria/internal/server/grpc/GrpcDocServicePlugin.java

-                    sortedEndpoints.stream().map(httpEndpoint -> httpEndpoint.spec().route().patternString())
+                    sortedEndpoints.stream()
+                                   .map(httpEndpoint -> httpEndpoint.spec().route())
+                                   .filter(route -> route.pathType() != RoutePathType.REGEX)


Can't we create the method info with the examplepath as is?
I'm a bit worried about the situation that all paths that have verb suffixes wouldn't have the example paths. 🤔

minwoox

👍 👍 👍 👍 👍

jrhee17 added 8 commits May 13, 2024 17:31

tmp

9dccdc9

globpath mapping doesn't matter for verb suffix matching

3bf47c5

minimal impl

7cf515f

lint

18aa1f1

working version

1404057

more failing tests

589a32f

revert methodinfo change

09cfdeb

checkstyle

bbbc212

jrhee17 added the improvement label May 13, 2024

jrhee17 added this to the 1.29.0 milestone May 13, 2024

jrhee17 added 2 commits May 14, 2024 10:14

handle flaky test due to duplicate route

e1fad09

refine tests

b2aa86b

jrhee17 mentioned this pull request May 16, 2024

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

Open

jrhee17 added 6 commits May 16, 2024 16:32

param falls back to exact

e2d93c2

cleanup

e1f4760

lint

669a416

fix test

e6a1525

add other path types

b74bff4

allow escaping only the first colon

eb624c4

jrhee17 commented May 16, 2024

View reviewed changes

jrhee17 marked this pull request as ready for review May 16, 2024 09:43

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

jrhee17 changed the title ~~Refactor gRPC verb suffix mapping~~ Support colons in path for server-side May 17, 2024

minwoox reviewed May 17, 2024

View reviewed changes

address comments by @minwoox

50d8dcc

jrhee17 force-pushed the feature/colon-path branch from 766d8c2 to 50d8dcc Compare May 22, 2024 07:33

documentation

4cfb0b6

minwoox approved these changes May 22, 2024

View reviewed changes

Merge branch 'main' into feature/colon-path

0fce731

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support colons in path for server-side #5676

Support colons in path for server-side #5676

jrhee17 commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

jrhee17 May 16, 2024

jrhee17 May 16, 2024

jrhee17 May 16, 2024

minwoox May 17, 2024

jrhee17 May 22, 2024

minwoox left a comment

minwoox May 17, 2024

minwoox left a comment

Support colons in path for server-side #5676

Are you sure you want to change the base?

Support colons in path for server-side #5676

Conversation

jrhee17 commented May 13, 2024 • edited

github-actions bot commented May 13, 2024 • edited

🔍 Build Scan® (commit: 0fce731)

jrhee17 May 16, 2024

Choose a reason for hiding this comment

jrhee17 May 16, 2024

Choose a reason for hiding this comment

jrhee17 May 16, 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 left a comment

Choose a reason for hiding this comment

minwoox May 17, 2024

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

🔍 Build Scan® (commit: `0fce731`)