arturobernalg commented on code in PR #464:
URL:
https://github.com/apache/httpcomponents-client/pull/464#discussion_r1244244483
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -165,9 +167,11 @@ public ClassicHttpResponse execute(
setResponseStatus(context,
CacheResponseStatus.CACHE_MODULE_RESPONSE);
return new BasicClassicHttpResponse(HttpStatus.SC_NOT_IMPLEMENTED);
}
- final HttpCacheEntry entry = responseCache.getCacheEntry(target,
request);
+ final CacheMatch result = responseCache.match(target, request);
+ final CacheHit hit = result != null ? result.hit : null;
+ final CacheHit root = result != null ? result.root : null;
- final SimpleHttpResponse fatalErrorResponse =
getFatallyNonCompliantResponse(request, context, entry != null);
+ final SimpleHttpResponse fatalErrorResponse =
getFatallyNonCompliantResponse(request, context, hit != null);
Review Comment:
I've noticed that the `hit != null` condition is being checked numerous
times in the code. To enhance code readability and reduce redundancy, I suggest
encapsulating this logic within the `CacheMatch` class itself. We could add a
`hasHit()` method as follows:
```
public boolean hasHit() {
return hit != null;
}
```
This way, we can simply call `cacheMatch.hasHit()` wherever we need to check
if hit is not null, improving the code's readability and maintainability."
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheMatch.java:
##########
@@ -26,30 +26,26 @@
*/
package org.apache.hc.client5.http.impl.cache;
-import org.apache.hc.client5.http.cache.HttpCacheEntry;
-
-/** Records a set of information describing a cached variant. */
-class Variant {
-
- private final String cacheKey;
- private final HttpCacheEntry entry;
-
- public Variant(final String cacheKey, final HttpCacheEntry entry) {
- this.cacheKey = cacheKey;
- this.entry = entry;
- }
+/**
+ * Represents a full match or a partial match (variant options)
+ * result of a cache lookup operation.
+ */
+class CacheMatch {
- public String getCacheKey() {
- return cacheKey;
- }
+ final CacheHit hit;
Review Comment:
@ok2c
Shouldn't this class be fully immutable?
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheHit.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+package org.apache.hc.client5.http.impl.cache;
+
+import org.apache.hc.client5.http.cache.HttpCacheEntry;
+
+class CacheHit {
+
+ final String rootKey;
+ final String variantKey;
+ final HttpCacheEntry entry;
+
+ public CacheHit(final String rootKey, final String variantKey, final
HttpCacheEntry entry) {
Review Comment:
Shouldn't this class be fully immutable? Also, shouldn't `rootKey` and
`entry` be null-safe?
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java:
##########
@@ -498,22 +502,24 @@ public void cancelled() {
storeRequestIfModifiedSinceFor304Response(request,
backendResponse);
} else {
LOG.debug("Backend response is not cacheable");
- responseCache.flushCacheEntriesFor(target, request, new
FutureCallback<Boolean>() {
+ if (!Method.isSafe(request.getMethod())) {
Review Comment:
Perhaps we could consider delegating this check
(`!Method.isSafe(request.getMethod())`) inside the method of the class
`HttpAsyncCache` and `BasicHttpCache` itself. This way, the method becomes more
resilient and we ensure the check is always performed when needed, reducing the
potential for errors.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -507,29 +522,26 @@ ClassicHttpResponse negotiateResponseFromVariants(
}
final String resultEtag = resultEtagHeader.getValue();
- final Variant matchingVariant = variants.get(resultEtag);
- if (matchingVariant == null) {
+ final CacheHit match = variantMap.get(resultEtag);
+ if (match == null) {
LOG.debug("304 response did not contain ETag matching one sent
in If-None-Match");
return callBackend(target, request, scope, chain);
}
- final HttpCacheEntry variantEntry = matchingVariant.getEntry();
-
- if (revalidationResponseIsTooOld(backendResponse, variantEntry)
+ if (revalidationResponseIsTooOld(backendResponse, match.entry)
&& (request.getEntity() == null ||
request.getEntity().isRepeatable())) {
final ClassicHttpRequest unconditional =
conditionalRequestBuilder.buildUnconditionalRequest(request);
return callBackend(target, unconditional, scope, chain);
}
recordCacheUpdate(scope.clientContext);
- final HttpCacheEntry responseEntry =
responseCache.updateVariantEntry(
- target, conditionalRequest, backendResponse, variantEntry,
requestDate, responseDate);
- if (shouldSendNotModifiedResponse(request, responseEntry)) {
- return
convert(responseGenerator.generateNotModifiedResponse(responseEntry), scope);
+ final CacheHit hit = responseCache.update(match, backendResponse,
requestDate, responseDate);
+ if (shouldSendNotModifiedResponse(request, hit.entry)) {
+ return
convert(responseGenerator.generateNotModifiedResponse(hit.entry), scope);
}
- final SimpleHttpResponse response =
responseGenerator.generateResponse(request, responseEntry);
- responseCache.reuseVariantEntryFor(target, request,
backendResponse, responseEntry, requestDate, responseDate);
+ final SimpleHttpResponse response =
responseGenerator.generateResponse(request, hit.entry);
+ responseCache.storeReusing(hit, target, request, backendResponse,
requestDate, responseDate);
Review Comment:
Shouldn't be the `entry` instead of the `hit`?
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -346,20 +350,19 @@ ClassicHttpResponse revalidateCacheEntry(
}
if (statusCode == HttpStatus.SC_NOT_MODIFIED) {
- final HttpCacheEntry updatedEntry = responseCache.updateEntry(
- target, request, cacheEntry, backendResponse,
requestDate, responseDate);
+ final CacheHit updated = responseCache.update(hit, request,
backendResponse, requestDate, responseDate);
if (suitabilityChecker.isConditional(request)
- && suitabilityChecker.allConditionalsMatch(request,
updatedEntry, Instant.now())) {
- return
convert(responseGenerator.generateNotModifiedResponse(updatedEntry), scope);
+ && suitabilityChecker.allConditionalsMatch(request,
updated.entry, Instant.now())) {
+ return
convert(responseGenerator.generateNotModifiedResponse(updated.entry), scope);
}
- return convert(responseGenerator.generateResponse(request,
updatedEntry), scope);
+ return convert(responseGenerator.generateResponse(request,
updated.entry), scope);
}
if (staleIfErrorAppliesTo(statusCode)
- && !staleResponseNotAllowed(requestCacheControl,
responseCacheControl, cacheEntry, getCurrentDate())
- &&
validityPolicy.mayReturnStaleIfError(requestCacheControl, responseCacheControl,
cacheEntry, responseDate)) {
+ && !staleResponseNotAllowed(requestCacheControl,
responseCacheControl, hit, getCurrentDate())
Review Comment:
I noticed in the `staleResponseNotAllowed` method we are passing in the
complete `CacheHit` object, but only the `entry` attribute within it is being
used. To maintain the original structure would it be feasible to pass just the
`entry` object instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
