Movatterモバイル変換


[0]ホーム

URL:


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

Feature/nested prefetching#964

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

jarekwg
Copy link
Contributor

@jarekwgjarekwg commentedJul 22, 2021
edited
Loading

Addresses#921,#337

Problem:
We need n+1 handling to work when nesting includes.
Currently the two offerings in drfjsonapi don't cut it:

  • PreloadIncludesMixin forces unnecessary manual effort, and is fundamentally flawed in that it cannot be set up to consider nesting properly. Either you don't prefetch and incur n+1, or you do prefetch, but you do so even when the given request doesn't need the data. Also the manual configuration is a headache to maintain.
  • AutoPrefetchMixin doesn't recurse, nor does it offer any way of opting out of fetching the additional data if sparsefieldsets haven't asked for it. Specifically, for reverse relations and M2Ms, there are three cases to consider: [1] don't return the field at all (no prefetch), [2] return only the ids (simple prefetch), and [3] return the entire relations (deep prefetch and recurse this same logic at the next level down).

This PR introduces a no-config approach, which'll

  • Automatically exclude any "expensive relations" (reverse FKs and m2ms) from queries that haven't used sparsefields. (This is spec compliant!I've checked.)
  • Offer a flag,INCLUDE_EXPENSVE_FIELDS, to still include expensive relations if one so wants (negating the first point, allowing an easier upgrade path).
  • Smartly use a combination ofprefetch_related andselect_related calls to prepare exactly the data that's needed.
  • Perform some validation to stop usage of the previous flawed implementations.

Obvs this is a large change and would require an appropriately-sized version bump to boot.

We're actually using this in production presently, having rolled it out not so long ago. It's cleaned up a lot of mess in our serializer definitions, and also provided a much better peace of mind that nothing is being missed.

I'm happy to babysit this through to getting merged, improving test coverage and backwards compatibility (it's currently only py39 compliant, but i can strip back some things to fix that), but wanted to get an early set of eyes on it, so that i can have a guarantee it'll be merged and my efforts aren't for nought.

Cheers!

@jarekwg
Copy link
ContributorAuthor

Closing. Closed PR will exist as a record of "future-ish plan". Will spin up some cleaner, smaller PRs to trickle these changes in in smaller chunks. See:#921 (reply in thread)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

1 participant
@jarekwg

[8]ページ先頭

©2009-2025 Movatter.jp