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 4 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);

}
46 changes: 46 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,46 @@
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 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 @@ -41,6 +41,10 @@ private List<Asset> assets() {
return contentMarkupHandler.assets();
}

List<IncludedService> includedServices() {
Copy link
Member

Choose a reason for hiding this comment

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

As this is only accessible for testing, maybe add the @VisibleForTesting annotation? We have not discussed whether we want to use this annotation, but I'd like to try it to see if it improves readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this, however we have three VisibleForTesting on the classpath, and none is from a dependency that is referenced in our pom directly. I would prefer to add my own annotation?

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 and apologize, because I used one of the three in my last PR. Should we add our own dependency to whatever small annotation lib provides this annotation? I don't think we should implement our own annotation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used VisibleForTesting from guava for now.

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,7 +1,9 @@
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;
Expand Down Expand Up @@ -86,24 +88,40 @@ 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);
}

String fallback() {
return fallback;
}

private String path() {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest making these methods public instead of package private, if they must be accessible now

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annotated with VisibleForTesting

String path() {
return attributes.getOrDefault("path", "");
}

public boolean isInRage(final ContentRange contentRange) {
return contentRange.isInRange(startOffset);
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,35 @@ 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.path() == null || context.path().trim().isEmpty()) {
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 method like FetchContext.hasEmptyPath() or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree, btw. this is the cause why I did not prevent null for path and fallback.

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.

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) {
if (context.ttl().isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

I like context.ttl().map(ttl -> request.withTtl(ttl)).orElse(request) more than the if construct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imho it is taste... changed.

return request.withTtl(context.ttl().get());
}
return 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
28 changes: 19 additions & 9 deletions src/main/java/com/rewedigital/composer/routing/Match.java
Original file line number Diff line number Diff line change
@@ -1,45 +1,55 @@
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);
public static Match of(final String backend, final Optional<Duration> ttl, final RouteTypeName routeType) {
Copy link
Member

Choose a reason for hiding this comment

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

You could overload the of factory method to have a variant without the ttl to simplify all the tests having to pass some Optional.empty() argument

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 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 && ttl == other.ttl;
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to use Objects.equals(ttl, other.ttl) instead of ==

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

}


}
12 changes: 10 additions & 2 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 @@ -35,4 +43,4 @@ public String toString() {
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) {
Copy link
Member

Choose a reason for hiding this comment

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

You could use com.spotify.apollo.environment.ConfigUtil here instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That utility is private and final

Copy link
Member

@thovid thovid Mar 12, 2018

Choose a reason for hiding this comment

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

On my classpath it is public final class ConfigUtil , from dependency com/spotify/apollo-environment/1.6.3/apollo-environment-1.6.3.jar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fault, I do not really remember why I rolled my own impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no version for Duration, therefore I rolled my own impl.

return config.hasPath(path) ? Optional.of(config.getDuration(path)) : Optional.empty();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ private List<HttpCookie> parseCookieHeader(final String cookieHeader) {
try {
return HttpCookie.parse(cookieHeader);
} catch (final Exception e) {
LOGGER.warn("erro parsing cookie headers, defaulting to empty session", e);
LOGGER.warn("error parsing cookie headers, defaulting to empty session", e);
return emptyList();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ private long parseExpiresAt(final String s) {
try {
return Long.parseLong(s);
} catch (final NumberFormatException ex) {
LOGGER.error("maleformatted expires-at: {} - expireing session", s, ex);
LOGGER.error("Malformatted expires-at: {} - expiring session", s, ex);
return -1;
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/main/resources/composer.conf
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
http.server.port = 8000
http.server.port = ${?HTTP_PORT}

# http client configuration, timeouts are in ms
http.client.connectTimeout = 2000
http.client.readTimeout = 5000
http.client.writeTimeout = 5000

# parser configuration
composer.html.include-tag = rewe-digital-include
Expand Down
Loading