Config path quote handling: keys with quotes (#40976)

As observed in #40944, when using `spack config add <path>`, the `path` might
contain keys that are enclosed in quotes.

This was broken in https://github.com/spack/spack/pull/39831, which assumed that
only the value (if present, the final element of the path) would use quotes.

This preserves the primary intended behavior of #39931 (allowing ":" in values when
using `spack config add`) while also allowing quotes on keys.

This has complicated the function `process_config_path`, but:
* It is not used outside of `config.py`
* The docstring has been updated to account for this
* Created an object to formalize the DSL, added a test for that, and
  refactored parsing to make use of regular expressions as well.
* Updated the parsing and also updated the `config_path_dsl` test with an explicit check.
  At a higher level, split the parsing to check if something is either a key or not:
  * in the first case, it is covered by a regex
  * in the second, it may be a YAML value, but in that case it would have to be the last
    entry of x:y:z, so in that case I attempt to use the YAML handling logic to parse it as such

lib/spack/spack/config.py

@@ -638,7 +638,6 @@ def get(self, path: str, default: Optional[Any] = None, scope: Optional[str] = N
 
         We use ``:`` as the separator, like YAML objects.
         """
-        # TODO: Currently only handles maps. Think about lists if needed.
         parts = process_config_path(path)
         section = parts.pop(0)
 
@@ -883,7 +882,9 @@ def add(fullpath: str, scope: Optional[str] = None) -> None:
     has_existing_value = True
     path = ""
     override = False
-    value = syaml.load_config(components[-1])
+    value = components[-1]
+    if not isinstance(value, syaml.syaml_str):
+        value = syaml.load_config(value)
     for idx, name in enumerate(components[:-1]):
         # First handle double colons in constructing path
         colon = "::" if override else ":" if path else ""
@@ -905,7 +906,7 @@ def add(fullpath: str, scope: Optional[str] = None) -> None:
 
             # construct value from this point down
             for component in reversed(components[idx + 1 : -1]):
-                value = {component: value}
+                value: Dict[str, str] = {component: value}  # type: ignore[no-redef]
             break
 
     if override:
@@ -916,7 +917,7 @@ def add(fullpath: str, scope: Optional[str] = None) -> None:
 
     # append values to lists
     if isinstance(existing, list) and not isinstance(value, list):
-        value = [value]
+        value: List[str] = [value]  # type: ignore[no-redef]
 
     # merge value into existing
     new = merge_yaml(existing, value)
@@ -1336,56 +1337,141 @@ def they_are(t):
     return copy.copy(source)
 
 
+class ConfigPath:
+    quoted_string = "(?:\"[^\"]+\")|(?:'[^']+')"
+    unquoted_string = "[^:'\"]+"
+    element = rf"(?:(?:{quoted_string})|(?:{unquoted_string}))"
+    next_key_pattern = rf"({element}[+-]?)(?:\:|$)"
+
+    @staticmethod
+    def _split_front(string, extract):
+        m = re.match(extract, string)
+        if not m:
+            return None, None
+        token = m.group(1)
+        return token, string[len(token) :]
+
+    @staticmethod
+    def _validate(path):
+        """Example valid config paths:
+
+        x:y:z
+        x:"y":z
+        x:y+:z
+        x:y::z
+        x:y+::z
+        x:y:
+        x:y::
+        """
+        first_key, path = ConfigPath._split_front(path, ConfigPath.next_key_pattern)
+        if not first_key:
+            raise ValueError(f"Config path does not start with a parse-able key: {path}")
+        path_elements = [first_key]
+        path_index = 1
+        while path:
+            separator, path = ConfigPath._split_front(path, r"(\:+)")
+            if not separator:
+                raise ValueError(f"Expected separator for {path}")
+
+            path_elements[path_index - 1] += separator
+            if not path:
+                break
+
+            element, remainder = ConfigPath._split_front(path, ConfigPath.next_key_pattern)
+            if not element:
+                # If we can't parse something as a key, then it must be a
+                # value (if it's valid).
+                try:
+                    syaml.load_config(path)
+                except spack.util.spack_yaml.SpackYAMLError as e:
+                    raise ValueError(
+                        "Remainder of path is not a valid key"
+                        f" and does not parse as a value {path}"
+                    ) from e
+                element = path
+                path = None  # The rest of the path was consumed into the value
+            else:
+                path = remainder
+
+            path_elements.append(element)
+            path_index += 1
+
+        return path_elements
+
+    @staticmethod
+    def process(path):
+        result = []
+        quote = "['\"]"
+        seen_override_in_path = False
+
+        path_elements = ConfigPath._validate(path)
+        last_element_idx = len(path_elements) - 1
+        for i, element in enumerate(path_elements):
+            override = False
+            append = False
+            prepend = False
+            quoted = False
+            if element.endswith("::") or (element.endswith(":") and i == last_element_idx):
+                if seen_override_in_path:
+                    raise syaml.SpackYAMLError(
+                        "Meaningless second override indicator `::' in path `{0}'".format(path), ""
+                    )
+                override = True
+                seen_override_in_path = True
+            element = element.rstrip(":")
+
+            if element.endswith("+"):
+                prepend = True
+            elif element.endswith("-"):
+                append = True
+            element = element.rstrip("+-")
+
+            if re.match(f"^{quote}", element):
+                quoted = True
+            element = element.strip("'\"")
+
+            if any([append, prepend, override, quoted]):
+                element = syaml.syaml_str(element)
+                if append:
+                    element.append = True
+                if prepend:
+                    element.prepend = True
+                if override:
+                    element.override = True
+
+            result.append(element)
+
+        return result
+
+
 def process_config_path(path: str) -> List[str]:
     """Process a path argument to config.set() that may contain overrides ('::' or
     trailing ':')
 
-    Note: quoted value path components will be processed as a single value (escaping colons)
+    Colons will be treated as static strings if inside of quotes,
-        quoted path components outside of the value will be considered ill formed and will
+    e.g. `this:is:a:path:'value:with:colon'` will yield:
-        raise.
-        e.g. `this:is:a:path:'value:with:colon'` will yield:
 
-            [this, is, a, path, value:with:colon]
+        [this, is, a, path, value:with:colon]
+
+    The path may consist only of keys (e.g. for a `get`) or may end in a value.
+    Keys are always strings: if a user encloses a key in quotes, the quotes
+    should be removed. Values with quotes should be treated as strings,
+    but without quotes, may be parsed as a different yaml object (e.g.
+    '{}' is a dict, but '"{}"' is a string).
+
+    This function does not know whether the final element of the path is a
+    key or value, so:
+
+    * It must strip the quotes, in case it is a key (so we look for "key" and
+      not '"key"'))
+    * It must indicate somehow that the quotes were stripped, in case it is a
+      value (so that we don't process '"{}"' as a YAML dict)
+
+    Therefore, all elements with quotes are stripped, and then also converted
+    to ``syaml_str`` (if treating the final element as a value, the caller
+    should not parse it in this case).
     """
-    result = []
+    return ConfigPath.process(path)
-    if path.startswith(":"):
-        raise syaml.SpackYAMLError(f"Illegal leading `:' in path `{path}'", "")
-    seen_override_in_path = False
-    while path:
-        front, sep, path = path.partition(":")
-        if (sep and not path) or path.startswith(":"):
-            if seen_override_in_path:
-                raise syaml.SpackYAMLError(
-                    f"Meaningless second override indicator `::' in path `{path}'", ""
-                )
-            path = path.lstrip(":")
-            front = syaml.syaml_str(front)
-            front.override = True  # type: ignore[attr-defined]
-            seen_override_in_path = True
-
-        elif front.endswith("+"):
-            front = front.rstrip("+")
-            front = syaml.syaml_str(front)
-            front.prepend = True  # type: ignore[attr-defined]
-
-        elif front.endswith("-"):
-            front = front.rstrip("-")
-            front = syaml.syaml_str(front)
-            front.append = True  # type: ignore[attr-defined]
-
-        result.append(front)
-
-        quote = "['\"]"
-        not_quote = "[^'\"]"
-
-        if re.match(f"^{quote}", path):
-            m = re.match(rf"^({quote}{not_quote}+{quote})$", path)
-            if not m:
-                raise ValueError("Quotes indicate value, but there are additional path entries")
-            result.append(m.group(1))
-            break
-
-    return result
 
 
 #

lib/spack/spack/test/config.py

@@ -284,6 +284,13 @@ def test_add_config_path(mutable_config):
     set_value = spack.config.get("config")["install_tree"]["projections"]["cmake"]
     assert set_value == "{architecture}/{compiler.name}-{compiler.version}/{name}-{version}-{hash}"
 
+    path = 'modules:default:tcl:all:environment:set:"{name}_ROOT":"{prefix}"'
+    spack.config.add(path)
+    set_value = spack.config.get("modules")["default"]["tcl"]["all"]["environment"]["set"]
+    assert r"{name}_ROOT" in set_value
+    assert set_value[r"{name}_ROOT"] == r"{prefix}"
+    assert spack.config.get('modules:default:tcl:all:environment:set:"{name}_ROOT"') == r"{prefix}"
+
     # NOTE:
     # The config path: "config:install_tree:root:<path>" is unique in that it can accept multiple
     # schemas (such as a dropped "root" component) which is atypical and may lead to passing tests
@@ -1188,7 +1195,7 @@ def test_set_dict_override(mock_low_high_config, write_config_file):
 
 
 def test_set_bad_path(config):
-    with pytest.raises(syaml.SpackYAMLError, match="Illegal leading"):
+    with pytest.raises(ValueError):
         with spack.config.override(":bad:path", ""):
             pass
 
@@ -1462,3 +1469,26 @@ def test_config_file_read_invalid_yaml(tmpdir, mutable_empty_config):
 
     with pytest.raises(spack.config.ConfigFileError, match="parsing YAML"):
         spack.config.read_config_file(filename)
+
+
+@pytest.mark.parametrize(
+    "path,it_should_work,expected_parsed",
+    [
+        ("x:y:z", True, ["x:", "y:", "z"]),
+        ("x+::y:z", True, ["x+::", "y:", "z"]),
+        ('x:y:"{z}"', True, ["x:", "y:", '"{z}"']),
+        ('x:"y"+:z', True, ["x:", '"y"+:', "z"]),
+        ('x:"y"trail:z', False, None),
+        ("x:y:[1.0]", True, ["x:", "y:", "[1.0]"]),
+        ("x:y:['1.0']", True, ["x:", "y:", "['1.0']"]),
+        ("x:{y}:z", True, ["x:", "{y}:", "z"]),
+        ("x:'{y}':z", True, ["x:", "'{y}':", "z"]),
+        ("x:{y}", True, ["x:", "{y}"]),
+    ],
+)
+def test_config_path_dsl(path, it_should_work, expected_parsed):
+    if it_should_work:
+        assert spack.config.ConfigPath._validate(path) == expected_parsed
+    else:
+        with pytest.raises(ValueError):
+            spack.config.ConfigPath._validate(path)