Merge pull request #5351 from hypothesis/fix-slow-user-stats-query

Use elasticsearch to get annotation_user_count

h/search/__init__.py

@@ -5,6 +5,9 @@
 from h.search.config import init
 from h.search.core import Search
 from h.search.query import (TopLevelAnnotationsFilter,
+                            DeletedFilter,
+                            Limiter,
+                            UserFilter,
                             AuthorityFilter,
                             TagsAggregation,
                             UsersAggregation,
@@ -14,6 +17,9 @@
 __all__ = (
     'Search',
     'TopLevelAnnotationsFilter',
+    'DeletedFilter',
+    'Limiter',
+    'UserFilter',
     'AuthorityFilter',
     'TagsAggregation',
     'UsersAggregation',

h/services/annotation_stats.py

@@ -2,12 +2,10 @@
 
 from __future__ import unicode_literals
 
-import sqlalchemy as sa
 from webob.multidict import MultiDict
 
-from h.models import Annotation
 from h.search import Search
-from h.search import TopLevelAnnotationsFilter
+from h.search import Limiter, DeletedFilter, UserFilter, TopLevelAnnotationsFilter
 
 
 class AnnotationStatsService(object):
@@ -16,36 +14,45 @@ class AnnotationStatsService(object):
     def __init__(self, request):
         self.request = request
 
-    def user_annotation_counts(self, userid):
-        """Return the count of annotations for this user."""
+    def user_annotation_count(self, userid):
+        """
+        Return the count of searchable top level annotations for this user.
+
+        If the logged in user has this userid, private annotations will be
+        included in this count, otherwise they will not.
+        """
+        params = MultiDict({'limit': 0, 'user': userid})
+        return self._search(params)
 
-        annotations = self.request.db.query(Annotation). \
-            filter_by(userid=userid, deleted=False). \
-            options(sa.orm.load_only('groupid', 'shared')).subquery()
-        grouping = sa.case([
-            (sa.not_(annotations.c.shared), 'private'),
-            (annotations.c.groupid == '__world__', 'public'),
-        ], else_='group')
+    def total_user_annotation_count(self, userid):
+        """
+        Return the count of all annotations for this user.
 
-        result = dict(self.request.db.query(grouping, sa.func.count(annotations.c.id)).group_by(grouping).all())
-        for key in ['public', 'group', 'private']:
-            result.setdefault(key, 0)
+        This disregards permissions, private/public, etc and returns the
+        total number of annotations the user has made (including replies).
+        """
+        params = MultiDict({'limit': 0, 'user': userid})
 
-        result['total'] = result['public'] + \
-            result['group'] + \
-            result['private']
+        search = Search(self.request, stats=self.request.stats)
+        search.clear()
+        search.append_modifier(Limiter())
+        search.append_modifier(DeletedFilter())
+        search.append_modifier(UserFilter())
 
-        return result
+        search_result = search.run(params)
+        return search_result.total
 
     def group_annotation_count(self, pubid):
         """
         Return the count of searchable top level annotations for this group.
         """
+        params = MultiDict({'limit': 0, 'group': pubid})
+        return self._search(params)
+
+    def _search(self, params):
         search = Search(self.request, stats=self.request.stats)
         search.append_modifier(TopLevelAnnotationsFilter())
 
-        params = MultiDict({'limit': 0, 'group': pubid})
-
         search_result = search.run(params)
         return search_result.total
 

h/views/activity.py

@@ -35,10 +35,13 @@ class SearchController(object):
 
     def __init__(self, request):
         self.request = request
+        # Cache a copy of the extracted query params for the child controllers to use if needed.
+        self.parsed_query_params = query.extract(self.request)
 
     @view_config(request_method='GET')
     def search(self):
-        q = query.extract(self.request)
+        # Make a copy of the query params to be consumed by search.
+        q = self.parsed_query_params.copy()
 
         # Check whether a redirect is required.
         query.check_url(self.request, q)
@@ -125,7 +128,6 @@ def user_annotation_count(aggregation, userid):
                     return user['count']
             return 0
 
-        q = query.extract(self.request)
         members = []
         moderators = []
         users_aggregation = result['search_results'].aggregations.get('users', [])
@@ -138,7 +140,7 @@ def user_annotation_count(aggregation, userid):
                                                        u.userid),
                         'faceted_by': _faceted_by_user(self.request,
                                                        u.username,
-                                                       q)}
+                                                       self.parsed_query_params)}
                        for u in self.group.members]
             members = sorted(members, key=lambda k: k['username'].lower())
         else:
