Hi, You have assuaged my concerns. Again, my lack of DRF expertise means I'm not comfortable formally reviewing this, but informally it looks good to me.
Regards, Daniel Stephen Finucane <[email protected]> writes: > 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 <[email protected]> > --- > patchwork/api/patch.py | 15 ++++++++------- > patchwork/models.py | 13 ++++++++++--- > patchwork/tests/test_rest_api.py | 5 ++--- > 3 files changed, 20 insertions(+), 13 deletions(-) > > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py > index 3af5994..811ba1e 100644 > --- a/patchwork/api/patch.py > +++ b/patchwork/api/patch.py > @@ -52,9 +52,11 @@ class PatchSerializer(HyperlinkedModelSerializer): > return request.build_absolute_uri(instance.get_mbox_url()) > > def get_tags(self, instance): > - # TODO(stephenfin): I don't think this is correct - too many queries > - return [{'name': x.tag.name, 'count': x.count} > - for x in instance.patchtag_set.all()] > + if instance.project.tags: > + return {x.name: getattr(instance, x.attr_name) > + for x in instance.project.tags} > + else: > + return None > > def get_headers(self, instance): > if instance.headers: > @@ -85,10 +87,9 @@ class PatchViewSet(PatchworkViewSet): > serializer_class = PatchSerializer > > 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') > 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 15a2936..6501958 100644 > --- a/patchwork/models.py > +++ b/patchwork/models.py > @@ -226,8 +226,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 > > # We need the project's use_tags field loaded for Project.tags(). > @@ -237,7 +237,14 @@ class PatchQuerySet(models.query.QuerySet): > qs = self.prefetch_related('project') > select = OrderedDict() > select_params = [] > - for tag in project.tags: > + > + # All projects have the same tags, so we're good to go here > + if project: > + tags = project.tags > + else: > + tags = Tag.objects.all() > + > + for tag in tags: > select[tag.attr_name] = ( > "coalesce(" > "(SELECT count FROM patchwork_patchtag" > diff --git a/patchwork/tests/test_rest_api.py > b/patchwork/tests/test_rest_api.py > index 3be8ecf..ddce4d7 100644 > --- a/patchwork/tests/test_rest_api.py > +++ b/patchwork/tests/test_rest_api.py > @@ -304,9 +304,8 @@ class TestPatchAPI(APITestCase): > content='Reviewed-by: Test User <[email protected]>\n') > resp = self.client.get(self.api_url(patch.id)) > tags = resp.data['tags'] > - self.assertEqual(1, len(tags)) > - self.assertEqual(1, tags[0]['count']) > - self.assertEqual('Reviewed-by', tags[0]['name']) > + self.assertEqual(3, len(tags)) > + self.assertEqual(1, tags['Reviewed-by']) > > def test_anonymous_create(self): > """Ensure anonymous "POST" operations are rejected.""" > -- > 2.7.4 > > _______________________________________________ > Patchwork mailing list > [email protected] > https://lists.ozlabs.org/listinfo/patchwork _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
