Devmode, Regression in 1.6.0: Cannot override property from profile via system property

[famod]

Describe the bug
Overriding a property that is defined in the %dev profile via system property (-D...) does not work anymore.

Expected behavior
System property must take precedence (as in 1.5.2 or lower)

Actual behavior
Profile in properties/yaml takes precedence.

To Reproduce
Steps to reproduce the behavior:

1. Clone https://github.com/famod/quarkus-quickstarts
2. git switch devmode-port-override
3. cd getting-started
4. mvn clean verify
5. mvn quarkus:dev starts on port 2081
6. mvn quarkus:dev -Dquarkus.http.port=0 (or any other port) still starts on port 2081
7. remove profile prefix from property or remove entire property in application.properties
8. mvn quarkus:dev -Dquarkus.http.port=0 starts with desired port

Configuration

%dev.quarkus.http.port=2081

Screenshots
n/a

Environment (please complete the following information):

• Output of uname -a or ver:

  MINGW64_NT-10.0-18363 BIGBLUE 3.0.7-338.x86_64 2019-11-21 23:07 UTC x86_64 Msys
  

• Output of java -version:

  openjdk version "11.0.7" 2020-04-14
  OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.7+10)
  OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.7+10, mixed mode)
  

• GraalVM version (if different from Java): n/a
• Quarkus version or git rev: 1.6.0.Final
• Build tool (ie. output of mvnw --version or gradlew --version): Apache Maven 3.6.3

[famod]

/cc @geoand

[geoand]

Darn... We really need to clarify the behavior we want and if we settle that the current behavior is the appropriate one, we need to make sure we communicate it in the release as a breaking change.

cc @radcortez @gsmet

[geoand]

And BTW, thanks for reporting and making the case clystal clear

[radcortez]

Interesting, this is the behaviour that we implemented in SmallRye Config (a property overrides a profile based configuration with higher priority) and that lead to #10320.

Let me have a closer look.

[famod]

FWIW, I have implemented the following workaround (via yaml, not sure this works with properties):

"%dev":
  quarkus:
    http:
      port: ${someproject.quarkus.http.port:2081}

And then mvn quarkus:dev -Dsomeproject.quarkus.http.port=0.

[radcortez]

Ok, I think the issue relates with #10320 resolution.

Quarkus before 1.6 prioritises properties from higher ordinals even if they are not part of the active profile. The implementation we have was also correct.

What changed was the execution order between resolution of profiles / expansion, leading to the issue in #10320 where a property was being expanded and failing because there was no expanding property. We fixed this by looking only into the profile, but that was actually a wrong fix, because it now introduced the bug described here.

I already have an idea on how to fix this. I'll send a PR shortly.

[geoand]

Great, thanks!

[radcortez]

An additional detail:

The profile / expansion order was changed, so we could avoid a circular dependency with a configuration like:\

my.prop=1
%dev.my.prop=${my.prop}

This means that a property is expanded before picking the profile. For the case of #10320 it ended up introducing another issue where we require the ordinal to know which one to use. Since expansion happens first, it ended up failing because it couldn't expand the value, but it didn't matter anyway because the property would never be used.

[radcortez]

@geoand I have the fix in SmallRye Config, which requires to update the dependency (I can release a 1.8.1 just with this fix), but I can extend the interceptor in Quarkus and apply the fix there. Which one do you prefer? :)

[geoand]

Given that we just released 1.6.0.Final, I think that we have time to go the proper route with the smallrye release.

cc @gsmet

[radcortez]

Ok, I'll try to add some more tests in Quarkus itself to pick up these regressions.

[geoand]

Sounds like a plan 👍

[gsmet]

Yes as long as the SmallRye Config update is minimal, I'm fine with backporting it to 1.6.1.Final.

[radcortez]

@gsmet @geoand this is current milestone for SmallRye Config 1.8.2 to fix this issue:
https://github.com/smallrye/smallrye-config/milestone/10?closed=1

It has a couple of more fixes. Probably the biggest change is around the YAML Config Source to fix smallrye/smallrye-config#216 and #6297. Let me know if this is ok to release.

[geoand]

I am on PTO for a couple days, but a quick look at the fixed issues look good to me

[gsmet]

Yeah, same here, I would say let's go with it.

[radcortez]

Ok, pushing a release. Sorry @geoand I missed to check you were on PTO :(

[geoand]

No worries 😊

…

On Thu, Jul 9, 2020, 19:34 Roberto Cortez ***@***.***> wrote: Ok, pushing a release. Sorry @geoand <https://github.com/geoand> I missed to check you were on PTO :( — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10570 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBMDP5SEARUJKUEDHS42EDR2XWRNANCNFSM4OUSK2LQ> .

[radcortez]

I've pushed the fix to SmallRye Config and updated Quarkus here:
#10615