Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

jarekwg
Copy link
Contributor

@jarekwg jarekwg commented Jul 22, 2021

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 of prefetch_related and select_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
Contributor Author

jarekwg commented Jul 23, 2021

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)

@jarekwg jarekwg closed this Jul 23, 2021
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.

1 participant