Opened 7 months ago

Closed 5 months ago

#35424 closed Bug (fixed)

Migration autodetector fails when order_with_respect_to is removed, but an _order field remains

Reported by: Stuart Attenborrow Owned by: Daniel Patrick
Component: Migrations Version: 4.2
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

We have a project with a large number of migrations that began failing during migrate/makemigrations due to order_with_respect_to having been removed, and an explicit _order field added. The historic migration contains this operation:

migrations.AlterOrderWithRespectTo(
    name='choice',
    order_with_respect_to=None,
),

This causes an issue with the code introduced in commit https://github.com/django/django/commit/aa4acc164d1247c0de515c959f7b09648b57dc42 which was subsequently "fixed" in https://code.djangoproject.com/ticket/33449 but remains failing in the case described above.

A minimal reproducible case has been created here: https://github.com/stuarta0/django-order-with-respect-to based on the django docs polls app.

A possible solution may be to fall back to field_name if order_with_respect_to is either missing, or present with None (which is what occurs above):

def get_field(self, field_name):
    if field_name == "_order":
        field_name = self.options.get("order_with_respect_to") or field_name
    return self.fields[field_name]

Attachments (1)

django-order-with-respect-to.zip (17.4 KB ) - added by Stuart Attenborrow 7 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 by Sarah Boyce, 7 months ago

Triage Stage: UnreviewedAccepted

Thank you for the report, I'm struggling to understand what's happening in your project. There's a number of migrations in there.
I don't think I have replicated the issue your seeing, what error do you see?

I can find the following possible issue that given an existing migrated model

class Foo(models.Model):
    order = models.IntegerField(default=999999)

    class Meta:
        order_with_respect_to = "order"

when you change that model to

class Foo(models.Model):
    _order = models.IntegerField(default=999999)

   class Meta:
        ordering = ("_order",)

that generates a migration like

from django.db import migrations


class Migration(migrations.Migration):

    dependencies = [
        ('app3', '0004_foo'),
    ]

    operations = [
        migrations.AlterModelOptions(
            name='foo',
            options={'ordering': ('_order',)},
        ),
        migrations.RenameField(
            model_name='foo',
            old_name='order',
            new_name='_order',
        ),
        migrations.AlterOrderWithRespectTo(
            name='foo',
            order_with_respect_to=None,
        ),
    ]

Which fails with django.core.exceptions.FieldDoesNotExist: Foo has no field named 'order'

But actually I don't think that's what you're talking about here? Can you reduce it down a bit further?

Last edited 7 months ago by Sarah Boyce (previous) (diff)

comment:2 by Sarah Boyce, 7 months ago

Resolution: needsinfo
Status: newclosed
Triage Stage: AcceptedUnreviewed

comment:3 by Stuart Attenborrow, 7 months ago

Thanks for looking into it. Due to the error you're seeing, the migrations had to be staggered to achieve the changes. e.g.:

Since the code is 8 years old and written by another author, I can't give you an explicit reason why the "_order" name was reused other than my assumption that "order" is a reserved word and "_order" was good enough for django internals at the time. However, it hasn't been an issue because django never had code that was structured in the way it is now.

The answer for new projects is probably to remake or squash migrations to avoid it, but it's quite difficult on a legacy project with this scale of migrations. My suggested change in the original report fixes the issue for this case, but I haven't run django tests to see if it fails in other cases.

comment:4 by Sarah Boyce, 7 months ago

Resolution: needsinfoworksforme

Hi Stuart,
I have cloned your test repo installed Django 4.2, ran migrations and they all apply without an error for me 🤔
I think you also need to share the error that you are seeing, I can't determine that Django is at fault here.

by Stuart Attenborrow, 7 months ago

comment:5 by Stuart Attenborrow, 7 months ago

Error shown below. I've added a zip that contains the same project, but with docker compose, and a poetry lock file to ensure we're looking at the same thing. Note I can reproduce this both in a docker container, and on a production EC2 instance using system python and virtual envs.

Edit: to run migrate I'm shelling into the container with docker compose run mysite bash followed by the command below:

/code/mysite# python manage.py migrate
Operations to perform:
  Apply all migrations: admin, auth, contenttypes, polls, sessions
Running migrations:
  No migrations to apply.
Traceback (most recent call last):
  File "/code/mysite/manage.py", line 22, in <module>
    main()
  File "/code/mysite/manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "/opt/venv/lib/python3.11/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/opt/venv/lib/python3.11/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/opt/venv/lib/python3.11/site-packages/django/core/management/base.py", line 412, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/opt/venv/lib/python3.11/site-packages/django/core/management/base.py", line 458, in execute
    output = self.handle(*args, **options)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/venv/lib/python3.11/site-packages/django/core/management/base.py", line 106, in wrapper
    res = handle_func(*args, **kwargs)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/venv/lib/python3.11/site-packages/django/core/management/commands/migrate.py", line 335, in handle
    changes = autodetector.changes(graph=executor.loader.graph)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/venv/lib/python3.11/site-packages/django/db/migrations/autodetector.py", line 46, in changes
    changes = self._detect_changes(convert_apps, graph)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/venv/lib/python3.11/site-packages/django/db/migrations/autodetector.py", line 197, in _detect_changes
    self.generate_altered_fields()
  File "/opt/venv/lib/python3.11/site-packages/django/db/migrations/autodetector.py", line 1117, in generate_altered_fields
    old_field = self.from_state.models[app_label, old_model_name].get_field(
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/venv/lib/python3.11/site-packages/django/db/migrations/state.py", line 765, in get_field
    return self.fields[field_name]
           ~~~~~~~~~~~^^^^^^^^^^^^
KeyError: None
Last edited 7 months ago by Stuart Attenborrow (previous) (diff)

comment:6 by Sarah Boyce, 7 months ago

Easy pickings: unset
Triage Stage: UnreviewedAccepted

Hi thank you Stuart for the extra context, I'm able to replicate your error too.
Note for others, it can "migrate" without error on the first run but then error on future calls to migrate.

comment:7 by Sarah Boyce, 7 months ago

Resolution: worksforme
Status: closednew

comment:8 by Daniel Patrick, 5 months ago

Owner: changed from nobody to Daniel Patrick
Status: newassigned

comment:9 by Daniel Patrick, 5 months ago

Has patch: set
Last edited 5 months ago by Daniel Patrick (previous) (diff)

comment:10 by Sarah Boyce, 5 months ago

Triage Stage: AcceptedReady for checkin

comment:11 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

Resolution: fixed
Status: assignedclosed

In d12184fe:

Fixed #35424 -- Checked order_with_respect_to is available when migrating _order fields.

Migrations would crash following the removal of an order_with_respect_to
field from a model and the addition of an _order field.

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