fix(persistence): reject a rename that would move an entity across catalogs - #5305
Open
flyingImer wants to merge 4 commits into
Open
fix(persistence): reject a rename that would move an entity across catalogs#5305flyingImer wants to merge 4 commits into
flyingImer wants to merge 4 commits into
Conversation
dimas-b
reviewed
Aug 17, 2026
dimas-b
left a comment
dimas-b
left a comment
Contributor
There was a problem hiding this comment.
Good catch! Thanks, @flyingImer !
flyingImer
force-pushed
the
fix/rename-entity-cross-catalog-guard
branch
from
August 18, 2026 04:15
666b858 to
1835475
Compare
ayushtkn
reviewed
Aug 24, 2026
ayushtkn
left a comment
ayushtkn
left a comment
Member
There was a problem hiding this comment.
Thanx @flyingImer for the fix, Nice Catch!!!
flyingImer
force-pushed
the
fix/rename-entity-cross-catalog-guard
branch
from
August 24, 2026 21:26
1835475 to
9cb8005
Compare
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
ayushtkn
approved these changes
Aug 26, 2026
ayushtkn
left a comment
ayushtkn
left a comment
Member
There was a problem hiding this comment.
Thanx @flyingImer for the fix
Collaborator
Author
|
@flyrain could you please help with a review and plan a merge on Aug 28? still leaving the community a few more days to check |
flyrain
reviewed
Sep 3, 2026
flyrain
left a comment
flyrain
left a comment
Contributor
There was a problem hiding this comment.
Nice catch and thanks for the fix @flyingImer! Left some comments!
| } | ||
| if (catalogPath == null | ||
| || catalogPath.isEmpty() | ||
| || newCatalogPath.get(0).getId() != catalogPath.get(0).getId()) { |
Contributor
There was a problem hiding this comment.
We may pass the object entityToRename to avoid checks on the original catalogPath, instead of we could use entityToRename.getCatalogId() to get the catalog id.
| if (catalogPath == null | ||
| || catalogPath.isEmpty() | ||
| || newCatalogPath.get(0).getId() != catalogPath.get(0).getId()) { | ||
| throw new IllegalArgumentException("Cannot rename an entity into a different catalog"); |
Contributor
There was a problem hiding this comment.
Can we put the entity name and the source and destination catalog ids(names should be even better) in the message for easier debugging?
| * @throws IllegalArgumentException if newCatalogPath is non-empty and its head names a different | ||
| * catalog than catalogPath's head, including when catalogPath is null or empty | ||
| */ | ||
| default void checkRenameStaysWithinCatalog( |
Contributor
There was a problem hiding this comment.
Having this method means we put a validation helper on the persistence SPI, which every backend now inherits as public API. It feels like an unnecessary new SPI. Instead, can we hold it in shared util class?
| default void checkRenameStaysWithinCatalog( | ||
| @Nullable List<PolarisEntityCore> catalogPath, | ||
| @Nullable List<PolarisEntityCore> newCatalogPath) { | ||
| if (newCatalogPath == null || newCatalogPath.isEmpty()) { |
Contributor
There was a problem hiding this comment.
I think we need to distinguish between a null and an empty newCatalogPath.
A null value means that no new parent was specified, so the rename does not re-parent the entity and we can return immediately. I'd even suggest to move the condition outside of this method, since we don't need to invoke this method in case of no re-parenting.
if (newCatalogPath != null) {
checkRenameStaysWithinCatalog();
}An empty path, however, explicitly represents the top-level realm scope. It should therefore participate in the catalog-ID validation. We could derive the destination catalog ID as follows:
long destinationCatalogId =
newCatalogPath.isEmpty()
? PolarisEntityConstants.getNullId()
: newCatalogPath.getFirst().getId();Then we can compare destinationCatalogId with the catalog ID of the entity being renamed. This would also reject an attempt to move a catalog-contained entity directly to the top-level scope.
…talogs renameEntity derives the target catalog id from the head of newCatalogPath for its name-collision pre-check, then persists the entity with only parentId re-pointed. A cross-catalog rename would store a row whose parent_id lives in the new catalog while its catalog_id still names the old one, and by-name lookups match that row from neither side. Moving an entity between catalogs is not a one-field change: its children, its grant records and its policy mappings all record the catalog it started in, and renameEntity writes none of them. Reject the call rather than half-perform it. No API can express a cross-catalog rename today, so the new test drives the manager directly, from the shared metastore-manager fixture.
An empty catalogPath is the documented shape for a top-level entity, so the previous check let any non-empty newCatalogPath through whenever the source path was null or empty. That is the same inconsistency this change is about: the entity keeps catalog id 0 while its parent_id points inside a catalog. Require a non-empty catalogPath whose head matches the destination, and cover it with a principal-role case in the shared fixture. Reverting only the predicate makes that assertion fail.
…ee backends The three implementations rejected a cross-catalog rename with different exception types: AtomicOperationMetaStoreManager and TransactionalMetaStoreManagerImpl threw IllegalStateException via getDiagnostics().check(), NoSqlMetaStoreManager threw IllegalArgumentException via Guava's checkArgument. IcebergExceptionMapper maps IllegalArgumentException to a 400 but has no case for IllegalStateException, so the identical rejection surfaced as a clean 400 on NoSQL and a 500 with a full "Unhandled exception" stack trace on the other two. Moved the check to a single default method on PolarisMetaStoreManager, called identically from all three implementations. This also unifies the rejection message, which previously differed by backend, and removes three near-duplicate blocks. The test assertion is narrowed from RuntimeException to IllegalArgumentException now that the type is the same everywhere; it could not be narrowed before, since IllegalStateException and IllegalArgumentException share no ancestor closer than RuntimeException.
…ity's own catalog Addresses review feedback on the previous commit. The check was a default method on PolarisMetaStoreManager, which made an internal validation part of the persistence SPI that every backend inherits. It is now a static helper, so implementations call it without the interface carrying it. It also derives the entity's current catalog from entityToRename.getCatalogId() rather than from the head of catalogPath. Both managers already treat that field as authoritative, looking the entity up by it a few lines later, and it is the value that actually lands in the persisted row, so comparing against it removes the assumption that catalogPath and the entity agree, and drops a parameter. Finally, a null and an empty newCatalogPath no longer mean the same thing. Null means no re-parenting was requested and nothing is checked. An empty list names the top-level realm scope, so it is now compared like any other destination, which rejects moving a catalog-contained entity out to the top level. That was previously allowed and is the same inconsistency in the other direction. The rejection message now carries the entity name and both catalog ids.
flyingImer
force-pushed
the
fix/rename-entity-cross-catalog-guard
branch
from
September 4, 2026 23:18
9cb8005 to
b6a251e
Compare
flyrain
approved these changes
Sep 4, 2026
flyrain
left a comment
flyrain
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Thanks @flyingImer !
github-project-automation
Bot
moved this from Done
to Ready to merge
in Basic Kanban Board
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
renameEntity accepts a destination path, but it does not support moving an entity across catalogs. Before this change, nothing enforced that restriction.
If newCatalogPath is rooted in a different catalog, the rename can persist an inconsistent entity: its parentId is changed to a parent in the destination catalog, while its catalogId still points to the source catalog.
That entity is then no longer addressable by its expected path. A lookup under the old path uses the old parent and misses it, while a lookup under the new path uses the destination catalog id and misses it there as well.
The current REST table-rename surface cannot express this case because its source and destination are Iceberg TableIdentifiers, which contain a namespace and name but no catalog. However, renameEntity itself does not state or enforce that restriction, so a direct or future caller can create invalid persisted state.
Root cause
The rename implementations treat newCatalogPath as a re-parenting operation within the existing catalog.
When checking for a name collision at the destination, they derive the target catalog id from the first element of newCatalogPath. But when the rename is persisted, only the entity's parent is re-pointed; its existing catalogId is retained.
That is valid for moving an entity between namespaces in the same catalog, but not across catalogs.
A cross-catalog move would require more than changing the entity's parent. Descendants, grant records, and policy mappings can also retain references to the catalog the entity belongs to. renameEntity does not migrate that state, so accepting a cross-catalog destination would only partially perform the move.
The fix
Reject a rename when both the current and destination paths are present and their root catalog ids differ.
The validation is added consistently to: AtomicOperationMetaStoreManager, TransactionalMetaStoreManagerImpl, NoSqlMetaStoreManager
Same-catalog renames and re-parenting continue to use the existing behavior.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)