Allow to fetch multiple config parameters as a Map

[essobedo]

fixes #578

Motivation

When we need a dynamic configuration, it can be very helpful to use a map. For example, let's say that I want to keep in my configuration the custom reason phrases to return to the enduser according to the http status code.

I would like to have:

The expected code

@ConfigProperty(name = "reasons")
Map<Integer, String> reasonPhrases; // or eventually Map<String, String>

The expected configuration

reasons.200=My custom reason phrase for OK
reasons.201=My custom reason phrase for Created
...

Modifications

• Adds a generic producer for Map
• Adds a MapConverter to convert the content of type <key1>=<value1>;<key2>=<value2>... into a Map.
• Adds map support to the ConfigMappingGenerator
• Adds related unit tests
• Adds new methods getValues(String name, Class<K> kClass, Class<V> vClass) and getOptionalValues(String name, Class<K> kClass, Class<V> vClass) to support properly the map from the class SmallRyeConfig

[essobedo]

Hi @radcortez it is the PR related to my comment. It is very basic because there are many things unspecified (like for example how to manage default values in case of @ConfigProperty, how to deal with config.getValue in case of map since we have no way to get the Parameterized types) but at least it covers simple uses cases. WDYT of this PR?

[radcortez]

Thanks for the PR!

how to manage default values in case of @ConfigProperty,
We could support something like key=value and parse the default value if the injection is a map. Alternatively, we could just disallow it all together and throw a validation exception.

how to deal with config.getValue in case of map since we have no way to get the Parameterized types)
We would need to support something similar to a GenericType like JAX-RS to carry type information, but for now we could just assume that the programmatic API for Map returns a Map<String,String>.

[essobedo]

how to manage default values in case of @ConfigProperty,
We could support something like key=value and parse the default value if the injection is a map. Alternatively, we could just disallow it all together and throw a validation exception.

I just added the support of default value with the next format <key1>=<value1>;<key2>=<value2>..., knowing that we can use a backslash to escape the characters = and ; if needed

how to deal with config.getValue in case of map since we have no way to get the Parameterized types)
We would need to support something similar to a GenericType like JAX-RS to carry type information, but for now we could just assume that the programmatic API for Map returns a Map<String,String>.

I added a new BuiltIn Converter for the Map to be able to support config.getValue

[essobedo]

@radcortez Hi, all your remarks have been addressed, this PR is ready for review now. Thank you in advance 🙏

[essobedo]

I added a new BuiltIn Converter for the Map to be able to support config.getValue

Finally I rather added new dedicated methods as extensions for the map support such that I don't need the Map converter to be part of the Builtin converters

[essobedo]

I expected to have it included into Quarkus 2.0, I guess it is too late now 😢

[radcortez]

I expected to have it included into Quarkus 2.0, I guess it is too late now 😢

I wouldn't be too worried about it. Quarkus has a very high release cadence, so it we miss the boat now, we will get the next one soon :)

Anyway, we still need to test this in Quarkus. Did you try it? In the meantime I'll have review the PR.

[radcortez]

I think this is going into the right direction. Left a few comments for us to discuss :) Thanks!

[radcortez]

@essobedo Thank you! I think we are almost done. Just one last detail: can we document the new feature here: https://github.com/smallrye/smallrye-config/blob/fecdcf541a0ad979cc3f83f8e5f4829da61e5be8/doc/modules/ROOT/pages/config/config.adoc. You can follow the indexed properties example.

[radcortez]

Also, I recommend to make a build and test it with Quarkus master. Did you try it with Quarkus as is?

[essobedo]

Also, I recommend to make a build and test it with Quarkus master. Did you try it with Quarkus as is?

Not yet but I will test it today and let you know the results.

[essobedo]

@radcortez just to let you know that I had to make couple of additional fixes to make it work with Quarkus, knowing that even on Quarkus side there are some little changes to make. I have not yet finished but I could make injection with @ConfigProperty work but I have one last bug to fix on Quarkus side with @ConfigProperties. Indeed I get the following error at runtime only, it should not be too hard to fix:

