Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#33449 closed Bug (fixed)

Migration autodetector crashes on models with field named _order, but not using order_with_respect_to.

Reported by: Fabian Büchler Owned by: Fabian Büchler
Component: Migrations Version: 4.0
Severity: Release blocker Keywords:
Cc: David Wobrock 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 (last modified by Fabian Büchler)

The commit https://github.com/django/django/commit/aa4acc164d1247c0de515c959f7b09648b57dc42 introduced a new function ModelState.get_field in django.db.migrations.state.

This converts the field name _order to the one defined in options['order_with_respect_to'] automatically, which fails if the model has a field _order but isn't using Meta.order_with_respect_to.

That is the case for models generated by django-simple-history (https://github.com/jazzband/django-simple-history) for models that are originally using Meta.order_with_respect_to: the resulting historical records model has only _order but is not using the Meta option.

This shows when running mange.py migrate or manage.py makemigrations:

$ ./manage.py makemigrations --dry-run
Waiting for port 'mysql:3306' timeout 1s (attempt 1/60)
Port 'mysql:3306' is open
Traceback (most recent call last):
  File "./manage.py", line 42, in <module>
    main()
  File "./manage.py", line 36, in main
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 425, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 419, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 373, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 417, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 90, in wrapped
    res = handle_func(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/commands/makemigrations.py", line 172, in handle
    changes = autodetector.changes(
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/autodetector.py", line 43, in changes
    changes = self._detect_changes(convert_apps, graph)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/autodetector.py", line 189, in _detect_changes
    self.generate_altered_fields()
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/autodetector.py", line 928, in generate_altered_fields
    old_field = self.from_state.models[app_label, old_model_name].get_field(old_field_name)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/state.py", line 689, in get_field
    self.options['order_with_respect_to']
KeyError: 'order_with_respect_to'

I believe this could be solved using a bit more defensive code, like:

    def get_field(self, field_name):
        if field_name == '_order' and 'order_with_respect_to' in self.options:
            field_name = self.options['order_with_respect_to']
        return self.fields[field_name]

Change History (10)

comment:1 by Fabian Büchler, 2 years ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 2 years ago

Cc: David Wobrock added
Severity: NormalRelease blocker
Summary: Migration autodetector fails for models with field named _order, but not using Meta.order_with_respect_toMigration autodetector crashes on models with field named _order, but not using order_with_respect_to.
Triage Stage: UnreviewedAccepted

Thanks for the report! Would you like to prepare a patch? (a regression test is required.)

Regression in aa4acc164d1247c0de515c959f7b09648b57dc42.
Reproduced at dc8bb35e39388d41b1f38b6c5d0181224e075f16.

comment:3 by Fabian Büchler, 2 years ago

I could do that, Mariusz. Do you have any suggestions regarding where to put the regression test, though? I checked before filing the issue and was a bit lost in the plentora of tests. Would it fit with the migration autodetector tests at https://github.com/django/django/blob/main/tests/migrations/test_autodetector.py ?

Last edited 2 years ago by Fabian Büchler (previous) (diff)

comment:4 by Fabian Büchler, 2 years ago

Owner: changed from nobody to Fabian Büchler
Status: newassigned

comment:5 by Fabian Büchler, 2 years ago

I've got a fix including regression tests prepared, just need to check with my employer regarding ICLA or CCLA before submitting a PR.
https://github.com/fabianbuechler/django/commit/b74ded041b17f7ec4386e87a0f84a097a8d6df4d

Version 0, edited 2 years ago by Fabian Büchler (next)

comment:6 by Fabian Büchler, 2 years ago

Has patch: set

PR submitted. CCLA pending.

comment:7 by Simon Charette, 2 years ago

Patch LGTM once a release note is added.

comment:8 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In eeff1787:

Fixed #33449 -- Fixed makemigrations crash on models without Meta.order_with_respect_to but with _order field.

Regression in aa4acc164d1247c0de515c959f7b09648b57dc42.

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

In b320802:

[4.0.x] Fixed #33449 -- Fixed makemigrations crash on models without Meta.order_with_respect_to but with _order field.

Regression in aa4acc164d1247c0de515c959f7b09648b57dc42.

Backport of eeff1787b0aa23016e4844c0f537d5093a95a356 from main

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