| Message ID | 1462919967-26088-11-git-send-email-andy.doan@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 10 May 17:39, Andy Doan wrote: > Building a user-friendly CLI becomes difficult when project-ids are > required. It also makes it almost impossible to work with the current > format of the .pwclientrc file. > > Signed-off-by: Andy Doan <andy.doan@linaro.org> Some comments below but nothing blocking. Reviewed-by: Stephen Finucane <stephen.finucane@intel.com> > --- > patchwork/tests/test_rest_api.py | 5 +++++ > patchwork/views/rest_api.py | 16 ++++++++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py > index 6f1a375..0b180e6 100644 > --- a/patchwork/tests/test_rest_api.py > +++ b/patchwork/tests/test_rest_api.py > @@ -59,6 +59,11 @@ class TestProjectAPI(APITestCase): > self.assertEqual(status.HTTP_200_OK, resp.status_code) > self.assertEqual(defaults.project.name, resp.data['name']) > > + # make sure we can look up by linkname > + resp = self.client.get(self.api_url(resp.data['linkname'])) > + self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertEqual(defaults.project.name, resp.data['name']) > + > def test_anonymous_writes(self): > """Ensure anonymous "write" operations are rejected.""" > defaults.project.save() > diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py > index e5a642a..e125bc1 100644 > --- a/patchwork/views/rest_api.py > +++ b/patchwork/views/rest_api.py > @@ -113,6 +113,22 @@ class ProjectViewSet(PatchworkViewSet): > permission_classes = (PatchworkPermission,) > serializer_class = ProjectSerializer > > + def retrieve(self, request, pk=None): > + try: > + int(pk) > + except ValueError: > + self.kwargs = {'linkname': pk} # try and lookup by linkname > + self.lookup_field = 'linkname' > + return super(ProjectViewSet, self).retrieve(request, pk) > + > + def partial_update(self, request, pk=None): > + try: > + int(pk) I really hope users don't do this, but are integer-only linknames possible? :) Maybe we should just rule this out... > + except ValueError: > + self.kwargs = {'linkname': pk} # try and lookup by linkname > + self.lookup_field = 'linkname' > + return super(ProjectViewSet, self).partial_update(request, pk) Much of this is duplicated and could be moved to a separate function, IMO.
On 05/17/2016 08:33 AM, Finucane, Stephen wrote: > On 10 May 17:39, Andy Doan wrote: >> Building a user-friendly CLI becomes difficult when project-ids are >> required. It also makes it almost impossible to work with the current >> format of the .pwclientrc file. >> >> Signed-off-by: Andy Doan <andy.doan@linaro.org> > > Some comments below but nothing blocking. > > Reviewed-by: Stephen Finucane <stephen.finucane@intel.com> >> diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py >> index e5a642a..e125bc1 100644 >> --- a/patchwork/views/rest_api.py >> +++ b/patchwork/views/rest_api.py >> @@ -113,6 +113,22 @@ class ProjectViewSet(PatchworkViewSet): >> permission_classes = (PatchworkPermission,) >> serializer_class = ProjectSerializer >> >> + def retrieve(self, request, pk=None): >> + try: >> + int(pk) >> + except ValueError: >> + self.kwargs = {'linkname': pk} # try and lookup by linkname >> + self.lookup_field = 'linkname' >> + return super(ProjectViewSet, self).retrieve(request, pk) >> + >> + def partial_update(self, request, pk=None): >> + try: >> + int(pk) > > I really hope users don't do this, but are integer-only linknames > possible? :) Maybe we should just rule this out... The difficulty I'm hitting is choosing the precedence. Suppose we have: {'id': 1, linkname: 'project-1'} {'id': 2, linkname: '1'} and get a request "/api/1.0/project/1/". Which do we prefer? v3 will give precedence to the project ID and return: {'id': 1, linkname: 'project-1'} but its a tad confusing.
On 18 May 22:25, Andy Doan wrote: > On 05/17/2016 08:33 AM, Finucane, Stephen wrote: > >On 10 May 17:39, Andy Doan wrote: > >>Building a user-friendly CLI becomes difficult when project-ids are > >>required. It also makes it almost impossible to work with the current > >>format of the .pwclientrc file. > >> > >>Signed-off-by: Andy Doan <andy.doan@linaro.org> > > > >Some comments below but nothing blocking. > > > >Reviewed-by: Stephen Finucane <stephen.finucane@intel.com> > > >>diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py > >>index e5a642a..e125bc1 100644 > >>--- a/patchwork/views/rest_api.py > >>+++ b/patchwork/views/rest_api.py > >>@@ -113,6 +113,22 @@ class ProjectViewSet(PatchworkViewSet): > >> permission_classes = (PatchworkPermission,) > >> serializer_class = ProjectSerializer > >> > >>+ def retrieve(self, request, pk=None): > >>+ try: > >>+ int(pk) > >>+ except ValueError: > >>+ self.kwargs = {'linkname': pk} # try and lookup by linkname > >>+ self.lookup_field = 'linkname' > >>+ return super(ProjectViewSet, self).retrieve(request, pk) > >>+ > >>+ def partial_update(self, request, pk=None): > >>+ try: > >>+ int(pk) > > > >I really hope users don't do this, but are integer-only linknames > >possible? :) Maybe we should just rule this out... > > The difficulty I'm hitting is choosing the precedence. Suppose we have: > > {'id': 1, linkname: 'project-1'} > {'id': 2, linkname: '1'} > > and get a request "/api/1.0/project/1/". Which do we prefer? v3 will > give precedence to the project ID and return: > > {'id': 1, linkname: 'project-1'} > > but its a tad confusing. I'm fine with this as it's very much a corner case. A comment in the relevant code might be beneficial for anyone who does encounter this issue and goes looking for answers. Stephen
diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py index 6f1a375..0b180e6 100644 --- a/patchwork/tests/test_rest_api.py +++ b/patchwork/tests/test_rest_api.py @@ -59,6 +59,11 @@ class TestProjectAPI(APITestCase): self.assertEqual(status.HTTP_200_OK, resp.status_code) self.assertEqual(defaults.project.name, resp.data['name']) + # make sure we can look up by linkname + resp = self.client.get(self.api_url(resp.data['linkname'])) + self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(defaults.project.name, resp.data['name']) + def test_anonymous_writes(self): """Ensure anonymous "write" operations are rejected.""" defaults.project.save() diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py index e5a642a..e125bc1 100644 --- a/patchwork/views/rest_api.py +++ b/patchwork/views/rest_api.py @@ -113,6 +113,22 @@ class ProjectViewSet(PatchworkViewSet): permission_classes = (PatchworkPermission,) serializer_class = ProjectSerializer + def retrieve(self, request, pk=None): + try: + int(pk) + except ValueError: + self.kwargs = {'linkname': pk} # try and lookup by linkname + self.lookup_field = 'linkname' + return super(ProjectViewSet, self).retrieve(request, pk) + + def partial_update(self, request, pk=None): + try: + int(pk) + except ValueError: + self.kwargs = {'linkname': pk} # try and lookup by linkname + self.lookup_field = 'linkname' + return super(ProjectViewSet, self).partial_update(request, pk) + class ChecksViewSet(PatchworkViewSet): serializer_class = ChecksSerializer
Building a user-friendly CLI becomes difficult when project-ids are required. It also makes it almost impossible to work with the current format of the .pwclientrc file. Signed-off-by: Andy Doan <andy.doan@linaro.org> --- patchwork/tests/test_rest_api.py | 5 +++++ patchwork/views/rest_api.py | 16 ++++++++++++++++ 2 files changed, 21 insertions(+)