Skip to content

Add support for related links using parent view and its permissions #451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Aug 17, 2018
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion example/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,35 @@ class Meta:


class AuthorSerializer(serializers.ModelSerializer):
bio = relations.ResourceRelatedField(
related_link_view_name='author-related',
self_link_view_name='author-relationships',
queryset=AuthorBio.objects,
)
entries = relations.ResourceRelatedField(
related_link_view_name='author-related',
self_link_view_name='author-relationships',
queryset=Entry.objects,
many=True
)
first_entry = relations.SerializerMethodResourceRelatedField(
related_link_view_name='author-related',
self_link_view_name='author-relationships',
model=Entry,
read_only=True,
source='get_first_entry'
)
included_serializers = {
'bio': AuthorBioSerializer,
'type': AuthorTypeSerializer
}

class Meta:
model = Author
fields = ('name', 'email', 'bio', 'entries', 'type')
fields = ('name', 'email', 'bio', 'entries', 'first_entry', 'type')

def get_first_entry(self, obj):
return obj.entries.first()


class WriterSerializer(serializers.ModelSerializer):
Expand Down
4 changes: 4 additions & 0 deletions example/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@
EntryViewSet.as_view({'get': 'retrieve'}),
name='entry-featured'),

url(r'^authors/(?P<pk>[^/.]+)/(?P<related_field>\w+)/$',
AuthorViewSet.as_view({'get': 'retrieve_related'}),
name='author-related'),

url(r'^entries/(?P<pk>[^/.]+)/relationships/(?P<related_field>\w+)',
EntryRelationshipView.as_view(),
name='entry-relationships'),
Expand Down
4 changes: 4 additions & 0 deletions example/urls_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@
EntryViewSet.as_view({'get': 'retrieve'}),
name='entry-featured'),

url(r'^authors/(?P<pk>[^/.]+)/(?P<related_field>\w+)/$',
AuthorViewSet.as_view({'get': 'retrieve_related'}),
name='author-related'),

url(r'^entries/(?P<pk>[^/.]+)/relationships/(?P<related_field>\w+)',
EntryRelationshipView.as_view(),
name='entry-relationships'),
Expand Down
5 changes: 5 additions & 0 deletions example/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@ class NonPaginatedEntryViewSet(EntryViewSet):
class AuthorViewSet(ModelViewSet):
queryset = Author.objects.all()
serializer_class = AuthorSerializer
related_serializers = {
'bio': 'example.serializers.AuthorBioSerializer',
'entries': 'example.serializers.EntrySerializer',
'first_entry': 'example.serializers.EntrySerializer'
}


class CommentViewSet(ModelViewSet):
Expand Down
14 changes: 13 additions & 1 deletion rest_framework_json_api/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,19 @@ def get_links(self, obj=None, lookup_field='pk'):
})
self_link = self.get_url('self', self.self_link_view_name, self_kwargs, request)

related_kwargs = {self.related_link_url_kwarg: kwargs[self.related_link_lookup_field]}
"""
Assuming RelatedField will be declared in two ways:
1. url(r'^authors/(?P<pk>[^/.]+)/(?P<related_field>\w+)/$',
AuthorViewSet.as_view({'get': 'retrieve_related'}))
2. url(r'^authors/(?P<author_pk>[^/.]+)/bio/$',
AuthorBioViewSet.as_view({'get': 'retrieve'}))
So, if related_link_url_kwarg == 'pk' it will add 'related_field' parameter to reverse()
"""
if self.related_link_url_kwarg == 'pk':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have more insight into generation of this links. Can you briefly explain why this is needed? In what case would this if match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah!
The if would match in any case you didn't pass the argument when declaring a field, or the argument value equals 'pk'.

When related_link_url_kwarg == 'pk' we can suppose the url was declared like this:

url(r'^authors/(?P<pk>[^/.]+)/(?P<related_field>\w+)/$',
        AuthorViewSet.as_view({'get': 'retrieve_related'}),
        name='author-related'),

and the route has 2 parameters: pk and related_field. Exactly like self_link. That is why we do: related_kwargs = self_kwargs in the next line.

In any other cases (related_link_url_kwarg != 'pk') we assume that we have "old-style" urls declaration like this:

url(r'^authors/(?P<author_pk>[^/.]+)/bio/$',
        AuthorBioViewSet.as_view({'get': 'retrieve'}),
        name='author-bio'),

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. What do you think about adding a small doc string explaining this?

Also do you think this could break backwards compatibility in case someone used pk in its related link? Although documentation states differently I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your added docstring looks good. Thanks.

related_kwargs = self_kwargs
else:
related_kwargs = {self.related_link_url_kwarg: kwargs[self.related_link_lookup_field]}

related_link = self.get_url('related', self.related_link_view_name, related_kwargs, request)

if self_link:
Expand Down
80 changes: 79 additions & 1 deletion rest_framework_json_api/views.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from collections import Iterable

