Different cache key by role#8510
Conversation
|
@sebastienros We put the feature in the OutputCache Module. In principle, it could go in the Orchard.Roles module (changing only dependencies). Let us know which one you prefer. |
|
|
||
| namespace Orchard.OutputCache.Handlers { | ||
| [OrchardFeature("Orchard.OutputCache.CacheByRole")] | ||
| public class CacheByRoleHandler : IRoleEventHandler { |
There was a problem hiding this comment.
I'd suggest instead to create a hash based on the permissions of the role, and cache this hash. Then the ICachingEventHandler can just use this value in the key. If the permissions change, the key change, and the old cache becomes obsolete after some time. That's how we do these things in OC.
There was a problem hiding this comment.
I like that alternative, because it would prevent the need to evict explicitly. I have a few considerations:
- From the User, to get all Permissions for all their roles we would need to go UserRolesPartRecord -> RoleRecord (this has the Role Name, which we may need anyway) -> RolesPermissionsRecord ->PermissionRecord (out of this I think we'd need Name and FeatureName).
- The Name of the Role may be needed to include the list of roles in the CacheKey anyway, to handle code that specifically checks for Roles rather than permissions (e.g. https://github.com/OrchardCMS/Orchard/blob/3336be6c779e0e4ca017c0cf3ff2630f5071e3a7/src/Orchard.Web/Modules/Orchard.Roles/Activities/IsInRoleActivity.cs or the rules for layers). Having the portion of the CacheKey based on the permissions would still be enough to prevent the need for explicit eviction.
- I would use, for each Permission, something like FeatureName.Name, to reduce the change of duplicates or weird corner cases.
There was a problem hiding this comment.
@sebastienros could please point out the bit in OC where you are doing what you describe. At a cursory search I did not find it, and I don't know that software well enough to reliably find it by myself.
The file I found that looks more relevant to this is https://github.com/OrchardCMS/OrchardCore/blob/339d74b251b53393dd510f8ad3027aa17d78500d/src/OrchardCore/OrchardCore.Infrastructure/Cache/CacheContextProviders/RolesCacheContextProvider.cs
I was looking for an ICacheContextProvider that that dealt with Permissions and I don't see it.
There was a problem hiding this comment.
We are not doing it for roles, it's just the technique of using hashes as part of the key so that it doesn't need to be evicted. Using a sliding expiration (or at least some absolute one) is necessary so the values don't stay cached forever.
There was a problem hiding this comment.
The code in this is only contributing to the cachekey. Expiration of the cache entries is controlled elsewhere, configurable through the settings of the OutputCache already.
…e_key_by_role' into 8509_different_cache_key_by_role
|
Cache evict removed for the following reasons:
|
|
I'll summarize the open questions regarding this PR here, for my clarity. The added code is its own feature in the Orchard.OutputCache Module.
The added code does not affect the expiration of whatever values in output cache it is involved with. It contributes to the cachekey, but those cached pages will expire following their own rules, configured elsewhere. That logic is already in place in the Orchard.OutputCache feature. I am not 100% sure on using GetHashCode() to hash the strings used as keys. It's fast, so that's a big plus. And we aren't persisting it anywhere, so that's fine too. On application restart the OutputCache is going to be empty, so even if the result changes it's won't be a problem. I am just slightly concerned about the possibility of collisions (https://docs.microsoft.com/en-us/archive/blogs/ericlippert/socks-birthdays-and-hash-collisions). Do you have a better recommendation? |
|
@sebastienros What do you think about this PR? |
3 participants
added role in the cache key as proposed #8509