Bug/3745 maven downloading exceptions wrong maven order

[api-from-the-ion]

What's changed?

I wrote the test for this issue.

What's your motivation?

Fix this issue

• MavenDownloadingExceptions in Maven Plugin 5.14.1 #3745

Anything in particular you'd like reviewers to focus on?

I analyzed this issue and found the culprit. I wrote the test, so everybody could see it. The issue is that we have artifact a using artifact d without specifying its version. Artifact b is the parent of artifact a and first imports artifact c and then depends on d, specifying the version of d. Artifact c also depends on d but specifies an earlier version.

According to this we should have a version of d in the a that is specified in b, because b is near to the a then c and therefore overrides the version of d. But right now, this test fails.

This is because in ResolvedPom#getManagedVersion we simply go through the list and get the version of the first matched artifact. In our case, it is an imported one because it came earlier in the pom.

The exception comes because the wrong version of the artifact isn't in the repository. It is overridden and not needed by the normal build process. As soon as we correct an issue with a wrongly fetched version, the exception should also disappear.

What we clearly need here is the depth information. We need a map instead of the list, where keys would be the elements of the current list and the value is the distance to the artifact from the current artifact. So we would filter all the matched artifacts, sort them and get the nearest. In case there is more than one artifact with the same near depth, the first one that occurred in the pom should be used according to the description from Maven, see above.

I don't know if #getManagedExclusions, #getManagedScope need the same treatment.

And it would be nice to write this with the stream API.

My problem right now is that because of the end of the year, I wouldn't have enough time for this until January. So if somebody would like to correct this it would be nice.

BTW, I have to add org.jetbrains.kotlin.jvm to the Gradle dependencies at least once. Otherwise, this package is missing, and some parts of the OKHTTP tests use it.

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Not right now.

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Thanks a lot for getting this started @api-from-the-ion ! Good to have a clear test to replicate the issue. Indeed Maven can be quite particular in the way it resolves dependency versions, and we'll need to replicate that perfectly to fix this issue.

[timtebeek]

Went through this again; your test looks really good to replicate the issue; indeed we'd need to take proximity into account when parsing to correctly resolve the version of d. If you want we can merge your test as a disabled test first, to prevent conflicts with the main branch, and then iterate on a fix separately.

[api-from-the-ion]

Yes, we can merge the test, disabled. Hopefully I would have some time next week to fix this.

I describe once again the real situation here. We have a project with some modules, which use javax.enterprise:cdi-api (d). Modules (a) only describe that this dependency is provided, but don't define the version. They all have a project main pom as a parent (b). Here we first import the common pom of all our projects (c); this one declares an older version of the cdi-api. But then we directly declare the cdi-api dependency in the common parent with a newer version. As a result, we only use a new version of cdi-api everywhere in the project. Looks like a direct declaration also overwrites an imported one; in this aspect, an import isn't different from the dependency and has higher depth. So our direct declaration would override the version because it has a lower depth.

In reality, the older version isn't even downloaded in my cache, which at the end causes the final exception. I hope my test reproduces this correctly.

I once again took a look at the usage of dependencyManagementof ResolvedPom and decided against the map scenario. ResolvedManagedDependency will be extended with proximity property, and then we can use it to group dependencies by the proximity in the cases, where version is used. But in more cases using dependencyManagement there are no operations with the version value, so they can remain as they are now, just working with the list without previous grouping.

One more question: I see mergeDependencyManagement is only called from the resolveParentDependenciesRecursively. So it is filled from the POM relations caused be the parent graph. But where is the merging from the relations of the dependency graph itself? So my project depends on the something, which itself depends on something and so on? I can't find it. This should also be important in this scenario?

[api-from-the-ion]

