Add HyperlinkedRelatedField and SerializerMethodHyperlinkedRelatedField fields #445

Anton-Shutik · 2018-07-17T09:53:13Z

Fixes #296

Description of the Change

Include only links in relationships objects by introducing HyperlinkedRelatedField and SerializerMethodHyperlinkedRelatedField.

Checklist

PR only contains one change (considered splitting up PR)
unit-test added
documentation updated
changelog entry added to CHANGELOG.md
author name in AUTHORS

sliverc · 2018-07-18T08:57:26Z

Thanks for looking into this. This approach is more DRF like so I prefer it over #443. I hoped though that HyperlinkRelatedField doesn't execute the query itself. But when I looked at the DRF code it seems it still needs SkipField workaround.

I have a feeling that this is actually an issue in DRF itself as HyperlinkRelatedField does execute a query for m2m relations even though it doesn't need it. If it is fixed there we could adjust this PR accordingly.

So what I suggest before we move forward is to investigate whether the same problem with executing of queries also exist in DRF HyperlinkRelatedField itself. If yes we open an issue in DRF and see what the feedback is. Once we have feedback I think we better now how to adjust this PR accordingly.

Would be great if you would have time to follow up on this investigation but also understand when you can't. I am also interested in a fix for this but I am not sure when I will have time to get to it.

Anton-Shutik · 2018-07-18T10:06:07Z

@sliverc I think DRF's HyperLinkRelatedField works correct with many=True
Let's see the docs, tracks field:

{
    'album_name': 'Graceland',
    'artist': 'Paul Simon',
    'tracks': [
        'http://www.example.com/api/tracks/45/',
        'http://www.example.com/api/tracks/46/',
        'http://www.example.com/api/tracks/47/',
        ...
    ]
}

It generates one link per each item in the list, but not one link for related collection, like we do in json-api.
So, I think we have to implement custom logic for this with ManyRelatedFieldWithNoData.

Are there other things you're confused with ?

sliverc · 2018-07-18T10:13:20Z

@Anton-Shutik
This makes total sense. Thanks for the follow up. Let's do it the way you have proposed it then it this PR. Could you follow up with tests, docs CI fixes in this PR?
Once done I would do a more detailed review of the code.

sliverc · 2018-07-18T13:12:39Z

Just on another note why I prefer this to render_data flag.

I can simply overwrite serializer_related_field like:

class MySerializer(ModelSerializer):
  serializer_related_field = HyperlinkRelatedField

this way all related fields are only represented as links without defining each one again in the serializer.

codecov-io · 2018-07-18T15:18:37Z

Codecov Report

Merging #445 into master will increase coverage by 0.41%.
The diff coverage is 96.51%.

@@            Coverage Diff            @@
##           master    #445      +/-   ##
=========================================
+ Coverage   92.68%   93.1%   +0.41%     
=========================================
  Files          54      54              
  Lines        2858    3000     +142     
=========================================
+ Hits         2649    2793     +144     
+ Misses        209     207       -2

Impacted Files	Coverage Δ
example/tests/integration/test_pagination.py	`100% <ø> (ø)`	⬆️
.../tests/integration/test_non_paginated_responses.py	`100% <ø> (ø)`	⬆️
example/urls_test.py	`100% <ø> (ø)`	⬆️
rest_framework_json_api/renderers.py	`86% <100%> (+0.67%)`	⬆️
example/tests/test_views.py	`100% <100%> (ø)`	⬆️
example/serializers.py	`100% <100%> (ø)`	⬆️
example/tests/test_relations.py	`100% <100%> (ø)`	⬆️
rest_framework_json_api/relations.py	`85.14% <89.13%> (+2.19%)`	⬆️
example/views.py	`91.89% <93.33%> (+0.36%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e33df0a...5cb5c82. Read the comment docs.

Anton-Shutik · 2018-07-18T15:27:46Z

@sliverc added test, updated the docs. Could you pls do a review ?

sliverc

A few small changes needed and this should be ready for merging (see inline comments).

Also changelog and authors files should be updated as well.

sliverc · 2018-07-19T06:37:26Z

docs/usage.md

@@ -427,6 +429,12 @@ class LineItemViewSet(viewsets.ModelViewSet):
        return queryset
 ```

