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

Support for "cache-control:no-cache" in request #196

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Support for "cache-control:no-cache" in request #196

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0efafcb
1) Added Support for "cache-control:no-cache" in request
Jan 28, 2017
95975f6
1) Bypass cache when no-cache present in request headers
Apr 8, 2017
3d53cbe
Reverted ClientTimeSpan = 50, ServerTimeSpan = 50 in unit test
Apr 8, 2017
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
39 changes: 26 additions & 13 deletions src/WebApi.OutputCache.V2/CacheOutputAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public int SharedTimeSpan
get // required for property visibility
{
if (!_sharedTimeSpan.HasValue)
throw new Exception("should not be called without value set");
throw new Exception("should not be called without value set");
return _sharedTimeSpan.Value;
}
set { _sharedTimeSpan = value; }
Expand All @@ -80,7 +80,7 @@ public int SharedTimeSpan
/// Class used to generate caching keys
/// </summary>
public Type CacheKeyGenerator { get; set; }

// cache repository
private IApiOutputCache _webApiCache;

Expand All @@ -106,7 +106,7 @@ protected virtual bool IsCachingAllowed(HttpActionContext actionContext, bool an
return false;
}

return actionContext.Request.Method == HttpMethod.Get;
return actionContext.Request.Method == HttpMethod.Get;
}

protected virtual void EnsureCacheTimeQuery()
Expand Down Expand Up @@ -156,12 +156,18 @@ protected virtual MediaTypeHeaderValue GetExpectedMediaType(HttpConfiguration co
return responseMediaType;
}

private bool IsNoCacheHeaderInRequest(HttpActionContext actionContext)
{
var cacheControl = actionContext.Request.Headers.CacheControl;
return cacheControl != null && cacheControl.NoCache;
}

