[apache#6061] fix(core): Fix the issues of list user or group details (…

…apache#6067) ### What changes were proposed in this pull request? Fix the issues of list user or group details ### Why are the changes needed? Fix: apache#6061 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added UT.
Abyss-lord · Jan 4, 2025 · 9513baf · 9513baf
1 parent 2bb614e
commit 9513baf
Show file tree

Hide file tree

Showing 9 changed files with 118 additions and 36 deletions.
diff --git a/...test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java b/...test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java
@@ -121,6 +121,10 @@ void testManageUsers() {
         users.stream().map(User::name).collect(Collectors.toList()));
     Assertions.assertEquals(Lists.newArrayList("role1"), users.get(2).roles());
 
+    // ISSUE-6061: Test listUsers with revoked users
+    metalake.revokeRolesFromUser(Lists.newArrayList("role1"), username);
+    Assertions.assertEquals(3, metalake.listUsers().length);
+
     // Get a not-existed user
     Assertions.assertThrows(NoSuchUserException.class, () -> metalake.getUser("not-existed"));
 
@@ -176,6 +180,10 @@ void testManageGroups() {
         groups.stream().map(Group::name).collect(Collectors.toList()));
     Assertions.assertEquals(Lists.newArrayList("role2"), groups.get(0).roles());
 
+    // ISSUE-6061: Test listGroups with revoked groups
+    metalake.revokeRolesFromGroup(Lists.newArrayList("role2"), groupName);
+    Assertions.assertEquals(2, metalake.listGroups().length);
+
     Assertions.assertTrue(metalake.removeGroup(groupName));
     Assertions.assertFalse(metalake.removeGroup(groupName));
 

diff --git a/...rg/apache/gravitino/storage/relational/mapper/provider/base/GroupMetaBaseSQLProvider.java b/...rg/apache/gravitino/storage/relational/mapper/provider/base/GroupMetaBaseSQLProvider.java
@@ -57,16 +57,19 @@ public String listExtendedGroupPOsByMetalakeId(Long metalakeId) {
         + " JSON_ARRAYAGG(rot.role_id) as roleIds"
         + " FROM "
         + GROUP_TABLE_NAME
-        + " gt LEFT OUTER JOIN "
+        + " gt LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + GROUP_ROLE_RELATION_TABLE_NAME
-        + " rt ON rt.group_id = gt.group_id"
-        + " LEFT OUTER JOIN "
+        + " WHERE deleted_at = 0)"
+        + " AS rt ON rt.group_id = gt.group_id"
+        + " LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + ROLE_TABLE_NAME
-        + " rot ON rot.role_id = rt.role_id"
+        + " WHERE deleted_at = 0)"
+        + " AS rot ON rot.role_id = rt.role_id"
         + " WHERE "
         + " gt.deleted_at = 0 AND"
-        + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND"
-        + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND gt.metalake_id = #{metalakeId}"
+        + " gt.metalake_id = #{metalakeId}"
         + " GROUP BY gt.group_id";
   }
 

diff --git a/...org/apache/gravitino/storage/relational/mapper/provider/base/UserMetaBaseSQLProvider.java b/...org/apache/gravitino/storage/relational/mapper/provider/base/UserMetaBaseSQLProvider.java
@@ -165,16 +165,19 @@ public String listExtendedUserPOsByMetalakeId(@Param("metalakeId") Long metalake
         + " JSON_ARRAYAGG(rot.role_id) as roleIds"
         + " FROM "
         + USER_TABLE_NAME
-        + " ut LEFT OUTER JOIN "
+        + " ut LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + USER_ROLE_RELATION_TABLE_NAME
-        + " rt ON rt.user_id = ut.user_id"
-        + " LEFT OUTER JOIN "
+        + " WHERE deleted_at = 0)"
+        + " AS rt ON rt.user_id = ut.user_id"
+        + " LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + ROLE_TABLE_NAME
-        + " rot ON rot.role_id = rt.role_id"
+        + " WHERE deleted_at = 0)"
+        + " AS rot ON rot.role_id = rt.role_id"
         + " WHERE "
         + " ut.deleted_at = 0 AND"
-        + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND"
-        + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND ut.metalake_id = #{metalakeId}"
+        + " ut.metalake_id = #{metalakeId}"
         + " GROUP BY ut.user_id";
   }
 

diff --git a/.../java/org/apache/gravitino/storage/relational/mapper/provider/h2/GroupMetaH2Provider.java b/.../java/org/apache/gravitino/storage/relational/mapper/provider/h2/GroupMetaH2Provider.java
@@ -37,16 +37,19 @@ public String listExtendedGroupPOsByMetalakeId(@Param("metalakeId") Long metalak
         + " '[' || GROUP_CONCAT('\"' || rot.role_id || '\"') || ']' as roleIds"
         + " FROM "
         + GROUP_TABLE_NAME
-        + " gt LEFT OUTER JOIN "
+        + " gt LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + GROUP_ROLE_RELATION_TABLE_NAME
-        + " rt ON rt.group_id = gt.group_id"
-        + " LEFT OUTER JOIN "
+        + " WHERE deleted_at = 0)"
+        + " AS rt ON rt.group_id = gt.group_id"
+        + " LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + ROLE_TABLE_NAME
-        + " rot ON rot.role_id = rt.role_id"
+        + " WHERE deleted_at = 0)"
+        + " AS rot ON rot.role_id = rt.role_id"
         + " WHERE "
         + " gt.deleted_at = 0 AND"
-        + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND"
-        + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND gt.metalake_id = #{metalakeId}"
+        + " gt.metalake_id = #{metalakeId}"
         + " GROUP BY gt.group_id";
   }
 }
