jerryshao commented on PR #10874:
URL: https://github.com/apache/gravitino/pull/10874#issuecomment-4419032398
### Code review
Found 5 issues:
1. **`batchInsertOwnerRels` SQL references a non-existent `updated_at`
column** — the INSERT in `OwnerMetaBaseSQLProvider` includes `updated_at` in
the column list and `#{po.updatedAt}` in VALUES, but the `owner_meta` table has
no such column and `OwnerRelPO` has no `updatedAt` field. The existing
`insertOwnerRel` method in the same file correctly omits it. This will throw a
SQL error on every hierarchical schema creation.
https://github.com/apache/gravitino/blob/9ab4c791bb8f50934698557f5500d84e4c7c6567/core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/OwnerMetaBaseSQLProvider.java#L128-L136
2. **`batchInsertOwnerRelations` ignores the `override` parameter** —
`JDBCBackend.batchInsertOwnerRelations()` accepts `boolean override` but never
passes it to `OwnerMetaService.batchSetOwners()`, which always soft-deletes
existing rows then re-inserts regardless. The interface Javadoc says "if true,
replace existing owner rows per metadata object first", implying
`override=false` should preserve existing rows.
https://github.com/apache/gravitino/blob/9ab4c791bb8f50934698557f5500d84e4c7c6567/core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java#L916-L940
3. **`JcasbinAuthorizer.ownerRel` cache is never invalidated for
auto-created ancestor schemas** — in `OwnerManager.setOwners()`,
`notifyOwnerChange()` is only called via `originOwner.ifPresent(...)`. For
newly auto-created parent schemas (no prior owner), `notifyOwnerChange` never
fires, so the `ownerRel` cache in `JcasbinAuthorizer` is not invalidated.
Authorization checks will not recognize the newly assigned owner on those
ancestors until the cache TTL expires.
https://github.com/apache/gravitino/blob/9ab4c791bb8f50934698557f5500d84e4c7c6567/core/src/main/java/org/apache/gravitino/authorization/OwnerManager.java#L196-L205
4. **No unit tests for `OwnerManager.setOwners` or
`OwnerMetaService.batchSetOwners`** — both are new non-trivial methods, but
`TestOwnerManager` and `TestOwnerMetaService` are not modified by this PR.
[CLAUDE.md](https://github.com/apache/gravitino/blob/main/CLAUDE.md) says
"Write unit tests for ALL new logic. NO tests = NO merge."
5. **`allScopes` produces an empty-string ancestor when the schema name
starts with the separator** — e.g. `allScopes(":A:B", ":")` returns `[":A:B",
":A", ""]`. The empty string is then passed to `MetadataObjects.of(parent,
scope, Type.SCHEMA)`, which validates `name != null && !name.isEmpty()` and
throws `IllegalArgumentException`. No upstream validation prevents a schema
name from starting with the configured separator.
https://github.com/apache/gravitino/blob/9ab4c791bb8f50934698557f5500d84e4c7c6567/core/src/main/java/org/apache/gravitino/catalog/HierarchicalSchemaUtil.java#L150-L163
🤖 Generated with [Claude Code](https://claude.ai/code)
<sub>- If this code review was useful, please react with 👍. Otherwise, react
with 👎.</sub>
--
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]
