[apache#6082] fix: Fix error code of creating role operation (apache#…

…6085) ### What changes were proposed in this pull request? We should return 400, if the role contains an error metalake metadata object. We should return 400, if the catalog doesn't exist. ### Why are the changes needed? Fix: apache#6082 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added UT.
Abyss-lord · Jan 3, 2025 · f893b5f · f893b5f
1 parent 6f63430
commit f893b5f
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 4 deletions.
diff --git a/core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java b/core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java
@@ -32,6 +32,7 @@
 import org.apache.gravitino.MetadataObject;
 import org.apache.gravitino.NameIdentifier;
 import org.apache.gravitino.authorization.AuthorizationUtils;
+import org.apache.gravitino.exceptions.IllegalMetadataObjectException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
 import org.apache.gravitino.exceptions.NoSuchRoleException;
 
@@ -125,6 +126,9 @@ public static void checkMetadataObject(String metalake, MetadataObject object) {
 
     switch (object.type()) {
       case METALAKE:
+        if (!metalake.equals(object.name())) {
+          throw new IllegalMetadataObjectException("The metalake object name must be %s", metalake);
+        }
         NameIdentifierUtil.checkMetalake(identifier);
         check(env.metalakeDispatcher().metalakeExists(identifier), exceptionToThrowSupplier);
         break;

diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/RoleOperations.java
@@ -142,10 +142,10 @@ public Response createRole(@PathParam("metalake") String metalake, RoleCreateReq
 
               Set<Privilege> privileges = Sets.newHashSet(object.privileges());
               AuthorizationUtils.checkDuplicatedNamePrivilege(privileges);
-              for (Privilege privilege : object.privileges()) {
-                AuthorizationUtils.checkPrivilege((PrivilegeDTO) privilege, object, metalake);
-              }
               try {
+                for (Privilege privilege : object.privileges()) {
+                  AuthorizationUtils.checkPrivilege((PrivilegeDTO) privilege, object, metalake);
+                }
                 MetadataObjectUtil.checkMetadataObject(metalake, object);
               } catch (NoSuchMetadataObjectException nsm) {
                 throw new IllegalMetadataObjectException(nsm);

diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestRoleOperations.java
@@ -63,6 +63,7 @@
 import org.apache.gravitino.dto.util.DTOConverters;
 import org.apache.gravitino.exceptions.IllegalNamespaceException;
 import org.apache.gravitino.exceptions.IllegalPrivilegeException;
+import org.apache.gravitino.exceptions.NoSuchCatalogException;
 import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
 import org.apache.gravitino.exceptions.NoSuchMetalakeException;
 import org.apache.gravitino.exceptions.NoSuchRoleException;
@@ -199,8 +200,31 @@ public void testCreateRole() {
         Privileges.UseCatalog.allow().condition(),
         roleDTO.securableObjects().get(0).privileges().get(0).condition());
 
+    // Test with a wrong metalake name
+    RoleCreateRequest reqWithWrongMetalake =
+        new RoleCreateRequest(
+            "role",
+            Collections.emptyMap(),
+            new SecurableObjectDTO[] {
+              DTOConverters.toDTO(
+                  SecurableObjects.ofMetalake(
+                      "unknown", Lists.newArrayList(Privileges.UseCatalog.allow()))),
+            });
+    Response respWithWrongMetalake =
+        target("/metalakes/metalake1/roles")
+            .request(MediaType.APPLICATION_JSON_TYPE)
+            .accept("application/vnd.gravitino.v1+json")
+            .post(Entity.entity(reqWithWrongMetalake, MediaType.APPLICATION_JSON_TYPE));
+    Assertions.assertEquals(
+        Response.Status.BAD_REQUEST.getStatusCode(), respWithWrongMetalake.getStatus());
+    Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, respWithWrongMetalake.getMediaType());
+    ErrorResponse withWrongMetalakeResponse = respWithWrongMetalake.readEntity(ErrorResponse.class);
+    Assertions.assertEquals(
+        ErrorConstants.ILLEGAL_ARGUMENTS_CODE, withWrongMetalakeResponse.getCode());
+
     // Test to a catalog which doesn't exist
-    when(catalogDispatcher.catalogExists(any())).thenReturn(false);
+    reset(catalogDispatcher);
+    when(catalogDispatcher.loadCatalog(any())).thenThrow(new NoSuchCatalogException("mock error"));
     Response respNotExist =
         target("/metalakes/metalake1/roles")
             .request(MediaType.APPLICATION_JSON_TYPE)