rewrite-spring icon indicating copy to clipboard operation
rewrite-spring copied to clipboard
verified publisher icon openrewrite

`spring-cloud-dependencies` upgrade drops `commons-collections4` version

Open timtebeek opened this issue 1 year ago • 9 comments

As reported on Slack by @wabrit

I have a composite recipe which makes some changes to a POM; the source POM declares a dependency on org.apache.commons:commons-collections4 which is expressed via a property and dependency i.e.

<project>
  ...
  <properties>
    <commons-collections4.version>4.4</commons-collections4.version>
  </properties>
  <dependencies>
    <dependency>
      <dependency>
	<groupId>org.apache.commons</groupId>
	<artifactId>commons-collections4</artifactId>
        <version>${commons-collections4.version}</version>
      </dependency>

My recipe doesn't attempt to modify this dependency, and running under OR 2.6 indeed this dependency is not modified. But under OR 2.15 after I run my recipe the dependency has had its <version> element removed, which causes a maven error Could not find artifact org.apache.commons:commons-collections4:pom:unknown I'm not sure what's caused the pom to be changed in this way, nor how to fix it.

timtebeek avatar Jul 25 '24 16:07 timtebeek

I've tried replicating this issue with a unit test, but so far no luck. Any indications of what's needed to reproduce the issue?

import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.config.Environment;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;

import static org.assertj.core.api.Assertions.assertThat;
import static org.openrewrite.java.Assertions.mavenProject;
import static org.openrewrite.maven.Assertions.pomXml;

class CommonsCollections4VersionTest implements RewriteTest {

    @Override
    public void defaults(RecipeSpec spec) {
        spec
          .recipe(Environment.builder()
            .scanRuntimeClasspath("org.openrewrite.java.spring")
            .build()
            .activateRecipes("org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_3")
          );
    }

    @Test
    @DocumentExample
    void retainCommonsCollections4Version() {
        rewriteRun(
          mavenProject("project",
            //language=xml
            pomXml(
              """
                <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
                    <modelVersion>4.0.0</modelVersion>
                    <groupId>com.example</groupId>
                    <artifactId>fooservice</artifactId>
                    <version>1.0-SNAPSHOT</version>
                    <parent>
                        <groupId>org.springframework.boot</groupId>
                        <artifactId>spring-boot-starter-parent</artifactId>
                        <version>2.0.0.RELEASE</version>
                        <relativePath/>
                    </parent>
                    <properties>
                        <commons-collections4.version>4.4</commons-collections4.version>
                    </properties>
                    <dependencies>
                        <dependency>
                            <groupId>org.apache.commons</groupId>
                            <artifactId>commons-collections4</artifactId>
                            <version>${commons-collections4.version}</version>
                        </dependency>
                    </dependencies>
                </project>
                """,
              spec -> spec.after(actual -> {
                  assertThat(actual)
                    .containsPattern("<version>3.3.\\d+</version>")
                    .contains("<version>${commons-collections4.version}</version>");
                  return actual;
              })
            )
          )
        );
    }
}

timtebeek avatar Jul 29 '24 15:07 timtebeek

Hi Tim - my composite recipe is based on org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_2 with some uses of the following added:

  • AddDependency
  • AddProperty
  • ChangeDependencyGroupIdAndArtifactId
  • ChangeDependencyScope
  • ChangePropertyValue
  • RemoveDependency
  • RemovePlugin
  • RemoveProperty
  • RemoveRedundantDependencyVersions
  • UpgradeDependencyVersion

along with custom recipes of my own divising.

The initial state of the pom reference a parent spring boot version of 2.7.0.

I'll start by removing the custom ones to see if it makes any difference to the outcome.

Also - if it helps adding the two seemingly unnecessary recipe calls prevents the problem occuring:

        new RemoveDependency("org.apache.commons", "commons-collections4", "compile"),
        new AddDependency("org.apache.commons", "commons-collections4", "${commons-collections4.version}",
            null, "compile", null, null, null,
            null, null, null, null),

wabrit avatar Jul 29 '24 16:07 wabrit

