From 0440d46232d811de7f163b9022ab48b3552847cd Mon Sep 17 00:00:00 2001 From: LanderOtto Date: Fri, 19 Dec 2025 16:00:55 +0100 Subject: [PATCH 1/3] Handled the `pickValue` in the `check_types` function to avoid unnecessary warnings. --- cwl_utils/parser/cwl_v1_2_utils.py | 66 +++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 15 deletions(-) diff --git a/cwl_utils/parser/cwl_v1_2_utils.py b/cwl_utils/parser/cwl_v1_2_utils.py index e40f5877..2ed46093 100644 --- a/cwl_utils/parser/cwl_v1_2_utils.py +++ b/cwl_utils/parser/cwl_v1_2_utils.py @@ -210,6 +210,7 @@ def check_all_types( extra_message = ( "pickValue is %s" % sink.pickValue if sink.pickValue is not None else None ) + sink_type = type_dict[sink.id] match sink: case cwl.WorkflowOutputParameter(): sourceName = "outputSource" @@ -231,7 +232,10 @@ def check_all_types( srcs_of_sink += [src_dict[parm_id]] if ( _is_conditional_step(param_to_step, parm_id) - and sink.pickValue is not None + and "null" != sink_type + and isinstance(sink_type, MutableSequence) + and "null" not in sink_type + and sink.pickValue is None ): validation["warning"].append( SrcSink( @@ -289,8 +293,9 @@ def check_all_types( for src in srcs_of_sink: check_result = check_types( type_dict[cast(str, src.id)], - type_dict[sink.id], + sink_type, linkMerge, + sink.pickValue, getattr(sink, "valueFrom", None), ) if check_result in ("warning", "exception"): @@ -304,6 +309,7 @@ def check_types( srctype: Any, sinktype: Any, linkMerge: str | None, + pickValue: str | None = None, valueFrom: str | None = None, ) -> str: """ @@ -313,19 +319,49 @@ def check_types( """ if valueFrom is not None: return "pass" - if linkMerge is None: - if can_assign_src_to_sink(srctype, sinktype, strict=True): - return "pass" - if can_assign_src_to_sink(srctype, sinktype, strict=False): - return "warning" - return "exception" - if linkMerge == "merge_nested": - return check_types( - cwl.ArraySchema(items=srctype, type_="array"), sinktype, None, None - ) - if linkMerge == "merge_flattened": - return check_types(merge_flatten_type(srctype), sinktype, None, None) - raise ValidationException(f"Invalid value {linkMerge} for linkMerge field.") + if pickValue is not None: + if isinstance(srctype, cwl.ArraySchema): + match pickValue: + case "all_non_null": + srctype = cwl.ArraySchema(items=srctype.items, type_="array") + if ( + isinstance(srctype.items, MutableSequence) + and "null" in srctype.items + ): + srctype.items = [ + elem for elem in srctype.items if elem != "null" + ] + case "first_non_null" | "the_only_non_null": + if ( + isinstance(srctype.items, MutableSequence) + and "null" in srctype.items + ): + srctype = [elem for elem in srctype.items if elem != "null"] + else: + srctype = srctype.items + case _: + raise ValidationException( + f"Invalid value {pickValue} for pickValue field." + ) + match linkMerge: + case None: + if can_assign_src_to_sink(srctype, sinktype, strict=True): + return "pass" + if can_assign_src_to_sink(srctype, sinktype, strict=False): + return "warning" + return "exception" + case "merge_nested": + return check_types( + cwl.ArraySchema(items=srctype, type_="array"), + sinktype, + None, + None, + None, + ) + case "merge_flattened": + return check_types(merge_flatten_type(srctype), sinktype, None, None, None) + case _: + raise ValidationException(f"Invalid value {linkMerge} for linkMerge field.") def content_limit_respected_read_bytes(f: IO[bytes]) -> bytes: From aac97413cfae31b1e7b78085146f2b116e694e03 Mon Sep 17 00:00:00 2001 From: LanderOtto Date: Fri, 19 Dec 2025 17:26:34 +0100 Subject: [PATCH 2/3] Handled the `pickValue` in the `check_types` function to avoid unnecessary warnings. --- cwl_utils/parser/cwl_v1_2_utils.py | 29 +---------------------------- 1 file changed, 1 insertion(+), 28 deletions(-) diff --git a/cwl_utils/parser/cwl_v1_2_utils.py b/cwl_utils/parser/cwl_v1_2_utils.py index 2ed46093..d8ee9009 100644 --- a/cwl_utils/parser/cwl_v1_2_utils.py +++ b/cwl_utils/parser/cwl_v1_2_utils.py @@ -295,7 +295,6 @@ def check_all_types( type_dict[cast(str, src.id)], sink_type, linkMerge, - sink.pickValue, getattr(sink, "valueFrom", None), ) if check_result in ("warning", "exception"): @@ -309,7 +308,6 @@ def check_types( srctype: Any, sinktype: Any, linkMerge: str | None, - pickValue: str | None = None, valueFrom: str | None = None, ) -> str: """ @@ -319,30 +317,6 @@ def check_types( """ if valueFrom is not None: return "pass" - if pickValue is not None: - if isinstance(srctype, cwl.ArraySchema): - match pickValue: - case "all_non_null": - srctype = cwl.ArraySchema(items=srctype.items, type_="array") - if ( - isinstance(srctype.items, MutableSequence) - and "null" in srctype.items - ): - srctype.items = [ - elem for elem in srctype.items if elem != "null" - ] - case "first_non_null" | "the_only_non_null": - if ( - isinstance(srctype.items, MutableSequence) - and "null" in srctype.items - ): - srctype = [elem for elem in srctype.items if elem != "null"] - else: - srctype = srctype.items - case _: - raise ValidationException( - f"Invalid value {pickValue} for pickValue field." - ) match linkMerge: case None: if can_assign_src_to_sink(srctype, sinktype, strict=True): @@ -356,10 +330,9 @@ def check_types( sinktype, None, None, - None, ) case "merge_flattened": - return check_types(merge_flatten_type(srctype), sinktype, None, None, None) + return check_types(merge_flatten_type(srctype), sinktype, None, None) case _: raise ValidationException(f"Invalid value {linkMerge} for linkMerge field.") From 69f57db99846d3088811ebfd05639092c84066ed Mon Sep 17 00:00:00 2001 From: LanderOtto Date: Tue, 23 Dec 2025 17:33:35 +0100 Subject: [PATCH 3/3] Added tests --- cwl_utils/parser/cwl_v1_2_utils.py | 32 +++++++----- .../testdata/checker_wf/no-warning-wf.cwl | 44 ++++++++++++++++ cwl_utils/testdata/checker_wf/warning-wf.cwl | 44 ++++++++++++++++ cwl_utils/testdata/checker_wf/warning-wf2.cwl | 43 ++++++++++++++++ cwl_utils/testdata/checker_wf/warning-wf3.cwl | 50 +++++++++++++++++++ cwl_utils/tests/test_parser_utils.py | 37 +++++++++++++- 6 files changed, 237 insertions(+), 13 deletions(-) create mode 100755 cwl_utils/testdata/checker_wf/no-warning-wf.cwl create mode 100755 cwl_utils/testdata/checker_wf/warning-wf.cwl create mode 100755 cwl_utils/testdata/checker_wf/warning-wf2.cwl create mode 100755 cwl_utils/testdata/checker_wf/warning-wf3.cwl diff --git a/cwl_utils/parser/cwl_v1_2_utils.py b/cwl_utils/parser/cwl_v1_2_utils.py index d8ee9009..2eb45685 100644 --- a/cwl_utils/parser/cwl_v1_2_utils.py +++ b/cwl_utils/parser/cwl_v1_2_utils.py @@ -221,7 +221,7 @@ def check_all_types( case _: continue if sourceField is not None: - if isinstance(sourceField, MutableSequence): + if isinstance(sourceField, MutableSequence) and len(sourceField) > 1: linkMerge: str | None = sink.linkMerge or ( "merge_nested" if len(sourceField) > 1 else None ) @@ -232,26 +232,34 @@ def check_all_types( srcs_of_sink += [src_dict[parm_id]] if ( _is_conditional_step(param_to_step, parm_id) - and "null" != sink_type - and isinstance(sink_type, MutableSequence) - and "null" not in sink_type and sink.pickValue is None ): - validation["warning"].append( - SrcSink( - src_dict[parm_id], - sink, - linkMerge, - message="Source is from conditional step, but pickValue is not used", + src_typ = aslist(type_dict[src_dict[parm_id].id]) + if "null" not in src_typ: + src_typ = ["null"] + cast(list[Any], src_typ) + if ( + not isinstance(sink_type, MutableSequence) + or "null" not in sink_type + ): + validation["warning"].append( + SrcSink( + src_dict[parm_id], + sink, + linkMerge, + message="Source is from conditional step, but pickValue is not used", + ) ) - ) + type_dict[src_dict[parm_id].id] = src_typ if _is_all_output_method_loop_step(param_to_step, parm_id): src_typ = type_dict[src_dict[parm_id].id] type_dict[src_dict[parm_id].id] = cwl.ArraySchema( items=src_typ, type_="array" ) else: - parm_id = cast(str, sourceField) + if isinstance(sourceField, MutableSequence): + parm_id = cast(str, sourceField[0]) + else: + parm_id = cast(str, sourceField) if parm_id not in src_dict: raise SourceLine(sink, sourceName, ValidationException).makeError( f"{sourceName} not found: {parm_id}" diff --git a/cwl_utils/testdata/checker_wf/no-warning-wf.cwl b/cwl_utils/testdata/checker_wf/no-warning-wf.cwl new file mode 100755 index 00000000..7b1e7a25 --- /dev/null +++ b/cwl_utils/testdata/checker_wf/no-warning-wf.cwl @@ -0,0 +1,44 @@ +#!/usr/bin/env cwl-runner +class: Workflow +cwlVersion: v1.2 +requirements: + ScatterFeatureRequirement: {} + MultipleInputFeatureRequirement: {} + StepInputExpressionRequirement: {} +inputs: + letters0: + type: [string, int] + default: "a0" + letters1: + type: string[] + default: ["a1", "b1"] + letters2: + type: [string, int] + default: "a2" + letters3: + type: string[] + default: ["a3", "b3"] + letters4: + type: string + default: "a4" + letters5: + type: string[] + default: ["a5", "b5", "c5"] + exec: + type: bool + default: False + +outputs: + single: + type: File? + outputSource: + - step1/txt + +steps: + - id: step1 + run: echo.cwl + in: + exec: exec + out: [txt] + when: $(inputs.exec) + diff --git a/cwl_utils/testdata/checker_wf/warning-wf.cwl b/cwl_utils/testdata/checker_wf/warning-wf.cwl new file mode 100755 index 00000000..1afb3e26 --- /dev/null +++ b/cwl_utils/testdata/checker_wf/warning-wf.cwl @@ -0,0 +1,44 @@ +#!/usr/bin/env cwl-runner +class: Workflow +cwlVersion: v1.2 +requirements: + ScatterFeatureRequirement: {} + MultipleInputFeatureRequirement: {} + StepInputExpressionRequirement: {} +inputs: + letters0: + type: [string, int] + default: "a0" + letters1: + type: string[] + default: ["a1", "b1"] + letters2: + type: [string, int] + default: "a2" + letters3: + type: string[] + default: ["a3", "b3"] + letters4: + type: string + default: "a4" + letters5: + type: string[] + default: ["a5", "b5", "c5"] + exec: + type: bool + default: False + +outputs: + single: + type: File + outputSource: + - step1/txt + +steps: + - id: step1 + run: echo.cwl + in: + exec: exec + out: [txt] + when: $(inputs.exec) + diff --git a/cwl_utils/testdata/checker_wf/warning-wf2.cwl b/cwl_utils/testdata/checker_wf/warning-wf2.cwl new file mode 100755 index 00000000..10ef7f3a --- /dev/null +++ b/cwl_utils/testdata/checker_wf/warning-wf2.cwl @@ -0,0 +1,43 @@ +#!/usr/bin/env cwl-runner +class: Workflow +cwlVersion: v1.2 +requirements: + ScatterFeatureRequirement: {} + MultipleInputFeatureRequirement: {} + StepInputExpressionRequirement: {} +inputs: + letters0: + type: [string, int] + default: "a0" + letters1: + type: string[] + default: ["a1", "b1"] + letters2: + type: [string, int] + default: "a2" + letters3: + type: string[] + default: ["a3", "b3"] + letters4: + type: string + default: "a4" + letters5: + type: string[] + default: ["a5", "b5", "c5"] + exec: + type: bool + default: False + +outputs: + single_a: + type: File + outputSource: step1/txt + +steps: + - id: step1 + run: echo.cwl + in: + exec: exec + out: [txt] + when: $(inputs.exec) + diff --git a/cwl_utils/testdata/checker_wf/warning-wf3.cwl b/cwl_utils/testdata/checker_wf/warning-wf3.cwl new file mode 100755 index 00000000..c2132166 --- /dev/null +++ b/cwl_utils/testdata/checker_wf/warning-wf3.cwl @@ -0,0 +1,50 @@ +#!/usr/bin/env cwl-runner +class: Workflow +cwlVersion: v1.2 +requirements: + ScatterFeatureRequirement: {} + MultipleInputFeatureRequirement: {} + StepInputExpressionRequirement: {} +inputs: + letters0: + type: [string, int] + default: "a0" + letters1: + type: string[] + default: ["a1", "b1"] + letters2: + type: [string, int] + default: "a2" + letters3: + type: string[] + default: ["a3", "b3"] + letters4: + type: string + default: "a4" + letters5: + type: string[] + default: ["a5", "b5", "c5"] + exec: + type: bool + default: False + +outputs: + pair: + type: File[] + outputSource: + - step1/txt + - step2/txt + +steps: + - id: step1 + run: echo.cwl + in: + exec: exec + out: [txt] + when: $(inputs.exec) + - id: step2 + run: echo.cwl + in: [] + out: [txt] + when: $(inputs.exec) + diff --git a/cwl_utils/tests/test_parser_utils.py b/cwl_utils/tests/test_parser_utils.py index d4d24fa7..8594b7e0 100644 --- a/cwl_utils/tests/test_parser_utils.py +++ b/cwl_utils/tests/test_parser_utils.py @@ -1,11 +1,13 @@ # SPDX-License-Identifier: Apache-2.0 """Test the CWL parsers utility functions.""" +import logging +import re import tempfile from collections.abc import MutableSequence from typing import cast import pytest -from pytest import raises +from pytest import raises, LogCaptureFixture from schema_salad.exceptions import ValidationException import cwl_utils.parser.cwl_v1_0 @@ -71,6 +73,39 @@ def test_static_checker_fail(cwlVersion: str) -> None: cwl_utils.parser.utils.static_checker(cwl_obj7) +def test_static_checker_warning(caplog: LogCaptureFixture) -> None: + uri1 = get_path("testdata/checker_wf/warning-wf.cwl").as_uri() + cwl_obj1 = load_document_by_uri(uri1) + with caplog.at_level(level=logging.WARNING, logger="cwl_utils"): + cwl_utils.parser.utils.static_checker(cwl_obj1) + assert "Source is from conditional step and may produce `null`" in caplog.text + caplog.clear() + + uri2 = get_path("testdata/checker_wf/no-warning-wf.cwl").as_uri() + cwl_obj2 = load_document_by_uri(uri2) + with caplog.at_level(level=logging.WARNING, logger="cwl_utils"): + cwl_utils.parser.utils.static_checker(cwl_obj2) + assert not caplog.text + caplog.clear() + + uri3 = get_path("testdata/checker_wf/warning-wf2.cwl").as_uri() + cwl_obj3 = load_document_by_uri(uri3) + with caplog.at_level(level=logging.WARNING, logger="cwl_utils"): + cwl_utils.parser.utils.static_checker(cwl_obj3) + assert "Source is from conditional step and may produce `null`" in caplog.text + caplog.clear() + + uri4 = get_path("testdata/checker_wf/warning-wf3.cwl").as_uri() + cwl_obj4 = load_document_by_uri(uri4) + with caplog.at_level(level=logging.WARNING, logger="cwl_utils"): + cwl_utils.parser.utils.static_checker(cwl_obj4) + assert re.search( + 'with sink \'pair\' of type {"name":.* "items": "File", "type": "array"}', + caplog.text, + ) + assert "Source is from conditional step, but pickValue is not used" in caplog.text + + @pytest.mark.parametrize("cwlVersion", ["v1_0", "v1_1", "v1_2"]) def test_static_checker_success(cwlVersion: str) -> None: """Confirm that static type checker correctly validates workflows."""