jerryshao commented on code in PR #13262:
URL: https://github.com/apache/gravitino/pull/13262#discussion_r4035771852
##########
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:
**conventions**: `CATALOG_PARENT_SCOPES` (originally meaning "scopes above a
catalog") is reused verbatim as the parent scope for the new USER/GROUP/ROLE
entries here and below, purely because both happen to resolve to the same
string (`"METALAKE"`) today. If Gravitino ever introduces an intermediate scope
between metalake and catalog and a maintainer updates this constant to reflect
catalog's new parent chain, the USER/GROUP/ROLE short-circuits — which have
nothing to do with catalogs — would silently change behavior too, since neither
the map entries nor the constant's name signal this incidental coupling. Worth
a distinctly-named constant (e.g. `METALAKE_ONLY_SCOPE`) for the principal-type
entries.
##########
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:
**efficiency / altitude**: This short-circuit (and the perf win it enables)
only fires when the caller has a metalake-level grant (ownership or
`MANAGE_USERS`/`MANAGE_GROUPS`/`MANAGE_GRANTS`). For the common case — a
regular, non-admin caller relying on `USER::SELF`/`GROUP::SELF`/`ROLE::SELF`
visibility — there's no batched equivalent, so every such request still falls
through to the unchanged O(N) per-entity loop in `doFilter`, regardless of list
size. Worse, `isSelf(ROLE)` in `JcasbinAuthorizer.java` calls
`MetadataIdConverter.getID(...)` directly per role, bypassing the cache
`isOwner` uses — an uncached single-entity DB fetch per role. Not introduced by
this PR, but the title ("Avoid per-principal lookups during list filtering")
reads as a general fix when the actual benefit is narrower — worth calling out
in the PR description that this speeds up metalake-admin callers specifically,
not the common self/membership-visibility case.
---
**altitude**: Separately,
`server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectRoleOperations.java`
lists `ROLE` entities using its own local expression constant
(`"METALAKE::OWNER || ROLE::OWNER || ROLE::SELF"`), a different string than
`LOAD_ROLE_AUTHORIZATION_EXPRESSION` registered here. Since this map matches by
exact expression string, that endpoint never hits the new short-circuit and
always pays full per-object cost, unlike `RoleOperations.listRoles`. Not a
correctness bug, but worth confirming intentional — and it shows the registry's
exact-string-match design is fragile: any caller not using the registered
constant silently gets no benefit.
---
**reuse**: The three new entries (USER/GROUP/ROLE) are structurally
identical copy-paste (`Map.of(expr, List.of(parentOwnerPath(...),
parentPrivilegePath(...)))`), unlike the pre-existing TABLE entry which already
got a dedicated helper (`tableLikeParentPrivilegePath`) for a similar repeated
shape. A `principalListPaths(expression, managePrivilege)` helper would
collapse each block to a one-line call and prevent a future partial edit
(fixing the pattern for one type but forgetting the others) from silently
drifting.
##########
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()))
+ .thenAnswer(
+ call -> {
+ MetadataObject object = call.getArgument(2);
+ Privilege.Name privilege = call.getArgument(3);
+ return object.type() == MetadataObject.Type.METALAKE
+ && (privilege == Privilege.Name.MANAGE_USERS
+ || privilege == Privilege.Name.MANAGE_GROUPS
+ || privilege == Privilege.Name.MANAGE_GRANTS);
+ });
+ FieldUtils.writeStaticField(
+ MetadataAuthzHelper.class, "executor", (Executor) Runnable::run,
true);
+ backend.initialize(config);
+ EntityIdService.initialize(new RelationalEntityStoreIdResolver());
+ try (Connection connection =
+ DriverManager.getConnection(
+ config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL), "root",
"test")) {
+ for (int size : new int[] {1, 1003, 10000}) {
+ String metalake = "scale" + size;
+ insertPrincipals(connection, size, metalake);
+ for (Entity.EntityType type :
+ List.of(Entity.EntityType.USER, Entity.EntityType.GROUP,
Entity.EntityType.ROLE)) {
Review Comment:
**test-coverage**: This regression test is hand-scoped to exactly
USER/GROUP/ROLE via a hardcoded enumeration. If a later change extends
owner-preload skipping (or the `LIST_SHORT_CIRCUITS` registry) to another
entity type, or a regression reintroduces per-principal lookups for a type
outside this trio, nothing here catches it. Compounds the gap noted on line 114
— the test doesn't even reach the logic for the types it does enumerate.
##########
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:
**test-coverage (high)**: This mock makes `authorizer.authorize(...)` always
grant METALAKE-level `MANAGE_USERS`/`MANAGE_GROUPS`/`MANAGE_GRANTS`, so
`allVisibleViaParentScope` always returns `true` and `filterByExpression`
short-circuits and returns *before* `preloadToCache`/`preloadOwner` are ever
called. This elaborate H2-backed test (1/1003/10000 entries, asserting exact
SELECT counts) never actually reaches the `preloadOwner` skip logic this PR
adds.
The "query count independent of list size" claim is only proven for the
pre-existing parent-scope shortcut path (already correct before this PR). The
"2 SELECTs vs 1" difference it measures traces to an unrelated, pre-existing
cost in `UserMetaService.listUsersByNamespace` (an extra metalake-id lookup
when `details=true`), not to the owner-preload fix at all. Only the simpler
mock-based `testPrincipalListDoesNotPreloadOwners` in
`TestMetadataAuthzHelper.java` (via `verifyNoInteractions(store)`) actually
proves the skip branch is taken — but it doesn't measure real query counts at
scale. So the PR's actual performance claim for the owner-preload fix is
unverified by any test in this diff, and a future regression reintroducing
per-principal owner lookups for USER/GROUP would not be caught here.
##########
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:
**altitude**: This guard is keyed on `entityType` membership in
`METADATA_OBJECT_ENTITY_TYPES`, not on whether the specific authorization
expression being evaluated actually references `OWNER`. If a future
authorization expression for USER or GROUP ever gains a
`USER::OWNER`/`GROUP::OWNER` token, this stays silently disabled for that type
because the guard only inspects `entityType` — reintroducing the exact
per-principal N+1 lookups this PR was written to eliminate, invisibly, until
someone profiles it again. "Has an owner" is really asserted in two
disconnected places (a comment in `OwnerManager.java` and this set), with
nothing keeping them in sync.
Separately, `METADATA_OBJECT_ENTITY_TYPES` is re-derived via
`Entity.EntityType.valueOf(type.name())` instead of reusing
`MetadataObjectUtil`'s existing authoritative `TYPE_TO_TYPE_MAP`. If a future
`MetadataObject.Type`/`Entity.EntityType` name pair ever diverges (the exact
case `TYPE_TO_TYPE_MAP` was built to handle safely), this ad hoc reconstruction
throws `IllegalArgumentException` at class-init instead of failing the
documented way.
---
**simplification**: `METADATA_OBJECT_ENTITY_TYPES.contains(entityType)` is
now evaluated twice for the same call — once at line ~403 (gating
`checkMetadataObjectName`) and again here — with no shared variable connecting
them. A reader auditing which types get owner-preload treatment has to notice
these are the same condition checked in two unrelated-looking places; if
someone changes the semantics of one without noticing the other, the two checks
could silently diverge on which entity types they treat as metadata objects.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java:
##########
@@ -821,4 +979,33 @@ private static void
makeCompletableFutureUseCurrentThread() {
throw new RuntimeException(e);
}
}
+
+ private static NameIdentifier[] principalIdentifiers(Entity.EntityType type,
int count) {
+ return IntStream.range(0, count)
+ .mapToObj(
+ i ->
+ switch (type) {
+ case USER -> NameIdentifierUtil.ofUser("testMetalake",
"user" + i);
+ case GROUP -> NameIdentifierUtil.ofGroup("testMetalake",
"group" + i);
+ default -> NameIdentifierUtil.ofRole("testMetalake", "role"
+ i);
+ })
+ .toArray(NameIdentifier[]::new);
+ }
+
+ private static String principalListExpression(Entity.EntityType type) {
Review Comment:
**reuse**: `principalListExpression(Entity.EntityType)` and the parallel
identifier/privilege switch helpers here duplicate logic that
`TestPrincipalListQueryCount.java` (same PR) reimplements inline via its own
type-keyed switch statements, rather than sharing one small helper or test
fixture. If a fourth principal-like list type is ever added, or one of the
`LOAD_*_AUTHORIZATION_EXPRESSION` constants is renamed, someone has to remember
to update both copies; the two test files currently agree only by accident.
##########
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:
**conventions**: This comment says resolving owners "adds two store lookups
per entry," but both `preloadToCache` and `preloadOwner` issue a single batched
call per list request (`batchGet`/`batchListEntitiesByRelation`), not one
lookup per identifier — confirmed by this PR's own
`TestPrincipalListQueryCount` test asserting the count stays constant (1-2)
regardless of list size. A future maintainer reading this comment while
deciding whether a similar preload is safe to add elsewhere may believe the
cost model is O(n) per entry and over-engineer a fix for a cost that's actually
O(1) round-trips. Suggested rewording: "adds two extra batched store
round-trips per list request."
--
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]