I extended the tests (it wasn't technically complete) and made the fix for exactly this issue. Then I rewrote two other managed methods of the ResolvedPom with stream API. Hope this one looks better. This can also be partly reviewed; hope I didn't change the logic and everything else is also OK.

I used a wrong bug ticket reference (twice) in the commit message, but now it's probably too late to change it?

Now I took a look once again at places where this dependencyManagement or its getter is used. I see two kinds of usage and connected possible issues:

1. The version will be determined, so it just the same scenario as with my bug here, but the logic is elsewhere, and therefore I didn't get the bug right now. I'll fix it the same way - with usage of proximity information.
2. The cases, where ResolvedManagedDependency from dependencyManagementitself is a return value of the method. The problem here: we don't know where this object will be used. It is possible, that this object will be used to determine the version, and then we once again will have the same issue. So, my suggestion for this second step: also give the result regarding the proximity.

I'll probably hide the dependencyManagement by removing the getter and allowing the access thought some methods. Maybe some method with predicate for filtering, or something else. If this gets too complex or running time consumption will be too high, we can change the list to the map anytime and let the calling methods deal with this change. But in this scenario, we couldn't force the use of the right proximity one.

[timtebeek]

Did you mean to close this PR @api-from-the-ion ?

[api-from-the-ion]

No - I just got a merge conflict and pressed the wrong button to solve it. But now all is solved and pushed, and we can start a review. I think.

I fixed the described bug, and then another situations like this. And then I have done a little bit cleanup. If you have some comments - just write it here. We can change or drop some things.

[api-from-the-ion]

Sorry, but I have to comment your changes. You removed the new package from Gradle completely. As I already described, I had problems running tests because this is needed by OKHTTP. So at least before the first run this should be in the kts. After that, one can comment it out, or it could be removed. Both locally. But not for the first initialization.

At the end we don't need both lines, they do just the same but through the different means. But one of them should be left inside, so the next person cloning this project could run the unit tests without such troubles.

Or maybe we need both lines so Gradle would download it?

P.S.: OK, saw that we got problems in CI run if this dependency is included. We can include this in the cache or make it otherwise available. Or go in the other direction, if it is possible: how to make it available locally, because it is needed by OKHHTP using tests and isn't downloaded otherwise and therefor missed here?
One can use the version 1.9.22, meanwhile.

[timtebeek]

Hi yes those commented out lines stuck out as weird to me; would have to dive in to know exactly what you're referring to, but I'm out part of the day today.

[api-from-the-ion]

This is only IntelliJ bug, here is the link. So using another IDE or command line wouldn't probably cause this issue. But for IJ users it probably would. So if we don't like to have this dependency directly and in old version - we need to document this issue, so the new developers wouldn't have to search for the reason of not running unit tests in the freshly cloned project.

[api-from-the-ion]

Hi yes those commented out lines stuck out as weird to me; would have to dive in to know exactly what you're referring to, but I'm out part of the day today.

Don't want to bother you much, but is it possible to review this further and to merge this? Because I already solved three merge conflicts and there would be some in the future as long as this isn't reviewed and merged.

[timtebeek]

Hi yes those commented out lines stuck out as weird to me; would have to dive in to know exactly what you're referring to, but I'm out part of the day today.

Don't want to bother you much, but is it possible to review this further and to merge this? Because I already solved three merge conflicts and there would be some in the future as long as this isn't reviewed and merged.

Hi yes, I really should and thanks for your patience & resolving conflicts while this has been open. It's tough getting to all open PRs, and this one in particular contains a lot of changes so needs more attention.

One thing that I'd spotted is that you've adopted the streams API where we had for loops before. That's something I'm likely to revert in a review, and wouldn't ask of you to change given how much effort you've already put into this PR. While I like the readability aspects of the streams API, on a whole in OpenRewrite we tend to favor simpler loop constructs as they come with fewer allocations, which we're sensitive to in terms of performance. Figure give you that context before you see the change on this PR.

Know that your work is much appreciated! Just need to allocate time for a proper review to get this across the line. 🙏🏻

[api-from-the-ion]

One thing that I'd spotted is that you've adopted the streams API where we had for loops before. That's something I'm likely to revert in a review, and wouldn't ask of you to change given how much effort you've already put into this PR. While I like the readability aspects of the streams API, on a whole in OpenRewrite we tend to favor simpler loop constructs as they come with fewer allocations, which we're sensitive to in terms of performance. Figure give you that context before you see the change on this PR.

Thank you for your replay and explanation. I would like to advocate for the stream changes a little bit. ;-)

Foremost: you are responsible for this product, so the final decision is always yours. ;-)

On the other hand: if we use parallel stream, some operations like filtering should run quicker than in simple for each loop with some ifs. I wouldn't like to introduce this change directly, but we could do it. Because filtering and collecting is really independent here and could be done parallel.

If you still want to change this, then could we replace this with older language constructs like ifs and loops, but still retain the functional interfaces? As I understand you correctly, you don't like the streams in some scenarios because the build up of the stream implementation takes more time than we gain by using them? Beside the better readability? But this isn't an issue with predicates and consumers. I eliminated the repeating code of the metadata with wrapper, so one just see the different part of code at the place of the wrapper's usage - the common code is in the wrapper. And the wrapper API (the interface, the method's parameters) are functional interfaces and totally independent of the inner stream implementation.

I took a look at the current master, and there are some places beside the tests where stream API is used. So this could be a really fresh decision not to use the stream API? I know that nobody does RTFM, but maybe this information for developers should be documented somewhere?

[timtebeek]

I didn't get to the above changes before my holiday unfortunately; I'll try to pick this up when I get back!