Use arg_spec type for comparisons on default and choices (#37741)

* Use arg_spec type for comparisons on default and choices

* Further improve type casting

* Make sure to capture output in more places

* Individually report invalid choices

* Update ignore.txt after resolving merge conflicts

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

@@ -113,6 +113,10 @@ Errors
   324       Value for "default" from the argument_spec does not match the documentation
   325       argument_spec defines type="bool" but documentation does not
   326       Value for "choices" from the argument_spec does not match the documentation
+  327       Default value from the documentation is not compatible with type defined in the argument_spec
+  328       Choices value from the documentation is not compatible with type defined in the argument_spec
+  329       Default value from the argument_spec is not compatible with type defined in the argument_spec
+  330       Choices value from the argument_spec is not compatible with type defined in the argument_spec
   ..
 ---------   -------------------
   **4xx**   **Syntax**

lib/ansible/plugins/filter/core.py

@@ -34,7 +34,7 @@ import time
 import uuid
 import yaml
 
-from collections import MutableMapping, MutableSequence
+from collections import Mapping, MutableMapping, MutableSequence
 import datetime
 from functools import partial
 from random import Random, SystemRandom, shuffle
@@ -326,7 +326,7 @@ def combine(*terms, **kwargs):
 
     dicts = []
     for t in terms:
-        if isinstance(t, MutableMapping):
+        if isinstance(t, (MutableMapping, Mapping)):
             dicts.append(t)
         elif isinstance(t, list):
             dicts.append(combine(*t, **kwargs))

test/sanity/validate-modules/main.py

@@ -45,7 +45,7 @@ from module_args import AnsibleModuleImportError, get_argument_spec
 
 from schema import doc_schema, metadata_1_1_schema, return_schema
 
-from utils import CaptureStd, compare_unordered_lists, maybe_convert_bool, parse_yaml
+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
@@ -1042,6 +1042,9 @@ class ModuleValidator(Validator):
             )
             return
 
+        # Use this to access type checkers later
+        module = NoArgsAnsibleModule({})
+
         provider_args = set()
         args_from_argspec = set()
         deprecated_args_from_argspec = set()
@@ -1072,14 +1075,46 @@ class ModuleValidator(Validator):
                 # don't validate docs<->arg_spec checks below
                 continue
 
+            _type = data.get('type', 'str')
+            if callable(_type):
+                _type_checker = _type
+            else:
+                _type_checker = module._CHECK_ARGUMENT_TYPES_DISPATCHER.get(_type)
+
             # TODO: needs to recursively traverse suboptions
-            doc_default = docs.get('options', {}).get(arg, {}).get('default', None)
+            arg_default = None
-            if data.get('type') == 'bool':
+            if 'default' in data and not is_empty(data['default']):
-                doc_default = maybe_convert_bool(doc_default)
+                try:
-            arg_default = data.get('default')
+                    with CaptureStd():
-            if 'default' in data and data.get('type') == 'bool':
+                        arg_default = _type_checker(data['default'])
-                arg_default = maybe_convert_bool(data['default'])
+                except (Exception, SystemExit):
-            if 'default' in data and arg_default != doc_default:
+                    self.reporter.error(
+                        path=self.object_path,
+                        code=329,
+                        msg=('Default value from the argument_spec (%r) is not compatible '
+                             'with type %r defined in the argument_spec' % (data['default'], _type))
+                    )
+                    continue
+            elif data.get('default') is None and _type == 'bool' and 'options' not in data:
+                arg_default = False
+            try:
+                doc_default = None
+                doc_options_arg = docs.get('options', {}).get(arg, {})
+                if 'default' in doc_options_arg and not is_empty(doc_options_arg['default']):
+                    with CaptureStd():
+                        doc_default = _type_checker(doc_options_arg['default'])
+                elif doc_options_arg.get('default') is None and _type == 'bool' and 'suboptions' not in doc_options_arg:
+                    doc_default = False
+            except (Exception, SystemExit):
+                self.reporter.error(
+                    path=self.object_path,
+                    code=327,
+                    msg=('Default value from the documentation (%r) is not compatible '
+                         'with type %r defined in the argument_spec' % (doc_options_arg.get('default'), _type))
+                )
+                continue
+
+            if arg_default != doc_default:
                 self.reporter.error(
                     path=self.object_path,
                     code=324,
@@ -1097,13 +1132,46 @@ class ModuleValidator(Validator):
                 )
 
             # TODO: needs to recursively traverse suboptions
