Remove the ADD_TRAILING_SLASH_TO_LOCATION feature flag

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

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<Boolean> ADD_TRAILING_SLASH_TO_LOCATION =
-      PolarisConfiguration.<Boolean>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<Boolean> ALLOW_OPTIMIZED_SIBLING_CHECK =
       PolarisConfiguration.<Boolean>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();
 

runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java

@@ -671,14 +671,8 @@ private void createNamespaceInternal(
       Namespace namespace,
       Map<String, String> 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();
     }
 

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<Error>();
+    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) {

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<PolarisEntityCore> 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())));
+  }
 }

runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogNoSqlOverlapTest.java

@@ -46,8 +46,6 @@ public Map<String, String> 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;
     }
   }

runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogOverlapTest.java

@@ -44,8 +44,6 @@ public Map<String, String> 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;
     }
   }

runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalogRelationalOverlapTest.java

@@ -45,9 +45,6 @@ public Map<String, String> 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");

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<String, String> defaults, Map<String, RealmOverridable.RealmOverrides> realmOverrides) {
+    FeaturesConfiguration config = mock(FeaturesConfiguration.class);
+    when(config.defaults()).thenReturn(defaults);
+    when(config.realmOverrides()).thenReturn(realmOverrides);
+    return config;
+  }
+
+  private static RealmOverridable.RealmOverrides mockRealmOverrides(Map<String, String> overrides) {
+    RealmOverridable.RealmOverrides realmOverrides = mock(RealmOverridable.RealmOverrides.class);
+    when(realmOverrides.overrides()).thenReturn(overrides);
+    return realmOverrides;
+  }
+}