From 4ec29a7b2126947bc530032a802949414226e2ad Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 3 Jul 2024 10:50:24 +0800 Subject: [PATCH 01/31] Add ListRoles --- .../gravitino/client/GravitinoClient.java | 20 ++++ .../gravitino/client/GravitinoMetalake.java | 41 +++++++ .../gravitino/client/TestUserGroup.java | 54 ++++++++++ .../test/authorization/AccessControlIT.java | 18 ++++ .../dto/responses/UserListResponse.java | 66 ++++++++++++ .../gravitino/dto/util/DTOConverters.java | 13 +++ .../AccessControlDispatcher.java | 18 ++++ .../authorization/AccessControlManager.java | 19 ++-- .../authorization/UserGroupManager.java | 23 ++++ .../hook/AccessControlHookDispatcher.java | 10 ++ .../storage/relational/JDBCBackend.java | 2 + .../mapper/UserMetaBaseSQLProvider.java | 15 +++ .../relational/mapper/UserMetaMapper.java | 3 + .../mapper/UserMetaSQLProviderFactory.java | 4 + .../relational/service/UserMetaService.java | 19 ++++ .../TestAccessControlManager.java | 25 +++++ .../service/TestUserMetaService.java | 31 ++++++ .../server/web/rest/UserOperations.java | 33 ++++++ .../server/web/rest/TestUserOperations.java | 101 ++++++++++++++++++ 19 files changed, 505 insertions(+), 10 deletions(-) create mode 100644 common/src/main/java/org/apache/gravitino/dto/responses/UserListResponse.java diff --git a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java index 9b7769200be..e074770e84f 100644 --- a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java +++ b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java @@ -168,6 +168,26 @@ public User getUser(String user) throws NoSuchUserException, NoSuchMetalakeExcep return getMetalake().getUser(user); } + /** + * Lists the users. + * + * @return The User list. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + */ + public User[] listUsers() { + return getMetalake().listUsers(); + } + + /** + * Lists the usernames. + * + * @return The username list. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + */ + public String[] listUserNames() { + return getMetalake().listUserNames(); + } + /** * Adds a new Group. * diff --git a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java index f13958cb526..9a13a9dd12f 100644 --- a/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java +++ b/clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoMetalake.java @@ -68,6 +68,7 @@ import org.apache.gravitino.dto.responses.SetResponse; import org.apache.gravitino.dto.responses.TagListResponse; import org.apache.gravitino.dto.responses.TagResponse; +import org.apache.gravitino.dto.responses.UserListResponse; import org.apache.gravitino.dto.responses.UserResponse; import org.apache.gravitino.exceptions.CatalogAlreadyExistsException; import org.apache.gravitino.exceptions.GroupAlreadyExistsException; @@ -515,6 +516,46 @@ public User getUser(String user) throws NoSuchUserException, NoSuchMetalakeExcep return resp.getUser(); } + /** + * Lists the users. + * + * @return The User list. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + */ + public User[] listUsers() throws NoSuchMetalakeException { + Map params = new HashMap<>(); + params.put("details", "true"); + + UserListResponse resp = + restClient.get( + String.format(API_METALAKES_USERS_PATH, name(), BLANK_PLACE_HOLDER), + params, + UserListResponse.class, + Collections.emptyMap(), + ErrorHandlers.userErrorHandler()); + resp.validate(); + + return resp.getUsers(); + } + + /** + * Lists the usernames. + * + * @return The username list. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + */ + public String[] listUserNames() throws NoSuchMetalakeException { + NameListResponse resp = + restClient.get( + String.format(API_METALAKES_USERS_PATH, name(), BLANK_PLACE_HOLDER), + NameListResponse.class, + Collections.emptyMap(), + ErrorHandlers.userErrorHandler()); + resp.validate(); + + return resp.getNames(); + } + /** * Adds a new Group. * diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/TestUserGroup.java b/clients/client-java/src/test/java/org/apache/gravitino/client/TestUserGroup.java index f3885a05f9c..67a3035ed8b 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/TestUserGroup.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/TestUserGroup.java @@ -24,6 +24,8 @@ import static org.apache.hc.core5.http.HttpStatus.SC_SERVER_ERROR; import java.time.Instant; +import java.util.Collections; +import java.util.Map; import org.apache.gravitino.authorization.Group; import org.apache.gravitino.authorization.User; import org.apache.gravitino.dto.AuditDTO; @@ -35,7 +37,9 @@ import org.apache.gravitino.dto.responses.ErrorResponse; import org.apache.gravitino.dto.responses.GroupResponse; import org.apache.gravitino.dto.responses.MetalakeResponse; +import org.apache.gravitino.dto.responses.NameListResponse; import org.apache.gravitino.dto.responses.RemoveResponse; +import org.apache.gravitino.dto.responses.UserListResponse; import org.apache.gravitino.dto.responses.UserResponse; import org.apache.gravitino.exceptions.GroupAlreadyExistsException; import org.apache.gravitino.exceptions.NoSuchGroupException; @@ -175,6 +179,56 @@ public void testRemoveUsers() throws Exception { Assertions.assertThrows(RuntimeException.class, () -> gravitinoClient.removeUser(username)); } + @Test + public void testListUserNames() throws Exception { + String userPath = withSlash(String.format(API_METALAKES_USERS_PATH, metalakeName, "")); + + NameListResponse listResponse = new NameListResponse(new String[] {"user1", "user2"}); + buildMockResource(Method.GET, userPath, null, listResponse, SC_OK); + + Assertions.assertArrayEquals(new String[] {"user1", "user2"}, gravitinoClient.listUserNames()); + + ErrorResponse errRespNoMetalake = + ErrorResponse.notFound(NoSuchMetalakeException.class.getSimpleName(), "metalake not found"); + buildMockResource(Method.GET, userPath, null, errRespNoMetalake, SC_NOT_FOUND); + Exception ex = + Assertions.assertThrows( + NoSuchMetalakeException.class, () -> gravitinoClient.listUserNames()); + Assertions.assertEquals("metalake not found", ex.getMessage()); + + // Test RuntimeException + ErrorResponse errResp = ErrorResponse.internalError("internal error"); + buildMockResource(Method.GET, userPath, null, errResp, SC_SERVER_ERROR); + Assertions.assertThrows(RuntimeException.class, () -> gravitinoClient.listUserNames()); + } + + @Test + public void testListUsers() throws Exception { + String userPath = withSlash(String.format(API_METALAKES_USERS_PATH, metalakeName, "")); + UserDTO user1 = mockUserDTO("user1"); + UserDTO user2 = mockUserDTO("user2"); + Map params = Collections.singletonMap("details", "true"); + UserListResponse listResponse = new UserListResponse(new UserDTO[] {user1, user2}); + buildMockResource(Method.GET, userPath, params, null, listResponse, SC_OK); + + User[] users = gravitinoClient.listUsers(); + Assertions.assertEquals(2, users.length); + assertUser(user1, users[0]); + assertUser(user2, users[1]); + + ErrorResponse errRespNoMetalake = + ErrorResponse.notFound(NoSuchMetalakeException.class.getSimpleName(), "metalake not found"); + buildMockResource(Method.GET, userPath, params, null, errRespNoMetalake, SC_NOT_FOUND); + Exception ex = + Assertions.assertThrows(NoSuchMetalakeException.class, () -> gravitinoClient.listUsers()); + Assertions.assertEquals("metalake not found", ex.getMessage()); + + // Test RuntimeException + ErrorResponse errResp = ErrorResponse.internalError("internal error"); + buildMockResource(Method.GET, userPath, params, null, errResp, SC_SERVER_ERROR); + Assertions.assertThrows(RuntimeException.class, () -> gravitinoClient.listUsers()); + } + @Test public void testAddGroups() throws Exception { String groupName = "group"; diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java index 76f2c1b0fe8..90c77ce2c79 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java @@ -20,8 +20,10 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import java.util.Arrays; import java.util.Collections; import java.util.Map; +import java.util.stream.Collectors; import org.apache.gravitino.Configs; import org.apache.gravitino.auth.AuthConstants; import org.apache.gravitino.authorization.Group; @@ -73,11 +75,27 @@ void testManageUsers() { Assertions.assertEquals(username, user.name()); Assertions.assertTrue(user.roles().isEmpty()); + // List users + String anotherUser = "another-user"; + metalake.addUser(anotherUser); + String[] usernames = metalake.listUserNames(); + Assertions.assertEquals( + Lists.newArrayList(AuthConstants.ANONYMOUS_USER, anotherUser, username), + Arrays.asList(usernames)); + User[] users = metalake.listUsers(); + Assertions.assertEquals( + Lists.newArrayList(AuthConstants.ANONYMOUS_USER, anotherUser, username), + Arrays.stream(users).map(User::name).collect(Collectors.toList())); + // Get a not-existed user Assertions.assertThrows(NoSuchUserException.class, () -> metalake.getUser("not-existed")); Assertions.assertTrue(metalake.removeUser(username)); + Assertions.assertFalse(metalake.removeUser(username)); + + // clean up + metalake.removeUser(anotherUser); } @Test diff --git a/common/src/main/java/org/apache/gravitino/dto/responses/UserListResponse.java b/common/src/main/java/org/apache/gravitino/dto/responses/UserListResponse.java new file mode 100644 index 00000000000..2b591184a24 --- /dev/null +++ b/common/src/main/java/org/apache/gravitino/dto/responses/UserListResponse.java @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.dto.responses; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.google.common.base.Preconditions; +import lombok.EqualsAndHashCode; +import lombok.Getter; +import lombok.ToString; +import org.apache.gravitino.dto.authorization.UserDTO; + +/** Represents a response containing a list of users. */ +@Getter +@ToString +@EqualsAndHashCode(callSuper = true) +public class UserListResponse extends BaseResponse { + + @JsonProperty("users") + private final UserDTO[] users; + + /** + * Constructor for UserListResponse. + * + * @param users The array of users. + */ + public UserListResponse(UserDTO[] users) { + super(0); + this.users = users; + } + + /** + * This is the constructor that is used by Jackson deserializer to create an instance of + * UserListResponse. + */ + public UserListResponse() { + super(0); + this.users = null; + } + + /** + * Validates the response data. + * + * @throws IllegalArgumentException if users are not set. + */ + @Override + public void validate() throws IllegalArgumentException { + super.validate(); + Preconditions.checkArgument(users != null, "users must not be null"); + } +} diff --git a/common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java b/common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java index d83460af182..8e706c139e9 100644 --- a/common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java +++ b/common/src/main/java/org/apache/gravitino/dto/util/DTOConverters.java @@ -678,6 +678,19 @@ public static CatalogDTO[] toDTOs(Catalog[] catalogs) { return Arrays.stream(catalogs).map(DTOConverters::toDTO).toArray(CatalogDTO[]::new); } + /** + * Converts an array of Users to an array of UserDTOs. + * + * @param users The users to be converted. + * @return The array of UserDTOs. + */ + public static UserDTO[] toDTOs(User[] users) { + if (ArrayUtils.isEmpty(users)) { + return new UserDTO[0]; + } + return Arrays.stream(users).map(DTOConverters::toDTO).toArray(UserDTO[]::new); + } + /** * Converts a DistributionDTO to a Distribution. * diff --git a/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java b/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java index fabc8acaaf3..fbeebd9449e 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/authorization/AccessControlDispatcher.java @@ -71,6 +71,24 @@ User addUser(String metalake, String user) */ User getUser(String metalake, String user) throws NoSuchUserException, NoSuchMetalakeException; + /** + * Lists the users. + * + * @param metalake The Metalake of the User. + * @return The User list. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + */ + User[] listUsers(String metalake) throws NoSuchMetalakeException; + + /** + * Lists the usernames. + * + * @param metalake The Metalake of the User. + * @return The username list. + * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. + */ + String[] listUserNames(String metalake) throws NoSuchMetalakeException; + /** * Adds a new Group. * diff --git a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java index aa890667dd4..222b1ffb5ae 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/AccessControlManager.java @@ -69,6 +69,15 @@ public User getUser(String metalake, String user) } @Override + public String[] listUserNames(String metalake) throws NoSuchMetalakeException { + return userGroupManager.listUserNames(metalake); + } + + @Override + public User[] listUsers(String metalake) throws NoSuchMetalakeException { + return userGroupManager.listUsers(metalake); + } + public Group addGroup(String metalake, String group) throws GroupAlreadyExistsException, NoSuchMetalakeException { return userGroupManager.addGroup(metalake, group); @@ -130,16 +139,6 @@ public Role getRole(String metalake, String role) return roleManager.getRole(metalake, role); } - /** - * Deletes a Role. - * - * @param metalake The Metalake of the Role. - * @param role The name of the Role. - * @return True if the Role was successfully deleted, false only when there's no such role, - * otherwise it will throw an exception. - * @throws NoSuchMetalakeException If the Metalake with the given name does not exist. - * @throws RuntimeException If deleting the Role encounters storage issues. - */ public boolean deleteRole(String metalake, String role) throws NoSuchMetalakeException { return roleManager.deleteRole(metalake, role); } diff --git a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java index 09427668969..058eaa95c3c 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java @@ -21,13 +21,16 @@ 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; import org.apache.gravitino.EntityStore; +import org.apache.gravitino.Namespace; import org.apache.gravitino.exceptions.GroupAlreadyExistsException; import org.apache.gravitino.exceptions.NoSuchEntityException; import org.apache.gravitino.exceptions.NoSuchGroupException; +import org.apache.gravitino.exceptions.NoSuchMetalakeException; import org.apache.gravitino.exceptions.NoSuchUserException; import org.apache.gravitino.exceptions.UserAlreadyExistsException; import org.apache.gravitino.meta.AuditInfo; @@ -46,6 +49,7 @@ class UserGroupManager { private static final Logger LOG = LoggerFactory.getLogger(UserGroupManager.class); + private static final String METALAKE_DOES_NOT_EXIST_MSG = "Metalake %s does not exist"; private final EntityStore store; private final IdGenerator idGenerator; @@ -109,6 +113,25 @@ User getUser(String metalake, String user) throws NoSuchUserException { } } + String[] listUserNames(String metalake) { + return Arrays.stream(listUsers(metalake)).map(User::name).toArray(String[]::new); + } + + User[] listUsers(String metalake) { + try { + Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake); + return store.list(namespace, UserEntity.class, Entity.EntityType.USER).stream() + .map(entity -> (User) entity) + .toArray(User[]::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); + } + } + Group addGroup(String metalake, String group) throws GroupAlreadyExistsException { try { AuthorizationUtils.checkMetalakeExists(metalake); diff --git a/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java b/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java index 44dc491a722..730563862e7 100644 --- a/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/hook/AccessControlHookDispatcher.java @@ -69,6 +69,16 @@ public User getUser(String metalake, String user) return dispatcher.getUser(metalake, user); } + @Override + public User[] listUsers(String metalake) throws NoSuchMetalakeException { + return dispatcher.listUsers(metalake); + } + + @Override + public String[] listUserNames(String metalake) throws NoSuchMetalakeException { + return dispatcher.listUserNames(metalake); + } + @Override public Group addGroup(String metalake, String group) throws GroupAlreadyExistsException, NoSuchMetalakeException { diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java index b23c7667388..4164594f500 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java @@ -104,6 +104,8 @@ public List list( return (List) TopicMetaService.getInstance().listTopicsByNamespace(namespace); case TAG: return (List) TagMetaService.getInstance().listTagsByNamespace(namespace); + case USER: + return (List) UserMetaService.getInstance().listUsersByNamespace(namespace); default: throw new UnsupportedEntityTypeException( "Unsupported entity type: %s for list operation", entityType); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java index a5db8e0f943..cfba535dcf7 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java @@ -138,6 +138,21 @@ public String listUsersByRoleId(@Param("roleId") Long roleId) { + " AND us.deleted_at = 0 AND re.deleted_at = 0"; } + public String listUserPOsByMetalake(@Param("metalakeName") String metalakeName) { + return "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 + + " 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"; + } + public String deleteUserMetasByLegacyTimeline( @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { return "DELETE FROM " 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 index ad794c39530..b5e1dc67c0e 100644 --- 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 @@ -54,6 +54,9 @@ UserPO selectUserMetaByMetalakeIdAndName( @InsertProvider(type = UserMetaSQLProviderFactory.class, method = "insertUserMeta") void insertUserMeta(@Param("userMeta") UserPO userPO); + @SelectProvider(type = UserMetaSQLProviderFactory.class, method = "listUserPOsByMetalake") + List listUserPOsByMetalake(@Param("metalakeName") String metalakeName); + @InsertProvider( type = UserMetaSQLProviderFactory.class, method = "insertUserMetaOnDuplicateKeyUpdate") diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java index ab49a454221..0a6dfae4eab 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java @@ -85,6 +85,10 @@ public static String listUsersByRoleId(@Param("roleId") Long roleId) { return getProvider().listUsersByRoleId(roleId); } + public static String listUserPOsByMetalake(@Param("metalakeName") String metalakeName) { + return getProvider().listUserPOsByMetalake(metalakeName); + } + public static String deleteUserMetasByLegacyTimeline( @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { return getProvider().deleteUserMetasByLegacyTimeline(legacyTimeline, limit); 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 index e7d0a435a1b..11e0833fe03 100644 --- 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 @@ -32,6 +32,7 @@ import org.apache.gravitino.Entity; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.exceptions.NoSuchEntityException; import org.apache.gravitino.meta.RoleEntity; @@ -246,6 +247,24 @@ public UserEntity updateUser( return newEntity; } + public List listUsersByNamespace(Namespace namespace) { + AuthorizationUtils.checkUserNamespace(namespace); + String metalakeName = namespace.level(0); + + List userPOs = + SessionUtils.getWithoutCommit( + UserMetaMapper.class, mapper -> mapper.listUserPOsByMetalake(metalakeName)); + + return userPOs.stream() + .map( + po -> + POConverters.fromUserPO( + po, + SupplierUtils.createRolePOsSupplier(po), + AuthorizationUtils.ofUserNamespace(metalakeName))) + .collect(Collectors.toList()); + } + public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { int[] userDeletedCount = new int[] {0}; int[] userRoleRelDeletedCount = new int[] {0}; diff --git a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java index fd27771a088..19c2f8f25c8 100644 --- a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java +++ b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java @@ -76,6 +76,15 @@ public class TestAccessControlManager { .withVersion(SchemaVersion.V_0_1) .build(); + private static BaseMetalake listMetalakeEntity = + BaseMetalake.builder() + .withId(1L) + .withName("metalake_list") + .withAuditInfo( + AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) + .withVersion(SchemaVersion.V_0_1) + .build(); + @BeforeAll public static void setUp() throws Exception { config = new Config(false) {}; @@ -86,6 +95,7 @@ public static void setUp() throws Exception { entityStore.setSerDe(null); entityStore.put(metalakeEntity, true); + entityStore.put(listMetalakeEntity, true); accessControlManager = new AccessControlManager(entityStore, new RandomIdGenerator(), config); FieldUtils.writeField(GravitinoEnv.getInstance(), "entityStore", entityStore, true); @@ -162,6 +172,21 @@ public void testRemoveUser() { Assertions.assertFalse(removed1); } + @Test + public void testListUsers() { + accessControlManager.addUser("metalake_list", "testList1"); + accessControlManager.addUser("metalake_list", "testList2"); + + // Test to list users + Assertions.assertArrayEquals( + new String[] {"testList2", "testList1"}, + accessControlManager.listUserNames("metalake_list")); + User[] users = accessControlManager.listUsers("metalake_list"); + Assertions.assertEquals(2, users.length); + Assertions.assertEquals("testList1", users[1].name()); + Assertions.assertEquals("testList2", users[0].name()); + } + @Test public void testAddGroup() { Group group = accessControlManager.addGroup("metalake", "testAdd"); 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 index 326ccfc2da3..a8e045eca84 100644 --- 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 @@ -118,6 +118,37 @@ void getUserByIdentifier() throws IOException { Sets.newHashSet(user2.roleNames()), Sets.newHashSet(actualUser.roleNames())); } + @Test + void testListUsers() throws IOException { + AuditInfo auditInfo = + AuditInfo.builder().withCreator("creator").withCreateTime(Instant.now()).build(); + BaseMetalake metalake = + createBaseMakeLake(RandomIdGenerator.INSTANCE.nextId(), metalakeName, auditInfo); + backend.insert(metalake, false); + + UserEntity user1 = + createUserEntity( + RandomIdGenerator.INSTANCE.nextId(), + AuthorizationUtils.ofUserNamespace(metalakeName), + "user1", + auditInfo); + + UserEntity user2 = + createUserEntity( + RandomIdGenerator.INSTANCE.nextId(), + AuthorizationUtils.ofUserNamespace(metalakeName), + "user2", + auditInfo); + + backend.insert(user1, false); + backend.insert(user2, false); + + UserMetaService userMetaService = UserMetaService.getInstance(); + Assertions.assertEquals( + Lists.newArrayList(user1, user2), + userMetaService.listUsersByNamespace(AuthorizationUtils.ofUserNamespace(metalakeName))); + } + @Test void insertUser() throws IOException { AuditInfo auditInfo = diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java index 1d93e0e6afa..24f34d652ab 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/UserOperations.java @@ -22,11 +22,13 @@ import com.codahale.metrics.annotation.Timed; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.DELETE; +import javax.ws.rs.DefaultValue; import javax.ws.rs.GET; import javax.ws.rs.POST; import javax.ws.rs.Path; import javax.ws.rs.PathParam; import javax.ws.rs.Produces; +import javax.ws.rs.QueryParam; import javax.ws.rs.core.Context; import javax.ws.rs.core.Response; import org.apache.gravitino.GravitinoEnv; @@ -34,7 +36,9 @@ import org.apache.gravitino.authorization.AccessControlDispatcher; import org.apache.gravitino.authorization.AuthorizationUtils; import org.apache.gravitino.dto.requests.UserAddRequest; +import org.apache.gravitino.dto.responses.NameListResponse; import org.apache.gravitino.dto.responses.RemoveResponse; +import org.apache.gravitino.dto.responses.UserListResponse; import org.apache.gravitino.dto.responses.UserResponse; import org.apache.gravitino.dto.util.DTOConverters; import org.apache.gravitino.lock.LockType; @@ -84,6 +88,35 @@ public Response getUser(@PathParam("metalake") String metalake, @PathParam("user } } + @GET + @Produces("application/vnd.gravitino.v1+json") + @Timed(name = "list-user." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) + @ResponseMetered(name = "list-user", absolute = true) + public Response listUsers( + @PathParam("metalake") String metalake, + @QueryParam("details") @DefaultValue("false") boolean verbose) { + try { + return Utils.doAs( + httpRequest, + () -> + TreeLockUtils.doWithTreeLock( + NameIdentifier.of(AuthorizationUtils.ofUserNamespace(metalake).levels()), + LockType.READ, + () -> { + if (verbose) { + return Utils.ok( + new UserListResponse( + DTOConverters.toDTOs(accessControlManager.listUsers(metalake)))); + } else { + return Utils.ok( + new NameListResponse(accessControlManager.listUserNames(metalake))); + } + })); + } catch (Exception e) { + return ExceptionHandlers.handleUserException(OperationType.LIST, "", metalake, e); + } + } + @POST @Produces("application/vnd.gravitino.v1+json") @Timed(name = "add-user." + MetricNames.HTTP_PROCESS_DURATION, absolute = true) diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java index d3209e0e22c..7f570e779f4 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestUserOperations.java @@ -43,7 +43,9 @@ import org.apache.gravitino.dto.requests.UserAddRequest; import org.apache.gravitino.dto.responses.ErrorConstants; import org.apache.gravitino.dto.responses.ErrorResponse; +import org.apache.gravitino.dto.responses.NameListResponse; import org.apache.gravitino.dto.responses.RemoveResponse; +import org.apache.gravitino.dto.responses.UserListResponse; import org.apache.gravitino.dto.responses.UserResponse; import org.apache.gravitino.exceptions.NoSuchMetalakeException; import org.apache.gravitino.exceptions.NoSuchUserException; @@ -294,4 +296,103 @@ public void testRemoveUser() { Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResponse.getCode()); Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResponse.getType()); } + + @Test + public void testListUsernames() { + when(manager.listUserNames(any())).thenReturn(new String[] {"user"}); + + Response resp = + target("/metalakes/metalake1/users/") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .get(); + Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp.getStatus()); + + NameListResponse listResponse = resp.readEntity(NameListResponse.class); + Assertions.assertEquals(0, listResponse.getCode()); + + Assertions.assertEquals(1, listResponse.getNames().length); + Assertions.assertEquals("user", listResponse.getNames()[0]); + + // Test to throw NoSuchMetalakeException + doThrow(new NoSuchMetalakeException("mock error")).when(manager).listUserNames(any()); + Response resp1 = + target("/metalakes/metalake1/users/") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .get(); + + Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(), resp1.getStatus()); + + ErrorResponse errorResponse = resp1.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE, errorResponse.getCode()); + Assertions.assertEquals(NoSuchMetalakeException.class.getSimpleName(), errorResponse.getType()); + + // Test to throw internal RuntimeException + doThrow(new RuntimeException("mock error")).when(manager).listUserNames(any()); + Response resp3 = + target("/metalakes/metalake1/users") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .get(); + + Assertions.assertEquals( + Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), resp3.getStatus()); + + ErrorResponse errorResponse2 = resp3.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResponse2.getCode()); + Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResponse2.getType()); + } + + @Test + public void testListUsers() { + User user = buildUser("user"); + when(manager.listUsers(any())).thenReturn(new User[] {user}); + + Response resp = + target("/metalakes/metalake1/users/") + .queryParam("details", "true") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .get(); + Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp.getStatus()); + + UserListResponse listResponse = resp.readEntity(UserListResponse.class); + Assertions.assertEquals(0, listResponse.getCode()); + + Assertions.assertEquals(1, listResponse.getUsers().length); + Assertions.assertEquals(user.name(), listResponse.getUsers()[0].name()); + Assertions.assertEquals(user.roles(), listResponse.getUsers()[0].roles()); + + // Test to throw NoSuchMetalakeException + doThrow(new NoSuchMetalakeException("mock error")).when(manager).listUsers(any()); + Response resp1 = + target("/metalakes/metalake1/users/") + .queryParam("details", "true") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .get(); + + Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(), resp1.getStatus()); + + ErrorResponse errorResponse = resp1.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE, errorResponse.getCode()); + Assertions.assertEquals(NoSuchMetalakeException.class.getSimpleName(), errorResponse.getType()); + + // Test to throw internal RuntimeException + doThrow(new RuntimeException("mock error")).when(manager).listUsers(any()); + Response resp3 = + target("/metalakes/metalake1/users") + .queryParam("details", "true") + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .get(); + + Assertions.assertEquals( + Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), resp3.getStatus()); + + ErrorResponse errorResponse2 = resp3.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResponse2.getCode()); + Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResponse2.getType()); + } } From 4b333a78d0ca00ef70da8e4373cd38500f933100 Mon Sep 17 00:00:00 2001 From: Rory Date: Sat, 14 Sep 2024 10:06:12 +0800 Subject: [PATCH 02/31] Add the metalake check --- .../org/apache/gravitino/authorization/UserGroupManager.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java index 058eaa95c3c..c7718912bdd 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java @@ -119,6 +119,8 @@ String[] listUserNames(String metalake) { User[] listUsers(String metalake) { try { + AuthorizationUtils.checkMetalakeExists(metalake); + Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake); return store.list(namespace, UserEntity.class, Entity.EntityType.USER).stream() .map(entity -> (User) entity) From 92c303259a42a5bd3afd5284d40a5c2a44e3d2ff Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 18 Sep 2024 17:46:18 +0800 Subject: [PATCH 03/31] Use join --- .../hadoop/HadoopCatalogOperations.java | 4 +- .../catalog/kafka/KafkaCatalogOperations.java | 2 +- .../test/authorization/AccessControlIT.java | 6 +- .../org/apache/gravitino/EntityStore.java | 10 ++- .../authorization/UserGroupManager.java | 9 ++- .../gravitino/catalog/CatalogManager.java | 7 +- .../gravitino/metalake/MetalakeManager.java | 2 +- .../gravitino/storage/kv/KvEntityStore.java | 3 +- .../storage/relational/JDBCBackend.java | 6 +- .../storage/relational/RelationalBackend.java | 6 +- .../relational/RelationalEntityStore.java | 5 +- .../mapper/UserMetaBaseSQLProvider.java | 22 ++++++ .../relational/mapper/UserMetaMapper.java | 6 ++ .../mapper/UserMetaSQLProviderFactory.java | 7 +- .../mapper/h2/UserMetaH2Provider.java | 52 +++++++++++++ .../storage/relational/po/CombinedUserPO.java | 73 +++++++++++++++++++ .../relational/service/UserMetaService.java | 39 ++++++---- .../relational/utils/POConverters.java | 39 ++++++++++ .../org/apache/gravitino/tag/TagManager.java | 3 +- .../storage/kv/TestKvEntityStorage.java | 2 +- .../storage/memory/TestMemoryEntityStore.java | 3 +- .../storage/relational/TestJDBCBackend.java | 17 +++-- .../service/TestUserMetaService.java | 39 ++++++++-- 23 files changed, 313 insertions(+), 49 deletions(-) create mode 100644 core/src/main/java/org/apache/gravitino/storage/relational/mapper/h2/UserMetaH2Provider.java create mode 100644 core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java diff --git a/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java b/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java index 1946464911f..2b68312ea18 100644 --- a/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java +++ b/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java @@ -149,7 +149,7 @@ public NameIdentifier[] listFilesets(Namespace namespace) throws NoSuchSchemaExc } List filesets = - store.list(namespace, FilesetEntity.class, Entity.EntityType.FILESET); + store.list(namespace, FilesetEntity.class, Entity.EntityType.FILESET, true); return filesets.stream() .map(f -> NameIdentifier.of(namespace, f.name())) .toArray(NameIdentifier[]::new); @@ -387,7 +387,7 @@ public String getFileLocation(NameIdentifier ident, String subPath) public NameIdentifier[] listSchemas(Namespace namespace) throws NoSuchCatalogException { try { List schemas = - store.list(namespace, SchemaEntity.class, Entity.EntityType.SCHEMA); + store.list(namespace, SchemaEntity.class, Entity.EntityType.SCHEMA, true); return schemas.stream() .map(s -> NameIdentifier.of(namespace, s.name())) .toArray(NameIdentifier[]::new); diff --git a/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java b/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java index 66f49a02c24..b360626bea5 100644 --- a/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java +++ b/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java @@ -391,7 +391,7 @@ public boolean dropTopic(NameIdentifier ident) { public NameIdentifier[] listSchemas(Namespace namespace) throws NoSuchCatalogException { try { List schemas = - store.list(namespace, SchemaEntity.class, Entity.EntityType.SCHEMA); + store.list(namespace, SchemaEntity.class, Entity.EntityType.SCHEMA, true); return schemas.stream() .map(s -> NameIdentifier.of(namespace, s.name())) .toArray(NameIdentifier[]::new); diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java index 90c77ce2c79..1ff39943646 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java @@ -79,13 +79,17 @@ void testManageUsers() { String anotherUser = "another-user"; metalake.addUser(anotherUser); String[] usernames = metalake.listUserNames(); + Arrays.sort(usernames); Assertions.assertEquals( Lists.newArrayList(AuthConstants.ANONYMOUS_USER, anotherUser, username), Arrays.asList(usernames)); User[] users = metalake.listUsers(); Assertions.assertEquals( Lists.newArrayList(AuthConstants.ANONYMOUS_USER, anotherUser, username), - Arrays.stream(users).map(User::name).collect(Collectors.toList())); + Arrays.stream(users) + .map(User::name) + .sorted(String::compareTo) + .collect(Collectors.toList())); // Get a not-existed user Assertions.assertThrows(NoSuchUserException.class, () -> metalake.getUser("not-existed")); diff --git a/core/src/main/java/org/apache/gravitino/EntityStore.java b/core/src/main/java/org/apache/gravitino/EntityStore.java index 1112efc4b8d..b6d04628da1 100644 --- a/core/src/main/java/org/apache/gravitino/EntityStore.java +++ b/core/src/main/java/org/apache/gravitino/EntityStore.java @@ -55,15 +55,19 @@ public interface EntityStore extends Closeable { *

Note. Depends on the isolation levels provided by the underlying storage, the returned list * may not be consistent. * - * @param namespace the namespace of the entities * @param class of the entity + * @param namespace the namespace of the entities * @param type the detailed type of the entity * @param entityType the general type of the entity - * @throws IOException if the list operation fails + * @param includeAllFields Some fields will have heavier cost, EntityStore provide an option to + * avoid fetching them to improve the performance. If true, the store fetch all the fields, + * otherwise false. * @return the list of entities + * @throws IOException if the list operation fails */ List list( - Namespace namespace, Class type, EntityType entityType) throws IOException; + Namespace namespace, Class type, EntityType entityType, boolean includeAllFields) + throws IOException; /** * Check if the entity with the specified {@link org.apache.gravitino.NameIdentifier} exists. diff --git a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java index c7718912bdd..79e4ee24455 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java @@ -114,15 +114,20 @@ User getUser(String metalake, String user) throws NoSuchUserException { } String[] listUserNames(String metalake) { - return Arrays.stream(listUsers(metalake)).map(User::name).toArray(String[]::new); + return Arrays.stream(listUsersInternal(metalake, false)).map(User::name).toArray(String[]::new); } User[] listUsers(String metalake) { + return listUsersInternal(metalake, true); + } + + private User[] listUsersInternal(String metalake, boolean includeAllFields) { try { AuthorizationUtils.checkMetalakeExists(metalake); Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake); - return store.list(namespace, UserEntity.class, Entity.EntityType.USER).stream() + return store.list(namespace, UserEntity.class, Entity.EntityType.USER, includeAllFields) + .stream() .map(entity -> (User) entity) .toArray(User[]::new); } catch (NoSuchEntityException e) { diff --git a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java index f351a3271fe..980fe1d6b01 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java +++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java @@ -276,7 +276,7 @@ public NameIdentifier[] listCatalogs(Namespace namespace) throws NoSuchMetalakeE checkMetalakeExists(metalakeIdent); try { - return store.list(namespace, CatalogEntity.class, EntityType.CATALOG).stream() + return store.list(namespace, CatalogEntity.class, EntityType.CATALOG, true).stream() .map(entity -> NameIdentifier.of(namespace, entity.name())) .toArray(NameIdentifier[]::new); @@ -293,7 +293,7 @@ public Catalog[] listCatalogsInfo(Namespace namespace) throws NoSuchMetalakeExce try { List catalogEntities = - store.list(namespace, CatalogEntity.class, EntityType.CATALOG); + store.list(namespace, CatalogEntity.class, EntityType.CATALOG, true); // Using provider as key to avoid loading the same type catalog instance multiple times Map> hiddenProps = new HashMap<>(); @@ -573,7 +573,8 @@ public boolean dropCatalog(NameIdentifier ident) { store.list( Namespace.of(ident.namespace().level(0), ident.name()), SchemaEntity.class, - EntityType.SCHEMA); + EntityType.SCHEMA, + true); // If there is only one schema, it must be the default schema, because we don't allow to // drop the default schema. if (schemas.size() == 1) { diff --git a/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java b/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java index 54298c0efdd..38db573aade 100644 --- a/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java +++ b/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java @@ -72,7 +72,7 @@ public MetalakeManager(EntityStore store, IdGenerator idGenerator) { @Override public BaseMetalake[] listMetalakes() { try { - return store.list(Namespace.empty(), BaseMetalake.class, EntityType.METALAKE).stream() + return store.list(Namespace.empty(), BaseMetalake.class, EntityType.METALAKE, true).stream() .toArray(BaseMetalake[]::new); } catch (IOException ioe) { LOG.error("Listing Metalakes failed due to storage issues.", ioe); diff --git a/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java b/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java index 20a9fb79eb0..1f0cd902733 100644 --- a/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java +++ b/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java @@ -126,7 +126,8 @@ public void setSerDe(EntitySerDe entitySerDe) { @Override public List list( - Namespace namespace, Class e, EntityType type) throws IOException { + Namespace namespace, Class e, EntityType type, boolean includeAllFields) + throws IOException { // Star means it's a wildcard List entities = Lists.newArrayList(); NameIdentifier identifier = NameIdentifier.of(namespace, BinaryEntityKeyEncoder.WILD_CARD); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java index 4164594f500..84dc55441f1 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java @@ -88,7 +88,8 @@ public void initialize(Config config) { @Override public List list( - Namespace namespace, Entity.EntityType entityType) throws IOException { + Namespace namespace, Entity.EntityType entityType, boolean includeAllFields) + throws IOException { switch (entityType) { case METALAKE: return (List) MetalakeMetaService.getInstance().listMetalakes(); @@ -105,7 +106,8 @@ public List list( case TAG: return (List) TagMetaService.getInstance().listTagsByNamespace(namespace); case USER: - return (List) UserMetaService.getInstance().listUsersByNamespace(namespace); + return (List) + UserMetaService.getInstance().listUsersByNamespace(namespace, includeAllFields); default: throw new UnsupportedEntityTypeException( "Unsupported entity type: %s for list operation", entityType); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java index f15060e7426..d5ba06a24d8 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java @@ -48,13 +48,17 @@ public interface RelationalBackend * * @param namespace The parent namespace of these entities. * @param entityType The type of these entities. + * @param includeAllFields Some fields will have heavier cost, EntityStore provide an option to + * avoid fetching them to improve the performance. If true, the store fetch all the fields, + * otherwise false. * @return The list of entities associated with the given parent namespace and entityType, or null * if the entities does not exist. * @throws NoSuchEntityException If the corresponding parent entity of these list entities cannot * be found. * @throws IOException If the store operation fails */ - List list(Namespace namespace, Entity.EntityType entityType) + List list( + Namespace namespace, Entity.EntityType entityType, boolean includeAllFields) throws NoSuchEntityException, IOException; /** diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java index 7eb1432c590..66f870d2435 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java @@ -88,8 +88,9 @@ public void setSerDe(EntitySerDe entitySerDe) { @Override public List list( - Namespace namespace, Class type, Entity.EntityType entityType) throws IOException { - return backend.list(namespace, entityType); + Namespace namespace, Class type, Entity.EntityType entityType, boolean includeAllFields) + throws IOException { + return backend.list(namespace, entityType, true); } @Override diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java index cfba535dcf7..ca440b31581 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java @@ -19,6 +19,7 @@ package org.apache.gravitino.storage.relational.mapper; +import static org.apache.gravitino.storage.relational.mapper.RoleMetaMapper.ROLE_TABLE_NAME; import static org.apache.gravitino.storage.relational.mapper.UserMetaMapper.USER_ROLE_RELATION_TABLE_NAME; import static org.apache.gravitino.storage.relational.mapper.UserRoleRelMapper.USER_TABLE_NAME; @@ -153,6 +154,27 @@ public String listUserPOsByMetalake(@Param("metalakeName") String metalakeName) + " AND ut.deleted_at = 0 AND mt.deleted_at = 0"; } + public String listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { + return "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," + + " JSON_ARRAYAGG(rot.role_name) as roleNames," + + " JSON_ARRAYAGG(rot.role_id) as roleIds" + + " FROM " + + USER_TABLE_NAME + + " ut JOIN " + + USER_ROLE_RELATION_TABLE_NAME + + " rt ON rt.user_id = ut.user_id" + + " JOIN " + + ROLE_TABLE_NAME + + " rot ON rot.role_id = rt.role_id" + + " WHERE " + + " ut.deleted_at = 0 AND rot.deleted_at = 0 AND rt.deleted_at = 0 AND ut.metalake_id = #{metalakeId}" + + " GROUP BY ut.user_id"; + } + public String deleteUserMetasByLegacyTimeline( @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { return "DELETE FROM " 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 index b5e1dc67c0e..47077e83b7f 100644 --- 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 @@ -20,6 +20,7 @@ package org.apache.gravitino.storage.relational.mapper; import java.util.List; +import org.apache.gravitino.storage.relational.po.CombinedUserPO; import org.apache.gravitino.storage.relational.po.UserPO; import org.apache.ibatis.annotations.DeleteProvider; import org.apache.ibatis.annotations.InsertProvider; @@ -57,6 +58,11 @@ UserPO selectUserMetaByMetalakeIdAndName( @SelectProvider(type = UserMetaSQLProviderFactory.class, method = "listUserPOsByMetalake") List listUserPOsByMetalake(@Param("metalakeName") String metalakeName); + @SelectProvider( + type = UserMetaSQLProviderFactory.class, + method = "listCombinedUserPOsByMetalakeId") + List listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId); + @InsertProvider( type = UserMetaSQLProviderFactory.class, method = "insertUserMetaOnDuplicateKeyUpdate") diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java index 0a6dfae4eab..b58c43291d3 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableMap; import java.util.Map; import org.apache.gravitino.storage.relational.JDBCBackend.JDBCBackendType; +import org.apache.gravitino.storage.relational.mapper.h2.UserMetaH2Provider; import org.apache.gravitino.storage.relational.mapper.postgresql.UserMetaPostgreSQLProvider; import org.apache.gravitino.storage.relational.po.UserPO; import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; @@ -48,8 +49,6 @@ public static UserMetaBaseSQLProvider getProvider() { static class UserMetaMySQLProvider extends UserMetaBaseSQLProvider {} - static class UserMetaH2Provider extends UserMetaBaseSQLProvider {} - public static String selectUserIdByMetalakeIdAndName( @Param("metalakeId") Long metalakeId, @Param("userName") String userName) { return getProvider().selectUserIdByMetalakeIdAndName(metalakeId, userName); @@ -89,6 +88,10 @@ public static String listUserPOsByMetalake(@Param("metalakeName") String metalak return getProvider().listUserPOsByMetalake(metalakeName); } + public static String listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { + return getProvider().listCombinedUserPOsByMetalakeId(metalakeId); + } + public static String deleteUserMetasByLegacyTimeline( @Param("legacyTimeline") Long legacyTimeline, @Param("limit") int limit) { return getProvider().deleteUserMetasByLegacyTimeline(legacyTimeline, limit); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/h2/UserMetaH2Provider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/h2/UserMetaH2Provider.java new file mode 100644 index 00000000000..d4d4621bc08 --- /dev/null +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/h2/UserMetaH2Provider.java @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.storage.relational.mapper.h2; + +import static org.apache.gravitino.storage.relational.mapper.RoleMetaMapper.ROLE_TABLE_NAME; +import static org.apache.gravitino.storage.relational.mapper.UserMetaMapper.USER_ROLE_RELATION_TABLE_NAME; +import static org.apache.gravitino.storage.relational.mapper.UserRoleRelMapper.USER_TABLE_NAME; + +import org.apache.gravitino.storage.relational.mapper.UserMetaBaseSQLProvider; +import org.apache.ibatis.annotations.Param; + +public class UserMetaH2Provider extends UserMetaBaseSQLProvider { + @Override + public String listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { + return "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," + + " '[' || GROUP_CONCAT('\"' || rot.role_name || '\"') || ']' as roleNames," + + " '[' || GROUP_CONCAT('\"' || rot.role_id || '\"') || ']' as roleIds" + + " FROM " + + USER_TABLE_NAME + + " ut LEFT OUTER JOIN " + + USER_ROLE_RELATION_TABLE_NAME + + " rt ON rt.user_id = ut.user_id" + + " LEFT OUTER JOIN " + + ROLE_TABLE_NAME + + " 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}" + + " GROUP BY ut.user_id"; + } +} diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java b/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java new file mode 100644 index 00000000000..4314970001f --- /dev/null +++ b/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.storage.relational.po; + +import com.google.common.base.Objects; + +/** + * This PO is only used for reading the data from multiple joined tables. So we don't need the inner + * class Builder. + */ +public class CombinedUserPO extends UserPO { + + private String roleNames; + private String roleIds; + + public String getRoleNames() { + return roleNames; + } + + public String getRoleIds() { + return roleIds; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof CombinedUserPO)) { + return false; + } + CombinedUserPO combinedUserPO = (CombinedUserPO) o; + return Objects.equal(getUserId(), combinedUserPO.getUserId()) + && Objects.equal(getUserName(), combinedUserPO.getUserName()) + && Objects.equal(getMetalakeId(), combinedUserPO.getMetalakeId()) + && Objects.equal(getAuditInfo(), combinedUserPO.getAuditInfo()) + && Objects.equal(getCurrentVersion(), combinedUserPO.getCurrentVersion()) + && Objects.equal(getLastVersion(), combinedUserPO.getLastVersion()) + && Objects.equal(getDeletedAt(), combinedUserPO.getDeletedAt()) + && Objects.equal(getRoleIds(), combinedUserPO.getRoleIds()) + && Objects.equal(getRoleNames(), combinedUserPO.getRoleNames()); + } + + @Override + public int hashCode() { + return Objects.hashCode( + getUserId(), + getUserName(), + getMetalakeId(), + getAuditInfo(), + getCurrentVersion(), + getLastVersion(), + getDeletedAt(), + getRoleIds(), + getRoleNames()); + } +} 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 index 11e0833fe03..5a263aa0568 100644 --- 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 @@ -40,6 +40,7 @@ import org.apache.gravitino.storage.relational.mapper.OwnerMetaMapper; import org.apache.gravitino.storage.relational.mapper.UserMetaMapper; import org.apache.gravitino.storage.relational.mapper.UserRoleRelMapper; +import org.apache.gravitino.storage.relational.po.CombinedUserPO; import org.apache.gravitino.storage.relational.po.RolePO; import org.apache.gravitino.storage.relational.po.UserPO; import org.apache.gravitino.storage.relational.po.UserRoleRelPO; @@ -247,22 +248,34 @@ public UserEntity updateUser( return newEntity; } - public List listUsersByNamespace(Namespace namespace) { + public List listUsersByNamespace(Namespace namespace, boolean includeAllFields) { AuthorizationUtils.checkUserNamespace(namespace); String metalakeName = namespace.level(0); - List userPOs = - SessionUtils.getWithoutCommit( - UserMetaMapper.class, mapper -> mapper.listUserPOsByMetalake(metalakeName)); - - return userPOs.stream() - .map( - po -> - POConverters.fromUserPO( - po, - SupplierUtils.createRolePOsSupplier(po), - AuthorizationUtils.ofUserNamespace(metalakeName))) - .collect(Collectors.toList()); + if (includeAllFields) { + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(namespace.level(0)); + List userPOs = + SessionUtils.getWithoutCommit( + UserMetaMapper.class, mapper -> mapper.listCombinedUserPOsByMetalakeId(metalakeId)); + return userPOs.stream() + .map( + po -> + POConverters.fromCombinedUserPO( + po, AuthorizationUtils.ofUserNamespace(metalakeName))) + .collect(Collectors.toList()); + } else { + List userPOs = + SessionUtils.getWithoutCommit( + UserMetaMapper.class, mapper -> mapper.listUserPOsByMetalake(metalakeName)); + return userPOs.stream() + .map( + po -> + POConverters.fromUserPO( + po, + Collections.emptyList(), + AuthorizationUtils.ofUserNamespace(metalakeName))) + .collect(Collectors.toList()); + } } public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java index 82d739a41cc..69220994db1 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java @@ -48,6 +48,7 @@ import org.apache.gravitino.meta.TopicEntity; import org.apache.gravitino.meta.UserEntity; import org.apache.gravitino.storage.relational.po.CatalogPO; +import org.apache.gravitino.storage.relational.po.CombinedUserPO; import org.apache.gravitino.storage.relational.po.FilesetPO; import org.apache.gravitino.storage.relational.po.FilesetVersionPO; import org.apache.gravitino.storage.relational.po.GroupPO; @@ -728,6 +729,44 @@ public static UserEntity fromUserPO(UserPO userPO, List rolePOs, Namespa } } + public static UserEntity fromCombinedUserPO(CombinedUserPO userPO, Namespace namespace) { + try { + UserEntity.Builder builder = + UserEntity.builder() + .withId(userPO.getUserId()) + .withName(userPO.getUserName()) + .withNamespace(namespace) + .withAuditInfo( + JsonUtils.anyFieldMapper().readValue(userPO.getAuditInfo(), AuditInfo.class)); + if (StringUtils.isNotBlank(userPO.getRoleNames())) { + List roleNamesFromJson = + JsonUtils.anyFieldMapper().readValue(userPO.getRoleNames(), List.class); + List roleNames = + roleNamesFromJson.stream().filter(StringUtils::isNotBlank).collect(Collectors.toList()); + if (!roleNames.isEmpty()) { + builder.withRoleNames(roleNames); + } + } + + if (StringUtils.isNotBlank(userPO.getRoleIds())) { + List roleIdsFromJson = + JsonUtils.anyFieldMapper().readValue(userPO.getRoleIds(), List.class); + List roleIds = + roleIdsFromJson.stream() + .filter(StringUtils::isNotBlank) + .map(Long::valueOf) + .collect(Collectors.toList()); + ; + if (!roleIds.isEmpty()) { + builder.withRoleIds(roleIds); + } + } + return builder.build(); + } catch (JsonProcessingException e) { + throw new RuntimeException("Failed to deserialize json object:", e); + } + } + /** * Convert {@link GroupPO} to {@link GroupEntity} * diff --git a/core/src/main/java/org/apache/gravitino/tag/TagManager.java b/core/src/main/java/org/apache/gravitino/tag/TagManager.java index 040ac9f1953..5b89e857d5e 100644 --- a/core/src/main/java/org/apache/gravitino/tag/TagManager.java +++ b/core/src/main/java/org/apache/gravitino/tag/TagManager.java @@ -98,7 +98,8 @@ public Tag[] listTagsInfo(String metalake) { try { return entityStore - .list(ofTagNamespace(metalake), TagEntity.class, Entity.EntityType.TAG).stream() + .list(ofTagNamespace(metalake), TagEntity.class, Entity.EntityType.TAG, true) + .stream() .toArray(Tag[]::new); } catch (IOException ioe) { LOG.error("Failed to list tags under metalake {}", metalake, ioe); diff --git a/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java b/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java index 75c3f30ba7c..ae3ddea8e77 100644 --- a/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java +++ b/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java @@ -134,7 +134,7 @@ void testCreateKvEntityStore() throws IOException { // Test scan and store list interface List catalogEntityList = - store.list(catalog.namespace(), CatalogEntity.class, Entity.EntityType.CATALOG); + store.list(catalog.namespace(), CatalogEntity.class, Entity.EntityType.CATALOG, true); Assertions.assertEquals(3, catalogEntityList.size()); Assertions.assertTrue(catalogEntityList.contains(catalog)); Assertions.assertTrue(catalogEntityList.contains(catalogCopy)); diff --git a/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java b/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java index 7cff7bcc553..fbf544193cc 100644 --- a/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java +++ b/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java @@ -82,7 +82,8 @@ public void setSerDe(EntitySerDe entitySerDe) {} @Override public List list( - Namespace namespace, Class cl, EntityType entityType) throws IOException { + Namespace namespace, Class cl, EntityType entityType, boolean includeAllFields) + throws IOException { return entityMap.entrySet().stream() .filter(e -> e.getKey().namespace().equals(namespace)) .map(entry -> (E) entry.getValue()) diff --git a/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java b/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java index 26430d2fb5e..c67c9697a12 100644 --- a/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java +++ b/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java @@ -713,24 +713,27 @@ public void testMetaLifeCycleFromCreationToDeletion() throws IOException { backend.insert(anotherTagEntity, false); // meta data list - List metaLakes = backend.list(metalake.namespace(), Entity.EntityType.METALAKE); + List metaLakes = + backend.list(metalake.namespace(), Entity.EntityType.METALAKE, true); assertTrue(metaLakes.contains(metalake)); - List catalogs = backend.list(catalog.namespace(), Entity.EntityType.CATALOG); + List catalogs = + backend.list(catalog.namespace(), Entity.EntityType.CATALOG, true); assertTrue(catalogs.contains(catalog)); - List schemas = backend.list(schema.namespace(), Entity.EntityType.SCHEMA); + List schemas = backend.list(schema.namespace(), Entity.EntityType.SCHEMA, true); assertTrue(schemas.contains(schema)); - List tables = backend.list(table.namespace(), Entity.EntityType.TABLE); + List tables = backend.list(table.namespace(), Entity.EntityType.TABLE, true); assertTrue(tables.contains(table)); - List filesets = backend.list(fileset.namespace(), Entity.EntityType.FILESET); + List filesets = + backend.list(fileset.namespace(), Entity.EntityType.FILESET, true); assertFalse(filesets.contains(fileset)); assertTrue(filesets.contains(filesetV2)); assertEquals("2", filesets.get(filesets.indexOf(filesetV2)).properties().get("version")); - List topics = backend.list(topic.namespace(), Entity.EntityType.TOPIC); + List topics = backend.list(topic.namespace(), Entity.EntityType.TOPIC, true); assertTrue(topics.contains(topic)); RoleEntity roleEntity = backend.get(role.nameIdentifier(), Entity.EntityType.ROLE); @@ -756,7 +759,7 @@ public void testMetaLifeCycleFromCreationToDeletion() throws IOException { TagEntity tagEntity = backend.get(tag.nameIdentifier(), Entity.EntityType.TAG); assertEquals(tag, tagEntity); - List tags = backend.list(tag.namespace(), Entity.EntityType.TAG); + List tags = backend.list(tag.namespace(), Entity.EntityType.TAG, true); assertTrue(tags.contains(tag)); assertEquals(1, tags.size()); 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 index a8e045eca84..c7b2cd9465c 100644 --- 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.Comparator; import java.util.List; import java.util.Optional; import java.util.function.Function; @@ -126,6 +127,11 @@ void testListUsers() throws IOException { createBaseMakeLake(RandomIdGenerator.INSTANCE.nextId(), metalakeName, auditInfo); backend.insert(metalake, false); + CatalogEntity catalog = + createCatalog( + RandomIdGenerator.INSTANCE.nextId(), Namespace.of(metalakeName), "catalog", auditInfo); + backend.insert(catalog, false); + UserEntity user1 = createUserEntity( RandomIdGenerator.INSTANCE.nextId(), @@ -133,20 +139,43 @@ void testListUsers() throws IOException { "user1", auditInfo); + RoleEntity role1 = + createRoleEntity( + RandomIdGenerator.INSTANCE.nextId(), + AuthorizationUtils.ofRoleNamespace("metalake"), + "role1", + auditInfo, + "catalog"); + backend.insert(role1, false); + + RoleEntity role2 = + createRoleEntity( + RandomIdGenerator.INSTANCE.nextId(), + AuthorizationUtils.ofRoleNamespace("metalake"), + "role2", + auditInfo, + "catalog"); + backend.insert(role2, false); + UserEntity user2 = createUserEntity( RandomIdGenerator.INSTANCE.nextId(), - AuthorizationUtils.ofUserNamespace(metalakeName), + AuthorizationUtils.ofUserNamespace("metalake"), "user2", - auditInfo); + auditInfo, + Lists.newArrayList(role1.name(), role2.name()), + Lists.newArrayList(role1.id(), role2.id())); backend.insert(user1, false); backend.insert(user2, false); UserMetaService userMetaService = UserMetaService.getInstance(); - Assertions.assertEquals( - Lists.newArrayList(user1, user2), - userMetaService.listUsersByNamespace(AuthorizationUtils.ofUserNamespace(metalakeName))); + List actualUsers = + userMetaService.listUsersByNamespace( + AuthorizationUtils.ofUserNamespace(metalakeName), true); + actualUsers.sort(Comparator.comparing(UserEntity::name)); + + Assertions.assertEquals(Lists.newArrayList(user1, user2), actualUsers); } @Test From 81cd6cb93bb5a90259dc8ca9d9d6c9fff2ab0409 Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 18 Sep 2024 19:03:13 +0800 Subject: [PATCH 04/31] fix test --- .../relational/service/TestUserMetaService.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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 index c7b2cd9465c..7de5a1113bc 100644 --- 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 @@ -174,8 +174,16 @@ void testListUsers() throws IOException { userMetaService.listUsersByNamespace( AuthorizationUtils.ofUserNamespace(metalakeName), true); actualUsers.sort(Comparator.comparing(UserEntity::name)); - - Assertions.assertEquals(Lists.newArrayList(user1, user2), actualUsers); + List expectUsers = Lists.newArrayList(user1, user2); + Assertions.assertEquals(expectUsers.size(), actualUsers.size()); + for (int index = 0; index < expectUsers.size(); index++) { + Assertions.assertEquals(expectUsers.get(index).name(), actualUsers.get(index).name()); + Assertions.assertEquals( + expectUsers.get(index).roleNames().size(), actualUsers.get(index).roleNames().size()); + for (String roleName : expectUsers.get(index).roleNames()) { + Assertions.assertTrue(actualUsers.get(index).roleNames().contains(roleName)); + } + } } @Test From 15144d25c74ee5556bd0c17aa1ea8ab09c2889b0 Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 18 Sep 2024 19:17:44 +0800 Subject: [PATCH 05/31] fix npe --- .../relational/service/TestUserMetaService.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) 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 index 7de5a1113bc..6f538e715dd 100644 --- 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 @@ -178,10 +178,14 @@ void testListUsers() throws IOException { Assertions.assertEquals(expectUsers.size(), actualUsers.size()); for (int index = 0; index < expectUsers.size(); index++) { Assertions.assertEquals(expectUsers.get(index).name(), actualUsers.get(index).name()); - Assertions.assertEquals( - expectUsers.get(index).roleNames().size(), actualUsers.get(index).roleNames().size()); - for (String roleName : expectUsers.get(index).roleNames()) { - Assertions.assertTrue(actualUsers.get(index).roleNames().contains(roleName)); + if (expectUsers.get(index).roleNames() == null) { + Assertions.assertNull(actualUsers.get(index).roleNames()); + } else { + Assertions.assertEquals( + expectUsers.get(index).roleNames().size(), actualUsers.get(index).roleNames().size()); + for (String roleName : expectUsers.get(index).roleNames()) { + Assertions.assertTrue(actualUsers.get(index).roleNames().contains(roleName)); + } } } } From 0e6b04087d232adac901033993ab41dd1567bd27 Mon Sep 17 00:00:00 2001 From: Rory Date: Wed, 18 Sep 2024 19:18:39 +0800 Subject: [PATCH 06/31] fix style --- .../storage/relational/service/TestUserMetaService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 6f538e715dd..0efd886ee4d 100644 --- 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 @@ -182,7 +182,7 @@ void testListUsers() throws IOException { Assertions.assertNull(actualUsers.get(index).roleNames()); } else { Assertions.assertEquals( - expectUsers.get(index).roleNames().size(), actualUsers.get(index).roleNames().size()); + expectUsers.get(index).roleNames().size(), actualUsers.get(index).roleNames().size()); for (String roleName : expectUsers.get(index).roleNames()) { Assertions.assertTrue(actualUsers.get(index).roleNames().contains(roleName)); } From 0a0d6b3809baa8fd8ed9ff11301ce6b7eec2cdb5 Mon Sep 17 00:00:00 2001 From: Rory Date: Thu, 19 Sep 2024 09:53:16 +0800 Subject: [PATCH 07/31] Modify PG SQL --- .../mapper/UserMetaBaseSQLProvider.java | 8 +++--- .../UserMetaPostgreSQLProvider.java | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java index ca440b31581..4f5383dc028 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java @@ -164,14 +164,16 @@ public String listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalake + " JSON_ARRAYAGG(rot.role_id) as roleIds" + " FROM " + USER_TABLE_NAME - + " ut JOIN " + + " ut LEFT OUTER JOIN " + USER_ROLE_RELATION_TABLE_NAME + " rt ON rt.user_id = ut.user_id" - + " JOIN " + + " LEFT OUTER JOIN " + ROLE_TABLE_NAME + " rot ON rot.role_id = rt.role_id" + " WHERE " - + " ut.deleted_at = 0 AND rot.deleted_at = 0 AND rt.deleted_at = 0 AND ut.metalake_id = #{metalakeId}" + + " 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}" + " GROUP BY ut.user_id"; } diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/postgresql/UserMetaPostgreSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/postgresql/UserMetaPostgreSQLProvider.java index af7d65d2ab5..77689bafdca 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/postgresql/UserMetaPostgreSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/postgresql/UserMetaPostgreSQLProvider.java @@ -18,6 +18,8 @@ */ package org.apache.gravitino.storage.relational.mapper.postgresql; +import static org.apache.gravitino.storage.relational.mapper.RoleMetaMapper.ROLE_TABLE_NAME; +import static org.apache.gravitino.storage.relational.mapper.UserMetaMapper.USER_ROLE_RELATION_TABLE_NAME; import static org.apache.gravitino.storage.relational.mapper.UserRoleRelMapper.USER_TABLE_NAME; import org.apache.gravitino.storage.relational.mapper.UserMetaBaseSQLProvider; @@ -66,4 +68,28 @@ public String insertUserMetaOnDuplicateKeyUpdate(UserPO userPO) { + " last_version = #{userMeta.lastVersion}," + " deleted_at = #{userMeta.deletedAt}"; } + + @Override + public String listCombinedUserPOsByMetalakeId(Long metalakeId) { + return "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," + + " JSON_AGG(rot.role_name) as roleNames," + + " JSON_AGG(rot.role_id) as roleIds" + + " FROM " + + USER_TABLE_NAME + + " ut LEFT OUTER JOIN " + + USER_ROLE_RELATION_TABLE_NAME + + " rt ON rt.user_id = ut.user_id" + + " LEFT OUTER JOIN " + + ROLE_TABLE_NAME + + " 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}" + + " GROUP BY ut.user_id"; + } } From 9d0821d7edf7a1ade1ded26ad0706d39bf0ee4ee Mon Sep 17 00:00:00 2001 From: Rory Date: Thu, 19 Sep 2024 10:47:59 +0800 Subject: [PATCH 08/31] fix comment --- core/src/main/java/org/apache/gravitino/EntityStore.java | 7 ++++--- .../gravitino/storage/relational/RelationalBackend.java | 7 ++++--- .../gravitino/storage/relational/po/CombinedUserPO.java | 3 ++- .../gravitino/storage/relational/utils/POConverters.java | 7 +++++++ 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/EntityStore.java b/core/src/main/java/org/apache/gravitino/EntityStore.java index b6d04628da1..7b49a20ab4c 100644 --- a/core/src/main/java/org/apache/gravitino/EntityStore.java +++ b/core/src/main/java/org/apache/gravitino/EntityStore.java @@ -59,9 +59,10 @@ public interface EntityStore extends Closeable { * @param namespace the namespace of the entities * @param type the detailed type of the entity * @param entityType the general type of the entity - * @param includeAllFields Some fields will have heavier cost, EntityStore provide an option to - * avoid fetching them to improve the performance. If true, the store fetch all the fields, - * otherwise false. + * @param includeAllFields Some fields may have a relatively high acquisition cost, EntityStore + * provide an optional setting to avoid fetching these high-cost fields to improve the + * performance. If true, the store fetches all the fields, Otherwise, it will avoid fetch + * specific high-cost fields. * @return the list of entities * @throws IOException if the list operation fails */ diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java index d5ba06a24d8..8ffe0ba3c5a 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java @@ -48,9 +48,10 @@ public interface RelationalBackend * * @param namespace The parent namespace of these entities. * @param entityType The type of these entities. - * @param includeAllFields Some fields will have heavier cost, EntityStore provide an option to - * avoid fetching them to improve the performance. If true, the store fetch all the fields, - * otherwise false. + * @param includeAllFields Some fields may have a relatively high acquisition cost, EntityStore + * provide an optional setting to avoid fetching these high-cost fields to improve the + * performance. If true, the store fetches all the fields, Otherwise, it will avoid fetch + * specific high-cost fields. * @return The list of entities associated with the given parent namespace and entityType, or null * if the entities does not exist. * @throws NoSuchEntityException If the corresponding parent entity of these list entities cannot diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java b/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java index 4314970001f..e6dec7cf920 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java @@ -21,7 +21,8 @@ import com.google.common.base.Objects; /** - * This PO is only used for reading the data from multiple joined tables. So we don't need the inner + * CombinedUserPO add extra roleNames and roleIds for UserPO. This PO is only used for reading the + * data from multiple joined tables. The PO won't be written to database. So we don't need the inner * class Builder. */ public class CombinedUserPO extends UserPO { diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java index 69220994db1..b4204dc2c12 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java @@ -729,6 +729,13 @@ public static UserEntity fromUserPO(UserPO userPO, List rolePOs, Namespa } } + /** + * Convert {@link CombinedUserPO} to {@link UserEntity} + * + * @param userPO CombinedUserPo object to be converted + * @param namespace Namespace object to be associated with the user + * @return UserEntity object from CombinedUserPO object + */ public static UserEntity fromCombinedUserPO(CombinedUserPO userPO, Namespace namespace) { try { UserEntity.Builder builder = From feaa46f61429ccfffc50e348c419367dbc13132c Mon Sep 17 00:00:00 2001 From: Rory Date: Thu, 19 Sep 2024 21:21:20 +0800 Subject: [PATCH 09/31] refactor the interface --- .../hadoop/HadoopCatalogOperations.java | 6 ++-- .../catalog/kafka/KafkaCatalogOperations.java | 3 +- .../org/apache/gravitino/EntityStore.java | 7 ++--- .../authorization/UserGroupManager.java | 14 ++++++--- .../gravitino/catalog/CatalogManager.java | 7 +++-- .../gravitino/metalake/MetalakeManager.java | 5 +++- .../gravitino/storage/kv/KvEntityStore.java | 3 +- .../storage/relational/JDBCBackend.java | 5 ++-- .../storage/relational/RelationalBackend.java | 8 ++--- .../relational/RelationalEntityStore.java | 8 +++-- .../relational/service/UserMetaService.java | 30 +++++++++++-------- .../org/apache/gravitino/tag/TagManager.java | 6 +++- .../storage/kv/TestKvEntityStorage.java | 7 ++++- .../storage/memory/TestMemoryEntityStore.java | 3 +- .../storage/relational/TestJDBCBackend.java | 19 +++++++----- .../service/TestUserMetaService.java | 3 +- 16 files changed, 86 insertions(+), 48 deletions(-) diff --git a/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java b/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java index 2b68312ea18..7b90ad1b70f 100644 --- a/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java +++ b/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java @@ -149,7 +149,8 @@ public NameIdentifier[] listFilesets(Namespace namespace) throws NoSuchSchemaExc } List filesets = - store.list(namespace, FilesetEntity.class, Entity.EntityType.FILESET, true); + store.list( + namespace, FilesetEntity.class, Entity.EntityType.FILESET, Collections.emptyList()); return filesets.stream() .map(f -> NameIdentifier.of(namespace, f.name())) .toArray(NameIdentifier[]::new); @@ -387,7 +388,8 @@ public String getFileLocation(NameIdentifier ident, String subPath) public NameIdentifier[] listSchemas(Namespace namespace) throws NoSuchCatalogException { try { List schemas = - store.list(namespace, SchemaEntity.class, Entity.EntityType.SCHEMA, true); + store.list( + namespace, SchemaEntity.class, Entity.EntityType.SCHEMA, Collections.emptyList()); return schemas.stream() .map(s -> NameIdentifier.of(namespace, s.name())) .toArray(NameIdentifier[]::new); diff --git a/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java b/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java index b360626bea5..cc13a3d4ded 100644 --- a/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java +++ b/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java @@ -391,7 +391,8 @@ public boolean dropTopic(NameIdentifier ident) { public NameIdentifier[] listSchemas(Namespace namespace) throws NoSuchCatalogException { try { List schemas = - store.list(namespace, SchemaEntity.class, Entity.EntityType.SCHEMA, true); + store.list( + namespace, SchemaEntity.class, Entity.EntityType.SCHEMA, Collections.emptyList()); return schemas.stream() .map(s -> NameIdentifier.of(namespace, s.name())) .toArray(NameIdentifier[]::new); diff --git a/core/src/main/java/org/apache/gravitino/EntityStore.java b/core/src/main/java/org/apache/gravitino/EntityStore.java index 7b49a20ab4c..2370eebd62b 100644 --- a/core/src/main/java/org/apache/gravitino/EntityStore.java +++ b/core/src/main/java/org/apache/gravitino/EntityStore.java @@ -59,15 +59,14 @@ public interface EntityStore extends Closeable { * @param namespace the namespace of the entities * @param type the detailed type of the entity * @param entityType the general type of the entity - * @param includeAllFields Some fields may have a relatively high acquisition cost, EntityStore + * @param allowMissingFields Some fields may have a relatively high acquisition cost, EntityStore * provide an optional setting to avoid fetching these high-cost fields to improve the - * performance. If true, the store fetches all the fields, Otherwise, it will avoid fetch - * specific high-cost fields. + * performance. * @return the list of entities * @throws IOException if the list operation fails */ List list( - Namespace namespace, Class type, EntityType entityType, boolean includeAllFields) + Namespace namespace, Class type, EntityType entityType, List allowMissingFields) throws IOException; /** diff --git a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java index 79e4ee24455..707fcffb6e5 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java @@ -23,9 +23,11 @@ import java.time.Instant; import java.util.Arrays; import java.util.Collections; +import java.util.List; import org.apache.gravitino.Entity; import org.apache.gravitino.EntityAlreadyExistsException; import org.apache.gravitino.EntityStore; +import org.apache.gravitino.Field; import org.apache.gravitino.Namespace; import org.apache.gravitino.exceptions.GroupAlreadyExistsException; import org.apache.gravitino.exceptions.NoSuchEntityException; @@ -114,19 +116,23 @@ User getUser(String metalake, String user) throws NoSuchUserException { } String[] listUserNames(String metalake) { - return Arrays.stream(listUsersInternal(metalake, false)).map(User::name).toArray(String[]::new); + return Arrays.stream( + listUsersInternal( + metalake, Lists.newArrayList(UserEntity.ROLE_NAMES, UserEntity.ROLE_IDS))) + .map(User::name) + .toArray(String[]::new); } User[] listUsers(String metalake) { - return listUsersInternal(metalake, true); + return listUsersInternal(metalake, Collections.emptyList()); } - private User[] listUsersInternal(String metalake, boolean includeAllFields) { + private User[] listUsersInternal(String metalake, List allowMissingFields) { try { AuthorizationUtils.checkMetalakeExists(metalake); Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake); - return store.list(namespace, UserEntity.class, Entity.EntityType.USER, includeAllFields) + return store.list(namespace, UserEntity.class, Entity.EntityType.USER, allowMissingFields) .stream() .map(entity -> (User) entity) .toArray(User[]::new); diff --git a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java index 980fe1d6b01..9f1cf36ae2a 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java +++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java @@ -276,7 +276,8 @@ public NameIdentifier[] listCatalogs(Namespace namespace) throws NoSuchMetalakeE checkMetalakeExists(metalakeIdent); try { - return store.list(namespace, CatalogEntity.class, EntityType.CATALOG, true).stream() + return store.list(namespace, CatalogEntity.class, EntityType.CATALOG, Collections.emptyList()) + .stream() .map(entity -> NameIdentifier.of(namespace, entity.name())) .toArray(NameIdentifier[]::new); @@ -293,7 +294,7 @@ public Catalog[] listCatalogsInfo(Namespace namespace) throws NoSuchMetalakeExce try { List catalogEntities = - store.list(namespace, CatalogEntity.class, EntityType.CATALOG, true); + store.list(namespace, CatalogEntity.class, EntityType.CATALOG, Collections.emptyList()); // Using provider as key to avoid loading the same type catalog instance multiple times Map> hiddenProps = new HashMap<>(); @@ -574,7 +575,7 @@ public boolean dropCatalog(NameIdentifier ident) { Namespace.of(ident.namespace().level(0), ident.name()), SchemaEntity.class, EntityType.SCHEMA, - true); + Collections.emptyList()); // If there is only one schema, it must be the default schema, because we don't allow to // drop the default schema. if (schemas.size() == 1) { diff --git a/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java b/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java index 38db573aade..b620ac65aa6 100644 --- a/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java +++ b/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java @@ -21,6 +21,7 @@ import com.google.common.collect.Maps; import java.io.IOException; import java.time.Instant; +import java.util.Collections; import java.util.Map; import org.apache.gravitino.Entity.EntityType; import org.apache.gravitino.EntityAlreadyExistsException; @@ -72,7 +73,9 @@ public MetalakeManager(EntityStore store, IdGenerator idGenerator) { @Override public BaseMetalake[] listMetalakes() { try { - return store.list(Namespace.empty(), BaseMetalake.class, EntityType.METALAKE, true).stream() + return store + .list(Namespace.empty(), BaseMetalake.class, EntityType.METALAKE, Collections.emptyList()) + .stream() .toArray(BaseMetalake[]::new); } catch (IOException ioe) { LOG.error("Listing Metalakes failed due to storage issues.", ioe); diff --git a/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java b/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java index 1f0cd902733..1943ff7f04d 100644 --- a/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java +++ b/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java @@ -47,6 +47,7 @@ import org.apache.gravitino.EntitySerDe; import org.apache.gravitino.EntitySerDeFactory; import org.apache.gravitino.EntityStore; +import org.apache.gravitino.Field; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; @@ -126,7 +127,7 @@ public void setSerDe(EntitySerDe entitySerDe) { @Override public List list( - Namespace namespace, Class e, EntityType type, boolean includeAllFields) + Namespace namespace, Class e, EntityType type, List allowMissingFields) throws IOException { // Star means it's a wildcard List entities = Lists.newArrayList(); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java index 84dc55441f1..e21a61572e6 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java @@ -31,6 +31,7 @@ import org.apache.gravitino.Configs; import org.apache.gravitino.Entity; import org.apache.gravitino.EntityAlreadyExistsException; +import org.apache.gravitino.Field; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.NameIdentifier; @@ -88,7 +89,7 @@ public void initialize(Config config) { @Override public List list( - Namespace namespace, Entity.EntityType entityType, boolean includeAllFields) + Namespace namespace, Entity.EntityType entityType, List allowMissingFields) throws IOException { switch (entityType) { case METALAKE: @@ -107,7 +108,7 @@ public List list( return (List) TagMetaService.getInstance().listTagsByNamespace(namespace); case USER: return (List) - UserMetaService.getInstance().listUsersByNamespace(namespace, includeAllFields); + UserMetaService.getInstance().listUsersByNamespace(namespace, allowMissingFields); default: throw new UnsupportedEntityTypeException( "Unsupported entity type: %s for list operation", entityType); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java index 8ffe0ba3c5a..3a0044b405c 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java @@ -25,6 +25,7 @@ import org.apache.gravitino.Config; import org.apache.gravitino.Entity; import org.apache.gravitino.EntityAlreadyExistsException; +import org.apache.gravitino.Field; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; @@ -48,10 +49,9 @@ public interface RelationalBackend * * @param namespace The parent namespace of these entities. * @param entityType The type of these entities. - * @param includeAllFields Some fields may have a relatively high acquisition cost, EntityStore + * @param allowMissingFields Some fields may have a relatively high acquisition cost, EntityStore * provide an optional setting to avoid fetching these high-cost fields to improve the - * performance. If true, the store fetches all the fields, Otherwise, it will avoid fetch - * specific high-cost fields. + * performance. * @return The list of entities associated with the given parent namespace and entityType, or null * if the entities does not exist. * @throws NoSuchEntityException If the corresponding parent entity of these list entities cannot @@ -59,7 +59,7 @@ public interface RelationalBackend * @throws IOException If the store operation fails */ List list( - Namespace namespace, Entity.EntityType entityType, boolean includeAllFields) + Namespace namespace, Entity.EntityType entityType, List allowMissingFields) throws NoSuchEntityException, IOException; /** diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java index 66f870d2435..c3fe4f7da3c 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java @@ -30,6 +30,7 @@ import org.apache.gravitino.EntityAlreadyExistsException; import org.apache.gravitino.EntitySerDe; import org.apache.gravitino.EntityStore; +import org.apache.gravitino.Field; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.MetadataObject; import org.apache.gravitino.NameIdentifier; @@ -88,9 +89,12 @@ public void setSerDe(EntitySerDe entitySerDe) { @Override public List list( - Namespace namespace, Class type, Entity.EntityType entityType, boolean includeAllFields) + Namespace namespace, + Class type, + Entity.EntityType entityType, + List allowMissingFields) throws IOException { - return backend.list(namespace, entityType, true); + return backend.list(namespace, entityType, allowMissingFields); } @Override 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 index 5a263aa0568..a3d97a36125 100644 --- 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 @@ -30,6 +30,7 @@ import java.util.function.Function; import java.util.stream.Collectors; import org.apache.gravitino.Entity; +import org.apache.gravitino.Field; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; @@ -248,22 +249,13 @@ public UserEntity updateUser( return newEntity; } - public List listUsersByNamespace(Namespace namespace, boolean includeAllFields) { + public List listUsersByNamespace( + Namespace namespace, List allowMissingFields) { AuthorizationUtils.checkUserNamespace(namespace); String metalakeName = namespace.level(0); - if (includeAllFields) { - Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(namespace.level(0)); - List userPOs = - SessionUtils.getWithoutCommit( - UserMetaMapper.class, mapper -> mapper.listCombinedUserPOsByMetalakeId(metalakeId)); - return userPOs.stream() - .map( - po -> - POConverters.fromCombinedUserPO( - po, AuthorizationUtils.ofUserNamespace(metalakeName))) - .collect(Collectors.toList()); - } else { + if (allowMissingFields.contains(UserEntity.ROLE_IDS) + && allowMissingFields.contains(UserEntity.ROLE_NAMES)) { List userPOs = SessionUtils.getWithoutCommit( UserMetaMapper.class, mapper -> mapper.listUserPOsByMetalake(metalakeName)); @@ -275,6 +267,18 @@ public List listUsersByNamespace(Namespace namespace, boolean includ Collections.emptyList(), AuthorizationUtils.ofUserNamespace(metalakeName))) .collect(Collectors.toList()); + + } else { + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(namespace.level(0)); + List userPOs = + SessionUtils.getWithoutCommit( + UserMetaMapper.class, mapper -> mapper.listCombinedUserPOsByMetalakeId(metalakeId)); + return userPOs.stream() + .map( + po -> + POConverters.fromCombinedUserPO( + po, AuthorizationUtils.ofUserNamespace(metalakeName))) + .collect(Collectors.toList()); } } diff --git a/core/src/main/java/org/apache/gravitino/tag/TagManager.java b/core/src/main/java/org/apache/gravitino/tag/TagManager.java index 5b89e857d5e..39c09b0f0a5 100644 --- a/core/src/main/java/org/apache/gravitino/tag/TagManager.java +++ b/core/src/main/java/org/apache/gravitino/tag/TagManager.java @@ -98,7 +98,11 @@ public Tag[] listTagsInfo(String metalake) { try { return entityStore - .list(ofTagNamespace(metalake), TagEntity.class, Entity.EntityType.TAG, true) + .list( + ofTagNamespace(metalake), + TagEntity.class, + Entity.EntityType.TAG, + Collections.emptyList()) .stream() .toArray(Tag[]::new); } catch (IOException ioe) { diff --git a/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java b/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java index ae3ddea8e77..94218e89624 100644 --- a/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java +++ b/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java @@ -31,6 +31,7 @@ import java.io.IOException; import java.nio.file.Files; import java.time.Instant; +import java.util.Collections; import java.util.List; import java.util.concurrent.CompletionService; import java.util.concurrent.ExecutionException; @@ -134,7 +135,11 @@ void testCreateKvEntityStore() throws IOException { // Test scan and store list interface List catalogEntityList = - store.list(catalog.namespace(), CatalogEntity.class, Entity.EntityType.CATALOG, true); + store.list( + catalog.namespace(), + CatalogEntity.class, + Entity.EntityType.CATALOG, + Collections.emptyList()); Assertions.assertEquals(3, catalogEntityList.size()); Assertions.assertTrue(catalogEntityList.contains(catalog)); Assertions.assertTrue(catalogEntityList.contains(catalogCopy)); diff --git a/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java b/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java index fbf544193cc..46ef26bd12a 100644 --- a/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java +++ b/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java @@ -34,6 +34,7 @@ import org.apache.gravitino.EntityAlreadyExistsException; import org.apache.gravitino.EntitySerDe; import org.apache.gravitino.EntityStore; +import org.apache.gravitino.Field; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.Metalake; import org.apache.gravitino.NameIdentifier; @@ -82,7 +83,7 @@ public void setSerDe(EntitySerDe entitySerDe) {} @Override public List list( - Namespace namespace, Class cl, EntityType entityType, boolean includeAllFields) + Namespace namespace, Class cl, EntityType entityType, List allowMissingFields) throws IOException { return entityMap.entrySet().stream() .filter(e -> e.getKey().namespace().equals(namespace)) diff --git a/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java b/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java index c67c9697a12..454fba97ae6 100644 --- a/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java +++ b/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java @@ -45,6 +45,7 @@ import java.sql.Statement; import java.time.Instant; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -714,26 +715,29 @@ public void testMetaLifeCycleFromCreationToDeletion() throws IOException { // meta data list List metaLakes = - backend.list(metalake.namespace(), Entity.EntityType.METALAKE, true); + backend.list(metalake.namespace(), Entity.EntityType.METALAKE, Collections.emptyList()); assertTrue(metaLakes.contains(metalake)); List catalogs = - backend.list(catalog.namespace(), Entity.EntityType.CATALOG, true); + backend.list(catalog.namespace(), Entity.EntityType.CATALOG, Collections.emptyList()); assertTrue(catalogs.contains(catalog)); - List schemas = backend.list(schema.namespace(), Entity.EntityType.SCHEMA, true); + List schemas = + backend.list(schema.namespace(), Entity.EntityType.SCHEMA, Collections.emptyList()); assertTrue(schemas.contains(schema)); - List tables = backend.list(table.namespace(), Entity.EntityType.TABLE, true); + List tables = + backend.list(table.namespace(), Entity.EntityType.TABLE, Collections.emptyList()); assertTrue(tables.contains(table)); List filesets = - backend.list(fileset.namespace(), Entity.EntityType.FILESET, true); + backend.list(fileset.namespace(), Entity.EntityType.FILESET, Collections.emptyList()); assertFalse(filesets.contains(fileset)); assertTrue(filesets.contains(filesetV2)); assertEquals("2", filesets.get(filesets.indexOf(filesetV2)).properties().get("version")); - List topics = backend.list(topic.namespace(), Entity.EntityType.TOPIC, true); + List topics = + backend.list(topic.namespace(), Entity.EntityType.TOPIC, Collections.emptyList()); assertTrue(topics.contains(topic)); RoleEntity roleEntity = backend.get(role.nameIdentifier(), Entity.EntityType.ROLE); @@ -759,7 +763,8 @@ public void testMetaLifeCycleFromCreationToDeletion() throws IOException { TagEntity tagEntity = backend.get(tag.nameIdentifier(), Entity.EntityType.TAG); assertEquals(tag, tagEntity); - List tags = backend.list(tag.namespace(), Entity.EntityType.TAG, true); + List tags = + backend.list(tag.namespace(), Entity.EntityType.TAG, Collections.emptyList()); assertTrue(tags.contains(tag)); assertEquals(1, tags.size()); 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 index 0efd886ee4d..3b7d1a90084 100644 --- 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; @@ -172,7 +173,7 @@ void testListUsers() throws IOException { UserMetaService userMetaService = UserMetaService.getInstance(); List actualUsers = userMetaService.listUsersByNamespace( - AuthorizationUtils.ofUserNamespace(metalakeName), true); + AuthorizationUtils.ofUserNamespace(metalakeName), Collections.emptyList()); actualUsers.sort(Comparator.comparing(UserEntity::name)); List expectUsers = Lists.newArrayList(user1, user2); Assertions.assertEquals(expectUsers.size(), actualUsers.size()); From 91ac43a0cdff071fef7a790ae9732593c67bb3cf Mon Sep 17 00:00:00 2001 From: Rory Date: Fri, 20 Sep 2024 18:30:00 +0800 Subject: [PATCH 10/31] rename interface --- .../hadoop/HadoopCatalogOperations.java | 6 +- .../catalog/kafka/KafkaCatalogOperations.java | 3 +- .../org/apache/gravitino/EntityStore.java | 27 ++- .../authorization/UserGroupManager.java | 4 +- .../gravitino/catalog/CatalogManager.java | 8 +- .../gravitino/metalake/MetalakeManager.java | 5 +- .../gravitino/storage/kv/KvEntityStore.java | 4 +- .../storage/relational/JDBCBackend.java | 4 +- .../storage/relational/RelationalBackend.java | 4 +- .../relational/RelationalEntityStore.java | 14 +- .../relational/service/UserMetaService.java | 7 +- .../org/apache/gravitino/tag/TagManager.java | 7 +- .../TestAccessControlManager.java | 162 +++++++++++++----- .../storage/kv/TestKvEntityStorage.java | 7 +- .../storage/memory/TestMemoryEntityStore.java | 4 +- 15 files changed, 167 insertions(+), 99 deletions(-) diff --git a/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java b/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java index 7b90ad1b70f..1946464911f 100644 --- a/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java +++ b/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java @@ -149,8 +149,7 @@ public NameIdentifier[] listFilesets(Namespace namespace) throws NoSuchSchemaExc } List filesets = - store.list( - namespace, FilesetEntity.class, Entity.EntityType.FILESET, Collections.emptyList()); + store.list(namespace, FilesetEntity.class, Entity.EntityType.FILESET); return filesets.stream() .map(f -> NameIdentifier.of(namespace, f.name())) .toArray(NameIdentifier[]::new); @@ -388,8 +387,7 @@ public String getFileLocation(NameIdentifier ident, String subPath) public NameIdentifier[] listSchemas(Namespace namespace) throws NoSuchCatalogException { try { List schemas = - store.list( - namespace, SchemaEntity.class, Entity.EntityType.SCHEMA, Collections.emptyList()); + store.list(namespace, SchemaEntity.class, Entity.EntityType.SCHEMA); return schemas.stream() .map(s -> NameIdentifier.of(namespace, s.name())) .toArray(NameIdentifier[]::new); diff --git a/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java b/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java index cc13a3d4ded..66f49a02c24 100644 --- a/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java +++ b/catalogs/catalog-kafka/src/main/java/org/apache/gravitino/catalog/kafka/KafkaCatalogOperations.java @@ -391,8 +391,7 @@ public boolean dropTopic(NameIdentifier ident) { public NameIdentifier[] listSchemas(Namespace namespace) throws NoSuchCatalogException { try { List schemas = - store.list( - namespace, SchemaEntity.class, Entity.EntityType.SCHEMA, Collections.emptyList()); + store.list(namespace, SchemaEntity.class, Entity.EntityType.SCHEMA); return schemas.stream() .map(s -> NameIdentifier.of(namespace, s.name())) .toArray(NameIdentifier[]::new); diff --git a/core/src/main/java/org/apache/gravitino/EntityStore.java b/core/src/main/java/org/apache/gravitino/EntityStore.java index 2370eebd62b..feb043eefe9 100644 --- a/core/src/main/java/org/apache/gravitino/EntityStore.java +++ b/core/src/main/java/org/apache/gravitino/EntityStore.java @@ -59,15 +59,34 @@ public interface EntityStore extends Closeable { * @param namespace the namespace of the entities * @param type the detailed type of the entity * @param entityType the general type of the entity - * @param allowMissingFields Some fields may have a relatively high acquisition cost, EntityStore + * @return the list of entities + * @throws IOException if the list operation fails + */ + List list( + Namespace namespace, Class type, EntityType entityType) throws IOException; + + /** + * List all the entities with the specified {@link org.apache.gravitino.Namespace}, and + * deserialize them into the specified {@link Entity} object. + * + *

Note. Depends on the isolation levels provided by the underlying storage, the returned list + * may not be consistent. + * + * @param class of the entity + * @param namespace the namespace of the entities + * @param type the detailed type of the entity + * @param entityType the general type of the entity + * @param skippingFields Some fields may have a relatively high acquisition cost, EntityStore * provide an optional setting to avoid fetching these high-cost fields to improve the * performance. * @return the list of entities * @throws IOException if the list operation fails */ - List list( - Namespace namespace, Class type, EntityType entityType, List allowMissingFields) - throws IOException; + default List list( + Namespace namespace, Class type, EntityType entityType, List skippingFields) + throws IOException { + throw new UnsupportedOperationException("Don't support to skip fields"); + } /** * Check if the entity with the specified {@link org.apache.gravitino.NameIdentifier} exists. diff --git a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java index 707fcffb6e5..8b9ed13b6fd 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java @@ -127,12 +127,12 @@ User[] listUsers(String metalake) { return listUsersInternal(metalake, Collections.emptyList()); } - private User[] listUsersInternal(String metalake, List allowMissingFields) { + private User[] listUsersInternal(String metalake, List skippingFields) { try { AuthorizationUtils.checkMetalakeExists(metalake); Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake); - return store.list(namespace, UserEntity.class, Entity.EntityType.USER, allowMissingFields) + return store.list(namespace, UserEntity.class, Entity.EntityType.USER, skippingFields) .stream() .map(entity -> (User) entity) .toArray(User[]::new); diff --git a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java index 9f1cf36ae2a..f351a3271fe 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java +++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java @@ -276,8 +276,7 @@ public NameIdentifier[] listCatalogs(Namespace namespace) throws NoSuchMetalakeE checkMetalakeExists(metalakeIdent); try { - return store.list(namespace, CatalogEntity.class, EntityType.CATALOG, Collections.emptyList()) - .stream() + return store.list(namespace, CatalogEntity.class, EntityType.CATALOG).stream() .map(entity -> NameIdentifier.of(namespace, entity.name())) .toArray(NameIdentifier[]::new); @@ -294,7 +293,7 @@ public Catalog[] listCatalogsInfo(Namespace namespace) throws NoSuchMetalakeExce try { List catalogEntities = - store.list(namespace, CatalogEntity.class, EntityType.CATALOG, Collections.emptyList()); + store.list(namespace, CatalogEntity.class, EntityType.CATALOG); // Using provider as key to avoid loading the same type catalog instance multiple times Map> hiddenProps = new HashMap<>(); @@ -574,8 +573,7 @@ public boolean dropCatalog(NameIdentifier ident) { store.list( Namespace.of(ident.namespace().level(0), ident.name()), SchemaEntity.class, - EntityType.SCHEMA, - Collections.emptyList()); + EntityType.SCHEMA); // If there is only one schema, it must be the default schema, because we don't allow to // drop the default schema. if (schemas.size() == 1) { diff --git a/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java b/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java index b620ac65aa6..54298c0efdd 100644 --- a/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java +++ b/core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java @@ -21,7 +21,6 @@ import com.google.common.collect.Maps; import java.io.IOException; import java.time.Instant; -import java.util.Collections; import java.util.Map; import org.apache.gravitino.Entity.EntityType; import org.apache.gravitino.EntityAlreadyExistsException; @@ -73,9 +72,7 @@ public MetalakeManager(EntityStore store, IdGenerator idGenerator) { @Override public BaseMetalake[] listMetalakes() { try { - return store - .list(Namespace.empty(), BaseMetalake.class, EntityType.METALAKE, Collections.emptyList()) - .stream() + return store.list(Namespace.empty(), BaseMetalake.class, EntityType.METALAKE).stream() .toArray(BaseMetalake[]::new); } catch (IOException ioe) { LOG.error("Listing Metalakes failed due to storage issues.", ioe); diff --git a/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java b/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java index 1943ff7f04d..20a9fb79eb0 100644 --- a/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java +++ b/core/src/main/java/org/apache/gravitino/storage/kv/KvEntityStore.java @@ -47,7 +47,6 @@ import org.apache.gravitino.EntitySerDe; import org.apache.gravitino.EntitySerDeFactory; import org.apache.gravitino.EntityStore; -import org.apache.gravitino.Field; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; @@ -127,8 +126,7 @@ public void setSerDe(EntitySerDe entitySerDe) { @Override public List list( - Namespace namespace, Class e, EntityType type, List allowMissingFields) - throws IOException { + Namespace namespace, Class e, EntityType type) throws IOException { // Star means it's a wildcard List entities = Lists.newArrayList(); NameIdentifier identifier = NameIdentifier.of(namespace, BinaryEntityKeyEncoder.WILD_CARD); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java index e21a61572e6..78e5755c496 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java @@ -89,7 +89,7 @@ public void initialize(Config config) { @Override public List list( - Namespace namespace, Entity.EntityType entityType, List allowMissingFields) + Namespace namespace, Entity.EntityType entityType, List skippingFields) throws IOException { switch (entityType) { case METALAKE: @@ -108,7 +108,7 @@ public List list( return (List) TagMetaService.getInstance().listTagsByNamespace(namespace); case USER: return (List) - UserMetaService.getInstance().listUsersByNamespace(namespace, allowMissingFields); + UserMetaService.getInstance().listUsersByNamespace(namespace, skippingFields); default: throw new UnsupportedEntityTypeException( "Unsupported entity type: %s for list operation", entityType); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java index 3a0044b405c..6a9b0b8645c 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java @@ -49,7 +49,7 @@ public interface RelationalBackend * * @param namespace The parent namespace of these entities. * @param entityType The type of these entities. - * @param allowMissingFields Some fields may have a relatively high acquisition cost, EntityStore + * @param skippingFields Some fields may have a relatively high acquisition cost, EntityStore * provide an optional setting to avoid fetching these high-cost fields to improve the * performance. * @return The list of entities associated with the given parent namespace and entityType, or null @@ -59,7 +59,7 @@ public interface RelationalBackend * @throws IOException If the store operation fails */ List list( - Namespace namespace, Entity.EntityType entityType, List allowMissingFields) + Namespace namespace, Entity.EntityType entityType, List skippingFields) throws NoSuchEntityException, IOException; /** diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java index c3fe4f7da3c..0c269945c00 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableMap; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.function.Function; import org.apache.gravitino.Config; @@ -89,12 +90,15 @@ public void setSerDe(EntitySerDe entitySerDe) { @Override public List list( - Namespace namespace, - Class type, - Entity.EntityType entityType, - List allowMissingFields) + Namespace namespace, Class type, Entity.EntityType entityType) throws IOException { + return backend.list(namespace, entityType, Collections.emptyList()); + } + + @Override + public List list( + Namespace namespace, Class type, Entity.EntityType entityType, List skippingFields) throws IOException { - return backend.list(namespace, entityType, allowMissingFields); + return backend.list(namespace, entityType, skippingFields); } @Override 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 index a3d97a36125..a3b84c6fdfc 100644 --- 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 @@ -249,13 +249,12 @@ public UserEntity updateUser( return newEntity; } - public List listUsersByNamespace( - Namespace namespace, List allowMissingFields) { + public List listUsersByNamespace(Namespace namespace, List skippingFields) { AuthorizationUtils.checkUserNamespace(namespace); String metalakeName = namespace.level(0); - if (allowMissingFields.contains(UserEntity.ROLE_IDS) - && allowMissingFields.contains(UserEntity.ROLE_NAMES)) { + if (skippingFields.contains(UserEntity.ROLE_IDS) + && skippingFields.contains(UserEntity.ROLE_NAMES)) { List userPOs = SessionUtils.getWithoutCommit( UserMetaMapper.class, mapper -> mapper.listUserPOsByMetalake(metalakeName)); diff --git a/core/src/main/java/org/apache/gravitino/tag/TagManager.java b/core/src/main/java/org/apache/gravitino/tag/TagManager.java index 39c09b0f0a5..040ac9f1953 100644 --- a/core/src/main/java/org/apache/gravitino/tag/TagManager.java +++ b/core/src/main/java/org/apache/gravitino/tag/TagManager.java @@ -98,12 +98,7 @@ public Tag[] listTagsInfo(String metalake) { try { return entityStore - .list( - ofTagNamespace(metalake), - TagEntity.class, - Entity.EntityType.TAG, - Collections.emptyList()) - .stream() + .list(ofTagNamespace(metalake), TagEntity.class, Entity.EntityType.TAG).stream() .toArray(Tag[]::new); } catch (IOException ioe) { LOG.error("Failed to list tags under metalake {}", metalake, ioe); diff --git a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java index 19c2f8f25c8..6a79d39aa73 100644 --- a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java +++ b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java @@ -18,7 +18,20 @@ */ package org.apache.gravitino.authorization; +import static org.apache.gravitino.Configs.CATALOG_CACHE_EVICTION_INTERVAL_MS; +import static org.apache.gravitino.Configs.DEFAULT_ENTITY_RELATIONAL_STORE; +import static org.apache.gravitino.Configs.ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER; +import static org.apache.gravitino.Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL; +import static org.apache.gravitino.Configs.ENTITY_RELATIONAL_STORE; +import static org.apache.gravitino.Configs.ENTITY_STORE; +import static org.apache.gravitino.Configs.RELATIONAL_ENTITY_STORE; import static org.apache.gravitino.Configs.SERVICE_ADMINS; +import static org.apache.gravitino.Configs.STORE_DELETE_AFTER_TIME; +import static org.apache.gravitino.Configs.STORE_TRANSACTION_MAX_SKEW_TIME; +import static org.apache.gravitino.Configs.TREE_LOCK_CLEAN_INTERVAL; +import static org.apache.gravitino.Configs.TREE_LOCK_MAX_NODE_IN_MEMORY; +import static org.apache.gravitino.Configs.TREE_LOCK_MIN_NODE_IN_MEMORY; +import static org.apache.gravitino.Configs.VERSION_RETENTION_COUNT; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; @@ -27,13 +40,20 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; +import java.io.File; import java.io.IOException; import java.time.Instant; +import java.util.Arrays; +import java.util.Comparator; import java.util.Map; +import java.util.UUID; import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.Catalog; import org.apache.gravitino.Config; import org.apache.gravitino.EntityStore; +import org.apache.gravitino.EntityStoreFactory; import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.Namespace; import org.apache.gravitino.StringIdentifier; import org.apache.gravitino.catalog.CatalogManager; import org.apache.gravitino.connector.BaseCatalog; @@ -45,15 +65,17 @@ import org.apache.gravitino.exceptions.NoSuchUserException; import org.apache.gravitino.exceptions.RoleAlreadyExistsException; import org.apache.gravitino.exceptions.UserAlreadyExistsException; +import org.apache.gravitino.lock.LockManager; import org.apache.gravitino.meta.AuditInfo; import org.apache.gravitino.meta.BaseMetalake; +import org.apache.gravitino.meta.CatalogEntity; import org.apache.gravitino.meta.SchemaVersion; import org.apache.gravitino.storage.RandomIdGenerator; -import org.apache.gravitino.storage.memory.TestMemoryEntityStore; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; public class TestAccessControlManager { @@ -62,9 +84,13 @@ public class TestAccessControlManager { private static EntityStore entityStore; private static CatalogManager catalogManager = mock(CatalogManager.class); - private static Config config; + private static final Config config = Mockito.mock(Config.class); private static String METALAKE = "metalake"; + private static final String JDBC_STORE_PATH = + "/tmp/gravitino_jdbc_entityStore_" + UUID.randomUUID().toString().replace("-", ""); + private static final String DB_DIR = JDBC_STORE_PATH + "/testdb"; + private static AuthorizationPlugin authorizationPlugin; private static BaseMetalake metalakeEntity = @@ -78,7 +104,7 @@ public class TestAccessControlManager { private static BaseMetalake listMetalakeEntity = BaseMetalake.builder() - .withId(1L) + .withId(2L) .withName("metalake_list") .withAuditInfo( AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) @@ -87,16 +113,51 @@ public class TestAccessControlManager { @BeforeAll public static void setUp() throws Exception { - config = new Config(false) {}; - config.set(SERVICE_ADMINS, Lists.newArrayList("admin1", "admin2")); - - entityStore = new TestMemoryEntityStore.InMemoryEntityStore(); + File dbDir = new File(DB_DIR); + dbDir.mkdirs(); + Mockito.when(config.get(SERVICE_ADMINS)).thenReturn(Lists.newArrayList("admin1", "admin2")); + Mockito.when(config.get(ENTITY_STORE)).thenReturn(RELATIONAL_ENTITY_STORE); + Mockito.when(config.get(ENTITY_RELATIONAL_STORE)).thenReturn(DEFAULT_ENTITY_RELATIONAL_STORE); + Mockito.when(config.get(ENTITY_RELATIONAL_JDBC_BACKEND_URL)) + .thenReturn(String.format("jdbc:h2:file:%s;DB_CLOSE_DELAY=-1;MODE=MYSQL", DB_DIR)); + Mockito.when(config.get(ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER)).thenReturn("org.h2.Driver"); + Mockito.when(config.get(STORE_TRANSACTION_MAX_SKEW_TIME)).thenReturn(1000L); + Mockito.when(config.get(STORE_DELETE_AFTER_TIME)).thenReturn(20 * 60 * 1000L); + Mockito.when(config.get(VERSION_RETENTION_COUNT)).thenReturn(1L); + Mockito.when(config.get(CATALOG_CACHE_EVICTION_INTERVAL_MS)).thenReturn(1000L); + Mockito.doReturn(100000L).when(config).get(TREE_LOCK_MAX_NODE_IN_MEMORY); + Mockito.doReturn(1000L).when(config).get(TREE_LOCK_MIN_NODE_IN_MEMORY); + Mockito.doReturn(36000L).when(config).get(TREE_LOCK_CLEAN_INTERVAL); + FieldUtils.writeField(GravitinoEnv.getInstance(), "lockManager", new LockManager(config), true); + entityStore = EntityStoreFactory.createEntityStore(config); entityStore.initialize(config); - entityStore.setSerDe(null); entityStore.put(metalakeEntity, true); entityStore.put(listMetalakeEntity, true); + CatalogEntity catalogEntity = + CatalogEntity.builder() + .withId(3L) + .withName("catalog") + .withNamespace(Namespace.of("metalake")) + .withType(Catalog.Type.RELATIONAL) + .withProvider("test") + .withAuditInfo( + AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) + .build(); + entityStore.put(catalogEntity, true); + CatalogEntity anotherCatalogEntity = + CatalogEntity.builder() + .withId(4L) + .withName("catalog") + .withNamespace(Namespace.of("metalake_list")) + .withType(Catalog.Type.RELATIONAL) + .withProvider("test") + .withAuditInfo( + AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) + .build(); + entityStore.put(anotherCatalogEntity, true); + accessControlManager = new AccessControlManager(entityStore, new RandomIdGenerator(), config); FieldUtils.writeField(GravitinoEnv.getInstance(), "entityStore", entityStore, true); FieldUtils.writeField( @@ -118,11 +179,11 @@ public static void tearDown() throws IOException { @Test public void testAddUser() { - User user = accessControlManager.addUser("metalake", "testAdd"); + User user = accessControlManager.addUser(METALAKE, "testAdd"); Assertions.assertEquals("testAdd", user.name()); Assertions.assertTrue(user.roles().isEmpty()); - user = accessControlManager.addUser("metalake", "testAddWithOptionalField"); + user = accessControlManager.addUser(METALAKE, "testAddWithOptionalField"); Assertions.assertEquals("testAddWithOptionalField", user.name()); Assertions.assertTrue(user.roles().isEmpty()); @@ -133,42 +194,42 @@ public void testAddUser() { // Test with UserAlreadyExistsException Assertions.assertThrows( - UserAlreadyExistsException.class, - () -> accessControlManager.addUser("metalake", "testAdd")); + UserAlreadyExistsException.class, () -> accessControlManager.addUser(METALAKE, "testAdd")); } @Test public void testGetUser() { - accessControlManager.addUser("metalake", "testGet"); + accessControlManager.addUser(METALAKE, "testGet"); - User user = accessControlManager.getUser("metalake", "testGet"); + User user = accessControlManager.getUser(METALAKE, "testGet"); Assertions.assertEquals("testGet", user.name()); // Test with NoSuchMetalakeException Assertions.assertThrows( - NoSuchMetalakeException.class, () -> accessControlManager.addUser("no-exist", "testAdd")); + NoSuchMetalakeException.class, () -> accessControlManager.getUser("no-exist", "testAdd")); // Test to get non-existed user Throwable exception = Assertions.assertThrows( - NoSuchUserException.class, () -> accessControlManager.getUser("metalake", "not-exist")); + NoSuchUserException.class, () -> accessControlManager.getUser(METALAKE, "not-exist")); Assertions.assertTrue(exception.getMessage().contains("User not-exist does not exist")); } @Test public void testRemoveUser() { - accessControlManager.addUser("metalake", "testRemove"); + accessControlManager.addUser(METALAKE, "testRemove"); // Test with NoSuchMetalakeException Assertions.assertThrows( - NoSuchMetalakeException.class, () -> accessControlManager.addUser("no-exist", "testAdd")); + NoSuchMetalakeException.class, + () -> accessControlManager.removeUser("no-exist", "testAdd")); // Test to remove user - boolean removed = accessControlManager.removeUser("metalake", "testRemove"); + boolean removed = accessControlManager.removeUser(METALAKE, "testRemove"); Assertions.assertTrue(removed); // Test to remove non-existed user - boolean removed1 = accessControlManager.removeUser("metalake", "no-exist"); + boolean removed1 = accessControlManager.removeUser(METALAKE, "no-exist"); Assertions.assertFalse(removed1); } @@ -178,69 +239,76 @@ public void testListUsers() { accessControlManager.addUser("metalake_list", "testList2"); // Test to list users - Assertions.assertArrayEquals( - new String[] {"testList2", "testList1"}, - accessControlManager.listUserNames("metalake_list")); + String[] expectUsernames = new String[] {"testList1", "testList2"}; + String[] actualUsernames = accessControlManager.listUserNames("metalake_list"); + Arrays.sort(actualUsernames); + Assertions.assertArrayEquals(expectUsernames, actualUsernames); User[] users = accessControlManager.listUsers("metalake_list"); - Assertions.assertEquals(2, users.length); - Assertions.assertEquals("testList1", users[1].name()); - Assertions.assertEquals("testList2", users[0].name()); + Arrays.sort(users, Comparator.comparing(User::name)); + Assertions.assertArrayEquals( + expectUsernames, Arrays.stream(users).map(User::name).toArray(String[]::new)); + + // Test with NoSuchMetalakeException + Assertions.assertThrows( + NoSuchMetalakeException.class, () -> accessControlManager.listUserNames("no-exist")); + Assertions.assertThrows( + NoSuchMetalakeException.class, () -> accessControlManager.listUsers("no-exist")); } @Test public void testAddGroup() { - Group group = accessControlManager.addGroup("metalake", "testAdd"); + Group group = accessControlManager.addGroup(METALAKE, "testAdd"); Assertions.assertEquals("testAdd", group.name()); Assertions.assertTrue(group.roles().isEmpty()); - group = accessControlManager.addGroup("metalake", "testAddWithOptionalField"); + group = accessControlManager.addGroup(METALAKE, "testAddWithOptionalField"); Assertions.assertEquals("testAddWithOptionalField", group.name()); Assertions.assertTrue(group.roles().isEmpty()); // Test with NoSuchMetalakeException Assertions.assertThrows( - NoSuchMetalakeException.class, () -> accessControlManager.addUser("no-exist", "testAdd")); + NoSuchMetalakeException.class, () -> accessControlManager.addGroup("no-exist", "testAdd")); // Test with GroupAlreadyExistsException Assertions.assertThrows( GroupAlreadyExistsException.class, - () -> accessControlManager.addGroup("metalake", "testAdd")); + () -> accessControlManager.addGroup(METALAKE, "testAdd")); } @Test public void testGetGroup() { - accessControlManager.addGroup("metalake", "testGet"); + accessControlManager.addGroup(METALAKE, "testGet"); - Group group = accessControlManager.getGroup("metalake", "testGet"); + Group group = accessControlManager.getGroup(METALAKE, "testGet"); Assertions.assertEquals("testGet", group.name()); // Test with NoSuchMetalakeException Assertions.assertThrows( - NoSuchMetalakeException.class, () -> accessControlManager.addUser("no-exist", "testAdd")); + NoSuchMetalakeException.class, () -> accessControlManager.getGroup("no-exist", "testAdd")); // Test to get non-existed group Throwable exception = Assertions.assertThrows( - NoSuchGroupException.class, - () -> accessControlManager.getGroup("metalake", "not-exist")); + NoSuchGroupException.class, () -> accessControlManager.getGroup(METALAKE, "not-exist")); Assertions.assertTrue(exception.getMessage().contains("Group not-exist does not exist")); } @Test public void testRemoveGroup() { - accessControlManager.addGroup("metalake", "testRemove"); + accessControlManager.addGroup(METALAKE, "testRemove"); // Test with NoSuchMetalakeException Assertions.assertThrows( - NoSuchMetalakeException.class, () -> accessControlManager.addUser("no-exist", "testAdd")); + NoSuchMetalakeException.class, + () -> accessControlManager.removeGroup("no-exist", "testAdd")); // Test to remove group - boolean removed = accessControlManager.removeGroup("metalake", "testRemove"); + boolean removed = accessControlManager.removeGroup(METALAKE, "testRemove"); Assertions.assertTrue(removed); // Test to remove non-existed group - boolean removed1 = accessControlManager.removeUser("metalake", "no-exist"); + boolean removed1 = accessControlManager.removeUser(METALAKE, "no-exist"); Assertions.assertFalse(removed1); } @@ -258,7 +326,7 @@ public void testCreateRole() { Role role = accessControlManager.createRole( - "metalake", + METALAKE, "create", props, Lists.newArrayList( @@ -273,7 +341,7 @@ public void testCreateRole() { RoleAlreadyExistsException.class, () -> accessControlManager.createRole( - "metalake", + METALAKE, "create", props, Lists.newArrayList( @@ -286,14 +354,14 @@ public void testLoadRole() { Map props = ImmutableMap.of("k1", "v1"); accessControlManager.createRole( - "metalake", + METALAKE, "loadRole", props, Lists.newArrayList( SecurableObjects.ofCatalog( "catalog", Lists.newArrayList(Privileges.UseCatalog.allow())))); - Role role = accessControlManager.getRole("metalake", "loadRole"); + Role role = accessControlManager.getRole(METALAKE, "loadRole"); Assertions.assertEquals("loadRole", role.name()); testProperties(props, role.properties()); @@ -301,7 +369,7 @@ public void testLoadRole() { // Test load non-existed group Throwable exception = Assertions.assertThrows( - NoSuchRoleException.class, () -> accessControlManager.getRole("metalake", "not-exist")); + NoSuchRoleException.class, () -> accessControlManager.getRole(METALAKE, "not-exist")); Assertions.assertTrue(exception.getMessage().contains("Role not-exist does not exist")); } @@ -310,7 +378,7 @@ public void testDropRole() { Map props = ImmutableMap.of("k1", "v1"); accessControlManager.createRole( - "metalake", + METALAKE, "testDrop", props, Lists.newArrayList( @@ -319,13 +387,13 @@ public void testDropRole() { // Test drop role reset(authorizationPlugin); - boolean dropped = accessControlManager.deleteRole("metalake", "testDrop"); + boolean dropped = accessControlManager.deleteRole(METALAKE, "testDrop"); Assertions.assertTrue(dropped); verify(authorizationPlugin).onRoleDeleted(any()); // Test drop non-existed role - boolean dropped1 = accessControlManager.deleteRole("metalake", "no-exist"); + boolean dropped1 = accessControlManager.deleteRole(METALAKE, "no-exist"); Assertions.assertFalse(dropped1); } diff --git a/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java b/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java index 94218e89624..75c3f30ba7c 100644 --- a/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java +++ b/core/src/test/java/org/apache/gravitino/storage/kv/TestKvEntityStorage.java @@ -31,7 +31,6 @@ import java.io.IOException; import java.nio.file.Files; import java.time.Instant; -import java.util.Collections; import java.util.List; import java.util.concurrent.CompletionService; import java.util.concurrent.ExecutionException; @@ -135,11 +134,7 @@ void testCreateKvEntityStore() throws IOException { // Test scan and store list interface List catalogEntityList = - store.list( - catalog.namespace(), - CatalogEntity.class, - Entity.EntityType.CATALOG, - Collections.emptyList()); + store.list(catalog.namespace(), CatalogEntity.class, Entity.EntityType.CATALOG); Assertions.assertEquals(3, catalogEntityList.size()); Assertions.assertTrue(catalogEntityList.contains(catalog)); Assertions.assertTrue(catalogEntityList.contains(catalogCopy)); diff --git a/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java b/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java index 46ef26bd12a..7cff7bcc553 100644 --- a/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java +++ b/core/src/test/java/org/apache/gravitino/storage/memory/TestMemoryEntityStore.java @@ -34,7 +34,6 @@ import org.apache.gravitino.EntityAlreadyExistsException; import org.apache.gravitino.EntitySerDe; import org.apache.gravitino.EntityStore; -import org.apache.gravitino.Field; import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.Metalake; import org.apache.gravitino.NameIdentifier; @@ -83,8 +82,7 @@ public void setSerDe(EntitySerDe entitySerDe) {} @Override public List list( - Namespace namespace, Class cl, EntityType entityType, List allowMissingFields) - throws IOException { + Namespace namespace, Class cl, EntityType entityType) throws IOException { return entityMap.entrySet().stream() .filter(e -> e.getKey().namespace().equals(namespace)) .map(entry -> (E) entry.getValue()) From d371b97def8018af89a242f67e9b2f583949fae5 Mon Sep 17 00:00:00 2001 From: Rory Date: Fri, 20 Sep 2024 18:38:52 +0800 Subject: [PATCH 11/31] fix test code errors --- .../gravitino/authorization/TestAccessControlManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java index 6a79d39aa73..1c7a26dec53 100644 --- a/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java +++ b/core/src/test/java/org/apache/gravitino/authorization/TestAccessControlManager.java @@ -308,7 +308,7 @@ public void testRemoveGroup() { Assertions.assertTrue(removed); // Test to remove non-existed group - boolean removed1 = accessControlManager.removeUser(METALAKE, "no-exist"); + boolean removed1 = accessControlManager.removeGroup(METALAKE, "no-exist"); Assertions.assertFalse(removed1); } @@ -366,7 +366,7 @@ public void testLoadRole() { Assertions.assertEquals("loadRole", role.name()); testProperties(props, role.properties()); - // Test load non-existed group + // Test load non-existed role Throwable exception = Assertions.assertThrows( NoSuchRoleException.class, () -> accessControlManager.getRole(METALAKE, "not-exist")); From 93d7d3880c1c9c07cdb2747e6326bce342b2aae1 Mon Sep 17 00:00:00 2001 From: Rory Date: Fri, 20 Sep 2024 21:47:07 +0800 Subject: [PATCH 12/31] Add a new interface --- .../storage/relational/po/CombinedUserPO.java | 20 +---- .../service/SupportsSkippingFields.java | 40 +++++++++ .../SupportsSkippingFieldsHandlers.java | 49 +++++++++++ .../relational/service/UserMetaService.java | 84 +++++++++++++------ 4 files changed, 152 insertions(+), 41 deletions(-) create mode 100644 core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java create mode 100644 core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java b/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java index e6dec7cf920..91732540d16 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java @@ -47,28 +47,14 @@ public boolean equals(Object o) { return false; } CombinedUserPO combinedUserPO = (CombinedUserPO) o; - return Objects.equal(getUserId(), combinedUserPO.getUserId()) - && Objects.equal(getUserName(), combinedUserPO.getUserName()) - && Objects.equal(getMetalakeId(), combinedUserPO.getMetalakeId()) - && Objects.equal(getAuditInfo(), combinedUserPO.getAuditInfo()) - && Objects.equal(getCurrentVersion(), combinedUserPO.getCurrentVersion()) - && Objects.equal(getLastVersion(), combinedUserPO.getLastVersion()) - && Objects.equal(getDeletedAt(), combinedUserPO.getDeletedAt()) + + return super.equals(o) && Objects.equal(getRoleIds(), combinedUserPO.getRoleIds()) && Objects.equal(getRoleNames(), combinedUserPO.getRoleNames()); } @Override public int hashCode() { - return Objects.hashCode( - getUserId(), - getUserName(), - getMetalakeId(), - getAuditInfo(), - getCurrentVersion(), - getLastVersion(), - getDeletedAt(), - getRoleIds(), - getRoleNames()); + return Objects.hashCode(super.hashCode(), getRoleIds(), getRoleNames()); } } diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java new file mode 100644 index 00000000000..df968589a1b --- /dev/null +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java @@ -0,0 +1,40 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.storage.relational.service; + +import java.util.List; +import org.apache.gravitino.Field; + +/** The handler supports to skip fields. */ +interface SupportsSkippingFields { + + /** + * The fields which could be skipped. + * + * @return The fields which could be skipped. + */ + List supportsSkippingFields(); + + /** + * The return value of the handler. + * + * @return The return value of the handler. + */ + R execute(); +} diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java new file mode 100644 index 00000000000..af38d53d86b --- /dev/null +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.storage.relational.service; + +import com.google.common.collect.Lists; +import java.util.HashSet; +import java.util.List; +import org.apache.gravitino.Field; + +/** + * This class is the collection wrapper of SupportsSkippingFields handler. The class will contains + * all the handlers can proceed the data. We can choose different handlers according to the skipping + * fields to acquire better performance. + * + * @param The value type which the handler will return. + */ +class SupportsSkippingFieldsHandlers { + private final List> methods = Lists.newArrayList(); + + // We should put the low-cost handler into the front of the list. + void addHandler(SupportsSkippingFields supportsSkippingFields) { + methods.add(supportsSkippingFields); + } + + T executeHandler(List skippingFields) { + for (SupportsSkippingFields method : methods) { + if (new HashSet<>(skippingFields).containsAll(method.supportsSkippingFields())) { + return method.execute(); + } + } + throw new IllegalArgumentException("Don't support skip fields"); + } +} 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 index a3b84c6fdfc..765429dfda8 100644 --- 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 @@ -253,8 +253,49 @@ public List listUsersByNamespace(Namespace namespace, List sk AuthorizationUtils.checkUserNamespace(namespace); String metalakeName = namespace.level(0); - if (skippingFields.contains(UserEntity.ROLE_IDS) - && skippingFields.contains(UserEntity.ROLE_NAMES)) { + SupportsSkippingFieldsHandlers> handlers = + new SupportsSkippingFieldsHandlers<>(); + handlers.addHandler(new ListSkippingRolesHandler(metalakeName)); + handlers.addHandler(new listAllFieldsHandler(metalakeName)); + + return handlers.executeHandler(skippingFields); + } + + public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { + int[] userDeletedCount = new int[] {0}; + int[] userRoleRelDeletedCount = new int[] {0}; + + SessionUtils.doMultipleWithCommit( + () -> + userDeletedCount[0] = + SessionUtils.doWithoutCommitAndFetchResult( + UserMetaMapper.class, + mapper -> mapper.deleteUserMetasByLegacyTimeline(legacyTimeline, limit)), + () -> + userRoleRelDeletedCount[0] = + SessionUtils.doWithoutCommitAndFetchResult( + UserRoleRelMapper.class, + mapper -> + mapper.deleteUserRoleRelMetasByLegacyTimeline(legacyTimeline, limit))); + + return userDeletedCount[0] + userRoleRelDeletedCount[0]; + } + + private static class ListSkippingRolesHandler + implements SupportsSkippingFields> { + private final String metalakeName; + + ListSkippingRolesHandler(String metalakeName) { + this.metalakeName = metalakeName; + } + + @Override + public List supportsSkippingFields() { + return Lists.newArrayList(UserEntity.ROLE_IDS, UserEntity.ROLE_NAMES); + } + + @Override + public List execute() { List userPOs = SessionUtils.getWithoutCommit( UserMetaMapper.class, mapper -> mapper.listUserPOsByMetalake(metalakeName)); @@ -266,9 +307,24 @@ public List listUsersByNamespace(Namespace namespace, List sk Collections.emptyList(), AuthorizationUtils.ofUserNamespace(metalakeName))) .collect(Collectors.toList()); + } + } + + private static class listAllFieldsHandler implements SupportsSkippingFields> { + final String metalakeName; + + listAllFieldsHandler(String metalakeName) { + this.metalakeName = metalakeName; + } + + @Override + public List supportsSkippingFields() { + return Collections.emptyList(); + } - } else { - Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(namespace.level(0)); + @Override + public List execute() { + Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(metalakeName); List userPOs = SessionUtils.getWithoutCommit( UserMetaMapper.class, mapper -> mapper.listCombinedUserPOsByMetalakeId(metalakeId)); @@ -280,24 +336,4 @@ public List listUsersByNamespace(Namespace namespace, List sk .collect(Collectors.toList()); } } - - public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { - int[] userDeletedCount = new int[] {0}; - int[] userRoleRelDeletedCount = new int[] {0}; - - SessionUtils.doMultipleWithCommit( - () -> - userDeletedCount[0] = - SessionUtils.doWithoutCommitAndFetchResult( - UserMetaMapper.class, - mapper -> mapper.deleteUserMetasByLegacyTimeline(legacyTimeline, limit)), - () -> - userRoleRelDeletedCount[0] = - SessionUtils.doWithoutCommitAndFetchResult( - UserRoleRelMapper.class, - mapper -> - mapper.deleteUserRoleRelMetasByLegacyTimeline(legacyTimeline, limit))); - - return userDeletedCount[0] + userRoleRelDeletedCount[0]; - } } From 1707153a0fff1be456f21b5d230f8ff8a378116b Mon Sep 17 00:00:00 2001 From: Rory Date: Fri, 20 Sep 2024 21:51:44 +0800 Subject: [PATCH 13/31] make default for list --- core/src/main/java/org/apache/gravitino/EntityStore.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/EntityStore.java b/core/src/main/java/org/apache/gravitino/EntityStore.java index feb043eefe9..d5f49f48986 100644 --- a/core/src/main/java/org/apache/gravitino/EntityStore.java +++ b/core/src/main/java/org/apache/gravitino/EntityStore.java @@ -20,6 +20,7 @@ import java.io.Closeable; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.function.Function; import org.apache.gravitino.Entity.EntityType; @@ -62,8 +63,10 @@ public interface EntityStore extends Closeable { * @return the list of entities * @throws IOException if the list operation fails */ - List list( - Namespace namespace, Class type, EntityType entityType) throws IOException; + default List list( + Namespace namespace, Class type, EntityType entityType) throws IOException { + return list(namespace, type, entityType, Collections.emptyList()); + } /** * List all the entities with the specified {@link org.apache.gravitino.Namespace}, and From ff1640fdeacb004226fa8a807df280a83ddca882 Mon Sep 17 00:00:00 2001 From: Rory Date: Fri, 20 Sep 2024 22:03:04 +0800 Subject: [PATCH 14/31] rename --- .../storage/relational/service/UserMetaService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 index 765429dfda8..bbfb2da426f 100644 --- 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 @@ -256,7 +256,7 @@ public List listUsersByNamespace(Namespace namespace, List sk SupportsSkippingFieldsHandlers> handlers = new SupportsSkippingFieldsHandlers<>(); handlers.addHandler(new ListSkippingRolesHandler(metalakeName)); - handlers.addHandler(new listAllFieldsHandler(metalakeName)); + handlers.addHandler(new ListAllFieldsHandler(metalakeName)); return handlers.executeHandler(skippingFields); } @@ -310,10 +310,10 @@ public List execute() { } } - private static class listAllFieldsHandler implements SupportsSkippingFields> { + private static class ListAllFieldsHandler implements SupportsSkippingFields> { final String metalakeName; - listAllFieldsHandler(String metalakeName) { + ListAllFieldsHandler(String metalakeName) { this.metalakeName = metalakeName; } From fdc9133bd6aae1532fd6e2cff6061ea5248142d8 Mon Sep 17 00:00:00 2001 From: Rory Date: Fri, 20 Sep 2024 22:08:57 +0800 Subject: [PATCH 15/31] Remove rebudant cast --- .../apache/gravitino/authorization/UserGroupManager.java | 7 +++---- 1 file changed, 3 insertions(+), 4 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 index 8b9ed13b6fd..bccf3a656b0 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java @@ -132,10 +132,9 @@ private User[] listUsersInternal(String metalake, List skippingFields) { AuthorizationUtils.checkMetalakeExists(metalake); Namespace namespace = AuthorizationUtils.ofUserNamespace(metalake); - return store.list(namespace, UserEntity.class, Entity.EntityType.USER, skippingFields) - .stream() - .map(entity -> (User) entity) - .toArray(User[]::new); + return store + .list(namespace, UserEntity.class, Entity.EntityType.USER, skippingFields) + .toArray(new User[0]); } catch (NoSuchEntityException e) { LOG.warn("Metalake {} does not exist", metalake, e); throw new NoSuchMetalakeException(METALAKE_DOES_NOT_EXIST_MSG, metalake); From 1adae396b922e5f75ed24207a7613dd3ee0674b6 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 11:44:17 +0800 Subject: [PATCH 16/31] Add user with role in IT --- .../test/authorization/AccessControlIT.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java index 1ff39943646..112890a3f64 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java @@ -75,6 +75,16 @@ void testManageUsers() { Assertions.assertEquals(username, user.name()); Assertions.assertTrue(user.roles().isEmpty()); + Map properties = Maps.newHashMap(); + properties.put("k1", "v1"); + SecurableObject metalakeObject = + SecurableObjects.ofMetalake( + metalakeName, Lists.newArrayList(Privileges.CreateCatalog.allow())); + + // Test the user with the role + metalake.createRole("role1", properties, Lists.newArrayList(metalakeObject)); + metalake.grantRolesToUser(Lists.newArrayList("role1"), username); + // List users String anotherUser = "another-user"; metalake.addUser(anotherUser); @@ -90,6 +100,7 @@ void testManageUsers() { .map(User::name) .sorted(String::compareTo) .collect(Collectors.toList())); + Assertions.assertEquals(Lists.newArrayList("role1"), users[2].roles()); // Get a not-existed user Assertions.assertThrows(NoSuchUserException.class, () -> metalake.getUser("not-existed")); @@ -100,6 +111,7 @@ void testManageUsers() { // clean up metalake.removeUser(anotherUser); + metalake.deleteRole("role1"); } @Test From be337b39787a76f6624c979958f0290bc2b1cd74 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 11:49:09 +0800 Subject: [PATCH 17/31] address comment --- .../relational/service/SupportsSkippingFieldsHandlers.java | 6 +++--- .../storage/relational/service/UserMetaService.java | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java index af38d53d86b..f3d89eef3b7 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java @@ -19,8 +19,8 @@ package org.apache.gravitino.storage.relational.service; import com.google.common.collect.Lists; -import java.util.HashSet; import java.util.List; +import java.util.Set; import org.apache.gravitino.Field; /** @@ -38,9 +38,9 @@ void addHandler(SupportsSkippingFields supportsSkippingFields) { methods.add(supportsSkippingFields); } - T executeHandler(List skippingFields) { + T execute(Set skippingFields) { for (SupportsSkippingFields method : methods) { - if (new HashSet<>(skippingFields).containsAll(method.supportsSkippingFields())) { + if (skippingFields.containsAll(method.supportsSkippingFields())) { return method.execute(); } } 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 index bbfb2da426f..5025c46897d 100644 --- 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 @@ -23,6 +23,7 @@ import com.google.common.collect.Sets; import java.io.IOException; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -258,7 +259,7 @@ public List listUsersByNamespace(Namespace namespace, List sk handlers.addHandler(new ListSkippingRolesHandler(metalakeName)); handlers.addHandler(new ListAllFieldsHandler(metalakeName)); - return handlers.executeHandler(skippingFields); + return handlers.execute(new HashSet<>(skippingFields)); } public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { From 5a03882a03fcee6a0a90e13c582121c2fd5db01a Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 14:47:08 +0800 Subject: [PATCH 18/31] Fix it --- .../test/authorization/AccessControlIT.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java index 112890a3f64..662e2c15922 100644 --- a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java +++ b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java @@ -22,6 +22,8 @@ import com.google.common.collect.Maps; import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; +import java.util.List; import java.util.Map; import java.util.stream.Collectors; import org.apache.gravitino.Configs; @@ -93,14 +95,14 @@ void testManageUsers() { Assertions.assertEquals( Lists.newArrayList(AuthConstants.ANONYMOUS_USER, anotherUser, username), Arrays.asList(usernames)); - User[] users = metalake.listUsers(); + List users = + Arrays.stream(metalake.listUsers()) + .sorted(Comparator.comparing(User::name)) + .collect(Collectors.toList()); Assertions.assertEquals( Lists.newArrayList(AuthConstants.ANONYMOUS_USER, anotherUser, username), - Arrays.stream(users) - .map(User::name) - .sorted(String::compareTo) - .collect(Collectors.toList())); - Assertions.assertEquals(Lists.newArrayList("role1"), users[2].roles()); + users.stream().map(User::name).collect(Collectors.toList())); + Assertions.assertEquals(Lists.newArrayList("role1"), users.get(2).roles()); // Get a not-existed user Assertions.assertThrows(NoSuchUserException.class, () -> metalake.getUser("not-existed")); From 6cddd60930c02c2a1c95c227bb4d582bec0b4168 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 16:16:18 +0800 Subject: [PATCH 19/31] Fix it --- .../storage/relational/utils/POConverters.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java index b4204dc2c12..6b1637e0c85 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java @@ -24,6 +24,7 @@ import java.time.Instant; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.apache.gravitino.Catalog; @@ -756,14 +757,19 @@ public static UserEntity fromCombinedUserPO(CombinedUserPO userPO, Namespace nam } if (StringUtils.isNotBlank(userPO.getRoleIds())) { - List roleIdsFromJson = + // Different JSON AGG from backends will produce different types data, we + // can only use Object. PostSQL produces the data with type Long. H2 produces + // the data with type String. + List roleIdsFromJson = JsonUtils.anyFieldMapper().readValue(userPO.getRoleIds(), List.class); List roleIds = roleIdsFromJson.stream() + .filter(Objects::nonNull) + .map(String::valueOf) .filter(StringUtils::isNotBlank) .map(Long::valueOf) .collect(Collectors.toList()); - ; + if (!roleIds.isEmpty()) { builder.withRoleIds(roleIds); } From c2c5c220aa20f2147dc76c772c7ae92146cd0a70 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 16:57:12 +0800 Subject: [PATCH 20/31] change log level --- .../org/apache/gravitino/authorization/UserGroupManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java index bccf3a656b0..c66e0974612 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java @@ -136,7 +136,7 @@ private User[] listUsersInternal(String metalake, List skippingFields) { .list(namespace, UserEntity.class, Entity.EntityType.USER, skippingFields) .toArray(new User[0]); } catch (NoSuchEntityException e) { - LOG.warn("Metalake {} does not exist", metalake, e); + LOG.error("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); From 7f5bf9b14480d53e8ca7de0051d3c4a8bd73e23c Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 17:03:35 +0800 Subject: [PATCH 21/31] Change type --- .../java/org/apache/gravitino/EntityStore.java | 5 +++-- .../gravitino/authorization/UserGroupManager.java | 15 +++++++++------ .../storage/relational/RelationalEntityStore.java | 3 ++- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/EntityStore.java b/core/src/main/java/org/apache/gravitino/EntityStore.java index d5f49f48986..4967e21ce78 100644 --- a/core/src/main/java/org/apache/gravitino/EntityStore.java +++ b/core/src/main/java/org/apache/gravitino/EntityStore.java @@ -22,6 +22,7 @@ import java.io.IOException; import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.function.Function; import org.apache.gravitino.Entity.EntityType; import org.apache.gravitino.exceptions.NoSuchEntityException; @@ -65,7 +66,7 @@ public interface EntityStore extends Closeable { */ default List list( Namespace namespace, Class type, EntityType entityType) throws IOException { - return list(namespace, type, entityType, Collections.emptyList()); + return list(namespace, type, entityType, Collections.emptySet()); } /** @@ -86,7 +87,7 @@ default List list( * @throws IOException if the list operation fails */ default List list( - Namespace namespace, Class type, EntityType entityType, List skippingFields) + Namespace namespace, Class type, EntityType entityType, Set skippingFields) throws IOException { throw new UnsupportedOperationException("Don't support to skip fields"); } diff --git a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java index c66e0974612..4b7b4f2d8c3 100644 --- a/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java +++ b/core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java @@ -23,7 +23,7 @@ import java.time.Instant; import java.util.Arrays; import java.util.Collections; -import java.util.List; +import java.util.Set; import org.apache.gravitino.Entity; import org.apache.gravitino.EntityAlreadyExistsException; import org.apache.gravitino.EntityStore; @@ -40,6 +40,7 @@ import org.apache.gravitino.meta.UserEntity; import org.apache.gravitino.storage.IdGenerator; import org.apache.gravitino.utils.PrincipalUtils; +import org.glassfish.jersey.internal.guava.Sets; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -116,18 +117,20 @@ User getUser(String metalake, String user) throws NoSuchUserException { } String[] listUserNames(String metalake) { - return Arrays.stream( - listUsersInternal( - metalake, Lists.newArrayList(UserEntity.ROLE_NAMES, UserEntity.ROLE_IDS))) + Set skippingFields = Sets.newHashSet(); + skippingFields.add(UserEntity.ROLE_NAMES); + skippingFields.add(UserEntity.ROLE_IDS); + + return Arrays.stream(listUsersInternal(metalake, skippingFields)) .map(User::name) .toArray(String[]::new); } User[] listUsers(String metalake) { - return listUsersInternal(metalake, Collections.emptyList()); + return listUsersInternal(metalake, Collections.emptySet()); } - private User[] listUsersInternal(String metalake, List skippingFields) { + private User[] listUsersInternal(String metalake, Set skippingFields) { try { AuthorizationUtils.checkMetalakeExists(metalake); diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java index 0c269945c00..8807bf3fa5e 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java @@ -24,6 +24,7 @@ import java.io.IOException; import java.util.Collections; import java.util.List; +import java.util.Set; import java.util.function.Function; import org.apache.gravitino.Config; import org.apache.gravitino.Configs; @@ -96,7 +97,7 @@ public List list( @Override public List list( - Namespace namespace, Class type, Entity.EntityType entityType, List skippingFields) + Namespace namespace, Class type, Entity.EntityType entityType, Set skippingFields) throws IOException { return backend.list(namespace, entityType, skippingFields); } From 882d70d218ab8dfd0efb42c34d602d45acd575ad Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 17:16:12 +0800 Subject: [PATCH 22/31] fix CI --- .../gravitino/storage/relational/JDBCBackend.java | 3 ++- .../storage/relational/RelationalBackend.java | 3 ++- .../storage/relational/RelationalEntityStore.java | 2 +- .../relational/service/UserMetaService.java | 2 +- .../storage/relational/TestJDBCBackend.java | 14 +++++++------- .../relational/service/TestUserMetaService.java | 2 +- 6 files changed, 14 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java index 78e5755c496..549c5fec2e4 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java @@ -26,6 +26,7 @@ import java.io.IOException; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Function; import org.apache.gravitino.Config; import org.apache.gravitino.Configs; @@ -89,7 +90,7 @@ public void initialize(Config config) { @Override public List list( - Namespace namespace, Entity.EntityType entityType, List skippingFields) + Namespace namespace, Entity.EntityType entityType, Set skippingFields) throws IOException { switch (entityType) { case METALAKE: diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java index 6a9b0b8645c..fe85754b4ef 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalBackend.java @@ -21,6 +21,7 @@ import java.io.Closeable; import java.io.IOException; import java.util.List; +import java.util.Set; import java.util.function.Function; import org.apache.gravitino.Config; import org.apache.gravitino.Entity; @@ -59,7 +60,7 @@ public interface RelationalBackend * @throws IOException If the store operation fails */ List list( - Namespace namespace, Entity.EntityType entityType, List skippingFields) + Namespace namespace, Entity.EntityType entityType, Set skippingFields) throws NoSuchEntityException, IOException; /** diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java index 8807bf3fa5e..d7403729f60 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java @@ -92,7 +92,7 @@ public void setSerDe(EntitySerDe entitySerDe) { @Override public List list( Namespace namespace, Class type, Entity.EntityType entityType) throws IOException { - return backend.list(namespace, entityType, Collections.emptyList()); + return backend.list(namespace, entityType, Collections.emptySet()); } @Override 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 index 5025c46897d..4ef50c7a9f1 100644 --- 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 @@ -250,7 +250,7 @@ public UserEntity updateUser( return newEntity; } - public List listUsersByNamespace(Namespace namespace, List skippingFields) { + public List listUsersByNamespace(Namespace namespace, Set skippingFields) { AuthorizationUtils.checkUserNamespace(namespace); String metalakeName = namespace.level(0); diff --git a/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java b/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java index 454fba97ae6..b141eb6963b 100644 --- a/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java +++ b/core/src/test/java/org/apache/gravitino/storage/relational/TestJDBCBackend.java @@ -715,29 +715,29 @@ public void testMetaLifeCycleFromCreationToDeletion() throws IOException { // meta data list List metaLakes = - backend.list(metalake.namespace(), Entity.EntityType.METALAKE, Collections.emptyList()); + backend.list(metalake.namespace(), Entity.EntityType.METALAKE, Collections.emptySet()); assertTrue(metaLakes.contains(metalake)); List catalogs = - backend.list(catalog.namespace(), Entity.EntityType.CATALOG, Collections.emptyList()); + backend.list(catalog.namespace(), Entity.EntityType.CATALOG, Collections.emptySet()); assertTrue(catalogs.contains(catalog)); List schemas = - backend.list(schema.namespace(), Entity.EntityType.SCHEMA, Collections.emptyList()); + backend.list(schema.namespace(), Entity.EntityType.SCHEMA, Collections.emptySet()); assertTrue(schemas.contains(schema)); List tables = - backend.list(table.namespace(), Entity.EntityType.TABLE, Collections.emptyList()); + backend.list(table.namespace(), Entity.EntityType.TABLE, Collections.emptySet()); assertTrue(tables.contains(table)); List filesets = - backend.list(fileset.namespace(), Entity.EntityType.FILESET, Collections.emptyList()); + backend.list(fileset.namespace(), Entity.EntityType.FILESET, Collections.emptySet()); assertFalse(filesets.contains(fileset)); assertTrue(filesets.contains(filesetV2)); assertEquals("2", filesets.get(filesets.indexOf(filesetV2)).properties().get("version")); List topics = - backend.list(topic.namespace(), Entity.EntityType.TOPIC, Collections.emptyList()); + backend.list(topic.namespace(), Entity.EntityType.TOPIC, Collections.emptySet()); assertTrue(topics.contains(topic)); RoleEntity roleEntity = backend.get(role.nameIdentifier(), Entity.EntityType.ROLE); @@ -764,7 +764,7 @@ public void testMetaLifeCycleFromCreationToDeletion() throws IOException { TagEntity tagEntity = backend.get(tag.nameIdentifier(), Entity.EntityType.TAG); assertEquals(tag, tagEntity); List tags = - backend.list(tag.namespace(), Entity.EntityType.TAG, Collections.emptyList()); + backend.list(tag.namespace(), Entity.EntityType.TAG, Collections.emptySet()); assertTrue(tags.contains(tag)); assertEquals(1, tags.size()); 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 index 3b7d1a90084..0d037317d90 100644 --- 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 @@ -173,7 +173,7 @@ void testListUsers() throws IOException { UserMetaService userMetaService = UserMetaService.getInstance(); List actualUsers = userMetaService.listUsersByNamespace( - AuthorizationUtils.ofUserNamespace(metalakeName), Collections.emptyList()); + AuthorizationUtils.ofUserNamespace(metalakeName), Collections.emptySet()); actualUsers.sort(Comparator.comparing(UserEntity::name)); List expectUsers = Lists.newArrayList(user1, user2); Assertions.assertEquals(expectUsers.size(), actualUsers.size()); From f246628753f2d3e13cb987f250db3d57f3ab5246 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 19:18:27 +0800 Subject: [PATCH 23/31] rename --- .../relational/mapper/UserMetaBaseSQLProvider.java | 2 +- .../storage/relational/mapper/UserMetaMapper.java | 6 +++--- .../relational/mapper/UserMetaSQLProviderFactory.java | 4 ++-- .../relational/mapper/h2/UserMetaH2Provider.java | 2 +- .../mapper/postgresql/UserMetaPostgreSQLProvider.java | 2 +- .../po/{CombinedUserPO.java => ExtendedUserPO.java} | 10 +++++----- .../storage/relational/service/UserMetaService.java | 8 ++++---- .../storage/relational/utils/POConverters.java | 8 ++++---- 8 files changed, 21 insertions(+), 21 deletions(-) rename core/src/main/java/org/apache/gravitino/storage/relational/po/{CombinedUserPO.java => ExtendedUserPO.java} (85%) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java index 4f5383dc028..d3a49623d7d 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaBaseSQLProvider.java @@ -154,7 +154,7 @@ public String listUserPOsByMetalake(@Param("metalakeName") String metalakeName) + " AND ut.deleted_at = 0 AND mt.deleted_at = 0"; } - public String listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { + public String listExtendedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { return "SELECT ut.user_id as userId, ut.user_name as userName," + " ut.metalake_id as metalakeId," + " ut.audit_info as auditInfo," 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 index 47077e83b7f..19bbb4eddd2 100644 --- 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 @@ -20,7 +20,7 @@ package org.apache.gravitino.storage.relational.mapper; import java.util.List; -import org.apache.gravitino.storage.relational.po.CombinedUserPO; +import org.apache.gravitino.storage.relational.po.ExtendedUserPO; import org.apache.gravitino.storage.relational.po.UserPO; import org.apache.ibatis.annotations.DeleteProvider; import org.apache.ibatis.annotations.InsertProvider; @@ -60,8 +60,8 @@ UserPO selectUserMetaByMetalakeIdAndName( @SelectProvider( type = UserMetaSQLProviderFactory.class, - method = "listCombinedUserPOsByMetalakeId") - List listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId); + method = "listExtendedUserPOsByMetalakeId") + List listExtendedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId); @InsertProvider( type = UserMetaSQLProviderFactory.class, diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java index b58c43291d3..9d6658bd7b0 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/UserMetaSQLProviderFactory.java @@ -88,8 +88,8 @@ public static String listUserPOsByMetalake(@Param("metalakeName") String metalak return getProvider().listUserPOsByMetalake(metalakeName); } - public static String listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { - return getProvider().listCombinedUserPOsByMetalakeId(metalakeId); + public static String listExtendedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { + return getProvider().listExtendedUserPOsByMetalakeId(metalakeId); } public static String deleteUserMetasByLegacyTimeline( diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/h2/UserMetaH2Provider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/h2/UserMetaH2Provider.java index d4d4621bc08..12779d2d70b 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/h2/UserMetaH2Provider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/h2/UserMetaH2Provider.java @@ -27,7 +27,7 @@ public class UserMetaH2Provider extends UserMetaBaseSQLProvider { @Override - public String listCombinedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { + public String listExtendedUserPOsByMetalakeId(@Param("metalakeId") Long metalakeId) { return "SELECT ut.user_id as userId, ut.user_name as userName," + " ut.metalake_id as metalakeId," + " ut.audit_info as auditInfo," diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/postgresql/UserMetaPostgreSQLProvider.java b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/postgresql/UserMetaPostgreSQLProvider.java index 77689bafdca..846880943ea 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/mapper/postgresql/UserMetaPostgreSQLProvider.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/mapper/postgresql/UserMetaPostgreSQLProvider.java @@ -70,7 +70,7 @@ public String insertUserMetaOnDuplicateKeyUpdate(UserPO userPO) { } @Override - public String listCombinedUserPOsByMetalakeId(Long metalakeId) { + public String listExtendedUserPOsByMetalakeId(Long metalakeId) { return "SELECT ut.user_id as userId, ut.user_name as userName," + " ut.metalake_id as metalakeId," + " ut.audit_info as auditInfo," diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java b/core/src/main/java/org/apache/gravitino/storage/relational/po/ExtendedUserPO.java similarity index 85% rename from core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java rename to core/src/main/java/org/apache/gravitino/storage/relational/po/ExtendedUserPO.java index 91732540d16..a0488868cda 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/po/CombinedUserPO.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/po/ExtendedUserPO.java @@ -25,7 +25,7 @@ * data from multiple joined tables. The PO won't be written to database. So we don't need the inner * class Builder. */ -public class CombinedUserPO extends UserPO { +public class ExtendedUserPO extends UserPO { private String roleNames; private String roleIds; @@ -43,14 +43,14 @@ public boolean equals(Object o) { if (this == o) { return true; } - if (!(o instanceof CombinedUserPO)) { + if (!(o instanceof ExtendedUserPO)) { return false; } - CombinedUserPO combinedUserPO = (CombinedUserPO) o; + ExtendedUserPO extendedUserPO = (ExtendedUserPO) o; return super.equals(o) - && Objects.equal(getRoleIds(), combinedUserPO.getRoleIds()) - && Objects.equal(getRoleNames(), combinedUserPO.getRoleNames()); + && Objects.equal(getRoleIds(), extendedUserPO.getRoleIds()) + && Objects.equal(getRoleNames(), extendedUserPO.getRoleNames()); } @Override 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 index 4ef50c7a9f1..0387676d883 100644 --- 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 @@ -42,7 +42,7 @@ import org.apache.gravitino.storage.relational.mapper.OwnerMetaMapper; import org.apache.gravitino.storage.relational.mapper.UserMetaMapper; import org.apache.gravitino.storage.relational.mapper.UserRoleRelMapper; -import org.apache.gravitino.storage.relational.po.CombinedUserPO; +import org.apache.gravitino.storage.relational.po.ExtendedUserPO; import org.apache.gravitino.storage.relational.po.RolePO; import org.apache.gravitino.storage.relational.po.UserPO; import org.apache.gravitino.storage.relational.po.UserRoleRelPO; @@ -326,13 +326,13 @@ public List supportsSkippingFields() { @Override public List execute() { Long metalakeId = MetalakeMetaService.getInstance().getMetalakeIdByName(metalakeName); - List userPOs = + List userPOs = SessionUtils.getWithoutCommit( - UserMetaMapper.class, mapper -> mapper.listCombinedUserPOsByMetalakeId(metalakeId)); + UserMetaMapper.class, mapper -> mapper.listExtendedUserPOsByMetalakeId(metalakeId)); return userPOs.stream() .map( po -> - POConverters.fromCombinedUserPO( + POConverters.fromExtendedUserPO( po, AuthorizationUtils.ofUserNamespace(metalakeName))) .collect(Collectors.toList()); } diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java index 6b1637e0c85..da1f3d06a3b 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java @@ -49,7 +49,7 @@ import org.apache.gravitino.meta.TopicEntity; import org.apache.gravitino.meta.UserEntity; import org.apache.gravitino.storage.relational.po.CatalogPO; -import org.apache.gravitino.storage.relational.po.CombinedUserPO; +import org.apache.gravitino.storage.relational.po.ExtendedUserPO; import org.apache.gravitino.storage.relational.po.FilesetPO; import org.apache.gravitino.storage.relational.po.FilesetVersionPO; import org.apache.gravitino.storage.relational.po.GroupPO; @@ -731,13 +731,13 @@ public static UserEntity fromUserPO(UserPO userPO, List rolePOs, Namespa } /** - * Convert {@link CombinedUserPO} to {@link UserEntity} + * Convert {@link ExtendedUserPO} to {@link UserEntity} * * @param userPO CombinedUserPo object to be converted * @param namespace Namespace object to be associated with the user - * @return UserEntity object from CombinedUserPO object + * @return UserEntity object from ExtendedUserPO object */ - public static UserEntity fromCombinedUserPO(CombinedUserPO userPO, Namespace namespace) { + public static UserEntity fromExtendedUserPO(ExtendedUserPO userPO, Namespace namespace) { try { UserEntity.Builder builder = UserEntity.builder() From 51f6b00399c4ecdceb9e96b1164c1733282d16c9 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 20:37:35 +0800 Subject: [PATCH 24/31] Remove double deny --- .../java/org/apache/gravitino/EntityStore.java | 2 +- .../org/apache/gravitino/meta/UserEntity.java | 18 ++++++++++++++++++ .../service/SupportsSkippingFields.java | 2 +- .../SupportsSkippingFieldsHandlers.java | 15 ++++++++++++--- .../relational/service/UserMetaService.java | 9 +++++---- 5 files changed, 37 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/EntityStore.java b/core/src/main/java/org/apache/gravitino/EntityStore.java index 4967e21ce78..dcb27f022a7 100644 --- a/core/src/main/java/org/apache/gravitino/EntityStore.java +++ b/core/src/main/java/org/apache/gravitino/EntityStore.java @@ -81,7 +81,7 @@ default List list( * @param type the detailed type of the entity * @param entityType the general type of the entity * @param skippingFields Some fields may have a relatively high acquisition cost, EntityStore - * provide an optional setting to avoid fetching these high-cost fields to improve the + * provides an optional setting to avoid fetching these high-cost fields to improve the * performance. * @return the list of entities * @throws IOException if the list operation fails diff --git a/core/src/main/java/org/apache/gravitino/meta/UserEntity.java b/core/src/main/java/org/apache/gravitino/meta/UserEntity.java index c71d731a99e..eefaacab920 100644 --- a/core/src/main/java/org/apache/gravitino/meta/UserEntity.java +++ b/core/src/main/java/org/apache/gravitino/meta/UserEntity.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import lombok.ToString; import org.apache.gravitino.Auditable; import org.apache.gravitino.Entity; @@ -30,6 +31,7 @@ import org.apache.gravitino.HasIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.authorization.User; +import org.glassfish.jersey.internal.guava.Sets; /** A class representing a user metadata entity in Apache Gravitino. */ @ToString @@ -154,6 +156,22 @@ public List roleIds() { return roleIds; } + /** + * Get the set of all the fields. + * + * @return The set of all the fields. + */ + public static Set fieldSet() { + Set fields = Sets.newHashSet(); + fields.add(ID); + fields.add(NAME); + fields.add(AUDIT_INFO); + fields.add(ROLE_IDS); + fields.add(ROLE_NAMES); + + return fields; + } + @Override public boolean equals(Object o) { if (this == o) return true; diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java index df968589a1b..cd284fcabf0 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java @@ -29,7 +29,7 @@ interface SupportsSkippingFields { * * @return The fields which could be skipped. */ - List supportsSkippingFields(); + List allSkippingFields(); /** * The return value of the handler. diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java index f3d89eef3b7..364c8f16e3b 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java @@ -24,7 +24,7 @@ import org.apache.gravitino.Field; /** - * This class is the collection wrapper of SupportsSkippingFields handler. The class will contains + * This class is the collection wrapper of SupportsSkippingFields handler. The class will contain * all the handlers can proceed the data. We can choose different handlers according to the skipping * fields to acquire better performance. * @@ -38,9 +38,18 @@ void addHandler(SupportsSkippingFields supportsSkippingFields) { methods.add(supportsSkippingFields); } - T execute(Set skippingFields) { + T execute(Set necessaryFields) { for (SupportsSkippingFields method : methods) { - if (skippingFields.containsAll(method.supportsSkippingFields())) { + boolean hasAllNecessaryFields = true; + + for (Field skipField : method.allSkippingFields()) { + if (necessaryFields.contains(skipField)) { + hasAllNecessaryFields = false; + break; + } + } + + if (hasAllNecessaryFields) { return method.execute(); } } 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 index 0387676d883..08bf85f78cb 100644 --- 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 @@ -23,7 +23,6 @@ import com.google.common.collect.Sets; import java.io.IOException; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -259,7 +258,9 @@ public List listUsersByNamespace(Namespace namespace, Set ski handlers.addHandler(new ListSkippingRolesHandler(metalakeName)); handlers.addHandler(new ListAllFieldsHandler(metalakeName)); - return handlers.execute(new HashSet<>(skippingFields)); + Set necessaryFields = UserEntity.fieldSet(); + necessaryFields.removeAll(skippingFields); + return handlers.execute(necessaryFields); } public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { @@ -291,7 +292,7 @@ private static class ListSkippingRolesHandler } @Override - public List supportsSkippingFields() { + public List allSkippingFields() { return Lists.newArrayList(UserEntity.ROLE_IDS, UserEntity.ROLE_NAMES); } @@ -319,7 +320,7 @@ private static class ListAllFieldsHandler implements SupportsSkippingFields supportsSkippingFields() { + public List allSkippingFields() { return Collections.emptyList(); } From 50170d1bc28a047c05b8058340efa06bd2f4ce5f Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 20:51:31 +0800 Subject: [PATCH 25/31] fix --- .../service/SupportsSkippingFields.java | 8 ++++---- .../SupportsSkippingFieldsHandlers.java | 12 ++---------- .../relational/service/UserMetaService.java | 18 +++++++++++------- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java index cd284fcabf0..0bb91834f90 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java @@ -18,18 +18,18 @@ */ package org.apache.gravitino.storage.relational.service; -import java.util.List; +import java.util.Set; import org.apache.gravitino.Field; /** The handler supports to skip fields. */ interface SupportsSkippingFields { /** - * The fields which could be skipped. + * The fields which could be required. * - * @return The fields which could be skipped. + * @return The fields which are required. */ - List allSkippingFields(); + Set requiredFields(); /** * The return value of the handler. diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java index 364c8f16e3b..bf103b8f516 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java @@ -38,18 +38,10 @@ void addHandler(SupportsSkippingFields supportsSkippingFields) { methods.add(supportsSkippingFields); } - T execute(Set necessaryFields) { + T execute(Set reuiredFields) { for (SupportsSkippingFields method : methods) { - boolean hasAllNecessaryFields = true; - for (Field skipField : method.allSkippingFields()) { - if (necessaryFields.contains(skipField)) { - hasAllNecessaryFields = false; - break; - } - } - - if (hasAllNecessaryFields) { + if (reuiredFields.containsAll(method.requiredFields())) { return method.execute(); } } 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 index 08bf85f78cb..e0a5584a476 100644 --- 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 @@ -258,9 +258,9 @@ public List listUsersByNamespace(Namespace namespace, Set ski handlers.addHandler(new ListSkippingRolesHandler(metalakeName)); handlers.addHandler(new ListAllFieldsHandler(metalakeName)); - Set necessaryFields = UserEntity.fieldSet(); - necessaryFields.removeAll(skippingFields); - return handlers.execute(necessaryFields); + Set requiredFields = Sets.newHashSet(UserEntity.fieldSet()); + requiredFields.removeAll(skippingFields); + return handlers.execute(requiredFields); } public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { @@ -292,8 +292,12 @@ private static class ListSkippingRolesHandler } @Override - public List allSkippingFields() { - return Lists.newArrayList(UserEntity.ROLE_IDS, UserEntity.ROLE_NAMES); + public Set requiredFields() { + Set requiredFields = Sets.newHashSet(UserEntity.fieldSet()); + requiredFields.remove(UserEntity.ROLE_IDS); + requiredFields.remove(UserEntity.ROLE_NAMES); + + return requiredFields; } @Override @@ -320,8 +324,8 @@ private static class ListAllFieldsHandler implements SupportsSkippingFields allSkippingFields() { - return Collections.emptyList(); + public Set requiredFields() { + return UserEntity.fieldSet(); } @Override From 162b64af78ae88eca8112490c505b34a25e537aa Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 20:52:08 +0800 Subject: [PATCH 26/31] fix --- .../relational/service/SupportsSkippingFieldsHandlers.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java index bf103b8f516..1c0ad47fccf 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java @@ -40,11 +40,11 @@ void addHandler(SupportsSkippingFields supportsSkippingFields) { T execute(Set reuiredFields) { for (SupportsSkippingFields method : methods) { - if (reuiredFields.containsAll(method.requiredFields())) { return method.execute(); } } + throw new IllegalArgumentException("Don't support skip fields"); } } From a3a7865055fdf67910a31de2747b5141ea6a3c06 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 20:55:52 +0800 Subject: [PATCH 27/31] rename --- .../relational/service/SupportsSkippingFields.java | 6 +++--- .../service/SupportsSkippingFieldsHandlers.java | 4 ++-- .../storage/relational/service/UserMetaService.java | 10 +++++----- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java index 0bb91834f90..1b67b89a8b5 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java @@ -25,11 +25,11 @@ interface SupportsSkippingFields { /** - * The fields which could be required. + * The fields which could be desired. * - * @return The fields which are required. + * @return The fields which are desired. */ - Set requiredFields(); + Set desiredFields(); /** * The return value of the handler. diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java index 1c0ad47fccf..e9827be7cac 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java @@ -38,9 +38,9 @@ void addHandler(SupportsSkippingFields supportsSkippingFields) { methods.add(supportsSkippingFields); } - T execute(Set reuiredFields) { + T execute(Set desiredFields) { for (SupportsSkippingFields method : methods) { - if (reuiredFields.containsAll(method.requiredFields())) { + if (desiredFields.containsAll(method.desiredFields())) { return method.execute(); } } 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 index e0a5584a476..8630fd5379f 100644 --- 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 @@ -258,9 +258,9 @@ public List listUsersByNamespace(Namespace namespace, Set ski handlers.addHandler(new ListSkippingRolesHandler(metalakeName)); handlers.addHandler(new ListAllFieldsHandler(metalakeName)); - Set requiredFields = Sets.newHashSet(UserEntity.fieldSet()); - requiredFields.removeAll(skippingFields); - return handlers.execute(requiredFields); + Set desiredFields = Sets.newHashSet(UserEntity.fieldSet()); + desiredFields.removeAll(skippingFields); + return handlers.execute(desiredFields); } public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { @@ -292,7 +292,7 @@ private static class ListSkippingRolesHandler } @Override - public Set requiredFields() { + public Set desiredFields() { Set requiredFields = Sets.newHashSet(UserEntity.fieldSet()); requiredFields.remove(UserEntity.ROLE_IDS); requiredFields.remove(UserEntity.ROLE_NAMES); @@ -324,7 +324,7 @@ private static class ListAllFieldsHandler implements SupportsSkippingFields requiredFields() { + public Set desiredFields() { return UserEntity.fieldSet(); } From 39e100d81096121d742ef1d6bf1c5dc1c8922dcd Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 21:02:31 +0800 Subject: [PATCH 28/31] rename --- ...ppingFields.java => SupportsDesiredFields.java} | 4 ++-- ...ers.java => SupportsDesiredFieldsHandlers.java} | 12 ++++++------ .../relational/service/UserMetaService.java | 14 +++++++------- 3 files changed, 15 insertions(+), 15 deletions(-) rename core/src/main/java/org/apache/gravitino/storage/relational/service/{SupportsSkippingFields.java => SupportsDesiredFields.java} (91%) rename core/src/main/java/org/apache/gravitino/storage/relational/service/{SupportsSkippingFieldsHandlers.java => SupportsDesiredFieldsHandlers.java} (79%) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFields.java similarity index 91% rename from core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java rename to core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFields.java index 1b67b89a8b5..42978fa5013 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFields.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFields.java @@ -21,8 +21,8 @@ import java.util.Set; import org.apache.gravitino.Field; -/** The handler supports to skip fields. */ -interface SupportsSkippingFields { +/** The handler supports to skip fields to acquire part desired fields. */ +interface SupportsDesiredFields { /** * The fields which could be desired. diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFieldsHandlers.java similarity index 79% rename from core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java rename to core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFieldsHandlers.java index e9827be7cac..14eff12c707 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsSkippingFieldsHandlers.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFieldsHandlers.java @@ -24,22 +24,22 @@ import org.apache.gravitino.Field; /** - * This class is the collection wrapper of SupportsSkippingFields handler. The class will contain - * all the handlers can proceed the data. We can choose different handlers according to the skipping + * This class is the collection wrapper of SupportsDesiredFields handler. The class will contain + * all the handlers can proceed the data. We can choose different handlers according to the desired * fields to acquire better performance. * * @param The value type which the handler will return. */ -class SupportsSkippingFieldsHandlers { - private final List> methods = Lists.newArrayList(); +class SupportsDesiredFieldsHandlers { + private final List> methods = Lists.newArrayList(); // We should put the low-cost handler into the front of the list. - void addHandler(SupportsSkippingFields supportsSkippingFields) { + void addHandler(SupportsDesiredFields supportsSkippingFields) { methods.add(supportsSkippingFields); } T execute(Set desiredFields) { - for (SupportsSkippingFields method : methods) { + for (SupportsDesiredFields method : methods) { if (desiredFields.containsAll(method.desiredFields())) { return method.execute(); } 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 index 8630fd5379f..6769088e6d5 100644 --- 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 @@ -253,9 +253,9 @@ public List listUsersByNamespace(Namespace namespace, Set ski AuthorizationUtils.checkUserNamespace(namespace); String metalakeName = namespace.level(0); - SupportsSkippingFieldsHandlers> handlers = - new SupportsSkippingFieldsHandlers<>(); - handlers.addHandler(new ListSkippingRolesHandler(metalakeName)); + SupportsDesiredFieldsHandlers> handlers = + new SupportsDesiredFieldsHandlers<>(); + handlers.addHandler(new ListDesiredRolesHandler(metalakeName)); handlers.addHandler(new ListAllFieldsHandler(metalakeName)); Set desiredFields = Sets.newHashSet(UserEntity.fieldSet()); @@ -283,11 +283,11 @@ public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { return userDeletedCount[0] + userRoleRelDeletedCount[0]; } - private static class ListSkippingRolesHandler - implements SupportsSkippingFields> { + private static class ListDesiredRolesHandler + implements SupportsDesiredFields> { private final String metalakeName; - ListSkippingRolesHandler(String metalakeName) { + ListDesiredRolesHandler(String metalakeName) { this.metalakeName = metalakeName; } @@ -316,7 +316,7 @@ public List execute() { } } - private static class ListAllFieldsHandler implements SupportsSkippingFields> { + private static class ListAllFieldsHandler implements SupportsDesiredFields> { final String metalakeName; ListAllFieldsHandler(String metalakeName) { From 154a64be9dcd3c8c9ca7aedf3f3a3b4ba16005b2 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 21:18:32 +0800 Subject: [PATCH 29/31] rename --- .../relational/service/SupportsDesiredFieldsHandlers.java | 6 +++--- .../storage/relational/service/UserMetaService.java | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFieldsHandlers.java b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFieldsHandlers.java index 14eff12c707..b5b10a7b6aa 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFieldsHandlers.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/service/SupportsDesiredFieldsHandlers.java @@ -24,8 +24,8 @@ import org.apache.gravitino.Field; /** - * This class is the collection wrapper of SupportsDesiredFields handler. The class will contain - * all the handlers can proceed the data. We can choose different handlers according to the desired + * This class is the collection wrapper of SupportsDesiredFields handler. The class will contain all + * the handlers can proceed the data. We can choose different handlers according to the desired * fields to acquire better performance. * * @param The value type which the handler will return. @@ -40,7 +40,7 @@ void addHandler(SupportsDesiredFields supportsSkippingFields) { T execute(Set desiredFields) { for (SupportsDesiredFields method : methods) { - if (desiredFields.containsAll(method.desiredFields())) { + if (method.desiredFields().containsAll(desiredFields)) { return method.execute(); } } 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 index 6769088e6d5..f64b4ab405a 100644 --- 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 @@ -283,8 +283,7 @@ public int deleteUserMetasByLegacyTimeline(long legacyTimeline, int limit) { return userDeletedCount[0] + userRoleRelDeletedCount[0]; } - private static class ListDesiredRolesHandler - implements SupportsDesiredFields> { + private static class ListDesiredRolesHandler implements SupportsDesiredFields> { private final String metalakeName; ListDesiredRolesHandler(String metalakeName) { From c74bf559dfdacc692e4212df0d724d37c10e6238 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 22:54:30 +0800 Subject: [PATCH 30/31] modify comment --- .../apache/gravitino/storage/relational/po/ExtendedUserPO.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/gravitino/storage/relational/po/ExtendedUserPO.java b/core/src/main/java/org/apache/gravitino/storage/relational/po/ExtendedUserPO.java index a0488868cda..919056c4883 100644 --- a/core/src/main/java/org/apache/gravitino/storage/relational/po/ExtendedUserPO.java +++ b/core/src/main/java/org/apache/gravitino/storage/relational/po/ExtendedUserPO.java @@ -21,7 +21,7 @@ import com.google.common.base.Objects; /** - * CombinedUserPO add extra roleNames and roleIds for UserPO. This PO is only used for reading the + * ExtendedUserPO add extra roleNames and roleIds for UserPO. This PO is only used for reading the * data from multiple joined tables. The PO won't be written to database. So we don't need the inner * class Builder. */ From 457172b9f06d64f9339399cb0245f8a09cdda224 Mon Sep 17 00:00:00 2001 From: Rory Date: Mon, 23 Sep 2024 23:02:38 +0800 Subject: [PATCH 31/31] use unmodified collection --- core/src/main/java/org/apache/gravitino/meta/UserEntity.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/gravitino/meta/UserEntity.java b/core/src/main/java/org/apache/gravitino/meta/UserEntity.java index eefaacab920..df47215b4b5 100644 --- a/core/src/main/java/org/apache/gravitino/meta/UserEntity.java +++ b/core/src/main/java/org/apache/gravitino/meta/UserEntity.java @@ -169,7 +169,7 @@ public static Set fieldSet() { fields.add(ROLE_IDS); fields.add(ROLE_NAMES); - return fields; + return Collections.unmodifiableSet(fields); } @Override