Repository-level data-table linting for data-manager / reference-data bundles

lib/galaxy/tool_util/data/bundles/script.py

@@ -0,0 +1,83 @@
+#!/usr/bin/env python
+"""``galaxy-tool-data-lint`` -- lint a data-manager / reference-data repository.
+
+Runs the repository-level data-table linters over a repository directory: the same
+checks Planemo's ``shed_lint`` applies (missing loc fixtures, malformed loc rows,
+data-manager tables nothing configures, output_ref mismatches, duplicate / conflicting
+table schemas), runnable standalone without a Planemo install.
+"""
+
+import argparse
+import sys
+from json import dumps
+from typing import (
+    List,
+    Optional,
+)
+
+from galaxy.tool_util.data.bundles.lint import find_and_lint_repository_data_tables
+from galaxy.tool_util.lint import LintContext
+
+DESCRIPTION = """
+Lint the data tables of a data-manager / reference-data repository. Reports conditions
+that can be proven from the repository's own files (a referenced loc file is absent, a
+loc row cannot supply every declared column, a data manager populates an unconfigured
+table, and similar). Exits non-zero when the configured fail level is reached.
+"""
+
+REPORT_LEVELS = ("all", "valid", "info", "warn", "error")
+
+
+def arg_parser() -> argparse.ArgumentParser:
+    parser = argparse.ArgumentParser(description=DESCRIPTION)
+    parser.add_argument("repository", help="Path to the repository root directory to lint.")
+    parser.add_argument(
+        "-s",
+        "--skip",
+        default="",
+        help="Comma-separated list of linter names to skip (e.g. ConsumerTableDefined).",
+    )
+    parser.add_argument(
+        "--report-level",
+        choices=REPORT_LEVELS,
+        default="all",
+        help="Lowest message level to print (ignored with --json).",
+    )
+    parser.add_argument(
+        "--fail-level",
+        choices=("warn", "error"),
+        default="error",
+        help="Exit non-zero when a message at this level or above is emitted (default: error).",
+    )
+    parser.add_argument(
+        "-j",
+        "--json",
+        default=False,
+        action="store_true",
+        help="Emit the collected messages as JSON instead of printing them.",
+    )
+    return parser
+
+
+def lint(repository: str, skip: str, report_level: str, fail_level: str, json: bool) -> int:
+    skip_types = [name.strip() for name in skip.split(",") if name.strip()]
+    # In JSON mode dispatch at SILENT so the linters do not also print as they run;
+    # messages still accumulate in message_list for serialization.
+    level = "silent" if json else report_level
+    lint_ctx = LintContext(level, skip_types=skip_types)
+    find_and_lint_repository_data_tables(lint_ctx, repository)
+    if json:
+        messages = [{"level": m.level, "message": m.message, "linter": m.linter} for m in lint_ctx.message_list]
+        print(dumps({"messages": messages}, indent=2))
+    return 1 if lint_ctx.failed(fail_level) else 0
+
+
+def main(argv: Optional[List[str]] = None) -> None:
+    if argv is None:
+        argv = sys.argv[1:]
+    args = arg_parser().parse_args(argv)
+    sys.exit(lint(args.repository, args.skip, args.report_level, args.fail_level, args.json))
+
+
+if __name__ == "__main__":
+    main()

lib/galaxy/tool_util/lint.py

@@ -52,6 +52,7 @@
 from collections.abc import Callable
 from enum import IntEnum
 from typing import (
+    Generic,
     TYPE_CHECKING,
     TypeVar,
 )
@@ -65,9 +66,13 @@
 )
 
 if TYPE_CHECKING:
-    from galaxy.tool_util.parser.interface import ToolSource
     from galaxy.tool_util_models import UserToolSource
 
+# The object a linter classifies -- a ToolSource for the tool linters, a
+# RepositoryDataTables for the repository data-table linters, etc. LintContext.lint
+# dispatches over this generically, and Linter is generic over it.
+LintTargetType = TypeVar("LintTargetType")
+
 
 class LintLevel(IntEnum):
     SILENT = 5
