From e1a5c80196b4bccabdf43b441d6449b66b1a9d0e Mon Sep 17 00:00:00 2001 From: Ricky Rosario Date: Tue, 30 Aug 2011 11:52:45 -0400 Subject: [PATCH] [bug 679533] redis_client() now raises exception if name passed isn't defined or is unable to connect to the instance. --- apps/dashboards/cron.py | 2 +- apps/dashboards/readouts.py | 19 ++++++----- apps/dashboards/tests/test_cron.py | 8 ++--- apps/dashboards/tests/test_views.py | 14 ++------- apps/dashboards/views.py | 13 +++++--- apps/kadmin/admin.py | 2 +- apps/karma/admin.py | 5 +-- apps/karma/manager.py | 26 ++++++++------- apps/karma/tasks.py | 3 +- apps/karma/tests/test_actions.py | 4 +-- apps/sumo/decorators.py | 13 ++++++++ apps/sumo/redis_utils.py | 49 +++++++++++++++++++++++++++++ apps/sumo/utils.py | 37 ---------------------- apps/sumo/views.py | 4 +-- 14 files changed, 114 insertions(+), 85 deletions(-) create mode 100644 apps/sumo/redis_utils.py diff --git a/apps/dashboards/cron.py b/apps/dashboards/cron.py index 6c7dcd8c1ad..bd1e9fb66ce 100644 --- a/apps/dashboards/cron.py +++ b/apps/dashboards/cron.py @@ -4,7 +4,7 @@ import cronjobs from dashboards.models import PERIODS, WikiDocumentVisits -from sumo.utils import redis_client +from sumo.redis_utils import redis_client from wiki.models import Document diff --git a/apps/dashboards/readouts.py b/apps/dashboards/readouts.py index af446ead9a8..fc36094cb03 100644 --- a/apps/dashboards/readouts.py +++ b/apps/dashboards/readouts.py @@ -5,22 +5,26 @@ is_ready_for_localization=False do not exist. """ +import logging + from django.conf import settings from django.db import connections, router from django.utils.datastructures import SortedDict import jingo from jinja2 import Markup -from redis.exceptions import ConnectionError from tower import ugettext as _, ugettext_lazy as _lazy from dashboards import THIS_WEEK, ALL_TIME, PERIODS from sumo.urlresolvers import reverse -from sumo.utils import redis_client +from sumo.redis_utils import redis_client, RedisError from wiki.models import (Document, MEDIUM_SIGNIFICANCE, MAJOR_SIGNIFICANCE, TYPO_SIGNIFICANCE) +log = logging.getLogger('k.dashboards.readouts') + + MOST_VIEWED = 1 MOST_RECENT = 2 @@ -601,21 +605,22 @@ class UnhelpfulReadout(Readout): modes = [] # This class is a namespace and doesn't get instantiated. + key = settings.HELPFULVOTES_UNHELPFUL_KEY try: - hide_readout = redis_client('helpfulvotes').llen(settings.HELPFULVOTES_UNHELPFUL_KEY) == 0 - except (AttributeError, ConnectionError): + hide_readout = redis_client('helpfulvotes').llen(key) == 0 + except RedisError as e: + log.error('Redis error: %s' % e) hide_readout = True def rows(self, max=None): REDIS_KEY = settings.HELPFULVOTES_UNHELPFUL_KEY try: redis = redis_client('helpfulvotes') - if redis is None: - raise ConnectionError length = redis.llen(REDIS_KEY) max_get = max or length output = redis.lrange(REDIS_KEY, 0, max_get) - except ConnectionError: + except RedisError as e: + log.error('Redis error: %s' % e) output = [] return [self._format_row(r) for r in output] diff --git a/apps/dashboards/tests/test_cron.py b/apps/dashboards/tests/test_cron.py index 039a4d577dc..81bc2920b3f 100644 --- a/apps/dashboards/tests/test_cron.py +++ b/apps/dashboards/tests/test_cron.py @@ -9,7 +9,7 @@ from dashboards.cron import (cache_most_unhelpful_kb_articles, _get_old_unhelpful, _get_current_unhelpful) from sumo.tests import TestCase -from sumo.utils import redis_client +from sumo.redis_utils import redis_client, RedisError from wiki.models import HelpfulVote from wiki.tests import revision @@ -136,13 +136,11 @@ def test_current_articles_not_in_old(self): class TopUnhelpfulArticlesCronTests(TestCase): def setUp(self): super(TopUnhelpfulArticlesCronTests, self).setUp() + self.REDIS_KEY = settings.HELPFULVOTES_UNHELPFUL_KEY try: self.redis = redis_client('helpfulvotes') - if self.redis is None: - raise SkipTest self.redis.flushdb() - self.REDIS_KEY = settings.HELPFULVOTES_UNHELPFUL_KEY - except (KeyError, AttributeError): + except RedisError: raise SkipTest def tearDown(self): diff --git a/apps/dashboards/tests/test_views.py b/apps/dashboards/tests/test_views.py index d619a9cc207..64a1e222924 100644 --- a/apps/dashboards/tests/test_views.py +++ b/apps/dashboards/tests/test_views.py @@ -10,21 +10,13 @@ from dashboards.readouts import CONTRIBUTOR_READOUTS from sumo.tests import TestCase from sumo.urlresolvers import reverse -from sumo.utils import redis_client +from sumo.redis_utils import redis_client, RedisError from users.tests import user, group from wiki.models import HelpfulVote from wiki.tests import revision class LocalizationDashTests(TestCase): - def setUp(self): - super(LocalizationDashTests, self).setUp() - try: - self.redis = redis_client('helpfulvotes') - self.redis.flushdb() - except (KeyError, AttributeError): - pass - def test_redirect_to_contributor_dash(self): """Should redirect to Contributor Dash if the locale is the default""" response = self.client.get(reverse('dashboards.localization', @@ -62,11 +54,11 @@ def setUp(self): self.client.login(username=self.user.username, password='testpass') self.group = group(name='Contributors', save=True) # Without this, there were unrelated failures with l10n dashboard + self.REDIS_KEY = settings.HELPFULVOTES_UNHELPFUL_KEY try: self.redis = redis_client('helpfulvotes') self.redis.flushdb() - self.REDIS_KEY = settings.HELPFULVOTES_UNHELPFUL_KEY - except (KeyError, AttributeError): + except RedisError: raise SkipTest def tearDown(self): diff --git a/apps/dashboards/views.py b/apps/dashboards/views.py index 96a4d372395..b38ca707bc6 100644 --- a/apps/dashboards/views.py +++ b/apps/dashboards/views.py @@ -1,6 +1,7 @@ import colorsys from functools import partial import json +import logging import math from django.conf import settings @@ -9,7 +10,6 @@ from django.views.decorators.http import require_GET import jingo -from redis.exceptions import ConnectionError from tower import ugettext as _ from access.decorators import login_required @@ -20,8 +20,12 @@ from dashboards.utils import render_readouts import forums as forum_constants from forums.models import Thread +from sumo.redis_utils import redis_client, RedisError from sumo.urlresolvers import reverse -from sumo.utils import paginate, redis_client, smart_int +from sumo.utils import paginate, smart_int + + +log = logging.getLogger('k.dashboards') def _kb_readout(request, readout_slug, readouts, locale=None, mode=None): @@ -141,8 +145,9 @@ def get_helpful_graph_async(request): redis = redis_client('helpfulvotes') length = redis.llen(REDIS_KEY) output = redis.lrange(REDIS_KEY, 0, length) - except ConnectionError: - pass + except RedisError as e: + log.error('Redis error: %s' % e) + output = [] def _format_r(strresult): result = strresult.split('::') diff --git a/apps/kadmin/admin.py b/apps/kadmin/admin.py index e7eddfb313a..693e6bc7be6 100644 --- a/apps/kadmin/admin.py +++ b/apps/kadmin/admin.py @@ -12,7 +12,7 @@ import jinja2 from redis import ConnectionError -from sumo.utils import redis_client +from sumo.redis_utils import redis_client def settings(request): diff --git a/apps/karma/admin.py b/apps/karma/admin.py index a25beed9061..9d93aa00511 100644 --- a/apps/karma/admin.py +++ b/apps/karma/admin.py @@ -44,8 +44,9 @@ def karma(request): return HttpResponseRedirect(request.path) kmgr = KarmaAction.objects - top_alltime = [_user_karma_alltime(u, kmgr) for u in kmgr.top_alltime()] - top_week = [_user_karma_week(u, kmgr) for u in kmgr.top_week()] + top_alltime = [_user_karma_alltime(u, kmgr) for + u in kmgr.top_alltime() or []] + top_week = [_user_karma_week(u, kmgr) for u in kmgr.top_week() or []] username = request.GET.get('username') user_karma = None diff --git a/apps/karma/manager.py b/apps/karma/manager.py index 17cd7d81c58..07b6f7c7ad9 100644 --- a/apps/karma/manager.py +++ b/apps/karma/manager.py @@ -1,12 +1,12 @@ from datetime import date, timedelta -import inspect import logging from django.contrib.auth.models import User from redis.exceptions import ConnectionError -from sumo.utils import redis_client +from sumo.decorators import for_all_methods +from sumo.redis_utils import redis_client, RedisError KEY_PREFIX = 'karma' # Prefix for the Redis keys used. @@ -14,16 +14,15 @@ log = logging.getLogger('k.karma') -def for_all_methods(decorator): - def decorate(cls): - for method in inspect.getmembers(cls, inspect.ismethod): - setattr(cls, method[0], decorator(getattr(cls, method[0]))) - return cls - return decorate +def _handle_redis_errors(func): + """Decorator for KarmaManager methods. - -def catch_and_log_redis_connection_errors(func): + It handles configuration and connection errors. + """ def wrapper(*args, **kwargs): + if not args[0].redis: + return None + try: return func(*args, **kwargs) except ConnectionError as e: @@ -32,12 +31,15 @@ def wrapper(*args, **kwargs): return wrapper -@for_all_methods(catch_and_log_redis_connection_errors) +@for_all_methods(_handle_redis_errors) class KarmaManager(object): """Manager for querying karma data in Redis.""" def __init__(self, redis=None): if not redis: - redis = redis_client(name='karma') + try: + redis = redis_client(name='karma') + except RedisError as e: + log.error('Redis error: %s' % e) self.redis = redis # Setters: diff --git a/apps/karma/tasks.py b/apps/karma/tasks.py index abf11e79d95..55628942762 100644 --- a/apps/karma/tasks.py +++ b/apps/karma/tasks.py @@ -6,7 +6,8 @@ AnswerMarkedNotHelpfulAction, FirstAnswerAction, SolutionAction) from questions.models import Question, AnswerVote -from sumo.utils import chunked, redis_client +from sumo.redis_utils import redis_client +from sumo.utils import chunked @task diff --git a/apps/karma/tests/test_actions.py b/apps/karma/tests/test_actions.py index 5909787359e..87652a19808 100644 --- a/apps/karma/tests/test_actions.py +++ b/apps/karma/tests/test_actions.py @@ -6,8 +6,8 @@ from karma.actions import KarmaAction from karma.manager import KarmaManager +from sumo.redis_utils import redis_client, RedisError from sumo.tests import TestCase -from sumo.utils import redis_client from users.tests import user @@ -30,7 +30,7 @@ def setUp(self): try: self.mgr = KarmaManager() redis_client('karma').flushdb() - except (KeyError, AttributeError): + except RedisError: raise SkipTest @mock.patch.object(waffle, 'switch_is_active') diff --git a/apps/sumo/decorators.py b/apps/sumo/decorators.py index e1058e49f3d..4fc10d51153 100644 --- a/apps/sumo/decorators.py +++ b/apps/sumo/decorators.py @@ -1,3 +1,5 @@ +import inspect + from django.conf import settings from django.http import HttpResponseRedirect @@ -14,3 +16,14 @@ def _checkssl(request, *args, **kwargs): return view_func(request, *args, **kwargs) return _checkssl + + +def for_all_methods(decorator): + """A class decorator to apply a decorator to all its methods.""" + def decorate(cls): + for method in inspect.getmembers(cls, inspect.ismethod): + # Skip __dunder__ methods + if not (method[0].startswith('__') and method[0].endswith('__')): + setattr(cls, method[0], decorator(getattr(cls, method[0]))) + return cls + return decorate diff --git a/apps/sumo/redis_utils.py b/apps/sumo/redis_utils.py new file mode 100644 index 00000000000..669a6111311 --- /dev/null +++ b/apps/sumo/redis_utils.py @@ -0,0 +1,49 @@ +from django.conf import settings +from django.core.cache import parse_backend_uri + +from redis import Redis, ConnectionError + + +class RedisError(Exception): + """An error with the redis configuration or connnection.""" + + +def redis_client(name): + """Get a Redis client. + + Uses the name argument to lookup the connection string in the + settings.REDIS_BACKEND dict. + """ + if name not in settings.REDIS_BACKENDS: + raise RedisError( + '{k} is not defined in settings.REDIS_BACKENDS'.format(k=name)) + + uri = settings.REDIS_BACKENDS[name] + _, server, params = parse_backend_uri(uri) + db = params.pop('db', 1) + try: + db = int(db) + except (ValueError, TypeError): + db = 1 + try: + socket_timeout = float(params.pop('socket_timeout')) + except (KeyError, ValueError): + socket_timeout = None + password = params.pop('password', None) + if ':' in server: + host, port = server.split(':') + try: + port = int(port) + except (ValueError, TypeError): + port = 6379 + else: + host = server + port = 6379 + redis = Redis(host=host, port=port, db=db, password=password, + socket_timeout=socket_timeout) + try: + redis.info() + except ConnectionError: + raise RedisError( + 'Unable to connect to redis backend: {k}'.format(k=name)) + return redis diff --git a/apps/sumo/utils.py b/apps/sumo/utils.py index 20f7807ffc1..00ebc6a4a9d 100644 --- a/apps/sumo/utils.py +++ b/apps/sumo/utils.py @@ -2,13 +2,10 @@ from django.conf import settings from django.contrib.sites.models import Site -from django.core.cache import parse_backend_uri from django.db import models from django.db.models.signals import pre_delete from django.utils.http import urlencode -from redis import Redis - from sumo import paginator @@ -120,37 +117,3 @@ def get_next_url(request): url = None return url - - -def redis_client(name): - """Get a Redis client. - - Uses the name argument to lookup the connection string in the - settings.REDIS_BACKEND dict. - """ - if name not in settings.REDIS_BACKENDS: - return None - - uri = settings.REDIS_BACKENDS[name] - _, server, params = parse_backend_uri(uri) - db = params.pop('db', 1) - try: - db = int(db) - except (ValueError, TypeError): - db = 1 - try: - socket_timeout = float(params.pop('socket_timeout')) - except (KeyError, ValueError): - socket_timeout = None - password = params.pop('password', None) - if ':' in server: - host, port = server.split(':') - try: - port = int(port) - except (ValueError, TypeError): - port = 6379 - else: - host = server - port = 6379 - return Redis(host=host, port=port, db=db, password=password, - socket_timeout=socket_timeout) diff --git a/apps/sumo/views.py b/apps/sumo/views.py index b0f9b36e8c6..87553e85deb 100644 --- a/apps/sumo/views.py +++ b/apps/sumo/views.py @@ -22,8 +22,8 @@ from redis import ConnectionError from session_csrf import anonymous_csrf +from sumo.redis_utils import redis_client from sumo.urlresolvers import reverse -from sumo.utils import redis_client from users.forms import AuthenticationForm @@ -34,7 +34,7 @@ def handle403(request): """A 403 message that looks nicer than the normal Apache forbidden page""" return jingo.render(request, 'handlers/403.html', - {'form': AuthenticationForm() }, status=403) + {'form': AuthenticationForm()}, status=403) def handle404(request):