2021-05-28 20:47:41,177 ERROR [io.qua.run.Application] (main) Failed to start application (with profile worker): java.lang.ClassNotFoundException: org.objectweb.asm.ClassVisitor
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
        at io.quarkus.bootstrap.runner.RunnerClassLoader.loadClass(RunnerClassLoader.java:104)
        at io.quarkus.bootstrap.runner.RunnerClassLoader.loadClass(RunnerClassLoader.java:60)
        at io.smallrye.config.ConfigMappingClass.<init>(ConfigMappingClass.java:35)
        at io.smallrye.config.ConfigMappingClass.createConfigurationClass(ConfigMappingClass.java:27)
        at io.smallrye.config.ConfigMappingClass.access$000(ConfigMappingClass.java:7)
        at io.smallrye.config.ConfigMappingClass$1.computeValue(ConfigMappingClass.java:11)
        at io.smallrye.config.ConfigMappingClass$1.computeValue(ConfigMappingClass.java:8)
        at java.base/java.lang.ClassValue.getFromHashMap(ClassValue.java:228)
        at java.base/java.lang.ClassValue.getFromBackup(ClassValue.java:210)
        at java.base/java.lang.ClassValue.get(ClassValue.java:116)
        at io.smallrye.config.ConfigMappingClass.getConfigurationClass(ConfigMappingClass.java:17)
        at io.smallrye.config.ConfigMappingLoader.getConfigMappingClass(ConfigMappingLoader.java:42)
        at io.smallrye.config.ConfigMappingProvider$Builder.addRoot(ConfigMappingProvider.java:887)
        at io.smallrye.config.ConfigMappings.mapConfiguration(ConfigMappings.java:52)
        at io.smallrye.config.ConfigMappings.registerConfigProperties(ConfigMappings.java:43)
        at io.quarkus.arc.runtime.ConfigRecorder.registerConfigProperties(ConfigRecorder.java:60)
        at io.quarkus.deployment.steps.ConfigBuildStep$registerConfigClasses-2116437784.deploy_0(ConfigBuildStep$registerConfigClasses-2116437784.zig:159)
        at io.quarkus.deployment.steps.ConfigBuildStep$registerConfigClasses-2116437784.deploy(ConfigBuildStep$registerConfigClasses-2116437784.zig:40)
        at io.quarkus.runner.ApplicationImpl.doStart(ApplicationImpl.zig:546)
        at io.quarkus.runtime.Application.start(Application.java:101)
        at io.quarkus.runtime.ApplicationLifecycleManager.run(ApplicationLifecycleManager.java:101)

[essobedo]

@radcortez After a deeper investigation, it appears that I need to move the method generateInterfaceName out of ConfigMappingGenerator to avoid loading ClassVisitor at runtime for nothing since the method doesn't use it and the byte code has been generated at build time already

[essobedo]

So I tested it successfully on Quarkus (modulo a small patch to support map) with the next use case:

The main configuration class:

@Singleton
public class GreetingConfiguration {

    @ConfigProperty(name = "root.numbers")
    public Map<Integer, String> numbers;

    @ConfigProperty(name = "root.numbers")
    public Optional<Map<Integer, String>> oNumbers;

    @ConfigProperty(name = "root.numbers")
    public Supplier<Map<Integer, String>> sNumbers;

    @ConfigProperties
    public RootConfig root;
}

The class RootConfig:

@ConfigProperties(prefix = "root")
public class RootConfig {

    public Map<Integer, String> numbers;
}

The yaml configuration used for the test:

root:
  numbers:
    1: one
    2: two
    3: three

And if I remove the configuration, I get java.util.NoSuchElementException: SRCFG00014: The config property root.numbers is required but it could not be found in any config source as expected.

Let me know if it is good enough for you or you have another uses in mind to test, meanwhile I start working on the doc.

[essobedo]

FYI here is my test branch for Quarkus https://github.com/quarkusio/quarkus/compare/main...essobedo:add-map-support?expand=1

[essobedo]

@radcortez I added the doc, so now all your remarks have been addressed

[radcortez]

@essobedo Thank you for your patience. Very good job. Just a few more minor things (leftovers from the other proposals and we should be good to go).

[essobedo]

@radcortez Done, please check again. Do you want me to squash my commits?

[radcortez]

@radcortez Done, please check again. Do you want me to squash my commits?

Yes, please. Thank you :)

[essobedo]

@radcortez Done, please check again. Do you want me to squash my commits?

Yes, please. Thank you :)

Done

[radcortez]

@essobedo Thank you very much for all the work. I hope you can continue to contribute :)

Can you please open a Draft PR for Quarkus with the changes required? It it fine to use the SNAPSHOT (the CI won't build it as draft). We can update the version once we release, so the PR is for us to not forget the changes.

I think we may need some other fixes for Quarkus in other areas, so we need these in before doing the final release.

[essobedo]

@essobedo Thank you very much for all the work. I hope you can continue to contribute :)

You're welcome, it is always with great pleasure that I contribute to open source projects. If you would like me to help you (if I can) on another tickets, please let me know, I will be more than happy to help 😄

Can you please open a Draft PR for Quarkus with the changes required? It it fine to use the SNAPSHOT (the CI won't build it as draft). We can update the version once we release, so the PR is for us to not forget the changes.

I think we may need some other fixes for Quarkus in other areas, so we need these in before doing the final release.

Here it is quarkusio/quarkus#17579

[rmanibus]

Thanks a lot @essobedo !