Adds a path prefix routing object. #412

mikkokar · 2019-05-16T08:43:56Z

Adds a new routing object called "PathPrefixRouter". It performs a routing decision based on longest path prefix.

Example usage:

    type: PathPrefixRouter
    config:
      routes:
        - { prefix: /, destination: root }
        - { prefix: /foo/, destination: foo }

The destination can be also inlined. Like so:

    type: PathPrefixRouter
    config:
      routes:
        - { prefix: /, destination: root }
        - prefix: /styx/
          destination:
            type: StaticResponseHandler
            config:
              status: 200
              content: "Hello, from Styx"

Add routingObjectDef utility method in favour of configBlock.

dvlato · 2019-05-16T10:43:19Z

components/proxy/src/main/java/com/hotels/styx/routing/handlers/PathPrefixRouter.java

+    public Optional<HttpHandler> route(LiveHttpRequest request) {
+        String path = request.path();
+
+        return routes.entrySet().stream()


Why don't we use floorEntry() with the default ordering ? That should return a correct path route.

This code is mimics the routing logic from BackendServicesRouter. It is important to keep both components identical until we deprecate BackendServicesRouter.

It is better to minimise the number of variables while migrating backend services router to the new routing object model.

Once the migration is successfully concluded and tested, we shall then optimise this part as a separate increment. Therefore we should leave this as it is. For now.

dvlato · 2019-05-16T11:11:46Z

components/proxy/src/main/java/com/hotels/styx/routing/handlers/RouteRefLookup.java

+/**
+ * Resolves a routing object reference from route database.
+ */
+public class RouteRefLookup implements Function<RoutingObjectReference, HttpHandler> {


I would consider creating an interface for this instead of implementing Function directly. This is not as much to have a slightly easier to read code but basically a solution to overcome the problem that the IDEs 'Find usages' will find any Function instead of only Function<RoutingObjectReference, HttpHandler>

kvosper · 2019-05-16T12:24:31Z

components/proxy/src/main/java/com/hotels/styx/routing/config/RoutingObjectFactory.java

@@ -44,28 +47,37 @@
 */
 public class RoutingObjectFactory {
    public static final ImmutableMap<String, Schema.FieldType> BUILTIN_HANDLER_SCHEMAS;
-    private static final ImmutableMap<String, HttpHandlerFactory> BUILTIN_HANDLER_FACTORIES;
+    public static final ImmutableMap<String, HttpHandlerFactory> BUILTIN_HANDLER_FACTORIES;
+    public static final Function<RoutingObjectReference, HttpHandler> DEFAULT_REFERENCE_LOOKUP = reference -> (request, ctx) -> Eventual.of(response(NOT_FOUND)


I think we should move the Eventual.of(response(NOT_FOUND) down to a new line.

kvosper · 2019-05-16T13:02:14Z

components/proxy/src/main/java/com/hotels/styx/routing/handlers/PathPrefixRouter.java

+                .map(Map.Entry::getValue);
+    }
+
+    public static class Factory implements HttpHandlerFactory {


We've generally been using the convention that X.Factory builds instances of X, but here PathPrefixRouter seems to be just an internal component of the HttpHandler except that it is public, with the handler just being a lambda.

What is the reason for this divergence?

It is to simplify unit testing.

Otherwise PathPrefixRouter would have to be a HttpHandler with a return type of Eventual<LiveHttpResponse>. This would complicate unit testing.

The Factory for every class in routing.handlers.* returns a routing object as a HttpHandler instance. From the consumer's point of view it doesn't matter if PathPrefixRouter is the underlying implementation, or merely just a part of it. But it being merely an internal component of an HttpHandler simplifies unit tests and the divergence is preferred.

kvosper · 2019-05-16T15:08:40Z

components/proxy/src/test/kotlin/com/hotels/styx/routing/handlers/PathPrefixRouterTest.kt

+
+            handler.handle(HttpRequest.get("/x").build())
+                    .toMono()
+                    ?.block()


Suggested change

?.block()

.block()

toMono is a kotlin method with a non-nullable return type

Ah okay. Good catch. I'll change them.

kvosper · 2019-05-16T15:08:49Z

components/proxy/src/test/kotlin/com/hotels/styx/routing/handlers/PathPrefixRouterTest.kt

+
+            handler.handle(HttpRequest.get("/foo/").build())
+                    .toMono()
+                    ?.block()


Suggested change

?.block()

.block()

toMono is a kotlin method with a non-nullable return type

kvosper · 2019-05-16T15:14:51Z

components/proxy/src/test/kotlin/com/hotels/styx/routing/handlers/PathPrefixRouterTest.kt

+
+            handler.handle(HttpRequest.get("/foo").build())
+                    .toMono()
+                    ?.block()


Suggested change

?.block()

.block()

kvosper · 2019-05-16T15:30:55Z

components/proxy/src/test/kotlin/com/hotels/styx/routing/handlers/RouteRefLookupTest.kt

+        val response = RouteRefLookup(routeDb).apply(RoutingObjectReference("handler1"))
+                .handle(get("/").build())
+                .toMono()
+                ?.block()


Suggested change

?.block()

.block()

kvosper · 2019-05-20T13:43:34Z

components/proxy/src/main/java/com/hotels/styx/routing/handlers/RouteRefLookup.java

+        public HttpHandler apply(RoutingObjectReference route) {
+            return this.routeDatabase.get(route.name())
+                    .map(RoutingObjectRecord::getHandler)
+                    .orElse((x, y) -> Eventual.of(response(NOT_FOUND)


During discussion we noticed that there may be a bug here and in many similar circumstances in which a LiveHttpRequest is not having its body consumed.

Fixed this handler. However I will review the usage of other handlers separately.

kvosper · 2019-05-20T15:53:35Z

components/proxy/src/main/java/com/hotels/styx/routing/handlers/RouteRefLookup.java

@@ -36,21 +36,25 @@
     * A StyxObjectStore based route reference lookup function.
     */
    class RouteDbRefLookup implements RouteRefLookup {


The indentation below here is incorrect on several lines.

This has been fixed.

mikkokar added 4 commits May 16, 2019 11:04

Add PathPrefixRouter.

3088872

Add routingObjectDef utility method in favour of configBlock.

Add path prefix router.

36b6406

Add system test.

19be02f

Tidy up tests.

845bd0e

mikkokar force-pushed the path-prefix-router branch from 26ab88f to 845bd0e Compare May 16, 2019 10:24

mikkokar requested review from kvosper and dvlato May 16, 2019 10:32

dvlato suggested changes May 16, 2019

View reviewed changes

dvlato reviewed May 16, 2019

View reviewed changes

kvosper suggested changes May 16, 2019

View reviewed changes

Address code review comments.

4ede209

dvlato approved these changes May 20, 2019

View reviewed changes

kvosper suggested changes May 20, 2019

View reviewed changes

Consume live request body in RouteRefLookup handler.

2e380f5

kvosper suggested changes May 20, 2019

View reviewed changes

Fix indentation in RouteRefLookup.

47a296a

kvosper approved these changes May 22, 2019

View reviewed changes

mikkokar merged commit d429407 into ExpediaGroup:master May 22, 2019

mikkokar deleted the path-prefix-router branch May 22, 2019 13:29

dvlato pushed a commit that referenced this pull request Jul 3, 2019

Adds a path prefix routing object. (#412)

cf0c326

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a path prefix routing object. #412

Adds a path prefix routing object. #412

mikkokar commented May 16, 2019 •

edited

Loading

dvlato May 16, 2019 •

edited

Loading

mikkokar May 20, 2019

dvlato May 16, 2019 •

edited

Loading

kvosper May 16, 2019

kvosper May 16, 2019

mikkokar May 20, 2019

kvosper May 16, 2019

mikkokar May 20, 2019

kvosper May 16, 2019

mikkokar May 20, 2019

kvosper May 16, 2019

kvosper May 16, 2019

kvosper May 20, 2019

mikkokar May 21, 2019

kvosper May 20, 2019

mikkokar May 21, 2019

Adds a path prefix routing object. #412

Adds a path prefix routing object. #412

Conversation

mikkokar commented May 16, 2019 • edited Loading

dvlato May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvlato May 16, 2019 • edited Loading

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

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

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

mikkokar commented May 16, 2019 •

edited

Loading

dvlato May 16, 2019 •

edited

Loading

dvlato May 16, 2019 •

edited

Loading