Skip to content

Commit

Permalink
Diagnostics and Quick fixes for lost config elements (#220)
Browse files Browse the repository at this point in the history
* init push

* revert featureservice, refactor, addfeature wip

* AddFeature indentations, clarification

* PR cleanup

* move featureListGraph to workspaces

* add feature clarification and cleanup

* Use new surefire with junit 5 support, skip tests in build step GHA

* Fix uri string in tests

* graph cache. github actions still fail, pass local

* correct trigger for empty featureManager

* PR comments

* reduce request delay 120 -> 10

* working state for "empty featureManager"

* add logger to addFeature

---------

Co-authored-by: Evie Lau <[email protected]>
  • Loading branch information
dshimo and evie-lau authored Oct 10, 2023
1 parent 0d7196c commit 9cd3b88
Show file tree
Hide file tree
Showing 17 changed files with 684 additions and 45 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ jobs:
java-version: 17
- name: Build Lemminx Liberty
working-directory: ./lemminx-liberty
run: ./mvnw clean package -ntp
run: ./mvnw clean package -ntp -DskipTests
- name: Test Lemminx Liberty
working-directory: ./lemminx-liberty
run: ./mvnw verify -ntp
11 changes: 10 additions & 1 deletion lemminx-liberty/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<groupId>io.openliberty.tools</groupId>
<artifactId>liberty-langserver-lemminx</artifactId>
<packaging>jar</packaging>
<version>2.0.2-SNAPSHOT</version>
<version>2.1-SNAPSHOT</version>

<name>lemminx-liberty</name>
<url>https://github.com/OpenLiberty/liberty-language-server</url>
Expand Down Expand Up @@ -55,6 +55,15 @@
<target>17</target>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.1.2</version>
</plugin>
<plugin>
<artifactId>maven-failsafe-plugin</artifactId>
<version>3.1.2</version>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-shade-plugin</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.eclipse.lsp4j.jsonrpc.CancelChecker;

import io.openliberty.tools.langserver.lemminx.codeactions.AddAttribute;
import io.openliberty.tools.langserver.lemminx.codeactions.AddFeature;
import io.openliberty.tools.langserver.lemminx.codeactions.CreateFile;
import io.openliberty.tools.langserver.lemminx.codeactions.EditAttribute;
import io.openliberty.tools.langserver.lemminx.codeactions.ReplaceFeature;
Expand Down Expand Up @@ -55,6 +56,7 @@ private void registerCodeActions() {
codeActionParticipants.put(LibertyDiagnosticParticipant.NOT_OPTIONAL_CODE, new EditAttribute());
codeActionParticipants.put(LibertyDiagnosticParticipant.IMPLICIT_NOT_OPTIONAL_CODE, new AddAttribute());
codeActionParticipants.put(LibertyDiagnosticParticipant.INCORRECT_FEATURE_CODE, new ReplaceFeature());
codeActionParticipants.put(LibertyDiagnosticParticipant.MISSING_CONFIGURED_FEATURE_CODE, new AddFeature());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,27 +23,41 @@
import org.eclipse.lsp4j.Range;
import org.eclipse.lsp4j.jsonrpc.CancelChecker;

import io.openliberty.tools.langserver.lemminx.data.FeatureListGraph;
import io.openliberty.tools.langserver.lemminx.data.LibertyRuntime;
import io.openliberty.tools.langserver.lemminx.services.FeatureService;
import io.openliberty.tools.langserver.lemminx.services.LibertyProjectsManager;
import io.openliberty.tools.langserver.lemminx.services.LibertyWorkspace;
import io.openliberty.tools.langserver.lemminx.services.SettingsService;
import io.openliberty.tools.langserver.lemminx.util.*;

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.logging.Logger;

public class LibertyDiagnosticParticipant implements IDiagnosticsParticipant {
private static final Logger LOGGER = Logger.getLogger(LibertyDiagnosticParticipant.class.getName());

public static final String LIBERTY_LEMMINX_SOURCE = "liberty-lemminx";

public static final String MISSING_FILE_MESSAGE = "The resource at the specified location could not be found.";
public static final String MISSING_FILE_CODE = "missing_file";

public static final String MISSING_CONFIGURED_FEATURE_MESSAGE = "This config element does not relate to a feature configured in the featureManager. Remove this element or add a relevant feature.";
public static final String MISSING_CONFIGURED_FEATURE_CODE = "lost_config_element";

public static final String NOT_OPTIONAL_MESSAGE = "The specified resource cannot be skipped. Check location value or set optional to true.";
public static final String NOT_OPTIONAL_CODE = "not_optional";
public static final String IMPLICIT_NOT_OPTIONAL_MESSAGE = "The specified resource cannot be skipped. Check location value or add optional attribute.";
public static final String IMPLICIT_NOT_OPTIONAL_CODE = "implicit_not_optional";

public static final String INCORRECT_FEATURE_CODE = "incorrect_feature";

private Set<String> includedFeatures;

@Override
public void doDiagnostics(DOMDocument domDocument, List<Diagnostic> diagnostics,
Expand All @@ -53,22 +67,29 @@ public void doDiagnostics(DOMDocument domDocument, List<Diagnostic> diagnostics,
try {
validateDom(domDocument, diagnostics);
} catch (IOException e) {
System.err.println("Error validating document " + domDocument.getDocumentURI());
System.err.println(e.getMessage());
LOGGER.severe("Error validating document " + domDocument.getDocumentURI());
LOGGER.severe(e.getMessage());
}
}

private void validateDom(DOMDocument domDocument, List<Diagnostic> list) throws IOException {
private void validateDom(DOMDocument domDocument, List<Diagnostic> diagnosticsList) throws IOException {
List<DOMNode> nodes = domDocument.getDocumentElement().getChildren();

List<Diagnostic> tempDiagnosticsList = new ArrayList<Diagnostic>();
includedFeatures = new HashSet<>();
LibertyWorkspace workspace = LibertyProjectsManager.getInstance().getWorkspaceFolder(domDocument.getDocumentURI());
// TODO: Consider adding a cached feature list onto repo to optimize
FeatureListGraph featureGraph = (workspace == null) ? new FeatureListGraph() : workspace.getFeatureListGraph();
for (DOMNode node : nodes) {
if (LibertyConstants.FEATURE_MANAGER_ELEMENT.equals(node.getNodeName())) {
validateFeature(domDocument, list, node);
} else if (LibertyConstants.INCLUDE_ELEMENT.equals(node.getNodeName())) {
validateIncludeLocation(domDocument, list, node);
String nodeName = node.getNodeName();
if (LibertyConstants.FEATURE_MANAGER_ELEMENT.equals(nodeName)) {
validateFeature(domDocument, diagnosticsList, node);
} else if (LibertyConstants.INCLUDE_ELEMENT.equals(nodeName)) {
validateIncludeLocation(domDocument, diagnosticsList, node);
} else if (featureGraph.isConfigElement(nodeName)) { // defaults to false
holdConfigElement(domDocument, node, tempDiagnosticsList);
}
}

validateConfigElements(diagnosticsList, tempDiagnosticsList, featureGraph);
}

private void validateFeature(DOMDocument domDocument, List<Diagnostic> list, DOMNode featureManager) {
Expand All @@ -80,7 +101,6 @@ private void validateFeature(DOMDocument domDocument, List<Diagnostic> list, DOM

// Search for duplicate features
// or features that do not exist
Set<String> includedFeatures = new HashSet<>();
List<DOMNode> features = featureManager.getChildren();
for (DOMNode featureNode : features) {
DOMNode featureTextNode = (DOMNode) featureNode.getChildNodes().item(0);
Expand All @@ -93,13 +113,13 @@ private void validateFeature(DOMDocument domDocument, List<Diagnostic> list, DOM
Range range = XMLPositionUtility.createRange(featureTextNode.getStart(), featureTextNode.getEnd(),
domDocument);
String message = "ERROR: The feature \"" + featureName + "\" does not exist.";
list.add(new Diagnostic(range, message, DiagnosticSeverity.Error, "liberty-lemminx", INCORRECT_FEATURE_CODE));
list.add(new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, INCORRECT_FEATURE_CODE));
} else {
if (includedFeatures.contains(featureName)) {
Range range = XMLPositionUtility.createRange(featureTextNode.getStart(),
featureTextNode.getEnd(), domDocument);
String message = "ERROR: " + featureName + " is already included.";
list.add(new Diagnostic(range, message, DiagnosticSeverity.Error, "liberty-lemminx"));
list.add(new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE));
} else {
includedFeatures.add(featureName);
}
Expand All @@ -117,7 +137,7 @@ private void validateFeature(DOMDocument domDocument, List<Diagnostic> list, DOM
* 2) performed in isConfigXMLFile
* 4) not yet implemented/determined
*/
private void validateIncludeLocation(DOMDocument domDocument, List<Diagnostic> list, DOMNode node) {
private void validateIncludeLocation(DOMDocument domDocument, List<Diagnostic> diagnosticsList, DOMNode node) {
String locAttribute = node.getAttribute("location");
if (locAttribute == null) {
return;
Expand All @@ -131,7 +151,7 @@ private void validateIncludeLocation(DOMDocument domDocument, List<Diagnostic> l
Range range = XMLPositionUtility.createRange(locNode.getStart(), locNode.getEnd(), domDocument);
if (!locAttribute.endsWith(".xml")) {
String message = "The specified resource is not an XML file.";
list.add(new Diagnostic(range, message, DiagnosticSeverity.Warning, "liberty-lemminx"));
diagnosticsList.add(new Diagnostic(range, message, DiagnosticSeverity.Warning, LIBERTY_LEMMINX_SOURCE));
return;
}

Expand All @@ -144,15 +164,56 @@ private void validateIncludeLocation(DOMDocument domDocument, List<Diagnostic> l
if (!configFile.exists()) {
DOMAttr optNode = node.getAttributeNode("optional");
if (optNode == null) {
list.add(new Diagnostic(range, IMPLICIT_NOT_OPTIONAL_MESSAGE, DiagnosticSeverity.Error, "liberty-lemminx", IMPLICIT_NOT_OPTIONAL_CODE));
diagnosticsList.add(new Diagnostic(range, IMPLICIT_NOT_OPTIONAL_MESSAGE, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, IMPLICIT_NOT_OPTIONAL_CODE));
} else if (optNode.getValue().equals("false")) {
Range optRange = XMLPositionUtility.createRange(optNode.getStart(), optNode.getEnd(), domDocument);
list.add(new Diagnostic(optRange, NOT_OPTIONAL_MESSAGE, DiagnosticSeverity.Error, "liberty-lemminx", NOT_OPTIONAL_CODE));
diagnosticsList.add(new Diagnostic(optRange, NOT_OPTIONAL_MESSAGE, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, NOT_OPTIONAL_CODE));
}
list.add(new Diagnostic(range, MISSING_FILE_MESSAGE, DiagnosticSeverity.Warning, "liberty-lemminx", MISSING_FILE_CODE));
diagnosticsList.add(new Diagnostic(range, MISSING_FILE_MESSAGE, DiagnosticSeverity.Warning, LIBERTY_LEMMINX_SOURCE, MISSING_FILE_CODE));
}
} catch (IllegalArgumentException e) {
list.add(new Diagnostic(range, MISSING_FILE_MESSAGE, DiagnosticSeverity.Warning, "liberty-lemminx-exception", MISSING_FILE_CODE));
diagnosticsList.add(new Diagnostic(range, MISSING_FILE_MESSAGE, DiagnosticSeverity.Warning, "liberty-lemminx-exception", MISSING_FILE_CODE));
}
}

/**
* Create temporary diagnostics for validation for single pass-through.
* @param domDocument
* @param diagnosticsList
* @param configElementNode
* @param tempDiagnosticsList
*/
private void holdConfigElement(DOMDocument domDocument, DOMNode configElementNode, List<Diagnostic> tempDiagnosticsList) {
String configElementName = configElementNode.getNodeName();
Range range = XMLPositionUtility
.createRange(configElementNode.getStart(), configElementNode.getEnd(), domDocument);
Diagnostic tempDiagnostic = new Diagnostic(range, MISSING_CONFIGURED_FEATURE_MESSAGE, null, LIBERTY_LEMMINX_SOURCE, MISSING_CONFIGURED_FEATURE_CODE);
tempDiagnostic.setSource(configElementName);
tempDiagnosticsList.add(tempDiagnostic);
}

/**
* Compare the required feature set with included feature set for each config element.
* @param diagnosticsList
* @param tempDiagnosticsList
* @param featureGraph
*/
private void validateConfigElements(List<Diagnostic> diagnosticsList, List<Diagnostic> tempDiagnosticsList, FeatureListGraph featureGraph) {
if (featureGraph.isEmpty()) {
return;
}
if (includedFeatures.isEmpty()) {
diagnosticsList.addAll(tempDiagnosticsList);
return;
}
for (Diagnostic tempDiagnostic : tempDiagnosticsList) {
String configElement = tempDiagnostic.getSource();
Set<String> includedFeaturesCopy = new HashSet<String>(includedFeatures);
Set<String> compatibleFeaturesList = featureGraph.getAllEnabledBy(configElement);
includedFeaturesCopy.retainAll(compatibleFeaturesList);
if (includedFeaturesCopy.isEmpty()) {
diagnosticsList.add(tempDiagnostic);
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/*******************************************************************************
* Copyright (c) 2023 IBM Corporation and others.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0.
*
* SPDX-License-Identifier: EPL-2.0
*
* Contributors:
* IBM Corporation - initial API and implementation
*******************************************************************************/
package io.openliberty.tools.langserver.lemminx.codeactions;

import java.util.List;
import java.util.Set;
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.DOMElement;
import org.eclipse.lemminx.dom.DOMNode;
import org.eclipse.lemminx.services.extensions.codeaction.ICodeActionParticipant;
import org.eclipse.lemminx.services.extensions.codeaction.ICodeActionRequest;
import org.eclipse.lemminx.utils.XMLPositionUtility;
import org.eclipse.lsp4j.CodeAction;
import org.eclipse.lsp4j.Diagnostic;
import org.eclipse.lsp4j.Range;
import org.eclipse.lsp4j.jsonrpc.CancelChecker;

import io.openliberty.tools.langserver.lemminx.services.LibertyProjectsManager;
import io.openliberty.tools.langserver.lemminx.util.LibertyConstants;

public class AddFeature implements ICodeActionParticipant {
Logger LOGGER = Logger.getLogger(AddFeature.class.getName());

/** This code action adresses 3 main situations:
* 1) Add a feature to an existing empty featureManager
* 2) Add a feature to an existing featureManager with children
* 3) Add a feature and new featureManager
*
* To calculate where to insert, each scenario will use a reference point to calculate range
* 1) The startTag of the featureManager
* 2) The last child of the featureManager
* 3) The startTag of the server.xml
*/
public static final String FEATURE_FORMAT = "<feature>%s</feature>";
public static final String FEATUREMANAGER_FORMAT =
"\n\t<featureManager>"+
"\n\t\t<feature>%s</feature>"+
"\n\t</featureManager>";

@Override
public void doCodeAction(ICodeActionRequest request, List<CodeAction> codeActions, CancelChecker cancelChecker) {
Diagnostic diagnostic = request.getDiagnostic();
DOMDocument document = request.getDocument();
TextDocument textDocument = document.getTextDocument();
// getAllEnabledBy would return all transitive features but typically offers too much
Set<String> featureCandidates = LibertyProjectsManager.getInstance()
.getWorkspaceFolder(document.getDocumentURI())
.getFeatureListGraph().get(diagnostic.getSource()).getEnabledBy();
if (featureCandidates.isEmpty()) {
return;
}

String insertText = "";
int referenceRangeStart = 0;
int referenceRangeEnd = 0;

for (DOMNode node : document.getDocumentElement().getChildren()) {
if (LibertyConstants.FEATURE_MANAGER_ELEMENT.equals(node.getNodeName())) {
DOMNode lastChild = node.getLastChild();
if (node.getChildren().size() > 1) {
// Situation 2
insertText = "\n" + FEATURE_FORMAT;
referenceRangeStart = lastChild.getStart();
referenceRangeEnd = lastChild.getEnd();
} else {
if (lastChild != null && (lastChild.hasChildNodes() || lastChild.isComment())) {
// Situation 2
insertText = "\n" + FEATURE_FORMAT;
referenceRangeStart = lastChild.getStart();
referenceRangeEnd = lastChild.getEnd();
} else {
// Situation 1
insertText = "\n\t" + FEATURE_FORMAT;
DOMElement featureManager = (DOMElement) node;
referenceRangeStart = featureManager.getStartTagOpenOffset();
referenceRangeEnd = featureManager.getStartTagCloseOffset()+1;
}
}
break;
}
}
// Situation 3
if (insertText.isEmpty()) {
insertText = FEATUREMANAGER_FORMAT;
DOMElement server = document.getDocumentElement();
referenceRangeStart = server.getStart();
referenceRangeEnd = server.getStartTagCloseOffset()+1;
}
Range referenceRange = XMLPositionUtility.createRange(referenceRangeStart, referenceRangeEnd, document);

String indent = " ";
try {
indent = request.getXMLGenerator().getWhitespacesIndent();
} catch (BadLocationException e) {
LOGGER.info("Defaulting indent to four spaces.");
}
insertText = IndentUtil.formatText(insertText, indent, referenceRange.getStart().getCharacter());

for (String feature : featureCandidates) {
String title = "Add feature " + feature;
codeActions.add(CodeActionFactory.insert(
title, referenceRange.getEnd(), String.format(insertText, feature), textDocument, diagnostic));
}
}
}
Loading

0 comments on commit 9cd3b88

Please sign in to comment.