Looking at existing code, I noticed that with no-cache we store an entry in
Cache for which CacheManager#inCache will return false .
I don't understand why we just skip the entry, currently we use on entry in
map for nothing.
So reading what you suggest + this I propose we change to the following:
- We test no-cache or no-store and if true we just return
- we remove check on (cacheControl.contains("public") ||
cacheControl.contains("private"))
Agree ?
On Sun, Jul 8, 2012 at 3:29 PM, sebb <[email protected]> wrote:
> On 8 July 2012 14:24, Philippe Mouawad <[email protected]> wrote:
> > but if we have this:
> > Cache-Control=no-cache, max-age=10.
> >
> > If we don't check we will cache which is wrong right ?
> > This header is wrong but I have already seen this on tests I did.
> >
> > Or I am misunderstanding ?
>
> I don't have the source to hand at present, but we should not cache at
> all if Cache-Control=no-cache; the max-age is then not relevant.
>
> > Regards
> > Philippe
> >
> > On Sun, Jul 8, 2012 at 3:19 PM, sebb <[email protected]> wrote:
> >
> >> On 8 July 2012 14:07, Philippe Mouawad <[email protected]>
> wrote:
> >> > Can't it also be no-cache ? no-store ?
> >> > If we don't check it , we could store in cache if server returns
> invalid
> >> > headers no ?
> >>
> >> What do we do if we don't check MaxAge?
> >>
> >> I don't think we should be more restrictive just because we are also
> >> checking the age.
> >>
> >> >
> >> > Thansk for looking at 53365.
> >> > Regards
> >> > Philippe
> >> >
> >> > On Sun, Jul 8, 2012 at 3:01 PM, sebb <[email protected]> wrote:
> >> >
> >> >> On 8 July 2012 10:40, <[email protected]> wrote:
> >> >> > Author: pmouawad
> >> >> > Date: Sun Jul 8 09:40:51 2012
> >> >> > New Revision: 1358710
> >> >> >
> >> >> > URL: http://svn.apache.org/viewvc?rev=1358710&view=rev
> >> >> > Log:
> >> >> > Bug 53521 - Cache Manager should cache content with
> >> Cache-control=private
> >> >> > Bugzilla Id: 53521
> >> >> >
> >> >> > Modified:
> >> >> >
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java
> >> >> >
> >> >>
> >>
> jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java
> >> >> > jmeter/trunk/xdocs/changes.xml
> >> >> >
> >> >> > Modified:
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java
> >> >> > URL:
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java?rev=1358710&r1=1358709&r2=1358710&view=diff
> >> >> >
> >> >>
> >>
> ==============================================================================
> >> >> > ---
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java
> >> >> (original)
> >> >> > +++
> >> >>
> >>
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java
> >> >> Sun Jul 8 09:40:51 2012
> >> >> > @@ -161,7 +161,7 @@ public class CacheManager extends Config
> >> >> > if (useExpires) {// Check that we are processing
> >> >> Expires/CacheControl
> >> >> > final String MAX_AGE = "max-age=";
> >> >> > // TODO - check for other CacheControl attributes?
> >> >> > - if (cacheControl != null &&
> >> cacheControl.contains("public")
> >> >> && cacheControl.contains(MAX_AGE)) {
> >> >> > + if (cacheControl != null &&
> >> >> (cacheControl.contains("public") ||
> cacheControl.contains("private")) &&
> >> >> cacheControl.contains(MAX_AGE)) {
> >> >>
> >> >> Not sure this is the correct fix.
> >> >> Do we really care if the string "public" or "private" is present so
> >> >> long as there is a MAX_AGE entry?
> >> >> We could just drop the check for "public" instead.
> >> >>
> >> >> > long maxAgeInSecs = Long.parseLong(
> >> >> >
> >> >>
> cacheControl.substring(cacheControl.indexOf(MAX_AGE)+MAX_AGE.length())
> >> >> > .split("[, ]")[0] // Bug 51932 - allow
> >> for
> >> >> optional trailing attributes
> >> >> >
> >> >> > Modified:
> >> >>
> >>
> jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java
> >> >> > URL:
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java?rev=1358710&r1=1358709&r2=1358710&view=diff
> >> >> >
> >> >>
> >>
> ==============================================================================
> >> >> > ---
> >> >>
> >>
> jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java
> >> >> (original)
> >> >> > +++
> >> >>
> >>
> jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java
> >> >> Sun Jul 8 09:40:51 2012
> >> >> > @@ -238,7 +238,30 @@ public class TestCacheManager extends JM
> >> >> > assertNotNull("Should find
> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
> >> >> > assertTrue("Should find valid
> >> >> entry",this.cacheManager.inCache(url));
> >> >> > }
> >> >> > +
> >> >> > + public void testPrivateCacheHttpClient() throws Exception{
> >> >> > + this.cacheManager.setUseExpires(true);
> >> >> > + this.cacheManager.testIterationStart(null);
> >> >> > + assertNull("Should not find
> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
> >> >> > + assertFalse("Should not find valid
> >> >> entry",this.cacheManager.inCache(url));
> >> >> > + ((HttpMethodStub)httpMethod).expires=makeDate(new
> >> >> Date(System.currentTimeMillis()));
> >> >> > + ((HttpMethodStub)httpMethod).cacheControl="private,
> >> max-age=10";
> >> >> > + this.cacheManager.saveDetails(httpMethod, sampleResultOK);
> >> >> > + assertNotNull("Should find
> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
> >> >> > + assertTrue("Should find valid
> >> >> entry",this.cacheManager.inCache(url));
> >> >> > + }
> >> >> >
> >> >> > + public void testNoCacheHttpClient() throws Exception{
> >> >> > + this.cacheManager.setUseExpires(true);
> >> >> > + this.cacheManager.testIterationStart(null);
> >> >> > + assertNull("Should not find
> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
> >> >> > + assertFalse("Should not find valid
> >> >> entry",this.cacheManager.inCache(url));
> >> >> > + ((HttpMethodStub)httpMethod).cacheControl="no-cache";
> >> >> > + this.cacheManager.saveDetails(httpMethod, sampleResultOK);
> >> >> > + assertNotNull("Should find
> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
> >> >> > + assertFalse("Should not find valid
> >> >> entry",this.cacheManager.inCache(url));
> >> >> > + }
> >> >> > +
> >> >> > public void testCacheHttpClientBug51932() throws Exception{
> >> >> > this.cacheManager.setUseExpires(true);
> >> >> > this.cacheManager.testIterationStart(null);
> >> >> >
> >> >> > Modified: jmeter/trunk/xdocs/changes.xml
> >> >> > URL:
> >> >>
> >>
> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1358710&r1=1358709&r2=1358710&view=diff
> >> >> >
> >> >>
> >>
> ==============================================================================
> >> >> > --- jmeter/trunk/xdocs/changes.xml (original)
> >> >> > +++ jmeter/trunk/xdocs/changes.xml Sun Jul 8 09:40:51 2012
> >> >> > @@ -59,6 +59,7 @@ or a Debug Sampler with all fields set t
> >> >> >
> >> >> > <h3>HTTP Samplers and Proxy</h3>
> >> >> > <ul>
> >> >> > +<li><bugzilla>53521</bugzilla> - Cache Manager should cache
> content
> >> >> with Cache-control=private</li>
> >> >> > </ul>
> >> >> >
> >> >> > <h3>Other Samplers</h3>
> >> >> >
> >> >> >
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > Cordialement.
> >> > Philippe Mouawad.
> >>
> >
> >
> >
> > --
> > Cordialement.
> > Philippe Mouawad.
>
--
Cordialement.
Philippe Mouawad.
