address comment

apache · Oct 18, 2024 · 075d1e0 · 075d1e0
1 parent f200001
commit 075d1e0
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 67 deletions.
diff --git a/clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java b/clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java
@@ -36,8 +36,8 @@
 import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
 import org.apache.gravitino.exceptions.IllegalMetadataObjectException;
 import org.apache.gravitino.exceptions.IllegalPrivilegeException;
-import org.apache.gravitino.exceptions.InUseException;
 import org.apache.gravitino.exceptions.IllegalRoleException;
+import org.apache.gravitino.exceptions.InUseException;
 import org.apache.gravitino.exceptions.MetalakeAlreadyExistsException;
 import org.apache.gravitino.exceptions.NoSuchCatalogException;
 import org.apache.gravitino.exceptions.NoSuchFilesetException;

diff --git a/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java b/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java
@@ -22,6 +22,7 @@
 import java.util.Map;
 import org.apache.gravitino.MetadataObject;
 import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.IllegalRoleException;
 import org.apache.gravitino.exceptions.NoSuchGroupException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
 import org.apache.gravitino.exceptions.NoSuchMetalakeException;
@@ -155,12 +156,12 @@ Group getGroup(String metalake, String group)
    * @param roles The names of the Role.
    * @return The User after granted.
    * @throws NoSuchUserException If the User with the given name does not exist.
-   * @throws NoSuchRoleException If the Role with the given name does not exist.
+   * @throws IllegalRoleException If the Role with the given name does not exist.
    * @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
    * @throws RuntimeException If granting roles to a user encounters storage issues.
    */
   User grantRolesToUser(String metalake, List<String> roles, String user)
-      throws NoSuchUserException, NoSuchRoleException, NoSuchMetalakeException;
+      throws NoSuchUserException, IllegalRoleException, NoSuchMetalakeException;
 
   /**
    * Grant roles to a group.
@@ -170,12 +171,12 @@ User grantRolesToUser(String metalake, List<String> roles, String user)
    * @param roles The names of the Role.
    * @return The Group after granted.
    * @throws NoSuchGroupException If the Group with the given name does not exist.
-   * @throws NoSuchRoleException If the Role with the given name does not exist.
+   * @throws IllegalRoleException If the Role with the given name does not exist.
    * @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
    * @throws RuntimeException If granting roles to a group encounters storage issues.
    */
   Group grantRolesToGroup(String metalake, List<String> roles, String group)
-      throws NoSuchGroupException, NoSuchRoleException, NoSuchMetalakeException;
+      throws NoSuchGroupException, IllegalRoleException, NoSuchMetalakeException;
 
   /**
    * Revoke roles from a group.
@@ -185,12 +186,12 @@ Group grantRolesToGroup(String metalake, List<String> roles, String group)
    * @param roles The name of the Role.
    * @return The Group after revoked.
    * @throws NoSuchGroupException If the Group with the given name does not exist.
-   * @throws NoSuchRoleException If the Role with the given name does not exist.
+   * @throws IllegalRoleException If the Role with the given name does not exist.
    * @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
    * @throws RuntimeException If revoking roles from a group encounters storage issues.
    */
   Group revokeRolesFromGroup(String metalake, List<String> roles, String group)
-      throws NoSuchGroupException, NoSuchRoleException, NoSuchMetalakeException;
+      throws NoSuchGroupException, IllegalRoleException, NoSuchMetalakeException;
 
   /**
    * Revoke roles from a user.
@@ -205,7 +206,7 @@ Group revokeRolesFromGroup(String metalake, List<String> roles, String group)
    * @throws RuntimeException If revoking roles from a user encounters storage issues.
    */
   User revokeRolesFromUser(String metalake, List<String> roles, String user)
