-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for properties with dynamic names #991
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ejba! Thank you very much for the PR. Highly appreciated.
While the original issue is reported against the YAML source, the problem is not in the YAML source implementation but in how the overall mechanism of multiple profiles is implemented.
The current approach works fine for non-dynamic lookups. The problem is when mapping a Map
, the config system has to rely on the list of property names available to discover the dynamic key to place in the Map
. If the list of property names does not contain the desired property in the active profile, the config system cannot discover it and the mapping will either fail or not map at all.
I believe a better fix may be provided in ProfileConfigSourceInteceptor
by rewriting the multiple profile keys to be part of the active profile, which would solve the issue for any configuration source and not just YAML. There is a relocate rule to rewrite the multiple profile keys in SmallRyeConfigBuilder
, which may also require some adjustments:
smallrye-config/implementation/src/main/java/io/smallrye/config/SmallRyeConfigBuilder.java
Lines 235 to 264 in 6bd3669
@Override | |
public ConfigSourceInterceptor getInterceptor(final ConfigSourceInterceptorContext context) { | |
Map<String, String> relocations = new HashMap<>(); | |
relocations.put(SmallRyeConfig.SMALLRYE_CONFIG_PROFILE, Config.PROFILE); | |
List<MultipleProfileProperty> multipleProfileProperties = new ArrayList<>(); | |
Iterator<String> names = context.iterateNames(); | |
while (names.hasNext()) { | |
String name = names.next(); | |
if (name.length() > 0 && name.charAt(0) == '%') { | |
NameIterator ni = new NameIterator(name); | |
String profileSegment = ni.getNextSegment(); | |
List<String> profiles = convertProfile(profileSegment.substring(1)); | |
if (profiles.size() > 1) { | |
multipleProfileProperties | |
.add(new MultipleProfileProperty(name, name.substring(profileSegment.length()), profiles)); | |
} | |
} | |
} | |
// Ordered properties by least number of profiles. Priority to the ones with most specific profiles. | |
for (MultipleProfileProperty multipleProfileProperty : multipleProfileProperties) { | |
for (String profile : multipleProfileProperty.getProfiles()) { | |
relocations.putIfAbsent("%" + profile + multipleProfileProperty.getRelocateName(), | |
multipleProfileProperty.getName()); | |
} | |
} | |
return new RelocateConfigSourceInterceptor(relocations); | |
} |
657b0ca
to
7c2d69d
Compare
Hi @radcortez! Thank you with your kind words. I should discuss the solution before opening a PR, apologies. |
@Test | ||
void multipleProfilesWithDynamicKey() { | ||
SmallRyeConfig config = new SmallRyeConfigBuilder() | ||
.withSources(config(SMALLRYE_CONFIG_PROFILE, "\"%common,prof\"", "config_ordinal", "1000")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't expect the profiles to be quoted or include the %
. Profiles are usually set as:
smallrye.config.profile=prod
smallrye.config.profile=common,prod
smallrye.config.profile=one,two,three
The %
is just to mark that the next name in the configuration property is the profile to be used, but it shouldn't be required when setting the profile.
Did you find a situation where you need the profiles to be quoted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't find any practical example that requires quoting profile names. I will remove this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe a better fix may be provided in ProfileConfigSourceInteceptor by rewriting the multiple profile keys to be part of the active profile ...
Looking at your comment, maybe I missed something. Why does ProfileConfigSourceInteceptor
need to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider:
@ConfigMapping
interface MyConfig {
Map<String, String> foo();
}
foo.bar=baz
%dev.foo.bar=baz
%common,dev.foo.bar=baz
In this case, because the second segment of foo.bar
is dynamic (the Map
key), we cannot query it directly. We need to iterate the list of property names and find the ones in the Map
namespace of foo
.
With profiles, I guess we could search the profile namespace too (or multiple ones like in this case). On the other hand, when we added profiles, to not explode the list of property names, or even confuse users with the added prefix, we rewrite profile configuration names in their non-profile version (active):
smallrye-config/implementation/src/main/java/io/smallrye/config/ProfileConfigSourceInterceptor.java
Lines 60 to 68 in 5547830
@Override | |
public Iterator<String> iterateNames(final ConfigSourceInterceptorContext context) { | |
final Set<String> names = new HashSet<>(); | |
final Iterator<String> namesIterator = context.iterateNames(); | |
while (namesIterator.hasNext()) { | |
names.add(normalizeName(namesIterator.next())); | |
} | |
return names.iterator(); | |
} |
But in the case of multiple profile configuration names, this is not working. I believe that rewriting the multiple property names in their non-profiled format should do the trick (as long as the name found has any of the profiles active).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ejba, how are you? Are you still planning to work on this? Thanks!
Signed-off-by: Emanuel Alves <[email protected]>
Closing in favour of #1080. |
Fixes #986