[PATCH v3 07/16] REST: Resolve performance issues with tags

[Daniel Axtens]

Hi,

You have assuaged my concerns. Again, my lack of DRF expertise means I'm
not comfortable formally reviewing this, but informally it looks good to
me.

Regards,
Daniel

Stephen Finucane <stephen at that.guru> writes:

> 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 at that.guru>
> ---
>  patchwork/api/patch.py           | 15 ++++++++-------
>  patchwork/models.py              | 13 ++++++++++---
>  patchwork/tests/test_rest_api.py |  5 ++---
>  3 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index 3af5994..811ba1e 100644
> --- 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 15a2936..6501958 100644
> --- 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 3be8ecf..ddce4d7 100644
> --- 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 at 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."""
> -- 
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork