Improve UpgradeDependencyVersion to update dependency version whose value is defined via a property from the parent POM. #4214
Conversation
…alue is defined via a property from the parent POM.
There was a problem hiding this comment.
Thanks a lot for the quick work here @nguyenhoan ! It might take me some time to get to a review due to travel over the next two weeks, but at first glance it looks good.
I want to look in more detail though, as this is one of our most heavily used recipes, so I want to be sure we cover all cases correctly.
rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java
Outdated
Show resolved
Hide resolved
|
Sure. Thanks! |
|
@timtebeek just wanted to check if you have an ETA for a full review of this PR? Thanks! |
|
Apologies @nguyenhoan ! Travel & catching up to what I've missed is taking a little longer than I had hoped; I plan to revisit this Monday. At first glance it's quality work; I just want to make sure there's no regressions given how often this recipe is used, and I'll have to update downstream use in tandem. Appreciate your patience & persistence in seeing this through! |
|
Thanks @timtebeek! Please let me know if you need anything from me. Look forward to the release of this enhancement. |
|
@timtebeek just a gentle ping on when this can be merged. Thanks! |
rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java
Outdated
Show resolved
Hide resolved
rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java
Outdated
Show resolved
Hide resolved
rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Finally found a moment to go over your PR in more detail. Two questions arose as I went through the code. Would you be able to look into those?
And thanks for your patience in all of the above; Really do appreciate the work; just a bit harder to wrap my head around a review with the changes that have gone in.
rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java
Outdated
Show resolved
Hide resolved
rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java
Outdated
Show resolved
Hide resolved
… newer version and use it to update the property instead of using the version in the argument which could contain a dependency version selector pattern.
| ResolvedDependency d = findDependency(tag); | ||
| if (d != null && d.getRepository() != null) { | ||
| // if the resolved dependency exists AND it does not represent an artifact that was parsed | ||
| // as a source file, attempt to find a new version. | ||
| try { | ||
| String newerVersion = findNewerVersion(d.getVersion(), | ||
| () -> downloadMetadata(d.getGroupId(), d.getArtifactId(), ctx), versionComparator, ctx); |
There was a problem hiding this comment.
Something I don't quite understand from looking over the code is that we appear to be looking at any dependency, not just dependencies whose groupId & artifactId match the one we're looking to upgrade. Am I reading that correctly?
rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks so much for getting this started @nguyenhoan , and your patience as I had to wrap my head around the changes. I've added a few smaller changes, mostly to familiarize myself with the code, and to better accommodate changes I expect to work in next. Great addition, and neat to better support such minimal property bumps from now on.
|
Thank you for helping with this important enhancement and bringing it to the destination! Could you please let me know when the relevant changes to other modules such as |
|
I've just opened a PR for rewrite-java-dependencies We release snapshot versions every day, and new releases of all openrewrite recipe modules typically every two weeks. We previous release was last week; so then likely by next week there should be a release containing this improvement. |
|
Awesome! Thanks! |
|
Your change has just been released; thanks again! |
|
Yay! Thank you! |
…alue is defined via a property from the parent POM. (openrewrite#4214) * Improve UpgradeDependencyVersion to update dependency version whose value is defined via a property from the parent POM. * Make `VersionComparator versionComparator` field to reduce the number of instantiations. * Remove debugging code. * Avoid using Java stream. * Add tests to verify behavior with remote parent property * Rename variable as suggested * Inline single use of private method delegate * Filter out `null` value instead of throwing exception. Store resolved newer version and use it to update the property instead of using the version in the argument which could contain a dependency version selector pattern. * Update test to show we support version patterns too * Limit property accumulation to matching dependencies * Restore check that ResolvedDependency.getRepository() != null * Do not look at managed or plugin dependency version properties yet * Compare actual new version, not version pattern * Break after finding the matching property * Pull up `getPomDeclaringProperty` closer to usage * Inline & document parent pom handling into `storeParentPomProperty` --------- Co-authored-by: Hoan Nguyen <hoanamzn@amazon.com> Co-authored-by: Tim te Beek <tim@moderne.io>
What's changed?
Improve UpgradeDependencyVersion to update dependency version whose value is defined via a property from the parent POM.
What's your motivation?
Anything in particular you'd like reviewers to focus on?
Anyone you would like to review specifically?
@timtebeek
Have you considered any alternatives or workarounds?
Any additional context
Checklist