@@ -78,15 +83,21 @@ class LintLevel(IntEnum):
     ALL = 0
 
 
-class Linter(ABC):
+class Linter(ABC, Generic[LintTargetType]):
     """
     a linter. needs to define a lint method and the code property.
     optionally a fix method can be given
+
+    Generic over the lint target so non-tool linters (e.g. the repository
+    data-table linters, which lint a ``RepositoryDataTables``) can subclass
+    ``Linter[SomeTarget]`` without violating the override contract. A bare
+    ``class Foo(Linter)`` is ``Linter[Any]`` -- the ordinary tool linters,
+    which annotate their own ``tool_source: ToolSource``.
     """
 
     @classmethod
     @abstractmethod
-    def lint(cls, tool_source: "ToolSource", lint_ctx: "LintContext"):
+    def lint(cls, tool_source: LintTargetType, lint_ctx: "LintContext"):
         """
         should add at most one message to the lint context
         """
@@ -105,6 +116,11 @@ def list_linters(cls) -> list[str]:
         list the names of all linter derived from Linter
         """
         submodules.import_submodules(galaxy.tool_util.linters)
+        # Repository data-table linters subclass Linter but live outside the
+        # tool_util.linters package; import here so they are always registered
+        # (function-level to avoid a circular import with this module).
+        from galaxy.tool_util.data.bundles import lint as _repo_lint  # noqa: F401
+
         return [s.__name__ for s in cls.__subclasses__()]
 
     list_listers: Callable[[], list[str]]  # deprecated alias
@@ -182,9 +198,6 @@ def __str__(self) -> str:
         return rval
 
 
-LintTargetType = TypeVar("LintTargetType")
-
-
 # TODO: Nothing inherently tool-y about LintContext and in fact
 # it is reused for repositories in planemo. Therefore, it should probably
 # be moved to galaxy.util.lint.

packages/tool_util/pyproject.toml

@@ -73,6 +73,7 @@ test = [
 galaxy-tool-format = "galaxy.tool_util.format:main"
 galaxy-tool-test = "galaxy.tool_util.verify.script:main"
 galaxy-tool-test-case-validation = "galaxy.tool_util.parameters.scripts.validate_test_cases:main"
+galaxy-tool-data-lint = "galaxy.tool_util.data.bundles.script:main"
 galaxy-tool-upgrade-advisor = "galaxy.tool_util.upgrade.script:main"
 validate-test-format = "galaxy.tool_util.validate_test_format:main"
 mulled-build = "galaxy.tool_util.deps.mulled.mulled_build:main"

test/unit/tool_util/data/repositories/.gitignore

@@ -0,0 +1,4 @@
+# These are test fixtures, not runtime Galaxy config. Re-include the config
+# filenames the top-level .gitignore drops so fixture repositories are complete.
+!data_manager_conf.xml
+!shed_data_manager_conf.xml

test/unit/tool_util/data/repositories/README.md

@@ -0,0 +1,76 @@
+# Repository data-table lint fixtures
+
+Miniature Tool Shed repositories used as **fixtures** (not runtime config) by the
+repository data-table linting tests. Each subdirectory is a self-contained repo
+laid out the way a real data-manager / reference-data repo is — `data_manager_conf.xml`,
+`tool_data_table_conf.xml.*`, `tool-data/*.loc.sample`, `test-data/*.loc`, consumer
+tool wrappers — so the tests build real `RepositoryDataTables` models over them with
+no mocks (`build_repository_data_tables(...)`).
+
+Consumed by:
+
+- `../test_repository_data_tables.py` — the parser/model builder
+- `../test_repository_data_table_lint.py` — the linters
+- `../test_data_lint_cli.py` — the `galaxy-tool-data-lint` CLI entry point
+
+## `.gitignore`
+
+Galaxy's top-level `.gitignore` drops `data_manager_conf.xml` and
+`shed_data_manager_conf.xml` as runtime config. Here those filenames are fixture
+content, so the local `.gitignore` re-includes (`!`) them — otherwise the fixture
+repos would be committed incomplete.
+
+## Fixtures
+
+### `fetch_genome_dbkeys_all_fasta/` — the clean, full repo
+
+The one well-formed repo (modeled on IUC's fetch-genome / `all_fasta` data manager);
+the happy-path baseline where every linter is expected to pass. Ships extra
+`data_manager_conf_*.xml` variants and two consumer wrappers so a single realistic
+repo can drive the error cases without a repo-per-case:
+
+- `data_manager_conf.xml` — clean manager (`all_fasta`, `__dbkeys__`)
+- `data_manager_conf_bad_output_ref.xml` — `output_ref` to a nonexistent output → `OutputRefValid`
+- `data_manager_conf_mixed_output_ref.xml` — one good + one bad ref (per-table iteration)
+- `data_manager_conf_missing_wrapper.xml` — `tool_file` points at a missing wrapper (outputs unresolved → not flagged)
+- `data_manager_conf_nested_tool.xml` — nested `<tool>` element form (output resolution)
+- `tools/consume_all_fasta.xml` — consumer with a **literal** `from_data_table`
+- `tools/consume_dynamic_table.xml` — consumer whose `from_data_table` stays **non-literal** after macro expansion (false-positive guard, tools-iuc#5003)
+
+### Missing loc fixtures — `MissingLocFixture`
+
+- `missing_loc/` — conf references a `.loc` that does not exist → one error
+- `missing_two/` — two missing locs → two errors
+- `sample_fallback/` — production loc resolved via the loader's own `.sample` fallback (`foo.loc.sample`); must **not** be reported missing
+- `tool_data_sample/` — Tool Shed layout: conf → `tool-data/bar.loc`, sample ships as `tool-data/bar.loc.sample`. The loader's `.sample` fallback misses this; `sample_backed` must recognize it so reference-data repos aren't falsely flagged.
+
+### Row-shape fixtures — `LocRowShape`
+
+- `broken_rows/` — `broken.loc` with a too-short row and a wrong-separator row → two errors
+- `missing_and_broken/` — one missing loc **and** one broken loc; both linters fire and no false "rows are fine" green is emitted off the unparsed missing file
+
+### Empty-loc fixture — `EmptyLocFile`
+
+- `empty_loc/` — ships an empty, comment-less `tool-data/undocumented.loc.sample`
+  (the Planemo #869 case → one warning) alongside a header-only
+  `tool-data/documented.loc.sample` that must **not** be flagged (a dataless file
+  with a format comment is the accepted convention).
+
+### Schema-conflict / duplicate fixtures — `DuplicateColumnNames`, `ConflictingTableSchema`
+
+Each declares the same table twice (or with a repeated column) in a way that would
+make the loader's merge raise; assembly must skip the loader and still report cleanly.
+
+- `dup_columns/` — duplicate column name within one table → `DuplicateColumnNames`
+- `conflicting_columns/` — same table, different column sets → `ConflictingTableSchema`
+- `conflicting_indexes/` — same column names, different index attributes → `ConflictingTableSchema`
+- `conflicting_separator/` — same table, different separators → `ConflictingTableSchema`
+
+### Core-table exclusion fixture — `find_and_lint_repository_data_tables`
+
+- `core_table_consumer/` — an index-builder data manager (modeled on IUC
+  `data_manager_bwa_mem2_index_builder`) that defines its own `bwa_mem2_indexes`
+  table but consumes the core `all_fasta` table via `from_data_table`. The one-call
+  `find_and_lint` entry seeds `DEFAULT_EXTERNAL_TABLE_NAMES` (`all_fasta`,
+  `fasta_indexes`, `__dbkeys__`), so the core reference must **not** warn; linted
+  without that seeding it does, proving the exclusion is what suppresses it.

test/unit/tool_util/data/repositories/broken_rows/test-data/broken.loc

@@ -0,0 +1,4 @@
+# value	name	path
+val1	name1	/data/path1
+val2	name2
+val3,name3,/data/path3

test/unit/tool_util/data/repositories/broken_rows/tool_data_table_conf.xml.test

@@ -0,0 +1,6 @@
+<tables>
+    <table name="broken" comment_char="#">
+        <columns>value, name, path</columns>
+        <file path="${__HERE__}/test-data/broken.loc" />
+    </table>
+</tables>

test/unit/tool_util/data/repositories/conflicting_columns/tool_data_table_conf.xml.test

@@ -0,0 +1,11 @@
+<tables>
+    <!-- Same table name declared twice with different columns. The loader's merge
+         would raise on this; the linter reports it from the raw declarations and
+         bundle assembly must not crash. -->
+    <table name="conflict_tbl" comment_char="#">
+        <columns>value, name</columns>
+    </table>
+    <table name="conflict_tbl" comment_char="#">
+        <columns>value, name, path</columns>
+    </table>
+</tables>

test/unit/tool_util/data/repositories/conflicting_indexes/tool_data_table_conf.xml.test

@@ -0,0 +1,14 @@
+<tables>
+    <!-- Same table name, same column NAMES, but different index attributes via the
+         explicit <column> form. The raw name tuples are identical, so a name-based
+         conflict check would miss this and the loader's merge would then crash; the
+         columns map (name->index) differs, so the map-based check catches it. -->
+    <table name="idx_tbl" comment_char="#">
+        <column name="value" index="0" />
+        <column name="path" index="1" />
+    </table>
+    <table name="idx_tbl" comment_char="#">
+        <column name="value" index="0" />
+        <column name="path" index="5" />
+    </table>
+</tables>

test/unit/tool_util/data/repositories/conflicting_separator/tool_data_table_conf.xml.test

@@ -0,0 +1,10 @@
+<tables>
+    <!-- Same table name and columns, but a different separator. The loader accepts
+         this (it only checks columns on merge), yet the schema is ambiguous. -->
+    <table name="sep_tbl" comment_char="#">
+        <columns>value, path</columns>
+    </table>
+    <table name="sep_tbl" comment_char="#" separator=",">
+        <columns>value, path</columns>
+    </table>
+</tables>

test/unit/tool_util/data/repositories/core_table_consumer/data_manager/bwa_mem2_index_builder.xml

@@ -0,0 +1,56 @@
+<tool id="bwa_mem2_index_builder_data_manager" name="BWA-MEM2 index builder" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" tool_type="manage_data" profile="23.0">
+    <description></description>
+    <macros>
+        <token name="@TOOL_VERSION@">2.2.1</token>
+        <token name="@VERSION_SUFFIX@">2</token>
+    </macros>
+    <requirements>
+        <requirement type="package" version="@TOOL_VERSION@">bwa-mem2</requirement>
+    </requirements>
+    <command detect_errors="exit_code"><![CDATA[
+        #set $fasta_file_name = str($all_fasta_source.fields.path).split('/')[-1]
+        mkdir -p '${out_file.extra_files_path}' &&
+        ln -s '${all_fasta_source.fields.path}' '${out_file.extra_files_path}/${fasta_file_name}' &&
+        bwa-mem2 index '${out_file.extra_files_path}/${fasta_file_name}' &&
+        rm '${out_file.extra_files_path}/${fasta_file_name}' &&
+        cp '$dmjson' '$out_file'
+        ]]>
+    </command>
+    <configfiles>
+        <configfile name="dmjson"><![CDATA[#slurp
+#set $fasta_file_name = str($all_fasta_source.fields.path).split('/')[-1]
+#set $value = $sequence_id or $all_fasta_source.fields.dbkey
+#set $name = $sequence_name or $all_fasta_source.fields.name
+{
+  "data_tables":{
+    "bwa_mem2_indexes":[
+      {
+        "value": "${value}",
+        "dbkey": "${all_fasta_source.fields.dbkey}",
+        "name": "${name}",
+        "path": "${fasta_file_name}"
+      }
+    ]
+  }
+}
+]]></configfile>
+    </configfiles>
+    <inputs>
+        <param name="all_fasta_source" type="select" label="Source FASTA Sequence">
+            <options from_data_table="all_fasta"/>
+        </param>
+        <param name="sequence_name" type="text" value="" label="Name of sequence" />
+        <param name="sequence_id" type="text" value="" label="ID for sequence" />
+    </inputs>
+    <outputs>
+        <data name="out_file" format="data_manager_json" />
+    </outputs>
+    <help>
+<![CDATA[
+Builds a BWA-MEM2 index from a FASTA sequence chosen from the core all_fasta table.
+]]>
+    </help>
+    <citations>
+        <citation type="doi">10.1038/nmeth.3317</citation>
+    </citations>
+</tool>

test/unit/tool_util/data/repositories/core_table_consumer/data_manager_conf.xml

@@ -0,0 +1,19 @@
+<?xml version="1.0"?>
+<data_managers>
+    <data_manager tool_file="data_manager/bwa_mem2_index_builder.xml" id="bwa_mem2_index_builder">
+        <data_table name="bwa_mem2_indexes">
+            <output>
+                <column name="value" />
+                <column name="dbkey" />
+                <column name="name" />
+                <column name="path" output_ref="out_file" >
+                    <move type="directory" relativize_symlinks="True">
+                        <target base="${GALAXY_DATA_MANAGER_DATA_PATH}">genomes/${dbkey}/bwa_mem_index/v2/${value}</target>
+                    </move>
+                    <value_translation>${GALAXY_DATA_MANAGER_DATA_PATH}/genomes/${dbkey}/bwa_mem_index/v2/${value}/${path}</value_translation>
+                    <value_translation type="function">abspath</value_translation>
+                </column>
+            </output>
+        </data_table>
+    </data_manager>
+</data_managers>

test/unit/tool_util/data/repositories/core_table_consumer/tool-data/bwa_mem2_index.loc.sample

@@ -0,0 +1,10 @@
+#This is a sample file distributed with Galaxy that enables tools
+#to use a directory of BWA-MEM2 indexed sequences data files. The loc
+#file has this format (white space characters are TAB characters):
+#
+#<unique_build_id>	<dbkey>	<display_name>	<file_path>
+#
+#So, for example, if you had phiX indexed and stored in
+#/depot/data2/galaxy/phiX/base/, the entry would look like this:
+#
+#phiX174	phiX	phiX Pretty	/depot/data2/galaxy/phiX/base/phiX.fa

test/unit/tool_util/data/repositories/core_table_consumer/tool_data_table_conf.xml.sample

@@ -0,0 +1,7 @@
+<tables>
+    <!-- Locations of indexes in the BWA-MEM2 mapper format-->
+    <table name="bwa_mem2_indexes" comment_char="#">
+        <columns>value, dbkey, name, path</columns>
+        <file path="tool-data/bwa_mem2_index.loc" />
+    </table>
+</tables>

test/unit/tool_util/data/repositories/dup_columns/tool_data_table_conf.xml.test

@@ -0,0 +1,7 @@
+<tables>
+    <!-- 'value' is declared twice. The parsed columns dict collapses it, so the
+         duplicate is only visible in the raw declared column list. -->
+    <table name="dup_cols" comment_char="#">
+        <columns>value, value, path</columns>
+    </table>
+</tables>

test/unit/tool_util/data/repositories/empty_loc/tool-data/documented.loc.sample

@@ -0,0 +1 @@
+#<value>	<path>