Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#22568 closed Bug (fixed)

FK to proxy model breaks migrate

Reported by: leftmoose Owned by: Andrew Godwin
Component: Migrations Version: 1.7-beta-2
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Situation similar to #22527

but for new 1.7 migrations

installed_apps = (
    ...
    app1, app2, app3
)

db: postgres (to enfoce constraints)

app1.models:
from django.db import models

class Model1(models.Model):
    foreign2 = models.ForeignKey('app2.Model2')

app2.models:
from app3.models import Model3

class Model2(Model3):
    class Meta:
        proxy = True

app3.models:
from django.db import models

# Create your models here.
class Model3(models.Model):
    pass
$ manage.py makemigrations app3
$ manage.py makemigrations app1

$ ./manage.py test
Creating test database for alias 'default'...
Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/david/dev/django_source/django/core/management/__init__.py", line 427, in execute_from_command_line
    utility.execute()
  File "/Users/david/dev/django_source/django/core/management/__init__.py", line 419, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/david/dev/django_source/django/core/management/commands/test.py", line 50, in run_from_argv
    super(Command, self).run_from_argv(argv)
  File "/Users/david/dev/django_source/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/Users/david/dev/django_source/django/core/management/commands/test.py", line 71, in execute
    super(Command, self).execute(*args, **options)
  File "/Users/david/dev/django_source/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File "/Users/david/dev/django_source/django/core/management/commands/test.py", line 88, in handle
    failures = test_runner.run_tests(test_labels)
  File "/Users/david/dev/django_source/django/test/runner.py", line 147, in run_tests
    old_config = self.setup_databases()
  File "/Users/david/dev/django_source/django/test/runner.py", line 109, in setup_databases
    return setup_databases(self.verbosity, self.interactive, **kwargs)
  File "/Users/david/dev/django_source/django/test/runner.py", line 297, in setup_databases
    verbosity, autoclobber=not interactive)
  File "/Users/david/dev/django_source/django/db/backends/creation.py", line 368, in create_test_db
    test_database=True)
  File "/Users/david/dev/django_source/django/core/management/__init__.py", line 167, in call_command
    return klass.execute(*args, **defaults)
  File "/Users/david/dev/django_source/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File "/Users/david/dev/django_source/django/core/management/commands/migrate.py", line 145, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "/Users/david/dev/django_source/django/db/migrations/executor.py", line 60, in migrate
    self.apply_migration(migration, fake=fake)
  File "/Users/david/dev/django_source/django/db/migrations/executor.py", line 88, in apply_migration
    if self.detect_soft_applied(migration):
  File "/Users/david/dev/django_source/django/db/migrations/executor.py", line 132, in detect_soft_applied
    apps = project_state.render()
  File "/Users/david/dev/django_source/django/db/migrations/state.py", line 52, in render
    raise InvalidBasesError("Cannot resolve bases for %r" % new_unrendered_models)
django.db.migrations.state.InvalidBasesError: Cannot resolve bases for [<django.db.migrations.state.ModelState object at 0x10b2a8dd0>]

Change History (13)

comment:1 by leftmoose, 11 years ago

looking into this one possible cause might be that proxy models are ignored by makemigrations. (explicitly skipped in MigrationAutodetector._detect_changes). that means the derived state loader.graph.project_state() doesn't know about the proxy model.

what's the reason for skipping proxy models (as opposed to creating migrations for them with no fields)?

comment:2 by Tim Graham, 11 years ago

Triage Stage: UnreviewedAccepted

comment:3 by leftmoose, 11 years ago

what's the criteria for release blockers? this breaks foreign keys to proxy models...

comment:4 by Anssi Kääriäinen, 11 years ago

A possible solution is to deconstruct foreign keys to point to the concrete model instead. Proxy models are used to override methods on the model. Migrations do not include custom methods. So having the foreign key directly to the concrete base model should be good enough.

The problem with this approach is that when descontructing the ForeignKey it is possible that only a string reference is known (that is, the self.rel.to is something like 'auth.User'). From the string representation it is impossible to know if the referenced model is a proxy model. When resurrecting the string from the migration file there is also no knowledge if the model is a proxy model.

I am no expert on migrations, but my understanding is that there is no fundamental reason why the proxy models couldn't be included in a way that they are only loaded to the app-cache. This might be hard and/or ugly to do, that might be a reason why it wasn't done. More likely the reason was that proxy models do not need any database changes, so there is no need to have them in migrations. But this ticket seems to contradict that assumption.

comment:5 by David S, 11 years ago

Owner: changed from nobody to David S
Status: newassigned

comment:6 by Andrew Godwin, 11 years ago

Severity: NormalRelease blocker

Just talked this through with davids on IRC; we've concluded that the probable best approach is some new operations, CreateProxyModel and DeleteProxyModel, and to have the autogenerator create those as it sees proxies come and go (there's no need for rename, as there's no data to preserve, we can just delete/create).

This is mainly because migrations can't deal with abstract inheritance well generally, and this is a cleaner way of inserting these into the ORM.

Also bumping to release blocker.

comment:7 by David S, 10 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

comment:8 by loic84, 10 years ago

I'd prefer a proxy=True attr and a couple of if statements over CreateProxyModel + DeleteProxyModel and RenameProxyModel. There is some code duplication and with the logic in two different places it's a lot easier to forget about proxy models.

comment:9 by David S, 10 years ago

seems worth a try. i also wonder if relationships to managed = False models suffer the same problem and need a similar solution. so maybe this becomes orm_only = True or similar

comment:10 by Andrew Godwin, 10 years ago

I'm going to claim ownership of this as I intend to look at this in the next week.

loic84: I'm going to investigate if we can get proxy=True working, but I remember that I thought there'd be a problem with that (but I can't remember what my supposed problem was). Given that #22470 means we need a general "change model options" operation, I know we'd need to make sure changing proxy=True to False and vice-versa was detected as an add and a delete, not as a change, as otherwise SchemaEditor will get itself in a twist trying to alter it.

comment:11 by Andrew Godwin, 10 years ago

Owner: changed from David S to Andrew Godwin

comment:12 by Andrew Godwin <andrew@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In c1276785f90b89c8bced606b9cbf7b1a612506e5:

Fixed #22568: Better proxy model support in migrations

comment:13 by Andrew Godwin <andrew@…>, 10 years ago

In a81282a512e706011747ec4cd1a990bae167edc6:

[1.7.x] Fixed #22568: Better proxy model support in migrations

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