make hash_params more robust in the face of many corner cases (#18701)

* make hash_params more robust in the face of many corner cases

Fixes #18680
Alternative fix to #18681

* add test case for role.hash_params

* Add role.hash_params test for more types

A set, a generator/iterable, and a Container that
is not Iterable.

lib/ansible/playbook/role/__init__.py

@@ -19,10 +19,10 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
-from ansible.compat.six import iteritems
-
+import collections
 import os
 
+from ansible.compat.six import iteritems, binary_type, text_type
 from ansible.errors import AnsibleError, AnsibleParserError
 from ansible.playbook.attribute import FieldAttribute
 from ansible.playbook.base import Base
@@ -41,25 +41,54 @@ __all__ = ['Role', 'hash_params']
 #       the role due to the fact that it would require the use of self
 #       in a static method. This is also used in the base class for
 #       strategies (ansible/plugins/strategy/__init__.py)
+
 def hash_params(params):
-    if not isinstance(params, dict):
-        if isinstance(params, list):
-            return frozenset(params)
+    """
+    Construct a data structure of parameters that is hashable.
+
+    This requires changing any mutable data structures into immutable ones.
+    We chose a frozenset because role parameters have to be unique.
+
+    .. warning::  this does not handle unhashable scalars.  Two things
+        mitigate that limitation:
+
+        1) There shouldn't be any unhashable scalars specified in the yaml
+        2) Our only choice would be to return an error anyway.
+    """
+    # Any container is unhashable if it contains unhashable items (for
+    # instance, tuple() is a Hashable subclass but if it contains a dict, it
+    # cannot be hashed)
+    if isinstance(params, collections.Container) and not isinstance(params, (text_type, binary_type)):
+        if isinstance(params, collections.Mapping):
+            try:
+                # Optimistically hope the contents are all hashable
+                new_params = frozenset(params.items())
+            except TypeError:
+                new_params = set()
+                for k, v in params.items():
+                    # Hash each entry individually
+                    new_params.update((k, hash_params(v)))
+                new_params = frozenset(new_params)
+
+        elif isinstance(params, (collections.Set, collections.Sequence)):
+            try:
+                # Optimistically hope the contents are all hashable
+                new_params = frozenset(params)
+            except TypeError:
+                new_params = set()
+                for v in params:
+                    # Hash each entry individually
+                    new_params.update(hash_params(v))
+                new_params = frozenset(new_params)
         else:
-            return params
-    else:
-        s = set()
-        for k,v in iteritems(params):
-            if isinstance(v, dict):
-                s.update((k, hash_params(v)))
-            elif isinstance(v, list):
-                things = []
-                for item in v:
-                    things.append(hash_params(item))
-                s.update((k, tuple(things)))
-            else:
-                s.update((k, v))
-        return frozenset(s)
+            # This is just a guess.
+            new_params = frozenset(params)
+        return new_params
+
+    # Note: We do not handle unhashable scalars but our only choice would be
+    # to raise an error there anyway.
+    return frozenset((params,))
+
 
 class Role(Base, Become, Conditional, Taggable):
 

test/units/playbook/role/test_role.py

@@ -19,18 +19,107 @@
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
+import collections
+
 from ansible.compat.tests import unittest
 from ansible.compat.tests.mock import patch, MagicMock
 
 from ansible.errors import AnsibleError, AnsibleParserError
 from ansible.playbook.block import Block
-from ansible.playbook.role import Role
-from ansible.playbook.role.include import RoleInclude
 from ansible.playbook.task import Task
 
 from units.mock.loader import DictDataLoader
 from units.mock.path import mock_unfrackpath_noop
 
+from ansible.playbook.role import Role
+from ansible.playbook.role.include import RoleInclude
+from ansible.playbook.role import hash_params
+
+
+class TestHashParams(unittest.TestCase):
+    def test(self):
+        params = {'foo': 'bar'}
+        res = hash_params(params)
+        self._assert_set(res)
+        self._assert_hashable(res)
+
+    def _assert_hashable(self, res):
+        a_dict = {}
+        try:
+            a_dict[res] = res
+        except TypeError as e:
+            self.fail('%s is not hashable: %s' % (res, e))
+
+    def _assert_set(self, res):
+        self.assertIsInstance(res, frozenset)
+
+    def test_dict_tuple(self):
+        params = {'foo': (1, 'bar',)}
+        res = hash_params(params)
+        self._assert_set(res)
+
+    def test_tuple(self):
+        params = (1, None, 'foo')
+        res = hash_params(params)
+        self._assert_hashable(res)
+
+    def test_tuple_dict(self):
+        params = ({'foo': 'bar'}, 37)
+        res = hash_params(params)
+        self._assert_hashable(res)
+
+    def test_list(self):
+        params = ['foo', 'bar', 1, 37, None]
+        res = hash_params(params)
+        self._assert_set(res)
+        self._assert_hashable(res)
+
+    def test_dict_with_list_value(self):
+        params = {'foo': [1, 4, 'bar']}
+        res = hash_params(params)
+        self._assert_set(res)
+        self._assert_hashable(res)
+
+    def test_empty_set(self):
+        params = set([])
+        res = hash_params(params)
+        self._assert_hashable(res)
+        self._assert_set(res)
+
+    def test_generator(self):
+        def my_generator():
+            for i in ['a', 1, None, {}]:
+                yield i
+
+        params = my_generator()
+        res = hash_params(params)
+        self._assert_hashable(res)
+
+    def test_container_but_not_iterable(self):
+        # This is a Container that is not iterable, which is unlikely but...
+        class MyContainer(collections.Container):
+            def __init__(self, some_thing):
+                self.data = []
+                self.data.append(some_thing)
+
+            def __contains__(self, item):
+                return item in self.data
+
+            def __hash__(self):
+                return hash(self.data)
+
+            def __len__(self):
+                return len(self.data)
+
+            def __call__(self):
+                return False
+
+        foo = MyContainer('foo bar')
+        params = foo
+
+        self.assertRaises(TypeError, hash_params, params)
+
+
 class TestRole(unittest.TestCase):
 
     def setUp(self):