[Django] #22568: FK to proxy model breaks migrate
Django
----------------------------+------------------------
Reporter: leftmoose | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------
Situation similar to #22527
but for new 1.7 migrations
{{{#!python
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>]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22568>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: leftmoose | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by leftmoose):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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)?
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:1>
Django
Reporter: leftmoose | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:2>
Django
Reporter: leftmoose | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by leftmoose):
what's the criteria for release blockers? this breaks foreign keys to
proxy models...
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:3>
Django
Reporter: leftmoose | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:4>
Django
Reporter: leftmoose | Owner: davids
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by davids):
* owner: nobody => davids
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:5>
Django
Reporter: leftmoose | Owner: davids
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewgodwin):
* severity: Normal => Release blocker
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:6>
Django
Reporter: leftmoose | Owner: davids
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by davids):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
https://github.com/django/django/pull/2645/
needs review + more tests
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:7>
Django
Reporter: leftmoose | Owner: davids
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by loic84):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:8>
Django
Reporter: leftmoose | Owner: davids
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by davids):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:9>
Django
Reporter: leftmoose | Owner: davids
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by andrewgodwin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:10>
Django
Reporter: leftmoose | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by andrewgodwin):
* owner: davids => andrewgodwin
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:11>
Django
Reporter: leftmoose | Owner: andrewgodwin
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Changes (by Andrew Godwin <andrew@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c1276785f90b89c8bced606b9cbf7b1a612506e5"]:
{{{
#!CommitTicketReference repository=""
revision="c1276785f90b89c8bced606b9cbf7b1a612506e5"
Fixed #22568: Better proxy model support in migrations
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:12>
Django
Reporter: leftmoose | Owner: andrewgodwin
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Comment (by Andrew Godwin <andrew@…>):
In [changeset:"a81282a512e706011747ec4cd1a990bae167edc6"]:
{{{
#!CommitTicketReference repository=""
revision="a81282a512e706011747ec4cd1a990bae167edc6"
[1.7.x] Fixed #22568: Better proxy model support in migrations
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22568#comment:13>