Skip to content

Fix/auto prefetch with m2m #333

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

Conversation

bor3ham
Copy link
Contributor

@bor3ham bor3ham commented Mar 14, 2017

Previously including relations with the optimised viewset through m2m's would crash as it was not correctly getting the model from the related descriptor

For example from the tests:

response = client.get(reverse("entry-list") + '?include=comments.author.bio')

The tests already covered the right use case, but were not using the prefetching viewset.

I had to update the test models / serializers slightly to support the auto prefetching (which requires the field names in the serializer match the models). I will create an issue to address this as an improvement

bor3ham added 6 commits March 14, 2017 11:29
That works through many to many field descriptors, previous way only
worked through many to ones.
Updates the tests to use the optimised queryset views provided in
views.py. For these to work, the serializer fields need to match the
model names so I have just changed the related_name in the test models.

This could be improved elsewhere, with the queryset prefetching based on
the serializer. But for the most basic use case, this is fine.
Based on the relation descriptor.
@codecov-io
Copy link

codecov-io commented Mar 14, 2017

Codecov Report

Merging #333 into develop will increase coverage by 0.82%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop    #333      +/-   ##
==========================================
+ Coverage    91.68%   92.5%   +0.82%     
==========================================
  Files           50      50              
  Lines         2368    2375       +7     
==========================================
+ Hits          2171    2197      +26     
+ Misses         197     178      -19
Impacted Files Coverage Δ
example/serializers.py 100% <ø> (ø)
example/tests/unit/test_renderers.py 100% <ø> (ø)
rest_framework_json_api/views.py 95.42% <100%> (+13.23%)
example/tests/test_relations.py 100% <100%> (ø)
example/models.py 94.82% <100%> (ø)
example/views.py 89.58% <100%> (ø)
example/tests/integration/test_includes.py 100% <100%> (ø)
example/tests/test_views.py 100% <100%> (ø)

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 d8c3dc4...c374c6d. Read the comment docs.

@@ -32,7 +36,7 @@

class ModelViewSet(viewsets.ModelViewSet):
def get_queryset(self, *args, **kwargs):
qs = super().get_queryset(*args, **kwargs)
qs = super(ModelViewSet, self).get_queryset(*args, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, nice! Thanks for this one.

@mblayman
Copy link
Collaborator

mblayman commented May 6, 2017

@bor3ham thanks for this work. I really like the improved coverage. 👍

@mblayman mblayman merged commit cdb8a52 into django-json-api:develop May 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants