FactCache changes

* Fix FactCache to conform to the dict API
  * update needs to take a dict rather than a key and a value
  * __init__ needs to allow for setting the intial dictionary
* Remove unneeded _display and _cache attributes
* Move ansible.plugins.cache.FactCache to
  ansible.vars.fact_cache.FactCache because this isn't part of the cache
  plugin API.
* Add backwards compatibility when calling update on the new FactCache
* Remove code for calling old FactCache. There's no way to call the old
  FactCache so there's no need for backwards compatible code for calling
  code.  Backwards compatibility is handling things which are calling
  the new FactCache.
* Port our code to the new FactCache location.

changelogs/fragments/vm_fix.yml

@@ -1,2 +1,6 @@
 bugfixes:
-    - fix issue with incorrect dict update in vars manager
+    - fix FactCache.update() to conform to the dict API.
+minor_changes:
+    - Moved the FactCache code from ansible.plugins.cache.FactCache to
+      ansible.vars.fact_cache.FactCache as it is not meant to be used to
+      implement cache plugins.

docs/docsite/rst/porting_guides/porting_guide_2.8.rst

@@ -52,6 +52,18 @@ Deprecated
         vars:
           ansible_aync_dir: /tmp/.ansible_async
 
+* Plugin writers who need a ``FactCache`` object should be aware of two deprecations:
+
+  1. The ``FactCache`` class has moved from ``ansible.plugins.cache.FactCache`` to
+     ``ansible.vars.fact_cache.FactCache``.  This is because the ``FactCache`` is not part of the
+     cache plugin API and cache plugin authors should not be subclassing it.  ``FactCache`` is still
+     available from its old location but will issue a deprecation warning when used from there.  The
+     old location will be removed in Ansible 2.12.
+
+  2. The ``FactCache.update()`` method has been converted to follow the dict API.  It now takes a
+     dictionary as its sole argument and updates itself with the dictionary's items.  The previous
+     API where ``update()`` took a key and a value will now issue a deprecation warning and will be
+     removed in 2.12.
 
 Modules
 =======

lib/ansible/plugins/cache/__init__.py

@@ -1,4 +1,5 @@
 # (c) 2014, Michael DeHaan <michael.dehaan@gmail.com>
+# (c) 2018, Ansible Project
 #
 # This file is part of Ansible
 #
@@ -29,10 +30,27 @@ from ansible.module_utils._text import to_bytes
 from ansible.module_utils.common._collections_compat import MutableMapping
 from ansible.plugins.loader import cache_loader
 from ansible.utils.display import Display
+from ansible.vars.fact_cache import FactCache as RealFactCache
 
 display = Display()
 
 
+class FactCache(RealFactCache):
+    """
+    This is for backwards compatibility.  Will be removed after deprecation.  It was removed as it
+    wasn't actually part of the cache plugin API.  It's actually the code to make use of cache
+    plugins, not the cache plugin itself.  Subclassing it wouldn't yield a usable Cache Plugin and
+    there was no facility to use it as anything else.
+    """
+    def __init__(self, *args, **kwargs):
+        display.deprecated('ansible.plugins.cache.FactCache has been moved to'
+                           ' ansible.vars.fact_cache.FactCache.  If you are looking for the class'
+                           ' to subclass for a cache plugin, you want'
+                           ' ansible.plugins.cache.BaseCacheModule or one of its subclasses.',
+                           version='2.12')
+        super(FactCache, self).__init__(*args, **kwargs)
+
+
 class BaseCacheModule(with_metaclass(ABCMeta, object)):
 
     # Backwards compat only.  Just import the global display instead
@@ -244,65 +262,6 @@ class BaseFileCacheModule(BaseCacheModule):
         pass
 
 
