Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#24266 closed Cleanup/optimization (fixed)

Change `get_parent_list` to return a list of parents ordered by __mro__ instead of a set

Reported by: Simon Charette Owned by: nobody
Component: Database layer (models, ORM) 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

The get_parent_list() method was introduced ​more than 6 years ago and has been returning an unordered set() since then.

From the method name suffix and the misleading __doc__ I guess the returned data type was changed late in the process of merging ​Malcom's queryset refactor in order to deal with the multiple ORM's ancestors existence checks by reducing ​time complexity.

While working on fixing a field shadowing issue in multi-inheritance scenarios (#24249) it was discovered that ​`get_parent_list()` actually returns an unordered set() which might causes non-deterministic behaviors. For example, another patch working on making sure unique checks take multi-inheritance into account (#15321) could result in different validation errors being raised from the same input based on the non-deterministic ordering of ancestors.

Since the undocumented get_parent_list() method seems to be ​used in the wild I suggest we deprecate and replace it by a get_ancestors() one with the following properties:

  • A more significant name given it returns ancestors regardless of lineage.
  • A return value ordered by the __mro__ of the model.
  • A return value with in operations with similar time complexity.

One of the available data structure with the required return value properties is django.utils.datastructures.OrderedSet but I wonder if the whole time complexity argument is not moot given multi-inheritance scenarios involving many ancestors shouldn't be that common.

In this case, given we don't mind about backward compatibility of such a private API, we could also fix get_parent_list() to actually return a list of unique ancestors ordered by __mro__.

Change History (6)

comment:1 by Tim Graham, 12 years ago

Triage Stage: Unreviewed → Accepted
Type: Uncategorized → Cleanup/optimization

comment:2 by Simon Charette, 12 years ago

Summary: Deprecate `Options.get_parent_list` in favor of a new `get_ancestors` one. → Change `get_parent_list` to return a list of parents ordered by __mro__ instead of a set

comment:3 by Simon Charette, 12 years ago

Has patch: set

Created a ​PR.

comment:4 by Tim Graham, 12 years ago

Triage Stage: Accepted → Ready for checkin

comment:5 by Simon Charette <charette.s@…>, 12 years ago

Resolution: → fixed
Status: new → closed

In 65e005f8cd9c656e558e53e6c8b890cd0fcc9e74:

Fixed #24266 -- Changed get_parent_list to return a list ordered by MRO.

Thanks to Aron Podrigal for the initial patch and Tim for the review.

comment:6 by Simon Charette <charette.s@…>, 12 years ago

In cbcf92e95f8a6d275b55069de3c57835814b502f:

[1.8.x] Fixed #24266 -- Changed get_parent_list to return a list ordered by MRO.

Thanks to Aron Podrigal for the initial patch and Tim for the review.

Backport of 65e005f8cd9c656e558e53e6c8b890cd0fcc9e74 from master

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