address comments

apache · Jul 18, 2024 · fba57b2 · fba57b2
1 parent a57e4e3
commit fba57b2
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 50 deletions.
diff --git a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java
@@ -21,6 +21,7 @@
 import com.google.common.collect.Lists;
 import java.io.IOException;
 import java.time.Instant;
+import java.util.Arrays;
 import java.util.Collections;
 import org.apache.gravitino.Entity;
 import org.apache.gravitino.EntityAlreadyExistsException;
@@ -113,19 +114,7 @@ User getUser(String metalake, String user) throws NoSuchUserException {
   }
 
   String[] listUserNames(String metalake) {
-    try {
-      AuthorizationUtils.checkMetalakeExists(metalake);
-      Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake);
-      return store.list(namespace, UserEntity.class, Entity.EntityType.USER).stream()
-          .map(UserEntity::name)
-          .toArray(String[]::new);
-    } catch (NoSuchEntityException e) {
-      LOG.warn("Metalake {} does not exist", metalake, e);
-      throw new NoSuchMetalakeException(METALAKE_DOES_NOT_EXIST_MSG, metalake);
-    } catch (IOException ioe) {
-      LOG.error("Listing user under metalake {} failed due to storage issues", metalake, ioe);
-      throw new RuntimeException(ioe);
-    }
+    return Arrays.stream(listUsers(metalake)).map(User::name).toArray(String[]::new);
   }
 
   User[] listUsers(String metalake) {

diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaMapper.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaMapper.java
@@ -61,16 +61,19 @@ UserPO selectUserMetaByMetalakeIdAndName(
       @Param("metalakeId") Long metalakeId, @Param("userName") String name);
 
   @Select(
-      "SELECT user_id as userId, user_name as userName,"
-          + " metalake_id as metalakeId,"
-          + " audit_info as auditInfo,"
-          + " current_version as currentVersion, last_version as lastVersion,"
-          + " deleted_at as deletedAt"
+      "SELECT ut.user_id as userId, ut.user_name as userName,"
+          + " ut.metalake_id as metalakeId,"
+          + " ut.audit_info as auditInfo,"
+          + " ut.current_version as currentVersion, ut.last_version as lastVersion,"
+          + " ut.deleted_at as deletedAt"
           + " FROM "
           + USER_TABLE_NAME
-          + " WHERE metalake_id = #{metalakeId}"
-          + " AND deleted_at = 0")
-  List<UserPO> listUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId);
+          + " ut JOIN "
+          + MetalakeMetaMapper.TABLE_NAME
+          + " mt ON ut.metalake_id = mt.metalake_id"
+          + " WHERE mt.metalake_name = #{metalakeName}"
+          + " AND ut.deleted_at = 0 AND mt.deleted_at = 0")
+  List<UserPO> listUserPOsByMetalake(@Param("metalakeName") String metalakeName);
 
   @Insert(
       "INSERT INTO "

diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java
@@ -26,7 +26,6 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.Function;
 import org.apache.gravitino.Entity;
 import org.apache.gravitino.HasIdentifier;
@@ -35,7 +34,6 @@
 import org.apache.gravitino.authorization.AuthorizationUtils;
 import org.apache.gravitino.exceptions.NoSuchEntityException;
 import org.apache.gravitino.meta.UserEntity;
-import org.apache.gravitino.storage.relational.mapper.MetalakeMetaMapper;
 import org.apache.gravitino.storage.relational.mapper.UserMetaMapper;
 import org.apache.gravitino.storage.relational.mapper.UserRoleRelMapper;
 import org.apache.gravitino.storage.relational.po.RolePO;
@@ -229,27 +227,10 @@ public List<UserEntity> listUsersByNamespace(Namespace namespace) {
 
     List<UserEntity> userEntities = Lists.newArrayList();
     String metalakeName = namespace.level(0);
-    List<UserPO> userPOs = Lists.newArrayList();
-    AtomicLong metalakeId = new AtomicLong();
-    SessionUtils.doMultipleWithoutCommit(
-        () -> {
-          Long id =
-              SessionUtils.doWithoutCommitAndFetchResult(
-                  MetalakeMetaMapper.class,
-                  mapper -> mapper.selectMetalakeIdMetaByName(metalakeName));
-          if (id == null) {
-            throw new NoSuchEntityException(
-                NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
-                Entity.EntityType.METALAKE.name().toLowerCase(),
-                metalakeName);
-          }
-          metalakeId.set(id);
-        },
-        () ->
-            userPOs.addAll(
-                SessionUtils.doWithoutCommitAndFetchResult(
-                    UserMetaMapper.class,
-                    mapper -> mapper.listUserPOsByMetalakeId(metalakeId.get()))));
+    List<UserPO> userPOs =
+        SessionUtils.getWithoutCommit(
+            UserMetaMapper.class, mapper -> mapper.listUserPOsByMetalake(metalakeName));
+
     for (UserPO userPO : userPOs) {
       List<RolePO> rolePOs = RoleMetaService.getInstance().listRolesByUserId(userPO.getUserId());
       userEntities.add(POConverters.fromUserPO(userPO, rolePOs, namespace));

diff --git a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestUserMetaService.java b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestUserMetaService.java
@@ -145,12 +145,6 @@ void testListUsers() throws IOException {
     Assertions.assertEquals(
         Lists.newArrayList(user1, user2),
         userMetaService.listUsersByNamespace(AuthorizationUtils.ofUserNamespace(metalakeName)));
-
-    // Test NoSuchMetalakeException
-    Assertions.assertThrows(
-        NoSuchEntityException.class,
-        () ->
-            userMetaService.listUsersByNamespace(AuthorizationUtils.ofUserNamespace("not-exist")));
   }
 
   @Test