You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "ok2c (via GitHub)" <gi...@apache.org> on 2023/05/15 20:56:55 UTC
[GitHub] [httpcomponents-client] ok2c opened a new pull request, #449: Support for request Cache-Control
ok2c opened a new pull request, #449:
URL: https://github.com/apache/httpcomponents-client/pull/449
@arturobernalg I have started working on Cache-Control improvements throughout the entire HTTP cache code. I added `RequestCacheControl` to represent request cache control directives similar to `CacheControl`. Please take a look and let me know if you find anything disagreeable. As the next step I will try to make sure the request and response `Cache-Control` headers in the request and the response get parted only once instead of multiple times in various places.
We may want to rename `CacheControl` to `ResponseCacheControl` for consistency. Please let me know what you think.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #449: Cache-Control improvements
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#discussion_r1199722257
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -290,38 +293,38 @@ private ClassicHttpResponse handleCacheHit(
if (requestContainsNoCacheDirective(request)) {
Review Comment:
@ok2c Corrected.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on pull request #449: Support for request Cache-Control
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#issuecomment-1548693023
> @arturobernalg I have started working on Cache-Control improvements throughout the entire HTTP cache code. I added `RequestCacheControl` to represent request cache control directives similar to `CacheControl`. Please take a look and let me know if you find anything disagreeable. As the next step I will try to make sure the request and response `Cache-Control` headers in the request and the response get parted only once instead of multiple times in various places.
>
> We may want to rename `CacheControl` to `ResponseCacheControl` for consistency. Please let me know what you think.
@ok2c
Regarding renaming 'CacheControl' to 'ResponseCacheControl', I'm in agreement with this proposal. It brings clarity and consistency to our codebase, thereby enhancing its comprehensibility.
Have we considered the possibility of creating an abstract class for common directives?
Overall, it looks good.
By the way, the use of 'BiConsumer' is a nice touch.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on pull request #449: Cache-Control improvements
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#issuecomment-1556151151
> @arturobernalg Please do another pass.
@ok2c
This change-set looks good to me.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #449: Cache-Control improvements
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#issuecomment-1556144301
@arturobernalg Please do another pass.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on a diff in pull request #449: Cache-Control improvements
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#discussion_r1199656964
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java:
##########
@@ -290,38 +293,38 @@ private ClassicHttpResponse handleCacheHit(
if (requestContainsNoCacheDirective(request)) {
Review Comment:
We can The method 'requestContainsNoCacheDirective' also parses the request, but we have already done that. We should simply pass the RequestCacheControl. Also, it should be RequestCacheControl, not ResponseCacheControl.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on a diff in pull request #449: Cache-Control improvements
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#discussion_r1199658427
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ConditionalRequestBuilder.java:
##########
@@ -58,26 +56,17 @@
public T buildConditionalRequest(final T request, final HttpCacheEntry cacheEntry) {
final T newRequest = messageCopier.create(request);
- final Header eTag = cacheEntry.getFirstHeader(HeaderConstants.ETAG);
+ final Header eTag = cacheEntry.getFirstHeader(HttpHeaders.ETAG);
if (eTag != null) {
- newRequest.setHeader(HeaderConstants.IF_NONE_MATCH, eTag.getValue());
+ newRequest.setHeader(HttpHeaders.IF_NONE_MATCH, eTag.getValue());
}
- final Header lastModified = cacheEntry.getFirstHeader(HeaderConstants.LAST_MODIFIED);
+ final Header lastModified = cacheEntry.getFirstHeader(HttpHeaders.LAST_MODIFIED);
if (lastModified != null) {
- newRequest.setHeader(HeaderConstants.IF_MODIFIED_SINCE, lastModified.getValue());
+ newRequest.setHeader(HttpHeaders.IF_MODIFIED_SINCE, lastModified.getValue());
}
- boolean mustRevalidate = false;
- final Iterator<HeaderElement> it = MessageSupport.iterate(cacheEntry, HeaderConstants.CACHE_CONTROL);
- while (it.hasNext()) {
- final HeaderElement elt = it.next();
- if (HeaderConstants.CACHE_CONTROL_MUST_REVALIDATE.equalsIgnoreCase(elt.getName())
- || HeaderConstants.CACHE_CONTROL_PROXY_REVALIDATE.equalsIgnoreCase(elt.getName())) {
- mustRevalidate = true;
- break;
- }
- }
- if (mustRevalidate) {
- newRequest.addHeader(HeaderConstants.CACHE_CONTROL, HeaderConstants.CACHE_CONTROL_MAX_AGE + "=0");
+ final ResponseCacheControl cacheControl = CacheControlHeaderParser.INSTANCE.parse(cacheEntry);
Review Comment:
Similarly, since we already possess the ResponseCacheControl, we should simply pass it as a parameter
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a diff in pull request #449: Cache-Control improvements
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#discussion_r1199721588
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java:
##########
@@ -69,84 +70,51 @@ class CachedResponseSuitabilityChecker {
this(new CacheValidityPolicy(), config);
}
- private boolean isFreshEnough(final HttpCacheEntry entry, final HttpRequest request, final Instant now) {
- if (validityStrategy.isResponseFresh(entry, now)) {
+ private boolean isFreshEnough(final RequestCacheControl requestCacheControl,
+ final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry,
+ final Instant now) {
+ if (validityStrategy.isResponseFresh(responseCacheControl, entry, now)) {
return true;
}
if (useHeuristicCaching &&
validityStrategy.isResponseHeuristicallyFresh(entry, now, heuristicCoefficient, heuristicDefaultLifetime)) {
return true;
}
- if (originInsistsOnFreshness(entry)) {
+ if (originInsistsOnFreshness(responseCacheControl, entry)) {
return false;
}
- final long maxStale = getMaxStale(request);
- if (maxStale == -1) {
+ if (requestCacheControl.getMaxStale() == -1) {
return false;
}
- return (maxStale > validityStrategy.getStaleness(entry, now).toSeconds());
+ return (requestCacheControl.getMaxStale() > validityStrategy.getStaleness(responseCacheControl, entry, now).toSeconds());
}
- private boolean originInsistsOnFreshness(final HttpCacheEntry entry) {
- if (validityStrategy.mustRevalidate(entry)) {
+ private boolean originInsistsOnFreshness(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry) {
+ if (responseCacheControl.isMustRevalidate()) {
return true;
}
if (!sharedCache) {
return false;
}
- return validityStrategy.proxyRevalidate(entry) ||
- validityStrategy.hasCacheControlDirective(entry, "s-maxage");
- }
-
- private long getMaxStale(final HttpRequest request) {
- // This is a header value, we leave as-is
- long maxStale = -1;
- final Iterator<HeaderElement> it = MessageSupport.iterate(request, HeaderConstants.CACHE_CONTROL);
- while (it.hasNext()) {
- final HeaderElement elt = it.next();
- if (HeaderConstants.CACHE_CONTROL_MAX_STALE.equals(elt.getName())) {
- if ((elt.getValue() == null || elt.getValue().trim().isEmpty()) && maxStale == -1) {
- maxStale = Long.MAX_VALUE;
- } else {
- try {
- long val = Long.parseLong(elt.getValue());
- if (val < 0) {
- val = 0;
- }
- if (maxStale == -1 || val < maxStale) {
- maxStale = val;
- }
- } catch (final NumberFormatException nfe) {
- // err on the side of preserving semantic transparency
- maxStale = 0;
- }
- }
- }
- }
- return maxStale;
+ final ResponseCacheControl cacheControl = CacheControlHeaderParser.INSTANCE.parse(entry);
Review Comment:
@arturobernalg No, you are not mistaken. I am just getting old. Corrected.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #449: Support for request Cache-Control
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#issuecomment-1555906894
@arturobernalg Could you please review this PR and let me know if you find anything disagreeable?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c merged pull request #449: Cache-Control improvements
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] arturobernalg commented on a diff in pull request #449: Cache-Control improvements
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #449:
URL: https://github.com/apache/httpcomponents-client/pull/449#discussion_r1199656573
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedResponseSuitabilityChecker.java:
##########
@@ -69,84 +70,51 @@ class CachedResponseSuitabilityChecker {
this(new CacheValidityPolicy(), config);
}
- private boolean isFreshEnough(final HttpCacheEntry entry, final HttpRequest request, final Instant now) {
- if (validityStrategy.isResponseFresh(entry, now)) {
+ private boolean isFreshEnough(final RequestCacheControl requestCacheControl,
+ final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry,
+ final Instant now) {
+ if (validityStrategy.isResponseFresh(responseCacheControl, entry, now)) {
return true;
}
if (useHeuristicCaching &&
validityStrategy.isResponseHeuristicallyFresh(entry, now, heuristicCoefficient, heuristicDefaultLifetime)) {
return true;
}
- if (originInsistsOnFreshness(entry)) {
+ if (originInsistsOnFreshness(responseCacheControl, entry)) {
return false;
}
- final long maxStale = getMaxStale(request);
- if (maxStale == -1) {
+ if (requestCacheControl.getMaxStale() == -1) {
return false;
}
- return (maxStale > validityStrategy.getStaleness(entry, now).toSeconds());
+ return (requestCacheControl.getMaxStale() > validityStrategy.getStaleness(responseCacheControl, entry, now).toSeconds());
}
- private boolean originInsistsOnFreshness(final HttpCacheEntry entry) {
- if (validityStrategy.mustRevalidate(entry)) {
+ private boolean originInsistsOnFreshness(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry) {
+ if (responseCacheControl.isMustRevalidate()) {
return true;
}
if (!sharedCache) {
return false;
}
- return validityStrategy.proxyRevalidate(entry) ||
- validityStrategy.hasCacheControlDirective(entry, "s-maxage");
- }
-
- private long getMaxStale(final HttpRequest request) {
- // This is a header value, we leave as-is
- long maxStale = -1;
- final Iterator<HeaderElement> it = MessageSupport.iterate(request, HeaderConstants.CACHE_CONTROL);
- while (it.hasNext()) {
- final HeaderElement elt = it.next();
- if (HeaderConstants.CACHE_CONTROL_MAX_STALE.equals(elt.getName())) {
- if ((elt.getValue() == null || elt.getValue().trim().isEmpty()) && maxStale == -1) {
- maxStale = Long.MAX_VALUE;
- } else {
- try {
- long val = Long.parseLong(elt.getValue());
- if (val < 0) {
- val = 0;
- }
- if (maxStale == -1 || val < maxStale) {
- maxStale = val;
- }
- } catch (final NumberFormatException nfe) {
- // err on the side of preserving semantic transparency
- maxStale = 0;
- }
- }
- }
- }
- return maxStale;
+ final ResponseCacheControl cacheControl = CacheControlHeaderParser.INSTANCE.parse(entry);
Review Comment:
I might be mistaken or perhaps missing something, but don't we already have the ResponseCacheControl? It seems like we just need to pass it through the 'isFreshEnough' method.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org