Fix a bug where a request has failed to be routed when both a '/path' and a '/{var}' path mappings exist with different HTTP methods #3340

hyangtack · 2021-02-09T14:57:50Z

Motivation:
Currently RoutingTrie can find a route only based on the request path. But there's lots of conditions
used to resolve a correct path such as HTTP methods, content-type header, accept header, and even
a certain HTTP header and/or parameter.
So we need to check whether a route is acceptable for the request or not earlier than now,
in order to avoid picking up wrong routes.

Modifications:

Introduce NodeProcessor<?> and pass a node into it when the node is selected as a candidate.
Create a RouteCandidateCollectingNodeProcessor and pass it to RoutingTrie#find method when finding
ServiceConfig instances from RoutingTrie.
- It holds Routed instances resolved while visiting the trie.
- If a node is not acceptable for the request, it would be dropped.
Create a new fallback route if the original route does not support all HTTP methods.
The fallback ServiceConfig would be served on the fallback route.
Use isFallback value defined in a DefaultRoute in order to avoid propagating
an exception raised while resolving a route to fallback services.
- For example, assume that a route /a/b/c/ supports HTTP GET method only. Its fallback route would
  be added like /a/b/c with supporting HTTP GET method as well.
  In this case, 404 not found would be better other than 405 method not allowed if a request is
  coming like DELETE /a/b/c, because the fallback route wasn't specified by a user.

Result:

Closes 405 Method Not Allowed if defines both exact path and param path #3330

codecov · 2021-02-20T14:47:59Z

Codecov Report

Merging #3340 (61fc71a) into master (9fa89e3) will decrease coverage by 0.00%.
The diff coverage is 77.31%.

❗ Current head 61fc71a differs from pull request most recent head 0aa427a. Consider uploading reports for the commit 0aa427a to get more accurate results

@@             Coverage Diff              @@
##             master    #3340      +/-   ##
============================================
- Coverage     74.31%   74.31%   -0.01%     
- Complexity    13527    13571      +44     
============================================
  Files          1174     1178       +4     
  Lines         51703    51916     +213     
  Branches       6631     6666      +35     
============================================
+ Hits          38422    38580     +158     
- Misses         9915     9958      +43     
- Partials       3366     3378      +12

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/com/linecorp/armeria/server/Route.java	`66.66% <ø> (ø)`	`2.00 <0.00> (ø)`
...internal/client/grpc/GrpcWebTrailersExtractor.java	`70.00% <ø> (-1.63%)`	`3.00 <0.00> (ø)`
...p/armeria/server/encoding/HttpEncodedResponse.java	`65.33% <16.66%> (-3.24%)`	`17.00 <2.00> (ø)`
...rp/armeria/server/encoding/HttpDecodedRequest.java	`44.00% <40.00%> (-6.00%)`	`5.00 <3.00> (+2.00)`	⬇️
...com/linecorp/armeria/scala/ExecutionContexts.scala	`50.00% <50.00%> (ø)`	`0.00 <0.00> (?)`
...ain/scala/com/linecorp/armeria/scala/package.scala	`50.00% <50.00%> (ø)`	`0.00 <0.00> (?)`
...com/linecorp/armeria/scala/ServerConversions.scala	`57.14% <57.14%> (ø)`	`3.00 <3.00> (?)`
...com/linecorp/armeria/scala/CommonConversions.scala	`60.00% <60.00%> (ø)`	`7.00 <7.00> (?)`
...er/protobuf/ProtobufResponseConverterFunction.java	`70.83% <66.66%> (-2.86%)`	`24.00 <4.00> (+6.00)`	⬇️
...m/linecorp/armeria/server/thrift/THttpService.java	`72.42% <67.64%> (-1.40%)`	`54.00 <11.00> (+2.00)`	⬇️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fa89e3...0aa427a. Read the comment docs.

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

hyangtack · 2021-03-10T14:57:22Z

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

