[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

@@ -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
 
 

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]

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):

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):

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('::')

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):

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

apps/karma/manager.py

@@ -1,29 +1,28 @@
 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.
 
 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:

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

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')

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

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

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)