KevinDockx / HttpCacheHeaders

ASP.NET Core middleware that adds HttpCache headers to responses (Cache-Control, Expires, ETag, Last-Modified), and implements cache expiration & validation models
MIT License
266 stars 56 forks source link

Disable caching for error responses #108

Closed DerStimmler closed 1 year ago

DerStimmler commented 1 year ago

Hi, I noticed that the cache headers are added to all responses, even if the status code of the response is in the 4xx range. I don't want errors to be cached. Is there a way to disable caching for error responses?

KevinDockx commented 1 year ago

Hi,

in principle it's up to the cache to decide whether to cache something with a specific status code. This middleware is not a cache, it simply adds the required headers & implements validation/expiration models so a cache can integrate with it. That being said: it does make sense to allow choosing whether to add the cache headers to non-successful responses. I've marked this as a feature request & I'll have a look :)

DerStimmler commented 1 year ago

After #110 is merged and we now can provide options to the middleware to ignore caching globally, we could add an option to ignore caching for error responses.

I have three ideas for this option.

  1. It's just a bool value. If set to true the middleware is ignoring all responses with status code equal or higher than 400. E.g.: options.IgnoreErrorResponses = true
  2. It's an array of int where you can provide all status codes you want to ignore one by one. E.g.: options.IgnoreStatusCodes = new[] {400, 401, 403, 500}
  3. It's an array of Range where you can provide ranges of status codes you want to ignore. E.g.: options.IgnoreStatusCodes = new[] {400..403, 500..504}

I like idea 3 the most because it's more flexible than idea 1 and at the same time more comfortable for the user than idea 2.

@KevinDockx what do you think?

If you're fine with that, I could provide a PR.

KevinDockx commented 1 year ago

Good idea! I'd go for the int version of IgnoreStatuscodes. The Range idea is indeed more comfortable, but if I'm not mistaken it doesn't allow enum values to be put in - and I'd expect people wanting to use HttpStatusCode.XXX instead of "hardcoded" int values.

Also, to make life a bit easier, it would be nice if we could provide some default values for "IgnoreStatusCodes", eg: options.IgnoreStatusCodes = HttpStatusCodes.AllErrors (which would translate to an int[] with all 400-level codes), HttpStatusCodes.AllFaults (500 range), HttpStatusCodes.AllErrorsAndFaults (400 & 500 range). These would simply be some static arrays that we provide (namespace / exact naming can be different). What do you think?

DerStimmler commented 1 year ago

You're right. Ranges don't allow enums, especially in this syntax.

The problem with an int array is that enums still have to be cast to int. Which is a bit ugly in my opinion. Doesn't work: int[] x = new int[] { HttpStatusCode.NotFound }; Works: int[] x = new int[] { (int) HttpStatusCode.NotFound };

To solve this, we'd have to use an array of HttpStatusCode. But the problem here is that only Status Codes from the RFC 2616 specification are present in this enum. You couldn't use 425 Too Early from the RFC 8470 for example.

I think, that in this case, the flexibility is more important than eventually ugly code. So I'm in with your suggestion.

We use an int array where people can add their status codes one by one (e.g. as native int or cast HttpStatusCode). For convenience, we provide some static arrays with predefined collections of status codes.

I'd name them HttpStatusCodes.ClientErrors (4XX range), HttpStatusCodes.ServerErrors (5XX range) and HttpStatusCodes.AllErrors (previous ranges combined).

What should be the default value for this option? I would say that by default no Status Code should be ignored.

I will create a PR in the next few days.

KevinDockx commented 1 year ago

Sounds good, thanks for helping out!