[PATCHv2 05/10] REST: Add Patches to the API

[Finucane, Stephen]

On 17 May 14:08, Finucane, Stephen wrote:
> On 10 May 17:39, Andy Doan wrote:
> > This exports patches via the REST API.
> 
> supernit: s/exports/exposes :)
> 
> > Security Constraints:
> >  * Anyone (logged in or not) can read all objects.
> >  * No one can create/delete objects.
> >  * Project maintainers are allowed to update (ie "patch"
> >    attributes)
> > 
> > NOTE: Patch.save was overridden incorrectly and had to be
> > fixed to work with DRF.
> 
> On further inspection, I think the responses still need a little work.
> Sorry for not spotting this the first go round.
> 
> Stephen
> 
> > Signed-off-by: Andy Doan <andy.doan at linaro.org>
> > ---
> >  patchwork/models.py              |   4 +-
> >  patchwork/rest_serializers.py    |   9 +++-
> >  patchwork/tests/test_rest_api.py | 110 ++++++++++++++++++++++++++++++++++++++-
> >  patchwork/views/rest_api.py      |   9 +++-
> >  4 files changed, 126 insertions(+), 6 deletions(-)
> > 
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index 4d454c7..6324273 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -359,14 +359,14 @@ class Patch(Submission):
> >          for tag in tags:
> >              self._set_tag(tag, counter[tag])
> >  
> > -    def save(self):
> > +    def save(self, **kwargs):
> >          if not hasattr(self, 'state') or not self.state:
> >              self.state = get_default_initial_patch_state()
> >  
> >          if self.hash is None and self.diff is not None:
> >              self.hash = hash_patch(self.diff).hexdigest()
> >  
> > -        super(Patch, self).save()
> > +        super(Patch, self).save(**kwargs)
> >  
> >          self.refresh_tag_counts()
> >  
> > diff --git a/patchwork/rest_serializers.py b/patchwork/rest_serializers.py
> > index 60633b4..52a0306 100644
> > --- a/patchwork/rest_serializers.py
> > +++ b/patchwork/rest_serializers.py
> > @@ -17,7 +17,7 @@
> >  # along with Patchwork; if not, write to the Free Software
> >  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> >  
> > -from patchwork.models import Person, Project
> > +from patchwork.models import Patch, Person, Project
> >  
> >  from rest_framework.serializers import ModelSerializer
> >  
> > @@ -30,3 +30,10 @@ class PersonSerializer(ModelSerializer):
> >  class ProjectSerializer(ModelSerializer):
> >      class Meta:
> >          model = Project
> > +
> > +
> > +class PatchSerializer(ModelSerializer):
> > +    class Meta:
> > +        model = Patch
> > +        read_only_fields = ('project', 'name', 'date', 'submitter', 'diff',
> > +                            'content', 'hash', 'msgid')
> 
> So I played around with think the responses could do with a little more
> work. Firstly, here's a sample response (with the really long fields
> "SNIPPED!").
> 
>     {
>         "id": 1,
>         "mbox_url": "/patch/1/mbox/",
>         "msgid": "<1391726961-32072-1-git-send-email-markus.mayer at linaro.org>",
>         "date": "2014-02-06T22:49:21",
>         "headers": "SNIPPED!",
>         "content": "SNIPPED!",
>         "name": "[RFC] parsemail.py: Don't search for patches in replies",
>         "diff": "SNIPPED!",
>         "commit_ref": null,
>         "pull_url": null,
>         "archived": false,
>         "hash": "312958438b253a6436397bb06816c09616d0833e",
>         "submitter": 1,
>         "project": 1,
>         "delegate": null,
>         "state": "New",
>         "tags": []
>     },
> 
> (1) We should have two serializers: one for 'list' and one for 'detail'
> (or 'get'). I would exclude 'headers', 'content' and 'diff' from the
> 'list' response (and test this).
> (2) We should use links for the 'project'/'submitter' fields, rather
> than IDs.  Alternatively, we could have a 'project_id' and
> 'project_url' field instead of simply 'project'. What do you prefer?
> (3) Should 'header' be a list, like 'tags'?
> (4) Could/should 'mbox_url' use an absolute link instead of the
> relative one?

(5) Use 'prefetch_related' on 'patch.tags', in addition to
'patch.checks'. There is a cascade of queries to the 'patchwork_tag'
field when accessing '/api/1.0/patches/' at the moment. In general,
we could probably do with making better use of 'defer',
'prefetch_related' and 'select_related'. The existing views code [1]
would be a good place to start here.

Stephen

[1] https://github.com/getpatchwork/patchwork/blob/8aae263/patchwork/views/__init__.py#L289

> I'm open to feedback on all of these, of course :) Also, apologies if
> the spec misguided you: I'll do some work on this over the weekend.
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork