Merge lp:~james-page/charm-helpers/caching_hookenv into lp:charm-helpers

[James Page]

Various refactoring + additions to hookenv

1) cached decorator

Allows caching of function + args to avoid repeated calls out the cli
to juju commands.

2) Normalizing missing unit/config/relation data to None

relation_get('missing') == None
config('missing') == None
unit_get('missing') == None

3) Normalized use of the Serializable object a bit

And added a unit test to cover missing attribute lookups

Question: maybe this should actually return 'None' rather than throwing
an exception inline with 2).

4) Tweaks some tests to be closer to juju behaviour with json

5) Tidied misc pep8/pylint warnings.

[lp:~james-page/charm-helpers/caching_hookenv]

29. By James Page on 2013-06-13

Also cache config helper

30. By James Page on 2013-06-13

Flush key for local_unit on relation_set

[Matthew Wedgwood (mew)]

Good stuff.

I have very slight reservations about the way that flush works because it's very imprecise about the keys it removes. It doesn't matter now, but it could be confusing if someone were to extend the module in the future. It's also arguable whether it would really matter if the cache was cleared unexpectedly.

+1

review: Approve

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2013-05-31 10:27:31 +0000
+++ charmhelpers/core/hookenv.py	2013-06-13 19:27:26 +0000
@@ -17,6 +17,44 @@
 DEBUG = "DEBUG"
 MARKER = object()
 
+cache = {}
+
+
+def cached(func):
+    ''' Cache return values for multiple executions of func + args
+
+    For example:
+
+        @cached
+        def unit_get(attribute):
+            pass
+
+        unit_get('test')
+
+    will cache the result of unit_get + 'test' for future calls.
+    '''
+    def wrapper(*args, **kwargs):
+        global cache
+        key = str((func, args, kwargs))
+        try:
+            return cache[key]
+        except KeyError:
+            res = func(*args, **kwargs)
+            cache[key] = res
+            return res
+    return wrapper
+
+
+def flush(key):
+    ''' Flushes any entries from function cache where the
+    key is found in the function+args '''
+    flush_list = []
+    for item in cache:
+        if key in item:
+            flush_list.append(item)
+    for item in flush_list:
+        del cache[item]
+
 
 def log(message, level=None):
     "Write a message to the juju log"
@@ -96,6 +134,7 @@
     return os.environ['JUJU_REMOTE_UNIT']
 
 
+@cached
 def config(scope=None):
     "Juju charm configuration"
     config_cmd_line = ['config-get']
@@ -103,13 +142,14 @@
         config_cmd_line.append(scope)
     config_cmd_line.append('--format=json')
     try:
-        config_data = json.loads(subprocess.check_output(config_cmd_line))
-    except (ValueError, OSError, subprocess.CalledProcessError) as err:
-        log(str(err), level=ERROR)
-        raise
-    return Serializable(config_data)
-
-
+        return Serializable(json.loads(
+                                subprocess.check_output(config_cmd_line)
+                                ))
+    except ValueError:
+        return None
+
+
+@cached
 def relation_get(attribute=None, unit=None, rid=None):
     _args = ['relation-get', '--format=json']
     if rid:
@@ -119,7 +159,7 @@
     if unit:
         _args.append(unit)
     try:
-        return json.loads(subprocess.check_output(_args))
+        return Serializable(json.loads(subprocess.check_output(_args)))
     except ValueError:
         return None
 
@@ -133,8 +173,11 @@
     for k, v in kwargs.items():
         relation_cmd_line.append('{}={}'.format(k, v))
     subprocess.check_call(relation_cmd_line)
-
-
+    # Flush cache of any relation-gets for local unit
+    flush(local_unit())
+
+
+@cached
 def relation_ids(reltype=None):
     "A list of relation_ids"
     reltype = reltype or relation_type()
@@ -145,6 +188,7 @@
     return []
 
 
+@cached
 def related_units(relid=None):
     "A list of related units"
     relid = relid or relation_id()
@@ -154,6 +198,7 @@
     return json.loads(subprocess.check_output(units_cmd_line))
 
 
