Refactor getConfiguration to use RealmContext (Part 2) (#1783)

Author: gh-yzou

polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfigurationStore.java

@@ -24,7 +24,6 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
-import org.apache.polaris.core.PolarisCallContext;
 import org.apache.polaris.core.context.RealmContext;
 import org.apache.polaris.core.entity.CatalogEntity;
 import org.slf4j.Logger;
@@ -37,48 +36,6 @@
 public interface PolarisConfigurationStore {
   Logger LOGGER = LoggerFactory.getLogger(PolarisConfigurationStore.class);
 
-  /**
-   * Retrieve the current value for a configuration key. May be null if not set.
-   *
-   * <p>This function will be deprecated, it can not be called outside of active request scope, such
-   * as background tasks (TaskExecutor). Please use the function getConfiguration(RealmContext
-   * realmContext, String configName) to get the configuration value in a more robust way. TODO:
-   * Remove this function and replace the usage with the function takes realm. Github issue
-   * https://github.com/apache/polaris/issues/1775
-   *
-   * @param ctx the current call context
-   * @param configName the name of the configuration key to check
-   * @return the current value set for the configuration key or null if not set
-   * @param <T> the type of the configuration value
-   */
-  @Deprecated
-  default <T> @Nullable T getConfiguration(PolarisCallContext ctx, String configName) {
-    return null;
-  }
-
-  /**
-   * Retrieve the current value for a configuration key. If not set, return the non-null default
-   * value.
-   *
-   * <p>This function will be deprecated, it can not be called outside of active request scope, such
-   * as background tasks (TaskExecutor). Please use the function getConfiguration(RealmContext
-   * realmContext, String configName, T defaultValue) to get the configuration value in a more
-   * robust way. TODO: Remove this function and replace the usage with the function takes realm.
-   * Github issue https://github.com/apache/polaris/issues/1775
-   *
-   * @param ctx the current call context
-   * @param configName the name of the configuration key to check
-   * @param defaultValue the default value if the configuration key has no value
-   * @return the current value or the supplied default value
-   */
-  @Deprecated
-  default <T> @Nonnull T getConfiguration(
-      PolarisCallContext ctx, String configName, @Nonnull T defaultValue) {
-    Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value");
-    T configValue = getConfiguration(ctx, configName);
-    return configValue != null ? configValue : defaultValue;
-  }
-
   /**
    * Retrieve the current value for a configuration key for a given realm. May be null if not set.
    *
@@ -102,7 +59,7 @@ public interface PolarisConfigurationStore {
    * @param <T> the type of the configuration value
    */
   default <T> @Nonnull T getConfiguration(
-      RealmContext realmContext, String configName, @Nonnull T defaultValue) {
+      @Nonnull RealmContext realmContext, String configName, @Nonnull T defaultValue) {
     Preconditions.checkNotNull(defaultValue, "Cannot pass null as a default value");
     T configValue = getConfiguration(realmContext, configName);
     return configValue != null ? configValue : defaultValue;
@@ -141,7 +98,7 @@ public interface PolarisConfigurationStore {
    * @param <T> the type of the configuration value
    */
   default <T> @Nonnull T getConfiguration(
-      RealmContext realmContext, PolarisConfiguration<T> config) {
+      @Nonnull RealmContext realmContext, PolarisConfiguration<T> config) {
     T result = getConfiguration(realmContext, config.key, config.defaultValue);
     return tryCast(config, result);
   }
@@ -157,7 +114,7 @@ public interface PolarisConfigurationStore {
    * @param <T> the type of the configuration value
    */
   default <T> @Nonnull T getConfiguration(
-      RealmContext realmContext,
+      @Nonnull RealmContext realmContext,
       @Nonnull CatalogEntity catalogEntity,
       PolarisConfiguration<T> config) {
     if (config.hasCatalogConfig() || config.hasCatalogConfigUnsafe()) {

polaris-core/src/main/java/org/apache/polaris/core/persistence/AtomicOperationMetaStoreManager.java

@@ -1517,7 +1517,7 @@ private void revokeGrantRecord(
                   callCtx
                       .getConfigurationStore()
                       .getConfiguration(
-                          callCtx,
+                          callCtx.getRealmContext(),
                           PolarisTaskConstants.TASK_TIMEOUT_MILLIS_CONFIG,
                           PolarisTaskConstants.TASK_TIMEOUT_MILLIS);
               return taskState == null

polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java

@@ -1956,7 +1956,7 @@ private PolarisEntityResolver resolveSecurableToRoleGrant(
                   callCtx
                       .getConfigurationStore()
                       .getConfiguration(
-                          callCtx,
+                          callCtx.getRealmContext(),
                           PolarisTaskConstants.TASK_TIMEOUT_MILLIS_CONFIG,
                           PolarisTaskConstants.TASK_TIMEOUT_MILLIS);
               return taskState == null

polaris-core/src/test/java/org/apache/polaris/core/storage/InMemoryStorageIntegrationTest.java

@@ -102,7 +102,8 @@ public void testValidateAccessToLocationsWithWildcard() {
             new PolarisConfigurationStore() {
               @SuppressWarnings("unchecked")
               @Override
-              public <T> @Nullable T getConfiguration(RealmContext ctx, String configName) {
+              public <T> @Nullable T getConfiguration(
+                  @Nonnull RealmContext ctx, String configName) {
                 return (T) config.get(configName);
               }
             },

polaris-core/src/test/java/org/apache/polaris/service/storage/PolarisConfigurationStoreTest.java

@@ -18,6 +18,7 @@
  */
 package org.apache.polaris.service.storage;
 
+import jakarta.annotation.Nonnull;
 import jakarta.annotation.Nullable;
 import java.util.List;
 import java.util.Map;
@@ -53,7 +54,7 @@ public void testConfigsCanBeCastedFromString() {
            */
           @SuppressWarnings("unchecked")
           @Override
-          public <T> @Nullable T getConfiguration(RealmContext ctx, String configName) {
+          public <T> @Nullable T getConfiguration(@Nonnull RealmContext ctx, String configName) {
             for (PolarisConfiguration<?> c : configs) {
               if (c.key.equals(configName)) {
                 return (T) String.valueOf(c.defaultValue);
@@ -84,7 +85,7 @@ public void testInvalidCastThrowsException() {
         new PolarisConfigurationStore() {
           @SuppressWarnings("unchecked")
           @Override
-          public <T> T getConfiguration(RealmContext ctx, String configName) {
+          public <T> T getConfiguration(@Nonnull RealmContext ctx, String configName) {
             return (T) "abc123";
           }
         };
@@ -163,7 +164,8 @@ public void testEntityOverrides() {
     PolarisConfigurationStore store =
         new PolarisConfigurationStore() {
           @Override
-          public <T> @Nullable T getConfiguration(RealmContext realmContext, String configName) {
+          public <T> @Nullable T getConfiguration(
+              @Nonnull RealmContext realmContext, String configName) {
             //noinspection unchecked
             return (T) Map.of("key2", "config-value2").get(configName);
           }

quarkus/service/src/test/java/org/apache/polaris/service/quarkus/config/DefaultConfigurationStoreTest.java

@@ -20,20 +20,15 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-import io.quarkus.test.junit.QuarkusMock;
 import io.quarkus.test.junit.QuarkusTest;
 import io.quarkus.test.junit.QuarkusTestProfile;
 import io.quarkus.test.junit.TestProfile;
 import jakarta.inject.Inject;
-import java.time.Clock;
 import java.util.Map;
-import org.apache.polaris.core.PolarisCallContext;
-import org.apache.polaris.core.PolarisDiagnostics;
 import org.apache.polaris.core.config.FeatureConfiguration;
 import org.apache.polaris.core.config.PolarisConfigurationStore;
 import org.apache.polaris.core.context.RealmContext;
 import org.apache.polaris.core.entity.CatalogEntity;
-import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
 import org.apache.polaris.service.config.DefaultConfigurationStore;
 import org.apache.polaris.service.config.FeaturesConfiguration;
 import org.assertj.core.api.Assertions;
@@ -73,12 +68,9 @@ public Map<String, String> getConfigOverrides() {
     }
   }
 
-  private PolarisCallContext polarisContext;
   private RealmContext realmContext;
 
-  @Inject MetaStoreManagerFactory managerFactory;
   @Inject PolarisConfigurationStore configurationStore;
-  @Inject PolarisDiagnostics diagServices;
   @Inject FeaturesConfiguration featuresConfiguration;
 
   @BeforeEach
@@ -89,60 +81,42 @@ public void before(TestInfo testInfo) {
                 testInfo.getTestMethod().map(java.lang.reflect.Method::getName).orElse("test"),
                 System.nanoTime());
     realmContext = () -> realmName;
-    polarisContext =
-        new PolarisCallContext(
-            realmContext,
-            managerFactory.getOrCreateSessionSupplier(realmContext).get(),
-            diagServices,
-            configurationStore,
-            Clock.systemDefaultZone());
-  }
-
-  @Test
-  public void testGetConfigurationWithNoRealmContext() {
-    Assertions.assertThatThrownBy(
-            () -> configurationStore.getConfiguration(polarisContext, "missingKeyWithoutDefault"))
-        .isInstanceOf(IllegalStateException.class);
   }
 
   @Test
   public void testGetConfiguration() {
-    QuarkusMock.installMockForType(realmContext, RealmContext.class);
-    Object value = configurationStore.getConfiguration(polarisContext, "missingKeyWithoutDefault");
+    Object value = configurationStore.getConfiguration(realmContext, "missingKeyWithoutDefault");
     assertThat(value).isNull();
     Object defaultValue =
-        configurationStore.getConfiguration(
-            polarisContext, "missingKeyWithDefault", "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(polarisContext, falseByDefaultKey))
+    assertThat((Boolean) configurationStore.getConfiguration(realmContext, falseByDefaultKey))
         .isFalse();
     // the trueByDefaultKey is set to true for all realms in Profile.getConfigOverrides
-    assertThat((Boolean) configurationStore.getConfiguration(polarisContext, trueByDefaultKey))
+    assertThat((Boolean) configurationStore.getConfiguration(realmContext, trueByDefaultKey))
         .isTrue();
   }
 
   @Test
   public void testGetRealmConfiguration() {
     // check the realmOne configuration
-    QuarkusMock.installMockForType(realmOneContext, RealmContext.class);
     // the falseByDefaultKey is set to `false` for all realms, but overwrite with value `true` for
     // realmOne.
-    assertThat((Boolean) configurationStore.getConfiguration(polarisContext, falseByDefaultKey))
+    assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, falseByDefaultKey))
         .isTrue();
     // the trueByDefaultKey is set to `false` for all realms, no overwrite for realmOne
-    assertThat((Boolean) configurationStore.getConfiguration(polarisContext, trueByDefaultKey))
+    assertThat((Boolean) configurationStore.getConfiguration(realmOneContext, trueByDefaultKey))
         .isTrue();
 
     // check the realmTwo configuration
-    QuarkusMock.installMockForType(realmTwoContext, RealmContext.class);
     // the falseByDefaultKey is set to `false` for all realms, no overwrite for realmTwo
-    assertThat((Boolean) configurationStore.getConfiguration(polarisContext, falseByDefaultKey))
+    assertThat((Boolean) configurationStore.getConfiguration(realmTwoContext, falseByDefaultKey))
         .isFalse();
     // the trueByDefaultKey is set to `false` for all realms, and overwrite with value `false` for
     // realmTwo
-    assertThat((Boolean) configurationStore.getConfiguration(polarisContext, trueByDefaultKey))
+    assertThat((Boolean) configurationStore.getConfiguration(realmTwoContext, trueByDefaultKey))
         .isFalse();
   }
 
@@ -167,28 +141,24 @@ void testGetConfigurationWithRealm() {
 
   @Test
   public void testInjectedConfigurationStore() {
-    QuarkusMock.installMockForType(realmContext, RealmContext.class);
     // the default value for trueByDefaultKey is `true`
-    boolean featureDefaultValue =
-        configurationStore.getConfiguration(polarisContext, trueByDefaultKey);
+    Boolean featureDefaultValue =
+        configurationStore.getConfiguration(realmContext, trueByDefaultKey);
     assertThat(featureDefaultValue).isTrue();
 
-    QuarkusMock.installMockForType(realmTwoContext, RealmContext.class);
     // the value for falseByDefaultKey is `false`, and no realm override for realmTwo
-    boolean realmTwoValue = configurationStore.getConfiguration(polarisContext, falseByDefaultKey);
+    Boolean realmTwoValue = configurationStore.getConfiguration(realmTwoContext, falseByDefaultKey);
     assertThat(realmTwoValue).isFalse();
 
     // Now, realmOne override falseByDefaultKey to `True`
-    QuarkusMock.installMockForType(realmOneContext, RealmContext.class);
-    boolean realmOneValue = configurationStore.getConfiguration(polarisContext, falseByDefaultKey);
+    Boolean realmOneValue = configurationStore.getConfiguration(realmOneContext, falseByDefaultKey);
     assertThat(realmOneValue).isTrue();
 
     assertThat(configurationStore).isInstanceOf(DefaultConfigurationStore.class);
   }
 
   @Test
   public void testInjectedFeaturesConfiguration() {
-    QuarkusMock.installMockForType(realmContext, RealmContext.class);
     assertThat(featuresConfiguration).isInstanceOf(QuarkusResolvedFeaturesConfiguration.class);
 
     assertThat(featuresConfiguration.defaults())
@@ -209,7 +179,6 @@ public void testInjectedFeaturesConfiguration() {
 
   @Test
   public void testRegisterAndUseFeatureConfigurations() {
-    QuarkusMock.installMockForType(realmContext, RealmContext.class);
     String prefix = "testRegisterAndUseFeatureConfigurations";
 
     FeatureConfiguration<Boolean> safeConfig =

service/common/src/main/java/org/apache/polaris/service/config/DefaultConfigurationStore.java

@@ -22,11 +22,9 @@
 import jakarta.annotation.Nonnull;
 import jakarta.annotation.Nullable;
 import jakarta.enterprise.context.ApplicationScoped;
-import jakarta.enterprise.inject.Instance;
 import jakarta.inject.Inject;
 import java.util.Map;
 import java.util.Optional;
-import org.apache.polaris.core.PolarisCallContext;
 import org.apache.polaris.core.config.PolarisConfigurationStore;
 import org.apache.polaris.core.context.RealmContext;
 import org.slf4j.Logger;
@@ -38,7 +36,6 @@ public class DefaultConfigurationStore implements PolarisConfigurationStore {
 
   private final Map<String, Object> defaults;
   private final Map<String, Map<String, Object>> realmOverrides;
-  @Inject private Instance<RealmContext> realmContextInstance;
 
   // FIXME the whole PolarisConfigurationStore + PolarisConfiguration needs to be refactored
   // to become a proper Quarkus configuration object
@@ -61,18 +58,6 @@ public DefaultConfigurationStore(
     return confgValue;
   }
 
-  @Override
-  public <T> @Nullable T getConfiguration(@Nonnull PolarisCallContext ctx, String configName) {
-    if (realmContextInstance.isResolvable()) {
-      RealmContext realmContext = realmContextInstance.get();
-      return getConfiguration(realmContext, configName);
-    } else {
-      LOGGER.debug(
-          "No RealmContext is injected when lookup value for configuration {} ", configName);
-      return getDefaultConfiguration(configName);
-    }
-  }
-
   private <T> @Nullable T getDefaultConfiguration(String configName) {
     @SuppressWarnings("unchecked")
     T confgValue = (T) defaults.get(configName);