Merge pull request #14582 from jmchilton/tool_data_table_tests

Unit tests and type fixes for tool data tables code.

lib/galaxy/managers/tool_data.py

@@ -1,5 +1,6 @@
 from pathlib import Path
 from typing import (
+    cast,
     Dict,
     Optional,
 )
@@ -9,6 +10,7 @@ from galaxy.structured_app import StructuredApp
 from galaxy.tools.data import (
     TabularToolDataField,
     TabularToolDataTable,
+    ToolDataTable,
 )
 from galaxy.tools.data._schema import (
     ToolDataDetails,
@@ -26,13 +28,12 @@ class ToolDataManager:
         self._app = app
 
     @property
-    def data_tables(self) -> Dict[str, TabularToolDataTable]:
+    def data_tables(self) -> Dict[str, ToolDataTable]:
         return self._app.tool_data_tables.data_tables
 
     def index(self) -> ToolDataEntryList:
         """Return all tool data tables."""
-        data_tables = [table.to_dict() for table in self.data_tables.values()]
-        return ToolDataEntryList.construct(__root__=data_tables)
+        return self._app.tool_data_tables.index()
 
     def show(self, table_name: str) -> ToolDataDetails:
         """Get details of a given data table"""
@@ -62,7 +63,7 @@ class ToolDataManager:
 
     def delete(self, table_name: str, values: Optional[str] = None) -> ToolDataDetails:
         """Removes an item from a data table"""
-        data_table = self._data_table(table_name)
+        data_table = self._tabular_data_table(table_name)
         if not values:
             raise exceptions.RequestParameterInvalidException("Invalid values for data table item specified.")
 
@@ -76,14 +77,17 @@ class ToolDataManager:
         data_table.remove_entry(split_values)
         return self._reload_data_table(table_name)
 
-    def _data_table(self, table_name: str) -> TabularToolDataTable:
+    def _tabular_data_table(self, table_name: str) -> TabularToolDataTable:
+        return cast(TabularToolDataTable, self._data_table(table_name))
+
+    def _data_table(self, table_name: str) -> ToolDataTable:
         try:
             return self.data_tables[table_name]
         except KeyError:
             raise exceptions.ObjectNotFound(f"No such data table {table_name}")
 
     def _data_table_field(self, table_name: str, field_name: str) -> TabularToolDataField:
-        out = self._data_table(table_name).get_field(field_name)
+        out = self._tabular_data_table(table_name).get_field(field_name)
         if out is None:
             raise exceptions.ObjectNotFound(f"No such field {field_name} in data table {table_name}.")
         return out

lib/galaxy/tools/data/__init__.py

@@ -17,7 +17,16 @@ import string
 import time
 from glob import glob
 from tempfile import NamedTemporaryFile
-from typing import List
+from typing import (
+    Any,
+    BinaryIO,
+    Dict,
+    List,
+    Optional,
+    Set,
+    Type,
+    Union,
+)
 
 import refgenconf
 import requests
@@ -29,6 +38,10 @@ from galaxy.util.dictifiable import Dictifiable
 from galaxy.util.filelock import FileLock
 from galaxy.util.renamed_temporary_file import RenamedTemporaryFile
 from galaxy.util.template import fill_template
+from ._schema import (
+    ToolDataEntry,
+    ToolDataEntryList,
+)
 
 log = logging.getLogger(__name__)
 
@@ -41,17 +54,19 @@ TOOL_DATA_TABLE_CONF_XML = """<?xml version="1.0"?>
 
 
 class ToolDataPathFiles:
+    update_time: float
+
     def __init__(self, tool_data_path):
         self.tool_data_path = os.path.abspath(tool_data_path)
         self.update_time = 0
 
     @property
-    def tool_data_path_files(self):
+    def tool_data_path_files(self) -> Set[str]:
         if time.time() - self.update_time > 1:
             self.update_files()
         return self._tool_data_path_files
 
-    def update_files(self):
+    def update_files(self) -> None:
         try:
             content = os.walk(self.tool_data_path)
             self._tool_data_path_files = set(
@@ -67,10 +82,10 @@ class ToolDataPathFiles:
             )
             self.update_time = time.time()
         except Exception:
-            log.exception()
+            log.exception("Failed to update _tool_data_path_files")
             self._tool_data_path_files = set()
 
-    def exists(self, path):
+    def exists(self, path: str) -> bool:
         path = os.path.abspath(path)
         if path in self.tool_data_path_files:
             return True
@@ -78,11 +93,20 @@ class ToolDataPathFiles:
             return os.path.exists(path)
 
 
+ConfigFilesT = Union[str, os.PathLike, List[Union[str, os.PathLike]]]
+
+
 class ToolDataTableManager(Dictifiable):
     """Manages a collection of tool data tables"""
 
+    data_tables: Dict[str, "ToolDataTable"]
+
     def __init__(
-        self, tool_data_path, config_filename=None, tool_data_table_config_path_set=None, other_config_dict=None
+        self,
+        tool_data_path: str,
+        config_filename: Optional[ConfigFilesT] = None,
+        tool_data_table_config_path_set=None,
+        other_config_dict=None,
     ):
         self.tool_data_path = tool_data_path
         # This stores all defined data table entries from both the tool_data_table_conf.xml file and the shed_tool_data_table_conf.xml file
@@ -96,41 +120,47 @@ class ToolDataTableManager(Dictifiable):
                 continue
             self.load_from_config_file(single_config_filename, self.tool_data_path, from_shed_config=False)
 
-    def __getitem__(self, key):
+    def index(self) -> ToolDataEntryList:
+        data_tables = [ToolDataEntry(**table.to_dict()) for table in self.data_tables.values()]
+        return ToolDataEntryList.construct(__root__=data_tables)
+
+    def __getitem__(self, key: str):
         return self.data_tables.__getitem__(key)
 
-    def __setitem__(self, key, value):
+    def __setitem__(self, key: str, value):
         return self.data_tables.__setitem__(key, value)
 
-    def __contains__(self, key):
+    def __contains__(self, key: str):
         return self.data_tables.__contains__(key)
 
-    def get(self, name, default=None):
+    def get(self, name: str, default=None):
         try:
             return self[name]
         except KeyError:
             return default
 
-    def set(self, name, value):
+    def set(self, name: str, value):
         self[name] = value
 
-    def get_tables(self):
+    def get_tables(self) -> Dict[str, "ToolDataTable"]:
         return self.data_tables
 
-    def to_dict(self):
+    def to_dict(self, view: str = "collection", value_mapper=None):
         return {name: data_table.to_dict(view="export") for name, data_table in self.data_tables.items()}
 
-    def to_json(self, path):
+    def to_json(self, path: Union[str, os.PathLike]) -> None:
         with open(path, "w") as out:
             out.write(json.dumps(self.to_dict()))
 
     @classmethod
-    def from_dict(cls, d):
+    def from_dict(cls, d: Dict[str, Any]):
         tdtm = cls.__new__(cls)
         tdtm.data_tables = {name: ToolDataTable.from_dict(data) for name, data in d.items()}
         return tdtm
 
-    def load_from_config_file(self, config_filename, tool_data_path, from_shed_config=False):
+    def load_from_config_file(
+        self, config_filename: ConfigFilesT, tool_data_path: Union[str, os.PathLike], from_shed_config: bool = False
+    ):
         """
         This method is called under 3 conditions:
 
@@ -141,9 +171,12 @@ class ToolDataTableManager(Dictifiable):
            Galaxy instance.  In this case, we have 2 entry types to handle, files whose root tag is <tables>, for example:
         """
         table_elems = []
+        config_filenames: List[Union[str, os.PathLike]]
         if not isinstance(config_filename, list):
-            config_filename = [config_filename]
-        for filename in config_filename:
+            config_filenames = [config_filename]
+        else:
+            config_filenames = config_filename
+        for filename in config_filenames:
             tree = util.parse_xml(filename)
             root = tree.getroot()
             for table_elem in root.findall("table"):
@@ -283,8 +316,9 @@ class ToolDataTableManager(Dictifiable):
         return list(table_names)
 
 
-class ToolDataTable:
+class ToolDataTable(Dictifiable):
     type_key: str
+    data: List
 
     @classmethod
     def from_elem(
@@ -395,7 +429,7 @@ class ToolDataTable:
         return self._update_version()
 
 
-class TabularToolDataTable(ToolDataTable, Dictifiable):
+class TabularToolDataTable(ToolDataTable):
     """
     Data stored in a tabular / separated value format on disk, allows multiple
     files to be merged but all must have the same column definitions:
@@ -520,7 +554,7 @@ class TabularToolDataTable(ToolDataTable, Dictifiable):
                         filename = f"{corrected_filename}.sample"
                         found = True
 
-            errors = []
+            errors: List[str] = []
             if found:
                 self.extend_data_with(filename, errors=errors)
                 self._update_version()
@@ -647,7 +681,7 @@ class TabularToolDataTable(ToolDataTable, Dictifiable):
         if not self.allow_duplicate_entries:
             self._deduplicate_data()
 
-    def parse_file_fields(self, filename, errors=None, here="__HERE__"):
+    def parse_file_fields(self, filename, errors: Optional[List[str]] = None, here="__HERE__"):
         """
         Parse separated lines from file and return a list of tuples.
 
@@ -783,10 +817,11 @@ class TabularToolDataTable(ToolDataTable, Dictifiable):
             if filename is None:
                 # If we reach this point, there is no data table with a corresponding .loc file.
                 raise MessageException(
-                    f"Unable to determine filename for persisting data table '{self.name}' values: '{self.fields}'."
+                    f"Unable to determine filename for persisting data table '{self.name}' values: '{fields}'."
                 )
             else:
                 log.debug("Persisting changes to file: %s", filename)
+                data_table_fh: BinaryIO
                 with FileLock(filename):
                     try:
                         if os.path.exists(filename):
@@ -802,8 +837,8 @@ class TabularToolDataTable(ToolDataTable, Dictifiable):
                     except OSError as e:
                         log.exception("Error opening data table file (%s): %s", filename, e)
                         raise
-                fields = f"{self.separator.join(fields)}\n"
-                data_table_fh.write(fields.encode("utf-8"))
+                fields_collapsed = f"{self.separator.join(fields)}\n"
+                data_table_fh.write(fields_collapsed.encode("utf-8"))
 
     def _remove_entry(self, values):
 
@@ -1086,4 +1121,5 @@ def expand_here_template(content, here=None):
 
 
 # Registry of tool data types by type_key
-tool_data_table_types = {cls.type_key: cls for cls in [TabularToolDataTable, RefgenieToolDataTable]}
+tool_data_table_types_list: List[Type[ToolDataTable]] = [TabularToolDataTable, RefgenieToolDataTable]
+tool_data_table_types = {cls.type_key: cls for cls in tool_data_table_types_list}

lib/galaxy/tools/data/_schema.py

@@ -1,6 +1,7 @@
 from typing import (
     Dict,
     List,
+    Optional,
 )
 
 from pydantic import Field
@@ -28,6 +29,12 @@ class ToolDataEntryList(Model):
         title="A list with details on individual data tables.",
     )
 
+    def find_entry(self, name: str) -> Optional[ToolDataEntry]:
+        for entry in self.__root__:
+            if entry.name == name:
+                return entry
+        return None
+
 
 class ToolDataDetails(ToolDataEntry):
     columns: List[str] = Field(

lib/galaxy/util/__init__.py

@@ -31,7 +31,6 @@ from email.mime.multipart import MIMEMultipart
 from email.mime.text import MIMEText
 from hashlib import md5
 from os.path import relpath
-from pathlib import Path
 from typing import (
     Any,
     Optional,
@@ -284,7 +283,7 @@ def unique_id(KEY_SIZE=128):
     return md5(random_bits).hexdigest()
 
 
-def parse_xml(fname: typing.Union[str, Path], strip_whitespace=True, remove_comments=True):
+def parse_xml(fname: typing.Union[str, os.PathLike], strip_whitespace=True, remove_comments=True):
     """Returns a parsed xml tree"""
     parser = None
     if remove_comments and LXML_AVAILABLE:
@@ -1018,7 +1017,7 @@ def asbool(obj):
     return bool(obj)
 
 
-def string_as_bool(string: typing.Any) -> bool:
+def string_as_bool(string: Any) -> bool:
     if str(string).lower() in ("true", "yes", "on", "1"):
         return True
     else:
@@ -1044,7 +1043,7 @@ def string_as_bool_or_none(string):
         return False
 
 
-def listify(item, do_strip=False) -> typing.List[typing.Any]:
+def listify(item, do_strip=False) -> typing.List[Any]:
     """
     Make a single item a single item list.
 

test/unit/app/tools/test_tool_data.py

@@ -0,0 +1,163 @@
+import pytest
+
+from galaxy.tools.data import ToolDataTableManager
+
+LOC_ALPHA_CONTENTS = """
+data1	data1name	${__HERE__}/data1/entry.txt
+data2	data2name	${__HERE__}/data2/entry.txt
+"""
+
+
+LOC_ALPHA_CONTENTS_V2 = """
+data1	data1name	${__HERE__}/data1/entry.txt
+data2	data2name	${__HERE__}/data2/entry.txt
+data3	data3name	${__HERE__}/data3/entry.txt
+"""
+
+
+LOC_BETA_CONTENTS_1 = """
+beta1	beta1name	${__HERE__}/beta1/entry.txt
+"""
+
+
+LOC_BETA_CONTENTS_2 = """
+beta2	beta2name	${__HERE__}/beta2/entry.txt
+"""
+
+
+TOOL_DATA_TABLE_CONF_XML = """<tables>
+  <table name="testalpha" comment_char="#">
+    <columns>value, name, path</columns>
+    <file path="${__HERE__}/testalpha.loc" />
+  </table>
+</tables>
+"""
+
+
+MERGED_TOOL_DATA_TABLE_CONF_XML_1 = """<tables>
+  <table name="testbeta" comment_char="#">
+    <columns>value, name, path</columns>
+    <file path="${__HERE__}/testbeta1.loc" />
+  </table>
+</tables>
+"""
+
+
+MERGED_TOOL_DATA_TABLE_CONF_XML_2 = """<tables>
+  <table name="testbeta" comment_char="#">
+    <columns>value, name, path</columns>
+    <file path="${__HERE__}/testbeta2.loc" />
+  </table>
+</tables>
+"""
+
+
+@pytest.fixture
+def tdt_manager(tmp_path) -> ToolDataTableManager:
+    _write_loc_files(tmp_path)
+    conf = tmp_path / "tool_data_table_conf.xml"
+    conf.write_text(TOOL_DATA_TABLE_CONF_XML)
+    return ToolDataTableManager(tmp_path, conf)
+
+
+@pytest.fixture
+def merged_tdt_manager(tmp_path) -> ToolDataTableManager:
+    _write_loc_files(tmp_path)
+    conf1 = tmp_path / "tool_data_table_conf_1.xml"
+    conf1.write_text(MERGED_TOOL_DATA_TABLE_CONF_XML_1)
+    conf2 = tmp_path / "tool_data_table_conf_2.xml"
+    conf2.write_text(MERGED_TOOL_DATA_TABLE_CONF_XML_2)
+    return ToolDataTableManager(tmp_path, f"{conf1},{conf2}")
+
+
+def _write_loc_files(tmp_path):
+    loc1 = tmp_path / "testalpha.loc"
+    loc1.write_text(LOC_ALPHA_CONTENTS)
+
+    loc2 = tmp_path / "testbeta1.loc"
+    loc2.write_text(LOC_BETA_CONTENTS_1)
+
+    loc3 = tmp_path / "testbeta2.loc"
+    loc3.write_text(LOC_BETA_CONTENTS_2)
+
+    data1 = tmp_path / "data1"
+    data1.mkdir()
+    data1_entry = data1 / "entry.txt"
+    data1_entry.write_text("This is data 1.")
+
+    data2 = tmp_path / "data2"
+    data2.mkdir()
+    data2_entry = data2 / "entry.txt"
+    data2_entry.write_text("This is data 2.")
+
+    data3 = tmp_path / "data3"
+    data3.mkdir()
+    data3_entry = data3 / "entry.txt"
+    data3_entry.write_text("This is data 3.")
+
+    beta1 = tmp_path / "beta1"
+    beta1.mkdir()
+    beta1_entry = beta1 / "entry.txt"
+    beta1_entry.write_text("This is beta 1.")
+
+    beta2 = tmp_path / "beta2"
+    beta2.mkdir()
+    beta2_entry = beta2 / "entry.txt"
+    beta2_entry.write_text("This is beta 2.")
+
+
+def test_data_tables_as_dictionary(tdt_manager):
+    assert "testalpha" in tdt_manager.data_tables
+    assert "testdelta" not in tdt_manager.data_tables
+
+
+def test_to_dict(tdt_manager):
+    as_dict = tdt_manager.to_dict()
+    assert "testalpha" in as_dict
+    assert "testdelta" not in as_dict
+    testalpha_as_dict = as_dict["testalpha"]
+    assert "columns" in testalpha_as_dict
+
+
+def test_index(tdt_manager):
+    index = tdt_manager.index()
+    assert len(index.__root__) >= 1
+    entry = index.find_entry("testalpha")
+    assert entry
+    entry = index.find_entry("testomega")
+    assert not entry
+
+
+def test_reload(tdt_manager, tmp_path):
+    assert len(tdt_manager["testalpha"].data) == 2
+    loc1 = tmp_path / "testalpha.loc"
+    loc1.write_text(LOC_ALPHA_CONTENTS_V2)
+    tdt_manager.reload_tables()
+    assert len(tdt_manager["testalpha"].data) == 3
+
+
+def test_reload_by_path(tdt_manager, tmp_path):
+    assert len(tdt_manager["testalpha"].data) == 2
+    loc1 = tmp_path / "testalpha.loc"
+    loc1.write_text(LOC_ALPHA_CONTENTS_V2)
+    tdt_manager.reload_tables(path=str(loc1))
+    assert len(tdt_manager["testalpha"].data) == 3
+
+
+def test_reload_by_name(tdt_manager, tmp_path):
+    assert len(tdt_manager["testalpha"].data) == 2
+    loc1 = tmp_path / "testalpha.loc"
+    loc1.write_text(LOC_ALPHA_CONTENTS_V2)
+    tdt_manager.reload_tables("testalpha")
+    assert len(tdt_manager["testalpha"].data) == 3
+
+
+def test_merging_tables(merged_tdt_manager):
+    assert len(merged_tdt_manager["testbeta"].data) == 2
+
+
+def test_to_json(merged_tdt_manager, tmp_path):
+    json_path = tmp_path / "as_json.json"
+    assert not json_path.exists()
+    merged_tdt_manager.to_json(json_path)
+    assert json_path.exists()