+@cached
 def relation_for_unit(unit=None, rid=None):
     "Get the json represenation of a unit's relation"
     unit = unit or remote_unit()
@@ -165,6 +210,7 @@
     return Serializable(relation)
 
 
+@cached
 def relations_for_id(relid=None):
     "Get relations of a specific relation ID"
     relation_data = []
@@ -176,6 +222,7 @@
     return relation_data
 
 
+@cached
 def relations_of_type(reltype=None):
     "Get relations of a specific type"
     relation_data = []
@@ -187,13 +234,14 @@
     return relation_data
 
 
+@cached
 def relation_types():
     "Get a list of relation types supported by this charm"
     charmdir = os.environ.get('CHARM_DIR', '')
     mdf = open(os.path.join(charmdir, 'metadata.yaml'))
     md = yaml.safe_load(mdf)
     rel_types = []
-    for key in ('provides','requires','peers'):
+    for key in ('provides', 'requires', 'peers'):
         section = md.get(key)
         if section:
             rel_types.extend(section.keys())
@@ -201,6 +249,7 @@
     return rel_types
 
 
+@cached
 def relations():
     rels = {}
     for reltype in relation_types():
@@ -229,9 +278,13 @@
     subprocess.check_call(_args)
 
 
+@cached
 def unit_get(attribute):
-    _args = ['unit-get', attribute]
-    return subprocess.check_output(_args).strip()
+    _args = ['unit-get', '--format=json', attribute]
+    try:
+        return json.loads(subprocess.check_output(_args))
+    except ValueError:
+        return None
 
 
 def unit_private_ip():