@@ -61,9 +61,6 @@ protected void configure(ServerBuilder sb) throws Exception {
            // returning `202 Accepted`.
            sb.service("/f/", service);
            sb.routeDecorator().pathPrefix("/f/").build((delegate, ctx, req) -> HttpResponse.of(202));
-
-            // This should never be invoked in this test.
-            sb.serviceUnder("/", service);


@trustin I removed this line because of the following test:

// `GET /e` should be redirected to `/e/`. // `DELETE /e` should NOT be redirected to `/e/`. sb.route().get("/e/").build(service);

Because my fix made /e/'s fallback route support HTTP GET only. So the request would be routed the '/' service if this line exists. Please let me know if I'm wrong. 😄

hyangtack · 2021-03-16T03:00:02Z

@trustin @ikhoon @minwoox Gentle ping 😄

ikhoon · 2021-03-16T03:01:16Z

Sorry. 😭 I will review this today! 🔥

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

minwoox · 2021-03-16T04:47:47Z

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

+    /**
+     * Returns the {@link PathMapping} of this {@link Route}.
+     */
+    PathMapping pathMapping();


PathMapping is package-private so we need to make it public.

Removed this method and fixed the following lines:
https://github.com/line/armeria/pull/3340/files#diff-d94019f9ef8458d0b4a04a1be6e2a6ebaf9f85cbab92e2ac6bc6bc42f89f0d2bR81-R96

Is it okay if I use CatchAllPathMapping.INSTANCE directly from there? I'm not sure, but I think the fallback route's PathMapping must be CatchAllPathMapping because /a/'s fallback path is always /a. So I think it would be better to use CatchAllPathMapping.INSTANCE rather than making PathMapping public. Please let me know what you think. 🙇‍♂️

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

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

minwoox · 2021-03-16T05:12:53Z

Sorry for the late review. 😄

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

ikhoon

Mostly looking good.

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

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

trustin · 2021-03-24T11:51:55Z

@hyangtack Sorry, but would you mind merging master to resolve the conflicts? 😅

… and a '/{var}' path mappings exist with different HTTP methods Motivation: Currently `RoutingTrie` can find a route only based on the request path. But there's lots of conditions used to resolve a correct path such as HTTP methods, content-type header, accept header, and even a certain HTTP header and/or parameter. So we need to check whether a route is acceptable for the request or not earlier than now, in order to avoid picking up wrong routes. Modifications: - Introduce `NodeProcessor<?>` and pass a node into it when the node is selected as a candidate. - Create a `ResultCachingNodeProcessor` and pass it to `RoutingTrie#find` method when finding `ServiceConfig` instances from `RoutingTrie`. - It holds `Routed` instances resolved while visiting the trie. - If a node is not acceptable for the request, it would be dropped. - Add `setDeferredExceptionToRoutingContext(boolean)` method to `Route` in order to avoid propagating an exception raised while resolving a route to fallback services. - By default, it sets `true`. It would be set as `false` if the original route does not support all HTTP methods and the fallback route needs to be added. - For example, assume that a route `/a/b/c/` supports HTTP GET method only. Its fallback route would be added like `/a/b/c` with supporting HTTP GET method as well. In this case, `404 not found` would be better other than `405 method not allowed` if a request is coming like `DELETE /a/b/c`, because the fallback route radically does not exist. Result: - Closes line#3330

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

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

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

… instead.

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

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

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

trustin

Awesome job as usual, @hyangtack. 💯

ikhoon

Many thanks for all your hard work! 🙇‍♂️ @hyangtack

trustin · 2021-03-29T05:55:25Z

@minwoox Ping

minwoox

Thanks! just one nit. 😄

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

minwoox

You are the superhero.

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

trustin · 2021-04-05T05:51:40Z

🎉🎉🎉

ikhoon added the defect label Feb 10, 2021

ikhoon added this to the 1.5.0 milestone Feb 10, 2021

hyangtack force-pushed the feature/fix_routing_bug branch from 764134c to 3592fd0 Compare February 14, 2021 11:40

ikhoon modified the milestones: 1.5.0, 1.6.0 Feb 15, 2021

hyangtack force-pushed the feature/fix_routing_bug branch 5 times, most recently from dc244fb to b089f3b Compare February 20, 2021 14:26

trustin reviewed Feb 26, 2021

View reviewed changes

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

hyangtack force-pushed the feature/fix_routing_bug branch 2 times, most recently from e68eca1 to 1f3bd54 Compare March 10, 2021 14:45

hyangtack changed the title ~~(WIP) Fix a bug where a request routing has failed when both a '/path…~~ Fix a bug where a request has failed to be routed when both a '/path' and a '/{var}' path mappings exist with different HTTP methods Mar 10, 2021

hyangtack marked this pull request as ready for review March 10, 2021 14:47

hyangtack requested review from ikhoon and minwoox as code owners March 10, 2021 14:47

hyangtack commented Mar 10, 2021

View reviewed changes

minwoox reviewed Mar 16, 2021

View reviewed changes

ikhoon reviewed Mar 16, 2021

View reviewed changes

ikhoon reviewed Mar 17, 2021

View reviewed changes

hyangtack force-pushed the feature/fix_routing_bug branch from b79201b to 5ab524e Compare March 20, 2021 03:59

hyangtack requested review from trustin, minwoox and ikhoon March 20, 2021 04:00

hyangtack and others added 7 commits March 24, 2021 23:14

Change variable names for readability

0f9d365

Update core/src/main/java/com/linecorp/armeria/server/Routers.java

3552cf4

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

Address comments from @minwoox and @ikhoon

5a51ce6

Add instance cache for newly created fallback routes

f1e7fd4

Cache ServiceConfig instance rather than Route

c57dec9

Copy isFallback value to a new builder when Route.toBuilder() is called

58de162

hyangtack force-pushed the feature/fix_routing_bug branch from 5ab524e to 58de162 Compare March 24, 2021 14:25

hyangtack commented Mar 24, 2021

View reviewed changes

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

hyangtack commented Mar 24, 2021

View reviewed changes

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

hyangtack added 2 commits March 25, 2021 22:15

Remove allowDeferredException from Route/RouteBuilder. Use isFallback…

fb49392

… instead.

Fix fallback route checking condition

a8ebd7d

trustin reviewed Mar 26, 2021

View reviewed changes

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

Address comments from @trustin

0aa427a

trustin approved these changes Mar 27, 2021

View reviewed changes

ikhoon approved these changes Mar 27, 2021

View reviewed changes

minwoox reviewed Mar 29, 2021

View reviewed changes

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

minwoox approved these changes Mar 30, 2021

View reviewed changes

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

trustin merged commit 1e33606 into line:master Apr 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug where a request has failed to be routed when both a '/path' and a '/{var}' path mappings exist with different HTTP methods #3340

Fix a bug where a request has failed to be routed when both a '/path' and a '/{var}' path mappings exist with different HTTP methods #3340

hyangtack commented Feb 9, 2021 •

edited

codecov bot commented Feb 20, 2021 •

edited

hyangtack Mar 10, 2021 •

edited

hyangtack commented Mar 16, 2021

ikhoon commented Mar 16, 2021

minwoox Mar 16, 2021

hyangtack Mar 16, 2021

minwoox commented Mar 16, 2021

ikhoon left a comment

trustin commented Mar 24, 2021

trustin left a comment

ikhoon left a comment

trustin commented Mar 29, 2021

minwoox left a comment

minwoox left a comment

trustin commented Apr 5, 2021

Fix a bug where a request has failed to be routed when both a '/path' and a '/{var}' path mappings exist with different HTTP methods #3340

Fix a bug where a request has failed to be routed when both a '/path' and a '/{var}' path mappings exist with different HTTP methods #3340

Conversation

hyangtack commented Feb 9, 2021 • edited

codecov bot commented Feb 20, 2021 • edited

Codecov Report

hyangtack Mar 10, 2021 • edited

Choose a reason for hiding this comment

hyangtack commented Mar 16, 2021

ikhoon commented Mar 16, 2021

minwoox Mar 16, 2021

Choose a reason for hiding this comment

hyangtack Mar 16, 2021

Choose a reason for hiding this comment

minwoox commented Mar 16, 2021

ikhoon left a comment

Choose a reason for hiding this comment

trustin commented Mar 24, 2021

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

trustin commented Mar 29, 2021

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

trustin commented Apr 5, 2021

hyangtack commented Feb 9, 2021 •

edited

codecov bot commented Feb 20, 2021 •

edited

hyangtack Mar 10, 2021 •

edited