Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTPCLIENT-2277: deprecation of obsolete config parameters and removal of oudated or meaningless tests #484

Merged
merged 3 commits into from
Sep 21, 2023
Merged

HTTPCLIENT-2277: deprecation of obsolete config parameters and removal of oudated or meaningless tests #484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff3569c
HTTPCLIENT-2277: Deprecated 303 response caching switch as no longer …
ok2c Sep 3, 2023
926b642
Javadoc improvements (no functional changes)
ok2c Sep 5, 2023
0cc6a6d
HTTPCLIENT-2277: Revision of HTTP cache protocol requirement and reco…
ok2c Sep 11, 2023
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
91 changes: 30 additions & 61 deletions httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,21 +30,18 @@
import org.apache.hc.core5.util.TimeValue;

/**
* <p>Java Beans-style configuration for caching {@link org.apache.hc.client5.http.classic.HttpClient}.
* Any class in the caching module that has configuration options should take a
* {@link CacheConfig} argument in one of its constructors. A
* {@code CacheConfig} instance has sane and conservative defaults, so the
* easiest way to specify options is to get an instance and then set just
* the options you want to modify from their defaults.</p>
*
* <p><b>N.B.</b> This class is only for caching-specific configuration; to
* configure the behavior of the rest of the client, configure the
* {@link org.apache.hc.client5.http.classic.HttpClient} used as the &quot;backend&quot;
* for the {@code CachingHttpClient}.</p>
* <p>Configuration for HTTP caches</p>
*
* <p>Cache configuration can be grouped into the following categories:</p>
*
* <p><b>Cache size.</b> If the backend storage supports these limits, you
* <p><b>Protocol options.</b> I some cases the HTTP protocol allows for
* conditional behaviors or optional protocol extensions. Such conditional
* protocol behaviors or extensions can be turned on or off here.
* See {@link CacheConfig#isNeverCacheHTTP10ResponsesWithQuery()},
* {@link CacheConfig#isNeverCacheHTTP11ResponsesWithQuery()},
* {@link CacheConfig#isStaleIfErrorEnabled()}</p>
*
* <p><b>Cache size.</b> If the backend storage supports these limits, one
* can specify the {@link CacheConfig#getMaxCacheEntries maximum number of
* cache entries} as well as the {@link CacheConfig#getMaxObjectSize()}
* maximum cacheable response body size}.</p>
Expand All @@ -54,46 +51,25 @@
* responses to requests with {@code Authorization} headers or responses
* marked with {@code Cache-Control: private}. If, however, the cache
* is only going to be used by one logical "user" (behaving similarly to a
* browser cache), then you will want to {@link
* CacheConfig#isSharedCache()} turn off the shared cache setting}.</p>
*
* <p><b>303 caching</b>. RFC2616 explicitly disallows caching 303 responses;
* however, the HTTPbis working group says they can be cached
* if explicitly indicated in the response headers and permitted by the request method.
* (They also indicate that disallowing 303 caching is actually an unintended
* spec error in RFC2616).
* This behavior is off by default, to err on the side of a conservative
* adherence to the existing standard, but you may want to
* {@link Builder#setAllow303Caching(boolean) enable it}.
* browser cache), then one may want to {@link CacheConfig#isSharedCache()}
* turn off the shared cache setting}.</p>
*
* <p><b>Weak ETags on PUT/DELETE If-Match requests</b>. RFC2616 explicitly
* prohibits the use of weak validators in non-GET requests, however, the
* HTTPbis working group says while the limitation for weak validators on ranged
* requests makes sense, weak ETag validation is useful on full non-GET
* requests; e.g., PUT with If-Match. This behavior is off by default, to err on
* the side of a conservative adherence to the existing standard, but you may
* want to {@link Builder#setWeakETagOnPutDeleteAllowed(boolean) enable it}.
*
* <p><b>Heuristic caching</b>. Per RFC2616, a cache may cache certain cache
* entries even if no explicit cache control headers are set by the origin.
* This behavior is off by default, but you may want to turn this on if you
* are working with an origin that doesn't set proper headers but where you
* still want to cache the responses. You will want to {@link
* CacheConfig#isHeuristicCachingEnabled()} enable heuristic caching},
* <p><b>Heuristic caching</b>. Per HTTP caching specification, a cache may
* cache certain cache entries even if no explicit cache control headers are
* set by the origin. This behavior is off by default, but you may want to
* turn this on if you are working with an origin that doesn't set proper
* headers but where one may still want to cache the responses. Use {@link
* CacheConfig#isHeuristicCachingEnabled()} to enable heuristic caching},
* then specify either a {@link CacheConfig#getHeuristicDefaultLifetime()
* default freshness lifetime} and/or a {@link
* CacheConfig#getHeuristicCoefficient() fraction of the time since
* the resource was last modified}. See Sections
* <a href="http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.2.2">
* 13.2.2</a> and <a href="http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.2.4">
* 13.2.4</a> of the HTTP/1.1 RFC for more details on heuristic caching.</p>
* the resource was last modified}.
*
* <p><b>Background validation</b>. The cache module supports the
* {@code stale-while-revalidate} directive of
* <a href="http://tools.ietf.org/html/rfc5861">RFC5861</a>, which allows
* certain cache entry revalidations to happen in the background. Asynchronous
* validation is enabled by default but it could be disabled by setting the number
* of re-validation workers to {@code 0} with {@link CacheConfig#getAsynchronousWorkers()}
* {@code stale-while-revalidate} directive, which allows certain cache entry
* revalidations to happen in the background. Asynchronous validation is enabled
* by default but it could be disabled by setting the number of re-validation
* workers to {@code 0} with {@link CacheConfig#getAsynchronousWorkers()}
* parameter</p>
*/
public class CacheConfig implements Cloneable {
Expand All @@ -113,8 +89,10 @@ public class CacheConfig implements Cloneable {
*/
public final static int DEFAULT_MAX_UPDATE_RETRIES = 1;

/** Default setting for 303 caching
/**
* @deprecated No longer applicable. Do not use.
*/
@Deprecated
public final static boolean DEFAULT_303_CACHING_ENABLED = false;

/**
Expand Down Expand Up @@ -147,7 +125,6 @@ public class CacheConfig implements Cloneable {
private final long maxObjectSize;
private final int maxCacheEntries;
private final int maxUpdateRetries;
private final boolean allow303Caching;
private final boolean heuristicCachingEnabled;
private final float heuristicCoefficient;
private final TimeValue heuristicDefaultLifetime;
Expand All @@ -168,7 +145,6 @@ public class CacheConfig implements Cloneable {
final long maxObjectSize,
final int maxCacheEntries,
final int maxUpdateRetries,
final boolean allow303Caching,
final boolean heuristicCachingEnabled,
final float heuristicCoefficient,
final TimeValue heuristicDefaultLifetime,
Expand All @@ -182,7 +158,6 @@ public class CacheConfig implements Cloneable {
this.maxObjectSize = maxObjectSize;
this.maxCacheEntries = maxCacheEntries;
this.maxUpdateRetries = maxUpdateRetries;
this.allow303Caching = allow303Caching;
this.heuristicCachingEnabled = heuristicCachingEnabled;
this.heuristicCoefficient = heuristicCoefficient;
this.heuristicDefaultLifetime = heuristicDefaultLifetime;
Expand Down Expand Up @@ -257,11 +232,11 @@ public int getMaxUpdateRetries(){
}

/**
* Returns whether 303 caching is enabled.
* @return {@code true} if it is enabled.
* @deprecated No longer applicable. Do not use.
*/
@Deprecated
public boolean is303CachingEnabled() {
return allow303Caching;
return true;
Copy link
Member

Choose a reason for hiding this comment

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

@ok2c Should the return value of the deprecated is303CachingEnabled() method be set to false to align with all the changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

@arturobernalg The cache protocol layer will now be treating 303 responses as potentially cacheable by default. This is why I thought the deprecated method should also return true. Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

@ok2c that its correct. my bad.

}

/**
Expand Down Expand Up @@ -356,7 +331,6 @@ public static class Builder {
private long maxObjectSize;
private int maxCacheEntries;
private int maxUpdateRetries;
private boolean allow303Caching;
private boolean heuristicCachingEnabled;
private float heuristicCoefficient;
private TimeValue heuristicDefaultLifetime;
Expand All @@ -371,7 +345,6 @@ public static class Builder {
this.maxObjectSize = DEFAULT_MAX_OBJECT_SIZE_BYTES;
this.maxCacheEntries = DEFAULT_MAX_CACHE_ENTRIES;
this.maxUpdateRetries = DEFAULT_MAX_UPDATE_RETRIES;
this.allow303Caching = DEFAULT_303_CACHING_ENABLED;
this.heuristicCachingEnabled = DEFAULT_HEURISTIC_CACHING_ENABLED;
this.heuristicCoefficient = DEFAULT_HEURISTIC_COEFFICIENT;
this.heuristicDefaultLifetime = DEFAULT_HEURISTIC_LIFETIME;
Expand Down Expand Up @@ -407,12 +380,10 @@ public Builder setMaxUpdateRetries(final int maxUpdateRetries) {
}

/**
* Enables or disables 303 caching.
* @param allow303Caching should be {@code true} to
* permit 303 caching, {@code false} to disable it.
* @deprecated Has no effect. Do not use.
*/
@Deprecated
public Builder setAllow303Caching(final boolean allow303Caching) {
this.allow303Caching = allow303Caching;
return this;
}

Expand Down Expand Up @@ -537,7 +508,6 @@ public CacheConfig build() {
maxObjectSize,
maxCacheEntries,
maxUpdateRetries,
allow303Caching,
heuristicCachingEnabled,
heuristicCoefficient,
heuristicDefaultLifetime,
Expand All @@ -557,7 +527,6 @@ public String toString() {
builder.append("[maxObjectSize=").append(this.maxObjectSize)
.append(", maxCacheEntries=").append(this.maxCacheEntries)
.append(", maxUpdateRetries=").append(this.maxUpdateRetries)
.append(", 303CachingEnabled=").append(this.allow303Caching)
.append(", heuristicCachingEnabled=").append(this.heuristicCachingEnabled)
.append(", heuristicCoefficient=").append(this.heuristicCoefficient)
.append(", heuristicDefaultLifetime=").append(this.heuristicDefaultLifetime)
Expand Down
3 changes: 1 addition & 2 deletions ...ache/src/main/java/org/apache/hc/client5/http/impl/cache/CachedHttpResponseGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,8 @@ SimpleHttpResponse generateNotModifiedResponse(final HttpCacheEntry entry) {
final SimpleHttpResponse response = new SimpleHttpResponse(HttpStatus.SC_NOT_MODIFIED, "Not Modified");

// The response MUST include the following headers
// (http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html)

// - Date, unless its omission is required by section 14.8.1
// - Date
Header dateHeader = entry.getFirstHeader(HttpHeaders.DATE);
if (dateHeader == null) {
dateHeader = new BasicHeader(HttpHeaders.DATE, DateUtils.formatStandardDate(Instant.now()));
Expand Down
15 changes: 1 addition & 14 deletions httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@

public class CachingExecBase {

final static boolean SUPPORTS_RANGE_AND_CONTENT_RANGE_HEADERS = false;

final AtomicLong cacheHits = new AtomicLong();
final AtomicLong cacheMisses = new AtomicLong();
final AtomicLong cacheUpdates = new AtomicLong();
Expand Down Expand Up @@ -98,7 +96,6 @@ public class CachingExecBase {
this.cacheConfig.getMaxObjectSize(),
this.cacheConfig.isSharedCache(),
this.cacheConfig.isNeverCacheHTTP10ResponsesWithQuery(),
this.cacheConfig.is303CachingEnabled(),
this.cacheConfig.isNeverCacheHTTP11ResponsesWithQuery(),
this.cacheConfig.isStaleIfErrorEnabled());
}
Expand Down Expand Up @@ -268,16 +265,6 @@ void setResponseStatus(final HttpContext context, final CacheResponseStatus valu
}
}

/**
* Reports whether this {@code CachingHttpClient} implementation
* supports byte-range requests as specified by the {@code Range}
* and {@code Content-Range} headers.
* @return {@code true} if byte-range requests are supported
*/
boolean supportsRangeAndContentRangeHeaders() {
return SUPPORTS_RANGE_AND_CONTENT_RANGE_HEADERS;
}

Instant getCurrentDate() {
return Instant.now();
}
Expand All @@ -299,7 +286,7 @@ boolean revalidationResponseIsTooOld(final HttpResponse backendResponse, final H
// either backend response or cached entry did not have a valid
// Date header, so we can't tell if they are out of order
// according to the origin clock; thus we can skip the
// unconditional retry recommended in 13.2.6 of RFC 2616.
// unconditional retry.
return DateSupport.isBefore(backendResponse, cacheEntry, HttpHeaders.DATE);
}

Expand Down
66 changes: 16 additions & 50 deletions ...ent5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,6 @@
import java.time.Duration;
import java.time.Instant;
import java.time.format.DateTimeFormatter;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;

Expand Down Expand Up @@ -71,20 +68,12 @@ class ResponseCachingPolicy {
*/
private static final DateTimeFormatter FORMATTER = DateTimeFormatter.RFC_1123_DATE_TIME;

private final static Set<Integer> CACHEABLE_STATUS_CODES =
new HashSet<>(Arrays.asList(HttpStatus.SC_OK,
HttpStatus.SC_NON_AUTHORITATIVE_INFORMATION,
HttpStatus.SC_MULTIPLE_CHOICES,
HttpStatus.SC_MOVED_PERMANENTLY,
HttpStatus.SC_GONE));

private static final Logger LOG = LoggerFactory.getLogger(ResponseCachingPolicy.class);

private final long maxObjectSizeBytes;
private final boolean sharedCache;
private final boolean neverCache1_0ResponsesWithQueryString;
private final boolean neverCache1_1ResponsesWithQueryString;
private final Set<Integer> uncacheableStatusCodes;

/**
* A flag indicating whether serving stale cache entries is allowed when an error occurs
Expand All @@ -94,32 +83,6 @@ class ResponseCachingPolicy {
*/
private final boolean staleIfErrorEnabled;

/**
* Define a cache policy that limits the size of things that should be stored
* in the cache to a maximum of {@link HttpResponse} bytes in size.
*
* @param maxObjectSizeBytes the size to limit items into the cache
* @param sharedCache whether to behave as a shared cache (true) or a
* non-shared/private cache (false)
* @param neverCache1_0ResponsesWithQueryString true to never cache HTTP 1.0 responses with a query string, false
* to cache if explicit cache headers are found.
* @param allow303Caching if this policy is permitted to cache 303 response
* @param neverCache1_1ResponsesWithQueryString {@code true} to never cache HTTP 1.1 responses with a query string,
* {@code false} to cache if explicit cache headers are found.
*/
public ResponseCachingPolicy(final long maxObjectSizeBytes,
final boolean sharedCache,
final boolean neverCache1_0ResponsesWithQueryString,
final boolean allow303Caching,
final boolean neverCache1_1ResponsesWithQueryString) {
this(maxObjectSizeBytes,
sharedCache,
neverCache1_0ResponsesWithQueryString,
allow303Caching,
neverCache1_1ResponsesWithQueryString,
false);
}

/**
* Constructs a new ResponseCachingPolicy with the specified cache policy settings and stale-if-error support.
*
Expand All @@ -129,8 +92,6 @@ public ResponseCachingPolicy(final long maxObjectSizeBytes,
* non-shared/private cache (false)
* @param neverCache1_0ResponsesWithQueryString {@code true} to never cache HTTP 1.0 responses with a query string,
* {@code false} to cache if explicit cache headers are found.
* @param allow303Caching {@code true} if this policy is permitted to cache 303 responses,
* {@code false} otherwise
* @param neverCache1_1ResponsesWithQueryString {@code true} to never cache HTTP 1.1 responses with a query string,
* {@code false} to cache if explicit cache headers are found.
* @param staleIfErrorEnabled {@code true} to enable the stale-if-error cache directive, which
Expand All @@ -141,18 +102,12 @@ public ResponseCachingPolicy(final long maxObjectSizeBytes,
public ResponseCachingPolicy(final long maxObjectSizeBytes,
final boolean sharedCache,
final boolean neverCache1_0ResponsesWithQueryString,
final boolean allow303Caching,
final boolean neverCache1_1ResponsesWithQueryString,
final boolean staleIfErrorEnabled) {
this.maxObjectSizeBytes = maxObjectSizeBytes;
this.sharedCache = sharedCache;
this.neverCache1_0ResponsesWithQueryString = neverCache1_0ResponsesWithQueryString;
this.neverCache1_1ResponsesWithQueryString = neverCache1_1ResponsesWithQueryString;
if (allow303Caching) {
uncacheableStatusCodes = new HashSet<>(Collections.singletonList(HttpStatus.SC_PARTIAL_CONTENT));
} else {
uncacheableStatusCodes = new HashSet<>(Arrays.asList(HttpStatus.SC_PARTIAL_CONTENT, HttpStatus.SC_SEE_OTHER));
}
this.staleIfErrorEnabled = staleIfErrorEnabled;
}

Expand All @@ -172,15 +127,15 @@ public boolean isResponseCacheable(final ResponseCacheControl cacheControl, fina
}

final int status = response.getCode();
if (CACHEABLE_STATUS_CODES.contains(status)) {
if (isKnownCacheableStatusCode(status)) {
// these response codes MAY be cached
cacheable = true;
} else if (uncacheableStatusCodes.contains(status)) {
} else if (isKnownNonCacheableStatusCode(status)) {
if (LOG.isDebugEnabled()) {
LOG.debug("{} response is not cacheable", status);
}
return false;
} else if (unknownStatusCode(status)) {
} else if (isUnknownStatusCode(status)) {
// a response with an unknown status code MUST NOT be
// cached
if (LOG.isDebugEnabled()) {
Expand Down Expand Up @@ -247,7 +202,19 @@ public boolean isResponseCacheable(final ResponseCacheControl cacheControl, fina
return cacheable || isExplicitlyCacheable(cacheControl, response);
}

private boolean unknownStatusCode(final int status) {
private static boolean isKnownCacheableStatusCode(final int status) {
return status == HttpStatus.SC_OK ||
status == HttpStatus.SC_NON_AUTHORITATIVE_INFORMATION ||
status == HttpStatus.SC_MULTIPLE_CHOICES ||
status == HttpStatus.SC_MOVED_PERMANENTLY ||
status == HttpStatus.SC_GONE;
}

private static boolean isKnownNonCacheableStatusCode(final int status) {
return status == HttpStatus.SC_PARTIAL_CONTENT;
}

private static boolean isUnknownStatusCode(final int status) {
if (status >= 100 && status <= 101) {
return false;
}
Expand Down Expand Up @@ -507,7 +474,6 @@ boolean responseContainsNoCacheDirective(final ResponseCacheControl responseCach
}

/**
* This method checks if a given HTTP status code is understood according to RFC 7231.
* Understood status codes include:
* - All 2xx (Successful) status codes (200-299)
* - All 3xx (Redirection) status codes (300-399)
Expand Down
Loading