-
Notifications
You must be signed in to change notification settings - Fork 2
Add the facility to override the request timeout per include and route. #28
Changes from 4 commits
961a9be
cf4557b
dd15e6e
7d923d7
edc5874
3844936
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
} | ||
|
||
/** | ||
* 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -41,6 +41,10 @@ private List<Asset> assets() { | |
return contentMarkupHandler.assets(); | ||
} | ||
|
||
List<IncludedService> includedServices() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As this is only accessible for testing, maybe add the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
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; | ||
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest making these methods There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a method like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could overload the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
|
||
|
||
} |
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; | ||
|
||
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And actually, I don't find the code using the route timeout? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
@@ -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; | ||
|
||
|
@@ -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) { | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That utility is private and final There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On my classpath it is There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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...There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect.