MonkeyCanCode commented on code in PR #4396:
URL: https://github.com/apache/polaris/pull/4396#discussion_r3230440480
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -1077,6 +1077,16 @@ public void deletePrincipal(String name) {
throw new ValidationException(
"Cannot reset credentials for a federated principal: %s",
principalName);
}
+ if (customClientId != null) {
+ PolarisPrincipalSecrets collidingSecrets =
+ metaStoreManager
+ .loadPrincipalSecrets(getCurrentPolarisContext(), customClientId)
+ .getPrincipalSecrets();
+ if (collidingSecrets != null
+ && collidingSecrets.getPrincipalId() !=
currentPrincipalEntity.getId()) {
Review Comment:
I tried removing the service layer check and delegating to the persistence
layer. However, the reset in the NoSQL backend already bumps the entity version
as part of updateEntityPropertiesIfNotChanged. Looking into the failed tests,
the full reset operation contains delete secret, update entity, then write new
secret which is not atomic across backends. In this case, if the write fails
due to a collision, the first two steps have already been committed, the
principal will then have no secret which would get into the same state.
Should we move this to a diff PR later to chain all three operations into a
single atomic commit? Or maybe I missed something here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
