1. Andrew Godwin
  2. south


shaib  committed 80430dc

Make Sqlite backend comply with new "no defaults left in db" policy

  • Participants
  • Parent commits 96b2fa6
  • Branches dont-leave-defaults

Comments (0)

Files changed (1)

File south/db/sqlite3.py

View file
         self._remake_table(table_name, added={
             field.column: self._column_sql_for_create(table_name, name, field, False),
+        # Now, remove any defaults
+        field._suppress_default = True
+        self.alter_column(table_name, name, field)
  1. charettes

    shaib this adds an extra CREATE INDEX statement to self.deferred_sql and raises the following exception when execute_deferred_sql is called: DatabaseError: index test_addc_6eb405ee already exists.

    Here's a failing assertion to trigger the issue.

    diff -r 6c69cb5f3710 south/tests/db.py
    --- a/south/tests/db.py Fri Jan 04 20:08:20 2013 +0000
    +++ b/south/tests/db.py Sat Jan 26 04:14:15 2013 -0500
    @@ -288,6 +288,9 @@
             val = db.execute("SELECT user_id FROM test_addc")[0][0]
             self.assertEquals(val, None)
             db.delete_column("test_addc", "add1")
    +        # make sure adding an indexed field works
    +        db.add_column("test_addc", "add2", models.CharField(max_length=15, db_index=True))
    +        db.execute_deferred_sql()
     def test_add_nullbool_column(self):
    1. shaib author

      I can see how that comes about... I have several comments:

      1) If I understand correctly, this means that even before my change, a migration with add_column+alter_column on the same column would break in a similar way; which is, I think, already a problem (although clearly a smaller one).

      2) Is there a reason for creating single-column indexes in deferred sql (rather than "inline")?

      3) Using alter_column there was the lazy way to achieve the goal (and "lazy" here is not intended as a derogative); if it doesn't work well, we'll find another way.

      4) Please add your assertion to the test suite (either as you did, or in its own test), so it's easier to make sure this works well on all backends.

      Thanks, Shai.

      1. charettes
        1. Yes because CREATE INDEX statements are are added to deferred_sql when no index exists in the database;
        2. We should ask Andrew Godwin for that;
        3. We might have to find another way, do you have any idea yet?
        4. I think the assertion makes sense in test_add_column so I'll leave it there. What about including it with the actual fix instead of commiting a failing assertion instead?

        We should really try to set a CI server for south, maybe the Divio or the DSF guys could host it somehow.

        1. Andrew Godwin repo owner

          I don't mind about inline vs. deferred index execution, I think it's like that because one of the backends needed it that way.

          As for CI servers, that's a Hard Problem, since said server needs at least three databases and three versions of Django running, which is why I never got round to it.

        2. shaib author

          I made PR#114 which fixes the problem using a less lazy way -- since the table is recreated anyway and data copied into it, I modified creation not to use a default in the definition at all, and instead modified the data-copying command to include the default in every row.

     def _get_full_table_description(self, connection, cursor, table_name):
         cursor.execute('PRAGMA table_info(%s)' % connection.ops.quote_name(table_name))
         The argument is accepted for API compatibility with the generic
         DatabaseOperations.alter_column() method.
+        # Change nulls to default if needed
+        if not field.null and field.has_default():
+            params = {
+                "column": self.quote_name(name),
+                "table_name": self.quote_name(table_name)
+            }            
+            self._update_nulls_to_default(params, field)
         # Remake the table correctly
+        field._suppress_default = True
         self._remake_table(table_name, altered={
             name: self._column_sql_for_create(table_name, name, field, explicit_name),