-
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #451 +/- ##
==========================================
+ Coverage 93.1% 93.24% +0.14%
==========================================
Files 54 54
Lines 3000 3123 +123
==========================================
+ Hits 2793 2912 +119
- Misses 207 211 +4
Continue to review full report at Codecov.
|
@Anton-Shutik Thanks for your PR. I hope I will get to review it in the next few days. |
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.
Thanks for working on this. See my inline comments.
I guess you will follow up on the other points of the check list (tests, doc, etc.) once we agree on the basic structure.
rest_framework_json_api/views.py
Outdated
""" | ||
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 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?
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 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.
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.
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 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.
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.
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?
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.
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 ? :)
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.
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.
rest_framework_json_api/views.py
Outdated
This mixin handles all related entities, whose Serializers are declared in "related_serializers" | ||
""" | ||
related_serializers = {} | ||
field_name_mapping = {} |
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.
what's the use case that this mapping is needed?
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.
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'}
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.
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?
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.
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 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?
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.
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.
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.
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 :)
rest_framework_json_api/views.py
Outdated
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 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?
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 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.
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.
fair enough, makes sense.
rest_framework_json_api/views.py
Outdated
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 comment
The 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 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
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.
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
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.
that would be more user friendly and a good improvement.
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.
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 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.
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 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 ?
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.
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.
rest_framework_json_api/relations.py
Outdated
@@ -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': |
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:
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'),
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.
rest_framework_json_api/views.py
Outdated
@@ -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 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.
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.
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.
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 think so too so let's extend ModelViewSet
and ReadOnlyViewSet
with this mixin
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.
Done
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.
Great.
@sliverc thanks for you comments. So, changes I'm going to do:
How that works for you ? |
@Anton-Shutik |
@sliverc Answered and addressed your comments. |
@Anton-Shutik see my comments above |
@sliverc Answered/addressed your notes, thank you. For now I see 1 unresolved place here: How do we resolve model's related field name? Options:
class AuthorSerializer():
featured_post = SerializerMethodRelatedField (source='get_featured_post') That way we have to declare I would give my 80% for 1st approach, and only 20% for the second. How do you think which one is better ? May be we need somebody's else opinion ? |
I really like the idea of So I think to find a solution how to retrieve relations will be crucial part of this feature. My first idea doesn't work in case of From a DJA user perspective I think relations are defined on the serializer - meaning we should also take it from there. Most likely the only way to implement this would be to instantiate the parent serializer and serialize the given model. Then ready the relation from serializer.data. Something like this: instance = self.get_object()
serializer = self.get_serializer(instance)
serializer.data['name-of-relation'] I haven't tried so this is just an idea. But if it works don't you think that is the much cleaner way to make sure that relations on serializer are exposed in the api with the same name? |
I think this is overkill. You need to serialize parent instance, just to get one related entity. What if parent serializer contains 20+ relations ? It will serialize all of them, but actually we will use only one. That might just hammer the database. serializer.data['name-of-relation'] will give us just json representing relation itself (say, object identifiers, related urls), but not entire related entity. Not tried, but almost sure that it will exactly like that. ps. small question Thanks |
@sliverc Did some improves, could you pls take a look ? It will be easier to review on per commit basis. Let me know if this version looks like final and I will start writing tests and docs. Thanks |
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.
Great work! See my inline comments above but in general two more things I see to be done:
- move
related_serializers
toSerializer
- remove
related_field_mapping
Otherwise it would be great if you could follow up on the docs and tests.
rest_framework_json_api/views.py
Outdated
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 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:
- related fields are defined on the serializer not the view so to define
related_serializers
on serializer feels more natural to me - 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. - 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 forModelSerializer
in the future as well.
What do you think?
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.
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.
rest_framework_json_api/views.py
Outdated
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 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.
Ahh yeah. I forgot DRY stands for don't repeat yourself. |
@sliverc done + added tests. Will add docs later. If you have a time, feel free to write some docs |
@sliverc added docs, updated changelog. Anything else required for merge ? |
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.
This looks good. Two small comments and I think we are ready to merge it.
rest_framework_json_api/views.py
Outdated
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 comment
The 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
@@ -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 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 the RelatedMixin
.
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.
@sliverc done |
@sliverc any more updates required in order to merge 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.
See my comments above.
Beside small changes I think the documentation needs to be improved. I am happy to work on this not to get too many back and forth but this will take some time till I will be able to get to it.
@n2ygk Could you also have a look at the documentation and maybe comment whether this looks good to you or what would be missing?
Thanks.
@@ -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 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 the RelatedMixin
.
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.
example/tests/test_views.py
Outdated
from example.models import Author, Blog, Comment, Entry | ||
from example.serializers import AuthorBioSerializer, EntrySerializer, AuthorTypeSerializer |
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.
This causes an isort error and should be fixed.
docs/usage.md
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be order-related?
docs/usage.md
Outdated
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be order-related?
docs/usage.md
Outdated
customer = ResourceRelatedField( | ||
queryset=Customer.objects, | ||
related_link_view_name='order-customer-detail', | ||
self_link_view_name='order-relationships' |
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.
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.
@sliverc Fixed imports, updated docs. If you have better idea about the docs - feel free to update it as you wish. |
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.
@Anton-Shutik: I've briefly looked at your documentation and example code and reading it made a lot of things "click" that I'd personally had trouble understanding when first trying to implement a comprehensive jsonapi app (with relationships, related, links, and so on). The example was not complete enough then but your improvements have changed that.
When I get time, I will refactor my "training" DJA app to use this new stuff and may have documentation improvements then, but from a first read, probably not.
This is great stuff. Thanks!
@sliverc I think this is ready to merge.
Then let's merge this and improve the documentation when needed as we gain more experience when using it. |
great! thanks! Are you going to release it on pypi ? |
Fixes #450
Fixes #426
Description of the Change
Add
RelatedMixin
. This introduces new approach in handling related urls/entities.RelatedMixin
will handle all related entities, configured inrelated_serializers
dict with no related views required. Also it will check permissions for parent object for any related entity.Checklist
CHANGELOG.md
AUTHORS