Ticket #782 (assigned defect)

Opened 4 years ago

Last modified 3 years ago

Integrity appears after removing null=True from field

Reported by: dloewenherz@… Owned by: andrew
Priority: major Milestone: 1.0
Component: migrations Version: 0.7.3
Keywords: Cc:

Description

I have a field called cc_zip_code which previously was allowed to be null. I just changed removed null=True from the field definition. After creating and running the migration, South throws an IntegrityError? on the field.

Running migrations for app:
 - Migrating forwards to 0033_auto__chg_field_client_cc_zip_code__chg_field_client_cc_exp_month__chg.
 > app:0031_auto__del_field_client_name__del_field_client_cc_first_name__del_field
 > app:0032_auto__del_field_client_cc_token__del_field_client_cc_invalid__del_fiel
 > app:0033_auto__chg_field_client_cc_zip_code__chg_field_client_cc_exp_month__chg
Traceback (most recent call last):
  File "./manage.py", line 14, in <module>
    execute_manager(settings)
  File "/usr/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 438, in execute_manager
    utility.execute()
  File "/usr/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 379, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python2.7/site-packages/django/core/management/base.py", line 191, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/usr/local/lib/python2.7/site-packages/django/core/management/base.py", line 220, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python2.7/site-packages/south/management/commands/migrate.py", line 105, in handle
    ignore_ghosts = ignore_ghosts,
  File "/usr/local/lib/python2.7/site-packages/south/migration/__init__.py", line 191, in migrate_app
    success = migrator.migrate_many(target, workplan, database)
  File "/usr/local/lib/python2.7/site-packages/south/migration/migrators.py", line 221, in migrate_many
    result = migrator.__class__.migrate_many(migrator, target, migrations, database)
  File "/usr/local/lib/python2.7/site-packages/south/migration/migrators.py", line 292, in migrate_many
    result = self.migrate(migration, database)
  File "/usr/local/lib/python2.7/site-packages/south/migration/migrators.py", line 125, in migrate
    result = self.run(migration)
  File "/usr/local/lib/python2.7/site-packages/south/migration/migrators.py", line 99, in run
    return self.run_migration(migration)
  File "/usr/local/lib/python2.7/site-packages/south/migration/migrators.py", line 81, in run_migration
    migration_function()
  File "/usr/local/lib/python2.7/site-packages/south/migration/migrators.py", line 57, in <lambda>
    return (lambda: direction(orm))
  File "/Users/dan/Web/codeyouridea/app/migrations/0033_auto__chg_field_client_cc_zip_code__chg_field_client_cc_exp_month__chg.py", line 12, in forwards
    db.alter_column('app_client', 'cc_zip_code', self.gf('django.db.models.fields.CharField')(default='90212', max_length=9))
  File "/usr/local/lib/python2.7/site-packages/south/db/sqlite3.py", line 180, in alter_column
    name: self._column_sql_for_create(table_name, name, field, explicit_name),
  File "/usr/local/lib/python2.7/site-packages/south/db/sqlite3.py", line 83, in _remake_table
    self._copy_data(table_name, temp_name, renames)
  File "/usr/local/lib/python2.7/site-packages/south/db/sqlite3.py", line 114, in _copy_data
    self.quote_name(src),
  File "/usr/local/lib/python2.7/site-packages/south/db/generic.py", line 150, in execute
    cursor.execute(sql, params)
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/util.py", line 34, in execute
    return self.cursor.execute(sql, params)
  File "/usr/local/lib/python2.7/site-packages/django/db/backends/sqlite3/base.py", line 234, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.IntegrityError: _south_new_app_client.cc_zip_code may not be NULL

If I try to run the migration again, it says:

django.db.utils.DatabaseError: table "_south_new_app_client" already exists

Change History

comment:1 Changed 4 years ago by andrew

  • Status changed from new to infoneeded
  • Milestone set to 1.0

Hmm, SQLite's support for modifying columns is somewhat dodgy, mostly because it doesn't actually exist in SQLite and we had to hack around it. It sounds like the alteration went wrong somehow, and now you can't do it again.

You could try deleting the temporary '_south_new...' table and trying again, but this is probably one of the several bugs in our SQLite implementation we have open on this tracker. If you could reduce it to a simple migration that always failed, we might be able to trace it down more.

comment:2 Changed 3 years ago by anonymous

  • Status changed from infoneeded to assigned

I've been having the latter problem a lot as well.
"_south_new..." table already exists is blocking me every time I try to run a local migration.

comment:3 Changed 3 years ago by anonymous

I have something similar, I just added 2 tables and I and now it keeps saying the same error, what ever command I make. Perhaps is the same problem.

comment:4 Changed 3 years ago by Jean-Baptiste Lab <jean-baptiste.lab@…>

I've had a similar issue (with 0.7.5) when changing an IntegerField? from null=True to null=False with SQLite.

The error occurs when migrating the data from the old table to the new one (the actual column alteration is fine).

In my case, the old table had some entries with null values in the "order" column.

The exact SQL query that gives trouble is the one that transfer the data, which is similar to this:

INSERT INTO "_south_new_<your_table_name>" ("id", "route_id", "post_id", "order") SELECT "id", "route_id", "post_id", "order" FROM "<your_table_name>";

The 2nd part of the query is the culprit because the "order" column is null for certain rows. There are possible work arounds, I'll summarize 2 here.

The first one involves modifying south code (in the _copy_data method of south/db/sqlite3.py) to issue a different query, much like this one:

INSERT INTO "_south_new_<your_table_name>" ("id", "route_id", "post_id", "order") SELECT "id", "route_id", "post_id", coelesce("order", <default_value>) FROM "<your_table_name>";

