Opened 4 weeks ago

Closed 3 weeks ago

#37004 closed Cleanup/optimization (fixed)

BaseModelFormSet could leverage totally_ordered property

Reported by: Jacob Walls Owned by: Rodrigo Vieira
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#36857 added a new property totally_ordered on QuerySet to capture the difference between a queryset having some ordering versus a "stable, deterministic" ordering.

I noticed that we probably want to use that new property here in BaseModelFormSet for the purpose described in #10163:

  • django/forms/models.py

    diff --git a/django/forms/models.py b/django/forms/models.py
    index 104369c0b0..3843203a1d 100644
    a b class BaseModelFormSet(BaseFormSet, AltersData):  
    776776            # If the queryset isn't already ordered we need to add an
    777777            # artificial ordering here to make sure that all formsets
    778778            # constructed from this queryset have the same form order.
    779             if not qs.ordered:
     779            if not qs.totally_ordered:
    780780                qs = qs.order_by(self.model._meta.pk.name)
    781781
    782782            # Removed queryset limiting here. As per discussion re: #13023

Tentatively assigning to Sarah to see if a good fit for any Djangonauts this session. This would involve cooking up a test.

Change History (10)

comment:1 by David Smith, 3 weeks ago

Triage Stage: UnreviewedAccepted

comment:2 by Rodrigo Vieira, 3 weeks ago

Hi, I'm a Djangonaut (Session 6) and if the author of #36857 or anyone else is not available/interested I'd like to give it a go, seems to be mostly making the change proposed and a few tests?

Version 0, edited 3 weeks ago by Rodrigo Vieira (next)

comment:3 by Simon Charette, 3 weeks ago

Go for it Rodrigo!

We'll likely want to augment the ordering with pk here and not entirely replace it (just like serializers do) otherwise it might break the natural ordering expected by users (e.g inlines ordered by name or publish date) so the proposed patch will need a bit of tweaking.

order_by also accepts "pk" so we can avoid model introspection here; self.model._meta.pk.name -> "pk".

comment:4 by Jacob Walls, 3 weeks ago

Owner: changed from Sarah Boyce to Rodrigo Vieira

comment:5 by Rodrigo Vieira, 3 weeks ago

Thanks for the pointers Simon! PR is here, following the same pattern as the code you pointed out in serializers. I added one test for each "branch" of the checks.

comment:6 by Rodrigo Vieira, 3 weeks ago

Has patch: set

comment:7 by Jacob Walls, 3 weeks ago

Patch needs improvement: set

comment:8 by Jacob Walls, 3 weeks ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:9 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

In 08aa1b5a:

Refs #37004 -- Added coverage for BaseModelFormSet.get_queryset() fallback ordering.

comment:10 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In 856c915:

Fixed #37004 -- Used QuerySet.totally_ordered in BaseModelFormSet.get_queryset() for stable ordering.

Note: See TracTickets for help on using tickets.
Back to Top