Ticket #14 (closed defect: fixed)

Opened 6 years ago

Last modified 4 years ago

default values like break sql output

Reported by: anonymous Owned by: andrew
Priority: major Milestone: 0.4
Component: databaseapi Version: 0.3
Keywords: Cc:


just checking against basestring in column_sql isn't robust enough


Change History

comment:1 Changed 6 years ago by iacobs@…

Not adding a default value if field.default is callable should suffice IMHO, that's what the Django ORM seems to do.

This worked for me:

Index: db/
--- db/	(revision 74)
+++ db/	(working copy)
@@ -141,7 +141,7 @@
         sqlparams = ()
         # if the field is "NOT NULL" and a default value is provided, create the column with it
         # this allows the addition of a NOT NULL field to a table with existing rows
-        if not field.null and field.has_default():
+        if not field.null and field.has_default() and not callable(field.default):
             default = field.get_default()
             if isinstance(default, basestring):
                 default = "'%s'" % default

comment:2 Changed 6 years ago by amccurdy

There's one piece missing here. If the column is marked as NOT NULL, then the db column must be provided a default value. Otherwise, the database, at least in PG's case, will complain about the column containing NULL values.

I had thought previously that running the alter in a transaction, followed by an update query to fix the fields prior to the commit would fix this behavior, but that doesn't seem to be working.

comment:3 Changed 6 years ago by amccurdy

I think we'd have to do something like this:

andy=# start transaction;
andy=# alter table foo add foo_date timestamp with time zone NULL;
andy=# update foo set foo_date = RESULT_OF_PYTHON_CALLABLE;  
andy=# alter table foo alter column foo_date set not null;
andy=# commit;

comment:4 Changed 6 years ago by andrew

  • Version set to 0.3
  • Milestone set to 0.4

Yes, Andy, I'd agree with that; it's the simplest translation of the whole process.

We could either do this automatically, or just raise an error when you try to add a NOT NULL column with a callable as a default (or, in fact, no default, as currently you get nasty SQL errors). The former seems nicer from a migration perspective, but I'm not sure where my "too much magic" stance lies here.

comment:5 Changed 6 years ago by amccurdy

I don't think this is "too much magic". In fact, it's exactly what happens within the Postgres black box when we specify a literal default value. The column gets added, and all the existing rows get updated.

comment:6 Changed 6 years ago by andrew

  • Status changed from new to assigned

Yes, good point; we'll go ahead and implement it as you suggested. I'll have a stab at it tomorrow.

comment:7 Changed 6 years ago by andrew

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

Fixed in [125]. The whole drop-default-if-we-don't-need-it part is already done as part of add_column, but not create_table; that's potentially another ticket.


Add a comment

Modify Ticket

as closed

E-mail address and user name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.