Skip to content

Commit

Permalink
RemoveRedundantDependencyVersions handles pluginManagement incorrectly (
Browse files Browse the repository at this point in the history 
#4493)

* Incorrect pluginManagement handling from #3932

- version shouldn't be removed from plugin in pluginManagement when no parent
- version should be removed from pluginManagement when matches parent
- plugin should be removed from pluginManagement when nothing else is left

* Restore methods to evaluate managed plugins separately

* Finish Tim’s implementation

* Apply formatter

* Reduce duplication

* Some tests with inherited plugins

* Rename variable as suggested

* Fix final unit test

---------

Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@DidierLoiseau @timtebeek
DidierLoiseau and timtebeek authored Sep 18, 2024
1 parent 117771d commit 4e70fad
Show file tree
Hide file tree
Showing 3 changed files with 404 additions and 33 deletions.
31 changes: 20 additions & 11 deletions rewrite-maven/src/main/java/org/openrewrite/maven/MavenVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ public class MavenVisitor<P> extends XmlVisitor<P> {
static final XPathMatcher PROFILE_MANAGED_DEPENDENCY_MATCHER = new XPathMatcher("/project/profiles/profile/dependencyManagement/dependencies/dependency");
static final XPathMatcher PROPERTY_MATCHER = new XPathMatcher("/project/properties/*");
static final XPathMatcher PLUGIN_MATCHER = new XPathMatcher("//plugins/plugin");
static final XPathMatcher MANAGED_PLUGIN_MATCHER = new XPathMatcher("//pluginManagement/plugins/plugin");
static final XPathMatcher PARENT_MATCHER = new XPathMatcher("/project/parent");


private transient Xml.@Nullable Document document;

@Nullable
Expand Down Expand Up @@ -221,6 +221,10 @@ public boolean isPluginTag(String groupId, @Nullable String artifactId) {
return isPluginTag() && hasPluginGroupId(groupId) && hasPluginArtifactId(artifactId);
}

public boolean isManagedPluginTag() {
return isTag("plugin") && MANAGED_PLUGIN_MATCHER.matches(getCursor());
}

private boolean hasPluginGroupId(String groupId) {
Xml.Tag tag = getCursor().getValue();
boolean isGroupIdFound = matchesGlob(tag.getChildValue("groupId").orElse("org.apache.maven.plugins"), groupId);
Expand Down Expand Up @@ -388,16 +392,21 @@ public boolean isDependencyLikeTag() {
}

public @Nullable Plugin findPlugin(Xml.Tag tag) {
List<Plugin> plugins = getResolutionResult().getPom().getPlugins();
if (plugins != null) {
for (Plugin resolvedPlugin : plugins) {
String reqGroup = resolvedPlugin.getGroupId();
String reqVersion = resolvedPlugin.getVersion();
if ((reqGroup == null || reqGroup.equals(tag.getChildValue("groupId").orElse(null))) &&
resolvedPlugin.getArtifactId().equals(tag.getChildValue("artifactId").orElse(null)) &&
(reqVersion == null || reqVersion.equals(tag.getChildValue("version").orElse(null)))) {
return resolvedPlugin;
}
return findPlugin(tag, getResolutionResult().getPom().getPlugins());
}

public @Nullable Plugin findManagedPlugin(Xml.Tag tag) {
return findPlugin(tag, getResolutionResult().getPom().getPluginManagement());
}

private static @Nullable Plugin findPlugin(Xml.Tag tag, List<Plugin> plugins) {
for (Plugin resolvedPlugin : plugins) {
String reqGroup = resolvedPlugin.getGroupId();
String reqVersion = resolvedPlugin.getVersion();
if ((reqGroup == null || reqGroup.equals(tag.getChildValue("groupId").orElse(null))) &&
resolvedPlugin.getArtifactId().equals(tag.getChildValue("artifactId").orElse(null)) &&
(reqVersion == null || reqVersion.equals(tag.getChildValue("version").orElse(null)))) {
return resolvedPlugin;
}
}
return null;
Expand Down
97 changes: 75 additions & 22 deletions rewrite-maven/src/main/java/org/openrewrite/maven/RemoveRedundantDependencyVersions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,7 @@
import org.openrewrite.semver.VersionComparator;
import org.openrewrite.xml.tree.Xml;

import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.*;

import static org.openrewrite.internal.StringUtils.matchesGlob;

Expand Down Expand Up @@ -168,7 +165,8 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
Xml.Document d = super.visitDocument(document, ctx);
if (d != document) {
d = (Xml.Document) new RemoveEmptyDependencyTags().visitNonNull(d, ctx);
d = (Xml.Document) new RemoveEmptyDependenciesTags().visitNonNull(d, ctx);
d = (Xml.Document) new RemoveEmptyPluginsTags().visitNonNull(d, ctx);
if (!comparator.equals(Comparator.EQ)) {
maybeUpdateModel();
}
Expand All @@ -178,19 +176,11 @@ public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {

@Override
public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (isDependencyTag() || isPluginTag()) {
if (isPluginTag()) {
Plugin p = findPlugin(tag);
if (p != null && matchesGroup(p) && matchesArtifact(p) && matchesVersion(p)) {
Xml.Tag version = tag.getChild("version").orElse(null);
return tag.withContent(ListUtils.map(tag.getContent(), c -> c == version ? null : c));
}
} else {
ResolvedDependency d = findDependency(tag);
if (d != null && matchesGroup(d) && matchesArtifact(d) && matchesVersion(d) && isNotExcepted(d)) {
Xml.Tag version = tag.getChild("version").orElse(null);
return tag.withContent(ListUtils.map(tag.getContent(), c -> c == version ? null : c));
}
if (isDependencyTag()) {
ResolvedDependency d = findDependency(tag);
if (d != null && matchesGroup(d) && matchesArtifact(d) && matchesVersion(d) && isNotExcepted(d)) {
Xml.Tag version = tag.getChild("version").orElse(null);
return tag.withContent(ListUtils.map(tag.getContent(), c -> c == version ? null : c));
}
} else if (isManagedDependencyTag()) {
ResolvedManagedDependency managed = findManagedDependency(tag);
Expand All @@ -201,6 +191,30 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
//noinspection DataFlowIssue
return null;
}
} else if (isPluginTag()) {
if (isManagedPluginTag()) {
Xml.Tag version = tag.getChild("version").orElse(null);
if (version == null) {
// version is not managed here
return tag;
}
Plugin p = findManagedPlugin(tag);
if (p != null && matchesGroup(p) && matchesArtifact(p) && matchesManagedVersion(p, ctx)) {
Set<String> gavTags = new HashSet<>(Arrays.asList("groupId", "artifactId", "version"));
if (tag.getChildren().stream().allMatch(t -> gavTags.contains(t.getName()))) {
// only the version was specified for this managed plugin, so no need to keep the declaration
return null;
}
// some other element is also declared (executions, configuration, dependencies…), so just remove the version
return tag.withContent(ListUtils.map(tag.getContent(), c -> c == version ? null : c));
}
} else {
Plugin p = findPlugin(tag);
if (p != null && matchesGroup(p) && matchesArtifact(p) && matchesVersion(p)) {
Xml.Tag version = tag.getChild("version").orElse(null);
return tag.withContent(ListUtils.map(tag.getContent(), c -> c == version ? null : c));
}
}
}
return super.visitTag(tag, ctx);
}
Expand Down Expand Up @@ -278,6 +292,33 @@ private boolean matchesVersion(Plugin p) {
return matchesComparator(managedVersion, p.getVersion());
}


/**
* This compares a managed plugin version to the version which would be used if only the parent's
* plugin management were in effect. This enables detection of managed plugin versions which
* could be left to the parent.
*/
private boolean matchesManagedVersion(Plugin p, ExecutionContext ctx) {
MavenResolutionResult mrr = getResolutionResult();
if (p.getVersion() == null || mrr.getPom().getRequested().getParent() == null) {
return false;
}
try {
GroupArtifactVersion parentGav = mrr.getPom().getRequested().getParent().getGav();
MavenPomDownloader mpd = new MavenPomDownloader(mrr.getProjectPoms(), ctx, mrr.getMavenSettings(), mrr.getActiveProfiles());
ResolvedPom parentPom = mpd.download(parentGav, null, mrr.getPom(), mrr.getPom().getRepositories())
.resolve(Collections.emptyList(), mpd, ctx);
return parentPom.getPluginManagement().stream()
.filter(plugin -> plugin.getGroupId().equals(p.getGroupId()) && plugin.getArtifactId().equals(p.getArtifactId()))
.findFirst()
.map(Plugin::getVersion)
.map(versionAccordingToParent -> matchesComparator(parentPom.getValue(versionAccordingToParent), p.getVersion()))
.orElse(false);
} catch (Exception e) {
return false;
}
}

private boolean matchesComparator(@Nullable String managedVersion, String requestedVersion) {
if (managedVersion == null) {
return false;
Expand Down Expand Up @@ -313,8 +354,8 @@ private boolean isNotExcepted(ResolvedDependency d) {
final String[] split = gav.split(":");
final String exceptedGroupId = split[0];
final String exceptedArtifactId = split[1];
if (matchesGlob(d.getGroupId(), exceptedGroupId)
&& matchesGlob(d.getArtifactId(), exceptedArtifactId)) {
if (matchesGlob(d.getGroupId(), exceptedGroupId) &&
matchesGlob(d.getArtifactId(), exceptedArtifactId)) {
return false;
}
}
Expand All @@ -324,7 +365,7 @@ && matchesGlob(d.getArtifactId(), exceptedArtifactId)) {
}

private static @Nullable String getManagedPluginVersion(ResolvedPom resolvedPom, @Nullable String groupId, String artifactId) {
for (Plugin p : resolvedPom.getPluginManagement()) {
for (Plugin p : ListUtils.concatAll(resolvedPom.getPluginManagement(), resolvedPom.getRequested().getPluginManagement())) {
if (Objects.equals(
Optional.ofNullable(p.getGroupId()).orElse("org.apache.maven.plugins"),
Optional.ofNullable(groupId).orElse("org.apache.maven.plugins")) &&
Expand All @@ -335,7 +376,7 @@ && matchesGlob(d.getArtifactId(), exceptedArtifactId)) {
return null;
}

private static class RemoveEmptyDependencyTags extends MavenIsoVisitor<ExecutionContext> {
private static class RemoveEmptyDependenciesTags extends MavenIsoVisitor<ExecutionContext> {
@Override
public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
Xml.Tag t = super.visitTag(tag, ctx);
Expand All @@ -346,4 +387,16 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
return t;
}
}

private static class RemoveEmptyPluginsTags extends MavenIsoVisitor<ExecutionContext> {
@Override
public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
Xml.Tag t = super.visitTag(tag, ctx);
if (("pluginManagement".equals(t.getName()) || "plugins".equals(t.getName())) && (t.getContent() == null || t.getContent().isEmpty())) {
//noinspection DataFlowIssue
return null;
}
return t;
}
}
}
Loading

0 comments on commit 4e70fad

Please sign in to comment.