-
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 11 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 |
---|---|---|
@@ -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,87 @@ 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" | ||
""" | ||
related_serializers = {} | ||
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. could we not use 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 could, but I think it will be better to keep it separate. Otherwise all related entities described here will be available via 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. 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. 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'm not against of that, but how do I add The only way I see is to use 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. That's true this wouldn't be possible anymore to have a include parameter allowed but not a accessible relationship. In the end it is the same data returned in included and as the relationship and with 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. 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 So, what I would do here is: 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. 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,\ | ||
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 looks goo to me. One more though on I think we should move it to the serializer because of following reasons:
What do you think? 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. 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() | ||
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.
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 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:and the route has 2 parameters:
pk
andrelated_field
. Exactly likeself_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: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.
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.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.
Your added docstring looks good. Thanks.