[PATCH v2 05/13] REST: Resolve performance issues with tags

[Stephen Fincane]

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