Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maven - UpdateMavenModel incompatible with CI-Friendly Versions #2049

Closed
nmck257 opened this issue Jul 15, 2022 · 6 comments
Closed

Maven - UpdateMavenModel incompatible with CI-Friendly Versions #2049

nmck257 opened this issue Jul 15, 2022 · 6 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@nmck257
Copy link
Collaborator

nmck257 commented Jul 15, 2022

Consider running OpenRewrite on a multi-module project, using the ${revision} pattern described here: https://maven.apache.org/maven-ci-friendly.html

If using recipes which trigger UpdateMavenModel, then you'll receive log output like this:

[INFO] --- rewrite-maven-plugin:4.27.0:dryRun (default-cli) @ sample ---
[INFO] Using active recipe(s) [my.maven.modifying.recipe]
[INFO] Using active styles(s) []
[INFO] Validating active recipes...
[INFO] Project [sample-parent] Resolving Poms...
[INFO] Project [sample-parent] Parsing Source Files
[INFO] Project [sample-rest] Resolving Poms...
[INFO] Project [sample-rest] Parsing Source Files
[INFO] Project [sample-web] Resolving Poms...
[INFO] Project [sample-web] Parsing Source Files
[INFO] Project [sample-app] Resolving Poms...
[INFO] Project [sample-app] Parsing Source Files
[INFO] Project [sample] Resolving Poms...
[INFO] Project [sample] Parsing Source Files
[INFO] Running recipe(s)...
[WARNING] Illegal character in path at index 73: file:/C:/Users/me/.m2/repository/net/company/sample/sample-parent/${revision}/sample-parent-${revision}.pom
[DEBUG] java.lang.IllegalArgumentException: Illegal character in path at index 73: file:/C:/Users/me/.m2/repository/net/company/sample/sample-parent/${revision}/sample-parent-${revision}.pom
    at java.net.URI.create (URI.java:883)
    at org.openrewrite.maven.internal.MavenPomDownloader.download (MavenPomDownloader.java:239)
    at org.openrewrite.maven.tree.ResolvedPom$Resolver.resolveParentPropertiesAndRepositoriesRecursively (ResolvedPom.java:351)
    at org.openrewrite.maven.tree.ResolvedPom$Resolver.resolveParentsRecursively (ResolvedPom.java:308)
    at org.openrewrite.maven.tree.ResolvedPom$Resolver.resolve (ResolvedPom.java:297)
    at org.openrewrite.maven.tree.ResolvedPom.resolve (ResolvedPom.java:142)
    at org.openrewrite.maven.UpdateMavenModel.updateResult (UpdateMavenModel.java:133)
    at org.openrewrite.maven.UpdateMavenModel.lambda$visitDocument$3 (UpdateMavenModel.java:107)
    at org.openrewrite.marker.Markers.lambda$computeByType$0 (Markers.java:96)
    at org.openrewrite.internal.ListUtils.lambda$map$0 (ListUtils.java:138)
    at org.openrewrite.internal.ListUtils.map (ListUtils.java:120)
    at org.openrewrite.internal.ListUtils.map (ListUtils.java:138)
    at org.openrewrite.marker.Markers.computeByType (Markers.java:91)
    at org.openrewrite.maven.UpdateMavenModel.visitDocument (UpdateMavenModel.java:40)
[WARNING] These recipes would make changes to sample-parent\pom.xml:
[...]

Overall recipe execution continues, but the maven model updates are cut off by the exception, impacting recipe correctness.

@nmck257
Copy link
Collaborator Author

nmck257 commented Jul 15, 2022

Debugging: I believe the issue is in the condition at ~MavenParser:128, causing the MavenResolutionResult.parent field to not be set for projects using these special placeholders for their version. This prevents the MavenPomDownloader later created during recipe execution to not be aware of other projectPoms.

I'm looking into a fix; need to decide how robust we need to be with validating the version and relativePath tags for this case.

@tkvangorder
Copy link
Contributor

