Optional property in ConfigMapping becomes mandatory #36150
Comments
|
cc @radcortez |
|
@juan-antonio-alba-db have you also tried with Quarkus 3.4.1? |
|
Yep, still happening in this version [INFO] -------------------------< my-groupId:my-test >------------------------- --- Help improve Quarkus ---Listening for transport dt_socket at address: 5005 |
|
I had that discussion with @radcortez a little while back about this. This is actually the desired behaviour. The optional does get validated because of the see discussion here; smallrye/smallrye-config#945 (comment) |
Correct! Thank you for the explanation @manofthepeace :) What marks the |
I did - and came to a different conclusion. This is a major issue for us. We look at this from a top-down perspective in terms of the config tree. Given the OP's example, we see this as "If the We are currently brooding about strategies to mitigate the pain of migrating to v3. |
|
Btw, if you work with Kotlin you might be able to work around this with the following beast. Fighting ugliness with even more ugliness. interface Server {
fun host(): String
@WithName("port")
fun portWithoutDefault(): Optional<Int>
}
fun Server.port(): Int = this.portWithoutDefault().orElse(8080) |
|
To complement my short snippet above here's a full reproducer including our ugly workaround with two unit tests in Kotlin. import io.smallrye.config.ConfigMapping
import io.smallrye.config.SmallRyeConfigBuilder
import io.smallrye.config.WithName
import io.smallrye.config.source.yaml.YamlConfigSource
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import java.util.Optional
class ConfigMappingTest {
@Test
fun `should map config despite missing child branch`() {
// given
// when
val parent: Parent = SmallRyeConfigBuilder()
.withSources(
YamlConfigSource(
"test", """
parent:
name: parent
"""
)
)
.addDefaultInterceptors()
.withMapping(Parent::class.java)
.build().getConfigMapping(Parent::class.java)
// then
assertThat(parent.child()).isEmpty
}
@Test
fun `should map config with default child age`() {
// given
// when
val parent: Parent = SmallRyeConfigBuilder()
.withSources(
YamlConfigSource(
"test", """
parent:
name: parent
child:
name: child
"""
)
)
.addDefaultInterceptors()
.withMapping(Parent::class.java)
.build().getConfigMapping(Parent::class.java)
// then
assertThat(parent.child().get()._age()).isEmpty
assertThat(parent.child().get().age()).isEqualTo(12)
assertThat(parent.child().get().hobbies()).isEmpty()
}
}
@ConfigMapping(prefix = "parent")
interface Parent {
fun name(): String
fun child(): Optional<Child>
interface Child {
fun name(): String
// This fails due to https://github.com/quarkusio/quarkus/issues/36150.
// @WithDefault("12")
// fun age(): Int
@WithName("age")
fun _age(): Optional<Int>
// Contrary to what you might expect, you do NOT get an empty set if no properties are configured.
// If you need to support that, you have to map to an Optional<Set<String>>.
// fun hobbies(): Set<String>
@WithName("hobbies")
fun _hobbies(): Optional<Set<String>>
// Maps are different from lists and sets (i.e. collections for indexed properties).
// If no properties are configured, you'll get an empty map!
fun grades(): Map<String, Float>
}
}
fun Parent.Child.age(): Int = this._age().orElse(12)
fun Parent.Child.hobbies(): Set<String> = this._hobbies().orElse(setOf())
Thinking about this some more, it occurred to me that maybe, just maybe, this "Optional + WithDefault" combination does not work as we expect for the same reason #845 is an open feature request. Both this and #845 affect use cases where you need to treat a whole branch in your config tree as optional. They may or may not be there - usually based on the value of some other property that you evaluate in the application. Maybe this is not seen as a viable use case? |
|
This was changed in smallrye/smallrye-config#919. There are a few motivations behind the change:
These are separate things. It is possible to implement such a feature (with the same restrictions).
I guess the issue here is how we look at the I still think that In this case, if the interface Connection {
@WithParentName
Optional<Host> host();
@WithDefault("8080")
int port();
interface Host {
String host();
}
} |
Indeed, for this simple example your proposed alternative is fine. My use case, however, is the same one discussed in #845: making entire branches of the config tree optional. A real-life but anonymized example of the YAML file: my-app:
# feature A is entirely optional, if disabled this whole branch is irrelevant
feature-a:
behavior-x:
type: ABC
validation:
enabled: false
claims:
a: b
c: d
roles: lol
behavior-z:
strategy:
header:
foo: bar
fuu: baz
identity-token:
name: whatever
transport: whatever
run-as:
foo: bar
feature-b:
# etc.In the
|
|
As an example, we use this on Quarkus to create a default datasource if none is found set by the user, but we don't want to force the user to set some property to trigger the optional creation. Also, in our case, if we don't use "feature A" the config is not included in the application. In those cases, you even get a warning stating that the configuration is unknown. Could that be a solution for you? At the moment, I need help finding an easy solution to support both scenarios with your structure. Yes, it worked before, but it required us to create the configuration programmatically, not to force the user to set a random property for the default datasource. I'm happy to discuss possible solutions for this. |
|
Ran into this one myself like a brick wall. It probably makes total sense if you're working on the implementation, but as a user, I expected the same thing as @marcelstoer. They are many use cases where, for example, an authentication config group would be optional, but if populated by any attribute, the required and Example: Just because I have a At the very least, this should be documented somewhere. |
We did that because with the previous behavior, retrieving the default value was only possible if another property of the tree was specified. It also had some inconsistencies... so I assume, that if the user explicitly set one of the defaults or an optional tree (but nothing else), should we trigger the creation or not? Right now, the rule is fairly simple: if a value definition exists (no matter where) it always triggers the tree creation. I'm open to discuss other rules. The challenge is that selectively creating the tree depending on where the value definition exists, may cause additional confusion to users. |
Describe the bug
Migrating to 3.2.6.Final from 2.16.8 we have detected that Optional properties in a ConfigMapping are becoming mandatory
Expected behavior
Optional property should not be mandatory to start up the quarkus application
Actual behavior
Error is raised because the property is not provided
How to Reproduce?
Reproduce
Make use of the Connection in the quarkus app, and try to start up the application which will raise the error
The problem appears when adding
@WithDefault("8080"), in case of removing this annotation the error disappears but then there is not default.Output of
uname -aorverNo response
Output of
java -versionjava version "17.0.7" 2023-04-18 LTS
GraalVM version (if different from Java)
No response
Quarkus version or git rev
3.2.6.Final
Build tool (ie. output of
mvnw --versionorgradlew --version)Maven home: /tools/apache-maven-3.8.2 Java version: 17.0.7, vendor: Oracle Corporation, runtime:
Additional information
No response
The text was updated successfully, but these errors were encountered: