-
Notifications
You must be signed in to change notification settings - Fork 301
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
Changes from 15 commits
ffe61c6
196d8ba
fc52dc4
73d51a5
7677042
bde21ec
9e78e67
d9b5f08
9decc1f
77ac0b2
6e0b47c
a210e63
c148507
c0f0dab
228b1e8
8191d6d
b8a902f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -443,6 +443,50 @@ class LineItemViewSet(viewsets.ModelViewSet): | |
not render `data`. Use this in case you only need links of relationships and want to lower payload | ||
and increase performance. | ||
|
||
#### Related urls | ||
|
||
There is a nice way to handle "related" urls like `/orders/3/lineitems/` or `/orders/3/customer/`. | ||
All you need is just add to `urls.py`: | ||
```python | ||
url(r'^orders/(?P<pk>[^/.]+)/(?P<related_field>\w+)/$', | ||
OrderViewSet.as_view({'get': 'retrieve_related'}), | ||
name='order-related'), | ||
``` | ||
Make sure that RelatedField declaration has `related_link_url_kwarg='pk'` or simply skipped (will be set by default): | ||
```python | ||
line_items = ResourceRelatedField( | ||
queryset=LineItem.objects, | ||
many=True, | ||
related_link_view_name='order-lineitems-list', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be order-related? |
||
related_link_url_kwarg='pk', | ||
self_link_view_name='order_relationships' | ||
) | ||
|
||
customer = ResourceRelatedField( | ||
queryset=Customer.objects, | ||
related_link_view_name='order-customer-detail', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be order-related? |
||
self_link_view_name='order-relationships' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. above it's order_relationships and here it's order-relationships. Maybe example should be a bit more complete to also show parent view in url so it is clear what is added here. I think this kind of goes into the direction to merge old way and new way in the documentation. |
||
) | ||
``` | ||
And, the most important part - declare serializer for each related entity: | ||
```python | ||
class OrderSerializer(serializers.HyperlinkedModelSerializer): | ||
... | ||
related_serializers = { | ||
'customer': 'example.serializers.CustomerSerializer', | ||
'line_items': 'example.serializers.LineItemSerializer' | ||
} | ||
``` | ||
Or, if you already have `included_serializers` declared and your `related_serializers` look the same, just skip it: | ||
```python | ||
class OrderSerializer(serializers.HyperlinkedModelSerializer): | ||
... | ||
included_serializers = { | ||
'customer': 'example.serializers.CustomerSerializer', | ||
'line_items': 'example.serializers.LineItemSerializer' | ||
} | ||
``` | ||
|
||
### RelationshipView | ||
`rest_framework_json_api.views.RelationshipView` is used to build | ||
relationship views (see the | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah! When url(r'^authors/(?P<pk>[^/.]+)/(?P<related_field>\w+)/$',
AuthorViewSet.as_view({'get': 'retrieve_related'}),
name='author-related'), and the route has 2 parameters: In any other cases ( url(r'^authors/(?P<author_pk>[^/.]+)/bio/$',
AuthorBioViewSet.as_view({'get': 'retrieve'}),
name='author-bio'), There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
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 ( | ||
|
@@ -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 | ||
|
@@ -98,12 +101,83 @@ def get_queryset(self, *args, **kwargs): | |
return qs | ||
|
||
|
||
class ModelViewSet(AutoPrefetchMixin, PrefetchForIncludesHelperMixin, viewsets.ModelViewSet): | ||
class RelatedMixin(object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason we shouldn't directly derive There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so too so let's extend There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
""" | ||
|
||
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'] | ||
|
||
# Try get the class from related_serializers | ||
if hasattr(parent_serializer_class, 'related_serializers'): | ||
class_str = parent_serializer_class.related_serializers.get(field_name, None) | ||
if class_str is None: | ||
raise NotFound | ||
|
||
elif hasattr(parent_serializer_class, 'included_serializers'): | ||
class_str = parent_serializer_class.included_serializers.get(field_name, None) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem to be covered by a test. It would be good to add one as this is also used in a success case. https://codecov.io/gh/django-json-api/django-rest-framework-json-api/pull/451/diff#D7-140 |
||
if class_str is None: | ||
raise NotFound | ||
|
||
else: | ||
assert False, \ | ||
'Either "included_serializers" or "related_serializers" should be configured' | ||
|
||
return import_class_from_dotted_path(class_str) | ||
|
||
return parent_serializer_class | ||
|
||
def get_related_field_name(self): | ||
return self.kwargs['related_field'] | ||
|
||
def get_related_instance(self): | ||
parent_obj = self.get_object() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! Really like it. I think we should now remove the We can still leave the method |
||
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 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should combine this documentation with https://django-rest-framework-json-api.readthedocs.io/en/stable/usage.html#related-fields ?
Maybe we do not need to document overwriting of
get_queryset
anymore as this is obsolete. Or do you still see a use case for it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still working and can be used. I think we can remove the docs when we deprecate this stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just read through the documentation again and I am not sure whether it is clear why there are two different ways to basically do the same.
I think the one difference is that with the old way overwriting
get_queryset
someone can define different permissions on view which can not so easily be done with theRelatedMixin
.Somehow we should try to merge those two documentation pieces into one, recommending to use
RelatedMixin
way but still documenting old way in case of having different permissions for relations.I first was planning to work on this but I do not really have time at hand. So if you have a suggestion feel free. Otherwise I might get to it at a later point.