diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index 2bedf4df1..acb323076 100644 --- a/RELEASE_NOTES.txt +++ b/RELEASE_NOTES.txt @@ -1,6 +1,8 @@ Changes since release 4.3 BETA2 ------------------- +* [HTTPCLIENT-1373] OPTIONS and TRACE should not invalidate cache + Contributed by James Leigh * [HTTPCLIENT-1383] HttpClient enters an infinite loop during NTLM authentication if the opposite endpoint keeps responding with a type 2 NTLM response after type 3 MTLM message has already been diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/BasicHttpCache.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/BasicHttpCache.java index 1a2fd7674..aa8270ef0 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/BasicHttpCache.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/BasicHttpCache.java @@ -27,9 +27,12 @@ package org.apache.http.impl.client.cache; import java.io.IOException; +import java.util.Arrays; import java.util.Date; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -52,6 +55,10 @@ import org.apache.http.util.EntityUtils; class BasicHttpCache implements HttpCache { + private static final Set safeRequestMethods = new HashSet( + Arrays.asList(HeaderConstants.HEAD_METHOD, + HeaderConstants.GET_METHOD, HeaderConstants.OPTIONS_METHOD, + HeaderConstants.TRACE_METHOD)); private final CacheKeyGenerator uriExtractor; private final ResourceFactory resourceFactory; @@ -83,12 +90,16 @@ public BasicHttpCache() { public void flushCacheEntriesFor(final HttpHost host, final HttpRequest request) throws IOException { - final String uri = uriExtractor.getURI(host, request); - storage.removeEntry(uri); + if (!safeRequestMethods.contains(request.getRequestLine().getMethod())) { + final String uri = uriExtractor.getURI(host, request); + storage.removeEntry(uri); + } } public void flushInvalidatedCacheEntriesFor(final HttpHost host, final HttpRequest request, final HttpResponse response) { - cacheInvalidator.flushInvalidatedCacheEntries(host, request, response); + if (!safeRequestMethods.contains(request.getRequestLine().getMethod())) { + cacheInvalidator.flushInvalidatedCacheEntries(host, request, response); + } } void storeInCache( diff --git a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheInvalidator.java b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheInvalidator.java index 69021e057..da55f2850 100644 --- a/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheInvalidator.java +++ b/httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheInvalidator.java @@ -202,11 +202,19 @@ public void flushInvalidatedCacheEntries(final HttpHost host, if (reqURL == null) { return; } - final URL canonURL = getContentLocationURL(reqURL, response); - if (canonURL == null) { - return; + final URL contentLocation = getContentLocationURL(reqURL, response); + if (contentLocation != null) { + flushLocationCacheEntry(reqURL, response, contentLocation); } - final String cacheKey = cacheKeyGenerator.canonicalizeUri(canonURL.toString()); + final URL location = getLocationURL(reqURL, response); + if (location != null) { + flushLocationCacheEntry(reqURL, response, location); + } + } + + private void flushLocationCacheEntry(final URL reqURL, + final HttpResponse response, final URL location) { + final String cacheKey = cacheKeyGenerator.canonicalizeUri(location.toString()); final HttpCacheEntry entry = getEntry(cacheKey); if (entry == null) { return; @@ -222,7 +230,7 @@ public void flushInvalidatedCacheEntries(final HttpHost host, return; } - flushUriIfSameHost(reqURL, canonURL); + flushUriIfSameHost(reqURL, location); } private URL getContentLocationURL(final URL reqURL, final HttpResponse response) { @@ -238,6 +246,19 @@ private URL getContentLocationURL(final URL reqURL, final HttpResponse response) return getRelativeURL(reqURL, contentLocation); } + private URL getLocationURL(final URL reqURL, final HttpResponse response) { + final Header clHeader = response.getFirstHeader("Location"); + if (clHeader == null) { + return null; + } + final String location = clHeader.getValue(); + final URL canonURL = getAbsoluteURL(location); + if (canonURL != null) { + return canonURL; + } + return getRelativeURL(reqURL, location); + } + private boolean responseAndEntryEtagsDiffer(final HttpResponse response, final HttpCacheEntry entry) { final Header entryEtag = entry.getFirstHeader(HeaderConstants.ETAG); diff --git a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestBasicHttpCache.java b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestBasicHttpCache.java index 3d4b9df93..a24f71811 100644 --- a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestBasicHttpCache.java +++ b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestBasicHttpCache.java @@ -48,13 +48,19 @@ import org.apache.http.HttpResponse; import org.apache.http.HttpStatus; import org.apache.http.HttpVersion; +import org.apache.http.client.cache.HeaderConstants; import org.apache.http.client.cache.HttpCacheEntry; import org.apache.http.client.cache.Resource; import org.apache.http.client.methods.HttpDelete; import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpHead; +import org.apache.http.client.methods.HttpOptions; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.client.methods.HttpTrace; +import org.apache.http.client.utils.DateUtils; import org.apache.http.entity.BasicHttpEntity; import org.apache.http.entity.ByteArrayEntity; -import org.apache.http.client.utils.DateUtils; +import org.apache.http.message.BasicHeader; import org.apache.http.message.BasicHttpResponse; import org.apache.http.util.EntityUtils; import org.junit.Assert; @@ -72,6 +78,105 @@ public void setUp() throws Exception { impl = new BasicHttpCache(new HeapResourceFactory(), backing, CacheConfig.DEFAULT); } + @Test + public void testDoNotFlushCacheEntriesOnGet() throws Exception { + final HttpHost host = new HttpHost("foo.example.com"); + final HttpRequest req = new HttpGet("/bar"); + final String key = (new CacheKeyGenerator()).getURI(host, req); + final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(); + + backing.map.put(key, entry); + + impl.flushCacheEntriesFor(host, req); + + assertEquals(entry, backing.map.get(key)); + } + + @Test + public void testDoNotFlushCacheEntriesOnHead() throws Exception { + final HttpHost host = new HttpHost("foo.example.com"); + final HttpRequest req = new HttpHead("/bar"); + final String key = (new CacheKeyGenerator()).getURI(host, req); + final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(); + + backing.map.put(key, entry); + + impl.flushCacheEntriesFor(host, req); + + assertEquals(entry, backing.map.get(key)); + } + + @Test + public void testDoNotFlushCacheEntriesOnOptions() throws Exception { + final HttpHost host = new HttpHost("foo.example.com"); + final HttpRequest req = new HttpOptions("/bar"); + final String key = (new CacheKeyGenerator()).getURI(host, req); + final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(); + + backing.map.put(key, entry); + + impl.flushCacheEntriesFor(host, req); + + assertEquals(entry, backing.map.get(key)); + } + + @Test + public void testDoNotFlushCacheEntriesOnTrace() throws Exception { + final HttpHost host = new HttpHost("foo.example.com"); + final HttpRequest req = new HttpTrace("/bar"); + final String key = (new CacheKeyGenerator()).getURI(host, req); + final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(); + + backing.map.put(key, entry); + + impl.flushCacheEntriesFor(host, req); + + assertEquals(entry, backing.map.get(key)); + } + + @Test + public void testFlushContentLocationEntryIfUnSafeRequest() + throws Exception { + final HttpHost host = new HttpHost("foo.example.com"); + final HttpRequest req = new HttpPost("/foo"); + final HttpResponse resp = HttpTestUtils.make200Response(); + resp.setHeader("Content-Location", "/bar"); + resp.setHeader(HeaderConstants.ETAG, "\"etag\""); + final String key = (new CacheKeyGenerator()).getURI(host, new HttpGet("/bar")); + + final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(new Header[] { + new BasicHeader("Date", DateUtils.formatDate(new Date())), + new BasicHeader("ETag", "\"old-etag\"") + }); + + backing.map.put(key, entry); + + impl.flushInvalidatedCacheEntriesFor(host, req, resp); + + assertNull(backing.map.get(key)); + } + + @Test + public void testDoNotFlushContentLocationEntryIfSafeRequest() + throws Exception { + final HttpHost host = new HttpHost("foo.example.com"); + final HttpRequest req = new HttpGet("/foo"); + final HttpResponse resp = HttpTestUtils.make200Response(); + resp.setHeader("Content-Location", "/bar"); + final String key = (new CacheKeyGenerator()).getURI(host, new HttpGet("/bar")); + + final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(new Header[] { + new BasicHeader("Date", DateUtils.formatDate(new Date())), + new BasicHeader("ETag", "\"old-etag\"") + }); + + backing.map.put(key, entry); + + impl.flushInvalidatedCacheEntriesFor(host, req, resp); + + assertEquals(entry, backing.map.get(key)); + } + @Test public void testCanFlushCacheEntriesAtUri() throws Exception { final HttpHost host = new HttpHost("foo.example.com"); diff --git a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCacheInvalidator.java b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCacheInvalidator.java index 942fdbf53..f8408b715 100644 --- a/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCacheInvalidator.java +++ b/httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestCacheInvalidator.java @@ -51,6 +51,9 @@ import org.apache.http.message.BasicHttpEntityEnclosingRequest; import org.apache.http.message.BasicHttpRequest; import org.apache.http.message.BasicHttpResponse; +import org.easymock.EasyMock; +import org.easymock.IAnswer; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -297,6 +300,28 @@ public void flushesEntryIfFresherAndSpecifiedByContentLocation() verifyMocks(); } + @Test + public void flushesEntryIfFresherAndSpecifiedByLocation() + throws Exception { + response.setStatusCode(201); + response.setHeader("ETag","\"new-etag\""); + response.setHeader("Date", DateUtils.formatDate(now)); + final String theURI = "http://foo.example.com:80/bar"; + response.setHeader("Location", theURI); + + final HttpCacheEntry entry = HttpTestUtils.makeCacheEntry(new Header[] { + new BasicHeader("Date", DateUtils.formatDate(tenSecondsAgo)), + new BasicHeader("ETag", "\"old-etag\"") + }); + + expect(mockStorage.getEntry(theURI)).andReturn(entry).anyTimes(); + mockStorage.removeEntry(theURI); + + replayMocks(); + impl.flushInvalidatedCacheEntries(host, request, response); + verifyMocks(); + } + @Test public void doesNotFlushEntryForUnsuccessfulResponse() throws Exception { @@ -312,6 +337,13 @@ public void doesNotFlushEntryForUnsuccessfulResponse() }); expect(mockStorage.getEntry(theURI)).andReturn(entry).anyTimes(); + mockStorage.removeEntry(theURI); + EasyMock.expectLastCall().andAnswer(new IAnswer() { + public Void answer() { + Assert.fail(); + return null; + } + }).anyTimes(); replayMocks(); impl.flushInvalidatedCacheEntries(host, request, response); @@ -374,6 +406,13 @@ public void doesNotFlushEntryIfContentLocationFromDifferentHost() }); expect(mockStorage.getEntry(cacheKey)).andReturn(entry).anyTimes(); + mockStorage.removeEntry(cacheKey); + EasyMock.expectLastCall().andAnswer(new IAnswer() { + public Void answer() { + Assert.fail(); + return null; + } + }).anyTimes(); replayMocks(); impl.flushInvalidatedCacheEntries(host, request, response); @@ -396,6 +435,13 @@ public void doesNotFlushEntrySpecifiedByContentLocationIfEtagsMatch() }); expect(mockStorage.getEntry(theURI)).andReturn(entry).anyTimes(); + mockStorage.removeEntry(theURI); + EasyMock.expectLastCall().andAnswer(new IAnswer() { + public Void answer() { + Assert.fail(); + return null; + } + }).anyTimes(); replayMocks(); impl.flushInvalidatedCacheEntries(host, request, response); @@ -416,6 +462,13 @@ public void doesNotFlushEntrySpecifiedByContentLocationIfOlder() }); expect(mockStorage.getEntry(theURI)).andReturn(entry).anyTimes(); + mockStorage.removeEntry(theURI); + EasyMock.expectLastCall().andAnswer(new IAnswer() { + public Void answer() { + Assert.fail(); + return null; + } + }).anyTimes(); replayMocks(); impl.flushInvalidatedCacheEntries(host, request, response); @@ -431,6 +484,13 @@ public void doesNotFlushEntryIfNotInCache() response.setHeader("Content-Location", theURI); expect(mockStorage.getEntry(theURI)).andReturn(null).anyTimes(); + mockStorage.removeEntry(theURI); + EasyMock.expectLastCall().andAnswer(new IAnswer() { + public Void answer() { + Assert.fail(); + return null; + } + }).anyTimes(); replayMocks(); impl.flushInvalidatedCacheEntries(host, request, response); @@ -451,6 +511,13 @@ public void doesNotFlushEntrySpecifiedByContentLocationIfResponseHasNoEtag() }); expect(mockStorage.getEntry(theURI)).andReturn(entry).anyTimes(); + mockStorage.removeEntry(theURI); + EasyMock.expectLastCall().andAnswer(new IAnswer() { + public Void answer() { + Assert.fail(); + return null; + } + }).anyTimes(); replayMocks(); impl.flushInvalidatedCacheEntries(host, request, response);