diff --git a/CHANGELOG.md b/CHANGELOG.md index b3e942a7ebf..9efd595504a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,8 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti ### Changes +- Removed the `ADD_TRAILING_SLASH_TO_LOCATION` feature flag (catalog config `polaris.config.add-trailing-slash-to-location`). Polaris now always appends a trailing slash to table and namespace base locations. A leftover config value is ignored, with a startup warning if it is set to `false`. + ### Deprecations ### Fixes diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java index 571e5e0f435..f7187b7a9fc 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java @@ -595,15 +595,6 @@ public static void enforceFeatureEnabledOrThrow( .defaultValue(false) .buildFeatureConfiguration(); - public static final FeatureConfiguration ADD_TRAILING_SLASH_TO_LOCATION = - PolarisConfiguration.builder() - .key("ADD_TRAILING_SLASH_TO_LOCATION") - .catalogConfig("polaris.config.add-trailing-slash-to-location") - .description( - "When set, the base location for a table or namespace will have `/` added as a suffix if not present") - .defaultValue(true) - .buildFeatureConfiguration(); - public static final FeatureConfiguration ALLOW_OPTIMIZED_SIBLING_CHECK = PolarisConfiguration.builder() .key("ALLOW_OPTIMIZED_SIBLING_CHECK") @@ -625,9 +616,9 @@ public static void enforceFeatureEnabledOrThrow( + "views, and namespaces. This is not a bypass mode, but enabling or disabling " + "it can change overlap-detection coverage for non-standard location layouts. " + "Only enable it when the required index and backfill state is known to be " - + "correct. For correct results, locations should end with a slash; see " - + "ADD_TRAILING_SLASH_TO_LOCATION. Supported by the JDBC and NoSQL metastore " - + "implementations.") + + "correct. Locations written by Polaris always end with a slash; locations " + + "stored by older versions without one are still handled. Supported by the " + + "JDBC and NoSQL metastore implementations.") .defaultValue(false) .buildFeatureConfiguration(); diff --git a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java index 1cea9890c3a..990093f2002 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java @@ -671,14 +671,8 @@ private void createNamespaceInternal( Namespace namespace, Map metadata, PolarisResolvedPathWrapper resolvedParent) { - String baseLocation = resolveNamespaceLocation(namespace, metadata); - - // Set / suffix - boolean requireTrailingSlash = - realmConfig.getConfig(FeatureConfiguration.ADD_TRAILING_SLASH_TO_LOCATION); - if (requireTrailingSlash && !baseLocation.endsWith("/")) { - baseLocation += "/"; - } + String baseLocation = + StorageLocation.ensureTrailingSlash(resolveNamespaceLocation(namespace, metadata)); NamespaceEntity entity = new NamespaceEntity.Builder(namespace) @@ -2817,15 +2811,11 @@ private void createTableLike( PolarisResolvedPathWrapper resolvedParent, boolean validateMetadataLocation) { IcebergTableLikeEntity icebergTableLikeEntity = IcebergTableLikeEntity.of(entity); - // Set / suffix - boolean requireTrailingSlash = - realmConfig.getConfig(FeatureConfiguration.ADD_TRAILING_SLASH_TO_LOCATION); - if (requireTrailingSlash - && icebergTableLikeEntity.getBaseLocation() != null - && !icebergTableLikeEntity.getBaseLocation().endsWith("/")) { + if (icebergTableLikeEntity.getBaseLocation() != null) { icebergTableLikeEntity = new IcebergTableLikeEntity.Builder(icebergTableLikeEntity) - .setBaseLocation(icebergTableLikeEntity.getBaseLocation() + "/") + .setBaseLocation( + StorageLocation.ensureTrailingSlash(icebergTableLikeEntity.getBaseLocation())) .build(); } @@ -2886,15 +2876,11 @@ private void updateTableLike( } IcebergTableLikeEntity icebergTableLikeEntity = new IcebergTableLikeEntity(entity); - // Set / suffix - boolean requireTrailingSlash = - realmConfig.getConfig(FeatureConfiguration.ADD_TRAILING_SLASH_TO_LOCATION); - if (requireTrailingSlash - && icebergTableLikeEntity.getBaseLocation() != null - && !icebergTableLikeEntity.getBaseLocation().endsWith("/")) { + if (icebergTableLikeEntity.getBaseLocation() != null) { icebergTableLikeEntity = new IcebergTableLikeEntity.Builder(icebergTableLikeEntity) - .setBaseLocation(icebergTableLikeEntity.getBaseLocation() + "/") + .setBaseLocation( + StorageLocation.ensureTrailingSlash(icebergTableLikeEntity.getBaseLocation())) .build(); } diff --git a/runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java b/runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java index 45fd34496cd..f41d003cae5 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/config/ProductionReadinessChecks.java @@ -61,6 +61,12 @@ public class ProductionReadinessChecks { private static final String REFLECTION_FREE_SERIALIZERS_PROPERTY = "quarkus.rest.jackson.optimization.enable-reflection-free-serializers"; + /** + * Key of the removed {@code ADD_TRAILING_SLASH_TO_LOCATION} feature flag, kept only to warn + * operators whose configuration still sets it. + */ + private static final String ADD_TRAILING_SLASH_TO_LOCATION_KEY = "ADD_TRAILING_SLASH_TO_LOCATION"; + /** * A warning sign ⚠ {@code 26A0} with variant selector {@code FE0F}. The sign is preceded by a * null character {@code 0000} to ensure that the warning sign is displayed correctly regardless @@ -371,6 +377,39 @@ public ProductionReadinessCheck checkOverlappingSiblingCheckSettings( : ProductionReadinessCheck.of(errors.toArray(new Error[0])); } + @Produces + public ProductionReadinessCheck checkAddTrailingSlashToLocation( + FeaturesConfiguration featureConfiguration) { + var message = + "ADD_TRAILING_SLASH_TO_LOCATION was removed and is ignored. Polaris always adds a " + + "trailing slash to table and namespace base locations. Remove this setting."; + var errors = new ArrayList(); + if ("false" + .equalsIgnoreCase( + featureConfiguration.defaults().get(ADD_TRAILING_SLASH_TO_LOCATION_KEY))) { + errors.add( + Error.of(message, format("polaris.features.\"%s\"", ADD_TRAILING_SLASH_TO_LOCATION_KEY))); + } + featureConfiguration + .realmOverrides() + .forEach( + (realmId, overrides) -> { + if ("false" + .equalsIgnoreCase( + overrides.overrides().get(ADD_TRAILING_SLASH_TO_LOCATION_KEY))) { + errors.add( + Error.of( + message, + format( + "polaris.features.realm-overrides.\"%s\".overrides.\"%s\"", + realmId, ADD_TRAILING_SLASH_TO_LOCATION_KEY))); + } + }); + return errors.isEmpty() + ? ProductionReadinessCheck.OK + : ProductionReadinessCheck.of(errors.toArray(new Error[0])); + } + @Produces public ProductionReadinessCheck checkGcsPrincipalAttribution( FeaturesConfiguration featureConfiguration) { 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 ee90c8b3b24..147f6a4c6d0 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 @@ -49,10 +49,15 @@ import org.apache.polaris.core.context.CallContext; import org.apache.polaris.core.entity.CatalogEntity; import org.apache.polaris.core.entity.PolarisEntity; +import org.apache.polaris.core.entity.PolarisEntityCore; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.entity.table.IcebergTableLikeEntity; import org.apache.polaris.core.identity.provider.ServiceIdentityProvider; import org.apache.polaris.core.persistence.MetaStoreManagerFactory; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet; +import org.apache.polaris.core.persistence.dao.entity.EntityResult; import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory; import org.apache.polaris.core.persistence.resolver.ResolverFactory; import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider; @@ -299,9 +304,11 @@ public void testParentChildLocationOverlapWithOptimizedSiblingCheck() { @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. + // The catalog now always stores locations with a trailing slash + // (ADD_TRAILING_SLASH_TO_LOCATION was removed). Simulate legacy slash-less data + // by rewriting the stored parent location directly, then verify the optimized sibling check + // still detects overlaps against it: ancestor equality terms used to be slash-terminated only, + // so a slash-less location_without_scheme was missed. Namespace ns = Namespace.of("ns-for-trailing-slash-overlap"); catalog().createNamespace(ns); @@ -310,9 +317,11 @@ public void testParentPrefixOverlapWithTrailingSlashMismatch() { 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()) + stripStoredBaseLocationTrailingSlash(ns, parentTable); + + // Guardrail: the stored location must be slash-less so overlap checks exercise the + // non-slash-terminated location_without_scheme path that QueryGenerator must handle. + assertThat(storedTableBaseLocation(ns, parentTable)) .as("parent must remain slash-less so overlap uses non-slash-terminated stored location") .isEqualTo(parentLoc) .doesNotEndWith("/"); @@ -335,4 +344,54 @@ public void testParentPrefixOverlapWithTrailingSlashMismatch() { .hasMessageContaining("Unable to create entity at location") .hasMessageContaining("conflicts with existing table or namespace"); } + + private String storedTableBaseLocation(Namespace ns, TableIdentifier table) { + return IcebergTableLikeEntity.of(readStoredTableLike(ns, table)).getBaseLocation(); + } + + /** + * Rewrites the stored base location of {@code table} without its trailing slash, simulating data + * written before Polaris always appended one. + */ + private void stripStoredBaseLocationTrailingSlash(Namespace ns, TableIdentifier table) { + PolarisEntity tableEntity = readStoredTableLike(ns, table); + IcebergTableLikeEntity tableLike = IcebergTableLikeEntity.of(tableEntity); + String baseLocation = tableLike.getBaseLocation(); + assertThat(baseLocation).endsWith("/"); + IcebergTableLikeEntity stripped = + new IcebergTableLikeEntity.Builder(tableLike) + .setBaseLocation(baseLocation.substring(0, baseLocation.length() - 1)) + .build(); + EntityResult result = + metaStoreManager.updateEntityPropertiesIfNotChanged( + polarisContext, tableLikeCatalogPath(ns), stripped); + assertThat(result.isSuccess()).isTrue(); + } + + private PolarisEntity readStoredTableLike(Namespace ns, TableIdentifier table) { + EntityResult result = + metaStoreManager.readEntityByName( + polarisContext, + tableLikeCatalogPath(ns), + PolarisEntityType.TABLE_LIKE, + PolarisEntitySubType.ICEBERG_TABLE, + table.name()); + assertThat(result.isSuccess()).isTrue(); + return PolarisEntity.of(result.getEntity()); + } + + private List tableLikeCatalogPath(Namespace ns) { + assertThat(ns.length()).as("test namespaces must be single-level").isEqualTo(1); + EntityResult result = + metaStoreManager.readEntityByName( + polarisContext, + List.of(PolarisEntity.toCore(catalogEntity)), + PolarisEntityType.NAMESPACE, + PolarisEntitySubType.NULL_SUBTYPE, + ns.level(0)); + assertThat(result.isSuccess()).isTrue(); + return List.of( + PolarisEntity.toCore(catalogEntity), + PolarisEntity.toCore(PolarisEntity.of(result.getEntity()))); + } } 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 3c8490a6e05..8bb3aa5bb5b 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,8 +46,6 @@ 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 76466004134..a66101a450b 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,8 +44,6 @@ 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 139974fbdd0..946174d9c46 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,9 +45,6 @@ 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"); diff --git a/runtime/service/src/test/java/org/apache/polaris/service/config/ProductionReadinessChecksAddTrailingSlashTest.java b/runtime/service/src/test/java/org/apache/polaris/service/config/ProductionReadinessChecksAddTrailingSlashTest.java new file mode 100644 index 00000000000..1f2662bc7d3 --- /dev/null +++ b/runtime/service/src/test/java/org/apache/polaris/service/config/ProductionReadinessChecksAddTrailingSlashTest.java @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.config; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Map; +import org.apache.polaris.core.config.ProductionReadinessCheck; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class ProductionReadinessChecksAddTrailingSlashTest { + + private static final String FLAG_KEY = "ADD_TRAILING_SLASH_TO_LOCATION"; + + private ProductionReadinessChecks checks; + + @BeforeEach + void setUp() { + checks = new ProductionReadinessChecks(); + } + + @Test + void flagUnsetReturnsOk() { + FeaturesConfiguration config = mockConfig(Map.of(), Map.of()); + + ProductionReadinessCheck result = checks.checkAddTrailingSlashToLocation(config); + + assertThat(result.ready()).isTrue(); + } + + @Test + void flagExplicitlyTrueReturnsOk() { + FeaturesConfiguration config = mockConfig(Map.of(FLAG_KEY, "true"), Map.of()); + + ProductionReadinessCheck result = checks.checkAddTrailingSlashToLocation(config); + + assertThat(result.ready()).isTrue(); + } + + @Test + void flagExplicitlyFalseReturnsWarning() { + FeaturesConfiguration config = mockConfig(Map.of(FLAG_KEY, "false"), Map.of()); + + ProductionReadinessCheck result = checks.checkAddTrailingSlashToLocation(config); + + assertThat(result.ready()).isFalse(); + assertThat(result.getErrors()) + .singleElement() + .satisfies( + error -> { + assertThat(error.offendingProperty()).contains(FLAG_KEY); + assertThat(error.severe()).isFalse(); + }); + } + + @Test + void realmOverrideFalseReturnsWarning() { + FeaturesConfiguration config = + mockConfig(Map.of(), Map.of("test-realm", mockRealmOverrides(Map.of(FLAG_KEY, "false")))); + + ProductionReadinessCheck result = checks.checkAddTrailingSlashToLocation(config); + + assertThat(result.ready()).isFalse(); + assertThat(result.getErrors()) + .singleElement() + .satisfies( + error -> { + assertThat(error.offendingProperty()).contains("test-realm").contains(FLAG_KEY); + assertThat(error.severe()).isFalse(); + }); + } + + private static FeaturesConfiguration mockConfig( + Map defaults, Map realmOverrides) { + FeaturesConfiguration config = mock(FeaturesConfiguration.class); + when(config.defaults()).thenReturn(defaults); + when(config.realmOverrides()).thenReturn(realmOverrides); + return config; + } + + private static RealmOverridable.RealmOverrides mockRealmOverrides(Map overrides) { + RealmOverridable.RealmOverrides realmOverrides = mock(RealmOverridable.RealmOverrides.class); + when(realmOverrides.overrides()).thenReturn(overrides); + return realmOverrides; + } +} diff --git a/site/content/in-dev/unreleased/configuration/config-sections/flags-polaris_features.md b/site/content/in-dev/unreleased/configuration/config-sections/flags-polaris_features.md index 34ad7212140..ec8736339cc 100644 --- a/site/content/in-dev/unreleased/configuration/config-sections/flags-polaris_features.md +++ b/site/content/in-dev/unreleased/configuration/config-sections/flags-polaris_features.md @@ -25,16 +25,6 @@ build: Feature configurations for Polaris. These are stable, user-facing settings. -##### `polaris.features."ADD_TRAILING_SLASH_TO_LOCATION"` - -When set, the base location for a table or namespace will have `/` added as a suffix if not present - -- **Type:** `Boolean` -- **Default:** `true` -- **Catalog Config:** `polaris.config.add-trailing-slash-to-location` - ---- - ##### `polaris.features."ALLOW_CLIENT_SPECIFIED_TABLE_LOCATION"` If set to true (the default), Polaris honors a `location` (and the `write.data.path` / `write.metadata.path` properties) explicitly supplied in a create or update request, subject to the usual structured-location, allowed-location, metadata-location, and overlap validation. If set to false, such requests are rejected, regardless of the other location compatibility flags. This setting does not apply to federated catalogs. @@ -460,7 +450,7 @@ How many times to retry refreshing metadata when the previous error was retryabl ##### `polaris.features."OPTIMIZED_SIBLING_CHECK"` -When set, Polaris uses an index to perform sibling overlap checks between tables, views, and namespaces. This is not a bypass mode, but enabling or disabling it can change overlap-detection coverage for non-standard location layouts. Only enable it when the required index and backfill state is known to be correct. For correct results, locations should end with a slash; see ADD_TRAILING_SLASH_TO_LOCATION. Supported by the JDBC and NoSQL metastore implementations. +When set, Polaris uses an index to perform sibling overlap checks between tables, views, and namespaces. This is not a bypass mode, but enabling or disabling it can change overlap-detection coverage for non-standard location layouts. Only enable it when the required index and backfill state is known to be correct. Locations written by Polaris always end with a slash; locations stored by older versions without one are still handled. Supported by the JDBC and NoSQL metastore implementations. - **Type:** `Boolean` - **Default:** `false`