REST: Resolve performance issues with tags

The list comprehension used to generate a list of tags resulted in a
significant number of duplicated queries. Resolve this by copying the
approach taken to minimize patch queries in the UI.

This changes the output of the response in two ways. First, counts for
all tag patches are now shown, even if the count is 0. Secondly, a
dictionary is returned, with the tag name as the key, rather than a
list. As such, the output for a typical patch is transformed from:

    [
      {
        'name': 'Reviewed-by',
        'count': 1
      }
    ]

to

    {
      'Reviewed-by': 1
      'Acked-by': 0,
      'Tested-by': 0
    }

How this is achieved is a little hacky, but reworked tags are on the
post-v2.0 which will allow this to be resolved.

Signed-off-by: Stephen Finucane <stephen@that.guru>

diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
index 3af5994fcd2a7ce1e9491d8c6cff402424e676ac..811ba1e947cedad2b377b6bbbb3e5bf9b14880ca 100644 (file)
--- a/patchwork/api/patch.py
+++ b/patchwork/api/patch.py
@@ -52,9 +52,11 @@ class PatchSerializer(HyperlinkedModelSerializer):
         return request.build_absolute_uri(instance.get_mbox_url())
 
     def get_tags(self, instance):
-        # TODO(stephenfin): I don't think this is correct - too many queries
-        return [{'name': x.tag.name, 'count': x.count}
-                for x in instance.patchtag_set.all()]
+        if instance.project.tags:
+            return {x.name: getattr(instance, x.attr_name)
+                    for x in instance.project.tags}
+        else:
+            return None
 
     def get_headers(self, instance):
         if instance.headers:
@@ -85,10 +87,9 @@ class PatchViewSet(PatchworkViewSet):
     serializer_class = PatchSerializer
 
     def get_queryset(self):
-        qs = super(PatchViewSet, self).get_queryset(
-        ).prefetch_related(
-            'check_set', 'patchtag_set'
-        ).select_related('state', 'submitter', 'delegate')
+        qs = super(PatchViewSet, self).get_queryset().with_tag_counts()\
+            .prefetch_related('check_set')\
+            .select_related('state', 'submitter', 'delegate')
         if 'pk' not in self.kwargs:
             # we are doing a listing, we don't need these fields
             qs = qs.defer('content', 'diff', 'headers')

diff --git a/patchwork/models.py b/patchwork/models.py
index cff9587fff2ed666c7328e6824648d1ee5fb31a7..4ae41bedd10dc17f8f0bcc89bf0b5cb2e4a36d65 100644 (file)
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -226,8 +226,8 @@ def get_default_initial_patch_state():
 
 class PatchQuerySet(models.query.QuerySet):
 
-    def with_tag_counts(self, project):
-        if not project.use_tags:
+    def with_tag_counts(self, project=None):
+        if project and not project.use_tags:
             return self
 
         # We need the project's use_tags field loaded for Project.tags().
@@ -237,7 +237,14 @@ class PatchQuerySet(models.query.QuerySet):
         qs = self.prefetch_related('project')
         select = OrderedDict()
         select_params = []
-        for tag in project.tags:
+
+        # All projects have the same tags, so we're good to go here
+        if project:
+            tags = project.tags
+        else:
+            tags = Tag.objects.all()
+
+        for tag in tags:
             select[tag.attr_name] = (
                 "coalesce("
                 "(SELECT count FROM patchwork_patchtag"

diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
index 3be8ecf4345bba21d3465b414523ae1321d455f6..ddce4d75310d03dd3df96a4d4caf9c50c28e9a8e 100644 (file)
--- a/patchwork/tests/test_rest_api.py
+++ b/patchwork/tests/test_rest_api.py
@@ -304,9 +304,8 @@ class TestPatchAPI(APITestCase):
             content='Reviewed-by: Test User <test@example.com>\n')
         resp = self.client.get(self.api_url(patch.id))
         tags = resp.data['tags']
-        self.assertEqual(1, len(tags))
-        self.assertEqual(1, tags[0]['count'])
-        self.assertEqual('Reviewed-by', tags[0]['name'])
+        self.assertEqual(3, len(tags))
+        self.assertEqual(1, tags['Reviewed-by'])
 
     def test_anonymous_create(self):
         """Ensure anonymous "POST" operations are rejected."""