+#### HyperLinkedRelatedField
+
+In order to improve performance by saving some sql queries we can skip `data`


Maybe we should also mention lower payload here?

sliverc · 2018-07-19T06:59:45Z

rest_framework_json_api/relations.py

@@ -140,6 +125,78 @@ def get_links(self, obj=None, lookup_field='pk'):
            return_data.update({'related': related_link})
        return return_data

+
+class HyperLinkedRelatedField(HyperLinkedMixin, SkipDataMixin, RelatedField):


In DRF it is called HyperlinkedRelatedField with a lower case linked. For consistency we should also stick to the same naming. This counts for all occurenced of HyperLinked changing to Hyperlinked.

sliverc · 2018-07-19T07:23:43Z

example/tests/test_relations.py

+    def get_blog(self, obj):
+        return obj.blog
+
+    def get_authors(self, obj):


shouldn't this be called get_comments? It would be good to expand the test so get_blog and get_comments is covered as well.

sliverc · 2018-07-19T07:26:09Z

example/views.py

+    def get_object(self):
+        entry_pk = self.kwargs.get('entry_pk', None)
+        if entry_pk is not None:
+            return Entry.objects.get(id=entry_pk).blog


This test code is actually not covered by a test. Can you expand your test accordingly?

sliverc · 2018-07-19T07:26:51Z

example/views.py

+        # Handle featured
+        entry_pk = self.kwargs.get('entry_pk', None)
+        if entry_pk is not None:
+            return Entry.objects.first()


Simply returning first entry seems strange to me. It would also be good to add a test which actually covers this though.

Anton-Shutik · 2018-07-19T11:27:49Z

@sliverc I did requested changes, would you mind taking another look ?
And when it is going to released ?

sliverc

One small change and we are ready for merging.

sliverc · 2018-07-19T12:17:25Z

docs/usage.md