public override void OnActionExecuting(HttpActionContext actionContext)
{
if (actionContext == null) throw new ArgumentNullException("actionContext");

if (!IsCachingAllowed(actionContext, AnonymousOnly)) return;

var config = actionContext.Request.GetConfiguration();

EnsureCacheTimeQuery();
Expand All @@ -175,6 +181,8 @@ public override void OnActionExecuting(HttpActionContext actionContext)

if (!_webApiCache.Contains(cachekey)) return;

if (IsNoCacheHeaderInRequest(actionContext)) return;
Copy link
Owner

Choose a reason for hiding this comment

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

can you move this up? there is no need to go through cache key generator if this is the case.
I'd put it right after IsCachingAllowed()


if (actionContext.Request.Headers.IfNoneMatch != null)
{
var etag = _webApiCache.Get<string>(cachekey + Constants.EtagKey);
Expand Down Expand Up @@ -223,6 +231,11 @@ public override async Task OnActionExecutedAsync(HttpActionExecutedContext actio
var responseMediaType = actionExecutedContext.Request.Properties[CurrentRequestMediaType] as MediaTypeHeaderValue ?? GetExpectedMediaType(httpConfig, actionExecutedContext.ActionContext);
var cachekey = cacheKeyGenerator.MakeCacheKey(actionExecutedContext.ActionContext, responseMediaType, ExcludeQueryStringFromCacheKey);

if (IsNoCacheHeaderInRequest(actionExecutedContext.ActionContext))
Copy link
Owner

Choose a reason for hiding this comment

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

this shouldn't be here - once a no cache request has been sent by any client, it shouldn't invalidate cache for all clients

{
_webApiCache.Remove(cachekey);
}

if (!string.IsNullOrWhiteSpace(cachekey) && !(_webApiCache.Contains(cachekey)))
{
SetEtag(actionExecutedContext.Response, CreateEtag(actionExecutedContext, cachekey, cacheTime));
Expand All @@ -242,12 +255,12 @@ public override async Task OnActionExecutedAsync(HttpActionExecutedContext actio
_webApiCache.Add(baseKey, string.Empty, cacheTime.AbsoluteExpiration);
_webApiCache.Add(cachekey, content, cacheTime.AbsoluteExpiration, baseKey);
Copy link
Author

Choose a reason for hiding this comment

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

Wouldn't this effectively invalidate the cache for all clients anyways since we are adding(replacing) the cache entry for the same exact cache key that we are explicitly removing in Line 234 ? Since the cache is public and not a per client private cache, do you see any use cases wherein it's harmful to have the cache invalidated for a specific key "globally", if requested so by one client by specifying "cache-control:no-cache"

Copy link
Owner

Choose a reason for hiding this comment

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

so this code would only run if the request wasn't cached in the first place.
I don't think you should invalidate the cache for everyone by any client call, since that would defeat the purpose of caching at all. I could then write a malicious client that just busts all the cache on the server all the time, which is definitely not good

Copy link
Author

Choose a reason for hiding this comment

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

Wouldn't it also run if we are bypassing the cache in 'OnActionExecuting' by checking if the client requested 'no-cache'?
Should we then not cache the fresh response at all for that, specific client call and just return the data we retrieve from executing the requested action and let the existing (stale) cached response be as is, so that it eventually expires per the expiration policy that was set on the action?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I do understand your point, since we already have the response at this point, we might as well use it to update the cache.

My main concern is that, in reality, cache-control headers are not really intended for the server, they are really intended for clients + intermediaries (such as proxies). So they instruct clients how to handle responses in terms of caching, and they instruct proxies how to deal with requests.

Building a convoluted caching logic on the server side based on them is not necessarily a good idea, or I am not sure everyone would be happy to have their cache getting busted at any point by and client from anywhere in the world. If you rely on caching to do performance lift for you, this is a performance black hole.

Maybe a better idea is to have this feature controlled by a flag on the caching attribute, something like [CacheOutput(AllowCacheBypass = true)]

Copy link
Author

Choose a reason for hiding this comment

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

I agree with all your points. When I read the http spec on 'no-cache', it was light on details on whether it is specific to client side only. Although majority of cases are for client side use, there is nothing in the spec that precludes using it on the server.
Perhaps the easiest option then, is to bypass the cache completely and let it expire on it own per the expiration period. I think the additional attribute makes sense and we can use that in combination with the presence or absence of no-cache.sound good?

Copy link
Author

Choose a reason for hiding this comment

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

One other thought is to use something like
[CacheOutput(AllowPrivate=true, AllowCacheBypass=true)] which enables a per client cache if the client passes in a 'cache-control: private' header, we could then allow for using 'no-cache' if the client chooses to refresh their cache, we then invalidate their private cache, invoke the action, cache the new data (as private again) and return back. This way the client cannot affect any other client and a malicious client would constantly be invalidating only their cache at the expense of the server spinning cpu cycles to invalidate. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

@filipw Any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to jump into this discussion without invitation, but I thought on something similar: when a client issues a no-cache is actually saying "bypass whatever you have cached", but not saying that "you should remove your cached data".
I implemented something similar in a project and our approach was much simpler: if the client sent a no-cache it was exactly the same as if the controller said the results shouldn't be cached. And to implement so we overloaded the IsCachingAllowed, adding a condition similar to what's evaluated in @abpatel IsNoCacheHeaderInRequest.
If you like, I can create a PR with that.

Copy link
Author

Choose a reason for hiding this comment

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

The more the merrier. :-)... That's something I've considered as well, but the issue IMHO that you run into is that at what point would the client know NOT to use no-cache anymore? I imagine the client would have to do some sort of time calculation based on max-age to know when the old cached value expires so that it can stop sending no-cache, and reap the benefits of caching instead of passing through the cache everytime



_webApiCache.Add(cachekey + Constants.ContentTypeKey,
contentType,
cacheTime.AbsoluteExpiration, baseKey);


_webApiCache.Add(cachekey + Constants.EtagKey,
etag,
cacheTime.AbsoluteExpiration, baseKey);
Expand All @@ -263,12 +276,12 @@ protected virtual void ApplyCacheHeaders(HttpResponseMessage response, CacheTime
if (cacheTime.ClientTimeSpan > TimeSpan.Zero || MustRevalidate || Private)
{
var cachecontrol = new CacheControlHeaderValue
{
MaxAge = cacheTime.ClientTimeSpan,
SharedMaxAge = cacheTime.SharedTimeSpan,
MustRevalidate = MustRevalidate,
Private = Private
};
{
MaxAge = cacheTime.ClientTimeSpan,
SharedMaxAge = cacheTime.SharedTimeSpan,
MustRevalidate = MustRevalidate,
Private = Private
};

response.Headers.CacheControl = cachecontrol;
}
Expand All @@ -293,4 +306,4 @@ private static void SetEtag(HttpResponseMessage message, string etag)
}
}
}
}
}
42 changes: 21 additions & 21 deletions test/WebApi.OutputCache.V2.Tests/ClientSideTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,21 +84,21 @@ public void maxage_mustrevalidate_headers_correct_with_clienttimeout_zero_with_m
}


[Test]
public void nocache_headers_correct()
{
var client = new HttpClient(_server);
var result = client.GetAsync(_url + "Get_nocache").Result;

Assert.IsTrue(result.Headers.CacheControl.NoCache,
"NoCache in result headers was expected to be true when CacheOutput.NoCache=true.");
Assert.IsTrue(result.Headers.Contains("Pragma"),
"result headers does not contain expected Pragma.");
Assert.IsTrue(result.Headers.GetValues("Pragma").Contains("no-cache"),
"expected no-cache Pragma was not found");
}

[Test]
[Test]
public void nocache_headers_correct()
{
var client = new HttpClient(_server);
var result = client.GetAsync(_url + "Get_nocache").Result;

Assert.IsTrue(result.Headers.CacheControl.NoCache,
"NoCache in result headers was expected to be true when CacheOutput.NoCache=true.");
Assert.IsTrue(result.Headers.Contains("Pragma"),
"result headers does not contain expected Pragma.");
Assert.IsTrue(result.Headers.GetValues("Pragma").Contains("no-cache"),
"expected no-cache Pragma was not found");
}

[Test]
public void maxage_mustrevalidate_true_headers_correct()
{
var client = new HttpClient(_server);
Expand All @@ -122,9 +122,9 @@ public void maxage_private_true_headers_correct()
public void maxage_mustrevalidate_headers_correct_with_cacheuntil()
{
var client = new HttpClient(_server);
var result = client.GetAsync(_url + "Get_until25012015_1700").Result;
var clientTimeSpanSeconds = new SpecificTime(2017, 01, 25, 17, 0, 0).Execute(DateTime.Now).ClientTimeSpan.TotalSeconds;
var resultCacheControlSeconds = ((TimeSpan) result.Headers.CacheControl.MaxAge).TotalSeconds;
var result = client.GetAsync(_url + "Get_until25012020_1700").Result;
var clientTimeSpanSeconds = new SpecificTime(2020, 01, 25, 17, 0, 0).Execute(DateTime.Now).ClientTimeSpan.TotalSeconds;
var resultCacheControlSeconds = ((TimeSpan)result.Headers.CacheControl.MaxAge).TotalSeconds;
Assert.IsTrue(Math.Round(clientTimeSpanSeconds - resultCacheControlSeconds) == 0);
Assert.IsFalse(result.Headers.CacheControl.MustRevalidate);
}
Expand All @@ -135,7 +135,7 @@ public void maxage_mustrevalidate_headers_correct_with_cacheuntil_today()
var client = new HttpClient(_server);
var result = client.GetAsync(_url + "Get_until2355_today").Result;

Assert.IsTrue(Math.Round(new ThisDay(23,55,59).Execute(DateTime.Now).ClientTimeSpan.TotalSeconds - ((TimeSpan)result.Headers.CacheControl.MaxAge).TotalSeconds) == 0);
Assert.IsTrue(Math.Round(new ThisDay(23, 55, 59).Execute(DateTime.Now).ClientTimeSpan.TotalSeconds - ((TimeSpan)result.Headers.CacheControl.MaxAge).TotalSeconds) == 0);
Assert.IsFalse(result.Headers.CacheControl.MustRevalidate);
}

Expand All @@ -145,7 +145,7 @@ public void maxage_mustrevalidate_headers_correct_with_cacheuntil_this_month()
var client = new HttpClient(_server);
var result = client.GetAsync(_url + "Get_until27_thismonth").Result;

