search

OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.43k stars 2.4k forks source link

Add caching to permission management #16416

Open Piedone opened 4 months ago

Piedone commented 4 months ago

Is your feature request related to a problem? Please describe.

IPermissionProvider.GetPermissionsAsync() on all implementations is called on every admin request by AdminMenuPermissionService in OrchardCore.AdminMenu if any of the AdminMenu nodes have any permissions configured: https://github.com/OrchardCMS/OrchardCore/blob/6312841ef0893e1585e93e325289794de4aada94/src/OrchardCore.Modules/OrchardCore.AdminMenu/Services/AdminMenuPermissionService.cs#L28-L33 This can be a performance issue if the permission provider does anything more involved, like the one in Queries (before https://github.com/OrchardCMS/OrchardCore/pull/16402), in Secure Media (what uses caching due to this), or Roles.

Also see https://github.com/OrchardCMS/OrchardCore/pull/15173/files#r1536280664 and https://github.com/OrchardCMS/OrchardCore/pull/16402#discussion_r1667352864 for context.

Describe the solution you'd like

An approach, but surely not the best one, is to cache in every provider that does non-trivial work. like SecureMediaPermissions does. This is viable, and as that provider also shows, may be necessary due to the variety of how such caching, and especially cache invalidation should be done.

A better one would be to cache on a higher level, though then invalidation becomes an issue.

Once done, the caching in SecureMediaPermissions can be removed.

Describe alternatives you've considered

Keeping everything as it is for now, since this doesn't seem to be a big enough deal yet.

github-actions[bot] commented 4 months ago

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

gvkries commented 3 months ago

I don't think it makes sense add an additional cache for permissions. Each provider has it's own needs and only a few (one at the moment) will have additional caching requirements. All other providers are already using cached data to generate the permission instances.

MikeAlhayek commented 3 months ago

If we don't add a global cache for permissions, we should set all _allPermissions properties and similar one in Permission providers to static to avoid having to create so many object on every request to reduce memory allocation and reduce garbage collection.

https://github.com/OrchardCMS/OrchardCore/blob/1b4bb4441856a15485f90bdbaeae4a1a4b222875/src/OrchardCore/OrchardCore.Email.Core/Permissions.cs#L11