In this issue there are two things going on:

  1. The explicit <version> element for the commons-collections4 dependency is removed by OpenRewrite
  2. maven then complains that it cannot resolve the dependency version (from the parent pom)

I restricted my recipe run to just using the OpenRewrite standard Spring Boot upgrade recipes and got the following results:

  • org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_3 SUCCESS i.e. (1) holds but (2) does not.
  • org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_2 FAIL i.e. both (1) and (2) above hold.
  • org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_1 SUCCESS i.e. (1) holds but (2) does not.
  • org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_0 SUCCESS i.e. (1) holds but (2) does not.

Which suggests something different in the 3_2 upgrade recipe and/or organisation of managed dependencies in the ancestor poms.

wabrit avatar Jul 30 '24 09:07 wabrit

Thanks for the detailed look! My first hunch was to look at differences in the Spring Boot managed dependencies, but I don't see commons-collections4 there at all 😕

  1. https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/3.0.13/spring-boot-dependencies-3.0.13.pom
  2. https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/3.1.12/spring-boot-dependencies-3.1.12.pom
  3. https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/3.2.8/spring-boot-dependencies-3.2.8.pom
  4. https://repo1.maven.org/maven2/org/springframework/boot/spring-boot-dependencies/3.3.2/spring-boot-dependencies-3.3.2.pom

How are you testing there? Because even when I change the unit test to work from 2.7.0 and upgrade to 3.2.x I do not see the test fail.

timtebeek avatar Jul 30 '24 12:07 timtebeek

Yes I don't think I've bottomed out exactly what is happening.Maybe the Spring Boot version is a red herring and commons-collections4 is being pulled in by another dependency (possibly spring cloud).

wabrit avatar Jul 30 '24 12:07 wabrit

OK - I think I can see what's going on.

Prior to running the recope, my pom also declared a dependencyManagement section for spring-cloud:

         <properties>
                ...
		<spring-cloud.version>2021.0.3</spring-cloud.version>
         <properties>

          <dependencyManagement>
		<dependencies>
			<dependency>
				<groupId>org.springframework.cloud</groupId>
				<artifactId>spring-cloud-dependencies</artifactId>
				<version>${spring-cloud.version}</version>
				<type>pom</type>
				<scope>import</scope>
			</dependency>
		</dependencies>
	</dependencyManagement>

After running the org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_3 recipe, the value of spring.cloud.version was updated to 2022.0.5.

I think at this point the standard O/R recipe determined that the commons-collections4 was dependency-managed somewhere in spring cloud and omitted the version from the dependency. All is fine at this point.

When I added back in my custom pom recipe, that included this:

new ChangePropertyValue("spring-cloud.version", "2023.0.3", true, null)

and it looks like this newer version of spring cloud no longer has a managed dependency for commons-collections4, which leaves the resultant pom in error.

Does that sound like a reasonable explanation, and if so is there anything I can do in my recipe short of a manual RemoveDependency/AddDependency step to restore the fully-versioned commons-collections4?

I wondered if I should be using ChangeManagedDependencyGroupIdAndArtifactId instead of just changing the version of spring cloud, but that recipe sounds like its expecting a change to either group or artifact.

wabrit avatar Aug 02 '24 14:08 wabrit

Thanks fro narrowing that down! With that suggestion I can now finally replicate that dropped version this with a unit test based on https://github.com/openrewrite/rewrite-spring/blob/e5589ab2531c1aa4b66e05ae7818e74816684f46/src/testWithSpringBoot_3_3/java/org/openrewrite/java/spring/boot3/SpringCloudVersionUpgradeTest.java#L43

