Merge f41d222eda into 6aa8192131

changelogs/fragments/argspec-required-mutually_exclusive.yml

@@ -0,0 +1,12 @@
+deprecated_features:
+- >-
+  The ``required_by`` feature for Python modules treats an explicit parameter passed in as ``None`` as "not specified".
+  This is deprecated because it is inconsistent with the other ``required`` and ``required_*`` checks.
+- >-
+  The ``required``, ``required_if``, ``required_together``, ``required_one_of``, and ``required_by`` checks for Python modules
+  warn when a module parameter is passed in as ``None``, but still accept that value.
+
+  The module can opt into the intended behavior by setting ``allow_none_value`` to ``True`` or ``False`` for the option in the
+  argument spec.
+  
+  (https://github.com/ansible/ansible/issues/69190)

lib/ansible/module_utils/common/arg_spec.py

@@ -26,6 +26,7 @@ from ansible.module_utils.common.warnings import deprecate, warn
 from ansible.module_utils.common.validation import (
     check_mutually_exclusive,
     check_required_arguments,
+    check_required_none,
 )
 
 from ansible.module_utils.errors import (
@@ -231,8 +232,9 @@ class ArgumentSpecValidator:
 
         result._no_log_values.update(_set_defaults(self.argument_spec, result._validated_parameters, False))
 
+        required_options = []
         try:
-            check_required_arguments(self.argument_spec, result._validated_parameters)
+            check_required_arguments(self.argument_spec, result._validated_parameters, add_required=required_options)
         except TypeError as e:
             result.errors.append(RequiredError(to_native(e)))
 
@@ -241,9 +243,13 @@ class ArgumentSpecValidator:
 
         for check in _ADDITIONAL_CHECKS:
             try:
-                check['func'](getattr(self, "_{attr}".format(attr=check['attr'])), result._validated_parameters)
+                check['func'](getattr(self, "_{attr}".format(attr=check['attr'])), result._validated_parameters, add_required=required_options)
             except TypeError as te:
                 result.errors.append(check['err'](to_native(te)))
+        try:
+            check_required_none(required_options, self.argument_spec, result._validated_parameters)
+        except TypeError as e:
+            result.errors.append(RequiredError(to_native(e)))
 
         result._no_log_values.update(_set_defaults(self.argument_spec, result._validated_parameters))
 

lib/ansible/module_utils/common/parameters.py

@@ -57,6 +57,7 @@ from ansible.module_utils.common.validation import (
     check_required_one_of,
     check_required_if,
     check_required_by,
+    check_required_none,
     check_type_bits,
     check_type_bool,
     check_type_bytes,
@@ -803,8 +804,9 @@ def _validate_sub_spec(
 
                 no_log_values.update(_set_defaults(sub_spec, sub_parameters, False))
 
+                required_options = []
                 try:
-                    check_required_arguments(sub_spec, sub_parameters, options_context)
+                    check_required_arguments(sub_spec, sub_parameters, options_context, add_required=required_options)
                 except TypeError as e:
                     errors.append(RequiredError(to_native(e)))
 
@@ -813,9 +815,13 @@ def _validate_sub_spec(
 
                 for check in _ADDITIONAL_CHECKS:
                     try:
-                        check['func'](value.get(check['attr']), sub_parameters, options_context)
+                        check['func'](value.get(check['attr']), sub_parameters, options_context, add_required=required_options)
                     except TypeError as e:
                         errors.append(check['err'](to_native(e)))
+                try:
+                    check_required_none(required_options, sub_spec, sub_parameters, options_context)
+                except TypeError as e:
+                    errors.append(RequiredError(to_native(e)))
 
                 no_log_values.update(_set_defaults(sub_spec, sub_parameters))
 

lib/ansible/module_utils/common/validation.py

@@ -13,6 +13,7 @@ from ansible.module_utils.common.text.converters import to_native
 from ansible.module_utils.common.collections import is_iterable
 from ansible.module_utils.common.text.converters import jsonify
 from ansible.module_utils.common.text.formatters import human_to_bytes
+from ansible.module_utils.common.warnings import deprecate
 from ansible.module_utils.parsing.convert_bool import boolean
 from ansible.module_utils.six import (
     binary_type,
@@ -22,11 +23,12 @@ from ansible.module_utils.six import (
 )
 
 
-def count_terms(terms, parameters):
+def count_terms(terms, parameters, add_terms=None):
     """Count the number of occurrences of a key in a given dictionary
 
     :arg terms: String or iterable of values to check
     :arg parameters: Dictionary of parameters
+    :arg add_terms: None or list to which all terms are added
 
     :returns: An integer that is the number of occurrences of the terms values
         in the provided dictionary.
@@ -35,6 +37,9 @@ def count_terms(terms, parameters):
     if not is_iterable(terms):
         terms = [terms]
 
+    if add_terms is not None:
+        add_terms.extend(terms)
+
     return len(set(terms).intersection(parameters))
 
 
@@ -99,7 +104,7 @@ def check_mutually_exclusive(terms, parameters, options_context=None):
     return results
 
 
-def check_required_one_of(terms, parameters, options_context=None):
+def check_required_one_of(terms, parameters, options_context=None, add_required=None):
     """Check each list of terms to ensure at least one exists in the given module
     parameters
 
@@ -110,6 +115,7 @@ def check_required_one_of(terms, parameters, options_context=None):
     :arg parameters: Dictionary of parameters
     :kwarg options_context: List of strings of parent key names if ``terms`` are
         in a sub spec.
+    :kwarg add_required: None or list to which all required parameters are added
 
     :returns: Empty list or raises :class:`TypeError` if the check fails.
     """
@@ -119,7 +125,7 @@ def check_required_one_of(terms, parameters, options_context=None):
         return results
 
     for term in terms:
-        count = count_terms(term, parameters)
+        count = count_terms(term, parameters, add_terms=add_required)
         if count == 0:
             results.append(term)
 
@@ -133,7 +139,7 @@ def check_required_one_of(terms, parameters, options_context=None):
     return results
 
 
-def check_required_together(terms, parameters, options_context=None):
+def check_required_together(terms, parameters, options_context=None, add_required=None):
     """Check each list of terms to ensure every parameter in each list exists
     in the given parameters.
 
@@ -145,6 +151,7 @@ def check_required_together(terms, parameters, options_context=None):
     :arg parameters: Dictionary of parameters
     :kwarg options_context: List of strings of parent key names if ``terms`` are
         in a sub spec.
+    :kwarg add_required: None or list to which all required parameters are added
 
     :returns: Empty list or raises :class:`TypeError` if the check fails.
     """
@@ -154,7 +161,7 @@ def check_required_together(terms, parameters, options_context=None):
         return results
 
     for term in terms:
-        counts = [count_terms(field, parameters) for field in term]
+        counts = [count_terms(field, parameters, add_terms=add_required) for field in term]
         non_zero = [c for c in counts if c > 0]
         if len(non_zero) > 0:
             if 0 in counts:
@@ -169,7 +176,7 @@ def check_required_together(terms, parameters, options_context=None):
     return results
 
 
-def check_required_by(requirements, parameters, options_context=None):
+def check_required_by(requirements, parameters, options_context=None, add_required=None):
     """For each key in requirements, check the corresponding list to see if they
     exist in parameters.
 
@@ -179,6 +186,7 @@ def check_required_by(requirements, parameters, options_context=None):
     :arg parameters: Dictionary of parameters
     :kwarg options_context: List of strings of parent key names if ``requirements`` are
         in a sub spec.
+    :kwarg add_required: None or list to which all required parameters are added
 
     :returns: Empty dictionary or raises :class:`TypeError` if the
     """
@@ -187,8 +195,13 @@ def check_required_by(requirements, parameters, options_context=None):
     if requirements is None:
         return result
 
+    found_in = " found in %s" % " -> ".join(options_context) if options_context else ''
+    deprecation_fmt = f'The explicit none (null) value specified to %s{found_in} will not be treated as "not specified" in the future'
+
     for (key, value) in requirements.items():
         if key not in parameters or parameters[key] is None:
+            if key in parameters and parameters[key] is None:
+                deprecate(msg=deprecation_fmt % key, version='2.20')
             continue
         result[key] = []
         # Support strings (single-item lists)
@@ -197,6 +210,10 @@ def check_required_by(requirements, parameters, options_context=None):
         for required in value:
             if required not in parameters or parameters[required] is None:
                 result[key].append(required)
+                if required in parameters:
+                    deprecate(msg=deprecation_fmt % required, version='2.20')
+            if add_required is not None:
+                add_required.append(required)
 
     if result:
         for key, missing in result.items():
@@ -209,7 +226,7 @@ def check_required_by(requirements, parameters, options_context=None):
     return result
 
 
-def check_required_arguments(argument_spec, parameters, options_context=None):
+def check_required_arguments(argument_spec, parameters, options_context=None, add_required=None):
     """Check all parameters in argument_spec and return a list of parameters
     that are required but not present in parameters.
 
@@ -220,6 +237,7 @@ def check_required_arguments(argument_spec, parameters, options_context=None):
     :arg parameters: Dictionary of parameters
     :kwarg options_context: List of strings of parent key names if ``argument_spec`` are
         in a sub spec.
+    :kwarg add_required: None or list to which all required parameters are added
 
     :returns: Empty list or raises :class:`TypeError` if the check fails.
     """
@@ -230,8 +248,11 @@ def check_required_arguments(argument_spec, parameters, options_context=None):
 
     for (k, v) in argument_spec.items():
         required = v.get('required', False)
-        if required and k not in parameters:
-            missing.append(k)
+        if required:
+            if add_required is not None:
+                add_required.append(k)
+            if k not in parameters:
+                missing.append(k)
 
     if missing:
         msg = "missing required arguments: %s" % ", ".join(sorted(missing))
@@ -242,7 +263,7 @@ def check_required_arguments(argument_spec, parameters, options_context=None):
     return missing
 
 
-def check_required_if(requirements, parameters, options_context=None):
+def check_required_if(requirements, parameters, options_context=None, add_required=None):
     """Check parameters that are conditionally required
 
     Raises :class:`TypeError` if the check fails
@@ -289,6 +310,7 @@ def check_required_if(requirements, parameters, options_context=None):
 
     :kwarg options_context: List of strings of parent key names if ``requirements`` are
         in a sub spec.
+    :kwarg add_required: None or list to which all required parameters are added
     """
     results = []
     if requirements is None:
@@ -314,7 +336,7 @@ def check_required_if(requirements, parameters, options_context=None):
 
         if key in parameters and parameters[key] == val:
             for check in requirements:
-                count = count_terms(check, parameters)
+                count = count_terms(check, parameters, add_terms=add_required)
                 if count == 0:
                     missing['missing'].append(check)
         if len(missing['missing']) and len(missing['missing']) >= max_missing_count:
@@ -334,6 +356,32 @@ def check_required_if(requirements, parameters, options_context=None):
     return results
 
 
+def check_required_none(required_options, argument_spec, parameters, options_context=None):
+    """Check whether required options having value None are allowed to do so.
+    :arg required_options: A list of the recorded options.
+    :arg argument_spec: The argument argument spec containing the required options.
+    :arg parameters: Dictionary of parameters.
+    :kwarg options_context: The context for the sub-specs.
+
+    Raises TypeError for parameters set to None where allow_none_value in the argument_spec is False.
+    """
+    for required_option in list(required_options):
+        if required_option not in parameters:
+            continue
+        if required_option not in argument_spec or parameters[required_option] is not None:
+            continue
+        allow_none_value = argument_spec[required_option].get('allow_none_value')
+        if allow_none_value:
+            continue
+
+        found_in = " found in %s" % " -> ".join(options_context) if options_context else ''
+        msg = f"required parameter {required_option}{found_in} {'cannot be' if allow_none_value is False else 'is'} none (null)"
+        if allow_none_value is False:
+            raise TypeError(to_native(msg))
+        else:
+            deprecate(msg, version='2.20')
+
+
 def check_missing_parameters(parameters, required_parameters=None):
     """This is for checking for required params when we can not check via
     argspec because we need more information than is simply given in the argspec.

test/integration/targets/argspec/library/argspec.py

@@ -12,9 +12,16 @@ def main():
         {
             'required': {
                 'required': True,
+                'type': 'raw',
             },
             'required_one_of_one': {},
             'required_one_of_two': {},
+            'required_one_of_three': {
+                'allow_none_value': True,
+            },
+            'required_one_of_four': {
+                'allow_none_value': False,
+            },
             'required_by_one': {},
             'required_by_two': {},
             'required_by_three': {},
@@ -252,7 +259,7 @@ def main():
             ('path', 'content', 'default_value',),
         ),
         required_one_of=(
-            ('required_one_of_one', 'required_one_of_two'),
+            ('required_one_of_one', 'required_one_of_two', 'required_one_of_three', 'required_one_of_four'),
         ),
         required_by={
             'required_by_one': ('required_by_two', 'required_by_three'),

test/integration/targets/argspec/tasks/main.yml

@@ -7,15 +7,39 @@
   register: argspec_required_fail
   ignore_errors: true
 
+- argspec:
+    required:
+    required_one_of_one: value
+  register: argspec_required_fail_none
+
 - argspec:
     required: value
     required_one_of_two: value
 
+- argspec:
+    required: value
+    # Explicit None is allowed for 'required_one_of_three'
+    required_one_of_three:
+  register: argspec_required_one_of_explicitly_allowed
+
+- argspec:
+    required: value
+    # Explicit None is not allowed for 'required_one_of_four'
+    required_one_of_four:
+  register: argspec_required_one_of_explicitly_disallowed
+  ignore_errors: true
+
 - argspec:
     required: value
   register: argspec_required_one_of_fail
   ignore_errors: true
 
+- argspec:
+    required: value
+    required_one_of_one:
+  register: argspec_required_one_of_fail_none
+  ignore_errors: true
+
 - argspec:
     required: value
     required_one_of_two: value
@@ -31,6 +55,33 @@
   register: argspec_required_by_fail
   ignore_errors: true
 
+- argspec:
+    required: value
+    required_one_of_two: value
+    required_by_one:
+    required_by_two:
+    required_by_three: value
+  register: argspec_required_by_fail_none_1
+  ignore_errors: true
+
+- argspec:
+    required: value
+    required_one_of_two: value
+    required_by_one: value
+    required_by_two:
+    required_by_three: value
+  register: argspec_required_by_fail_none_2
+  ignore_errors: true
+
+- argspec:
+    required: value
+    required_one_of_two: value
+    required_by_one:
+    required_by_two:
+    required_by_three: value
+  register: argspec_required_by_fail_none_3
+  ignore_errors: true
+
 - argspec:
     state: absent
     required: value
@@ -43,6 +94,14 @@
   register: argspec_required_if_fail
   ignore_errors: true
 
+- argspec:
+    state: present
+    path:
+    required: value
+    required_one_of_one: value
+  register: argspec_required_if_fail_none
+  ignore_errors: true
+
 - argspec:
     state: present
     path: foo
@@ -64,6 +123,15 @@
   register: argspec_mutually_exclusive_fail
   ignore_errors: true
 
+- argspec:
+    state: present
+    content: foo
+    path:
+    required: value
+    required_one_of_one: value
+  register: argspec_mutually_exclusive_fail_none
+  ignore_errors: true
+
 - argspec:
     mapping:
       foo: bar
@@ -134,6 +202,15 @@
   register: argspec_required_together_fail
   ignore_errors: true
 
+- argspec:
+    required_together:
+      - thing: foo
+        other:
+    required: value
+    required_one_of_one: value
+  register: argspec_required_together_fail_none
+  ignore_errors: true
+
 - argspec:
     required_together:
       - thing: foo
@@ -162,6 +239,15 @@
   register: argspec_required_if_fail_2
   ignore_errors: true
 
+- argspec:
+    required_if:
+      - thing: foo
+        other:
+    required: value
+    required_one_of_one: value
+  register: argspec_required_if_fail_2_none
+  ignore_errors: true
+
 - argspec:
     required_one_of:
       - thing: foo
@@ -177,6 +263,14 @@
   register: argspec_required_one_of_fail_2
   ignore_errors: true
 
+- argspec:
+    required_one_of:
+      - thing:
+    required: value
+    required_one_of_one: value
+  register: argspec_required_one_of_fail_2_none
+  ignore_errors: true
+
 - argspec:
     required_by:
       - thing: foo
@@ -192,6 +286,33 @@
   register: argspec_required_by_fail_2
   ignore_errors: true
 
+- argspec:
+    required_by:
+      - thing: foo
+        other:
+    required: value
+    required_one_of_one: value
+  register: argspec_required_by_fail_2_none_1
+  ignore_errors: true
+
+- argspec:
+    required_by:
+      - thing:
+        other: bar
+    required: value
+    required_one_of_one: value
+  register: argspec_required_by_fail_2_none_2
+  ignore_errors: true
+
+- argspec:
+    required_by:
+      - thing:
+        other:
+    required: value
+    required_one_of_one: value
+  register: argspec_required_by_fail_2_none_3
+  ignore_errors: true
+
 - argspec:
     json: !!str '{"foo": "bar"}'
     required: value
@@ -240,6 +361,14 @@
   register: argspec_fail_required_together_2
   ignore_errors: true
 
+- argspec:
+    required_together_one: foo
+    required_together_two:
+    required: value
+    required_one_of_one: value
+  register: argspec_fail_required_together_2_none
+  ignore_errors: true
+
 - argspec:
     suboptions_list_no_elements:
       - thing: foo
@@ -493,14 +622,33 @@
 - assert:
     that:
       - argspec_required_fail is failed
+      - argspec_required_fail_none is succeeded
+      - "'required parameter required is none (null)' == argspec_required_fail_none.deprecations[0]['msg']"
 
       - argspec_required_one_of_fail is failed
+      - argspec_required_one_of_fail_none is succeeded
+      - "'required parameter required_one_of_one is none (null)' == argspec_required_one_of_fail_none.deprecations[0]['msg']"
+
+      - argspec_required_one_of_explicitly_allowed.deprecations is undefined
+      - argspec_required_one_of_explicitly_disallowed is failed
+      - argspec_required_one_of_explicitly_disallowed.msg == 'required parameter required_one_of_four cannot be none (null)'
 
       - argspec_required_by_fail is failed
+      - argspec_required_by_fail_none_1 is succeeded
+      - argspec_required_by_fail_none_1.deprecations | length == 1
+      - argspec_required_by_fail_none_1.deprecations[0].msg == 'The explicit none (null) value specified to required_by_one will not be treated as "not specified" in the future'
+      - argspec_required_by_fail_none_2 is failed
+      - argspec_required_by_fail_none_2.msg == "missing parameter(s) required by 'required_by_one': required_by_two"
+      - argspec_required_by_fail_none_3 is succeeded
+      - argspec_required_by_fail_none_3.deprecations | length == 1
+      - argspec_required_by_fail_none_3.deprecations[0].msg == 'The explicit none (null) value specified to required_by_one will not be treated as "not specified" in the future'
 
       - argspec_required_if_fail is failed
+      - argspec_required_if_fail_none is succeeded
+      - "'required parameter path is none (null)' == argspec_required_if_fail_none.deprecations[0]['msg']"
 
       - argspec_mutually_exclusive_fail is failed
+      - argspec_mutually_exclusive_fail_none is failed
 
       - argspec_good_mapping is successful
       - >-
@@ -519,12 +667,26 @@
       - argspec_bad_mapping_list is failed
 
       - argspec_required_together_fail is failed
+      - argspec_required_together_fail_none is succeeded
+      - "'required parameter other found in required_together is none (null)' == argspec_required_together_fail_none.deprecations[0]['msg']"
 
       - argspec_required_if_fail_2 is failed
+      - argspec_required_if_fail_2_none is succeeded
+      - "'required parameter other found in required_if is none (null)' == argspec_required_if_fail_2_none.deprecations[0]['msg']"
 
       - argspec_required_one_of_fail_2 is failed
+      - argspec_required_one_of_fail_2_none is succeeded
+      - "'required parameter thing found in required_one_of is none (null)' == argspec_required_one_of_fail_2_none.deprecations[0]['msg']"
 
       - argspec_required_by_fail_2 is failed
+      - argspec_required_by_fail_2_none_1 is failed
+      - argspec_required_by_fail_2_none_1.msg == "missing parameter(s) required by 'thing': other"
+      - argspec_required_by_fail_2_none_2 is succeeded
+      - argspec_required_by_fail_2_none_2.deprecations | length == 1
+      - argspec_required_by_fail_2_none_2.deprecations[0].msg == 'The explicit none (null) value specified to thing found in required_by will not be treated as "not specified" in the future'
+      - argspec_required_by_fail_2_none_3 is succeeded
+      - argspec_required_by_fail_2_none_3.deprecations | length == 1
+      - argspec_required_by_fail_2_none_3.deprecations[0].msg == 'The explicit none (null) value specified to thing found in required_by will not be treated as "not specified" in the future'
 
       - argspec_good_json_string is successful
       - >-
@@ -537,6 +699,8 @@
       - argspec_fail_on_missing_params_bad is failed
 
       - argspec_fail_required_together_2 is failed
+      - argspec_fail_required_together_2_none is succeeded
+      - "'required parameter required_together_two is none (null)' == argspec_fail_required_together_2_none.deprecations[0]['msg']"
 
       - >-
         argspec_suboptions_list_no_elements.suboptions_list_no_elements.0 == {'thing': 'foo'}

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

@@ -299,6 +299,7 @@ def argument_spec_schema(for_collection):
             'choices': Any([object], (object,)),
             'context': dict,
             'required': bool,
+            'allow_none_value': bool,
             'no_log': bool,
             'aliases': Any(list_string_types, tuple(list_string_types)),
             'apply_defaults': bool,