Issue #337

[jsenecal]

• Adds Django Debug Toolbar, and add a failing performance (query count) test
• Adds a helper for specifying a prefetch_for_related attribute on your viewset to help with prefetching includes.
• Fixes some of the example code

Thanks to @aidanlister for his contribution on this.

[codecov-io]

Codecov Report

Merging #366 into develop will increase coverage by 0.19%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           develop   #366      +/-   ##
=========================================
+ Coverage    92.81%    93%   +0.19%     
=========================================
  Files           51     51              
  Lines         2644   2659      +15     
=========================================
+ Hits          2454   2473      +19     
+ Misses         190    186       -4

Impacted Files	Coverage Δ
setup.py	80% <ø> (ø)	⬆️
rest_framework_json_api/serializers.py	84.05% <ø> (-0.12%)	⬇️
example/factories.py	96.87% <ø> (ø)
example/serializers.py	100% <100%> (ø)	⬆️
example/tests/test_performance.py	100% <100%> (ø)
rest_framework_json_api/views.py	95.85% <100%> (+0.34%)	⬆️
example/settings/dev.py	100% <100%> (ø)	⬆️
example/views.py	90.74% <100%> (+0.17%)	⬆️
rest_framework_json_api/relations.py	82.85% <100%> (+0.5%)	⬆️
rest_framework_json_api/exceptions.py
... and 1 more

---

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 1659d8c...7343def. Read the comment docs.

[aidanlister]

Looks great!

[mblayman]

@jsenecal It looks like Travis is failing because of isort (curiously, only Python 3.4 🤔 ). I think isort gets a little tricky because the config file is trying to be correct for the rest_framework_json_api, but it also has to satisfy the included example package. I'm guessing you can solve this by including --thirdparty packaging to the isort command that checks example.

$ isort --check-only --verbose --recursive --diff --thirdparty pytest \
    --thirdparty polymorphic --thirdparty pytest_factoryboy \
    --thirdparty packaging example

I'll look at the code closer to give it some review.

[mblayman]

Why is this ignoring manage.py?

[jsenecal]

@aidanlister must have a custom one (introduced by da0c94c).

We're not using it to start the app anyways ($ django-admin.py runserver --settings=example.settings)

[mblayman]

Since example is what runs on the demo site, should we leave the form version on by default instead of the perf version so that people can add extra data to play with it?

[jsenecal]

I guess it would make sense indeed - let me fix this

[mblayman]

👏 I'm a fan of any time a * import can be removed.

[mblayman]

I like what's here a lot!

What do you think about documentation for this stuff? IMO, features don't really exist until they are documented so that users know how to use them. With that stated, it doesn't matter to me if the features are documented in this branch or in some other branch as long as there is some prose before in the docs before the next release.

Once Travis is green, I'm ok with merging. If you decide to add some docs, I can give it another look.

Thanks for pulling this together with @aidanlister. 👍

[jsenecal]

I still have an issue with the way relations are extracted first level... I had to prefetch things that I shouldn't have to only to make tests pass... Feels like the queryset might be over reaching to overcome an issue with drf_jsonapi.

[jsenecal]

So, it really happens in b96d79d merged a year ago.

@jerel do you remember the conversation that was around this ?

[jsenecal]

After further investigation, the problem does not occur inside extract_relationships - Still looking

[aidanlister]

@jsenecal the problem is in two places, setting use_pk_only_optimization to False, and extract_attributes. You have to whack them both, which made it so hard to find the source of the explosion.

I think you should probably remove the select_related in this because the tests should fail until we fix that other issue?

[jsenecal]

Thanks for this - I merged already but will update my branch to reflect this conversation

[jsenecal]

@mblayman I did add some docs.

See my notes about this and b96d79d

Should we open another issue for that and merge this ?

[aidanlister]

@jsenecal thanks for including my cite, might be better to give some context there though? Let me write something:

[aidanlister]

Using the helper to prefetch, rather than attempting to minimise queries via select_related might give you better performance depending on the characteristics of your data and database.

For example:

If you have a single model, e.g. Book, which has four relations e.g. Author, Publisher, CopyrightHolder, Category.

To display 25 books and related models, you would need to either do:

a) 1 query via selected_related, e.g. SELECT * FROM books LEFT JOIN author LEFT JOIN publisher LEFT JOIN CopyrightHolder LEFT JOIN Category

b) 4 small queries via prefetch_related.

If you have 1M books, 50k authors, 10k categories, 10k copyrightholders in the select_related scenario, you've just created a in-memory table with 1e18 rows which will likely exhaust any available memory and slow your database to crawl. The prefetch_related case will issue 4 queries, but they will be small and fast queries.

No need to cite me, my paragraph in that issue is about the query explosion from displaying FKs -- let's merge this and we'll keep working out of that issue.