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.

Sign up for GitHub

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

DependencyManagement logic bug when versions are set via Maven property #4274

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

DependencyManagement logic bug when versions are set via Maven property #4274

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b7c4611
Added tests per Slack conversation with Tim te Beek
Jun 20, 2024
eb19222
Apply formatter to minimize automated review comments
timtebeek Jul 25, 2024
1328b9b
Drop tabs from indentation
timtebeek Jul 25, 2024
06f4a2c
Update rewrite-maven/src/test/java/org/openrewrite/maven/UpgradeTrans…
timtebeek Jul 25, 2024
b192e2a
Drop commented out code blocks; link test to issue
timtebeek Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
273 changes: 273 additions & 0 deletions ...ite-maven/src/test/java/org/openrewrite/maven/UpgradeTransitiveDependencyVersionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import static org.openrewrite.maven.Assertions.pomXml;

// Bonus: If there's a way to modify these assertions to do an XML-based comparison instead of a string compare please point that out. These are unnecessarily brittle as-is due to caring about whitespace.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, while I certainly understand the sentiment, we actually care deeply about the white space. Consider for instance the case where we would add the correct dependency, but it's completely left aligned, lacks newlines between tags or uses tabs instead of spaces. All of those would lead to a developer reviewing the PR to reject the changes, or require that a formatter is applied before a merge. That doesn't scale well. I hope you understand!

Suggested change
// Bonus: If there's a way to modify these assertions to do an XML-based comparison instead of a string compare please point that out. These are unnecessarily brittle as-is due to caring about whitespace.

class UpgradeTransitiveDependencyVersionTest implements RewriteTest {

@Override
Expand Down Expand Up @@ -98,4 +99,276 @@ void leavesDirectDependencyUntouched() {
""")
);
}

/*
Demonstrates that this recipe currently takes no "upgrade dependency" action when it observes that a BOM manages the dependency version and that BOM appears to already set it to the desired version number.
The problem with this seems to be that it is looking at the BOM in isolation and doesn't realize that a maven property set in the master POM is overriding a property value in the BOM
and thus causing Maven to use the version number from the parent POM's property value instead of the one seen in the BOM.

See the XML comments in the "before" XML below.
*/
@Test
void upgradeTransitiveDependencyVersion_NewIssue_NotYetFixed() {
rewriteRun(
spec -> spec.recipe(new UpgradeTransitiveDependencyVersion("commons-codec", "commons-codec", "1.15", null,
null, null, null, null, null, null))
/* .executionContext(
MavenExecutionContextView
.view(new InMemoryExecutionContext())
.setRepositories(List.of(
MavenRepository.builder().id("jenkins").uri(myOrganizationsInternalMavenRepoURL).build()
))

)*/,
pomXml(
"""
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

<modelVersion>4.0.0</modelVersion>

<parent> <!-- Sets Maven property commons-codec.version = 1.10 -->
<groupId>com.ijson.common</groupId>
<artifactId>ijson-parent-pom</artifactId>
<version>1.0.8</version>
</parent>

<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1.0.0</version>

<dependencyManagement>
<dependencies>
<dependency>
<!-- Uses Maven property commons-codec.version = 1.15 NOTE that Maven honors the property in the master pom so we end up with v1.10 on the classpath. -->
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>2.5.15</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<!-- Pulls in commons-codec transitively -->
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<version>4.5.6</version>
</dependency>
</dependencies>
</project>

""",
"""
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

<modelVersion>4.0.0</modelVersion>

<parent> <!-- Sets Maven property commons-codec.version = 1.10 -->
<groupId>com.ijson.common</groupId>
<artifactId>ijson-parent-pom</artifactId>
<version>1.0.8</version>
</parent>

<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1.0.0</version>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>commons-codec</groupId>
<artifactId>commons-codec</artifactId>
<version>1.15</version>
</dependency>
<dependency>
<!-- Uses Maven property commons-codec.version = 1.15 NOTE that Maven honors the property in the master pom so we end up with v1.10 on the classpath. -->
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>2.5.15</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<!-- Pulls in commons-codec transitively -->
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<version>4.5.6</version>
</dependency>
</dependencies>
</project>

"""
)
);
}

// Demonstrates that transitive dependency versions can be upgraded when their version is managed by a master POM.
@Test
void upgradeTransitiveDependencyVersion_WorksForMasterPomManagedDeps() {
rewriteRun(
spec -> spec.recipe(new UpgradeTransitiveDependencyVersion("org.apache.commons", "commons-lang3", "3.14.0", null,
null, null, null, null, null, null))
/*.executionContext(
MavenExecutionContextView
.view(new InMemoryExecutionContext())
.setRepositories(List.of(
MavenRepository.builder().id("jenkins").uri(myOrganizationsInternalMavenRepoURL).build()
))

)*/,
pomXml(
"""
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

<modelVersion>4.0.0</modelVersion>
<parent>
<!-- This parent's dependencyManagement has an entry for commons-lang3 setting the version to 3.9 -->
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j</artifactId>
<version>2.13.3</version>
</parent>

<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>

<dependencies>
<!-- Pulls in commons-lang3 transitively -->
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-text</artifactId>
<version>1.12.0</version>
</dependency>
</dependencies>
</project>
""",
"""
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

<modelVersion>4.0.0</modelVersion>
<parent>
<!-- This parent's dependencyManagement has an entry for commons-lang3 setting the version to 3.9 -->
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j</artifactId>
<version>2.13.3</version>
</parent>

<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>3.14.0</version>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<!-- Pulls in commons-lang3 transitively -->
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-text</artifactId>
<version>1.12.0</version>
</dependency>
</dependencies>
</project>
"""
)
);
}

// Demonstrates that transitive dependency versions can be upgraded when their version is managed by a BOM specified in the dependencyManagement section.
@Test
void upgradeTransitiveDependencyVersion_WorksForBillOfMaterialsManagedDeps() {
rewriteRun(
spec -> spec.recipe(new UpgradeTransitiveDependencyVersion("org.springframework.boot", "spring-boot-actuator", "2.7.0", null,
null, null, null, null, null, null))
/*.executionContext(
MavenExecutionContextView
.view(new InMemoryExecutionContext())
.setRepositories(List.of(
MavenRepository.builder().id("jenkins").uri(myOrganizationsInternalMavenRepoURL).build()
))

)*/,
pomXml(
"""
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

<modelVersion>4.0.0</modelVersion>

<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1.0.0</version>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>2.5.15</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<!-- Pulls in spring-boot-actuator transitively -->
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-actuator</artifactId>
<version>2.7.0</version>
timtebeek marked this conversation as resolved.
Show resolved Hide resolved
</dependency>
</dependencies>
</project>
""",
"""
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

<modelVersion>4.0.0</modelVersion>

<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1.0.0</version>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-actuator</artifactId>
<version>2.7.0</version>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>2.5.15</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<!-- Pulls in spring-boot-actuator transitively -->
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-actuator</artifactId>
<version>2.7.0</version>
</dependency>
</dependencies>
</project>
"""
)
);
}
}
Loading