Ticket #288 (closed defect: fixed)

Opened 5 years ago

Last modified 5 years ago

altering a column in sqlite fails when Meta.unique_together is defined

Reported by: legutierr@… Owned by: andrew
Priority: critical Milestone: 0.7
Component: migrations Version: 0.6.2
Keywords: sqlite3.OperationalError: Cannot add a NOT NULL column with default value NULL Cc:

Description

There is a problem when altering a column in sqlite.

I debugged the problem by stepping through the code into the south.db.sqlite3.DatabaseOperations object. An exception is thrown at line 96.

The failure takes place because the object thinks for some reason that the Meta.unique_together items are fields to be included in the field list of the SQL create statement. Of course, the items in the Meta.unique_together list are the names of actual listed fields so SQLite raises an exception.

This is what I saw stepping through the code, thrown by cursor.execute(sql) at line 96:

OperationalError: 'duplicate column name: name'

This is what I saw when running the program outside of pdb:

sqlite3.OperationalError: Cannot add a NOT NULL column with default value NULL

I cannot find the origin of the problem or a fix, but it is relevant that self._fields["<table_name>"] is an incorrect list (i.e. it lists the Meta.unique_together items as fields, which list is then used to construct the sql), so the problem may be in the constructor.

I don't have time to learn the system enough to create a patch, but this may be enough of a start to fix the problem.

Change History

comment:1 Changed 5 years ago by andrew

  • Status changed from new to assigned
  • Milestone changed from 0.6.3 to 0.7

Mmm, this needs to go in with the SQLite module overhaul we're going to have. Scheduling for 0.7.

comment:2 Changed 5 years ago by legutierr@…

I was just reviewing this, and the second exception that I pasted in (the one about the default value) is the wrong one. It actually prints a different exception that mentions something about the duplicate name (nothing about not null and defaults), but I don't have the literal text now because I've worked around the problem and can't regenerate.

Everything else is correct, though. I just copied the wrong thing in.

comment:3 Changed 5 years ago by legutierr@…

I believe that I have located the problem. It is that the db._fields variable is being populated directly by a regex operating on a table schema definition generated by SQL, without validation. The regex assumes that anything inside quotes is the name of a field, and anything following a quoted string before a comma is a type. This means that the contents of a UNIQUE clause are considered field definitions, which is, of course, wrong.

This happens in the _populate_current_structure() method of south/db/sqlite3.py at line 35.

The following code is by no means a final fix, but at least it solves the problem I had, which is the incorrect inclusion of the UNIQUE clause in the field list.

    def _populate_current_structure(self, table_name, force=False):
        # get if we don't have it already or are being forced to refresh it
        if force or not table_name in self._fields.keys():
            cursor = connection.cursor()
            cursor.execute(GET_TABLE_DEF_SQL % table_name)
            create_table = cursor.fetchall()[0][0]
            first = create_table.find('(')
            last = create_table.rfind(')')
            # rip out the CREATE TABLE xxx ( ) and only get the field definitions plus
            # add the trailing comma to make the next part easier
            fields_part = create_table[first+1: last] + ','

            ###
            ### THE FIX...
            ### REMOVE ANY UNIQUE CLAUSE, PROBABLY AT THE END, BUT MAYBE ANYWHERE
            ###
            fields_part = "".join(re.split(r'UNIQUE *\([^\)]*\)', fields_part))

            # pull out the field name and definition for each field
            self._fields[table_name] = \
                re.findall(r'"(\S+?)"(.*?),', fields_part, re.DOTALL)

comment:4 Changed 5 years ago by andrew

Thanks for that; we're planning to solve this problem more permanently by using Django's introspection features for SQLite (providing they work properly), and if not, using a decent SQL parser! I'll leave this on the 0.7 queue to make sure it gets included.

comment:5 Changed 5 years ago by andrew

  • Status changed from assigned to closed
  • Resolution set to fixed

This now appears to work perfectly fine against the most recent 0.7 commits, where SQLite support is all nice. Closing.

Note: See TracTickets for help on using tickets.