Skip to content
Open
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 @@ -302,7 +302,7 @@ public void completed(final CacheMatch result) {
final CacheHit root = result != null ? result.root : null;
if (hit == null) {
if (requestCollapsingEnabled && root == null && !requestCacheControl.isOnlyIfCached()) {
final String cacheKey = CacheKeyGenerator.INSTANCE.generateKey(target, cacheRequest);
final String cacheKey = CacheKeyGenerator.INSTANCE.generateKey(target, cacheRequest, SimpleHttpRequest::getBodyText);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make more sense to use byte[] here and SimpleHttpRequest::getBodyBytes at the call sites? getBodyText() can collapse different binary or invalid byte sequences into the same String, which would make the eventual cache key ambiguous.

final CacheRequestCollapser.Token token = collapser.enter(cacheKey);
if (token.isLeader()) {
handleCacheMiss(requestCacheControl, null, target, cacheRequest, scope, chain, new AsyncExecCallback() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public BasicHttpAsyncCache(final ResourceFactory resourceFactory, final HttpAsyn

@Override
public Cancellable match(final HttpHost host, final SimpleHttpRequest request, final FutureCallback<CacheMatch> callback) {
final String rootKey = cacheKeyGenerator.generateKey(host, request);
final String rootKey = cacheKeyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
if (LOG.isDebugEnabled()) {
LOG.debug("Get cache entry: {}", rootKey);
}
Expand Down Expand Up @@ -390,7 +390,7 @@ public Cancellable store(
final Instant requestSent,
final Instant responseReceived,
final FutureCallback<CacheHit> callback) {
final String rootKey = cacheKeyGenerator.generateKey(host, request);
final String rootKey = cacheKeyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
if (LOG.isDebugEnabled()) {
LOG.debug("Create cache entry: {}", rootKey);
}
Expand Down Expand Up @@ -465,7 +465,7 @@ public Cancellable storeFromNegotiated(

storeInternal(negotiated.getEntryKey(), updatedEntry, null);

final String rootKey = cacheKeyGenerator.generateKey(host, request);
final String rootKey = cacheKeyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
final HttpCacheEntry copy = cacheEntryFactory.copy(updatedEntry);
final String variantKey = cacheKeyGenerator.generateVariantKey(request, copy);
return store(rootKey, variantKey, copy, callback);
Expand Down Expand Up @@ -551,7 +551,7 @@ public Cancellable evictInvalidatedEntries(
final int status = response.getCode();
if (status >= HttpStatus.SC_SUCCESS && status < HttpStatus.SC_CLIENT_ERROR &&
!Method.isSafe(request.getMethod())) {
final String rootKey = cacheKeyGenerator.generateKey(host, request);
final String rootKey = cacheKeyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
evict(rootKey);
final URI requestUri = CacheSupport.requestUriNormalizedOrNull(host, request);
if (requestUri != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ private void removeInternal(final String cacheKey) {

@Override
public CacheMatch match(final HttpHost host, final SimpleHttpRequest request) {
final String rootKey = cacheKeyGenerator.generateKey(host, request);
final String rootKey = cacheKeyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
if (LOG.isDebugEnabled()) {
LOG.debug("Get cache root entry: {}", rootKey);
}
Expand Down Expand Up @@ -223,7 +223,7 @@ public CacheHit store(
final ByteArrayBuffer content,
final Instant requestSent,
final Instant responseReceived) {
final String rootKey = cacheKeyGenerator.generateKey(host, request);
final String rootKey = cacheKeyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
if (LOG.isDebugEnabled()) {
LOG.debug("Create cache entry: {}", rootKey);
}
Expand Down Expand Up @@ -294,7 +294,7 @@ public CacheHit storeFromNegotiated(
negotiated.entry);
storeInternal(negotiated.getEntryKey(), updatedEntry);

final String rootKey = cacheKeyGenerator.generateKey(host, request);
final String rootKey = cacheKeyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
final HttpCacheEntry copy = cacheEntryFactory.copy(updatedEntry);
final String variantKey = cacheKeyGenerator.generateVariantKey(request, copy);
return store(rootKey, variantKey, copy);
Expand Down Expand Up @@ -347,7 +347,7 @@ public void evictInvalidatedEntries(final HttpHost host, final SimpleHttpRequest
final int status = response.getCode();
if (status >= HttpStatus.SC_SUCCESS && status < HttpStatus.SC_CLIENT_ERROR &&
!Method.isSafe(request.getMethod())) {
final String rootKey = cacheKeyGenerator.generateKey(host, request);
final String rootKey = cacheKeyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
evict(rootKey);
final URI requestUri = CacheSupport.requestUriNormalizedOrNull(host, request);
if (requestUri != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.Spliterators;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.stream.StreamSupport;

import org.apache.hc.client5.http.cache.HttpCacheEntry;
Expand Down Expand Up @@ -72,6 +73,10 @@ public class CacheKeyGenerator implements Resolver<URI, String> {
*/
public static final CacheKeyGenerator INSTANCE = new CacheKeyGenerator();

/**
* @deprecated Do not use.
*/
@Deprecated
@Override
public String resolve(final URI uri) {
return generateKey(uri);
Expand Down Expand Up @@ -135,9 +140,33 @@ public String generateKey(final URI requestUri) {
* @param host The host for this request
* @param request the {@link HttpRequest}
* @return cache key
*
* @deprecated Use {@link #generateKey(HttpHost, HttpRequest, Function)}
*/
@Deprecated
public String generateKey(final HttpHost host, final HttpRequest request) {
return generateKey(host, request, r -> null);
}

/**
* Computes a root key for the given {@link HttpHost}, {@link HttpRequest} and
* request body that can be used as a unique identifier for cached resources.
*
* @param host The host for this request
* @param request the {@link HttpRequest}
* @param bodyExtractor function to extract request body. May be {@code null}
* @return cache key
*
* @since 5.7
*/
public <T extends HttpRequest> String generateKey(final HttpHost host,
final T request,
final Function<T, String> bodyExtractor) {
final String s = CacheSupport.requestUriRaw(host, request);
final String body = bodyExtractor != null ? bodyExtractor.apply(request) : null;
if (body != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it supposed to fail now if the body extractor succeeds?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@desiderantes This is a temporary sanity check as the caching layer simply does not work correctly with entity enclosing requests. This is something I presume you are going to fix in your pull request.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good, just wanted to confirm

throw new UnsupportedOperationException();
}
try {
return generateKey(new URI(s));
} catch (final URISyntaxException ex) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class TestBasicHttpAsyncCache {
private Instant now;
private Instant tenSecondsAgo;
private SimpleHttpAsyncCacheStorage backing;
private CacheKeyGenerator keyGenerator;
private BasicHttpAsyncCache impl;

@BeforeEach
Expand All @@ -77,6 +78,7 @@ void setUp() {
now = Instant.now();
tenSecondsAgo = now.minusSeconds(10);
backing = Mockito.spy(new SimpleHttpAsyncCacheStorage());
keyGenerator = CacheKeyGenerator.INSTANCE;
impl = new BasicHttpAsyncCache(HeapResourceFactory.INSTANCE, backing);
}

Expand All @@ -102,7 +104,7 @@ void testGetCacheEntryFetchesFromCacheOnCacheHitIfNoVariants() throws Exception
final HttpHost host = new HttpHost("foo.example.com");
final SimpleHttpRequest request = new SimpleHttpRequest("GET", "http://foo.example.com/bar");

final String key = CacheKeyGenerator.INSTANCE.generateKey(host, request);
final String key = keyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);

backing.map.put(key,entry);

Expand Down Expand Up @@ -275,7 +277,7 @@ void testGetVariantCacheEntriesReturnsAllVariants() throws Exception {
final SimpleHttpRequest req1 = new SimpleHttpRequest("GET", uri);
req1.setHeader("Accept-Encoding", "gzip");

final String rootKey = CacheKeyGenerator.INSTANCE.generateKey(uri);
final String rootKey = keyGenerator.generateKey(uri);

final HttpResponse resp1 = HttpTestUtils.make200Response();
resp1.setHeader("Date", DateUtils.formatStandardDate(now));
Expand Down Expand Up @@ -547,7 +549,7 @@ void testInvalidatesUnsafeRequests() throws Exception {
final SimpleHttpRequest request = new SimpleHttpRequest( "POST", "/path");
final HttpResponse response = HttpTestUtils.make200Response();

final String key = CacheKeyGenerator.INSTANCE.generateKey(host, request);
final String key = keyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);

backing.putEntry(key, HttpTestUtils.makeCacheEntry());

Expand Down Expand Up @@ -587,7 +589,7 @@ void testDoesNotInvalidateSafeRequests() throws Exception {
@Test
void testInvalidatesUnsafeRequestsWithVariants() throws Exception {
final SimpleHttpRequest request = new SimpleHttpRequest( "POST", "/path");
final String rootKey = CacheKeyGenerator.INSTANCE.generateKey(host, request);
final String rootKey = keyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
final Set<String> variants = new HashSet<>();
variants.add("{var1}");
variants.add("{var2}");
Expand Down Expand Up @@ -618,12 +620,12 @@ void testInvalidatesUnsafeRequestsWithVariants() throws Exception {
@Test
void testInvalidateUriSpecifiedByContentLocationAndFresher() throws Exception {
final SimpleHttpRequest request = new SimpleHttpRequest( "PUT", "/foo");
final String rootKey = CacheKeyGenerator.INSTANCE.generateKey(host, request);
final String rootKey = keyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
final URI contentUri = new URIBuilder()
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand All @@ -650,12 +652,12 @@ void testInvalidateUriSpecifiedByContentLocationAndFresher() throws Exception {
@Test
void testInvalidateUriSpecifiedByLocationAndFresher() throws Exception {
final SimpleHttpRequest request = new SimpleHttpRequest( "PUT", "/foo");
final String rootKey = CacheKeyGenerator.INSTANCE.generateKey(host, request);
final String rootKey = keyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
final URI contentUri = new URIBuilder()
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand Down Expand Up @@ -702,12 +704,12 @@ void testDoesNotInvalidateForUnsuccessfulResponse() throws Exception {
@Test
void testInvalidateUriSpecifiedByContentLocationNonCanonical() throws Exception {
final SimpleHttpRequest request = new SimpleHttpRequest( "PUT", "/foo");
final String rootKey = CacheKeyGenerator.INSTANCE.generateKey(host, request);
final String rootKey = keyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
final URI contentUri = new URIBuilder()
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand Down Expand Up @@ -737,12 +739,12 @@ void testInvalidateUriSpecifiedByContentLocationNonCanonical() throws Exception
@Test
void testInvalidateUriSpecifiedByContentLocationRelative() throws Exception {
final SimpleHttpRequest request = new SimpleHttpRequest( "PUT", "/foo");
final String rootKey = CacheKeyGenerator.INSTANCE.generateKey(host, request);
final String rootKey = keyGenerator.generateKey(host, request, SimpleHttpRequest::getBodyText);
final URI contentUri = new URIBuilder()
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand Down Expand Up @@ -776,7 +778,7 @@ void testDoesNotInvalidateUriSpecifiedByContentLocationOtherOrigin() throws Exce
.setHost("bar.example.com")
.setPath("/")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand All @@ -801,7 +803,7 @@ void testDoesNotInvalidateUriSpecifiedByContentLocationIfEtagsMatch() throws Exc
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"same-etag\"");
Expand All @@ -828,7 +830,7 @@ void testDoesNotInvalidateUriSpecifiedByContentLocationIfOlder() throws Exceptio
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand All @@ -855,7 +857,7 @@ void testDoesNotInvalidateUriSpecifiedByContentLocationIfResponseHasNoEtag() thr
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.removeHeaders("ETag");
Expand All @@ -882,7 +884,7 @@ void testDoesNotInvalidateUriSpecifiedByContentLocationIfEntryHasNoEtag() throws
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag", "\"some-etag\"");
Expand All @@ -908,7 +910,7 @@ void testInvalidatesUriSpecifiedByContentLocationIfResponseHasNoDate() throws Ex
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag", "\"new-etag\"");
Expand All @@ -935,7 +937,7 @@ void testInvalidatesUriSpecifiedByContentLocationIfEntryHasNoDate() throws Excep
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand All @@ -961,7 +963,7 @@ void testInvalidatesUriSpecifiedByContentLocationIfResponseHasMalformedDate() th
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand All @@ -988,7 +990,7 @@ void testInvalidatesUriSpecifiedByContentLocationIfEntryHasMalformedDate() throw
.setHttpHost(host)
.setPath("/bar")
.build();
final String contentKey = CacheKeyGenerator.INSTANCE.generateKey(contentUri);
final String contentKey = keyGenerator.generateKey(contentUri);

final HttpResponse response = HttpTestUtils.make200Response();
response.setHeader("ETag","\"new-etag\"");
Expand Down
Loading
Loading