diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts index f02125d0832a..718588460726 100644 --- a/client/src/api/schema/schema.ts +++ b/client/src/api/schema/schema.ts @@ -23754,7 +23754,23 @@ export interface components { * @example 0123456789ABCDEF */ id: string; - representation: components["schemas"]["UserToolSource-Output"]; + /** Representation */ + representation: + | components["schemas"]["UserToolSource-Output"] + | { + [key: string]: unknown; + }; + /** + * Representation Errors + * @default [] + */ + representation_errors: string[]; + /** + * Representation Status + * @default ok + * @enum {string} + */ + representation_status: "ok" | "lifted" | "invalid"; /** Tool Format */ tool_format: string | null; /** Tool Id */ diff --git a/client/src/components/Panels/UserToolPanel.vue b/client/src/components/Panels/UserToolPanel.vue index 0afb120457e3..301509f7f077 100644 --- a/client/src/components/Panels/UserToolPanel.vue +++ b/client/src/components/Panels/UserToolPanel.vue @@ -50,12 +50,21 @@ const currentItemId = computed(() => { return match ? match[0] : undefined; }); +function repField(tool: UnprivilegedToolResponse, key: string): string | undefined { + // representation may be a typed UserToolSource (status ok/lifted) or a raw + // dict (status invalid). Both shapes still hold name/id/etc by string key, + // but the union type loses property access — read defensively. + const rep = tool.representation as Record | undefined; + const value = rep?.[key]; + return typeof value === "string" ? value : undefined; +} + function cardClicked(tool: UnprivilegedToolResponse) { if (props.inPanel) { emit("unprivileged-tool-clicked", tool); } if (props.inWorkflowEditor) { - emit("onInsertTool", tool.representation.id, tool.representation.name, tool.uuid); + emit("onInsertTool", repField(tool, "id"), repField(tool, "name"), tool.uuid); } else { router.push(`/?tool_uuid=${tool.uuid}`); } @@ -78,13 +87,31 @@ function newTool() { } function getToolBadges(tool: UnprivilegedToolResponse) { - return [ + const badges = [ { id: "version", - label: tool.representation.version ?? "", + label: repField(tool, "version") ?? "", title: "Version of this custom tool", }, ]; + if (tool.representation_status === "lifted") { + badges.push({ + id: "status", + label: "needs update", + title: + "This tool's stored definition uses conventions that are no longer valid; " + + "they are ignored on read. Re-save the tool to clean it up.", + }); + } else if (tool.representation_status === "invalid") { + badges.push({ + id: "status", + label: "schema error", + title: + "This tool's stored definition does not satisfy the current schema. " + + "Open it in the editor to repair.", + }); + } + return badges; } function getToolSecondaryActions(tool: UnprivilegedToolResponse) { @@ -141,7 +168,7 @@ function getToolSecondaryActions(tool: UnprivilegedToolResponse) { :active="tool.uuid === currentItemId" :badges="getToolBadges(tool)" :secondary-actions="getToolSecondaryActions(tool)" - :title="tool.representation.name" + :title="repField(tool, 'name') ?? tool.uuid" :title-icon="{ icon: faWrench }" title-size="text" :update-time="tool.create_time" @@ -150,7 +177,7 @@ function getToolSecondaryActions(tool: UnprivilegedToolResponse) { diff --git a/client/src/composables/agentActions.ts b/client/src/composables/agentActions.ts index 61c6558142bc..66da1a16696a 100644 --- a/client/src/composables/agentActions.ts +++ b/client/src/composables/agentActions.ts @@ -144,7 +144,8 @@ export function useAgentActions() { return; } - toast.success(`Tool "${data.representation.name}" saved successfully!`); + const repName = (data.representation as Record | undefined)?.name ?? data.uuid; + toast.success(`Tool "${String(repName)}" saved successfully!`); unprivilegedToolStore.load(true); router.push(`/tools/editor/${data.uuid}`); } catch (err) { diff --git a/lib/galaxy/tool_util_models/__init__.py b/lib/galaxy/tool_util_models/__init__.py index fb0f6629de2a..df99f75a51e9 100644 --- a/lib/galaxy/tool_util_models/__init__.py +++ b/lib/galaxy/tool_util_models/__init__.py @@ -9,6 +9,7 @@ Dict, List, Optional, + Tuple, Union, ) @@ -21,6 +22,7 @@ model_validator, RootModel, Tag, + ValidationError, ) from typing_extensions import ( Annotated, @@ -155,6 +157,95 @@ class YamlToolSource(_DynamicToolSourceBase): DynamicToolSources = Annotated[Union[UserToolSource, YamlToolSource], Field(discriminator="class_")] +# --------------------------------------------------------------------------- +# Schema-drift "lift" for stored DynamicTool.value rows. +# +# The strict `UserToolSource` schema is the single source of truth for both +# input and output. Stored rows from older Galaxy versions may carry fields +# that the current schema rejects (e.g. legacy internal-model fields after the +# YAML narrowing) or values that fail tightened constraints (e.g. a future +# `container` regex). `lift_user_tool_source` validates against the strict +# schema and: +# - on success: returns ("ok", parsed_model, []). +# - on `extra_forbidden`-only failure: strips the offending paths and +# re-validates. Returns ("lifted", parsed_model, dropped_paths). +# - on any other failure: returns ("invalid", original_dict, error_summary). +# The endpoint exposes this so legacy/broken rows don't crash the API. +# --------------------------------------------------------------------------- + +LiftStatus = Literal["ok", "lifted", "invalid"] + + +def _navigable_path(value: Any, loc: tuple) -> Tuple[Optional[Any], List[Any]]: + """Walk `loc` against the structure of `value`, skipping steps that don't + correspond to a real key/index (pydantic inserts discriminator literals + like `"data"` into the loc for tagged unions). Returns the parent + container of the leaf and the cleaned path components, or (None, []) if + the path can't be resolved.""" + cur: Any = value + *prefix, leaf = loc + cleaned: List[Any] = [] + for step in prefix: + if isinstance(cur, list) and isinstance(step, int) and 0 <= step < len(cur): + cur = cur[step] + cleaned.append(step) + elif isinstance(cur, dict) and step in cur: + cur = cur[step] + cleaned.append(step) + # else: skip — discriminator tag or stale path + cleaned.append(leaf) + return cur, cleaned + + +def _format_loc(value: Any, loc: tuple) -> str: + _parent, cleaned = _navigable_path(value, loc) + return ".".join(str(p) for p in cleaned if p != "representation") + + +def _strip_path(value: dict, loc: tuple) -> bool: + """Remove the field at the given pydantic error `loc` from `value`. Returns + True if a key was actually removed.""" + parent, cleaned = _navigable_path(value, loc) + if not cleaned or not isinstance(parent, dict): + return False + leaf = cleaned[-1] + if leaf in parent: + del parent[leaf] + return True + return False + + +def lift_user_tool_source( + value: dict, +) -> Tuple[LiftStatus, Union["UserToolSource", Dict[str, Any]], List[str]]: + """Validate `value` against the strict UserToolSource, lifting drift where + safe. See module docstring above for the contract. + """ + import copy + + try: + return ("ok", UserToolSource.model_validate(value), []) + except ValidationError as e: + errors = e.errors() + + extra_forbidden = [err for err in errors if err.get("type") == "extra_forbidden"] + other = [err for err in errors if err.get("type") != "extra_forbidden"] + if extra_forbidden and not other: + stripped = copy.deepcopy(value) + dropped: List[str] = [] + for err in extra_forbidden: + loc = tuple(err["loc"]) + if _strip_path(stripped, loc): + dropped.append(_format_loc(value, loc)) + try: + return ("lifted", UserToolSource.model_validate(stripped), dropped) + except ValidationError as e2: + errors = e2.errors() + + summary = [f"{_format_loc(value, tuple(err['loc']))}: {err.get('msg', err.get('type', ''))}" for err in errors] + return ("invalid", value, summary) + + class ParsedTool(ToolSourceBaseModel): id: str version: Optional[str] diff --git a/lib/galaxy/webapps/galaxy/api/dynamic_tools.py b/lib/galaxy/webapps/galaxy/api/dynamic_tools.py index 5c7baec6a6fd..011c743ca5f6 100644 --- a/lib/galaxy/webapps/galaxy/api/dynamic_tools.py +++ b/lib/galaxy/webapps/galaxy/api/dynamic_tools.py @@ -2,10 +2,12 @@ from datetime import datetime from typing import ( Any, + Literal, Optional, Union, ) +from fastapi import Response from pydantic import BaseModel from galaxy.exceptions import ( @@ -29,7 +31,10 @@ from galaxy.tool_util.parameters import input_models_for_tool_source from galaxy.tool_util.parameters.convert import cwl_runtime_model from galaxy.tool_util.parser.yaml import YamlToolSource -from galaxy.tool_util_models import UserToolSource +from galaxy.tool_util_models import ( + lift_user_tool_source, + UserToolSource, +) from galaxy.tool_util_models.dynamic_tool_models import ( DynamicToolPayload, DynamicUnprivilegedToolCreatePayload, @@ -48,6 +53,22 @@ DatabaseIdOrUUID = Union[DecodedDatabaseIdField, str] +def _set_lift_headers(response: Response, status: str, errors: list[str]) -> None: + """Surface lift status as response headers so consumers can react without + parsing the response body. Headers are computed dynamically per request and + are intentionally not declared in the OpenAPI schema.""" + if status == "ok" or not errors: + return + if status == "lifted": + compact = ",".join(errors) + response.headers["X-Galaxy-Deprecated-Fields"] = compact + response.headers["Warning"] = f'299 - "Some conventions are no longer valid; ignored on read: {compact}"' + elif status == "invalid": + compact = "; ".join(errors) + response.headers["X-Galaxy-Schema-Errors"] = compact + response.headers["Warning"] = f'299 - "Stored tool no longer satisfies schema: {compact}"' + + class UnprivilegedToolResponse(BaseModel): id: EncodedDatabaseIdField uuid: str @@ -56,7 +77,32 @@ class UnprivilegedToolResponse(BaseModel): tool_id: Optional[str] tool_format: Optional[str] create_time: datetime - representation: UserToolSource + # Either a strict UserToolSource (status="ok" or "lifted") or the raw + # stored dict (status="invalid"). Consumers narrow on `representation_status`. + representation: Union[UserToolSource, dict[str, Any]] + representation_status: Literal["ok", "lifted", "invalid"] = "ok" + representation_errors: list[str] = [] + + +def _build_unprivileged_tool_response(d: dict[str, Any]) -> UnprivilegedToolResponse: + """Run the lift over the stored representation and assemble the response. + This is the single place where the lift result is unpacked, so the helper's + invariants (status='ok' → strict model, 'lifted' → strict model + dropped + paths, 'invalid' → raw dict + error summary) are enforced exactly once.""" + raw_representation = d.get("representation") or {} + status, representation, errors = lift_user_tool_source(raw_representation) + return UnprivilegedToolResponse( + id=d["id"], + uuid=d["uuid"], + active=d["active"], + hidden=d["hidden"], + tool_id=d.get("tool_id"), + tool_format=d.get("tool_format"), + create_time=d["create_time"], + representation=representation, + representation_status=status, + representation_errors=errors, + ) @router.cbv @@ -66,17 +112,38 @@ class UnprivilegedToolsApi: dynamic_tools_manager: DynamicToolManager = depends(DynamicToolManager) @router.get("/api/unprivileged_tools", response_model_exclude_defaults=True) - def index(self, active: bool = True, trans: ProvidesUserContext = DependsOnTrans) -> list[UnprivilegedToolResponse]: + def index( + self, + response: Response, + active: bool = True, + trans: ProvidesUserContext = DependsOnTrans, + ) -> list[UnprivilegedToolResponse]: if not trans.user: return [] - return [t.to_dict() for t in self.dynamic_tools_manager.list_unprivileged_tools(trans.user, active=active)] + result = [ + _build_unprivileged_tool_response(t.to_dict()) + for t in self.dynamic_tools_manager.list_unprivileged_tools(trans.user, active=active) + ] + # Aggregate header: any tool with drift surfaces it on the index call. + worst = "ok" + aggregate: list[str] = [] + for tool in result: + if tool.representation_status == "invalid": + worst = "invalid" + elif tool.representation_status == "lifted" and worst == "ok": + worst = "lifted" + aggregate.extend(tool.representation_errors) + _set_lift_headers(response, worst, aggregate) + return result @router.get("/api/unprivileged_tools/{uuid}", response_model_exclude_defaults=True) - def show(self, uuid: str, user: User = DependsOnUser) -> UnprivilegedToolResponse: + def show(self, response: Response, uuid: str, user: User = DependsOnUser) -> UnprivilegedToolResponse: dynamic_tool = self.dynamic_tools_manager.get_unprivileged_tool_by_uuid(user, uuid) if dynamic_tool is None: raise ObjectNotFound() - return UnprivilegedToolResponse(**dynamic_tool.to_dict()) + tool = _build_unprivileged_tool_response(dynamic_tool.to_dict()) + _set_lift_headers(response, tool.representation_status, tool.representation_errors) + return tool @router.post("/api/unprivileged_tools", response_model_exclude_defaults=True) def create( @@ -86,7 +153,10 @@ def create( user, payload, ) - return UnprivilegedToolResponse(**dynamic_tool.to_dict()) + # Just-created tools are validated through strict input, so the lift + # is a no-op here, but going through the same builder keeps behavior + # uniform. + return _build_unprivileged_tool_response(dynamic_tool.to_dict()) @router.post("/api/unprivileged_tools/build") def build( diff --git a/lib/galaxy/webapps/galaxy/api/tools.py b/lib/galaxy/webapps/galaxy/api/tools.py index ae0412b0fc45..ff7fc3134e7b 100644 --- a/lib/galaxy/webapps/galaxy/api/tools.py +++ b/lib/galaxy/webapps/galaxy/api/tools.py @@ -63,7 +63,10 @@ ToolParameterT, ) from galaxy.tool_util.verify import ToolTestDescriptionDict -from galaxy.tool_util_models import UserToolSource +from galaxy.tool_util_models import ( + lift_user_tool_source, + UserToolSource, +) from galaxy.tools.evaluation import global_tool_errors from galaxy.tools.fetch.workbooks import ( FetchWorkbookCollectionType, @@ -878,7 +881,24 @@ def raw_tool_source(self, trans: GalaxyWebTransaction, id, **kwds): trans.response.headers["language"] = tool.tool_source.language if dynamic_tool := getattr(tool, "dynamic_tool", None): if dynamic_tool.value.get("class") == "GalaxyUserTool": - return UserToolSource(**dynamic_tool.value).model_dump_json( + status, lifted, errors = lift_user_tool_source(dynamic_tool.value) + if status == "lifted" and errors: + compact = ",".join(errors) + trans.response.headers["X-Galaxy-Deprecated-Fields"] = compact + trans.response.headers["Warning"] = ( + f'299 - "Some conventions are no longer valid; ignored on read: {compact}"' + ) + elif status == "invalid": + compact = "; ".join(errors) + trans.response.headers["X-Galaxy-Schema-Errors"] = compact + trans.response.headers["Warning"] = f'299 - "Stored tool no longer satisfies schema: {compact}"' + # Return the raw stored value so callers can still inspect / + # repair the YAML manually. + import json + + return json.dumps(dynamic_tool.value) + assert isinstance(lifted, UserToolSource) + return lifted.model_dump_json( by_alias=True, exclude_defaults=True, exclude_unset=True, diff --git a/test/unit/tool_util_models/test_user_tool_source_response.py b/test/unit/tool_util_models/test_user_tool_source_response.py new file mode 100644 index 000000000000..44c713c3668e --- /dev/null +++ b/test/unit/tool_util_models/test_user_tool_source_response.py @@ -0,0 +1,136 @@ +"""Regression tests for `lift_user_tool_source`. + +Stored DynamicTool.value rows on long-lived servers (e.g. test.galaxyproject.org) +predate the YAML narrowing in commit ec5cfe6cce6 and contain wider internal-model +fields. The lift helper validates against the strict schema, strips drift-only +errors, and falls back to a raw-dict shape for genuinely broken stored data so +the API doesn't 500. See Sentry GALAXY-TEST-588ZYT7JSX3V0. +""" + +from typing import ( + Any, + Dict, +) + +from galaxy.tool_util_models import ( + lift_user_tool_source, + UserToolSource, +) + +LEGACY_DATA_INPUT: Dict[str, Any] = { + "type": "data", + "name": "input", + "format": ["data"], + "parameter_type": "gx_data", + "argument": None, + "hidden": False, + "is_dynamic": False, + "extensions": ["data"], +} + +LEGACY_TEXT_INPUT: Dict[str, Any] = { + "type": "text", + "name": "msg", + "value": "hello", + "parameter_type": "gx_text", + "argument": None, + "hidden": False, + "is_dynamic": False, + "default_options": [], +} + +BASE_TOOL: Dict[str, Any] = { + "class": "GalaxyUserTool", + "id": "legacy-tool", + "name": "Legacy", + "container": "busybox", + "shell_command": "echo $(inputs.msg)", + "outputs": [], +} + + +def _legacy_tool_value(): + return {**BASE_TOOL, "inputs": [LEGACY_DATA_INPUT.copy(), LEGACY_TEXT_INPUT.copy()]} + + +def test_lift_status_ok_for_clean_representation(): + clean = { + **BASE_TOOL, + "inputs": [ + {"type": "data", "name": "input", "format": ["data"]}, + {"type": "text", "name": "msg", "value": "hi"}, + ], + } + status, parsed, errors = lift_user_tool_source(clean) + assert status == "ok" + assert isinstance(parsed, UserToolSource) + assert errors == [] + + +def test_lift_status_lifted_for_drift_only(): + status, parsed, errors = lift_user_tool_source(_legacy_tool_value()) + assert status == "lifted" + assert isinstance(parsed, UserToolSource) + # Each legacy key is reported with its compact path; discriminator tags are + # stripped from the user-facing path. + assert "inputs.0.parameter_type" in errors + assert "inputs.0.argument" in errors + assert "inputs.1.default_options" in errors + # Lifted model no longer carries the legacy fields. + dumped = parsed.model_dump(by_alias=True) + for inp in dumped["inputs"]: + assert "parameter_type" not in inp + assert "argument" not in inp + + +def test_lift_handles_nested_conditional_drift(): + value = { + **BASE_TOOL, + "inputs": [ + { + "type": "conditional", + "name": "advanced", + "test_parameter": {"type": "boolean", "name": "use", "value": False}, + "whens": [ + {"discriminator": True, "parameters": [LEGACY_DATA_INPUT.copy()]}, + {"discriminator": False, "parameters": []}, + ], + }, + ], + } + status, parsed, errors = lift_user_tool_source(value) + assert status == "lifted" + assert isinstance(parsed, UserToolSource) + assert any("parameter_type" in e for e in errors) + + +def test_lift_status_invalid_for_real_schema_error(): + """A required-field violation can't be auto-lifted; helper returns the raw + dict and a human-readable summary so the endpoint can still serve a + response without 500-ing.""" + bad = _legacy_tool_value() + del bad["name"] # name is a required str + status, parsed, errors = lift_user_tool_source(bad) + assert status == "invalid" + assert isinstance(parsed, dict) + assert any("name" in e and "required" in e.lower() for e in errors) + + +def test_lift_status_invalid_when_value_constraint_fails(): + """Simulates a future tightening of the schema (e.g. a stricter constraint + on a scalar field): even if drift exists, a real value-level error pushes + the result to 'invalid' rather than 'lifted'.""" + bad = _legacy_tool_value() + bad["shell_command"] = 123 # must be a str + status, parsed, errors = lift_user_tool_source(bad) + assert status == "invalid" + assert isinstance(parsed, dict) + assert any("shell_command" in e for e in errors) + + +def test_lift_does_not_mutate_input(): + original = _legacy_tool_value() + snapshot = {**original, "inputs": [dict(i) for i in original["inputs"]]} + lift_user_tool_source(original) + assert original["inputs"][0] == snapshot["inputs"][0] + assert original["inputs"][1] == snapshot["inputs"][1]