diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d8b1c9dfc..955ef1e1c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti - `TableCleanupTaskHandler` now clamps `TABLE_METADATA_CLEANUP_BATCH_SIZE` to at least 1. Previously a non-positive realm override caused an infinite loop (0) or `IllegalArgumentException` (<0) when splitting metadata files for cleanup. - `ManifestFileCleanupTaskHandler` now handles Iceberg v2 delete manifests in addition to data manifests. Previously, `DROP TABLE PURGE` on a v2 table that had been updated via merge-on-read DML left position-delete files and their manifests as orphans in object storage; the cleanup task failed silently because `ManifestFiles.read()` rejects delete manifests. - OPA authorizer now includes the realm identifier in the authorization context sent to OPA (`input.context.realm`). This ensures OPA policies can enforce tenant isolation across realms, preventing potential collisions if identical principal or resource names exist in different realms. +- Fixed a false-negative in the JDBC optimized location-overlap check (`OPTIMIZED_SIBLING_CHECK`). Ancestor locations stored in `location_without_scheme` without a trailing slash were not matched by the generated ancestor equality terms, allowing nested table/namespace locations to be created under existing prefixes. The query now emits both slash-terminated and non-slash-terminated prefix terms and uses a slash-terminated `LIKE` pattern for descendant matching. ## [1.6.0] diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java index d35773a81c..0668cebecf 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java @@ -23,6 +23,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -390,6 +391,10 @@ static PreparedQuery generateEntityTableExistQuery() { * a path on one storage type may give a false positive for overlapping with another storage type. * This should be combined with a check using `StorageLocation`. * + *

