Module validation: sanity check mutually_exclusive, required_if, requ…

…ired_xxx ... (ansible#66961)

* required_if checks should have three or four parts.

* Validate mutually_exclusive, required_together, required_one_of, required_if and required_by.

* Simplify code.

* Improve messages.

* Add changelog.

* Sanity check.

* Update docs.

* Update ignore.txt.

* Don't continue with tests when terms are not strings.

* Remove ignore.txt entry.

* Make sure validate-modules doesn't choke on things already flagged by schema test.

* Check required_if requirements list for strings.

changelogs/fragments/66961-ansible-test-required-mutually.yml

@@ -0,0 +1,2 @@
+minor_changes:
+- "ansible-test - ``mutually_exclusive``, ``required_if``, ``required_by``, ``required_together`` and ``required_one_of`` in modules are now validated."

docs/docsite/rst/dev_guide/testing_validate-modules.rst

@@ -141,4 +141,22 @@ Codes
   use-run-command-not-os-call                                  Imports              Error                  ``os.call`` used instead of ``module.run_command``
   use-run-command-not-popen                                    Imports              Error                  ``subprocess.Popen`` used instead of ``module.run_command``
   use-short-gplv3-license                                      Documentation        Error                  GPLv3 license header should be the :ref:`short form <copyright>` for new modules
+  mutually_exclusive-type                                      Documentation        Error                  mutually_exclusive entry contains non-string value
+  mutually_exclusive-collision                                 Documentation        Error                  mutually_exclusive entry has repeated terms
+  mutually_exclusive-unknown                                   Documentation        Error                  mutually_exclusive entry contains option which does not appear in argument_spec (potentially an alias of an option?)
+  required_one_of-type                                         Documentation        Error                  required_one_of entry contains non-string value
+  required_one_of-collision                                    Documentation        Error                  required_one_of entry has repeated terms
+  required_one_of-unknown                                      Documentation        Error                  required_one_of entry contains option which does not appear in argument_spec (potentially an alias of an option?)
+  required_together-type                                       Documentation        Error                  required_together entry contains non-string value
+  required_together-collision                                  Documentation        Error                  required_together entry has repeated terms
+  required_together-unknown                                    Documentation        Error                  required_together entry contains option which does not appear in argument_spec (potentially an alias of an option?)
+  required_if-is_one_of-type                                   Documentation        Error                  required_if entry has a fourth value which is not a bool
+  required_if-requirements-type                                Documentation        Error                  required_if entry has a third value (requirements) which is not a list or tuple
+  required_if-requirements-collision                           Documentation        Error                  required_if entry has repeated terms in requirements
+  required_if-requirements-unknown                             Documentation        Error                  required_if entry's requirements contains option which does not appear in argument_spec (potentially an alias of an option?)
+  required_if-unknown-key                                      Documentation        Error                  required_if entry's key does not appear in argument_spec (potentially an alias of an option?)
+  required_if-key-in-requirements                              Documentation        Error                  required_if entry contains its key in requirements list/tuple
+  required_if-value-type                                       Documentation        Error                  required_if entry's value is not of the type specified for its key
+  required_by-collision                                        Documentation        Error                  required_by entry has repeated terms
+  required_by-unknown                                          Documentation        Error                  required_by entry contains option which does not appear in argument_spec (potentially an alias of an option?)
 ============================================================   ==================   ====================   =========================================================================================

test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py

@@ -50,7 +50,7 @@
 from .utils import CaptureStd, NoArgsAnsibleModule, compare_unordered_lists, is_empty, parse_yaml
 from voluptuous.humanize import humanize_error
 
-from ansible.module_utils.six import PY3, with_metaclass
+from ansible.module_utils.six import PY3, with_metaclass, string_types
 
 if PY3:
     # Because there is no ast.TryExcept in Python 3 ast module
@@ -1152,7 +1152,200 @@ def _validate_ansible_module_call(self, docs):
 
         self._validate_argument_spec(docs, spec, kwargs)
 
-    def _validate_argument_spec(self, docs, spec, kwargs, context=None):
+    def _validate_list_of_module_args(self, name, terms, spec, context):
+        if terms is None:
+            return
+        if not isinstance(terms, (list, tuple)):
+            # This is already reported by schema checking
+            return
+        for check in terms:
+            if not isinstance(check, (list, tuple)):
+                # This is already reported by schema checking
+                continue
+            bad_term = False
+            for term in check:
+                if not isinstance(term, string_types):
+                    msg = name
+                    if context:
+                        msg += " found in %s" % " -> ".join(context)
+                    msg += " must contain strings in the lists or tuples; found value %r" % (term, )
+                    self.reporter.error(
+                        path=self.object_path,
+                        code=name + '-type',
+                        msg=msg,
+                    )
+                    bad_term = True
+            if bad_term:
+                continue
+            if len(set(check)) != len(check):
+                msg = name
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " has repeated terms"
+                self.reporter.error(
+                    path=self.object_path,
+                    code=name + '-collision',
+                    msg=msg,
+                )
+            if not set(check) <= set(spec):
+                msg = name
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " contains terms which are not part of argument_spec: %s" % ", ".join(sorted(set(check).difference(set(spec))))
+                self.reporter.error(
+                    path=self.object_path,
+                    code=name + '-unknown',
+                    msg=msg,
+                )
+
+    def _validate_required_if(self, terms, spec, context, module):
+        if terms is None:
+            return
+        if not isinstance(terms, (list, tuple)):
+            # This is already reported by schema checking
+            return
+        for check in terms:
+            if not isinstance(check, (list, tuple)) or len(check) not in [3, 4]:
+                # This is already reported by schema checking
+                continue
+            if len(check) == 4 and not isinstance(check[3], bool):
+                msg = "required_if"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " must have forth value omitted or of type bool; got %r" % (check[3], )
+                self.reporter.error(
+                    path=self.object_path,
+                    code='required_if-is_one_of-type',
+                    msg=msg,
+                )
+            requirements = check[2]
+            if not isinstance(requirements, (list, tuple)):
+                msg = "required_if"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " must have third value (requirements) being a list or tuple; got type %r" % (requirements, )
+                self.reporter.error(
+                    path=self.object_path,
+                    code='required_if-requirements-type',
+                    msg=msg,
+                )
+                continue
+            bad_term = False
+            for term in requirements:
+                if not isinstance(term, string_types):
+                    msg = "required_if"
+                    if context:
+                        msg += " found in %s" % " -> ".join(context)
+                    msg += " must have only strings in third value (requirements); got %r" % (term, )
+                    self.reporter.error(
+                        path=self.object_path,
+                        code='required_if-requirements-type',
+                        msg=msg,
+                    )
+                    bad_term = True
+            if bad_term:
+                continue
+            if len(set(requirements)) != len(requirements):
+                msg = "required_if"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " has repeated terms in requirements"
+                self.reporter.error(
+                    path=self.object_path,
+                    code='required_if-requirements-collision',
+                    msg=msg,
+                )
+            if not set(requirements) <= set(spec):
+                msg = "required_if"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " contains terms in requirements which are not part of argument_spec: %s" % ", ".join(sorted(set(requirements).difference(set(spec))))
+                self.reporter.error(
+                    path=self.object_path,
+                    code='required_if-requirements-unknown',
+                    msg=msg,
+                )
+            key = check[0]
+            if key not in spec:
+                msg = "required_if"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " must have its key %s in argument_spec" % key
+                self.reporter.error(
+                    path=self.object_path,
+                    code='required_if-unknown-key',
+                    msg=msg,
+                )
+                continue
+            if key in requirements:
+                msg = "required_if"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " contains its key %s in requirements" % key
+                self.reporter.error(
+                    path=self.object_path,
+                    code='required_if-key-in-requirements',
+                    msg=msg,
+                )
+            value = check[1]
+            if value is not None:
+                _type = spec[key].get('type', 'str')
+                if callable(_type):
+                    _type_checker = _type
+                else:
+                    _type_checker = module._CHECK_ARGUMENT_TYPES_DISPATCHER.get(_type)
+                try:
+                    with CaptureStd():
+                        dummy = _type_checker(value)
+                except (Exception, SystemExit):
+                    msg = "required_if"
+                    if context:
+                        msg += " found in %s" % " -> ".join(context)
+                    msg += " has value %r which does not fit to %s's parameter type %r" % (value, key, _type)
+                    self.reporter.error(
+                        path=self.object_path,
+                        code='required_if-value-type',
+                        msg=msg,
+                    )
+
+    def _validate_required_by(self, terms, spec, context):
+        if terms is None:
+            return
+        if not isinstance(terms, Mapping):
+            # This is already reported by schema checking
+            return
+        for key, value in terms.items():
+            if isinstance(value, string_types):
+                value = [value]
+            if not isinstance(value, (list, tuple)):
+                # This is already reported by schema checking
+                continue
+            for term in value:
+                if not isinstance(term, string_types):
+                    # This is already reported by schema checking
+                    continue
+            if len(set(value)) != len(value) or key in value:
+                msg = "required_by"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " has repeated terms"
+                self.reporter.error(
+                    path=self.object_path,
+                    code='required_by-collision',
+                    msg=msg,
+                )
+            if not set(value) <= set(spec) or key not in spec:
+                msg = "required_by"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " contains terms which are not part of argument_spec: %s" % ", ".join(sorted(set(value).difference(set(spec))))
+                self.reporter.error(
+                    path=self.object_path,
+                    code='required_by-unknown',
+                    msg=msg,
+                )
+
+    def _validate_argument_spec(self, docs, spec, kwargs, context=None, last_context_spec=None):
         if not self.analyze_arg_spec:
             return
 
@@ -1162,6 +1355,9 @@ def _validate_argument_spec(self, docs, spec, kwargs, context=None):
         if context is None:
             context = []
 
+        if last_context_spec is None:
+            last_context_spec = kwargs
+
         try:
             if not context:
                 add_fragments(docs, self.object_path, fragment_loader=fragment_loader)
@@ -1172,6 +1368,12 @@ def _validate_argument_spec(self, docs, spec, kwargs, context=None):
         # Use this to access type checkers later
         module = NoArgsAnsibleModule({})
 
+        self._validate_list_of_module_args('mutually_exclusive', last_context_spec.get('mutually_exclusive'), spec, context)
+        self._validate_list_of_module_args('required_together', last_context_spec.get('required_together'), spec, context)
+        self._validate_list_of_module_args('required_one_of', last_context_spec.get('required_one_of'), spec, context)
+        self._validate_required_if(last_context_spec.get('required_if'), spec, context, module)
+        self._validate_required_by(last_context_spec.get('required_by'), spec, context)
+
         provider_args = set()
         args_from_argspec = set()
         deprecated_args_from_argspec = set()
@@ -1541,7 +1743,8 @@ def _validate_argument_spec(self, docs, spec, kwargs, context=None):
                         code='missing-suboption-docs',
                         msg=msg
                     )
-                self._validate_argument_spec({'options': doc_suboptions}, spec_suboptions, kwargs, context=context + [arg])
+                self._validate_argument_spec({'options': doc_suboptions}, spec_suboptions, kwargs,
+                                             context=context + [arg], last_context_spec=data)
 
         for arg in args_from_argspec:
             if not str(arg).isidentifier():

test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py

@@ -75,7 +75,7 @@ def sequence_of_sequences(min=None, max=None):
     'mutually_exclusive': sequence_of_sequences(min=2),
     'required_together': sequence_of_sequences(min=2),
     'required_one_of': sequence_of_sequences(min=2),
-    'required_if': sequence_of_sequences(min=3),
+    'required_if': sequence_of_sequences(min=3, max=4),
     'required_by': Schema({str: Any(list_string_types, tuple_string_types, *string_types)}),
 }