@@ -435,6 +437,12 @@ class LineItemViewSet(viewsets.ModelViewSet):
        return queryset
 ```

+#### HyperLinkedRelatedField


Documentation needs to be updated to use the lowercase linked version as well.

Anton-Shutik · 2018-07-19T12:28:51Z

@sliverc updated. What about release date ?
Thanks

sliverc · 2018-07-19T13:10:18Z

Merged. Thanks for your effort. In terms of release date: we do not have a fix release schedule but try to release often but not after every PR.

jasonm · 2018-07-28T18:26:09Z

What good timing - we were looking for exactly this today! (Use case of avoiding N+1 API queries from DS.JSONAPIAdapter) Thanks for the contribution @Anton-Shutik and merging @sliverc !

Anton-Shutik · 2018-07-30T07:05:12Z

@jasonm Enjoy! :)

benjaminlong · 2019-02-27T12:41:16Z

Thanks @Anton-Shutik ! Just integrated your work, it was smooth. Love it !

Anton-Shutik mentioned this pull request Jul 17, 2018

"Data" key optional in relationships #443

Closed

5 tasks

sliverc self-requested a review July 18, 2018 10:13

Anton-Shutik force-pushed the links-mixin branch 12 times, most recently from 3cf26c8 to 4eb75b3 Compare July 18, 2018 15:10

Anton-Shutik force-pushed the links-mixin branch from 96fadd3 to 4e2000e Compare July 18, 2018 15:57

sliverc requested changes Jul 19, 2018

View reviewed changes

Anton-Shutik added 4 commits July 19, 2018 15:08

Added SkipDataMixin, HyperLinkedMixin

3127cf8

Added HyperLinked fields to example

af5e222

Added tests

659ea97

Fixed failing tests

42a9448

Anton-Shutik force-pushed the links-mixin branch from 157d878 to 8210c5e Compare July 19, 2018 12:13

sliverc changed the title ~~Added SkipDataMixin, HyperLinkedMixin~~ Add HyperlinkedRelatedField and SerializerMethodHyperlinkedRelatedField fields Jul 19, 2018

sliverc requested changes Jul 19, 2018

View reviewed changes

Anton-Shutik added 4 commits July 19, 2018 15:26

Added docs

ca65729

Added few words to docs

80b819a

Renamed HyperLinked to Hyperlinked

69e1a22

Renamed get_authors to get_comments, added tests

4fc3047

Anton-Shutik force-pushed the links-mixin branch from 8210c5e to f433b70 Compare July 19, 2018 12:27

Anton-Shutik added 2 commits July 19, 2018 15:33

Added more tests

802b0b6

Updated change log, added myself to authors

3e5f0a8

Anton-Shutik force-pushed the links-mixin branch from f433b70 to 3e5f0a8 Compare July 19, 2018 12:33

Clarify docs of HyperlinkedResourceRelatedField

5cb5c82

sliverc approved these changes Jul 19, 2018

View reviewed changes

sliverc merged commit cb7f830 into django-json-api:master Jul 19, 2018

Anton-Shutik deleted the links-mixin branch July 19, 2018 14:51

n2ygk mentioned this pull request Aug 10, 2018

Example app name conflicts #454

Closed

n2ygk added this to the 2.6.0 milestone Sep 18, 2018

This was referenced Nov 13, 2019

Pagination of nested relationships #178

Closed

proposal: DJA DefaultRouter #467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HyperlinkedRelatedField and SerializerMethodHyperlinkedRelatedField fields #445

Add HyperlinkedRelatedField and SerializerMethodHyperlinkedRelatedField fields #445

Anton-Shutik commented Jul 17, 2018 •

edited by sliverc

Loading

sliverc commented Jul 18, 2018 •

edited

Loading

Anton-Shutik commented Jul 18, 2018

sliverc commented Jul 18, 2018

sliverc commented Jul 18, 2018

codecov-io commented Jul 18, 2018 •

edited

Loading

Anton-Shutik commented Jul 18, 2018

sliverc left a comment

sliverc Jul 19, 2018

Anton-Shutik Jul 19, 2018

sliverc Jul 19, 2018

Anton-Shutik Jul 19, 2018

sliverc Jul 19, 2018

sliverc Jul 19, 2018

sliverc Jul 19, 2018

Anton-Shutik commented Jul 19, 2018

sliverc left a comment

sliverc Jul 19, 2018

Anton-Shutik commented Jul 19, 2018

sliverc commented Jul 19, 2018

jasonm commented Jul 28, 2018

Anton-Shutik commented Jul 30, 2018

benjaminlong commented Feb 27, 2019

Add HyperlinkedRelatedField and SerializerMethodHyperlinkedRelatedField fields #445

Add HyperlinkedRelatedField and SerializerMethodHyperlinkedRelatedField fields #445

Conversation

Anton-Shutik commented Jul 17, 2018 • edited by sliverc Loading

Description of the Change

Checklist

sliverc commented Jul 18, 2018 • edited Loading

Anton-Shutik commented Jul 18, 2018

sliverc commented Jul 18, 2018

sliverc commented Jul 18, 2018

codecov-io commented Jul 18, 2018 • edited Loading

Codecov Report

Anton-Shutik commented Jul 18, 2018

sliverc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anton-Shutik commented Jul 19, 2018

sliverc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anton-Shutik commented Jul 19, 2018

sliverc commented Jul 19, 2018

jasonm commented Jul 28, 2018

Anton-Shutik commented Jul 30, 2018

benjaminlong commented Feb 27, 2019

Anton-Shutik commented Jul 17, 2018 •

edited by sliverc

Loading

sliverc commented Jul 18, 2018 •

edited

Loading

codecov-io commented Jul 18, 2018 •

edited

Loading