-
Notifications
You must be signed in to change notification settings - Fork 300
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 2 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 ( | ||
|
@@ -98,6 +100,54 @@ def get_queryset(self, *args, **kwargs): | |
return qs | ||
|
||
|
||
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. |
||
field_name_mapping = {} | ||
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. what's the use case that this mapping is needed? 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 is used when url field_name_mapping = {'biography': '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. Is this not the same which can already be done in the serializer 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. 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. 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. 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? 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.
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 ?
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. 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. One more idea :) # 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 | ||
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 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 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 it is correct. What if we go to http://jsonapi.org/format/#fetching-relationships-responses-404 Spec does not describe exactly our case, but I think 404 is correct behavior. 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. 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 | ||
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. same here as above. 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. If there is no attribute on model - return 404. This is similar behavior to the answer above 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. Or probably we can do like: 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 would be more user friendly and a good improvement. 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. 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. 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 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. 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.
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 ? 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. 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 | ||
|
||
|
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.