You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "flyrain (via GitHub)" <gi...@apache.org> on 2026/09/03 00:45:06 UTC
Re: [PR] fix(persistence): reject a rename that would move an entity across catalogs [polaris]
flyrain commented on code in PR #5305:
URL: https://github.com/apache/polaris/pull/5305#discussion_r3919169254
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -304,6 +304,34 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
@Nullable List<PolarisEntityCore> newCatalogPath,
@NonNull PolarisEntity renamedEntity);
+ /**
+ * Reject a rename whose newCatalogPath is rooted in a different catalog than catalogPath.
+ * Implementations of {@link #renameEntity} should call this before persisting.
+ *
+ * <p>Moving an entity between catalogs is not a one-field change: its children, its grant
+ * records, and its policy mappings all still reference the catalog it started in, and a rename
+ * does not update any of those. A caller that wants a real cross-catalog move should get a new,
+ * explicit contract for it rather than working around this check.
+ *
+ * @param catalogPath the entity's current path, or null/empty if it is a top-level entity
+ * @param newCatalogPath the destination path, or null/empty if the entity is not being
+ * re-parented
+ * @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(
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @Nullable List<PolarisEntityCore> newCatalogPath) {
+ if (newCatalogPath == null || newCatalogPath.isEmpty()) {
+ return;
+ }
+ if (catalogPath == null
+ || catalogPath.isEmpty()
+ || newCatalogPath.get(0).getId() != catalogPath.get(0).getId()) {
+ throw new IllegalArgumentException("Cannot rename an entity into a different catalog");
Review 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?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -304,6 +304,34 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
@Nullable List<PolarisEntityCore> newCatalogPath,
@NonNull PolarisEntity renamedEntity);
+ /**
+ * Reject a rename whose newCatalogPath is rooted in a different catalog than catalogPath.
+ * Implementations of {@link #renameEntity} should call this before persisting.
+ *
+ * <p>Moving an entity between catalogs is not a one-field change: its children, its grant
+ * records, and its policy mappings all still reference the catalog it started in, and a rename
+ * does not update any of those. A caller that wants a real cross-catalog move should get a new,
+ * explicit contract for it rather than working around this check.
+ *
+ * @param catalogPath the entity's current path, or null/empty if it is a top-level entity
+ * @param newCatalogPath the destination path, or null/empty if the entity is not being
+ * re-parented
+ * @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(
Review 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?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -304,6 +304,34 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
@Nullable List<PolarisEntityCore> newCatalogPath,
@NonNull PolarisEntity renamedEntity);
+ /**
+ * Reject a rename whose newCatalogPath is rooted in a different catalog than catalogPath.
+ * Implementations of {@link #renameEntity} should call this before persisting.
+ *
+ * <p>Moving an entity between catalogs is not a one-field change: its children, its grant
+ * records, and its policy mappings all still reference the catalog it started in, and a rename
+ * does not update any of those. A caller that wants a real cross-catalog move should get a new,
+ * explicit contract for it rather than working around this check.
+ *
+ * @param catalogPath the entity's current path, or null/empty if it is a top-level entity
+ * @param newCatalogPath the destination path, or null/empty if the entity is not being
+ * re-parented
+ * @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(
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @Nullable List<PolarisEntityCore> newCatalogPath) {
+ if (newCatalogPath == null || newCatalogPath.isEmpty()) {
Review 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.
```java
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:
```java
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.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java:
##########
@@ -304,6 +304,34 @@ default BaseResult bootstrapPolarisService(@NonNull PolarisCallContext callCtx)
@Nullable List<PolarisEntityCore> newCatalogPath,
@NonNull PolarisEntity renamedEntity);
+ /**
+ * Reject a rename whose newCatalogPath is rooted in a different catalog than catalogPath.
+ * Implementations of {@link #renameEntity} should call this before persisting.
+ *
+ * <p>Moving an entity between catalogs is not a one-field change: its children, its grant
+ * records, and its policy mappings all still reference the catalog it started in, and a rename
+ * does not update any of those. A caller that wants a real cross-catalog move should get a new,
+ * explicit contract for it rather than working around this check.
+ *
+ * @param catalogPath the entity's current path, or null/empty if it is a top-level entity
+ * @param newCatalogPath the destination path, or null/empty if the entity is not being
+ * re-parented
+ * @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(
+ @Nullable List<PolarisEntityCore> catalogPath,
+ @Nullable List<PolarisEntityCore> newCatalogPath) {
+ if (newCatalogPath == null || newCatalogPath.isEmpty()) {
+ return;
+ }
+ if (catalogPath == null
+ || catalogPath.isEmpty()
+ || newCatalogPath.get(0).getId() != catalogPath.get(0).getId()) {
Review 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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org