feat: Added functionality to Gradle ChangeDependency to avoid duplica… #4523
Conversation
…te already existing new dependencies Refs: openrewrite#4514
| private J.MethodInvocation updateDependency(J.MethodInvocation m, ExecutionContext ctx) { | ||
| List<Expression> depArgs = m.getArguments(); | ||
| if (depArgs.get(0) instanceof J.Literal) { | ||
| String gav = (String) ((J.Literal) depArgs.get(0)).getValue(); | ||
| if (gav != null) { | ||
| Dependency original = DependencyStringNotationConverter.parse(gav); | ||
| if (original != null && depMatcher.matches(original.getGroupId(), original.getArtifactId())) { | ||
| if (original != null && oldDepMatcher.matches(original.getGroupId(), original.getArtifactId())) { | ||
| if (isNewDependencyPresent) { |
There was a problem hiding this comment.
This either needs to be evaluated when we have found the dependency OR precomputed by configurations that contain it as a first order dependency.
With the current implementation, if the new dependency was in testImplementation, but the old is in implementation, we would incorrectly remove the necessary dependency.
The converse side of this, where the new dependency has now been added to a configuration upstream of the current one, I think right now we should leave as out of scope and for a future cleanup duplicate dependencies recipe instead.
There was a problem hiding this comment.
@shanman190: I think the current behaviour need to be fixed anyway, because it deliveries very strange result, if new dependency already exists:
rewriteRun(
spec -> spec.recipe(new ChangeDependency("commons-lang", "commons-lang", "org.apache.commons", "commons-lang3", "3.11.x", null, null)),
buildGradle(
"""
plugins {
id "java-library"
}
repositories {
mavenCentral()
}
dependencies {
implementation "commons-lang:commons-lang:2.6"
implementation group: "commons-lang", name: "commons-lang", version: "2.6"
implementation group: "org.apache.commons", name: "commons-lang3", version: "3.11"
}
""",
"""
plugins {
id "java-library"
}
repositories {
mavenCentral()
}
dependencies {
implementation "org.apache.commons:commons-lang3:2.6"
implementation group: "org.apache.commons", name: "commons-lang3", version: "2.6"
implementation "org.apache.commons:commons-lang3:3.11"
}
"""```
Recipe replaced commons-lang:commons-lang with new groupId and artifactId, but kept the version from old dependency.
I will update the fix to take dependnecy type into account
There was a problem hiding this comment.
@shanman190: do you generally interpret this as consistent case to have dependency in testImplementation and the same dependency with older version in implementation?
There was a problem hiding this comment.
Gradle configurations are a foundational component of Gradle. I was merely using implementation and testImplementation as examples. The more specific issue is when the old dependency is in a configuration upstream from the new dependency. (Ex: api -> implementation; implementation -> testImplementation).
The present implementation doesn't account for the Gradle configuration that the existing dependency is in or if the new dependency is strictly upstream from itself and as a result, illustrated by my example, shows how the current implementation would become broken and can lead directly to code that no longer compiles in these situations. A recipe that would result in code that fails to compile would violate OpenRewrite's do no harm principle.
Just to be clear, what you're wanting to achieve is possible, the decisioning just needs to happen where I'm indicating and taking into account the current configuration (gotten by looking up the configuration using MethodInvocation#getSimpleName), then to be able to remove more often (ie. The new dependency is not in the same configuration as the old) looking at the configurations upstream to see if the new dependency is present.
If this still isn't clear enough, I'll add a couple of test cases that I would expect to pass to help illustrate the problem and intended positive and negative cases.
Refs: #4521