Merge ~jugmac00/launchpad:fix-pymemcache-regression-bytes-vs-str into launchpad:master

[Jürgen Gmach]

Fix pymemcache regression in SnapStoreClient.listChannels

[Jürgen Gmach (jugmac00)]

For the reviewer:
- I had to remove one assertion: self.assertEqual([], hosting_fixture.getInventory.calls), which I think is ok, as getInventory is the method were the expected exception is raised
- I had to implement the two new methods also in MemcacheFixture - this is a bit dangerous, as duplicated code could be changed in one place and not in the other, and we basically test MemcacheFixture rather than the real code.
- I have not created a dedicated test to test the behavior when the Client accesses bytes instead of the expected str - I could inject bytes in MemcacheFixture._cache, but then again, we test the fixture, not the real code.

[Colin Watson (cjwatson)]

review: Needs Fixing

[Jürgen Gmach (jugmac00)]

Thanks for the review. I think I applied all suggestions, with exception to the super vs direct class name thing, see comment below.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index b915da8..12cfdf4 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -9,7 +9,6 @@ __all__ = [
 
 from datetime import datetime
 from functools import partial
-import json
 import operator
 import os.path
 
@@ -833,15 +832,12 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
             memcache_key = six.ensure_binary(
                 '%s:bzr-file-list:%s:%s:%s' % (
                     instance_name, self.id, revision_id, dirname))
-            cached_file_list = memcache_client.get(memcache_key)
-            if cached_file_list is not None:
-                try:
-                    file_list = json.loads(cached_file_list)
-                except Exception:
-                    logger.exception(
-                        'Cannot load cached file list for %s:%s:%s; deleting' %
-                        (self.unique_name, revision_id, dirname))
-                    memcache_client.delete(memcache_key)
+            description = "file list for %s:%s:%s" % (
+                self.unique_name, revision_id, dirname
+            )
+            file_list = memcache_client.get_json(
+                memcache_key, logger, description, default=unset)
+
         if file_list is unset:
             try:
                 inventory = hosting_client.getInventory(
@@ -854,7 +850,7 @@ class Branch(SQLBase, WebhookTargetMixin, BzrIdentityMixin):
             if enable_memcache:
                 # Cache the file list in case there's a request for another
                 # file in the same directory.
-                memcache_client.set(memcache_key, json.dumps(file_list))
+                memcache_client.set_json(memcache_key, file_list)
         file_id = (file_list or {}).get(os.path.basename(filename))
         if file_id is None:
             raise BranchFileNotFound(
diff --git a/lib/lp/code/model/gitref.py b/lib/lp/code/model/gitref.py
index ca4c05b..8571c4a 100644
--- a/lib/lp/code/model/gitref.py
+++ b/lib/lp/code/model/gitref.py
@@ -9,7 +9,6 @@ __all__ = [
     ]
 
 from functools import partial
-import json
 import re
 
 from lazr.lifecycle.event import ObjectCreatedEvent
@@ -353,23 +352,17 @@ class GitRefMixin:
             if stop is not None:
                 memcache_key += ":stop=%s" % stop
             memcache_key = six.ensure_binary(memcache_key)
-            cached_log = memcache_client.get(memcache_key)
-            if cached_log is not None:
-                try:
-                    log = json.loads(cached_log)
-                except Exception:
-                    if logger is not None:
-                        logger.exception(
-                            "Cannot load cached log information for %s:%s; "
-                            "deleting" % (path, start))
-                    memcache_client.delete(memcache_key)
+
+            description = "log information for %s:%s" % (path, start)
+            log = memcache_client.get_json(memcache_key, logger, description)
+
         if log is None:
             if enable_hosting:
                 hosting_client = getUtility(IGitHostingClient)
                 log = removeSecurityProxy(hosting_client.getLog(
                     path, start, limit=limit, stop=stop, logger=logger))
                 if enable_memcache:
-                    memcache_client.set(memcache_key, json.dumps(log))
+                    memcache_client.set_json(memcache_key, log)
             else:
                 # Fall back to synthesising something reasonable based on
                 # information in our own database.
diff --git a/lib/lp/services/memcache/client.py b/lib/lp/services/memcache/client.py
index e04be16..55b9d03 100644
--- a/lib/lp/services/memcache/client.py
+++ b/lib/lp/services/memcache/client.py
@@ -4,9 +4,11 @@
 """Launchpad Memcache client."""
 
 __all__ = [
+    'MemcacheClient',
     'memcache_client_factory',
     ]
 
+import json
 import re
 
 from pymemcache.client.hash import HashClient
@@ -18,8 +20,48 @@ from pymemcache.serde import (
 from lp.services.config import config
 
 
+class MemcacheClient(HashClient):
+    """memcached client with added JSON handling"""
+
+    def get_json(self, key, logger, description, default=None):
+        """Returns decoded JSON data from a memcache instance for a given key
+
+        In case of a decoding issue, and given a logger and a description, an
+        error message gets logged.
+
+        The `default` value is used when no value could be retrieved, or an
+        error happens.
+
+        :returns: dict or the default value
+        """
+        data = self.get(key)
+        if data is not None:
+            try:
+                rv = json.loads(data)
+            # the exceptions are chosen deliberately in order to gracefully
+            # handle invalid data
+            except (TypeError, ValueError):
+                if logger and description:
+                    logger.exception(
+                        "Cannot load cached %s; deleting" % description
+                    )
+                self.delete(key)
+                rv = default
+        else:
+            rv = default
+        return rv
+
+    def set_json(self, key, value, expire=0):
+        """Saves the given key/value pair, after converting the value.
+
+        `expire` (optional int) is the number of seconds until the item
+            expires from the cache; zero means no expiry.
+        """
+        self.set(key, json.dumps(value), expire)
+
+
 def memcache_client_factory(timeline=True):
-    """Return a pymemcache HashClient for Launchpad."""
+    """Return an extended pymemcache client for Launchpad."""
     # example value for config.memcache.servers:
     # (127.0.0.1:11242,1)
     servers = [
@@ -31,7 +73,7 @@ def memcache_client_factory(timeline=True):
         from lp.services.memcache.timeline import TimelineRecordingClient
         client_factory = TimelineRecordingClient
     else:
-        client_factory = HashClient
+        client_factory = MemcacheClient
     return client_factory(
         servers,
         serializer=python_memcache_serializer,
diff --git a/lib/lp/services/memcache/testing.py b/lib/lp/services/memcache/testing.py
index c4809f4..23ef232 100644
--- a/lib/lp/services/memcache/testing.py
+++ b/lib/lp/services/memcache/testing.py
@@ -9,11 +9,12 @@ import time as _time
 
 import fixtures
 
+from lp.services.memcache.client import MemcacheClient
 from lp.services.memcache.interfaces import IMemcacheClient
 from lp.testing.fixture import ZopeUtilityFixture
 
 
-class MemcacheFixture(fixtures.Fixture):
+class MemcacheFixture(fixtures.Fixture, MemcacheClient):
     """A trivial in-process memcache fixture."""
 
     def __init__(self):
diff --git a/lib/lp/services/memcache/tests/test_memcache_client.py b/lib/lp/services/memcache/tests/test_memcache_client.py
index ef93fff..ee49844 100644
--- a/lib/lp/services/memcache/tests/test_memcache_client.py
+++ b/lib/lp/services/memcache/tests/test_memcache_client.py
@@ -7,8 +7,10 @@ from lazr.restful.utils import get_current_browser_request
 from pymemcache.exceptions import MemcacheIllegalInputError
 from zope.component import getUtility
 
+from lp.services.log.logger import BufferLogger
 from lp.services.memcache.client import memcache_client_factory
 from lp.services.memcache.interfaces import IMemcacheClient
+from lp.services.memcache.testing import MemcacheFixture
 from lp.services.timeline.requesttimeline import get_request_timeline
 from lp.testing import TestCase
 from lp.testing.layers import LaunchpadZopelessLayer
@@ -75,3 +77,21 @@ class MemcacheClientFactoryTestCase(TestCase):
         client.set('foo', 'bar')
         self.assertEqual('bar', client.get('foo'))
         self.assertEqual(base_action_count, len(timeline.actions))
+
+
+class MemcacheClientJSONTestCase(TestCase):
+    def setUp(self):
+        super().setUp()
+        self.client = self.useFixture(MemcacheFixture())
+        self.logger = BufferLogger()
+
+    def test_handle_invalid_data(self):
+        self.client.set("key", b"invalid_data")
+        description = "binary data"
+
+        self.client.get_json("key", self.logger, description)
+
+        self.assertEqual(
+            "ERROR Cannot load cached binary data; deleting\n",
+            self.logger.content.as_text()
+        )
diff --git a/lib/lp/services/memcache/timeline.py b/lib/lp/services/memcache/timeline.py
index 9e1bf3e..3427e73 100644
--- a/lib/lp/services/memcache/timeline.py
+++ b/lib/lp/services/memcache/timeline.py
@@ -10,13 +10,13 @@ __all__ = [
 import logging
 
 from lazr.restful.utils import get_current_browser_request
-from pymemcache.client.hash import HashClient
 
 from lp.services import features
+from lp.services.memcache.client import MemcacheClient
 from lp.services.timeline.requesttimeline import get_request_timeline
 
 
-class TimelineRecordingClient(HashClient):
+class TimelineRecordingClient(MemcacheClient):
 
     def __get_timeline_action(self, suffix, key):
         request = get_current_browser_request()
@@ -36,7 +36,7 @@ class TimelineRecordingClient(HashClient):
             return None
         action = self.__get_timeline_action("get", key)
         try:
-            return HashClient.get(self, key)
+            return super().get(key)
         finally:
             action.finish()
 
@@ -45,7 +45,7 @@ class TimelineRecordingClient(HashClient):
             return None
         action = self.__get_timeline_action("set", key)
         try:
-            success = HashClient.set(self, key, value, expire=expire)
+            success = super().set(key, value, expire=expire)
             if success:
                 logging.debug("Memcache set succeeded for %s", key)
             else:
diff --git a/lib/lp/snappy/model/snapstoreclient.py b/lib/lp/snappy/model/snapstoreclient.py
index c43d705..9532567 100644
--- a/lib/lp/snappy/model/snapstoreclient.py
+++ b/lib/lp/snappy/model/snapstoreclient.py
@@ -390,19 +390,13 @@ class SnapStoreClient:
         """See `ISnapStoreClient`."""
         if config.snappy.store_search_url is None:
             return _default_store_channels
-        channels = None
         memcache_client = getUtility(IMemcacheClient)
         search_host = urlsplit(config.snappy.store_search_url).hostname
         memcache_key = ("%s:channels" % search_host).encode("UTF-8")
-        cached_channels = memcache_client.get(memcache_key)
-        if cached_channels is not None:
-            try:
-                channels = json.loads(cached_channels)
-            except ValueError:
-                log.exception(
-                    "Cannot load cached channels for %s; deleting" %
-                    search_host)
-                memcache_client.delete(memcache_key)
+        description = "channels for %s" % search_host
+
+        channels = memcache_client.get_json(memcache_key, log, description)
+
         if (channels is None and
                 not getFeatureFlag(u"snap.disable_channel_search")):
             path = "api/v1/channels"
@@ -423,8 +417,8 @@ class SnapStoreClient:
             DAY_IN_SECONDS = 60 * 60 * 24
             # pymemcache's `expire` expects an int
             expire_time = int(time.time()) + DAY_IN_SECONDS
-            memcache_client.set(
-                memcache_key, json.dumps(channels), expire_time)
+            memcache_client.set_json(
+                memcache_key, channels, expire_time)
         if channels is None:
             channels = _default_store_channels
         return channels