From a5deee57aca184bbe36ce46ecb1db3f4a535e93e Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Wed, 13 May 2026 14:41:51 -0400 Subject: [PATCH] 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)