Assert.IsTrue(Math.Round(new ThisMonth(27,0,0,0).Execute(DateTime.Now).ClientTimeSpan.TotalSeconds - ((TimeSpan)result.Headers.CacheControl.MaxAge).TotalSeconds) == 0);
Assert.IsTrue(Math.Round(new ThisMonth(27, 0, 0, 0).Execute(DateTime.Now).ClientTimeSpan.TotalSeconds - ((TimeSpan)result.Headers.CacheControl.MaxAge).TotalSeconds) == 0);
Assert.IsFalse(result.Headers.CacheControl.MustRevalidate);
}

Expand Down Expand Up @@ -183,7 +183,7 @@ public void shared_max_age_header_correct()
{
var client = new HttpClient(_server);
var result = client.GetAsync(_url + "Get_c100_s100_sm200").Result;
Assert.AreEqual(result.Headers.CacheControl.SharedMaxAge,TimeSpan.FromSeconds(200));
Assert.AreEqual(result.Headers.CacheControl.SharedMaxAge, TimeSpan.FromSeconds(200));
}

[Test]
Expand Down
16 changes: 15 additions & 1 deletion test/WebApi.OutputCache.V2.Tests/ServerSideTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,26 @@ public void set_cache_to_predefined_value()
_cache.Verify(s => s.Add(It.Is<string>(x => x == "webapi.outputcache.v2.tests.testcontrollers.samplecontroller-get_c100_s100:application/json; charset=utf-8"), It.IsAny<object>(), It.Is<DateTimeOffset>(x => x <= DateTime.Now.AddSeconds(100)), It.Is<string>(x => x == "webapi.outputcache.v2.tests.testcontrollers.samplecontroller-get_c100_s100")), Times.Once());
}

[Test]
public void nocache_in_request_refreshes_cache()
{
var client = new HttpClient(_server);
client.DefaultRequestHeaders.CacheControl =
new CacheControlHeaderValue { NoCache = true };
var result = client.GetAsync(_url + "Get_c100_s100").Result;
_cache.Verify(s => s.Contains(It.Is<string>(x => x == "webapi.outputcache.v2.tests.testcontrollers.samplecontroller-get_c100_s100:application/json; charset=utf-8")), Times.Exactly(2));
_cache.Verify(s => s.Remove(It.Is<string>(x => x == "webapi.outputcache.v2.tests.testcontrollers.samplecontroller-get_c100_s100:application/json; charset=utf-8")), Times.Exactly(1));
_cache.Verify(s => s.Add(It.Is<string>(x => x == "webapi.outputcache.v2.tests.testcontrollers.samplecontroller-get_c100_s100"), It.IsAny<object>(), It.Is<DateTimeOffset>(x => x <= DateTime.Now.AddSeconds(100)), null), Times.Once());
_cache.Verify(s => s.Add(It.Is<string>(x => x == "webapi.outputcache.v2.tests.testcontrollers.samplecontroller-get_c100_s100:application/json; charset=utf-8"), It.IsAny<object>(), It.Is<DateTimeOffset>(x => x <= DateTime.Now.AddSeconds(100)), It.Is<string>(x => x == "webapi.outputcache.v2.tests.testcontrollers.samplecontroller-get_c100_s100")), Times.Once());
}


[Test]
public void set_cache_to_predefined_value_c100_s0()
{
var client = new HttpClient(_server);
var result = client.GetAsync(_url + "Get_c100_s0").Result;

// NOTE: Should we expect the _cache to not be called at all if the ServerTimeSpan is 0?
_cache.Verify(s => s.Contains(It.Is<string>(x => x == "webapi.outputcache.v2.tests.testcontrollers.samplecontroller-get_c100_s0:application/json; charset=utf-8")), Times.Once());
// NOTE: Server timespan is 0, so there should not have been any Add at all.
Expand Down
28 changes: 15 additions & 13 deletions test/WebApi.OutputCache.V2.Tests/TestControllers/SampleController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using System.Net;
using System.Net.Http;
using System.Web.Http;
using System.Web.Http.Controllers;
using System.Web.Http.Filters;
using WebApi.OutputCache.V2.TimeAttributes;

namespace WebApi.OutputCache.V2.Tests.TestControllers
Expand All @@ -25,17 +27,17 @@ public string Get_c0_s100()
return "test";
}

