Ca add variable (#347)

* code changes to add variable code action Signed-off-by: Arun Venmany <[email protected]> * some refactoring Signed-off-by: Arun Venmany <[email protected]> * adding a blank space in add variable text, since variable processing will consider only non-empty vars Signed-off-by: Arun Venmany <[email protected]> * changes to add new variables to variableMap Signed-off-by: Arun Venmany <[email protected]> * added more diagnostic tests Signed-off-by: Arun Venmany <[email protected]> * changed variable empty value diagnostic severity to warning Signed-off-by: Arun Venmany <[email protected]> * changes based on review comments Signed-off-by: Arun Venmany <[email protected]> * changes based on review comments Signed-off-by: Arun Venmany <[email protected]> --------- Signed-off-by: Arun Venmany <[email protected]>
OpenLiberty · Feb 21, 2025 · d3c5dbe · d3c5dbe
1 parent 349a02c
commit d3c5dbe
Show file tree

Hide file tree

Showing 8 changed files with 271 additions and 30 deletions.
diff --git a/lemminx-liberty/src/it/variable-processing-ol-it/src/test/java/LibertyWorkspaceIT.java b/lemminx-liberty/src/it/variable-processing-ol-it/src/test/java/LibertyWorkspaceIT.java
@@ -204,17 +204,27 @@ public void testInvalidVariableDiagnosticWithCodeAction() throws IOException, Ba
         List<String> variables = new ArrayList<>();
         variables.add("default.https.port");
         List<CodeAction> codeActions = new ArrayList<>();
+        TextEdit texted;
+        CodeAction invalidCodeAction;
         for (String nextVar : variables) {
             String variableInDoc = String.format("${%s}", nextVar);
-            TextEdit texted = te(invalid1.getRange().getStart().getLine(), invalid1.getRange().getStart().getCharacter(),
+            texted = te(invalid1.getRange().getStart().getLine(), invalid1.getRange().getStart().getCharacter(),
                     invalid1.getRange().getEnd().getLine(), invalid1.getRange().getEnd().getCharacter(), variableInDoc);
-            CodeAction invalidCodeAction = ca(invalid1, texted);
+            invalidCodeAction = ca(invalid1, texted);
             codeActions.add(invalidCodeAction);
             invalidCodeAction.getEdit()
                     .getDocumentChanges()
                     .get(0).getLeft().getTextDocument()
                     .setUri(serverXmlFile.toURI().toString());
         }
-        XMLAssert.testCodeActionsFor(serverXML, serverXmlFile.toURI().toString(), invalid1, codeActions.get(0));
+        texted = te(7, 0,
+                7, 0, String.format("    <variable name=\"%s\" value=\" \"/> %s","default.https",System.lineSeparator()));
+        invalidCodeAction = ca(invalid1, texted);
+        invalidCodeAction.getEdit()
+                .getDocumentChanges()
+                .get(0).getLeft().getTextDocument()
+                .setUri(serverXmlFile.toURI().toString());
+        codeActions.add(invalidCodeAction);
+        XMLAssert.testCodeActionsFor(serverXML, serverXmlFile.toURI().toString(), invalid1, codeActions.get(0), codeActions.get(1));
     }
 }
diff --git a/...y/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyCompletionParticipant.java b/...y/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyCompletionParticipant.java
@@ -57,6 +57,7 @@ public void onAttributeValue(String valuePrefix, ICompletionRequest request, ICo
         Properties variableProps = SettingsService.getInstance()
                 .getVariablesForServerXml(request.getXMLDocument()
                         .getDocumentURI());
+        LibertyUtils.checkAndAddNewVariables(request.getXMLDocument(), variableProps);
         //getting all existing variables in current completion prefix string
         List<VariableLoc> variables = LibertyUtils.getVariablesFromTextContent(valuePrefix);
         String variablePrefix = "";

diff --git a/...y/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyDiagnosticParticipant.java b/...y/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyDiagnosticParticipant.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
-* Copyright (c) 2020, 2024 IBM Corporation and others.
+* Copyright (c) 2020, 2025 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
@@ -126,24 +126,11 @@ private void validateVariables(DOMDocument domDocument, List<Diagnostic> diagnos
             diagnosticsList.add(diag);
             return;
         }
-        for (VariableLoc variable : variables) {
-            if (!variablesMap.containsKey(variable.getValue())) {
-                String variableInDoc = String.format("${%s}", variable.getValue());
-                //range is used in ReplaceVariable to provide quick fix.
-                // we just need the variable value range here as ${} is added in replace variable message
-                Range range = XMLPositionUtility.createRange(variable.getStartLoc() - 2, variable.getEndLoc() + 1,
-                        domDocument);
-                String message = "ERROR: The variable \"" + variable.getValue() + "\" does not exist.";
-
-                Diagnostic diag = new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, INCORRECT_VARIABLE_CODE);
-                diag.setData(variable.getValue());
-                diagnosticsList.add(diag);
-            }
-        }
+        LibertyUtils.checkAndAddNewVariables(domDocument, variablesMap);
+        validateVariableExists(domDocument, diagnosticsList, variables, variablesMap);
+        validateVariableDataTypeValues(domDocument,diagnosticsList,variablesMap);
     }
 
