openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.15k stars 320 forks source link

ChangeDependencyGroupIdAndArtifactId can duplicate new dependency #4514

Open ashakirin opened 1 week ago

ashakirin commented 1 week ago

In case if new dependency already exists, it will be doubled:

@Test
void shouldNotAddNewIfDependencyAlreadyExistsManaged() {
    rewriteRun(
      spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
        "javax.activation",
        "javax.activation-api",
        "jakarta.activation",
        "jakarta.activation-api",
        null,
        null
      )),
      pomXml(
        """
          <project>
              <modelVersion>4.0.0</modelVersion>
              <groupId>com.mycompany.app</groupId>
              <artifactId>my-app</artifactId>
              <version>1</version>
              <dependencies>
                  <dependency>
                      <groupId>javax.activation</groupId>
                      <artifactId>javax.activation-api</artifactId>
                  </dependency>
                  <dependency>
                      <groupId>jakarta.activation</groupId>
                      <artifactId>jakarta.activation-api</artifactId>
                  </dependency>
              </dependencies>
              <dependencyManagement>
                  <dependencies>
                      <dependency>
                          <groupId>javax.activation</groupId>
                          <artifactId>javax.activation-api</artifactId>
                          <version>1.2.0</version>
                      </dependency>
                      <dependency>
                          <groupId>jakarta.activation</groupId>
                          <artifactId>jakarta.activation-api</artifactId>
                          <version>1.2.1</version>
                      </dependency>
                  </dependencies>
              </dependencyManagement>
          </project>
          """,
        """
          <project>
              <modelVersion>4.0.0</modelVersion>
              <groupId>com.mycompany.app</groupId>
              <artifactId>my-app</artifactId>
              <version>1</version>
              <dependencies>
                  <dependency>
                      <groupId>jakarta.activation</groupId>
                      <artifactId>jakarta.activation-api</artifactId>
                  </dependency>
              </dependencies>
              <dependencyManagement>
                  <dependencies>
                      <dependency>
                          <groupId>javax.activation</groupId>
                          <artifactId>javax.activation-api</artifactId>
                          <version>1.2.0</version>
                      </dependency>
                      <dependency>
                          <groupId>jakarta.activation</groupId>
                          <artifactId>jakarta.activation-api</artifactId>
                          <version>1.2.1</version>
                      </dependency>
                  </dependencies>
              </dependencyManagement>
          </project>
          """
      )
    );
}

Of course, it is possible to add RemoveDuplicateDependencies recipe, but more clean solution is to avoid adding duplicate at all.

timtebeek commented 1 week ago

Thanks for the report here @ashakirin ! Indeed I see this fail with

diff --git a/pom.xml b/pom.xml
index 05c1384..71dba0e 100644
--- a/pom.xml
+++ b/pom.xml
@@ -8,6 +8,10 @@ 
             <groupId>jakarta.activation</groupId>
             <artifactId>jakarta.activation-api</artifactId>
         </dependency>
+        <dependency>
+            <groupId>jakarta.activation</groupId>
+            <artifactId>jakarta.activation-api</artifactId>
+        </dependency>
     </dependencies>
     <dependencyManagement>
         <dependencies>

We should really be making the idiomatic change here of removing the old dependency of the new is already present. Did you already try to adjust the recipe code to make that work?

ashakirin commented 1 week ago

Yes, I provided PR: https://github.com/openrewrite/rewrite/pull/4515