Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Use REST framework serializer functionality to extract includes #632

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

Merged
sliverc merged 4 commits into django-json-api:master from Anton-Shutik:get-related-instance
Jun 4, 2019

Conversation

Copy link
Contributor

@Anton-Shutik Anton-Shutik commented May 16, 2019

Fixes #

Let's say we have code like below:

class ParentSerializer(HyperlinkedModelSerializer):
 child = json_api_relations.ResourceRelatedField(
 related_link_view_name='parent-child',
 related_link_url_kwarg='parent_pk',
 self_link_view_name='parent-relationships',
 read_only=True,
 source='intermidiate_object.child',
 model=Child
 )

Url GET api/parent/1/ works fine until I want to "include" child in the response (GET api/parent/1/?include=child). It does not add the included section, because renderer fails to calculate child property in extract_relation_instance method.
Child is not just Foreign key, it is a property of intermidiate_object that we have on the parent. I'm pretty sure we have to calculate it exactly same way we do when serializing parent serializer.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

Copy link
Member

sliverc commented May 24, 2019

I like where this PR is going. This change might not be 100% backwards compatible especially the serializer_method part. However a change like suggested in #639 could solve this.

Copy link
Member

sliverc commented May 24, 2019

😄 just looked at the SerializerMethodResourceRelatedField implementation which actually overwrites get_attribute, so your change should work. If you could add a test and changelog that would be great.

Hint: currently failing tests should be fixed with #634

Copy link

codecov bot commented May 27, 2019

Codecov Report

Merging #632 into master will decrease coverage by 0.82%.
The diff coverage is 100%.

Impacted file tree graph 

@@ Coverage Diff @@
## master #632 +/- ##
==========================================
- Coverage 96.16% 95.34% -0.83% 
==========================================
 Files 56 56 
 Lines 2866 2855 -11 
==========================================
- Hits 2756 2722 -34 
- Misses 110 133 +23
Impacted Files Coverage Δ
rest_framework_json_api/renderers.py 86.79% <100%> (-0.58%) ⬇️
rest_framework_json_api/compat.py 50% <0%> (-50%) ⬇️
rest_framework_json_api/metadata.py 25% <0%> (-23.34%) ⬇️
rest_framework_json_api/django_filters/backends.py 86.66% <0%> (-13.34%) ⬇️
example/tests/test_views.py 100% <0%> (ø) ⬆️

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 b207f99...430cd3e. Read the comment docs.

Copy link

codecov bot commented May 27, 2019 •
edited
Loading

Codecov Report

Merging #632 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph 

@@ Coverage Diff @@
## master #632 +/- ##
==========================================
- Coverage 96.16% 96.13% -0.04% 
==========================================
 Files 56 56 
 Lines 2870 2869 -1 
==========================================
- Hits 2760 2758 -2 
- Misses 110 111 +1
Impacted Files Coverage Δ
example/tests/unit/test_renderers.py 100% <100%> (ø) ⬆️
rest_framework_json_api/renderers.py 86.79% <100%> (-0.58%) ⬇️

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 01e92be...82a68b6. Read the comment docs.

Copy link
Contributor Author

@sliverc Done

@Anton-Shutik Anton-Shutik force-pushed the get-related-instance branch 2 times, most recently from ffb2b46 to a4ecd9a Compare May 28, 2019 13:25
@Anton-Shutik Anton-Shutik force-pushed the get-related-instance branch 2 times, most recently from 756370e to aab3455 Compare May 28, 2019 14:38
Copy link
Contributor Author

@sliverc Are there any more updates required ?

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.

Test looks good. See inline comment for a small change.

@sliverc sliverc force-pushed the get-related-instance branch from 49c33b0 to 588e7fc Compare June 4, 2019 11:25
@sliverc sliverc changed the title (削除) Use DRF code to fetch related instance (削除ここまで) (追記) Allow dotted source attributes in serializer fields to extract includes (追記ここまで) Jun 4, 2019
@sliverc sliverc force-pushed the get-related-instance branch from 588e7fc to 82a68b6 Compare June 4, 2019 11:38
@sliverc sliverc changed the title (削除) Allow dotted source attributes in serializer fields to extract includes (削除ここまで) (追記) Use REST framework serializer functionality to extract includes (追記ここまで) Jun 4, 2019
@sliverc sliverc merged commit b884535 into django-json-api:master Jun 4, 2019
Copy link
Member

sliverc commented Jun 4, 2019

Merged. Thanks for your work.

Copy link
Contributor Author

Cool, thanks. Any expected release date ?

@Anton-Shutik Anton-Shutik deleted the get-related-instance branch June 4, 2019 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sliverc sliverc sliverc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /