Bug: SmallRyeConfig#getValues: mapFactory is not used

[magicprinc]

io.smallrye.config.SmallRyeConfig#getValues(java.lang.String, org.eclipse.microprofile.config.spi.Converter<K>, org.eclipse.microprofile.config.spi.Converter<V>, java.util.function.IntFunction<java.util.Map<K,V>>)

public <K, V> Map<K, V> getValues(
    String name,
    Converter<K> keyConverter,
    Converter<V> valueConverter,
    IntFunction<Map<K, V>> mapFactory // 1️⃣ any Map could be created
){
  try {
    // 2️⃣ but in most cases caller will receive HashMap (with optional ClassCastException)
    return getValue(name, newMapConverter(keyConverter, valueConverter));
  } catch (NoSuchElementException e) {
    Map<String, String> mapKeys = getMapKeys(name);
    if (mapKeys.isEmpty()) {
        throw new NoSuchElementException(ConfigMessages.msg.propertyNotFound(name));
    }
    // 3️⃣ only this branch creates required type of Map
    Map<K, V> map = mapFactory.apply(mapKeys.size());
    for (Map.Entry<String, String> entry : mapKeys.entrySet()){
       map.put(convertValue(ConfigValue.builder().withName(entry.getKey()).withValue(entry.getKey()).build(),
                keyConverter), getValue(entry.getValue(), valueConverter));
    }

[dmlloyd]

I agree that this is inconsistent. We should have a variation of newMapConverter which accepts a map factory, using HashMap::new as the default.

As an aside, this way of doing map conversion is pretty weird: it first tries to convert a single key to a map and then tries to read multiple keys as a map. I don't like the double-duty because the user will already know what they want, and I doubt very much that the thing that they want for any given property is both behaviors.

[radcortez]

Before we added support to Map with the key in the property name, we accepted an inline value to create the Map as key=value;another-key=value. A similar thing happened with Collection values, where MP Config defines commas to separate values, and we added the indexed format as [x].

We ended up proving double-duty methods because when we are creating the Config, we never know which format is being used, so we need to try both. Of course, if the user is calling these methods directly, we could force to use the single responsibility APIs. Ideally, we should deprecate and later remove the inline value version for both Collection and Map. We could easily do that for the Map part, but for Collections it would be harder without going against the spec.

[magicprinc]

I like inline collections and Maps.
BTW, this is the only way for ConfigProperty.defaultValue to handle them.
So please, don't remove 🙏

But my bug report is not about it, but about the created Map class.
E.g. I need LinkedHashMap, but SRC creates HashMap and I receive ClassCastException

The first sentence in @dmlloyd comment is an exact match.

[dmlloyd]

I don't think there is any reason to get rid of inline maps, and definitely no reason to get rid of inline collections. But I don't think it makes a lot of sense to try one and fall back to the other.

[radcortez]

The intention was to make it convenient so you are only required to call one method regardless of the flavor you use to configure such properties.

We could split the API methods, but we still need a way to know which one to use. Either we try one and fall back to the other one, or we need to make it a configuration to tell which format to use. Or do you have another idea?

[dmlloyd]

If you are referring to config mappings, as they exist today, then making it configurable is probably the choice which is most consistent with the original design philosophy of this project.

In an ideal world, this would be config source dependent since some have a native representation for lists, and some do not. Being real, we're really just talking about properties and env vars, since they are the only formats without this support. If this were a green-field project, it would make the most sense for a config source to indicate the kind of value that exists at a node in the graph, and to develop a syntax for properties-style configuration that isn't dependent on the converter in use (it'd probably be a language that is similar to, but not exactly the same as, the properties file specification in order to support extended syntax). The syntax for system property overrides would probably be different since it is a different use case, and would be more optimized for command-line usage (this is because using the same logic for property files and system properties can be viewed as an implementation detail that leaked up to the specification).

Note however that every step we take away from our ideal world digs us deeper into a hole from which we may never escape.

[magicprinc]

Vert.x Config is an interesting step in this direction (and they have a lot of extensions to read configuration from everywhere).
Quarkus potentially has two mature configuration systems out-of-the box.
The main problem of Vert.x Config is: you need Vert.x to run it. I.e. one vert.x to read configs and the second - main - to start with them.
The second problem: .properties files are the most popular format for configs (application.properties to be specific :-), not json or other "typed" formats

BTW, the main goal of this bug report is to fix the type of the Map: use factory