Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#23043 closed Bug (fixed)

Fix or document inconsistent handling of field defaults by migrations

Reported by: timo Owned by: nobody
Component: Migrations Version: 1.7-rc-1
Severity: Release blocker Keywords:
Cc: andrewgodwin, shai Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If I create a model with the field my_field = models.CharField(max_length=200, default='Foo'), there won't be a default set at the database level: question character varying(200) NOT NULL. If I have that same field without a default, but then add one in a migration, a default will be created in the database: question character varying(200) NOT NULL DEFAULT 'Foo'::character varying. I think a default shouldn't be set in the latter case because we generally can't transform callable defaults into database defaults.

Change History (5)

comment:1 Changed 12 months ago by shai

  • Cc andrewgodwin shai added
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Cleanup/optimization to Bug

I think this is a bug, not a cleanup/optimization, and a release blocker at that.

South had added defaults in the database until version 0.7.4; I think it took another release or two to get rid of them completely. The realization that defaults should never be left in the database (they should only be used when adding new columns to existing tables, and then removed) was christened in pain and angst -- some related to callable, as @timo noted; another ill-effect, unique to Oracle, was that the presence of default date-time values required special, non-trivial handling for schema backup and restore (ask me if you want details; irrelevant here).

Defaults in a Django-managed database are evil. Kill them with fire.

(the change in type and severity is tentative, feel free to revert if you have can convince yourself otherwise)

comment:2 Changed 12 months ago by andrewgodwin

There is code in the schema backend specifically to remove defaults; since I'm on a plane for the next 12 hours I won't be able to investigate this further, but https://github.com/django/django/blob/master/django/db/backends/schema.py#L412 is supposed to drop the default on add and ensure this doesn't happen.

Can the reporter say what database backend they're using? Or someone else double-check this? The schema provided looks PostgreSQL-ish but I can't be certain.

I'm happy this being a blocker, we don't want to give the impression Django is setting database defaults now.

comment:3 Changed 12 months ago by timo

Yes, I tested with PostgreSQL, but it looks like it's probably an issue on all DBs. It looks like the BaseDatabaseSchemaEditor._alter_field() method doesn't drop database defaults when it's finished.

comment:4 Changed 12 months ago by Andrew Godwin <andrew@…>

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

In 5875b8d13333bf709b54f91a143304826d57f2fd:

Fixed #23043: alter_field drops defaults too

comment:5 Changed 12 months ago by Andrew Godwin <andrew@…>

In 2fb1939a9efae24560d41fbb7f6a280a1d2e8d06:

[1.7.x] Fixed #23043: alter_field drops defaults too

Note: See TracTickets for help on using tickets.
Back to Top