[Django] #31499: Store ModeState.fields into a dict.
Django
------------------------------------------------+------------------------
Reporter: Simon Charette | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
`ModeState` initially stored its `fields` into a `List[Tuple[str,
models.Field]]` because
[https://github.com/django/django/blob/290eb987644c31aabcfd52d81bd5adcf7a1972aa/django/db/migrations/state.py#L368-L370
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
[https://docs.djangoproject.com/en/3.0/topics/forms/modelforms/#selecting-
the-fields-to-use 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31499>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Simon Charette | Owner: nobody
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31499#comment:1>
Django
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* owner: nobody => Simon Charette
* status: new => assigned
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/31499#comment:2>
Django
Reporter: Simon Charette | Owner: Simon
Type: | Charette
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"06889d62063f0d12aaf618101dfc1b07333117be" 06889d62]:
{{{
#!CommitTicketReference repository=""
revision="06889d62063f0d12aaf618101dfc1b07333117be"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31499#comment:5>