-
Notifications
You must be signed in to change notification settings - Fork 299
Remove requested resources from included field #540
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
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. The test looks good. In terms of implementation this should work this way. I am wondering though would it be possible to actually skip rendering the included data instead of deleting it at the end? This might be a slight performance improvement. What do you think?
If it makes the code more complex I am otherwise fine with how it is implemented right now.
We could add the actual resource to the included_cache and then remove it again later. However this introduces complexity and reduces performance for every request only to optimize performance for an edge case. After all it will rarely happen that the actual resource also turns up in the included field.
Fair point. Let's leave it this way.
Could you rebase to master and solve linting issue (see https://travis-ci.org/django-json-api/django-rest-framework-json-api/jobs/474761705)
Once this is done it is ready for merging.
Codecov Report
@@ Coverage Diff @@ ## master #540 +/- ## ========================================== + Coverage 95.51% 95.54% +0.03% ========================================== Files 55 55 Lines 2829 2853 +24 ========================================== + Hits 2702 2726 +24 Misses 127 127
Continue to review full report at Codecov.
|
f806ee3 to
41872b1
Compare
Merged. Thanks for your contribution.
Thanks for your help to fix this.
Fixes #524
Description of the Change
The resource(s) in the data field of the response are removed from the included_cache before building the included field for the response.
I added an integration test which checks whether this works on a single comment. If I request a comment and ask for all the comments of its associated blog entry with
?include=entry.commentsall comments except the one which I requested should turn up in the included field.Checklist
CHANGELOG.mdAUTHORS