Support verb suffix for HTTP endpoint #5613

sato9818 · 2024-04-16T10:21:00Z

Motivation:

We didn't implement supporting "verb" part from #3786, for example,
customVerb from https://service.name/v1/some/resource/name:customVerb.

: is a control character of a parameter variable in RoutingTrie,
so there's no way to specify a plain : character on a service path.

This changeset if a follow-up of #4064, originally written by @hyangtack.

Modifications:

Add VerbSuffixPathMapping which checks whether the request path has a verb or not.
Fix HttpJsonTranscodingPathParser.Stringifier to produce a verb with a leading : character.
Make RoutingTrie handle a escape character \.
Add an explanation about the verb suffix in Armeria documentation.

Result:

A user can specify a verb on HTTP endpoint.

Co-authored-by: Hyangtack Lee [email protected]

github-actions · 2024-04-16T11:47:44Z

🔍 Build Scan® (commit: `70f9b28`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/i34l6esrn5nla
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/mckr4in7bsd5c
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/clysoim5p3obu
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/zwjr7rqsmjn66
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/bdl2emfad3k5a
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/b4scxzt4fpv4g
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/ldniddwxmjpmo
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/j7qq2jpuc4kns

trustin · 2024-04-19T06:39:12Z

/cc @hyangtack

trustin

Nice follow-up work, @sato9818! Left some minor comments 🙇

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

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

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

trustin · 2024-04-19T06:53:35Z

core/src/test/java/com/linecorp/armeria/server/RouteTest.java

+ // Q: Should this be allowed?
+ route = Route.builder().path("/foo/{bar:biz}").build();
+ assertThat(route.pathType()).isSameAs(RoutePathType.PARAMETERIZED);
+ assertThat(route.paths()).containsExactly("/foo/:", "/foo/:");


Great question! I think we should not allow it or treat it as {bar} because we will introduce some option expressions in the future, e.g. {bar:type=int,range=[0,10]}

core/src/test/java/com/linecorp/armeria/server/RouteTest.java

trustin

Almost there 😄

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

trustin · 2024-04-23T13:48:34Z

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

@@ -418,4 +427,63 @@ Node<V> build() {
 return new Node<>(type, path, children, parameterChild, catchAllChild, values);
 }
 }
+
+ private static class PathReader {


Nice abstraction

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

trustin

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

ikhoon

Overall looks very nice. 😃

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

ikhoon · 2024-05-02T11:55:21Z

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

+ return node;
+ } else {
+ final Node<V> verb = node.children.get(':');
+ final String pathVerbPart = path.substring(lastColon);


Micro optimization) What do you think of implementing a method such as findVerb to find a verb part from path? findVerb will scan path from the last character up to begin to find :.

String verbPart = findVerb(path, begin); if (verbPart == null) { return node; } else { final Node<V> verb = node.children.get(':'); ... }

I think we should use the fineVerb method defined in VerbSuffixPathMapping because the verb found by the method has to be last segment of a path 👍

The regular expression may consume more CPU cycles which is inefficient in finding an exact character.

Thank you for your advice! Implemented the findVerb function.

minwoox

Looks great! Left minor suggestions. 👍

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

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

minwoox · 2024-05-07T05:12:46Z

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

 return withLeadingSlash ? '/' + path : path;
 }

