Skip to content

Commit

Permalink
Address code review comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@mikkokar
mikkokar committed Jun 14, 2019
1 parent a66af84 commit ae09dbe
Show file tree
Hide file tree
Showing 11 changed files with 37 additions and 38 deletions.
4 changes: 2 additions & 2 deletions components/proxy/src/main/java/com/hotels/styx/admin/handlers/RoutingObjectHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import com.hotels.styx.api.HttpRequest;
import com.hotels.styx.api.HttpResponse;
import com.hotels.styx.api.WebServiceHandler;
import com.hotels.styx.routing.RoutingObjectAdapter;
import com.hotels.styx.routing.RoutingObjectDecorator;
import com.hotels.styx.routing.RoutingObjectRecord;
import com.hotels.styx.routing.config.Builtins;
import com.hotels.styx.routing.config.RoutingObjectFactory;
Expand Down Expand Up @@ -96,7 +96,7 @@ public RoutingObjectHandler(StyxObjectStore<RoutingObjectRecord> routeDatabase,

try {
RoutingObjectDefinition payload = YAML_MAPPER.readValue(body, RoutingObjectDefinition.class);
RoutingObjectAdapter adapter = new RoutingObjectAdapter(Builtins.build(emptyList(), routingObjectFactoryContext, payload));
RoutingObjectDecorator adapter = new RoutingObjectDecorator(Builtins.build(emptyList(), routingObjectFactoryContext, payload));

routeDatabase.insert(name, new RoutingObjectRecord(payload.type(), ImmutableSet.copyOf(payload.tags()), payload.config(), adapter))
.ifPresent(previous -> previous.getRoutingObject().stop());
Expand Down
4 changes: 2 additions & 2 deletions ...ls/styx/routing/RoutingObjectAdapter.java → .../styx/routing/RoutingObjectDecorator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
* Adapts routing object into Styx core.
* Provides a load balancing metric for the adapted routing object.
*/
public class RoutingObjectAdapter implements RoutingObject {
public class RoutingObjectDecorator implements RoutingObject {

private final RoutingObject delegate;

Expand All @@ -42,7 +42,7 @@ public class RoutingObjectAdapter implements RoutingObject {
* Routing object adapater constructor.
* @param routingObject
*/
public RoutingObjectAdapter(RoutingObject routingObject) {
public RoutingObjectDecorator(RoutingObject routingObject) {
this.delegate = requireNonNull(routingObject);
}

Expand Down
2 changes: 1 addition & 1 deletion components/proxy/src/main/java/com/hotels/styx/routing/handlers/HostProxy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ private static Connection.Factory connectionFactory(
.responseTimeoutMillis(responseTimeoutMillis)
.build()
)
.tlsSettings(Optional.ofNullable(tlsSettings).orElse(null))
.tlsSettings(tlsSettings)
.build();

if (connectionExpiration > 0) {
Expand Down
4 changes: 2 additions & 2 deletions components/proxy/src/main/java/com/hotels/styx/startup/StyxServerComponents.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
import com.hotels.styx.client.netty.eventloop.PlatformAwareClientEventLoopGroupFactory;
import com.hotels.styx.infrastructure.configuration.yaml.JsonNodeConfig;
import com.hotels.styx.proxy.plugin.NamedPlugin;
import com.hotels.styx.routing.RoutingObjectAdapter;
import com.hotels.styx.routing.RoutingObjectDecorator;
import com.hotels.styx.routing.RoutingObjectRecord;
import com.hotels.styx.routing.config.Builtins;
import com.hotels.styx.routing.config.RoutingObjectFactory;
Expand Down Expand Up @@ -109,7 +109,7 @@ private StyxServerComponents(Builder builder) {
.map(StyxServerComponents::readHttpHandlers)
.orElse(ImmutableMap.of())
.forEach((name, record) -> {
RoutingObjectAdapter adapter = new RoutingObjectAdapter(Builtins.build(ImmutableList.of(name), routingObjectContext, record));
RoutingObjectDecorator adapter = new RoutingObjectDecorator(Builtins.build(ImmutableList.of(name), routingObjectContext, record));
routeObjectStore.insert(name, new RoutingObjectRecord(record.type(), ImmutableSet.copyOf(record.tags()), record.config(), adapter))
.ifPresent(previous -> previous.getRoutingObject().stop());
});
Expand Down
7 changes: 5 additions & 2 deletions components/proxy/src/main/kotlin/com/hotels/styx/routing/RoutingObjectRecord.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,19 @@ package com.hotels.styx.routing

import com.fasterxml.jackson.databind.JsonNode

/**
* Holds routing object and its associated metadata.
*/
internal data class RoutingObjectRecord(
val type: String,
val tags: Set<String>,
val config: JsonNode,
val routingObject: RoutingObjectAdapter) {
val routingObject: RoutingObjectDecorator) {
companion object {
fun create(type: String, tags: Set<String>, config: JsonNode, routingObject: RoutingObject) = RoutingObjectRecord(
type,
tags,
config,
RoutingObjectAdapter(routingObject))
RoutingObjectDecorator(routingObject))
}
}
8 changes: 4 additions & 4 deletions components/proxy/src/test/kotlin/com/hotels/styx/admin/handlers/RoutingObjectHandlerTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import com.hotels.styx.api.HttpResponseStatus.CREATED
import com.hotels.styx.api.HttpResponseStatus.NOT_FOUND
import com.hotels.styx.api.HttpResponseStatus.OK
import com.hotels.styx.routing.RoutingObjectFactoryContext
import com.hotels.styx.routing.RoutingObjectAdapter
import com.hotels.styx.routing.RoutingObjectDecorator
import com.hotels.styx.routing.RoutingObjectRecord
import com.hotels.styx.routing.db.StyxObjectStore
import com.hotels.styx.routing.handle
Expand Down Expand Up @@ -64,7 +64,7 @@ class RoutingObjectHandlerTest : FeatureSpec({
routeDatabase.get("staticResponse").isPresent shouldBe true
routeDatabase.get("staticResponse").get().type shouldBe "StaticResponseHandler"
routeDatabase.get("staticResponse").get().config.shouldBeTypeOf<ObjectNode>()
routeDatabase.get("staticResponse").get().routingObject.shouldBeTypeOf<RoutingObjectAdapter>()
routeDatabase.get("staticResponse").get().routingObject.shouldBeTypeOf<RoutingObjectDecorator>()
}

scenario("Retrieving objects") {
Expand Down Expand Up @@ -154,7 +154,7 @@ class RoutingObjectHandlerTest : FeatureSpec({

scenario("Replacing existing objects triggers lifecycle methods") {
val db = StyxObjectStore<RoutingObjectRecord>()
val mockObject = RoutingObjectAdapter(mockObject())
val mockObject = RoutingObjectDecorator(mockObject())

db.insert("staticResponse", RoutingObjectRecord("StaticResponseHandler", mockk(), mockk(), mockObject))
db.get("staticResponse").isPresent shouldBe true
Expand Down Expand Up @@ -182,7 +182,7 @@ class RoutingObjectHandlerTest : FeatureSpec({

scenario("Removing existing objects triggers lifecycle methods") {
val db = StyxObjectStore<RoutingObjectRecord>()
val mockObject = RoutingObjectAdapter(mockObject())
val mockObject = RoutingObjectDecorator(mockObject())

db.insert("staticResponse", RoutingObjectRecord("StaticResponseHandler", mockk(), mockk(), mockObject))

Expand Down
6 changes: 3 additions & 3 deletions .../styx/routing/RoutingObjectAdapterTest.kt → ...tyx/routing/RoutingObjectDecoratorTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import io.mockk.verify
import reactor.core.publisher.toMono
import reactor.test.publisher.TestPublisher

class RoutingObjectAdapterTest : FeatureSpec({
class RoutingObjectDecoratorTest : FeatureSpec({
val request = get("/").build()

feature("Maintains load balancing metrics") {
Expand All @@ -40,7 +40,7 @@ class RoutingObjectAdapterTest : FeatureSpec({
.returnsMany(listOf(Eventual(publisher1), Eventual(publisher2), Eventual(publisher3)))
}

RoutingObjectAdapter(delegate)
RoutingObjectDecorator(delegate)
.let {

it.handle(request)
Expand Down Expand Up @@ -73,7 +73,7 @@ class RoutingObjectAdapterTest : FeatureSpec({
feature("Calls close on delegate") {
val delegate = mockk<RoutingObject>(relaxed = true)

RoutingObjectAdapter(delegate).stop()
RoutingObjectDecorator(delegate).stop()

verify { delegate.stop() }
}
Expand Down
4 changes: 2 additions & 2 deletions components/proxy/src/test/kotlin/com/hotels/styx/routing/handlers/HostProxyTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class HostProxyTest : FeatureSpec() {
HostProxy(HostAndPort.fromString("localhost:80"), client, mockk()).handle(request.stream(), mockk())

verify {
client?.sendRequest(ofType(LiveHttpRequest::class))
client!!.sendRequest(ofType(LiveHttpRequest::class))
}
}

Expand All @@ -70,7 +70,7 @@ class HostProxyTest : FeatureSpec() {
exception.message shouldBe ("HostProxy localhost:80 is stopped but received traffic.")

verify(exactly = 0) {
client?.sendRequest(any())
client!!.sendRequest(any())
}
}

Expand Down
26 changes: 11 additions & 15 deletions components/proxy/src/test/kotlin/com/hotels/styx/routing/handlers/LoadBalancingGroupTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,12 @@ class LoadBalancingGroupTest : FeatureSpec() {
}
}

frequencies["appx-01"]?.shouldBeGreaterThan(20)
frequencies["appx-02"]?.shouldBeGreaterThan(20)
frequencies["appx-03"]?.shouldBeGreaterThan(20)
frequencies["appx-01"]!!.shouldBeGreaterThan(20)
frequencies["appx-02"]!!.shouldBeGreaterThan(20)
frequencies["appx-03"]!!.shouldBeGreaterThan(20)

frequencies["appy-01"]?.shouldBeNull()
frequencies["appy-02"]?.shouldBeNull()
frequencies["appy-01"].shouldBeNull()
frequencies["appy-02"].shouldBeNull()
}


Expand Down Expand Up @@ -111,11 +111,9 @@ class LoadBalancingGroupTest : FeatureSpec() {
}
}

println("Frequencies: " + frequencies)

frequencies["appx-04"]?.shouldBeGreaterThan(20)
frequencies["appx-05"]?.shouldBeGreaterThan(20)
frequencies["appx-06"]?.shouldBeGreaterThan(20)
frequencies["appx-04"]!!.shouldBeGreaterThan(20)
frequencies["appx-05"]!!.shouldBeGreaterThan(20)
frequencies["appx-06"]!!.shouldBeGreaterThan(20)
}

scenario("... and detects replaced origins") {
Expand All @@ -136,11 +134,9 @@ class LoadBalancingGroupTest : FeatureSpec() {
}
}

println("Frequencies: " + frequencies)

frequencies["appx-04-a"]?.shouldBeGreaterThan(20)
frequencies["appx-05-b"]?.shouldBeGreaterThan(20)
frequencies["appx-06-c"]?.shouldBeGreaterThan(20)
frequencies["appx-04-a"]!!.shouldBeGreaterThan(20)
frequencies["appx-05-b"]!!.shouldBeGreaterThan(20)
frequencies["appx-06-c"]!!.shouldBeGreaterThan(20)
}

scenario("... and emits NoAvailableHostsException when load balancing group is empty") {
Expand Down
4 changes: 2 additions & 2 deletions components/proxy/src/test/kotlin/com/hotels/styx/routing/handlers/RouteRefLookupTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import com.hotels.styx.api.ByteStream
import com.hotels.styx.api.HttpRequest.get
import com.hotels.styx.api.HttpResponseStatus.NOT_FOUND
import com.hotels.styx.api.LiveHttpRequest
import com.hotels.styx.routing.RoutingObjectAdapter
import com.hotels.styx.routing.RoutingObjectDecorator
import com.hotels.styx.routing.RoutingObjectRecord
import com.hotels.styx.routing.config.RoutingObjectReference
import com.hotels.styx.routing.db.StyxObjectStore
Expand All @@ -39,7 +39,7 @@ import java.util.Optional

class RouteRefLookupTest : StringSpec({
"Retrieves handler from route database" {
val handler = RoutingObjectAdapter(mockk())
val handler = RoutingObjectDecorator(mockk())

val routeDb = mockk<StyxObjectStore<RoutingObjectRecord>>()
every { routeDb.get(any()) } returns Optional.of(RoutingObjectRecord("StaticResponseHandler", mockk(), mockk(), handler))
Expand Down
6 changes: 3 additions & 3 deletions system-tests/ft-suite/src/test/kotlin/com/hotels/styx/routing/LoadBalancingGroupSpec.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,9 @@ class LoadBalancingGroupSpec : FeatureSpec() {
}
}

// scenario("!Routes to new origin when the origin indicated by sticky session cookie is no longer available") {
// TODO("Styx doesn't support this as of today")
// }
scenario("!Routes to new origin when the origin indicated by sticky session cookie is no longer available") {
// See bug: https://github.com/HotelsDotCom/styx/issues/434
}
}

feature("Origins restriction") {
Expand Down

0 comments on commit ae09dbe

Please sign in to comment.