Reduce mercurial work in the tool shed's tool source and install info paths

lib/galaxy/tool_shed/util/hg_util.py

@@ -2,6 +2,11 @@
 import os
 import subprocess
 
+from mercurial import (
+    error,
+    scmutil,
+)
+
 from galaxy.tool_shed.util import basic_util
 from galaxy.util import unicodify
 
@@ -52,11 +57,21 @@ def copy_file_from_manifest(repo, changeset_revision, filename, dir):
 
 def get_changectx_for_changeset(repo, changeset_revision, **kwd):
     """Retrieve a specified changectx from a repository."""
-    for changeset in repo.changelog:
-        ctx = repo[changeset]
-        if str(ctx) == changeset_revision:
-            return ctx
-    return None
+    # repo[changeset_revision] cannot be used here, it resolves revision symbols and so would read an
+    # all-digit hash such as "123456789012" as a revision number.
+    try:
+        node = scmutil.resolvehexnodeidprefix(repo, changeset_revision.encode())
+    except (error.LookupError, error.WdirUnsupported):
+        return None
+    if node is None:
+        return None
+    ctx = repo[node]
+    # resolvehexnodeidprefix also matches shorter prefixes, and it resolves the null node - which
+    # INITIAL_CHANGELOG_HASH refers to and which is not part of the changelog. The changelog scan this
+    # replaces only ever matched a full changeset revision hash of a real changeset.
+    if ctx.rev() < 0 or str(ctx) != changeset_revision:
+        return None
+    return ctx
 
 
 def get_config_from_disk(config_file: str, relative_install_dir: str) -> str | None:

lib/tool_shed/managers/tools.py

@@ -11,10 +11,6 @@
 )
 from galaxy.tool_shed.metadata.metadata_generator import RepositoryMetadataToolDict
 from galaxy.tool_shed.util.basic_util import remove_dir
-from galaxy.tool_shed.util.hg_util import (
-    clone_repository,
-    get_changectx_for_changeset,
-)
 from galaxy.tool_util.model_factory import parse_tool_custom
 from galaxy.tool_util.parser import (
     get_tool_source,
@@ -27,7 +23,7 @@
     ProvidesRepositoriesContext,
     SessionRequestContext,
 )
-from tool_shed.util.common_util import generate_clone_url_for
+from tool_shed.util.hg_util import archive_repository_revision
 from tool_shed.webapp.model import RepositoryMetadata
 from tool_shed.webapp.search.tool_search import ToolSearch
 from tool_shed_client.schema import ShedParsedTool
