Changing AddDependencyVisitor so it is able to broaden the scope of a direct dependency if needed

[steve-aom-elliott]

What's changed?

• Changed the logic for AddDependency / AddDependencyVisitor so that if it encounters an existing dependency with a narrower scope and an equal / lower version compared to the requested, it will instead opt to broaden the scope of the dependency.
• If the existing dependency has the same scope, but a lower version, the version will be upgraded
• Dropped acceptable request of import scope in favour of compile, as import is only valid for dependencies listed in dependencyManagement

What's your motivation?

• org.openrewrite.maven.AddDependency may add a duplicate when scopes are different #6426

Anything in particular you'd like reviewers to focus on?

• When requesting a dependency that is available transitively, but at a lower version than requested, should we still be preventing it from adding a direct dependency if acceptTransitive is used? Currently if acceptTransitive is true, it will not add the dependency, but if acceptTransitive is false or not set, then it will add the dependency.
  • I feel like either we should be consistent between these two for the given situation or we should add to the option description for version to explicitly call out that it will be ignored if acceptTransitive is true and a transitive dependency matched on groupId and artifactId is found.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Give how we already have a recipe to change scope I think it's fine indeed not to add a dependency if it is already present:
https://github.com/openrewrite/rewrite/blob/main/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyScope.java

Thanks for looking into it!

[steve-aom-elliott]

@sambsnyd Hoping to get your input on this change for AddDependencyVisitor to make it more broad when it's detecting whether the dependency already exists (existing at any scope rather than the same scope essentially)

[sambsnyd]

I agree the scope check should change, but just removing it isn't quite right.

In addDependencyDoesntAddWhenExistingDependencyWithScopeRuntime test the recipe is configured to add the dependency to the compile scope. If a larger composite recipe or migration is telling AddDependency to do this it is presumably because symbols from that jar are or will be needed to compile the code going forward. The runtime scope does not achieve that, so leaving the dependency in the runtime scope may result in errors. The most appropriate thing the recipe can do here is change the scope to compile (or, equivalently, removing the <scope>)

Consider the inverse situation: If the existing dependency starts off in the compile scope and the recipe is configured to add it to the runtime scope then no change needs to be made. Anything in the compile scope makes its way into the runtime scope automatically. So the most appropriate thing for the recipe to do in that case is nothing.

[timtebeek]

I think you were aware already, but there was that one more test failure in AddDependency now:

AddDependencyTest > addDependencyUpgradesWhenExistingDependencyHasNarrowerScope() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "project/pom.xml":
    diff --git a/project/pom.xml b/project/pom.xml
    index 2040b2e..da1a094 100644
    --- a/project/pom.xml
    +++ b/project/pom.xml
    @@ -6,7 +6,7 @@ 
             <dependency>
                 <groupId>com.google.guava</groupId>
                 <artifactId>guava</artifactId>
    -            <version>29.0-jre</version>
    +            <version>28.0-jre</version>
                 <scope>compile</scope>
             </dependency>
         </dependencies>
    ] 

[steve-aom-elliott]

I think you were aware already, but there was that one more test failure in AddDependency now:

AddDependencyTest > addDependencyUpgradesWhenExistingDependencyHasNarrowerScope() FAILED
    org.opentest4j.AssertionFailedError: [Unexpected result in "project/pom.xml":
    diff --git a/project/pom.xml b/project/pom.xml
    index 2040b2e..da1a094 100644
    --- a/project/pom.xml
    +++ b/project/pom.xml
    @@ -6,7 +6,7 @@ 
             <dependency>
                 <groupId>com.google.guava</groupId>
                 <artifactId>guava</artifactId>
    -            <version>29.0-jre</version>
    +            <version>28.0-jre</version>
                 <scope>compile</scope>
             </dependency>
         </dependencies>
    ] 

Yes, currently working through the logic to try to restrict changes so that:

