You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2023/05/11 19:44:52 UTC
[GitHub] [httpcomponents-client] arturobernalg opened a new pull request, #446: HTTPCLIENT-2273: Enhance "no-cache" directive handling
arturobernalg opened a new pull request, #446:
URL: https://github.com/apache/httpcomponents-client/pull/446
This pull request addresses ticket [HTTPCLIENT-2273](https://issues.apache.org/jira/browse/HTTPCLIENT-2273), which identifies an issue with the Apache HttpComponents client's handling of the "no-cache" directive when used with specific header fields, as outlined in RFC 7234.
Changes made to address this issue include:
- Updating the caching module to correctly identify and handle "no-cache" directives with specific header fields. This was done by modifying the `responseContainsNoCacheDirective` method.
- Modifying the `handleCacheHit` method to ensure that a cached entry is revalidated with the origin server when a "no-cache" directive with specific header fields is present in the response.
- Ensuring that the rest of the response is still cacheable when the specified header fields are present, as long as the response complies with other caching requirements.
These changes allow an origin server to prevent the reuse of certain header fields in a response, while still allowing caching of the rest of the response. Corresponding unit tests have been updated to reflect these changes.
--
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 #446: HTTPCLIENT-2273: Enhance "no-cache" directive handling
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #446:
URL: https://github.com/apache/httpcomponents-client/pull/446#discussion_r1193122728
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -162,6 +169,19 @@ public final CacheControl parse(final Header header) {
mustRevalidate = true;
} else if (name.equalsIgnoreCase(HeaderConstants.CACHE_CONTROL_NO_CACHE)) {
noCache = true;
+ if (value != null) {
+ final Tokenizer.Cursor valueCursor = new Tokenizer.Cursor(cursor.getPos() - value.length() -1, buffer.length());
Review Comment:
> @arturobernalg My apologies. My suggestion was completely unfounded. Your initial implementation was perfectly fine. I restored it in your commit. Sorry about that.
np @ok2c
--
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 #446: HTTPCLIENT-2273: Enhance "no-cache" directive handling
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #446:
URL: https://github.com/apache/httpcomponents-client/pull/446#discussion_r1193118447
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -162,6 +169,19 @@ public final CacheControl parse(final Header header) {
mustRevalidate = true;
} else if (name.equalsIgnoreCase(HeaderConstants.CACHE_CONTROL_NO_CACHE)) {
noCache = true;
+ if (value != null) {
+ final Tokenizer.Cursor valueCursor = new Tokenizer.Cursor(cursor.getPos() - value.length() -1, buffer.length());
Review Comment:
@arturobernalg My apologies. My suggestion was completely unfounded. Your initial implementation was perfectly fine. I restored it in your commit. Sorry about that.
--
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 #446: HTTPCLIENT-2273: Enhance "no-cache" directive handling
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on code in PR #446:
URL: https://github.com/apache/httpcomponents-client/pull/446#discussion_r1193043166
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -162,6 +169,19 @@ public final CacheControl parse(final Header header) {
mustRevalidate = true;
} else if (name.equalsIgnoreCase(HeaderConstants.CACHE_CONTROL_NO_CACHE)) {
noCache = true;
+ if (value != null) {
+ final Tokenizer.Cursor valueCursor = new Tokenizer.Cursor(cursor.getPos() - value.length() -1, buffer.length());
Review Comment:
I made some changes to avoid creating an additional Tokenizer.Cursor, but I'm not completely certain if it's what you're looking for..
--
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 #446: HTTPCLIENT-2273: Enhance "no-cache" directive handling
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on code in PR #446:
URL: https://github.com/apache/httpcomponents-client/pull/446#discussion_r1193014635
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControl.java:
##########
@@ -129,6 +140,7 @@ public CacheControl(final long maxAge, final long sharedMaxAge, final boolean mu
this.proxyRevalidate = proxyRevalidate;
this.cachePublic = cachePublic;
this.stale_while_revalidate = stale_while_revalidate;
+ this.noCacheFields = noCacheFields;
Review Comment:
@arturobernalg Please make this set unmodifiable
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java:
##########
@@ -162,6 +169,19 @@ public final CacheControl parse(final Header header) {
mustRevalidate = true;
} else if (name.equalsIgnoreCase(HeaderConstants.CACHE_CONTROL_NO_CACHE)) {
noCache = true;
+ if (value != null) {
+ final Tokenizer.Cursor valueCursor = new Tokenizer.Cursor(cursor.getPos() - value.length() -1, buffer.length());
Review Comment:
@ There is a way to parse the value in a single pass without creating an extra `Tokenizer.Cursor`. It is nice to have, though.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java:
##########
@@ -525,4 +544,51 @@ private CacheControl parseCacheControlHeader(final MessageHeaders messageHeaders
}
}
+ /**
+ * Determines if the given {@link HttpCacheEntry} requires revalidation based on the presence of the {@code no-cache} directive
+ * in the Cache-Control header.
+ * <p>
+ * The method returns true in the following cases:
+ * - If the {@code no-cache} directive is present without any field names.
+ * - If the {@code no-cache} directive is present with field names, and at least one of these field names is present
+ * in the headers of the {@link HttpCacheEntry}.
+ * <p>
+ * If the {@code no-cache} directive is not present in the Cache-Control header, the method returns {@code false}.
+ *
+ * @param entry the {@link HttpCacheEntry} containing the headers to check for the {@code no-cache} directive.
+ * @return true if revalidation is required based on the {@code no-cache} directive, {@code false} otherwise.
+ */
+ boolean responseContainsNoCacheDirective(final HttpCacheEntry entry) {
+ final CacheControl responseCacheControl = parseCacheControlHeader(entry);
+
+ if (responseCacheControl != null) {
+ final Set<String> noCacheFields = responseCacheControl.getNoCacheFields();
+
+ // If no-cache directive is present and has no field names
+ if (responseCacheControl.isNoCache() && noCacheFields.isEmpty()) {
+ LOG.debug("No-cache directive present without field names. Revalidation required.");
+ return true;
+ }
+
+ // If no-cache directive is present with field names
+ if (responseCacheControl.isNoCache()) {
+ final HeaderGroup headerGroup = new HeaderGroup();
+ headerGroup.setHeaders(entry.getHeaders());
Review Comment:
@arturobernalg This looks completely unnecessary. `HttpCacheEntry`has `#getFirstHeader` method already.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControl.java:
##########
@@ -94,14 +97,20 @@ final class CacheControl {
*/
private final long stale_while_revalidate;
Review Comment:
@arturobernalg Minor nitpick: could you please make this variable use the naming consistent with other variables?
--
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 #446: HTTPCLIENT-2273: Enhance "no-cache" directive handling
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #446:
URL: https://github.com/apache/httpcomponents-client/pull/446#issuecomment-1546715076
@arturobernalg
1. While reviewing your code I realized that `CacheControlHeaderParser` in its present form was wrong. It assumes that there can be only a single header `Cache-Control` header, which is wrong. There can be multiple headers, all of which must be parsed and taken into account. I also would like the parser to be able to parse `Cache-Control` with values in one pass with a single cursor.
Headers
```
Cache-Control: this, that
```
and
```
Cache-Control: this
Cache-Control: that
```
must be treated as equivalent and must produce the same `Cache-Control` structure.
2. I found out that `Cache-Control` get parsed over and over again multiple times in the course of a request execution. This is wasteful and conceptually wrong.
I am going to address these two issues, once your PR is merged.
--
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 #446: HTTPCLIENT-2273: Enhance "no-cache" directive handling
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #446:
URL: https://github.com/apache/httpcomponents-client/pull/446#issuecomment-1546745527
> @arturobernalg
>
> 1. While reviewing your code I realized that `CacheControlHeaderParser` in its present form was wrong. It assumes that there can be only a single header `Cache-Control` header, which is wrong. There can be multiple headers, all of which must be parsed and taken into account.
>
> Headers
>
> ```
> Cache-Control: this, that
> ```
>
> and
>
> ```
> Cache-Control: this
> Cache-Control: that
> ```
>
> must be treated as equivalent and must produce the same `Cache-Control` structure. I also would like the parser to be able to parse `Cache-Control` with values in one pass with a single cursor.
>
> 2. I found out that `Cache-Control` get parsed over and over again multiple times in the course of a request execution. This is wasteful and conceptually wrong.
>
> I am going to address these two issues, once your PR is merged.
@ok2c , you're right. I didn't realize that. My apologies. I have made the necessary changes to resolve the issue. After merging the HTTPCLIENT-2273 PR, I will update the branch and fix the conflict. You can find the PR here: https://github.com/apache/httpcomponents-client/pull/447.
Regarding point 2, I would also like to address it, but I need some guidance or instructions to proceed."
--
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 #446: HTTPCLIENT-2273: Enhance "no-cache" directive handling
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c merged PR #446:
URL: https://github.com/apache/httpcomponents-client/pull/446
--
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