-class FactCache(MutableMapping):
-
-    def __init__(self, *args, **kwargs):
-
-        self._plugin = cache_loader.get(C.CACHE_PLUGIN)
-        if not self._plugin:
-            raise AnsibleError('Unable to load the facts cache plugin (%s).' % (C.CACHE_PLUGIN))
-
-        # Backwards compat: self._display isn't really needed, just import the global display and use that.
-        self._display = display
-
-        # in memory cache so plugins don't expire keys mid run
-        self._cache = {}
-
-    def __getitem__(self, key):
-        if not self._plugin.contains(key):
-            raise KeyError
-        return self._plugin.get(key)
-
-    def __setitem__(self, key, value):
-        self._plugin.set(key, value)
-
-    def __delitem__(self, key):
-        self._plugin.delete(key)
-
-    def __contains__(self, key):
-        return self._plugin.contains(key)
-
-    def __iter__(self):
-        return iter(self._plugin.keys())
-
-    def __len__(self):
-        return len(self._plugin.keys())
-
-    def copy(self):
-        """ Return a primitive copy of the keys and values from the cache. """
-        return dict(self)
-
-    def keys(self):
-        return self._plugin.keys()
-
-    def flush(self):
-        """ Flush the fact cache of all keys. """
-        self._plugin.flush()
-
-    def update(self, host_facts):
-        """ We override the normal update to ensure we always use the 'setter' method """
-        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])
-
-
 class InventoryFileCacheModule(BaseFileCacheModule):
     """
     A caching module backed by file based storage.

lib/ansible/plugins/inventory/constructed.py

@@ -63,10 +63,10 @@ import os
 from ansible import constants as C
 from ansible.errors import AnsibleParserError
 from ansible.inventory.helpers import get_group_vars
-from ansible.plugins.cache import FactCache
 from ansible.plugins.inventory import BaseInventoryPlugin, Constructable
 from ansible.module_utils._text import to_native
 from ansible.utils.vars import combine_vars
+from ansible.vars.fact_cache import FactCache
 
 
 class InventoryModule(BaseInventoryPlugin, Constructable):

lib/ansible/plugins/inventory/generator.py

@@ -73,13 +73,12 @@ EXAMPLES = '''
 
 import os
 
+from itertools import product
+
 from ansible import constants as C
 from ansible.errors import AnsibleParserError
-from ansible.plugins.cache import FactCache
 from ansible.plugins.inventory import BaseInventoryPlugin
 
-from itertools import product
-
 
 class InventoryModule(BaseInventoryPlugin):
     """ constructs groups and vars using Jinja2 template expressions """
@@ -90,8 +89,6 @@ class InventoryModule(BaseInventoryPlugin):
 
         super(InventoryModule, self).__init__()
 
-        self._cache = FactCache()
-
     def verify_file(self, path):
 
         valid = False

lib/ansible/vars/fact_cache.py

@@ -0,0 +1,82 @@
+# Copyright: (c) 2014, Michael DeHaan <michael.dehaan@gmail.com>
+# Copyright: (c) 2018, 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
+
+
+from ansible import constants as C
+from ansible.errors import AnsibleError
+from ansible.module_utils.common._collections_compat import MutableMapping
+from ansible.plugins.loader import cache_loader
+from ansible.utils.display import Display
+
+
+display = Display()
+
+
+class FactCache(MutableMapping):
+
+    def __init__(self, *args, **kwargs):
+
+        self._plugin = cache_loader.get(C.CACHE_PLUGIN)
+        if not self._plugin:
+            raise AnsibleError('Unable to load the facts cache plugin (%s).' % (C.CACHE_PLUGIN))
+
+        super(FactCache, self).__init__(*args, **kwargs)
+
+    def __getitem__(self, key):
+        if not self._plugin.contains(key):
+            raise KeyError
+        return self._plugin.get(key)
+
+    def __setitem__(self, key, value):
+        self._plugin.set(key, value)
+
+    def __delitem__(self, key):
+        self._plugin.delete(key)
+
+    def __contains__(self, key):
+        return self._plugin.contains(key)
+
+    def __iter__(self):
+        return iter(self._plugin.keys())
+
+    def __len__(self):
+        return len(self._plugin.keys())
+
+    def copy(self):
+        """ Return a primitive copy of the keys and values from the cache. """
+        return dict(self)
+
+    def keys(self):
+        return self._plugin.keys()
+
+    def flush(self):
+        """ Flush the fact cache of all keys. """
+        self._plugin.flush()
+
+    def update(self, *args):
+        """ We override the normal update to ensure we always use the 'setter' method """
+        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]}
+        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])

lib/ansible/vars/manager.py

@@ -36,10 +36,10 @@ from ansible.errors import AnsibleError, AnsibleParserError, AnsibleUndefinedVar
 from ansible.inventory.host import Host
 from ansible.inventory.helpers import sort_groups, get_group_vars
 from ansible.module_utils._text import to_native
-from ansible.module_utils.common._collections_compat import MutableMapping, Sequence
+from ansible.module_utils.common._collections_compat import Mapping, MutableMapping, Sequence
 from ansible.module_utils.six import iteritems, text_type, string_types
 from ansible.plugins.loader import lookup_loader, vars_loader
-from ansible.plugins.cache import FactCache
+from ansible.vars.fact_cache import FactCache
 from ansible.template import Templar
 from ansible.utils.display import Display
 from ansible.utils.listify import listify_lookup_plugin_terms
@@ -133,8 +133,8 @@ class VariableManager:
     @extra_vars.setter
     def extra_vars(self, value):
         ''' ensures a clean copy of the extra_vars are used to set the value '''
-        if not isinstance(value, MutableMapping):
+        if not isinstance(value, Mapping):
-            raise AnsibleAssertionError("the type of 'value' for extra_vars should be a MutableMapping, but is a %s" % type(value))
+            raise AnsibleAssertionError("the type of 'value' for extra_vars should be a Mapping, but is a %s" % type(value))
         self._extra_vars = value.copy()
 
     def set_inventory(self, inventory):
@@ -633,13 +633,7 @@ class VariableManager:
             raise AnsibleAssertionError("the type of 'facts' to set for host_facts should be a dict but is a %s" % type(facts))
 
         try:
-            try:
+            self._fact_cache.update({host.name: facts})
-                # this is a cache plugin, not a dictionary
-                self._fact_cache.update({host.name: facts})
-            except TypeError:
-                # this is here for backwards compatibilty for the time cache plugins were not 'dict compatible'
-                self._fact_cache.update(host.name, facts)
-                display.deprecated("Your configured fact cache plugin is using a deprecated form of the 'update' method", version="2.12")
         except KeyError:
             self._fact_cache[host.name] = facts
 
@@ -648,8 +642,8 @@ class VariableManager:
         Sets or updates the given facts for a host in the fact cache.
         '''
 
-        if not isinstance(facts, dict):
+        if not isinstance(facts, Mapping):
-            raise AnsibleAssertionError("the type of 'facts' to set for nonpersistent_facts should be a dict but is a %s" % type(facts))
+            raise AnsibleAssertionError("the type of 'facts' to set for nonpersistent_facts should be a Mapping but is a %s" % type(facts))
 
         try:
             self._nonpersistent_fact_cache[host.name].update(facts)