Quarkus: Config properties of inactive profiles are being expanded in 1.6.0.CR1

[sirAlexander]

Describe the bug
Just tried this version of quarkus as there is a critical fix I need i.e https://github.com/quarkusio/quarkus/issues/10215

but on running mvn quarkus:dev the application seems to ignore all Dev config in the application.properties i.e. %dev.quarkus .datasource..... and instead uses prod properties.

Seems like quite the regression. @geoand @gsmet

[geoand]

Very serious indeed, thanks for reporting!

I'll look into it, but I have a feeling that it might be caused by https://github.com/quarkusio/quarkus/pull/9184. If that turns out to be true, I'll revert that PR

[geoand]

I was not able to reproduce the problem with either 1.6.0.CR1 or Quarkus from master.

Can you provide some more details @sirAlexander ?

[geoand]

Actually, let me try one more thing

[geoand]

I tried another thing:

quarkus.http.cors=false
%dev.quarkus.http.cors=true

and reading the value of io.quarkus.vertx.http.runtime.HttpConfiguration#corsEnabled in dev mode and it turned out to be false as expected.

I also tried a flyway example with

quarkus.flyway.migrate-at-start=false
%dev.quarkus.flyway.migrate-at-start=true

where again things worked as expected.
@sirAlexander can you provide a reproducer for the issue you are seeing?

[sirAlexander]

@geoand this is what I changed in my pom.xml, not sure if it is the correct way to do it

<quarkus-plugin.version>1.6.0.CR1</quarkus-plugin.version>
        <quarkus.platform.artifact-id>quarkus-bom</quarkus.platform.artifact-id>
        <quarkus.platform.group-id>io.quarkus</quarkus.platform.group-id>
        <quarkus.platform.version>1.6.0.CR1</quarkus.platform.version>

so using quarkus-bom instead of quarkus-universal-bom

[geoand]

That is correct. But since I am unable to reproduce the issue you are seeing, it would be great if you could provide a small reproducer project that exhibits this behavior.

[sirAlexander]

@geoand I've updated the reproducer here if that can help >> https://github.com/sirAlexander/quarkus-reproducer

running mvn quarkus:dev reproduces the issue

[geoand]

I'll take a look, thanks

[geoand]

OK, I understand what the problem is.

Basically Quarkus is trying to expand the env vars in:

quarkus.datasource.jdbc.url=jdbc:mysql://${DB_HOST}/${DB_NAME}

even though there is a dev mode property that overrides quarkus.datasource.jdbc.url:

%dev.quarkus.datasource.jdbc.url=jdbc:mysql://localhost:3306/web_analytics

So a simple workaround for the time being is to use quarkus.datasource.jdbc.url=jdbc:mysql://${DB_HOST:dummy}/${DB_NAME:dummy}

However I am in favor of reverting #9184 unless we can get a fix in for 1.6.0.Final. WDYT @radcortez, @gsmet ?

[sirAlexander]

cheers @geoand will put the workaround in place for now. Thanks

[sirAlexander]

only worry is, this would mean lots of quarkus users out there in the wild will need to go over and update all their configs

[geoand]

Yes, which is why I propose we revert the PR specified above if we cannot come up with a fix in time for 1.6.0.Final.

[radcortez]

Let me try to have a look and see if we can find a fix for it. If not, we can revert.

[radcortez]

Ok, the issue is related with this:
https://github.com/smallrye/smallrye-config/pull/275#discussion_r404816069

When we implemented this in SmallRye Config, we did it a way that profile only takes priority if it has an higher ordinal. This would allow you to override the configuration name without having to resort to use the profile + config name.

Unfortunately, the old Profile resolution in Quarkus just blindly uses any profile config that it found, even if it is in a very low ordinal source:
https://github.com/quarkusio/quarkus/commit/2a95fc3400a30af061c73a1cf43b870232afdb4e#r40209567

So the new code, ends up introducing a backwards incompatible change. In this case the issue we see might look strange because it is related with expansion. The main property needs to be evaluated as well to compare priorities, so expansion is also evaluated, before comparing the values ordinals.

I believe the resolution is fairly simple. We can just drop the current SmallRye Profile Resolution and override it with an implementation compatible with the previous DeploymentProfileConfigSource.

[geoand]

Do you plan to open a PR @radcortez ? A test case would also be great to have

[radcortez]

Yes, working on possible fix right now :)

[geoand]

Excellent 👍

[radcortez]

This probably also caused the issue reported in the original PR: https://github.com/quarkusio/quarkus/pull/9184#issuecomment-650180456

