[v3,03/16] REST: Remove '_url' suffixes

This was a design decision made when implementing the REST API. The
idea was that these items were URLs to related objects and should be
indicated as such. However, this was a faulty assumption as the
Patchwork API, unlike other some other APIs (GitHub), does not also
include a full representation of said objects, like so:

    {
      "url": "http://localhost:8000/api/1.0/patches/1/",
      ...
      "delegate_url": "http://localhost:8000/api/1.0/users/1",
      "delegate": {
        "url": "http://localhost:8000/api/1.0/users/1/",
        "username": "admin",
        "first_name": "",
        "last_name": "",
        "email": ""
      }
    }

Since there is no intention to support this design yet, there isn't
really any reason to fight django-rest-framework in appending these
suffixes. Simply remove them. This changes the output for most endpoints
from something like this:

    {
      "url": "http://localhost:8000/api/1.0/patches/1",
      ...
      "delegate_url": "http://localhost:8000/api/1.0/users/1"
    }

to:

    {
      "url": "http://localhost:8000/api/1.0/patches/1",
      ...
      "delegate": "http://localhost:8000/api/1.0/users/1"
    }

This will affect all endpoints with nested resources.

Note that the API version is not bumped as the API is still considered
unreleased.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Reviewed-by: Andy Doan <andy.doan@linaro.org>
---
 patchwork/api/base.py            | 13 -------------
 patchwork/api/check.py           |  1 -
 patchwork/api/patch.py           | 10 +++++-----
 patchwork/api/person.py          |  5 +++--
 patchwork/tests/test_rest_api.py | 10 +++++-----
 5 files changed, 13 insertions(+), 26 deletions(-)

Stephen Finucane <stephen@that.guru> writes:

> This was a design decision made when implementing the REST API. The
> idea was that these items were URLs to related objects and should be
> indicated as such. However, this was a faulty assumption as the
> Patchwork API, unlike other some other APIs (GitHub), does not also
> include a full representation of said objects, like so:
>
>     {
>       "url": "http://localhost:8000/api/1.0/patches/1/",
>       ...
>       "delegate_url": "http://localhost:8000/api/1.0/users/1",
>       "delegate": {
>         "url": "http://localhost:8000/api/1.0/users/1/",
>         "username": "admin",
>         "first_name": "",
>         "last_name": "",
>         "email": ""
>       }
>     }
>
> Since there is no intention to support this design yet, there isn't
> really any reason to fight django-rest-framework in appending these
> suffixes. Simply remove them. This changes the output for most endpoints
> from something like this:
>
>     {
>       "url": "http://localhost:8000/api/1.0/patches/1",
>       ...
>       "delegate_url": "http://localhost:8000/api/1.0/users/1"
>     }
>
> to:
>
>     {
>       "url": "http://localhost:8000/api/1.0/patches/1",
>       ...
>       "delegate": "http://localhost:8000/api/1.0/users/1"
>     }


Thanks!

Reviewed-by: Daniel Axtens <dja@axtens.net>

Regards,
Daniel

