Handle unicode at the caching layer more elegantly

This patchset resolves an issue where in some cases unicode would
cause the cache key generator to raise a UnicodeEncodeError due to
the name/value being outside of the standard ascii character set.
Included is a fix to the cache backend debug code to utilize repr
for passing the keys/values to the logger.

Tests in test_backend provided by chenxiao <chenxiao@cn.ibm.com>

Closes-bug: 1237892
Change-Id: Ic99503987851128cd41d83ad1ea50dc4a132fbd3

keystone/common/cache/core.py

@@ -28,10 +28,8 @@
 
 CONF = config.CONF
 LOG = log.getLogger(__name__)
-REGION = dogpile.cache.make_region()
 
 make_region = dogpile.cache.make_region
-on_arguments = REGION.cache_on_arguments
 
 dogpile.cache.register_backend(
     'keystone.common.cache.noop',
@@ -41,33 +39,39 @@
 
 class DebugProxy(proxy.ProxyBackend):
     """Extra Logging ProxyBackend."""
+    # NOTE(morganfainberg): Pass all key/values through repr to ensure we have
+    # a clean description of the information.  Without use of repr, it might
+    # be possible to run into encode/decode error(s). For logging/debugging
+    # purposes encode/decode is irrelevant and we should be looking at the
+    # data exactly as it stands.
+
     def get(self, key):
         value = self.proxied.get(key)
         msg = _('CACHE_GET: Key: "%(key)s" Value: "%(value)s"')
-        LOG.debug(msg % {'key': key, 'value': value})
+        LOG.debug(msg % {'key': repr(key), 'value': repr(value)})
         return value
 
     def get_multi(self, keys):
         values = self.proxied.get_multi(keys)
         msg = _('CACHE_GET_MULTI: "%(keys)s" Values: "%(values)s"')
-        LOG.debug(msg % {'keys': keys, 'values': values})
+        LOG.debug(msg % {'keys': repr(keys), 'values': repr(values)})
         return values
 
     def set(self, key, value):
         msg = _('CACHE_SET: Key: "%(key)s" Value: "%(value)s"')
-        LOG.debug(msg % {'key': key, 'value': value})
+        LOG.debug(msg % {'key': repr(key), 'value': repr(value)})
         return self.proxied.set(key, value)
 
     def set_multi(self, keys):
-        LOG.debug(_('CACHE_SET_MULTI: "%s"') % keys)
+        LOG.debug(_('CACHE_SET_MULTI: "%s"') % repr(keys))
         self.proxied.set_multi(keys)
 
     def delete(self, key):
         self.proxied.delete(key)
-        LOG.debug(_('CACHE_DELETE: "%s"') % key)
+        LOG.debug(_('CACHE_DELETE: "%s"') % repr(key))
 
     def delete_multi(self, keys):
-        LOG.debug(_('CACHE_DELETE_MULTI: "%s"') % keys)
+        LOG.debug(_('CACHE_DELETE_MULTI: "%s"') % repr(keys))
         self.proxied.delete_multi(keys)
 
 
@@ -176,3 +180,23 @@ def should_cache(value):
         conf_group = getattr(CONF, section)
         return getattr(conf_group, 'caching', True)
     return should_cache
+
+
+def key_generate_to_str(s):
+    # NOTE(morganfainberg): Since we need to stringify all arguments, attempt
+    # to stringify and handle the Unicode error explicitly as needed.
+    try:
+        return str(s)
+    except UnicodeEncodeError:
+        return s.encode('utf-8')
+
+
+def function_key_generator(namespace, fn, to_str=key_generate_to_str):
+    # NOTE(morganfainberg): This wraps dogpile.cache's default
+    # function_key_generator to change the default to_str mechanism.
+    return util.function_key_generator(namespace, fn, to_str=to_str)
+
+
+REGION = dogpile.cache.make_region(
+    function_key_generator=function_key_generator)
+on_arguments = REGION.cache_on_arguments

keystone/tests/test_backend.py

@@ -1607,6 +1607,16 @@ def test_delete_role_404(self):
                           self.identity_api.delete_role,
                           uuid.uuid4().hex)
 
+    def test_create_update_delete_unicode_project(self):
+        unicode_project_name = u'name \u540d\u5b57'
+        project = {'id': uuid.uuid4().hex,
+                   'name': unicode_project_name,
+                   'description': uuid.uuid4().hex,
+                   'domain_id': CONF.identity.default_domain_id}
+        self.assignment_api.create_project(project['id'], project)
+        self.assignment_api.update_project(project['id'], project)
+        self.assignment_api.delete_project(project['id'])
+
     def test_create_project_case_sensitivity(self):
         # create a ref with a lowercase name
         ref = {

keystone/tests/test_keystoneclient.py

@@ -297,6 +297,62 @@ def test_tenant_create_update_and_delete(self):
         self.assertFalse([t for t in client.tenants.list()
                          if t.id == tenant.id])
 
+    def test_tenant_create_update_and_delete_unicode(self):
+        from keystoneclient import exceptions as client_exceptions
+
+        tenant_name = u'original \u540d\u5b57'
+        tenant_description = 'My original tenant!'
+        tenant_enabled = True
+        client = self.get_client(admin=True)
+
+        # create, get, and list a tenant
+        tenant = client.tenants.create(tenant_name,
+                                       description=tenant_description,
+                                       enabled=tenant_enabled)
+        self.assertEqual(tenant.name, tenant_name)
+        self.assertEqual(tenant.description, tenant_description)
+        self.assertIs(tenant.enabled, tenant_enabled)
+
+        tenant = client.tenants.get(tenant.id)
+        self.assertEqual(tenant.name, tenant_name)
+        self.assertEqual(tenant.description, tenant_description)
+        self.assertIs(tenant.enabled, tenant_enabled)
+
+        # multiple tenants exist due to fixtures, so find the one we're testing
+        tenant = [t for t in client.tenants.list() if t.id == tenant.id].pop()
+        self.assertEqual(tenant.name, tenant_name)
+        self.assertEqual(tenant.description, tenant_description)
+        self.assertIs(tenant.enabled, tenant_enabled)
+
+        # update, get, and list a tenant
+        tenant_name = u'updated \u540d\u5b57'
+        tenant_description = 'Updated tenant!'
+        tenant_enabled = False
+        tenant = client.tenants.update(tenant.id,
+                                       tenant_name=tenant_name,
+                                       enabled=tenant_enabled,
+                                       description=tenant_description)
+        self.assertEqual(tenant.name, tenant_name)
+        self.assertEqual(tenant.description, tenant_description)
+        self.assertIs(tenant.enabled, tenant_enabled)
+
+        tenant = client.tenants.get(tenant.id)
+        self.assertEqual(tenant.name, tenant_name)
+        self.assertEqual(tenant.description, tenant_description)
+        self.assertIs(tenant.enabled, tenant_enabled)
+
+        tenant = [t for t in client.tenants.list() if t.id == tenant.id].pop()
+        self.assertEqual(tenant.name, tenant_name)
+        self.assertEqual(tenant.description, tenant_description)
+        self.assertIs(tenant.enabled, tenant_enabled)
+
+        # delete, get, and list a tenant
+        client.tenants.delete(tenant.id)
+        self.assertRaises(client_exceptions.NotFound, client.tenants.get,
+                          tenant.id)
+        self.assertFalse([t for t in client.tenants.list()
+                         if t.id == tenant.id])
+
     def test_tenant_create_no_name(self):
         from keystoneclient import exceptions as client_exceptions
         client = self.get_client(admin=True)