arturobernalg commented on code in PR #475:
URL:
https://github.com/apache/httpcomponents-client/pull/475#discussion_r1297637055
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java:
##########
@@ -71,28 +69,23 @@ public class CachingExecBase {
final CacheableRequestPolicy cacheableRequestPolicy;
final CachedResponseSuitabilityChecker suitabilityChecker;
final ResponseProtocolCompliance responseCompliance;
- final RequestProtocolCompliance requestCompliance;
final CacheConfig cacheConfig;
private static final Logger LOG =
LoggerFactory.getLogger(CachingExecBase.class);
- private static final TimeValue ONE_DAY = TimeValue.ofHours(24);
-
CachingExecBase(
final CacheValidityPolicy validityPolicy,
final ResponseCachingPolicy responseCachingPolicy,
Review Comment:
I believe we can also remove `responseCachingPolicy`.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java:
##########
@@ -187,8 +164,7 @@ SimpleHttpResponse generateCachedResponse(
final ResponseCacheControl responseCacheControl,
Review Comment:
I think we can remove `responseCacheControl`
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseProtocolCompliance.java:
##########
@@ -28,198 +28,51 @@
import java.io.IOException;
import java.time.Instant;
-import java.util.ArrayList;
-import java.util.List;
-import org.apache.hc.client5.http.ClientProtocolException;
-import org.apache.hc.client5.http.utils.DateUtils;
-import org.apache.hc.core5.http.Header;
-import org.apache.hc.core5.http.HeaderElement;
-import org.apache.hc.core5.http.HeaderElements;
+import org.apache.hc.core5.annotation.Contract;
+import org.apache.hc.core5.annotation.ThreadingBehavior;
+import org.apache.hc.core5.http.EntityDetails;
+import org.apache.hc.core5.http.HttpException;
import org.apache.hc.core5.http.HttpHeaders;
-import org.apache.hc.core5.http.HttpRequest;
import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpResponseInterceptor;
import org.apache.hc.core5.http.HttpStatus;
-import org.apache.hc.core5.http.HttpVersion;
-import org.apache.hc.core5.http.Method;
-import org.apache.hc.core5.http.ProtocolVersion;
import org.apache.hc.core5.http.message.BasicHeader;
-import org.apache.hc.core5.http.message.MessageSupport;
-
-class ResponseProtocolCompliance {
-
- private static final String UNEXPECTED_100_CONTINUE = "The incoming
request did not contain a "
- + "100-continue header, but the response was a Status 100,
continue.";
- private static final String UNEXPECTED_PARTIAL_CONTENT = "partial content
was returned for a request that did not ask for it";
-
- /**
- * When we get a response from a down stream server (Origin Server)
- * we attempt to see if it is HTTP 1.1 Compliant and if not, attempt to
- * make it so.
- *
- * @param originalRequest The original {@link HttpRequest}
- * @param request The {@link HttpRequest} that generated an origin hit and
response
- * @param response The {@link HttpResponse} from the origin server
- * @throws IOException Bad things happened
- */
- public void ensureProtocolCompliance(
- final HttpRequest originalRequest,
- final HttpRequest request,
- final HttpResponse response) throws IOException {
- requestDidNotExpect100ContinueButResponseIsOne(originalRequest,
response);
-
- transferEncodingIsNotReturnedTo1_0Client(originalRequest, response);
-
- ensurePartialContentIsNotSentToAClientThatDidNotRequestIt(request,
response);
-
- ensure200ForOPTIONSRequestWithNoBodyHasContentLengthZero(request,
response);
-
- ensure206ContainsDateHeader(response);
-
- ensure304DoesNotContainExtraEntityHeaders(response);
-
- identityIsNotUsedInContentEncoding(response);
-
- warningsWithNonMatchingWarnDatesAreRemoved(response);
- }
-
- private void warningsWithNonMatchingWarnDatesAreRemoved(
- final HttpResponse response) {
- final Instant responseDate = DateUtils.parseStandardDate(response,
HttpHeaders.DATE);
- if (responseDate == null) {
- return;
- }
-
- final Header[] warningHeaders =
response.getHeaders(HttpHeaders.WARNING);
-
- if (warningHeaders == null || warningHeaders.length == 0) {
- return;
- }
-
- final List<Header> newWarningHeaders = new ArrayList<>();
- boolean modified = false;
- for(final Header h : warningHeaders) {
- for(final WarningValue wv : WarningValue.getWarningValues(h)) {
- final Instant warnInstant = wv.getWarnDate();
- if (warnInstant == null || warnInstant.equals(responseDate)) {
- newWarningHeaders.add(new
BasicHeader(HttpHeaders.WARNING,wv.toString()));
- } else {
- modified = true;
- }
- }
- }
- if (modified) {
- response.removeHeaders(HttpHeaders.WARNING);
- for(final Header h : newWarningHeaders) {
- response.addHeader(h);
- }
- }
- }
-
- private void identityIsNotUsedInContentEncoding(final HttpResponse
response) {
- final Header[] hdrs =
response.getHeaders(HttpHeaders.CONTENT_ENCODING);
- if (hdrs == null || hdrs.length == 0) {
- return;
- }
- final List<Header> newHeaders = new ArrayList<>();
- boolean modified = false;
- for (final Header h : hdrs) {
- final StringBuilder buf = new StringBuilder();
- boolean first = true;
- for (final HeaderElement elt : MessageSupport.parse(h)) {
- if ("identity".equalsIgnoreCase(elt.getName())) {
- modified = true;
- } else {
- if (!first) {
- buf.append(",");
- }
- buf.append(elt);
- first = false;
- }
- }
- final String newHeaderValue = buf.toString();
- if (!newHeaderValue.isEmpty()) {
- newHeaders.add(new BasicHeader(HttpHeaders.CONTENT_ENCODING,
newHeaderValue));
- }
- }
- if (!modified) {
- return;
- }
- response.removeHeaders(HttpHeaders.CONTENT_ENCODING);
- for (final Header h : newHeaders) {
- response.addHeader(h);
- }
- }
-
- private void ensure206ContainsDateHeader(final HttpResponse response) {
- if (response.getFirstHeader(HttpHeaders.DATE) == null) {
- response.addHeader(HttpHeaders.DATE,
DateUtils.formatStandardDate(Instant.now()));
- }
-
- }
-
- private void
ensurePartialContentIsNotSentToAClientThatDidNotRequestIt(final HttpRequest
request,
- final HttpResponse response) throws IOException {
- if (request.getFirstHeader(HttpHeaders.RANGE) != null
- || response.getCode() != HttpStatus.SC_PARTIAL_CONTENT) {
- return;
- }
- throw new ClientProtocolException(UNEXPECTED_PARTIAL_CONTENT);
- }
-
- private void
ensure200ForOPTIONSRequestWithNoBodyHasContentLengthZero(final HttpRequest
request,
- final HttpResponse response) {
- if (!Method.OPTIONS.isSame(request.getMethod())) {
- return;
- }
-
- if (response.getCode() != HttpStatus.SC_OK) {
- return;
- }
-
- if (response.getFirstHeader(HttpHeaders.CONTENT_LENGTH) == null) {
- response.addHeader(HttpHeaders.CONTENT_LENGTH, "0");
- }
- }
-
- private void ensure304DoesNotContainExtraEntityHeaders(final HttpResponse
response) {
- final String[] disallowedEntityHeaders = { HttpHeaders.ALLOW,
HttpHeaders.CONTENT_ENCODING,
- "Content-Language", HttpHeaders.CONTENT_LENGTH, "Content-MD5",
- "Content-Range", HttpHeaders.CONTENT_TYPE,
HttpHeaders.LAST_MODIFIED
- };
+import org.apache.hc.core5.http.protocol.HttpContext;
+
+/**
+ * This response interceptor removes common {@literal Content-} headers from
the 304
+ * response messages based on the provision of the HTTP specification that 304
+ * status code represents a confirmation that the requested resource content
has
+ * not changed since its retrieval or the last update and adds {@literal Date}
+ * header if it is not present in the response message..
+ */
+@Contract(threading = ThreadingBehavior.IMMUTABLE)
+class ResponseProtocolCompliance implements HttpResponseInterceptor {
Review Comment:
Is this change related to the guidelines in '[3.2. Updating Stored Header
Fields'](https://www.rfc-editor.org/rfc/rfc9111.html#name-updating-stored-header-fiel)
from the RFC9111 specification?
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheConfig.java:
##########
@@ -118,7 +118,10 @@ public class CacheConfig implements Cloneable {
public final static boolean DEFAULT_303_CACHING_ENABLED = false;
/** Default setting to allow weak tags on PUT/DELETE methods
+ *
+ * @deprecated Do not use.
*/
+ @Deprecated
Review Comment:
Since it's a public class, IMO we should also update the class's JavaDoc.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java:
##########
@@ -71,28 +69,23 @@ public class CachingExecBase {
final CacheableRequestPolicy cacheableRequestPolicy;
final CachedResponseSuitabilityChecker suitabilityChecker;
final ResponseProtocolCompliance responseCompliance;
Review Comment:
We can safely delete `responseCompliance` as well.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java:
##########
@@ -112,12 +112,11 @@ class AsyncCachingExec extends CachingExecBase implements
AsyncExecChainHandler
final CacheableRequestPolicy cacheableRequestPolicy,
final CachedResponseSuitabilityChecker suitabilityChecker,
final ResponseProtocolCompliance responseCompliance,
Review Comment:
I think we can remove `responseCompliance`
--
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-unsubscr...@hc.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org
For additional commands, e-mail: dev-h...@hc.apache.org