diff --git a/...n/java/org/apache/gravitino/storage/relational/mapper/provider/h2/UserMetaH2Provider.java b/...n/java/org/apache/gravitino/storage/relational/mapper/provider/h2/UserMetaH2Provider.java
@@ -37,16 +37,19 @@ public String listExtendedUserPOsByMetalakeId(@Param("metalakeId") Long metalake
         + " '[' || GROUP_CONCAT('\"' || rot.role_id || '\"') || ']' as roleIds"
         + " FROM "
         + USER_TABLE_NAME
-        + " ut LEFT OUTER JOIN "
+        + " ut LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + USER_ROLE_RELATION_TABLE_NAME
-        + " rt ON rt.user_id = ut.user_id"
-        + " LEFT OUTER JOIN "
+        + " WHERE deleted_at = 0)"
+        + " AS rt ON rt.user_id = ut.user_id"
+        + " LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + ROLE_TABLE_NAME
-        + " rot ON rot.role_id = rt.role_id"
+        + " WHERE deleted_at = 0)"
+        + " AS rot ON rot.role_id = rt.role_id"
         + " WHERE "
         + " ut.deleted_at = 0 AND "
-        + "(rot.deleted_at = 0 OR rot.deleted_at is NULL) AND "
-        + "(rt.deleted_at = 0 OR rt.deleted_at is NULL) AND ut.metalake_id = #{metalakeId}"
+        + " ut.metalake_id = #{metalakeId}"
         + " GROUP BY ut.user_id";
   }
 }
diff --git a/.../gravitino/storage/relational/mapper/provider/postgresql/GroupMetaPostgreSQLProvider.java b/.../gravitino/storage/relational/mapper/provider/postgresql/GroupMetaPostgreSQLProvider.java
@@ -80,16 +80,19 @@ public String listExtendedGroupPOsByMetalakeId(Long metalakeId) {
         + " JSON_AGG(rot.role_id) as roleIds"
         + " FROM "
         + GROUP_TABLE_NAME
-        + " gt LEFT OUTER JOIN "
+        + " gt LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + GROUP_ROLE_RELATION_TABLE_NAME
-        + " rt ON rt.group_id = gt.group_id"
-        + " LEFT OUTER JOIN "
+        + " WHERE deleted_at = 0)"
+        + " AS rt ON rt.group_id = gt.group_id"
+        + " LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + ROLE_TABLE_NAME
-        + " rot ON rot.role_id = rt.role_id"
+        + " WHERE deleted_at = 0)"
+        + " AS rot ON rot.role_id = rt.role_id"
         + " WHERE "
         + " gt.deleted_at = 0 AND"
-        + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND"
-        + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND gt.metalake_id = #{metalakeId}"
+        + " gt.metalake_id = #{metalakeId}"
         + " GROUP BY gt.group_id";
   }
 }
diff --git a/...e/gravitino/storage/relational/mapper/provider/postgresql/UserMetaPostgreSQLProvider.java b/...e/gravitino/storage/relational/mapper/provider/postgresql/UserMetaPostgreSQLProvider.java
@@ -80,16 +80,19 @@ public String listExtendedUserPOsByMetalakeId(Long metalakeId) {
         + " JSON_AGG(rot.role_id) as roleIds"
         + " FROM "
         + USER_TABLE_NAME
-        + " ut LEFT OUTER JOIN "
+        + " ut LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + USER_ROLE_RELATION_TABLE_NAME
-        + " rt ON rt.user_id = ut.user_id"
-        + " LEFT OUTER JOIN "
+        + " WHERE deleted_at = 0)"
+        + " AS rt ON rt.user_id = ut.user_id"
+        + " LEFT OUTER JOIN ("
+        + " SELECT * FROM "
         + ROLE_TABLE_NAME
-        + " rot ON rot.role_id = rt.role_id"
+        + " WHERE deleted_at = 0)"
+        + " AS rot ON rot.role_id = rt.role_id"
         + " WHERE "
         + " ut.deleted_at = 0 AND"
-        + " (rot.deleted_at = 0 OR rot.deleted_at is NULL) AND"
-        + " (rt.deleted_at = 0 OR rt.deleted_at is NULL) AND ut.metalake_id = #{metalakeId}"
+        + " ut.metalake_id = #{metalakeId}"
         + " GROUP BY ut.user_id";
   }
 }
diff --git a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestGroupMetaService.java
@@ -27,6 +27,7 @@
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.time.Instant;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
@@ -189,6 +190,33 @@ void testListGroups() throws IOException {
         }
       }
     }
+
+    // ISSUE-6061: Test listGroupsByNamespace with revoked users
+    Function<GroupEntity, GroupEntity> revokeUpdater =
+        group -> {
+          AuditInfo updateAuditInfo =
+              AuditInfo.builder()
+                  .withCreator(group.auditInfo().creator())
+                  .withCreateTime(group.auditInfo().createTime())
+                  .withLastModifier("revokeGroup")
+                  .withLastModifiedTime(Instant.now())
+                  .build();
+
+          return GroupEntity.builder()
+              .withNamespace(group.namespace())
+              .withId(group.id())
+              .withName(group.name())
+              .withRoleNames(Collections.emptyList())
+              .withRoleIds(Collections.emptyList())
+              .withAuditInfo(updateAuditInfo)
+              .build();
+        };
+
+    Assertions.assertNotNull(groupMetaService.updateGroup(group2.nameIdentifier(), revokeUpdater));
+    actualGroups =
+        groupMetaService.listGroupsByNamespace(
+            AuthorizationUtils.ofGroupNamespace(metalakeName), true);
+    Assertions.assertEquals(expectGroups.size(), actualGroups.size());
   }
 
   @Test

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
@@ -27,6 +27,7 @@
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.time.Instant;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
@@ -188,6 +189,33 @@ void testListUsers() throws IOException {
         }
       }
     }
+
+    // ISSUE-6061: Test listUsersByNamespace with revoked users
+    Function<UserEntity, UserEntity> revokeUpdater =
+        user -> {
+          AuditInfo updateAuditInfo =
+              AuditInfo.builder()
+                  .withCreator(user.auditInfo().creator())
+                  .withCreateTime(user.auditInfo().createTime())
+                  .withLastModifier("revokeUser")
+                  .withLastModifiedTime(Instant.now())
+                  .build();
+
+          return UserEntity.builder()
+              .withNamespace(user.namespace())
+              .withId(user.id())
+              .withName(user.name())
+              .withRoleNames(Collections.emptyList())
+              .withRoleIds(Collections.emptyList())
+              .withAuditInfo(updateAuditInfo)
+              .build();
+        };
+
+    Assertions.assertNotNull(userMetaService.updateUser(user1.nameIdentifier(), revokeUpdater));
+    actualUsers =
+        userMetaService.listUsersByNamespace(
+            AuthorizationUtils.ofUserNamespace(metalakeName), true);
+    Assertions.assertEquals(expectUsers.size(), actualUsers.size());
   }
 
   @Test