Here is my _copy_data method:

    def _copy_data(self, src, dst, field_renames={}):
        "Used to copy data into a new table"
        # Make a list of all the fields to select
        cursor = self._get_connection().cursor()
        src_fields = [[column_info[0], column_info[-1]] for column_info in self._get_connection().introspection.get_table_description(cursor, src)]
        dst_fields = [column_info[0] for column_info in self._get_connection().introspection.get_table_description(cursor, dst)]
        src_fields_new = []
        dst_fields_new = []
        for field in src_fields:
            if field[0] in field_renames:
                dst_fields_new.append(self.quote_name(field_renames[field[0]]))
            elif field[0] in dst_fields:
                dst_fields_new.append(self.quote_name(field[0]))
            else:
                continue
            if not field[1]:
                src_fields_new.append(self.quote_name(field[0]))
            else:
                src_fields_new.append('coalesce(' + self.quote_name(field[0]) + ', 1)')
        # Copy over the data
        self.execute("INSERT INTO %s (%s) SELECT %s FROM %s;" % (
            self.quote_name(dst),
            ', '.join(dst_fields_new),
            ', '.join(src_fields_new),
            self.quote_name(src),
        ))

I gave up on that path however, as it hard-codes the default value (see the src_fields_new.append('coalesce(' + self.quote_name(field[0]) + ', 1)') line).
I'm pretty sure it is possible to get the actual default model field value from the model definition, but I'm not familiar with the code at all and have some time constraints...

The other possibility (which I ended up using) is to manipulate the objects with null values in the forwards() method of the migration:

    def forwards(self, orm):
        for one_obj in orm.YourModel.objects.all():
                if not one_obj.order:
                    one_obj.order = <your default value>
                    one_obj.save()
        # Changing field 'YourModel.order'
        db.alter_column('your_table', 'order', self.gf('django.db.models.fields.IntegerField')())

Which worked like a charm.

I hope this gives a South developer with more insight some good pointers as to what needs to be done to get these sorts of migrations to work automatically without manual tweaking.

Thanks for an absolutely brilliant tool!

comment:5 follow-up: ↓ 6 Changed 3 years ago by andrew

Hmm, the problem here is that the model isn't readily available inside the backend methods (though this is changing at some point in the future) - I think that workaround will have to suffice for now until that change lands.

comment:6 in reply to: ↑ 5 Changed 3 years ago by Jean-Baptiste Lab <jean-baptiste.lab@…>

Replying to andrew:

Hmm, the problem here is that the model isn't readily available inside the backend methods (though this is changing at some point in the future) - I think that workaround will have to suffice for now until that change lands.

Actually, the SQLite PRAGMA table_info (see http://www.sqlite.org/pragma.html#pragma_table_info) also yields the column's default value (if any) so it may be possible to get some sane (and user-controlled! :) defaults in there.

I can see though that django's sqlite3 introspection _table_info() does not handle this so custom code should be added or a patch submitted to django.

On a side note: I don't know how other backends handle this type of migrations.

comment:7 follow-up: ↓ 8 Changed 3 years ago by andrew

Ah, but Django doesn't store default values in the database, it applies them itself before they even hit the database, so that's why that is being ignored.

I believe the other backends may just raise an error on this type of migration, come to think of it - applying NOT NULL to a column with NULL values isn't possible, after all.

comment:8 in reply to: ↑ 7 Changed 3 years ago by Jean-Baptiste Lab <jean-baptiste.lab@…>

Replying to andrew:

Ah, but Django doesn't store default values in the database, it applies them itself before they even hit the database, so that's why that is being ignored.

That's true if the model field is declared as null=True, blank=False, however, when a model field is declared as null=False, default=<default value>, the SQL issued (at least for the SQLite backend, I've not checked others) contains a DEFAULT statement, for instance:

some_bool = models.BooleanField(default=False)

translates into SQL as:

"some_bool" bool NOT NULL DEFAULT False

which would allow migrations to make use of that default value.

comment:9 follow-up: ↓ 10 Changed 3 years ago by andrew

Do you mean the SQL emitted by South, or Django? South emits default values on column creation (and then clears them afterwards on most backend), Django (I thought) didn't.

comment:10 in reply to: ↑ 9 Changed 3 years ago by Jean-Baptiste Lab <jean-baptiste.lab@…>

Replying to andrew:

Do you mean the SQL emitted by South, or Django? South emits default values on column creation (and then clears them afterwards on most backend), Django (I thought) didn't.

I think it depends on whether you specify a default value in the model definition. If yes, then the SQL issued by both South and Django contains this DEFAULT clause, if not, the default value is only applied when updating the table data and thrown away afterwards (I think that's what the keep_default=False in the migration suggests).

comment:11 follow-up: ↓ 12 Changed 3 years ago by Andrew Swihart

This seems crazy to me, why can't you apply NOT NULL to a column w/ NULL values. Why can't you convert existing NULL values to an empty string, for example with Char fields?

comment:12 in reply to: ↑ 11 Changed 3 years ago by Jean-Baptiste Lab <jean-baptiste.lab@…>

Replying to Andrew Swihart:

This seems crazy to me, why can't you apply NOT NULL to a column w/ NULL values. Why can't you convert existing NULL values to an empty string, for example with Char fields?

This is exactly what I suggested (using the forwards() approach) in my previous comment 4.

I believe that South ought to be handle migrating a field from null=True to null=False automatically and I'm pretty sure it can be done, I'll try to see if I can produce a patch.

Note: See TracTickets for help on using tickets.