From 7febf6e6902961a02ff655fb9193893fd6f07532 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Thu, 26 Jun 2025 16:21:24 -0700 Subject: [PATCH 01/12] Rename SUPPORTED_CATALOG_CONNECTION_TYPES (Part 1) --- .../polaris/core/config/FeatureConfiguration.java | 8 ++++++++ .../src/main/resources/application.properties | 1 + .../polaris/service/admin/PolarisServiceImpl.java | 15 +++++++++++++-- .../service/admin/PolarisServiceImplTest.java | 3 +++ 4 files changed, 25 insertions(+), 2 deletions(-) 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 5ee36d030f..c0be957fc5 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 @@ -270,6 +270,14 @@ public static void enforceFeatureEnabledOrThrow( .defaultValue(List.of(ConnectionType.ICEBERG_REST.name())) .buildFeatureConfiguration(); + public static final FeatureConfiguration> + SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES = + PolarisConfiguration.>builder() + .key("SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES") + .description("The list of supported external catalog connection types for federation") + .defaultValue(List.of(ConnectionType.ICEBERG_REST.name())) + .buildFeatureConfiguration(); + public static final FeatureConfiguration> SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES = PolarisConfiguration.>builder() diff --git a/runtime/defaults/src/main/resources/application.properties b/runtime/defaults/src/main/resources/application.properties index d88df31d4d..b8d42b9dd7 100644 --- a/runtime/defaults/src/main/resources/application.properties +++ b/runtime/defaults/src/main/resources/application.properties @@ -112,6 +112,7 @@ polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=false polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE"] # polaris.features."ENABLE_CATALOG_FEDERATION"=true polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] +polaris.features."ENABLE_EXTERNAL_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] polaris.features."SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES"=["OAUTH", "BEARER"] # realm overrides diff --git a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java index 071e6b61aa..765e35275d 100644 --- a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java +++ b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java @@ -186,7 +186,7 @@ private void validateExternalCatalog(Catalog catalog) { private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigInfo) { String connectionType = connectionConfigInfo.getConnectionType().name(); - List supportedConnectionTypes = + List legacySupportedConnectionTypes = callContext .getPolarisCallContext() .getConfigurationStore() @@ -196,7 +196,18 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI .stream() .map(s -> s.toUpperCase(Locale.ROOT)) .toList(); - if (!supportedConnectionTypes.contains(connectionType)) { + List supportedConnectionTypes = + callContext + .getPolarisCallContext() + .getConfigurationStore() + .getConfiguration( + callContext.getRealmContext(), + FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES) + .stream() + .map(s -> s.toUpperCase(Locale.ROOT)) + .toList(); + if (!supportedConnectionTypes.contains(connectionType) + && !legacySupportedConnectionTypes.contains(connectionType)) { throw new IllegalStateException("Unsupported connection type: " + connectionType); } } diff --git a/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java b/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java index fab1d7da41..c4d92ee4ac 100644 --- a/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java @@ -78,6 +78,9 @@ void setUp() { when(configurationStore.getConfiguration( realmContext, FeatureConfiguration.SUPPORTED_CATALOG_CONNECTION_TYPES)) .thenReturn(List.of("ICEBERG_REST")); + when(configurationStore.getConfiguration( + realmContext, FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES)) + .thenReturn(List.of("ICEBERG_REST")); when(configurationStore.getConfiguration( realmContext, FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES)) .thenReturn(List.of("OAUTH")); From 742b6c845117d84e2c5113fd55e9a1cc51c45962 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Thu, 26 Jun 2025 16:29:13 -0700 Subject: [PATCH 02/12] Update description --- .../org/apache/polaris/core/config/FeatureConfiguration.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c0be957fc5..e1e96fb17d 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 @@ -266,7 +266,7 @@ public static void enforceFeatureEnabledOrThrow( public static final FeatureConfiguration> SUPPORTED_CATALOG_CONNECTION_TYPES = PolarisConfiguration.>builder() .key("SUPPORTED_CATALOG_CONNECTION_TYPES") - .description("The list of supported catalog connection types for federation") + .description("(Deprecated) The list of supported catalog connection types for federation") .defaultValue(List.of(ConnectionType.ICEBERG_REST.name())) .buildFeatureConfiguration(); From 7d8f87bc935612fbb870785debcbd78eed9a9879 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Mon, 30 Jun 2025 11:07:15 -0700 Subject: [PATCH 03/12] Add deprecateKey to PolarisConfiguration --- .../config/BehaviorChangeConfiguration.java | 3 +- .../core/config/FeatureConfiguration.java | 11 +- .../core/config/PolarisConfiguration.java | 40 ++++- .../core/config/PolarisConfigurationTest.java | 140 ++++++++++++++++++ .../service/admin/PolarisServiceImpl.java | 13 +- .../service/admin/PolarisServiceImplTest.java | 3 - 6 files changed, 182 insertions(+), 28 deletions(-) create mode 100644 polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java index db5176e7c3..157a4a872c 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java @@ -34,11 +34,12 @@ public class BehaviorChangeConfiguration extends PolarisConfiguration { protected BehaviorChangeConfiguration( String key, + Optional deprecatedKey, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { - super(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + super(key, deprecatedKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); } public static final BehaviorChangeConfiguration VALIDATE_VIEW_LOCATION_OVERLAP = 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 e1e96fb17d..30095f8c83 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 @@ -36,11 +36,12 @@ public class FeatureConfiguration extends PolarisConfiguration { protected FeatureConfiguration( String key, + Optional deprecatedKey, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { - super(key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + super(key, deprecatedKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); } /** @@ -263,17 +264,11 @@ public static void enforceFeatureEnabledOrThrow( .defaultValue(true) .buildFeatureConfiguration(); - public static final FeatureConfiguration> SUPPORTED_CATALOG_CONNECTION_TYPES = - PolarisConfiguration.>builder() - .key("SUPPORTED_CATALOG_CONNECTION_TYPES") - .description("(Deprecated) The list of supported catalog connection types for federation") - .defaultValue(List.of(ConnectionType.ICEBERG_REST.name())) - .buildFeatureConfiguration(); - public static final FeatureConfiguration> SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES = PolarisConfiguration.>builder() .key("SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES") + .deprecatedKey("SUPPORTED_CATALOG_CONNECTION_TYPES") .description("The list of supported external catalog connection types for federation") .defaultValue(List.of(ConnectionType.ICEBERG_REST.name())) .buildFeatureConfiguration(); diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index 31ae187955..4440ce533b 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -41,6 +41,7 @@ public abstract class PolarisConfiguration { public final String key; public final String description; + public final Optional deprecatedKey; public final T defaultValue; private final Optional catalogConfigImpl; private final Optional catalogConfigUnsafeImpl; @@ -55,9 +56,21 @@ public abstract class PolarisConfiguration { */ private static void registerConfiguration(PolarisConfiguration configuration) { for (PolarisConfiguration existingConfiguration : allConfigurations) { - if (existingConfiguration.key.equals(configuration.key)) { + if (existingConfiguration.key.equals(configuration.key) + || (configuration.deprecatedKey.isPresent() + && existingConfiguration.key.equals(configuration.deprecatedKey.get()))) { throw new IllegalArgumentException( - String.format("Config '%s' is already in use", configuration.key)); + String.format("Config '%s' is already in use", existingConfiguration.key)); + } else if (existingConfiguration.deprecatedKey.isPresent() + && (configuration.key.equals(existingConfiguration.deprecatedKey.get()) + || (configuration.deprecatedKey.isPresent() + && configuration + .deprecatedKey + .get() + .equals(existingConfiguration.deprecatedKey.get())))) { + throw new IllegalArgumentException( + String.format( + "Config '%s' is already in use", existingConfiguration.deprecatedKey.get())); } else { var configs = Stream.of( @@ -86,11 +99,13 @@ public static List> getAllConfigurations() { @SuppressWarnings("unchecked") protected PolarisConfiguration( String key, + Optional deprecatedKey, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { this.key = key; + this.deprecatedKey = deprecatedKey; this.description = description; this.defaultValue = defaultValue; this.catalogConfigImpl = catalogConfig; @@ -98,6 +113,17 @@ protected PolarisConfiguration( this.typ = (Class) defaultValue.getClass(); } + public boolean hasDeprecatedKey() { + return deprecatedKey.isPresent(); + } + + public String deprecatedKey() { + return deprecatedKey.orElseThrow( + () -> + new IllegalStateException( + "Attempted to read a deprecated key from a configuration that doesn't have one.")); + } + public boolean hasCatalogConfig() { return catalogConfigImpl.isPresent(); } @@ -126,6 +152,7 @@ T cast(Object value) { public static class Builder { private String key; + private Optional deprecatedKey = Optional.empty(); private String description; private T defaultValue; private Optional catalogConfig = Optional.empty(); @@ -136,6 +163,11 @@ public Builder key(String key) { return this; } + public Builder deprecatedKey(String deprecatedKey) { + this.deprecatedKey = Optional.of(deprecatedKey); + return this; + } + public Builder description(String description) { this.description = description; return this; @@ -190,7 +222,7 @@ public FeatureConfiguration buildFeatureConfiguration() { validateOrThrow(); FeatureConfiguration config = new FeatureConfiguration<>( - key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + key, deprecatedKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } @@ -203,7 +235,7 @@ public BehaviorChangeConfiguration buildBehaviorChangeConfiguration() { } BehaviorChangeConfiguration config = new BehaviorChangeConfiguration<>( - key, description, defaultValue, catalogConfig, catalogConfigUnsafe); + key, deprecatedKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } diff --git a/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java new file mode 100644 index 0000000000..b173ac86c4 --- /dev/null +++ b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java @@ -0,0 +1,140 @@ +/* + * 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.core.config; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +public class PolarisConfigurationTest { + + @Test + void testFeatureConfigurationWithDeprecatedKey() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("FEATURE_CONFIG_WITH_DEPRECATED_KEY") + .deprecatedKey("OLD_FEATURE_CONFIG_WITH_DEPRECATED_KEY") + .description("Test configuration with deprecated key") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThat(config.hasDeprecatedKey()).isTrue(); + assertThat(config.deprecatedKey()).isEqualTo("OLD_FEATURE_CONFIG_WITH_DEPRECATED_KEY"); + assertThat(config.key).isEqualTo("FEATURE_CONFIG_WITH_DEPRECATED_KEY"); + assertThat(config.description).isEqualTo("Test configuration with deprecated key"); + assertThat(config.defaultValue).isTrue(); + } + + @Test + void testBehaviorChangeConfigurationWithDeprecatedKey() { + BehaviorChangeConfiguration config = + PolarisConfiguration.builder() + .key("BEHAVIOR_CONFIG_WITH_DEPRECATED_KEY") + .deprecatedKey("OLD_BEHAVIOR_CONFIG_WITH_DEPRECATED_KEY") + .description("Test behavior configuration with deprecated key") + .defaultValue("test-value") + .buildBehaviorChangeConfiguration(); + + assertThat(config.hasDeprecatedKey()).isTrue(); + assertThat(config.deprecatedKey()).isEqualTo("OLD_BEHAVIOR_CONFIG_WITH_DEPRECATED_KEY"); + assertThat(config.key).isEqualTo("BEHAVIOR_CONFIG_WITH_DEPRECATED_KEY"); + assertThat(config.defaultValue).isEqualTo("test-value"); + } + + @Test + void testDuplicateKeyValidation() { + PolarisConfiguration.builder() + .key("DUPLICATE_KEY_TEST_1") + .description("First configuration") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThatThrownBy( + () -> + PolarisConfiguration.builder() + .key("DUPLICATE_KEY_TEST_1") + .description("Second configuration") + .defaultValue(false) + .buildFeatureConfiguration()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Config 'DUPLICATE_KEY_TEST_1' is already in use"); + } + + @Test + void testDuplicateDeprecatedKeyValidation() { + PolarisConfiguration.builder() + .key("NEW_KEY_DEPRECATED_TEST_2") + .deprecatedKey("DEPRECATED_KEY_TEST_2") + .description("First configuration with deprecated key") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThatThrownBy( + () -> + PolarisConfiguration.builder() + .key("ANOTHER_NEW_KEY_2") + .deprecatedKey("DEPRECATED_KEY_TEST_2") + .description("Second configuration with same deprecated key") + .defaultValue(false) + .buildFeatureConfiguration()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Config 'DEPRECATED_KEY_TEST_2' is already in use"); + } + + @Test + void testNewKeyMatchingExistingDeprecatedKeyValidation() { + PolarisConfiguration.builder() + .key("ORIGINAL_KEY_3") + .deprecatedKey("OLD_DEPRECATED_KEY_3") + .description("First configuration with deprecated key") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThatThrownBy( + () -> + PolarisConfiguration.builder() + .key("OLD_DEPRECATED_KEY_3") + .description("Configuration with key matching existing deprecated key") + .defaultValue(false) + .buildFeatureConfiguration()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Config 'OLD_DEPRECATED_KEY_3' is already in use"); + } + + @Test + void testDeprecatedKeyMatchingExistingKeyValidation() { + PolarisConfiguration.builder() + .key("EXISTING_KEY_4") + .description("First configuration") + .defaultValue(true) + .buildFeatureConfiguration(); + + assertThatThrownBy( + () -> + PolarisConfiguration.builder() + .key("NEW_KEY_FOR_VALIDATION_4") + .deprecatedKey("EXISTING_KEY_4") + .description("Configuration with deprecated key matching existing key") + .defaultValue(false) + .buildFeatureConfiguration()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Config 'EXISTING_KEY_4' is already in use"); + } +} diff --git a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java index 765e35275d..a3e7c19f6c 100644 --- a/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java +++ b/service/common/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java @@ -186,16 +186,6 @@ private void validateExternalCatalog(Catalog catalog) { private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigInfo) { String connectionType = connectionConfigInfo.getConnectionType().name(); - List legacySupportedConnectionTypes = - callContext - .getPolarisCallContext() - .getConfigurationStore() - .getConfiguration( - callContext.getRealmContext(), - FeatureConfiguration.SUPPORTED_CATALOG_CONNECTION_TYPES) - .stream() - .map(s -> s.toUpperCase(Locale.ROOT)) - .toList(); List supportedConnectionTypes = callContext .getPolarisCallContext() @@ -206,8 +196,7 @@ private void validateConnectionConfigInfo(ConnectionConfigInfo connectionConfigI .stream() .map(s -> s.toUpperCase(Locale.ROOT)) .toList(); - if (!supportedConnectionTypes.contains(connectionType) - && !legacySupportedConnectionTypes.contains(connectionType)) { + if (!supportedConnectionTypes.contains(connectionType)) { throw new IllegalStateException("Unsupported connection type: " + connectionType); } } diff --git a/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java b/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java index c4d92ee4ac..347096fba5 100644 --- a/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java +++ b/service/common/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplTest.java @@ -75,9 +75,6 @@ void setUp() { when(callContext.getPolarisCallContext()).thenReturn(polarisCallContext); when(callContext.getRealmContext()).thenReturn(realmContext); when(polarisCallContext.getConfigurationStore()).thenReturn(configurationStore); - when(configurationStore.getConfiguration( - realmContext, FeatureConfiguration.SUPPORTED_CATALOG_CONNECTION_TYPES)) - .thenReturn(List.of("ICEBERG_REST")); when(configurationStore.getConfiguration( realmContext, FeatureConfiguration.SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES)) .thenReturn(List.of("ICEBERG_REST")); From 32225674f89b17c820847734c098ce3dc2dad585 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Mon, 30 Jun 2025 11:33:04 -0700 Subject: [PATCH 04/12] deprecatedKey -> legacyKey --- .../config/BehaviorChangeConfiguration.java | 4 +- .../core/config/FeatureConfiguration.java | 6 +-- .../core/config/PolarisConfiguration.java | 41 +++++++++-------- .../core/config/PolarisConfigurationTest.java | 44 +++++++++---------- 4 files changed, 47 insertions(+), 48 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java index 157a4a872c..bd5311c454 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java @@ -34,12 +34,12 @@ public class BehaviorChangeConfiguration extends PolarisConfiguration { protected BehaviorChangeConfiguration( String key, - Optional deprecatedKey, + Optional legacyKey, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { - super(key, deprecatedKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); + super(key, legacyKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); } public static final BehaviorChangeConfiguration VALIDATE_VIEW_LOCATION_OVERLAP = 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 30095f8c83..ecbb2a8ce4 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 @@ -36,12 +36,12 @@ public class FeatureConfiguration extends PolarisConfiguration { protected FeatureConfiguration( String key, - Optional deprecatedKey, + Optional legacyKey, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { - super(key, deprecatedKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); + super(key, legacyKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); } /** @@ -268,7 +268,7 @@ public static void enforceFeatureEnabledOrThrow( SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES = PolarisConfiguration.>builder() .key("SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES") - .deprecatedKey("SUPPORTED_CATALOG_CONNECTION_TYPES") + .legacyKey("SUPPORTED_CATALOG_CONNECTION_TYPES") .description("The list of supported external catalog connection types for federation") .defaultValue(List.of(ConnectionType.ICEBERG_REST.name())) .buildFeatureConfiguration(); diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index 4440ce533b..51faa65a1f 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -41,7 +41,7 @@ public abstract class PolarisConfiguration { public final String key; public final String description; - public final Optional deprecatedKey; + public final Optional legacyKey; public final T defaultValue; private final Optional catalogConfigImpl; private final Optional catalogConfigUnsafeImpl; @@ -57,20 +57,19 @@ public abstract class PolarisConfiguration { private static void registerConfiguration(PolarisConfiguration configuration) { for (PolarisConfiguration existingConfiguration : allConfigurations) { if (existingConfiguration.key.equals(configuration.key) - || (configuration.deprecatedKey.isPresent() - && existingConfiguration.key.equals(configuration.deprecatedKey.get()))) { + || (configuration.legacyKey.isPresent() + && existingConfiguration.key.equals(configuration.legacyKey.get()))) { throw new IllegalArgumentException( String.format("Config '%s' is already in use", existingConfiguration.key)); - } else if (existingConfiguration.deprecatedKey.isPresent() - && (configuration.key.equals(existingConfiguration.deprecatedKey.get()) - || (configuration.deprecatedKey.isPresent() + } else if (existingConfiguration.legacyKey.isPresent() + && (configuration.key.equals(existingConfiguration.legacyKey.get()) + || (configuration.legacyKey.isPresent() && configuration - .deprecatedKey + .legacyKey .get() - .equals(existingConfiguration.deprecatedKey.get())))) { + .equals(existingConfiguration.legacyKey.get())))) { throw new IllegalArgumentException( - String.format( - "Config '%s' is already in use", existingConfiguration.deprecatedKey.get())); + String.format("Config '%s' is already in use", existingConfiguration.legacyKey.get())); } else { var configs = Stream.of( @@ -99,13 +98,13 @@ public static List> getAllConfigurations() { @SuppressWarnings("unchecked") protected PolarisConfiguration( String key, - Optional deprecatedKey, + Optional legacyKey, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { this.key = key; - this.deprecatedKey = deprecatedKey; + this.legacyKey = legacyKey; this.description = description; this.defaultValue = defaultValue; this.catalogConfigImpl = catalogConfig; @@ -113,12 +112,12 @@ protected PolarisConfiguration( this.typ = (Class) defaultValue.getClass(); } - public boolean hasDeprecatedKey() { - return deprecatedKey.isPresent(); + public boolean hasLegacyKey() { + return legacyKey.isPresent(); } - public String deprecatedKey() { - return deprecatedKey.orElseThrow( + public String legacyKey() { + return legacyKey.orElseThrow( () -> new IllegalStateException( "Attempted to read a deprecated key from a configuration that doesn't have one.")); @@ -152,7 +151,7 @@ T cast(Object value) { public static class Builder { private String key; - private Optional deprecatedKey = Optional.empty(); + private Optional legacyKey = Optional.empty(); private String description; private T defaultValue; private Optional catalogConfig = Optional.empty(); @@ -163,8 +162,8 @@ public Builder key(String key) { return this; } - public Builder deprecatedKey(String deprecatedKey) { - this.deprecatedKey = Optional.of(deprecatedKey); + public Builder legacyKey(String legacyKey) { + this.legacyKey = Optional.of(legacyKey); return this; } @@ -222,7 +221,7 @@ public FeatureConfiguration buildFeatureConfiguration() { validateOrThrow(); FeatureConfiguration config = new FeatureConfiguration<>( - key, deprecatedKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); + key, legacyKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } @@ -235,7 +234,7 @@ public BehaviorChangeConfiguration buildBehaviorChangeConfiguration() { } BehaviorChangeConfiguration config = new BehaviorChangeConfiguration<>( - key, deprecatedKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); + key, legacyKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } diff --git a/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java index b173ac86c4..a6216431b8 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java @@ -26,35 +26,35 @@ public class PolarisConfigurationTest { @Test - void testFeatureConfigurationWithDeprecatedKey() { + void testFeatureConfigurationWithLegacyKey() { FeatureConfiguration config = PolarisConfiguration.builder() - .key("FEATURE_CONFIG_WITH_DEPRECATED_KEY") - .deprecatedKey("OLD_FEATURE_CONFIG_WITH_DEPRECATED_KEY") + .key("FEATURE_CONFIG_WITH_LEGACY_KEY") + .legacyKey("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY") .description("Test configuration with deprecated key") .defaultValue(true) .buildFeatureConfiguration(); - assertThat(config.hasDeprecatedKey()).isTrue(); - assertThat(config.deprecatedKey()).isEqualTo("OLD_FEATURE_CONFIG_WITH_DEPRECATED_KEY"); - assertThat(config.key).isEqualTo("FEATURE_CONFIG_WITH_DEPRECATED_KEY"); + assertThat(config.hasLegacyKey()).isTrue(); + assertThat(config.legacyKey()).isEqualTo("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY"); + assertThat(config.key).isEqualTo("FEATURE_CONFIG_WITH_LEGACY_KEY"); assertThat(config.description).isEqualTo("Test configuration with deprecated key"); assertThat(config.defaultValue).isTrue(); } @Test - void testBehaviorChangeConfigurationWithDeprecatedKey() { + void testBehaviorChangeConfigurationWithLegacyKey() { BehaviorChangeConfiguration config = PolarisConfiguration.builder() - .key("BEHAVIOR_CONFIG_WITH_DEPRECATED_KEY") - .deprecatedKey("OLD_BEHAVIOR_CONFIG_WITH_DEPRECATED_KEY") + .key("BEHAVIOR_CONFIG_WITH_LEGACY_KEY") + .legacyKey("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY") .description("Test behavior configuration with deprecated key") .defaultValue("test-value") .buildBehaviorChangeConfiguration(); - assertThat(config.hasDeprecatedKey()).isTrue(); - assertThat(config.deprecatedKey()).isEqualTo("OLD_BEHAVIOR_CONFIG_WITH_DEPRECATED_KEY"); - assertThat(config.key).isEqualTo("BEHAVIOR_CONFIG_WITH_DEPRECATED_KEY"); + assertThat(config.hasLegacyKey()).isTrue(); + assertThat(config.legacyKey()).isEqualTo("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); + assertThat(config.key).isEqualTo("BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); assertThat(config.defaultValue).isEqualTo("test-value"); } @@ -78,10 +78,10 @@ void testDuplicateKeyValidation() { } @Test - void testDuplicateDeprecatedKeyValidation() { + void testDuplicateLegacyKeyValidation() { PolarisConfiguration.builder() .key("NEW_KEY_DEPRECATED_TEST_2") - .deprecatedKey("DEPRECATED_KEY_TEST_2") + .legacyKey("LEGACY_KEY_TEST_2") .description("First configuration with deprecated key") .defaultValue(true) .buildFeatureConfiguration(); @@ -90,19 +90,19 @@ void testDuplicateDeprecatedKeyValidation() { () -> PolarisConfiguration.builder() .key("ANOTHER_NEW_KEY_2") - .deprecatedKey("DEPRECATED_KEY_TEST_2") + .legacyKey("LEGACY_KEY_TEST_2") .description("Second configuration with same deprecated key") .defaultValue(false) .buildFeatureConfiguration()) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Config 'DEPRECATED_KEY_TEST_2' is already in use"); + .hasMessage("Config 'LEGACY_KEY_TEST_2' is already in use"); } @Test - void testNewKeyMatchingExistingDeprecatedKeyValidation() { + void testNewKeyMatchingExistingLegacyKeyValidation() { PolarisConfiguration.builder() .key("ORIGINAL_KEY_3") - .deprecatedKey("OLD_DEPRECATED_KEY_3") + .legacyKey("OLD_LEGACY_KEY_3") .description("First configuration with deprecated key") .defaultValue(true) .buildFeatureConfiguration(); @@ -110,16 +110,16 @@ void testNewKeyMatchingExistingDeprecatedKeyValidation() { assertThatThrownBy( () -> PolarisConfiguration.builder() - .key("OLD_DEPRECATED_KEY_3") + .key("OLD_LEGACY_KEY_3") .description("Configuration with key matching existing deprecated key") .defaultValue(false) .buildFeatureConfiguration()) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Config 'OLD_DEPRECATED_KEY_3' is already in use"); + .hasMessage("Config 'OLD_LEGACY_KEY_3' is already in use"); } @Test - void testDeprecatedKeyMatchingExistingKeyValidation() { + void testLegacyKeyMatchingExistingKeyValidation() { PolarisConfiguration.builder() .key("EXISTING_KEY_4") .description("First configuration") @@ -130,7 +130,7 @@ void testDeprecatedKeyMatchingExistingKeyValidation() { () -> PolarisConfiguration.builder() .key("NEW_KEY_FOR_VALIDATION_4") - .deprecatedKey("EXISTING_KEY_4") + .legacyKey("EXISTING_KEY_4") .description("Configuration with deprecated key matching existing key") .defaultValue(false) .buildFeatureConfiguration()) From 0236466dd39657d0c24e7d98f4af2000724e5133 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Mon, 30 Jun 2025 11:36:52 -0700 Subject: [PATCH 05/12] rename in application.properties --- runtime/defaults/src/main/resources/application.properties | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/runtime/defaults/src/main/resources/application.properties b/runtime/defaults/src/main/resources/application.properties index b8d42b9dd7..133b782505 100644 --- a/runtime/defaults/src/main/resources/application.properties +++ b/runtime/defaults/src/main/resources/application.properties @@ -111,8 +111,7 @@ polaris.realm-context.require-header=false polaris.features."ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING"=false polaris.features."SUPPORTED_CATALOG_STORAGE_TYPES"=["S3","GCS","AZURE"] # polaris.features."ENABLE_CATALOG_FEDERATION"=true -polaris.features."SUPPORTED_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] -polaris.features."ENABLE_EXTERNAL_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] +polaris.features."SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES"=["ICEBERG_REST"] polaris.features."SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES"=["OAUTH", "BEARER"] # realm overrides From c51edb72f0c30ce0c91168ac6f8ab004c207af88 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Mon, 30 Jun 2025 12:48:56 -0700 Subject: [PATCH 06/12] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0c93053a2..51809a8d8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti ### Changes ### Deprecations +- Deprecates the `SUPPORTED_CATALOG_CONNECTION_TYPES` and replaces it with `SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES`. ### Fixes From 9e79d080bdbfb681ded4ed182d3bf0bceb328269 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Tue, 1 Jul 2025 09:59:27 -0700 Subject: [PATCH 07/12] Support multiple legacyKeys --- .../config/BehaviorChangeConfiguration.java | 5 +- .../core/config/FeatureConfiguration.java | 5 +- .../core/config/PolarisConfiguration.java | 58 +++++++++++-------- .../core/config/PolarisConfigurationTest.java | 20 ++++--- 4 files changed, 52 insertions(+), 36 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java index bd5311c454..d8392eee16 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java @@ -19,6 +19,7 @@ package org.apache.polaris.core.config; import java.util.Optional; +import java.util.Set; /** * Internal configuration flags for non-feature behavior changes in Polaris. These flags control @@ -34,12 +35,12 @@ public class BehaviorChangeConfiguration extends PolarisConfiguration { protected BehaviorChangeConfiguration( String key, - Optional legacyKey, + Set legacyKeys, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { - super(key, legacyKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); + super(key, legacyKeys, description, defaultValue, catalogConfig, catalogConfigUnsafe); } public static final BehaviorChangeConfiguration VALIDATE_VIEW_LOCATION_OVERLAP = 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 ecbb2a8ce4..06fd98164b 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 @@ -20,6 +20,7 @@ import java.util.List; import java.util.Optional; +import java.util.Set; import org.apache.polaris.core.admin.model.AuthenticationParameters; import org.apache.polaris.core.admin.model.StorageConfigInfo; import org.apache.polaris.core.connection.ConnectionType; @@ -36,12 +37,12 @@ public class FeatureConfiguration extends PolarisConfiguration { protected FeatureConfiguration( String key, - Optional legacyKey, + Set legacyKeys, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { - super(key, legacyKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); + super(key, legacyKeys, description, defaultValue, catalogConfig, catalogConfigUnsafe); } /** diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index 51faa65a1f..9a7499ec47 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -19,8 +19,10 @@ package org.apache.polaris.core.config; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -41,7 +43,7 @@ public abstract class PolarisConfiguration { public final String key; public final String description; - public final Optional legacyKey; + public final Set legacyKeys; public final T defaultValue; private final Optional catalogConfigImpl; private final Optional catalogConfigUnsafeImpl; @@ -57,19 +59,21 @@ public abstract class PolarisConfiguration { private static void registerConfiguration(PolarisConfiguration configuration) { for (PolarisConfiguration existingConfiguration : allConfigurations) { if (existingConfiguration.key.equals(configuration.key) - || (configuration.legacyKey.isPresent() - && existingConfiguration.key.equals(configuration.legacyKey.get()))) { + || configuration.legacyKeys.contains(existingConfiguration.key)) { throw new IllegalArgumentException( String.format("Config '%s' is already in use", existingConfiguration.key)); - } else if (existingConfiguration.legacyKey.isPresent() - && (configuration.key.equals(existingConfiguration.legacyKey.get()) - || (configuration.legacyKey.isPresent() - && configuration - .legacyKey - .get() - .equals(existingConfiguration.legacyKey.get())))) { + } else if (existingConfiguration.legacyKeys.contains(configuration.key)) { throw new IllegalArgumentException( - String.format("Config '%s' is already in use", existingConfiguration.legacyKey.get())); + String.format("Config '%s' is already in use", configuration.key)); + } else if (!configuration.legacyKeys.isEmpty()) { + Set legacyKeys = new HashSet<>(existingConfiguration.legacyKeys); + legacyKeys.retainAll(configuration.legacyKeys); + if (!legacyKeys.isEmpty()) { + throw new IllegalArgumentException( + String.format( + "Config '%s' is already in use", + legacyKeys.stream().collect(Collectors.joining(",")))); + } } else { var configs = Stream.of( @@ -98,13 +102,13 @@ public static List> getAllConfigurations() { @SuppressWarnings("unchecked") protected PolarisConfiguration( String key, - Optional legacyKey, + Set legacyKeys, String description, T defaultValue, Optional catalogConfig, Optional catalogConfigUnsafe) { this.key = key; - this.legacyKey = legacyKey; + this.legacyKeys = legacyKeys; this.description = description; this.defaultValue = defaultValue; this.catalogConfigImpl = catalogConfig; @@ -112,15 +116,16 @@ protected PolarisConfiguration( this.typ = (Class) defaultValue.getClass(); } - public boolean hasLegacyKey() { - return legacyKey.isPresent(); + public boolean hasLegacyKeys() { + return !legacyKeys.isEmpty(); } - public String legacyKey() { - return legacyKey.orElseThrow( - () -> - new IllegalStateException( - "Attempted to read a deprecated key from a configuration that doesn't have one.")); + public String legacyKeys() { + if (legacyKeys.isEmpty()) { + throw new IllegalStateException( + "Attempted to read legacy keys from a configuration that doesn't have any."); + } + return legacyKeys.stream().collect(Collectors.joining(",")); } public boolean hasCatalogConfig() { @@ -151,7 +156,7 @@ T cast(Object value) { public static class Builder { private String key; - private Optional legacyKey = Optional.empty(); + private Set legacyKeys = new HashSet<>(); private String description; private T defaultValue; private Optional catalogConfig = Optional.empty(); @@ -163,7 +168,12 @@ public Builder key(String key) { } public Builder legacyKey(String legacyKey) { - this.legacyKey = Optional.of(legacyKey); + legacyKeys.add(legacyKey); + return this; + } + + public Builder legacyKeys(Set legacyKeys) { + this.legacyKeys.addAll(legacyKeys); return this; } @@ -221,7 +231,7 @@ public FeatureConfiguration buildFeatureConfiguration() { validateOrThrow(); FeatureConfiguration config = new FeatureConfiguration<>( - key, legacyKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); + key, legacyKeys, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } @@ -234,7 +244,7 @@ public BehaviorChangeConfiguration buildBehaviorChangeConfiguration() { } BehaviorChangeConfiguration config = new BehaviorChangeConfiguration<>( - key, legacyKey, description, defaultValue, catalogConfig, catalogConfigUnsafe); + key, legacyKeys, description, defaultValue, catalogConfig, catalogConfigUnsafe); PolarisConfiguration.registerConfiguration(config); return config; } diff --git a/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java index a6216431b8..02b64600de 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java @@ -35,8 +35,8 @@ void testFeatureConfigurationWithLegacyKey() { .defaultValue(true) .buildFeatureConfiguration(); - assertThat(config.hasLegacyKey()).isTrue(); - assertThat(config.legacyKey()).isEqualTo("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY"); + assertThat(config.hasLegacyKeys()).isTrue(); + assertThat(config.legacyKeys()).isEqualTo("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY"); assertThat(config.key).isEqualTo("FEATURE_CONFIG_WITH_LEGACY_KEY"); assertThat(config.description).isEqualTo("Test configuration with deprecated key"); assertThat(config.defaultValue).isTrue(); @@ -52,8 +52,8 @@ void testBehaviorChangeConfigurationWithLegacyKey() { .defaultValue("test-value") .buildBehaviorChangeConfiguration(); - assertThat(config.hasLegacyKey()).isTrue(); - assertThat(config.legacyKey()).isEqualTo("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); + assertThat(config.hasLegacyKeys()).isTrue(); + assertThat(config.legacyKeys()).isEqualTo("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); assertThat(config.key).isEqualTo("BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); assertThat(config.defaultValue).isEqualTo("test-value"); } @@ -78,10 +78,12 @@ void testDuplicateKeyValidation() { } @Test - void testDuplicateLegacyKeyValidation() { + void testDuplicateLegacyKeysValidation() { PolarisConfiguration.builder() .key("NEW_KEY_DEPRECATED_TEST_2") - .legacyKey("LEGACY_KEY_TEST_2") + .legacyKey("LEGACY_KEY_TEST_2_1") + .legacyKey("LEGACY_KEY_TEST_2_2") + .legacyKey("LEGACY_KEY_TEST_2_3") .description("First configuration with deprecated key") .defaultValue(true) .buildFeatureConfiguration(); @@ -90,12 +92,13 @@ void testDuplicateLegacyKeyValidation() { () -> PolarisConfiguration.builder() .key("ANOTHER_NEW_KEY_2") - .legacyKey("LEGACY_KEY_TEST_2") + .legacyKey("LEGACY_KEY_TEST_2_1") + .legacyKey("LEGACY_KEY_TEST_2_3") .description("Second configuration with same deprecated key") .defaultValue(false) .buildFeatureConfiguration()) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Config 'LEGACY_KEY_TEST_2' is already in use"); + .hasMessage("Config 'LEGACY_KEY_TEST_2_3,LEGACY_KEY_TEST_2_1' is already in use"); } @Test @@ -131,6 +134,7 @@ void testLegacyKeyMatchingExistingKeyValidation() { PolarisConfiguration.builder() .key("NEW_KEY_FOR_VALIDATION_4") .legacyKey("EXISTING_KEY_4") + .legacyKey("EXISTING_KEY_4_1") .description("Configuration with deprecated key matching existing key") .defaultValue(false) .buildFeatureConfiguration()) From fa45fde59759619b9ff3f39b9f77bf7ec3c90cb9 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Wed, 2 Jul 2025 10:55:04 -0700 Subject: [PATCH 08/12] Update CHANGELOG.md Co-authored-by: Dmitri Bourlatchkov --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51809a8d8a..e4842b816b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti ### Changes ### Deprecations -- Deprecates the `SUPPORTED_CATALOG_CONNECTION_TYPES` and replaces it with `SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES`. +- Deprecates the `SUPPORTED_CATALOG_CONNECTION_TYPES` configuration property and replaces it with `SUPPORTED_EXTERNAL_CATALOG_CONNECTION_TYPES`. ### Fixes From 2147b66bd2887873a62f358f89c3e4a6259154c9 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Wed, 2 Jul 2025 13:39:07 -0700 Subject: [PATCH 09/12] Support lookups based on legacyKeys --- .../config/PolarisConfigurationStore.java | 20 +++- .../PolarisConfigurationStoreTest.java | 107 +++++++++++++++++- 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index 4923d97eea..acd65c2216 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -99,7 +99,25 @@ public interface PolarisConfigurationStore { */ default @Nonnull T getConfiguration( @Nonnull RealmContext realmContext, PolarisConfiguration config) { - T result = getConfiguration(realmContext, config.key, config.defaultValue); + T result = getConfiguration(realmContext, config.key); + + if (result == null && config.hasLegacyKeys()) { + for (String legacyKey : config.legacyKeys) { + result = getConfiguration(realmContext, legacyKey); + if (result != null) { + LOGGER.warn( + "Legacy configuration key '{}' is in use. Please use '{}' instead.", + legacyKey, + config.key); + break; + } + } + } + + if (result == null) { + result = config.defaultValue; + } + return tryCast(config, result); } diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java index 4b190d3d15..e6ffffa58d 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java @@ -19,7 +19,6 @@ package org.apache.polaris.service.storage; import jakarta.annotation.Nonnull; -import jakarta.annotation.Nullable; import java.util.List; import java.util.Map; import java.util.function.Function; @@ -54,7 +53,7 @@ public void testConfigsCanBeCastedFromString() { */ @SuppressWarnings("unchecked") @Override - public @Nullable T getConfiguration(@Nonnull RealmContext ctx, String configName) { + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { for (PolarisConfiguration c : configs) { if (c.key.equals(configName)) { return (T) String.valueOf(c.defaultValue); @@ -163,9 +162,9 @@ public void testEntityOverrides() { PolarisConfigurationStore store = new PolarisConfigurationStore() { + @SuppressWarnings("unchecked") @Override - public @Nullable T getConfiguration( - @Nonnull RealmContext realmContext, String configName) { + public T getConfiguration(@Nonnull RealmContext realmContext, String configName) { //noinspection unchecked return (T) Map.of("key2", "config-value2").get(configName); } @@ -186,4 +185,104 @@ public void testEntityOverrides() { Assertions.assertEquals( "entity-legacy4", store.getConfiguration(testRealmContext, entity, cfg.apply(4))); } + + @Test + public void testLegacyKeyLookup() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("LEGACY_KEY_TEST_NEW_KEY") + .legacyKey("LEGACY_TEST_OLD_KEY_1") + .legacyKey("LEGACY_TEST_OLD_KEY_2") + .description("Test config with legacy keys") + .defaultValue("default-value") + .buildFeatureConfiguration(); + + PolarisConfigurationStore store = + new PolarisConfigurationStore() { + @SuppressWarnings("unchecked") + @Override + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { + Map values = Map.of("LEGACY_TEST_OLD_KEY_1", "legacy-value-1"); + return (T) values.get(configName); + } + }; + + String result = store.getConfiguration(testRealmContext, config); + Assertions.assertEquals("legacy-value-1", result); + } + + @Test + public void testMainKeyTakesPrecedenceOverLegacyKeys() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("PRECEDENCE_TEST_NEW_KEY") + .legacyKey("PRECEDENCE_TEST_OLD_KEY") + .description("Test config with legacy keys") + .defaultValue("default-value") + .buildFeatureConfiguration(); + + PolarisConfigurationStore store = + new PolarisConfigurationStore() { + @SuppressWarnings("unchecked") + @Override + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { + Map values = + Map.of( + "PRECEDENCE_TEST_NEW_KEY", "new-value", + "PRECEDENCE_TEST_OLD_KEY", "legacy-value"); + return (T) values.get(configName); + } + }; + + String result = store.getConfiguration(testRealmContext, config); + Assertions.assertEquals("new-value", result); + } + + @Test + public void testFallbackToDefaultWhenNoKeysFound() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("FALLBACK_TEST_NEW_KEY") + .legacyKey("FALLBACK_TEST_OLD_KEY") + .description("Test config with legacy keys") + .defaultValue("default-value") + .buildFeatureConfiguration(); + + PolarisConfigurationStore store = + new PolarisConfigurationStore() { + @Override + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { + return null; + } + }; + + String result = store.getConfiguration(testRealmContext, config); + Assertions.assertEquals("default-value", result); + } + + @Test + public void testLegacyKeyWarningIsLogged() { + FeatureConfiguration config = + PolarisConfiguration.builder() + .key("WARNING_TEST_NEW_KEY") + .legacyKey("WARNING_TEST_LEGACY_KEY") + .description("Test config for legacy key warning") + .defaultValue("default-value") + .buildFeatureConfiguration(); + + PolarisConfigurationStore store = + new PolarisConfigurationStore() { + @SuppressWarnings("unchecked") + @Override + public T getConfiguration(@Nonnull RealmContext ctx, String configName) { + if ("WARNING_TEST_LEGACY_KEY".equals(configName)) { + return (T) "legacy-value"; + } + return null; + } + }; + + String result = store.getConfiguration(testRealmContext, config); + Assertions.assertEquals("legacy-value", result); + } } From 6087435613ce13d48ed4fdaca5448593974eb43c Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Wed, 2 Jul 2025 14:10:34 -0700 Subject: [PATCH 10/12] Remove incorect WARN test --- .../PolarisConfigurationStoreTest.java | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java b/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java index e6ffffa58d..dc1ad6235b 100644 --- a/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java @@ -259,30 +259,4 @@ public T getConfiguration(@Nonnull RealmContext ctx, String configName) { String result = store.getConfiguration(testRealmContext, config); Assertions.assertEquals("default-value", result); } - - @Test - public void testLegacyKeyWarningIsLogged() { - FeatureConfiguration config = - PolarisConfiguration.builder() - .key("WARNING_TEST_NEW_KEY") - .legacyKey("WARNING_TEST_LEGACY_KEY") - .description("Test config for legacy key warning") - .defaultValue("default-value") - .buildFeatureConfiguration(); - - PolarisConfigurationStore store = - new PolarisConfigurationStore() { - @SuppressWarnings("unchecked") - @Override - public T getConfiguration(@Nonnull RealmContext ctx, String configName) { - if ("WARNING_TEST_LEGACY_KEY".equals(configName)) { - return (T) "legacy-value"; - } - return null; - } - }; - - String result = store.getConfiguration(testRealmContext, config); - Assertions.assertEquals("legacy-value", result); - } } From 14076b58ece88f9abf333e21c63127aa91024153 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Thu, 3 Jul 2025 17:48:48 -0700 Subject: [PATCH 11/12] Address review comments --- .../apache/polaris/core/config/PolarisConfiguration.java | 8 ++------ .../polaris/core/config/PolarisConfigurationStore.java | 2 +- .../polaris/core/config/PolarisConfigurationTest.java | 4 ++-- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java index 9a7499ec47..f7ba93f6c7 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java @@ -120,12 +120,8 @@ public boolean hasLegacyKeys() { return !legacyKeys.isEmpty(); } - public String legacyKeys() { - if (legacyKeys.isEmpty()) { - throw new IllegalStateException( - "Attempted to read legacy keys from a configuration that doesn't have any."); - } - return legacyKeys.stream().collect(Collectors.joining(",")); + public Set legacyKeys() { + return legacyKeys; } public boolean hasCatalogConfig() { diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index acd65c2216..6c1c9d9a23 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -102,7 +102,7 @@ public interface PolarisConfigurationStore { T result = getConfiguration(realmContext, config.key); if (result == null && config.hasLegacyKeys()) { - for (String legacyKey : config.legacyKeys) { + for (String legacyKey : config.legacyKeys()) { result = getConfiguration(realmContext, legacyKey); if (result != null) { LOGGER.warn( diff --git a/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java index 02b64600de..56bfd028f0 100644 --- a/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java +++ b/polaris-core/src/test/java/org/apache/polaris/core/config/PolarisConfigurationTest.java @@ -36,7 +36,7 @@ void testFeatureConfigurationWithLegacyKey() { .buildFeatureConfiguration(); assertThat(config.hasLegacyKeys()).isTrue(); - assertThat(config.legacyKeys()).isEqualTo("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY"); + assertThat(config.legacyKeys().contains("OLD_FEATURE_CONFIG_WITH_LEGACY_KEY")).isTrue(); assertThat(config.key).isEqualTo("FEATURE_CONFIG_WITH_LEGACY_KEY"); assertThat(config.description).isEqualTo("Test configuration with deprecated key"); assertThat(config.defaultValue).isTrue(); @@ -53,7 +53,7 @@ void testBehaviorChangeConfigurationWithLegacyKey() { .buildBehaviorChangeConfiguration(); assertThat(config.hasLegacyKeys()).isTrue(); - assertThat(config.legacyKeys()).isEqualTo("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); + assertThat(config.legacyKeys().contains("OLD_BEHAVIOR_CONFIG_WITH_LEGACY_KEY")).isTrue(); assertThat(config.key).isEqualTo("BEHAVIOR_CONFIG_WITH_LEGACY_KEY"); assertThat(config.defaultValue).isEqualTo("test-value"); } From d99408a4584f3d5f35f2a62521c879ec9f5a17d8 Mon Sep 17 00:00:00 2001 From: Pooja Nilangekar Date: Mon, 7 Jul 2025 12:04:37 -0700 Subject: [PATCH 12/12] Log WARNINGS about multiple legacy keys --- .../polaris/core/config/PolarisConfigurationStore.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java index 6c1c9d9a23..0bbde0548f 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java @@ -103,13 +103,15 @@ public interface PolarisConfigurationStore { if (result == null && config.hasLegacyKeys()) { for (String legacyKey : config.legacyKeys()) { - result = getConfiguration(realmContext, legacyKey); - if (result != null) { + T legacyResult = getConfiguration(realmContext, legacyKey); + if (legacyResult != null) { LOGGER.warn( "Legacy configuration key '{}' is in use. Please use '{}' instead.", legacyKey, config.key); - break; + if (result == null) { + result = legacyResult; + } } } }