flyingImer commented on code in PR #4506:
URL: https://github.com/apache/polaris/pull/4506#discussion_r3313450522
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -322,6 +351,9 @@ public Table registerTable(TableIdentifier identifier,
String metadataFileLocati
throw new IllegalStateException(
String.format("Failed to fetch resolved parent for TableIdentifier
'%s'", identifier));
}
+
+ validateLocationForTableLike(identifier, metadataFileLocation,
resolvedParent);
Review Comment:
This validation is new for the normal register path too, not only for
overwrite=true. I think the hardening makes sense, but it can reject register
calls that previously succeeded when the metadata file was outside the resolved
storage scope/table location. I believe we should mention that behavior change
in the changelog and PR description?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/common/CatalogHandler.java:
##########
@@ -215,6 +215,53 @@ protected void
authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
initializeCatalog();
}
+ /**
+ * Authorizes a register-table-with-overwrite operation. If the table
already exists, {@code
+ * REGISTER_TABLE_OVERWRITE} is authorized against the table entity. If the
table does not exist,
+ * {@code REGISTER_TABLE} is authorized against the parent namespace.
+ */
+ protected void authorizeRegisterTableOverwriteOrThrow(TableIdentifier
identifier) {
+ Namespace namespace = identifier.namespace();
+ resolutionManifest = newResolutionManifest();
+ resolutionManifest.addPath(
+ new ResolverPath(Arrays.asList(namespace.levels()),
PolarisEntityType.NAMESPACE));
+ resolutionManifest.addPassthroughPath(
+ new ResolverPath(
+ PolarisCatalogHelpers.tableIdentifierToList(identifier),
+ PolarisEntityType.TABLE_LIKE,
+ true /* optional */));
+ resolutionManifest.resolveAll();
+
+ PolarisResolvedPathWrapper tableTarget =
+ resolutionManifest.getResolvedPath(
Review Comment:
FYI, my claude pointed out a ux nit:
> if this identifier is occupied by a view, this falls through to the
namespace REGISTER_TABLE path and the later create path reports the conflict.
That is probably fine authz-wise, but the error may be less clear than "a view
already exists at this identifier"
The ux callout makes sense to me, but I haven't dived too deep into how to
remediate it
--
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]
