Merge pull request #23170 from guerler/toolrequests.004

Remove legacy unqualified repeat synthesis from test-case resolution

lib/galaxy/tool_util/parameters/case.py

@@ -324,15 +324,16 @@ def test_case_state(
 
     for test_input in inputs:
         input_name = test_input["name"]
-        if input_name not in handled_inputs:
+        if input_name not in handled_inputs and not _input_name_was_handled_by_legacy_fallback(
+            input_name, handled_inputs, profile
+        ):
             unhandled_inputs.append(input_name)
 
     tool_state = TestCaseToolState(state)
     if validate:
         tool_state.validate(tool_parameter_bundle, name=name)
         for input_name in unhandled_inputs:
-            if not _input_name_was_handled_by_legacy_fallback(input_name, handled_inputs, profile):
-                raise Exception(f"Invalid parameter name found {input_name}")
+            raise Exception(f"Invalid parameter name found {input_name}")
     return TestCaseStateAndWarnings(tool_state, warnings, unhandled_inputs)
 
 
@@ -431,7 +432,8 @@ def _merge_into_state(
         if input_name not in state_at_level:
             state_at_level[input_name] = repeat_state_array
 
-        repeat_instance_inputs = _repeat_inputs_to_array(state_path, tool_input.parameters, context.inputs)
+        repeat_instance_inputs = _repeat_inputs_to_array(state_path, context.inputs)
+        supplied_instances = len(repeat_instance_inputs)
         if tool_input.min is not None:
             while len(repeat_instance_inputs) < tool_input.min:
                 repeat_instance_inputs.append([])
@@ -440,14 +442,18 @@ def _merge_into_state(
                 repeat_state_array.append({})
 
             repeat_instance_prefix = f"{state_path}_{i}"
-            handled_inputs.update(
-                _merge_level_into_state(
-                    tool_input.parameters,
-                    context.for_inputs(repeat_instance_inputs[i]),
-                    repeat_state_array[i],
-                    repeat_instance_prefix,
-                )
+            instance_handled_inputs = _merge_level_into_state(
+                tool_input.parameters,
+                context.for_inputs(repeat_instance_inputs[i]),
+                repeat_state_array[i],
+                repeat_instance_prefix,
             )
+            # Instances past the ones the test actually supplied exist only to satisfy min.
+            # Nothing resolved into them, so their paths must not count as handling a raw
+            # input - the legacy suffix match would otherwise read queries_0|input2 as
+            # covering a bare input2 that was in fact dropped.
+            if i < supplied_instances:
+                handled_inputs.update(instance_handled_inputs)
     elif isinstance(tool_input, (SectionParameterModel,)):
         section_state = state_at_level.get(input_name, {})
         if input_name not in state_at_level:
@@ -504,9 +510,7 @@ def _merge_into_state(
     return handled_inputs
 
 
-def _repeat_inputs_to_array(
-    state_path: str, parameters: list[ToolParameterT], inputs: ToolSourceTestInputs
-) -> list[ToolSourceTestInputs]:
+def _repeat_inputs_to_array(state_path: str, inputs: ToolSourceTestInputs) -> list[ToolSourceTestInputs]:
     inputs_as_dict = _inputs_as_dict(inputs)
     repeat_instance_input_dicts = repeat_inputs_to_array(state_path, inputs_as_dict)
     if not repeat_instance_input_dicts and "|" in state_path:
@@ -520,21 +524,7 @@ def _repeat_inputs_to_array(
             repeat_instance_input_dicts = repeat_inputs_to_array(candidate_path, inputs_as_dict)
             if repeat_instance_input_dicts:
                 break
-    repeat_instance_inputs = [list(instance_inputs.values()) for instance_inputs in repeat_instance_input_dicts]
-    if repeat_instance_inputs:
-        return repeat_instance_inputs
-
-    legacy_repeat_inputs: list[ToolSourceTestInputs] = []
-    for parameter in parameters:
-        parameter_name = parameter.name
-        matching_inputs = [input for input in inputs if input["name"] == parameter_name]
-        for i, input in enumerate(matching_inputs):
-            while len(legacy_repeat_inputs) <= i:
-                legacy_repeat_inputs.append([])
-            synthetic_input = cast(ToolSourceTestInput, dict(input))
-            synthetic_input["name"] = f"{state_path}_{i}|{parameter_name}"
-            legacy_repeat_inputs[i].append(synthetic_input)
-    return legacy_repeat_inputs
+    return [list(instance_inputs.values()) for instance_inputs in repeat_instance_input_dicts]
 
 
 def _select_which_when(

lib/galaxy/tool_util/verify/parse.py

@@ -80,10 +80,17 @@ def parse_tool_test_descriptions(
                 tool_parameter_bundle = input_models_for_tool_source(tool_source)
                 parameters = tool_parameter_bundle.parameters
             validated_test_case = case_state(raw_test_dict, parameters, profile, validate=validate_on_load)
-            request_and_schema = TestRequestAndSchema(
-                validated_test_case.tool_state,
-                tool_parameter_bundle or ToolParameterBundleModel(parameters=parameters),
-            )
+            if validated_test_case.unhandled_inputs:
+                # Inputs that map to no parameter (e.g. legacy unqualified repeats) can't be
+                # represented in a modern request; fall back to the legacy API.
+                validation_skipped_reason = (
+                    f"could not build request: unhandled inputs {validated_test_case.unhandled_inputs}"
+                )
+            else:
+                request_and_schema = TestRequestAndSchema(
+                    validated_test_case.tool_state,
+                    tool_parameter_bundle or ToolParameterBundleModel(parameters=parameters),
+                )
         except Exception as e:
             if validate_on_load:
                 validation_exception = e

test/functional/tools/async_min_repeat_unqualified.xml

@@ -0,0 +1,38 @@
+<tool id="async_min_repeat_unqualified" name="async_min_repeat_unqualified" version="1.0.0" profile="22.01">
+    <description>async regression: unqualified repeat param on a repeat with min set</description>
+    <!--
+      The multi_repeats anti-pattern (a param inside a <repeat> supplied at the top
+      level of the test by its bare name), with one addition: the repeat sets min="1".
+
+      Sync (POST /api/tools, the default GALAXY_TEST_USE_LEGACY_TOOL_API=always): the
+      legacy path maps the bare name into the repeat; job runs.
+
+      Async (GALAXY_TEST_USE_LEGACY_TOOL_API=if_needed): min="1" pads an empty repeat
+      instance, whose visited-but-unresolved path `queries_0|input2` is then read as
+      proof the bare `input2` was handled. unhandled_inputs comes back empty, so no
+      fallback to the legacy API happens and the request is built and submitted as
+      {"queries": [{}]} - the value is silently dropped and the job request fails:
+        queries.0.input2 - Field required
+
+      Drop min="1" and the same tool degrades correctly: the input is reported
+      unhandled, no request is built, and the test falls back to the legacy API.
+      See async_repeat_unqualified_no_min.xml for that control case.
+    -->
+    <command><![CDATA[
+cat #for $q in $queries# '${q.input2}' #end for# > '$out_file1'
+    ]]></command>
+    <inputs>
+        <repeat name="queries" title="Query" min="1">
+            <param name="input2" type="data" format="txt" label="Select" />
+        </repeat>
+    </inputs>
+    <outputs>
+        <data name="out_file1" format="txt" />
+    </outputs>
+    <tests>
+        <test>
+            <param name="input2" value="simple_line.txt" />
+            <output name="out_file1" file="simple_line.txt" />
+        </test>
+    </tests>
+</tool>

test/functional/tools/async_repeat_unqualified_no_min.xml

@@ -0,0 +1,32 @@
+<tool id="async_repeat_unqualified_no_min" name="async_repeat_unqualified_no_min" version="1.0.0" profile="22.01">
+    <description>async control: unqualified repeat param, no min on the repeat</description>
+    <!--
+      Control for async_min_repeat_unqualified.xml - byte-for-byte the same tool
+      except the repeat has no min attribute.
+
+      Async (GALAXY_TEST_USE_LEGACY_TOOL_API=if_needed): nothing pads an empty
+      instance, so the bare `input2` is correctly reported in unhandled_inputs, no
+      request is built, and the interactor falls back to the legacy API. Job runs.
+
+      This is the intended post-#23170 behaviour for the whole class of legacy
+      unqualified-repeat tests. It passing while async_min_repeat_unqualified fails
+      is what isolates `min` as the trigger.
+    -->
+    <command><![CDATA[
+cat #for $q in $queries# '${q.input2}' #end for# > '$out_file1'
+    ]]></command>
+    <inputs>
+        <repeat name="queries" title="Query">
+            <param name="input2" type="data" format="txt" label="Select" />
+        </repeat>
+    </inputs>
+    <outputs>
+        <data name="out_file1" format="txt" />
+    </outputs>
+    <tests>
+        <test>
+            <param name="input2" value="simple_line.txt" />
+            <output name="out_file1" file="simple_line.txt" />
+        </test>
+    </tests>
+</tool>

test/functional/tools/sample_tool_conf.xml

@@ -125,6 +125,8 @@
   <tool file="async_nested_conditional_shared_discriminator.xml" />
   <tool file="async_nested_conditional_shared_discriminator_bare.xml" />
   <tool file="async_posix_regex_validator.xml" />
+  <tool file="async_min_repeat_unqualified.xml" />
+  <tool file="async_repeat_unqualified_no_min.xml" />
   <tool file="output_empty_work_dir.xml" />
   <tool file="output_filter_with_input.xml" />
   <tool file="output_filter_with_input_optional.xml" />

test/unit/tool_util/test_parameter_test_cases.py

@@ -47,6 +47,8 @@ TOOLS_THAT_USE_UNQUALIFIED_PARAMETER_ACCESS = [
     "disambiguate_cond.xml",
     "multi_repeats.xml",
     "implicit_default_conds.xml",
+    "async_min_repeat_unqualified.xml",
+    "async_repeat_unqualified_no_min.xml",
 ]
 
 TOOLS_THAT_USE_SELECT_BY_VALUE = [
@@ -420,19 +422,22 @@ def test_legacy_partial_conditional_paths_are_resolved_for_request_state():
     dict_verify_each(tool_state.input_state, expectations)
 
 
-def test_legacy_unqualified_repeat_inputs_are_expanded_for_request_state():
+def test_legacy_unqualified_repeat_inputs_are_not_expanded():
+    # Unqualified repeat params (the multi_repeats anti-pattern) are no longer synthesized into
+    # repeat instances; use explicit <repeat> tags. The bare param is rejected on validation.
     tool_source = tool_source_for("multi_repeats")
     test_cases = tool_source.parse_tests_to_dict()["tests"]
+    with pytest.raises(Exception, match="Invalid parameter name found input2"):
+        case_state_for(tool_source, test_cases[2])
 
-    test_case_state = case_state_for(tool_source, test_cases[2]).tool_state
 
-    expectations = [
-        (["queries", 0, "input2", "path"], "simple_line.txt"),
-        (["queries", 1, "input2", "path"], "simple_line.txt"),
-        (["more_queries", 0, "more_queries_input", "path"], "simple_line.txt"),
-        (["more_queries", 1, "more_queries_input", "path"], "simple_line.txt"),
-    ]
-    dict_verify_each(test_case_state.input_state, expectations)
+def test_unrepresentable_test_case_falls_back_to_legacy_request():
+    # A test whose inputs can't be represented in a modern request (unqualified repeats) yields no
+    # request, so the interactor falls back to the legacy tool API rather than erroring.
+    tests = list(parse_tool_test_descriptions(tool_source_for("multi_repeats")))
+    description = tests[2].to_dict()
+    assert description["error"] is False
+    assert description["request"] is None
 
 
 def test_legacy_unqualified_repeat_inside_conditional_is_resolved():