Skip to content

Conversation

LilyFirefly
Copy link
Contributor

@LilyFirefly LilyFirefly force-pushed the ticket_470 branch 3 times, most recently from 38d7d7a to 000fab6 Compare November 23, 2020 11:12
Comment on lines +20 to +60
Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@hannseman hannseman left a 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:

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"

@LilyFirefly
Copy link
Contributor Author

I haven't looked into that bit yet. Thanks for the advice!

@LilyFirefly LilyFirefly force-pushed the ticket_470 branch 3 times, most recently from b750c81 to ba4fb6c Compare November 23, 2020 19:44
@LilyFirefly LilyFirefly force-pushed the ticket_470 branch 6 times, most recently from e6b21f7 to 2d901b8 Compare November 25, 2020 23:05
@LilyFirefly LilyFirefly marked this pull request as ready for review November 25, 2020 23:08
@LilyFirefly
Copy link
Contributor Author

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.

Copy link
Member

@charettes charettes left a 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.

@LilyFirefly LilyFirefly force-pushed the ticket_470 branch 3 times, most recently from 4e94e47 to d805f27 Compare November 28, 2020 19:46
Comment on lines +20 to +60
Copy link
Member

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?

Base automatically changed from master to main March 9, 2021 06:21
@LilyFirefly
Copy link
Contributor Author

@hannseman Thanks for catching that!

Copy link
Member

@charettes charettes left a 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.

@LilyFirefly
Copy link
Contributor Author

I just tried this PR on a local project and adding a new field with db_default set will cause the default to be dropped.

class TestModel(models.Model):
    foo = models.CharField(
        max_length=255, db_default="hej", null=True, blank=True
    )

Adding the field foo to TestModel will generate the following SQL:

BEGIN;
--
-- Add field foo to testmodel
--
ALTER TABLE "shipping_testmodel" ADD COLUMN "foo" varchar(255) DEFAULT 'hej' NULL;
ALTER TABLE "shipping_testmodel" ALTER COLUMN "foo" DROP DEFAULT;
COMMIT;

Is this a problem in the current version of the PR? I can't replicate the issue now.

@hannseman
Copy link
Contributor

hannseman commented Jun 5, 2021

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 null=True to null=False, for example 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:

You are trying to change the nullable field 'foo' on testmodel to non-nullable without a default; we can't do that (the database needs something to populate existing rows).
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Ignore for now, and let me handle existing rows with NULL myself (e.g. because you added a RunPython or RunSQL operation to handle NULL values in a previous data migration)
 3) Quit, and let me add a default in models.py

Not really sure how we want to handle this case as there could be NULL values in the column set explicitly by the user. We should handle it somehow at least since selecting option 1 and for example setting the prompted default to"prompteddefault" will generate the following migration:

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 db_default we could skip the prompt and be fine with:

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 null=False, for example:

bar = models.CharField(max_length=255, db_default="hej", null=False)

This gives this prompt:

You are trying to add a non-nullable field 'bar' to testmodel without a default; we can't do that (the database needs something to populate existing rows).
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Quit, and let me add a default in models.py
Select an option:

@LilyFirefly
Copy link
Contributor Author

Thanks @hannseman! I'll get on those today!

@LilyFirefly LilyFirefly force-pushed the ticket_470 branch 3 times, most recently from 6a3d635 to 0a2555f Compare June 6, 2021 15:21
@LilyFirefly
Copy link
Contributor Author

@hannseman Those should be fixed now.

@LilyFirefly LilyFirefly force-pushed the ticket_470 branch 2 times, most recently from 9e77954 to bb042c7 Compare June 6, 2021 18:45
@LilyFirefly
Copy link
Contributor Author

@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?

@LilyFirefly LilyFirefly requested a review from charettes June 6, 2021 18:53
@LilyFirefly LilyFirefly force-pushed the ticket_470 branch 2 times, most recently from 9d6f59e to 159b1ac Compare June 7, 2021 08:01
@LilyFirefly
Copy link
Contributor Author

I had an idea how to enable bulk create to work fully on sqlite last night, which worked. Instead of using the DEFAULT keyword in sql, I can get the db_default expression from the field and pass that to the database.

I'm still only doing this on sqlite where the new supports_default_keyword_in_insert database feature is False. I think the DEFAULT keyword is cleaner where supported. But the sqlite implementation would work on all databases I think, so we could go that way too.

@hannseman
Copy link
Contributor

@hannseman Those should be fixed now.

Cool! Looks much better now!

@LilyFirefly
Copy link
Contributor Author

Thanks @hannseman! Your test-driving has been very helpful for finding areas that need more work!

Copy link
Member

@felixxm felixxm left a 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

Copy link
Member

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.
@felixxm
Copy link
Member

felixxm commented Apr 28, 2022

Closing due to inactivity.

@Ian-Foote Feel-free to send a new patch when you have time (maybe during the DjangoCon EU sprints 😉), thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants