Merge pull request #5450 from oliver-sanders/4955

parsec: catch section/option conflict in validation

CHANGES.md

@@ -21,6 +21,10 @@ configuration files.
 
 ### Fixes
 
+[5450](https://github.com/cylc/cylc-flow/pull/5450) - Validation provides
+better error messages if [sections] and settings are mixed up in a
+configuration.
+
 [5445](https://github.com/cylc/cylc-flow/pull/5445) - Fix remote tidy
  bug where install target is not explicit in platform definition.
 

cylc/flow/config.py

@@ -356,13 +356,6 @@ def __init__(
         except KeyError:
             parameter_templates = {}
 
-        # Check that parameter templates are a section
-        if not hasattr(parameter_templates, 'update'):
-            raise WorkflowConfigError(
-                '[task parameters][templates] is a section. Don\'t use it '
-                'as a parameter.'
-            )
-
         # parameter values and templates are normally needed together.
         self.parameters = (parameter_values, parameter_templates)
 

cylc/flow/parsec/upgrade.py

@@ -104,7 +104,13 @@ def obsolete(self, vn, oldkeys, silent=False, is_section=False):
     def get_item(self, keys):
         item = self.cfg
         for key in keys:
-            item = item[key]
+            try:
+                item = item[key]
+            except TypeError:
+                raise UpgradeError(
+                    f'{self.show_keys(keys[:-1], True)}'
+                    f' ("{keys[-2]}" should be a [section] not a setting)'
+                )
         return item
 
     def put_item(self, keys, val):

cylc/flow/parsec/validate.py

@@ -204,10 +204,28 @@ def validate(self, cfg_root, spec_root):
                 else:
                     speckey = key
                 specval = spec[speckey]
-                if isinstance(value, dict) and not specval.is_leaf():
+
+                cfg_is_section = isinstance(value, dict)
+                spec_is_section = not specval.is_leaf()
+                if cfg_is_section and not spec_is_section:
+                    # config is a [section] but it should be a setting=
+                    raise IllegalItemError(
+                        keys,
+                        key,
+                        msg=f'"{key}" should be a setting not a [section]',
+                    )
+                if (not cfg_is_section) and spec_is_section:
+                    # config is a setting= but it should be a [section]
+                    raise IllegalItemError(
+                        keys,
+                        key,
+                        msg=f'"{key}" should be a [section] not a setting',
+                    )
+
+                if cfg_is_section and spec_is_section:
                     # Item is dict, push to queue
                     queue.append([value, specval, keys + [key]])
-                elif value is not None and specval.is_leaf():
+                elif value is not None and not spec_is_section:
                     # Item is value, coerce according to value type
                     cfg[key] = self.coercers[specval.vdr](value, keys + [key])
                     if specval.options:

tests/functional/validate/76-section-section-transpose.t

@@ -0,0 +1,61 @@
+#!/usr/bin/env bash
+# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE.
+# Copyright (C) NIWA & British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#-------------------------------------------------------------------------------
+# Test handling of mixed up sections vs settings
+
+. "$(dirname "$0")/test_header"
+
+set_test_number 6
+
+# 1. section as setting (normal)
+TEST_NAME='section-as-setting-normal'
+cat > 'flow.cylc' <<__HEREDOC__
+[runtime]
+    [[foo]]
+        environment = 42
+__HEREDOC__
+run_fail "${TEST_NAME}-validate" cylc validate .
+grep_ok  \
+    'IllegalItemError: \[runtime\]\[foo\]environment - ("environment" should be a \[section\] not a setting)' \
+    "${TEST_NAME}-validate.stderr"
+
+
+# 2. section as setting (via upgrader)
+# NOTE: if this test fails it is likely because the upgrader for "scheduling"
+# has been removed, convert this to use a new deprecated section
+TEST_NAME='section-as-setting-upgrader'
+cat > 'flow.cylc' <<__HEREDOC__
+scheduling = 22
+__HEREDOC__
+
+run_fail "${TEST_NAME}-validate" cylc validate .
+grep_ok  \
+    'UpgradeError: \[scheduling\] ("scheduling" should be a \[section\] not a setting' \
+    "${TEST_NAME}-validate.stderr"
+
+
+# 3. setting as section
+TEST_NAME='setting-as-section'
+cat > 'flow.cylc' <<__HEREDOC__
+[scheduling]
+    [[initial cycle point]]
+__HEREDOC__
+
+run_fail "${TEST_NAME}-validate" cylc validate .
+grep_ok  \
+    'IllegalItemError: \[scheduling\]initial cycle point - ("initial cycle point" should be a setting not a \[section\])' \
+    "${TEST_NAME}-validate.stderr"

tests/integration/test_config.py

@@ -172,22 +172,6 @@ def test_no_graph(flow, validate):
     assert 'missing [scheduling][[graph]] section.' in str(exc_ctx.value)
 
 
-def test_parameter_templates_setting(flow, one_conf, validate):
-    """It should fail if [task parameter]templates is a setting.
-
-    It should be a section.
-    """
-    reg = flow({
-        **one_conf,
-        'task parameters': {
-            'templates': 'foo'
-        }
-    })
-    with pytest.raises(WorkflowConfigError) as exc_ctx:
-        validate(reg)
-    assert '[templates] is a section' in str(exc_ctx.value)
-
-
 @pytest.mark.parametrize(
     'section', [
         'external-trigger',