Remove defaults / overrides from feature configurations

[eric-maynard]

After this change, the format of realm overrides has changed from

• polaris.features.realm-overrides."my-realm"."feature"=true

to:

• polaris.features.realm-overrides."my-realm".overrides."feature"=true

@adutra suggested a fix here which is applied in this PR. It turns out that applying the same fix can also remove defaults from the key used to set properties on the service level, so that is done here as well.

[gh-yzou]

@eric-maynard i just have one quick question, is there any fix you needed for QuarkusBehaviorChangesConfiguration? i suspect it might have similar problem if it is in use

[eric-maynard]

Good call @gh-yzou, I think you're right. Added there as well

[adutra]

I think this line change goes too far; basically with this annotation we go from:

polaris.features.realm-overrides."my-realm"."feature"=true

to:

polaris.features."my-realm"."feature"=true

Which is definitely more concise, but I wonder if SmallRye will be smart enough to "understand" that "my-realm" is a realm ID and not a feature configuration name 🤔

@eric-maynard did you validate that this works? If it does, I'm OK to do this but we'd need to revisit documentation + helm chart.

[eric-maynard]

I validated that the service starts up with:

polaris.features."my-realm"."INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST"=true

But you're right that SmallRye may just think my-realm is a feature... actually, I don't see how it would be differentiating. Since it seems that don't have a test for this, let's play it safe and add the realm-overrides back in

[adutra]

Well it might be able to differentiate by the number of available path segments, e.g. the two lines below are (at least for us humans) unambiguously defined to be a default for the first one, and an override for the second one:

polaris.features.INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST"=true
polaris.features."my-realm"."INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST"=false

But I agree that it may be a bit risky to let a machine figure this subtlety out.

[eric-maynard]

From what I've read, it will first check all the explicitly-named parts of the structure, and then put the remainder in the Map<String, ...> defaults. From what I have seen testing it works now. I copied in a similar test to the one on @gh-yzou's PR here.

[adutra]

FYI this is creating an unwanted behavior: I injected FeaturesConfiguration in DefaultConfigurationStoreTest and here are the contents of featuresConfiguration.defaults():

realm-overrides."realm1"."ALLOW_SPECIFYING_FILE_IO_IMPL"=false
ALLOW_SPECIFYING_FILE_IO_IMPL=true
ENABLE_GENERIC_TABLES=false
ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING=false
SUPPORTED_CATALOG_CONNECTION_TYPES=["ICEBERG_REST"]
SUPPORTED_CATALOG_STORAGE_TYPES=["S3","GCS","AZURE","FILE"]

You will notice the strange realm-overrides."realm1"."ALLOW_SPECIFYING_FILE_IO_IMPL=false entry.

The contents of featuresConfiguration.realmOverrides() look correct though:

realm1={
  ALLOW_SPECIFYING_FILE_IO_IMPL=false
}

At this point, I wonder if a better solution wouldn't be to simply define one single map here to hold all the configs:

public interface QuarkusFeaturesConfiguration extends FeaturesConfiguration {
  @WithParentName
  @Override
  Map<String, String> allConfigs();
}

And parse that map manually (we are already doing it half-manually anyway):

public interface FeaturesConfiguration {

  Map<String, String> allConfigs();

  default Map<String, Object> parseDefaults(ObjectMapper objectMapper) {
    Map<String, Object> defaults = new HashMap<>();
    for (String key : allConfigs().keySet()) {
      if (!key.contains(".")) {
        defaults.put(key, convertProperty(objectMapper, key, allConfigs().get(key)));
      }
    }
    return defaults;
  }

  default Map<String, Map<String, Object>> parseRealmOverrides(ObjectMapper objectMapper) {
    Map<String, Map<String, Object>> realmOverrides = new HashMap<>();
    for (String key : allConfigs().keySet()) {
      if (key.contains(".")) {
        String[] parts = key.split("\\.");
        String realm = parts[0];
        String configName = parts[1];
        Map<String, Object> realmValues = realmOverrides.computeIfAbsent(realm, k -> new HashMap<>());
        realmValues.put(configName, convertProperty(objectMapper, key, allConfigs().get(key)));
      }
    }
    return realmOverrides;
  }

  private static Object convertProperty(ObjectMapper objectMapper, String key, String value) {
    try {
      JsonNode node = objectMapper.readTree(value);
      return configValue(node);
    } catch (JsonProcessingException e) {
      throw new RuntimeException(
          "Invalid JSON value for feature configuration: " + key, e);
    }
  }

  private static Object configValue(JsonNode node) {
    // same as before
  }
}

WDYT?

(this obviously assumes that a feature configuration name cannot contain a dot.)

[eric-maynard]

I see the same thing, thanks for highlighting this. One doubt I have is this: if we assume config names can't contain a dot, is the current behavior okay? Since realm-overrides."realm1"."ALLOW_SPECIFYING_FILE_IO_IMPL=false cannot be misinterpreted as a config value named realm-overrides.realm1.ALLOW_SPECIFYING_FILE_IO_IMPL

Also, either way I can add some code enforcing that config keys don't contain a dot.