>
> This will affect all endpoints with nested resources.
>
> Note that the API version is not bumped as the API is still considered
> unreleased.
>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> Reviewed-by: Andy Doan <andy.doan@linaro.org>
> ---
>  patchwork/api/base.py            | 13 -------------
>  patchwork/api/check.py           |  1 -
>  patchwork/api/patch.py           | 10 +++++-----
>  patchwork/api/person.py          |  5 +++--
>  patchwork/tests/test_rest_api.py | 10 +++++-----
>  5 files changed, 13 insertions(+), 26 deletions(-)
>
> diff --git a/patchwork/api/base.py b/patchwork/api/base.py
> index 7333a7f..3639ebe 100644
> --- a/patchwork/api/base.py
> +++ b/patchwork/api/base.py
> @@ -21,22 +21,9 @@ from django.conf import settings
>  from rest_framework import permissions
>  from rest_framework.pagination import PageNumberPagination
>  from rest_framework.response import Response
> -from rest_framework.serializers import HyperlinkedModelSerializer
> -from rest_framework.serializers import HyperlinkedRelatedField
>  from rest_framework.viewsets import ModelViewSet
>  
>  
> -class URLSerializer(HyperlinkedModelSerializer):
> -    """Just like parent but puts _url for fields"""
> -
> -    def to_representation(self, instance):
> -        data = super(URLSerializer, self).to_representation(instance)
> -        for name, field in self.fields.items():
> -            if isinstance(field, HyperlinkedRelatedField) and name != 'url':
> -                data[name + '_url'] = data.pop(name)
> -        return data
> -
> -
>  class LinkHeaderPagination(PageNumberPagination):
>      """Provide pagination based on rfc5988.
>  
> diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> index 31ade07..26a2595 100644
> --- a/patchwork/api/check.py
> +++ b/patchwork/api/check.py
> @@ -58,7 +58,6 @@ class CheckSerializer(ModelSerializer):
>          url = self.context['request'].build_absolute_uri(reverse(
>              'api_1.0:patch-detail', args=[instance.patch.id]))
>          data['url'] = url + 'checks/%s/' % instance.id
> -        data['users_url'] = data.pop('user')
>          return data
>  
>      class Meta:
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index c36a11b..2dc77d1 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -19,12 +19,12 @@
>  
>  import email.parser
>  
> +from rest_framework.serializers import HyperlinkedModelSerializer
>  from rest_framework.serializers import ListSerializer
>  from rest_framework.serializers import SerializerMethodField
>  
>  from patchwork.api.base import PatchworkPermission
>  from patchwork.api.base import PatchworkViewSet
> -from patchwork.api.base import URLSerializer
>  from patchwork.models import Patch
>  
>  
> @@ -37,8 +37,8 @@ class PatchListSerializer(ListSerializer):
>          return super(PatchListSerializer, self).to_representation(data)
>  
>  
> -class PatchSerializer(URLSerializer):
> -    mbox_url = SerializerMethodField()
> +class PatchSerializer(HyperlinkedModelSerializer):
> +    mbox = SerializerMethodField()
>      state = SerializerMethodField()
>  
>      class Meta:
> @@ -53,13 +53,13 @@ class PatchSerializer(URLSerializer):
>      def get_state(self, obj):
>          return obj.state.name
>  
> -    def get_mbox_url(self, patch):
> +    def get_mbox(self, patch):
>          request = self.context.get('request', None)
>          return request.build_absolute_uri(patch.get_mbox_url())
>  
>      def to_representation(self, instance):
>          data = super(PatchSerializer, self).to_representation(instance)
> -        data['checks_url'] = data['url'] + 'checks/'
> +        data['checks'] = data['url'] + 'checks/'
>          data['check'] = instance.combined_check_state
>          headers = data.get('headers')
>          if headers is not None:
> diff --git a/patchwork/api/person.py b/patchwork/api/person.py
> index b1168f7..758b21d 100644
> --- a/patchwork/api/person.py
> +++ b/patchwork/api/person.py
> @@ -17,13 +17,14 @@
>  # along with Patchwork; if not, write to the Free Software
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
> +from rest_framework.serializers import HyperlinkedModelSerializer
> +
>  from patchwork.api.base import AuthenticatedReadOnly
>  from patchwork.api.base import PatchworkViewSet
> -from patchwork.api.base import URLSerializer
>  from patchwork.models import Person
>  
>  
> -class PersonSerializer(URLSerializer):
> +class PersonSerializer(HyperlinkedModelSerializer):
>      class Meta:
>          model = Person
>          fields = ('email', 'name', 'user')
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index 44a2859..3be8ecf 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -175,7 +175,7 @@ class TestPersonAPI(APITestCase):
>          self.assertEqual(1, len(resp.data))
>          self.assertEqual(user.username, resp.data[0]['name'])
>          self.assertEqual(user.email, resp.data[0]['email'])
> -        self.assertIn('users/%d/' % user.id, resp.data[0]['user_url'])
> +        self.assertIn('users/%d/' % user.id, resp.data[0]['user'])
>  
>      def test_unlinked_user(self):
>          person = create_person()
> @@ -187,7 +187,7 @@ class TestPersonAPI(APITestCase):
>          self.assertEqual(2, len(resp.data))
>          self.assertEqual(person.name,
>                           resp.data[0]['name'])
> -        self.assertIsNone(resp.data[0]['user_url'])
> +        self.assertIsNone(resp.data[0]['user'])
>  
>      def test_readonly(self):
>          user = create_maintainer()
> @@ -291,13 +291,13 @@ class TestPatchAPI(APITestCase):
>          self.assertEqual(status.HTTP_200_OK, resp.status_code)
>          self.assertEqual(patch.name, resp.data['name'])
>          self.assertIn(TestProjectAPI.api_url(patch.project.id),
> -                      resp.data['project_url'])
> +                      resp.data['project'])
>          self.assertEqual(patch.msgid, resp.data['msgid'])
>          self.assertEqual(patch.diff, resp.data['diff'])
>          self.assertIn(TestPersonAPI.api_url(patch.submitter.id),
> -                      resp.data['submitter_url'])
> +                      resp.data['submitter'])
>          self.assertEqual(patch.state.name, resp.data['state'])
> -        self.assertIn(patch.get_mbox_url(), resp.data['mbox_url'])
> +        self.assertIn(patch.get_mbox_url(), resp.data['mbox'])
>  
>      def test_detail_tags(self):
>          patch = create_patch(
> -- 
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork