Improve performance of the PermissionsService #5001
Conversation
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
ryuyu,
chenriksson,
cristinamanum and
skofman1
| } | ||
|
|
||
| if (isUserAdmin) | ||
| if (isUserAdmin && |
There was a problem hiding this comment.
you can get a minor perf improvement if you switch between the admin check and the owner check.
Admin performing an action is not as common as user performing an action, so you can avoid the check in most cases.
| return PermissionLevelsMatch(PermissionLevel.Anonymous, actionPermissionLevel); | ||
| } | ||
|
|
||
| private static bool PermissionLevelsMatch(PermissionLevel first, PermissionLevel second) |
There was a problem hiding this comment.
nit: This is more like PermissionLevelsIntersect right? Match to me sounds like equality.
| @@ -103,10 +103,17 @@ private void CreateOrganizationOwnerAndAddUserAsMember(List<User> owners, User u | |||
|
|
|||
| private void AssertPermissionLevels(IEnumerable<User> owners, User user, PermissionLevel expectedLevel) | |||
| { | |||
| Assert.Equal(expectedLevel, PermissionsService.GetPermissionLevel(owners, user)); | |||
| for (int i = 0; i < Enum.GetValues(typeof(PermissionLevel)).Cast<int>().Max() * 2; i++) | |||
There was a problem hiding this comment.
Looks like the max value for permissions level (i.e., IsOwner | IsSiteAdmin | IsOrganizationAdmin | IsOrganizationCollaborator), and is also used for test data above. Maybe create a readonly class variable for it?
There was a problem hiding this comment.
nit: My only concern with ReturnsExpectedPermissionLevels is whether it would be obvious from a test failure what the issue is. In the case of failure, will debugging be necessary to determine the issue? Looks comprehensive though.
There was a problem hiding this comment.
yup, it's for iterating through all possible combinations of permissions
There was a problem hiding this comment.
added readonly variable
|
Was there a perceived performance impact? How was this measured? |
|
@joelverhagen performance was never measured, but it seems very reasonable that in the majority of cases, skipping the organizations check when we know the user is an owner should be more performant. |
Previously, the
PermissionsServicewould1 - Get a list of all permissions that the user has
2 - Check that list with the permissions required by the action
This PR now allows the
PermissionsServiceto short-circuit and stop checking permissions when it finds that the user has a permission that's required.E.g. if I am an owner of the package and this action is allowed if I have owner permissions, I don't need to check if I am a member of any organizations that own the package.