Skip to content
This repository has been archived by the owner on Jun 12, 2020. It is now read-only.

Add the facility to override the request timeout per include and route. #28

Merged
merged 6 commits into from
Mar 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
*/
public interface ContentFetcher {

CompletableFuture<Response<String>> fetch(String path, String fallback, CompositionStep step);
CompletableFuture<Response<String>> fetch(FetchContext fetchContext, CompositionStep step);

}
50 changes: 50 additions & 0 deletions src/main/java/com/rewedigital/composer/composing/FetchContext.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package com.rewedigital.composer.composing;

import static java.util.Objects.requireNonNull;

import java.time.Duration;
import java.util.Optional;

/**
* A simple parameter object for {@link ContentFetcher}s.
*/
public class FetchContext {

private final String path;
private final String fallback;
private final Optional<Duration> ttl;

private FetchContext(final String path, final String fallback, final Optional<Duration> ttl) {
this.path = path;
this.fallback = fallback;
this.ttl = requireNonNull(ttl);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why requireNonNull for ttl but not for path or fallback? I really start to feel we need to find some consistent rule for these checks...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path and fallback is used as if it is valid to have null in there...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. But than, maybe Optional is better for both. I would like to have a codebase where null is always a mistake ;-)

I'll open an issue to discuss this kind of global code style questions, ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect.

}

/**
* Builds a simple parameter object for {@link ContentFetcher}s.
*
* @param path to fetch from.
* @param fallback the fallback returned in case of an error.
* @param ttl how long the fetch should take.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if ttl is the right name: shouldn't it be timeout or something like that?

Copy link
Contributor Author

@AndreasKl AndreasKl Mar 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss that @ work.

* @return the parameter object.
*/
public static FetchContext of(final String path, final String fallback, final Optional<Duration> ttl) {
return new FetchContext(path, fallback, ttl);
}

public boolean hasEmptyPath() {
return path == null || path().trim().isEmpty();
}

public String path() {
return path;
}

public String fallback() {
return fallback;
}

public Optional<Duration> ttl() {
return ttl;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
import org.attoparser.output.OutputMarkupHandler;
import org.attoparser.util.TextUtil;

import com.google.common.annotations.VisibleForTesting;

class IncludeMarkupHandler extends AbstractMarkupHandler {

private final char[] includeTag;
Expand Down Expand Up @@ -41,6 +43,11 @@ private List<Asset> assets() {
return contentMarkupHandler.assets();
}

@VisibleForTesting
List<IncludedService> includedServices() {
return includedServices;
}

@Override
public void handleOpenElementStart(final char[] buffer, final int nameOffset, final int nameLen, final int line,
final int col)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
package com.rewedigital.composer.composing;

import java.time.Duration;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.annotations.VisibleForTesting;
import com.spotify.apollo.Response;

/**
Expand Down Expand Up @@ -86,24 +89,43 @@ private IncludedService(final Builder builder) {
this.fallback = builder.fallback;
}


public CompletableFuture<IncludedService.WithResponse> fetch(final ContentFetcher fetcher,
final CompositionStep parentStep) {
final CompositionStep step = parentStep.childWith(path());
return fetcher.fetch(path(), fallback(), step)
return fetcher.fetch(FetchContext.of(path(), fallback(), ttl()), step)
.thenApply(r -> new WithResponse(step, startOffset, endOffset, r));
}

private String fallback() {
public boolean isInRage(final ContentRange contentRange) {
return contentRange.isInRange(startOffset);
}

@VisibleForTesting
public String fallback() {
return fallback;
}

private String path() {
@VisibleForTesting
public String path() {
return attributes.getOrDefault("path", "");
}

public boolean isInRage(final ContentRange contentRange) {
return contentRange.isInRange(startOffset);
@VisibleForTesting
public Optional<Duration> ttl() {
return longFromMap("ttl").map(Duration::ofMillis);
}

private Optional<Long> longFromMap(final String name) {
if (!attributes.containsKey(name)) {
return Optional.empty();
}
final String unparsedValue = attributes.get(name);
try {
return Optional.of(Long.parseLong(unparsedValue));
} catch (final NumberFormatException nfEx) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a test showing what happens when the data type is wrong?

LOGGER.info("Not able to evaluate {} for path {} with value {}.", name, path(), unparsedValue);
}
return Optional.empty();
}

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.rewedigital.composer.composing;

import java.util.Objects;
import static java.util.Objects.requireNonNull;

import java.util.concurrent.CompletableFuture;

import org.slf4j.Logger;
Expand All @@ -16,18 +17,17 @@ public class RecursionAwareContentFetcher implements ContentFetcher {
private final int maxRecursion;

public RecursionAwareContentFetcher(final ContentFetcher contentFetcher, final int maxRecursion) {
this.contentFetcher = Objects.requireNonNull(contentFetcher);
this.contentFetcher = requireNonNull(contentFetcher);
this.maxRecursion = maxRecursion;
}

@Override
public CompletableFuture<Response<String>> fetch(final String path, final String fallback,
final CompositionStep step) {
public CompletableFuture<Response<String>> fetch(final FetchContext context, final CompositionStep step) {
if (maxRecursion < step.depth()) {
LOGGER.warn("Max recursion depth execeeded for " + step.callStack());
return CompletableFuture.completedFuture(Response.forPayload(fallback));
LOGGER.warn("Max recursion depth exceeded for " + step.callStack());
return CompletableFuture.completedFuture(Response.forPayload(context.fallback()));
}
return contentFetcher.fetch(path, fallback, step);
return contentFetcher.fetch(context, step);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -26,26 +26,31 @@ public class ValidatingContentFetcher implements ContentFetcher {

public ValidatingContentFetcher(final Client client, final Map<String, Object> parsedPathArguments,
final SessionRoot session) {
this.session = session;
this.session = requireNonNull(session);
this.client = requireNonNull(client);
this.parsedPathArguments = requireNonNull(parsedPathArguments);
}

@Override
public CompletableFuture<Response<String>> fetch(final String path, final String fallback,
final CompositionStep step) {
if (path == null || path.trim().isEmpty()) {
public CompletableFuture<Response<String>> fetch(final FetchContext context, final CompositionStep step) {
if (context.hasEmptyPath()) {
LOGGER.warn("Empty path attribute in include found - callstack: " + step.callStack());
return CompletableFuture.completedFuture(Response.forPayload(""));
}

final String expandedPath = UriTemplate.fromTemplate(path).expand(parsedPathArguments);
return client.send(session.enrich(Request.forUri(expandedPath, "GET")))
final String expandedPath = UriTemplate.fromTemplate(context.path()).expand(parsedPathArguments);
final Request request = session.enrich(withTtl(Request.forUri(expandedPath, "GET"), context));

return client.send(request)
.thenApply(response -> acceptHtmlOnly(response, expandedPath))
.thenApply(r -> toStringPayload(r, fallback))
.thenApply(r -> toStringPayload(r, context.fallback()))
.toCompletableFuture();
}

private Request withTtl(final Request request, final FetchContext context) {
return context.ttl().map(t -> request.withTtl(t)).orElse(request);
}

private Response<String> toStringPayload(final Response<ByteString> response, final String fallback) {
final String value = response.payload().map(p -> p.utf8()).orElse(fallback);
return response.withPayload(value);
Expand Down
34 changes: 26 additions & 8 deletions src/main/java/com/rewedigital/composer/routing/Match.java
Original file line number Diff line number Diff line change
@@ -1,45 +1,63 @@
package com.rewedigital.composer.routing;

import java.time.Duration;
import java.util.Objects;
import java.util.Optional;

public class Match {

private final String backend;
private final Optional<Duration> ttl;
private final RouteTypeName routeType;

private Match(final String backend, final RouteTypeName routeType) {
private Match(final String backend, final Optional<Duration> ttl, final RouteTypeName routeType) {
this.backend = backend;
this.ttl = ttl;
this.routeType = routeType;
}

public static Match of(final String backend, final RouteTypeName routeType) {
return new Match(backend, routeType);
return new Match(backend, Optional.empty(), routeType);
}

public static Match of(final String backend, final Duration ttl, final RouteTypeName routeType) {
return new Match(backend, Optional.of(ttl), routeType);
}

public static Match of(final String backend, final Optional<Duration> ttl, final RouteTypeName routeType) {
return new Match(backend, ttl, routeType);
}

public String backend() {
return backend;
}

public Optional<Duration> ttl() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be me not understanding the coverage report, but it looks like this new method is not covered by any test?

return ttl;
}

public RouteType routeType(final RouteTypes routeTypes) {
return routeType.from(routeTypes);
}

@Override
public int hashCode() {
return Objects.hash(backend, routeType);
return Objects.hash(backend, ttl, routeType);
}

@Override
public boolean equals(final Object obj) {
if (this == obj)
if (this == obj) {
return true;
if (obj == null)
}
if (obj == null) {
return false;
if (getClass() != obj.getClass())
}
if (getClass() != obj.getClass()) {
return false;
}
final Match other = (Match) obj;
return Objects.equals(backend, other.backend) && routeType == other.routeType;
return Objects.equals(backend, other.backend) && routeType == other.routeType && Objects.equals(ttl, other.ttl);
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@ public ProxyRoute(final SessionAwareProxyClient templateClient) {
@Override
public CompletionStage<ResponseWithSession<ByteString>> execute(final RouteMatch rm, final RequestContext context,
final SessionRoot session) {
return templateClient.fetch(rm.expandedPath(), context, session);
return templateClient.fetch(rm, context, session);
}
}
19 changes: 14 additions & 5 deletions src/main/java/com/rewedigital/composer/routing/RouteMatch.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package com.rewedigital.composer.routing;

import static java.util.Objects.requireNonNull;

import java.time.Duration;
import java.util.Collections;
import java.util.Map;
import java.util.Optional;

import com.damnhandy.uri.template.UriTemplate;

Expand All @@ -11,14 +15,18 @@ public class RouteMatch {
private final Map<String, Object> parsedPathArguments;

public RouteMatch(final Match backend, final Map<String, String> parsedPathArguments) {
this.backend = backend;
this.backend = requireNonNull(backend);
this.parsedPathArguments = Collections.<String, Object>unmodifiableMap(parsedPathArguments);
}

public String backend() {
return backend.backend();
}

public Optional<Duration> ttl() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be me not understanding the coverage report, but it looks like there is not test covering this new method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And actually, I don't find the code using the route timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

return backend.ttl();
}

public RouteType routeType(final RouteTypes routeTypes) {
return backend.routeType(routeTypes);
}
Expand All @@ -27,12 +35,13 @@ public Map<String, Object> parsedPathArguments() {
return parsedPathArguments;
}

public String expandedPath() {
return UriTemplate.fromTemplate(backend.backend()).expand(parsedPathArguments);
}

@Override
public String toString() {
return "RouteMatch [backend=" + backend + ", parsedPathArguments=" + parsedPathArguments + "]";
}

public String expandedPath() {
return UriTemplate.fromTemplate(backend.backend()).expand(parsedPathArguments);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package com.rewedigital.composer.routing;

import java.util.List;
import java.util.stream.Collectors;
import static java.util.stream.Collectors.toList;

import java.time.Duration;
import java.util.List;
import java.util.Optional;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -25,7 +27,7 @@ public static RoutingConfiguration fromConfig(final Config config) {
}

private static List<Rule<Match>> buildLocalRoutes(final ConfigList routesConfig) {
return routesConfig.stream().map(RoutingConfiguration::buildLocalRoute).collect(Collectors.toList());
return routesConfig.stream().map(RoutingConfiguration::buildLocalRoute).collect(toList());
}

private static Rule<Match> buildLocalRoute(final ConfigValue configValue) {
Expand All @@ -34,16 +36,20 @@ private static Rule<Match> buildLocalRoute(final ConfigValue configValue) {
final String method = config.getString("method");
final String type = config.getString("type");
final String target = config.getString("target");
final Optional<Duration> ttl = optionalDuration(config, "ttl");

final Rule<Match> result = Rule.fromUri(path, method, Match.of(target, RouteTypeName.valueOf(type)));
LOGGER.info("Registered local route for path={}, method={}, target={}, type={}", path, method,
target, type);
final Rule<Match> result = Rule.fromUri(path, method, Match.of(target, ttl, RouteTypeName.valueOf(type)));
LOGGER.info("Registered local route for path={}, method={}, target={}, type={} with a request ttl={}", path,
method,target, type, ttl);
return result;

}

public List<Rule<Match>> localRules() {
return localRoutes;
}

private static Optional<Duration> optionalDuration(final Config config, final String path) {
return config.hasPath(path) ? Optional.of(config.getDuration(path)) : Optional.empty();
}

}
Loading