From 94a027ca9c5ada27d00d95b8a38fcab0c39153d4 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 May 2026 17:01:08 -0400 Subject: [PATCH 01/13] Add shared data table consistency check Adds DataTableColumnMismatch and DataTableFileConflict, raised by ToolDataTableManager.assert_data_table_consistency. Wired into load_from_config_file so the invariant also fires at server startup. Refs galaxyproject/galaxy#21448. --- lib/galaxy/tool_util/data/__init__.py | 128 +++++++++++++++++---- test/unit/tool_util/data/test_tool_data.py | 76 ++++++++++++ 2 files changed, 184 insertions(+), 20 deletions(-) diff --git a/lib/galaxy/tool_util/data/__init__.py b/lib/galaxy/tool_util/data/__init__.py index 344f05d504b..0b1be8651e3 100644 --- a/lib/galaxy/tool_util/data/__init__.py +++ b/lib/galaxy/tool_util/data/__init__.py @@ -23,6 +23,7 @@ from typing import ( BinaryIO, Callable, Dict, + Iterable, List, Optional, overload, @@ -140,6 +141,41 @@ class FileNameInfoT(TypedDict): LoadInfoT = Tuple[Tuple[Element, Optional[StrPath]], Dict[str, Any]] +class DataTableColumnMismatch(Exception): + """Two data tables share a name but declare different columns.""" + + def __init__(self, table_name: str, existing_columns: Dict[str, int], incoming_columns: Dict[str, int]): + self.table_name = table_name + self.existing_columns = existing_columns + self.incoming_columns = incoming_columns + super().__init__( + f"Data table {table_name!r} is already registered with columns {existing_columns}, " + f"refusing to register conflicting columns {incoming_columns}." + ) + + +class DataTableFileConflict(Exception): + """Two data tables with different names reference the same loc file.""" + + def __init__( + self, + path: str, + candidate_name: str, + candidate_columns: Dict[str, int], + existing_name: str, + existing_columns: Dict[str, int], + ): + self.path = path + self.candidate_name = candidate_name + self.candidate_columns = candidate_columns + self.existing_name = existing_name + self.existing_columns = existing_columns + super().__init__( + f"Data table {candidate_name!r} declares loc file {path!r}, but that file is already " + f"registered to data table {existing_name!r}." + ) + + class ToolDataTable(Dictifiable): type_key: str data: List[List[str]] @@ -516,6 +552,38 @@ class TabularToolDataTable(ToolDataTable): def get_version_fields(self): return (self._loaded_content_version, self.get_fields()) + @staticmethod + def parse_column_spec_element( + config_element: Element, + ) -> Tuple[Dict[str, int], int, Dict[str, str]]: + """ + Parse column definitions into ``(columns, largest_index, empty_field_values)``. + Does not mutate or assert — callers layer their own validation. + """ + columns: Dict[str, int] = {} + empty_field_values: Dict[str, str] = {} + largest_index = 0 + columns_elem = config_element.find("columns") + if columns_elem is not None: + column_names = util.xml_text(columns_elem) + for index, name in enumerate(n.strip() for n in column_names.split(",")): + columns[name] = index + largest_index = index + else: + for column_elem in config_element.findall("column"): + name = column_elem.get("name") + index_attr = column_elem.get("index") + if name is None or index_attr is None: + continue + index = int(index_attr) + columns[name] = index + if index > largest_index: + largest_index = index + empty_field_value = column_elem.get("empty_field_value", None) + if empty_field_value is not None: + empty_field_values[name] = empty_field_value + return columns, largest_index, empty_field_values + def parse_column_spec(self, config_element: Element) -> None: """ Parse column definitions, which can either be a set of 'column' elements @@ -525,27 +593,12 @@ class TabularToolDataTable(ToolDataTable): A column named 'value' is required. """ - self.columns: Dict[str, int] = {} - if config_element.find("columns") is not None: - column_names = util.xml_text(config_element.find("columns")) - column_names = [n.strip() for n in column_names.split(",")] - for index, name in enumerate(column_names): - self.columns[name] = index - self.largest_index = index - else: - self.largest_index = 0 + if config_element.find("columns") is None: for column_elem in config_element.findall("column"): - name = column_elem.get("name") - assert name is not None, "Required 'name' attribute missing from column def" - index_attr = column_elem.get("index") - assert index_attr is not None, "Required 'index' attribute missing from column def" - index = int(index_attr) - self.columns[name] = index - if index > self.largest_index: - self.largest_index = index - empty_field_value = column_elem.get("empty_field_value", None) - if empty_field_value is not None: - self.empty_field_values[name] = empty_field_value + assert column_elem.get("name") is not None, "Required 'name' attribute missing from column def" + assert column_elem.get("index") is not None, "Required 'index' attribute missing from column def" + self.columns, self.largest_index, parsed_empty_field_values = self.parse_column_spec_element(config_element) + self.empty_field_values.update(parsed_empty_field_values) assert "value" in self.columns, "Required 'value' column missing from column def" if "name" not in self.columns: self.columns["name"] = self.columns["value"] @@ -965,6 +1018,36 @@ class ToolDataTableManager(Dictifiable): def get_tables(self) -> Dict[str, "ToolDataTable"]: return self.data_tables + def assert_data_table_consistency( + self, + candidate_name: str, + candidate_columns: Dict[str, int], + candidate_file_paths: Iterable[str], + ) -> None: + """ + Raise if registering ``candidate_name`` would conflict with current state: + an existing table with the same name but different columns, or any + ``candidate_file_paths`` already owned by a different table name. + """ + existing = self.data_tables.get(candidate_name) + if existing is not None: + existing_columns = getattr(existing, "columns", None) + if existing_columns is not None and existing_columns != candidate_columns: + raise DataTableColumnMismatch(candidate_name, existing_columns, candidate_columns) + candidate_realpaths = {os.path.realpath(p) for p in candidate_file_paths if p} + if not candidate_realpaths: + return + for other_name, other_table in self.data_tables.items(): + if other_name == candidate_name: + continue + other_filenames = getattr(other_table, "filenames", None) or {} + for other_path in other_filenames: + if os.path.realpath(other_path) in candidate_realpaths: + other_columns = getattr(other_table, "columns", None) or {} + raise DataTableFileConflict( + other_path, candidate_name, candidate_columns, other_name, other_columns + ) + def to_dict( self, view: str = "collection", value_mapper: Optional[Dict[str, Callable]] = None ) -> Dict[str, Dict[str, Any]]: @@ -1002,6 +1085,11 @@ class ToolDataTableManager(Dictifiable): other_config_dict=self.other_config_dict, ) table_elems.append(table_elem) + self.assert_data_table_consistency( + table.name, + getattr(table, "columns", {}) or {}, + getattr(table, "filenames", {}) or {}, + ) if table.name not in self.data_tables: self.data_tables[table.name] = table log.debug("Loaded tool data table '%s' from file '%s'", table.name, config_filename) diff --git a/test/unit/tool_util/data/test_tool_data.py b/test/unit/tool_util/data/test_tool_data.py index 3ed525fe37f..e2d8fc16261 100644 --- a/test/unit/tool_util/data/test_tool_data.py +++ b/test/unit/tool_util/data/test_tool_data.py @@ -1,9 +1,32 @@ +import pytest + +from galaxy.tool_util.data import ( + DataTableColumnMismatch, + DataTableFileConflict, +) + LOC_ALPHA_CONTENTS_V2 = """ data1 data1name ${__HERE__}/data1/entry.txt data2 data2name ${__HERE__}/data2/entry.txt data3 data3name ${__HERE__}/data3/entry.txt """ +CONFLICTING_TABLE_CONF_XML = """ + + value, name, path + +
+ +""" + +COLUMN_DIVERGENT_TABLE_CONF_XML = """ + + value, name, path, extra + +
+ +""" + def test_data_tables_as_dictionary(tdt_manager): assert "testalpha" in tdt_manager.data_tables @@ -60,3 +83,56 @@ def test_to_json(merged_tdt_manager, tmp_path): assert not json_path.exists() merged_tdt_manager.to_json(json_path) assert json_path.exists() + + +def test_assert_data_table_consistency_accepts_new_table(tdt_manager, tmp_path): + tdt_manager.assert_data_table_consistency( + "brand_new_table", + {"value": 0, "name": 1, "path": 2}, + [str(tmp_path / "brand_new_table.loc")], + ) + + +def test_assert_data_table_consistency_accepts_matching_redefinition(tdt_manager, tmp_path): + existing = tdt_manager["testalpha"] + tdt_manager.assert_data_table_consistency( + "testalpha", + existing.columns, + list(existing.filenames), + ) + + +def test_assert_data_table_consistency_raises_column_mismatch(tdt_manager, tmp_path): + with pytest.raises(DataTableColumnMismatch) as exc_info: + tdt_manager.assert_data_table_consistency( + "testalpha", + {"value": 0, "name": 1, "path": 2, "extra": 3}, + [str(tmp_path / "testalpha.loc")], + ) + assert exc_info.value.table_name == "testalpha" + + +def test_assert_data_table_consistency_raises_file_conflict(tdt_manager, tmp_path): + shared_loc = str(tmp_path / "testalpha.loc") + with pytest.raises(DataTableFileConflict) as exc_info: + tdt_manager.assert_data_table_consistency( + "other_table", + {"value": 0, "name": 1, "path": 2}, + [shared_loc], + ) + assert exc_info.value.candidate_name == "other_table" + assert exc_info.value.existing_name == "testalpha" + + +def test_load_from_config_file_raises_on_file_path_conflict(tdt_manager, tmp_path): + conflicting_conf = tmp_path / "conflict.xml" + conflicting_conf.write_text(CONFLICTING_TABLE_CONF_XML.format(loc_path=str(tmp_path / "testalpha.loc"))) + with pytest.raises(DataTableFileConflict): + tdt_manager.load_from_config_file(str(conflicting_conf), str(tmp_path), from_shed_config=True) + + +def test_load_from_config_file_raises_on_column_mismatch_same_name(tdt_manager, tmp_path): + column_conflict_conf = tmp_path / "column_conflict.xml" + column_conflict_conf.write_text(COLUMN_DIVERGENT_TABLE_CONF_XML.format(loc_path=str(tmp_path / "testalpha.loc"))) + with pytest.raises(DataTableColumnMismatch): + tdt_manager.load_from_config_file(str(column_conflict_conf), str(tmp_path), from_shed_config=True) From cf33b2a5f077fb6572b5f7b92516ce78c74bdb97 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 May 2026 17:01:14 -0400 Subject: [PATCH 02/13] Skip data table registration for non-Data-Manager shed repos Gate install_tool_data_tables and handle_missing_data_table_entry on the repository being a Data Manager. Non-DM repos no longer mutate shed_tool_data_table_conf.xml with .loc.sample contents. Refs galaxyproject/galaxy#21448. --- .../galaxy_install/install_manager.py | 12 +- test/unit/tool_shed/test_install_manager.py | 109 ++++++++++++++++++ 2 files changed, 116 insertions(+), 5 deletions(-) create mode 100644 test/unit/tool_shed/test_install_manager.py diff --git a/lib/galaxy/tool_shed/galaxy_install/install_manager.py b/lib/galaxy/tool_shed/galaxy_install/install_manager.py index 929adf13b37..12a284cf045 100644 --- a/lib/galaxy/tool_shed/galaxy_install/install_manager.py +++ b/lib/galaxy/tool_shed/galaxy_install/install_manager.py @@ -170,7 +170,8 @@ class InstallRepositoryManager: session.add(tool_shed_repository) session.commit() - if "sample_files" in irmm_metadata_dict: + is_data_manager = "data_manager" in irmm_metadata_dict + if is_data_manager and "sample_files" in irmm_metadata_dict: sample_files = irmm_metadata_dict.get("sample_files", []) tool_index_sample_files = stdtm.get_tool_index_sample_files(sample_files) tool_data_table_conf_filename, tool_data_table_elems = stdtm.install_tool_data_tables( @@ -191,10 +192,11 @@ class InstallRepositoryManager: sample_files_copied = [str(s) for s in tool_index_sample_files] repository_tools_tups = irmm.get_repository_tools_tups() if repository_tools_tups: - # Handle missing data table entries for tool parameters that are dynamically generated select lists. - repository_tools_tups = stdtm.handle_missing_data_table_entry( - relative_install_dir, tool_path, repository_tools_tups - ) + if is_data_manager: + # Only Data Manager repos register data tables on install. + repository_tools_tups = stdtm.handle_missing_data_table_entry( + relative_install_dir, tool_path, repository_tools_tups + ) # Handle missing index files for tool parameters that are dynamically generated select lists. repository_tools_tups, sample_files_copied = tool_util.handle_missing_index_file( self.app, tool_path, sample_files, repository_tools_tups, sample_files_copied diff --git a/test/unit/tool_shed/test_install_manager.py b/test/unit/tool_shed/test_install_manager.py new file mode 100644 index 00000000000..e3e0b2e970c --- /dev/null +++ b/test/unit/tool_shed/test_install_manager.py @@ -0,0 +1,109 @@ +"""Unit tests for install-time data table registration gating.""" + +from typing import ( + Any, +) +from unittest import mock + +from galaxy.tool_shed.galaxy_install import install_manager + +INSTALL_MANAGER_MODULE = "galaxy.tool_shed.galaxy_install.install_manager" + + +def _make_install_repository_manager(): + app = mock.MagicMock(name="app") + app.install_model.context = mock.MagicMock(name="session") + register_mock = mock.MagicMock(name="add_new_entries_from_config_file") + app.tool_data_tables.add_new_entries_from_config_file = register_mock + irm = install_manager.InstallRepositoryManager.__new__(install_manager.InstallRepositoryManager) + irm.app = app + irm.install_model = app.install_model + irm.tpm = mock.MagicMock(name="tpm") + return irm, register_mock + + +def _invoke_handle(irm, metadata_dict: dict[str, Any], repository_tools_tups: list[Any]): + irmm_instance = mock.MagicMock(name="irmm_instance") + irmm_instance.get_metadata_dict.return_value = metadata_dict + irmm_instance.get_repository_tools_tups.return_value = repository_tools_tups + + stdtm_instance = mock.MagicMock(name="stdtm_instance") + stdtm_instance.get_tool_index_sample_files.return_value = [] + stdtm_instance.install_tool_data_tables.return_value = ( + "/dev/null/tool_data_table_conf.xml", + [object()], + ) + stdtm_instance.handle_missing_data_table_entry.side_effect = lambda *_a, **_k: repository_tools_tups + + patches = [ + mock.patch(f"{INSTALL_MANAGER_MODULE}.InstalledRepositoryMetadataManager", return_value=irmm_instance), + mock.patch(f"{INSTALL_MANAGER_MODULE}.ShedToolDataTableManager", return_value=stdtm_instance), + mock.patch( + f"{INSTALL_MANAGER_MODULE}.repository_util.get_tool_shed_status_for_installed_repository", return_value=None + ), + mock.patch(f"{INSTALL_MANAGER_MODULE}.tool_util.copy_sample_files"), + mock.patch( + f"{INSTALL_MANAGER_MODULE}.tool_util.handle_missing_index_file", + side_effect=lambda *_a, **_k: (repository_tools_tups, []), + ), + mock.patch(f"{INSTALL_MANAGER_MODULE}.data_manager.DataManagerHandler"), + ] + for p in patches: + p.start() + try: + repo = mock.MagicMock(name="tool_shed_repository") + repo.changeset_revision = "abc" + repo.installed_changeset_revision = "abc" + irm._InstallRepositoryManager__handle_repository_contents( + tool_shed_repository=repo, + tool_path="/tmp/tool_path", + repository_clone_url="http://tool-shed/repos/owner/name", + relative_install_dir="owner/name/abc", + tool_shed="tool-shed", + tool_section=None, + shed_tool_conf=None, + ) + finally: + for p in patches: + p.stop() + return stdtm_instance + + +def test_non_data_manager_repo_skips_sample_files_registration(): + irm, register_mock = _make_install_repository_manager() + stdtm_instance = _invoke_handle(irm, {"sample_files": ["foo.loc.sample"]}, []) + stdtm_instance.install_tool_data_tables.assert_not_called() + register_mock.assert_not_called() + + +def test_data_manager_repo_registers_sample_files(): + irm, register_mock = _make_install_repository_manager() + metadata = { + "sample_files": ["foo.loc.sample"], + "tools": [{"id": "t"}], + "data_manager": {"data_managers": {}}, + } + fake_tup = (mock.MagicMock(), "guid", mock.MagicMock()) + stdtm_instance = _invoke_handle(irm, metadata, [fake_tup]) + stdtm_instance.install_tool_data_tables.assert_called_once() + register_mock.assert_called_once() + + +def test_non_data_manager_repo_skips_handle_missing_data_table_entry(): + irm, _ = _make_install_repository_manager() + metadata = {"tools": [{"id": "t"}], "sample_files": []} + fake_tup = (mock.MagicMock(), "guid", mock.MagicMock()) + stdtm_instance = _invoke_handle(irm, metadata, [fake_tup]) + stdtm_instance.handle_missing_data_table_entry.assert_not_called() + + +def test_data_manager_repo_invokes_handle_missing_data_table_entry(): + irm, _ = _make_install_repository_manager() + metadata = { + "tools": [{"id": "t"}], + "sample_files": [], + "data_manager": {"data_managers": {}}, + } + fake_tup = (mock.MagicMock(), "guid", mock.MagicMock()) + stdtm_instance = _invoke_handle(irm, metadata, [fake_tup]) + stdtm_instance.handle_missing_data_table_entry.assert_called_once() From 90c8058ec6213d5fd43062d711f23a757c5fec2a Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 May 2026 17:01:14 -0400 Subject: [PATCH 03/13] Share Data Manager loc files at tool_data_path root Copy DM .loc files to tool_data_path/ instead of a per-revision subdir, and rewrite to match. Run assert_data_table_consistency before persisting so column or file conflicts fail the install. Refs galaxyproject/galaxy#21448, fixes galaxyproject/galaxy#20151. --- .../tool_shed/tools/data_table_manager.py | 68 +++++-- .../unit/tool_shed/test_data_table_manager.py | 191 ++++++++++++++++++ 2 files changed, 245 insertions(+), 14 deletions(-) create mode 100644 test/unit/tool_shed/test_data_table_manager.py diff --git a/lib/galaxy/tool_shed/tools/data_table_manager.py b/lib/galaxy/tool_shed/tools/data_table_manager.py index 61fbaafd455..588066571dd 100644 --- a/lib/galaxy/tool_shed/tools/data_table_manager.py +++ b/lib/galaxy/tool_shed/tools/data_table_manager.py @@ -8,6 +8,10 @@ from typing import ( from galaxy.tool_shed.galaxy_install.client import InstallationTarget from galaxy.tool_shed.util import hg_util +from galaxy.tool_util.data import ( + DataTableColumnMismatch, + DataTableFileConflict, +) from galaxy.util import ( Element, SubElement, @@ -25,6 +29,16 @@ log = logging.getLogger(__name__) RequiredAppT = Union["BasicSharedApp", InstallationTarget] +def _parse_table_columns(table_elem: Element) -> dict[str, int]: + """Parse a ```` element's column spec into a name->index mapping.""" + from galaxy.tool_util.data import TabularToolDataTable + + columns, _, _ = TabularToolDataTable.parse_column_spec_element(table_elem) + if "value" in columns and "name" not in columns: + columns["name"] = columns["value"] + return columns + + class BaseShedToolDataTableManager: def __init__(self, app: RequiredAppT): self.app = app @@ -151,28 +165,36 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): TOOL_DATA_TABLE_FILE_SAMPLE_NAME = f"{TOOL_DATA_TABLE_FILE_NAME}.sample" SAMPLE_SUFFIX = ".sample" SAMPLE_SUFFIX_OFFSET = -len(SAMPLE_SUFFIX) + LOC_SAMPLE_SUFFIX = ".loc.sample" target_dir, tool_path, relative_target_dir = self.get_target_install_dir(tool_shed_repository) + # .loc files are shared across DM revisions: install at the root of tool_data_path. + shared_loc_dir = self.app.config.tool_data_path for sample_file in tool_index_sample_files: path, filename = os.path.split(sample_file) target_filename = filename if target_filename.endswith(SAMPLE_SUFFIX): target_filename = target_filename[:SAMPLE_SUFFIX_OFFSET] source_file = os.path.join(tool_path, sample_file) + if filename.endswith(LOC_SAMPLE_SUFFIX): + target_path_filename = os.path.join(shared_loc_dir, target_filename) + install_dest_dir = shared_loc_dir + else: + target_path_filename = os.path.join(target_dir, target_filename) + install_dest_dir = target_dir # We're not currently uninstalling index files, do not overwrite existing files. - target_path_filename = os.path.join(target_dir, target_filename) if not os.path.exists(target_path_filename) or target_filename == TOOL_DATA_TABLE_FILE_NAME: shutil.copy2(source_file, target_path_filename) else: log.debug( "Did not copy sample file '%s' to install directory '%s' because file already exists.", filename, - target_dir, + install_dest_dir, ) # For provenance and to simplify introspection, let's keep the original data table sample file around. if filename == TOOL_DATA_TABLE_FILE_SAMPLE_NAME: shutil.copy2(source_file, os.path.join(target_dir, filename)) tool_data_table_conf_filename = os.path.join(target_dir, TOOL_DATA_TABLE_FILE_NAME) - elems = [] + elems: list = [] if os.path.exists(tool_data_table_conf_filename): tree, error_message = xml_util.parse_xml(tool_data_table_conf_filename) if tree: @@ -191,20 +213,36 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): tool_data_table_conf_filename, TOOL_DATA_TABLE_FILE_SAMPLE_NAME, ) + registered_tables = self.app.tool_data_tables.data_tables + kept_elems: list = [] for elem in elems: - if elem.tag == "table": - for file_elem in elem.findall("file"): - path = file_elem.get("path", None) - if path: - file_elem.set("path", os.path.normpath(os.path.join(target_dir, os.path.split(path)[1]))) - # Store repository info in the table tag set for trace-ability. - self.generate_repository_info_elem_from_repository(tool_shed_repository, parent_elem=elem) - if elems: + if elem.tag != "table": + kept_elems.append(elem) + continue + candidate_file_paths: list[str] = [] + for file_elem in elem.findall("file"): + path = file_elem.get("path", None) + if path: + new_path = os.path.normpath(os.path.join(shared_loc_dir, os.path.split(path)[1])) + file_elem.set("path", new_path) + candidate_file_paths.append(new_path) + table_name = elem.get("name") or "" + incoming_columns = _parse_table_columns(elem) + self.app.tool_data_tables.assert_data_table_consistency(table_name, incoming_columns, candidate_file_paths) + existing = registered_tables.get(table_name) if table_name else None + if existing is not None and getattr(existing, "columns", None) is not None: + # Already registered with matching columns; skip to avoid duplicate
entry. + continue + # Store repository info in the table tag set for trace-ability. + self.generate_repository_info_elem_from_repository(tool_shed_repository, parent_elem=elem) + kept_elems.append(elem) + if kept_elems: # Remove old data_table - os.unlink(tool_data_table_conf_filename) + if os.path.exists(tool_data_table_conf_filename): + os.unlink(tool_data_table_conf_filename) # Persist new data_table content. - self.app.tool_data_tables.to_xml_file(tool_data_table_conf_filename, elems) - return tool_data_table_conf_filename, elems + self.app.tool_data_tables.to_xml_file(tool_data_table_conf_filename, kept_elems) + return tool_data_table_conf_filename, kept_elems # For backwards compatibility with exisiting data managers @@ -212,6 +250,8 @@ ToolDataTableManager = ShedToolDataTableManager __all__ = ( + "DataTableColumnMismatch", + "DataTableFileConflict", "ToolDataTableManager", "ShedToolDataTableManager", ) diff --git a/test/unit/tool_shed/test_data_table_manager.py b/test/unit/tool_shed/test_data_table_manager.py new file mode 100644 index 00000000000..940d99cd7f7 --- /dev/null +++ b/test/unit/tool_shed/test_data_table_manager.py @@ -0,0 +1,191 @@ +"""Unit tests for shed-side data table install behavior.""" + +import os +from unittest import mock + +import pytest + +from galaxy.tool_shed.tools.data_table_manager import ( + DataTableColumnMismatch, + DataTableFileConflict, + ShedToolDataTableManager, +) +from galaxy.tool_util.data import ToolDataTableManager +from galaxy.util import ( + Element, + SubElement, +) + + +class _FakeTableRegistry(ToolDataTableManager): + def __init__(self): + self.data_tables: dict = {} + self.to_xml_calls: list = [] + + def to_xml_file(self, shed_tool_data_table_config, new_elems=None, remove_elems=None): + self.to_xml_calls.append((shed_tool_data_table_config, new_elems)) + with open(shed_tool_data_table_config, "wb") as fh: + fh.write(b"") + + +SAMPLE_TABLE_CONF = """\ + +
+ value, dbkey, name, path + +
+ +""" + +LOC_SAMPLE_CONTENT = "# all_fasta.loc sample\n" + + +def _write(path: str, contents: str) -> None: + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as fh: + fh.write(contents) + + +def _make_stdtm(tmp_path): + repo_dir = str(tmp_path / "repo") + tool_data_path = str(tmp_path / "tool-data") + shed_tool_data_path = str(tmp_path / "shed_tool_data") + relative_target_dir = "owner/name/abc" + + os.makedirs(tool_data_path) + os.makedirs(os.path.join(shed_tool_data_path, relative_target_dir)) + _write(os.path.join(repo_dir, "tool_data_table_conf.xml.sample"), SAMPLE_TABLE_CONF) + _write(os.path.join(repo_dir, "tool-data", "all_fasta.loc.sample"), LOC_SAMPLE_CONTENT) + + app = mock.MagicMock(name="app") + app.config.tool_data_path = tool_data_path + app.config.shed_tool_data_path = shed_tool_data_path + registry = _FakeTableRegistry() + app.tool_data_tables = registry + captured = {"to_xml_calls": registry.to_xml_calls} + + stdtm = ShedToolDataTableManager(app) + + repo = mock.MagicMock(name="tool_shed_repository") + repo.name = "data_manager_fetch_genome_dbkeys_all_fasta" + repo.owner = "iuc" + repo.installed_changeset_revision = "abc" + repo.tool_shed = "tool-shed" + repo.get_tool_relative_path.return_value = (repo_dir, relative_target_dir) + + sample_files = [ + "tool_data_table_conf.xml.sample", + os.path.join("tool-data", "all_fasta.loc.sample"), + ] + return stdtm, repo, sample_files, captured, tool_data_path, shed_tool_data_path, relative_target_dir + + +def _registered_table(columns, filenames=None): + existing = mock.MagicMock(spec=["columns", "filenames"]) + existing.columns = columns + existing.filenames = filenames or {} + return existing + + +def test_loc_file_lands_at_shared_root_not_per_revision(tmp_path): + stdtm, repo, samples, captured, tool_data_path, shed_tool_data_path, _ = _make_stdtm(tmp_path) + _, kept_elems = stdtm.install_tool_data_tables(repo, samples) + + shared_loc = os.path.join(tool_data_path, "all_fasta.loc") + assert os.path.exists(shared_loc) + per_rev_loc = os.path.join(shed_tool_data_path, "owner/name/abc", "all_fasta.loc") + assert not os.path.exists(per_rev_loc) + + assert len(kept_elems) == 1 + file_elems = list(kept_elems[0].findall("file")) + assert len(file_elems) == 1 + assert file_elems[0].get("path") == shared_loc + + +def test_existing_loc_file_is_not_overwritten(tmp_path): + stdtm, repo, samples, _, tool_data_path, _, _ = _make_stdtm(tmp_path) + shared_loc = os.path.join(tool_data_path, "all_fasta.loc") + _write(shared_loc, "preexisting DM-populated content\n") + + stdtm.install_tool_data_tables(repo, samples) + + with open(shared_loc) as fh: + assert fh.read() == "preexisting DM-populated content\n" + + +def test_column_mismatch_raises(tmp_path): + stdtm, repo, samples, captured, _, _, _ = _make_stdtm(tmp_path) + stdtm.app.tool_data_tables.data_tables = { + "all_fasta": _registered_table({"value": 0, "name": 1, "path": 2}), + } + + with pytest.raises(DataTableColumnMismatch) as exc_info: + stdtm.install_tool_data_tables(repo, samples) + assert exc_info.value.table_name == "all_fasta" + assert not captured["to_xml_calls"] + + +def test_column_match_dedupes_without_writing(tmp_path): + stdtm, repo, samples, captured, _, _, _ = _make_stdtm(tmp_path) + matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} + stdtm.app.tool_data_tables.data_tables = { + "all_fasta": _registered_table(matching_columns), + } + + _, kept_elems = stdtm.install_tool_data_tables(repo, samples) + + assert kept_elems == [] + assert not captured["to_xml_calls"] + + +def test_column_match_with_column_elements_dedupes(tmp_path): + stdtm, repo, _, captured, _, _, _ = _make_stdtm(tmp_path) + column_form_conf = """\ + + + + + + + +
+ +""" + repo_dir, _ = repo.get_tool_relative_path.return_value + _write(os.path.join(repo_dir, "tool_data_table_conf.xml.sample"), column_form_conf) + stdtm.app.tool_data_tables.data_tables = { + "all_fasta": _registered_table({"value": 0, "dbkey": 1, "name": 2, "path": 3}), + } + + _, kept_elems = stdtm.install_tool_data_tables( + repo, + ["tool_data_table_conf.xml.sample", os.path.join("tool-data", "all_fasta.loc.sample")], + ) + assert kept_elems == [] + + +def test_file_path_conflict_raises(tmp_path): + stdtm, repo, samples, captured, tool_data_path, _, _ = _make_stdtm(tmp_path) + shared_loc = os.path.join(tool_data_path, "all_fasta.loc") + stdtm.app.tool_data_tables.data_tables = { + "other_table": _registered_table( + {"value": 0, "name": 1}, + filenames={shared_loc: {"found": True}}, + ), + } + + with pytest.raises(DataTableFileConflict) as exc_info: + stdtm.install_tool_data_tables(repo, samples) + assert exc_info.value.candidate_name == "all_fasta" + assert exc_info.value.existing_name == "other_table" + assert not captured["to_xml_calls"] + + +def test_parse_table_columns_aliases_name_to_value(): + from galaxy.tool_shed.tools.data_table_manager import _parse_table_columns + + elem = Element("table") + cols = SubElement(elem, "columns") + cols.text = "value, path" + parsed = _parse_table_columns(elem) + assert parsed == {"value": 0, "path": 1, "name": 0} From 0618a7f7781695e339044c9f5185d130a75d5b12 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 May 2026 17:01:14 -0400 Subject: [PATCH 04/13] Update tool shed install tests for non-DM data table skip --- lib/tool_shed/test/base/testcase.py | 12 ++++++++++++ ...1010_install_repository_with_tool_dependencies.py | 6 ++++-- test/integration/test_repository_operations.py | 9 +++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/lib/tool_shed/test/base/testcase.py b/lib/tool_shed/test/base/testcase.py index f485e711fe1..b945f4db3b5 100644 --- a/lib/tool_shed/test/base/testcase.py +++ b/lib/tool_shed/test/base/testcase.py @@ -1655,6 +1655,18 @@ class ShedTestCase(ShedApiTestCase): assert self._installation_client self._installation_client.refresh_tool_shed_repository(repo) + def verify_no_installed_repository_data_table_entries(self, table_names): + """Assert that none of ``table_names`` appears in shed_tool_data_table_conf.xml.""" + shed_tool_data_table_conf = self.shed_tool_data_table_conf + if not os.path.exists(shed_tool_data_table_conf): + return + data_tables, error_message = xml_util.parse_xml(shed_tool_data_table_conf) + assert not error_message, f"Failed to parse {shed_tool_data_table_conf}: {error_message}" + assert data_tables is not None + registered_names = {t.get("name") for t in data_tables.findall("table")} + for name in table_names: + assert name not in registered_names, f"Unexpected data table entry '{name}' in {shed_tool_data_table_conf}" + def verify_installed_repository_data_table_entries(self, required_data_table_entries): # The value of the received required_data_table_entries will be something like: [ 'sam_fa_indexes' ] shed_tool_data_table_conf = self.shed_tool_data_table_conf diff --git a/lib/tool_shed/test/functional/test_1010_install_repository_with_tool_dependencies.py b/lib/tool_shed/test/functional/test_1010_install_repository_with_tool_dependencies.py index 4e66f2b8b2d..42dcde1637e 100644 --- a/lib/tool_shed/test/functional/test_1010_install_repository_with_tool_dependencies.py +++ b/lib/tool_shed/test/functional/test_1010_install_repository_with_tool_dependencies.py @@ -63,5 +63,7 @@ class TestToolWithToolDependencies(ShedTestCase): self.verify_installed_repository_metadata_unchanged(repository_name, common.test_user_1_name) def test_0025_verify_sample_files(self): - """Verify that the installed repository populated shed_tool_data_table.xml and the sample files.""" - self.verify_installed_repository_data_table_entries(required_data_table_entries=["sam_fa_indexes"]) + """Non-Data-Manager repositories no longer auto-register data tables on install + (galaxyproject/galaxy#21448); confirm sam_fa_indexes is absent from + shed_tool_data_table_conf.xml.""" + self.verify_no_installed_repository_data_table_entries(table_names=["sam_fa_indexes"]) diff --git a/test/integration/test_repository_operations.py b/test/integration/test_repository_operations.py index 3bdfbc039bb..33a0e5e9527 100644 --- a/test/integration/test_repository_operations.py +++ b/test/integration/test_repository_operations.py @@ -61,6 +61,15 @@ class TestRepositoryInstallIntegrationTestCase(integration_util.IntegrationTestC self.install_repository(*repo) self.uninstall_repository(*repo) + def test_non_data_manager_install_skips_data_table_registration(self): + """Non-Data-Manager repos must not register data tables on install.""" + non_dm_repo = ("devteam", "bwa", "051eba708f43") + non_dm_table_names = {"bwa_indexes", "bwa_mem_indexes"} + self.install_repository(*non_dm_repo) + registered = set(self._app.tool_data_tables.data_tables.keys()) + leaked = non_dm_table_names & registered + assert not leaked, f"Unexpected data tables registered by non-DM repo: {sorted(leaked)}" + def test_repository_update(self): response = self._install_repository(revision=REVISION_4, version="0.0.3", allow_upgraded=True)[0] assert int(response["ctx_rev"]) >= 4 From a5deee57aca184bbe36ce46ecb1db3f4a535e93e Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 13 May 2026 14:41:51 -0400 Subject: [PATCH 05/13] Drop DataTableFileConflict; two tables may share a loc file Galaxy ships picard_indexes and srma_indexes pointing at the same loc file, so the cross-table file-path check raised on every startup. Keep only the same-name column-mismatch protection and update the production-shed test to expect no per-revision tool_data_table_conf.xml for non-DM repos. --- .../tool_shed/tools/data_table_manager.py | 10 +--- lib/galaxy/tool_util/data/__init__.py | 48 +---------------- test/unit/app/test_galaxy_install.py | 3 +- .../unit/tool_shed/test_data_table_manager.py | 18 ------- test/unit/tool_util/data/test_tool_data.py | 53 ++----------------- 5 files changed, 10 insertions(+), 122 deletions(-) diff --git a/lib/galaxy/tool_shed/tools/data_table_manager.py b/lib/galaxy/tool_shed/tools/data_table_manager.py index 588066571dd..caeecea1dbe 100644 --- a/lib/galaxy/tool_shed/tools/data_table_manager.py +++ b/lib/galaxy/tool_shed/tools/data_table_manager.py @@ -8,10 +8,7 @@ from typing import ( from galaxy.tool_shed.galaxy_install.client import InstallationTarget from galaxy.tool_shed.util import hg_util -from galaxy.tool_util.data import ( - DataTableColumnMismatch, - DataTableFileConflict, -) +from galaxy.tool_util.data import DataTableColumnMismatch from galaxy.util import ( Element, SubElement, @@ -219,16 +216,14 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): if elem.tag != "table": kept_elems.append(elem) continue - candidate_file_paths: list[str] = [] for file_elem in elem.findall("file"): path = file_elem.get("path", None) if path: new_path = os.path.normpath(os.path.join(shared_loc_dir, os.path.split(path)[1])) file_elem.set("path", new_path) - candidate_file_paths.append(new_path) table_name = elem.get("name") or "" incoming_columns = _parse_table_columns(elem) - self.app.tool_data_tables.assert_data_table_consistency(table_name, incoming_columns, candidate_file_paths) + self.app.tool_data_tables.assert_data_table_consistency(table_name, incoming_columns) existing = registered_tables.get(table_name) if table_name else None if existing is not None and getattr(existing, "columns", None) is not None: # Already registered with matching columns; skip to avoid duplicate entry. @@ -251,7 +246,6 @@ ToolDataTableManager = ShedToolDataTableManager __all__ = ( "DataTableColumnMismatch", - "DataTableFileConflict", "ToolDataTableManager", "ShedToolDataTableManager", ) diff --git a/lib/galaxy/tool_util/data/__init__.py b/lib/galaxy/tool_util/data/__init__.py index 0b1be8651e3..9ea6e02aed2 100644 --- a/lib/galaxy/tool_util/data/__init__.py +++ b/lib/galaxy/tool_util/data/__init__.py @@ -23,7 +23,6 @@ from typing import ( BinaryIO, Callable, Dict, - Iterable, List, Optional, overload, @@ -154,28 +153,6 @@ class DataTableColumnMismatch(Exception): ) -class DataTableFileConflict(Exception): - """Two data tables with different names reference the same loc file.""" - - def __init__( - self, - path: str, - candidate_name: str, - candidate_columns: Dict[str, int], - existing_name: str, - existing_columns: Dict[str, int], - ): - self.path = path - self.candidate_name = candidate_name - self.candidate_columns = candidate_columns - self.existing_name = existing_name - self.existing_columns = existing_columns - super().__init__( - f"Data table {candidate_name!r} declares loc file {path!r}, but that file is already " - f"registered to data table {existing_name!r}." - ) - - class ToolDataTable(Dictifiable): type_key: str data: List[List[str]] @@ -1022,31 +999,13 @@ class ToolDataTableManager(Dictifiable): self, candidate_name: str, candidate_columns: Dict[str, int], - candidate_file_paths: Iterable[str], ) -> None: - """ - Raise if registering ``candidate_name`` would conflict with current state: - an existing table with the same name but different columns, or any - ``candidate_file_paths`` already owned by a different table name. - """ + """Raise if ``candidate_name`` is already registered with different columns.""" existing = self.data_tables.get(candidate_name) if existing is not None: existing_columns = getattr(existing, "columns", None) if existing_columns is not None and existing_columns != candidate_columns: raise DataTableColumnMismatch(candidate_name, existing_columns, candidate_columns) - candidate_realpaths = {os.path.realpath(p) for p in candidate_file_paths if p} - if not candidate_realpaths: - return - for other_name, other_table in self.data_tables.items(): - if other_name == candidate_name: - continue - other_filenames = getattr(other_table, "filenames", None) or {} - for other_path in other_filenames: - if os.path.realpath(other_path) in candidate_realpaths: - other_columns = getattr(other_table, "columns", None) or {} - raise DataTableFileConflict( - other_path, candidate_name, candidate_columns, other_name, other_columns - ) def to_dict( self, view: str = "collection", value_mapper: Optional[Dict[str, Callable]] = None @@ -1085,11 +1044,6 @@ class ToolDataTableManager(Dictifiable): other_config_dict=self.other_config_dict, ) table_elems.append(table_elem) - self.assert_data_table_consistency( - table.name, - getattr(table, "columns", {}) or {}, - getattr(table, "filenames", {}) or {}, - ) if table.name not in self.data_tables: self.data_tables[table.name] = table log.debug("Loaded tool data table '%s' from file '%s'", table.name, config_filename) diff --git a/test/unit/app/test_galaxy_install.py b/test/unit/app/test_galaxy_install.py index a45dbf0ad54..0705b1dd5e4 100644 --- a/test/unit/app/test_galaxy_install.py +++ b/test/unit/app/test_galaxy_install.py @@ -43,6 +43,7 @@ def test_against_production_shed(tmp_path: Path): assert tool_guid in f.read() repo_path = tmp_path / "tools" / "toolshed.g2.bx.psu.edu" / "repos" / repo_owner / repo_name / repo_revision assert repo_path.exists() + # featurecounts is not a Data Manager — install must not register a per-revision data table config. tool_data_table_path = ( tmp_path / "tool_data" @@ -53,7 +54,7 @@ def test_against_production_shed(tmp_path: Path): / repo_revision / "tool_data_table_conf.xml" ) - assert tool_data_table_path.exists() + assert not tool_data_table_path.exists() install_model_context = cast("install_model_scoped_session", install_target.install_model.session) query = install_model_context.query(ToolShedRepository).where(ToolShedRepository.name == repo_name) diff --git a/test/unit/tool_shed/test_data_table_manager.py b/test/unit/tool_shed/test_data_table_manager.py index 940d99cd7f7..a22acd8ee9f 100644 --- a/test/unit/tool_shed/test_data_table_manager.py +++ b/test/unit/tool_shed/test_data_table_manager.py @@ -7,7 +7,6 @@ import pytest from galaxy.tool_shed.tools.data_table_manager import ( DataTableColumnMismatch, - DataTableFileConflict, ShedToolDataTableManager, ) from galaxy.tool_util.data import ToolDataTableManager @@ -164,23 +163,6 @@ def test_column_match_with_column_elements_dedupes(tmp_path): assert kept_elems == [] -def test_file_path_conflict_raises(tmp_path): - stdtm, repo, samples, captured, tool_data_path, _, _ = _make_stdtm(tmp_path) - shared_loc = os.path.join(tool_data_path, "all_fasta.loc") - stdtm.app.tool_data_tables.data_tables = { - "other_table": _registered_table( - {"value": 0, "name": 1}, - filenames={shared_loc: {"found": True}}, - ), - } - - with pytest.raises(DataTableFileConflict) as exc_info: - stdtm.install_tool_data_tables(repo, samples) - assert exc_info.value.candidate_name == "all_fasta" - assert exc_info.value.existing_name == "other_table" - assert not captured["to_xml_calls"] - - def test_parse_table_columns_aliases_name_to_value(): from galaxy.tool_shed.tools.data_table_manager import _parse_table_columns diff --git a/test/unit/tool_util/data/test_tool_data.py b/test/unit/tool_util/data/test_tool_data.py index e2d8fc16261..b73eaf31507 100644 --- a/test/unit/tool_util/data/test_tool_data.py +++ b/test/unit/tool_util/data/test_tool_data.py @@ -1,9 +1,6 @@ import pytest -from galaxy.tool_util.data import ( - DataTableColumnMismatch, - DataTableFileConflict, -) +from galaxy.tool_util.data import DataTableColumnMismatch LOC_ALPHA_CONTENTS_V2 = """ data1 data1name ${__HERE__}/data1/entry.txt @@ -11,14 +8,6 @@ data2 data2name ${__HERE__}/data2/entry.txt data3 data3name ${__HERE__}/data3/entry.txt """ -CONFLICTING_TABLE_CONF_XML = """ -
- value, name, path - -
- -""" - COLUMN_DIVERGENT_TABLE_CONF_XML = """ value, name, path, extra @@ -85,54 +74,22 @@ def test_to_json(merged_tdt_manager, tmp_path): assert json_path.exists() -def test_assert_data_table_consistency_accepts_new_table(tdt_manager, tmp_path): +def test_assert_data_table_consistency_accepts_new_table(tdt_manager): tdt_manager.assert_data_table_consistency( "brand_new_table", {"value": 0, "name": 1, "path": 2}, - [str(tmp_path / "brand_new_table.loc")], ) -def test_assert_data_table_consistency_accepts_matching_redefinition(tdt_manager, tmp_path): +def test_assert_data_table_consistency_accepts_matching_redefinition(tdt_manager): existing = tdt_manager["testalpha"] - tdt_manager.assert_data_table_consistency( - "testalpha", - existing.columns, - list(existing.filenames), - ) + tdt_manager.assert_data_table_consistency("testalpha", existing.columns) -def test_assert_data_table_consistency_raises_column_mismatch(tdt_manager, tmp_path): +def test_assert_data_table_consistency_raises_column_mismatch(tdt_manager): with pytest.raises(DataTableColumnMismatch) as exc_info: tdt_manager.assert_data_table_consistency( "testalpha", {"value": 0, "name": 1, "path": 2, "extra": 3}, - [str(tmp_path / "testalpha.loc")], ) assert exc_info.value.table_name == "testalpha" - - -def test_assert_data_table_consistency_raises_file_conflict(tdt_manager, tmp_path): - shared_loc = str(tmp_path / "testalpha.loc") - with pytest.raises(DataTableFileConflict) as exc_info: - tdt_manager.assert_data_table_consistency( - "other_table", - {"value": 0, "name": 1, "path": 2}, - [shared_loc], - ) - assert exc_info.value.candidate_name == "other_table" - assert exc_info.value.existing_name == "testalpha" - - -def test_load_from_config_file_raises_on_file_path_conflict(tdt_manager, tmp_path): - conflicting_conf = tmp_path / "conflict.xml" - conflicting_conf.write_text(CONFLICTING_TABLE_CONF_XML.format(loc_path=str(tmp_path / "testalpha.loc"))) - with pytest.raises(DataTableFileConflict): - tdt_manager.load_from_config_file(str(conflicting_conf), str(tmp_path), from_shed_config=True) - - -def test_load_from_config_file_raises_on_column_mismatch_same_name(tdt_manager, tmp_path): - column_conflict_conf = tmp_path / "column_conflict.xml" - column_conflict_conf.write_text(COLUMN_DIVERGENT_TABLE_CONF_XML.format(loc_path=str(tmp_path / "testalpha.loc"))) - with pytest.raises(DataTableColumnMismatch): - tdt_manager.load_from_config_file(str(column_conflict_conf), str(tmp_path), from_shed_config=True) From 07c5d445da70630bcaca6aedffcb24034d7d8174 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 13 May 2026 16:56:19 -0400 Subject: [PATCH 06/13] Ensure tool_data_path exists; assert non-DM skip via persisted XML install_tool_data_tables used to create tool_data_path as a side effect of os.makedirs(target_dir); skipping it for non-DM repos means copy_sample_files now blows up with FileNotFoundError. Explicitly mkdir(exist_ok=True) before copying. The non-DM integration test was asserting against the in-memory tool_data_tables dict, which is populated as tools are loaded regardless of registration; switch to verify_no_installed_repository_data_table_entries, which inspects shed_tool_data_table_conf.xml. --- lib/galaxy/tool_shed/galaxy_install/install_manager.py | 2 ++ test/integration/test_repository_operations.py | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/tool_shed/galaxy_install/install_manager.py b/lib/galaxy/tool_shed/galaxy_install/install_manager.py index 12a284cf045..cee14887f0d 100644 --- a/lib/galaxy/tool_shed/galaxy_install/install_manager.py +++ b/lib/galaxy/tool_shed/galaxy_install/install_manager.py @@ -188,6 +188,8 @@ class InstallRepositoryManager: sample_files = irmm_metadata_dict.get("sample_files", []) tool_index_sample_files = stdtm.get_tool_index_sample_files(sample_files) tool_data_path = self.app.config.tool_data_path + # Used to be created as a side effect of install_tool_data_tables; that no longer runs for non-DM repos. + os.makedirs(tool_data_path, exist_ok=True) tool_util.copy_sample_files(tool_data_path, tool_index_sample_files, tool_path=tool_path) sample_files_copied = [str(s) for s in tool_index_sample_files] repository_tools_tups = irmm.get_repository_tools_tups() diff --git a/test/integration/test_repository_operations.py b/test/integration/test_repository_operations.py index 33a0e5e9527..3c9a415f7e6 100644 --- a/test/integration/test_repository_operations.py +++ b/test/integration/test_repository_operations.py @@ -1,4 +1,5 @@ import os +import xml.etree.ElementTree as ET from collections import namedtuple from sqlalchemy import select @@ -62,13 +63,14 @@ class TestRepositoryInstallIntegrationTestCase(integration_util.IntegrationTestC self.uninstall_repository(*repo) def test_non_data_manager_install_skips_data_table_registration(self): - """Non-Data-Manager repos must not register data tables on install.""" + """Non-Data-Manager repos must not persist data table entries in shed_tool_data_table_conf.xml.""" non_dm_repo = ("devteam", "bwa", "051eba708f43") non_dm_table_names = {"bwa_indexes", "bwa_mem_indexes"} self.install_repository(*non_dm_repo) - registered = set(self._app.tool_data_tables.data_tables.keys()) + shed_conf = self._app.config.shed_tool_data_table_config + registered = {t.get("name") for t in ET.parse(shed_conf).getroot().findall("table")} leaked = non_dm_table_names & registered - assert not leaked, f"Unexpected data tables registered by non-DM repo: {sorted(leaked)}" + assert not leaked, f"Unexpected data tables in {shed_conf}: {sorted(leaked)}" def test_repository_update(self): response = self._install_repository(revision=REVISION_4, version="0.0.3", allow_upgraded=True)[0] From 3e00f529badfe9d75d1f42d6a809a9d4c6cf86b6 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 14 May 2026 14:07:57 -0400 Subject: [PATCH 07/13] Merge & dedup data table entries on shed install (no DM gate) Replace the non-Data-Manager skip with a uniform install path: every repo's tables get registered. First install copies the .loc.sample to tool_data_path and writes the
entry without a sub-element; subsequent installs of the same table append unique rows from their .loc.sample to the shared loc file, prefixed by a comment_char attribution line. get_filename_for_source falls back to a no-repo_info filename so DMs on legacy installs keep matching their existing -tagged files. Co-Authored-By: Claude Opus 4.7 --- .../galaxy_install/install_manager.py | 2 +- .../tool_shed/tools/data_table_manager.py | 42 ++++++++--- lib/galaxy/tool_util/data/__init__.py | 65 +++++++++++++++++ .../integration/test_repository_operations.py | 22 ++++-- test/unit/app/test_galaxy_install.py | 5 +- .../unit/tool_shed/test_data_table_manager.py | 46 +++++++++++- test/unit/tool_shed/test_install_manager.py | 6 +- test/unit/tool_util/data/test_tool_data.py | 70 +++++++++++++++++++ 8 files changed, 237 insertions(+), 21 deletions(-) diff --git a/lib/galaxy/tool_shed/galaxy_install/install_manager.py b/lib/galaxy/tool_shed/galaxy_install/install_manager.py index cee14887f0d..2b3a1efa8d2 100644 --- a/lib/galaxy/tool_shed/galaxy_install/install_manager.py +++ b/lib/galaxy/tool_shed/galaxy_install/install_manager.py @@ -171,7 +171,7 @@ class InstallRepositoryManager: session.commit() is_data_manager = "data_manager" in irmm_metadata_dict - if is_data_manager and "sample_files" in irmm_metadata_dict: + if "sample_files" in irmm_metadata_dict: sample_files = irmm_metadata_dict.get("sample_files", []) tool_index_sample_files = stdtm.get_tool_index_sample_files(sample_files) tool_data_table_conf_filename, tool_data_table_elems = stdtm.install_tool_data_tables( diff --git a/lib/galaxy/tool_shed/tools/data_table_manager.py b/lib/galaxy/tool_shed/tools/data_table_manager.py index caeecea1dbe..574f3a628c8 100644 --- a/lib/galaxy/tool_shed/tools/data_table_manager.py +++ b/lib/galaxy/tool_shed/tools/data_table_manager.py @@ -8,7 +8,10 @@ from typing import ( from galaxy.tool_shed.galaxy_install.client import InstallationTarget from galaxy.tool_shed.util import hg_util -from galaxy.tool_util.data import DataTableColumnMismatch +from galaxy.tool_util.data import ( + DataTableColumnMismatch, + TabularToolDataTable, +) from galaxy.util import ( Element, SubElement, @@ -28,8 +31,6 @@ RequiredAppT = Union["BasicSharedApp", InstallationTarget] def _parse_table_columns(table_elem: Element) -> dict[str, int]: """Parse a ``
`` element's column spec into a name->index mapping.""" - from galaxy.tool_util.data import TabularToolDataTable - columns, _, _ = TabularToolDataTable.parse_column_spec_element(table_elem) if "value" in columns and "name" not in columns: columns["name"] = columns["value"] @@ -157,6 +158,28 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): os.makedirs(target_dir) return target_dir, tool_path, relative_target_dir + def _merge_loc_sample_entries( + self, + existing_table: TabularToolDataTable, + elem: Element, + loc_basename_to_source: dict, + tool_shed_repository: "ToolShedRepository", + ) -> None: + attribution = ( + f"Added by {tool_shed_repository.owner}/{tool_shed_repository.name}" + f"@{tool_shed_repository.installed_changeset_revision}" + ) + for file_elem in elem.findall("file"): + shared_path = file_elem.get("path") + basename = os.path.basename(shared_path) if shared_path else None + source_loc_sample = loc_basename_to_source.get(basename) if basename else None + if not source_loc_sample or not os.path.exists(source_loc_sample): + continue + # Keep `${__HERE__}` literal so the appended rows match the shared loc's existing format. + new_rows = existing_table.parse_file_fields(source_loc_sample, here="${__HERE__}") + if new_rows: + existing_table.append_entries_with_attribution(new_rows, attribution) + def install_tool_data_tables(self, tool_shed_repository: "ToolShedRepository", tool_index_sample_files): TOOL_DATA_TABLE_FILE_NAME = "tool_data_table_conf.xml" TOOL_DATA_TABLE_FILE_SAMPLE_NAME = f"{TOOL_DATA_TABLE_FILE_NAME}.sample" @@ -164,8 +187,10 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): SAMPLE_SUFFIX_OFFSET = -len(SAMPLE_SUFFIX) LOC_SAMPLE_SUFFIX = ".loc.sample" target_dir, tool_path, relative_target_dir = self.get_target_install_dir(tool_shed_repository) - # .loc files are shared across DM revisions: install at the root of tool_data_path. + # .loc files are shared across repository installations: install at the root of tool_data_path. shared_loc_dir = self.app.config.tool_data_path + # Map shared loc basename -> source .loc.sample, used to merge entries when a table is reinstalled. + loc_basename_to_source: dict[str, str] = {} for sample_file in tool_index_sample_files: path, filename = os.path.split(sample_file) target_filename = filename @@ -175,6 +200,7 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): if filename.endswith(LOC_SAMPLE_SUFFIX): target_path_filename = os.path.join(shared_loc_dir, target_filename) install_dest_dir = shared_loc_dir + loc_basename_to_source[target_filename] = source_file else: target_path_filename = os.path.join(target_dir, target_filename) install_dest_dir = target_dir @@ -225,11 +251,11 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): incoming_columns = _parse_table_columns(elem) self.app.tool_data_tables.assert_data_table_consistency(table_name, incoming_columns) existing = registered_tables.get(table_name) if table_name else None - if existing is not None and getattr(existing, "columns", None) is not None: - # Already registered with matching columns; skip to avoid duplicate
entry. + if isinstance(existing, TabularToolDataTable) and existing.columns is not None: + # Already registered with matching columns. Merge any rows from this install's + # .loc.sample(s) into the shared loc file, then skip adding a duplicate
. + self._merge_loc_sample_entries(existing, elem, loc_basename_to_source, tool_shed_repository) continue - # Store repository info in the table tag set for trace-ability. - self.generate_repository_info_elem_from_repository(tool_shed_repository, parent_elem=elem) kept_elems.append(elem) if kept_elems: # Remove old data_table diff --git a/lib/galaxy/tool_util/data/__init__.py b/lib/galaxy/tool_util/data/__init__.py index 9ea6e02aed2..624e36d4509 100644 --- a/lib/galaxy/tool_util/data/__init__.py +++ b/lib/galaxy/tool_util/data/__init__.py @@ -684,6 +684,7 @@ class TabularToolDataTable(ToolDataTable): source_repo_info_model = source.repo_info source_repo_info = source_repo_info_model.model_dump() if source_repo_info_model else None filename = default + shared_fallback: Optional[str] = None for name, value in self.filenames.items(): repo_info = value.get("tool_shed_repository") if (not source_repo_info and not repo_info) or ( @@ -691,6 +692,14 @@ class TabularToolDataTable(ToolDataTable): ): filename = name break + if source_repo_info and not repo_info and shared_fallback is None: + # Loc file registered without repo_info (shared install) — use as fallback + # when a tool-shed source has no exact match. Preserves legacy behavior where + # a ``-tagged filename matches exactly. + shared_fallback = name + else: + if shared_fallback is not None: + filename = shared_fallback return filename def _add_entry( @@ -762,6 +771,62 @@ class TabularToolDataTable(ToolDataTable): fields_collapsed = f"{self.separator.join(fields)}\n" data_table_fh.write(fields_collapsed.encode("utf-8")) + def append_entries_with_attribution( + self, + entries: List[List[str]], + attribution: str, + allow_duplicates: bool = False, + ) -> int: + """Append ``entries`` to this table's loc file with an attribution comment line. + + When ``allow_duplicates`` is False, dedupes entries by the ``value`` column + (the table's primary key) against existing in-memory rows and the pending + batch. Writes a single ``{comment_char} {attribution}`` line before the first + new row. No-op if no rows survive the dedup. + """ + filename: Optional[str] = self.get_filename_for_source(None) + if filename is None: + for name in self.filenames: + filename = name + break + if filename is None: + raise MessageException(f"Unable to determine filename for appending entries to data table '{self.name}'.") + value_index = self.columns.get("value", 0) + existing_values: Optional[Set[str]] = None + if not allow_duplicates: + existing_values = {row[value_index] for row in self.data if value_index < len(row)} + new_rows: List[List[str]] = [] + for entry in entries: + fields = self._replace_field_separators(list(entry)) + if self.largest_index >= len(fields): + log.warning( + "Skipping fields (%s) for data table '%s': only %d field(s) provided.", + fields, + self.name, + len(fields), + ) + continue + if existing_values is not None and fields[value_index] in existing_values: + continue + new_rows.append(fields) + if existing_values is not None: + existing_values.add(fields[value_index]) + if not new_rows: + return self._loaded_content_version + with FileLock(filename): + if os.path.exists(filename) and os.stat(filename).st_size > 0: + with open(filename, "rb+") as fh: + fh.seek(-1, 2) + if fh.read(1) not in (b"\n", b"\r"): + fh.write(b"\n") + with open(filename, "ab") as fh: + fh.write(f"{self.comment_char} {attribution}\n".encode()) + for fields in new_rows: + fh.write(f"{self.separator.join(fields)}\n".encode()) + for fields in new_rows: + self.data.append(fields) + return self._update_version() + def _locate_filename(self, tool_data_file_path, entry_source, bundle_mode): if tool_data_file_path is not None: filename = tool_data_file_path diff --git a/test/integration/test_repository_operations.py b/test/integration/test_repository_operations.py index 3c9a415f7e6..a31130a2ac2 100644 --- a/test/integration/test_repository_operations.py +++ b/test/integration/test_repository_operations.py @@ -62,15 +62,27 @@ class TestRepositoryInstallIntegrationTestCase(integration_util.IntegrationTestC self.install_repository(*repo) self.uninstall_repository(*repo) - def test_non_data_manager_install_skips_data_table_registration(self): - """Non-Data-Manager repos must not persist data table entries in shed_tool_data_table_conf.xml.""" + def test_non_data_manager_install_registers_data_tables_without_repo_info(self): + """Non-Data-Manager repos register tables in shed_tool_data_table_conf.xml without the + sub-element, and their loc files land at tool_data_path root.""" non_dm_repo = ("devteam", "bwa", "051eba708f43") non_dm_table_names = {"bwa_indexes", "bwa_mem_indexes"} self.install_repository(*non_dm_repo) shed_conf = self._app.config.shed_tool_data_table_config - registered = {t.get("name") for t in ET.parse(shed_conf).getroot().findall("table")} - leaked = non_dm_table_names & registered - assert not leaked, f"Unexpected data tables in {shed_conf}: {sorted(leaked)}" + table_elems = {t.get("name"): t for t in ET.parse(shed_conf).getroot().findall("table")} + missing = non_dm_table_names - table_elems.keys() + assert not missing, f"Expected tables not registered in {shed_conf}: {sorted(missing)}" + for name in non_dm_table_names: + assert ( + table_elems[name].find("tool_shed_repository") is None + ), f"Table {name!r} should not have a sub-element on new installs" + file_elems = table_elems[name].findall("file") + assert file_elems, f"Table {name!r} has no entry" + for file_elem in file_elems: + loc_path = file_elem.get("path") or "" + assert loc_path.startswith( + self._app.config.tool_data_path + ), f"Loc file for {name!r} not at tool_data_path root: {loc_path}" def test_repository_update(self): response = self._install_repository(revision=REVISION_4, version="0.0.3", allow_upgraded=True)[0] diff --git a/test/unit/app/test_galaxy_install.py b/test/unit/app/test_galaxy_install.py index 0705b1dd5e4..db67343423f 100644 --- a/test/unit/app/test_galaxy_install.py +++ b/test/unit/app/test_galaxy_install.py @@ -43,7 +43,8 @@ def test_against_production_shed(tmp_path: Path): assert tool_guid in f.read() repo_path = tmp_path / "tools" / "toolshed.g2.bx.psu.edu" / "repos" / repo_owner / repo_name / repo_revision assert repo_path.exists() - # featurecounts is not a Data Manager — install must not register a per-revision data table config. + # All shed installs now register data tables; the per-revision config is written for any repo + # with sample files, regardless of whether it's a Data Manager. tool_data_table_path = ( tmp_path / "tool_data" @@ -54,7 +55,7 @@ def test_against_production_shed(tmp_path: Path): / repo_revision / "tool_data_table_conf.xml" ) - assert not tool_data_table_path.exists() + assert tool_data_table_path.exists() install_model_context = cast("install_model_scoped_session", install_target.install_model.session) query = install_model_context.query(ToolShedRepository).where(ToolShedRepository.name == repo_name) diff --git a/test/unit/tool_shed/test_data_table_manager.py b/test/unit/tool_shed/test_data_table_manager.py index a22acd8ee9f..1bed277d1d6 100644 --- a/test/unit/tool_shed/test_data_table_manager.py +++ b/test/unit/tool_shed/test_data_table_manager.py @@ -9,7 +9,10 @@ from galaxy.tool_shed.tools.data_table_manager import ( DataTableColumnMismatch, ShedToolDataTableManager, ) -from galaxy.tool_util.data import ToolDataTableManager +from galaxy.tool_util.data import ( + TabularToolDataTable, + ToolDataTableManager, +) from galaxy.util import ( Element, SubElement, @@ -80,9 +83,10 @@ def _make_stdtm(tmp_path): def _registered_table(columns, filenames=None): - existing = mock.MagicMock(spec=["columns", "filenames"]) + existing = mock.MagicMock(spec=TabularToolDataTable) existing.columns = columns existing.filenames = filenames or {} + existing.parse_file_fields.return_value = [] return existing @@ -99,6 +103,10 @@ def test_loc_file_lands_at_shared_root_not_per_revision(tmp_path): file_elems = list(kept_elems[0].findall("file")) assert len(file_elems) == 1 assert file_elems[0].get("path") == shared_loc + # New installs should not stamp a sub-element on the
: + # the loc-file location is deterministic and DMs on legacy installs fall through to + # the shared-no-repo_info match in get_filename_for_source. + assert kept_elems[0].find("tool_shed_repository") is None def test_existing_loc_file_is_not_overwritten(tmp_path): @@ -163,6 +171,40 @@ def test_column_match_with_column_elements_dedupes(tmp_path): assert kept_elems == [] +def test_second_install_merges_loc_sample_rows_with_attribution(tmp_path): + stdtm, repo, samples, captured, _, _, _ = _make_stdtm(tmp_path) + matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} + existing = _registered_table(matching_columns) + incoming_rows = [["hg19", "hg19", "human (hg19)", "/data/hg19.fa"]] + existing.parse_file_fields.return_value = incoming_rows + stdtm.app.tool_data_tables.data_tables = {"all_fasta": existing} + + _, kept_elems = stdtm.install_tool_data_tables(repo, samples) + + assert kept_elems == [] + assert not captured["to_xml_calls"] + existing.append_entries_with_attribution.assert_called_once() + call_args = existing.append_entries_with_attribution.call_args + assert call_args.args[0] == incoming_rows + attribution = call_args.args[1] + assert "iuc/data_manager_fetch_genome_dbkeys_all_fasta" in attribution + assert "abc" in attribution + + +def test_second_install_with_empty_loc_sample_does_not_append(tmp_path): + stdtm, repo, samples, captured, _, _, _ = _make_stdtm(tmp_path) + matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} + existing = _registered_table(matching_columns) + existing.parse_file_fields.return_value = [] + stdtm.app.tool_data_tables.data_tables = {"all_fasta": existing} + + _, kept_elems = stdtm.install_tool_data_tables(repo, samples) + + assert kept_elems == [] + assert not captured["to_xml_calls"] + existing.append_entries_with_attribution.assert_not_called() + + def test_parse_table_columns_aliases_name_to_value(): from galaxy.tool_shed.tools.data_table_manager import _parse_table_columns diff --git a/test/unit/tool_shed/test_install_manager.py b/test/unit/tool_shed/test_install_manager.py index e3e0b2e970c..940dfb9799d 100644 --- a/test/unit/tool_shed/test_install_manager.py +++ b/test/unit/tool_shed/test_install_manager.py @@ -69,11 +69,11 @@ def _invoke_handle(irm, metadata_dict: dict[str, Any], repository_tools_tups: li return stdtm_instance -def test_non_data_manager_repo_skips_sample_files_registration(): +def test_non_data_manager_repo_registers_sample_files(): irm, register_mock = _make_install_repository_manager() stdtm_instance = _invoke_handle(irm, {"sample_files": ["foo.loc.sample"]}, []) - stdtm_instance.install_tool_data_tables.assert_not_called() - register_mock.assert_not_called() + stdtm_instance.install_tool_data_tables.assert_called_once() + register_mock.assert_called_once() def test_data_manager_repo_registers_sample_files(): diff --git a/test/unit/tool_util/data/test_tool_data.py b/test/unit/tool_util/data/test_tool_data.py index b73eaf31507..c627af78675 100644 --- a/test/unit/tool_util/data/test_tool_data.py +++ b/test/unit/tool_util/data/test_tool_data.py @@ -93,3 +93,73 @@ def test_assert_data_table_consistency_raises_column_mismatch(tdt_manager): {"value": 0, "name": 1, "path": 2, "extra": 3}, ) assert exc_info.value.table_name == "testalpha" + + +def test_get_filename_for_source_falls_back_to_shared_filename(tdt_manager): + table = tdt_manager["testalpha"] + [shared_filename] = list(table.filenames) + assert table.filenames[shared_filename].get("tool_shed_repository") is None + source_with_unknown_repo = { + "tool_shed": "tool-shed", + "repository_name": "repo", + "repository_owner": "owner", + "installed_changeset_revision": "abc", + } + assert table.get_filename_for_source(source_with_unknown_repo) == shared_filename + + +def test_get_filename_for_source_prefers_exact_repo_match_over_shared(tdt_manager): + table = tdt_manager["testalpha"] + [shared_filename] = list(table.filenames) + legacy_info = { + "tool_shed": "tool-shed", + "repository_name": "legacy", + "repository_owner": "owner", + "installed_changeset_revision": "deadbeef", + } + legacy_filename = f"{shared_filename}.legacy" + table.filenames[legacy_filename] = dict( + found=True, + filename=legacy_filename, + from_shed_config=True, + tool_data_path=None, + config_element=None, + tool_shed_repository=legacy_info, + errors=[], + ) + assert table.get_filename_for_source(legacy_info) == legacy_filename + other_info = dict(legacy_info, repository_name="unknown") + assert table.get_filename_for_source(other_info) == shared_filename + + +def test_append_entries_with_attribution_appends_and_dedupes(tdt_manager): + table = tdt_manager["testalpha"] + [loc_filename] = list(table.filenames) + initial_rows = len(table.data) + new_entries = [ + ["data3", "data3name", "${__HERE__}/data3/entry.txt"], + ["data1", "data1name", "${__HERE__}/data1/entry.txt"], # duplicate, must be skipped + ] + table.append_entries_with_attribution(new_entries, "added by owner/foo@rev1") + with open(loc_filename) as fh: + contents = fh.read() + assert "# added by owner/foo@rev1" in contents + assert contents.count("data3\tdata3name") == 1 + assert contents.count("data1\tdata1name") == 1 + assert len(table.data) == initial_rows + 1 + + +def test_append_entries_with_attribution_noop_when_all_duplicates(tdt_manager): + table = tdt_manager["testalpha"] + [loc_filename] = list(table.filenames) + with open(loc_filename) as fh: + before = fh.read() + rows_before = list(table.data) + table.append_entries_with_attribution( + [["data1", "data1name", "${__HERE__}/data1/entry.txt"]], + "added by owner/foo@rev1", + ) + with open(loc_filename) as fh: + after = fh.read() + assert after == before + assert table.data == rows_before From 99f951108ee8a11e8cdccccfd7d2656a54a5378e Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 14 May 2026 14:43:36 -0400 Subject: [PATCH 08/13] Revert test_1010 to assert positive data-table registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Toolshed tests caught test_0025_verify_sample_files still asserting the old "non-DM repos skip data-table registration" behavior. The design pivot reverted that — non-DM repos now register tables — so the test goes back to its original positive assertion and the unused verify_no_installed_repository_data_table_entries helper is removed. Co-Authored-By: Claude Opus 4.7 --- lib/tool_shed/test/base/testcase.py | 12 ------------ ...1010_install_repository_with_tool_dependencies.py | 6 ++---- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/lib/tool_shed/test/base/testcase.py b/lib/tool_shed/test/base/testcase.py index b945f4db3b5..f485e711fe1 100644 --- a/lib/tool_shed/test/base/testcase.py +++ b/lib/tool_shed/test/base/testcase.py @@ -1655,18 +1655,6 @@ class ShedTestCase(ShedApiTestCase): assert self._installation_client self._installation_client.refresh_tool_shed_repository(repo) - def verify_no_installed_repository_data_table_entries(self, table_names): - """Assert that none of ``table_names`` appears in shed_tool_data_table_conf.xml.""" - shed_tool_data_table_conf = self.shed_tool_data_table_conf - if not os.path.exists(shed_tool_data_table_conf): - return - data_tables, error_message = xml_util.parse_xml(shed_tool_data_table_conf) - assert not error_message, f"Failed to parse {shed_tool_data_table_conf}: {error_message}" - assert data_tables is not None - registered_names = {t.get("name") for t in data_tables.findall("table")} - for name in table_names: - assert name not in registered_names, f"Unexpected data table entry '{name}' in {shed_tool_data_table_conf}" - def verify_installed_repository_data_table_entries(self, required_data_table_entries): # The value of the received required_data_table_entries will be something like: [ 'sam_fa_indexes' ] shed_tool_data_table_conf = self.shed_tool_data_table_conf diff --git a/lib/tool_shed/test/functional/test_1010_install_repository_with_tool_dependencies.py b/lib/tool_shed/test/functional/test_1010_install_repository_with_tool_dependencies.py index 42dcde1637e..4e66f2b8b2d 100644 --- a/lib/tool_shed/test/functional/test_1010_install_repository_with_tool_dependencies.py +++ b/lib/tool_shed/test/functional/test_1010_install_repository_with_tool_dependencies.py @@ -63,7 +63,5 @@ class TestToolWithToolDependencies(ShedTestCase): self.verify_installed_repository_metadata_unchanged(repository_name, common.test_user_1_name) def test_0025_verify_sample_files(self): - """Non-Data-Manager repositories no longer auto-register data tables on install - (galaxyproject/galaxy#21448); confirm sam_fa_indexes is absent from - shed_tool_data_table_conf.xml.""" - self.verify_no_installed_repository_data_table_entries(table_names=["sam_fa_indexes"]) + """Verify that the installed repository populated shed_tool_data_table.xml and the sample files.""" + self.verify_installed_repository_data_table_entries(required_data_table_entries=["sam_fa_indexes"]) From 962afa6200ad63a2d5b17416e526d6e175508fa7 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 14 May 2026 15:44:46 -0400 Subject: [PATCH 09/13] Register shared loc path with existing table on merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When install_tool_data_tables hits a table that's already registered (e.g. __dbkeys__ from Galaxy's shipped refgenie config, or bwa_indexes from the shipped tabular sample), we skip adding another
to shed_tool_data_table_conf.xml — but that also skipped registering the shared loc file path in the existing table's `filenames` dict. Data Managers then couldn't locate where to persist entries ("Unable to determine filename for persisting data table '__dbkeys__'") because get_filename_for_source had nothing to fall back to. Register the shared loc path on the existing table with tool_shed_repository=None so the new fallback in get_filename_for_source finds it. Also fix the integration test for devteam/bwa: bwa_indexes / bwa_mem_indexes already exist in tool_data_table_conf.xml.sample, so their absence from shed_tool_data_table_conf.xml is correct dedup; assert the shared loc file lands at tool_data_path instead. Co-Authored-By: Claude Opus 4.7 --- .../tool_shed/tools/data_table_manager.py | 31 ++++++++++++++++-- .../integration/test_repository_operations.py | 32 ++++++++----------- .../unit/tool_shed/test_data_table_manager.py | 19 +++++++++++ 3 files changed, 62 insertions(+), 20 deletions(-) diff --git a/lib/galaxy/tool_shed/tools/data_table_manager.py b/lib/galaxy/tool_shed/tools/data_table_manager.py index 574f3a628c8..d1629ef5a91 100644 --- a/lib/galaxy/tool_shed/tools/data_table_manager.py +++ b/lib/galaxy/tool_shed/tools/data_table_manager.py @@ -171,8 +171,11 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): ) for file_elem in elem.findall("file"): shared_path = file_elem.get("path") - basename = os.path.basename(shared_path) if shared_path else None - source_loc_sample = loc_basename_to_source.get(basename) if basename else None + if not shared_path: + continue + self._register_shared_loc_with_existing_table(existing_table, shared_path, elem) + basename = os.path.basename(shared_path) + source_loc_sample = loc_basename_to_source.get(basename) if not source_loc_sample or not os.path.exists(source_loc_sample): continue # Keep `${__HERE__}` literal so the appended rows match the shared loc's existing format. @@ -180,6 +183,30 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): if new_rows: existing_table.append_entries_with_attribution(new_rows, attribution) + def _register_shared_loc_with_existing_table( + self, + existing_table: TabularToolDataTable, + shared_path: str, + elem: Element, + ) -> None: + """Make the shared loc file path known to the existing table's ``filenames``. + + Without this, Data Manager persist calls can't locate a destination loc file + when the table was first registered from a shipped or refgenie config that has + no tabular ```` entry (e.g. ``__dbkeys__``). + """ + if shared_path in existing_table.filenames: + return + existing_table.filenames[shared_path] = dict( + found=os.path.exists(shared_path), + filename=shared_path, + from_shed_config=True, + tool_data_path=self.app.config.tool_data_path, + config_element=elem, + tool_shed_repository=None, + errors=[], + ) + def install_tool_data_tables(self, tool_shed_repository: "ToolShedRepository", tool_index_sample_files): TOOL_DATA_TABLE_FILE_NAME = "tool_data_table_conf.xml" TOOL_DATA_TABLE_FILE_SAMPLE_NAME = f"{TOOL_DATA_TABLE_FILE_NAME}.sample" diff --git a/test/integration/test_repository_operations.py b/test/integration/test_repository_operations.py index a31130a2ac2..89a42106572 100644 --- a/test/integration/test_repository_operations.py +++ b/test/integration/test_repository_operations.py @@ -62,27 +62,23 @@ class TestRepositoryInstallIntegrationTestCase(integration_util.IntegrationTestC self.install_repository(*repo) self.uninstall_repository(*repo) - def test_non_data_manager_install_registers_data_tables_without_repo_info(self): - """Non-Data-Manager repos register tables in shed_tool_data_table_conf.xml without the - sub-element, and their loc files land at tool_data_path root.""" + def test_non_data_manager_install_lands_loc_files_at_tool_data_path(self): + """Non-Data-Manager repos install their .loc files to tool_data_path root, and any +
entries written to shed_tool_data_table_conf.xml have no + sub-element. Tables that already exist in Galaxy's shipped tool_data_table_conf.xml.sample + are deduped (not re-added to shed_tool_data_table_conf.xml).""" non_dm_repo = ("devteam", "bwa", "051eba708f43") - non_dm_table_names = {"bwa_indexes", "bwa_mem_indexes"} + non_dm_loc_files = {"bwa_index.loc"} self.install_repository(*non_dm_repo) + for loc_file in non_dm_loc_files: + shared_loc = os.path.join(self._app.config.tool_data_path, loc_file) + assert os.path.exists(shared_loc), f"Expected shared loc file at {shared_loc}" shed_conf = self._app.config.shed_tool_data_table_config - table_elems = {t.get("name"): t for t in ET.parse(shed_conf).getroot().findall("table")} - missing = non_dm_table_names - table_elems.keys() - assert not missing, f"Expected tables not registered in {shed_conf}: {sorted(missing)}" - for name in non_dm_table_names: - assert ( - table_elems[name].find("tool_shed_repository") is None - ), f"Table {name!r} should not have a sub-element on new installs" - file_elems = table_elems[name].findall("file") - assert file_elems, f"Table {name!r} has no entry" - for file_elem in file_elems: - loc_path = file_elem.get("path") or "" - assert loc_path.startswith( - self._app.config.tool_data_path - ), f"Loc file for {name!r} not at tool_data_path root: {loc_path}" + if os.path.exists(shed_conf): + for table_elem in ET.parse(shed_conf).getroot().findall("table"): + assert ( + table_elem.find("tool_shed_repository") is None + ), f"Table {table_elem.get('name')!r} should not have a sub-element" def test_repository_update(self): response = self._install_repository(revision=REVISION_4, version="0.0.3", allow_upgraded=True)[0] diff --git a/test/unit/tool_shed/test_data_table_manager.py b/test/unit/tool_shed/test_data_table_manager.py index 1bed277d1d6..f0392d6f7dd 100644 --- a/test/unit/tool_shed/test_data_table_manager.py +++ b/test/unit/tool_shed/test_data_table_manager.py @@ -205,6 +205,25 @@ def test_second_install_with_empty_loc_sample_does_not_append(tmp_path): existing.append_entries_with_attribution.assert_not_called() +def test_second_install_registers_shared_loc_with_existing_table(tmp_path): + """When merging into an existing table whose ``filenames`` is empty (e.g. refgenie tables + or shipped tables whose loc resolution failed), the shared loc path must be registered so + Data Managers can persist entries.""" + stdtm, repo, samples, _, tool_data_path, _, _ = _make_stdtm(tmp_path) + matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} + existing = _registered_table(matching_columns) + existing.parse_file_fields.return_value = [] + stdtm.app.tool_data_tables.data_tables = {"all_fasta": existing} + + stdtm.install_tool_data_tables(repo, samples) + + shared_loc = os.path.join(tool_data_path, "all_fasta.loc") + assert shared_loc in existing.filenames + info = existing.filenames[shared_loc] + assert info["tool_shed_repository"] is None + assert info["from_shed_config"] is True + + def test_parse_table_columns_aliases_name_to_value(): from galaxy.tool_shed.tools.data_table_manager import _parse_table_columns From 652bb74707a95f51455e6697044986df2e065e57 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 15 May 2026 10:51:23 -0400 Subject: [PATCH 10/13] Isolate shed-managed loc files under tool_data_path/shed/ Galaxy-managed loc files for shed-installed tools now land under `tool_data_path/shed/` instead of the `tool_data_path` root. This keeps them clearly separate from admin-configured loc files and from anything shipped via `tool_data_table_conf.xml.sample`, avoiding overlap with predefined tables. Also fixes the DM persist failure (`Unable to determine filename for persisting data table '__dbkeys__'`) by writing the (stamp-less) `
` entry to `shed_tool_data_table_conf.xml` so the shared loc file's association survives reload via `merge_tool_data_table`. Writes are deduped against the existing shed config to avoid bloat. Refgenie- backed and other non-tabular table types skip the `.loc.sample` row merge (their `parse_file_fields` is not designed to read a `.loc`). --- .../tool_shed/tools/data_table_manager.py | 70 ++++++++------ .../integration/test_repository_operations.py | 17 ++-- .../unit/tool_shed/test_data_table_manager.py | 93 ++++++++++++++----- 3 files changed, 124 insertions(+), 56 deletions(-) diff --git a/lib/galaxy/tool_shed/tools/data_table_manager.py b/lib/galaxy/tool_shed/tools/data_table_manager.py index d1629ef5a91..4ff11d7a43e 100644 --- a/lib/galaxy/tool_shed/tools/data_table_manager.py +++ b/lib/galaxy/tool_shed/tools/data_table_manager.py @@ -165,6 +165,15 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): loc_basename_to_source: dict, tool_shed_repository: "ToolShedRepository", ) -> None: + """Append any non-comment rows from this install's .loc.sample files to the shared loc file, + attributing them to the installing repository. 99% of .loc.sample files are empty/comments, + in which case this is a no-op. + + Skipped for non-tabular table types (e.g. ``RefgenieToolDataTable``) whose + ``parse_file_fields`` is not designed to read a ``.loc.sample``. + """ + if getattr(existing_table, "type_key", "tabular") != "tabular": + return attribution = ( f"Added by {tool_shed_repository.owner}/{tool_shed_repository.name}" f"@{tool_shed_repository.installed_changeset_revision}" @@ -173,7 +182,6 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): shared_path = file_elem.get("path") if not shared_path: continue - self._register_shared_loc_with_existing_table(existing_table, shared_path, elem) basename = os.path.basename(shared_path) source_loc_sample = loc_basename_to_source.get(basename) if not source_loc_sample or not os.path.exists(source_loc_sample): @@ -183,29 +191,33 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): if new_rows: existing_table.append_entries_with_attribution(new_rows, attribution) - def _register_shared_loc_with_existing_table( - self, - existing_table: TabularToolDataTable, - shared_path: str, - elem: Element, - ) -> None: - """Make the shared loc file path known to the existing table's ``filenames``. + def _shed_config_has_matching_entry(self, table_name: str, elem: Element) -> bool: + """Return True if ``shed_tool_data_table_config`` already has a ``
`` with the same + ``name`` and identical set of ```` entries as ``elem``. - Without this, Data Manager persist calls can't locate a destination loc file - when the table was first registered from a shipped or refgenie config that has - no tabular ```` entry (e.g. ``__dbkeys__``). + Used to avoid writing duplicate ``
`` entries for tables that have already been + registered by a prior install. The shed config remains the persistent source of the + shared loc file's association with the data table name — on reload, ``merge_tool_data_table`` + re-applies the filename info to the in-memory table. """ - if shared_path in existing_table.filenames: - return - existing_table.filenames[shared_path] = dict( - found=os.path.exists(shared_path), - filename=shared_path, - from_shed_config=True, - tool_data_path=self.app.config.tool_data_path, - config_element=elem, - tool_shed_repository=None, - errors=[], - ) + config = self.app.config.shed_tool_data_table_config + if not config or not os.path.exists(config): + return False + try: + tree, _ = xml_util.parse_xml(config) + except OSError as e: + log.warning("Could not read shed_tool_data_table_config '%s' for dedup check: %s", config, e) + return False + if tree is None: + return False + elem_paths = {fe.get("path") for fe in elem.findall("file") if fe.get("path")} + for existing_elem in tree.getroot().findall("table"): + if existing_elem.get("name") != table_name: + continue + existing_paths = {fe.get("path") for fe in existing_elem.findall("file") if fe.get("path")} + if elem_paths == existing_paths: + return True + return False def install_tool_data_tables(self, tool_shed_repository: "ToolShedRepository", tool_index_sample_files): TOOL_DATA_TABLE_FILE_NAME = "tool_data_table_conf.xml" @@ -214,8 +226,11 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): SAMPLE_SUFFIX_OFFSET = -len(SAMPLE_SUFFIX) LOC_SAMPLE_SUFFIX = ".loc.sample" target_dir, tool_path, relative_target_dir = self.get_target_install_dir(tool_shed_repository) - # .loc files are shared across repository installations: install at the root of tool_data_path. - shared_loc_dir = self.app.config.tool_data_path + # Galaxy-managed loc files for shed-installed tools live under tool_data_path/shed/ so they + # are clearly separated from admin-configured loc files in tool_data_path and from any + # entries shipped via tool_data_table_conf.xml.sample. + shared_loc_dir = os.path.join(self.app.config.tool_data_path, "shed") + os.makedirs(shared_loc_dir, exist_ok=True) # Map shared loc basename -> source .loc.sample, used to merge entries when a table is reinstalled. loc_basename_to_source: dict[str, str] = {} for sample_file in tool_index_sample_files: @@ -280,9 +295,12 @@ class ShedToolDataTableManager(BaseShedToolDataTableManager): existing = registered_tables.get(table_name) if table_name else None if isinstance(existing, TabularToolDataTable) and existing.columns is not None: # Already registered with matching columns. Merge any rows from this install's - # .loc.sample(s) into the shared loc file, then skip adding a duplicate
. + # .loc.sample(s) into the shared loc file. self._merge_loc_sample_entries(existing, elem, loc_basename_to_source, tool_shed_repository) - continue + if self._shed_config_has_matching_entry(table_name, elem): + # An identical
entry already exists in shed_tool_data_table_config. + # Don't write another one (it would just duplicate the shared loc reference). + continue kept_elems.append(elem) if kept_elems: # Remove old data_table diff --git a/test/integration/test_repository_operations.py b/test/integration/test_repository_operations.py index 89a42106572..46880eae6fd 100644 --- a/test/integration/test_repository_operations.py +++ b/test/integration/test_repository_operations.py @@ -62,17 +62,22 @@ class TestRepositoryInstallIntegrationTestCase(integration_util.IntegrationTestC self.install_repository(*repo) self.uninstall_repository(*repo) - def test_non_data_manager_install_lands_loc_files_at_tool_data_path(self): - """Non-Data-Manager repos install their .loc files to tool_data_path root, and any -
entries written to shed_tool_data_table_conf.xml have no - sub-element. Tables that already exist in Galaxy's shipped tool_data_table_conf.xml.sample - are deduped (not re-added to shed_tool_data_table_conf.xml).""" + def test_non_data_manager_install_lands_loc_files_under_shed_subdir(self): + """Non-Data-Manager repos install their .loc files under tool_data_path/shed/ (keeping them + separate from admin-configured loc files at the tool_data_path root and from anything + shipped via tool_data_table_conf.xml.sample). Any
entries written to + shed_tool_data_table_conf.xml have no sub-element.""" non_dm_repo = ("devteam", "bwa", "051eba708f43") non_dm_loc_files = {"bwa_index.loc"} self.install_repository(*non_dm_repo) + shed_loc_dir = os.path.join(self._app.config.tool_data_path, "shed") for loc_file in non_dm_loc_files: - shared_loc = os.path.join(self._app.config.tool_data_path, loc_file) + shared_loc = os.path.join(shed_loc_dir, loc_file) assert os.path.exists(shared_loc), f"Expected shared loc file at {shared_loc}" + # The loc file should NOT also be written at the tool_data_path root. + assert not os.path.exists( + os.path.join(self._app.config.tool_data_path, loc_file) + ), f"Shed loc file should not land at tool_data_path root: {loc_file}" shed_conf = self._app.config.shed_tool_data_table_config if os.path.exists(shed_conf): for table_elem in ET.parse(shed_conf).getroot().findall("table"): diff --git a/test/unit/tool_shed/test_data_table_manager.py b/test/unit/tool_shed/test_data_table_manager.py index f0392d6f7dd..588632e0816 100644 --- a/test/unit/tool_shed/test_data_table_manager.py +++ b/test/unit/tool_shed/test_data_table_manager.py @@ -52,6 +52,7 @@ def _make_stdtm(tmp_path): repo_dir = str(tmp_path / "repo") tool_data_path = str(tmp_path / "tool-data") shed_tool_data_path = str(tmp_path / "shed_tool_data") + shed_tool_data_table_config = str(tmp_path / "shed_data_table_conf.xml") relative_target_dir = "owner/name/abc" os.makedirs(tool_data_path) @@ -62,6 +63,7 @@ def _make_stdtm(tmp_path): app = mock.MagicMock(name="app") app.config.tool_data_path = tool_data_path app.config.shed_tool_data_path = shed_tool_data_path + app.config.shed_tool_data_table_config = shed_tool_data_table_config registry = _FakeTableRegistry() app.tool_data_tables = registry captured = {"to_xml_calls": registry.to_xml_calls} @@ -82,20 +84,39 @@ def _make_stdtm(tmp_path): return stdtm, repo, sample_files, captured, tool_data_path, shed_tool_data_path, relative_target_dir -def _registered_table(columns, filenames=None): +def _registered_table(columns, filenames=None, type_key="tabular"): existing = mock.MagicMock(spec=TabularToolDataTable) existing.columns = columns existing.filenames = filenames or {} + existing.type_key = type_key existing.parse_file_fields.return_value = [] return existing -def test_loc_file_lands_at_shared_root_not_per_revision(tmp_path): +def _write_shed_config_with_entry(stdtm, table_name, file_path): + """Pre-populate ``shed_tool_data_table_config`` with a single ``
`` matching ``elem``.""" + shed_config = stdtm.app.config.shed_tool_data_table_config + contents = f""" + +
+ value, dbkey, name, path + +
+ +""" + with open(shed_config, "w") as fh: + fh.write(contents) + + +def test_loc_file_lands_under_shed_subdir_not_per_revision(tmp_path): stdtm, repo, samples, captured, tool_data_path, shed_tool_data_path, _ = _make_stdtm(tmp_path) _, kept_elems = stdtm.install_tool_data_tables(repo, samples) - shared_loc = os.path.join(tool_data_path, "all_fasta.loc") + shared_loc = os.path.join(tool_data_path, "shed", "all_fasta.loc") assert os.path.exists(shared_loc) + # The loc file does NOT land at the tool_data_path root (which is for admin-configured loc + # files) — Galaxy-managed shed loc files are isolated under tool_data_path/shed/. + assert not os.path.exists(os.path.join(tool_data_path, "all_fasta.loc")) per_rev_loc = os.path.join(shed_tool_data_path, "owner/name/abc", "all_fasta.loc") assert not os.path.exists(per_rev_loc) @@ -111,7 +132,8 @@ def test_loc_file_lands_at_shared_root_not_per_revision(tmp_path): def test_existing_loc_file_is_not_overwritten(tmp_path): stdtm, repo, samples, _, tool_data_path, _, _ = _make_stdtm(tmp_path) - shared_loc = os.path.join(tool_data_path, "all_fasta.loc") + shared_loc = os.path.join(tool_data_path, "shed", "all_fasta.loc") + os.makedirs(os.path.dirname(shared_loc), exist_ok=True) _write(shared_loc, "preexisting DM-populated content\n") stdtm.install_tool_data_tables(repo, samples) @@ -132,8 +154,11 @@ def test_column_mismatch_raises(tmp_path): assert not captured["to_xml_calls"] -def test_column_match_dedupes_without_writing(tmp_path): - stdtm, repo, samples, captured, _, _, _ = _make_stdtm(tmp_path) +def test_column_match_first_install_writes_table_entry(tmp_path): + """When a table is already registered in memory but has not yet been written to + ``shed_tool_data_table_config``, we still write a (stamp-less) ```` entry so the + shared loc file's association with the table survives reload.""" + stdtm, repo, samples, captured, tool_data_path, _, _ = _make_stdtm(tmp_path) matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} stdtm.app.tool_data_tables.data_tables = { "all_fasta": _registered_table(matching_columns), @@ -141,12 +166,31 @@ def test_column_match_dedupes_without_writing(tmp_path): _, kept_elems = stdtm.install_tool_data_tables(repo, samples) + assert len(kept_elems) == 1 + file_elems = list(kept_elems[0].findall("file")) + assert len(file_elems) == 1 + assert file_elems[0].get("path") == os.path.join(tool_data_path, "shed", "all_fasta.loc") + assert kept_elems[0].find("tool_shed_repository") is None + + +def test_column_match_subsequent_install_dedupes_shed_config_entry(tmp_path): + """If shed_tool_data_table_config already has a ``
`` with the same name and same + ````, don't write another one.""" + stdtm, repo, samples, captured, tool_data_path, _, _ = _make_stdtm(tmp_path) + matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} + stdtm.app.tool_data_tables.data_tables = { + "all_fasta": _registered_table(matching_columns), + } + _write_shed_config_with_entry(stdtm, "all_fasta", os.path.join(tool_data_path, "shed", "all_fasta.loc")) + + _, kept_elems = stdtm.install_tool_data_tables(repo, samples) + assert kept_elems == [] assert not captured["to_xml_calls"] -def test_column_match_with_column_elements_dedupes(tmp_path): - stdtm, repo, _, captured, _, _, _ = _make_stdtm(tmp_path) +def test_column_match_with_column_elements_writes_entry(tmp_path): + stdtm, repo, _, captured, tool_data_path, _, _ = _make_stdtm(tmp_path) column_form_conf = """\
@@ -168,16 +212,19 @@ def test_column_match_with_column_elements_dedupes(tmp_path): repo, ["tool_data_table_conf.xml.sample", os.path.join("tool-data", "all_fasta.loc.sample")], ) - assert kept_elems == [] + assert len(kept_elems) == 1 + assert kept_elems[0].find("tool_shed_repository") is None def test_second_install_merges_loc_sample_rows_with_attribution(tmp_path): - stdtm, repo, samples, captured, _, _, _ = _make_stdtm(tmp_path) + stdtm, repo, samples, captured, tool_data_path, _, _ = _make_stdtm(tmp_path) matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} existing = _registered_table(matching_columns) incoming_rows = [["hg19", "hg19", "human (hg19)", "/data/hg19.fa"]] existing.parse_file_fields.return_value = incoming_rows stdtm.app.tool_data_tables.data_tables = {"all_fasta": existing} + # Pre-populate shed config so kept_elems stays empty — we're testing the row-merge path. + _write_shed_config_with_entry(stdtm, "all_fasta", os.path.join(tool_data_path, "shed", "all_fasta.loc")) _, kept_elems = stdtm.install_tool_data_tables(repo, samples) @@ -192,11 +239,12 @@ def test_second_install_merges_loc_sample_rows_with_attribution(tmp_path): def test_second_install_with_empty_loc_sample_does_not_append(tmp_path): - stdtm, repo, samples, captured, _, _, _ = _make_stdtm(tmp_path) + stdtm, repo, samples, captured, tool_data_path, _, _ = _make_stdtm(tmp_path) matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} existing = _registered_table(matching_columns) existing.parse_file_fields.return_value = [] stdtm.app.tool_data_tables.data_tables = {"all_fasta": existing} + _write_shed_config_with_entry(stdtm, "all_fasta", os.path.join(tool_data_path, "shed", "all_fasta.loc")) _, kept_elems = stdtm.install_tool_data_tables(repo, samples) @@ -205,23 +253,20 @@ def test_second_install_with_empty_loc_sample_does_not_append(tmp_path): existing.append_entries_with_attribution.assert_not_called() -def test_second_install_registers_shared_loc_with_existing_table(tmp_path): - """When merging into an existing table whose ``filenames`` is empty (e.g. refgenie tables - or shipped tables whose loc resolution failed), the shared loc path must be registered so - Data Managers can persist entries.""" +def test_merge_skipped_for_non_tabular_table_types(tmp_path): + """Refgenie-backed (and other non-tabular) tables don't get their .loc.sample parsed + during install — refgenie's ``parse_file_fields`` expects YAML, not a .loc, so calling + it on a ``.loc.sample`` is incorrect.""" stdtm, repo, samples, _, tool_data_path, _, _ = _make_stdtm(tmp_path) matching_columns = {"value": 0, "dbkey": 1, "name": 2, "path": 3} - existing = _registered_table(matching_columns) - existing.parse_file_fields.return_value = [] + existing = _registered_table(matching_columns, type_key="refgenie") stdtm.app.tool_data_tables.data_tables = {"all_fasta": existing} + _write_shed_config_with_entry(stdtm, "all_fasta", os.path.join(tool_data_path, "shed", "all_fasta.loc")) - stdtm.install_tool_data_tables(repo, samples) - - shared_loc = os.path.join(tool_data_path, "all_fasta.loc") - assert shared_loc in existing.filenames - info = existing.filenames[shared_loc] - assert info["tool_shed_repository"] is None - assert info["from_shed_config"] is True + _, kept_elems = stdtm.install_tool_data_tables(repo, samples) + assert kept_elems == [] + existing.parse_file_fields.assert_not_called() + existing.append_entries_with_attribution.assert_not_called() def test_parse_table_columns_aliases_name_to_value(): From d73d47a86c5eb7289520f69945cd2a5ec5a4f212 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Mon, 18 May 2026 11:32:51 -0400 Subject: [PATCH 11/13] Fix bwa loc filename in integration test (bwa_mem_index, not bwa_index) The bwa@051eba708f43 repo only registers the bwa_mem_indexes table (bwa_mem_index.loc); the bwa_indexes table comes from a different repo. Use the actual filename the repo installs so the assertion can find the expected shed-managed loc file. --- test/integration/test_repository_operations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/test_repository_operations.py b/test/integration/test_repository_operations.py index 46880eae6fd..fadf5cdd92c 100644 --- a/test/integration/test_repository_operations.py +++ b/test/integration/test_repository_operations.py @@ -68,7 +68,7 @@ class TestRepositoryInstallIntegrationTestCase(integration_util.IntegrationTestC shipped via tool_data_table_conf.xml.sample). Any
entries written to shed_tool_data_table_conf.xml have no sub-element.""" non_dm_repo = ("devteam", "bwa", "051eba708f43") - non_dm_loc_files = {"bwa_index.loc"} + non_dm_loc_files = {"bwa_mem_index.loc"} self.install_repository(*non_dm_repo) shed_loc_dir = os.path.join(self._app.config.tool_data_path, "shed") for loc_file in non_dm_loc_files: From 85a72afca6cf1fa17c114364f18be03754c7396d Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Mon, 18 May 2026 13:25:35 -0400 Subject: [PATCH 12/13] Copy shed sample files to tool_data_path/shed/, not the root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `copy_sample_files` during tool registration was still landing shed-installed `.loc` files at the `tool_data_path` root alongside admin-configured ones — `install_tool_data_tables` had already been isolated under `tool_data_path/shed/`, but this second copy path was not. Direct both `copy_sample_files` invocations to the shed loc dir so all Galaxy-managed loc files stay isolated. --- .../tool_shed/galaxy_install/install_manager.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/tool_shed/galaxy_install/install_manager.py b/lib/galaxy/tool_shed/galaxy_install/install_manager.py index 2b3a1efa8d2..e92d020640f 100644 --- a/lib/galaxy/tool_shed/galaxy_install/install_manager.py +++ b/lib/galaxy/tool_shed/galaxy_install/install_manager.py @@ -187,10 +187,13 @@ class InstallRepositoryManager: ) sample_files = irmm_metadata_dict.get("sample_files", []) tool_index_sample_files = stdtm.get_tool_index_sample_files(sample_files) - tool_data_path = self.app.config.tool_data_path - # Used to be created as a side effect of install_tool_data_tables; that no longer runs for non-DM repos. - os.makedirs(tool_data_path, exist_ok=True) - tool_util.copy_sample_files(tool_data_path, tool_index_sample_files, tool_path=tool_path) + # Galaxy-managed loc files for shed-installed tools live under tool_data_path/shed/ + # so they're separated from admin-configured loc files at tool_data_path root. + shed_loc_dir = os.path.join(self.app.config.tool_data_path, "shed") + os.makedirs(shed_loc_dir, exist_ok=True) + tool_util.copy_sample_files( + shed_loc_dir, tool_index_sample_files, tool_path=tool_path + ) sample_files_copied = [str(s) for s in tool_index_sample_files] repository_tools_tups = irmm.get_repository_tools_tups() if repository_tools_tups: @@ -203,10 +206,9 @@ class InstallRepositoryManager: repository_tools_tups, sample_files_copied = tool_util.handle_missing_index_file( self.app, tool_path, sample_files, repository_tools_tups, sample_files_copied ) - # Copy remaining sample files included in the repository to the ~/tool-data directory of the - # local Galaxy instance. + # Copy remaining sample files included in the repository to the shed loc dir. tool_util.copy_sample_files( - tool_data_path, sample_files, tool_path=tool_path, sample_files_copied=sample_files_copied + shed_loc_dir, sample_files, tool_path=tool_path, sample_files_copied=sample_files_copied ) self.tpm.add_to_tool_panel( repository_name=tool_shed_repository.name, From 1daf8f3cc3eea12344c693931d6391e195bbe1b4 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Mon, 18 May 2026 14:55:16 -0400 Subject: [PATCH 13/13] Update toolshed test helper for shed/ loc subdir; fix black formatting verify_installed_repository_data_table_entries was looking for the installed loc file at tool_data_path root, but shed-installed loc files now live under tool_data_path/shed/. Also collapse a black-fmt-flagged multi-line copy_sample_files call onto one line. --- lib/galaxy/tool_shed/galaxy_install/install_manager.py | 4 +--- lib/tool_shed/test/base/testcase.py | 4 +++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/tool_shed/galaxy_install/install_manager.py b/lib/galaxy/tool_shed/galaxy_install/install_manager.py index e92d020640f..6cacd54e71b 100644 --- a/lib/galaxy/tool_shed/galaxy_install/install_manager.py +++ b/lib/galaxy/tool_shed/galaxy_install/install_manager.py @@ -191,9 +191,7 @@ class InstallRepositoryManager: # so they're separated from admin-configured loc files at tool_data_path root. shed_loc_dir = os.path.join(self.app.config.tool_data_path, "shed") os.makedirs(shed_loc_dir, exist_ok=True) - tool_util.copy_sample_files( - shed_loc_dir, tool_index_sample_files, tool_path=tool_path - ) + tool_util.copy_sample_files(shed_loc_dir, tool_index_sample_files, tool_path=tool_path) sample_files_copied = [str(s) for s in tool_index_sample_files] repository_tools_tups = irmm.get_repository_tools_tups() if repository_tools_tups: diff --git a/lib/tool_shed/test/base/testcase.py b/lib/tool_shed/test/base/testcase.py index f485e711fe1..0299ce3d52e 100644 --- a/lib/tool_shed/test/base/testcase.py +++ b/lib/tool_shed/test/base/testcase.py @@ -1709,7 +1709,9 @@ class ShedTestCase(ShedApiTestCase): ), f"The hard-coded Galaxy tool-data directory is missing for the {required_data_table_entry} entry." assert os.path.exists(galaxy_tool_data_dir), "The Galaxy tool-data directory does not exist." # Make sure the loc_file_name was correctly copied into the configured directory location. - configured_file_location = os.path.join(self.tool_data_path, loc_file_name) + # Shed-installed loc files now live under tool_data_path/shed/ to keep them + # separate from admin-configured ones. + configured_file_location = os.path.join(self.tool_data_path, "shed", loc_file_name) assert os.path.isfile( configured_file_location ), f'The expected copied file "{configured_file_location}" is missing.'