Fixed #27222 -- Refreshed model field values assigned expressions on save(). #19285
Conversation
|
The unveiled failures are related to this ticket TL;DR the documented pattern for copying model instances breaks when some fields are deferred on the original instance. This poses a problem with this patch as it marks any field assigned a database expression at |
|
This is now based on #19445 |
django/db/models/base.py
Outdated
There was a problem hiding this comment.
If an explicit literal value is provided for a field there is no need to return it.
There was a problem hiding this comment.
Except that I've written some code like this for MongodB where the id is ObjectIdAutoField: Question.objects.create(id="000000000000000000000013"). Previously, this would refresh id to the returned value, ObjectId("000000000000000000000013"). What do you think?
There was a problem hiding this comment.
I guess the same problem might happen with some form of UUID field that returns uuid.UUID instances I guess. Could you write a regression test for it and file an issue?
There was a problem hiding this comment.
Yes, I imagine UUIDAutoField (ticket-32577) could reproduce it. Do you have a more generic way in mind? I could try for some minimal implementation of UUIDFIeld + django.contrib.postgres.functions.RandomUUID to create a regression test in postgres_tests but this might be unnecessarily complicated in light of the MongoDB tests.
This works as a regression test, at least on SQLite and PostgreSQL. I'm unsure if all databases will accept it.
a = AutoModel.objects.create(pk="1")
self.assertEqual(a.pk, 1)
There was a problem hiding this comment.
This works as a regression test, at least on SQLite and PostgreSQL. I'm unsure if all databases will accept it.
That's perfect, the whole notion of auto-field is problematic anyway as discussed on #18365 (as any field with a db_default and primary_key=True should be allowed as a primary key in the first place).
I'm surprised we don't already have a test that covers this use case though 🤔
There was a problem hiding this comment.
PR for the regression: #19868
|
Rebased. |
83e6e61 to
6a44639
Compare
6a44639 to
13b0d30
Compare
|
@adamchainz Thanks 👍 Updated. |
13b0d30 to
86e49ca
Compare
86e49ca to
f2d2c25
Compare
f2d2c25 to
387a523
Compare
|
buildbot, test on oracle. |
387a523 to
758ae8d
Compare
…pdate. This required implementing UPDATE RETURNING machinery that heavily borrows from the INSERT one.
…save(). Removed the can_return_columns_from_insert skip gates on existing field_defaults tests to confirm the expected number of queries are performed and that returning field overrides are respected.
758ae8d to
f04a3b8
Compare
|
buildbot, test on oracle. |
|
Thanks to everyone for the review a particularly @felixxm for seeing this through 🎉 🙇 |
| def test_save_expressions(self): | ||
| article = Article(pub_date=Now()) | ||
| article.save() | ||
| expected_num_queries = ( | ||
| 0 if connection.features.can_return_columns_from_insert else 1 | ||
| ) | ||
| with self.assertNumQueries(expected_num_queries): | ||
| article_pub_date = article.pub_date | ||
| self.assertIsInstance(article_pub_date, datetime) | ||
| # Sleep slightly to ensure a different database level NOW(). | ||
| time.sleep(0.1) | ||
| article.pub_date = Now() | ||
| article.save() | ||
| expected_num_queries = ( | ||
| 0 if connection.features.can_return_rows_from_update else 1 | ||
| ) | ||
| with self.assertNumQueries(expected_num_queries): | ||
| self.assertIsInstance(article.pub_date, datetime) | ||
| self.assertGreater(article.pub_date, article_pub_date) |
There was a problem hiding this comment.
It's fine to keep this test here if you feel it's most appropriate (I understand the intention is to test changes in Model, not to test expressions themselves), but wanted to note that similar tests are in expressions. Here's a list of skipped test for MongoDB:
"Insert expressions aren't supported.": {
"bulk_create.tests.BulkCreateTests.test_bulk_insert_now",
"bulk_create.tests.BulkCreateTests.test_bulk_insert_expressions",
"expressions.tests.BasicExpressionsTests.test_new_object_create",
"expressions.tests.BasicExpressionsTests.test_new_object_save",
"expressions.tests.BasicExpressionsTests.test_object_create_with_aggregate",
"expressions.tests.BasicExpressionsTests.test_object_create_with_f_expression_in_subquery",
# PI()
"db_functions.math.test_round.RoundTests.test_decimal_with_precision",
"db_functions.math.test_round.RoundTests.test_float_with_precision",
},
Trac ticket number
ticket-27222