jerryshao commented on code in PR #14728:
URL: https://github.com/apache/gravitino/pull/14728#discussion_r4492066020
##########
core/src/main/java/org/apache/gravitino/catalog/ModelOperationDispatcher.java:
##########
@@ -56,6 +56,22 @@ public ModelOperationDispatcher(
super(catalogManager, store, idGenerator, secretManager);
}
+ /**
+ * Checks whether a model exists in a catalog that supports models.
+ *
+ * @param ident the model identifier
+ * @return whether the model exists
+ */
+ @Override
+ public boolean modelExists(NameIdentifier ident) {
+ boolean supportsModels =
+ doWithCatalog(
+ getCatalogIdentifier(ident),
+ wrapper -> wrapper.doWithCatalog(catalog -> catalog.ops()
instanceof ModelCatalog),
+ RuntimeException.class);
+ return supportsModels && ModelDispatcher.super.modelExists(ident);
Review Comment:
Is returning false for catalogs that do not support models the intended
contract, instead of throwing an unsupported-operation error as other model
calls do? Please confirm callers (for example existence checks in
authorization) expect this.
##########
core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java:
##########
@@ -476,15 +476,21 @@ private static SecurableObject
updateGrantedSecurableObject(
RoleEntity roleEntity,
SecurableObject targetObject,
AuthorizationPluginCallbackWrapper authorizationPluginCallbackWrapper) {
- // Removed duplicated privileges by set
- Set<Privilege> updatePrivileges = Sets.newHashSet();
- updatePrivileges.addAll(targetObject.privileges());
- // If old object contains all the privileges to grant, the object doesn't
- // need to update.
- if (updatePrivileges.containsAll(privileges)) {
+ Set<Privilege> updatePrivileges =
Sets.newHashSet(targetObject.privileges());
+ Set<Privilege> canonicalPrivileges =
+ targetObject.privileges().stream()
+ .map(p -> AuthorizationUtils.replaceLegacyPrivilege(p.name(),
p.condition()))
+ .collect(Collectors.toSet());
+ for (Privilege privilege : privileges) {
+ if (canonicalPrivileges.add(
+ AuthorizationUtils.replaceLegacyPrivilege(privilege.name(),
privilege.condition()))) {
+ updatePrivileges.add(privilege);
+ }
+ }
+ // Preserve the stored spelling when an equivalent privilege is already
held.
+ if (updatePrivileges.size() == targetObject.privileges().size()) {
Review Comment:
The no-change check compares set size with the list size, so a stored
privilege list that already holds duplicates would be treated as changed and
rewritten. Comparing the sets of canonical privileges would be more robust.
--
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]
