Merge pull request #205 from galaxyproject/refactor_validation

Refactor validation

server/galaxyls/constants.py

@@ -1,7 +1,5 @@
 """This module contains definitions for constants used by the Galaxy Tools Language Server."""
 
-SERVER_NAME = "Galaxy Tools LS"
-
 
 class Commands:
     AUTO_CLOSE_TAGS = "gls.completion.autoCloseTags"

server/galaxyls/server.py

@@ -48,10 +48,7 @@
     CompletionMode,
     GalaxyToolsConfiguration,
 )
-from galaxyls.constants import (
-    Commands,
-    SERVER_NAME,
-)
+from galaxyls.constants import Commands
 from galaxyls.services.language import GalaxyToolLanguageService
 from galaxyls.services.validation import DocumentValidator
 from galaxyls.services.xml.document import XmlDocument
@@ -72,7 +69,7 @@ class GalaxyToolsLanguageServer(LanguageServer):
 
     def __init__(self) -> None:
         super().__init__()
-        self.service = GalaxyToolLanguageService(SERVER_NAME)
+        self.service = GalaxyToolLanguageService()
         self.configuration: GalaxyToolsConfiguration = GalaxyToolsConfiguration()
 
 

server/galaxyls/services/language.py

@@ -61,8 +61,8 @@ class GalaxyToolLanguageService:
     by the LSP.
     """
 
-    def __init__(self, server_name: str) -> None:
-        self.xsd_service = GalaxyToolXsdService(server_name)
+    def __init__(self) -> None:
+        self.xsd_service = GalaxyToolXsdService()
         self.format_service = GalaxyToolFormatService()
         self.xsd_tree = self.xsd_service.xsd_parser.get_tree()
         self.xml_context_service = XmlContextService(self.xsd_tree)
@@ -81,9 +81,7 @@ def set_workspace(self, workspace: Workspace) -> None:
         )
 
     def get_diagnostics(self, xml_document: XmlDocument) -> List[Diagnostic]:
-        """Validates the Galaxy tool XML document and returns a list
-        of diagnostics if there are any problems.
-        """
+        """Validates the Galaxy tool XML document and returns a list of diagnostics if there are any problems."""
         return self.xsd_service.validate_document(xml_document) + self.linter.lint_document(xml_document)
 
     def get_documentation(self, xml_document: XmlDocument, position: Position) -> Optional[Hover]:
@@ -105,9 +103,7 @@ def get_documentation(self, xml_document: XmlDocument, position: Position) -> Op
         return None
 
     def format_document(self, content: str, params: DocumentFormattingParams) -> List[TextEdit]:
-        """Given the document contents returns the list of TextEdits
-        needed to properly format and layout the document.
-        """
+        """Given the document contents returns the list of TextEdits needed to properly format and layout the document."""
         return self.format_service.format(content, params)
 
     def get_completion(

server/galaxyls/services/tools/linting.py

@@ -1,17 +1,18 @@
-from typing import List, cast
-from lxml import etree
+from typing import List
 
 from galaxy.tool_util.lint import (
-    lint_tool_source_with,
+    lint_xml_with,
     LintContext,
     LintLevel,
     XMLLintMessageXPath,
 )
-from galaxy.tool_util.parser import get_tool_source
-from galaxy.util import xml_macros
+from pygls.lsp.types import (
+    Diagnostic,
+    DiagnosticSeverity,
+)
+
 from galaxyls.services.tools.common import ToolLinter
 from galaxyls.services.xml.document import XmlDocument
-from pygls.lsp.types import Diagnostic, Range, DiagnosticSeverity
 
 
 class GalaxyToolLinter(ToolLinter):
@@ -20,21 +21,20 @@ class GalaxyToolLinter(ToolLinter):
     def lint_document(self, xml_document: XmlDocument) -> List[Diagnostic]:
         """ """
         result: List[Diagnostic] = []
-        if xml_document.is_macros_file:
+        xml_tree = xml_document.xml_tree_expanded
+        if not xml_document.is_tool_file or xml_tree is None:
             return result
-        xml_tree, _ = xml_macros.load_with_references(xml_document.document.path)
-        tool_source = get_tool_source(xml_tree=xml_tree)
         lint_context = LintContext(level=LintLevel.SILENT, lint_message_class=XMLLintMessageXPath)
-        context = lint_tool_source_with(lint_context, tool_source)
+        context = lint_xml_with(lint_context, xml_tree)
         result.extend(
             [
-                self._to_diagnostic(lint_message, xml_tree, xml_document, DiagnosticSeverity.Error)
+                self._to_diagnostic(lint_message, xml_document, DiagnosticSeverity.Error)
                 for lint_message in context.error_messages
             ]
         )
         result.extend(
             [
-                self._to_diagnostic(lint_message, xml_tree, xml_document, DiagnosticSeverity.Warning)
+                self._to_diagnostic(lint_message, xml_document, DiagnosticSeverity.Warning)
                 for lint_message in context.warn_messages
             ]
         )
@@ -43,22 +43,14 @@ def lint_document(self, xml_document: XmlDocument) -> List[Diagnostic]:
     def _to_diagnostic(
         self,
         lint_message: XMLLintMessageXPath,
-        xml_tree: etree._ElementTree,
         xml_document: XmlDocument,
         level: DiagnosticSeverity,
     ) -> Diagnostic:
-        range = self._get_range_from_xpath(lint_message.xpath, xml_tree, xml_document)
+        range = xml_document.get_element_range_from_xpath_or_default(lint_message.xpath)
         result = Diagnostic(
             range=range,
             message=lint_message.message,
             source=self.diagnostics_source,
             severity=level,
         )
         return result
-
-    def _get_range_from_xpath(self, xpath: str, xml_tree: etree._ElementTree, xml_document: XmlDocument) -> Range:
-        result = None
-        found = cast(list, xml_tree.xpath(xpath))[0]
-        if found is not None:
-            result = xml_document.get_element_name_range_at_line(found.tag, found.sourceline - 1)
-        return result or xml_document.get_default_range()

server/galaxyls/services/xml/document.py

@@ -1,10 +1,14 @@
 from typing import (
+    Any,
+    cast,
     Dict,
     List,
     Optional,
 )
 
 from anytree.search import findall
+from galaxy.util import xml_macros
+from lxml import etree
 from pygls.lsp.types import (
     Position,
     Range,
@@ -44,6 +48,8 @@ def __init__(self, document: Document):
             "tool": DocumentType.TOOL,
             "macros": DocumentType.MACROS,
         }
+        self._xml_tree: Optional[etree._ElementTree] = None
+        self._xml_tree_expanded: Optional[etree._ElementTree] = None
 
     @property
     def node_type(self) -> NodeType:
@@ -103,6 +109,40 @@ def is_tool_file(self) -> bool:
         """Indicates if the document is a tool definition file."""
         return self.document_type == DocumentType.TOOL
 
+    @property
+    def xml_tree(self) -> Optional[etree._ElementTree]:
+        """Internal XML tree structure."""
+        if self._xml_tree is None:
+            try:
+                tree = etree.parse(self.document.path)
+                self._xml_tree = tree
+            except etree.XMLSyntaxError:
+                pass  # Invalid XML document
+        return self._xml_tree
+
+    @property
+    def xml_has_syntax_errors(self) -> bool:
+        return self.xml_tree is None
+
+    @property
+    def xml_tree_expanded(self) -> Optional[etree._ElementTree]:
+        """Internal XML tree structure after expanding macros.
+
+        If there are no macro definitions, it returns the same as `xml_tree` property."""
+        if self._xml_tree_expanded is None:
+            if self.uses_macros:
+                try:
+                    expanded_tool_tree, _ = xml_macros.load_with_references(self.document.path)
+                    self._xml_tree_expanded = expanded_tool_tree
+                except etree.XMLSyntaxError:
+                    pass  # Invalid XML document
+                except BaseException:
+                    pass  # TODO: Errors expanding macros should be catch during validation
+            if self._xml_tree_expanded is None:
+                # Fallback to the non-expanded version if something failed
+                self._xml_tree_expanded = self.xml_tree
+        return self._xml_tree_expanded
+
     def get_node_at(self, offset: int) -> Optional[XmlSyntaxNode]:
         """Gets the syntax node a the given offset."""
         return self.root.find_node_at(offset) if self.root else None
@@ -231,12 +271,38 @@ def get_default_range(self) -> Range:
             return self.get_element_name_range(self.root) or DEFAULT_RANGE
         return DEFAULT_RANGE
 
-    def get_element_name_range_at_line(self, name: str, line_number: int) -> Range:
-        """Gets the range of the element with the given tag name located at the given line number."""
-        line_text = self.document.lines[line_number]
-        start = line_text.index(f"<{name}") + 1
-        end = start + len(name)
-        return Range(
-            start=Position(line=line_number, character=start),
-            end=Position(line=line_number, character=end),
-        )
+    def get_tree_element_from_xpath(self, tree, xpath: Optional[str]) -> Optional[Any]:
+        if xpath is None or tree is None:
+            return None
+        element: Any = tree.xpath(xpath)
+        if element is not None:
+            if isinstance(element, list):
+                element = cast(list, element)
+                if len(element) > 0:
+                    return element[0]
+        return None
+
+    def get_element_from_xpath(self, xpath: Optional[str]) -> Optional[Any]:
+        return self.get_tree_element_from_xpath(self.xml_tree, xpath)
+
+    def get_internal_element_range_or_default(self, element: Optional[Any]) -> Range:
+        if element is not None:
+            line_number = element.sourceline - 1
+            line_text = self.document.lines[line_number]
+            if isinstance(element, etree._Comment):
+                text = cast(str, element.text)
+                start = line_text.index(f"{text}")
+                end = start + len(text)
+            else:
+                # Prepend '<' for searching tag names
+                start = line_text.index(f"<{element.tag}") + 1
+                end = start + len(element.tag)
+            return Range(
+                start=Position(line=line_number, character=start),
+                end=Position(line=line_number, character=end),
+            )
+        return self.get_default_range()
+
+    def get_element_range_from_xpath_or_default(self, xpath: Optional[str]) -> Range:
+        element = self.get_element_from_xpath(xpath)
+        return self.get_internal_element_range_or_default(element)

server/galaxyls/services/xsd/service.py

@@ -22,7 +22,7 @@
 )
 from galaxyls.services.xsd.parser import GalaxyToolXsdParser
 from galaxyls.services.xsd.types import XsdBase
-from galaxyls.services.xsd.validation import GalaxyToolValidationService
+from galaxyls.services.xsd.validation import GalaxyToolSchemaValidationService
 
 NO_DOC_MARKUP = MarkupContent(kind=MarkupKind.Markdown, value=MSG_NO_DOCUMENTATION_AVAILABLE)
 
@@ -34,13 +34,12 @@ class GalaxyToolXsdService:
     the XSD schema and validate XML files against it.
     """
 
-    def __init__(self, server_name: str) -> None:
+    def __init__(self) -> None:
         """Initializes the validator by loading the XSD."""
-        self.server_name = server_name
         self.xsd_doc: etree._ElementTree = etree.parse(str(TOOL_XSD_FILE))
         self.xsd_schema = etree.XMLSchema(self.xsd_doc)
         self.xsd_parser = GalaxyToolXsdParser(self.xsd_doc.getroot())
-        self.validator = GalaxyToolValidationService(server_name, self.xsd_schema)
+        self.validator = GalaxyToolSchemaValidationService(self.xsd_schema)
 
     def validate_document(self, xml_document: XmlDocument) -> List[Diagnostic]:
         """Validates the Galaxy tool xml using the XSD schema and returns a list