=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2013-05-31 10:27:31 +0000
+++ tests/core/test_hookenv.py	2013-06-13 19:27:26 +0000
@@ -22,6 +22,7 @@
         interface: mock
 """
 
+
 class SerializableTest(TestCase):
     def test_serializes_object_to_json(self):
         foo = {
@@ -45,6 +46,14 @@
 
         self.assertEqual(wrapped.bar, 'baz')
 
+    def test_raises_error_from_inner_object_as_dict(self):
+        foo = {
+            'bar': 'baz',
+        }
+        wrapped = hookenv.Serializable(foo)
+
+        self.assertRaises(AttributeError, getattr, wrapped, 'baz')
+
     def test_dict_methods_from_inner_object(self):
         foo = {
             'bar': 'baz',
@@ -80,6 +89,11 @@
 
 
 class HelpersTest(TestCase):
+    def setUp(self):
+        super(HelpersTest, self).setUp()
+        # Reset hookenv cache for each test
+        hookenv.cache = {}
+
     @patch('subprocess.call')
     def test_logs_messages_to_juju_with_default_level(self, mock_call):
         hookenv.log('foo')
@@ -111,21 +125,22 @@
 
     @patch('subprocess.check_output')
     def test_gets_charm_config_with_scope(self, check_output):
-        config_data = {'foo': 'bar'}
+        config_data = 'bar'
         check_output.return_value = json.dumps(config_data)
 
         result = hookenv.config(scope='baz')
 
-        self.assertEqual(result.foo, 'bar')
+        self.assertEqual(result, 'bar')
         check_output.assert_called_with(['config-get', 'baz', '--format=json'])
 
     @patch('subprocess.check_output')
-    @patch('charmhelpers.core.hookenv.log')
-    def test_logs_and_reraises_on_config_error(self, log, check_output):
-        error = 'some error'
-        check_output.side_effect = ValueError(error)
-
-        self.assertRaisesRegexp(ValueError, error, hookenv.config)
+    def test_gets_missing_charm_config_with_scope(self, check_output):
+        check_output.return_value = ''
+
+        result = hookenv.config(scope='baz')
+
+        self.assertEqual(result, None)
+        check_output.assert_called_with(['config-get', 'baz', '--format=json'])
 
     @patch('charmhelpers.core.hookenv.os')
     def test_gets_the_local_unit(self, os_):
@@ -392,9 +407,9 @@
     @patch('charmhelpers.core.hookenv.relation_get')
     def test_gets_relations(self, relation_get, related_units,
                             relation_ids, relation_types):
-        relation_types.return_value = ['t1','t2']
+        relation_types.return_value = ['t1', 't2']
         relation_ids.return_value = ['i1']
-        related_units.return_value = ['u1','u2']
+        related_units.return_value = ['u1', 'u2']
         relation_get.return_value = {'key': 'val'}
 
         result = hookenv.relations()
@@ -414,7 +429,6 @@
             },
        })
 
-
     @patch('charmhelpers.core.hookenv.config')
     @patch('charmhelpers.core.hookenv.relation_type')
     @patch('charmhelpers.core.hookenv.local_unit')
@@ -460,59 +474,77 @@
 
     @patch('subprocess.check_output')
     def test_gets_relation(self, check_output):
-        json_string = '{"foo": "BAR"}'
-        check_output.return_value = json_string
-
+        data = {"foo": "BAR"}
+        check_output.return_value = json.dumps(data)
         result = hookenv.relation_get()
 
-        self.assertEqual(result['foo'], 'BAR')
+        self.assertEqual(result.foo, 'BAR')
         check_output.assert_called_with(['relation-get', '--format=json', '-'])
 
     @patch('subprocess.check_output')
     def test_gets_relation_with_scope(self, check_output):
-        json_string = '{"foo": "BAR"}'
-        check_output.return_value = json_string
-
-        result = hookenv.relation_get(attribute='baz-scope')
-
-        self.assertEqual(result['foo'], 'BAR')
+        check_output.return_value = json.dumps('bar')
+
+        result = hookenv.relation_get(attribute='baz-scope')
+
+        self.assertEqual(result, 'bar')
+        check_output.assert_called_with(['relation-get', '--format=json',
+                                         'baz-scope'])
+
+    @patch('subprocess.check_output')
+    def test_gets_missing_relation_with_scope(self, check_output):
+        check_output.return_value = ""
+
+        result = hookenv.relation_get(attribute='baz-scope')
+
+        self.assertEqual(result, None)
         check_output.assert_called_with(['relation-get', '--format=json',
                                          'baz-scope'])
 
     @patch('subprocess.check_output')
     def test_gets_relation_with_unit_name(self, check_output):
-        json_string = '{"foo": "BAR"}'
-        check_output.return_value = json_string
+        check_output.return_value = json.dumps('BAR')
 
         result = hookenv.relation_get(attribute='baz-scope', unit='baz-unit')
 
-        self.assertEqual(result['foo'], 'BAR')
+        self.assertEqual(result, 'BAR')
         check_output.assert_called_with(['relation-get', '--format=json',
                                          'baz-scope', 'baz-unit'])
 
+    @patch('charmhelpers.core.hookenv.local_unit')
+    @patch('subprocess.check_call')
+    @patch('subprocess.check_output')
+    def test_relation_set_flushes_local_unit_cache(self, check_output,
+                                                   check_call, local_unit):
+        check_output.return_value = json.dumps('BAR')
+        local_unit.return_value = 'baz_unit'
+        hookenv.relation_get(attribute='baz_scope', unit='baz_unit')
+        hookenv.relation_get(attribute='bar_scope')
+        self.assertTrue(len(hookenv.cache) == 2)
+        hookenv.relation_set(baz_scope='hello')
+        # relation_set should flush any entries for local_unit
+        self.assertTrue(len(hookenv.cache) == 1)
+
     @patch('subprocess.check_output')
     def test_gets_relation_with_relation_id(self, check_output):
-        json_string = '{"foo": "BAR"}'
-        check_output.return_value = json_string
+        check_output.return_value = json.dumps('BAR')
 
         result = hookenv.relation_get(attribute='baz-scope', unit='baz-unit',
                                       rid=123)
 
-        self.assertEqual(result['foo'], 'BAR')
+        self.assertEqual(result, 'BAR')
         check_output.assert_called_with(['relation-get', '--format=json', '-r',
                                          123, 'baz-scope', 'baz-unit'])
 
     @patch('subprocess.check_call')
     def test_sets_relation_with_kwargs(self, check_call_):
         hookenv.relation_set(foo="bar")
-        check_call_.assert_called_with(['relation-set','foo=bar'])
-
+        check_call_.assert_called_with(['relation-set', 'foo=bar'])
 
     @patch('subprocess.check_call')
     def test_sets_relation_with_dict(self, check_call_):
-        hookenv.relation_set(relation_settings={"foo":"bar"})
-        check_call_.assert_called_with(['relation-set','foo=bar'])
-
+        hookenv.relation_set(relation_settings={"foo": "bar"})
+        check_call_.assert_called_with(['relation-set', 'foo=bar'])
 
     @patch('subprocess.check_call')
     def test_sets_relation_with_relation_id(self, check_call_):
@@ -520,7 +552,6 @@
         check_call_.assert_called_with(['relation-set', '-r', 'foo',
                                          'bar=baz'])
 
-
     def test_lists_relation_types(self):
         open_ = mock_open()
         open_.return_value = StringIO(CHARM_METADATA)
@@ -528,36 +559,61 @@
             with patch.dict('os.environ', {'CHARM_DIR': '/var/empty'}):
                 reltypes = set(hookenv.relation_types())
         open_.assert_called_once_with('/var/empty/metadata.yaml')
-        self.assertEqual(set(('testreqs','testprov','testpeer')), reltypes)
-
+        self.assertEqual(set(('testreqs', 'testprov', 'testpeer')), reltypes)
 
     @patch('subprocess.check_call')
     def test_opens_port(self, check_call_):
         hookenv.open_port(443, "TCP")
         hookenv.open_port(80)
         hookenv.open_port(100, "UDP")
-        calls = [call(['open-port','443/TCP']),
-                 call(['open-port','80/TCP']),
-                 call(['open-port','100/UDP']),
+        calls = [call(['open-port', '443/TCP']),
+                 call(['open-port', '80/TCP']),
+                 call(['open-port', '100/UDP']),
                 ]
         check_call_.assert_has_calls(calls)
 
-
     @patch('subprocess.check_call')
     def test_closes_port(self, check_call_):
         hookenv.close_port(443, "TCP")
         hookenv.close_port(80)
         hookenv.close_port(100, "UDP")
-        calls = [call(['close-port','443/TCP']),
-                 call(['close-port','80/TCP']),
-                 call(['close-port','100/UDP']),
+        calls = [call(['close-port', '443/TCP']),
+                 call(['close-port', '80/TCP']),
+                 call(['close-port', '100/UDP']),
                 ]
         check_call_.assert_has_calls(calls)
 
     @patch('subprocess.check_output')
     def test_gets_unit_attribute(self, check_output_):
-        hookenv.unit_get('foo')
-        check_output_.assert_called_with(['unit-get', 'foo'])
+        check_output_.return_value = json.dumps('bar')
+        self.assertEqual(hookenv.unit_get('foo'), 'bar')
+        check_output_.assert_called_with(['unit-get', '--format=json', 'foo'])
+
+    @patch('subprocess.check_output')
+    def test_gets_missing_unit_attribute(self, check_output_):
+        check_output_.return_value = ""
+        self.assertEqual(hookenv.unit_get('foo'), None)
+        check_output_.assert_called_with(['unit-get', '--format=json', 'foo'])
+
+    def test_cached_decorator(self):
+        calls = []
+        values = {
+            'hello': 'world',
+            'foo': 'bar',
+            'baz': None
+            }
+
+        @hookenv.cached
+        def cache_function(attribute):
+            calls.append(attribute)
+            return values[attribute]
+
+        self.assertEquals(cache_function('hello'), 'world')
+        self.assertEquals(cache_function('hello'), 'world')
+        self.assertEquals(cache_function('foo'), 'bar')
+        self.assertEquals(cache_function('baz'), None)
+        self.assertEquals(cache_function('baz'), None)
+        self.assertEquals(calls, ['hello', 'foo', 'baz'])
 
 
 class HooksTest(TestCase):
@@ -604,4 +660,3 @@
         self.assertRaises(hookenv.UnregisteredHookError, hooks.execute,
                           ['brew'])
         self.assertEqual(execs, [True])
-