[v2,05/13] 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>
Cc: Andy Doan <andy.doan@linaro.org>
---
 patchwork/api/patch.py           | 12 +++++-------
 patchwork/models.py              | 13 ++++++++++---
 patchwork/tests/test_rest_api.py |  5 ++---
 3 files changed, 17 insertions(+), 13 deletions(-)

Hi Stephen,

Always keen on performance improvements! I think the new data format is
better too.

I'm not 100% across the DRF view of the world, so apologies if there are
any dumb questions.

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

I notice you've added the with_tag_counts. Is that required every time
we get the query_set? What's the motivation for doing it here?

>          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 a27dda6..d1d0857 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -225,8 +225,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

I must admit some confusion here. Previously project was not
nullabe. What's the motivation for making it nullable?

And one final question - given our recent history with broken parsing -
is this all going to behave fine in the presence of malformed tags? I
don't think this particular patch (or patch set) really touch it, but
probably worth thinking about while we're in this area.

Regards,
Daniel

On Mon, 2016-11-21 at 14:18 +1100, Daniel Axtens wrote:
> Hi Stephen,
> 
> Always keen on performance improvements! I think the new data format
> is
> better too.
> 
> I'm not 100% across the DRF view of the world, so apologies if there
> are
> any dumb questions.
> 
> > 
> >      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')
> 
> I notice you've added the with_tag_counts. Is that required every
> time
> we get the query_set? What's the motivation for doing it here?

If we want to access the tags, then yes. If you don't do this, you need
to calculate the tag counts in Python which results in a cascade of
queries. It's annoying but necessary.

> > 
> >          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 a27dda6..d1d0857 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -225,8 +225,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
> 
> I must admit some confusion here. Previously project was not
> nullabe. What's the motivation for making it nullable?

In the 'get_queryset' function above, we don't actually have a project
instance available to pass to 'with_tag_counts'. Seeing as the only
reason we even pass 'with_tag_counts' is to validate whether tags
should be displayed (project.tags == Tag.objects.all() for all
projects), it's not really necessary. It's a bit hacky, but I'll fix
post-2.0. 

> And one final question - given our recent history with broken parsing
> -
> is this all going to behave fine in the presence of malformed tags? I
> don't think this particular patch (or patch set) really touch it, but
> probably worth thinking about while we're in this area.

There aren't really such as thing as malformed tags. We don't store the
body of the tags, for example. I think we're good here, but we can
revisit if we see funky stuff in the wild.

Stephen