From a0ae37eb3ea96a394f10d3f38e8b454d2d7a98e0 Mon Sep 17 00:00:00 2001 From: Morgan Fainberg Date: Sun, 13 Oct 2013 18:34:24 -0700 Subject: [PATCH] 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 Closes-bug: 1237892 Change-Id: Ic99503987851128cd41d83ad1ea50dc4a132fbd3 --- keystone/common/cache/core.py | 40 +++++++++++++++---- keystone/tests/test_backend.py | 10 +++++ keystone/tests/test_keystoneclient.py | 56 +++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 8 deletions(-) diff --git a/keystone/common/cache/core.py b/keystone/common/cache/core.py index fe4a54a678..0a8da70710 100644 --- a/keystone/common/cache/core.py +++ b/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 diff --git a/keystone/tests/test_backend.py b/keystone/tests/test_backend.py index 22d9d8d690..7dd34770aa 100644 --- a/keystone/tests/test_backend.py +++ b/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 = { diff --git a/keystone/tests/test_keystoneclient.py b/keystone/tests/test_keystoneclient.py index 19206a7dd2..5aa74a7816 100644 --- a/keystone/tests/test_keystoneclient.py +++ b/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)