Refs #18597 -- Refactor new hooks for BaseModelFormSet and BaseInlineFormSet #17818
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
| @@ -1311,6 +1311,51 @@ Then when you create your inline formset, pass in the optional argument | |||
| >>> author = Author.objects.get(name="Mike Royko") | |||
| >>> formset = BookFormSet(instance=author) | |||
|
|
|||
| .. _inline-formset-interaction-with-prefetching: | |||
There was a problem hiding this comment.
get_instance_by_queryset_index() should also be documented under models.BaseModelFormSet docs. Is it useful in itself?
There was a problem hiding this comment.
I'm not sure how to mention get_instance_by_queryset_index() in the 'Model formsets' section.
I can't think of a reason to override that method on it's own.
docs/topics/forms/modelforms.txt
Outdated
| The ``get_instance_by_queryset_index`` and ``prepare_queryset`` hooks were added. | ||
|
|
There was a problem hiding this comment.
This entire section is new so this comment is unnecessary
| The ``get_instance_by_queryset_index`` and ``prepare_queryset`` hooks were added. |
docs/topics/forms/modelforms.txt
Outdated
|
|
||
| The ``get_instance_by_queryset_index`` and ``prepare_queryset`` hooks were added. | ||
|
|
||
| You'll need to create a subclass to take advantage of |
There was a problem hiding this comment.
Reading this section, it's not clear for me what we want to achieve. We should start from describing what we're trying to do here.
docs/topics/forms/modelforms.txt
Outdated
| return queryset | ||
| return super().prepare_queryset(queryset) | ||
|
|
||
| Note that you'll also need to ensure that the related object has a default sort:: |
There was a problem hiding this comment.
My immediate question is "why?".
tests/model_formsets/tests.py
Outdated
| """ | ||
| Override BaseInlineFormSet.get_instance_by_queryset_index and | ||
| BaseInlineFormSet.prepare_queryset (#18597). | ||
| """ |
There was a problem hiding this comment.
This docstring doesn't describe anything, I'd remove it.
tests/model_formsets/tests.py
Outdated
| with CaptureQueriesContext(connection) as ctx: | ||
| formset = BookFormSet(instance=author) | ||
| self.assertEqual(len(formset.forms), 6) | ||
| self.assertEqual(len(ctx), 1) # Only 1 query |
There was a problem hiding this comment.
You can use self.assertNumQueries():
| with CaptureQueriesContext(connection) as ctx: | |
| formset = BookFormSet(instance=author) | |
| self.assertEqual(len(formset.forms), 6) | |
| self.assertEqual(len(ctx), 1) # Only 1 query | |
| with self.assertNumQueries(1): | |
| formset = BookFormSet(instance=author) | |
| self.assertEqual(len(formset.forms), 6 |
tests/model_formsets/tests.py
Outdated
| Book.objects.create(pk=2, author=author, title="Les Fleurs du Mal") | ||
| Book.objects.create(pk=3, author=author, title="Flowers of Evil") | ||
|
|
||
| # Check the default issues one query |
There was a problem hiding this comment.
To add more explanation, comments should state the expected behavior and omit prefixes like "Check" since all tests check things. This guideline is from Python coding style.
tests/model_formsets/tests.py
Outdated
| with CaptureQueriesContext(connection) as ctx: | ||
| formset = BookFormSet(instance=author_with_books) | ||
| self.assertEqual(len(formset.forms), 6) | ||
| self.assertFalse(ctx.captured_queries) # No queries, prefetch_related is used |
There was a problem hiding this comment.
| with CaptureQueriesContext(connection) as ctx: | |
| formset = BookFormSet(instance=author_with_books) | |
| self.assertEqual(len(formset.forms), 6) | |
| self.assertFalse(ctx.captured_queries) # No queries, prefetch_related is used | |
| with assertNumQueries(0): | |
| formset = BookFormSet(instance=author_with_books) | |
| self.assertEqual(len(formset.forms), 6) |
tests/model_formsets/tests.py
Outdated
| with CaptureQueriesContext(connection) as ctx: | ||
| formset = BookFormSet(instance=author) | ||
| self.assertEqual(len(formset.forms), 6) | ||
| self.assertEqual(len(ctx), 1) # Only 1 query |
There was a problem hiding this comment.
| with CaptureQueriesContext(connection) as ctx: | |
| formset = BookFormSet(instance=author) | |
| self.assertEqual(len(formset.forms), 6) | |
| self.assertEqual(len(ctx), 1) # Only 1 query | |
| with assertNumQueries(1): | |
| formset = BookFormSet(instance=author) | |
| self.assertEqual(len(formset.forms), 6) |
docs/releases/5.1.txt
Outdated
| * New hooks were added to :class:`~django.forms.models.BaseModelFormSet` and | ||
| :class:`~django.forms.models.BaseInlineFormSet` to allow custom subclasses to | ||
| utilize :meth:`~django.db.models.query.QuerySet.prefetch_related` | ||
| (:ticket:`18597`). See :ref:`inline-formset-interaction-with-prefetching` for |
There was a problem hiding this comment.
Ticket number is unnecessary here
| (:ticket:`18597`). See :ref:`inline-formset-interaction-with-prefetching` for | |
| See :ref:`inline-formset-interaction-with-prefetching` for |
…FormSet. Added two new hooks: - BaseModelFormSet.get_instance_by_queryset_index - BaseInlineFormSet.prepare_queryset Added documentation showing how new hooks can be used to allow an inline formset to use prefetch_related. Added a test for this scenario.
shjohnson-pi
force-pushed
the
ticket_18597
branch
from
e90a916
to
79ccecd
Compare
|
Thank you for the detailed feedback felixxm. I've made the following changes and rebased.
|
Added two new hooks:
Added documentation showing how new hooks can be used to allow an inline formset to use prefetch_related. Added a test for this scenario.
Context:
Because it's been ~2 years since there's been progress on these tickets, I'm sure extra time will be needed to determine if this is still the best direction to fix the inline/cache problem.
Also, this is my first major pull request for Django, so all feedback is welcome. In particular my documentation and tests may need changes.