+ private static String toPathString(List<PathSegment> segments, PathMappingType type) {
+ final PathSegment lastSegment = segments.get(segments.size() - 1);
+ if (lastSegment instanceof VerbPathSegment) {


Question: Shouldn't we do some validation here such as checking if the body "*" when POST is used? 🤔

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

Line 67 in 17b64b9

// We don't check 'Verb' part because we don't use it when generating a corresponding path pattern.

https://cloud.google.com/apis/design/custom_methods#http_mapping

Thank you for your suggestion! As you said, I think we need to validate the verb part so I used a regex to make sure the verb part is in valid.
In my opinion, we don't need to validate if the body is "*" when POST is used because some poeple might want to use a body other than * according to this stackoverflow. What do you think of this?

After rereading the spec, I realize that it uses the term 'shall' at the top.

The following guidelines shall be applied when choosing the HTTP mapping: ... - If the HTTP verb used for the custom method allows an HTTP request body (this applies to POST, PUT, PATCH, or a custom HTTP verb), the HTTP configuration of that custom method must use the body: "*" clause and all remaining request message fields shall map to the HTTP request body.

Because they use must in the sentence, I thought we should prohibit the value except "*" but I was wrong. 😓
Let's just remove this line though:

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

Line 67 in 17b64b9

// We don't check 'Verb' part because we don't use it when generating a corresponding path pattern.

minwoox

Looks fantastic. Thanks, @sato9818! 👍 👍 👍

minwoox · 2024-05-07T12:28:03Z

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

 return withLeadingSlash ? '/' + path : path;
 }

+ private static String toPathString(List<PathSegment> segments, PathMappingType type) {
+ final PathSegment lastSegment = segments.get(segments.size() - 1);
+ if (lastSegment instanceof VerbPathSegment) {


After rereading the spec, I realize that it uses the term 'shall' at the top.

The following guidelines shall be applied when choosing the HTTP mapping: ... - If the HTTP verb used for the custom method allows an HTTP request body (this applies to POST, PUT, PATCH, or a custom HTTP verb), the HTTP configuration of that custom method must use the body: "*" clause and all remaining request message fields shall map to the HTTP request body.

Because they use must in the sentence, I thought we should prohibit the value except "*" but I was wrong. 😓
Let's just remove this line though:

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

Line 67 in 17b64b9

// We don't check 'Verb' part because we don't use it when generating a corresponding path pattern.

minwoox

Looks fantastic. Thanks, @sato9818! 👍 👍 👍

minwoox

Looks fantastic. Thanks, @sato9818! 👍 👍 👍

ikhoon

Awesome work! 🚀💯

trustin · 2024-05-11T04:03:32Z

@jrhee17 is currently looking for ways to improve this PR, so please bear with us 😅

jrhee17

Let's get this merged! Thanks @sato9818 ! 🙇 👍 🙇

sato9818 added 3 commits April 15, 2024 18:39

Copy from line#4064

17b64b9

Fix codes and add tests

f1923cc

Add test

a3ebb81

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin April 16, 2024 10:21

sato9818 changed the title ~~Support verb suffix for grpc http endpoint~~ [WIP] Support verb suffix for grpc http endpoint Apr 16, 2024

Format and add VerbSuffixPathMappingTest

ecd938d

sato9818 changed the title ~~[WIP] Support verb suffix for grpc http endpoint~~ Support verb suffix for gRPC HTTP endpoint Apr 17, 2024

sato9818 marked this pull request as ready for review April 17, 2024 01:50

trustin added the new feature label Apr 19, 2024

trustin reviewed Apr 19, 2024

View reviewed changes

sato9818 added 2 commits April 22, 2024 12:59

Address review comments

2242c65

Remove comments in RouteTest

c053144

sato9818 requested a review from trustin April 23, 2024 02:27

trustin reviewed Apr 23, 2024

View reviewed changes

Add colonAndVerb var in VerbSuffixPathMapping

f231a3a

trustin approved these changes Apr 24, 2024

View reviewed changes

sato9818 changed the title ~~Support verb suffix for gRPC HTTP endpoint~~ Support verb suffix for HTTP endpoint Apr 24, 2024

Add an explanation about verb suffix in documentation.

66287f5

trustin added this to the 1.29.0 milestone Apr 30, 2024

trustin mentioned this pull request Apr 30, 2024

Support verb suffix from gRPC HTTP endpoint #4064

Closed

ikhoon reviewed Apr 30, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/RoutingTrie.java Outdated Show resolved Hide resolved

sato9818 added 2 commits May 2, 2024 15:03

Address a review comment and add some test for it

0c547a5

Add additional tests on the annotated service test

1737716

ikhoon reviewed May 2, 2024

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/RoutingTrie.java Show resolved Hide resolved

Fix the condition to determine a path has verb suffix

1559d7b

ikhoon reviewed May 2, 2024

View reviewed changes

minwoox reviewed May 2, 2024

View reviewed changes

sato9818 added 4 commits May 2, 2024 22:16

Use findverb on VerbSuffixPathMapping and modify regex

f6c3dd5

Address review comments

c6cf8c5

Delete check argument

33a6f67

Use moveIndex()

df30459

minwoox reviewed May 7, 2024

View reviewed changes

sato9818 added 3 commits May 7, 2024 18:16

Add a verb validation

f33aeca

Implement fineVerb

8ca3475

Format

4537a82

minwoox approved these changes May 7, 2024

View reviewed changes

Delete a comment

70f9b28

ikhoon approved these changes May 8, 2024

View reviewed changes

jrhee17 approved these changes May 13, 2024

View reviewed changes

jrhee17 merged commit 8a94bcc into line:main May 13, 2024
14 of 15 checks passed

jrhee17 mentioned this pull request May 16, 2024

Support colons in path for server-side #5676

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support verb suffix for HTTP endpoint #5613

Support verb suffix for HTTP endpoint #5613

sato9818 commented Apr 16, 2024 •

edited by trustin

github-actions bot commented Apr 16, 2024 •

edited

trustin commented Apr 19, 2024

trustin left a comment

trustin Apr 19, 2024

trustin left a comment

trustin Apr 23, 2024

trustin left a comment

ikhoon left a comment

ikhoon May 2, 2024

sato9818 May 2, 2024

ikhoon May 7, 2024

sato9818 May 7, 2024

minwoox left a comment

minwoox May 7, 2024

sato9818 May 7, 2024

minwoox May 7, 2024

minwoox left a comment

minwoox May 7, 2024

minwoox left a comment

minwoox left a comment

ikhoon left a comment

trustin commented May 11, 2024

jrhee17 left a comment

Support verb suffix for HTTP endpoint #5613

Support verb suffix for HTTP endpoint #5613

Conversation

sato9818 commented Apr 16, 2024 • edited by trustin

github-actions bot commented Apr 16, 2024 • edited

🔍 Build Scan® (commit: 70f9b28)

trustin commented Apr 19, 2024

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

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

trustin commented May 11, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

sato9818 commented Apr 16, 2024 •

edited by trustin

github-actions bot commented Apr 16, 2024 •

edited

🔍 Build Scan® (commit: `70f9b28`)