alter_column generates invalid SQL for PostgreSQL

[Erik Allik <eallik@…>]

I've changed a field to allow NULLs by adding blank=True and null=True, then autogenerated a migration and ran it. This is what (the relevant part of) my migration looks like:

class Migration:
    def forwards(self, orm):
        db.alter_column('localsite_dish', 'price', models.PositiveIntegerField(_('price'), null=True, blank=True))
    
    def backwards(self, orm):
        db.alter_column('localsite_dish', 'price', models.PositiveIntegerField(_('price')))

When running django-admin.py migrate, I get the following output:

Running migrations for localsite:
 - Migrating forwards to 0015_change_dish_price_to_null.
 > localsite: 0015_change_dish_price_to_null
   = ALTER TABLE "localsite_dish" ALTER COLUMN "price" TYPE None, ALTER COLUMN "price" DROP DEFAULT, ALTER COLUMN "price" DROP NOT NULL; []

The part that says TYPE None is invalid.

[Erik Allik <eallik@…>]

Changing PositiveIntegerField? to IntegerField? solves the problem as the column types are really the same, PositiveIntegerField? just adds a non-zero check constraint on creation. So chaning the alter_column migration to use IntegerField? instead is perfectly valid. But the bug still remains of course...

[mark.ellul@…]

Hi,

I have hit the issue with a ManyToManyField?, I put it in my model without a null=True then added it... and bam hit this issue.

Just thought I'd let you know!

[andrew]

Mark, that's a different issue, where the autodetection is incorrectly thinking that changing ManyToManyFields? needs database changes - the fix for that is mostly written.

[mark.ellul@…]

thanks for you feedback Andrew... I deleted the migrations in question and autogenerated them again.

Hats off to you mate, this is one of the best extension to a framework I have ever used!!!!!

[sealibora@…]

After hitting the same problem and looking a bit at the code:
TYPE None is caused by method alter_column in db/generic.py lines 223-224:

# Add _id or whatever if we need to
if not explicit_name:

field.set_attributes_from_name(name)

As explicit_name is True by default then set_attributes_from_name() does not get called resulting field.column not being set.

That in turn will cause an KeyError? in django/db/models/fields/init.py in method db_type of class Field:

data = DictWrapper?(self.dict, connection.ops.quote_name, "qn_")
try:

return connection.creation.data_types[self.get_internal_type()] % data

except KeyError?:

return None

because both PositiveIntegerField? and PositiveSmallIntegerField? need 'column' to produce correct data types for Postgres (snip from django/db/backends/postgresql/creation.py):

'PositiveIntegerField?': 'integer CHECK ("%(column)s" >= 0)',
'PositiveSmallIntegerField?': 'smallint CHECK ("%(column)s" >= 0)',

So one could call alter_column like this:

db.alter_column('localsite_dish', 'price', models.PositiveIntegerField?(_('price'), null=True, blank=True), explicit_name=False)

but this would bring us to the next problem: TYPE 'integer CHECK ("price" >= 0)' and that's a problem that can't be solved without modifying south.
So right now i use a workaround found by Eric, renaming PositiveIntegerField? to IntegerField? and PositiveSmallIntegerField? to SmallIntegerField?.

I think a bigger problem will be changing field types, then definetely the CHECKs have to go. The solution to this bug should probably fix #83 too ;)
Thanks for south

[andrew]

Excellent bug reporting and detective work there! Fixed in [223].

[sealibora@…]

a test for postgres and a fix for #125

[sealibora@…]

Hei Andrew,
your solution unfortunately broke altering Float and DateTime? fields, at least in Postgres. I attached a little patch that adds a new test 'test_alter_column_postgres_multiword' and a quick fix that makes the test pass.

Didn't test with MySQL as I don't have it set up :(

[andrew]

Ack, I knew taking only the first word was a bad choice, but couldn't think of a counterexample, but datetime is so obvious now.

Patch applied in [227] and I then ran the test against MySQL and fixed it/made a few other changes in [228]. Thanks for picking up on that so quick, and my apologies for being a dumbass.