From 45251f910cae4d973e6abc754419954afcd64114 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Thu, 16 Feb 2017 14:59:14 -0800 Subject: [PATCH] Make BaseFileCache into an abstractbaseclass so it's a proper interface Push the opening and closing of files into the _load and _dump methods so that we don't invoke the slow codec machinery without reason. --- lib/ansible/plugins/cache/base.py | 69 ++++++++++++++++----------- lib/ansible/plugins/cache/jsonfile.py | 14 ++++-- lib/ansible/plugins/cache/pickle.py | 16 +++---- lib/ansible/plugins/cache/yaml.py | 14 ++++-- test/sanity/pep8/legacy-files.txt | 1 - 5 files changed, 66 insertions(+), 48 deletions(-) diff --git a/lib/ansible/plugins/cache/base.py b/lib/ansible/plugins/cache/base.py index 47988a958fe..2847dbf2de0 100644 --- a/lib/ansible/plugins/cache/base.py +++ b/lib/ansible/plugins/cache/base.py @@ -21,8 +21,6 @@ __metaclass__ = type import os import time import errno -import codecs - from abc import ABCMeta, abstractmethod from ansible import constants as C @@ -74,12 +72,9 @@ class BaseFileCacheModule(BaseCacheModule): """ A caching module backed by file based storage. """ - plugin_name = None - read_mode = 'r' - write_mode = 'w' - encoding = 'utf-8' def __init__(self, *args, **kwargs): + self.plugin_name = self.__module__.split('.')[-1] self._timeout = float(C.CACHE_PLUGIN_TIMEOUT) self._cache = {} self._cache_dir = None @@ -89,7 +84,8 @@ class BaseFileCacheModule(BaseCacheModule): self._cache_dir = os.path.expanduser(os.path.expandvars(C.CACHE_PLUGIN_CONNECTION)) if not self._cache_dir: - raise AnsibleError("error, '%s' cache plugin requires the 'fact_caching_connection' config option to be set (to a writeable directory path)" % self.plugin_name) + raise AnsibleError("error, '%s' cache plugin requires the 'fact_caching_connection' config option" + " to be set (to a writeable directory path)" % self.plugin_name) if not os.path.exists(self._cache_dir): try: @@ -111,15 +107,16 @@ class BaseFileCacheModule(BaseCacheModule): cachefile = "%s/%s" % (self._cache_dir, key) try: - with codecs.open(cachefile, self.read_mode, encoding=self.encoding) as f: - try: - value = self._load(f) - self._cache[key] = value - return value - except ValueError as e: - display.warning("error in '%s' cache plugin while trying to read %s : %s. Most likely a corrupt file, so erasing and failing." % (self.plugin_name, cachefile, to_bytes(e))) - self.delete(key) - raise AnsibleError("The cache file %s was corrupt, or did not otherwise contain valid data. It has been removed, so you can re-run your command now." % cachefile) + try: + value = self._load(cachefile) + self._cache[key] = value + return value + except ValueError as e: + display.warning("error in '%s' cache plugin while trying to read %s : %s." + " Most likely a corrupt file, so erasing and failing." % (self.plugin_name, cachefile, to_bytes(e))) + self.delete(key) + raise AnsibleError("The cache file %s was corrupt, or did not otherwise contain valid data." + " It has been removed, so you can re-run your command now." % cachefile) except (OSError,IOError) as e: display.warning("error in '%s' cache plugin while trying to read %s : %s" % (self.plugin_name, cachefile, to_bytes(e))) raise KeyError @@ -132,17 +129,9 @@ class BaseFileCacheModule(BaseCacheModule): cachefile = "%s/%s" % (self._cache_dir, key) try: - f = codecs.open(cachefile, self.write_mode, encoding=self.encoding) + self._dump(value, cachefile) except (OSError,IOError) as e: display.warning("error in '%s' cache plugin while trying to write to %s : %s" % (self.plugin_name, cachefile, to_bytes(e))) - pass - else: - self._dump(value, f) - finally: - try: - f.close() - except UnboundLocalError: - pass def has_expired(self, key): @@ -212,9 +201,31 @@ class BaseFileCacheModule(BaseCacheModule): ret[key] = self.get(key) return ret - def _load(self, f): - raise AnsibleError("Plugin '%s' must implement _load method" % self.plugin_name) + @abstractmethod + def _load(self, filepath): + """ + Read data from a filepath and return it as a value + + :arg filepath: The filepath to read from. + :returns: The value stored in the filepath + + This method reads from the file on disk and takes care of any parsing + and transformation of the data before returning it. The value + returned should be what Ansible would expect if it were uncached data. + + .. note:: Filehandles have advantages but calling code doesn't know + whether this file is text or binary, should be decoded, or accessed via + a library function. Therefore the API uses a filepath and opens + the file inside of the method. + """ + pass - def _dump(self, value, f): - raise AnsibleError("Plugin '%s' must implement _dump method" % self.plugin_name) + @abstractmethod + def _dump(self, value, filepath): + """ + Write data to a filepath + :arg value: The value to store + :arg filepath: The filepath to store it at + """ + pass diff --git a/lib/ansible/plugins/cache/jsonfile.py b/lib/ansible/plugins/cache/jsonfile.py index 9f10dfd1f2b..eb8c2afbd3b 100644 --- a/lib/ansible/plugins/cache/jsonfile.py +++ b/lib/ansible/plugins/cache/jsonfile.py @@ -19,6 +19,8 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +import codecs + try: import simplejson as json except ImportError: @@ -31,10 +33,12 @@ class CacheModule(BaseFileCacheModule): """ A caching module backed by json files. """ - plugin_name = 'jsonfile' - def _load(self, f): - return json.load(f) + def _load(self, filepath): + # Valid JSON is always UTF-8 encoded. + with codecs.open(filepath, 'r', encoding='utf-8') as f: + return json.load(f) - def _dump(self, value, f): - f.write(jsonify(value, format=True)) + def _dump(self, value, filepath): + with codecs.open(filepath, 'w', encoding='utf-8') as f: + f.write(jsonify(value, format=True)) diff --git a/lib/ansible/plugins/cache/pickle.py b/lib/ansible/plugins/cache/pickle.py index 060c0381ecb..bea44248ea7 100644 --- a/lib/ansible/plugins/cache/pickle.py +++ b/lib/ansible/plugins/cache/pickle.py @@ -30,13 +30,13 @@ class CacheModule(BaseFileCacheModule): """ A caching module backed by pickle files. """ - plugin_name = 'pickle' - read_mode = 'rb' - write_mode = 'wb' - encoding = None - def _load(self, f): - return pickle.load(f) + def _load(self, filepath): + # Pickle is a binary format + with open(filepath, 'rb') as f: + return pickle.load(f) - def _dump(self, value, f): - pickle.dump(value, f) + def _dump(self, value, filepath): + with open(filepath, 'wb') as f: + # Use pickle protocol 2 which is compatible with Python 2.3+. + pickle.dump(value, f, protocol=2) diff --git a/lib/ansible/plugins/cache/yaml.py b/lib/ansible/plugins/cache/yaml.py index ecfb62fe698..a00eb8482d8 100644 --- a/lib/ansible/plugins/cache/yaml.py +++ b/lib/ansible/plugins/cache/yaml.py @@ -19,6 +19,9 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type + +import codecs + import yaml from ansible.parsing.yaml.loader import AnsibleLoader @@ -29,10 +32,11 @@ class CacheModule(BaseFileCacheModule): """ A caching module backed by yaml files. """ - plugin_name = 'yaml' - def _load(self, f): - return AnsibleLoader(f).get_single_data() + def _load(self, filepath): + with codecs.open(filepath, 'r', encoding='utf-8') as f: + return AnsibleLoader(f).get_single_data() - def _dump(self, value, f): - yaml.dump(value, f, Dumper=AnsibleDumper, default_flow_style=False) + def _dump(self, value, filepath): + with codecs.open(filepath, 'w', encoding='utf-8') as f: + yaml.dump(value, f, Dumper=AnsibleDumper, default_flow_style=False) diff --git a/test/sanity/pep8/legacy-files.txt b/test/sanity/pep8/legacy-files.txt index 12c2f9a01c6..4eb2a21df26 100644 --- a/test/sanity/pep8/legacy-files.txt +++ b/test/sanity/pep8/legacy-files.txt @@ -243,7 +243,6 @@ lib/ansible/playbook/role/metadata.py lib/ansible/plugins/action/set_fact.py lib/ansible/plugins/action/set_stats.py lib/ansible/plugins/action/synchronize.py -lib/ansible/plugins/cache/base.py lib/ansible/plugins/callback/default.py lib/ansible/plugins/callback/logentries.py lib/ansible/plugins/callback/oneline.py