Ticket #578 (reopened defect)

Opened 4 years ago

Last modified 8 months ago

Boolean default values are not set correctly when adding new columns with sqlite

Reported by: luper.rouch@… Owned by: andrew
Priority: minor Milestone: 1.0
Component: databaseapi Version: 0.7.2
Keywords: sqlite boolean Cc: luper.rouch@…, shai@…

Description

If you add a new BooleanField to a model, with default=True in a SQLite database, the column will be filled with 'True' strings after migration, and that won't be interpreted as True booleans in Django.

This is because SQLite doesn't have a boolean type, and Django expects booleans to be stored as '1' or '0' in SQLite databases. The problem is specific to SQLite, other database backends recognize "DEFAULT True" properly as a boolean value.

To fix this we need some sort of database-specific mechanism to transform Python values to their SQL representation. Attached is a patch with a suggestion of API to do this properly, and the fix for this particular SQLite problem.

Note: the patch also uses django's Field.get_db_prep_save() method to transform the default values to their database representation, which I believe is the correct way to process values before passing them to the database layer.

Attachments

fix-sqlite-bool-defaults.patch (3.7 KB) - added by Luper Rouch <luper.rouch@…> 4 years ago.
south_sqlite3_bool_default.diff (790 bytes) - added by Colm Dougan <colm.dougan@…> 13 months ago.
south_sqlite3_bool_default.diff
south_sqlite3_bool_default_with_tests.diff (2.6 KB) - added by Colm Dougan 8 months ago.
Revised patch with tests

Change History

Changed 4 years ago by Luper Rouch <luper.rouch@…>

comment:1 Changed 4 years ago by andrew

  • Status changed from new to accepted
  • Milestone changed from 0.7.3 to 1.0

That's quite a change to the module, but I see exactly where you're coming from. I'll review it over in a bit - hopefully it will be fine.

comment:2 Changed 4 years ago by andrew

  • Cc luper.rouch@… added

comment:3 Changed 4 years ago by christian@…

just ran into this.

comment:4 Changed 3 years ago by klaas@…

Note:

This bug equally applies to default=False, i.e. "False" string values are inserted into the database in that case.

comment:5 Changed 3 years ago by anonymous

I was just burned by this too.

comment:6 Changed 3 years ago by tony@…

Oops, didn't mean to post as anonymous. Here's what I saw:

>>> Company.objects.filter(approved=False).count()
2
>>> Company.objects.filter(approved=True).count()
1
>>> Company.objects.all().count()
43
>>> Company.objects.filter(approved__isnull=True).count()
0

In the actual sqlite database, the majority of the rows had approved="False", but there were two where it was 0 and one where it was 1.

comment:7 Changed 3 years ago by bradley.ayers@…

I just ran into this too.

comment:8 Changed 3 years ago by andrew

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

Fixed in [916f74766c26].

comment:9 Changed 2 years ago by michel@…

Can this be reopened?
In version 0.7.6 I still ran into this problem.
The migration of a field with percentage in it (in my case a dateformat '%B %Y').
Adding this field went OK in migrations.

The problem started with any migration in this table after this migrations.
Recreating the table will fail, see: http://stackoverflow.com/questions/12442768

or this stacktrace:

 File "/media/storage/django/sites/palmrif/eggs/South-0.7.6-py2.7.egg/south/db/sqlite3.py", line 31, in add_column
    field.column: self._column_sql_for_create(table_name, name, field, False),
  File "/media/storage/django/sites/palmrif/eggs/South-0.7.6-py2.7.egg/south/db/generic.py", line 44, in _cache_clear
    return func(self, table, *args, **opts)
  File "/media/storage/django/sites/palmrif/eggs/South-0.7.6-py2.7.egg/south/db/sqlite3.py", line 103, in _remake_table
    ", ".join(["%s %s" % (self.quote_name(cname), ctype) for cname, ctype in definitions.items()]),

comment:10 Changed 2 years ago by Michel van Leeuwen <michel@…>

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:11 Changed 2 years ago by Michel van Leeuwen <michel@…>

Oops, sorry wrong ticket.
I was struggling with 2 problems:
default boolean value and the percentage in deafult value.
I found out it was the percentage value
Now I reopened the wrong ticket.....

comment:12 Changed 2 years ago by andrew

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

comment:13 Changed 15 months ago by anonymous

Just got bit by this one too. I was on 0.8.1. I've just now upgraded to 0.8.2 but it looks like the issue is still present there too (mainly because I do not see anything resembling Luper Rouch's patch in there).

comment:14 Changed 14 months ago by lars@…

  • Status changed from closed to reopened
  • Resolution fixed deleted

Yep, string 'True' and 'False' in the database on sqlite! on south 0.8.2

Changed 13 months ago by Colm Dougan <colm.dougan@…>

south_sqlite3_bool_default.diff

comment:15 Changed 13 months ago by Colm Dougan <colm.dougan@…>

I added a draft patch to fix this. Not sure if it is the best approach but it works for me. If you have any feedback I'd be happy to incorporate it and resubmit.

comment:16 Changed 12 months ago by anonymous

This fixed the issue for me as well, any change it can get upstream-ed?

Thanks!

comment:17 Changed 11 months ago by anonymous

just ran into this issue using south 0.8.2.

comment:18 Changed 11 months ago by anonymous

i also had a look at the changelog of 0.8.4 and it is not listed as fixed.

can somebody please fix this in 0.8.5?

comment:19 Changed 9 months ago by anonymous

I've run into the same issue with 0.8.4. A fix would be awesome.

comment:20 Changed 8 months ago by joshcartme@…

This one just got me too, added a BooleanField? with default=False to an SQLite db using south (0.8.4). After the migrations ran the database contained a bunch of "False" strings for the Boolean values. Filtering on that particular field in a queryset did not work until I fixed the db values.

comment:21 Changed 8 months ago by alex@…

Yep, same here. I ended up monkeypatching Colm Dougan's fix into specific migrations. Worked like a charm!
This seems like such an easy and harmless patch to include in the next release, so why hasn't it been done yet?

Changed 8 months ago by Colm Dougan

Revised patch with tests

comment:22 follow-up: ↓ 23 Changed 8 months ago by Colm Dougan

I have submitted a revised patch including tests. As before: if you have any feedback I'd be happy to incorporate it and resubmit.

comment:23 in reply to: ↑ 22 Changed 8 months ago by shai

  • Cc shai@… added

Replying to Colm Dougan:

I have submitted a revised patch including tests. As before: if you have any feedback I'd be happy to incorporate it and resubmit.

Please compare your patch with PR 126. I think the PR fixes things in a place that you missed, and uses the _default_value_workaround() that is standard across South backends. Your patch, on the other hand, currently applies, I presume.

Also, while at it -- and while other tests should probably verify it anyway -- I would amend the tests to make sure that the defaults are only used for adding the column (that is, that after the add_column operation, trying to insert a record without specifying a value for the new column fails). I would also consider a refactoring to share code between the two tests.

HTH,
Shai.

Note: See TracTickets for help on using tickets.