Open AlexIvchenko opened 1 year ago
This seems like a legitimate bug to me. I should have added a check to see if there was a converter for the given type before assuming it was a group.
Correct. Unfortunately, it is not that easy because the mapping introspection and generation (which assumes that the interface is a group), is independent of SmallRyeConfig (no access to the current runtime discovered / registered Converters), to determine if the interface is a group or if there is a converter registered for it.
Adding a @WithConverter(...)
achieves the desired behavior, with the downside of adding the annotation.
I'm unsure if we should expose the runtime converters to the mapping generation. For instance, in Quarkus, all mappings are generated at build time. If the generation is dependent of the runtime configuration, that would mean that we cannot generate the mappings anymore (at least for runtime config). While unlikely, a converter could be registered for runtime config and it could shade an element of the mapping (a group in static config and a converted element in runtime config).
I can't think of a good way to properly choose between a group or a converter (without access to the runtime list, or metadata). Another related issue is the config processor to generate the docs. At first glance, I believe there is no way for us to reliably find which converters are going to be available at each phase, but I need to put more thought in this.
Any thoughts @dmlloyd?
I think you are right: unless you know ahead of time which interfaces have a converter, you would need the explicit annotation.
Hypothetically you could replicate the discovery process of SmallRye Config, if we have defined it rigidly enough. For ServiceLoader
-based converters this is could be fine using just what is in jandex. But implicit converter detection could easily give the wrong result, if (for example) there happens to be an of(String)
method on the group interface (which is definitely not impossible).
I guess an explicit annotation for this case is needed. Maybe though we want to make the value
argument optional for WithConverter
in order to indicate that it can use an implicit converter instead of specifying an explicit one. At least this would make it slightly less onerous.
Hypothetically you could replicate the discovery process of SmallRye Config, if we have defined it rigidly enough. For
ServiceLoader
-based converters this is could be fine using just what is in jandex. But implicit converter detection could easily give the wrong result, if (for example) there happens to be anof(String)
method on the group interface (which is definitely not impossible).
Yes, I thought about this, but we also allow access to the config builder, meaning that Converter could be registered programmatically. Converter's discovery would always be very unreliable.
I guess an explicit annotation for this case is needed. Maybe though we want to make the
value
argument optional forWithConverter
in order to indicate that it can use an implicit converter instead of specifying an explicit one. At least this would make it slightly less onerous.
Sounds reasonable.
I have library interface and two implementations which are useful for configuring my application e.g.:
I want to use
Interface
type as config property, e.g.:However, I am getting an error:
Looks like smallrye-config doesn't check if there is a registered converter for an interface and always consider interfaces as nested config mapping.
If I use a particular implementation of interfaces in config and converter, it's working:
Documentation of microprofile-config doesn't mention that it's prohibited to use converters for interfaces and probably this functionality can be implemented.