dennishuo commented on code in PR #321:
URL: https://github.com/apache/polaris/pull/321#discussion_r1777733297
##########
polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1873,6 +1878,51 @@ private boolean sendNotificationForTableLike(
if (notificationType == NotificationType.DROP) {
return dropTableLike(PolarisEntitySubType.TABLE, tableIdentifier,
Map.of(), false /* purge */)
.isSuccess();
+ } else if (notificationType == NotificationType.VALIDATE) {
+ // In this mode we don't want to make any mutations, so we won't
auto-create non-existing
+ // parent namespaces. This means when we want to validate
allowedLocations for the proposed
+ // table metadata location, we must independently find the deepest
non-null parent namespace
+ // of the TableIdentifier, which may even be the base CatalogEntity if
no parent namespaces
+ // actually exist yet. We can then extract the right StorageInfo entity
via a normal call
+ // to findStorageInfoFromHierarchy.
+ PolarisResolvedPathWrapper resolvedStorageEntity = null;
+ Optional<PolarisEntity> storageInfoEntity = Optional.empty();
+ for (int i = tableIdentifier.namespace().length(); i >= 0; i--) {
Review Comment:
Correct. This is validated in the updated
[PolarisCatalogHandlerWrapperAuthzTest.java](https://github.com/apache/polaris/pull/321/files#diff-e24f0ec3325e14f90aa79137c1bf533824f1a1bcaa6b9f821f3a4ed60f771fa5)
##########
polaris-service/src/test/java/org/apache/polaris/service/catalog/BasePolarisCatalogTest.java:
##########
@@ -360,6 +360,141 @@ public void testCreateNestedNamespaceUnderMissingParent()
{
.hasMessageContaining("Parent");
}
+ @Test
+ public void testValidateNotificationWhenTableAndNamespacesDontExist() {
+ Assumptions.assumeTrue(
+ requiresNamespaceCreate(),
+ "Only applicable if namespaces must be created before adding
children");
+ Assumptions.assumeTrue(
+ supportsNestedNamespaces(), "Only applicable if nested namespaces are
supported");
+ Assumptions.assumeTrue(
+ supportsNotifications(), "Only applicable if notifications are
supported");
+
+ final String tableLocation =
"s3://externally-owned-bucket/validate_table/";
+ final String tableMetadataLocation = tableLocation +
"metadata/v1.metadata.json";
+ BasePolarisCatalog catalog = catalog();
+
+ Namespace namespace = Namespace.of("parent", "child1");
+ TableIdentifier table = TableIdentifier.of(namespace, "table");
+
+ // For a VALIDATE request we can pass in a full metadata JSON filename or
just the table's
+ // metadata directory; either way the path will be validated to be under
the allowed locations,
+ // but any actual metadata JSON file will not be accessed.
+ NotificationRequest request = new NotificationRequest();
+ request.setNotificationType(NotificationType.VALIDATE);
+ TableUpdateNotification update = new TableUpdateNotification();
+ update.setMetadataLocation(tableMetadataLocation);
+ update.setTableName(table.name());
+ update.setTableUuid(UUID.randomUUID().toString());
+ update.setTimestamp(230950845L);
+ request.setPayload(update);
+
+ // We should be able to send the notification without creating the
metadata file since it's
+ // only validating the ability to send the CREATE/UPDATE notification
possibly before actually
+ // creating the table at all on the remote catalog.
+ Assertions.assertThat(catalog.sendNotification(table, request))
+ .as("Notification should be sent successfully")
+ .isTrue();
+ Assertions.assertThat(catalog.namespaceExists(namespace))
+ .as("Intermediate namespaces should not be created")
+ .isFalse();
+ Assertions.assertThat(catalog.tableExists(table))
+ .as("Table should not be created for a VALIDATE notification")
+ .isFalse();
+
+ // Now also check that despite creating the metadata file, the validation
call still doesn't
+ // create any namespaces or tables.
+ InMemoryFileIO fileIO = (InMemoryFileIO) catalog.getIo();
+ fileIO.addFile(
+ tableMetadataLocation,
+
TableMetadataParser.toJson(createSampleTableMetadata(tableLocation)).getBytes(UTF_8));
+
+ Assertions.assertThat(catalog.sendNotification(table, request))
+ .as("Notification should be sent successfully")
+ .isTrue();
+ Assertions.assertThat(catalog.namespaceExists(namespace))
+ .as("Intermediate namespaces should not be created")
+ .isFalse();
+ Assertions.assertThat(catalog.tableExists(table))
+ .as("Table should not be created for a VALIDATE notification")
+ .isFalse();
+ }
+
+ @Test
+ public void testValidateNotificationInDisallowedLocation() {
+ Assumptions.assumeTrue(
+ requiresNamespaceCreate(),
+ "Only applicable if namespaces must be created before adding
children");
+ Assumptions.assumeTrue(
+ supportsNestedNamespaces(), "Only applicable if nested namespaces are
supported");
+ Assumptions.assumeTrue(
+ supportsNotifications(), "Only applicable if notifications are
supported");
+
+ // The location of the metadata JSON file specified in the create will be
forbidden.
+ // For a VALIDATE call we can pass in the metadata/ prefix itself instead
of a metadata JSON
+ // filename.
+ final String tableLocation = "s3://forbidden-table-location/table/";
+ final String tableMetadataLocation = tableLocation + "metadata/";
+ BasePolarisCatalog catalog = catalog();
+
+ Namespace namespace = Namespace.of("parent", "child1");
+ TableIdentifier table = TableIdentifier.of(namespace, "table");
+
+ NotificationRequest request = new NotificationRequest();
+ request.setNotificationType(NotificationType.VALIDATE);
+ TableUpdateNotification update = new TableUpdateNotification();
+ update.setMetadataLocation(tableMetadataLocation);
+ update.setTableName(table.name());
+ update.setTableUuid(UUID.randomUUID().toString());
+ update.setTimestamp(230950845L);
+ request.setPayload(update);
+
+ Assertions.assertThatThrownBy(() -> catalog.sendNotification(table,
request))
+ .isInstanceOf(ForbiddenException.class)
+ .hasMessageContaining("Invalid location");
+ }
+
+ @Test
+ public void testValidateNotificationFailToCreateFileIO() {
Review Comment:
Authz tests are covered in
[PolarisCatalogHandlerWrapperAuthzTest.java](https://github.com/apache/polaris/pull/321/files#diff-e24f0ec3325e14f90aa79137c1bf533824f1a1bcaa6b9f821f3a4ed60f771fa5)
--
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]