• If the requested would be a version downgrade -> make no changes, even if it would broaden scope (this is a safety case for situations where we have multiple recipes for different release versions of something to prevent it from taking an already correct dependency and changing it to a broader scope instance of it despite having asked for a lower version. I've seen our recipes where we later narrow a scope and upgrade a version, and this check prevents undoing that)
• If the requested would either leave the version unchanged or upgrade it -> if scope would be equal or greater -> try and upgrade version and broaden scope as applicable (those recipes can bail early already)
• If the scope would be a narrowing -> make no changes

[timtebeek]

Looks like we've had a previous effort here as well; any lessons there we can apply here? Do we need both or just one?

• Maven AddDependency widens scope instead of duplicating dependency #5813

[steve-aom-elliott]

Pushed up the very rough last point I reached but am working on rewriting it more simply here: https://github.com/openrewrite/rewrite/tree/6426-v2 where I tried to take a less elaborate approach. Still hitting some issues with exception markers doubling up such that I'm working on resolving still.

[steve-aom-elliott]

Finished reworking the code so now it handles the placeholders, broadening of scope and upgrading of version correctly. Will review for improvements / cleanup tomorrow.

[timtebeek]

We're now seeing the lang3 version dropped in a second cycle in

RemoveRedundantDependencyVersionsTest > supportSpringBootDependenciesPlugin() FAILED
    org.opentest4j.AssertionFailedError: [Expected recipe to complete in 1 cycle, but took at least one more cycle. Between the last two executed cycles there were changes to "build.gradle"] 
    expected: 
      "plugins {
          id "java"
          id("org.springframework.boot") version "3.5.6"
          id("io.spring.dependency-management") version "1.1.7"
      }
  
      repositories {
          mavenCentral()
      }
  
      dependencies {
          implementation('org.springframework.boot:spring-boot')
          implementation("org.apache.commons:commons-lang3:3.14.0")
          implementation("org.openrewrite:rewrite-core")
          implementation("javax.validation:validation-api")
          implementation("org.openrewrite.recipe:rewrite-quarkus")
      }
  
      dependencyManagement {
          dependencies {
              dependency("javax.validation:validation-api:2.0.1.Final")
              dependencySet('org.openrewrite:8.62.2') {
                  entry 'rewrite-core'
              }
          }
          imports {
              mavenBom "org.openrewrite.recipe:rewrite-recipe-bom:3.14.1"
          }
      }"
     but was: 
      "plugins {
          id "java"
          id("org.springframework.boot") version "3.5.6"
          id("io.spring.dependency-management") version "1.1.7"
      }
  
      repositories {
          mavenCentral()
      }
  
      dependencies {
          implementation('org.springframework.boot:spring-boot')
          implementation("org.apache.commons:commons-lang3")
          implementation("org.openrewrite:rewrite-core")
          implementation("javax.validation:validation-api")
          implementation("org.openrewrite.recipe:rewrite-quarkus")
      }
  
      dependencyManagement {
          dependencies {
              dependency("javax.validation:validation-api:2.0.1.Final")
              dependencySet('org.openrewrite:8.62.2') {
                  entry 'rewrite-core'
              }
          }
          imports {
              mavenBom "org.openrewrite.recipe:rewrite-recipe-bom:3.14.1"
          }
      }"
        at app//org.openrewrite.test.LargeSourceSetCheckingExpectedCycles.afterCycle(LargeSourceSetCheckingExpectedCycles.java:97)
        at app//org.openrewrite.RecipeScheduler.runRecipeCycles(RecipeScheduler.java:97)
        at app//org.openrewrite.RecipeScheduler.scheduleRun(RecipeScheduler.java:42)
        at app//org.openrewrite.Recipe.run(Recipe.java:451)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:400)
        at app//org.openrewrite.test.RewriteTest.rewriteRun(RewriteTest.java:132)
        at app//org.openrewrite.gradle.RemoveRedundantDependencyVersionsTest.supportSpringBootDependenciesPlugin(RemoveRedundantDependencyVersionsTest.java:741)

The change itself is good; it taking two cycles is unfortunate. Sharing here to decide what to do.

[steve-aom-elliott]

@timtebeek So what's odd here is that test (which involves Gradle) failing on this run considering I've only changed the Maven AddDependency