This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new eb1e718a4c [#13299] improvement(authz): bound the metadata store
queries when listing models, model versions and job templates (#13300)
eb1e718a4c is described below
commit eb1e718a4c91208117360e85ce1120046879d98d
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.
---
.../server/authorization/MetadataAuthzHelper.java | 57 +++---
.../authorization/TestMetadataAuthzHelper.java | 192 +++++++++++++++++++--
.../gravitino/server/web/rest/ModelOperations.java | 54 +++---
.../server/web/rest/TestModelOperations.java | 72 ++++++++
4 files changed, 296 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 3599ffb54d..0499fed142 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";
@@ -136,6 +136,18 @@ public class MetadataAuthzHelper {
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))),
Entity.EntityType.SCHEMA,
Map.of(
AuthorizationExpressionConstants.FILTER_SCHEMA_AUTHORIZATION_EXPRESSION,
@@ -403,9 +415,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));
@@ -435,13 +446,6 @@ public class MetadataAuthzHelper {
nameIdentifiers.length);
}
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();
@@ -597,8 +601,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;
}
@@ -624,21 +630,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 c6f7b722e8..e04d6577ca 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");
