Option parsing: warn if both an option and its alias are specified for a module (#53698)

* Print warning when both an option and its alias is specified.

* Improve output.

* Put warnings into self._warnings directly, resp. use self.warn() when handling subspecs.

* Add changelog.

* Add unit test.

changelogs/fragments/53698-alias-collision-warning.yaml

@@ -0,0 +1,2 @@
+minor_changes:
+- Ansible will now warn if two aliases of the same option are used for Python modules.

lib/ansible/module_utils/basic.py

@@ -1413,14 +1413,17 @@ class AnsibleModule(object):
             self.fail_json(msg="An unknown error was encountered while attempting to validate the locale: %s" %
                            to_native(e), exception=traceback.format_exc())
 
-    def _handle_aliases(self, spec=None, param=None):
+    def _handle_aliases(self, spec=None, param=None, option_prefix=''):
         if spec is None:
             spec = self.argument_spec
         if param is None:
             param = self.params
 
         # this uses exceptions as it happens before we can safely call fail_json
-        alias_results, self._legal_inputs = handle_aliases(spec, param)
+        alias_warnings = []
+        alias_results, self._legal_inputs = handle_aliases(spec, param, alias_warnings=alias_warnings)
+        for option, alias in alias_warnings:
+            self._warnings.append('Both option %s and its alias %s are set.' % (option_prefix + option, option_prefix + alias))
         return alias_results
 
     def _handle_no_log_values(self, spec=None, param=None):
@@ -1665,7 +1668,7 @@ class AnsibleModule(object):
     def _check_type_bits(self, value):
         return check_type_bits(value)
 
-    def _handle_options(self, argument_spec=None, params=None):
+    def _handle_options(self, argument_spec=None, params=None, prefix=''):
         ''' deal with options to create sub spec '''
         if argument_spec is None:
             argument_spec = self.argument_spec
@@ -1692,12 +1695,17 @@ class AnsibleModule(object):
                 else:
                     elements = params[k]
 
-                for param in elements:
+                for idx, param in enumerate(elements):
                     if not isinstance(param, dict):
                         self.fail_json(msg="value of %s must be of type dict or list of dict" % k)
 
+                    new_prefix = prefix + k
+                    if wanted == 'list':
+                        new_prefix += '[%d]' % idx
+                    new_prefix += '.'
+
                     self._set_fallbacks(spec, param)
-                    options_aliases = self._handle_aliases(spec, param)
+                    options_aliases = self._handle_aliases(spec, param, option_prefix=new_prefix)
 
                     self._handle_no_log_values(spec, param)
                     options_legal_inputs = list(spec.keys()) + list(options_aliases.keys())
@@ -1723,7 +1731,7 @@ class AnsibleModule(object):
                     self._set_defaults(pre=False, spec=spec, param=param)
 
                     # handle multi level options (sub argspec)
-                    self._handle_options(spec, param)
+                    self._handle_options(spec, param, new_prefix)
                 self._options_context.pop()
 
     def _get_wanted_type(self, wanted, k):

lib/ansible/module_utils/common/parameters.py

@@ -112,9 +112,13 @@ def list_deprecations(argument_spec, params):
     return deprecations
 
 
-def handle_aliases(argument_spec, params):
+def handle_aliases(argument_spec, params, alias_warnings=None):
     """Return a two item tuple. The first is a dictionary of aliases, the second is
-    a list of legal inputs."""
+    a list of legal inputs.
+
+    If a list is provided to the alias_warnings parameter, it will be filled with tuples
+    (option, alias) in every case where both an option and its alias are specified.
+    """
 
     legal_inputs = ['_ansible_%s' % k for k in PASS_VARS]
     aliases_results = {}  # alias:canon
@@ -135,6 +139,8 @@ def handle_aliases(argument_spec, params):
             legal_inputs.append(alias)
             aliases_results[alias] = k
             if alias in params:
+                if k in params and alias_warnings is not None:
+                    alias_warnings.append((k, alias))
                 params[k] = params[alias]
 
     return aliases_results, legal_inputs

test/units/module_utils/basic/test_argument_spec.py

@@ -233,6 +233,14 @@ class TestComplexArgSpecs:
         assert isinstance(am.params['foo'], str)
         assert am.params['foo'] == 'hello'
 
+    @pytest.mark.parametrize('stdin', [{'foo': 'hello1', 'dup': 'hello2'}], indirect=['stdin'])
+    def test_complex_duplicate_warning(self, stdin, complex_argspec):
+        """Test that the complex argspec issues a warning if we specify an option both with its canonical name and its alias"""
+        am = basic.AnsibleModule(**complex_argspec)
+        assert isinstance(am.params['foo'], str)
+        assert 'Both option foo and its alias dup are set.' in am._warnings
+        assert am.params['foo'] == 'hello2'
+
     @pytest.mark.parametrize('stdin', [{'foo': 'hello', 'bam': 'test'}], indirect=['stdin'])
     def test_complex_type_fallback(self, mocker, stdin, complex_argspec):
         """Test that the complex argspec works if we get a required parameter via fallback"""