@Test
@Issue("https://github.com/openrewrite/rewrite-spring/issues/556")
void upgradeSpringCloudVersion() {
    rewriteRun(
      mavenProject("project",
        //language=xml
        pomXml(
          """
            <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                     xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
                <modelVersion>4.0.0</modelVersion>
                <groupId>com.example</groupId>
                <artifactId>fooservice</artifactId>
                <version>1.0-SNAPSHOT</version>
                <parent>
                    <groupId>org.springframework.boot</groupId>
                    <artifactId>spring-boot-starter-parent</artifactId>
                    <version>2.2.2.RELEASE</version>
                    <relativePath/>
                </parent>
                <properties>
                    <java.version>11</java.version>
                    <spring-cloud.version>Hoxton.SR9</spring-cloud.version>
                    <mockito.version>2.18.3</mockito.version>
                    <commons-collections4.version>4.4</commons-collections4.version>
                </properties>
                <dependencyManagement>
                    <dependencies>
                        <dependency>
                            <groupId>org.springframework.cloud</groupId>
                            <artifactId>spring-cloud-dependencies</artifactId>
                            <version>${spring-cloud.version}</version>
                            <type>pom</type>
                            <scope>import</scope>
                        </dependency>
                    </dependencies>
                </dependencyManagement>
                <dependencies>
                    <dependency>
                        <groupId>org.apache.commons</groupId>
                        <artifactId>commons-collections4</artifactId>
                        <version>${commons-collections4.version}</version>
                    </dependency>
                </dependencies>
            </project>
            """,
          spec -> spec.after(actual -> {
              assertThat(actual)
                .containsPattern("<version>3.3.\\d+</version>")
                .containsPattern("<spring-cloud.version>2023.0.\\d+</spring-cloud.version>")
                .contains("<version>${commons-collections4.version}</version>");
              return actual;
          })
        )
      )
    );
}

This now fails with

Expecting actual:
  "<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <groupId>com.example</groupId>
    <artifactId>fooservice</artifactId>
    <version>1.0-SNAPSHOT</version>
    <parent>
        <groupId>org.springframework.boot</groupId>
        <artifactId>spring-boot-starter-parent</artifactId>
        <version>3.3.2</version>
        <relativePath/>
    </parent>
    <properties>
        <java.version>17</java.version>
        <spring-cloud.version>2023.0.3</spring-cloud.version>
        <mockito.version>2.18.3</mockito.version>
        <commons-collections4.version>4.4</commons-collections4.version>
    </properties>
    <dependencyManagement>
        <dependencies>
            <dependency>
                <groupId>org.springframework.cloud</groupId>
                <artifactId>spring-cloud-dependencies</artifactId>
                <version>${spring-cloud.version}</version>
                <type>pom</type>
                <scope>import</scope>
            </dependency>
        </dependencies>
    </dependencyManagement>
    <dependencies>
        <dependency>
            <groupId>org.apache.commons</groupId>
            <artifactId>commons-collections4</artifactId>
        </dependency>
    </dependencies>
</project>"
to contain:
  "<version>${commons-collections4.version}</version>"

timtebeek avatar Aug 02 '24 23:08 timtebeek

It's getting a bit late to think through what we should be doing here, but welcome to help explore the proper fix here.

timtebeek avatar Aug 02 '24 23:08 timtebeek

I think I finally narrowed it down to a change between these two versions:

  • https://github.com/spring-cloud/spring-cloud-kubernetes/blob/v3.1.1/spring-cloud-kubernetes-dependencies/pom.xml#L38
  • https://github.com/spring-cloud/spring-cloud-kubernetes/blob/v3.1.2/spring-cloud-kubernetes-dependencies/pom.xml

That dependency managed version was introduced a couple years before

  • https://github.com/spring-cloud/spring-cloud-kubernetes/issues/943
  • https://github.com/spring-cloud/spring-cloud-kubernetes/pull/947
  • https://github.com/spring-cloud/spring-cloud-kubernetes/commit/55c4f4ed10e5afe17726848f5f1ac87d66d20da1

We've then got multiple levels of <scope>import</scope>; and we cycle through those upgrades coming from older Spring Cloud versions; we're correct in detecting that the dependency became managed, and drop that version.

What we fail to do is reintroduce the dependency version when the dependendy becomes unmanaged again. We do that when changing the parent; I'm thinking we lack that when changing (nested?) <scope>import boms.

The above means we can likely reproduce this on a lot smaller scale in openrewrite/rewrite using just the recipes that made changes here: UpgradeDependencyVersion for Maven.

timtebeek avatar Aug 17 '24 14:08 timtebeek