-            doc_choices = docs.get('options', {}).get(arg, {}).get('choices', [])
+            doc_choices = []
-            if not compare_unordered_lists(data.get('choices', []), doc_choices):
+            try:
+                for choice in docs.get('options', {}).get(arg, {}).get('choices', []):
+                    try:
+                        with CaptureStd():
+                            doc_choices.append(_type_checker(choice))
+                    except (Exception, SystemExit):
+                        self.reporter.error(
+                            path=self.object_path,
+                            code=328,
+                            msg=('Choices value from the documentation (%r) is not compatible '
+                                 'with type %r defined in the argument_spec' % (choice, _type))
+                        )
+                        raise StopIteration()
+            except StopIteration:
+                continue
+
+            arg_choices = []
+            try:
+                for choice in data.get('choices', []):
+                    try:
+                        with CaptureStd():
+                            arg_choices.append(_type_checker(choice))
+                    except (Exception, SystemExit):
+                        self.reporter.error(
+                            path=self.object_path,
+                            code=330,
+                            msg=('Choices value from the argument_spec (%r) is not compatible '
+                                 'with type %r defined in the argument_spec' % (choice, _type))
+                        )
+                        raise StopIteration()
+            except StopIteration:
+                continue
+
+            if not compare_unordered_lists(arg_choices, doc_choices):
                 self.reporter.error(
                     path=self.object_path,
                     code=326,
                     msg=('Value for "choices" from the argument_spec (%r) for "%s" does not match the '
-                         'documentation (%r)' % (data.get('choices', []), arg, doc_choices))
+                         'documentation (%r)' % (arg_choices, arg, doc_choices))
                 )
 
         if docs:

test/sanity/validate-modules/module_args.py

@@ -45,22 +45,14 @@ def add_mocks(filename):
     pre_sys_modules = list(sys.modules.keys())
 
     module_mock = mock.MagicMock()
-    mocks = []
     for module_class in MODULE_CLASSES:
-        mocks.append(
+        p = mock.patch('%s.__init__' % module_class, new=module_mock).start()
-            mock.patch('%s.__init__' % module_class, new=module_mock)
-        )
-    for m in mocks:
-        p = m.start()
         p.side_effect = AnsibleModuleCallError('AnsibleModuleCallError')
-    mocks.append(
+    mock.patch('ansible.module_utils.basic._load_params').start()
-        mock.patch('ansible.module_utils.basic._load_params').start()
-    )
 
     yield module_mock
 
-    for m in mocks:
+    mock.patch.stopall()
-        m.stop()
 
     # Clean up imports to prevent issues with mutable data being used in modules
     for k in list(sys.modules.keys()):

test/sanity/validate-modules/utils.py

@@ -25,6 +25,7 @@ import yaml
 import yaml.reader
 
 from ansible.module_utils._text import to_text
+from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.parsing.convert_bool import boolean
 
 
@@ -117,15 +118,11 @@ def parse_yaml(value, lineno, module, name, load_all=False):
     return data, errors, traces
 
 
-def maybe_convert_bool(value):
+def is_empty(value):
-    """Safe conversion to boolean, catching TypeError and returning the original result
+    """Evaluate null like values excluding False"""
-
+    if value is False:
-    Only used in doc<->arg_spec comparisons
+        return False
-    """
+    return not bool(value)
-    try:
-        return boolean(value)
-    except TypeError:
-        return value
 
 
 def compare_unordered_lists(a, b):
@@ -136,3 +133,11 @@ def compare_unordered_lists(a, b):
       - unhashable elements
     """
     return len(a) == len(b) and all(x in b for x in a)
+
+
+class NoArgsAnsibleModule(AnsibleModule):
+    """AnsibleModule that does not actually load params. This is used to get access to the
+    methods within AnsibleModule without having to fake a bunch of data
+    """
+    def _load_params(self):
+        self.params = {}