@nmck257 I have some time to dig into this. I suspect we just missing a getValue() and therefore there is no property place holder replacements.

@nmck257
Copy link
Collaborator Author

nmck257 commented Jul 15, 2022

@tkvangorder cool -- yeah, if there's a more root-cause fix than my PR (#2050), then by all means.

jkschneider pushed a commit that referenced this issue Jul 17, 2022
@nmck257
Copy link
Collaborator Author

nmck257 commented Jul 18, 2022

Should be closed with 86f9f63

@nmck257
Copy link
Collaborator Author

nmck257 commented Nov 8, 2022

I'm seeing issues with this again, albeit different exceptions. My previous PRs seem to have not 100% covered it.

@Test
fun `can connect project poms when using CI-friendly version properties`() = rewriteRun(
    pomXml("""
        <?xml version="1.0" encoding="UTF-8"?>
        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
          <modelVersion>4.0.0</modelVersion>
          <groupId>net.sample</groupId>
          <artifactId>sample</artifactId>
          <version>${'$'}{revision}</version>
          <packaging>pom</packaging>
        
          <modules>
            <module>sample-parent</module>
            <module>sample-app</module>
            <module>sample-rest</module>
            <module>sample-web</module>
          </modules>
        
          <properties>
            <revision>0.0.0-SNAPSHOT</revision>
          </properties>
        </project>
    """.trimIndent()) { spec -> spec.path("pom.xml") },
    pomXml("""
        <?xml version="1.0" encoding="UTF-8"?>
        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
          <modelVersion>4.0.0</modelVersion>
          <artifactId>sample-parent</artifactId>
          <groupId>net.sample</groupId>
          <version>${'$'}{revision}</version>
          <packaging>pom</packaging>
        
          <properties>
            <revision>0.0.0-SNAPSHOT</revision>
          </properties>
        
          <dependencyManagement>
            <dependencies>
              <dependency>
                <groupId>net.sample</groupId>
                <artifactId>sample-web</artifactId>
                <version>${'$'}{project.version}</version>
              </dependency>
              <dependency>
                <groupId>net.sample</groupId>
                <artifactId>sample-rest</artifactId>
                <version>${'$'}{project.version}</version>
              </dependency>
            </dependencies>
          </dependencyManagement>
        </project>
    """.trimIndent()) { spec -> spec.path("parent/pom.xml") },
    pomXml("""
        <?xml version="1.0" encoding="UTF-8"?>
        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
        
          <modelVersion>4.0.0</modelVersion>
          <artifactId>sample-app</artifactId>
          <packaging>jar</packaging>
        
          <parent>
            <groupId>net.sample</groupId>
            <artifactId>sample-parent</artifactId>
            <version>${'$'}{revision}</version>
            <relativePath>../parent/pom.xml</relativePath>
          </parent>
        
          <dependencies>
            <dependency>
              <groupId>net.sample</groupId>
              <artifactId>sample-rest</artifactId>
            </dependency>
        
            <dependency>
              <groupId>net.sample</groupId>
              <artifactId>sample-web</artifactId>
            </dependency>
          </dependencies>
        </project>
    """.trimIndent()) { spec -> spec.path("app/pom.xml") },
    pomXml("""
        <?xml version="1.0" encoding="UTF-8"?>
        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
        
          <modelVersion>4.0.0</modelVersion>
          <artifactId>sample-rest</artifactId>
          <packaging>jar</packaging>
        
          <parent>
            <groupId>net.sample</groupId>
            <artifactId>sample-parent</artifactId>
            <version>${'$'}{revision}</version>
            <relativePath>../parent/pom.xml</relativePath>
          </parent>
        </project>
    """.trimIndent()) { spec -> spec.path("rest/pom.xml") },
    pomXml("""
        <?xml version="1.0" encoding="UTF-8"?>
        <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
        
          <modelVersion>4.0.0</modelVersion>
          <artifactId>sample-web</artifactId>
          <packaging>jar</packaging>
        
          <parent>
            <groupId>net.sample</groupId>
            <artifactId>sample-parent</artifactId>
            <version>${'$'}{revision}</version>
            <relativePath>../parent/pom.xml</relativePath>
          </parent>
        </project>
    """.trimIndent()) { spec -> spec.path("web/pom.xml") },
)

...yields:

java.lang.AssertionError: Failed to parse sources or run recipe
[...]
Caused by: org.openrewrite.maven.internal.MavenDownloadingException: Unable to download dependency [net.sample:sample-rest:0.0.0-SNAPSHOT] from the following repositories :
[...] // my repositories

Debugging, the download request comes during resolution of the sample-app module (which has sample-rest) as a dependency.

The version check (L389) continues to be thorny... in this case, in the relevant iteration, the left side is ${revision} and the right side is 0.0.0-SNAPSHOT, as the (non-resolved) pom object doesn't have all its inherited properties available for resolution.

// The pom being examined might be from a remote repository or a local filesystem.
// First try to match the requested download with one of the project POMs.
for (Pom projectPom : projectPoms.values()) {
if (gav.getGroupId().equals(projectPom.getGroupId()) &&
gav.getArtifactId().equals(projectPom.getArtifactId()) &&
Objects.equals(projectPom.getValue(projectPom.getVersion()), projectPom.getValue(gav.getVersion()))) {
return projectPom;
}
}

A naive solution could be removing this version check.

A more intricate solution could be refactoring this method (and MavenParser::parseInputs) such that we use module interdependencies to inform project pom resolution order and make the resolved-thus-far project poms available to the MavenPomDownloader::download method, thus allowing better property resolution for the version check. Currently, project pom resolution order is arbitrary, and module analysis happens after resolution.

@nmck257 nmck257 reopened this Nov 8, 2022
@nmck257
Copy link
Collaborator Author

nmck257 commented Nov 10, 2022

cc @jkschneider

I've drafted changes to resolve the project poms in interdependency-order.

And, I've started changing the MavenPomDownloader to use ResolvedPoms (whichever ones are available) for projectPoms instead of raw Poms. But, things start falling down there -- using ResolvedPoms works well for this property resolution use case, but the recursive pom downloads which happen during resolution (which just needs raw poms) will break if we don't provide the full list of project poms.

Relevant lines which misbehave if the projectPoms collection is incomplete:

if (containingPom != null && containingPom.getRequested().getSourcePath() != null &&
!StringUtils.isBlank(relativePath)) {
Path folderContainingPom = containingPom.getRequested().getSourcePath().getParent();
if (folderContainingPom != null) {
Pom maybeLocalPom = projectPoms.get(folderContainingPom.resolve(Paths.get(relativePath, "pom.xml"))
.normalize());
// Even poms published to remote repositories still contain relative paths to their parent poms
// So double check that the GAV coordinates match so that we don't get a relative path from a remote
// pom like ".." or "../.." which coincidentally _happens_ to have led to an unrelated pom on the local filesystem
if (maybeLocalPom != null
&& gav.getGroupId().equals(maybeLocalPom.getGroupId())
&& gav.getArtifactId().equals(maybeLocalPom.getArtifactId())
&& gav.getVersion().equals(maybeLocalPom.getVersion())) {
return maybeLocalPom;
}
}
}

This issue pushes us towards providing the MavenPomDownloader both the full list of raw project poms, and also the partial list of resolved-thus-far poms.

Or -- noting that Maven does not allow arbitrary properties in the version field, only the CI-friendly version properties revision, sha1, or changelist (ref) -- we could continue passing MavenPomDownloader only the raw project poms, and add a little logic to the section mentioned in my previous comment to do a "partial, property-only resolution". Basically, identify where a project pom's parent is also a project pom, and use that to get the set of transitive properties, ignoring any transitive properties which may come from non-project parents.

I'm leaning towards that last approach. And, with that in place, I don't actually see a clear benefit to resolving the project poms in interdependency-order, but if you're interested in that code, I could open a separate PR for further evaluation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

3 participants