Skip to content

Adapted ChangeDependency to keep existing version variables when null version as input #5863

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Adapted ChangeDependency to keep existing version variables when null version as input #5863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ae70d0
Adapted ChangeDependency to keep existing version variables when null…
Jenson3210 Aug 6, 2025
7de679d
Merge branch 'main' into change-dependency-nullversion
timtebeek Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions rewrite-gradle/src/main/java/org/openrewrite/gradle/ChangeDependency.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,15 @@ private J.MethodInvocation updateDependency(J.MethodInvocation m, ExecutionConte
}
if (original != updated) {
String replacement = updated.toStringNotation();
Copy link
Member

@jkschneider jkschneider Aug 7, 2025

Choose a reason for hiding this comment

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

Is this solution overfit to the problem? What if the artifactId is a variable? Are we having to introduce the below logic because toStringNotation strips the variable?

Copy link
Contributor Author

@Jenson3210 Jenson3210 Aug 7, 2025

Choose a reason for hiding this comment

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

Are we having to introduce the below logic because toStringNotation strips the variable?

We have to introduce something like this because DependencyStringNotationConverter.parse((String) requireNonNull(literal.getValue())) only receives the literal part from the G.String.
In theory, this could be group only if artifact is a variable also, in which the recipe would even give a NPE earlier in the code (as artifactId is not nullable for dependency/Gav). But as far as I remember, we mostly do not have support for setups like that as also in other recipes the assumption is always made: if it is a GString, the variable part is the version

This is also the case in our GradleDependency Trait, which uses the same DependencyStringConverter

Is this solution overfit to the problem?

Not sure what you mean with overfit, but I made it like this so that our G.String remains a G.String if the version is a variable so that later recipes that touch dependencies/versions receive the correct lst element instead of a literal that contains a variable reference. Other recipes that would do an update/replace by variable lookup would expect a G.String and would think a J.Literal contains the exact version.

Copy link
Contributor

@shanman190 shanman190 Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Just to chime in on the "overfit" part is that there is existing precedent in many of the other Gradle recipes to this respect. I agree that only expecting for only the version to be a template variable is a little assumptive, but it is the most prevalent form and very rarely do I see either the group or artifact being parameterized in real life builds.

To take this a step further, even using parameters in the way described no longer aligns with Gradle best practices and the advice there is to utilize a version catalog instead.

Copy link
Contributor Author

@Jenson3210 Jenson3210 Aug 18, 2025

Choose a reason for hiding this comment

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

@jkschneider I noticed this one was moved to need additional context.
What additional context would be needed here? Or was @shanman190 and my input sufficient to take this further?

J.Literal newLiteral = literal.withValue(replacement)
.withValueSource(gstring.getDelimiter() + replacement + gstring.getDelimiter());
m = m.withArguments(singletonList(newLiteral));
List<Expression> newArguments;
if (newVersion == null) {
newArguments = ListUtils.mapFirst(m.getArguments(), gstr ->
gstring.withStrings(ListUtils.mapFirst(gstring.getStrings(), lit ->
literal.withValue(replacement).withValueSource(replacement))));
} else {
newArguments = singletonList(literal.withValue(replacement).withValueSource(gstring.getDelimiter() + replacement + gstring.getDelimiter()));
}
m = m.withArguments(newArguments);
}
}
}
Expand Down Expand Up @@ -584,9 +590,15 @@ private J.MethodInvocation updateDependency(J.MethodInvocation m, ExecutionConte
}
if (original != updated) {
String replacement = updated.toStringNotation();
J.Literal newLiteral = literal.withValue(replacement)
.withValueSource(template.getDelimiter() + replacement + template.getDelimiter());
m = m.withArguments(singletonList(newLiteral));
List<Expression> newArguments;
if (newVersion == null) {
newArguments = ListUtils.mapFirst(m.getArguments(), tmplt ->
template.withStrings(ListUtils.mapFirst(template.getStrings(), lit ->
literal.withValue(replacement).withValueSource(replacement))));
} else {
newArguments = singletonList(literal.withValue(replacement).withValueSource(template.getDelimiter() + replacement + template.getDelimiter()));
}
m = m.withArguments(newArguments);
}
}
}
Expand Down
74 changes: 74 additions & 0 deletions rewrite-gradle/src/test/java/org/openrewrite/gradle/ChangeDependencyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,43 @@ implementation platform("org.apache.commons:commons-lang3:3.11")
);
}

@Test
void GStringNullVersionPreservesVariables() {
rewriteRun(
spec -> spec.recipe(new ChangeDependency("commons-lang", "commons-lang", "org.apache.commons", "commons-lang3", null, null, null)),
buildGradle(
"""
plugins {
id "java-library"
}

repositories {
mavenCentral()
}

def version = '2.6'
dependencies {
implementation platform("commons-lang:commons-lang:${version}")
}
""",
"""
plugins {
id "java-library"
}

repositories {
mavenCentral()
}

def version = '2.6'
dependencies {
implementation platform("org.apache.commons:commons-lang3:${version}")
}
"""
)
);
}

@Test
void changeDependencyWithLowerVersionAfter() {
rewriteRun(
Expand Down Expand Up @@ -524,6 +561,43 @@ void kotlinDslStringInterpolation() {
);
}

@Test
void KStringTemplateNullVersionPreservesVariables() {
rewriteRun(
spec -> spec.recipe(new ChangeDependency("commons-lang", "commons-lang", "org.apache.commons", "commons-lang3", null, null, null)),
buildGradleKts(
"""
plugins {
`java-library`
}

repositories {
mavenCentral()
}

dependencies {
val commonsLangVersion = "2.6"
implementation("commons-lang:commons-lang:${commonsLangVersion}")
}
""",
"""
plugins {
`java-library`
}

repositories {
mavenCentral()
}

dependencies {
val commonsLangVersion = "2.6"
implementation("org.apache.commons:commons-lang3:${commonsLangVersion}")
}
"""
)
);
}

@Test
void dependencyPluginManagedDependencies() {
rewriteRun(
Expand Down