From: Stephen Finucane Date: Mon, 15 May 2017 23:13:34 +0000 (+0100) Subject: REST: Embed nested element bodies instead of URLs X-Git-Tag: v2.0.0-rc2~9 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8a430c18d4d70a46b128020411bbe5a63f298506;p=thirdparty%2Fpatchwork.git REST: Embed nested element bodies instead of URLs In developing a client for the Patchwork REST API, git-pw, it was noted that it should be possible to embed some information about nested resources in order to prevent the need for additional requests [1]. It was seen that this would be particularly beneficial for list operations, where each element in the N sized list could theoretically require an additional request for each of the M nested fields, resulting in N * (M + 1) total requests. Upon experimenting with the 2.0 RC1 API, this optimization was found to be less of a nice-to-have (and possibly something for the 2.1 release) and more of a must-have, particularly once one took network latency for each request into account. During testing with 'git-pw', simple list operations were found to take an average of 31 requests per operation, of which only one for was the resource endpoint itself ('GET /api/series'). As each of these requests took ~2 seconds a piece, listing was essentially broken. While local caching could be used to offset some of this demand, this will result in (a) significantly larger, more complex clients or (b) instances that strain under the load of dumb clients making multiple requests per operation. Instead, the server should be smarter about embedding the data that would actually be required by clients. Resolve the issue by embedding summarized versions of various nested fields instead of merely linking to them. Nesting is only a single level deep, to avoid large/complex database queries and with the expectation that only these basic fields (resource names, dates, etc.) would be required. These summary serializers are kept in their own module, to encourage consistent results throughout the API and to prevent circular import errors. This will have the side effect of slightly increasing load on the server due to the additional serialization required. However, this load is largely mitigated through the avoidance of deeper nesting as noted above. In addition, any increase in load seen will be a fraction of the demand that repeat requests will incur. While it would be possible to make nesting optional (by way of an 'embed' or 'expand' parameter), it is expected that this would be an atypical request and would result in far more complicated serialization code. [1] https://github.com/stephenfin/git-pw/blob/21e0e593/git_pw/patch.py#L88-L89 Signed-off-by: Stephen Finucane --- diff --git a/patchwork/api/base.py b/patchwork/api/base.py index 07979905..09b3bef2 100644 --- a/patchwork/api/base.py +++ b/patchwork/api/base.py @@ -22,6 +22,7 @@ from django.shortcuts import get_object_or_404 from rest_framework import permissions from rest_framework.pagination import PageNumberPagination from rest_framework.response import Response +from rest_framework.serializers import HyperlinkedIdentityField class LinkHeaderPagination(PageNumberPagination): @@ -72,3 +73,21 @@ class MultipleFieldLookupMixin(object): filter_kwargs[field_name] = self.kwargs[field] return get_object_or_404(queryset, **filter_kwargs) + + +class CheckHyperlinkedIdentityField(HyperlinkedIdentityField): + + def get_url(self, obj, view_name, request, format): + # Unsaved objects will not yet have a valid URL. + if obj.pk is None: + return None + + return self.reverse( + view_name, + kwargs={ + 'patch_id': obj.patch.id, + 'check_id': obj.id, + }, + request=request, + format=format, + ) diff --git a/patchwork/api/bundle.py b/patchwork/api/bundle.py index 88d74a5d..92899566 100644 --- a/patchwork/api/bundle.py +++ b/patchwork/api/bundle.py @@ -25,13 +25,19 @@ from rest_framework.serializers import SerializerMethodField from patchwork.api.base import PatchworkPermission from patchwork.api.filters import BundleFilter +from patchwork.api.embedded import PatchSerializer +from patchwork.api.embedded import ProjectSerializer +from patchwork.api.embedded import UserSerializer from patchwork.compat import is_authenticated from patchwork.models import Bundle class BundleSerializer(HyperlinkedModelSerializer): + project = ProjectSerializer(read_only=True) mbox = SerializerMethodField() + owner = UserSerializer(read_only=True) + patches = PatchSerializer(many=True, read_only=True) def get_mbox(self, instance): request = self.context.get('request') @@ -44,9 +50,6 @@ class BundleSerializer(HyperlinkedModelSerializer): read_only_fields = ('owner', 'patches', 'mbox') extra_kwargs = { 'url': {'view_name': 'api-bundle-detail'}, - 'project': {'view_name': 'api-project-detail'}, - 'owner': {'view_name': 'api-user-detail'}, - 'patches': {'view_name': 'api-patch-detail'}, } diff --git a/patchwork/api/check.py b/patchwork/api/check.py index d3682652..66b46017 100644 --- a/patchwork/api/check.py +++ b/patchwork/api/check.py @@ -20,13 +20,13 @@ from rest_framework.exceptions import PermissionDenied from rest_framework.generics import ListCreateAPIView from rest_framework.generics import RetrieveAPIView -from rest_framework.relations import HyperlinkedRelatedField from rest_framework.serializers import CurrentUserDefault from rest_framework.serializers import HiddenField from rest_framework.serializers import HyperlinkedModelSerializer -from rest_framework.serializers import HyperlinkedIdentityField +from patchwork.api.base import CheckHyperlinkedIdentityField from patchwork.api.base import MultipleFieldLookupMixin +from patchwork.api.embedded import UserSerializer from patchwork.api.filters import CheckFilter from patchwork.models import Check from patchwork.models import Patch @@ -40,29 +40,11 @@ class CurrentPatchDefault(object): return self.patch -class CheckHyperlinkedIdentityField(HyperlinkedIdentityField): - - def get_url(self, obj, view_name, request, format): - # Unsaved objects will not yet have a valid URL. - if obj.pk is None: - return None - - return self.reverse( - view_name, - kwargs={ - 'patch_id': obj.patch.id, - 'check_id': obj.id, - }, - request=request, - format=format, - ) - - class CheckSerializer(HyperlinkedModelSerializer): - user = HyperlinkedRelatedField( - 'api-user-detail', read_only=True, default=CurrentUserDefault()) - patch = HiddenField(default=CurrentPatchDefault()) + url = CheckHyperlinkedIdentityField('api-check-detail') + patch = HiddenField(default=CurrentPatchDefault()) + user = UserSerializer(read_only=True, default=CurrentUserDefault()) def run_validation(self, data): for val, label in Check.STATE_CHOICES: diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py index e45680bc..797cadfd 100644 --- a/patchwork/api/cover.py +++ b/patchwork/api/cover.py @@ -23,18 +23,20 @@ import django from rest_framework.generics import ListAPIView from rest_framework.generics import RetrieveAPIView from rest_framework.serializers import HyperlinkedModelSerializer -from rest_framework.serializers import HyperlinkedRelatedField from rest_framework.serializers import SerializerMethodField from patchwork.api.filters import CoverLetterFilter +from patchwork.api.embedded import PersonSerializer +from patchwork.api.embedded import ProjectSerializer +from patchwork.api.embedded import SeriesSerializer from patchwork.models import CoverLetter class CoverLetterListSerializer(HyperlinkedModelSerializer): - series = HyperlinkedRelatedField( - many=True, - read_only=True, - view_name='api-series-detail') + + project = ProjectSerializer(read_only=True) + submitter = PersonSerializer(read_only=True) + series = SeriesSerializer(many=True, read_only=True) class Meta: model = CoverLetter @@ -43,8 +45,6 @@ class CoverLetterListSerializer(HyperlinkedModelSerializer): read_only_fields = fields extra_kwargs = { 'url': {'view_name': 'api-cover-detail'}, - 'project': {'view_name': 'api-project-detail'}, - 'submitter': {'view_name': 'api-person-detail'}, } @@ -58,8 +58,7 @@ class CoverLetterDetailSerializer(CoverLetterListSerializer): class Meta: model = CoverLetter fields = CoverLetterListSerializer.Meta.fields + ('headers', 'content') - read_only_fields = CoverLetterListSerializer.Meta.read_only_fields + ( - 'headers', 'content') + read_only_fields = fields extra_kwargs = CoverLetterListSerializer.Meta.extra_kwargs diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py new file mode 100644 index 00000000..122422a4 --- /dev/null +++ b/patchwork/api/embedded.py @@ -0,0 +1,162 @@ +# Patchwork - automated patch tracking system +# Copyright (C) 2017 Stephen Finucane +# +# This file is part of the Patchwork package. +# +# Patchwork is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# Patchwork is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Patchwork; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + +"""Serializers for embedded use. + +A collection of serializers. None of the serializers here should reference +nested fields. +""" + +from rest_framework.serializers import CharField +from rest_framework.serializers import HyperlinkedModelSerializer +from rest_framework.serializers import SerializerMethodField + +from patchwork.api.base import CheckHyperlinkedIdentityField +from patchwork import models + + +class MboxMixin(HyperlinkedModelSerializer): + """Embed an link to the mbox URL. + + This field is just way too useful to leave out of even the embedded + serialization. + """ + + mbox = SerializerMethodField() + + def get_mbox(self, instance): + request = self.context.get('request') + return request.build_absolute_uri(instance.get_mbox_url()) + + +class BundleSerializer(MboxMixin, HyperlinkedModelSerializer): + + class Meta: + model = models.Bundle + fields = ('id', 'url', 'name', 'mbox') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-bundle-detail'}, + } + + +class CheckSerializer(HyperlinkedModelSerializer): + + url = CheckHyperlinkedIdentityField('api-check-detail') + + def to_representation(self, instance): + data = super(CheckSerializer, self).to_representation(instance) + data['state'] = instance.get_state_display() + return data + + class Meta: + model = models.Check + fields = ('id', 'url', 'date', 'state', 'target_url', 'context') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-check-detail'}, + + } + + +class CoverLetterSerializer(HyperlinkedModelSerializer): + + class Meta: + model = models.CoverLetter + fields = ('id', 'url', 'msgid', 'date', 'name') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-cover-detail'}, + } + + +class PatchSerializer(MboxMixin, HyperlinkedModelSerializer): + + class Meta: + model = models.Patch + fields = ('id', 'url', 'msgid', 'date', 'name', 'mbox') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-patch-detail'}, + } + + +class PersonSerializer(HyperlinkedModelSerializer): + + class Meta: + model = models.Person + fields = ('id', 'url', 'name', 'email') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-person-detail'}, + } + + +class ProjectSerializer(HyperlinkedModelSerializer): + + link_name = CharField(max_length=255, source='linkname') + list_id = CharField(max_length=255, source='listid') + list_email = CharField(max_length=200, source='listemail') + + class Meta: + model = models.Project + fields = ('id', 'url', 'name', 'link_name', 'list_id', 'list_email', + 'web_url', 'scm_url', 'webscm_url') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-project-detail'}, + } + + +class SeriesSerializer(MboxMixin, HyperlinkedModelSerializer): + + class Meta: + model = models.Series + fields = ('id', 'url', 'date', 'name', 'version', 'mbox') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-series-detail'}, + } + + +class UserSerializer(HyperlinkedModelSerializer): + + class Meta: + model = models.User + fields = ('id', 'url', 'username', 'first_name', 'last_name', 'email') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-user-detail'}, + } + + +class UserProfileSerializer(HyperlinkedModelSerializer): + + username = CharField(source='user.username') + first_name = CharField(source='user.first_name') + last_name = CharField(source='user.last_name') + email = CharField(source='user.email') + + class Meta: + model = models.UserProfile + fields = ('id', 'url', 'username', 'first_name', 'last_name', 'email') + read_only_fields = fields + extra_kwargs = { + 'url': {'view_name': 'api-user-detail'}, + } diff --git a/patchwork/api/event.py b/patchwork/api/event.py index a0600226..cc9270ae 100644 --- a/patchwork/api/event.py +++ b/patchwork/api/event.py @@ -20,20 +20,32 @@ from collections import OrderedDict from rest_framework.generics import ListAPIView -from rest_framework.reverse import reverse -from rest_framework.serializers import HyperlinkedModelSerializer +from rest_framework.serializers import ModelSerializer from rest_framework.serializers import SerializerMethodField +from patchwork.api.embedded import CheckSerializer +from patchwork.api.embedded import CoverLetterSerializer +from patchwork.api.embedded import PatchSerializer +from patchwork.api.embedded import ProjectSerializer +from patchwork.api.embedded import SeriesSerializer +from patchwork.api.embedded import UserSerializer from patchwork.api.filters import EventFilter from patchwork.api.patch import StateField from patchwork.models import Event -class EventSerializer(HyperlinkedModelSerializer): +class EventSerializer(ModelSerializer): + project = ProjectSerializer(read_only=True) + patch = PatchSerializer(read_only=True) + series = SeriesSerializer(read_only=True) + cover = CoverLetterSerializer(read_only=True) previous_state = StateField() current_state = StateField() + previous_delegate = UserSerializer() + current_delegate = UserSerializer() created_check = SerializerMethodField() + created_check = CheckSerializer() _category_map = { Event.CATEGORY_COVER_CREATED: ['cover'], @@ -48,15 +60,6 @@ class EventSerializer(HyperlinkedModelSerializer): Event.CATEGORY_SERIES_COMPLETED: ['series'], } - def get_created_check(self, instance): - if not instance.patch or not instance.created_check: - return - - return self.context.get('request').build_absolute_uri( - reverse('api-check-detail', kwargs={ - 'patch_id': instance.patch.id, - 'check_id': instance.created_check.id})) - def to_representation(self, instance): data = super(EventSerializer, self).to_representation(instance) payload = OrderedDict() @@ -80,15 +83,6 @@ class EventSerializer(HyperlinkedModelSerializer): 'cover', 'previous_state', 'current_state', 'previous_delegate', 'current_delegate', 'created_check') read_only_fields = fields - extra_kwargs = { - 'project': {'view_name': 'api-project-detail'}, - 'patch': {'view_name': 'api-patch-detail'}, - 'series': {'view_name': 'api-series-detail'}, - 'cover': {'view_name': 'api-cover-detail'}, - 'previous_delegate': {'view_name': 'api-user-detail'}, - 'current_delegate': {'view_name': 'api-user-detail'}, - 'created_check': {'view_name': 'api-check-detail'}, - } class EventList(ListAPIView): diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index 7247b110..f0c72250 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -29,6 +29,10 @@ from rest_framework.serializers import SerializerMethodField from patchwork.api.base import PatchworkPermission from patchwork.api.filters import PatchFilter +from patchwork.api.embedded import PersonSerializer +from patchwork.api.embedded import ProjectSerializer +from patchwork.api.embedded import SeriesSerializer +from patchwork.api.embedded import UserSerializer from patchwork.models import Patch from patchwork.models import State from patchwork.parser import clean_subject @@ -73,11 +77,16 @@ class StateField(RelatedField): class PatchListSerializer(HyperlinkedModelSerializer): - mbox = SerializerMethodField() + + project = ProjectSerializer(read_only=True) state = StateField() - tags = SerializerMethodField() + submitter = PersonSerializer(read_only=True) + delegate = UserSerializer() + mbox = SerializerMethodField() + series = SeriesSerializer(many=True, read_only=True) check = SerializerMethodField() checks = SerializerMethodField() + tags = SerializerMethodField() def get_mbox(self, instance): request = self.context.get('request') @@ -106,15 +115,11 @@ class PatchListSerializer(HyperlinkedModelSerializer): 'checks', 'tags') extra_kwargs = { 'url': {'view_name': 'api-patch-detail'}, - 'project': {'view_name': 'api-project-detail'}, - 'submitter': {'view_name': 'api-person-detail'}, - 'delegate': {'view_name': 'api-user-detail'}, - 'series': {'view_name': 'api-series-detail', - 'lookup_url_kwarg': 'pk'}, } class PatchDetailSerializer(PatchListSerializer): + headers = SerializerMethodField() prefixes = SerializerMethodField() diff --git a/patchwork/api/person.py b/patchwork/api/person.py index 574fa842..d002afff 100644 --- a/patchwork/api/person.py +++ b/patchwork/api/person.py @@ -22,17 +22,20 @@ from rest_framework.generics import ListAPIView from rest_framework.generics import RetrieveAPIView from rest_framework.permissions import IsAuthenticated +from patchwork.api.embedded import UserSerializer from patchwork.models import Person class PersonSerializer(HyperlinkedModelSerializer): + + user = UserSerializer(read_only=True) + class Meta: model = Person fields = ('id', 'url', 'name', 'email', 'user') read_only_fields = fields extra_kwargs = { 'url': {'view_name': 'api-person-detail'}, - 'user': {'view_name': 'api-user-detail'}, } diff --git a/patchwork/api/project.py b/patchwork/api/project.py index 8fb8984a..11d65049 100644 --- a/patchwork/api/project.py +++ b/patchwork/api/project.py @@ -22,19 +22,19 @@ from rest_framework.generics import ListAPIView from rest_framework.generics import RetrieveUpdateAPIView from rest_framework.serializers import CharField from rest_framework.serializers import HyperlinkedModelSerializer -from rest_framework.serializers import HyperlinkedRelatedField from patchwork.api.base import PatchworkPermission +from patchwork.api.embedded import UserProfileSerializer from patchwork.models import Project class ProjectSerializer(HyperlinkedModelSerializer): + link_name = CharField(max_length=255, source='linkname') list_id = CharField(max_length=255, source='listid') list_email = CharField(max_length=200, source='listemail') - maintainers = HyperlinkedRelatedField( - many=True, read_only=True, view_name='api-user-detail', - source='maintainer_project') + maintainers = UserProfileSerializer(many=True, read_only=True, + source='maintainer_project') class Meta: model = Project diff --git a/patchwork/api/series.py b/patchwork/api/series.py index 01f1cbb1..12f9277c 100644 --- a/patchwork/api/series.py +++ b/patchwork/api/series.py @@ -24,12 +24,20 @@ from rest_framework.serializers import SerializerMethodField from patchwork.api.base import PatchworkPermission from patchwork.api.filters import SeriesFilter +from patchwork.api.embedded import CoverLetterSerializer +from patchwork.api.embedded import PatchSerializer +from patchwork.api.embedded import PersonSerializer +from patchwork.api.embedded import ProjectSerializer from patchwork.models import Series class SeriesSerializer(HyperlinkedModelSerializer): + project = ProjectSerializer(read_only=True) + submitter = PersonSerializer(read_only=True) mbox = SerializerMethodField() + cover_letter = CoverLetterSerializer(read_only=True) + patches = PatchSerializer(read_only=True, many=True) def get_mbox(self, instance): request = self.context.get('request') @@ -44,10 +52,6 @@ class SeriesSerializer(HyperlinkedModelSerializer): 'received_all', 'mbox', 'cover_letter', 'patches') extra_kwargs = { 'url': {'view_name': 'api-series-detail'}, - 'project': {'view_name': 'api-project-detail'}, - 'submitter': {'view_name': 'api-person-detail'}, - 'cover_letter': {'view_name': 'api-cover-detail'}, - 'patches': {'view_name': 'api-patch-detail'}, } diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py index 6aed4db9..9b94c47f 100644 --- a/patchwork/tests/test_rest_api.py +++ b/patchwork/tests/test_rest_api.py @@ -60,6 +60,9 @@ class TestProjectAPI(APITestCase): self.assertEqual(project_obj.name, project_json['name']) self.assertEqual(project_obj.linkname, project_json['link_name']) self.assertEqual(project_obj.listid, project_json['list_id']) + + # nested fields + self.assertEqual(len(project_json['maintainers']), project_obj.maintainer_project.all().count()) @@ -175,8 +178,9 @@ class TestPersonAPI(APITestCase): else: self.assertEqual(person_obj.user.username, person_json['name']) self.assertEqual(person_obj.user.email, person_json['email']) - self.assertIn(TestUserAPI.api_url(person_obj.user.id), - person_json['user']) + # nested fields + self.assertEqual(person_obj.user.id, + person_json['user']['id']) def test_list(self): """This API requires authenticated users.""" @@ -307,10 +311,13 @@ class TestPatchAPI(APITestCase): self.assertEqual(patch_obj.msgid, patch_json['msgid']) self.assertEqual(patch_obj.state.name, patch_json['state']) self.assertIn(patch_obj.get_mbox_url(), patch_json['mbox']) - self.assertIn(TestPersonAPI.api_url(patch_obj.submitter.id), - patch_json['submitter']) - self.assertIn(TestProjectAPI.api_url(patch_obj.project.id), - patch_json['project']) + + # nested fields + + self.assertEqual(patch_obj.submitter.id, + patch_json['submitter']['id']) + self.assertEqual(patch_obj.project.id, + patch_json['project']['id']) def test_list(self): """Validate we can list a patch.""" @@ -450,8 +457,11 @@ class TestCoverLetterAPI(APITestCase): def assertSerialized(self, cover_obj, cover_json): self.assertEqual(cover_obj.id, cover_json['id']) self.assertEqual(cover_obj.name, cover_json['name']) - self.assertIn(TestPersonAPI.api_url(cover_obj.submitter.id), - cover_json['submitter']) + + # nested fields + + self.assertEqual(cover_obj.submitter.id, + cover_json['submitter']['id']) def test_list(self): """Validate we can list cover letters.""" @@ -512,16 +522,20 @@ class TestSeriesAPI(APITestCase): self.assertEqual(series_obj.id, series_json['id']) self.assertEqual(series_obj.name, series_json['name']) self.assertIn(series_obj.get_mbox_url(), series_json['mbox']) - self.assertIn(TestProjectAPI.api_url(series_obj.project.id), - series_json['project']) - self.assertIn(TestPersonAPI.api_url(series_obj.submitter.id), - series_json['submitter']) + + # nested fields + + self.assertEqual(series_obj.project.id, + series_json['project']['id']) + self.assertEqual(series_obj.submitter.id, + series_json['submitter']['id']) self.assertEqual(series_obj.patches.count(), len(series_json['patches'])) + if series_obj.cover_letter: - self.assertIn( - TestCoverLetterAPI.api_url(series_obj.cover_letter.id), - series_json['cover_letter']) + self.assertEqual( + series_obj.cover_letter.id, + series_json['cover_letter']['id']) def test_list(self): """Validate we can list series.""" @@ -692,12 +706,15 @@ class TestBundleAPI(APITestCase): self.assertEqual(bundle_obj.name, bundle_json['name']) self.assertEqual(bundle_obj.public, bundle_json['public']) self.assertIn(bundle_obj.get_mbox_url(), bundle_json['mbox']) + + # nested fields + self.assertEqual(bundle_obj.patches.count(), len(bundle_json['patches'])) - self.assertIn(TestUserAPI.api_url(bundle_obj.owner.id), - bundle_json['owner']) - self.assertIn(TestProjectAPI.api_url(bundle_obj.project.id), - bundle_json['project']) + self.assertEqual(bundle_obj.owner.id, + bundle_json['owner']['id']) + self.assertEqual(bundle_obj.project.id, + bundle_json['project']['id']) def test_list(self): """Validate we can list bundles."""