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

Do not merge in parent dependency where child overrides the version or scope #4434

Merged
merged 9 commits into from
Aug 22, 2024
Merged

Do not merge in parent dependency where child overrides the version or scope #4434

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4037e3a
Add proposed testcase showcasing the problem
Laurens-W Aug 21, 2024
1f09dfb
Merge branch 'main' into add-dependency-override-parent
Laurens-W Aug 21, 2024
dc09e6c
Refactor tests after clarifying actual problem
Laurens-W Aug 21, 2024
b318f42
Merge branch 'main' into add-dependency-override-parent
Laurens-W Aug 22, 2024
f852b72
Add testcase to maven parser test
Laurens-W Aug 22, 2024
72d2900
Child dependency takes precedence over parent
Laurens-W Aug 22, 2024
6bbbbd6
Add comments and remove commented out line
Laurens-W Aug 22, 2024
f02be6b
De-stream
Laurens-W Aug 22, 2024
e87676b
Apply suggestions from code review
timtebeek Aug 22, 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
18 changes: 17 additions & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,8 @@ private void resolveParentDependenciesRecursively(List<Pom> pomAncestry) throws
for (Profile profile : pom.getProfiles()) {
if (profile.isActive(activeProfiles)) {
mergeDependencyManagement(profile.getDependencyManagement(), pomAncestry);
// If two profiles are activated that both define the same dependency but with different scope/version
// The first one now takes precedence, which is not how maven processes them, especially at runtime
timtebeek marked this conversation as resolved.
Show resolved Hide resolved
mergeRequestedDependencies(profile.getDependencies());
}
}
Expand Down Expand Up @@ -508,7 +510,21 @@ private void mergeRequestedDependencies(List<Dependency> incomingRequestedDepend
//If it's empty, we ensure to create a mutable list.
requestedDependencies = new ArrayList<>(incomingRequestedDependencies);
} else {
requestedDependencies.addAll(incomingRequestedDependencies);
// When a child dependency has overriden a parent dependency (either version or scope)
// We shouldn't add the parent definition when requested; the child takes precedence
for (Dependency incReqDep : incomingRequestedDependencies) {
boolean found = false;
for (Dependency reqDep : requestedDependencies) {
if (reqDep.getGav().getGroupId().equals(incReqDep.getGav().getGroupId())
&& reqDep.getArtifactId().equals(incReqDep.getArtifactId())) {
found = true;
break;
}
}
if (!found) {
requestedDependencies.add(incReqDep);
}
}
}
}
}
Expand Down
181 changes: 181 additions & 0 deletions rewrite-maven/src/test/java/org/openrewrite/maven/AddDependencyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.jupiter.params.provider.ValueSource;
import org.openrewrite.DocumentExample;
import org.openrewrite.Issue;
import org.openrewrite.java.ChangePackage;
import org.openrewrite.java.JavaParser;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;
Expand Down Expand Up @@ -1283,6 +1284,186 @@ void addDependenciesOnEmptyProject() {
);
}

@Test
void dependencyThatIsTransitivelyProvidedWithWrongScopeShouldBeAdded() {
rewriteRun(
spec -> spec
.parser(JavaParser.fromJavaVersion()
.dependsOn(
"""
package main.java.checkerframework.checker.nullness.qual;
public @interface NonNull {}
"""
)
)
.recipes(
new AddDependency("org.checkerframework", "checker-qual", "3.44.0",
null, null, null, "main.java.checkerframework..*", null, null, null, null,
true),
new ChangePackage("main.java.checkerframework", "org.checkerframework", true)
),
mavenProject("parent",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>parent</artifactId>
<version>1</version>
<modules>
<module>child</module>
</modules>
<dependencies>
<!-- Has transitive dependency on org.checkerframework:checker-qual -->
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>31.1-jre</version>
</dependency>
</dependencies>
</project>
"""
),
mavenProject("child",
srcMainJava(
java(
"""
import main.java.checkerframework.checker.nullness.qual.NonNull;
class Foo {}
""",
"""
import org.checkerframework.checker.nullness.qual.NonNull;
class Foo {}
"""
)
),
pomXml(
"""
<project>
<parent>
<groupId>com.mycompany.app</groupId>
<artifactId>parent</artifactId>
<version>1</version>
<relativePath>../pom.xml</relativePath>
</parent>
<artifactId>child</artifactId>
<dependencies>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>31.1-jre</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
""",
"""
<project>
<parent>
<groupId>com.mycompany.app</groupId>
<artifactId>parent</artifactId>
<version>1</version>
<relativePath>../pom.xml</relativePath>
</parent>
<artifactId>child</artifactId>
<dependencies>
<dependency>
<groupId>org.checkerframework</groupId>
<artifactId>checker-qual</artifactId>
<version>3.44.0</version>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>31.1-jre</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
"""
)
)
)
);
}

@Test
void dependencyThatIsTransitivelyProvidedWithCorrectScopeShouldNotBeAdded() {
rewriteRun(
spec -> spec
.parser(JavaParser.fromJavaVersion()
.dependsOn(
"""
package main.java.checkerframework.checker.nullness.qual;
public @interface NonNull {}
"""
)
)
.recipes(
new AddDependency("org.checkerframework", "checker-qual", "3.44.0",
null, null, null, "main.java.checkerframework..*", null, null, null, null,
true),
new ChangePackage("main.java.checkerframework", "org.checkerframework", true)
),
mavenProject("parent",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>parent</artifactId>
<version>1</version>
<modules>
<module>child</module>
</modules>
<dependencies>
<!-- Has transitive dependency on org.checkerframework:checker-qual -->
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>31.1-jre</version>
</dependency>
</dependencies>
</project>
"""
),
mavenProject("child",
srcTestJava(
java(
"""
import main.java.checkerframework.checker.nullness.qual.NonNull;
class Foo {}
""",
"""
import org.checkerframework.checker.nullness.qual.NonNull;
class Foo {}
"""
)
),
pomXml(
"""
<project>
<parent>
<groupId>com.mycompany.app</groupId>
<artifactId>parent</artifactId>
<version>1</version>
<relativePath>../pom.xml</relativePath>
</parent>
<artifactId>child</artifactId>
<dependencies>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>31.1-jre</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
"""
)
)
)
);
}

private AddDependency addDependency(@SuppressWarnings("SameParameterValue") String gav) {
return addDependency(gav, null, null, null);
}
Expand Down
61 changes: 61 additions & 0 deletions rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3202,4 +3202,65 @@ void multiModulePropertyVersionShouldAddModules() {
)
);
}

@Test
void childDependencyDefinitionShouldTakePrecedence() {
rewriteRun(
mavenProject("parent",
pomXml(
"""
<project>
<groupId>com.mycompany.app</groupId>
<artifactId>parent</artifactId>
<version>1</version>
<modules>
<module>child</module>
</modules>
<dependencies>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>31.1-jre</version>
</dependency>
</dependencies>
</project>
"""
),
mavenProject("child",
pomXml(
"""
<project>
<parent>
<groupId>com.mycompany.app</groupId>
<artifactId>parent</artifactId>
<version>1</version>
<relativePath>../pom.xml</relativePath>
</parent>
<artifactId>child</artifactId>
<dependencies>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>31.1-jre</version>
<scope>test</scope>
</dependency>
</dependencies>
</project>
""",
spec -> spec.afterRecipe(pomXml -> {
MavenResolutionResult res = pomXml.getMarkers().findFirst(MavenResolutionResult.class).orElseThrow();
assertThat(res.getDependencies().get(Scope.Compile)).isEmpty();
assertThat(res.getDependencies().get(Scope.Runtime)).isEmpty();
assertThat(res.getDependencies().get(Scope.Provided)).isEmpty();
assertThat(res.getDependencies().get(Scope.Test)).isNotEmpty().anyMatch(dep -> dep.getGroupId().equals("com.google.guava") && dep.getArtifactId().equals("guava"));
}
)
)
)
)
);
}

// TODO replicate above test case but with profiles

timtebeek marked this conversation as resolved.
Show resolved Hide resolved
}
Loading