@@ -152,11 +154,11 @@ def user_annotation_count(aggregation, userid):
                                                               u.userid),
                                'faceted_by': _faceted_by_user(self.request,
                                                               u.username,
-                                                              q)}
+                                                              self.parsed_query_params)}
                               for u in [self.group.creator]]
                 moderators = sorted(moderators, key=lambda k: k['username'].lower())
 
-        group_annotation_count = self._get_total_annotations_in_group(result, q, self.request)
+        group_annotation_count = self._get_total_annotations_in_group(result, self.request)
 
         result['stats'] = {
             'annotation_count': group_annotation_count,
@@ -205,14 +207,14 @@ def user_annotation_count(aggregation, userid):
 
         return result
 
-    def _get_total_annotations_in_group(self, result, q, request):
+    def _get_total_annotations_in_group(self, result, request):
         """
         Get number of annotations in group.
 
         If the search result already has this number don't run a query, just re-use it.
         """
         group_annotation_count = result['search_results'].total
-        if len(q) > 1:
+        if len(self.parsed_query_params) > 1:
             group_annotation_count = (self.request
                                       .find_service(name='annotation_stats')
                                       .group_annotation_count(self.group.pubid))
@@ -350,11 +352,7 @@ def domain(user):
                 return None
             return urlparse.urlparse(user.uri).netloc
 
-        user_annotation_counts = self.request.find_service(name='annotation_stats').user_annotation_counts(self.user.userid)
-        annotation_count = user_annotation_counts['public']
-        if self.request.authenticated_userid == self.user.userid:
-            annotation_count = user_annotation_counts['total']
-
+        annotation_count = self._get_total_user_annotations(result, self.request)
         result['stats'] = {
             'annotation_count': annotation_count,
         }
@@ -384,6 +382,19 @@ def domain(user):
 
         return result
 
+    def _get_total_user_annotations(self, result, request):
+        """
+        Get number of annotations that the user has made.
+
+        If the search result already has this number don't run a query, just re-use it.
+        """
+        user_annotation_count = result['search_results'].total
+        if len(self.parsed_query_params) > 1:
+            user_annotation_count = (self.request
+                                     .find_service(name='annotation_stats')
+                                     .user_annotation_count(self.user.userid))
+        return user_annotation_count
+
     @view_config(request_param='back')
     def back(self):
         return _back(self.request)

h/views/admin/users.py

@@ -37,8 +37,7 @@ def users_index(request):
 
     if user is not None:
         svc = request.find_service(name='annotation_stats')
-        counts = svc.user_annotation_counts(user.userid)
-        user_meta['annotations_count'] = counts['total']
+        user_meta['annotations_count'] = svc.total_user_annotation_count(user.userid)
 
     return {
         'default_authority': request.default_authority,

tests/h/services/annotation_stats_test.py

@@ -8,68 +8,76 @@
 from h.services.annotation_stats import AnnotationStatsService
 from h.services.annotation_stats import annotation_stats_factory
 from h.search import Search
-from h.search import TopLevelAnnotationsFilter
+from h.search import Limiter, DeletedFilter, UserFilter, TopLevelAnnotationsFilter
 
 
 class TestAnnotationStatsService(object):
-    def test_user_annotation_counts_returns_count_of_annotations_for_user(self, svc, factories):
-        userid = '123'
-        for i in range(3):
-            factories.Annotation(userid=userid, shared=True)
-        for i in range(2):
-            factories.Annotation(userid=userid, shared=False)
-        for i in range(4):
-            factories.Annotation(userid=userid, groupid='abc', shared=True)
-
-        results = svc.user_annotation_counts(userid)
-
-        assert results['public'] == 3
-        assert results['private'] == 2
-        assert results['group'] == 4
-
-    @pytest.mark.parametrize('public,group,private,expected_total', [
-        (3, 2, 4, 9),
-        (3, 2, 0, 5),
-        (3, 0, 4, 7),
-        (0, 2, 4, 6),
-     ])
-    def test_user_annotation_counts_includes_total_count_of_annotations_for_user(
-            self, svc, factories, public, group, private, expected_total):
-        userid = '123'
-        for i in range(public):
-            factories.Annotation(userid=userid, shared=True)
-        for i in range(private):
-            factories.Annotation(userid=userid, shared=False)
-        for i in range(group):
-            factories.Annotation(userid=userid, groupid='abc', shared=True)
-
-        results = svc.user_annotation_counts(userid)
-
-        assert results['total'] == expected_total
-
-    def test_user_annotation_counts_returns_default_values(self, svc, factories):
-        results = svc.user_annotation_counts('123')
-
-        assert results['public'] == 0
-        assert results['private'] == 0
-        assert results['group'] == 0
-        assert results['total'] == 0
-
-    def test_user_annotation_counts_ignores_deleted_annotations(self, svc, factories):
-        userid = '123'
-        for i in range(3):
-            factories.Annotation(userid=userid, deleted=True, shared=True)
-        for i in range(2):
-            factories.Annotation(userid=userid, deleted=True, shared=False)
-        for i in range(4):
-            factories.Annotation(userid=userid, deleted=True, groupid='abc', shared=True)
-
-        results = svc.user_annotation_counts(userid)
-
-        assert results['public'] == 0
-        assert results['private'] == 0
-        assert results['group'] == 0
-        assert results['total'] == 0
+    def test_total_user_annotation_count_calls_search_with_request_and_stats(
+        self, svc, search, pyramid_request,
+    ):
+        svc.total_user_annotation_count('userid')
+
+        search.assert_called_with(pyramid_request, stats=pyramid_request.stats)
+
+    def test_total_user_annotation_count_calls_run_with_userid_and_limit(
+        self, svc, search,
+    ):
+        svc.total_user_annotation_count('userid')
+
+        search.return_value.run.assert_called_with({"limit": 0, "user": "userid"})
+
+    def test_toal_user_annotation_count_attaches_correct_modifiers(
+        self, svc, search, limiter, deleted_filter, user_filter,
+    ):
+        svc.total_user_annotation_count('userid')
+
+        assert search.return_value.clear.called
+
+        assert search.return_value.append_modifier.call_count == 3
+        search.return_value.append_modifier.assert_has_calls([
+            mock.call(limiter.return_value),
+            mock.call(deleted_filter.return_value),
+            mock.call(user_filter.return_value)])
+
+    def test_total_user_annotation_count_returns_total(
+        self, svc, search,
+    ):
+        search.return_value.run.return_value.total = 3
+
+        anns = svc.total_user_annotation_count('userid')
+
+        assert anns == 3
+
+    def test_user_annotation_count_calls_search_with_request_and_stats(
+        self, svc, search, pyramid_request,
+    ):
+        svc.user_annotation_count('userid')
+
+        search.assert_called_with(pyramid_request, stats=pyramid_request.stats)
+
+    def test_user_annotation_count_calls_run_with_userid_and_limit(
+        self, svc, search,
+    ):
+        svc.user_annotation_count('userid')
+
+        search.return_value.run.assert_called_with({"limit": 0, "user": "userid"})
+
+    def test_user_annotation_count_excludes_replies(
+        self, svc, search, top_level_annotation_filter,
+    ):
+        svc.user_annotation_count('userid')
+
+        search.return_value.append_modifier.assert_called_with(
+            top_level_annotation_filter.return_value)
+
+    def test_user_annotation_count_returns_total(
+        self, svc, search,
+    ):
+        search.return_value.run.return_value.total = 3
+
+        anns = svc.user_annotation_count('userid')
+
+        assert anns == 3
 
     def test_group_annotation_count_calls_search_with_request_and_stats(
         self, svc, search, pyramid_request,
@@ -137,3 +145,21 @@ def search(patch):
 def top_level_annotation_filter(patch):
     return patch('h.services.annotation_stats.TopLevelAnnotationsFilter',
                  autospec=TopLevelAnnotationsFilter, spec_set=True)
+
+
+@pytest.fixture
+def limiter(patch):
+    return patch('h.services.annotation_stats.Limiter',
+                 autospec=Limiter, spec_set=True)
+
+
+@pytest.fixture
+def deleted_filter(patch):
+    return patch('h.services.annotation_stats.DeletedFilter',
+                 autospec=DeletedFilter, spec_set=True)
+
+
+@pytest.fixture
+def user_filter(patch):
+    return patch('h.services.annotation_stats.UserFilter',
+                 autospec=UserFilter, spec_set=True)