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
267 stars 56 forks source link

Provide a way to store validator values in a scalable manner #112

Closed SeanFarrow closed 1 month ago

SeanFarrow commented 1 year ago

It would be really nice if there was a way to store validator values in a scalable manner for applications that use 2 or more servers.

I am proposing a new project/NuGet package Marvin.Cache.Headers.DistributedCache that allows users to store the validator values in any of the distributed caches supported my Microsoft..

Would a PR be accepted for this? I definitely have a use-case for this and will need to write the code anyway, so thought it was better to contribute, rather than people needing to duplicate efforts.

KevinDockx commented 1 year ago

Hi Sean,

I think something like that would benefit lots of people, so I'd definitely accept such a PR. I'd slightly adjust the naming though: something like Marvin.Cache.Headers.DistributedStore seems to make more sense, as it would contain a distributed version of the validator value store. In any case, thanks a lot for contributing!

SeanFarrow commented 1 year ago

Hi Kevin,

I completely agree with the name change.

What versions of .Net are you wanting to support with V7.X? In terms of testing, do we just want a set of unit tests that check the cache has been called, or do we want to write a set of integration tests as well (using things like Redis/Docker)? I’m conscious of adding dependencies you might not want.

Thanks, Sean.

From: Kevin Dockx @.> Sent: 31 January 2023 10:49 To: KevinDockx/HttpCacheHeaders @.> Cc: Sean Farrow @.>; Author @.> Subject: Re: [KevinDockx/HttpCacheHeaders] Provide a way to store validator values in a scalable manner (Issue #112)

Hi Sean,

I think something like that would benefit lots of people, so I'd definitely accept such a PR. I'd slightly adjust the naming though: something like Marvin.Cache.Headers.DistributedStore seems to make more sense, as it would contain a distributed version of the validator value store. In any case, thanks a lot for contributing!

— Reply to this email directly, view it on GitHubhttps://github.com/KevinDockx/HttpCacheHeaders/issues/112#issuecomment-1410140830, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7R6UV5G2NJ4JM3A3TTWVDU3PANCNFSM6AAAAAAUMIBAXA. You are receiving this because you authored the thread.Message ID: @.***>

KevinDockx commented 1 year ago

At the very least: the LTS versions that are currently still supported by MS (that's currently only .NET 6), and, preferably, the current version: .NET 7. If Core 3.1 could still be supported that would be great (it only recently got out of support), but it's not a deal breaker. .NET 5 is a "current" version that's been out of support for some time, so it's not that important as far as I'm concerned.

Unit tests are fine by me. No need to test the integration as a whole, I think we should be able to assume that any provider that supports the distributed cache contract has its own set of tests.

SeanFarrow commented 1 year ago

With v7 of the Microsoft.Extensions.Caching.Abstractions NuGet package we can support all the current frameworks.

The only thing that looks hard to do with the current IDistributedCache implementations is return the currently cached keys (without relying on a secondly list/dictionary). Work to resolve this is planned in .Net 8.

From: Kevin Dockx @.> Sent: 31 January 2023 16:59 To: KevinDockx/HttpCacheHeaders @.> Cc: Sean Farrow @.>; Author @.> Subject: Re: [KevinDockx/HttpCacheHeaders] Provide a way to store validator values in a scalable manner (Issue #112)

At the very least: the LTS versions that are currently still supported by MS (that's currently only .NET 6), and, preferably, the current version: .NET 7. If Core 3.1 could still be supported that would be great (it only recently got out of support), but it's not a deal breaker. .NET 5 is a "current" version that's been out of support for some time, so it's not that important as far as I'm concerned.

Unit tests are fine by me. No need to test the integration as a whole, I think we should be able to assume that any provider that supports the distributed cache contract has its own set of tests.

— Reply to this email directly, view it on GitHubhttps://github.com/KevinDockx/HttpCacheHeaders/issues/112#issuecomment-1410742418, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7SBLSLC3O4BQ4SNHRDWVFAGHANCNFSM6AAAAAAUMIBAXA. You are receiving this because you authored the thread.Message ID: @.***>

KevinDockx commented 1 year ago

Abstractions package looks good, judging from the dependency list we should indeed be able to target every current supported framework.

In regards to returning the cached keys: hmm, that's a bit annoying. I haven't looked into it in detail yet, but do you think that a certain change to the current marvin.cache.headers package would help? I'd be open to consider that :)

SeanFarrow commented 1 year ago

Possibly, but firstly, what is the purpose of storing the keys in a separate dictionary in the in-memory implementation? Once I understand that, I may be able to come up with an alternative that works for both in-memory and distributed stores without writing a lot of code for each.

Thanks, Sean.

From: Kevin Dockx @.> Sent: 01 February 2023 17:27 To: KevinDockx/HttpCacheHeaders @.> Cc: Sean Farrow @.>; Author @.> Subject: Re: [KevinDockx/HttpCacheHeaders] Provide a way to store validator values in a scalable manner (Issue #112)

Abstractions package looks good, judging from the dependency list we should indeed be able to target every current supported framework.

In regards to returning the cached keys: hmm, that's a bit annoying. I haven't looked into it in detail yet, but do you think that a certain change to the current marvin.cache.headers package would help? I'd be open to consider that :)

— Reply to this email directly, view it on GitHubhttps://github.com/KevinDockx/HttpCacheHeaders/issues/112#issuecomment-1412442481, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7V7GWSYFCHXU6E5YNTWVKMHHANCNFSM6AAAAAAUMIBAXA. You are receiving this because you authored the thread.Message ID: @.***>

KevinDockx commented 1 year ago

Whenever a request comes in, the middleware needs to know whether a likewise request previously came in to know what must be returned (a regular response, a 304 or a 412). To be able to do this calculation, it needs the previously generated ETag and/or last-Modified dates related to that request. It's those values that are stored in the ValidatorValue store.

The key identifies the incoming request. Which parts of the request are taken into account for a match can be configured: typically, this is a combo of headers, path & query string, but it can be manipulated by stetting the VaryBy request header The related value stored for that key = the etag & last-modified date.

So that's the main purpose for which I use the ValidatorValue store - this translates to the "_store" variable. The second dictionary "_storeKeyStore" is there to support invalidating a request. For that, store keys must be searched, and the second dictionary makes that easier & faster.

Does that help?

Potentially we could consider splitting that up in 2 different stores, or investigating whether we'd get there with just one dictionary.

SeanFarrow commented 1 year ago

Yes, massively, thanks. My feeling is we probably could get away with a single dictionary, but I wonder whether we wait and see what .Net 8 is going to give us in terms of key snapshotting as this will make our lives a lot easier. This will particularly be the cache if it comes to IMemoryCache. I can make the interface work for now, but it won’t be pretty as it will need access to the underlying cache to get the keys. So we either have a separate interface to get the cached keys, knowing this will be short lived, or pass in a function that does this work, what is your preference? I think I would probably go with a separate interface myself. The only downside to this is we need to implement it for both Redis and SQL Server, out of the box.

Thanks, Sean.

From: Kevin Dockx @.> Sent: 02 February 2023 09:29 To: KevinDockx/HttpCacheHeaders @.> Cc: Sean Farrow @.>; Author @.> Subject: Re: [KevinDockx/HttpCacheHeaders] Provide a way to store validator values in a scalable manner (Issue #112)

Whenever a request comes in, the middleware needs to know whether a likewise request previously came in to know what must be returned (a regular response, a 304 or a 412). To be able to do this calculation, it needs the previously generated ETag and/or last-Modified dates related to that request. It's those values that are stored in the ValidatorValue store.

The key identifies the incoming request. Which parts of the request are taken into account for a match can be configured: typically, this is a combo of headers, path & query string, but it can be manipulated by stetting the VaryBy request header The related value stored for that key = the etag & last-modified date.

So that's the main purpose for which I use the ValidatorValue store - this translates to the "_store" variable. The second dictionary "_storeKeyStore" is there to support invalidating a request. For that, store keys must be searched, and the second dictionary makes that easier & faster.

Does that help?

Potentially we could consider splitting that up in 2 different stores, or investigating whether we'd get there with just one dictionary.

— Reply to this email directly, view it on GitHubhttps://github.com/KevinDockx/HttpCacheHeaders/issues/112#issuecomment-1413411616, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7UOT5AHJ4PIXAQ3D7DWVN467ANCNFSM6AAAAAAUMIBAXA. You are receiving this because you authored the thread.Message ID: @.***>

KevinDockx commented 1 year ago

I'm not familiar with the upcoming changes in regards to key snapshotting, but it would be good to have something that works for older versions: not everyone will be able to immediately upgrade to a new version once it's released. With that in mind, such an approach will probably be useful for lots of people for a few years to come.

A separate interface sound good! :)

SeanFarrow commented 1 year ago

That makes sense.

I think we should provide an implementation for Redis out of the box.

Microsoft also provide a distributed cache implementation that supports SQL server, but I don't know whether you want to support that as well?

KevinDockx commented 1 year ago

Well, once the plumbing is there in principle everyone could contribute and make different implementations, right? I'd be inclined to make a "main" DistributedCache NuGet package, and then provide implementations in separate packages. Redis seems the obvious choice to start with. We (or anyone else really) can then make other implementations as needed.

SeanFarrow commented 1 year ago

I completely agree. I’ll start work on that over the next few days. Thanks, Sean. From: Kevin Dockx @.> Sent: 07 February 2023 18:58 To: KevinDockx/HttpCacheHeaders @.> Cc: Sean Farrow @.>; Author @.> Subject: Re: [KevinDockx/HttpCacheHeaders] Provide a way to store validator values in a scalable manner (Issue #112)

Well, once the plumbing is there in principle everyone could contribute and make different implementations, right? I'd be inclined to make a "main" DistributedCache NuGet package, and then provide implementations in separate packages. Redis seems the obvious choice to start with. We (or anyone else really) can then make other implementations as needed.

— Reply to this email directly, view it on GitHubhttps://github.com/KevinDockx/HttpCacheHeaders/issues/112#issuecomment-1421291901, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7WV3CONFJISDHQBFV3WWKLLVANCNFSM6AAAAAAUMIBAXA. You are receiving this because you authored the thread.Message ID: @.***>

SeanFarrow commented 1 year ago

I'm currently implementing this and wondered whether you had any objection to changing the FindStoreKeysByKeyPartAsync method of the IValidatorValueStore to return an IAsyncEnumerable. This won't help the in-memory implementation but will help the distributed one.

I can see it's called by the middleware itself and don't believe it is used anywhere else.

I'm suggesting this now as the next version with be a major upgrade, so it makes sense.

KevinDockx commented 1 year ago

IValidatorValueStore is a contract for which developers can provide their own implementation, so changing the method signature would break those implementations. That's not to say we can't change it, but it is something we need to keep in mind :-)

Will that method solely be used for the distributed cache implementations, or do you want to make the in-memory implemenation use it as well? And should the middleware use that new method, or doesn't that matter?

If the in-memory implementation or middleware should use that new method, I think we should change the contract. Otherwise, we may be able to get away with extending it.

SeanFarrow commented 1 year ago

I’m thinking about changing the contract.

The distributed cache will just be another implementation of the IValidatorValueStore interface, but due to the way we have to query Redis, it makes sense to be fully async here. We could extend the contract, but then the in-memory cache will potentially do syn-over-async.

I’ll raise a separate issue for this later today.

Thanks, Sean.

From: Kevin Dockx @.> Sent: 13 February 2023 07:42 To: KevinDockx/HttpCacheHeaders @.> Cc: Sean Farrow @.>; Author @.> Subject: Re: [KevinDockx/HttpCacheHeaders] Provide a way to store validator values in a scalable manner (Issue #112)

IValidatorValueStore is a contract for which developers can provide their own implementation, so changing the method signature would break those implementations. That's not to say we can't change it, but it is something we need to keep in mind :-)

Will that method solely be used for the distributed cache implementations, or do you want to make the in-memory implemenation use it as well? And should the middleware use that new method, or doesn't that matter?

If the in-memory implementation or middleware should use that new method, I think we should change the contract. Otherwise, we may be able to get away with extending it.

— Reply to this email directly, view it on GitHubhttps://github.com/KevinDockx/HttpCacheHeaders/issues/112#issuecomment-1427481443, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7V7KVYEC5744M3UHRDWXHQS5ANCNFSM6AAAAAAUMIBAXA. You are receiving this because you authored the thread.Message ID: @.***>

SeanFarrow commented 1 year ago

I'll finish this next weekend now that issue #114 has been merged.

SeanFarrow commented 1 year ago

I'm very close to finishing this, but the remaining issue we have left is how to re-create keys.

Currently there is no way of knowing which part of the key separated by a single is which after ToString is called.

KevinDockx commented 1 year ago

Are you talking about the StoreKey? That's a Dictionary<string,string>, so it should be possible to get the different parts from it. If not, we could agree on using a fixed separator that we won't allow in any of the values (which may require additional validation of values before storing them). Or are you talking about something else here?