Skip to content

Allow defining of select_related per include #600

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
merged 12 commits into from
May 29, 2019

Conversation

Anton-Shutik
Copy link
Contributor

@Anton-Shutik Anton-Shutik commented Mar 18, 2019

Fixes #591

Description of the Change

This PR adds SelectAndPrefetchForIncludesMixin which handles include GET parameter and adds select_related and/or prefetch_related calls to queryset in order to save some sql queries.

This also deprecates PrefetchForIncludesHelperMixin.

AutoPrefetchMixin is modified so if developer declared prefetch_for_includes that means that it's handled "manually" and no need to auto prefetch related entities.

Let's say we have a code like below:

class Product(models.Model):
    name = CharField()
    tags = M2MField()
    variant = FKField()


class ProductSerializer():
    tags = ResourceRelatedField()
    

class VariantSerializer():
    product = ResourceRelatedField()


class VariantViewset():
    serializer_class = VariantSerializer
    prefetch_for_include = {
    'product': 'product__tags'
}
    

Line 'product': 'product__tags' will save one query per included product, because it will prefetch tags.

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 sliverc left a 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. See my inline comments. And I assume once we have settled on the implementation you will follow up on the other PR TODOs.

@codecov
Copy link

codecov bot commented May 17, 2019

Codecov Report

Merging #600 into master will decrease coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #600      +/-   ##
==========================================
- Coverage   95.66%   95.39%   -0.28%     
==========================================
  Files          56       56              
  Lines        2861     2865       +4     
==========================================
- Hits         2737     2733       -4     
- Misses        124      132       +8
Impacted Files Coverage Δ
example/tests/test_performance.py 100% <100%> (ø) ⬆️
rest_framework_json_api/compat.py 50% <0%> (-50%) ⬇️
rest_framework_json_api/django_filters/backends.py 86.66% <0%> (-13.34%) ⬇️

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 db5cd72...650dad0. Read the comment docs.

@codecov
Copy link

codecov bot commented May 17, 2019

Codecov Report

Merging #600 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #600      +/-   ##
==========================================
+ Coverage   96.16%   96.16%   +<.01%     
==========================================
  Files          56       56              
  Lines        2866     2870       +4     
==========================================
+ Hits         2756     2760       +4     
  Misses        110      110
Impacted Files Coverage Δ
example/tests/test_performance.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 f3e67a7...db5251d. Read the comment docs.

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.

This PR is shaping up nicely. I like the auto select_related part.

See my inline comments for some last changes and questions.

@Anton-Shutik Anton-Shutik force-pushed the master branch 4 times, most recently from 83215a4 to 0d126bc Compare May 24, 2019 13:10
@sliverc sliverc changed the title Added SelectAndPrefetchForIncludesMixin, deprecated PrefetchForInclud… Allow defining of select_related per include May 28, 2019
@sliverc sliverc merged commit 8bb123c into django-json-api:master May 29, 2019
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.

Do select_related for ?include
2 participants