Allow specifying optional config section with YAML configuration

[hrstoyanov]

Describe the bug
See here
Expected behavior
Optional configuration sections should be allowed with Quarkus YAML config.

Actual behavior
I know of no way of specifying optional section with YAML config.

To Reproduce
Unpack this project
issue_optional_config.zip
and follow the instruction here

[hrstoyanov]

@geoand

[hrstoyanov]

@geoand thanks for looking into this issue, any chance this gets fixed in 1.11.x? if I use JSON, would that work?

[geoand]

Most likely yes, but I can't make any guarantee on which micro it will be in.

[geoand]

I took a look at this and actually I am not sure I want to go through with it.

The reason is because the semantics of having the config section be Optional are a little hazy.
Say you make the sectional Optional, then under what conditional does rhe method return a value? When at least one property is set, when all properties are set?
If you go with the first option (which on the face of it seems the most reasonable), what happens when you invoke a method of that sub section that does not return an Optional but whose property has not been set?
It just seems to me to be weird to deal with a consistent and non suprising way... I assume that if we do do this, then whatever way it's done, some folks will be surprised by its behavior.

[hrstoyanov]

@geoand
Not sure why it is confusing - if an optional section is partially populated, extracting values from non-existing fields should result in default value for the type, or depending on any additional field annotations?

Take a look at this example:

sql_proxy:
  name: proxyname
  server:
    mssql:
      //server: localhost - missing
      //user: john - missing
      //password: changeme - missing
      port: 1143
      ...

 interface MSSQLConfig {
        @ConfigProperty(name = "server")  String server();
        @ConfigProperty(name = "user", defaultValue="smith")  String user();
        @ConfigProperty(name = "password")  @NotBlank String password();
        @ConfigProperty(name = "port") int port();
}
public interface ServerConfig {
    @ConfigProperty(name = "mssql") Optional<MSSQLConfig> mssql();
}

I would expect this:

   Optional<MSSQLConfig> optionalMSSQL = ServerConfig.msssql();

   assert optionalMSSQL.isPresent(); //Case 1. Because the section itself  exists! Otherwise - the optional should be empty/not present.
   MSSQLConfig mssql = optional.get();  // ... could potentially be NULL
   String server = mssql.server(); //Case 2. This is OK, we default to NULL, if missing, since it is not required.
   String user = mssql.user(); //Case 3. This is OK, we default to "smith", if missing.
   String password = mssql.password(); //Case 4. Runtime exception, since value is required and missing. 
   int port = mssql.port(); //Case 5. This is OK, we default to 0, if missing. Or maybe Integer.NaN?

What else is a developer to do without optional sections? How would you deal with the case where you want to make MSSQL section optional, so you can have ORACLE and PostGress as alternatives ?

[geoand]

I explained why I think it's confusing, I don't think there is much point in reiterating it.

You mention the default value for the subsections, but what would happen if there is no default value?

The best I can do is give it some thought and get back to you, but I'll be honest, I don't think I'll implement this.

[geoand]

Another complication is that in MP Config which is used at the lowest level for configuration, there is no support for multi level configuration - as one uses in YAML - so that means that all configuration options are mapped to a flat property structure.
This really complicates things from a YAML point of view.
Unfortunately we can't do much at the moment, but it's certainly a design choice that needs to be reevaluated in the future

[geoand]

After taking another look at this, I stand by my initial assement. Once we have a redesign of MP Config based on what I mention above, we can have another look at this

[hrstoyanov]

i tried several hacks this weekend, but nothing worked.. Dumping MPConfig, will have to create my own parser.

Feel free to close this issue.

[geoand]

I won't close it as it's a valid feature request. I will gladly implement it once MP Config has the appropriate architecture in place that will allow me to do so.

[hrstoyanov]

thanks. i appreciate it.

[geoand]

You bet!

[radcortez]

@hrstoyanov please check this: smallrye/smallrye-config@b6f6fba. Is this what you are looking for?

I believe that PR will provide the missing pieces to support YAML mapping properly. Please, provide me your most complicated mapping and I'll add it as a test case :)

[hrstoyanov]

Would check it this weekend! And TOML (I have not forgotten!) ... You guys rock!

[radcortez]

This is now supported with SmallRye Config @ConfigMapping, so I will consider there is an option to support the expected behaviour (even if it requires to use a different API).

Moving forward, we may replace or use as a backend implementation the SR @ConfigMapping in the Quarkus @ConfigProperties.

@hrstoyanov if you disagree, please reopen.

[radcortez]

Here is some documentation as well:
https://smallrye.io/docs/smallrye-config/mapping/mapping.html

[hrstoyanov]

Please re-open based on this onversation:
https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Need.20some.20help.20with.20Smallrye.20ConfigMapping

[radcortez]

Reopened due to some additional issues found in the implementation.

[geoand]

What are the additional implementation issues?

[radcortez]

This is in SR ConfigMapping. We have an issue where when we have defaults set with the annotation at the same level for collection types, the defaults are not being applied properly.

[geoand]

Understood, thanks

[radcortez]

@hrstoyanov this will be fixed with smallrye/smallrye-config#550.

[hrstoyanov]

Thanks @radcortez , will test as soon as itis available in the maven repo!