Skip to content

Refactor PolarisConfigurationStore to streamline value typing #3324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dimas-b wants to merge 2 commits into from
Closed

Refactor PolarisConfigurationStore to streamline value typing #3324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f292b0
Refactor PolarisConfigurationStore to streamline value typing
dimas-b Dec 24, 2025
a71fbcb
update javadoc
dimas-b Jan 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,27 @@ public interface PolarisConfigurationStore {
/**
* Retrieve the current value for a configuration key for a given realm. May be null if not set.
*
* <p>This method is meant to be overridden by concrete configuration store implementations.

@snazy snazy Jan 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I wonder whether this function and the getConfiguration functions should have a default implementation at all. Wouldn't it be cleaner to move that to the actual implementation? Mean, this interface isn't really an interface but rather a not-really abstract class.

@dimas-b dimas-b Jan 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've been thinking about splitting this into an API/SPI pair too, but I thought it might be too intrusive... Let me actually try that refactoring and see what's impacted.

@dimas-b dimas-b Jan 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prerequisite refactoring: #3412

*
* <p>This method is <strong>not</strong> meant to be called by code that needs access to
* configuration values. Configuration consumers should call typed methods that take a {@link
* PolarisConfiguration} parameter instead.
*
* @param realmContext the realm context
* @param configName the name of the configuration key to check
* @return the current value set for the configuration key for the given realm, or null if not set
* @param <T> the type of the configuration value
*/
default @Nullable Object getConfigValue(@Nonnull RealmContext realmContext, String configName) {
return getConfiguration(realmContext, configName);
}

/**
* Legacy method for getting current value for a configuration key.
*
* @deprecated Call / override {@link #getConfigValue(RealmContext, String)} instead.
*/
@Deprecated
@SuppressWarnings("DeprecatedIsStillUsed")
default <T> @Nullable T getConfiguration(@Nonnull RealmContext realmContext, String configName) {
return null;
}
Expand All @@ -61,7 +77,8 @@ public interface PolarisConfigurationStore {
default <T> @Nonnull T getConfiguration(
@Nonnull RealmContext realmContext, String configName, @Nonnull T defaultValue) {
Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value");
T configValue = getConfiguration(realmContext, configName);
@SuppressWarnings("unchecked")
T configValue = (T) getConfigValue(realmContext, configName);
return configValue != null ? configValue : defaultValue;
}

Expand Down
4 changes: 3 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/config/RealmConfigImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ public RealmConfigImpl(PolarisConfigurationStore configurationStore, RealmContex

@Override
public <T> @Nullable T getConfig(String configName) {
return configurationStore.getConfiguration(realmContext, configName);
@SuppressWarnings("unchecked")
T configValue = (T) configurationStore.getConfigValue(realmContext, configName);
return configValue;
}

@Override
Expand Down
6 changes: 2 additions & 4 deletions ...is-core/src/test/java/org/apache/polaris/core/storage/InMemoryStorageIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,8 @@ public MockedConfigurationStore(Map<String, Object> defaults) {
}

@Override
public <T> @Nullable T getConfiguration(@Nonnull RealmContext realmContext, String configName) {
@SuppressWarnings("unchecked")
T confgValue = (T) defaults.get(configName);
return confgValue;
public @Nullable Object getConfigValue(@Nonnull RealmContext realmContext, String configName) {
return defaults.get(configName);
}
}
}
3 changes: 1 addition & 2 deletions ...-core/src/test/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,8 @@ public void testCacheMissForAnotherPrincipal() {
.thenReturn(
new RealmConfigImpl(
new PolarisConfigurationStore() {
@SuppressWarnings("unchecked")
@Override
public String getConfiguration(@Nonnull RealmContext ctx, String configName) {
public Object getConfigValue(@Nonnull RealmContext ctx, String configName) {
if (configName.equals(
FeatureConfiguration.INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL
.key())) {
Expand Down
15 changes: 6 additions & 9 deletions ...-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,11 @@ public void testConfigsCanBeCastedFromString() {
* Ad-hoc configuration store implementation that just returns the stringified version of
* the config's default value
*/
@SuppressWarnings("unchecked")
@Override
public <T> @Nullable T getConfiguration(@Nonnull RealmContext ctx, String configName) {
public Object getConfigValue(@Nonnull RealmContext ctx, String configName) {
for (PolarisConfiguration<?> c : configs) {
if (c.key().equals(configName)) {
return (T) String.valueOf(c.defaultValue());
return String.valueOf(c.defaultValue());
}
}

Expand All @@ -83,10 +82,9 @@ public void testInvalidCastThrowsException() {

PolarisConfigurationStore store =
new PolarisConfigurationStore() {
@SuppressWarnings("unchecked")
@Override
public <T> T getConfiguration(@Nonnull RealmContext ctx, String configName) {
return (T) "abc123";
public Object getConfigValue(@Nonnull RealmContext ctx, String configName) {
return "abc123";
}
};

Expand Down Expand Up @@ -164,10 +162,9 @@ public void testEntityOverrides() {
PolarisConfigurationStore store =
new PolarisConfigurationStore() {
@Override
public <T> @Nullable T getConfiguration(
public @Nullable Object getConfigValue(
@Nonnull RealmContext realmContext, String configName) {
//noinspection unchecked
return (T) Map.of("key2", "config-value2").get(configName);
return Map.of("key2", "config-value2").get(configName);
}
};

Expand Down
4 changes: 2 additions & 2 deletions ...est/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo;
import org.assertj.core.api.Assertions;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.jspecify.annotations.Nullable;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
Expand All @@ -62,9 +63,8 @@ class AwsCredentialsStorageIntegrationTest extends BaseStorageIntegrationTest {
public static final RealmConfig PRINCIPAL_INCLUDER_REALM_CONFIG =
new RealmConfigImpl(
new PolarisConfigurationStore() {
@SuppressWarnings("unchecked")
@Override
public String getConfiguration(@Nonnull RealmContext ctx, String configName) {
public @Nullable Object getConfigValue(@Nonnull RealmContext ctx, String configName) {
if (configName.equals(
FeatureConfiguration.INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL.key())) {
return "true";
Expand Down
16 changes: 5 additions & 11 deletions ...me/service/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,20 +47,14 @@ public DefaultConfigurationStore(
}

@Override
public <T> @Nullable T getConfiguration(@Nonnull RealmContext realmContext, String configName) {
public @Nullable Object getConfigValue(@Nonnull RealmContext realmContext, String configName) {
String realm = realmContext.getRealmIdentifier();
LOGGER.debug("Get configuration value for {} with realm {}", configName, realm);
@SuppressWarnings("unchecked")
T confgValue =
(T)
Optional.ofNullable(realmOverrides.getOrDefault(realm, Map.of()).get(configName))
.orElseGet(() -> getDefaultConfiguration(configName));
return confgValue;
return Optional.ofNullable(realmOverrides.getOrDefault(realm, Map.of()).get(configName))
.orElseGet(() -> getDefaultConfiguration(configName));
}

private <T> @Nullable T getDefaultConfiguration(String configName) {
@SuppressWarnings("unchecked")
T confgValue = (T) defaults.get(configName);
return confgValue;
private @Nullable Object getDefaultConfiguration(String configName) {
return defaults.get(configName);
}
}
54 changes: 26 additions & 28 deletions ...ervice/src/test/java/org/apache/polaris/service/config/DefaultConfigurationStoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.apache.polaris.service.config;

import static java.lang.Boolean.FALSE;
import static java.lang.Boolean.TRUE;
import static org.assertj.core.api.Assertions.assertThat;

import io.quarkus.test.junit.QuarkusTest;
Expand Down Expand Up @@ -83,74 +85,70 @@ public void before(TestInfo testInfo) {

@Test
public void testGetConfiguration() {
Object value = configurationStore.getConfiguration(realmContext, "missingKeyWithoutDefault");
Object value = configurationStore.getConfigValue(realmContext, "missingKeyWithoutDefault");
assertThat(value).isNull();
Object defaultValue =
configurationStore.getConfiguration(realmContext, "missingKeyWithDefault", "defaultValue");
assertThat(defaultValue).isEqualTo("defaultValue");

// the falseByDefaultKey is set to false for all realms in Profile.getConfigOverrides
assertThat((Boolean) configurationStore.getConfiguration(realmContext, falseByDefaultKey))
.isFalse();
assertThat(configurationStore.getConfigValue(realmContext, falseByDefaultKey)).isEqualTo(FALSE);
// the trueByDefaultKey is set to true for all realms in Profile.getConfigOverrides
assertThat((Boolean) configurationStore.getConfiguration(realmContext, trueByDefaultKey))
.isTrue();
assertThat(configurationStore.getConfigValue(realmContext, trueByDefaultKey)).isEqualTo(TRUE);
}

@Test
public void testGetRealmConfiguration() {
// check the realmOne configuration
// the falseByDefaultKey is set to `false` for all realms, but overwrite with value `true` for
// realmOne.
assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, falseByDefaultKey))
.isTrue();
assertThat(configurationStore.getConfigValue(realmOneContext, falseByDefaultKey))
.isEqualTo(TRUE);
// the trueByDefaultKey is set to `false` for all realms, no overwrite for realmOne
assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, trueByDefaultKey))
.isTrue();
assertThat(configurationStore.getConfigValue(realmOneContext, trueByDefaultKey))
.isEqualTo(TRUE);

// check the realmTwo configuration
// the falseByDefaultKey is set to `false` for all realms, no overwrite for realmTwo
assertThat((Boolean) configurationStore.getConfiguration(realmTwoContext, falseByDefaultKey))
.isFalse();
assertThat(configurationStore.getConfigValue(realmTwoContext, falseByDefaultKey))
.isEqualTo(FALSE);
// the trueByDefaultKey is set to `false` for all realms, and overwrite with value `false` for
// realmTwo
assertThat((Boolean) configurationStore.getConfiguration(realmTwoContext, trueByDefaultKey))
.isFalse();
assertThat(configurationStore.getConfigValue(realmTwoContext, trueByDefaultKey))
.isEqualTo(FALSE);
}

@Test
void testGetConfigurationWithRealm() {
// the falseByDefaultKey is set to `false` for all realms, but overwrite with value `true` for
// realmOne.
assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, falseByDefaultKey))
.isTrue();
assertThat(configurationStore.getConfigValue(realmOneContext, falseByDefaultKey))
.isEqualTo(TRUE);
// the trueByDefaultKey is set to `false` for all realms, no overwrite for realmOne
assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, trueByDefaultKey))
.isTrue();
assertThat(configurationStore.getConfigValue(realmOneContext, trueByDefaultKey))
.isEqualTo(TRUE);

// the falseByDefaultKey is set to `false` for all realms, no overwrite for realmTwo
assertThat((Boolean) configurationStore.getConfiguration(realmTwoContext, falseByDefaultKey))
.isFalse();
assertThat(configurationStore.getConfigValue(realmTwoContext, falseByDefaultKey))
.isEqualTo(FALSE);
// the trueByDefaultKey is set to `false` for all realms, and overwrite with value `false` for
// realmTwo
assertThat((Boolean) configurationStore.getConfiguration(realmTwoContext, trueByDefaultKey))
.isFalse();
assertThat(configurationStore.getConfigValue(realmTwoContext, trueByDefaultKey))
.isEqualTo(FALSE);
}

@Test
public void testInjectedConfigurationStore() {
// the default value for trueByDefaultKey is `true`
Boolean featureDefaultValue =
configurationStore.getConfiguration(realmContext, trueByDefaultKey);
assertThat(featureDefaultValue).isTrue();
assertThat(configurationStore.getConfigValue(realmContext, trueByDefaultKey)).isEqualTo(TRUE);

// the value for falseByDefaultKey is `false`, and no realm override for realmTwo
Boolean realmTwoValue = configurationStore.getConfiguration(realmTwoContext, falseByDefaultKey);
assertThat(realmTwoValue).isFalse();
assertThat(configurationStore.getConfigValue(realmTwoContext, falseByDefaultKey))
.isEqualTo(FALSE);

// Now, realmOne override falseByDefaultKey to `True`
Boolean realmOneValue = configurationStore.getConfiguration(realmOneContext, falseByDefaultKey);
assertThat(realmOneValue).isTrue();
assertThat(configurationStore.getConfigValue(realmOneContext, falseByDefaultKey))
.isEqualTo(TRUE);

assertThat(configurationStore).isInstanceOf(DefaultConfigurationStore.class);
}
Expand Down
6 changes: 2 additions & 4 deletions runtime/service/src/testFixtures/java/org/apache/polaris/service/TestServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,8 @@ public MockedConfigurationStore(Map<String, Object> defaults) {
}

@Override
public <T> @Nullable T getConfiguration(@Nonnull RealmContext realmContext, String configName) {
@SuppressWarnings("unchecked")
T confgValue = (T) defaults.get(configName);
return confgValue;
public @Nullable Object getConfigValue(@Nonnull RealmContext realmContext, String configName) {
return defaults.get(configName);
}
}

Expand Down