Change Dependency recipes do not work with dependencies defined in jvm-test-suites

[sihutch]

I am using

• OpenRewrite v6.17.1
• Maven/Gradle plugin v6.17.1
• Gradle v7.6.4

How are you running OpenRewrite?

I am running OpenRewrite using the Gradle Plugin, and my project is a single module project.

My build.gradle has the same dependency defined in the main dependencies and also with a jvm-test-suites section as shown below

dependencies {
    rewrite("org.openrewrite.recipe:rewrite-java-dependencies:1.14.0")
    implementation"javax.servlet:javax.servlet-api:3.1.0"
}

testing {
    suites {
        test {
            useJUnitJupiter()
            dependencies {
                implementation project()
                implementation"javax.servlet:javax.servlet-api:3.1.0"
            }
        }
    }
}

When I run either of the two recipes shown below only the javax.servlet:javax.servlet-api:3.1.0 in the main dependencies section is changed.

1. org.openrewrite.java.dependencies.ChangeDependency

type: specs.openrewrite.org/v1beta/recipe
name: com.sihutch.ChangeDependencyExample
displayName: Change Gradle or Maven dependency example
recipeList:
  - org.openrewrite.java.dependencies.ChangeDependency:
      oldGroupId: javax.servlet
      oldArtifactId: javax.servlet-api
      newGroupId: jakarta.servlet
      newArtifactId: jakarta.servlet-api
      newVersion: 6.x

1. org.openrewrite.gradle.ChangeDependency

type: specs.openrewrite.org/v1beta/recipe
name: com.sihutch.ChangeGradleDependencyExample
displayName: Change Gradle dependency example
recipeList:
    - org.openrewrite.gradle.ChangeDependency:
          oldGroupId: javax.servlet
          oldArtifactId: javax.servlet-api
          newGroupId: jakarta.servlet
          newArtifactId: jakarta.servlet-api
          newVersion: 6.x

I have created an example project here

I was expecting the dependencies in the jvm-test-suite block to also be changed. Can you please confirm whether this is a bug or something that is not implemented ? If that later, is there another recipe that you can recommend that can handle the jvm-test-suite case ?

Thanks

Si

[timtebeek]

I think you're correct in expecting such cases to be handled by org.openrewrite.gradle.ChangeDependency; I'd consider that something not yet implemented. You're welcome to help explore a fix, perhaps starting with a unit test on a draft PR similar to this test.

[sihutch]

Thanks for getting back to me @timtebeek. I have created a PR with the failing test.

[shanman190]

#3665 (comment)

I'll share this oldie, but goodie reference. I envision a state that any of the Gradle dependency recipes work correctly over JVM test suites. As you can tell from the comment and what you've probably gathered from the recipes' behaviors, this is yet another location for dependencies to be declared and handled correctly.

This particular aspect will be made a little bit easier when the Trait feature lands for Gradle dependencies, but in fair warning fixing this issue will be a challenging one for a beginner. If you're still up for it, we can absolutely assist you in your journey.

[sihutch]

Thanks @shanman190. If someone could outline a high-level solution, along with any areas of potential difficulty, or point me to some specific areas to familiarize myself with, then I'd be happy to take a look and see if I can make progress.

[shanman190]

So for this one, I'd start by debugging the existing recipes to see if the dependencies block is being picked up as a DependencyHandlerSpec. If it is then there might be something else going wrong with the recipe.

If that isn't the case, then I'd reuse something that I formulated for the MigrateGradleEnterpriseToDevelocity recipe in so far as detecting where we are at in the buildscript. (See https://github.com/openrewrite/rewrite/blob/main/rewrite-gradle%2Fsrc%2Fmain%2Fjava%2Forg%2Fopenrewrite%2Fgradle%2Fplugins%2FMigrateGradleEnterpriseToDevelocity.java#L204-L220). From there, it's then making sure to accept the new locations into the existing update or change code paths.

The Groovy parser a lot of the time will leave off the type information and rely upon its dynamic dispatch, so that's what makes the initial variable hard to tie down.

[sihutch]

Thanks @shanman190. I will take a look

[sihutch]

@shanman190 The issue was that the dependencies block is not being picked up as a DependencyHandlerSpec.

I have updated my PR. This is not a complete solution, but was sufficient to make my failing test pass and to illustrate the problem.

For a more complete solution, I have a few questions

1. The 'testing' extension that provides the DSL is only added when the jvm-test-suite plugin is applied. Is it acceptable to add this to the RewriteGradleProject as I have done in my commit, or is there a better way to handle plugins?
2. In my commit example, I have handled the' test' suite, but suites can be named anything. Do you know of a mechanism for handing a child of suite with any name and then delegating that closure? For example, the test method below could be any method name.

interface SuitesSpec {
    void test (@DelegatesTo(strategy=Closure.DELEGATE_ONLY, value=SuiteTestSpec) Closure cl)
}

[shanman190]

Hi @sihutch,

That's unfortunate to hear that the method was not being type attributed as a DependencyHandlerSpec, but neither is it terribly surprising either.

As far as more stable remediation, what I'd probably now pivot to is code based upon a new PR of mine found here which introduces the Gradle Trait API for identifying dependencies.

I think the trait should receive one small update near the beginning of the block to confirm that the current method invocation is contained within a parent method invocation named dependencies. With each of Gradle's usages, they have always placed dependencies within a block named dependencies.

So envisioning something about like this:

1. Update the GradleTraitMatcher#test method

if (!isWithin("dependencies")) {
    return null;
}

2. Within ChangeDependency make updates to no longer use the old style dependency test and instead use the GradleDependency.Matcher trait matcher.

GradleDependency.Matcher gradleDependencyMatcher = new GradleDependency.Matcher();
if (!matcher.get(getCursor()).isPresent()) {
    // This method invocation is not an add dependency invocation
    return m;
}

[sihutch]

@shanman190 - Cool. That's a much better solution

[sihutch]

@shanman190 I have picked this issue up again as this PR #4354 seems to have stalled, and we need this functionality.

I have implemented the trait approach from above, and it works; however, I am getting a single failing test in 'ChangeDependencyTest`

This test is failing. I have debugged a bit, and it is because the nameToConfiguration map in the Gradle project is empty. This test differs from the others as the dependencies are within the build script. Where should I look to try to solve this?

[shanman190]

@sihutch, alright, the issue with that test is that we are grabbing the configurations from the project, but the classpath configuration is associated to the buildscript itself. Like in other places, we should probably just use one off handling for this configuration in particular (old style MethodMatcher on DependencyHandlerSpec). In the future, we may look into how to get this configuration into the GradleProject marker as well and can remove the special handling in the future.

[sihutch]

Closing as fixed by #4376