Copilot commented on code in PR #4506:
URL: https://github.com/apache/polaris/pull/4506#discussion_r3273740945
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java:
##########
@@ -935,6 +935,62 @@ public void testRegisterAndLoadTableWithReturnedETag() {
}
}
+ /**
+ * Build an Iceberg table, then invoke a subsequent register table call that
overwrites the
+ * existing table metadata. Verifies that the overwrite is applied and the
catalog reflects the
+ * updated metadata.
+ */
+ @Test
+ public void testRegisterTableOverwriteViaRest() {
+ Namespace ns = Namespace.of("ns_overwrite_rest");
+ restCatalog.createNamespace(ns);
+
+ // Create a source table and capture its metadata location
+ Table source = restCatalog.buildTable(TableIdentifier.of(ns,
"source_table"), SCHEMA).create();
+ String currentMetadataLocation =
+ ((BaseTable) source).operations().current().metadataFileLocation();
+ String metadataDir =
+ currentMetadataLocation.substring(0,
currentMetadataLocation.lastIndexOf('/') + 1);
+ String newMetadataLocation = metadataDir + "overwrite-v1.metadata.json";
+
+ try (ResolvingFileIO resolvingFileIO = new ResolvingFileIO()) {
+ initializeClientFileIO(resolvingFileIO);
+ resolvingFileIO.setConf(new Configuration());
+
+ // Write a new metadata file (re-using the current metadata contents)
+ TableMetadataParser.write(
+ ((BaseTable) source).operations().current(),
+ resolvingFileIO.newOutputFile(newMetadataLocation));
+
+ // Invoke REST register with overwrite=true
+ Invocation registerInvocation =
+ catalogApi
+ .request("v1/" + currentCatalogName +
"/namespaces/ns_overwrite_rest/register")
+ .buildPost(
+ Entity.json(
+ Map.of(
+ "name",
+ "source_table",
+ "metadata-location",
+ newMetadataLocation,
+ "overwrite",
+ true)));
+
+ try (Response registerResponse = registerInvocation.invoke()) {
+
assertThat(registerResponse.getStatus()).isEqualTo(Response.Status.OK.getStatusCode());
+ }
+
+ // Reload the table via REST client and verify metadata-location updated
+ Table loaded = restCatalog.loadTable(TableIdentifier.of(ns,
"source_table"));
+ assertThat(((BaseTable)
loaded).operations().current().metadataFileLocation())
+ .isEqualTo(newMetadataLocation);
+
+ // Clean up the test metadata files
+ resolvingFileIO.deleteFile(currentMetadataLocation);
+ resolvingFileIO.deleteFile(newMetadataLocation);
Review Comment:
This test deletes both the old and the *new* metadata file while leaving the
table registered. Because the per-test cleanup purges and drops the created
catalog, a table pointing at a deleted metadata file can cause purge/drop
cleanup to fail or become flaky. Prefer dropping/purging the table (and/or
namespace) before deleting the active metadata file, or only delete the old
metadata file that is no longer referenced.
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/RbacOperationSemantics.java:
##########
@@ -286,6 +288,7 @@ private static void register(
register(CREATE_TABLE_STAGED, TABLE_CREATE);
register(CREATE_TABLE_STAGED_WITH_WRITE_DELEGATION,
EnumSet.of(TABLE_CREATE, TABLE_WRITE_DATA));
register(REGISTER_TABLE, TABLE_CREATE);
+ register(REGISTER_TABLE_OVERWRITE, TABLE_FULL_METADATA);
register(LOAD_TABLE, TABLE_READ_PROPERTIES);
Review Comment:
Adding a new PolarisAuthorizableOperation requires wiring it into
non-default authorizers as well. For example, the Ranger extension’s
RangerPolarisOperationSemantics currently has no mapping for
REGISTER_TABLE_OVERWRITE, which causes RangerPolarisAuthorizer to deny the
operation (semantics==null). Please update the Ranger operation semantics
mapping (and any other authorizer extensions) to include
REGISTER_TABLE_OVERWRITE with the intended required privileges.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -612,13 +612,46 @@ public LoadTableResponse createTableStaged(
* @return ETagged {@link LoadTableResponse} to uniquely identify the table
metadata
*/
public LoadTableResponse registerTable(Namespace namespace,
RegisterTableRequest request) {
- PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.REGISTER_TABLE;
- authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
- op, TableIdentifier.of(namespace, request.name()));
+ TableIdentifier identifier = TableIdentifier.of(namespace, request.name());
+ boolean overwrite = request.overwrite();
+
+ if (overwrite) {
+ authorizeCreateTableLikeUnderNamespaceOperationOrThrow(
+ PolarisAuthorizableOperation.REGISTER_TABLE_OVERWRITE, identifier);
Review Comment:
The overwrite=true path is authorized against the *namespace* via
authorizeCreateTableLikeUnderNamespaceOperationOrThrow(...). Because RBAC
checks privileges transitively along the authorized target path, this bypasses
table-level grants (e.g., TABLE_FULL_METADATA granted directly on the existing
table) and can incorrectly deny valid overwrite requests. Consider branching on
whether the table currently exists: authorize against the table
(authorizeBasicTableLikeOperationOrThrow with REGISTER_TABLE_OVERWRITE) when it
exists, and fall back to the existing REGISTER_TABLE/create-style namespace
authorization when it does not.
--
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]
