Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added property version migration #4330

Conversation

marcel-gepardec
Copy link
Contributor

Property version migration

What's changed?

Depending on the content inside the version tag the migration decides if the version is changed inside the dependency tag or inside the properties. I also added a new option to the ChangeDependencyGroupIdAndArtifactId recipe implementation.
The new option is changePropertyVersionNames. It is set per default on false. If it is set on true than it changes the property variable name from whatsoever to the new [artifactId].version

Example

Recipe with changePropertyVersionNames on true:

---
type: specs.openrewrite.org/v1beta/recipe
name: com.example.openrewrite.migrate.JavaxPersistenceToJakartaPersistence
displayName: Change Maven managed dependency groupId, artifactId and optionally the version example
recipeList:
  - org.openrewrite.maven.ChangeDependencyGroupIdAndArtifactId:
      oldGroupId: javax.persistence
      oldArtifactId: javax.persistence-api
      newGroupId: jakarta.persistence
      newArtifactId: jakarta.persistence-api
      newVersion: latest.release
      changePropertyVersionNames: true

Before:

                  ...
                  <properties>
                          <javax.persistence.version>2.2</javax.persistence.version>
                  </properties>
                  <dependencyManagement>
                      <dependencies>
                          <dependency>
                              <groupId>javax.persistence</groupId>
                              <artifactId>javax.persistence-api</artifactId>
                              <version>${javax.persistence.version}</version>
                          </dependency>
                      </dependencies>
                  </dependencyManagement>
                  <dependencies>
                      <dependency>
                           <groupId>javax.persistence</groupId>
                           <artifactId>javax.persistence-api</artifactId>
                      </dependency>
                  </dependencies>
                  ...

After:

                  ...
                  <properties>
                          <jakarta.persistence-api.version>3.2.0</jakarta.persistence-api.version>
                  </properties>
                  <dependencyManagement>
                      <dependencies>
                          <dependency>
                              <groupId>jakarta.persistence</groupId>
                              <artifactId>jakarta.persistence-api</artifactId>
                              <version>${jakarta.persistence-api.version}</version>
                          </dependency>
                      </dependencies>
                  </dependencyManagement>
                  <dependencies>
                      <dependency>
                           <groupId>jakarta.persistence</groupId>
                           <artifactId>jakarta.persistence-api</artifactId>
                      </dependency>
                  </dependencies>
                  ...

What's your motivation?

While migrating one of my projects I noticed that in my pom every single version inside the dependencies changed from a property variable to a normal version.

@timtebeek
Copy link
Contributor

Hi @marcel-gepardec ; Appreciate the effort you've put in here. I do wonder if this would be better as a separate recipe though, as to not complicate the options and logic further for these already not trivial recipes. There's a lot to factor in, such as parents, BOMs and their order, managed dependencies and properties. To add optional renaming on top of that adds complexity which isn't often used, especially considering that these recipes are a often include in framework migrations where the option is not set. That way folks would still see properties not adhering to this specification.

Comparatively I'd imagine a separate recipe that only renames properties from whatever they are named currently to a prefix/suffix naming scheme might be easier to maintain going forward. And even then we'd need to take into account if a property is actually overriding a property as the often used Spring managed dependency version properties, and handle that appropriately.

What are your thoughts on the above?

@ErhardSiegl
Copy link
Contributor

My thoughts on this: There are two issues here:

  1. If there is a property in a version tag, change the property value with the new version instead of changing the version tag
  2. Change the property name according to some naming scheme

I also think those two should be separated in different recipes.
The second seems to be rather complicated to me, especially if several dependencies use the same property name. Its probably a case by case issue and project specific.

The first seems to me as expected behavior. It may lead to problems if two or more dependencies use the same property, but after migration the version numbers differ for some reason. What should the recipe do? Override the first change, how high is the risk of breaking things? Change the version tag instead, how complicated is this to implement?

@timtebeek
Copy link
Contributor

Thanks for clarifying! Indeed best to separate those two concerns.

I'd have expected 1. to already be handled with the recipe we have, also based on the tests & changes I've seen up to now. If this isn't the case then a separate unit test to replicate the issue is probably best, and we can then work from there.

Let's split off the 2. concern into a separate issue, if we think it's feasible to implement at all in a reliable manner without supervision. Best not to make those changes part of this PR, however well intended.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • rewrite-java-test/src/test/java/org/openrewrite/java/UseStaticImportTest.java
    • lines 336-336

@timtebeek
Copy link
Contributor

Closing as discussed on #4366 (comment)
We'll continue with this PR instead

@timtebeek timtebeek closed this Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants