[PATCH] REST: Fix duplicate project queries

[Stephen Finucane]

On Sun, 2020-02-02 at 11:29 +0100, Mete Polat wrote:
> Hi Stephen,
> 
> On 01.02.20 15:05, Stephen Finucane wrote:
> > On Wed, 2020-01-29 at 20:01 +0100, Mete Polat wrote:
> > > Eliminates duplicate project queries caused by calling
> > > get_absolute_url() in the embedded serializers. Following foreign keys
> > > with 'series__project' will cache the project of the series as well as
> > > the series itself.
> > > 
> > > Signed-off-by: Mete Polat <metepolat2000 at gmail.com>
> > 
> > Hey Mete,
> > 
> > > ---
> > > There are still some duplicates in various /api/ views but it looks like
> > > those are caused by the REST framework itself.
> > > 
> > >  patchwork/api/cover.py  | 2 +-
> > >  patchwork/api/event.py  | 4 ++--
> > >  patchwork/api/patch.py  | 2 +-
> > >  patchwork/api/series.py | 5 +++--
> > >  4 files changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> > > index caf9a386efa5..9e86d47e00e5 100644
> > > --- a/patchwork/api/cover.py
> > > +++ b/patchwork/api/cover.py
> > > @@ -101,7 +101,7 @@ class CoverLetterList(ListAPIView):
> > >  
> > >      def get_queryset(self):
> > >          return CoverLetter.objects.all()\
> > > -            .select_related('project', 'submitter', 'series')\
> > > +            .select_related('project', 'submitter', 'series__project')\
> > >              .defer('content', 'headers')
> > >  
> > >  
> > > diff --git a/patchwork/api/event.py b/patchwork/api/event.py
> > > index a066faaec63b..fdff6a4f2fa6 100644
> > > --- a/patchwork/api/event.py
> > > +++ b/patchwork/api/event.py
> > > @@ -86,7 +86,7 @@ class EventList(ListAPIView):
> > >  
> > >      def get_queryset(self):
> > >          return Event.objects.all()\
> > > -            .prefetch_related('project', 'patch', 'series', 'cover',
> > > -                              'previous_state', 'current_state',
> > > +            .prefetch_related('project', 'patch__project', 'series__project',
> > > +                              'cover', 'previous_state', 'current_state',
> > >                                'previous_delegate', 'current_delegate',
> > >                                'created_check')
> > 
> > The rest of these look good but I wasn't able to produce a test that
> > proved this particular change was doing anything. Are you sure this
> > particular change works and, if so, could you suggest one or more
> > scenarios that I could use to validate this?
> > 
> 
> Just checked it again. Without patch__project, I get tons of these
> duplicate queries:
> 
> SELECT `patchwork_project`.`id`,
>        `patchwork_project`.`linkname`,
>        `patchwork_project`.`name`,
>        `patchwork_project`.`listid`,
>        `patchwork_project`.`listemail`,
>        `patchwork_project`.`subject_match`,
>        `patchwork_project`.`web_url`,
>        `patchwork_project`.`scm_url`,
>        `patchwork_project`.`webscm_url`,
>        `patchwork_project`.`list_archive_url`,
>        `patchwork_project`.`list_archive_url_format`,
>        `patchwork_project`.`commit_url_format`,
>        `patchwork_project`.`send_notifications`,
>        `patchwork_project`.`use_tags`
>   FROM `patchwork_project`
>  WHERE `patchwork_project`.`id` = 2
> 
> opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/contrib/staticfiles/handlers.py
> in __call__(65)
>   return self.application(environ, start_response)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/views/decorators/csrf.py
> in wrapped_view(54)
>   return view_func(*args, **kwargs)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/django/views/generic/base.py
> in view(71)
>   return self.dispatch(request, *args, **kwargs)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/views.py
> in dispatch(502)
>   response = handler(request, *args, **kwargs)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/generics.py
> in get(199)
>   return self.list(request, *args, **kwargs)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/mixins.py
> in list(43)
>   return self.get_paginated_response(serializer.data)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py
> in data(757)
>   ret = super().data
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py
> in data(261)
>   self._data = self.to_representation(self.instance)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py
> in to_representation(674)
>   return [
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py
> in <listcomp>(675)
>   self.child.to_representation(item) for item in iterable
> /home/patchwork/patchwork/patchwork/api/event.py in to_representation(51)
>   data = super(EventSerializer, self).to_representation(instance)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py
> in to_representation(526)
>   ret[field.field_name] = field.to_representation(attribute)
> /home/patchwork/patchwork/patchwork/api/embedded.py in to_representation(57)
>   return self._Serializer(context=self.context).to_representation(data)
> /home/patchwork/patchwork/patchwork/api/base.py in to_representation(90)
>   data = super(BaseHyperlinkedModelSerializer, self).to_representation(
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/serializers.py
> in to_representation(526)
>   ret[field.field_name] = field.to_representation(attribute)
> /opt/pyenv/versions/3.8.0/lib/python3.8/site-packages/rest_framework/fields.py
> in to_representation(1873)
>   return method(value)
> /home/patchwork/patchwork/patchwork/api/embedded.py in get_web_url(81)
>   return request.build_absolute_uri(instance.get_absolute_url())
> /home/patchwork/patchwork/patchwork/models.py in get_absolute_url(600)
>   kwargs={'project_id': self.project.linkname,
> 
> Same goes for series__project.
> I didn't write a test case but I was able to manually produce this
> behavior by just viewing the /api/events on my local instance and by
> optionally filtering for patch/series_created events. I think you should
> be able to reproduce this when there are enough of these patch/series
> events.

Ok, managed to get this to have an effect once I started creating a few
more events. Have applied this along with the test patch I sent
yesterday. Thanks!

Stephen

> Best regards,
> 
> Mete
> 
> > Cheers,
> > Stephen
> > 
> > > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> > > index a29a1ab0eb71..1a3ce9057490 100644
> > > --- a/patchwork/api/patch.py
> > > +++ b/patchwork/api/patch.py
> > > @@ -176,7 +176,7 @@ class PatchList(ListAPIView):
> > >          return Patch.objects.all()\
> > >              .prefetch_related('check_set')\
> > >              .select_related('project', 'state', 'submitter', 'delegate',
> > > -                            'series')\
> > > +                            'series__project')\
> > >              .defer('content', 'diff', 'headers')
> > >  
> > >  
> > > diff --git a/patchwork/api/series.py b/patchwork/api/series.py
> > > index f7bb8c06a6c9..df28f95dab1b 100644
> > > --- a/patchwork/api/series.py
> > > +++ b/patchwork/api/series.py
> > > @@ -55,8 +55,9 @@ class SeriesMixin(object):
> > >      serializer_class = SeriesSerializer
> > >  
> > >      def get_queryset(self):
> > > -        return Series.objects.all().prefetch_related('patches',)\
> > > -            .select_related('submitter', 'cover_letter', 'project')
> > > +        return Series.objects.all()\
> > > +            .prefetch_related('patches__project',)\
> > > +            .select_related('submitter', 'cover_letter__project', 'project')
> > >  
> > >  
> > >  class SeriesList(SeriesMixin, ListAPIView):