Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add caching to permission management #16416

Open
Piedone opened this issue Jul 9, 2024 · 3 comments
Open

Add caching to permission management #16416

Piedone opened this issue Jul 9, 2024 · 3 comments

Comments

@Piedone
Copy link
Member

Piedone commented Jul 9, 2024

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:

foreach (var permissionProvider in _permissionProviders)
{
var permissions = await permissionProvider.GetPermissionsAsync();
_permissions.AddRange(permissions);
}
This can be a performance issue if the permission provider does anything more involved, like the one in Queries (before #16402), in Secure Media (what uses caching due to this), or Roles.

Also see https://github.com/OrchardCMS/OrchardCore/pull/15173/files#r1536280664 and #16402 (comment) 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.

Copy link
Contributor

github-actions bot commented Jul 9, 2024

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
Copy link
Contributor

gvkries commented Jul 17, 2024

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
Copy link
Member

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.

private readonly IEnumerable<Permission> _allPermissions =

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants