-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
Fixed #470 -- Support database defaults for fields #13709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
38d7d7a
to
000fab6
Compare
000fab6
to
6d4d3cc
Compare
tests/field_defaults/tests.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's in scope to consolidate the behaviour here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference in behaviour? All that seems to change between the different backend tests is the use of utcnow
versus now
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the difference between postgres and the other two.
There's also a difference between mysql and sqlite - sqlite's CURRENT_TIMESTAMP
doesn't have sub-second precision, which means the difference between now
and a.pub_date
has the opposite sign.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really looking forward to this! 👍
Have you looked into how to prevent dropping the default? Maybe it's just a matter of adding a check for has_db_default
to prevent the DROP DEFAULT
:
django/django/db/backends/base/schema.py
Lines 825 to 831 in 9159d17
if needs_database_default: | |
changes_sql, params = self._alter_column_default_sql(model, old_field, new_field, drop=True) | |
sql = self.sql_alter_column % { | |
"table": self.quote_name(model._meta.db_table), | |
"changes": changes_sql, | |
} | |
self.execute(sql, params) |
BTW you've got some double quote string-literals in the test case "Default headline"
I haven't looked into that bit yet. Thanks for the advice! |
b750c81
to
ba4fb6c
Compare
ba4fb6c
to
9a117c8
Compare
e6b21f7
to
2d901b8
Compare
I think this is at a good state for a serious review now. I haven't tackled oracle support at all, I still need to add documentation, there's some cleanup to do and I suspect there's more tests that will be useful to add. But I think this demonstrates the general approach works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work so far @Ian-Foote, left a few comments.
4e94e47
to
d805f27
Compare
tests/field_defaults/tests.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference in behaviour? All that seems to change between the different backend tests is the use of utcnow
versus now
?
@hannseman Thanks for catching that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments but overall I think this is heading in the right direction.
I'm not entirely sold on the dual allowed_default
and check_field_default_support
methods as were usually a bit more liberal in what kind of expression we allow to reach the database layer and error out.
Is this a problem in the current version of the PR? I can't replicate the issue now. |
Looks better! Nice! I also tried to add a migration which moves a field from models.CharField(max_length=255, db_default="hej", null=True, blank=True) to models.CharField(max_length=255, db_default="hej", null=False, blank=True) This gives me this prompt:
Not really sure how we want to handle this case as there could be BEGIN;
--
-- Alter field foo on testmodel
--
ALTER TABLE "shipping_testmodel" ALTER COLUMN "foo" SET DEFAULT 'prompteddefault';
UPDATE "shipping_testmodel" SET "foo" = 'hej' WHERE "foo" IS NULL;
ALTER TABLE "shipping_testmodel" ALTER COLUMN "foo" SET NOT NULL;
ALTER TABLE "shipping_testmodel" ALTER COLUMN "foo" DROP DEFAULT;
COMMIT; I guess when using UPDATE "shipping_testmodel" SET "foo" = 'hej' WHERE "foo" IS NULL;
ALTER TABLE "shipping_testmodel" ALTER COLUMN "foo" SET NOT NULL; There's also an issue when adding a new field with bar = models.CharField(max_length=255, db_default="hej", null=False) This gives this prompt:
|
Thanks @hannseman! I'll get on those today! |
6a3d635
to
0a2555f
Compare
@hannseman Those should be fixed now. |
9e77954
to
bb042c7
Compare
@charettes Do you have any ideas how to handle the bulk create on sqlite better? Or is my current implementation essentially the best we can do? |
9d6f59e
to
159b1ac
Compare
I had an idea how to enable bulk create to work fully on sqlite last night, which worked. Instead of using the I'm still only doing this on sqlite where the new |
Cool! Looks much better now! |
Thanks @hannseman! Your test-driving has been very helpful for finding areas that need more work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ian-Foote Thanks for this patch 👍 I left initial comments (without checking tests and docs). Unfortunately it doesn't work on Oracle and I have a single failure on MySQL (8.0.25):
======================================================================
FAIL: test_field_database_defaults_mysql (field_defaults.tests.DefaultTests)
----------------------------------------------------------------------
...
File "/django/tests/field_defaults/tests.py", line 49, in test_field_database_defaults_mysql
self.assertLess((a.pub_date - now).seconds, 5)
File "/usr/lib/python3.8/unittest/case.py", line 1298, in assertLess
self.fail(self._formatMessage(msg, standardMsg))
File "/usr/lib/python3.8/unittest/case.py", line 753, in fail
raise self.failureException(msg)
AssertionError: 7200 not less than 5
django/db/backends/base/schema.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this doesn't work on Oracle where bind variables are not supported in DDL statements:
django.db.utils.DatabaseError: DPI-1059: bind variables are not supported in DDL statements
We should use a solution similar to that used in the functional indexes.
Special thanks to Hannes Ljungberg for finding multiple implementation gaps. Thanks also to Simon Charette and Adam Johnson for reviews.
8834c01
to
dc731d9
Compare
Closing due to inactivity. @Ian-Foote Feel-free to send a new patch when you have time (maybe during the DjangoCon EU sprints 😉), thanks. |
https://code.djangoproject.com/ticket/470