Fix issues with broadcast string coercion; address code review

cylc/flow/parsec/validate.py

@@ -26,7 +26,7 @@
 import shlex
 from collections import deque
 from textwrap import dedent
-from typing import List, Dict, Any, Tuple
+from typing import List, Dict, Any, Optional, Tuple
 
 from metomi.isodatetime.data import Duration, TimePoint
 from metomi.isodatetime.dumpers import TimePointDumper
@@ -511,6 +511,22 @@ def parse_int_range(cls, value):
         else:
             return None
 
+    @classmethod
+    def _unquote(cls, keys: List[str], value: str) -> Optional[str]:
+        """Unquote value."""
+        for substr, rec in (
+            ("'''", cls._REC_MULTI_LINE_SINGLE),
+            ('"""', cls._REC_MULTI_LINE_DOUBLE),
+            ('"', cls._REC_DQ_VALUE),
+            ("'", cls._REC_SQ_VALUE)
+        ):
+            if value.startswith(substr):
+                match = rec.match(value)
+                if not match:
+                    raise IllegalValueError("string", keys, value)
+                return match[1]
+        return None
+
     @classmethod
     def strip_and_unquote(cls, keys: List[str], value: str) -> str:
         """Remove leading and trailing spaces and unquote value.
@@ -529,25 +545,13 @@ def strip_and_unquote(cls, keys: List[str], value: str) -> str:
             'foo'
 
         """
-        for substr, rec in (
-            ("'''", cls._REC_MULTI_LINE_SINGLE),
-            ('"""', cls._REC_MULTI_LINE_DOUBLE),
-            ('"', cls._REC_DQ_VALUE),
-            ("'", cls._REC_SQ_VALUE)
-        ):
-            if value.startswith(substr):
-                match = rec.match(value)
-                if not match:
-                    raise IllegalValueError("string", keys, value)
-                value = match.groups()[0]
-                break
-        else:  # no break
-            # unquoted
-            value = value.split(r'#', 1)[0]
+        val = cls._unquote(keys, value)
+        if val is None:
+            val = value.split(r'#', 1)[0]
 
         # Note strip() removes leading and trailing whitespace, including
         # initial newlines on a multiline string:
-        return dedent(value).strip()
+        return dedent(val).strip()
 
     @classmethod
     def strip_and_unquote_list(cls, keys, value):
@@ -1144,17 +1148,17 @@ def __init__(self):
 
     @classmethod
     def coerce_str(cls, value, keys) -> str:
-        """Coerce value to a string.
+        """Coerce value to a string. Unquotes & strips lead/trail whitespace.
+
+        Prevents ParsecValidator from assuming '#' means comments;
+        '#' has valid uses in shell script such as parameter substitution.
 
         Examples:
-            >>> BroadcastConfigValidator.coerce_str('abc#def', None)
-            'abc#def'
+            >>> BroadcastConfigValidator.coerce_str('echo "${FOO#*bar}"', None)
+            'echo "${FOO#*bar}"'
         """
-        # Prevent ParsecValidator from assuming '#' means comments;
-        # '#' has valid uses in shell script such as parameter substitution
-        if isinstance(value, str) and '#' in value:
-            value = f'"{value}"'
-        return ParsecValidator.coerce_str(value, keys)
+        val = ParsecValidator._unquote(keys, value) or value
+        return dedent(val).strip()
 
     @classmethod
     def strip_and_unquote_list(cls, keys, value):
@@ -1183,7 +1187,7 @@ def strip_and_unquote_list(cls, keys, value):
     # BACK COMPAT: BroadcastConfigValidator.coerce_interval
     # The DB at 8.0.x stores Interval values as neither ISO8601 duration
     # string or DurationFloat. This has been fixed at 8.1.0, and
-    # the following class acts as a bridge between fixed and broken.
+    # the following method acts as a bridge between fixed and broken.
     # url:
     #     https://github.com/cylc/cylc-flow/pull/5138
     # from:

tests/functional/broadcast/10-file-1/broadcast.cylc

@@ -1,7 +1,8 @@
 script="""
 printenv CYLC_FOOBAR
 
-# (This hash char should not cause the rest of the script to be stripped out)
+# This hash char should not cause the rest of the script to be stripped out
+# - https://github.com/cylc/cylc-flow/pull/5933
 
 if (($CYLC_TASK_TRY_NUMBER < 2 )); then
     false

tests/unit/parsec/test_validate.py

@@ -18,12 +18,13 @@
 from typing import List
 
 import pytest
-from pytest import approx
+from pytest import approx, param
 
 from cylc.flow.parsec.config import ConfigNode as Conf
 from cylc.flow.parsec.OrderedDict import OrderedDictWithDefaults
 from cylc.flow.parsec.exceptions import IllegalValueError
 from cylc.flow.parsec.validate import (
+    BroadcastConfigValidator,
     CylcConfigValidator as VDR,
     DurationFloat,
     ListValueError,
@@ -502,9 +503,51 @@ def test_coerce_str_list():
         assert validator.coerce_str_list(value, ['whatever']) == results
 
 
-def test_strip_and_unquote():
+@pytest.mark.parametrize('value, expected', [
+    param(
+        "'a'",
+        'a',
+        id="single quotes"
+    ),
+    param(
+        '"\'a\'"',
+        "'a'",
+        id="single quotes inside double quotes"
+    ),
+    param(
+        '" a b" # comment',
+        ' a b',
+        id="comment outside"
+    ),
+    param(
+        '"""bene\ngesserit"""',
+        'bene\ngesserit',
+        id="multiline double quotes"
+    ),
+    param(
+        "'''kwisatz\n  haderach'''",
+        'kwisatz\n  haderach',
+        id="multiline single quotes"
+    ),
+    param(
+        '"""a\nb"""  # comment',
+        'a\nb',
+        id="multiline with comment outside"
+    ),
+])
+def test_unquote(value: str, expected: str):
+    """Test strip_and_unquote."""
+    assert ParsecValidator._unquote(['a'], value) == expected
+
+
+@pytest.mark.parametrize('value', [
+    '"""',
+    "'''",
+    "'don't do this'",
+])
+def test_strip_and_unquote__bad(value: str):
     with pytest.raises(IllegalValueError):
-        ParsecValidator.strip_and_unquote(['a'], '"""')
+        ParsecValidator.strip_and_unquote(['a'], value)
 
 
 def test_strip_and_unquote_list_parsec():
@@ -696,3 +739,23 @@ def test_type_help_examples():
                 except Exception:
                     raise Exception(
                         f'Example "{example}" failed for type "{vdr}"')
+
+
+@pytest.mark.parametrize('value, expected', [
+    param(
+        """
+        a="don't have a cow"
+        a=${a#*have}
+        echo "$a" # let's see what happens
+        """,
+        "a=\"don't have a cow\"\na=${a#*have}\necho \"$a\" # let's see what happens",
+        id="multiline"
+    ),
+    param(
+        '"sleep 30 # ja!"  ',
+        'sleep 30 # ja!',
+        id="quoted"
+    ),
+])
+def test_broadcast_coerce_str(value: str, expected: str):
+    assert BroadcastConfigValidator.coerce_str(value, ['whatever']) == expected