Simplify FactCache.update()

We only need FactCache.update() for the backwards compatibility shim as
MutableMapping.update() will do the right thing.

2 files changed, 45 insertions, 19 deletions

diff --git a/lib/ansible/vars/fact_cache.py b/lib/ansible/vars/fact_cache.py
index 4db8fd5b44..25ffe70e49 100644
--- a/lib/ansible/vars/fact_cache.py
+++ b/lib/ansible/vars/fact_cache.py
@@ -59,24 +59,40 @@ class FactCache(MutableMapping):
         self._plugin.flush()
 
     def update(self, *args):
-        """ We override the normal update to ensure we always use the 'setter' method """
+        """
+        Backwards compat shim
+
+        We thought we needed this to ensure we always called the plugin's set() method but
+        MutableMapping.update() will call our __setitem__() just fine.  It's the calls to update
+        that we need to be careful of.  This contains a bug::
+
+            fact_cache[host.name].update(facts)
+
+        It retrieves a *copy* of the facts for host.name and then updates the copy.  So the changes
+        aren't persisted.
+
+        Instead we need to do::
+
+            fact_cache.update({host.name, facts})
+
+        Which will use FactCache's update() method.
+
+        We currently need this shim for backwards compat because the update() method that we had
+        implemented took key and value as arguments instead of taking a dict.  We can remove the
+        shim in 2.12 as MutableMapping.update() should do everything that we need.
+        """
         if len(args) == 2:
-            display.deprecated('Calling FactCache.update(key, value) is deprecated.  Use the'
-                               ' normal dict.update() signature of FactCache.update({key: value})'
-                               ' instead.', version='2.12')
-            host_facts = {args[0]: args[1]}
+            # Deprecated.  Call the new function with this name
+            display.deprecated('Calling FactCache().update(key, value) is deprecated.  Use'
+                               ' FactCache().first_order_merge(key, value) if you want the old'
+                               ' behaviour or use FactCache().update({key: value}) if you want'
+                               ' dict-like behaviour.', version='2.12')
+            return self.first_order_merge(*args)
+
         elif len(args) == 1:
             host_facts = args[0]
+
         else:
             raise TypeError('update expected at most 1 argument, got {0}'.format(len(args)))
 
-        for key in host_facts:
-            try:
-                host_cache = self._plugin.get(key)
-                if host_cache:
-                    host_cache.update(host_facts[key])
-                else:
-                    host_cache = host_facts[key]
-                self._plugin.set(key, host_cache)
-            except KeyError:
-                self._plugin.set(key, host_facts[key])
+        super(FactCache, self).update(host_facts)
diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py
index c5561b51fe..afe1b61362 100644
--- a/lib/ansible/vars/manager.py
+++ b/lib/ansible/vars/manager.py
@@ -629,13 +629,23 @@ class VariableManager:
         Sets or updates the given facts for a host in the fact cache.
         '''
 
-        if not isinstance(facts, dict):
-            raise AnsibleAssertionError("the type of 'facts' to set for host_facts should be a dict but is a %s" % type(facts))
+        if not isinstance(facts, Mapping):
+            raise AnsibleAssertionError("the type of 'facts' to set for host_facts should be a Mapping but is a %s" % type(facts))
 
         try:
-            self._fact_cache.update({host.name: facts})
+            host_cache = self._fact_cache[host.name]
         except KeyError:
-            self._fact_cache[host.name] = facts
+            # We get to set this as new
+            host_cache = facts
+        else:
+            if not isinstance(host_cache, MutableMapping):
+                raise TypeError('The object retrieved for {0} must be a MutableMapping but was'
+                                ' a {1}'.format(host.name, type(host_cache)))
+            # Update the existing facts
+            host_cache.update(facts)
+
+        # Save the facts back to the backing store
+        self._fact_cache[host.name] = host_cache
 
     def set_nonpersistent_facts(self, host, facts):
         '''