Skip to content

Commit

Permalink
Do not merge in parent dependency where child overrides the version o…
Browse files Browse the repository at this point in the history 
…r scope (#4434)

* Add proposed testcase showcasing the problem

* Refactor tests after clarifying actual problem

* Add testcase to maven parser test

* Child dependency takes precedence over parent

* Add comments and remove commented out line

* De-stream

* Apply suggestions from code review

---------

Co-authored-by: Laurens Westerlaken <[email protected]>
Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@Laurens-W @timtebeek
3 people authored Aug 22, 2024
1 parent bfb045b commit a87527a
Show file tree
Hide file tree
Showing 3 changed files with 254 additions and 1 deletion.
16 changes: 15 additions & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,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
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
58 changes: 58 additions & 0 deletions rewrite-maven/src/test/java/org/openrewrite/maven/MavenParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3202,4 +3202,62 @@ 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"));
}
)
)
)
)
);
}
}

0 comments on commit a87527a

Please sign in to comment.