openrewrite
Maven - ExcludeDependency excludes transitive dependencies which are already test-scoped by a parent
See org.springframework.boot:spring-boot-starter-actuator:2.5.14 for an example.
If you run ExcludeDependency for org.junit.vintage:junit-vintage-engine on a project with the above actuator dependency, as the JUnit4To5 recipe does, then it adds this junit exclusion to actuator dependency. But, if you run mvn dependency:tree on the actuator dependency, org.junit.vintage:junit-vintage-engine does not appear:
$ mvn dependency:tree -f org/springframework/boot/spring-boot-starter-actuator/2.5.14/spring-boot-starter-actuator-2.5.14.pom
[INFO] Scanning for projects...
[INFO]
[INFO] -------< org.springframework.boot:spring-boot-starter-actuator >--------
[INFO] Building spring-boot-starter-actuator 2.5.14
[INFO] --------------------------------[ jar ]---------------------------------
[INFO]
[INFO] --- maven-dependency-plugin:2.8:tree (default-cli) @ spring-boot-starter-actuator ---
[INFO] org.springframework.boot:spring-boot-starter-actuator:jar:2.5.14
[INFO] +- org.springframework.boot:spring-boot-starter:jar:2.5.14:compile
[INFO] | +- org.springframework.boot:spring-boot:jar:2.5.14:compile
[INFO] | | \- org.springframework:spring-context:jar:5.3.20:compile
[INFO] | | +- org.springframework:spring-aop:jar:5.3.20:compile
[INFO] | | +- org.springframework:spring-beans:jar:5.3.20:compile
[INFO] | | \- org.springframework:spring-expression:jar:5.3.20:compile
[INFO] | +- org.springframework.boot:spring-boot-autoconfigure:jar:2.5.14:compile
[INFO] | +- org.springframework.boot:spring-boot-starter-logging:jar:2.5.14:compile
[INFO] | | +- ch.qos.logback:logback-classic:jar:1.2.11:compile
[INFO] | | | +- ch.qos.logback:logback-core:jar:1.2.11:compile
[INFO] | | | \- org.slf4j:slf4j-api:jar:1.7.32:compile
[INFO] | | +- org.apache.logging.log4j:log4j-to-slf4j:jar:2.17.2:compile
[INFO] | | | \- org.apache.logging.log4j:log4j-api:jar:2.17.2:compile
[INFO] | | \- org.slf4j:jul-to-slf4j:jar:1.7.36:compile
[INFO] | +- jakarta.annotation:jakarta.annotation-api:jar:1.3.5:compile
[INFO] | +- org.springframework:spring-core:jar:5.3.20:compile
[INFO] | | \- org.springframework:spring-jcl:jar:5.3.20:compile
[INFO] | \- org.yaml:snakeyaml:jar:1.28:compile
[INFO] +- org.springframework.boot:spring-boot-actuator-autoconfigure:jar:2.5.14:compile
[INFO] | +- org.springframework.boot:spring-boot-actuator:jar:2.5.14:compile
[INFO] | +- com.fasterxml.jackson.core:jackson-databind:jar:2.12.6.1:runtime
[INFO] | | +- com.fasterxml.jackson.core:jackson-annotations:jar:2.12.6:runtime
[INFO] | | \- com.fasterxml.jackson.core:jackson-core:jar:2.12.6:runtime
[INFO] | \- com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.12.6:runtime
[INFO] \- io.micrometer:micrometer-core:jar:1.7.12:compile
[INFO] +- org.hdrhistogram:HdrHistogram:jar:2.1.12:compile
[INFO] \- org.latencyutils:LatencyUtils:jar:2.0.3:runtime
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.258 s
[INFO] Finished at: 2022-06-03T10:39:32-04:00
[INFO] ------------------------------------------------------------------------
Debugging ExcludeDependency, org.junit.vintage:junit-vintage-engine is found on this "path":
org/springframework/boot/spring-boot-starter-actuator/2.5.14/spring-boot-starter-actuator-2.5.14.pom
org/springframework/boot/spring-boot-starter/2.5.14/spring-boot-starter-2.5.14.pom
org/springframework/boot/spring-boot-starter-logging/2.5.14/spring-boot-starter-logging-2.5.14.pom
org/apache/logging/log4j/log4j-to-slf4j/2.17.2/log4j-to-slf4j-2.17.2.pom
The pom for log4j-to-slf4j 2.17.2 does show org.junit.vintage:junit-vintage-engine, but, if you check the parent pom log4j 2.17.2, then you'll see that org.junit.vintage:junit-vintage-engine (with other junit4 dependencies) appear as managed dependencies with test scope. This scoping is effective on log4j-to-slf4j, so the junit4 dependencies aren't actually exported. mvn dependency:tree on log4j-to-slf4j confirms this.
But, rewrite-maven gets fooled, sees junit-vintage-engine as a transitive dependency, and excludes it. The root issue might even affect other logic reliant on analyzing transitive dependencies; not sure.
@nmck257 Yep, this definitely does not look right, thank you for the detailed description, it is really helpful when tracking these types of issues down.
Dropping some notes since I did a little research on this recently:
I crafted some test cases:
@Test // failure -- adds the exclusion as described in this issue
fun springParent() = assertUnchanged(
before = """
<project>
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
<version>2.5.14</version>
</parent>
<groupId>com.example</groupId>
<artifactId>demo</artifactId>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-actuator</artifactId>
</dependency>
</dependencies>
</project>
""",
recipe = ExcludeDependency("junit", "junit", null)
)
@Test // success
fun customParent() = assertUnchanged(
dependsOn = arrayOf("""
<project>
<groupId>com.example</groupId>
<artifactId>parent</artifactId>
<version>1.0.0</version>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-actuator</artifactId>
<version>2.5.14</version>
</dependency>
</dependencies>
</dependencyManagement>
</project>
"""),
before = """
<project>
<parent>
<groupId>com.example</groupId>
<artifactId>parent</artifactId>
<version>1.0.0</version>
</parent>
<groupId>com.example</groupId>
<artifactId>demo</artifactId>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-actuator</artifactId>
</dependency>
</dependencies>
</project>
""",
recipe = ExcludeDependency("junit", "junit", null)
)
@Test // success
fun directVersion() = assertUnchanged(
before = """
<project>
<groupId>com.example</groupId>
<artifactId>demo</artifactId>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-actuator</artifactId>
<version>2.5.14</version>
</dependency>
</dependencies>
</project>
""",
recipe = ExcludeDependency("junit", "junit", null)
)
Seems that my original issue description doesn't fully explain it -- if it did, then all three of these test cases would fail.
Instead, after some debugging in ResolvedPom::resolveDependencies, I think I see the logical issue.
In this section, we're deciding whether a (nested) dependency should be a dependency of the root pom, based on whether or not the (nested) dependency's scope puts it in the runtime classpath: https://github.com/openrewrite/rewrite/blob/a5e1847acf05c06139d5cff5ababe75e50437ac9/rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java#L618-L621
That code calls here: https://github.com/openrewrite/rewrite/blob/a5e1847acf05c06139d5cff5ababe75e50437ac9/rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java#L645-L658
...which calls here: https://github.com/openrewrite/rewrite/blob/a5e1847acf05c06139d5cff5ababe75e50437ac9/rewrite-maven/src/main/java/org/openrewrite/maven/tree/ResolvedPom.java#L270-L278
It looks like we're using the current, aggregate dependencyManagement of the root pom to infer the scope of the nested dependency, to decide whether the nested dependency is a transitive runtime dependency of the project which directly references it.
It seems like that's not quite how Maven works. If a nested dependency is not a runtime dependency of a project when viewed in isolation, then that nested dependency will not become a transitive runtime dependency of that project, regardless of what the dependencyManagement looks like in the project which indirectly references it.
Assuming that's all correct, then I think I see what needs to change (in those codeblocks above), buuut, I'm not sure that the existing code produces the data we need (ie a bottom-up view of dependency management active on each dependency at each depth).
Will keep investigating, maybe soon