-      throws NoSuchUserException, NoSuchRoleException, NoSuchMetalakeException;
+      throws NoSuchUserException, IllegalRoleException, NoSuchMetalakeException;
 
   /**
    * Judges whether the user is the service admin.

diff --git a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java
@@ -25,6 +25,7 @@
 import org.apache.gravitino.EntityStore;
 import org.apache.gravitino.MetadataObject;
 import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.IllegalRoleException;
 import org.apache.gravitino.exceptions.NoSuchGroupException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
 import org.apache.gravitino.exceptions.NoSuchMetalakeException;
@@ -107,25 +108,25 @@ public String[] listGroupNames(String metalake) throws NoSuchMetalakeException {
 
   @Override
   public User grantRolesToUser(String metalake, List<String> roles, String user)
-      throws NoSuchUserException, NoSuchRoleException, NoSuchMetalakeException {
+      throws NoSuchUserException, IllegalRoleException, NoSuchMetalakeException {
     return permissionManager.grantRolesToUser(metalake, roles, user);
   }
 
   @Override
   public Group grantRolesToGroup(String metalake, List<String> roles, String group)
-      throws NoSuchGroupException, NoSuchRoleException, NoSuchMetalakeException {
+      throws NoSuchGroupException, IllegalRoleException, NoSuchMetalakeException {
     return permissionManager.grantRolesToGroup(metalake, roles, group);
   }
 
   @Override
   public Group revokeRolesFromGroup(String metalake, List<String> roles, String group)
-      throws NoSuchGroupException, NoSuchRoleException, NoSuchMetalakeException {
+      throws NoSuchGroupException, IllegalRoleException, NoSuchMetalakeException {
     return permissionManager.revokeRolesFromGroup(metalake, roles, group);
   }
 
   @Override
   public User revokeRolesFromUser(String metalake, List<String> roles, String user)
-      throws NoSuchUserException, NoSuchRoleException, NoSuchMetalakeException {
+      throws NoSuchUserException, IllegalRoleException, NoSuchMetalakeException {
     return permissionManager.revokeRolesFromUser(metalake, roles, user);
   }
 

diff --git a/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java b/core/src/main/java/org/apache/gravitino/authorization/PermissionManager.java
@@ -33,6 +33,7 @@
 import org.apache.gravitino.Entity;
 import org.apache.gravitino.EntityStore;
 import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.exceptions.IllegalRoleException;
 import org.apache.gravitino.exceptions.NoSuchEntityException;
 import org.apache.gravitino.exceptions.NoSuchGroupException;
 import org.apache.gravitino.exceptions.NoSuchRoleException;
@@ -129,6 +130,8 @@ User grantRolesToUser(String metalake, List<String> roles, String user) {
     } catch (NoSuchEntityException nse) {
       LOG.warn("Failed to grant, user {} does not exist in the metalake {}", user, metalake, nse);
       throw new NoSuchUserException(USER_DOES_NOT_EXIST_MSG, user, metalake);
+    } catch (NoSuchRoleException nsr) {
+      throw new IllegalRoleException(nsr);
     } catch (IOException ioe) {
       LOG.error(
           "Failed to grant role {} to user {} in the metalake {} due to storage issues",
@@ -208,6 +211,8 @@ Group grantRolesToGroup(String metalake, List<String> roles, String group) {
     } catch (NoSuchEntityException nse) {
       LOG.warn("Failed to grant, group {} does not exist in the metalake {}", group, metalake, nse);
       throw new NoSuchGroupException(GROUP_DOES_NOT_EXIST_MSG, group, metalake);
+    } catch (NoSuchRoleException nsr) {
+      throw new IllegalRoleException(nsr);
     } catch (IOException ioe) {
       LOG.error(
           "Failed to grant role {} to group {} in the metalake {} due to storage issues",
@@ -288,6 +293,8 @@ Group revokeRolesFromGroup(String metalake, List<String> roles, String group) {
       LOG.warn(
           "Failed to revoke, group {} does not exist in the metalake {}", group, metalake, nse);
       throw new NoSuchGroupException(GROUP_DOES_NOT_EXIST_MSG, group, metalake);
+    } catch (NoSuchRoleException nsr) {
+      throw new IllegalRoleException(nsr);
     } catch (IOException ioe) {
       LOG.error(
           "Failed to revoke role {} from  group {} in the metalake {} due to storage issues",
@@ -366,6 +373,8 @@ User revokeRolesFromUser(String metalake, List<String> roles, String user) {
     } catch (NoSuchEntityException nse) {
       LOG.warn("Failed to revoke, user {} does not exist in the metalake {}", user, metalake, nse);
       throw new NoSuchUserException(USER_DOES_NOT_EXIST_MSG, user, metalake);
+    } catch (NoSuchRoleException nsr) {
+      throw new IllegalRoleException(nsr);
     } catch (IOException ioe) {
       LOG.error(
           "Failed to revoke role {} from  user {} in the metalake {} due to storage issues",

diff --git a/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java
@@ -33,6 +33,7 @@
 import org.apache.gravitino.authorization.SecurableObject;
 import org.apache.gravitino.authorization.User;
 import org.apache.gravitino.exceptions.GroupAlreadyExistsException;
+import org.apache.gravitino.exceptions.IllegalRoleException;
 import org.apache.gravitino.exceptions.NoSuchGroupException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
 import org.apache.gravitino.exceptions.NoSuchMetalakeException;
@@ -111,25 +112,25 @@ public String[] listGroupNames(String metalake) throws NoSuchMetalakeException {
 
   @Override
   public User grantRolesToUser(String metalake, List<String> roles, String user)
-      throws NoSuchUserException, NoSuchRoleException, NoSuchMetalakeException {
+      throws NoSuchUserException, IllegalRoleException, NoSuchMetalakeException {
     return dispatcher.grantRolesToUser(metalake, roles, user);
   }
 
   @Override
   public Group grantRolesToGroup(String metalake, List<String> roles, String group)
-      throws NoSuchGroupException, NoSuchRoleException, NoSuchMetalakeException {
+      throws NoSuchGroupException, IllegalRoleException, NoSuchMetalakeException {
     return dispatcher.grantRolesToGroup(metalake, roles, group);
   }
 
   @Override
   public Group revokeRolesFromGroup(String metalake, List<String> roles, String group)
-      throws NoSuchGroupException, NoSuchRoleException, NoSuchMetalakeException {
+      throws NoSuchGroupException, IllegalRoleException, NoSuchMetalakeException {
     return dispatcher.revokeRolesFromGroup(metalake, roles, group);
   }
 
   @Override
   public User revokeRolesFromUser(String metalake, List<String> roles, String user)
-      throws NoSuchUserException, NoSuchRoleException, NoSuchMetalakeException {
+      throws NoSuchUserException, IllegalRoleException, NoSuchMetalakeException {
     return dispatcher.revokeRolesFromUser(metalake, roles, user);
   }
 

diff --git a/.../test/java/org/apache/gravitino/authorization/TestAccessControlManagerForPermissions.java b/.../test/java/org/apache/gravitino/authorization/TestAccessControlManagerForPermissions.java
@@ -40,6 +40,7 @@
 import org.apache.gravitino.catalog.CatalogManager;
 import org.apache.gravitino.connector.BaseCatalog;
 import org.apache.gravitino.connector.authorization.AuthorizationPlugin;
+import org.apache.gravitino.exceptions.IllegalRoleException;
 import org.apache.gravitino.exceptions.NoSuchGroupException;
 import org.apache.gravitino.exceptions.NoSuchMetalakeException;
 import org.apache.gravitino.exceptions.NoSuchRoleException;
@@ -215,9 +216,9 @@ public void testGrantRoleToUser() {
         NoSuchMetalakeException.class,
         () -> accessControlManager.grantRolesToUser(notExist, ROLE, USER));
 
-    // Throw NoSuchRoleException
+    // Throw IllegalRoleException
     Assertions.assertThrows(
-        NoSuchRoleException.class,
+        IllegalRoleException.class,
         () -> accessControlManager.grantRolesToUser(METALAKE, Lists.newArrayList(notExist), USER));
 
     // Throw NoSuchUserException
@@ -249,9 +250,9 @@ public void testRevokeRoleFromUser() {
         NoSuchMetalakeException.class,
         () -> accessControlManager.revokeRolesFromUser(notExist, ROLE, USER));
 
-    // Throw NoSuchRoleException
+    // Throw IllegalRoleException
     Assertions.assertThrows(
-        NoSuchRoleException.class,
+        IllegalRoleException.class,
         () ->
             accessControlManager.revokeRolesFromUser(METALAKE, Lists.newArrayList(notExist), USER));
 
@@ -293,9 +294,9 @@ public void testGrantRoleToGroup() {
         NoSuchMetalakeException.class,
         () -> accessControlManager.grantRolesToGroup(notExist, ROLE, GROUP));
 
-    // Throw NoSuchRoleException
+    // Throw IllegalRoleException
     Assertions.assertThrows(
-        NoSuchRoleException.class,
+        IllegalRoleException.class,
         () ->
             accessControlManager.grantRolesToGroup(METALAKE, Lists.newArrayList(notExist), GROUP));
 
@@ -328,9 +329,9 @@ public void testRevokeRoleFormGroup() {
         NoSuchMetalakeException.class,
         () -> accessControlManager.revokeRolesFromGroup(notExist, ROLE, GROUP));
 
-    // Throw NoSuchRoleException
+    // Throw IllegalRoleException
     Assertions.assertThrows(
-        NoSuchRoleException.class,
+        IllegalRoleException.class,
         () ->
             accessControlManager.revokeRolesFromGroup(
                 METALAKE, Lists.newArrayList(notExist), GROUP));
@@ -375,7 +376,7 @@ public void testGrantPrivilegeToRole() {
 
     Assertions.assertEquals(2, objects.size());
 
-    // Throw NoSuchRoleException
+    // Throw IllegalRoleException
     Assertions.assertThrows(
         NoSuchRoleException.class,
         () ->

diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/PermissionOperations.java
@@ -45,8 +45,6 @@
 import org.apache.gravitino.dto.responses.RoleResponse;
 import org.apache.gravitino.dto.responses.UserResponse;
 import org.apache.gravitino.dto.util.DTOConverters;
-import org.apache.gravitino.exceptions.IllegalRoleException;
-import org.apache.gravitino.exceptions.NoSuchRoleException;
 import org.apache.gravitino.lock.LockType;
 import org.apache.gravitino.lock.TreeLockUtils;
 import org.apache.gravitino.metrics.MetricNames;
@@ -89,17 +87,12 @@ public Response grantRolesToUser(
                       TreeLockUtils.doWithTreeLock(
                           NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
                           LockType.READ,
-                          () -> {
-                            try {
-                              return Utils.ok(
+                          () ->
+                              Utils.ok(
                                   new UserResponse(
                                       DTOConverters.toDTO(
                                           accessControlManager.grantRolesToUser(
-                                              metalake, request.getRoleNames(), user))));
-                            } catch (NoSuchRoleException nsr) {
-                              throw new IllegalRoleException(nsr);
-                            }
-                          })));
+                                              metalake, request.getRoleNames(), user)))))));
     } catch (Exception e) {
       return ExceptionHandlers.handleUserPermissionOperationException(
           OperationType.GRANT, StringUtils.join(request.getRoleNames(), ","), user, e);
@@ -126,17 +119,12 @@ public Response grantRolesToGroup(
                       TreeLockUtils.doWithTreeLock(
                           NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
                           LockType.READ,
-                          () -> {
-                            try {
-                              return Utils.ok(
+                          () ->
+                              Utils.ok(
                                   new GroupResponse(
                                       DTOConverters.toDTO(
                                           accessControlManager.grantRolesToGroup(
-                                              metalake, request.getRoleNames(), group))));
-                            } catch (NoSuchRoleException nsr) {
-                              throw new IllegalRoleException(nsr);
-                            }
-                          })));
+                                              metalake, request.getRoleNames(), group)))))));
     } catch (Exception e) {
       return ExceptionHandlers.handleGroupPermissionOperationException(
           OperationType.GRANT, StringUtils.join(request.getRoleNames(), ","), group, e);
@@ -163,17 +151,12 @@ public Response revokeRolesFromUser(
                       TreeLockUtils.doWithTreeLock(
                           NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
                           LockType.READ,
-                          () -> {
-                            try {
-                              return Utils.ok(
+                          () ->
+                              Utils.ok(
                                   new UserResponse(
                                       DTOConverters.toDTO(
                                           accessControlManager.revokeRolesFromUser(
-                                              metalake, request.getRoleNames(), user))));
-                            } catch (NoSuchRoleException nsr) {
-                              throw new IllegalRoleException(nsr);
-                            }
-                          })));
+                                              metalake, request.getRoleNames(), user)))))));
     } catch (Exception e) {
       return ExceptionHandlers.handleUserPermissionOperationException(
           OperationType.REVOKE, StringUtils.join(request.getRoleNames(), ","), user, e);
@@ -200,17 +183,12 @@ public Response revokeRolesFromGroup(
                       TreeLockUtils.doWithTreeLock(
                           NameIdentifier.of(AuthorizationUtils.ofRoleNamespace(metalake).levels()),
                           LockType.READ,
-                          () -> {
-                            try {
-                              return Utils.ok(
+                          () ->
+                              Utils.ok(
                                   new GroupResponse(
                                       DTOConverters.toDTO(
                                           accessControlManager.revokeRolesFromGroup(
-                                              metalake, request.getRoleNames(), group))));
-                            } catch (NoSuchRoleException nsr) {
-                              throw new IllegalRoleException(nsr);
-                            }
-                          })));
+                                              metalake, request.getRoleNames(), group)))))));
     } catch (Exception e) {
       return ExceptionHandlers.handleGroupPermissionOperationException(
           OperationType.REVOKE, StringUtils.join(request.getRoleNames()), group, e);