could we not useincluded_serailizers on the defined serializer instead?

I could, but I think it will be better to keep it separate. Otherwise all related entities described here will be available viainclude by default, and there is no way to remove it frominclude list and keep inrelated list.

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.

I'm not against of that, but how do I addincluded 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 useincluded_serializers ifrelated_serializer is None, and userelated_serializer otherwise. Also we need to declare related_serializer as None by default.

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 withRelationMixin 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?

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 likeinclude=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 useincluded_serializers for both inclusion and related links. When they need different behavior - they use related_serializers dict.
Sounds like a plan ? :)

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.

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

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

field_name_mapping= {'biography':'bio'}

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

Just tried. No. We trying to get related_attribute frommodel, 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.

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?

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?

Itried (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.

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

# Pseudo codedefget_related_instance(self):parent_serializer=super(RelatedMixin,self).get_serializer_class()returnparent_serializer.get_related_entity_instance()

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

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 useNotFound here?

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.

fair enough, makes sense.

same here as above.

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

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

that would be more user friendly and a good improvement.

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.

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.

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 ?

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.

You have more insight into generation of this links. Can you briefly explain why this is needed? In what case would this if match?

Oh yeah!
Theif would match in any case you didn't pass the argument when declaring a field, or the argument value equals 'pk'.

Whenrelated_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 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:

url(r'^authors/(?P<author_pk>[^/.]+)/bio/$',AuthorBioViewSet.as_view({'get':'retrieve'}),name='author-bio'),

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 usedpk in its related link? Although documentation states differently I guess.

Your added docstring looks good. Thanks.

Is there any reason we shouldn't directly deriveModelViewSet from this mixin? This would be easier to use but have to think about whether it breaks backwards compatibility.

Good idea. That was my initial PR, so I made things as simple as possible.
The only case it might brake things is whenrelated_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.

I think so too so let's extendModelViewSet andReadOnlyViewSet with this mixin

Done

Great.

This looks goo to me. One more though onrelated_serializers.

I think we should move it to the serializer because of following reasons:

1. related fields are defined on the serializer not the view so to definerelated_serializers on serializer feels more natural to me
2. asincluded_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.
3. Serializers have meta classing. This is out of scope for this PR but in the future we can doimport_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?

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.

Nice! Really like it. I think we should now remove therelated_field_mapping as not needed in most cases anymore.

We can still leave the methodget_related_field_name so if someone wants to have its own mapping or whatever logic to get related field name they can overwrite it.

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

Do you think we should combine this documentation withhttps://django-rest-framework-json-api.readthedocs.io/en/stable/usage.html#related-fields ?

Maybe we do not need to document overwriting ofget_queryset anymore as this is obsolete. Or do you still see a use case for it?

It is still working and can be used. I think we can remove the docs when we deprecate this stuff.

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 overwritingget_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 useRelatedMixin 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.

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 overwritingget_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 useRelatedMixin 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.

This causes an isort error and should be fixed.

shouldn't this be order-related?

shouldn't this be order-related?

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.