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.

[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.

[codingjoe]

@felixxm excellent points. I added plenty more tests especially around migrations. I also added another field check to warn users about column references. Let me know if you have any more feedback. Best, Joe

[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.

[luzfcb]

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.

PostgreSQL 13 now includes a native gen_random_uuid() function without requires the pgcrypto extension and in teory the RandomUUID can be used without run CryptoExtension on the migration . https://www.postgresql.org/docs/13/functions-uuid.html

That said, A small brainstorm, ignore if it doesn't make sense

I'm especially curious to know what django's behavior will be on INSERT (create/bulk_create) and/or UPDATE, when we have a field with unique=True and the default is an expression that runs some database function (like gen_random_uuid()) and for some reason generated an item with a value that already exists in the column, that is, generating a unique constraint error.

1 - Will Django just raise the exception? 2 - Will Django try a few times again before raising an exception? (Maybe an Expressions API has some way of indicating that the expression supports this hypothetical feature)

In short, Django would somehow manage to recover from this type of error on its own, and if not, how should an application writer handle handling these errors?

The most common use case I think would be use UUIDField as primary key or a value to using externally (like in rest APIs to avoid exposing the primary key):

from django.db import models
from django.contrib.postgres.functions import RandomUUID

class Question(models.Model):
    external_id = models.UUIDField(unique=True, null=False, default=RandomUUID, db_index=True, editable=False)

[codingjoe]

@luzfcb thank you for your concern, but I believe this isn't the best place to discuss this. I would suggest opening a new ticket for this. But as a quick info, thus far the ORM doesn't handle PG extension dependencies. If you use fields or functions that require extensions, you will need to install those manually.

[felixxm]

@codingjoe Thanks for all your efforts :1st_place_medal: and sorry for the late reply :broken_heart: . 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 🎄