From 8e8f5872094d9320b569ea7615e7c3c3beba7bbd Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 19 May 2026 13:02:21 +0200 Subject: [PATCH] LazyTool: restore entry-only to_dict fast path (no panel materialise) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With ``link_details`` no longer gating any field, the toolbox-internal panel / listing callers stopped passing it (``f64858bf55e``). ``LazyTool.to_dict`` can now return the entry-shape dict for the default-call case — every panel-view tool serves from the index, zero materialise. Materialise still fires when the caller asks for the parsed-Tool payload: ``io_details=True`` (the ``/api/tools/{id}`` show endpoint contract) or ``tool_help=True`` (rendered help). Failure-to-materialise still falls back to the entry-shape dict so the show endpoint never returns 500 when a specific tool's XML chokes the parameter factory. The ``link`` field is derived from ``self.id`` matching what ``Tool.to_dict`` now emits unconditionally — ``/tool_runner?tool_id={id}`` for the standard tool runner. Tests updated: - ``test_to_dict_entry_fast_path_does_not_materialise`` asserts the default call serves the entry dict and never invokes the callback. - ``test_to_dict_io_details_materialises`` covers the show contract. - ``test_to_dict_falls_back_to_entry_when_materialise_fails`` covers the parameter-factory-failure case. 23/23 unit tests pass. --- lib/galaxy/tools/lazy_toolbox.py | 36 ++++++++++++----------- test/unit/app/tools/test_lazy_tool.py | 41 +++++++++++++++++---------- 2 files changed, 45 insertions(+), 32 deletions(-) diff --git a/lib/galaxy/tools/lazy_toolbox.py b/lib/galaxy/tools/lazy_toolbox.py index e7ef1cebde0..2a8b09aeaca 100644 --- a/lib/galaxy/tools/lazy_toolbox.py +++ b/lib/galaxy/tools/lazy_toolbox.py @@ -291,30 +291,30 @@ class LazyTool: return True def to_dict(self, trans=None, link_details: bool = False, tool_help: bool = False, **kw) -> dict[str, Any]: - """API serialisation — always materialises. + """API serialisation. - The flat ``/api/tools?in_panel=False`` listing already bypasses - ``LazyTool.to_dict`` via ``services/tools.py:list_tools`` which - calls ``entry.to_api_dict()`` directly off the index — so the - only callers that reach this method are paths that need the - parsed payload: ``/api/tools/`` show (with or without - ``io_details``), the panel-view walk in ``to_panel_view`` / - ``ToolSection.to_dict``, and the per-tool ``Tool.to_dict`` reads - from filters / managers. + Entry-shape fast path covers the panel-view walk + (``AbstractToolBox.get_tool_to_dict`` no longer passes + ``link_details=True``; see commit ``f64858bf55e``) and the flat + ``/api/tools?in_panel=False`` listing handler. Materialise only + when the caller asks for ``io_details=True`` (the ``/api/tools/{id}`` + show endpoint, which needs ``inputs`` / ``outputs`` / parameter + tree) or ``tool_help=True`` (rendered help payload). On materialise failure (tool XML the parameter factory can't handle — ``upload_dataset``, ``column="value"`` against an - unresolvable column-name spec, …) we fall back to an entry-only + unresolvable column-name spec, …) we fall back to the entry-only dict so the caller still gets a renderable shape; eager mode catches the same in ``_load_tool_tag_set`` and drops the tool from ``_tools_by_id``. """ - try: - return self._materialize().to_dict( - trans, link_details=link_details, tool_help=tool_help, **kw - ) - except Exception as e: - log.warning("LazyTool.to_dict: materialise failed for %s, falling back to entry: %s", self.id, e) + if kw.get("io_details") or tool_help: + try: + return self._materialize().to_dict( + trans, link_details=link_details, tool_help=tool_help, **kw + ) + except Exception as e: + log.warning("LazyTool.to_dict: materialise failed for %s, falling back to entry: %s", self.id, e) entry = self._entry return { "id": self.id, @@ -328,7 +328,9 @@ class LazyTool: "model_class": "Tool", "panel_section_id": entry.panel_section_id, "panel_section_name": entry.panel_section_name, - "link": f"/api/tools/{self.id}", + # Cheap URL pattern — matches what ``Tool.to_dict`` now emits + # unconditionally for the materialised side. + "link": f"/tool_runner?tool_id={self.id}", } # --- materialisation --- diff --git a/test/unit/app/tools/test_lazy_tool.py b/test/unit/app/tools/test_lazy_tool.py index b2c51b43bff..48d21b5b9ad 100644 --- a/test/unit/app/tools/test_lazy_tool.py +++ b/test/unit/app/tools/test_lazy_tool.py @@ -109,28 +109,26 @@ def test_old_id_short_circuits_for_shed_ids(): assert _stub(_entry(id="local_tool")).old_id == "local_tool" -def test_to_dict_falls_back_to_entry_dict_when_materialise_fails(): - # ``LazyTool.to_dict`` always tries to materialise — the flat - # ``/api/tools?in_panel=False`` listing path bypasses it entirely via - # ``entry.to_api_dict()``, so every caller that reaches here needs the - # parsed payload. On materialise failure we return the entry-shape - # dict so the panel render / show endpoint still gets *something*. +def test_to_dict_entry_fast_path_does_not_materialise(): + # Default call (no ``io_details``, no ``tool_help``) is the panel-view + # path — the toolbox no longer asks for ``link_details``. Serve the + # entry-shape dict directly; never materialise. def boom(_e): - raise RuntimeError("materialise failed") + raise AssertionError(f"unexpected materialise for {_e.id!r}") t = LazyTool(_entry(), materialize_callback=boom, is_admin_user=lambda u: False) - d = t.to_dict(trans=None, link_details=False) + d = t.to_dict(trans=None) assert d["id"] == "bowtie2" assert d["model_class"] == "Tool" - assert d["link"] == "/api/tools/bowtie2" + assert d["link"] == "/tool_runner?tool_id=bowtie2" -def test_to_dict_materialises_exactly_once_per_call(): +def test_to_dict_io_details_materialises(): calls = [] class _Real: def to_dict(self, trans, link_details, tool_help, **kw): - calls.append("real-to_dict") + calls.append(("real-to_dict", kw.get("io_details"))) return {"id": "real"} def mat(_e): @@ -138,10 +136,23 @@ def test_to_dict_materialises_exactly_once_per_call(): return _Real() t = LazyTool(_entry(), materialize_callback=mat, is_admin_user=lambda u: False) - assert t.to_dict(trans=None, link_details=True) == {"id": "real"} - assert t.to_dict(trans=None, link_details=True) == {"id": "real"} - # First call materialises; second reuses ``_real``. - assert calls == ["mat", "real-to_dict", "real-to_dict"] + # ``io_details=True`` is the show-endpoint contract — materialise. + assert t.to_dict(trans=None, io_details=True) == {"id": "real"} + # Second call reuses cached ``_real``. + assert t.to_dict(trans=None, io_details=True) == {"id": "real"} + assert calls == ["mat", ("real-to_dict", True), ("real-to_dict", True)] + + +def test_to_dict_falls_back_to_entry_when_materialise_fails(): + # If a tool can't materialise (e.g. ``upload_dataset`` parameter factory + # failure) the show endpoint still gets the entry-shape dict. + def boom(_e): + raise RuntimeError("materialise failed") + + t = LazyTool(_entry(), materialize_callback=boom, is_admin_user=lambda u: False) + d = t.to_dict(trans=None, io_details=True) + assert d["id"] == "bowtie2" + assert d["model_class"] == "Tool" def test_allow_user_access_uses_index_data_without_materialise():