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():