Allow pom download failures

[adastraperangusta]

What's changed?

This PR adds a org.openrewrite.allowPomDownloadFailure property , inspired by the existing org.openrewrite.test.readMavenSettingsFromDisk property. When set to true, the MavenPomDowloader class doesn't throw an exception when it doesn't find a suitable pom to download for a dependency, and instead returns a minimal object.

What's your motivation?

Some maven projects are correct (mave wise) but don't have all the pom's for their dependencies (see #4687), which prevents maven recipes to work on these kind of project.

• Fix openrewrite maven recipes shoudn't try to resolve every single pom #4687

Have you considered any alternatives or workarounds?

This is a workaround, I have the feeling that the real solution would be to use maven libraries to build the maven model instead of re-implementing it. But it would be quite an heavy change.

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 for exploring the fix here @adastraperangusta !

Saving others a click by copying the test failure here:

MavenPomDownloaderTest > WithNativeHttpURLConnectionAndTLS > mirrorsOverrideRepositoriesInPom() FAILED
    org.opentest4j.AssertionFailedError: 
    expected: "https://artifactory.moderne.ninja/artifactory/moderne-cache/"
     but was: "https://artifactory.moderne.ninja/artifactory/moderne-cache"
        at app//org.openrewrite.maven.internal.MavenPomDownloaderTest$WithNativeHttpURLConnectionAndTLS.mirrorsOverrideRepositoriesInPom(MavenPomDownloaderTest.java:251)

[adastraperangusta]

Hello,

I have applied the changes and rebased the PR.
Everything seems OK now.

[timtebeek]

Thanks for the work here so far @adastraperangusta ; I worry that we might be a little too permissive with this change, as also pointed out on this comment. Right now we allow any non existing GAV, whereas it seems Maven perhaps only allows a missing pom.xml if the matching .jar is present. What are your thoughts on replicating that here?

[adastraperangusta]

Yes, I understand that. It's hack, which doesn't bother that much because when I try to apply a recipe based on the MavenParser, the project is OK maven wise. And no "random" dependency would go missing.

However, I'm ready to give a try to check if jar is present before allowing download failure or not, but how is it possible to detect this when we are in org.openrewrite.maven.internal.MavenPomDownloader#download() method ?
It's beyond my current understanding of the code.

[timtebeek]

Appreciate your willingness to help out! I don't quite know just yet how we'd handle that; perhaps we can throw and catch a specific exception when the pom can not be found, and catch that some place where it's easier to check the .jar before rethrowing?

[adastraperangusta]

I have added test cases for expected behaviour :

• trying to download a pom without a corresponding jar should fail
• trying to download a pom with a corresponding jar shouldn't fail

The first test fails since the logic isn't implemented yet.

Now after reading the code I have the feeling that the only mean to achieve this is to double each pom request. Everytime a request to the pom URI fails for a repo, we should try the same request for the jar's URI.
If jar is found, then don't throw failure.
BTW by doing like this we can drop the configuration property.

[timtebeek]

Thanks for the tests already! Also let me know if you'd like me to step in; the tests will help me explore alternatives more easily.

[timtebeek]

A quick note to let you know here that I'm exploring options here to handle this without any option to enable, but merely catching the failure to download the pom, and then looking to see if the .jar can be accessed, and if so (and only then) continue as normal.

[adastraperangusta]

Hello,

I have update the logic for the 'file' URI type so that when pom download is failed but a local jar is found, the RawPom is built from the jar information which allow to return the needed pom information.
With this logic the property isn't needed anymore, so I removed it.

The 'http' scheme remains to be improved the same way. I have added the corresponding tests cases.

Regards,

[timtebeek]

Thanks @adastraperangusta ! I'll try to rework what you've done into the approach I had in mind, as we also want to account for jars that can be downloaded; especially appreciate the tests you have written; they will make it easier to refactor. 🙏🏻

[adastraperangusta]

I have added the logic for remote pom branch.
The download() method is growing complicated and surely needs some refactor.

Regards,

[timtebeek]

Thanks again @adastraperangusta ; I'll take it from here to avoid conflicts. I've started a parallel effort in this PR to compare solutions:

• Tolerate missing pom when jar is present #4777

[timtebeek]

Thanks a lot for the thoughtful work here @adastraperangusta ! Especially appreciate you writing tests for each combination of with/without jar and using file or http transfer.

I've added some light polish like switching to a separate method that skips downloading the jar bytes, as we're only interested in whether the response was successful. I've also added some tests in ResolvedPomTest to ensure we only attempt to reach out once there.