From 3eb52b48b8d0f3379e257ab0817fef4e1b7f4c5e Mon Sep 17 00:00:00 2001
From: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Date: Fri, 29 Oct 2021 18:44:49 +0200
Subject: config add: infer type based on JSON schema validation errors
 (#27035)

- [x] Allow dding enumerated types and types whose default value is forbidden by the schema
- [x] Add a test for using enumerated types in the tests for `spack config add`
- [x] Make `config add` tests use the `mutable_config` fixture so they do not
      affect other tests

Co-authored-by: Todd Gamblin <tgamblin@llnl.gov>
---
 lib/spack/spack/config.py          | 43 +++++++++++++++++++++++++-------------
 lib/spack/spack/test/cmd/config.py |  2 +-
 lib/spack/spack/test/config.py     | 19 +++++++++++++----
 3 files changed, 45 insertions(+), 19 deletions(-)

(limited to 'lib')

diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
index c82729b007..f3fc73e4b4 100644
--- a/lib/spack/spack/config.py
+++ b/lib/spack/spack/config.py
@@ -1067,22 +1067,36 @@ def get_valid_type(path):
     path given, the priority order is ``list``, ``dict``, ``str``, ``bool``,
     ``int``, ``float``.
     """
+    types = {
+        'array': list,
+        'object': syaml.syaml_dict,
+        'string': str,
+        'boolean': bool,
+        'integer': int,
+        'number': float
+    }
+
     components = process_config_path(path)
     section = components[0]
-    for type in (list, syaml.syaml_dict, str, bool, int, float):
-        try:
-            ret = type()
-            test_data = ret
-            for component in reversed(components):
-                test_data = {component: test_data}
-            validate(test_data, section_schemas[section])
-            return ret
-        except (ConfigFormatError, AttributeError):
-            # This type won't validate, try the next one
-            # Except AttributeError because undefined behavior of dict ordering
-            # in python 3.5 can cause the validator to raise an AttributeError
-            # instead of a ConfigFormatError.
-            pass
+
+    # Use None to construct the test data
+    test_data = None
+    for component in reversed(components):
+        test_data = {component: test_data}
+
+    try:
+        validate(test_data, section_schemas[section])
+    except (ConfigFormatError, AttributeError) as e:
+        jsonschema_error = e.validation_error
+        if jsonschema_error.validator == 'type':
+            return types[jsonschema_error.validator_value]()
+        elif jsonschema_error.validator == 'anyOf':
+            for subschema in jsonschema_error.validator_value:
+                anyof_type = subschema.get('type')
+                if anyof_type is not None:
+                    return types[anyof_type]()
+    else:
+        return type(None)
     raise ConfigError("Cannot determine valid type for path '%s'." % path)
 
 
@@ -1301,6 +1315,7 @@ class ConfigFormatError(ConfigError):
     def __init__(self, validation_error, data, filename=None, line=None):
         # spack yaml has its own file/line marks -- try to find them
         # we prioritize these over the inputs
+        self.validation_error = validation_error
         mark = self._get_mark(validation_error, data)
         if mark:
             filename = mark.name
diff --git a/lib/spack/spack/test/cmd/config.py b/lib/spack/spack/test/cmd/config.py
index acadbc761d..63c3cab92a 100644
--- a/lib/spack/spack/test/cmd/config.py
+++ b/lib/spack/spack/test/cmd/config.py
@@ -225,7 +225,7 @@ def test_config_with_c_argument(mutable_empty_config):
     # Add the path to the config
     config("add", args.config_vars[0], scope='command_line')
     output = config("get", 'config')
-    assert "config:\n  install_root:\n  - root: /path/to/config.yaml" in output
+    assert "config:\n  install_root:\n    root: /path/to/config.yaml" in output
 
 
 def test_config_add_ordered_dict(mutable_empty_config):
diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py
index fd6af8640b..6e65eee3bf 100644
--- a/lib/spack/spack/test/config.py
+++ b/lib/spack/spack/test/config.py
@@ -279,21 +279,32 @@ repos_high = {'repos': ["/some/other/path"]}
 # Test setting config values via path in filename
 
 
-def test_add_config_path():
-
+def test_add_config_path(mutable_config):
     # Try setting a new install tree root
     path = "config:install_tree:root:/path/to/config.yaml"
-    spack.config.add(path, scope="command_line")
+    spack.config.add(path)
     set_value = spack.config.get('config')['install_tree']['root']
     assert set_value == '/path/to/config.yaml'
 
     # Now a package:all setting
     path = "packages:all:compiler:[gcc]"
-    spack.config.add(path, scope="command_line")
+    spack.config.add(path)
     compilers = spack.config.get('packages')['all']['compiler']
     assert "gcc" in compilers
 
 
+@pytest.mark.regression('17543,23259')
+def test_add_config_path_with_enumerated_type(mutable_config):
+    spack.config.add("config:concretizer:clingo")
+    assert spack.config.get('config')['concretizer'] == "clingo"
+
+    spack.config.add("config:concretizer:original")
+    assert spack.config.get('config')['concretizer'] == "original"
+
+    with pytest.raises(spack.config.ConfigError):
+        spack.config.add("config:concretizer:foo")
+
+
 def test_add_config_filename(mock_low_high_config, tmpdir):
 
     config_yaml = tmpdir.join('config-filename.yaml')
-- 
cgit v1.2.3-70-g09d2