-
-
     private void validateFeaturesAndPlatforms(DOMDocument domDocument, List<Diagnostic> list, DOMNode featureManager, Set<String> includedFeatures) {
 
         LibertyRuntime runtimeInfo = LibertyUtils.getLibertyRuntimeInfo(domDocument);
@@ -613,4 +600,80 @@ private static String getOtherFeatureName(Set<String> includedFeatures, Map<Stri
                 .findFirst().orElse(null);
         return otherFeatureName;
     }
+
+
+    /**
+     * Validate variable node attributes and values
+     * TODO add DataType level validations for each variables
+     * ie, make sure any variable used is of correct datatype
+     * for example, when a variable is used in one xml element and the valid value is number, but variable defined value is NaN
+     * @param domDocument xml document
+     * @param diagnosticsList existing diagnostic list
+     * @param variablesMap populated variables from all liberty config files
+     */
+    private void validateVariableDataTypeValues(DOMDocument domDocument, List<Diagnostic> diagnosticsList, Properties variablesMap) {
+        List<DOMNode> nodes = domDocument.getDocumentElement().getChildren();
+
+        for (DOMNode node : nodes) {
+            String nodeName = node.getNodeName();
+            if (LibertyConstants.VARIABLE_ELEMENT.equals(nodeName)) {
+                String varName =  node.getAttribute("name");
+                Range range = XMLPositionUtility.createRange(node.getStart(), node.getEnd(),
+                        domDocument);
+                if (!varName.isEmpty()) {
+                    validateVariableValueNonEmpty(diagnosticsList, node, varName, range);
+                }
+                else{
+                    String message = "ERROR: The variable should have a valid name defined in name attribute.";
+                    Diagnostic diag = new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, INCORRECT_VARIABLE_CODE);
+                    diagnosticsList.add(diag);
+                }
+            }
+        }
+    }
+
+    /**
+     * validate variable value is not null or empty
+     * @param diagnosticsList diagnostics list
+     * @param varNode current variable DomNode
+     * @param varName current variable Name Attribute value
+     * @param range diagnostics range
+     */
+    private static void validateVariableValueNonEmpty(List<Diagnostic> diagnosticsList, DOMNode varNode, String varName, Range range) {
+        // A variable can have either a value attribute OR a defaultValue attribute.
+        String varValue = varNode.getAttribute("value");
+        String varDefaultValue = varNode.getAttribute("defaultValue");
+        if(varValue==null && varDefaultValue==null){
+            String message = "ERROR: The variable \"" + varName + "\" should have value or defaultValue attribute defined.";
+            Diagnostic diag = new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, INCORRECT_VARIABLE_CODE);
+            diagnosticsList.add(diag);
+        }
+        else if((varValue!=null && varValue.trim().isEmpty())||(varDefaultValue!=null && varDefaultValue.trim().isEmpty())){
+            String message = "WARNING: The variable \"" + varName + "\" should have a valid value defined.";
+            Diagnostic diag = new Diagnostic(range, message, DiagnosticSeverity.Warning, LIBERTY_LEMMINX_SOURCE, INCORRECT_VARIABLE_CODE);
+            diagnosticsList.add(diag);
+        }
+    }
+
+    /**
+     * validate variable is defined for any usage
+     * @param domDocument xml document
+     * @param diagnosticsList diagnostics list
+     * @param variables variables in use in server xml
+     * @param variablesMap all variables defined in liberty config files
+     */
+    private static void validateVariableExists(DOMDocument domDocument, List<Diagnostic> diagnosticsList, List<VariableLoc> variables, Properties variablesMap) {
+        for (VariableLoc variable : variables) {
+            if (!variablesMap.containsKey(variable.getValue())) {
+                //range is used in ReplaceVariable to provide quick fix.
+                // we just need the variable value range here as ${} is added in replace variable message
+                Range range = XMLPositionUtility.createRange(variable.getStartLoc() - 2, variable.getEndLoc() + 1,
+                        domDocument);
+                String message = "ERROR: The variable \"" + variable.getValue() + "\" does not exist.";
+                Diagnostic diag = new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, INCORRECT_VARIABLE_CODE);
+                diag.setData(variable.getValue());
+                diagnosticsList.add(diag);
+            }
+        }
+    }
 }
