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

Daniel Axtens dja at axtens.net
Mon Nov 21 14:18:21 AEDT 2016


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


More information about the Patchwork mailing list