Move _handle_aliases() out of basic.py (#48578)

Refinements:
- return legal_inputs and update class properties
- remove redundant arguments from method and handle in caller
- add better exception types to method

* Add unit tests for handle_aliases

5 files changed, 189 insertions, 47 deletions

diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py
index bcd8edd5c0..5eb50cc8a4 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -39,24 +39,6 @@ FILE_ATTRIBUTES = {
     'Z': 'compresseddirty',
 }
 
-PASS_VARS = {
-    'check_mode': 'check_mode',
-    'debug': '_debug',
-    'diff': '_diff',
-    'keep_remote_files': '_keep_remote_files',
-    'module_name': '_name',
-    'no_log': 'no_log',
-    'remote_tmp': '_remote_tmp',
-    'selinux_special_fs': '_selinux_special_fs',
-    'shell_executable': '_shell',
-    'socket': '_socket_path',
-    'syslog_facility': '_syslog_facility',
-    'string_conversion_action': '_string_conversion_action',
-    'tmpdir': '_tmpdir',
-    'verbosity': '_verbosity',
-    'version': 'ansible_version',
-}
-
 PASS_BOOLS = ('no_log', 'debug', 'diff')
 
 # Ansible modules can be written in any language.
@@ -171,6 +153,11 @@ from ansible.module_utils.common.sys_info import (
     get_platform_subclass,
 )
 from ansible.module_utils.pycompat24 import get_exception, literal_eval
+from ansible.module_utils.common.parameters import (
+    handle_aliases,
+    PASS_VARS,
+)
+
 from ansible.module_utils.six import (
     PY2,
     PY3,
@@ -186,11 +173,9 @@ from ansible.module_utils._text import to_native, to_bytes, to_text
 from ansible.module_utils.common._utils import get_all_subclasses as _get_all_subclasses
 from ansible.module_utils.parsing.convert_bool import BOOLEANS, BOOLEANS_FALSE, BOOLEANS_TRUE, boolean
 
-
-# Note: When getting Sequence from collections, it matches with strings.  If
+# Note: When getting Sequence from collections, it matches with strings. If
 # this matters, make sure to check for strings before checking for sequencetype
 SEQUENCETYPE = frozenset, KeysView, Sequence
-
 PASSWORD_MATCH = re.compile(r'^(?:.+[-_\s])?pass(?:[-_\s]?(?:word|phrase|wrd|wd)?)(?:[-_\s].+)?$', re.I)
 
 _NUMBERTYPES = tuple(list(integer_types) + [float])
@@ -792,7 +777,7 @@ class AnsibleModule(object):
         self._string_conversion_action = ''
 
         self.aliases = {}
-        self._legal_inputs = ['_ansible_%s' % k for k in PASS_VARS]
+        self._legal_inputs = []
         self._options_context = list()
         self._tmpdir = None
 
@@ -807,7 +792,7 @@ class AnsibleModule(object):
         # append to legal_inputs and then possibly check against them
         try:
             self.aliases = self._handle_aliases()
-        except Exception as e:
+        except (ValueError, TypeError) as e:
             # Use exceptions here because it isn't safe to call fail_json until no_log is processed
             print('\n{"failed": true, "msg": "Module alias error: %s"}' % to_native(e))
             sys.exit(1)
@@ -1581,32 +1566,14 @@ class AnsibleModule(object):
                            to_native(e), exception=traceback.format_exc())
 
     def _handle_aliases(self, spec=None, param=None):
-        # this uses exceptions as it happens before we can safely call fail_json
-        aliases_results = {}  # alias:canon
+        if spec is None:
+            spec = self.argument_spec
         if param is None:
             param = self.params
 
-        if spec is None:
-            spec = self.argument_spec
-        for (k, v) in spec.items():
-            self._legal_inputs.append(k)
-            aliases = v.get('aliases', None)
-            default = v.get('default', None)
-            required = v.get('required', False)
-            if default is not None and required:
-                # not alias specific but this is a good place to check this
-                raise Exception("internal error: required and default are mutually exclusive for %s" % k)
-            if aliases is None:
-                continue
-            if not isinstance(aliases, SEQUENCETYPE) or isinstance(aliases, (binary_type, text_type)):
-                raise Exception('internal error: aliases must be a list or tuple')
-            for alias in aliases:
-                self._legal_inputs.append(alias)
-                aliases_results[alias] = k
-                if alias in param:
-                    param[k] = param[alias]
-
-        return aliases_results
+        # this uses exceptions as it happens before we can safely call fail_json
+        alias_results, self._legal_inputs = handle_aliases(spec, param)
+        return alias_results
 
     def _handle_no_log_values(self, spec=None, param=None):
         if spec is None:
diff --git a/lib/ansible/module_utils/common/parameters.py b/lib/ansible/module_utils/common/parameters.py
new file mode 100644
index 0000000000..b534ab0d93
--- /dev/null
+++ b/lib/ansible/module_utils/common/parameters.py
@@ -0,0 +1,65 @@
+# -*- coding: utf-8 -*-
+# Copyright (c) 2019 Ansible Project
+# Simplified BSD License (see licenses/simplified_bsd.txt or https://opensource.org/licenses/BSD-2-Clause)
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+from ansible.module_utils._text import to_native
+from ansible.module_utils.common._collections_compat import Mapping
+from ansible.module_utils.common.collections import is_iterable
+
+from ansible.module_utils.six import (
+    binary_type,
+    integer_types,
+    text_type,
+)
+
+# Python2 & 3 way to get NoneType
+NoneType = type(None)
+
+PASS_VARS = {
+    'check_mode': 'check_mode',
+    'debug': '_debug',
+    'diff': '_diff',
+    'keep_remote_files': '_keep_remote_files',
+    'module_name': '_name',
+    'no_log': 'no_log',
+    'remote_tmp': '_remote_tmp',
+    'selinux_special_fs': '_selinux_special_fs',
+    'shell_executable': '_shell',
+    'socket': '_socket_path',
+    'string_conversion_action': '_string_conversion_action',
+    'syslog_facility': '_syslog_facility',
+    'tmpdir': '_tmpdir',
+    'verbosity': '_verbosity',
+    'version': 'ansible_version',
+}
+
+
+def handle_aliases(argument_spec, params):
+    """Return a two item tuple. The first is a dictionary of aliases, the second is
+    a list of legal inputs."""
+
+    legal_inputs = ['_ansible_%s' % k for k in PASS_VARS]
+    aliases_results = {}  # alias:canon
+
+    for (k, v) in argument_spec.items():
+        legal_inputs.append(k)
+        aliases = v.get('aliases', None)
+        default = v.get('default', None)
+        required = v.get('required', False)
+        if default is not None and required:
+            # not alias specific but this is a good place to check this
+            raise ValueError("internal error: required and default are mutually exclusive for %s" % k)
+        if aliases is None:
+            continue
+        if not is_iterable(aliases) or isinstance(aliases, (binary_type, text_type)):
+            raise TypeError('internal error: aliases must be a list or tuple')
+        for alias in aliases:
+            legal_inputs.append(alias)
+            aliases_results[alias] = k
+            if alias in params:
+                params[k] = params[alias]
+
+    return aliases_results, legal_inputs
diff --git a/test/units/executor/module_common/test_recursive_finder.py b/test/units/executor/module_common/test_recursive_finder.py
index 0b919f6987..e61ad1efc8 100644
--- a/test/units/executor/module_common/test_recursive_finder.py
+++ b/test/units/executor/module_common/test_recursive_finder.py
@@ -44,7 +44,9 @@ MODULE_UTILS_BASIC_IMPORTS = frozenset((('_text',),
                                         ('basic',),
                                         ('common', '__init__'),
                                         ('common', '_collections_compat'),
+                                        ('common', 'collections'),
                                         ('common', 'file'),
+                                        ('common', 'parameters'),
                                         ('common', 'process'),
                                         ('common', 'sys_info'),
                                         ('common', '_utils'),
@@ -58,8 +60,13 @@ MODULE_UTILS_BASIC_IMPORTS = frozenset((('_text',),
 
 MODULE_UTILS_BASIC_FILES = frozenset(('ansible/module_utils/_text.py',
                                       'ansible/module_utils/basic.py',
-                                      'ansible/module_utils/common/__init__.py',
+                                      'ansible/module_utils/six/__init__.py',
+                                      'ansible/module_utils/_text.py',
                                       'ansible/module_utils/common/_collections_compat.py',
+                                      'ansible/module_utils/common/collections.py',
+                                      'ansible/module_utils/common/parameters.py',
+                                      'ansible/module_utils/parsing/convert_bool.py',
+                                      'ansible/module_utils/common/__init__.py',
                                       'ansible/module_utils/common/file.py',
                                       'ansible/module_utils/common/process.py',
                                       'ansible/module_utils/common/sys_info.py',
diff --git a/test/units/module_utils/basic/test_atomic_move.py b/test/units/module_utils/basic/test_atomic_move.py
index 1dea91702c..dce3941906 100644
--- a/test/units/module_utils/basic/test_atomic_move.py
+++ b/test/units/module_utils/basic/test_atomic_move.py
@@ -62,6 +62,7 @@ def fake_stat(mocker):
     stat1.st_mode = 0o0644
     stat1.st_uid = 0
     stat1.st_gid = 0
+    stat1.st_flags = 0
     yield stat1
 
 
diff --git a/test/units/module_utils/common/parameters/test_handle_aliases.py b/test/units/module_utils/common/parameters/test_handle_aliases.py
new file mode 100644
index 0000000000..bc88437fba
--- /dev/null
+++ b/test/units/module_utils/common/parameters/test_handle_aliases.py
@@ -0,0 +1,102 @@
+# -*- coding: utf-8 -*-
+# Copyright (c) 2019 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 handle_aliases
+from ansible.module_utils._text import to_native
+
+DEFAULT_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',
+]
+
+
+def test_handle_aliases_no_aliases():
+    argument_spec = {
+        'name': {'type': 'str'},
+    }
+
+    params = {
+        'name': 'foo',
+        'path': 'bar'
+    }
+
+    expected = (
+        {},
+        DEFAULT_LEGAL_INPUTS + ['name'],
+    )
+    expected[1].sort()
+
+    result = handle_aliases(argument_spec, params)
+    result[1].sort()
+    assert expected == result
+
+
+def test_handle_aliases_basic():
+    argument_spec = {
+        'name': {'type': 'str', 'aliases': ['surname', 'nick']},
+    }
+
+    params = {
+        'name': 'foo',
+        'path': 'bar',
+        'surname': 'foo',
+        'nick': 'foo',
+    }
+
+    expected = (
+        {'surname': 'name', 'nick': 'name'},
+        DEFAULT_LEGAL_INPUTS + ['name', 'surname', 'nick'],
+    )
+    expected[1].sort()
+
+    result = handle_aliases(argument_spec, params)
+    result[1].sort()
+    assert expected == result
+
+
+def test_handle_aliases_value_error():
+    argument_spec = {
+        'name': {'type': 'str', 'aliases': ['surname', 'nick'], 'default': 'bob', 'required': True},
+    }
+
+    params = {
+        'name': 'foo',
+    }
+
+    with pytest.raises(ValueError) as ve:
+        handle_aliases(argument_spec, params)
+        assert 'internal error: aliases must be a list or tuple' == to_native(ve.error)
+
+
+def test_handle_aliases_type_error():
+    argument_spec = {
+        'name': {'type': 'str', 'aliases': 'surname'},
+    }
+
+    params = {
+        'name': 'foo',
+    }
+
+    with pytest.raises(TypeError) as te:
+        handle_aliases(argument_spec, params)
+        assert 'internal error: required and default are mutually exclusive' in to_native(te.error)