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.

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

feat: Added functionality to Gradle ChangeDependency to avoid duplica… #4523

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: Added functionality to Gradle ChangeDependency to avoid duplica… #4523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 37 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 @@ -18,6 +18,7 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import lombok.EqualsAndHashCode;
import lombok.Value;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.gradle.marker.GradleDependencyConfiguration;
Expand All @@ -36,9 +37,11 @@
import org.openrewrite.maven.MavenDownloadingException;
import org.openrewrite.maven.tree.GroupArtifact;
import org.openrewrite.maven.tree.GroupArtifactVersion;
import org.openrewrite.maven.tree.ResolvedDependency;
import org.openrewrite.maven.tree.ResolvedGroupArtifactVersion;
import org.openrewrite.semver.DependencyMatcher;
import org.openrewrite.semver.Semver;
import org.openrewrite.semver.VersionComparator;

import java.util.*;

Expand Down Expand Up @@ -156,7 +159,10 @@ public Validated<Object> validate() {
@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(new FindGradleProject(FindGradleProject.SearchCriteria.Marker).getVisitor(), new GroovyIsoVisitor<ExecutionContext>() {
final DependencyMatcher depMatcher = requireNonNull(DependencyMatcher.build(oldGroupId + ":" + oldArtifactId).getValue());
final DependencyMatcher oldDepMatcher = requireNonNull(DependencyMatcher.build(oldGroupId + ":" + oldArtifactId).getValue());
final VersionComparator versionComparator = newVersion != null ? Semver.validate(newVersion, versionPattern).getValue() : null;
final DependencyMatcher newDepMatcher = new DependencyMatcher(newGroupId, newArtifactId, versionComparator);
boolean isNewDependencyPresent;

GradleProject gradleProject;

Expand All @@ -168,6 +174,7 @@ public G.CompilationUnit visitCompilationUnit(G.CompilationUnit cu, ExecutionCon
}

gradleProject = maybeGp.get();
isNewDependencyPresent = checkForNewDependencyPresence(gradleProject, newDepMatcher);

G.CompilationUnit g = super.visitCompilationUnit(cu, ctx);
if (g != cu) {
Expand All @@ -176,6 +183,20 @@ public G.CompilationUnit visitCompilationUnit(G.CompilationUnit cu, ExecutionCon
return g;
}

private boolean checkForNewDependencyPresence(GradleProject gp, DependencyMatcher dependencyMatcher) {
boolean isPresent = false;
Map<String, GradleDependencyConfiguration> nameToConfiguration = gp.getNameToConfiguration();
for (GradleDependencyConfiguration gdc : nameToConfiguration.values()) {
List<ResolvedDependency> resolvedDependencies = gdc.getDirectResolved();
isPresent = resolvedDependencies.stream()
.anyMatch(r -> dependencyMatcher.matches(r.getGroupId(), r.getArtifactId(), r.getVersion()));
if (isPresent) {
break;
}
}
return isPresent;
}

@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
J.MethodInvocation m = super.visitMethodInvocation(method, ctx);
Expand All @@ -198,13 +219,17 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu
return m;
}

@NullMarked
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ashakirin ashakirin Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shanman190: do you generally interpret this as consistent case to have dependency in testImplementation and the same dependency with older version in implementation?

Copy link
Contributor

@shanman190 shanman190 Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

return null;
}
Dependency updated = original;
if (!StringUtils.isBlank(newGroupId) && !updated.getGroupId().equals(newGroupId)) {
updated = updated.withGroupId(newGroupId);
Expand Down Expand Up @@ -238,7 +263,10 @@ private J.MethodInvocation updateDependency(J.MethodInvocation m, ExecutionConte

J.Literal literal = (J.Literal) strings.get(0);
Dependency original = DependencyStringNotationConverter.parse((String) requireNonNull(literal.getValue()));
if (original != null && depMatcher.matches(original.getGroupId(), original.getArtifactId())) {
if (original != null && oldDepMatcher.matches(original.getGroupId(), original.getArtifactId())) {
if (isNewDependencyPresent) {
return null;
}
Dependency updated = original;
if (!StringUtils.isBlank(newGroupId) && !updated.getGroupId().equals(newGroupId)) {
updated = updated.withGroupId(newGroupId);
Expand Down Expand Up @@ -307,9 +335,12 @@ private J.MethodInvocation updateDependency(J.MethodInvocation m, ExecutionConte
if (groupId == null || artifactId == null) {
return m;
}
if (!depMatcher.matches(groupId, artifactId)) {
if (!oldDepMatcher.matches(groupId, artifactId)) {
return m;
}
if (isNewDependencyPresent && oldDepMatcher.matches(groupId, artifactId)) {
return null;
}
String updatedGroupId = groupId;
if (!StringUtils.isBlank(newGroupId) && !updatedGroupId.equals(newGroupId)) {
updatedGroupId = newGroupId;
Expand Down Expand Up @@ -364,7 +395,7 @@ private GradleProject updateGradleModel(GradleProject gp) {
for (GradleDependencyConfiguration gdc : nameToConfiguration.values()) {
GradleDependencyConfiguration newGdc = gdc;
newGdc = newGdc.withRequested(ListUtils.map(gdc.getRequested(), requested -> {
if (depMatcher.matches(requested.getGroupId(), requested.getArtifactId())) {
if (oldDepMatcher.matches(requested.getGroupId(), requested.getArtifactId())) {
GroupArtifactVersion gav = requested.getGav();
if (newGroupId != null) {
gav = gav.withGroupId(newGroupId);
Expand All @@ -379,7 +410,7 @@ private GradleProject updateGradleModel(GradleProject gp) {
return requested;
}));
newGdc = newGdc.withDirectResolved(ListUtils.map(gdc.getDirectResolved(), resolved -> {
if (depMatcher.matches(resolved.getGroupId(), resolved.getArtifactId())) {
if (oldDepMatcher.matches(resolved.getGroupId(), resolved.getArtifactId())) {
ResolvedGroupArtifactVersion gav = resolved.getGav();
if (newGroupId != null) {
gav = gav.withGroupId(newGroupId);
Expand Down
37 changes: 37 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 @@ -67,6 +67,43 @@ void relocateDependency() {
);
}

@Test
void removeOldDependenciesIfNewAlreadyPresents() {
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 "org.apache.commons:commons-lang3:3.11"
}
""",
"""
plugins {
id "java-library"
}

repositories {
mavenCentral()
}

dependencies {
implementation "org.apache.commons:commons-lang3:3.11"
}
"""
)
);
}

@Test
void changeGroupIdOnly() {
rewriteRun(
Expand Down