Search code, repositories, users, issues, pull requests...

Copy link

Member

sliverc commented Jul 18, 2018 •

edited

Loading

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.

Copy link

Contributor Author

Anton-Shutik commented Jul 18, 2018

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

Copy link

Member

sliverc commented Jul 18, 2018

@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 sliverc self-requested a review

July 18, 2018 10:13

Continue to review full report at Codecov.

Copy link

Member

sliverc commented Jul 18, 2018

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.

@Anton-Shutik Anton-Shutik force-pushed the links-mixin branch 12 times, most recently from 3cf26c8 to 4eb75b3 Compare

July 18, 2018 15:10

@codecov-io

Copy link

codecov-io commented Jul 18, 2018 •

edited

Loading

Codecov Report

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

Impacted file tree graph

@@ 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

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

Copy link

Contributor Author

Anton-Shutik commented Jul 18, 2018

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

@Anton-Shutik Anton-Shutik force-pushed the links-mixin branch from 96fadd3 to 4e2000e Compare

July 18, 2018 15:57

sliverc

sliverc requested changes

View reviewed changes

Copy link

Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

docs/usage.md Outdated

#### HyperLinkedRelatedField

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

Copy link

Member

@sliverc sliverc Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should also mention lower payload here?

Copy link

Contributor Author

@Anton-Shutik Anton-Shutik Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

rest_framework_json_api/relations.py Outdated

return return_data

class HyperLinkedRelatedField(HyperLinkedMixin, SkipDataMixin, RelatedField):

Copy link

Member

@sliverc sliverc Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@Anton-Shutik Anton-Shutik Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

example/tests/test_relations.py Outdated

def get_blog(self, obj):

return obj.blog

def get_authors(self, obj):

Copy link

Member

@sliverc sliverc Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link

Member

@sliverc sliverc Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

example/views.py Outdated

# Handle featured

entry_pk = self.kwargs.get('entry_pk', None)

if entry_pk is not None:

return Entry.objects.first()

Copy link

Member

@sliverc sliverc Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Contributor Author

Anton-Shutik commented Jul 19, 2018

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

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 Anton-Shutik force-pushed the links-mixin branch from 157d878 to 8210c5e Compare

July 19, 2018 12:13

@sliverc sliverc changed the title ~~(削除) Added SkipDataMixin, HyperLinkedMixin (削除ここまで)~~ (追記) Add HyperlinkedRelatedField and SerializerMethodHyperlinkedRelatedField fields (追記ここまで)

sliverc

sliverc requested changes

View reviewed changes

Copy link

Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change and we are ready for merging.

docs/usage.md Outdated

return queryset

```

#### HyperLinkedRelatedField

Copy link

Member

@sliverc sliverc Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 Anton-Shutik force-pushed the links-mixin branch from 8210c5e to f433b70 Compare

July 19, 2018 12:27

Copy link

Contributor Author

Anton-Shutik commented Jul 19, 2018

@sliverc updated. What about release date ?
Thanks

Anton-Shutik added 2 commits

July 19, 2018 15:33


 Added more tests

802b0b6


 Updated change log, added myself to authors

3e5f0a8

@Anton-Shutik Anton-Shutik force-pushed the links-mixin branch from f433b70 to 3e5f0a8 Compare

July 19, 2018 12:33


 Clarify docs of HyperlinkedResourceRelatedField

5cb5c82

sliverc

sliverc approved these changes

View reviewed changes

@sliverc sliverc merged commit cb7f830 into django-json-api:master

Copy link

Member

sliverc commented Jul 19, 2018

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.

@Anton-Shutik Anton-Shutik deleted the links-mixin branch

July 19, 2018 14:51

@jasonm

Copy link

jasonm commented Jul 28, 2018

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 !