yuqi1129 commented on code in PR #13262: URL: https://github.com/apache/gravitino/pull/13262#discussion_r4036336247
########## server-common/src/test/java/org/apache/gravitino/server/authorization/TestPrincipalListQueryCount.java: ########## @@ -0,0 +1,262 @@ +/* + * 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.server.authorization; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyList; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.Statement; +import java.time.Instant; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.concurrent.Executor; +import org.apache.commons.lang3.reflect.FieldUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.Entity; +import org.apache.gravitino.EntityStore; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.HasIdentifier; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.Namespace; +import org.apache.gravitino.SupportsRelationOperations; +import org.apache.gravitino.UserPrincipal; +import org.apache.gravitino.authorization.AuthorizationUtils; +import org.apache.gravitino.authorization.GravitinoAuthorizer; +import org.apache.gravitino.authorization.Privilege; +import org.apache.gravitino.json.JsonUtils; +import org.apache.gravitino.meta.AuditInfo; +import org.apache.gravitino.meta.EntityIdResolver; +import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants; +import org.apache.gravitino.storage.relational.JDBCBackend; +import org.apache.gravitino.storage.relational.RelationalEntityStoreIdResolver; +import org.apache.gravitino.storage.relational.service.EntityIdService; +import org.apache.gravitino.utils.PrincipalUtils; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.MockedStatic; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Counts real database queries in list loading and filtering, independently of list size. */ +class TestPrincipalListQueryCount { + private static final Logger LOG = LoggerFactory.getLogger(TestPrincipalListQueryCount.class); + + @TempDir Path tempDir; + + @Test + void testManagementListsHaveBoundedQueries() throws Exception { + Config config = new Config(false) {}; + config.set( + Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL, + "jdbc:h2:file:" + tempDir.resolve("metadata") + ";MODE=MYSQL;AUTO_SERVER=FALSE"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER, "org.h2.Driver"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_USER, "root"); + config.set(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_PASSWORD, "test"); + config.set(Configs.ENABLE_AUTHORIZATION, true); + EntityIdResolver previousResolver = + (EntityIdResolver) + FieldUtils.readStaticField(EntityIdService.class, "entityIdResolver", true); + Object previousExecutor = + FieldUtils.readStaticField(MetadataAuthzHelper.class, "executor", true); + List<Executable> queryCountAssertions = new ArrayList<>(); + try (MockedStatic<GravitinoEnv> envStatic = mockStatic(GravitinoEnv.class); + MockedStatic<GravitinoAuthorizerProvider> providerStatic = + mockStatic(GravitinoAuthorizerProvider.class); + JDBCBackend backend = new JDBCBackend()) { + GravitinoEnv env = mock(GravitinoEnv.class); + envStatic.when(GravitinoEnv::getInstance).thenReturn(env); + when(env.config()).thenReturn(config); + when(env.cacheEnabled()).thenReturn(true); + EntityStore store = mock(EntityStore.class); + SupportsRelationOperations relations = mock(SupportsRelationOperations.class); + when(env.entityStore()).thenReturn(store); + when(store.relationOperations()).thenReturn(relations); + when(relations.batchListEntitiesByRelation( + eq(SupportsRelationOperations.Type.OWNER_REL), anyList(), any())) + .thenAnswer( + call -> + backend.batchListEntitiesByRelation( + call.getArgument(0), call.getArgument(1), call.getArgument(2))); + GravitinoAuthorizerProvider provider = mock(GravitinoAuthorizerProvider.class); + providerStatic.when(GravitinoAuthorizerProvider::getInstance).thenReturn(provider); + GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class); + when(provider.getGravitinoAuthorizer()).thenReturn(authorizer); + // Isolate the storage/list-filter path: the caller has a metalake management grant. + when(authorizer.authorize(any(), any(), any(), any(), any())) Review Comment: Addressed in adf6cc00fa. The H2 test now covers both management grants and user/group self-filtering with no parent grant, at 1/1,003/10,000 entries and for both list representations. The latter reaches the preload stage and checks the filtered result size as well as SELECT counts. I also mutation-tested this: removing only the owner-preload guard makes the fallback assertions fail (1,003 entries: 2,009 name-list SELECTs and 2,010 detail-list SELECTs instead of 1/2), while the management case still passes. The guard is restored. The description now explicitly separates the two optimizations and limits query-count claims to the mocked-authorizer storage/filter path. ########## server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java: ########## @@ -108,6 +108,24 @@ public class MetadataAuthzHelper { private static final Map<Entity.EntityType, Map<String, List<ParentScopeAccessPath>>> LIST_SHORT_CIRCUITS = Map.of( + Entity.EntityType.USER, Review Comment: Addressed in adf6cc00fa and clarified the title/description. The parent shortcut benefits qualifying metalake owners/managers; ordinary filtering remains O(N), and ordinary role-membership ID lookups are out of scope. Ordinary user/group callers do still benefit from skipping unnecessary owner-ID resolution, which the new fallback H2 case measures. MetadataObjectRoleOperations intentionally retains its different expression: replacing it with LOAD_ROLE would introduce MANAGE_GRANTS visibility. Added a regression proving that a management grant does not short-circuit that expression. Exact-expression matching remains conservative; optimizing that endpoint would require its own sufficient paths. Also extracted principalListPaths(expression, managementPrivilege) for the three registry entries. ########## server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java: ########## @@ -597,7 +615,10 @@ private static void preloadToCache( } private static void preloadOwner(Entity.EntityType entityType, NameIdentifier[] nameIdentifiers) { - if (!GravitinoEnv.getInstance().cacheEnabled()) { + // Only metadata objects can have owners. Resolving every user/group ID here adds two store + // lookups per entry even though their visibility expressions never consult an object owner. + if (!METADATA_OBJECT_ENTITY_TYPES.contains(entityType) Review Comment: Addressed the mapping and duplicated checks in adf6cc00fa: the set now uses MetadataObjectUtil.toEntityType, backed by its authoritative map, and a single isMetadataObject value gates both name validation and owner preloading. The guard intentionally follows the ownership data model rather than parsing expression tokens: USER/GROUP are not metadata-object types, so adding USER::OWNER/GROUP::OWNER alone would not make ownership supported. A future ownership feature must extend that model/mapping too. The code comment now states this contract; a positive regression also verifies that ROLE still preloads owners on the fallback path. ########## server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java: ########## @@ -108,6 +108,24 @@ public class MetadataAuthzHelper { private static final Map<Entity.EntityType, Map<String, List<ParentScopeAccessPath>>> LIST_SHORT_CIRCUITS = Map.of( + Entity.EntityType.USER, + Map.of( + AuthorizationExpressionConstants.LOAD_USER_AUTHORIZATION_EXPRESSION, + List.of( + parentOwnerPath(CATALOG_PARENT_SCOPES), Review Comment: Addressed in adf6cc00fa. Principal paths now use a dedicated METALAKE_ONLY_SCOPE through principalListPaths; they no longer depend on CATALOG_PARENT_SCOPES. ########## server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java: ########## @@ -597,7 +615,10 @@ private static void preloadToCache( } private static void preloadOwner(Entity.EntityType entityType, NameIdentifier[] nameIdentifiers) { - if (!GravitinoEnv.getInstance().cacheEnabled()) { + // Only metadata objects can have owners. Resolving every user/group ID here adds two store Review Comment: The batch API hides per-identifier SQL here: JDBCBackend.batchListEntitiesByRelation delegates OWNER_REL to OwnerMetaService.batchGetOwner, whose identifier stream calls EntityIdService.getEntityId for every entry. RelationalEntityStoreIdResolver then queries the metalake ID and user/group ID separately, before the batched owner-relation queries. I clarified that call chain in the comment and added a real-H2 fallback regression in adf6cc00fa. Removing the guard experimentally produces 2,009/2,010 SELECTs for 1,003 user/group name/detail entries, versus 1/2 with the guard. The previous management-only test returned before this work and therefore could not establish its cost. Rewording it as two batched round-trips would be inaccurate. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
