Opened 5 years ago

Closed 5 years ago

#31499 closed Cleanup/optimization (fixed)

Store ModeState.fields into a dict.

Reported by: Simon Charette Owned by: Simon Charette
Component: Migrations 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

ModeState initially stored its fields into a List[Tuple[str, models.Field]] because it wanted to preserve ordering.

However the auto-detector doesn't consider field re-ordering as a state change and Django doesn't support table column reordering in the first place. The only reason I'm aware of for keeping field ordering is to generate model forms out of them which is unlikely to happen during migrations and if it was the case the only the order in which field are ordered and validated would change if Meta.fields = '__all__ is used which is discouraged.

Given storing fields this way results in awkward and inefficient lookup by name for no apparent benefits and that dict now preserves insertion ordering I suggest we switch ModelState.fields to Dict[str, models.Field]. I suggest we do the same for ModelState.indexes and .constraints since they suggest from the same awkwardness which was likely cargo culted from ModelState.fields design decision.

Change History (5)

comment:1 by Simon Charette, 5 years ago

Has patch: set
Version 0, edited 5 years ago by Simon Charette (next)

comment:2 by Mariusz Felisiak, 5 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:3 by Mariusz Felisiak, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:4 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 696024fb:

Refs #31499 -- Ignored field ordering to determine ModelState equality.

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 06889d62:

Fixed #31499 -- Stored ModelState.fields into a dict.

This allows the removal of its O(n) .get_field_by_name method and many
other awkward access patterns.

While fields were initially stored in a list to preserve the initial
model definiton field ordering the auto-detector doesn't take field
ordering into account and no operations exists to reorder fields of a
model.

This makes the preservation of the field ordering completely superflous
because field reorganization after the creation of the model state
wouldn't be taken into account.

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