from django.core.exceptions import ImproperlyConfigured
from django.db.models import Model
from django.db.models.fields.related_descriptors import (
Expand All @@ -9,6 +11,7 @@
from django.db.models.manager import Manager
from django.db.models.query import QuerySet
from django.urls import NoReverseMatch
from django.utils.module_loading import import_string as import_class_from_dotted_path
from rest_framework import generics, viewsets
from rest_framework.exceptions import MethodNotAllowed, NotFound
from rest_framework.response import Response
Expand Down Expand Up @@ -98,12 +101,87 @@ def get_queryset(self, *args, **kwargs):
return qs


class ModelViewSet(AutoPrefetchMixin, PrefetchForIncludesHelperMixin, viewsets.ModelViewSet):
class RelatedMixin(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason we shouldn't directly derive ModelViewSet from this mixin? This would be easier to use but have to think about whether it breaks backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. That was my initial PR, so I made things as simple as possible.
The only case it might brake things is when related_field will appear in the view kwargs(see def get_serializer_class). But I don't think people will add it by accident.
This is the only method I'm overriding from base class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so too so let's extend ModelViewSet and ReadOnlyViewSet with this mixin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.

"""
This mixin handles all related entities, whose Serializers are declared in "related_serializers"
"""
related_serializers = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we not use included_serailizers on the defined serializer instead?

Copy link
Contributor Author

@Anton-Shutik Anton-Shutik Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could, but I think it will be better to keep it separate. Otherwise all related entities described here will be available via include by default, and there is no way to remove it from include list and keep in related list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be not more consistent from an api caller when a relationship can be either fetch per link and per include? An api caller can then decide what he prefers. This way it would also be DRY.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against of that, but how do I add included serializer, without adding related link ?
Let's say I want GET api/post/1/?include=author, but at the same time I want api/post/1/author/ return 404 ?

The only way I see is to use included_serializers if related_serializer is None, and use related_serializer otherwise. Also we need to declare related_serializer as None by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true this wouldn't be possible anymore to have a include parameter allowed but not a accessible relationship.
I am more wondering whether there is really use case for this where this is needed?

In the end it is the same data returned in included and as the relationship and with RelationMixin permissions are also defined by parent resource which is the same with included.
Or do you have a specific use case in mind where this could be handy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, probably I have the case. Let's say we want client to "enforce" fetching related resource from the "related" url rather then just by including it within parent resource. My example is:

{
    "data": {
        "type": "product",
        "id": "1",
        "attributes": {
            ...
        },
        "relationships": {
            "reviews": {
                "data": [
                    {
                        "type": "product-review",
                        "id": "1"
                    },
# A lot of object identifiers
                    {
                        "type": "product-review",
                        "id": "1000"
                    },
                ],
                "meta": {
                    "count": 1000
                },
                "links": {
                    "self": "api/product/1/relationships/reviews/",
                    "related": "api/product/1/reviews/"
                }
            },

Let's say I do not want to give client an ability to fetch such a huge response from api/product/1/ endpoint, because it is cached by product id for example. Of source we can use full_url (with get params like include=reviews) as cache key in the case, but who knows how do people use it.

So, what I would do here is:
Use included_serializers by default when related_serializer is None. And use related_serializers when it is not None. Such a way we give both opportunities and people can use included_serializers for both inclusion and related links. When they need different behavior - they use related_serializers dict.
Sounds like a plan ? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure on the use case :). But yeah that's a plan let's do it like you suggested. Users might come up with use case when they use it I guess.

related_field_mapping = {}

def retrieve_related(self, request, *args, **kwargs):
serializer_kwargs = {}
instance = self.get_related_instance()

if hasattr(instance, 'all'):
instance = instance.all()

if callable(instance):
instance = instance()

if instance is None:
return Response(data=None)

if isinstance(instance, Iterable):
serializer_kwargs['many'] = True

serializer = self.get_serializer(instance, **serializer_kwargs)
return Response(serializer.data)

def get_serializer_class(self):
parent_serializer_class = super(RelatedMixin, self).get_serializer_class()

if 'related_field' in self.kwargs:
field_name = self.kwargs['related_field']

assert hasattr(parent_serializer_class, 'included_serializers')\
or self.related_serializers,\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks goo to me. One more though on related_serializers.

I think we should move it to the serializer because of following reasons:

  1. related fields are defined on the serializer not the view so to define related_serializers on serializer feels more natural to me
  2. as included_serializers is also on serializer it is more consistent. User can then simply decide whether he simply wants related serializers or included serializers at the same spot.
  3. Serializers have meta classing. This is out of scope for this PR but in the future we can do import_class_from_dotted_path during loading of classes and not on each requests. PolymorphicModelSerializer already does meta classing something we should introduce for ModelSerializer in the future as well.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I do think that related urls and "calculating" related entities is not a part of serializer itself, and should be done in the RelatedMixin. But I'm also ok to do it in the serializer since it just works.

'Either "included_serializers" or ' \
'"related_serializers" should be configured'

# Try get the class from related_serializers
class_str = self.related_serializers.get(field_name, None)

if class_str is None:
# Class was not found in related_serializers, look for it in included_serializers
class_str = getattr(self, 'included_serializers', {}).get(field_name, None)

if class_str is None:
raise NotFound
return import_class_from_dotted_path(class_str)

return parent_serializer_class

def get_related_field_name(self):
field_name = self.kwargs['related_field']
if field_name in self.related_field_mapping:
return self.related_field_mapping[field_name]
return field_name

def get_related_instance(self):
parent_obj = self.get_object()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Really like it. I think we should now remove the related_field_mapping as not needed in most cases anymore.

We can still leave the method get_related_field_name so if someone wants to have its own mapping or whatever logic to get related field name they can overwrite it.

parent_serializer = self.serializer_class(parent_obj)
field_name = self.get_related_field_name()
field = parent_serializer.fields.get(field_name, None)

if field is not None:
return field.get_attribute(parent_obj)
else:
try:
return getattr(parent_obj, field_name)
except AttributeError:
raise NotFound


class ModelViewSet(AutoPrefetchMixin,
PrefetchForIncludesHelperMixin,
RelatedMixin,
viewsets.ModelViewSet):
pass


class ReadOnlyModelViewSet(AutoPrefetchMixin,
PrefetchForIncludesHelperMixin,
RelatedMixin,
viewsets.ReadOnlyModelViewSet):
pass

Expand Down