Quarkus 1.6.0.Final does not resolve profile-scoped variables correctly in the configuration

[evisentin]

the following configuration does not work on Quarkus 1.6.0.Final:

#----------------------------------------
my.application.http.port=8081
%dev.my.application.http.port=8082
%test.my.application.http.port=8083

quarkus.http.port=${my.application.http.port}
#----------------------------------------

when I use mvn quarkus:dev the service picks the wrong port: 8081 ( it ignores the %dev profile and resolves the wrong version of 'my.application.http.port' ),
while it works perfectly with Quarkus 1.5.2.Final

This configuration, on the other hand, works fine with both versions

#----------------------------------------
quarkus.http.port=8081
%dev.quarkus.http.port=8082
%test.quarkus.http.port=8083
#----------------------------------------

[geoand]

cc @radcortez

[radcortez]

Checking.

[radcortez]

Hey @dmlloyd, just want to check something with you.

This is happening because our interceptor Expression expansion does not reevaluate the entire chain, so to resolve the profile (if exists) in the expression. We had that discussion, about having a way to reevaluate the chain, or just proceed to the next interceptor and we decided to just proceed.

In this case, we would need to reevaluate the chain. In fact, if we look into the old ConfigExpander:
2a95fc3#r40557184

We were calling an explicit getValue which would reevaluate the entire config, instead of proceeding to the next interceptor like we are doing in the ExpressionConfigSourceInterceptor:
https://github.com/smallrye/smallrye-config/blob/7fbc28930b14428891d42a649daafb40b5a64530/implementation/src/main/java/io/smallrye/config/ExpressionConfigSourceInterceptor.java#L38

What do you think? Should we add the capability to reevaluate the entire chain, so profiles can also be resolved in expressions?

[dmlloyd]

Possibly. But in this particular case, would it not make more sense to put the profile interceptor later in the chain than the expression interceptor?

[radcortez]

That may be also a possibility.

In that case, configurations like:

"my.prop", "1",
 "%prof.my.prop", "${my.prop}"

Will fall into a recursion and won't work. Which they didn't anyway in Quarkus 1.5, so maybe this isn't a big deal and we assume that a profile lookup on the same property is not supported.

[dmlloyd]

The interception mechanism was meant to solve these recursion issues, so it would be nice to do this "right" and order the interceptors to get the desired behavior.

[radcortez]

Yes, but in one case you want to resolve an expansion with a profile, so you need the order to be Expand -> Profile, and in the other you want to resolve the profile and then expand, so you need Profile -> Expand.

BTW the case were you get the recursion if you reorder the interceptors, is when you point a profile property to the main property (with the same name). Regular cases work just fine. So maybe it doesn't make much sense to set a %prof.my.prop to my.prop.

[evisentin]

@radcortez, forgive me if I break into your conversation.

Of course it does not make any sense at all to set a %prof.my.prop to my.prop. I totally agree with you.

I just want to point out the case I used in the issue is not this.

# same property, default + 2 profiles
my.application.http.port=8081
%dev.my.application.http.port=8082
%test.my.application.http.port=8083

# assignment ( not to the same property )
quarkus.http.port=${my.application.http.port}

[dmlloyd]

Yes, but in one case you want to resolve an expansion with a profile, so you need the order to be Expand -> Profile, and in the other you want to resolve the profile and then expand, so you need Profile -> Expand.

I don't think that's true. There doesn't seem to be any reason or use case to justify doing profile mapping first, since you get the same result from the perspective of the expression expander. In fact there's a good reason to put it towards the end of the chain: no other interceptor really needs any awareness of profiles.

BTW the case were you get the recursion if you reorder the interceptors, is when you point a profile property to the main property (with the same name). Regular cases work just fine. So maybe it doesn't make much sense to set a %prof.my.prop to my.prop.

[radcortez]

@evisentin no worries, feel free to provide any feedback :)

Yes, we understand the reported use case. I was just discussing about some other possible implications. Considering the example cases:

#Case A
app.http.port=8081,
%dev.app.http.port=8082,
%test.app.http.port=8083,
real.port=${app.http.port}

#Case B
app.http.port=${dev.app.http.port}
%dev.app.http.port=8081

Currently, Case B works and Case A doesn't, because order is Profile resolution first and then Expansion. If we change the order, Expansion first and then Profile, Case A now works, but Case B stops working, which is fine by me.

If we want to have Case A and B to work together, we would need the ability to reevaluate the chain, like described here: #10643 (comment)

[dmlloyd]

@evisentin no worries, feel free to provide any feedback :)

Yes, we understand the reported use case. I was just discussing about some other possible implications. Considering the example cases:

#Case A
app.http.port=8081,
%dev.app.http.port=8082,
%test.app.http.port=8083,
real.port=${app.http.port}

#Case B
app.http.port=${dev.app.http.port}
%dev.app.http.port=8081

Currently, Case B works and Case A doesn't, because order is Profile resolution first and then Expansion. If we change the order, Expansion first and then Profile, Case A now works, but Case B stops working, which is fine by me.

I do not believe this is true (though I guess you meant the expression to be ${%dev.app.http.port}). Profile resolution should (or at least could) still pass through properties where the profile was explicitly given, if we want this case to continue to work.

[radcortez]

Ops, I've messed something up on the Case B sample. It should look like this:

#Case B
app.http.port=8081
%dev.app.http.port=${app.http.port}

In this case:

Expand app.http.port
Profile lookup on app.http.port and %dev.app.http.port (to pick the highest one)
Profile %dev.app.http.port wins
Expand to app.http.port
Loop :)

[dmlloyd]

Ops, I've messed something up on the Case B sample. It should look like this:

#Case B
app.http.port=8081
%dev.app.http.port=${app.http.port}

In this case:

Expand app.http.port
Profile lookup on app.http.port and %dev.app.http.port (to pick the highest one)
Profile %dev.app.http.port wins
Expand to app.http.port
Loop :)

OK, yeah that case was always expected to fail AFAICT, for the same reason that foo=${foo} is always expected to fail.

[radcortez]

Yes, there is no way around foo=${foo}, but for %dev.foo=${foo} it may be different since they can be considered two properties.

Anyway, not much point to continue the discussion around this. Let's just assume that this is not supported and I'll fix the interceptor order in SmallRye Config.

Thanks @dmlloyd !

[radcortez]

Pushed a fix to SmallRye Config and Quarkus update here: #10615