[Django] #24061: SQLite backend: Deleting last field from model: migration generates SQL with Syntax Error
Django
with Syntax Error
----------------------------------------------+--------------------
Reporter: SonOfLilit | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
{{{
$ pip freeze
Django==1.7.1
wsgiref==0.1.2
}}}
I add a model that inherits from another model, makemigrations, delete
that model, makemigrations, and the SQLite backend generates this illegal
SQL when trying to delete the last field "model_ptr" from the table:
{{{
INSERT INTO "bug_b__new" () SELECT FROM "bug_b"
}}}
Easy repro:
{{{
$ mkvirtualenv bug
$ git clone g...@github.com:SonOfLilit/django-sqlite-migrations-bug.git
$ cd django-sqlite-migrations-bug
$ python manage.py test
}}}
Here is what happens:
{{{
$ python -m pdb manage.py test
> d:\projects\galago\bug\sqlitebug\manage.py(2)<module>()
-> import os
(Pdb) c
Creating test database for alias 'default'...
Traceback (most recent call last):
File "c:\Python27\Lib\pdb.py", line 1314, in main
pdb._runscript(mainpyfile)
File "c:\Python27\Lib\pdb.py", line 1233, in _runscript
self.run(statement)
File "c:\Python27\Lib\bdb.py", line 400, in run
exec cmd in globals, locals
File "<string>", line 1, in <module>
File "manage.py", line 2, in <module>
import os
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\__init__.py", line 385, in
execute_from_command_line
utility.execute()
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\__init__.py", line 377, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\commands\test.py", line 50, in
run_from_argv
super(Command, self).run_from_argv(argv)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\base.py", line 288, in run_from_argv
self.execute(*args, **options.__dict__)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\commands\test.py", line 71, in execute
super(Command, self).execute(*args, **options)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\base.py", line 338, in execute
output = self.handle(*args, **options)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\commands\test.py", line 88, in handle
failures = test_runner.run_tests(test_labels)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\test\runner.py", line 147, in run_tests
old_config = self.setup_databases()
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\test\runner.py", line 109, in setup_databases
return setup_databases(self.verbosity, self.interactive, **kwargs)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\test\runner.py", line 299, in setup_databases
serialize=connection.settings_dict.get("TEST", {}).get("SERIALIZE",
True),
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\backends\creation.py", line 377, in create_test_db
test_flush=True,
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\__init__.py", line 115, in call_command
return klass.execute(*args, **defaults)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\base.py", line 338, in execute
output = self.handle(*args, **options)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\core\management\commands\migrate.py", line 160, in handle
executor.migrate(targets, plan, fake=options.get("fake", False))
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\migrations\executor.py", line 63, in migrate
self.apply_migration(migration, fake=fake)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\migrations\executor.py", line 97, in apply_migration
migration.apply(project_state, schema_editor)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\migrations\migration.py", line 107, in apply
operation.database_forwards(self.app_label, schema_editor,
project_state, new_state)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\migrations\operations\fields.py", line 84, in
database_forwards
schema_editor.remove_field(from_model,
from_model._meta.get_field_by_name(self.name)[0])
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\backends\sqlite3\schema.py", line 185, in remove_field
self._remake_table(model, delete_fields=[field])
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\backends\sqlite3\schema.py", line 135, in _remake_table
self.quote_name(model._meta.db_table),
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\backends\schema.py", line 99, in execute
cursor.execute(sql, params)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\backends\utils.py", line 65, in execute
return self.cursor.execute(sql, params)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\utils.py", line 94, in __exit__
six.reraise(dj_exc_type, dj_exc_value, traceback)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\backends\utils.py", line 65, in execute
return self.cursor.execute(sql, params)
File "c:\Users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\backends\sqlite3\base.py", line 485, in execute
return Database.Cursor.execute(self, query, params)
OperationalError: near ")": syntax error
Uncaught exception. Entering post mortem debugging
Running 'cont' or 'step' will restart the program
> c:\users\sonoflilit\.virtualenvs\galago\lib\site-
packages\django\db\backends\sqlite3\base.py(485)execute()
-> return Database.Cursor.execute(self, query, params)
(Pdb) print query
INSERT INTO "bug_b__new" () SELECT FROM "bug_b"
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24061>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
with Syntax Error
Reporter: SonOfLilit | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
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 SonOfLilit):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
My workaround is to comment out the problematic `RemoveField` in the
migration. Does it have any adverse effects? I'm not aware of any...
{{{
operations = [
# bug in SQLite backend
# https://code.djangoproject.com/ticket/24061
# migrations.RemoveField(
# model_name='student',
# name='user_ptr',
# ),
migrations.RemoveField(
model_name='assignment',
name='student',
),
migrations.RemoveField(
model_name='exercise',
name='students',
),
migrations.DeleteModel(
name='Student',
),
]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:1>
Django
with Syntax Error
Reporter: SonOfLilit | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by AgentK):
This can be solved by using Python's SQLite API instead of Django's.
Here's how you can do it.
import sqlite3
conn = sqlite3.connect ("/path/to/database/file")
cur = conn.cursor ()
query = "whatever query you need to execute"
data = [item1, item2]
cur.execute (query, data)
conn.commit ()
conn.close ()
I think the code is fairly self explanatory.
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:2>
Django
with Syntax Error
Reporter: SonOfLilit | Owner: coldmind
Type: Bug | Status: assigned
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* status: new => assigned
* owner: nobody => coldmind
Comment:
Checked on sqlite and django 1.7.2, issue exists - redundant `RemoveField`
has been created.
I'll check on other backends, I think issue exists not only for sqlite.
(Leaving unaccepted to be reviewed by core devs, maybe this ticket related
to another one).
I'll try to fix it will be accepted.
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:2>
Django
creates redundant removefield operation
----------------------------+------------------------------------
Reporter: SonOfLilit | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* component: Database layer (models, ORM) => Migrations
* version: 1.7 => master
* stage: Unreviewed => Accepted
Comment:
Checked on postgres and current django master, issue exists.
I'm checking this as accepted.
After fix this should be backported to 1.7 branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:3>
Django
creates redundant removefield operation
----------------------------+------------------------------------
Reporter: SonOfLilit | Owner: coldmind
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
Opened question:
{{{
truecoldmind> Can anyone explain the exact reason why while deleting model
in migration need to create foreign key deletion operations?
<truecoldmind> I'm talking about this
https://github.com/django/django/blob/572ad9a92e08797f12e547477efcce7893159cfb/django/db/migrations/autodetector.py#L619
<truecoldmind> if I understand right, when deleting model, where was a
foreign key, deleting a whole model will delete foreign key index too. But
field operation removal will be created anyway.
}}}
After this I found another related bug.
Consider following:
{{{
class ModelA(models.Model):
field1 = models.BooleanField(default=False)
class ModelB(models.Model):
another_field1 = models.TextField(blank=True)
related_field = models.ForeignKey(ModelA)
}}}
After creating inital migration, I deleted model, so migration will look
like:
{{{
class Migration(migrations.Migration):
dependencies = [
('blankapp', '0001_initial'),
]
operations = [
migrations.RemoveField(
model_name='modelb',
name='related_field',
),
migrations.DeleteModel(
name='ModelB',
),
]
}}}
And, when you will try to migrate backwards, `LookupError: App 'blankapp'
doesn't have a 'modelb' model.` will be raised, because migration will try
to add `related_field` to model which does not exists yet.
Should this issue be resolved in this ticket or in another?
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:4>
Django
creates redundant removefield operation
----------------------------+------------------------------------
Reporter: SonOfLilit | Owner: coldmind
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by MarkusH):
Regarding your first question: the autodetector needs to remove all
foreign key fields **in the same app** that point towards to model you are
removing to keep a working state.
Regarding your second question: I don't see a correlation between those
two issues. Please open a new ticket for that problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:5>
Django
creates redundant removefield operation
----------------------------+------------------------------------
Reporter: SonOfLilit | Owner: coldmind
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
{{{
<truecoldmind> MarkusH, and issue that I posted in last comment is related
if RemoveField operation on foreignkey (when deleting a whole model) is
redundant
<MarkusH> truecoldmind: a state that is working.
<MarkusH> if you keep a fk field around w/o the reverenced model being
available, it is as if you define a fk w/o the model being available.
<MarkusH> truecoldmind: Try a models.py with https://dpaste.de/WAud
<MarkusH> Django will complain and fail to start
<MarkusH> if the FK fields are not removed, you end up with an internal
state that is exactly as shown there
<shaib> MarkusH: At a glance, it looks like those operations remove the
FKs from the models to be removed, not those pointing at them.
<truecoldmind> MarkusH, lets look on example https://dpaste.de/ThsG. When
deleting ModelB, RemoveField operation will be created. For what? We
remove only ModelB, this foreignkey is the field of ModelB, and will be
deleted if ModelB will be dropped
<shaib> truecoldmind: As I said above, I think this is to make sure that
the reverse relationships are removed from the models that remain.
<MarkusH> ok, I'll have a more detailed look at the code
<truecoldmind> shaib, in that case foreignkey is only part of ModelB and
does not affects other models
<shaib> no, ModelA has a modelb attribute. Look up reverse relationships.
<shaib> Sorry, modelb_set.
<truecoldmind> shaib, it have it because foreignkey is present in model.
How it is related to schema migration ?
<MarkusH> This is indeed weired.
<shaib> migrations also manage the models in memory (so that data
migrations can use them)
<MarkusH> I don't see a reason for the RemoveField operations (yet)
<MarkusH> truecoldmind: the reverse relation for a FK field (e.g. FK from
Book to Author: book_set) is created by the FK field
<MarkusH> shaib: please correct me if I'm wrong ;)
<MarkusH> So, if I delete the Book model (and thus don't have a FK to
Author anymore), there can't be a reverse relation
<shaib> It would be better IMO if removing a model would clean up all
reverse relations created by FKs in that model, but I'm not sure this is
the case.
<apollo13> that ^
<truecoldmind> MarkusH, yes. and with Book as OneToOne field will be as
author.book. But I don't understand how it is related to table drop
operation
<MarkusH> shaib: that's what I'd expected as well
<MarkusH> I guess I'm missing something
<apollo13> if you look at the comment of that function, might be that the
optimizer is missing something…
<MarkusH> apollo13: I don't see the reason for adding the RemoveField
operations in the first place
<apollo13> maybe simplicity for now
<truecoldmind> apollo13, what simplicity? If you look at #24061, this
thing is producing at least 2 bugs (first is it the title of ticket,
second is in comments)
<ticketbot> https://code.djangoproject.com/ticket/24061
<apollo13> so?
<truecoldmind> apollo13, so what is the exact reason for RemoveField in
this concrete example?
<apollo13> truecoldmind: as I said: maybe simplicity
<shaib> I'll elaborate: Perhaps the people who wrote migrations realized
they need to clean up reverse relationships and thought it was simpler to
do in the autodetector than in the model-deletion operation.
}}}
----
Some thoughts how it can be resolved:
{{{
<truecoldmind> maybe another operation is needed here, where only
`state_forwards` (without database operations) will be available?
<truecoldmind> for example, we can create `StateOperation(Operation)`,
then prohibit database operations here, so we will can do cleanup in that
way. Or it is must be hidden from user that will be reading the migration
source?
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:6>
Django
----------------------------+------------------------------------
Reporter: SonOfLilit | Owner: coldmind
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:7>
Django
----------------------------+------------------------------------
Reporter: SonOfLilit | Owner: coldmind
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by coldmind:
Old description:
New description:
{{{
$ pip freeze
Django==1.7.1
wsgiref==0.1.2
}}}
I add a model that inherits from another model, makemigrations, delete
that model, makemigrations, and the SQLite backend generates this illegal
SQL when trying to delete the last field "model_ptr" from the table:
{{{
INSERT INTO "bug_b__new" () SELECT FROM "bug_b"
}}}
Easy repro:
{{{
$ mkvirtualenv bug
$ git clone g...@github.com:SonOfLilit/django-sqlite-migrations-bug.git
$ cd django-sqlite-migrations-bug
$ python manage.py test
}}}
Here is what happens: https://dpaste.de/LYqP
----
'''RELATED ISSUE:''
Consider following:
{{{
class ModelA(models.Model):
field1 = models.BooleanField(default=False)
class ModelB(models.Model):
another_field1 = models.TextField(blank=True)
related_field = models.ForeignKey(ModelA)
}}}
After creating inital migration, I deleted model, so migration will look
like:
{{{
class Migration(migrations.Migration):
dependencies = [
('blankapp', '0001_initial'),
]
operations = [
migrations.RemoveField(
model_name='modelb',
name='related_field',
),
migrations.DeleteModel(
name='ModelB',
),
]
}}}
And, when you will try to migrate backwards, LookupError: App 'blankapp'
doesn't have a 'modelb' model. will be raised, because migration will try
to add related_field to model which does not exists yet.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:8>
Django
----------------------------+------------------------------------
Reporter: SonOfLilit | Owner: coldmind
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
{{{
Sorry, I was under the impression there was an optimiser rule for this,
but I just checked in the rules list [1] and there's not one, so I was
mistaken. The fix is to add a collapse rule there.
Reverse relationship cleanup should already be handled by DeleteModel; if
not, that's a _separate_ bug from the RemoveFields not being collapsed in.
Both would need the cleanup logic as both are possible.
[1]
https://github.com/django/django/blob/master/django/db/migrations/optimizer.py#L29
On Wed, Jan 7, 2015 at 10:58 AM, coldmind <soka...@yandex.ru> wrote:
Thanks for your response!
I don't understand clearly what you mean in "if you're still seeing
them".
Autodetector should add RemoveField to migrations only in these
specific cases, and not always, right?
And what about situation when it is not a specific case, but we need
to do reverse-relationship cleanup?
It maybe should be placed as I proposed in gist.
On 01/07/2015 08:42 PM, Andrew Godwin wrote:
> It's not necessary, and should be optimised away by the optimiser -
if you're still seeing them then the optimiser is the place to patch.
>
> Your proposed patch would remove them entirely and break the
operational dependencies system (sometimes you need to remove a foreignkey
first, so you can remove another model, so you can remove the original
model).
>
> Andrew
>
> On Mon, Jan 5, 2015 at 8:26 AM, Andriy Sokolosvkiy
<soka...@yandex.ru> wrote:
>
> Hi!
> I need some advice from you with ticket
https://code.djangoproject.com/ticket/24061, because you wrote those part
of code.
>
> The main question is - what is the exact purpose of RemoveField,
when deleting a model?
> If it is only for state cleanup, I think this code can be moved
like this, to prevent bugs, that RemoveField brings:
> https://gist.github.com/coldmind/75325c5649e010367422
>
>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:9>
Django
Reporter: SonOfLilit | Owner: coldmind
Type: Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* type: Bug => Cleanup/optimization
Comment:
Seems some commit (maybe
https://github.com/django/django/commit/fdc2cc948725866212a9bcc97b9b7cf21bb49b90)
fixed backwards migration. It is working now, at least it is not crashing
(checked two issues from ticket description).
But maybe we can at least add an optimizer rule here to make migration
more cleaner.
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:10>
Django
--------------------------------------+------------------------------------
Reporter: SonOfLilit | Owner: coldmind
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* status: assigned => closed
* resolution: => fixed
Comment:
Could you open a new ticket for your thought on "make migration more
clearner". I'll mark this as fixed.
--
Ticket URL: <https://code.djangoproject.com/ticket/24061#comment:11>