aliostad / CacheCow

An implementation of HTTP Caching in .NET Core and 4.5.2+ for both the client and the server
MIT License
847 stars 172 forks source link

CacheCow.Client doesn't support no-cache header when specified by the Request #259

Closed BrettJaner closed 3 years ago

BrettJaner commented 3 years ago

When the no-cache header is included in the request, CacheCow.Client still serves the response from cache. The expected behavior should be that the cached resource is revalidated with the origin server. Here's sample repro:

using System;
using System.Net.Http;
using System.Net.Http.Headers;

using CacheCow.Client;

namespace CacheCow.BugRepro
{
    class Program
    {
        static void Main(string[] args)
        {
            using (var client = new HttpClient(
                new CachingHandler
                    {
                        InnerHandler = new HttpClientHandler()
                    })
            {
                BaseAddress = new Uri("https://chrome-caching-bug-repro.azurewebsites.net")
            })
            {
                var response = client.GetAsync("my-number").Result;
                Console.WriteLine($"----- GET /my-number -----");
                Console.WriteLine($"{response.Headers}");

                response = client.GetAsync("my-number").Result;
                Console.WriteLine($"----- GET /my-number -----");
                Console.WriteLine($"{response.Headers}");

                var request = new HttpRequestMessage(HttpMethod.Get, "my-number");
                request.Headers.CacheControl = new CacheControlHeaderValue { NoCache = true };
                response = client.SendAsync(request).Result;
                Console.WriteLine($"----- GET /my-number (with no-cache) -----");
                Console.WriteLine($"{response.Headers}");
            }
        }
    }
}

Output

----- GET /my-number ----- Cache-Control: public, max-age=3600 ETag: "tlifxqsNyCzxIJnRwtQKuZToQQw" Date: Mon, 26 Apr 2021 21:17:20 GMT x-cachecow-client: 2.8.1.0;did-not-exist=true

----- GET /my-number ----- Cache-Control: public, max-age=3600 ETag: "tlifxqsNyCzxIJnRwtQKuZToQQw" Date: Mon, 26 Apr 2021 21:17:20 GMT x-cachecow-client: 2.8.1.0;did-not-exist=false;retrieved-from-cache=true

----- GET /my-number (with no-cache) ----- Cache-Control: public, max-age=3600 ETag: "tlifxqsNyCzxIJnRwtQKuZToQQw" Date: Mon, 26 Apr 2021 21:17:20 GMT x-cachecow-client: 2.8.1.0;did-not-exist=false;retrieved-from-cache=true

BrettJaner commented 3 years ago

Hey @aliostad, would you like me to take a stab at the PR for this? I was thinking we'd just need to modify the CachingHandler.ResponseValidator to return ResponseValidationResult.MustRevalidate when the no-cache header is provided in the Request?

aliostad commented 3 years ago

Sure, by all means. Only that if you could create a feature branch so I could bring to the branch and then merge. I hate master merges as sometimes I had backout stuff.

BrettJaner commented 3 years ago

So do you need to create me a feature branch for which I will submit my PR against? I guess I'm under the impression that it shouldn't matter what branch I develop on in my fork and I don't have access to create a branch in your repo. Please correct me if I'm wrong.

aliostad commented 3 years ago

As long as it is not the master, please.

aliostad commented 3 years ago

Your fix was published in version 2.8.2. Thanks for your efforts and spotting his bug.

BrettJaner commented 3 years ago

No problem. Thank you for your guidance in my HTTP caching exploration.