yuqi1129 commented on PR #13262: URL: https://github.com/apache/gravitino/pull/13262#issuecomment-5714560457
> ## Automated review > Re-reviewed this PR (diff + full PR-branch source, cross-checked against `main`). No correctness bugs confirmed — the core change (gate `preloadOwner` on `isMetadataObject`, add metalake-scope short-circuits for USER/GROUP/ROLE) is narrowly scoped, mirrors the existing TABLE/SCHEMA/CATALOG short-circuit pattern, and is backed by thorough regression/query-count tests. > > I initially suspected the `Entity.EntityType.valueOf(type.name())` → `MetadataObjectUtil::toEntityType` refactor (`MetadataAuthzHelper.java`) could throw at class-init time for `MODEL_VERSION`, but confirmed `MetadataObjectUtil.TYPE_TO_TYPE_MAP` covers all `MetadataObject.Type` values on this branch — not an issue. > > 5 minor findings, none blocking: > > 1. **[MetadataAuthzHelper.java](https://github.com/apache/gravitino/blob/adf6cc00fa68f233cc1783be9424a1f46e50725f/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java#L165-L172) — owner short-circuit skips per-object deny for principal lists.** > `principalListPaths`'s `parentOwnerPath(METALAKE_ONLY_SCOPE)` carries an empty `denyPrivileges` set, so a metalake owner now sees the _entire_ unfiltered user/group/role list without any per-object deny check — previously USER/GROUP/ROLE listing had no short-circuit at all and always ran full per-object authorization. This mirrors the pre-existing TABLE/SCHEMA/CATALOG owner-path pattern and is explicitly asserted by `testPrincipalListMetalakeOwner` (zero `hasDenyPolicy` calls), so it looks intentional — but it's the first time owner-supremacy applies to _principal_ listings rather than data objects. Worth an explicit confirmation that hiding an individual user/group/role from a metalake owner via a deny policy is not a supported case. > 2. **[TestPrincipalListQueryCount.java#L486](https://github.com/apache/gravitino/blob/adf6cc00fa68f233cc1783be9424a1f46e50725f/server-common/src/test/java/org/apache/gravitino/server/authorization/TestPrincipalListQueryCount.java#L486) — duplicates existing H2 backend test bootstrap.** > This test manually re-implements H2/`JDBCBackend` bootstrap (Config setup, `EntityIdService.initialize(new RelationalEntityStoreIdResolver())`, teardown) that `core/src/test/java/org/apache/gravitino/storage/relational/BackendTestExtension.java` already provides. `server-common/build.gradle.kts` only depends on `core`'s main sourceSet, not its test fixtures, so this isn't a trivial reuse today — flagging as a maintenance-cost duplication to consider (test-fixtures dependency, or accept consciously). > 3. **[MetadataAuthzHelper.java#L100-L101](https://github.com/apache/gravitino/blob/adf6cc00fa68f233cc1783be9424a1f46e50725f/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java#L100-L101) — duplicate-valued constant.** > `METALAKE_ONLY_SCOPE = "METALAKE"` duplicates the value of the pre-existing `CATALOG_PARENT_SCOPES = "METALAKE"` right below it. Two differently-named constants holding the same literal risk silently desyncing later; consider reusing one. > 4. **[TestPrincipalListQueryCount.java#L262](https://github.com/apache/gravitino/blob/adf6cc00fa68f233cc1783be9424a1f46e50725f/server-common/src/test/java/org/apache/gravitino/server/authorization/TestPrincipalListQueryCount.java#L262) — redundant UPDATE in test setup.** > `insertPrincipals`'s `UPDATE role_meta SET properties = '{}' WHERE properties IS NULL` has no `metalake_id` filter, so each of the three size iterations (1, 1003, 10000) rescans/rewrites all previously-inserted roles from earlier metalakes in the same run rather than just the newly inserted batch. Harmless at this scale, but wasted work a `WHERE metalake_id = ?` would avoid. > 5. **[MetadataAuthzHelper.java#L406](https://github.com/apache/gravitino/blob/adf6cc00fa68f233cc1783be9424a1f46e50725f/server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java#L406) — untested branch of the new gate.** > The new `isMetadataObject` gate on `preloadOwner` has no test for entity types that are neither metadata objects nor principals (e.g. `MODEL_VERSION`, `TABLE_STATISTIC`, `AUDIT`). Currently a dead path (no REST call site passes those types here), so low risk — noting only for coverage completeness. > > 🤖 Generated with [Claude Code](https://claude.com/claude-code) Those 5 finds are nit and improvement, I will defer them. -- 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]
