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)
`<table>` 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`).

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 ``<table>`` with the same
+        ``name`` and identical set of ``<file path>`` 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 ``<file>`` entry (e.g. ``__dbkeys__``).
+        Used to avoid writing duplicate ``<table>`` 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 <table>.
+                # .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 <table> 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

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
-        <table> entries written to shed_tool_data_table_conf.xml have no <tool_shed_repository>
-        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 <table> entries written to
+        shed_tool_data_table_conf.xml have no <tool_shed_repository> 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"):

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 ``<table>`` matching ``elem``."""
+    shed_config = stdtm.app.config.shed_tool_data_table_config
+    contents = f"""<?xml version="1.0"?>
+<tables>
+    <table name="{table_name}" comment_char="#">
+        <columns>value, dbkey, name, path</columns>
+        <file path="{file_path}" />
+    </table>
+</tables>
+"""
+    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) ``<table>`` 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 ``<table>`` with the same name and same
+    ``<file path>``, 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 = """\
 <tables>
     <table name="all_fasta" comment_char="#">
@@ -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():