From c3cf80fc7587e8414a8634d120d23a8f7dd1446a Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 18 Feb 2025 14:18:32 +0100 Subject: [PATCH 1/4] Fix data table filters that operate on ``value`` field This is a little cleaner. Also replaces the `list[tuple[str, str, bool]]` return type of get_options with a NamedTuple. Fixes https://github.com/galaxyproject/galaxy/issues/19609 --- lib/galaxy/tool_util/parameters/models.py | 25 ++++ lib/galaxy/tools/parameters/basic.py | 114 ++++++++++-------- .../tools/parameters/dynamic_options.py | 69 ++++++++--- test/unit/app/tools/test_dynamic_options.py | 3 +- test/unit/app/tools/test_evaluation.py | 3 +- test/unit/app/tools/test_select_parameters.py | 17 ++- 6 files changed, 155 insertions(+), 76 deletions(-) diff --git a/lib/galaxy/tool_util/parameters/models.py b/lib/galaxy/tool_util/parameters/models.py index d0e7e6bbcb8..a7f28e8e4a5 100644 --- a/lib/galaxy/tool_util/parameters/models.py +++ b/lib/galaxy/tool_util/parameters/models.py @@ -13,6 +13,7 @@ from typing import ( Optional, Sequence, Type, + TYPE_CHECKING, TypeVar, Union, ) @@ -42,6 +43,7 @@ from typing_extensions import ( ) from galaxy.exceptions import RequestParameterInvalidException +from galaxy.model import HistoryDatasetAssociation from galaxy.tool_util.parser.interface import ( DrillDownOptionsDict, JsonTestCollectionDefDict, @@ -66,6 +68,10 @@ from ._types import ( union_type, ) +if TYPE_CHECKING: + from galaxy.model import DatasetInstance + from galaxy.security.idencoding import IdEncodingHelper + # TODO: # - implement data_ref on rules and implement some cross model validation @@ -106,6 +112,25 @@ class ConnectedValue(BaseModel): discriminator: Literal["ConnectedValue"] = Field(alias="__class__") +class ParameterOption(NamedTuple): + name: str + value: str + selected: bool = False + dataset: Optional["DatasetInstance"] = None + + def serialize(self, security: "IdEncodingHelper"): + if self.dataset: + return ( + self.name, + { + "src": "hda" if isinstance(self.dataset, HistoryDatasetAssociation) else "ldda", + "id": security.encode_id(self.dataset.id), + }, + self.selected, + ) + return (self.name, self.value, self.selected) + + def allow_connected_value(type: Type): return union_type([type, ConnectedValue]) diff --git a/lib/galaxy/tools/parameters/basic.py b/lib/galaxy/tools/parameters/basic.py index 1767c7e4e14..195e13639b5 100644 --- a/lib/galaxy/tools/parameters/basic.py +++ b/lib/galaxy/tools/parameters/basic.py @@ -13,10 +13,11 @@ import urllib.parse from collections.abc import MutableMapping from typing import ( Any, + cast, Dict, List, Optional, - Tuple, + Sequence, TYPE_CHECKING, Union, ) @@ -42,7 +43,9 @@ from galaxy.model import ( from galaxy.model.dataset_collections import builder from galaxy.schema.fetch_data import FilesPayload from galaxy.tool_util.parameters.factory import get_color_value +from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tool_util.parser import get_input_source as ensure_input_source +from galaxy.tool_util.parser.interface import DrillDownOptionsDict from galaxy.tool_util.parser.util import ( boolean_is_checked, boolean_true_and_false_values, @@ -131,6 +134,10 @@ def parse_dynamic_options(param, input_source): return dynamic_options.DynamicOptions(options_elem, param) +def serialize_options(security: "IdEncodingHelper", options: Sequence[ParameterOption]): + return [option.serialize(security) for option in options] + + # Describe a parameter value error where there is no actual supplied # parameter - e.g. just a specification issue. NO_PARAMETER_VALUE = object() @@ -175,7 +182,7 @@ class ToolParameter(UsesDictVisibleKeys): >>> from galaxy.util.bunch import Bunch >>> from galaxy.util import XML - >>> trans = Bunch(app=None) + >>> trans = Bunch(app=None, security=lambda x: x) >>> p = ToolParameter(None, XML('')) >>> assert p.name == 'parameter_name' >>> assert sorted(p.to_dict(trans).items()) == [('argument', '--parameter-name'), ('help', ''), ('help_format', 'html'), ('hidden', False), ('is_dynamic', False), ('label', ''), ('model_class', 'ToolParameter'), ('name', 'parameter_name'), ('optional', False), ('refresh_on_change', False), ('type', 'text'), ('value', None)] @@ -925,7 +932,7 @@ class SelectToolParameter(ToolParameter): >>> from galaxy.util.bunch import Bunch >>> from galaxy.util import XML - >>> trans = Bunch(app=None, history=Bunch(), workflow_building_mode=False) + >>> trans = Bunch(app=None, history=Bunch(), workflow_building_mode=False, security=lambda x: x) >>> p = SelectToolParameter(None, XML( ... ''' ... @@ -983,13 +990,15 @@ class SelectToolParameter(ToolParameter): call_other_values.update(other_values.dict) return call_other_values - def get_options(self, trans, other_values): + def get_options(self, trans, other_values) -> Sequence[Union[ParameterOption, DrillDownOptionsDict]]: if self.options: return self.options.get_options(trans, other_values) elif self.dynamic_options: call_other_values = self._get_dynamic_options_call_other_values(trans, other_values) try: - return eval(self.dynamic_options, self.tool.code_namespace, call_other_values) + return [ + ParameterOption(*o) for o in eval(self.dynamic_options, self.tool.code_namespace, call_other_values) + ] except Exception as e: log.debug( "Error determining dynamic options for parameter '%s' in tool '%s':", @@ -999,22 +1008,21 @@ class SelectToolParameter(ToolParameter): ) return [] else: - return self.static_options + return [ParameterOption(*o) for o in self.static_options] def get_legal_values(self, trans, other_values, value): """ determine the set of values of legal options """ - return { - history_item_dict_to_python(v, trans.app, self.name) or v - for _, v, _ in self.get_options(trans, other_values) - } + options = cast(List[ParameterOption], self.get_options(trans, other_values)) + return {option.dataset or option.value for option in options} def get_legal_names(self, trans, other_values): """ - determine a mapping from names to values for all legal options + determine the set of values of legal options """ - return {n: v for n, v, _ in self.get_options(trans, other_values)} + options = cast(List[ParameterOption], self.get_options(trans, other_values)) + return {option.name: option.value for option in options} def from_json(self, value, trans, other_values=None): return self._select_from_json(value, trans, other_values=other_values, require_legal_value=True) @@ -1136,17 +1144,17 @@ class SelectToolParameter(ToolParameter): def get_initial_value(self, trans, other_values): try: - options = list(self.get_options(trans, other_values)) + options = cast(List[ParameterOption], self.get_options(trans, other_values)) except ImplicitConversionRequired: return None if not options: return None - value = [optval for _, optval, selected in options if selected] + value = [option.value for option in options if option.selected] if len(value) == 0: if not self.optional and not self.multiple and options: # Nothing selected, but not optional and not a multiple select, with some values, # so we have to default to something (the HTML form will anyway) - value2 = options[0][1] + value2: Optional[Union[str, List[str]]] = options[0].value else: value2 = None elif len(value) == 1 or not self.multiple: @@ -1186,8 +1194,8 @@ class SelectToolParameter(ToolParameter): d = super().to_dict(trans, other_values) # Get options, value. - options = self.get_options(trans, other_values) - d["options"] = options + options = cast(List[ParameterOption], self.get_options(trans, other_values)) + d["options"] = serialize_options(trans.security, options) d["display"] = self.display d["multiple"] = self.multiple d["textable"] = is_runtime_context(trans, other_values) @@ -1212,7 +1220,7 @@ class GenomeBuildParameter(SelectToolParameter): >>> # Create a mock transaction with 'hg17' as the current build >>> from galaxy.util.bunch import Bunch >>> from galaxy.util import XML - >>> trans = Bunch(app=None, history=Bunch(genome_build='hg17'), db_builds=read_dbnames(None)) + >>> trans = Bunch(app=None, history=Bunch(genome_build='hg17'), db_builds=read_dbnames(None), security=lambda x:x) >>> p = GenomeBuildParameter(None, XML('')) >>> print(p.name) _name @@ -1232,12 +1240,14 @@ class GenomeBuildParameter(SelectToolParameter): self.static_options = [(value, key, False) for key, value in self._get_dbkey_names()] self.is_dynamic = True - def get_options(self, trans, other_values): + def get_options(self, trans, other_values) -> Sequence[ParameterOption]: last_used_build = object() if trans.history: last_used_build = trans.history.genome_build - for dbkey, build_name in self._get_dbkey_names(trans=trans): - yield build_name, dbkey, (dbkey == last_used_build) + return [ + ParameterOption(build_name, dbkey, (dbkey == last_used_build)) + for dbkey, build_name in self._get_dbkey_names(trans=trans) + ] def get_legal_values(self, trans, other_values, value): return {dbkey for dbkey, _ in self._get_dbkey_names(trans=trans)} @@ -1247,16 +1257,16 @@ class GenomeBuildParameter(SelectToolParameter): d = ToolParameter.to_dict(self, trans) # Get options, value - options is a generator here, so compile to list - options = list(self.get_options(trans, {})) - value = options[0][1] + options = self.get_options(trans, {}) + value = options[0].value for option in options: - if option[2]: + if option.selected: # Found selected option. - value = option[1] + value = option.value d.update( { - "options": options, + "options": serialize_options(trans, options), "value": value, "display": self.display, "multiple": self.multiple, @@ -1344,13 +1354,13 @@ class SelectTagParameter(SelectToolParameter): tags.add(tag.user_value) return list(tags) - def get_options(self, trans, other_values): + def get_options(self, trans, other_values) -> Sequence[ParameterOption]: """ Show tags """ options = [] for tag in self.get_tag_list(other_values): - options.append((f"Tags: {tag}", tag, False)) + options.append(ParameterOption(f"Tags: {tag}", tag, False)) return options def get_initial_value(self, trans, other_values): @@ -1517,11 +1527,11 @@ class ColumnListParameter(SelectToolParameter): column_list = [c for c in column_list if c in this_column_list] return column_list - def get_options(self, trans, other_values): + def get_options(self, trans, other_values) -> Sequence[ParameterOption]: """ Show column labels rather than c1..cn if use_header_names=True """ - options: List[Tuple[str, Union[str, Tuple[str, str]], bool]] = [] + options: Sequence[ParameterOption] = [] column_list = self.get_column_list(trans, other_values) if not column_list: return options @@ -1535,7 +1545,10 @@ class ColumnListParameter(SelectToolParameter): and dataset.metadata.element_is_set("column_names") ): try: - options = [(f"c{c}: {dataset.metadata.column_names[int(c) - 1]}", c, False) for c in column_list] + options = [ + ParameterOption(f"c{c}: {dataset.metadata.column_names[int(c) - 1]}", c, False) + for c in column_list + ] except IndexError: # ignore and rely on fallback pass @@ -1544,13 +1557,13 @@ class ColumnListParameter(SelectToolParameter): with open(dataset.get_file_name()) as f: head = f.readline() cnames = head.rstrip("\n\r ").split("\t") - options = [(f"c{c}: {cnames[int(c) - 1]}", c, False) for c in column_list] + options = [ParameterOption(f"c{c}: {cnames[int(c) - 1]}", c, False) for c in column_list] except Exception: # ignore and rely on fallback pass if not options: # fallback if no options list could be built so far - options = [(f"Column: {col}", col, False) for col in column_list] + options = [ParameterOption(f"Column: {col}", col, False) for col in column_list] return options def get_initial_value(self, trans, other_values): @@ -1613,7 +1626,7 @@ class DrillDownSelectToolParameter(SelectToolParameter): >>> from galaxy.util.bunch import Bunch >>> app = Bunch(config=Bunch(tool_data_path=None)) >>> tool = Bunch(app=app) - >>> trans = Bunch(app=app, history=Bunch(genome_build='hg17'), db_builds=read_dbnames(None)) + >>> trans = Bunch(app=app, history=Bunch(genome_build='hg17'), db_builds=read_dbnames(None), security=lambda x: x) >>> p = DrillDownSelectToolParameter(tool, XML( ... ''' ... @@ -1680,18 +1693,17 @@ class DrillDownSelectToolParameter(SelectToolParameter): except Exception: return [] - def get_options(self, trans=None, other_values=None): + def get_options(self, trans=None, other_values=None) -> List[DrillDownOptionsDict]: other_values = other_values or {} if self.is_dynamic: if self.dynamic_options: - options = self._get_options_from_code(trans=trans, other_values=other_values) - else: - options = [] - return options + return self._get_options_from_code(trans=trans, other_values=other_values) + return [] + return self.options def get_legal_values(self, trans, other_values, value): - def recurse_options(legal_values, options): + def recurse_options(legal_values, options: List[DrillDownOptionsDict]): for option in options: legal_values.append(option["value"]) recurse_options(legal_values, option["options"]) @@ -1737,16 +1749,16 @@ class DrillDownSelectToolParameter(SelectToolParameter): other_values = other_values or {} def get_options_list(value): - def get_base_option(value, options): + def get_base_option(value, options: List[DrillDownOptionsDict]): for option in options: if value == option["value"]: return option - rval = get_base_option(value, option["options"]) + rval = get_base_option(value, option["options"] or []) if rval: return rval return None # not found - def recurse_option(option_list, option): + def recurse_option(option_list, option: DrillDownOptionsDict): if not option["options"]: option_list.append(option["value"]) else: @@ -1754,8 +1766,10 @@ class DrillDownSelectToolParameter(SelectToolParameter): recurse_option(option_list, opt) rval: List[str] = [] - base_option = get_base_option(value, self.get_options(other_values=other_values)) - recurse_option(rval, base_option) + options = self.get_options(other_values=other_values) + base_option = get_base_option(value, options) + if base_option: + recurse_option(rval, base_option) return rval or [value] if value is None: @@ -1781,11 +1795,11 @@ class DrillDownSelectToolParameter(SelectToolParameter): return rval def get_initial_value(self, trans, other_values): - def recurse_options(initial_values, options): + def recurse_options(initial_values, options: List[DrillDownOptionsDict]): for option in options: if option["selected"]: initial_values.append(option["value"]) - recurse_options(initial_values, option["options"]) + recurse_options(initial_values, option["options"] or []) # More working around dynamic options for workflow options = self.get_options(trans=trans, other_values=other_values) @@ -1798,11 +1812,11 @@ class DrillDownSelectToolParameter(SelectToolParameter): return initial_values def to_text(self, value): - def get_option_display(value, options): + def get_option_display(value, options: List[DrillDownOptionsDict]): for option in options: if value == option["value"]: return option["name"] - rval = get_option_display(value, option["options"]) + rval = get_option_display(value, option["options"] or []) if rval: return rval return None # not found @@ -1824,7 +1838,7 @@ class DrillDownSelectToolParameter(SelectToolParameter): else: rval = [] for val in value: - rval.append(get_option_display(val, self.options) or val) + rval.append(get_option_display(val, self.options or val)) if rval: return "\n".join(map(str, rval)) return "Nothing selected." diff --git a/lib/galaxy/tools/parameters/dynamic_options.py b/lib/galaxy/tools/parameters/dynamic_options.py index d40d328491f..a759be19c9c 100644 --- a/lib/galaxy/tools/parameters/dynamic_options.py +++ b/lib/galaxy/tools/parameters/dynamic_options.py @@ -15,7 +15,10 @@ from typing import ( cast, Dict, get_args, + List, Optional, + Sequence, + Set, ) from typing_extensions import Literal @@ -28,6 +31,7 @@ from galaxy.model import ( MetadataFile, User, ) +from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tools.expressions import do_eval from galaxy.tools.parameters.workflow_utils import ( is_runtime_value, @@ -92,7 +96,7 @@ class StaticValueFilter(Filter): self.column = d_option.column_spec_to_index(column) self.keep = string_as_bool(elem.get("keep", "True")) - def filter_options(self, options, trans, other_values): + def filter_options(self, options: Sequence[ParameterOption], trans, other_values): rval = [] filter_value = self.value try: @@ -128,7 +132,7 @@ class RegexpFilter(Filter): self.column = d_option.column_spec_to_index(column) self.keep = string_as_bool(elem.get("keep", "True")) - def filter_options(self, options, trans, other_values): + def filter_options(self, options: Sequence[ParameterOption], trans, other_values): rval = [] filter_value = self.value try: @@ -184,7 +188,9 @@ class DataMetaFilter(Filter): def get_dependency_name(self): return self.ref_name - def filter_options(self, options, trans, other_values): + def filter_options(self, options: Sequence[ParameterOption], trans, other_values): + options = list(options) + def _add_meta(meta_value, m): if isinstance(m, list): meta_value |= set(m) @@ -224,7 +230,7 @@ class DataMetaFilter(Filter): # - for data sets: the meta data value # in both cases only meta data that is set (i.e. differs from the no_value) # is considered - meta_value = set() + meta_value: Set[Any] = set() for r in ref: if not r.metadata.element_is_set(self.key): continue @@ -236,7 +242,7 @@ class DataMetaFilter(Filter): return copy.deepcopy(options) if self.column is not None: - rval = [] + rval: List[ParameterOption] = [] for fields in options: if compare_meta_value(fields[self.column], meta_value): rval.append(fields) @@ -246,7 +252,7 @@ class DataMetaFilter(Filter): self.dynamic_option.columns = {"name": 0, "value": 1, "selected": 2} self.dynamic_option.largest_index = 2 for value in meta_value: - options.append((value, value, False)) + options.append(ParameterOption(value, value, False)) return options @@ -286,7 +292,7 @@ class ParamValueFilter(Filter): def get_dependency_name(self): return self.ref_name - def filter_options(self, options, trans, other_values): + def filter_options(self, options: Sequence[ParameterOption], trans, other_values): ref = other_values.get(self.ref_name, None) if ref is None or is_runtime_value(ref): ref = [] @@ -336,7 +342,7 @@ class UniqueValueFilter(Filter): def get_dependency_name(self): return self.dynamic_option.dataset_ref_name - def filter_options(self, options, trans, other_values): + def filter_options(self, options: Sequence[ParameterOption], trans, other_values): rval = [] seen = set() for fields in options: @@ -365,12 +371,17 @@ class MultipleSplitterFilter(Filter): assert columns is not None, "Required 'column' attribute missing from filter" self.columns = [d_option.column_spec_to_index(column) for column in columns.split(",")] - def filter_options(self, options, trans, other_values): + def filter_options(self, options: Sequence[ParameterOption], trans, other_values): rval = [] for fields in options: for column in self.columns: - for field in fields[column].split(self.separator): - rval.append(fields[0:column] + [field] + fields[column + 1 :]) + field = fields[column] + if isinstance(field, str): + for split_field in field.split(self.separator): + new_options = list(fields[0:column]) + [split_field] + list(fields[column + 1 :]) + # tested in filter_multiple_splitter.xml + option = tuple(new_options) + rval.append(option) return rval @@ -766,6 +777,19 @@ class DynamicOptions: options = filter.filter_options(options, trans, other_values) return options + @staticmethod + def to_parameter_options(options): + rval: List[ParameterOption] = [] + for option in options: + if isinstance(option, ParameterOption): + rval.append(option) + else: + if len(option) == 1: + rval.append(ParameterOption(option[0], option[0])) + else: + rval.append(ParameterOption(*option[:3])) + return rval + def get_user_options(self, user: User): # stored metadata are key: value pairs, turn into flat lists of correct order fields = [] @@ -785,6 +809,8 @@ class DynamicOptions: field_entry = [] for column_key in self.tool_data_table.columns.keys(): field_entry.append(data_table_entry[column_key]) + if hda := data_table_entry.get("__hda__"): + field_entry.append(hda) fields.append(field_entry) return fields @@ -797,7 +823,7 @@ class DynamicOptions: if path := value.get("path"): # maybe a hack, should probably pass around dataset or src id combinations ? value["path"] = os.path.join(hda.extra_files_path, path) - value["value"] = {"src": "hda", "id": hda.id} + value["__hda__"] = hda return table_entries def get_option_from_dataset(self, dataset): @@ -834,15 +860,15 @@ class DynamicOptions: rval.append(fields[field_index]) return rval - def get_options(self, trans, other_values): + def get_options(self, trans, other_values) -> Sequence[ParameterOption]: - rval = [] + rval: List[ParameterOption] = [] - def to_triple(values): + def to_option(values): if len(values) == 2: - return [str(values[0]), str(values[1]), False] + return ParameterOption(str(values[0]), str(values[1]), False) else: - return [str(values[0]), str(values[1]), bool(values[2])] + return ParameterOption(str(values[0]), str(values[1]), bool(values[2])) if from_url_options := self.from_url_options: context = User.user_template_environment(trans.user) @@ -880,7 +906,7 @@ class DynamicOptions: data = [] # We only support the very specific ["name", "value", "selected"] format for now. - rval = [to_triple(d) for d in data] + rval = [to_option(d) for d in data] if ( self.file_fields is not None or self.tool_data_table is not None @@ -889,11 +915,14 @@ class DynamicOptions: ): options = self.get_fields(trans, other_values) for fields in options: - rval.append((fields[self.columns["name"]], fields[self.columns["value"]], False)) + name = fields[self.columns["name"]] + value = fields[self.columns["value"]] + hda = fields[-1] if isinstance(fields[-1], HistoryDatasetAssociation) else None + rval.append(ParameterOption(name, value, False, dataset=hda)) else: for filter in self.filters: rval = filter.filter_options(rval, trans, other_values) - return rval + return self.to_parameter_options(rval) def column_spec_to_index(self, column_spec): """ diff --git a/test/unit/app/tools/test_dynamic_options.py b/test/unit/app/tools/test_dynamic_options.py index 05ec365a379..508acdb3a07 100644 --- a/test/unit/app/tools/test_dynamic_options.py +++ b/test/unit/app/tools/test_dynamic_options.py @@ -1,4 +1,5 @@ from galaxy.app_unittest_utils.galaxy_mock import MockApp +from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tools.parameters.dynamic_options import DynamicOptions from galaxy.util import XML from galaxy.util.bunch import Bunch @@ -60,4 +61,4 @@ def test_dynamic_option_cache(): "start_index": 0, }, ) - assert from_url_option.get_options(trans, {}) == [["chr2L", "23513712", False]] + assert from_url_option.get_options(trans, {}) == [ParameterOption("chr2L", "23513712", False)] diff --git a/test/unit/app/tools/test_evaluation.py b/test/unit/app/tools/test_evaluation.py index e571b5fd889..b2864ea3360 100644 --- a/test/unit/app/tools/test_evaluation.py +++ b/test/unit/app/tools/test_evaluation.py @@ -12,6 +12,7 @@ from galaxy.model import ( JobToInputDatasetAssociation, JobToOutputDatasetAssociation, ) +from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tool_util.parser.output_objects import ToolOutput from galaxy.tools.evaluation import ToolEvaluator @@ -171,7 +172,7 @@ class TestToolEvaluator(TestCase, UsesApp): return ["/old/path/human"] def get_options(trans, other_values): - return [["", "/old/path/human", ""]] + return [ParameterOption("", "/old/path/human", False)] parameter.options = Bunch(get_field_by_name_for_value=get_field_by_name_for_value, get_options=get_options) self.tool.set_params({"index_path": parameter}) diff --git a/test/unit/app/tools/test_select_parameters.py b/test/unit/app/tools/test_select_parameters.py index 656325490a9..c41f547fa4b 100644 --- a/test/unit/app/tools/test_select_parameters.py +++ b/test/unit/app/tools/test_select_parameters.py @@ -4,6 +4,7 @@ import pytest from galaxy import model from galaxy.model.base import transaction +from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tools.parameters.workflow_utils import RuntimeValue from .util import BaseParameterTestCase @@ -42,15 +43,23 @@ class TestSelectToolParameter(BaseParameterTestCase): def test_filter_param_value(self): self.options_xml = """""" - assert ("testname1", "testpath1", False) in self.param.get_options(self.trans, {"input_bam": "testname1"}) - assert ("testname2", "testpath2", False) in self.param.get_options(self.trans, {"input_bam": "testname2"}) + assert ParameterOption("testname1", "testpath1", False) in self.param.get_options( + self.trans, {"input_bam": "testname1"} + ) + assert ParameterOption("testname2", "testpath2", False) in self.param.get_options( + self.trans, {"input_bam": "testname2"} + ) assert len(self.param.get_options(self.trans, {"input_bam": "testname3"})) == 0 def test_filter_param_value2(self): # Same test as above, but filtering on a different column. self.options_xml = """""" - assert ("testname1", "testpath1", False) in self.param.get_options(self.trans, {"input_bam": "testpath1"}) - assert ("testname2", "testpath2", False) in self.param.get_options(self.trans, {"input_bam": "testpath2"}) + assert ParameterOption("testname1", "testpath1", False) in self.param.get_options( + self.trans, {"input_bam": "testpath1"} + ) + assert ParameterOption("testname2", "testpath2", False) in self.param.get_options( + self.trans, {"input_bam": "testpath2"} + ) assert len(self.param.get_options(self.trans, {"input_bam": "testpath3"})) == 0 # TODO: Good deal of overlap here with TestDataToolParameter, refactor. From 172ef05f269012f1be0430839883fd6cf84bc79e Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 18 Feb 2025 15:01:44 +0100 Subject: [PATCH 2/4] Skip bundles that don't match data table definition Fixes https://github.com/galaxyproject/galaxy/issues/18749 --- lib/galaxy/tools/parameters/dynamic_options.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/tools/parameters/dynamic_options.py b/lib/galaxy/tools/parameters/dynamic_options.py index a759be19c9c..045feec674a 100644 --- a/lib/galaxy/tools/parameters/dynamic_options.py +++ b/lib/galaxy/tools/parameters/dynamic_options.py @@ -807,11 +807,19 @@ class DynamicOptions: by_dbkey.update(table_entries) for data_table_entry in by_dbkey.values(): field_entry = [] - for column_key in self.tool_data_table.columns.keys(): - field_entry.append(data_table_entry[column_key]) if hda := data_table_entry.get("__hda__"): field_entry.append(hda) - fields.append(field_entry) + missing_columns = False + for column_key in self.tool_data_table.columns.keys(): + if column_key not in data_table_entry: + # currrent data table definition (as in self.tool_data_table) + # may not match against the data manager bundle. + # Breaking here fixes https://github.com/galaxyproject/galaxy/issues/18749. + missing_columns = True + break + field_entry.append(data_table_entry[column_key]) + if not missing_columns: + fields.append(field_entry) return fields @staticmethod From be3db670892b81caefa705c7c1fe115630c31dc8 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 18 Feb 2025 17:46:57 +0100 Subject: [PATCH 3/4] Skip data_manager_json files that aren't bundles The file_size hack turns out to not work, since people report related bugs that they shouldn't have been able to run into without generating bundles. It's probably still true that bundles should produce a distinct datatype. --- lib/galaxy/datatypes/text.py | 1 + lib/galaxy/managers/workflows.py | 20 +++----------------- lib/galaxy/model/__init__.py | 18 +++++++++++++++++- lib/galaxy/tools/__init__.py | 1 + 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/lib/galaxy/datatypes/text.py b/lib/galaxy/datatypes/text.py index 95fa625aa78..6c6b587aaa4 100644 --- a/lib/galaxy/datatypes/text.py +++ b/lib/galaxy/datatypes/text.py @@ -138,6 +138,7 @@ class DataManagerJson(Json): MetadataElement( name="data_tables", default=None, desc="Data tables represented by this dataset", readonly=True, visible=True ) + MetadataElement(name="is_bundle", default=False, desc="Dataset represents bundle", readonly=True, visible=True) def set_meta(self, dataset: DatasetProtocol, overwrite: bool = True, **kwd): super().set_meta(dataset=dataset, overwrite=overwrite, **kwd) diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index 83c00eae972..7f5196661a7 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -31,8 +31,6 @@ from pydantic import ( ) from sqlalchemy import ( and_, - Cast, - ColumnElement, desc, false, func, @@ -40,7 +38,6 @@ from sqlalchemy import ( select, true, ) -from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import ( aliased, joinedload, @@ -72,6 +69,7 @@ from galaxy.model import ( StoredWorkflow, StoredWorkflowTagAssociation, StoredWorkflowUserShareAssociation, + to_json, User, Workflow, WorkflowInvocation, @@ -2067,26 +2065,14 @@ class WorkflowContentsManager(UsesAnnotations): ) -> Optional[model.StoredWorkflow]: sa_session = self.app.model.session - def to_json(column, keys: List[str]): - assert sa_session.bind - if sa_session.bind.dialect.name == "postgresql": - cast: Union[ColumnElement[Any], Cast[Any]] = func.cast(func.convert_from(column, "UTF8"), JSONB) - for key in keys: - cast = cast.__getitem__(key) - return cast.astext - else: - for key in keys: - column = func.json_extract(column, f"$.{key}") - return column - stmnt = ( select(model.StoredWorkflow) .join(model.Workflow, model.Workflow.id == model.StoredWorkflow.latest_workflow_id) .filter( and_( model.StoredWorkflow.deleted == false(), - to_json(model.Workflow.source_metadata, ["trs_tool_id"]) == trs_id, - to_json(model.Workflow.source_metadata, ["trs_version_id"]) == trs_version, + to_json(sa_session, model.Workflow.source_metadata, ["trs_tool_id"]) == trs_id, + to_json(sa_session, model.Workflow.source_metadata, ["trs_version_id"]) == trs_version, ) ) ) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index c5053cc4849..a8829719afe 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -68,8 +68,10 @@ from sqlalchemy import ( bindparam, Boolean, case, + Cast, Column, column, + ColumnElement, DateTime, delete, desc, @@ -100,6 +102,7 @@ from sqlalchemy import ( update, VARCHAR, ) +from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.exc import ( CompileError, OperationalError, @@ -318,6 +321,19 @@ def get_uuid(uuid: Optional[Union[UUID, str]] = None) -> UUID: return UUID(str(uuid)) +def to_json(sa_session, column, keys: List[str]): + assert sa_session.bind + if sa_session.bind.dialect.name == "postgresql": + cast: Union[ColumnElement[Any], Cast[Any]] = func.cast(func.convert_from(column, "UTF8"), JSONB) + for key in keys: + cast = cast.__getitem__(key) + return cast.astext + else: + for key in keys: + column = func.json_extract(column, f"$.{key}") + return column + + class Base(DeclarativeBase, _HasTable): __abstract__ = True metadata = MetaData(naming_convention=NAMING_CONVENTION) @@ -904,8 +920,8 @@ class User(Base, Dictifiable, RepresentById): Dataset.state == "ok", # excludes data manager runs that actually populated tables. # maybe track this formally by creating a different datatype for bundles ? - Dataset.total_size != Dataset.file_size, HistoryDatasetAssociation._metadata.contains(data_table), + to_json(session, HistoryDatasetAssociation._metadata, ["is_bundle"]) == "true", ) .order_by(HistoryDatasetAssociation.id) ) diff --git a/lib/galaxy/tools/__init__.py b/lib/galaxy/tools/__init__.py index 46b86643399..502cdad1729 100644 --- a/lib/galaxy/tools/__init__.py +++ b/lib/galaxy/tools/__init__.py @@ -3261,6 +3261,7 @@ class DataManagerTool(OutputParameterJSONTool): create=True, preserve_symlinks=True, ) + hda.metadata.is_bundle = True else: raise Exception("Unknown data manager mode encountered type...") From 8c4149b094a71a84c8779350fe140c1d5a7965a6 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 19 Feb 2025 17:48:29 +0100 Subject: [PATCH 4/4] Fix dependency structure for ParameterOption --- lib/galaxy/tool_util/parameters/models.py | 25 ---------------- lib/galaxy/tools/parameters/basic.py | 2 +- .../tools/parameters/dynamic_options.py | 2 +- lib/galaxy/tools/parameters/options.py | 29 +++++++++++++++++++ test/unit/app/tools/test_dynamic_options.py | 2 +- test/unit/app/tools/test_evaluation.py | 2 +- test/unit/app/tools/test_select_parameters.py | 2 +- 7 files changed, 34 insertions(+), 30 deletions(-) create mode 100644 lib/galaxy/tools/parameters/options.py diff --git a/lib/galaxy/tool_util/parameters/models.py b/lib/galaxy/tool_util/parameters/models.py index a7f28e8e4a5..d0e7e6bbcb8 100644 --- a/lib/galaxy/tool_util/parameters/models.py +++ b/lib/galaxy/tool_util/parameters/models.py @@ -13,7 +13,6 @@ from typing import ( Optional, Sequence, Type, - TYPE_CHECKING, TypeVar, Union, ) @@ -43,7 +42,6 @@ from typing_extensions import ( ) from galaxy.exceptions import RequestParameterInvalidException -from galaxy.model import HistoryDatasetAssociation from galaxy.tool_util.parser.interface import ( DrillDownOptionsDict, JsonTestCollectionDefDict, @@ -68,10 +66,6 @@ from ._types import ( union_type, ) -if TYPE_CHECKING: - from galaxy.model import DatasetInstance - from galaxy.security.idencoding import IdEncodingHelper - # TODO: # - implement data_ref on rules and implement some cross model validation @@ -112,25 +106,6 @@ class ConnectedValue(BaseModel): discriminator: Literal["ConnectedValue"] = Field(alias="__class__") -class ParameterOption(NamedTuple): - name: str - value: str - selected: bool = False - dataset: Optional["DatasetInstance"] = None - - def serialize(self, security: "IdEncodingHelper"): - if self.dataset: - return ( - self.name, - { - "src": "hda" if isinstance(self.dataset, HistoryDatasetAssociation) else "ldda", - "id": security.encode_id(self.dataset.id), - }, - self.selected, - ) - return (self.name, self.value, self.selected) - - def allow_connected_value(type: Type): return union_type([type, ConnectedValue]) diff --git a/lib/galaxy/tools/parameters/basic.py b/lib/galaxy/tools/parameters/basic.py index 195e13639b5..18329f486ec 100644 --- a/lib/galaxy/tools/parameters/basic.py +++ b/lib/galaxy/tools/parameters/basic.py @@ -43,7 +43,6 @@ from galaxy.model import ( from galaxy.model.dataset_collections import builder from galaxy.schema.fetch_data import FilesPayload from galaxy.tool_util.parameters.factory import get_color_value -from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tool_util.parser import get_input_source as ensure_input_source from galaxy.tool_util.parser.interface import DrillDownOptionsDict from galaxy.tool_util.parser.util import ( @@ -52,6 +51,7 @@ from galaxy.tool_util.parser.util import ( ParameterParseException, text_input_is_optional, ) +from galaxy.tools.parameters.options import ParameterOption from galaxy.tools.parameters.workflow_utils import ( NO_REPLACEMENT, workflow_building_modes, diff --git a/lib/galaxy/tools/parameters/dynamic_options.py b/lib/galaxy/tools/parameters/dynamic_options.py index 045feec674a..ec41da876c4 100644 --- a/lib/galaxy/tools/parameters/dynamic_options.py +++ b/lib/galaxy/tools/parameters/dynamic_options.py @@ -31,8 +31,8 @@ from galaxy.model import ( MetadataFile, User, ) -from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tools.expressions import do_eval +from galaxy.tools.parameters.options import ParameterOption from galaxy.tools.parameters.workflow_utils import ( is_runtime_value, workflow_building_modes, diff --git a/lib/galaxy/tools/parameters/options.py b/lib/galaxy/tools/parameters/options.py new file mode 100644 index 00000000000..4fd23c3fcae --- /dev/null +++ b/lib/galaxy/tools/parameters/options.py @@ -0,0 +1,29 @@ +from typing import ( + NamedTuple, + Optional, +) + +from galaxy.model import ( + DatasetInstance, + HistoryDatasetAssociation, +) +from galaxy.security.idencoding import IdEncodingHelper + + +class ParameterOption(NamedTuple): + name: str + value: str + selected: bool = False + dataset: Optional["DatasetInstance"] = None + + def serialize(self, security: "IdEncodingHelper"): + if self.dataset: + return ( + self.name, + { + "src": "hda" if isinstance(self.dataset, HistoryDatasetAssociation) else "ldda", + "id": security.encode_id(self.dataset.id), + }, + self.selected, + ) + return (self.name, self.value, self.selected) diff --git a/test/unit/app/tools/test_dynamic_options.py b/test/unit/app/tools/test_dynamic_options.py index 508acdb3a07..3de57c332e5 100644 --- a/test/unit/app/tools/test_dynamic_options.py +++ b/test/unit/app/tools/test_dynamic_options.py @@ -1,6 +1,6 @@ from galaxy.app_unittest_utils.galaxy_mock import MockApp -from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tools.parameters.dynamic_options import DynamicOptions +from galaxy.tools.parameters.options import ParameterOption from galaxy.util import XML from galaxy.util.bunch import Bunch from galaxy.work.context import WorkRequestContext diff --git a/test/unit/app/tools/test_evaluation.py b/test/unit/app/tools/test_evaluation.py index b2864ea3360..372f9b28b65 100644 --- a/test/unit/app/tools/test_evaluation.py +++ b/test/unit/app/tools/test_evaluation.py @@ -12,7 +12,6 @@ from galaxy.model import ( JobToInputDatasetAssociation, JobToOutputDatasetAssociation, ) -from galaxy.tool_util.parameters.models import ParameterOption from galaxy.tool_util.parser.output_objects import ToolOutput from galaxy.tools.evaluation import ToolEvaluator @@ -28,6 +27,7 @@ from galaxy.tools.parameters.grouping import ( ConditionalWhen, Repeat, ) +from galaxy.tools.parameters.options import ParameterOption from galaxy.util import XML from galaxy.util.bunch import Bunch from galaxy.util.unittest import TestCase diff --git a/test/unit/app/tools/test_select_parameters.py b/test/unit/app/tools/test_select_parameters.py index c41f547fa4b..63a56ad59e2 100644 --- a/test/unit/app/tools/test_select_parameters.py +++ b/test/unit/app/tools/test_select_parameters.py @@ -4,7 +4,7 @@ import pytest from galaxy import model from galaxy.model.base import transaction -from galaxy.tool_util.parameters.models import ParameterOption +from galaxy.tools.parameters.options import ParameterOption from galaxy.tools.parameters.workflow_utils import RuntimeValue from .util import BaseParameterTestCase