Equality terms are generated for each prefix of the location in both slash-terminated and + * non-slash-terminated forms so that ancestors stored with or without a trailing slash are both + * matched. + * * @param realmId A realm to search within * @param schemaVersion The schema version of entities table to query * @param catalogId A catalog entity to search within @@ -405,18 +410,35 @@ public static PreparedQuery generateOverlapQuery( List conditions = new ArrayList<>(); List parameters = new ArrayList<>(); - String[] components = locationWithoutScheme.split("/"); - StringBuilder pathBuilder = new StringBuilder(); + // Normalize by stripping any trailing slash so the scan below only emits meaningful prefixes. + // The slash-terminated form is re-added as the last prefix term below. + String normalizedLocation = locationWithoutScheme; + if (normalizedLocation.endsWith("/")) { + normalizedLocation = normalizedLocation.substring(0, normalizedLocation.length() - 1); + } + + Set prefixTerms = new LinkedHashSet<>(); + for (int i = 0; i < normalizedLocation.length(); i++) { + if (normalizedLocation.charAt(i) == '/') { + if (i > 0) { + prefixTerms.add(normalizedLocation.substring(0, i)); + } + prefixTerms.add(normalizedLocation.substring(0, i + 1)); + } + } + prefixTerms.add(normalizedLocation); + prefixTerms.add(normalizedLocation + "/"); - for (String component : components) { - pathBuilder.append(component).append("/"); + for (String prefix : prefixTerms) { conditions.add("location_without_scheme = ?"); - parameters.add(pathBuilder.toString()); + parameters.add(prefix); } - // Add LIKE condition to match children + // Add LIKE condition to match children. Slash-terminate the location so the pattern only + // matches true descendants (e.g. //bucket/ns/tA/% matches //bucket/ns/tA/child but not + // //bucket/ns/tA_backup). conditions.add("location_without_scheme LIKE ?"); - parameters.add(locationWithoutScheme + "%"); + parameters.add(StorageLocation.ensureTrailingSlash(locationWithoutScheme) + "%"); String locationClause = String.join(" OR ", conditions); String clause = " WHERE realm_id = ? AND catalog_id = ? AND (" + locationClause + ")"; diff --git a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java index 04ae231a88..3b61f6bbe1 100644 --- a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java +++ b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/QueryGeneratorTest.java @@ -390,6 +390,7 @@ void testGenerateOverlapQuery() { + " properties, internal_properties, grant_records_version, location_without_scheme FROM" + " POLARIS_SCHEMA.ENTITIES WHERE realm_id = ? AND catalog_id = ? AND (location_without_scheme = ?" + " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR" + + " location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR" + " location_without_scheme = ? OR location_without_scheme LIKE ?)", QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/tmp/location/").sql()); Assertions.assertThatCollection( @@ -400,8 +401,29 @@ void testGenerateOverlapQuery() { -123L, "/", "//", + "//bucket", "//bucket/", + "//bucket/tmp", "//bucket/tmp/", + "//bucket/tmp/location", + "//bucket/tmp/location/", + "//bucket/tmp/location/%"); + + // A location without a trailing slash produces the same prefix terms so that ancestors stored + // in either form are matched. + Assertions.assertThatCollection( + QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://bucket/tmp/location") + .parameters()) + .containsExactly( + "realmId", + -123L, + "/", + "//", + "//bucket", + "//bucket/", + "//bucket/tmp", + "//bucket/tmp/", + "//bucket/tmp/location", "//bucket/tmp/location/", "//bucket/tmp/location/%"); @@ -409,25 +431,45 @@ void testGenerateOverlapQuery() { "SELECT id, catalog_id, parent_id, type_code, name, entity_version, sub_type_code," + " create_timestamp, drop_timestamp, purge_timestamp, to_purge_timestamp, last_update_timestamp," + " properties, internal_properties, grant_records_version, location_without_scheme FROM" - + " POLARIS_SCHEMA.ENTITIES WHERE realm_id = ? AND catalog_id = ? AND (location_without_scheme = ? OR location_without_scheme = ?" - + " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme LIKE ?)", + + " POLARIS_SCHEMA.ENTITIES WHERE realm_id = ? AND catalog_id = ? AND (location_without_scheme = ?" + + " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR" + + " location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR" + + " location_without_scheme LIKE ?)", QueryGenerator.generateOverlapQuery("realmId", 2, -123, "/tmp/location/").sql()); Assertions.assertThatCollection( QueryGenerator.generateOverlapQuery("realmId", 2, -123, "/tmp/location/").parameters()) .containsExactly( - "realmId", -123L, "/", "//", "///", "///tmp/", "///tmp/location/", "///tmp/location/%"); + "realmId", + -123L, + "/", + "//", + "///", + "///tmp", + "///tmp/", + "///tmp/location", + "///tmp/location/", + "///tmp/location/%"); assertEquals( "SELECT id, catalog_id, parent_id, type_code, name, entity_version, sub_type_code," + " create_timestamp, drop_timestamp, purge_timestamp, to_purge_timestamp, last_update_timestamp," + " properties, internal_properties, grant_records_version, location_without_scheme" + " FROM POLARIS_SCHEMA.ENTITIES WHERE realm_id = ? AND catalog_id = ? AND (location_without_scheme = ?" - + " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme LIKE ?)", + + " OR location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme = ? OR" + + " location_without_scheme = ? OR location_without_scheme = ? OR location_without_scheme LIKE ?)", QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://バケツ/\"loc.ation\"/").sql()); Assertions.assertThatCollection( QueryGenerator.generateOverlapQuery("realmId", 2, -123, "s3://バケツ/\"loc.ation\"/") .parameters()) .containsExactly( - "realmId", -123L, "/", "//", "//バケツ/", "//バケツ/\"loc.ation\"/", "//バケツ/\"loc.ation\"/%"); + "realmId", + -123L, + "/", + "//", + "//バケツ", + "//バケツ/", + "//バケツ/\"loc.ation\"", + "//バケツ/\"loc.ation\"/", + "//バケツ/\"loc.ation\"/%"); } } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java index 9b7503b325..a28e4b0be8 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogOverlapTest.java @@ -19,6 +19,7 @@ package org.apache.polaris.service.catalog.iceberg; import static org.apache.polaris.service.admin.PolarisAuthzTestBase.SCHEMA; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.when; @@ -294,4 +295,43 @@ public void testParentChildLocationOverlapWithOptimizedSiblingCheck() { .hasMessageContaining("Unable to create entity at location") .hasMessageContaining("conflicts with existing table or namespace"); } + + @Test + public void testParentPrefixOverlapWithTrailingSlashMismatch() { + // Profiles disable ADD_TRAILING_SLASH_TO_LOCATION so the parent is stored without a trailing + // slash. That is the OPTIMIZED_SIBLING_CHECK false-negative for JDBC: ancestor equality terms + // used to be slash-terminated only, so location_without_scheme without '/' was missed. + Namespace ns = Namespace.of("ns-for-trailing-slash-overlap"); + catalog().createNamespace(ns); + + TableIdentifier parentTable = TableIdentifier.of(ns, "trailing-parent"); + String parentLoc = STORAGE_LOCATION + "/trailing-overlap/parent"; + assertThat(parentLoc).doesNotEndWith("/"); + catalog().buildTable(parentTable, SCHEMA).withLocation(parentLoc).create(); + + // Guardrail: if trailing-slash normalization were still on, this test would not exercise the + // slash-less location_without_scheme path that QueryGenerator must handle. + assertThat(catalog().loadTable(parentTable).location()) + .as("parent must remain slash-less so overlap uses non-slash-terminated stored location") + .isEqualTo(parentLoc) + .doesNotEndWith("/"); + + // Creating a child table under the slash-less parent location must be rejected. + TableIdentifier childTable = TableIdentifier.of(ns, "trailing-child"); + String childLoc = STORAGE_LOCATION + "/trailing-overlap/parent/child"; + assertThatThrownBy( + () -> catalog().buildTable(childTable, SCHEMA).withLocation(childLoc).create()) + .isInstanceOf(ForbiddenException.class) + .hasMessageContaining("Unable to create entity at location") + .hasMessageContaining("conflicts with existing table or namespace"); + + // The same must hold when the slash-terminated form of the same prefix is used. + TableIdentifier siblingTable = TableIdentifier.of(ns, "trailing-sibling"); + String siblingLoc = STORAGE_LOCATION + "/trailing-overlap/parent/"; + assertThatThrownBy( + () -> catalog().buildTable(siblingTable, SCHEMA).withLocation(siblingLoc).create()) + .isInstanceOf(ForbiddenException.class) + .hasMessageContaining("Unable to create entity at location") + .hasMessageContaining("conflicts with existing table or namespace"); + } } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogNoSqlOverlapTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogNoSqlOverlapTest.java index 8bb3aa5bb5..3c8490a6e0 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogNoSqlOverlapTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogNoSqlOverlapTest.java @@ -46,6 +46,8 @@ public Map getConfigOverrides() { overrides.put("polaris.features.\"ALLOW_TABLE_LOCATION_OVERLAP\"", "false"); overrides.put("polaris.features.\"OPTIMIZED_SIBLING_CHECK\"", "true"); overrides.put("polaris.features.\"ALLOW_OPTIMIZED_SIBLING_CHECK\"", "true"); + // Keep locations as written so slash-less base locations are stored as-is. + overrides.put("polaris.features.\"ADD_TRAILING_SLASH_TO_LOCATION\"", "false"); return overrides; } } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogOverlapTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogOverlapTest.java index a66101a450..7646600413 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogOverlapTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogOverlapTest.java @@ -44,6 +44,8 @@ public Map getConfigOverrides() { overrides.put("polaris.features.\"ALLOW_TABLE_LOCATION_OVERLAP\"", "false"); overrides.put("polaris.features.\"OPTIMIZED_SIBLING_CHECK\"", "true"); overrides.put("polaris.features.\"ALLOW_OPTIMIZED_SIBLING_CHECK\"", "true"); + // Keep locations as written so slash-less base locations are stored as-is. + overrides.put("polaris.features.\"ADD_TRAILING_SLASH_TO_LOCATION\"", "false"); return overrides; } } diff --git a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogRelationalOverlapTest.java b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogRelationalOverlapTest.java index 946174d9c4..139974fbdd 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogRelationalOverlapTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogRelationalOverlapTest.java @@ -45,6 +45,9 @@ public Map getConfigOverrides() { overrides.put("polaris.features.\"ALLOW_TABLE_LOCATION_OVERLAP\"", "false"); overrides.put("polaris.features.\"OPTIMIZED_SIBLING_CHECK\"", "true"); overrides.put("polaris.features.\"ALLOW_OPTIMIZED_SIBLING_CHECK\"", "true"); + // Keep locations as written so JDBC optimized sibling checks exercise slash-less + // location_without_scheme values (the false-negative fixed in QueryGenerator). + overrides.put("polaris.features.\"ADD_TRAILING_SLASH_TO_LOCATION\"", "false"); overrides.put("polaris.persistence.type", "relational-jdbc"); overrides.put("polaris.persistence.auto-bootstrap-types", "relational-jdbc"); overrides.put("quarkus.datasource.db-kind", "h2");