[CacheOutput(NoCache=true)]
[CacheOutput(NoCache = true)]
public string Get_nocache()
{
return "test";
}

[CacheOutput(ClientTimeSpan = 0, ServerTimeSpan = 100, MustRevalidate = true)]
public string Get_c0_s100_mustR()
{
return "test";
}
[CacheOutput(ClientTimeSpan = 0, ServerTimeSpan = 100, MustRevalidate = true)]
public string Get_c0_s100_mustR()
{
return "test";
}

[CacheOutput(ClientTimeSpan = 50, MustRevalidate = true)]
public string Get_c50_mustR()
Expand Down Expand Up @@ -64,7 +66,7 @@ public string Get_s50_exclude_fakecallback(int? id = null, string callback = nul
[CacheOutput(ServerTimeSpan = 50, ExcludeQueryStringFromCacheKey = false)]
public string Get_s50_exclude_false(int id)
{
return "test"+id;
return "test" + id;
}

[CacheOutput(ServerTimeSpan = 50, ExcludeQueryStringFromCacheKey = true)]
Expand All @@ -73,13 +75,13 @@ public string Get_s50_exclude_true(int id)
return "test" + id;
}

[CacheOutputUntil(2017,01,25,17,00)]
public string Get_until25012015_1700()
[CacheOutputUntil(2020, 01, 25, 17, 00)]
public string Get_until25012020_1700()
{
return "test";
}

[CacheOutputUntilToday(23,55)]
[CacheOutputUntilToday(23, 55)]
public string Get_until2355_today()
{
return "value";
Expand All @@ -91,7 +93,7 @@ public string Get_until27_thismonth()
return "value";
}

[CacheOutputUntilThisYear(7,31)]
[CacheOutputUntilThisYear(7, 31)]
public string Get_until731_thisyear()
{
return "value";
Expand Down Expand Up @@ -125,7 +127,7 @@ public string Get_request_exception_noCache()
[CacheOutput(ClientTimeSpan = 50, ServerTimeSpan = 50)]
public string Get_request_httpResponseException_noCache()
{
throw new HttpResponseException(new HttpResponseMessage(HttpStatusCode.Conflict){ReasonPhrase = "Fault shouldn't cache"});
throw new HttpResponseException(new HttpResponseMessage(HttpStatusCode.Conflict) { ReasonPhrase = "Fault shouldn't cache" });
}

[CacheOutput(ClientTimeSpan = 50, ServerTimeSpan = 50)]
Expand Down Expand Up @@ -160,4 +162,4 @@ public string Get_c100_s100_sm200()
}

}
}
}
10 changes: 9 additions & 1 deletion test/WebApi.OutputCache.V2.Tests/packages.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,13 @@
<package id="Microsoft.AspNet.WebApi.Core" version="5.2.3" targetFramework="net45" userInstalled="true" />
<package id="Moq" version="4.0.10827" targetFramework="net45" userInstalled="true" />
<package id="Newtonsoft.Json" version="6.0.4" targetFramework="net45" userInstalled="true" />
<package id="NUnit" version="2.6.2" targetFramework="net45" userInstalled="true" />
<package id="NUnit" version="3.6.0" targetFramework="net45" userInstalled="true" />
<package id="NUnit.Console" version="3.6.0" targetFramework="net45" />
<package id="NUnit.ConsoleRunner" version="3.6.0" targetFramework="net45" />
<package id="NUnit.Extension.NUnitProjectLoader" version="3.5.0" targetFramework="net45" />
<package id="NUnit.Extension.NUnitV2Driver" version="3.6.0" targetFramework="net45" />
<package id="NUnit.Extension.NUnitV2ResultWriter" version="3.5.0" targetFramework="net45" />
<package id="NUnit.Extension.TeamCityEventListener" version="1.0.2" targetFramework="net45" />
<package id="NUnit.Extension.VSProjectLoader" version="3.5.0" targetFramework="net45" />
<package id="NUnit3TestAdapter" version="3.6.0" targetFramework="net45" />
</packages>