Rewrite Maven: When parsing a SourceFile of type Maven always try to resolve dependencies in 'repo.maven.apache.org/maven2' repo with JUnit Tests

ferblaca commented 2 years ago

Running openRewrite tests and parse maven sourceFiles with XmlParser, if there are dependencies maven always tries to resolve them in:

file:/home/{USER}/.m2/repository/
https://repo.maven.apache.org/maven2

This may seem correct, but the problem is that my maven configuration in the setting.xml I have other repositories configured:

     <repositories>
        <repository>
          <id>inditex-artifacts</id>
          <url>https://my.jfrog.io/my.org/maven-public</url>
          <layout>default</layout>
          <releases>
            <enabled>true</enabled>
          </releases>
          <snapshots>
            <enabled>true</enabled>
          </snapshots>
        </repository>
      </repositories>

In fact, if in a Test sourceFile I configure a dependency that neither exists in my local repo nor exists in https://repo.maven.apache.org/maven2, since the artifact is deployed in https://my.jfrog.io/my.org/maven-public, the error when performing MavenParser.builder().build().parse(sourceFileMaven) is as follows:

org.openrewrite.maven.internal.MavenDownloadingException: Unable to download dependency from the following repositories :
  - file:/home/fbc/.m2/repository/
  - https://repo.maven.apache.org/maven2
    at org.openrewrite.maven.internal.MavenPomDownloader.download(MavenPomDownloader.java:309)

I only reproduce this behaviour when running Tests. However, in a final application using the same maven configuration it is able to resolve the artifacts in my personal repository. Could it be related to the issue #1688 ?

On the other hand, is there any way that when parsing a Maven SourceFile with dependencies I can exclude the verification that exists and is downloadable from a repository? The problem is that we want to develop recipes for dependency releases that will be deployed together with the recipes, so those maven dependencies are not yet deployed in our repository, so the Maven parse doesn't give problems sometimes.

Thank you very much!!

nmck257 commented 2 years ago

Agreed that this looks to overlap w/ #1688 (from the outside at least), and I'd love to see them both resolved (ie consistently use the config from settings.xml wherever possible, vs hardcoded repo locations)

tkvangorder commented 2 years ago

Yeah, both of these issues seem to be around needing to read the maven settings file. They are slightly different, so I am going to keep both of these issues open.

@ferblaca The artifacts need to be downloaded from a repository so that we can build an accurate semantic model for the maven project's structure.

traceyyoshima commented 2 years ago

Fixed by #1688

rdifrango commented 2 years ago

@traceyyoshima @tkvangorder in what version is this resolved as I'm still seeing this in 4.27.0 and reported it as follows:

https://github.com/openrewrite/rewrite-maven-plugin/issues/400

ferblaca commented 2 years ago

Hello @tkvangorder @traceyyoshima !

I have picked up this issue and have upgraded to Open Rewrite version 7.29.0.

My surprise is that this same problem keeps reproducing itself, i.e. when I run a maven Open Rewrite test, in the source pom resolution it is using the default properties to get the dependencies instead of the custom ones in the settings.xml.

I've created a test to make it easier to explain the problem at which indicates a change parent dependency ("com.group.foo:parent-foo:1.0.0") which I want to modify with the ChangeParentPom recipe.

My maven settings.xml has custom property:

localRepository to the path /home/fbc/repositories/maven-repo.

The repository to a custom repository:

  <repositories>
    <repository>
      <id>custom-repo</id>
      <url>https://custom.jfrog.io/maven-public</url>
      <layout>default</layout>
      <releases>
        <enabled>true</enabled>
      </releases>
      <snapshots>
        <enabled>true</enabled>
      </snapshots>
    </repository>
  </repositories>

When running the Test with Junit, either from the IDE or via terminal with maven, as it is a made up maven dependency it will give error in the Test execution indicating that it cannot resolve it. But instead of giving error indicating that it cannot find it in the custom properties of settings.xml it indicates the following:

org.openrewrite.maven.internal.MavenDownloadingException: Unable to download dependency com.group.foo:parent-foo:1.0.0 from the following repositories :
  - file:/home/fbc/.m2/repository/
  - https://repo.maven.apache.org/maven2

    at org.openrewrite.maven.internal.MavenPomDownloader.download(MavenPomDownloader.java:311)
    at org.openrewrite.maven.tree.ResolvedPom$Resolver.resolveParentPropertiesAndRepositoriesRecursively(ResolvedPom.java:351)

Could you check it?

nmck257 commented 2 years ago

@ferblaca - let me clarify the fix a bit:

For JUnit tests, you need to opt in to having OpenRewrite locate and parse your settings.xml, by setting the property org.openrewrite.test.readMavenSettingsFromDisk for your JVM process. We decided against enabling this by default, as the behavior (hunting down things on the filesystem and using it to modify test behavior) could lead to unexpected and unpredictable results in CI/CD environments.

This PR (merged manually) has more context: https://github.com/openrewrite/rewrite/pull/1955

Side note: when running recipes from rewrite-maven-plugin, it should "just work", since Maven will already know about your settings.xml and will pass that info forward to OpenRewrite.

ferblaca commented 2 years ago

@ferblaca - let me clarify the fix a bit:

For JUnit tests, you need to opt in to having OpenRewrite locate and parse your settings.xml, by setting the property org.openrewrite.test.readMavenSettingsFromDisk for your JVM process. We decided against enabling this by default, as the behavior (hunting down things on the filesystem and using it to modify test behavior) could lead to unexpected and unpredictable results in CI/CD environments.

This PR (merged manually) has more context: #1955

Side note: when running recipes from rewrite-maven-plugin, it should "just work", since Maven will already know about your settings.xml and will pass that info forward to OpenRewrite.

Hi @nmck257!

thanks for replying and clarifying the existence of this property, which I have not seen anywhere in the documentation and I think it can be useful for Junit tests (without running the maven plugin).

I've done some testing and it does indeed work by adding the -Dorg.openrewrite.test.readMavenSettingsFromDisk=true argument to the maven run. But.... how could I set this property directly in the Test java in the MavenSettings? the API of the RewriteTest.java class was modified and now I don't have it clear...

For example, how could I set this property programmatically in this test?

thank you very much!

nmck257 commented 2 years ago

@ferblaca

The most expedient option is probably adding this excerpt to your test class (though it does duplicate the exception-handling logic from RewriteTest's default implementation of the same method; don't recall if you can invoke an interface's default implementation of a method from an overriding implementation of that method):

@Override
public ExecutionContext defaultExecutionContext(SourceSpec<?>[] sourceSpecs) {
    ExecutionContext ctx = new InMemoryExecutionContext(t -> fail("Failed to parse sources or run recipe", t));
    MavenExecutionContextView.view(ctx).setMavenSettings(MavenSettings.readMavenSettingsFromDisk(ctx));
    return ctx;
}

cc @tkvangorder , @sambsnyd There might be a tidier solution now that SourceSpec supports a customizeExecutionContext function. And, fair feedback that the property isn't findable in the documentation -- I can help with a PR on that if I can get pointed to the right place to put it.

openrewrite / rewrite

Rewrite Maven: When parsing a SourceFile of type Maven always try to resolve dependencies in 'repo.maven.apache.org/maven2' repo with JUnit Tests #1801