[MNG-6401] - Cannot interpolate property in proxy port of settings.xml

[jkatada]

Following this checklist to help us incorporate your contribution quickly and easily:

• [x] Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
• [x] Each commit in the pull request should have a meaningful subject line and body.
• [x] Format the pull request title like [MNG-XXX] - Fixes bug in ApproximateQuantiles, where you replace MNG-XXX with the appropriate JIRA issue. Best practice is to use the JIRA issue title in the pull request title and in the first line of the commit message.
• [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• [x] Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
• [x] You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.

• [x] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• [x] In any other case, please file an Apache Individual Contributor License Agreement.

[michael-o]

As far as I understand this PR, you are interpolating the settings before it is fed to SettingsXpp3Reader?

[michael-o]

Can you also provide an IT for that? @hboutemy, @rfscholte This is a very interesting case and I can confirm that our approach is incorrect. We need to interpolate the XML first and then unmarshal to objects and not the other way around. Everything, but String will be killed otherwise.

[jkatada]

As far as I understand this PR, you are interpolating the settings before it is fead to SettingsXpp3Reader?

Yes, you are right.

Can you also provide an IT for that?

I added the test to maven-integration-testing project. The PR is here. It tests not only int field (proxy.port) but also boolean field (proxy.active).

[rfscholte]

Looks interesting. It also looks more efficient. I would like to avoid effectiveSettings here, because effective is the result after merging. Better call it interpolatedSettings.

[jkatada]

@rfscholte I changed the variable name. Please check.

[hboutemy]

on String interpolation vs Object interpolation: String based interpolation was replaced by Object interpolation a few years ago (perhaps during the switch from Maven 2 to Maven 3, I need to get precise facts if necessary): I don't precisely know why (is Object interpolation more robust, against XML injection?)

what I see in this precise case of proxy port interpolation: it's an int, then it's hard to do Object interpolation on "${param}" value once it has been transfrmed to a String... at least, this give us the good root cause: if we want interpolation with Object implementation, we need to change the field type to String

I know that we have such a case with booleans, where we do not put the effective field type in the descriptor but String for more flexibility: in this boolean case, it was about inheritance, to make the difference between inheriting and overriding with a value. Then there is a hand-written getter to get the field as boolean (instead of the internal String)

I don't know if we have such cases in Maven core with ints, but the previous pattern hand-writing a typed getter to int should do the job.

Now what's the best solution for now:

• switch back to String based interpolation instead of Object interpolation?
• change the field from int to String and add the hand-written getter?

at this time, I don't have strong opinion: I wanted to write and share the analysis.

Now I need to dig into the reasons why we switched from String based interpolation to Object based: this is where we should find the best view on what issue could arise when getting back to former solution

[michael-o]

I know that we have such a case with booleans, where we do not put the effective field type in the descriptor but String for more flexibility: in this boolean case, it was about inheritance, to make the difference between inheriting and overriding with a value. Then there is a hand-written getter to get the field as boolean (instead of the internal String)

This is another issue with our interpolation. Can you point me to that handwritten spot?

I don't know if we have such cases in Maven core with ints, but the previous pattern hand-writing a typed getter to int should do the job.

The core model does not use ints, but only booleans for activation.

switch back to String based interpolation instead of Object interpolation?

I don't see any quality difference between those two, from user POV

change the field from int to String and add the hand-written getter?

This is going to be hard because a lot of our code relies on the int #getPort().

If this PR breaks our approach w/o understanding previous motives, I'd stall it for 3.6.0 or later.

[hboutemy]

on handwritten, look at optional field in maven.mdo I'm working on another branch to apply the same strategy: I'm currently running core-its to check it works as expected, will report soon

[hboutemy]

branch 6401-2 pushed, with port and active fields as String and handwritten methods to provide compatibility

[michael-o]

@hboutemy , looks good to me. Did you have a chance to run the new IT on?

[agschrei]

are there any additional steps pending for this fix? I just ran into this issue myself and if there is anything I can do to get this merged I'd be happy to get involved.

[michael-o]

@agschrei As far as I remember we have discussed this with @rfscholte and there was some logical error either in the expectation or the PR. Can't remember where.

[berenddeboer]

So we're now close 5 years after this bug was first report. Numerous developers have run into this bug and wasted their time googling this. Was this really necessary????

[jkatada]

@michael-o My understanding of the current status of this PR is as follows

1. My PR is a regression to string interpolation. This PR should not be merged.
2. The MNG-6401-2 branch by @hboutemy can resolve this issue while keeping object interpolation.

The MNG-6401-2 branch can be rebased onto master and passed my new IT.

[INFO] Tests run: 925, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 396.822 s - in org.apache.maven.it.IntegrationTestSuite

So I think the MNG-6401-2 branch should be merged.

Is there anything else I can do to close this PR?

[WilliamYuhangLee]

Hi, can we please get some traction on this? It's now the middle of 2023 and yet Maven still has silly issues like this 😢

[jjaychen1e]

Any updates?

[gnodet]

Superseded by #1194