[Django] #34946: Adding a field with default and db_default drops database level DEFAULT
Django
-------------------------------------------+--------------------------
Reporter: Simon Charette | Owner: nobody
Type: Bug | Status: assigned
Component: Migrations | Version: 5.0
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+--------------------------
[https://docs.djangoproject.com/en/dev/ref/models/fields/#db-default The
documentation] for `Field.db_default` states
> If both `db_default` and `Field.default` are set, `default` will take
precedence when creating instances in Python code. `db_default` will still
be set at the database level and will be used when inserting rows outside
of the ORM or when adding a new field in a migration.
However adding a field with both results in the following SQL
{{{#!sql
ALTER TABLE "foo" ADD COLUMN "bar" integer DEFAULT 42 NOT NULL;
ALTER TABLE "foo" ALTER COLUMN "bar" DROP DEFAULT;
}}}
The `DEFAULT` should never be dropped if the field has a `db_default`
defined.
The following patch reproduces and demonstrate
[https://github.com/django/django/commit/7414704e88d73dafbcfbb85f9bc54cb6111439d3
#diff-
7cc40f1cb85a8f19c80b2e901626d78e4729047dbbb0ed52861f5894ff688115R1673-R1716
why the test] that intended to cover this case didn't catch it.
{{{#!diff
diff --git a/tests/migrations/test_operations.py
b/tests/migrations/test_operations.py
index d58da8b7ac..57a9086c19 100644
--- a/tests/migrations/test_operations.py
+++ b/tests/migrations/test_operations.py
@@ -1692,15 +1692,22 @@ def test_add_field_both_defaults(self):
field = new_state.models[app_label, "pony"].fields["height"]
self.assertEqual(field.default, 3)
self.assertEqual(field.db_default, Value(4))
- project_state.apps.get_model(app_label,
"pony").objects.create(weight=4)
+ pre_pony_pk = (
+ project_state.apps.get_model(app_label,
"pony").objects.create(weight=4).pk
+ )
self.assertColumnNotExists(table_name, "height")
# Add field.
with connection.schema_editor() as editor:
operation.database_forwards(app_label, editor, project_state,
new_state)
self.assertColumnExists(table_name, "height")
+ post_pony_pk = (
+ project_state.apps.get_model(app_label,
"pony").objects.create(weight=10).pk
+ )
new_model = new_state.apps.get_model(app_label, "pony")
- old_pony = new_model.objects.get()
- self.assertEqual(old_pony.height, 4)
+ pre_pony = new_model.objects.get(pk=pre_pony_pk)
+ self.assertEqual(pre_pony.height, 4)
+ post_pony = new_model.objects.get(pk=post_pony_pk)
+ self.assertEqual(post_pony.height, 4)
new_pony = new_model.objects.create(weight=5)
if not connection.features.can_return_columns_from_insert:
new_pony.refresh_from_db()
diff --git a/tests/schema/tests.py b/tests/schema/tests.py
index 20a00e29f7..80fdead190 100644
--- a/tests/schema/tests.py
+++ b/tests/schema/tests.py
@@ -2257,6 +2257,24 @@ class Meta:
columns = self.column_classes(AuthorDbDefault)
self.assertEqual(columns["renamed_year"][1].default, "1985")
+ @isolate_apps("schema")
+ def test_add_field_both_defaults_preserves_db_default(self):
+ class Author(Model):
+ class Meta:
+ app_label = "schema"
+
+ #self.isolated_local_models = [Author]
+ with connection.schema_editor() as editor:
+ editor.create_model(Author)
+
+ field = IntegerField(default=1985, db_default=1988)
+ field.set_attributes_from_name("birth_year")
+ field.model = Author
+ with connection.schema_editor() as editor:
+ editor.add_field(Author, field)
+ columns = self.column_classes(Author)
+ self.assertEqual(columns["birth_year"][1].default, "1988")
+
@skipUnlessDBFeature(
"supports_column_check_constraints",
"can_introspect_check_constraints"
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34946>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Simon Charette | Owner: nobody
Type: Bug | Status: assigned
Component: Migrations | Version: 5.0
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34946#comment:1>
Django
Reporter: Simon Charette | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: 5.0
Severity: Release blocker | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: nobody => Simon Charette
* stage: Unreviewed => Accepted
Comment:
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/34946#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: 5.0
Severity: Release blocker | Resolution:
migrations |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Paolo Melchiorre (added)
* keywords: => sql, default, migrations
--
Ticket URL: <https://code.djangoproject.com/ticket/34946#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: sql, default, | Triage Stage: Accepted
migrations |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
Replying to [ticket:34946 Simon Charette]:
> However adding a field with both results in the following SQL
>
> {{{#!sql
> ALTER TABLE "foo" ADD COLUMN "bar" integer DEFAULT 42 NOT NULL;
> ALTER TABLE "foo" ALTER COLUMN "bar" DROP DEFAULT;
> }}}
Thank you Simon for the report. I've been trying to reproduce the issue
exactly as described to progress with the PR review. I've created a test
app with a `Foo` model with a dummy `name` field, and then I've added a
`bar` integer field with both `default` and `db_default`. I can't get the
SQL that you reported, so far I'm only obtaining a SQL that would create
an intermediary table. I think I'm missing something to craft the proper
reproducer:
{{{
BEGIN;
--
-- Add field bar to foo
--
CREATE TABLE "new__ticket_34946_foo" ("id" integer NOT NULL PRIMARY KEY
AUTOINCREMENT, "bar" integer DEFAULT 42 NOT NULL, "name" varchar(10)
DEFAULT 'dbdef' NOT NULL);
INSERT INTO "new__ticket_34946_foo" ("id", "name") SELECT "id", "name"
FROM "ticket_34946_foo";
DROP TABLE "ticket_34946_foo";
ALTER TABLE "new__ticket_34946_foo" RENAME TO "ticket_34946_foo";
COMMIT;
}}}
I've also tried this with a `Foo` model with no other field than `bar`,
same result. Could you help me understand the steps to reproduce the error
as reported? Thank you!
--
Ticket URL: <https://code.djangoproject.com/ticket/34946#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Migrations | Version: 5.0
Severity: Release blocker | Resolution:
Keywords: sql, default, | Triage Stage: Accepted
migrations |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Hello Natalia!
SQLite doesn't support alteration such as `DROP DEFAULT` so it's not
affected by this issue, you should be able to reproduce with Postgres or
MySQL.
--
Ticket URL: <https://code.djangoproject.com/ticket/34946#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette
Component: Migrations | Version: 5.0
Severity: Release blocker | Resolution: fixed
Keywords: sql, default, | Triage Stage: Accepted
migrations |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8a28e983df091d94eaba77cb82fbe3ef60a80799" 8a28e983]:
{{{
#!CommitTicketReference repository=""
revision="8a28e983df091d94eaba77cb82fbe3ef60a80799"
Fixed #34946 -- Preserved db_default on combined default field addition.
Regression in 7414704e88d73dafbcfbb85f9bc54cb6111439d3.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34946#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Simon Charette | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Migrations | Version: 5.0
Severity: Release blocker | Resolution: fixed
Keywords: sql, default, | Triage Stage: Accepted
migrations |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0265eaa500aa01e5328989b5cd814db7234f3777" 0265eaa5]:
{{{
#!CommitTicketReference repository=""
revision="0265eaa500aa01e5328989b5cd814db7234f3777"
[5.0.x] Fixed #34946 -- Preserved db_default on combined default field
addition.
Regression in 7414704e88d73dafbcfbb85f9bc54cb6111439d3.
Backport of 8a28e983df091d94eaba77cb82fbe3ef60a80799 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34946#comment:7>