[Django] #28714: Django Makemigrations don't check if field name from indexes exists on model before creating a migration.
Django
model before creating a migration.
-------------------------------------+-------------------------------------
Reporter: Gabriel | Owner: nobody
Type: Bug | Status: new
Component: | Version: 1.11
Migrations | Keywords: indexes, migrate,
Severity: Normal | makemigrations
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
When using makemigration with indexes on class Meta in the Model Django
don't check whether the field name in fields parameter exists on Model.
Because of this both the migration files (in migrations/*.py) and the
model class on model.py will have a index that referrer to a non existing
field, so when **manage.py migrate** is run, the expection
FieldDoesNotExist will be raised, but in this moment the programmer will
have to edit both the migration file and model class to fix the error,
especially if is not possible to remove the migrations file or is more
convenient to edit the migration in projects with many Apps and many
dependency between those Apps.
Would be more convenient if makemigration before generate the new
migration files did this verification.
Example of model's code with wrong field name ("''names''") on indexes:
{{{
class ItemType(models.Model):
name = models.CharField(_('Name'), unique=True, max_length=255)
class Meta:
managed = True
db_table = 'item_type'
ordering = ['name']
indexes = [
models.Index(fields=['id'],
name='item_type_index_1'),
models.Index(fields=['-id'],
name='item_type_index_2'),
models.Index(fields=['names'],
name='item_type_index_3'),
models.Index(fields=['-names'],
name='item_type_index_4')
]
}}}
This code will not raise any exception on **makemigrations**, only the
generated code on migration file will when **migrate** is used:
{{{
migrations.AddIndex(
model_name='itemtype',
index=models.Index(fields=['names'],
name='item_type_index_3'),
),
migrations.AddIndex(
model_name='itemtype',
index=models.Index(fields=['-names'],
name='item_type_index_4'),
),
}}}
Traceback (manage.py migrate):
{{{
Traceback (most recent call last):
File "D:\Project-2015\cultural\manage.py", line 10, in <module>
execute_from_command_line(sys.argv)
File "C:\Python36\lib\site-packages\django\core\management\__init__.py",
line 364, in execute_from_command_line
utility.execute()
File "C:\Python36\lib\site-packages\django\core\management\__init__.py",
line 356, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "C:\Python36\lib\site-packages\django\core\management\base.py",
line 283, in run_from_argv
self.execute(*args, **cmd_options)
File "C:\Python36\lib\site-packages\django\core\management\base.py",
line 330, in execute
output = self.handle(*args, **options)
File "C:\Python36\lib\site-
packages\django\core\management\commands\migrate.py", line 204, in handle
fake_initial=fake_initial,
File "C:\Python36\lib\site-packages\django\db\migrations\executor.py",
line 115, in migrate
state = self._migrate_all_forwards(state, plan, full_plan, fake=fake,
fake_initial=fake_initial)
File "C:\Python36\lib\site-packages\django\db\migrations\executor.py",
line 145, in _migrate_all_forwards
state = self.apply_migration(state, migration, fake=fake,
fake_initial=fake_initial)
File "C:\Python36\lib\site-packages\django\db\migrations\executor.py",
line 244, in apply_migration
state = migration.apply(state, schema_editor)
File "C:\Python36\lib\site-packages\django\db\migrations\migration.py",
line 129, in apply
operation.database_forwards(self.app_label, schema_editor, old_state,
project_state)
File "C:\Python36\lib\site-
packages\django\db\migrations\operations\models.py", line 788, in
database_forwards
schema_editor.add_index(model, self.index)
File "C:\Python36\lib\site-packages\django\db\backends\base\schema.py",
line 331, in add_index
self.execute(index.create_sql(model, self))
File "C:\Python36\lib\site-packages\django\db\models\indexes.py", line
65, in create_sql
sql_parameters = self.get_sql_create_template_values(model,
schema_editor, using)
File "C:\Python36\lib\site-packages\django\db\models\indexes.py", line
48, in get_sql_create_template_values
fields = [model._meta.get_field(field_name) for field_name, order in
self.fields_orders]
File "C:\Python36\lib\site-packages\django\db\models\indexes.py", line
48, in <listcomp>
fields = [model._meta.get_field(field_name) for field_name, order in
self.fields_orders]
File "C:\Python36\lib\site-packages\django\db\models\options.py", line
619, in get_field
raise FieldDoesNotExist("%s has no field named '%s'" %
(self.object_name, field_name))
django.core.exceptions.FieldDoesNotExist: ItemType has no field named
'names'
}}}
Attached is this model example with a incorrect index field name that will
not raise exception on makemigration only on migrate.
This was checked on Django 1.11.5 with Python 3.6.2 on Windows 10 64bits.
To reproduce this error:
1. Add the Item App to a existing Django project
2. Run manage.py makemigrations item
3. Run manage.py migrate
--
Ticket URL: <https://code.djangoproject.com/ticket/28714>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
model before creating a migration.
-------------------------------------+-------------------------------------
Reporter: Gabriel | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: indexes, migrate, | Triage Stage:
makemigrations | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "item_app.zip" added.
Folder with App named item.
Django
model before creating a migration.
-------------------------------------+-------------------------------------
Reporter: Gabriel | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: indexes, migrate, | Triage Stage:
makemigrations | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> File "C:\Python36\lib\site-
> packages\django\core\management\__init__.py", line 364, in
> execute_from_command_line
> utility.execute()
New description:
Traceback (manage.py migrate):
To reproduce this bug:
1. Add the Item App to a existing Django project
2. Run manage.py makemigrations item
3. Run manage.py migrate
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28714#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Gabriel | Owner: nobody
Cleanup/optimization |
Component: Core (System | Version: 1.11
checks) |
Severity: Normal | Resolution:
Keywords: indexes, migrate, | Triage Stage: Accepted
makemigrations |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => Cleanup/optimization
* component: Migrations => Core (System checks)
* stage: Unreviewed => Accepted
Comment:
My first thought is that adding system checks for the `Index` class is the
way to solve this.
--
Ticket URL: <https://code.djangoproject.com/ticket/28714#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Core (System | Version: 1.11
checks) |
Severity: Normal | Resolution:
Keywords: indexes, migrate, | Triage Stage: Accepted
makemigrations |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => hui shang
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28714#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Gabriel | Owner: hui shang
Type: | Status: assigned
Cleanup/optimization |
Component: Core (System | Version: 1.11
checks) |
Severity: Normal | Resolution:
Keywords: indexes, migrate, | Triage Stage: Accepted
makemigrations |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/9303 PR]
Thanks Gabriel report this.
I add a check function in ''django.core.checks.model_checks'', to check
all field name in indexes are valid field of the model.
Let me know if I can make it better.
--
Ticket URL: <https://code.djangoproject.com/ticket/28714#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Gabriel | Owner:
Cleanup/optimization | Status: closed
Component: Core (System | Version: 1.11
checks) |
Keywords: indexes, migrate, | Triage Stage: Accepted
makemigrations |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"f1aa58479cdc6051dd2e97feca2d584c43aee1e7" f1aa584]:
{{{
#!CommitTicketReference repository=""
revision="f1aa58479cdc6051dd2e97feca2d584c43aee1e7"
Fixed #28714 -- Added system checks for invalid model field names in
Meta.indexes.
Thanks Gabriel for the report and Adam Johnson for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28714#comment:5>