CORE-242 bulk membership update #1613
Conversation
| @@ -1038,7 +1039,6 @@ from ${GroupMemberTable as groupMemberTable} | |||
| } | |||
| removeAllGroupMembers(groupId) | |||
| insertGroupMembers(groupId, memberList) | |||
| updateGroupUpdatedDateAndVersion(id) | |||
There was a problem hiding this comment.
this is an internal function, calling updateGroupUpdatedDateAndVersion is redundant
| _ <- directoryDAO.updateGroupUpdatedDateAndVersionWithSession( | ||
| FullyQualifiedPolicyId(policyId.resource, policyId.accessPolicyName), | ||
| samRequestContext | ||
| ) |
There was a problem hiding this comment.
I think this was here as a failsafe but was double doing work. I made sure it is correctly called in the DAO. Maybe a follow on would be to figure out a trigger even though I despise triggers.
| @@ -851,51 +906,14 @@ class ResourceService( | |||
| ): IO[Set[ValidatableAccessPolicy]] = | |||
| policies.toList | |||
| .traverse { case (accessPolicyName, accessPolicyMembershipRequest) => | |||
| for { | |||
| // grouping member emails and policy emails together | |||
There was a problem hiding this comment.
It looks like this code was getting the email address for each policy and adding them to memberEmails of the policy. The problem is that policies that do not exist are just ignored. There is also no reason to load a policy by its identifiers, get its email, then use the email to load its identifiers. I changes all this to validate policies (so I could use that for the new api) then use the policy identifiers directly.
| validatableAccessPolicy.emailsToSubjects.values.flatten.toSet ++ validatableAccessPolicy.memberPolicies | ||
| .getOrElse(Set.empty) | ||
| .map(_.toFullyQualifiedPolicyId), |
There was a problem hiding this comment.
See comment below. memberPolicies are no longer included in emailsToSubjects so add them here when constructing the policy
I changed the ticket ;) |
| This endpoint is used to add and remove members of multiple policies across multiple resource in one api call. | ||
| Each policy is updated in order in its own transaction. If a policy does not exist on a resource, | ||
| processing stops with an error but all updates up to that point are committed. Missing member ids, emails or policies fail fast. Additions are processed | ||
| before removals so if a subject is in both the add and remove lists for a policy, the ultimate result is removal. |
There was a problem hiding this comment.
What about checking that the add and remove lists are different and throwing an error if there's any overlap?
|
Ticket: https://broadworkbench.atlassian.net/browse/CORE-242
In addition to add the new api, refactored how some other calls validate (see comments in PR).
PR checklist