Ticket #125 (closed defect: fixed)

Opened 5 years ago

Last modified 5 years ago

alter_column generates invalid SQL for PostgreSQL

Reported by: Erik Allik <eallik@…> Owned by: andrew
Priority: major Milestone: 0.5
Component: databaseapi Version: 0.6-pre
Keywords: alter_column Cc: eallik@…

Description

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.

Attachments

125.patch (4.1 KB) - added by sealibora@… 5 years ago.
a test for postgres and a fix for #125

Change History

comment:1 Changed 5 years ago by 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...

comment:2 Changed 5 years ago by 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!

comment:3 Changed 5 years ago by andrew

  • Status changed from new to assigned

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.

comment:4 Changed 5 years ago by andrew

  • Milestone set to 0.5

comment:5 Changed 5 years ago by 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!!!!!

comment:6 Changed 5 years ago by 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

comment:7 Changed 5 years ago by andrew

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

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

Changed 5 years ago by sealibora@…

a test for postgres and a fix for #125

comment:8 Changed 5 years ago by 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 :(

comment:9 Changed 5 years ago by 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.

Note: See TracTickets for help on using tickets.