diff --git a/...iberty/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyHoverParticipant.java b/...iberty/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyHoverParticipant.java
@@ -51,6 +51,7 @@ public Hover onAttributeValue(IHoverRequest request, CancelChecker cancelChecker
         Properties variableMap = SettingsService.getInstance()
                 .getVariablesForServerXml(request.getXMLDocument()
                         .getDocumentURI());
+        LibertyUtils.checkAndAddNewVariables(request.getXMLDocument(), variableMap);
         StringBuilder stringBuilder = new StringBuilder();
         Iterator<VariableLoc> varIter = variables.iterator();
         while (varIter.hasNext()) {

diff --git a/...ty/src/main/java/io/openliberty/tools/langserver/lemminx/codeactions/ReplaceVariable.java b/...ty/src/main/java/io/openliberty/tools/langserver/lemminx/codeactions/ReplaceVariable.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2024 IBM Corporation and others.
+ * Copyright (c) 2024, 2025 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
@@ -15,14 +15,14 @@
 
 import com.google.gson.JsonPrimitive;
 import io.openliberty.tools.langserver.lemminx.services.SettingsService;
+import io.openliberty.tools.langserver.lemminx.util.LibertyUtils;
 import org.eclipse.lemminx.commons.CodeActionFactory;
 import org.eclipse.lemminx.dom.DOMDocument;
 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.Position;
 import org.eclipse.lsp4j.jsonrpc.CancelChecker;
 
 import java.util.List;
@@ -53,19 +53,27 @@ public void doCodeAction(ICodeActionRequest request, List<CodeAction> codeAction
             final boolean replaceVariable = invalidVariable != null && !invalidVariable.isBlank();
 
             if (replaceVariable) {
-                Properties existingVariables=SettingsService.getInstance().getVariablesForServerXml(document.getDocumentURI());
+                Properties existingVariables = SettingsService.getInstance().getVariablesForServerXml(document.getDocumentURI());
+                LibertyUtils.checkAndAddNewVariables(document, existingVariables);
                 // filter with entered word -> may not be required
                 String finalInvalidVariable = invalidVariable;
                 Set<Map.Entry<Object, Object>> filteredVariables = existingVariables
-                        .entrySet().stream().filter(entry ->
-                                entry.getKey().toString().toLowerCase()
-                                        .contains(finalInvalidVariable.toLowerCase()))
+                        .entrySet().stream()
+                        .filter(entry -> LibertyUtils.containsEachOther(entry.toString(), finalInvalidVariable, false))
                         .collect(Collectors.toSet());
                 for (Map.Entry<Object, Object> nextVariable : filteredVariables) {
                     String title = "Replace attribute value with " + nextVariable.getKey();
                     String variableInDoc = String.format("${%s}", nextVariable.getKey().toString());
                     codeActions.add(CodeActionFactory.replace(title, diagnostic.getRange(), variableInDoc, document.getTextDocument(), diagnostic));
                 }
+                // code action for add variable
+                Position insertPos = new Position();
+                // variable will be added in the current line where diagnostic is showing
+                // line separator in end of text insert will move current diagnostic line to 1 line
+                insertPos.setLine(diagnostic.getRange().getEnd().getLine());
+                codeActions.add(CodeActionFactory.insert("Add variable " + invalidVariable, insertPos,
+                        String.format("    <variable name=\"%s\" value=\" \"/> %s", invalidVariable, System.lineSeparator()),
+                        document.getTextDocument(), diagnostic));
             }
         } catch (Exception e) {
             // BadLocationException not expected

diff --git a/...-liberty/src/main/java/io/openliberty/tools/langserver/lemminx/util/LibertyConstants.java b/...-liberty/src/main/java/io/openliberty/tools/langserver/lemminx/util/LibertyConstants.java
@@ -32,6 +32,7 @@ private LibertyConstants() {
     public static final String PLATFORM_ELEMENT = "platform";
     public static final String PUBLIC_VISIBILITY = "PUBLIC";
     public static final String PRIVATE_VISIBILITY = "PRIVATE";
+    public static final String VARIABLE_ELEMENT = "variable";
 
     // following URI standard of using "/"
     public static final String WLP_USER_CONFIG_DIR = "/usr/shared/config/";

diff --git a/lemminx-liberty/src/main/java/io/openliberty/tools/langserver/lemminx/util/LibertyUtils.java b/lemminx-liberty/src/main/java/io/openliberty/tools/langserver/lemminx/util/LibertyUtils.java
@@ -23,14 +23,18 @@
 import java.nio.file.WatchKey;
 import java.nio.file.WatchService;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.Properties;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
+import io.openliberty.tools.common.plugins.util.VariableUtility;
 import io.openliberty.tools.langserver.lemminx.models.feature.VariableLoc;
 import org.eclipse.lemminx.dom.DOMDocument;
 
@@ -42,6 +46,8 @@
 import io.openliberty.tools.langserver.lemminx.services.LibertyWorkspace;
 import io.openliberty.tools.langserver.lemminx.services.SettingsService;
 
+import javax.xml.xpath.XPathExpressionException;
+
 public class LibertyUtils {
 
     private static final Logger LOGGER = Logger.getLogger(LibertyUtils.class.getName());
@@ -625,4 +631,58 @@ public static List<VariableLoc> getVariablesFromTextContent(String docContent) {
         }
         return variables;
     }
+
+    /**
+     * check whether two strings contains each other
+     *
+     * @param string        string 1
+     * @param otherString   string 2
+     * @param caseSensitive is comparison case-sensitive or not
+     * @return boolean result
+     */
+    public static boolean containsEachOther(String string, String otherString, boolean caseSensitive) {
+        if (!caseSensitive) {
+            string = string.toLowerCase();
+            otherString = otherString.toLowerCase();
+        }
+        return string.contains(otherString) || otherString.contains(string);
+    }
+
+    /**
+     * Add new variables to variableProps
+     * Used to update local variable map with latest data
+     * Checks the xml document for any new variables added,
+     * if any new variable is found, its added to local variable map
+     *
+     * @param document      xml document
+     * @param variableProps current variable properties map
+     */
+    public static void checkAndAddNewVariables(DOMDocument document, Properties variableProps) {
+        List<Properties> existingVars;
+        try {
+            existingVars = VariableUtility.parseVariables(document, false, false, true);
+        } catch (XPathExpressionException e) {
+            LOGGER.warning("unable to parse variables for %s. Error message is %s ".formatted(document.getDocumentURI(), e.getMessage()));
+            return;
+        }
+        // a dirty check, verifies whether all variables in server.xml is present in variable map
+        // if not, we consider this variable is added recently with code action or manually
+        Map<String, String> additionalVarMap = new HashMap<>();
+        Map<String, String> combined = new HashMap<>();
+        //put defaultValue first
+        for (final String name : existingVars.get(1).stringPropertyNames()) {
+            additionalVarMap.put(name, existingVars.get(1).getProperty(name));
+        }
+        for (final String name : existingVars.get(0).stringPropertyNames()) {
+            additionalVarMap.put(name, existingVars.get(0).getProperty(name));
+        }
+        for (Map.Entry<String, String> entry : additionalVarMap.entrySet()) {
+            if (!variableProps.containsKey(entry.getKey())) {
+                combined.put(entry.getKey(), entry.getValue());
+            }
+        }
+        if (!combined.isEmpty()) {
+            variableProps.putAll(combined);
+        }
+    }
 }