Resolved #30032 -- Added support for query expressions as default values

[codingjoe]

ticket-30032

TL;DR

Adds support for query expression as default values for PostgreSQL and Oracle. The expression are not stored on the database as defaults but are passed on INSERT and UPDATE.

[codingjoe]

@felixxm I updated the release note to include mention Oracle support for database default. I believe this concludes issue 29444. It's been plenty fun :) Let me know if you have any change requests.

[codingjoe]

Ref #11568

[codingjoe]

@apollo13, @charettes maybe you would be interested in reviewing this too, since you have been very involved with the returning API changes and it all kind of lead to this ;)

[auvipy]

what about migrations?

[codingjoe]

what about migrations?

Can you elaborate a bit more what you mean? This is a rather small change, I am happy to put in more effort to move this over the finish line.

[charettes]

I'd like to see more tests around invalid expression such as ones that span joins, aggregation, window functions. I think we'll want to add checks for that.

Also primary key interactions should be tested (e.g. (primary_key=True , default=Func('uuid'))) and the documentation should make it clear that this is not a db_default option that results in using the SQL DEFAULT expr at the table definition level. We already get report of users surprised Django doesn't use DEFAULT when only Python callable are supported and I feel this blurs the line even more.

[codingjoe]

I'd like to see more tests around invalid expression such as ones that span joins, aggregation, window functions. I think we'll want to add checks for that.

Actually, most of them would work. However, since we do not allow F expressions in insert, you will get a ValueError:

ValueError: Failed to insert expression "Max(Col(queries_returningmodel, queries.ReturningModel.id))" on queries.ReturningModel.max_id. F() expressions can only be used to update, not to insert.

Besides it being borderline impossible to implement a comprehensive check, without refactoring all expressions, I don't even know if it's wise to add a one. See strictly speaking any value is allowed and most that don't work should. However, if they don't you will get an error. Analog to that, you can provide a callable, that returns a completely different type than the field at hand. We don't have a check for that, and that might be a good thing.

I don't know I am torn about this. I am happy to add tests tho. But I might need a bit more input before I can progress.

Also primary key interactions should be tested (e.g. (primary_key=True , default=Func('uuid'))) and the documentation should make it clear that this is not a db_default option that results in using the SQL DEFAULT expr at the table definition level. We already get report of users surprised Django doesn't use DEFAULT when only Python callable are supported and I feel this blurs the line even more.

I added a note about the behavior. There already is a test for a primary key, I didn't use UUID, since this the functions are different across Oracle and Postgres and in the latter the uuid-ossp extension is required, which I don't want to enable it the suite.

[ngnpope]

A scattering of thoughts on what is currently implemented. Maybe some more complex tests using some math functions and operators, and ExpressionWrapper could be added?

[codingjoe]

@auvipy you seem very excited, would you care to review the patch to move things along a little quicker? This is a rather complex patch, so every pair eye helps.

[codingjoe]

@InvalidInterrupt thanks for the thorough review, it really helped to get this forward. If you are happy with my changes let me know and I will squash my changes to get this ready for check in.

[cjerdonek]

A couple comments.

[cjerdonek]

This assignment and tuple creation / unpacking, etc. isn't used in the case that hasattr is true. I also noted this below, but the conversation is marked resolved.

[cjerdonek]

Can this be if params? Does the if block need to execute if params is an empty tuple (the method default)?

[felixxm]

@codingjoe Thanks for all your efforts 🥇 and sorry for the late reply 💔 . Unfortunately, we need to make few things to make it reviewable again:

• rebase from the main branch,
• apply black,
• re-target to Django 4.1, and
• check failing tests.

I hope it makes sense.

[codingjoe]

@felixxm I know the drill 😉

[felixxm]

@felixxm I know the drill wink

I can promise a detailed review before Django 4.1 feature freeze, if it will be reviewable again.

[nessita]

Hey @codingjoe, while reading all the commentary for ticket-32577, I was pointed at this PR.

Do you have time to keep working on this PR? I could help with the rebasing and restyling if you don't have the time. Let me know!

[nessita]

@codingjoe I have rebased and fixed conflicts and code style. There were a couple of changes I wasn't sure about so you'd need to review and fix accordingly:

1. Implementation of db_returning changed since you made this branch, so the part and hasattr(self.default, "as_sql") needs proper placing.
2. Many tests fail using sqlite because of the raise NotImplementedError in NativeUUID4.as_sql, so this needs fixing.

Hopefully you can pick up this PR from here! Let us know. Thanks!

[nessita]

I also had to revert this change because lots of tests (specifically in custom_pk where failing:

diff --git a/django/db/models/base.py b/django/db/models/base.py
index 0f8af9f9204e3..11b7f04dd34ef 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -872,7 +872,7 @@ def _save_table(self, raw=False, cls=None, force_insert=False,
             results = self._do_insert(cls._base_manager, using, fields, returning_fields, raw)
             if results:
                 for value, field in zip(results[0], returning_fields):
-                    setattr(self, field.attname, value)
+                    setattr(self, field.attname, field.to_python(value))
         return updated
 
     def _do_update(self, base_qs, using, pk_val, values, update_fields, forced_update):

[felixxm]

Closing due to inactivity.

[codingjoe]

I'll try to get to it this Xmas 🎄