Skip to content

Commit

Permalink
Add Code Actions for Managed Dependency Version override
Browse files Browse the repository at this point in the history
This PR adds Maven project validation for managed dependency version element values override/duplication
Also this adds Code Actions for the removal of the duplicating/overriding dependency version elements and according JUnit test cases
  • Loading branch information
vrubezhny committed Feb 21, 2023
1 parent 3277f4e commit a64d68e
Show file tree
Hide file tree
Showing 10 changed files with 270 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ private DOMConstants() {
public static final String VERSION_ELT = "version";
public static final String PLUGINS_ELT = "plugins";
public static final String PLUGIN_ELT = "plugin";
public static final String PROFILE_ELT = "profile";
public static final String PROFILES_ELT = "profiles";
public static final String PARENT_ELT = "parent";
public static final String SCOPE_ELT = "scope";
public static final String PHASE_ELT = "phase";
Expand All @@ -40,6 +42,7 @@ private DOMConstants() {
public static final String CLASSIFIER_ELT = "classifier";
public static final String EXCLUSION_ELT = "exclusion";
public static final String EXCLUSIONS_ELT = "exclusions";
public static final String ID_ELT = "id";

public static final String TARGET_PATH_ELT = "targetPath";
public static final String DIRECTORY_ELT = "directory";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.eclipse.aether.repository.WorkspaceReader;
import org.eclipse.lemminx.dom.DOMDocument;
import org.eclipse.lemminx.extensions.maven.participants.codeaction.MavenIdPartRemovalCodeAction;
import org.eclipse.lemminx.extensions.maven.participants.codeaction.MavenManagedVersionRemovalCodeAction;
import org.eclipse.lemminx.extensions.maven.participants.codeaction.MavenNoGrammarConstraintsCodeAction;
import org.eclipse.lemminx.extensions.maven.participants.completion.MavenCompletionParticipant;
import org.eclipse.lemminx.extensions.maven.participants.definition.MavenDefinitionParticipant;
Expand Down Expand Up @@ -482,6 +483,7 @@ private void registerCodeActionParticipants(XMLExtensionsRegistry registry) {
}
codeActionParticipants.add(new MavenNoGrammarConstraintsCodeAction());
codeActionParticipants.add(new MavenIdPartRemovalCodeAction());
codeActionParticipants.add(new MavenManagedVersionRemovalCodeAction());

codeActionParticipants.stream().forEach(registry::registerCodeActionParticipant);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@

public enum MavenSyntaxErrorCode implements IXMLErrorCode {
DuplicationOfParentGroupId,
DuplicationOfParentVersion;
DuplicationOfParentVersion,
OverridingOfManagedDependency;

private final String code;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*******************************************************************************
* Copyright (c) 2023 Red Hat Inc. and others.
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
*******************************************************************************/
package org.eclipse.lemminx.extensions.maven.participants.codeaction;

import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

import org.eclipse.lemminx.commons.BadLocationException;
import org.eclipse.lemminx.commons.CodeActionFactory;
import org.eclipse.lemminx.commons.TextDocument;
import org.eclipse.lemminx.dom.DOMDocument;
import org.eclipse.lemminx.dom.DOMNode;
import org.eclipse.lemminx.extensions.maven.MavenSyntaxErrorCode;
import org.eclipse.lemminx.extensions.maven.utils.ParticipantUtils;
import org.eclipse.lemminx.services.extensions.codeaction.ICodeActionParticipant;
import org.eclipse.lemminx.services.extensions.codeaction.ICodeActionRequest;
import org.eclipse.lsp4j.CodeAction;
import org.eclipse.lsp4j.Diagnostic;
import org.eclipse.lsp4j.Range;
import org.eclipse.lsp4j.jsonrpc.CancelChecker;

public class MavenManagedVersionRemovalCodeAction implements ICodeActionParticipant {
private static final Logger LOGGER = Logger.getLogger(MavenIdPartRemovalCodeAction.class.getName());

/**
* string that gets included in pom.xml file comments and makes the marker
* manager to ignore the managed version override marker
*/
String MARKER_IGNORE_MANAGED = "$NO-MVN-MAN-VER$";//$NON-NLS-1$

public MavenManagedVersionRemovalCodeAction() {
}

@Override
public void doCodeAction(ICodeActionRequest request, List<CodeAction> codeActions, CancelChecker cancelChecker) {
Diagnostic diagnostic = request.getDiagnostic();
if (!ParticipantUtils.match(diagnostic, MavenSyntaxErrorCode.OverridingOfManagedDependency.getCode())) {
return;
}

DOMDocument document = request.getDocument();
Range diagnosticRange = diagnostic.getRange();

TextDocument textDocument = document.getTextDocument();
try {
int startOffset = document.offsetAt(diagnosticRange.getStart());
DOMNode version = document.findNodeAt(startOffset);
if (version != null) {
// "Adds comment markup next to the affected element. No longer shows the warning afterwards
Range valueRange = new Range(textDocument.positionAt(version.getEnd()), textDocument.positionAt(version.getEnd()));
CodeAction addIgnoreMarkup = CodeActionFactory.insert("Ignore this warning", valueRange.getEnd(),
"<!--" + MARKER_IGNORE_MANAGED + "-->", document.getTextDocument(), diagnostic);
codeActions.add(addIgnoreMarkup);

// It removes the current definition to rely on value inherited from parent
valueRange = new Range(textDocument.positionAt(version.getStart()), textDocument.positionAt(version.getEnd()));
CodeAction addSchemaAction = CodeActionFactory.remove(
"Remove version declaration",
valueRange, document.getTextDocument(), diagnostic);
codeActions.add(addSchemaAction);
}
} catch (BadLocationException e) {
LOGGER.log(Level.SEVERE, "Unable to remove specified element", e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,27 @@
*******************************************************************************/
package org.eclipse.lemminx.extensions.maven.participants.diagnostics;

import static org.eclipse.lemminx.extensions.maven.DOMConstants.ARTIFACT_ID_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.CLASSIFIER_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.DEPENDENCY_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.DEPENDENCIES_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.GROUP_ID_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.ID_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.PARENT_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.PROFILE_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.PROFILES_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.TYPE_ELT;
import static org.eclipse.lemminx.extensions.maven.DOMConstants.VERSION_ELT;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.logging.Logger;

import org.apache.maven.model.Dependency;
import org.apache.maven.model.DependencyManagement;
import org.apache.maven.project.MavenProject;

import org.eclipse.lemminx.dom.DOMDocument;
Expand Down Expand Up @@ -114,7 +126,7 @@ private Optional<List<Diagnostic>> validateParentMatchingGroupIdVersion(Diagnost
}

/**
* TODO: Validates if a dependency version duplicates or overrides a managed dependency version
* Validates if a dependency version duplicates or overrides a managed dependency version
*
* @param diagnosticRequest
* @return
Expand All @@ -125,7 +137,82 @@ private Optional<List<Diagnostic>> validateManagedDependencies(DiagnosticRequest
return Optional.empty();
}

List<DOMElement> candidates = new ArrayList<>();
Optional<DOMElement> dependencies = DOMUtils.findChildElement(node, DEPENDENCIES_ELT);
dependencies.ifPresent(dependenciesElement -> {
DOMUtils.findChildElements(dependenciesElement, DEPENDENCY_ELT).stream()
.filter(dependency -> DOMUtils.findChildElement(dependency, VERSION_ELT).isPresent())
.forEach(candidates::add);
});

// we should also consider <dependencies> section in the profiles, but profile
// are optional and so is their
// dependencyManagement section.. that makes handling our markers more complex.
// see MavenProject.getInjectedProfileIds() for a list of currently active
// profiles in effective pom
String currentProjectKey = mavenProject.getGroupId() + ":" + mavenProject.getArtifactId() + ":" //$NON-NLS-1$//$NON-NLS-2$
+ mavenProject.getVersion();
List<String> activeprofiles = mavenProject.getInjectedProfileIds().get(currentProjectKey);
// remember what profile we found the dependency in.
Optional<DOMElement> profiles = DOMUtils.findChildElement(node, PROFILES_ELT);
profiles.ifPresent(profilesElement -> {
DOMUtils.findChildElements(profilesElement, PROFILE_ELT).stream()
.forEach(profile -> {
Optional<String> idString = DOMUtils.findChildElementText(profile, ID_ELT);
if (idString.isPresent() && activeprofiles.contains(idString.get())) {
Optional<DOMElement> profileDependencies = DOMUtils.findChildElement(profile, DEPENDENCIES_ELT);
profileDependencies.ifPresent(dependenciesElement -> {
DOMUtils.findChildElements(dependenciesElement, DEPENDENCY_ELT).stream()
.filter(dependency -> DOMUtils.findChildElement(dependency, VERSION_ELT).isPresent())
.forEach(dependency -> {
candidates.add(dependency);
});
});
}
});
});

//collect the managed dep ids
Map<String, Dependency> managed = new HashMap<>();
DependencyManagement dm = mavenProject.getDependencyManagement();
if(dm != null) {
List<Dependency> deps = dm.getDependencies();
deps.stream().filter(dep -> dep.getVersion() != null)
.forEach(dep -> managed.put(dep.getManagementKey(), dep));
}

//now we have all the candidates, match them against the effective managed set
List<Diagnostic> diagnostics = new ArrayList<>();
candidates.stream().forEach(dep -> {
Optional<DOMElement> version = DOMUtils.findChildElement(dep, VERSION_ELT);
version.ifPresent(v -> {
String grpString = DOMUtils.findChildElementText(dep, GROUP_ID_ELT).orElse(null);
String artString = DOMUtils.findChildElementText(dep, ARTIFACT_ID_ELT).orElse(null);
String versionString = DOMUtils.findElementText(v).orElse(null);
if(grpString != null && artString != null && versionString != null) {
String typeString = DOMUtils.findChildElementText(dep, TYPE_ELT).orElse(null);
String classifier = DOMUtils.findChildElementText(dep, CLASSIFIER_ELT).orElse(null);
String id = getDependencyKey(grpString, artString, typeString, classifier);
if(managed.containsKey(id)) {
Dependency managedDep = managed.get(id);
String managedVersion = managedDep == null ? null : managedDep.getVersion();
String msg = versionString.equals(managedVersion)
? "Duplicating managed version %s for %s"
: "Overriding managed version %s for %s";

DOMDocument xmlDocument = diagnosticRequest.getXMLDocument();
diagnostics.add(new Diagnostic(
XMLPositionUtility.createRange(v.getStartTagCloseOffset() + 1,
v.getEndTagOpenOffset(), xmlDocument),
String.format(msg, managedVersion, artString),
DiagnosticSeverity.Warning,
xmlDocument.getDocumentURI(),
MavenSyntaxErrorCode.OverridingOfManagedDependency.getCode()));
}
}
});
});

return Optional.of(diagnostics);
}

Expand All @@ -142,4 +229,13 @@ private Optional<List<Diagnostic>> validateManagedPlugins(DiagnosticRequest diag
List<Diagnostic> diagnostics = new ArrayList<>();
return Optional.of(diagnostics);
}

private static String getDependencyKey(String groupId, String artifactId, String type, String classifier) {
StringBuilder key = new StringBuilder(groupId).append(":").append(artifactId).append(":") //$NON-NLS-1$ //$NON-NLS-2$
.append(type == null ? "jar" : type);//$NON-NLS-1$
if (classifier != null) {
key.append(":").append(classifier);//$NON-NLS-1$
}
return key.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ public static Optional<DOMElement> findChildElement(DOMNode parent, String eleme
.filter(DOMElement.class::isInstance).map(DOMElement.class::cast).findAny();
}

public static List<DOMElement> findChildElements(DOMElement parent, String elementName) {
return parent.getChildren().stream().filter(node -> elementName.equals(node.getLocalName()))
.filter(DOMElement.class::isInstance).map(DOMElement.class::cast).collect(Collectors.toList());
}

public static Optional<String> findChildElementText(DOMNode rootNode, final String elementName) {
return findChildElement(rootNode, elementName) //
.stream() //
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,44 @@ public void testCodeActionsForParentGroupIdRemoval() throws Exception {
testCodeAction(xmlDocument,true, expectedDiagnostic, expectedCodeAction);
}

// @TODO: EDITOR_HINT_MANAGED_DEPENDENCY_OVERRIDE (from m2e.ui)
// EDITOR_HINT_MANAGED_DEPENDENCY_OVERRIDE (from m2e.ui)
@Test
public void testCodeActionsForManagedDependencyOverride() throws Exception {
DOMDocument xmlDocument = createDOMDocument("/codeactions-test/pom-overriding-of-managed-version.xml", xmlLanguageService);

Diagnostic expectedDiagnostic = d(17, 15, 17, 21,
MavenSyntaxErrorCode.OverridingOfManagedDependency,
"Overriding managed version 3.12.0 for commons-lang3");

// Test diagnostic and code action for a different version value
CodeAction expectedCodeAction_1 = ca(expectedDiagnostic, teOp("pom.xml", 17, 31, 17, 31, "<!--$NO-MVN-MAN-VER$-->"));
CodeAction expectedCodeAction_2 = ca(expectedDiagnostic, teOp("pom.xml", 17, 6, 17, 31, ""));
testCodeAction(xmlDocument,false, expectedDiagnostic, expectedCodeAction_1, expectedCodeAction_2);
}

@Test
public void testCodeActionsForManagedDependencyDuplicate() throws Exception {
DOMDocument xmlDocument = createDOMDocument("/codeactions-test/pom-duplication-of-managed-version.xml", xmlLanguageService);

Diagnostic expectedDiagnostic = d(17, 15, 17, 21,
MavenSyntaxErrorCode.OverridingOfManagedDependency,
"Duplicating managed version 3.12.0 for commons-lang3");

// Test diagnostic and code action for the same version value
CodeAction expectedCodeAction_1 = ca(expectedDiagnostic, teOp("pom.xml", 17, 31, 17, 31, "<!--$NO-MVN-MAN-VER$-->"));
CodeAction expectedCodeAction_2 = ca(expectedDiagnostic, teOp("pom.xml", 17, 6, 17, 31, ""));
testCodeAction(xmlDocument,true, expectedDiagnostic, expectedCodeAction_1, expectedCodeAction_2);
}


// @TODO: EDITOR_HINT_MANAGED_PLUGIN_OVERRIDE (from m2e.ui)
// @TODO: EDITOR_HINT_CONFLICTING_LIFECYCLEMAPPING (from m2e.core)
// @TODO: EDITOR_HINT_NOT_COVERED_MOJO_EXECUTION (from m2e.core)
// @TODO: EDITOR_HINT_MISSING_CONFIGURATOR (from m2e.core)
// @TODO: EDITOR_HINT_IMPLICIT_LIFECYCLEMAPPINGEDITOR_HINT_IMPLICIT_LIFECYCLEMAPPING


private void testCodeAction(DOMDocument xmlDocument, boolean ignoreNoGrammar, Diagnostic expectedDiagnostic, CodeAction expectedCodeAction) throws BadLocationException {
private void testCodeAction(DOMDocument xmlDocument, boolean ignoreNoGrammar, Diagnostic expectedDiagnostic, CodeAction... expectedCodeAction) throws BadLocationException {
// Test for expected diagnostics is returned
ContentModelSettings settings = createContentModelSettings(ignoreNoGrammar);
xmlLanguageService.setDocumentProvider((uri) -> xmlDocument);
Expand All @@ -109,7 +138,7 @@ private void testCodeAction(DOMDocument xmlDocument, boolean ignoreNoGrammar, Di
assertDiagnostics(actual, Arrays.asList(expectedDiagnostic), true);

// Test for expected code action is returned
testCodeActionsFor(xmlDocument.getText(), expectedCodeAction.getDiagnostics().get(0), (String) null, "pom.xml",
testCodeActionsFor(xmlDocument.getText(), expectedCodeAction[0].getDiagnostics().get(0), (String) null, "pom.xml",
sharedSettings, xmlLanguageService, -1, expectedCodeAction);
}

Expand Down
14 changes: 14 additions & 0 deletions lemminx-maven/src/test/resources/codeactions-test/parent/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,18 @@
<artifactId>just-a-parent</artifactId>
<version>1.0.0</version>
<packaging>pom</packaging>

<properties>
<commons-lang3-version>3.12.0</commons-lang3-version>
</properties>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>${commons-lang3-version}</version>
</dependency>
</dependencies>
</dependencyManagement>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.eclipse.test</groupId>
<artifactId>just-a-parent</artifactId>
<version>1.0.0</version>
<relativePath>parent/pom.xml</relativePath>
</parent>
<artifactId>just-a-pom</artifactId>
<packaging>pom</packaging>

<dependencies>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>3.12.0</version>
</dependency>
</dependencies>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.eclipse.test</groupId>
<artifactId>just-a-parent</artifactId>
<version>1.0.0</version>
<relativePath>parent/pom.xml</relativePath>
</parent>
<artifactId>just-a-pom</artifactId>
<packaging>pom</packaging>

<dependencies>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>3.12.1</version>
</dependency>
</dependencies>
</project>

0 comments on commit a64d68e

Please sign in to comment.