@@ -97,23 +93,19 @@ def get_repository_metadata_tool_dict(
 
 
 def parsed_tool_model_cached_for(
-    trans: ProvidesRepositoriesContext, trs_tool_id: str, tool_version: str, repository_clone_url: str | None = None
+    trans: ProvidesRepositoriesContext, trs_tool_id: str, tool_version: str
 ) -> ShedParsedTool:
     model_cache = trans.app.model_cache
     parsed_tool = model_cache.get_cache_entry_for(ShedParsedTool, trs_tool_id, tool_version)
     if parsed_tool is not None:
         return parsed_tool
-    parsed_tool = parsed_tool_model_for(trans, trs_tool_id, tool_version, repository_clone_url=repository_clone_url)
+    parsed_tool = parsed_tool_model_for(trans, trs_tool_id, tool_version)
     model_cache.insert_cache_entry_for(parsed_tool, trs_tool_id, tool_version)
     return parsed_tool
 
 
-def parsed_tool_model_for(
-    trans: ProvidesRepositoriesContext, trs_tool_id: str, tool_version: str, repository_clone_url: str | None = None
-) -> ShedParsedTool:
-    tool_source, repository_metadata = tool_source_for(
-        trans, trs_tool_id, tool_version, repository_clone_url=repository_clone_url
-    )
+def parsed_tool_model_for(trans: ProvidesRepositoriesContext, trs_tool_id: str, tool_version: str) -> ShedParsedTool:
+    tool_source, repository_metadata = tool_source_for(trans, trs_tool_id, tool_version)
     parsed_tool = parse_tool_custom(tool_source, ShedParsedTool)
     if repository_metadata:
         revision_model = get_repository_revision_metadata_model(
@@ -124,10 +116,10 @@ def parsed_tool_model_for(
 
 
 def tool_source_for(
-    trans: ProvidesRepositoriesContext, trs_tool_id: str, tool_version: str, repository_clone_url: str | None = None
+    trans: ProvidesRepositoriesContext, trs_tool_id: str, tool_version: str
 ) -> tuple[ToolSource, RepositoryMetadata | None]:
     if "~" in trs_tool_id:
-        return _shed_tool_source_for(trans, trs_tool_id, tool_version, repository_clone_url)
+        return _shed_tool_source_for(trans, trs_tool_id, tool_version)
     else:
         tool_source = _stock_tool_source_for(trs_tool_id, tool_version)
         if tool_source is None:
@@ -136,28 +128,30 @@ def tool_source_for(
 
 
 def _shed_tool_source_for(
-    trans: ProvidesRepositoriesContext, trs_tool_id: str, tool_version: str, repository_clone_url: str | None = None
+    trans: ProvidesRepositoriesContext, trs_tool_id: str, tool_version: str
 ) -> tuple[ToolSource, RepositoryMetadata]:
     rval = get_repository_metadata_tool_dict(trans, trs_tool_id, tool_version)
     repository_metadata, tool_version_metadata = rval
     tool_config = tool_version_metadata["tool_config"]
 
-    repo = repository_metadata.repository.hg_repo
-    ctx = get_changectx_for_changeset(repo, repository_metadata.changeset_revision)
     work_dir = tempfile.mkdtemp(prefix="tmp-toolshed-tool_source")
-    if repository_clone_url is None:
-        repository_clone_url = generate_clone_url_for(trans, repository_metadata.repository)
     try:
-        cloned_ok, error_message = clone_repository(repository_clone_url, work_dir, str(ctx.rev()))
-        if error_message:
+        # Materialize the whole revision, not just the tool file - tool XML may import macros
+        # from sibling files in the repository.
+        archive_dir = os.path.join(work_dir, "repo")
+        try:
+            archive_repository_revision(
+                trans.app, repository_metadata.repository, archive_dir, repository_metadata.changeset_revision
+            )
+        except Exception:
             raise InternalServerError("Failed to materialize target repository revision")
         repo_files_dir = repository_metadata.repository.hg_repository_path(trans.app.config.file_path)
         if not repo_files_dir:
             raise InconsistentApplicationState(
                 f"Failed to resolve repository path from hgweb_config_manager for [{trs_tool_id}], inconsistent repository state or application configuration"
             )
         repo_rel_tool_path = relpath(tool_config, repo_files_dir)
-        path_to_tool = os.path.join(work_dir, repo_rel_tool_path)
+        path_to_tool = os.path.join(archive_dir, repo_rel_tool_path)
         if not os.path.exists(path_to_tool):
             raise InconsistentApplicationState(
                 f"Target tool expected at [{path_to_tool}] and not found, inconsistent repository state or application configuration"

lib/tool_shed/test/test_data/repos/column_maker_with_macros/0/column_maker.xml

@@ -0,0 +1,20 @@
+<tool id="Add_a_column_with_macros1" name="Compute" version="@TOOL_VERSION@">
+  <macros>
+    <import>macros.xml</import>
+  </macros>
+  <description>an expression on every row</description>
+  <command interpreter="python">
+    column_maker.py $input $out_file1 "$cond" $round ${input.metadata.columns} "${input.metadata.column_types}"
+  </command>
+  <inputs>
+    <param name="cond" size="40" type="text" value="c3-c2" label="Add expression"/>
+    <param format="tabular" name="input" type="data" label="as a new column to"/>
+    <expand macro="round_param"/>
+  </inputs>
+  <outputs>
+    <data format="input" name="out_file1" metadata_source="input"/>
+  </outputs>
+  <help>
+Adds a column to a dataset.
+  </help>
+</tool>

lib/tool_shed/test/test_data/repos/column_maker_with_macros/0/macros.xml

@@ -0,0 +1,9 @@
+<macros>
+  <token name="@TOOL_VERSION@">1.2.0</token>
+  <xml name="round_param">
+    <param name="round" type="select" label="Round result?">
+      <option value="no">NO</option>
+      <option value="yes">YES</option>
+    </param>
+  </xml>
+</macros>

lib/tool_shed/util/metadata_util.py

@@ -7,6 +7,7 @@
 from sqlalchemy import select
 
 from galaxy.tool_shed.util.hg_util import (
+    get_changectx_for_changeset,
     INITIAL_CHANGELOG_HASH,
     reversed_lower_upper_bounded_changelog,
 )
@@ -191,16 +192,17 @@ def get_next_downloadable_changeset_revision(app, repository, after_changeset_re
         changeset_revision = changeset_revisions[0]
         if changeset_revision == after_changeset_revision:
             return after_changeset_revision
-    found_after_changeset_revision = False
     repo = repository.hg_repo
-    for changeset in repo.changelog:
-        changeset_revision = str(repo[changeset])
-        if found_after_changeset_revision:
-            if changeset_revision in changeset_revisions:
-                return changeset_revision
-        elif changeset_revision == after_changeset_revision:
-            # We've found the changeset in the changelog for which we need to get the next downloadable changeset.
-            found_after_changeset_revision = True
+    after_ctx = get_changectx_for_changeset(repo, after_changeset_revision)
+    if after_ctx is None:
+        return None
+    # Order the downloadable revisions by their position in the changelog and return the first one
+    # past after_changeset_revision.  Note that RepositoryMetadata.numeric_revision cannot be used
+    # for this - it is not updated when a changeset_revision is moved ahead to a new repository tip.
+    downloadable_ctxs = [get_changectx_for_changeset(repo, cs) for cs in changeset_revisions]
+    for ctx in sorted((ctx for ctx in downloadable_ctxs if ctx is not None), key=lambda ctx: ctx.rev()):
+        if ctx.rev() > after_ctx.rev():
+            return str(ctx)
     return None
 
 

lib/tool_shed/util/repository_util.py

@@ -45,8 +45,8 @@
 )
 from tool_shed.util.common_util import generate_clone_url_for
 from tool_shed.util.hg_util import (
-    changeset2rev,
     create_hgrc_file,
+    get_changectx_for_changeset,
     get_hgrc_path,
     init_repository,
 )
@@ -307,8 +307,10 @@ def get_repo_info_dict(trans: "ProvidesRepositoriesContext", repository_id, chan
         has_repository_dependencies_only_if_compiling_contained_td = False
         includes_tool_dependencies = False
         includes_tools_for_display_in_tool_panel = False
-    repo_path = repository.repo_path(app)
-    ctx_rev = str(changeset2rev(repo_path, changeset_revision))
+    ctx = get_changectx_for_changeset(repository.hg_repo, changeset_revision)
+    if ctx is None:
+        raise Exception(f"Error looking for changeset '{changeset_revision}'")
+    ctx_rev = str(ctx.rev())
     repo_info_dict = create_repo_info_dict(
         app=app,
         repository_clone_url=repository_clone_url,

test/unit/tool_shed/test_hg_revision_lookup.py

@@ -0,0 +1,73 @@
+import pytest
+
+from galaxy.tool_shed.util.hg_util import (
+    get_changectx_for_changeset,
+    INITIAL_CHANGELOG_HASH,
+)
+from tool_shed.context import ProvidesRepositoriesContext
+from tool_shed.util.metadata_util import (
+    get_metadata_revisions,
+    get_next_downloadable_changeset_revision,
+)
+from tool_shed.util.repository_util import get_repo_info_dict
+from tool_shed.webapp.model import Repository
+from ._util import upload_directories_to_repository
+
+
+def changelog(repository: Repository) -> list[str]:
+    repo = repository.hg_repo
+    return [str(repo[changeset]) for changeset in repo.changelog]
+
+
+@pytest.mark.parametrize("fixture", ["column_maker", "column_maker_with_download_gaps"])
+def test_get_changectx_for_changeset(
+    provides_repositories: ProvidesRepositoriesContext, new_repository: Repository, fixture: str
+):
+    upload_directories_to_repository(provides_repositories, new_repository, fixture)
+    repo = new_repository.hg_repo
+    revisions = changelog(new_repository)
+    assert len(revisions) > 1
+
+    for expected_rev, changeset_revision in enumerate(revisions):
+        ctx = get_changectx_for_changeset(repo, changeset_revision)
+        assert ctx is not None
+        assert ctx.rev() == expected_rev
+
+    assert get_changectx_for_changeset(repo, INITIAL_CHANGELOG_HASH) is None
+    assert get_changectx_for_changeset(repo, "deadbeefdead") is None
+    assert get_changectx_for_changeset(repo, "zzzzzzzzzzzz") is None
+    assert get_changectx_for_changeset(repo, "") is None
+    # A prefix of a real hash is not a changeset revision.
+    assert get_changectx_for_changeset(repo, revisions[0][:4]) is None
+
+
+def test_next_downloadable_changeset_revision_follows_changelog_order(
+    provides_repositories: ProvidesRepositoriesContext, new_repository: Repository
+):
+    # This repository has a revision without installable metadata between two that have it, which
+    # leaves RepositoryMetadata.numeric_revision out of step with the real changelog position.
+    upload_directories_to_repository(provides_repositories, new_repository, "column_maker_with_download_gaps")
+    app = provides_repositories.app
+    revisions = changelog(new_repository)
+    downloadable = [changeset_revision for _rev, changeset_revision in get_metadata_revisions(app, new_repository)]
+
+    for position, changeset_revision in enumerate(revisions):
+        expected = next((cs for cs in revisions[position + 1 :] if cs in downloadable), None)
+        assert get_next_downloadable_changeset_revision(app, new_repository, changeset_revision) == expected
+
+    assert get_next_downloadable_changeset_revision(app, new_repository, "deadbeefdead") is None
+
+
+@pytest.mark.parametrize("fixture", ["column_maker", "column_maker_with_download_gaps"])
+def test_repo_info_dict_ctx_rev_is_the_mercurial_revision(
+    provides_repositories: ProvidesRepositoriesContext, new_repository: Repository, fixture: str
+):
+    upload_directories_to_repository(provides_repositories, new_repository, fixture)
+    app = provides_repositories.app
+    encoded_id = app.security.encode_id(new_repository.id)
+    revisions = changelog(new_repository)
+
+    for _rev, changeset_revision in get_metadata_revisions(app, new_repository):
+        repo_info_dict = get_repo_info_dict(provides_repositories, encoded_id, changeset_revision)[0]
+        ctx_rev = repo_info_dict[new_repository.name][3]
+        assert ctx_rev == str(revisions.index(changeset_revision))

test/unit/tool_shed/test_tool_source.py

@@ -14,25 +14,32 @@ def test_get_tool(provides_repositories: ProvidesRepositoriesContext, new_reposi
     name = new_repository.name
     encoded_id = f"{owner}~{name}~Add_a_column1"
 
-    repo_path = new_repository.repo_path(app=provides_repositories.app)
-    tool_source = tool_source_for(provides_repositories, encoded_id, "1.2.0", repository_clone_url=repo_path)[0]
+    tool_source = tool_source_for(provides_repositories, encoded_id, "1.2.0")[0]
     assert tool_source.parse_id() == "Add_a_column1"
-    bundle = parsed_tool_model_for(provides_repositories, encoded_id, "1.2.0", repository_clone_url=repo_path)
+    bundle = parsed_tool_model_for(provides_repositories, encoded_id, "1.2.0")
     assert len(bundle.inputs) == 3
 
-    cached_bundle = parsed_tool_model_cached_for(
-        provides_repositories, encoded_id, "1.2.0", repository_clone_url=repo_path
-    )
+    cached_bundle = parsed_tool_model_cached_for(provides_repositories, encoded_id, "1.2.0")
     assert len(cached_bundle.inputs) == 3
 
-    cached_bundle = parsed_tool_model_cached_for(
-        provides_repositories, encoded_id, "1.2.0", repository_clone_url=repo_path
-    )
+    cached_bundle = parsed_tool_model_cached_for(provides_repositories, encoded_id, "1.2.0")
     assert len(cached_bundle.inputs) == 3
 
 
+def test_get_tool_expands_macros_from_sibling_files(
+    provides_repositories: ProvidesRepositoriesContext, new_repository: Repository
+):
+    upload_directories_to_repository(provides_repositories, new_repository, "column_maker_with_macros")
+    owner = new_repository.user.username
+    name = new_repository.name
+    encoded_id = f"{owner}~{name}~Add_a_column_with_macros1"
+
+    tool_source = tool_source_for(provides_repositories, encoded_id, "1.2.0")[0]
+    assert tool_source.parse_version() == "1.2.0"
+    bundle = parsed_tool_model_for(provides_repositories, encoded_id, "1.2.0")
+    assert len(bundle.inputs) == 3
+
+
 def test_stock_bundle(provides_repositories: ProvidesRepositoriesContext):
-    cached_bundle = parsed_tool_model_cached_for(
-        provides_repositories, "__ZIP_COLLECTION__", "1.0.0", repository_clone_url=None
-    )
+    cached_bundle = parsed_tool_model_cached_for(provides_repositories, "__ZIP_COLLECTION__", "1.0.0")
     assert len(cached_bundle.inputs) == 2