Fixed #24424 - sqlite schema editor causes SQL syntax error

[adnam]

Replaces pull request #4223

[MarkusH]

Closing per comment on the ticket: https://code.djangoproject.com/ticket/24424#comment:11

[adnam]

@MarkusH in your comment to the ticket you mentioned that "the fundamental problem is not just the SQLite backend, but also the autodetector that generates models without any fields".

I think this is not quite right, because the the case where I discovered this bug only happened with the SQLite schema editor. Of course, under normal circumstances it would be absurd to have a model with zero fields (not even an id field), but there is an edge case where this can happen: when a model "A" extends an abstract model "B" and is modified to extend abstract model "C". There may be other cases, the bug seems to have been first reported 5 years ago. This change only seems to break with the SQLite schema editor because SQLite has limited "ALTER TABLE" support and works around this by copying data between temporary tables. The empty SQL table is not the final destination, rather a midway step in a supposedly transactional migration. This midway step would not be manifested with MySQL, Postgres etc.

Essentially I believe the bug is in the function django.db.backends.sqlite3.schema._remake_table() because:

• The list model._meta.local_fields may be empty for various reasons (L51)
• Therefore mapping may be empty (L54)
• Therefore field_maps may be empty (L139)
• Therefore invalid SQL may be executed.

Therefore I would appreciate you reopen this PR and merge.

Thanks and regards,

Adam
(CC @timgraham)

[MarkusH]

Hey @adnam , the error doesn't only manifest on SQLite. When you run the test on MySQL it fails with

Traceback (most recent call last):
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python3.3/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python3.3/django/db/backends/mysql/base.py", line 124, in execute
    return self.cursor.execute(query, args)
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python3.3/tests/.env/lib/python3.3/site-packages/MySQLdb/cursors.py", line 219, in execute
    self.errorhandler(self, exc, value)
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python3.3/tests/.env/lib/python3.3/site-packages/MySQLdb/connections.py", line 38, in defaulterrorhandler
    raise errorvalue
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python3.3/tests/.env/lib/python3.3/site-packages/MySQLdb/cursors.py", line 205, in execute
    r = self._query(query)
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python3.3/tests/.env/lib/python3.3/site-packages/MySQLdb/cursors.py", line 372, in _query
    rowcount = self._do_query(q)
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python3.3/tests/.env/lib/python3.3/site-packages/MySQLdb/cursors.py", line 336, in _do_query
    db.query(q)
  File "/home/jenkins/workspace/pull-requests-trusty/database/mysql/label/trusty-pr/python/python3.3/tests/.env/lib/python3.3/site-packages/MySQLdb/connections.py", line 282, in query
    _mysql.connection.query(self, query)
_mysql_exceptions.OperationalError: (1090, "You can't delete all columns with ALTER TABLE; use DROP TABLE instead")

[adnam]

Hi @MarkusH, which test was that? The test I wrote has a decorator to ensure it only runs against sqlite.

The point I'm trying to make is that the empty-model problem does not manifest itself with MySQL and Postgres migrations, because these databases have greater ALTER TABLE support than sqlite; hence the test only concerns the sqlite db backend.

[MarkusH]

I'm referring to 235a839 in #4223: https://github.com/adnam/django/blob/235a8394734b63a83e9b709bc6696a66e39eb2b9/tests/schema/tests.py#L1375

The test shouldn't behave differently on any backend thus I'm -1 on the @skipUnless decorator. Side note: PG behaves as expected and ends up with a table without columns.

[adnam]

Yes, strangely postgres can have tables without columns ... it must be a new database paradigm called "no column store" \o/ ^_^

I can only emphasise that the "empty table" state is a midway point of an sqlite migration, not the intended final result. Perhaps I could add a comment explaining this to the unit test, or find somewhere else for it to live? I honestly believe the real bug is in _remake_table() as I mentioned above.