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 2 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
11 changes: 11 additions & 0 deletions example/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,17 @@ 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
)
included_serializers = {
'bio': AuthorBioSerializer,
'type': AuthorTypeSerializer
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
9 changes: 7 additions & 2 deletions example/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
import rest_framework_json_api.renderers
from rest_framework_json_api.pagination import PageNumberPagination
from rest_framework_json_api.utils import format_drf_errors
from rest_framework_json_api.views import ModelViewSet, RelationshipView
from rest_framework_json_api.views import ModelViewSet, RelatedMixin, RelationshipView

from example.models import Author, Blog, Comment, Company, Entry, Project
from example.serializers import (
AuthorBioSerializer,
AuthorSerializer,
BlogSerializer,
CommentSerializer,
Expand Down Expand Up @@ -92,9 +93,13 @@ class NonPaginatedEntryViewSet(EntryViewSet):
pagination_class = NoPagination


class AuthorViewSet(ModelViewSet):
class AuthorViewSet(RelatedMixin, ModelViewSet):
queryset = Author.objects.all()
serializer_class = AuthorSerializer
related_serializers = {
'bio': AuthorBioSerializer,
'entries': EntrySerializer
}


class CommentViewSet(ModelViewSet):
Expand Down
6 changes: 5 additions & 1 deletion rest_framework_json_api/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,11 @@ 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]}
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
50 changes: 50 additions & 0 deletions 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 Down Expand Up @@ -98,6 +100,54 @@ def get_queryset(self, *args, **kwargs):
return qs


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.

field_name_mapping = {}
Copy link
Member

Choose a reason for hiding this comment

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

what's the use case that this mapping is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is used when url related_field name differs from model field name. Let's say we have field Author.bio, but url we want is /author/1/biography/. Then we do:

field_name_mapping = {'biography': 'bio'}

Copy link
Member

Choose a reason for hiding this comment

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

Is this not the same which can already be done in the serializer with source attribute on a field? Would it be not more consistent to take the mapping information form the serializer directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tried. No. We trying to get related_attribute from model, but not from serializer, so it won't work. I can remove it, but there will be no opportunity to rename relation in the url.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure? I assume for consistency the url relation part should have the same name as the relation in the serializer right?

What we could to ensure this is get the field declaration from the serializer class, read the source attribute and use this to get the relation from the model we have access to in case it is set. This should work or not?

Copy link
Contributor Author

@Anton-Shutik Anton-Shutik Aug 9, 2018

Choose a reason for hiding this comment

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

Are you sure? I assume for consistency the url relation part should have the same name as the relation in the serializer right?

100% agree with you. But there are some cases when we need to have different name. For example Author.entry_set. It would be nicer to have url like /author/1/entries/, right ?

What we could to ensure this is get the field declaration from the serializer class, read the source attribute and use this to get the relation from the model we have access to in case it is set. This should work or not?

I tried (If you know better/nicer way to do it - pls let me know ). Looks like we have to create Serializer instance to be able to get the field. That looks more complex, but seems like it works.
What do we do when serializer does not have such a field ? Should we just return 500 or try to use related_name we got from the url ? Will it be possible to have a related link without declaring the field on the serializer ? You probably will want to deny such a case, but seems like json api spec allows it, or at least does not deny.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One more idea :)
What if related url entity is defined as SerializerMethodField (so model does not have such an attribute) on parent serializer and gets caclulated via get_featured_post (for example) method on parent serializer ? May be it is better to rewrite it this way ?

# Pseudo code
 def get_related_instance(self):
        parent_serializer = super(RelatedMixin, self).get_serializer_class()
        return parent_serializer.get_related_entity_instance()

Actually I don't like the idea, because we depend on parent serializer. Just shared the idea with you :)


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):
if 'related_field' in self.kwargs:
field_name = self.get_related_field_name()
_class = self.related_serializers.get(field_name, None)
if _class is None:
raise NotFound
Copy link
Member

Choose a reason for hiding this comment

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

this seems to be a configuration issue, so rather seems to be a 500 error to me which could contain more detail what is missing. Or do you have a specific purpose to use NotFound here?

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 think it is correct. What if we go to /author/1/unexisting_relation/ ? I think it should return 404 as the url simply does not exist. There is no such a relation on Author model.
If there is a field on a Author model, but there is no serializer declaration for that field - it will give 404 as well. That might be used for "closing/hiding" some related resources.

http://jsonapi.org/format/#fetching-relationships-responses-404

Spec does not describe exactly our case, but I think 404 is correct behavior.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough, makes sense.

return _class
return super(RelatedMixin, self).get_serializer_class()

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

def get_related_instance(self):
try:
return getattr(self.get_object(), self.get_related_field_name())
except AttributeError:
raise NotFound
Copy link
Member

Choose a reason for hiding this comment

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

same here as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is no attribute on model - return 404. This is similar behavior to the answer above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or probably we can do like:
If parent entity does not have related_attribute - return 404;
If parent has related entity, but do not have declared serializer for it - return 500 with some description

Copy link
Member

Choose a reason for hiding this comment

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

that would be more user friendly and a good improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tried to do that. But what if we want to force api/author/1/bio/ return 404 ? It looks like impossible, because it will start giving 500 all the time. So, I think we have to leave it as is.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it could be by looking up whether the related field is defined on the serializer. If yes then it would be a 500 error if it is not defined or if it is defined a 404.

However I see this might be a bit of a overkill just for an error message. Let's leave it as it is and make sure that it is well documented.

Copy link
Contributor Author

@Anton-Shutik Anton-Shutik Aug 9, 2018

Choose a reason for hiding this comment

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

I guess it could be by looking up whether the related field is defined on the serializer. If yes then it would be a 500 error if it is not defined or if it is defined a 404.

Not sure I got you :) So, if there is no attribute on a model, we check if the attribute is declared on a serializer. If it is declared - we return 500 saying smth like "Wrong config" and if it is not declared - return 404 ?

I will agree with case return 404, because in "500" case we should just get correct model attribute name from related field "source".

Well, honestly speaking I don't like the fact that RelatedMixin depends on parent's entity serializer class, like fetching field source name and so on. Probably that is unreasonable, but who knows, haha. I like keep thing as simple as possible, then its easier to support it.

So, I would keep it as is. Any minds ?

Copy link
Member

Choose a reason for hiding this comment

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

As said above I also think this is a bit of a overkill. It was just some thoughts on how it could be done.

But let's leave it as is and let's make sure the documentation is clear on it.



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

Expand Down