This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-eb1e718a-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 9fa83abe1817423b94d923501f7dc6a4d0473257 Author: Qi Yu <[email protected]> AuthorDate: Fri Sep 18 16:26:09 2026 +0800 [#13299] improvement(authz): bound the metadata store queries when listing models, model versions and job templates (#13300) ### What changes were proposed in this pull request? - `MetadataAuthzHelper`: remove `preloadOwner`, skip `preloadToCache` for entity types the entity cache does not keep, and register parent-scope list short-circuits for `MODEL` (`FILTER_MODEL_AUTHORIZATION_EXPRESSION`) and `JOB_TEMPLATE` (`LOAD_JOB_TEMPLATE_AUTHORIZATION_EXPRESSION`). - `ModelOperations.listModelVersions`: filter the whole version list in one `filterByExpression` call instead of one call per version. ### Why are the changes needed? `preloadOwner` resolved every listed identifier's id one by one (`OwnerMetaService.batchGetOwner` → `EntityIdService.getEntityId`), and for non-cacheable types (`MODEL`, `JOB_TEMPLATE`) each resolution is two store round trips in their own transactions. Its result has had no consumer since #12006 removed relation data from the entity cache. Listing 2,000 models ran 16,044 SQL statements; 500 job templates 4,064; 1,000 model versions 4,024 (one user lookup per version). After this change: 24, 16 and 20 statements; 5,000 models list in 0.13 s. Fix: #13299 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - New tests in `TestMetadataAuthzHelper` (model/job template short-circuit hit, deny fallback, no batch get for non-cacheable types, no owner preloading) and `TestModelOperations` (all versions filtered in one call, filter result honoured). - `./gradlew :server-common:test --tests TestMetadataAuthzHelper --tests TestPrincipalListQueryCount :server:test --tests TestModelOperations --tests TestJobOperations -PskipITs` - Manually against PostgreSQL 16 with `log_statement=all`, counting statements per list request before/after. # Conflicts: # server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java --- .../server/authorization/MetadataAuthzHelper.java | 69 ++++---- .../authorization/TestMetadataAuthzHelper.java | 192 +++++++++++++++++++-- .../gravitino/server/web/rest/ModelOperations.java | 54 +++--- .../server/web/rest/TestModelOperations.java | 72 ++++++++ 4 files changed, 308 insertions(+), 79 deletions(-) diff --git a/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java b/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java index 36e0f3d698..2c6f861f5f 100644 --- a/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java +++ b/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java @@ -35,16 +35,15 @@ import java.util.stream.Collectors; 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.MetadataObject; import org.apache.gravitino.Metalake; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; -import org.apache.gravitino.SupportsRelationOperations; import org.apache.gravitino.authorization.AuthorizationRequestContext; import org.apache.gravitino.authorization.GravitinoAuthorizer; import org.apache.gravitino.authorization.Privilege; +import org.apache.gravitino.cache.BaseEntityCache; import org.apache.gravitino.dto.tag.MetadataObjectDTO; import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants; import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionEvaluator; @@ -67,7 +66,9 @@ public class MetadataAuthzHelper { /** * Entity types that support batch get operations for cache preloading. These types have - * implemented the batchGetByIdentifier method in their respective MetaService classes. + * implemented the batchGetByIdentifier method in their respective MetaService classes and are + * cacheable (see {@link BaseEntityCache#isCacheable(Entity.EntityType)}); a batch get of a + * non-cacheable type such as MODEL or JOB_TEMPLATE would be discarded, so it is not issued. */ private static final List<Entity.EntityType> SUPPORTED_PRELOAD_ENTITY_TYPES = Arrays.asList( @@ -77,11 +78,9 @@ public class MetadataAuthzHelper { Entity.EntityType.TABLE, Entity.EntityType.FILESET, Entity.EntityType.TOPIC, - Entity.EntityType.MODEL, Entity.EntityType.TAG, Entity.EntityType.POLICY, - Entity.EntityType.JOB, - Entity.EntityType.JOB_TEMPLATE); + Entity.EntityType.JOB); /** * Topic and Table may be from the external system and the schema may not exist in Gravitino, so @@ -96,6 +95,7 @@ public class MetadataAuthzHelper { .collect(Collectors.toUnmodifiableSet()); private static final String TABLE_PARENT_SCOPES = "METALAKE, CATALOG, SCHEMA"; + private static final String MODEL_PARENT_SCOPES = "METALAKE, CATALOG, SCHEMA"; private static final String SCHEMA_PARENT_SCOPES = "METALAKE, CATALOG"; private static final String METALAKE_ONLY_SCOPE = "METALAKE"; private static final String CATALOG_PARENT_SCOPES = "METALAKE"; @@ -127,7 +127,31 @@ public class MetadataAuthzHelper { List.of( parentOwnerPath(TABLE_PARENT_SCOPES), parentPrivilegePath(Privilege.Name.SELECT_TABLE, TABLE_PARENT_SCOPES), +<<<<<<< HEAD parentPrivilegePath(Privilege.Name.MODIFY_TABLE, TABLE_PARENT_SCOPES))), +======= + parentPrivilegePath(Privilege.Name.MODIFY_TABLE, TABLE_PARENT_SCOPES)), + AuthorizationExpressionConstants.LIST_TABLE_LIKE_AUTHORIZATION_EXPRESSION, + List.of( + parentOwnerPath(TABLE_PARENT_SCOPES), + tableLikeParentPrivilegePath(Privilege.Name.PROBE_TABLE_LIKE), + tableLikeParentPrivilegePath(Privilege.Name.SELECT_TABLE), + tableLikeParentPrivilegePath(Privilege.Name.MODIFY_TABLE), + tableLikeParentPrivilegePath(Privilege.Name.CREATE_TABLE), + tableLikeParentPrivilegePath(Privilege.Name.CREATE_VIEW))), + Entity.EntityType.MODEL, + Map.of( + AuthorizationExpressionConstants.FILTER_MODEL_AUTHORIZATION_EXPRESSION, + List.of( + parentOwnerPath(MODEL_PARENT_SCOPES), + parentPrivilegePath(Privilege.Name.USE_MODEL, MODEL_PARENT_SCOPES))), + Entity.EntityType.JOB_TEMPLATE, + Map.of( + AuthorizationExpressionConstants.LOAD_JOB_TEMPLATE_AUTHORIZATION_EXPRESSION, + List.of( + parentOwnerPath(METALAKE_ONLY_SCOPE), + parentPrivilegePath(Privilege.Name.USE_JOB_TEMPLATE, METALAKE_ONLY_SCOPE))), +>>>>>>> eb1e718a4 ([#13299] improvement(authz): bound the metadata store queries when listing models, model versions and job templates (#13300)) Entity.EntityType.SCHEMA, Map.of( AuthorizationExpressionConstants.FILTER_SCHEMA_AUTHORIZATION_EXPRESSION, @@ -354,9 +378,8 @@ public class MetadataAuthzHelper { // per-object loop over every catalog in the metalake. NameIdentifier[] nameIdentifiers = Arrays.stream(entities).map(toNameIdentifier).toArray(NameIdentifier[]::new); - boolean isMetadataObject = METADATA_OBJECT_ENTITY_TYPES.contains(entityType); if (enableAuthorization() && nameIdentifiers.length > 0) { - if (isMetadataObject) { + if (METADATA_OBJECT_ENTITY_TYPES.contains(entityType)) { Arrays.stream(nameIdentifiers) .forEach( identifier -> NameIdentifierUtil.checkMetadataObjectName(identifier, entityType)); @@ -370,13 +393,6 @@ public class MetadataAuthzHelper { } } preloadToCache(entityType, nameIdentifiers); - // Ownership is defined on metadata objects, independently of the filter expression. - // Users/groups are not metadata objects. OwnerMetaService.batchGetOwner resolves IDs per - // identifier, so calling it for users/groups would still perform two SELECTs per entry - // before the batched owner-relation queries. - if (isMetadataObject) { - preloadOwner(entityType, nameIdentifiers); - } GravitinoAuthorizer authorizer = GravitinoAuthorizerProvider.getInstance().getGravitinoAuthorizer(); @@ -532,8 +548,10 @@ public class MetadataAuthzHelper { return; } - // Only preload entity types that support batch get operations - if (!SUPPORTED_PRELOAD_ENTITY_TYPES.contains(entityType)) { + // Only preload entity types that support batch get operations and that the entity cache + // keeps; the batch get result is otherwise dropped on the floor. + if (!SUPPORTED_PRELOAD_ENTITY_TYPES.contains(entityType) + || !BaseEntityCache.isCacheable(entityType)) { return; } @@ -559,21 +577,4 @@ public class MetadataAuthzHelper { entityType, EntityClassMapper.getEntityClass(entityType)); } - - private static void preloadOwner(Entity.EntityType entityType, NameIdentifier[] nameIdentifiers) { - if (!GravitinoEnv.getInstance().cacheEnabled()) { - return; - } - EntityStore entityStore = GravitinoEnv.getInstance().entityStore(); - try { - entityStore - .relationOperations() - .batchListEntitiesByRelation( - SupportsRelationOperations.Type.OWNER_REL, - Arrays.stream(nameIdentifiers).toList(), - entityType); - } catch (Exception e) { - LOG.warn("Ignore preloadOwner error:{}", e.getMessage(), e); - } - } } diff --git a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java index e1ba874a68..5b5d245a0a 100644 --- a/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java +++ b/server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java @@ -22,10 +22,12 @@ import static org.apache.gravitino.server.authorization.PrincipalListTestUtils.p import static org.apache.gravitino.server.authorization.PrincipalListTestUtils.principalManagementPrivilege; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anySet; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -36,6 +38,7 @@ import java.lang.reflect.Method; import java.util.Arrays; import java.util.Set; import java.util.concurrent.Executor; +import java.util.stream.IntStream; import org.apache.gravitino.Config; import org.apache.gravitino.Configs; import org.apache.gravitino.Entity; @@ -413,15 +416,117 @@ public class TestMetadataAuthzHelper { }); } - /** Roles still preload owners when no parent path authorizes the whole list. */ + /** + * Owner relations are never batch-loaded ahead of the per-object loop: the entity store no longer + * caches them, so the batch call was a discarded round trip that resolved every listed + * identifier's id one by one. + */ @Test - public void testRoleListFallbackPreloadsOwners() throws Exception { + public void testRoleListFallbackDoesNotPreloadOwners() { EntityStore store = mock(EntityStore.class); SupportsRelationOperations relations = mock(SupportsRelationOperations.class); when(gravitinoEnv.entityStore()).thenReturn(store); when(gravitinoEnv.cacheEnabled()).thenReturn(true); - when(store.relationOperations()).thenReturn(relations); + lenient().when(store.relationOperations()).thenReturn(relations); NameIdentifier[] identifiers = principalIdentifiers(Entity.EntityType.ROLE, 3); + try { + withAuthorizer( + mock(GravitinoAuthorizer.class), + () -> + Assertions.assertEquals( + 0, + MetadataAuthzHelper.filterByExpression( + "testMetalake", + principalListExpression(Entity.EntityType.ROLE), + Entity.EntityType.ROLE, + identifiers) + .length)); + verifyNoInteractions(relations); + } finally { + when(gravitinoEnv.cacheEnabled()).thenReturn(false); + when(gravitinoEnv.entityStore()).thenReturn(null); + } + } + + /** + * A USE_MODEL grant on the schema makes every model in it visible, so the list returns without + * touching the entity store or authorizing any single model. + */ + @Test + public void testListShortCircuitModelViaSchemaGrant() { + EntityStore store = mock(EntityStore.class); + when(gravitinoEnv.entityStore()).thenReturn(store); + when(gravitinoEnv.cacheEnabled()).thenReturn(true); + when(gravitinoEnv.internalAccessControlDispatcher()) + .thenReturn(mock(AccessControlDispatcher.class)); + GravitinoAuthorizer authorizer = + mockParentGrantAuthorizer(MetadataObject.Type.SCHEMA, Privilege.Name.USE_MODEL); + NameIdentifier[] models = models(2000); + try { + withAuthorizer( + authorizer, + () -> { + NameIdentifier[] filtered = + MetadataAuthzHelper.filterByExpression( + "testMetalake", + AuthorizationExpressionConstants.FILTER_MODEL_AUTHORIZATION_EXPRESSION, + Entity.EntityType.MODEL, + models); + Assertions.assertSame(models, filtered); + verify(authorizer, never()) + .authorize( + any(), + eq("testMetalake"), + argThat(object -> object.type() == MetadataObject.Type.MODEL), + any(), + any()); + verifyNoInteractions(store); + }); + } finally { + when(gravitinoEnv.cacheEnabled()).thenReturn(false); + when(gravitinoEnv.entityStore()).thenReturn(null); + when(gravitinoEnv.internalAccessControlDispatcher()).thenReturn(null); + } + } + + /** A possible USE_MODEL deny disables the schema-grant path and each model is checked. */ + @Test + public void testListShortCircuitModelFallsBackWhenDenyMayExist() { + GravitinoAuthorizer authorizer = + mockParentGrantAuthorizer(MetadataObject.Type.SCHEMA, Privilege.Name.USE_MODEL); + when(authorizer.hasDenyPolicy(any(), eq("testMetalake"), anySet(), any())).thenReturn(true); + when(authorizer.deny(any(), eq("testMetalake"), any(), any(), any())) + .thenAnswer( + invocation -> { + MetadataObject object = invocation.getArgument(2); + return object.type() == MetadataObject.Type.MODEL && "m1".equals(object.name()); + }); + NameIdentifier[] models = models(3); + withAuthorizer( + authorizer, + () -> { + NameIdentifier[] filtered = + MetadataAuthzHelper.filterByExpression( + "testMetalake", + AuthorizationExpressionConstants.FILTER_MODEL_AUTHORIZATION_EXPRESSION, + Entity.EntityType.MODEL, + models); + Assertions.assertArrayEquals(new NameIdentifier[] {models[0], models[2]}, filtered); + }); + } + + /** + * Models are not cacheable, so even the per-object fallback never issues the batch get whose + * result the cache would drop. + */ + @Test + public void testModelListFallbackDoesNotBatchLoadEntities() { + EntityStore store = mock(EntityStore.class); + when(gravitinoEnv.entityStore()).thenReturn(store); + when(gravitinoEnv.cacheEnabled()).thenReturn(true); + when(gravitinoEnv.internalAccessControlDispatcher()) + .thenReturn(mock(AccessControlDispatcher.class)); + NameIdentifier[] models = models(3); try { withAuthorizer( mock(GravitinoAuthorizer.class), @@ -430,22 +535,87 @@ public class TestMetadataAuthzHelper { 0, MetadataAuthzHelper.filterByExpression( "testMetalake", - principalListExpression(Entity.EntityType.ROLE), - Entity.EntityType.ROLE, - identifiers) + AuthorizationExpressionConstants.FILTER_MODEL_AUTHORIZATION_EXPRESSION, + Entity.EntityType.MODEL, + models) .length); + verifyNoInteractions(store); + }); + } finally { + when(gravitinoEnv.cacheEnabled()).thenReturn(false); + when(gravitinoEnv.entityStore()).thenReturn(null); + when(gravitinoEnv.internalAccessControlDispatcher()).thenReturn(null); + } + } + + /** A USE_JOB_TEMPLATE grant on the metalake lists every job template with constant work. */ + @Test + public void testListShortCircuitJobTemplateViaMetalakeGrant() { + EntityStore store = mock(EntityStore.class); + when(gravitinoEnv.entityStore()).thenReturn(store); + when(gravitinoEnv.cacheEnabled()).thenReturn(true); + GravitinoAuthorizer authorizer = + mockParentGrantAuthorizer(MetadataObject.Type.METALAKE, Privilege.Name.USE_JOB_TEMPLATE); + NameIdentifier[] templates = + IntStream.range(0, 500) + .mapToObj(i -> NameIdentifierUtil.ofJobTemplate("testMetalake", "tpl" + i)) + .toArray(NameIdentifier[]::new); + try { + withAuthorizer( + authorizer, + () -> { + NameIdentifier[] filtered = + MetadataAuthzHelper.filterByExpression( + "testMetalake", + AuthorizationExpressionConstants.LOAD_JOB_TEMPLATE_AUTHORIZATION_EXPRESSION, + Entity.EntityType.JOB_TEMPLATE, + templates); + Assertions.assertSame(templates, filtered); + verify(authorizer, times(1)) + .authorize( + any(), eq("testMetalake"), any(), eq(Privilege.Name.USE_JOB_TEMPLATE), any()); + verifyNoInteractions(store); }); - verify(relations) - .batchListEntitiesByRelation( - SupportsRelationOperations.Type.OWNER_REL, - Arrays.asList(identifiers), - Entity.EntityType.ROLE); } finally { when(gravitinoEnv.cacheEnabled()).thenReturn(false); when(gravitinoEnv.entityStore()).thenReturn(null); } } + /** Without a metalake-scope grant, job templates are still filtered one by one. */ + @Test + public void testJobTemplateListNoParentGrantFallsBackToPerObject() { + GravitinoAuthorizer authorizer = mock(GravitinoAuthorizer.class); + NameIdentifier[] templates = + new NameIdentifier[] { + NameIdentifierUtil.ofJobTemplate("testMetalake", "tpl1"), + NameIdentifierUtil.ofJobTemplate("testMetalake", "tpl2") + }; + when(authorizer.isOwner(any(), eq("testMetalake"), any(), any())) + .thenAnswer( + invocation -> { + MetadataObject object = invocation.getArgument(2); + return object.type() == MetadataObject.Type.JOB_TEMPLATE + && "tpl2".equals(object.name()); + }); + withAuthorizer( + authorizer, + () -> + Assertions.assertArrayEquals( + new NameIdentifier[] {templates[1]}, + MetadataAuthzHelper.filterByExpression( + "testMetalake", + AuthorizationExpressionConstants.LOAD_JOB_TEMPLATE_AUTHORIZATION_EXPRESSION, + Entity.EntityType.JOB_TEMPLATE, + templates))); + } + + private static NameIdentifier[] models(int count) { + return IntStream.range(0, count) + .mapToObj(i -> NameIdentifierUtil.ofModel("testMetalake", "testCatalog", "s1", "m" + i)) + .toArray(NameIdentifier[]::new); + } + /** A role expression without MANAGE_GRANTS must not inherit that list shortcut. */ @Test public void testDifferentRoleExpressionDoesNotUseManagementGrant() { diff --git a/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java b/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java index 446bdf5545..5f626d75b6 100644 --- a/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java +++ b/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java @@ -266,52 +266,38 @@ public class ModelOperations { return Utils.doAs( httpRequest, () -> { + // All versions are filtered in one call so the authorization state loaded for the + // request (user, roles, model id, owner) is resolved once instead of once per version. if (verbose) { ModelVersion[] modelVersions = modelDispatcher.listModelVersionInfos(modelId); modelVersions = modelVersions == null ? new ModelVersion[0] : modelVersions; modelVersions = - Arrays.stream(modelVersions) - .filter( - modelVersion -> { - NameIdentifier[] nameIdentifiers = - new NameIdentifier[] { - NameIdentifierUtil.ofModelVersion( - metalake, catalog, schema, model, modelVersion.version()) - }; - return MetadataAuthzHelper.filterByExpression( - metalake, - AuthorizationExpressionConstants - .LOAD_MODEL_AUTHORIZATION_EXPRESSION, - Entity.EntityType.MODEL_VERSION, - nameIdentifiers) - .length - > 0; - }) - .toArray(ModelVersion[]::new); + MetadataAuthzHelper.filterByExpression( + metalake, + AuthorizationExpressionConstants.LOAD_MODEL_AUTHORIZATION_EXPRESSION, + Entity.EntityType.MODEL_VERSION, + modelVersions, + modelVersion -> + NameIdentifierUtil.ofModelVersion( + metalake, catalog, schema, model, modelVersion.version())); LOG.info("List {} versions of model {}", modelVersions.length, modelId); return Utils.ok( new ModelVersionInfoListResponse(DTOConverters.toDTOs(modelVersions))); } else { int[] versions = modelDispatcher.listModelVersions(modelId); versions = versions == null ? new int[0] : versions; + Integer[] boxedVersions = Arrays.stream(versions).boxed().toArray(Integer[]::new); versions = - Arrays.stream(versions) - .filter( - modelVersion -> { - NameIdentifier[] nameIdentifiers = - new NameIdentifier[] { + Arrays.stream( + MetadataAuthzHelper.filterByExpression( + metalake, + AuthorizationExpressionConstants.LOAD_MODEL_AUTHORIZATION_EXPRESSION, + Entity.EntityType.MODEL_VERSION, + boxedVersions, + version -> NameIdentifierUtil.ofModelVersion( - metalake, catalog, schema, model, modelVersion) - }; - return MetadataAuthzHelper.filterByExpression( - metalake, - AuthorizationExpressionConstants - .LOAD_MODEL_AUTHORIZATION_EXPRESSION, - Entity.EntityType.MODEL_VERSION, - nameIdentifiers) - .length - > 0; - }) + metalake, catalog, schema, model, version))) + .mapToInt(Integer::intValue) .toArray(); LOG.info("List {} versions of model {}", versions.length, modelId); return Utils.ok(new ModelVersionListResponse(versions)); diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestModelOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestModelOperations.java index e69ec2a0b2..0eb656f118 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestModelOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestModelOperations.java @@ -28,6 +28,7 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableMap; import java.io.IOException; import java.time.Instant; +import java.util.Arrays; import java.util.Collections; import java.util.Map; import javax.servlet.http.HttpServletRequest; @@ -37,6 +38,7 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.gravitino.Config; +import org.apache.gravitino.Entity.EntityType; import org.apache.gravitino.GravitinoEnv; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; @@ -67,6 +69,8 @@ import org.apache.gravitino.model.ModelChange; import org.apache.gravitino.model.ModelVersion; import org.apache.gravitino.model.ModelVersionChange; import org.apache.gravitino.rest.RESTUtils; +import org.apache.gravitino.server.authorization.MetadataAuthzHelper; +import org.apache.gravitino.server.authorization.expression.AuthorizationExpressionConstants; import org.apache.gravitino.utils.NameIdentifierUtil; import org.apache.gravitino.utils.NamespaceUtil; import org.glassfish.jersey.internal.inject.AbstractBinder; @@ -75,6 +79,7 @@ import org.glassfish.jersey.test.TestProperties; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.mockito.MockedStatic; import org.mockito.Mockito; public class TestModelOperations extends BaseOperationsTest { @@ -538,6 +543,73 @@ public class TestModelOperations extends BaseOperationsTest { Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResp1.getType()); } + /** + * Every version of a model is authorized by the same model-level expression, so the list is + * filtered in one call. Filtering each version separately built a fresh authorization context per + * version and reloaded the caller's user record once per version. + */ + @Test + public void testListModelVersionsFiltersAllVersionsInOneCall() throws IllegalAccessException { + NameIdentifier modelId = NameIdentifierUtil.ofModel(metalake, catalog, schema, "model1"); + when(modelDispatcher.listModelVersions(modelId)).thenReturn(new int[] {0, 1, 2}); + ModelVersion[] versionInfos = + new ModelVersion[] { + mockModelVersion(0, ImmutableMap.of("n0", "u0"), new String[0], "c0"), + mockModelVersion(1, ImmutableMap.of("n1", "u1"), new String[0], "c1"), + mockModelVersion(2, ImmutableMap.of("n2", "u2"), new String[0], "c2") + }; + when(modelDispatcher.listModelVersionInfos(modelId)).thenReturn(versionInfos); + ModelOperations modelOperations = new ModelOperations(modelDispatcher); + FieldUtils.writeField(modelOperations, "httpRequest", mock(HttpServletRequest.class), true); + + try (MockedStatic<MetadataAuthzHelper> metadataAuthzHelper = + Mockito.mockStatic(MetadataAuthzHelper.class)) { + // The authorizer keeps the first and last element of whatever list it is handed. + metadataAuthzHelper + .when( + () -> + MetadataAuthzHelper.filterByExpression( + Mockito.eq(metalake), + Mockito.eq( + AuthorizationExpressionConstants.LOAD_MODEL_AUTHORIZATION_EXPRESSION), + Mockito.eq(EntityType.MODEL_VERSION), + Mockito.any(Object[].class), + Mockito.any())) + .thenAnswer( + invocation -> { + Object[] entities = invocation.getArgument(3); + Object[] kept = Arrays.copyOf(entities, 2); + kept[1] = entities[entities.length - 1]; + return kept; + }); + + Response resp = modelOperations.listModelVersions(metalake, catalog, schema, "model1", false); + Assertions.assertEquals(Response.Status.OK.getStatusCode(), resp.getStatus()); + Assertions.assertArrayEquals( + new int[] {0, 2}, ((ModelVersionListResponse) resp.getEntity()).getVersions()); + + Response verboseResp = + modelOperations.listModelVersions(metalake, catalog, schema, "model1", true); + Assertions.assertEquals(Response.Status.OK.getStatusCode(), verboseResp.getStatus()); + ModelVersionDTO[] kept = + ((ModelVersionInfoListResponse) verboseResp.getEntity()).getVersions(); + Assertions.assertEquals(2, kept.length); + Assertions.assertEquals(0, kept[0].version()); + Assertions.assertEquals(2, kept[1].version()); + + // One filter call per request, each handed the whole version list. + metadataAuthzHelper.verify( + () -> + MetadataAuthzHelper.filterByExpression( + Mockito.eq(metalake), + Mockito.anyString(), + Mockito.eq(EntityType.MODEL_VERSION), + Mockito.argThat((Object[] entities) -> entities.length == 3), + Mockito.any()), + Mockito.times(2)); + } + } + @Test public void testListModelVersionInfos() { NameIdentifier modelId = NameIdentifierUtil.ofModel(metalake, catalog, schema, "model1");
