Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,7 @@ public void authorizeOrThrow(
@Nullable List<PolarisResolvedPathWrapper> secondaries) {
boolean enforceCredentialRotationRequiredState =
featureConfig.getConfiguration(
callContext.getPolarisCallContext(),
callContext.getRealmContext(),
FeatureConfiguration.ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING);
if (enforceCredentialRotationRequiredState
&& authenticatedPrincipal
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public static void enforceFeatureEnabledOrThrow(
callContext
.getPolarisCallContext()
.getConfigurationStore()
.getConfiguration(callContext.getPolarisCallContext(), featureConfig);
.getConfiguration(callContext.getRealmContext(), featureConfig);
if (!enabled) {
throw new UnsupportedOperationException("Feature not enabled: " + featureConfig.key);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ public static <T> T loadConfig(PolarisConfiguration<T> configuration) {
return callContext
.getPolarisCallContext()
.getConfigurationStore()
.getConfiguration(callContext.getPolarisCallContext(), configuration);
.getConfiguration(callContext.getRealmContext(), configuration);
}

public static <T> Builder<T> builder() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,32 +133,31 @@ public interface PolarisConfigurationStore {
}

/**
* Retrieve the current value for a configuration. TODO: update the function to take RealmContext
* instead of PolarisCallContext. Github issue https://github.com/apache/polaris/issues/1775
* Retrieve the current value for a configuration.
*
* @param ctx the current call context
* @param realmContext the current realm context
* @param config the configuration to load
* @return the current value set for the configuration key or null if not set
* @param <T> the type of the configuration value
*/
default <T> @Nonnull T getConfiguration(PolarisCallContext ctx, PolarisConfiguration<T> config) {
T result = getConfiguration(ctx, config.key, config.defaultValue);
default <T> @Nonnull T getConfiguration(
RealmContext realmContext, PolarisConfiguration<T> config) {
T result = getConfiguration(realmContext, config.key, config.defaultValue);
return tryCast(config, result);
}

/**
* Retrieve the current value for a configuration, overriding with a catalog config if it is
* present. TODO: update the function to take RealmContext instead of PolarisCallContext Github
* issue https://github.com/apache/polaris/issues/1775
* present.
*
* @param ctx the current call context
* @param realmContext the current realm context
* @param catalogEntity the catalog to check for an override
* @param config the configuration to load
* @return the current value set for the configuration key or null if not set
* @param <T> the type of the configuration value
*/
default <T> @Nonnull T getConfiguration(
PolarisCallContext ctx,
RealmContext realmContext,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why not keep the old param name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think realmContext is a better parameter name that indicates clearly what the parameter is. "ctx" is very confusing by during the read of code like whether it is for CallContext or PolarisCallContext.

@Nonnull CatalogEntity catalogEntity,
PolarisConfiguration<T> config) {
if (config.hasCatalogConfig() || config.hasCatalogConfigUnsafe()) {
Expand All @@ -182,6 +181,6 @@ public interface PolarisConfigurationStore {
return tryCast(config, propertyValue);
}
}
return getConfiguration(ctx, config);
return getConfiguration(realmContext, config);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public static Set<Endpoint> getSupportedGenericTableEndpoints(CallContext callCo
.getPolarisCallContext()
.getConfigurationStore()
.getConfiguration(
callContext.getPolarisCallContext(), FeatureConfiguration.ENABLE_GENERIC_TABLES);
callContext.getRealmContext(), FeatureConfiguration.ENABLE_GENERIC_TABLES);

return genericTableEnabled ? GENERIC_TABLE_ENDPOINTS : ImmutableSet.of();
}
Expand All @@ -99,7 +99,7 @@ public static Set<Endpoint> getSupportedPolicyEndpoints(CallContext callContext)
.getPolarisCallContext()
.getConfigurationStore()
.getConfiguration(
callContext.getPolarisCallContext(), FeatureConfiguration.ENABLE_POLICY_STORE);
callContext.getRealmContext(), FeatureConfiguration.ENABLE_POLICY_STORE);
return policyStoreEnabled ? POLICY_STORE_ENDPOINTS : ImmutableSet.of();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,11 @@ public InMemoryStorageIntegration(String identifierOrId) {

boolean allowWildcardLocation =
Optional.ofNullable(CallContext.getCurrentContext())
.flatMap(c -> Optional.ofNullable(c.getPolarisCallContext()))
.map(
pc ->
pc.getConfigurationStore()
.getConfiguration(pc, "ALLOW_WILDCARD_LOCATION", false))
ctx ->
ctx.getPolarisCallContext()
.getConfigurationStore()
.getConfiguration(ctx.getRealmContext(), "ALLOW_WILDCARD_LOCATION", false))
Comment on lines -81 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so, because we need to "undo" the translation from CallContext to PolarisCallContext so that we can recover the RealmContext.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, didn't notice there was a pc :-)

.orElse(false);

if (allowWildcardLocation && allowedLocationStrings.contains("*")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ public static Optional<PolarisStorageConfigurationInfo> forEntityPath(
.getPolarisCallContext()
.getConfigurationStore()
.getConfiguration(
CallContext.getCurrentContext().getPolarisCallContext(),
CallContext.getCurrentContext().getRealmContext(),
catalog,
FeatureConfiguration.ALLOW_UNSTRUCTURED_TABLE_LOCATION);
if (!allowEscape
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.apache.polaris.core.PolarisDiagnostics;
import org.apache.polaris.core.config.PolarisConfigurationStore;
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -100,7 +101,7 @@ public void testValidateAccessToLocationsWithWildcard() {
new PolarisConfigurationStore() {
@SuppressWarnings("unchecked")
@Override
public <T> @Nullable T getConfiguration(PolarisCallContext ctx, String configName) {
public <T> @Nullable T getConfiguration(RealmContext ctx, String configName) {
return (T) config.get(configName);
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,19 @@
import java.util.Map;
import java.util.function.Function;
import java.util.function.Supplier;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.config.BehaviorChangeConfiguration;
import org.apache.polaris.core.config.FeatureConfiguration;
import org.apache.polaris.core.config.PolarisConfiguration;
import org.apache.polaris.core.config.PolarisConfigurationStore;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.entity.CatalogEntity;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

/** Unit test for the default behaviors of the PolarisConfigurationStore interface. */
public class PolarisConfigurationStoreTest {
private final RealmContext testRealmContext = () -> "testRealm";

@Test
public void testConfigsCanBeCastedFromString() {
List<PolarisConfiguration<?>> configs =
Expand All @@ -52,7 +53,7 @@ public void testConfigsCanBeCastedFromString() {
*/
@SuppressWarnings("unchecked")
@Override
public <T> @Nullable T getConfiguration(PolarisCallContext ctx, String configName) {
public <T> @Nullable T getConfiguration(RealmContext ctx, String configName) {
for (PolarisConfiguration<?> c : configs) {
if (c.key.equals(configName)) {
return (T) String.valueOf(c.defaultValue);
Expand All @@ -68,9 +69,8 @@ public void testConfigsCanBeCastedFromString() {

// Ensure that we can fetch all the configs and that the value is what we expect, which
// is the config's default value based on how we've implemented PolarisConfigurationStore above.
PolarisCallContext polarisCallContext = Mockito.mock(PolarisCallContext.class);
for (PolarisConfiguration<?> c : configs) {
Assertions.assertEquals(c.defaultValue, store.getConfiguration(polarisCallContext, c));
Assertions.assertEquals(c.defaultValue, store.getConfiguration(testRealmContext, c));
}
}

Expand All @@ -84,15 +84,14 @@ public void testInvalidCastThrowsException() {
new PolarisConfigurationStore() {
@SuppressWarnings("unchecked")
@Override
public <T> T getConfiguration(PolarisCallContext ctx, String configName) {
public <T> T getConfiguration(RealmContext ctx, String configName) {
return (T) "abc123";
}
};

PolarisCallContext polarisCallContext = Mockito.mock(PolarisCallContext.class);
for (PolarisConfiguration<?> c : configs) {
Assertions.assertThrows(
NumberFormatException.class, () -> store.getConfiguration(polarisCallContext, c));
NumberFormatException.class, () -> store.getConfiguration(testRealmContext, c));
}
}

Expand All @@ -106,18 +105,18 @@ private static <T> PolarisConfiguration<T> buildConfig(String key, T defaultValu

private static class PolarisConfigurationConsumer {

private final PolarisCallContext polarisCallContext;
private final RealmContext realmContext;
private final PolarisConfigurationStore configurationStore;

public PolarisConfigurationConsumer(
PolarisCallContext polarisCallContext, PolarisConfigurationStore configurationStore) {
this.polarisCallContext = polarisCallContext;
RealmContext realmContext, PolarisConfigurationStore configurationStore) {
this.realmContext = realmContext;
this.configurationStore = configurationStore;
}

public <T> T consumeConfiguration(
PolarisConfiguration<Boolean> config, Supplier<T> code, T defaultVal) {
if (configurationStore.getConfiguration(polarisCallContext, config)) {
if (configurationStore.getConfiguration(realmContext, config)) {
return code.get();
}
return defaultVal;
Expand All @@ -127,7 +126,7 @@ public <T> T consumeConfiguration(
@Test
public void testBehaviorAndFeatureConfigs() {
PolarisConfigurationConsumer consumer =
new PolarisConfigurationConsumer(null, new PolarisConfigurationStore() {});
new PolarisConfigurationConsumer(testRealmContext, new PolarisConfigurationStore() {});

FeatureConfiguration<Boolean> featureConfig =
PolarisConfiguration.<Boolean>builder()
Expand Down Expand Up @@ -164,22 +163,25 @@ public void testEntityOverrides() {
PolarisConfigurationStore store =
new PolarisConfigurationStore() {
@Override
public <T> @Nullable T getConfiguration(PolarisCallContext ctx, String configName) {
public <T> @Nullable T getConfiguration(RealmContext realmContext, String configName) {
//noinspection unchecked
return (T) Map.of("key2", "config-value2").get(configName);
}
};

PolarisCallContext ctx = null;
CatalogEntity entity =
new CatalogEntity.Builder()
.addProperty("polaris.config.catalog-key3", "entity-new3")
.addProperty("legacy-key4", "entity-legacy4")
.build();

Assertions.assertEquals("test-default1", store.getConfiguration(ctx, entity, cfg.apply(1)));
Assertions.assertEquals("config-value2", store.getConfiguration(ctx, entity, cfg.apply(2)));
Assertions.assertEquals("entity-new3", store.getConfiguration(ctx, entity, cfg.apply(3)));
Assertions.assertEquals("entity-legacy4", store.getConfiguration(ctx, entity, cfg.apply(4)));
Assertions.assertEquals(
"test-default1", store.getConfiguration(testRealmContext, entity, cfg.apply(1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

testRealmContext is not relevant here and does not affect the test's behaviour. I'd prefer to keep null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sg! I will update this in my part 2 PR.

Assertions.assertEquals(
"config-value2", store.getConfiguration(testRealmContext, entity, cfg.apply(2)));
Assertions.assertEquals(
"entity-new3", store.getConfiguration(testRealmContext, entity, cfg.apply(3)));
Assertions.assertEquals(
"entity-legacy4", store.getConfiguration(testRealmContext, entity, cfg.apply(4)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,8 @@ public void testUpdateNotificationCreateTableWithLocalFilePrefix() {
PolarisCallContext polarisCallContext = callContext.getPolarisCallContext();
if (!polarisCallContext
.getConfigurationStore()
.getConfiguration(polarisCallContext, FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES)
.getConfiguration(
callContext.getRealmContext(), FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES)
.contains("FILE")) {
Assertions.assertThatThrownBy(() -> catalog.sendNotification(table, request))
.isInstanceOf(ForbiddenException.class)
Expand Down Expand Up @@ -1100,7 +1101,8 @@ public void testUpdateNotificationCreateTableWithHttpPrefix() {
PolarisCallContext polarisCallContext = callContext.getPolarisCallContext();
if (!polarisCallContext
.getConfigurationStore()
.getConfiguration(polarisCallContext, FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES)
.getConfiguration(
callContext.getRealmContext(), FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES)
.contains("FILE")) {
Assertions.assertThatThrownBy(() -> catalog.sendNotification(table, request))
.isInstanceOf(ForbiddenException.class)
Expand All @@ -1121,7 +1123,8 @@ public void testUpdateNotificationCreateTableWithHttpPrefix() {

if (!polarisCallContext
.getConfigurationStore()
.getConfiguration(polarisCallContext, FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES)
.getConfiguration(
callContext.getRealmContext(), FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES)
.contains("FILE")) {
Assertions.assertThatThrownBy(() -> catalog.sendNotification(table, newRequest))
.isInstanceOf(ForbiddenException.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,14 +238,13 @@ public void testRegisterAndUseFeatureConfigurations() {

CatalogEntity catalog = new CatalogEntity.Builder().build();

Assertions.assertThat(configurationStore.getConfiguration(polarisContext, catalog, safeConfig))
Assertions.assertThat(configurationStore.getConfiguration(realmContext, catalog, safeConfig))
.isTrue();

Assertions.assertThat(
configurationStore.getConfiguration(polarisContext, catalog, unsafeConfig))
Assertions.assertThat(configurationStore.getConfiguration(realmContext, catalog, unsafeConfig))
.isTrue();

Assertions.assertThat(configurationStore.getConfiguration(polarisContext, catalog, bothConfig))
Assertions.assertThat(configurationStore.getConfiguration(realmContext, catalog, bothConfig))
.isTrue();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ private boolean catalogOverlapsWithExistingCatalog(CatalogEntity catalogEntity)
getCurrentPolarisContext()
.getConfigurationStore()
.getConfiguration(
getCurrentPolarisContext(), FeatureConfiguration.ALLOW_OVERLAPPING_CATALOG_URLS);
callContext.getRealmContext(), FeatureConfiguration.ALLOW_OVERLAPPING_CATALOG_URLS);

if (allowOverlappingCatalogUrls) {
return false;
Expand Down Expand Up @@ -778,7 +778,8 @@ public void deleteCatalog(String name) {
boolean cleanup =
polarisCallContext
.getConfigurationStore()
.getConfiguration(polarisCallContext, FeatureConfiguration.CLEANUP_ON_CATALOG_DROP);
.getConfiguration(
callContext.getRealmContext(), FeatureConfiguration.CLEANUP_ON_CATALOG_DROP);
DropEntityResult dropEntityResult =
metaStoreManager.dropEntityIfExists(
getCurrentPolarisContext(), null, entity, Map.of(), cleanup);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ private void validateStorageConfig(StorageConfigInfo storageConfigInfo) {
polarisCallContext
.getConfigurationStore()
.getConfiguration(
polarisCallContext, FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES);
callContext.getRealmContext(),
FeatureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES);
if (!allowedStorageTypes.contains(storageConfigInfo.getStorageType().name())) {
LOGGER
.atWarn()
Expand All @@ -179,7 +180,7 @@ private void validateConnectionConfigInfo(Catalog catalog) {
.getPolarisCallContext()
.getConfigurationStore()
.getConfiguration(
callContext.getPolarisCallContext(),
callContext.getRealmContext(),
FeatureConfiguration.SUPPORTED_CATALOG_CONNECTION_TYPES)
.stream()
.map(s -> s.toUpperCase(Locale.ROOT))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@
import org.apache.iceberg.view.ViewMetadata;
import org.apache.iceberg.view.ViewOperations;
import org.apache.iceberg.view.ViewRepresentation;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.config.FeatureConfiguration;
import org.apache.polaris.core.config.PolarisConfigurationStore;
import org.apache.polaris.core.context.RealmContext;

/**
* CODE_COPIED_TO_POLARIS Copied from CatalogHandler in Iceberg 1.8.0 Contains a collection of
Expand All @@ -95,13 +95,13 @@ public class CatalogHandlerUtils {
private static final Schema EMPTY_SCHEMA = new Schema();
private static final String INITIAL_PAGE_TOKEN = "";

private final PolarisCallContext polarisCallContext;
private final RealmContext realmContext;
private final PolarisConfigurationStore configurationStore;

@Inject
public CatalogHandlerUtils(
PolarisCallContext polarisCallContext, PolarisConfigurationStore configurationStore) {
this.polarisCallContext = polarisCallContext;
RealmContext realmContext, PolarisConfigurationStore configurationStore) {
this.realmContext = realmContext;
this.configurationStore = configurationStore;
}

Expand Down Expand Up @@ -609,6 +609,6 @@ protected ViewMetadata commit(ViewOperations ops, UpdateTableRequest request) {

private int maxCommitRetries() {
return configurationStore.getConfiguration(
polarisCallContext, FeatureConfiguration.ICEBERG_COMMIT_MAX_RETRIES);
realmContext, FeatureConfiguration.ICEBERG_COMMIT_MAX_RETRIES);
}
}
Loading
Loading