Opened 9 years ago

Closed 9 years ago

Last modified 9 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, 9 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 by Simon Charette, 9 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, 9 years ago

Has patch: set

Created a PR.

comment:4 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: newclosed

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@…>, 9 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