mirror of
https://github.com/galaxyproject/galaxy.git
synced 2026-10-29 17:42:12 +08:00
Fix optional text default_value handling across input formats
strictify() and _fill_default_for() in convert.py ignored
parameter.default_value for optional text params, hardcoding None
for absent values. Additionally both used `or` which swallowed
empty string defaults due to Python falsiness.
This caused inconsistent behavior for absent params on tools with
value="" across input formats:
| Input | optional (no default) | optional value="" | optional value="" + regex |
|-------|----------------------|-------------------|--------------------------|
| {} | None | "" | "" |
| None | None | None | None |
| "" | "" | "" | "" |
| valid | value | value | value |
| bad | value | value | fails |
All rows now consistent across legacy, 21.01, and request formats.
Test tools added:
- gx_text_optional_with_empty_default (optional text, value="", no validator)
- gx_text_optional_with_empty_default_regex_validation (the wild case)
- gx_text_optional_regex_validation updated to have no default
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
John Chilton
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
ecb803ddae
commit
a557064111
@@ -195,14 +195,14 @@ def strictify(relaxed_state: RelaxedRequestToolState, input_models: ToolParamete
|
||||
if not parameter.optional:
|
||||
# restore legacy behavior of allowing empty string implicit default
|
||||
# for these non-optional inputs.
|
||||
tool_state[parameter_name] = ""
|
||||
tool_state[parameter_name] = parameter.default_value if parameter.default_value is not None else ""
|
||||
else:
|
||||
tool_state[parameter_name] = None
|
||||
tool_state[parameter_name] = parameter.default_value
|
||||
else:
|
||||
# legacy behavior of converting explicit None into implicit null. We should introduce
|
||||
# a layer somewhere to deal with this behavior further up the stack and clean up these models.
|
||||
if not parameter.optional and tool_state[parameter_name] is None:
|
||||
tool_state[parameter_name] = ""
|
||||
tool_state[parameter_name] = parameter.default_value if parameter.default_value is not None else ""
|
||||
|
||||
def _strictify_parameters(tool_state: Dict[str, Any], input_models: ToolParameterBundle) -> None:
|
||||
for parameter in input_models.parameters:
|
||||
@@ -399,14 +399,14 @@ def _fill_default_for(tool_state: Dict[str, Any], parameter: ToolParameterT) ->
|
||||
if not parameter.optional:
|
||||
# restore legacy behavior of allowing empty string implicit default
|
||||
# for these non-optional inputs.
|
||||
tool_state[parameter_name] = parameter.default_value or ""
|
||||
tool_state[parameter_name] = parameter.default_value if parameter.default_value is not None else ""
|
||||
else:
|
||||
tool_state[parameter_name] = parameter.default_value or None
|
||||
tool_state[parameter_name] = parameter.default_value
|
||||
else:
|
||||
# legacy behavior of converting explicit None into implicit null. We should introduce
|
||||
# a layer somewhere to deal with this behavior further up the stack and clean up these models.
|
||||
if not parameter.optional and tool_state[parameter_name] is None:
|
||||
tool_state[parameter_name] = parameter.default_value or ""
|
||||
tool_state[parameter_name] = parameter.default_value if parameter.default_value is not None else ""
|
||||
|
||||
|
||||
def _initialize_section_state(parameter: SectionParameterModel, tool_state: Dict[str, Any]) -> Dict[str, Any]:
|
||||
|
||||
@@ -735,15 +735,58 @@ def test_null_to_text_tool_with_validation(required_tool: RequiredTool, tool_inp
|
||||
required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": ""})).assert_fails()
|
||||
|
||||
|
||||
@requires_tool_id("gx_text_optional_with_empty_default")
|
||||
def test_optional_text_with_empty_default(required_tool: RequiredTool, tool_input_format: DescribeToolInputs):
|
||||
# absent — all formats resolve to value="" default
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": ""})
|
||||
|
||||
# explicit null
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": None}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": None})
|
||||
|
||||
# explicit empty string
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": ""}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": ""})
|
||||
|
||||
# provided value
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": "hello"}))
|
||||
execute.assert_has_single_job.with_output("output").with_contents_stripped("hello")
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": "hello"})
|
||||
|
||||
|
||||
@requires_tool_id("gx_text_optional_regex_validation")
|
||||
def test_optional_text_with_regex_validation(required_tool: RequiredTool, tool_input_format: DescribeToolInputs):
|
||||
# absent param — legacy/21.01 resolve to value="" default, runtime skips regex for empty optional
|
||||
# absent — no value="" default so all formats treat as None
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json(
|
||||
{"parameter": None} if tool_input_format.is_request else {"parameter": ""}
|
||||
)
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": None})
|
||||
|
||||
# explicit null — all formats treat as "not provided" for optional
|
||||
# explicit null
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": None}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": None})
|
||||
|
||||
# explicit empty string — optional so skip regex validation, "" stays as ""
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": ""}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": ""})
|
||||
|
||||
# valid value
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": "0.5"}))
|
||||
execute.assert_has_single_job.with_output("output").with_contents_stripped("0.5")
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": "0.5"})
|
||||
|
||||
# invalid value should fail regardless of format
|
||||
required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": "abc"})).assert_fails()
|
||||
|
||||
|
||||
@requires_tool_id("gx_text_optional_with_empty_default_regex_validation")
|
||||
def test_optional_text_with_empty_default_with_regex_validation(
|
||||
required_tool: RequiredTool, tool_input_format: DescribeToolInputs
|
||||
):
|
||||
# absent — all formats resolve to value="" default
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": ""})
|
||||
|
||||
# explicit null
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": None}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": None})
|
||||
|
||||
@@ -751,7 +794,7 @@ def test_optional_text_with_regex_validation(required_tool: RequiredTool, tool_i
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": ""}))
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": ""})
|
||||
|
||||
# valid value should pass all formats
|
||||
# valid value
|
||||
execute = required_tool.execute().with_inputs(tool_input_format.when.any({"parameter": "0.5"}))
|
||||
execute.assert_has_single_job.with_output("output").with_contents_stripped("0.5")
|
||||
execute.assert_has_single_job.with_output("inputs_json").with_json({"parameter": "0.5"})
|
||||
|
||||
@@ -7,7 +7,7 @@ cat '$inputs' >> $inputs_json;
|
||||
<inputs name="inputs" filename="inputs.json" />
|
||||
</configfiles>
|
||||
<inputs>
|
||||
<param name="parameter" type="text" value="" optional="true">
|
||||
<param name="parameter" type="text" optional="true">
|
||||
<validator type="regex">[0-9.:]+</validator>
|
||||
</param>
|
||||
</inputs>
|
||||
|
||||
@@ -0,0 +1,18 @@
|
||||
<tool id="gx_text_optional_with_empty_default" name="gx_text_optional_with_empty_default" version="1.0.0">
|
||||
<command><![CDATA[
|
||||
echo '$parameter' >> '$output';
|
||||
cat '$inputs' >> $inputs_json;
|
||||
]]></command>
|
||||
<configfiles>
|
||||
<inputs name="inputs" filename="inputs.json" />
|
||||
</configfiles>
|
||||
<inputs>
|
||||
<param name="parameter" type="text" value="" optional="true" />
|
||||
</inputs>
|
||||
<outputs>
|
||||
<data name="output" format="txt" />
|
||||
<data name="inputs_json" format="json" />
|
||||
</outputs>
|
||||
<tests>
|
||||
</tests>
|
||||
</tool>
|
||||
+20
@@ -0,0 +1,20 @@
|
||||
<tool id="gx_text_optional_with_empty_default_regex_validation" name="gx_text_optional_with_empty_default_regex_validation" version="1.0.0">
|
||||
<command><![CDATA[
|
||||
echo '$parameter' >> '$output';
|
||||
cat '$inputs' >> $inputs_json;
|
||||
]]></command>
|
||||
<configfiles>
|
||||
<inputs name="inputs" filename="inputs.json" />
|
||||
</configfiles>
|
||||
<inputs>
|
||||
<param name="parameter" type="text" value="" optional="true">
|
||||
<validator type="regex">[0-9.:]+</validator>
|
||||
</param>
|
||||
</inputs>
|
||||
<outputs>
|
||||
<data name="output" format="txt" />
|
||||
<data name="inputs_json" format="json" />
|
||||
</outputs>
|
||||
<tests>
|
||||
</tests>
|
||||
</tool>
|
||||
Reference in New Issue
Block a user