New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #24686 -- Allow moving a model between apps. #16905
base: main
Are you sure you want to change the base?
Conversation
I think more tests are required in test_move_model_with_fkdef test_move_model_with_fk(self): app_label_1 = "test_mmw_fk_1" app_label_2 = "test_mmw_fk_2" project_state = self.apply_operations( app_label_1, ProjectState(), operations=[ migrations.CreateModel( "Rider", fields=[ ("id", models.AutoField(primary_key=True)), ], ), migrations.CreateModel( "TempRider", fields=[ ("id", models.AutoField(primary_key=True)), ], ), ], ) project_state = self.apply_operations( app_label_2, project_state, operations=[ migrations.CreateModel( "Pony", fields=[ ("id", models.AutoField(primary_key=True)), ("riders", models.ForeignKey( f"{app_label_1}.TempRider", on_delete=models.CASCADE)), ], ), ], ) This test is passing on sqlite3 and mysql. |
3337523
to
dfce113
Compare
1146288
to
a24b61e
Compare
I have created two subclasses |
a24b61e
to
0e08148
Compare
Thanks 👍 . Most (all?) of new test apps won't be necessary when we will add auto-detection of moved models. |
Should i remove all the test apps then? |
They can stay for now, we can remove them later when auto-detection is implemented. |
a03687f
to
4b22b17
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DevilsAutumn Thanks for updates 👍 I've tried this patch on a sample project (two app, three models) and both migrate
and sqlmigrate
crash 😞
$ python manage.py sqlmigrate test_one 0003
Traceback (most recent call last):
File "/django/tickets_projects/ticket_24686/manage.py", line 22, in <module>
main()
File "/django/tickets_projects/ticket_24686/manage.py", line 18, in main
execute_from_command_line(sys.argv)
File "/django/django/core/management/__init__.py", line 442, in execute_from_command_line
utility.execute()
File "/django/django/core/management/__init__.py", line 436, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/django/django/core/management/base.py", line 412, in run_from_argv
self.execute(*args, **cmd_options)
File "/django/django/core/management/commands/sqlmigrate.py", line 38, in execute
return super().execute(*args, **options)
File "/django/django/core/management/base.py", line 458, in execute
output = self.handle(*args, **options)
File "/django/django/core/management/commands/sqlmigrate.py", line 80, in handle
sql_statements = loader.collect_sql(plan)
File "/django/django/db/migrations/loader.py", line 381, in collect_sql
state = migration.apply(state, schema_editor, collect_sql=True)
File "/django/django/db/migrations/migration.py", line 132, in apply
operation.database_forwards(
File "/django/django/db/migrations/operations/fields.py", line 228, in database_forwards
to_model = to_state.apps.get_model(app_label, self.model_name)
File "/django/django/utils/functional.py", line 47, in __get__
res = instance.__dict__[self.name] = self.func(instance)
File "/django/django/db/migrations/state.py", line 566, in apps
return StateApps(self.real_apps, self.models)
File "/django/django/db/migrations/state.py", line 637, in __init__
raise ValueError("\n".join(error.msg for error in errors))
ValueError: The field test_one.MyRelatedModels.field_m2m was declared with a lazy reference to 'test_one.mymodel', but app 'test_one' doesn't provide model 'mymodel'.
The field test_one.MyRelatedModels_field_m2m.mymodel was declared with a lazy reference to 'test_one.mymodel', but app 'test_one' doesn't provide model 'mymodel'.
$ python manage.py migrate
Operations to perform:
Apply all migrations: admin, auth, contenttypes, sessions, test_one, test_two
Running migrations:
Applying test_one.0001_initial... OK
Applying test_two.0001_initial... OK
Applying test_one.0002_alter_mymodel_options... OK
Applying test_two.0002_mymodel_alter_myothermodel_field_fk... OK
Applying test_one.0003_delete_mymodel_alter_myrelatedmodels_field_fk_and_more...Traceback (most recent call last):
File "/django/tickets_projects/ticket_24686/manage.py", line 22, in <module>
main()
File "/django/tickets_projects/ticket_24686/manage.py", line 18, in main
execute_from_command_line(sys.argv)
File "/django/django/core/management/__init__.py", line 442, in execute_from_command_line
utility.execute()
File "/django/django/core/management/__init__.py", line 436, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/django/django/core/management/base.py", line 412, in run_from_argv
self.execute(*args, **cmd_options)
File "/django/django/core/management/base.py", line 458, in execute
output = self.handle(*args, **options)
File "/django/django/core/management/base.py", line 106, in wrapper
res = handle_func(*args, **kwargs)
File "/django/django/core/management/commands/migrate.py", line 356, in handle
post_migrate_state = executor.migrate(
File "/django/django/db/migrations/executor.py", line 135, in migrate
state = self._migrate_all_forwards(
File "/django/django/db/migrations/executor.py", line 167, in _migrate_all_forwards
state = self.apply_migration(
File "/django/django/db/migrations/executor.py", line 252, in apply_migration
state = migration.apply(state, schema_editor)
File "/django/django/db/migrations/migration.py", line 132, in apply
operation.database_forwards(
File "/django/django/db/migrations/operations/fields.py", line 235, in database_forwards
schema_editor.alter_field(from_model, from_field, to_field)
File "/django/django/db/backends/sqlite3/schema.py", line 123, in alter_field
if self._is_moving_model_field(old_field, new_field):
File "/django/django/db/backends/base/schema.py", line 1617, in _is_moving_model_field
old_app_label = old_field.remote_field.model._meta.app_label
AttributeError: 'str' object has no attribute '_meta'
It's critical to this project to run many experiments locally with various configurations of apps and models before moving forward.
By mistake i removed the record of dependency for altering model options in old app because of which the fields in old app were being altered after the DeleteModel() operation of moving model. I'm now working on a failing case - when i run makemigrations again after migrations of moving model are generated, a new migration for altering model options for moved model is generated. If I pass |
On SQLite, tables with relations to a moved model are rebuild (we should add a test to check performed queries and fix this), e.g. $ python manage.py sqlmigrate test_one 0003
BEGIN;
--
-- Alter field field_fk on myrelatedmodels
--
CREATE TABLE "new__test_one_myrelatedmodels" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "field_fk_id" bigint NULL REFERENCES "test_one_mymodel" ("id") DEFERRABLE INITIALLY DEFERRED);
INSERT INTO "new__test_one_myrelatedmodels" ("id", "field_fk_id") SELECT "id", "field_fk_id" FROM "test_one_myrelatedmodels";
DROP TABLE "test_one_myrelatedmodels";
ALTER TABLE "new__test_one_myrelatedmodels" RENAME TO "test_one_myrelatedmodels";
CREATE INDEX "test_one_myrelatedmodels_field_fk_id_2e7a4f46" ON "test_one_myrelatedmodels" ("field_fk_id");
--
-- Alter field field_m2m on myrelatedmodels
--
-- (no-op)
--
-- Delete model mymodel
--
-- (no-op)
COMMIT; $ python manage.py sqlmigrate test_two 0002
BEGIN;
--
-- Create model MyModel
--
-- (no-op)
--
-- Alter field field_fk on myothermodel
--
CREATE TABLE "new__test_two_myothermodel" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "field_fk_id" bigint NULL REFERENCES "test_one_mymodel" ("id") DEFERRABLE INITIALLY DEFERRED);
INSERT INTO "new__test_two_myothermodel" ("id", "field_fk_id") SELECT "id", "field_fk_id" FROM "test_two_myothermodel";
DROP TABLE "test_two_myothermodel";
ALTER TABLE "new__test_two_myothermodel" RENAME TO "test_two_myothermodel";
CREATE INDEX "test_two_myothermodel_field_fk_id_d5d44589" ON "test_two_myothermodel" ("field_fk_id");
COMMIT; Also, please fix |
Its because in
Should i still add a test to check performed queries? |
Yes, you are right. |
I was testing this patch for different use cases and I came across a scenario with potential error: Consider 3 models , If we only move Model |
4a75dfc
to
125aa2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the current approach is to set the models (both the removed and the added one) to managed=False
so that the operations performed on them are not applied in the database.
If this has other advantages over the use of SeparateDatabaseAndState
, then it may be justified, however, as the code stands -- if I understand correctly -- the added model is left unmanaged in the end (that is, take model A
from app first
to app second
, and now -- in the migrations -- second.A
is unmanaged). The next makemigrations
will create a migration to change it back to managed, which will surprise the user.
In general, the use of managed
here feels wrong, even if the code is simpler in the implementation and shorter in the generated migration -- because it introduces an unrelated concept into the generated migration code.
@shaib Thankyou for reviewing! I'll push the suggested changes shortly.
If we're marking the model |
I believe it is at least as otherwise any usage of the un-managed models in the moved-to application will crash if the moved-from managed model table doesn't exist or has the wrong table name. Keep in mind that there's no way to guarantee that a migration runs immediately after another, any other migration with similar requirement could be interleaved and try to use the moved-to unmanaged models so we must ensure that models are always in a usable state at migrations boundaries. |
un-managed model creation migration can't depend on the optional rename + marking model unmanaged migration both. If we did so, the next migrations will be optimized and the model will be created as managed(due to And if we modify the code to run But it might work if un-managed model creation migration depends only on the optional rename migration and rest remains the same. 🤔 |
aa9b733
to
1b944f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I squashed commits, rebased, and adjusted to recent changes (7dd3e69). I think it's ready for another round of reviews.
2c92e98
to
ad15d7b
Compare
cool 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't checkout the code locally, but from what I read, this looks pretty sweet 👍
Good job!
ad15d7b
to
d0b9e73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some notes on the documentation. This seems to be getting really close.
docs/topics/migrations.txt
Outdated
unless the moved model in new app defines a ``Meta.db_table`` that matches | ||
the old one. | ||
|
||
To move a model (without a table rename), we first create the new model, making |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph explains the migration operations generated by the framework for a model move, and I think this should be said very explicitly, because as the text stands, it can be understood to describe the operations the user needs to take. So I would change the text here along these lines -- clearly separate and describe what the user needs to do, and what the framework does.
Something like:
"""
To move a model from one app to another, move the model's code (the class definition) from the old app's models.py
file to the new app's models.py
, and run the makemigrations
management command. This command will generate migrations in both apps, with the following operations:
- First, create the new model, making it point to the already existing table. To make Django not attempt to create that table (which would fail), mark it
managed=False
upon creation (after this operation, we momentarily have two models pointing at the same table). - Then, to make it possible to remove the old model without deleting the table, mark it
managed=False
as well. - Finally, remove the old model and mark the new one
managed=True
(these are two migrations, as each is in its own app)
Note that this only handles the migrations part -- you will still need to handle other aspects of the move (such as changing all existing references to the model to point to its new location) yourself (in fact, you will probably need to handle these before running makemigrations
, for Django to be able to load the apps).
"""
(the above is my style of writing, Django documentation usually prefers not to use parentheses so liberally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really need this. Just sayin' :) |
d995e28
to
52fe627
Compare
Was able to do a simple case on SQLite and it moved very well 🥳 experience felt magical I think I have found an issue when moving on SQLite in the multitable inheritance case. Starting pointMigrations applied and all tables have some data app1/models.pyfrom django.db import models
class Base(models.Model):
data = models.TextField(default="Example")
class ExtendBase(Base):
field_data = models.TextField(default="Field data") Movingapp1/models.pyfrom django.db import models
from app2.models import Base
class ExtendBase(Base):
field_data = models.TextField(default="Field data") app2/models.pyfrom django.db import models
class Base(models.Model):
data = models.TextField(default="Example") I run: console
Created migration filesapp1\migrations\0006_alter_base_table.py# Generated by Django 5.1 on 2024-04-19 13:10
from django.db import migrations
class Migration(migrations.Migration):
dependencies = [
('app1', '0005_delete_mysimplemodel'),
]
operations = [
migrations.AlterModelTable(
name='base',
table='app2_base',
),
] app1\migrations\0007_alter_extendbase_base_ptr_alter_base_options.py# Generated by Django 5.1 on 2024-04-19 13:10
import django.db.models.deletion
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('app1', '0006_alter_base_table'),
('app2', '0003_base'),
]
operations = [
migrations.AlterField(
model_name='extendbase',
name='base_ptr',
field=models.OneToOneField(auto_created=True, on_delete=django.db.models.deletion.CASCADE, parent_link=True, primary_key=True, serialize=False, to='app2.base'),
),
migrations.AlterModelOptions(
name='base',
options={'managed': False},
),
]
app1\migrations\0008_delete_base.py# Generated by Django 5.1 on 2024-04-19 13:10
from django.db import migrations
class Migration(migrations.Migration):
dependencies = [
('app1', '0007_alter_extendbase_base_ptr_alter_base_options'),
('app2', '0004_alter_base_options'),
]
operations = [
migrations.DeleteModel(
name='base',
),
] app2\migrations\0003_base.py# Generated by Django 5.1 on 2024-04-19 13:10
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('app1', '0006_alter_base_table'),
('app2', '0002_initial'),
]
operations = [
migrations.CreateModel(
name='base',
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('data', models.TextField(default='Example')),
],
options={
'indexes': [],
'constraints': [],
'managed': False,
},
),
] app2\migrations\0004_alter_base_options.py# Generated by Django 5.1 on 2024-04-19 13:10
from django.db import migrations
class Migration(migrations.Migration):
dependencies = [
('app1', '0007_alter_extendbase_base_ptr_alter_base_options'),
('app2', '0003_base'),
]
operations = [
migrations.AlterModelOptions(
name='base',
options={},
),
] When migrating it crashed with the following error:
Can you confirm that you can replicate? |
@sarahboyce I was able to reproduce this issue, thanks. Looking into it. |
cca7146
to
bdea39d
Compare
This is happening because when we are moving There are two simple doc solutions for this (keeping the complications in mind) :
One complicated solution can be to create a new operation CCing @charettes to know his views on this. |
@DevilsAutumn I have found something else around Django's contenttypes framework that needs investigating and a proposed solution Roughly the issue is that ContentTypes stores the app_label and model name of all models. This looks a bit like this:
When I move error if useful
I am using the model that is used in the docs example I think this needs a test and a fix 🤔 |
Google Summer of Code 2023
Ticket: #24686
Forum: here.
Discussion: PR
Proposal: Allow moving a model between apps.
Phase 1: Focusing on operations for moving model + tests related + documentation.
Phase 2: Auto-detecting moved model, generating migrations and squashing migrations + tests related + documentation.