Add cut-down route object database #398

mikkokar · 2019-04-12T14:20:50Z

This pull request:

Adds simplistic routing object database. it is almost a stub only at the moment, but it will be gaining more features as we implement application routing in terms of routing objects.
Renames routing related classes for better consistency.
Refactors the pipeline builder code to simplify (fingers crossed) the codebase.

kvosper · 2019-04-12T16:12:58Z

components/api/src/main/java/com/hotels/styx/api/configuration/ObjectStore.java

+
+/**
+ *
+ * A glorified map with extra features, like tagging objects


The comment describes extra features, but actually this interface provides a more restricted feature set than java.util.Map - it only provides equivalents to get and put, but with a non-generic key type.

The interface comment should only describe the interface itself, not its implementations.

kvosper · 2019-04-12T16:15:50Z

components/proxy/src/main/java/com/hotels/styx/BuiltInInterceptors.java

+import static com.hotels.styx.api.configuration.ConfigurationContextResolver.EMPTY_CONFIGURATION_CONTEXT_RESOLVER;
+
+/**
+ * Built in interceptors.


Suggested change

* Built in interceptors.

* Provides a list of interceptors that are required by the Styx HTTP pipeline for core functionality.

kvosper · 2019-04-12T16:16:04Z

components/proxy/src/main/java/com/hotels/styx/BuiltInInterceptors.java

+/**
+ * Built in interceptors.
+ */
+public final class BuiltInInterceptors {


Suggested change

public final class BuiltInInterceptors {

final class BuiltInInterceptors {

kvosper · 2019-04-12T16:18:24Z

components/proxy/src/main/java/com/hotels/styx/StyxPipelineFactory.java

+            Environment environment,
+            Map<String, StyxService> services,
+            List<NamedPlugin> plugins) {
+        this.routeDb = routeDb;


Null checks please.

kvosper · 2019-04-15T09:46:28Z

components/proxy/src/main/java/com/hotels/styx/StyxPipelineFactory.java

+    }
+
+    private HttpHandler configuredPipeline(RoutingObjectFactory routingObjectFactory) {
+        HttpPipelineFactory pipelineBuilder;


Please combine declaration and assignment.

kvosper · 2019-04-15T12:48:42Z

components/proxy/src/main/kotlin/com/hotels/styx/routing/db/StyxObjectStore.kt

+ * Styx Route Database.
+ */
+class StyxObjectStore<T> : ObjectStore<T> {
+    val objects = ConcurrentHashMap<String, Record<T>>();


Suggested change

val objects = ConcurrentHashMap<String, Record<T>>();

private val objects = ConcurrentHashMap<String, Record<T>>();

kvosper · 2019-04-15T12:56:10Z

components/proxy/src/test/kotlin/com/hotels/styx/routing/config/RoutingObjectFactoryTest.kt

+import reactor.core.publisher.Mono
+import java.util.Optional
+
+class RoutingObjectFactoryTest : StringSpec({


Please merge/rebase from change to proxy's pom.xml to mark kotlin test sources appropriately.

kvosper · 2019-04-15T13:05:13Z

components/proxy/src/test/kotlin/com/hotels/styx/routing/config/RoutingObjectFactoryTest.kt

+    every { routeObjectStore.get("aHandler") } returns Optional.of(RoutingObjectRecord("name", setOf(), mockk(), mockHandler))
+
+    "Builds a new handler as per RoutingObjectDefinition" {
+        val routeDef = RoutingObjectDefinition("handler-def", "DelegateHandler", mockk<JsonNode>())


Suggested change

val routeDef = RoutingObjectDefinition("handler-def", "DelegateHandler", mockk<JsonNode>())

val routeDef = RoutingObjectDefinition("handler-def", "DelegateHandler", mockk())

kvosper · 2019-04-15T13:32:26Z

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

-            mapOf())
+
+    val routeObjectStore = mockk<StyxObjectStore<RoutingObjectRecord>>()
+    every { routeObjectStore.get("secureHandler") } returns Optional.of(RoutingObjectRecord("secureHandler", setOf(), mockk(), HttpHandler { _, _ -> Eventual.of(response(OK).header("source", "secure").build()) }))


These two lines beginning with every are quite long (and with quite a variety of things in them). Perhaps they could be arranged differently?

kvosper · 2019-04-15T13:54:18Z

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

-        } returns HttpHandler {_, _ -> Eventual.of(response(OK).build())}
-
-        val router = ConditionRouter.ConfigFactory().build(listOf("config", "config"), builtinsFactory, config)
+            """.trimIndent()))

        verify {
            builtinsFactory.build(listOf("config", "config", "routes", "destination[0]"), any())


The syntax for referring to parents here looks a little confusing to me. Wouldn't it make more sense for the [0] to be attached to the routes rather than the destination, since routes contains an array, whereas the destination is a field within each array item?

Possibly.

However, there is only one possible route attribute in the attribute containment hierarchy. OTOH there are two different destination attributes. The error message simply means "first destination attribute" or "second destination attribute".

By the way I'm investigating to see if the attribute parentage path could be removed altogether.

Either way, a change to this behaviour would be a separate enhancement, as it bit out of scope.

dvlato · 2019-04-18T14:31:50Z

components/proxy/src/main/java/com/hotels/styx/StyxPipelineFactory.java

+    public HttpHandler create(StyxServerComponents config) {
+        boolean requestTracking = environment.configuration().get("requestTracking", Boolean.class).orElse(false);
+
+        List<HttpInterceptor> internalInterceptors = internalStyxInterceptors(environment.styxConfig());


Should we inline this?

dvlato · 2019-04-18T14:39:56Z

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

    private final JsonNode config;

-    public RouteHandlerDefinition(String name, String type, JsonNode config) {
+    public RoutingObjectDefinition(String name, String type, List<String> tags, JsonNode config) {
        this.name = name;


is name nullable? It's initialized to an empty string in the Builder...

dvlato · 2019-04-18T14:41:08Z

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

+    private final boolean requestTracking;
+
+    @VisibleForTesting
+    public RoutingObjectFactory(


Can this be visible at package level/protected? I haven't checked.

It needs to be public because it is used in tests from couple of other packages: c.h.s.routing.config, and c.h.s.routing.handlers.

dvlato · 2019-04-18T15:00:02Z

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

@@ -42,6 +43,8 @@ class BackendServiceProxyTest : StringSpec({
    val baRequest = LiveHttpRequest.get("/ba/x").build()

    val environment = Environment.Builder().build()
+    val context = RoutingContext(environment = environment).get()


Maybe we should specify when we are going to use named parameters (see other methods below that would potentially benefit from them).

Do you mean "specify when we are going to use ..." as in coding guidelines?

Add RouteDatabase. Refactor HTTP pipeline construction code.

- RoutingObjectConfig to RoutingObjectConfiguration - RouteObjectRecord to RoutingObjectRecord

- Introduce HttpHandlerFactory.Context class to simplify factory interfaces

mikkokar requested review from dvlato and kvosper April 12, 2019 14:26

kvosper suggested changes Apr 15, 2019

View reviewed changes

dvlato reviewed Apr 18, 2019

View reviewed changes

mikkokar added 13 commits April 23, 2019 14:54

Rename ConfigFactory objects.

1bd55c3

Rename RouteHandlerNNN -> RoutingObjectNNN

17a19ec

Rename RouteHandlerFactory to RoutingObjectFactory.

4a64fbc

Remove ProxyToBackend routing object.

1fcc9ca

Add RouteDatabase. Refactor HTTP pipeline construction code.

Temp save: RouteDatabase re-development.

69b76c5

Add route object store.

c7c4bcc

Tidy up.

2d9f961

Back off support for tags until needed.

def7ad0

Tidy up.

230116c

Rename classes:

616e6a4

- RoutingObjectConfig to RoutingObjectConfiguration - RouteObjectRecord to RoutingObjectRecord

Refactor routing object loading:

51b9e77

- Introduce HttpHandlerFactory.Context class to simplify factory interfaces

Fix SA failures.

03b4462

Address code review comments.

ff1bf0e

mikkokar force-pushed the refactor-styx-routing2 branch from b4a1b67 to ff1bf0e Compare April 23, 2019 14:01

kvosper approved these changes Apr 24, 2019

View reviewed changes

mikkokar changed the title ~~Add stub route object database~~ Add cut-down route object database Apr 25, 2019

mikkokar merged commit 112c800 into ExpediaGroup:master Apr 25, 2019

mikkokar deleted the refactor-styx-routing2 branch April 25, 2019 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cut-down route object database #398

Add cut-down route object database #398

mikkokar commented Apr 12, 2019

kvosper Apr 12, 2019

kvosper Apr 12, 2019

kvosper Apr 12, 2019

kvosper Apr 12, 2019

kvosper Apr 15, 2019

kvosper Apr 15, 2019

kvosper Apr 15, 2019

kvosper Apr 15, 2019

kvosper Apr 15, 2019

kvosper Apr 15, 2019

mikkokar Apr 23, 2019

dvlato Apr 18, 2019

dvlato Apr 18, 2019

dvlato Apr 18, 2019

mikkokar Apr 23, 2019

dvlato Apr 18, 2019

mikkokar Apr 23, 2019

	* Built in interceptors.
	* Provides a list of interceptors that are required by the Styx HTTP pipeline for core functionality.

	public final class BuiltInInterceptors {
	final class BuiltInInterceptors {

	val objects = ConcurrentHashMap<String, Record<T>>();
	private val objects = ConcurrentHashMap<String, Record<T>>();

	val routeDef = RoutingObjectDefinition("handler-def", "DelegateHandler", mockk<JsonNode>())
	val routeDef = RoutingObjectDefinition("handler-def", "DelegateHandler", mockk())

Add cut-down route object database #398

Add cut-down route object database #398

Conversation

mikkokar commented Apr 12, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment