arg_spec - rework _check_arguments() (#72447)

* Move _syslog_facitily to __init__
  No good reason it should not be set for each object

* Move internal property setting to private method
* Create check_arguments() function
* Remove unused import
* Rename function to better match its behavior
  Change the behavior to return a set, either empty or populated, with unsupported keys.
  Accept legal_inputs as optional which will not required calling handle_aliases before calling
  get_unsupported_parameters().

* Add changelog
* Rework function behavior and documentation
  I realized I missed the original intent of this method when moving it to a function. It
  is meant to compared the parameter keys to legal inputs always, not compare
  parameter keys to argument spec keys, even though the argument spec keys should
  be a subset of legal inputs.

* Add tests
* Fix typo.
* Set internal properties when handling suboptions

changelogs/fragments/arg_spec-check_arguments-handle_aliases.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - create ``get_unsupported_parameters`` validation function (https://github.com/ansible/ansible/pull/72447/files)

lib/ansible/module_utils/basic.py

@@ -156,6 +156,7 @@ from ansible.module_utils.common.sys_info import (
 )
 from ansible.module_utils.pycompat24 import get_exception, literal_eval
 from ansible.module_utils.common.parameters import (
+    get_unsupported_parameters,
     handle_aliases,
     list_deprecations,
     list_no_log_values,
@@ -692,6 +693,7 @@ class AnsibleModule(object):
         self._diff = False
         self._socket_path = None
         self._shell = None
+        self._syslog_facility = 'LOG_USER'
         self._verbosity = 0
         # May be used to set modifications to the environment for any
         # run_command invocation
@@ -728,6 +730,7 @@ class AnsibleModule(object):
         # a known valid (LANG=C) if it's an invalid/unavailable locale
         self._check_locale()
 
+        self._set_internal_properties()
         self._check_arguments()
 
         # check exclusive early
@@ -1527,29 +1530,20 @@ class AnsibleModule(object):
             deprecate(message['msg'], version=message.get('version'), date=message.get('date'),
                       collection_name=message.get('collection_name'))
 
-    def _check_arguments(self, spec=None, param=None, legal_inputs=None):
+    def _set_internal_properties(self, argument_spec=None, module_parameters=None):
-        self._syslog_facility = 'LOG_USER'
+        if argument_spec is None:
-        unsupported_parameters = set()
+            argument_spec = self.argument_spec
-        if spec is None:
+        if module_parameters is None:
-            spec = self.argument_spec
+            module_parameters = self.params
-        if param is None:
-            param = self.params
-        if legal_inputs is None:
-            legal_inputs = self._legal_inputs
-
-        for k in list(param.keys()):
-
-            if k not in legal_inputs:
-                unsupported_parameters.add(k)
 
         for k in PASS_VARS:
             # handle setting internal properties from internal ansible vars
             param_key = '_ansible_%s' % k
-            if param_key in param:
+            if param_key in module_parameters:
                 if k in PASS_BOOLS:
-                    setattr(self, PASS_VARS[k][0], self.boolean(param[param_key]))
+                    setattr(self, PASS_VARS[k][0], self.boolean(module_parameters[param_key]))
                 else:
-                    setattr(self, PASS_VARS[k][0], param[param_key])
+                    setattr(self, PASS_VARS[k][0], module_parameters[param_key])
 
                 # clean up internal top level params:
                 if param_key in self.params:
@@ -1559,6 +1553,17 @@ class AnsibleModule(object):
                 if not hasattr(self, PASS_VARS[k][0]):
                     setattr(self, PASS_VARS[k][0], PASS_VARS[k][1])
 
+    def _check_arguments(self, spec=None, param=None, legal_inputs=None):
+        unsupported_parameters = set()
+        if spec is None:
+            spec = self.argument_spec
+        if param is None:
+            param = self.params
+        if legal_inputs is None:
+            legal_inputs = self._legal_inputs
+
+        unsupported_parameters = get_unsupported_parameters(spec, param, legal_inputs)
+
         if unsupported_parameters:
             msg = "Unsupported parameters for (%s) module: %s" % (self._name, ', '.join(sorted(list(unsupported_parameters))))
             if self._options_context:
@@ -1571,6 +1576,7 @@ class AnsibleModule(object):
                     supported_parameters.append(key)
             msg += " Supported parameters include: %s" % (', '.join(supported_parameters))
             self.fail_json(msg=msg)
+
         if self.check_mode and not self.supports_check_mode:
             self.exit_json(skipped=True, msg="remote module (%s) does not support check mode" % self._name)
 
@@ -1817,6 +1823,7 @@ class AnsibleModule(object):
 
                     options_legal_inputs = list(spec.keys()) + list(options_aliases.keys())
 
+                    self._set_internal_properties(spec, param)
                     self._check_arguments(spec, param, options_legal_inputs)
 
                     # check exclusive early

lib/ansible/module_utils/common/parameters.py

@@ -195,3 +195,28 @@ def handle_aliases(argument_spec, params, alias_warnings=None):
                 params[k] = params[alias]
 
     return aliases_results, legal_inputs
+
+
+def get_unsupported_parameters(argument_spec, module_parameters, legal_inputs=None):
+    """Check keys in module_parameters against those provided in legal_inputs
+    to ensure they contain legal values. If legal_inputs are not supplied,
+    they will be generated using the argument_spec.
+
+    :arg argument_spec: Dictionary of parameters, their type, and valid values.
+    :arg module_parameters: Dictionary of module parameters.
+    :arg legal_inputs: List of valid key names property names. Overrides values
+        in argument_spec.
+
+    :returns: Set of unsupported parameters. Empty set if no unsupported parameters
+        are found.
+    """
+
+    if legal_inputs is None:
+        aliases, legal_inputs = handle_aliases(argument_spec, module_parameters)
+
+    unsupported_parameters = set()
+    for k in module_parameters.keys():
+        if k not in legal_inputs:
+            unsupported_parameters.add(k)
+
+    return unsupported_parameters

lib/ansible/module_utils/common/validation.py

@@ -9,7 +9,7 @@ import os
 import re
 
 from ast import literal_eval
-from ansible.module_utils._text import to_native, to_text
+from ansible.module_utils._text import to_native
 from ansible.module_utils.common._json_compat import json
 from ansible.module_utils.common.collections import is_iterable
 from ansible.module_utils.common.text.converters import jsonify

test/units/module_utils/common/parameters/test_check_arguments.py

@@ -0,0 +1,65 @@
+# -*- coding: utf-8 -*-
+# Copyright (c) 2020 Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+
+import pytest
+
+from ansible.module_utils.common.parameters import get_unsupported_parameters
+
+
+@pytest.fixture
+def argument_spec():
+    return {
+        'state': {'aliases': ['status']},
+        'enabled': {},
+    }
+
+
+def mock_handle_aliases(*args):
+    aliases = {}
+    legal_inputs = [
+        '_ansible_check_mode',
+        '_ansible_debug',
+        '_ansible_diff',
+        '_ansible_keep_remote_files',
+        '_ansible_module_name',
+        '_ansible_no_log',
+        '_ansible_remote_tmp',
+        '_ansible_selinux_special_fs',
+        '_ansible_shell_executable',
+        '_ansible_socket',
+        '_ansible_string_conversion_action',
+        '_ansible_syslog_facility',
+        '_ansible_tmpdir',
+        '_ansible_verbosity',
+        '_ansible_version',
+        'state',
+        'status',
+        'enabled',
+    ]
+
+    return aliases, legal_inputs
+
+
+@pytest.mark.parametrize(
+    ('module_parameters', 'legal_inputs', 'expected'),
+    (
+        ({'fish': 'food'}, ['state', 'enabled'], set(['fish'])),
+        ({'state': 'enabled', 'path': '/var/lib/path'}, None, set(['path'])),
+        ({'state': 'enabled', 'path': '/var/lib/path'}, ['state', 'path'], set()),
+        ({'state': 'enabled', 'path': '/var/lib/path'}, ['state'], set(['path'])),
+        ({}, None, set()),
+        ({'state': 'enabled'}, None, set()),
+        ({'status': 'enabled', 'enabled': True, 'path': '/var/lib/path'}, None, set(['path'])),
+        ({'status': 'enabled', 'enabled': True}, None, set()),
+    )
+)
+def test_check_arguments(argument_spec, module_parameters, legal_inputs, expected, mocker):
+    mocker.patch('ansible.module_utils.common.parameters.handle_aliases', side_effect=mock_handle_aliases)
+    result = get_unsupported_parameters(argument_spec, module_parameters, legal_inputs)
+
+    assert result == expected