rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

ChangeDependencyGroupIdAndArtifactId can duplicate new dependency

Open ashakirin opened this issue 1 year ago • 2 comments

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.

ashakirin avatar Sep 23 '24 08:09 ashakirin

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?

timtebeek avatar Sep 23 '24 10:09 timtebeek

Yes, I provided PR:

  • https://github.com/openrewrite/rewrite/pull/4515

ashakirin avatar Sep 23 '24 11:09 ashakirin

Fixed in #4515 and the follow up commits; thanks again!

timtebeek avatar Oct 27 '24 22:10 timtebeek