[radcortez]

10322 Should fix this.

@sirAlexander do you have any chance to pull it, build it locally and test it with your code? That would be super helpful. Thank you :)

[geoand]

@radcortez I tried your PR and it does fix the issue, thanks!

I think we really need a Quarkus integration test that will ensure we don't run into this again in the future. Do you mind if I push a commit to your branch adding one?

[radcortez]

Not at all. Go ahead.

[radcortez]

Ok, the issue is related with this:
smallrye/smallrye-config#275 (comment)

When we implemented this in SmallRye Config, we did it a way that profile only takes priority if it has an higher ordinal. This would allow you to override the configuration name without having to resort to use the profile + config name.

Unfortunately, the old Profile resolution in Quarkus just blindly uses any profile config that it found, even if it is in a very low ordinal source:
2a95fc3#r40209567

So the new code, ends up introducing a backwards incompatible change. In this case the issue we see might look strange because it is related with expansion. The main property needs to be evaluated as well to compare priorities, so expansion is also evaluated, before comparing the values ordinals.

I believe the resolution is fairly simple. We can just drop the current SmallRye Profile Resolution and override it with an implementation compatible with the previous DeploymentProfileConfigSource.

@dmlloyd Do you have any thoughts about this? Eventually to support it like we discussed it is a breaking change for Quarkus.

[dmlloyd]

Ok, the issue is related with this:
smallrye/smallrye-config#275 (comment)
When we implemented this in SmallRye Config, we did it a way that profile only takes priority if it has an higher ordinal. This would allow you to override the configuration name without having to resort to use the profile + config name.
Unfortunately, the old Profile resolution in Quarkus just blindly uses any profile config that it found, even if it is in a very low ordinal source:
2a95fc3#r40209567
So the new code, ends up introducing a backwards incompatible change. In this case the issue we see might look strange because it is related with expansion. The main property needs to be evaluated as well to compare priorities, so expansion is also evaluated, before comparing the values ordinals.
I believe the resolution is fairly simple. We can just drop the current SmallRye Profile Resolution and override it with an implementation compatible with the previous DeploymentProfileConfigSource.

@dmlloyd Do you have any thoughts about this? Eventually to support it like we discussed it is a breaking change for Quarkus.

Hmmm. That is not how I remember it working, and I thought we had tests to verify. I hope the new implementation (and corresponding spec work) was not incorrect due to my bad memory...

[radcortez]

Yes, you can confirm the Quarkus implementation here:
https://github.com/quarkusio/quarkus/commit/2a95fc3400a30af061c73a1cf43b870232afdb4e#r40209567, which returns the profile value straight away if it finds one.

In SmallRye impl and MP spec, we did implement it differently (by using the source ordinal), which in my opinion is correct. It just introduced here a possible backwards compatible issue to Quarkus.

At some point, we may need to decide to change it or introduce some additional configuration to define the profile resolution mode (any vs ordinal).

[dmlloyd]

The challenge is that "correct" is meaningful only if that behavior supports the use cases. But I agree we can always change it if needed.

[gsmet]

@radcortez I fail to see how the use case described here is not something we want to support out of the box? It looks like a perfectly valid one.

[radcortez]

@gsmet you mean Profile resolution with priority?

It is supported in SmallRye Config, but it does introduce a backwards incompatible change with Quarkus, since Quarkus profile implementation, that we replaced in #9184 only lookups the profile value no matter the priority. So, it certain cases, depending on how you have your config setup, you may experience different behaviour.

What we did in #10322 was to override SmallRye Config Profile resolution to be the same as Quarkus pre #9184.

Now, if we think we want to support this we either had a behaviour flag so we keep some compatibility, or we need to clearly communicate we are changing the behaviour. What do you think?

[gsmet]

I was probably unclear. What I don't understand is that in the aforementioned case, the old behavior looks like what we would want i.e. the dev profile overriding the default value and the default value thus not being resolved.

I don't understand why we would resolve the default value if overridden by the dev profile?

[radcortez]

Me and @dmlloyd had a discussion about this.

The issue is when you set up a profile value in a really low priority source, for instance microprofile-config.properties and then you have no way to override it, or to empty it in higher ordinal sources.

[dmlloyd]

Another way to look at it is, let's say I'm not aware of how the config is structured for a given project. I just want to set property XXX to YYY. I would expect that using -DXXX=YYY will always just work, even if there was a profile-specific value for that property in a lower config source that I wasn't aware of.