[PATCH] REST: Fix duplicate project queries

Sun Feb 2 21:29:12 AEDT 2020

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.

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