Fix bugs in cylc broadcast (#5933)

broadcast: fix issues with filepaths and hash characters

* Fix traceback when using `cylc broadcast -F` with a relative filepath
* Fix `#` char bug in `cylc broadcast`

changes.d/5933.fix.md

@@ -0,0 +1 @@
+Fixed bug in `cylc broadcast` (and the GUI Edit Runtime command) where everything after a `#` character in a setting would be stripped out.

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
@@ -373,7 +373,7 @@ def coerce_range(cls, value, keys):
         return Range((int(min_), int(max_)))
 
     @classmethod
-    def coerce_str(cls, value, keys):
+    def coerce_str(cls, value, keys) -> str:
         """Coerce value to a string.
 
         Examples:
@@ -385,7 +385,7 @@ def coerce_str(cls, value, keys):
         """
         if isinstance(value, list):
             # handle graph string merging
-            vraw = []
+            vraw: List[str] = []
             vals = [value]
             while vals:
                 val = vals.pop()
@@ -512,41 +512,46 @@ def parse_int_range(cls, value):
             return None
 
     @classmethod
-    def strip_and_unquote(cls, keys, value):
+    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.
 
         Args:
-            keys (list):
+            keys:
                 Keys in nested dict that represents the raw configuration.
-            value (str):
+            value:
                 String value in raw configuration.
 
-        Return (str):
+        Return:
             Processed value.
 
         Examples:
             >>> ParsecValidator.strip_and_unquote(None, '" foo "')
             '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:
-            # 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):
@@ -1136,23 +1141,25 @@ def _coerce_type(cls, value):
         return val
 
 
-# BACK COMPAT: BroadcastConfigValidator
-# 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.
-# url:
-#     https://github.com/cylc/cylc-flow/pull/5138
-# from:
-#    8.0.x
-# to:
-#    8.1.x
-# remove at:
-#    8.x
 class BroadcastConfigValidator(CylcConfigValidator):
     """Validate and Coerce DB loaded broadcast config to internal objects."""
     def __init__(self):
         CylcConfigValidator.__init__(self)
 
+    @classmethod
+    def coerce_str(cls, value, keys) -> str:
+        """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('echo "${FOO#*bar}"', None)
+            'echo "${FOO#*bar}"'
+        """
+        val = ParsecValidator._unquote(keys, value) or value
+        return dedent(val).strip()
+
     @classmethod
     def strip_and_unquote_list(cls, keys, value):
         """Remove leading and trailing spaces and unquote list value.
@@ -1177,6 +1184,18 @@ def strip_and_unquote_list(cls, keys, value):
             value = value.lstrip('[').rstrip(']')
         return ParsecValidator.strip_and_unquote_list(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 method acts as a bridge between fixed and broken.
+    # url:
+    #     https://github.com/cylc/cylc-flow/pull/5138
+    # from:
+    #    8.0.x
+    # to:
+    #    8.1.x
+    # remove at:
+    #    8.x
     @classmethod
     def coerce_interval(cls, value, keys):
         """Coerce an ISO 8601 interval into seconds.

cylc/flow/scripts/broadcast.py

@@ -82,6 +82,7 @@
 from ansimarkup import parse as cparse
 from copy import deepcopy
 from functools import partial
+import os.path
 import re
 import sys
 from tempfile import NamedTemporaryFile
@@ -203,7 +204,7 @@ def files_to_settings(settings, setting_files, cancel_mode=False):
                 handle.seek(0, 0)
                 cfg.loadcfg(handle.name)
         else:
-            cfg.loadcfg(setting_file)
+            cfg.loadcfg(os.path.abspath(setting_file))
     stack = [([], cfg.get(sparse=True))]
     while stack:
         keys, item = stack.pop()

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

@@ -1,6 +1,9 @@
 script="""
 printenv CYLC_FOOBAR
 
+# 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
 fi

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,
@@ -456,11 +457,9 @@ def test_coerce_int_list():
             validator.coerce_int_list(value, ['whatever'])
 
 
-def test_coerce_str():
-    """Test coerce_str."""
-    validator = ParsecValidator()
-    # The good
-    for value, result in [
+@pytest.mark.parametrize(
+    'value, expected',
+    [
         ('', ''),
         ('Hello World!', 'Hello World!'),
         ('"Hello World!"', 'Hello World!'),
@@ -474,9 +473,15 @@ def test_coerce_str():
          'Hello:\n    foo\nGreet\n    baz'),
         ('False', 'False'),
         ('None', 'None'),
-        (['a', 'b'], 'a\nb')
-    ]:
-        assert validator.coerce_str(value, ['whatever']) == result
+        (['a', 'b'], 'a\nb'),
+        ('abc#def', 'abc'),
+    ]
+)
+def test_coerce_str(value: str, expected: str):
+    """Test coerce_str."""
+    validator = ParsecValidator()
+    # The good
+    assert validator.coerce_str(value, ['whatever']) == expected
 
 
 def test_coerce_str_list():
@@ -498,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():
@@ -692,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