-
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 all 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,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); | ||
} | ||
|
||
/** | ||
* 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 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 |
---|---|---|
@@ -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; | ||
|
||
/** | ||
|
@@ -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) { | ||
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 |
---|---|---|
@@ -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() { | ||
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 && Objects.equals(ttl, other.ttl); | ||
} | ||
|
||
|
||
} |
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); | ||
} | ||
|
@@ -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); | ||
} | ||
} | ||
} |
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.