[Django] #35731: Fields with db_default, and without default, are initialized to instance of DatabaseDefault

13 views
Skip to first unread message

Django

unread,
Sep 4, 2024, 7:42:29 PM9/4/24
to django-...@googlegroups.com
#35731: Fields with db_default, and without default, are initialized to instance of
DatabaseDefault
-------------------------------------+-------------------------------------
Reporter: kylebebak | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: 5.0 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
For example, if client code creates a model `Foo` with `val =
IntegerField(db_default=10)`, does `foo = Foo()`, and accesses `foo.val`,
they get an instance of `django.db.models.expressions.DatabaseDefault`.

This `DatabaseDefault` seems to be used for bookkeeping until the model
instance is written to the DB, after which `foo.val` is an `int`. IMO this
is not a good design, because it's a case of an implementation detail
(setting a value for the field once it's saved to the DB) changing the
model's public interface (IMO a model instance's field values are part of
its public interface).

If instead we do `val = IntegerField()`, and `foo = Foo()`, and access
`foo.val`, we get `None`, s.t. the type of `foo.val` is `int | None`.
Using `db_default` means that the type of `foo.val` is now `int |
DatabaseDefault`. `DatabaseDefault` is a bookkeeping type that client code
usually shouldn't interact with. If users aren't aware of `db_default`'s
implementation, they might still write code like this, which would be
broken: `if foo.val is not None: print(foo.val + 10)`.

Because `DatabaseDefault` is for bookkeeping, it seems like there's no
reason the model instance couldn't store its `DatabaseDefault` instances
on a "private" field which wouldn't affect the model's public interface.
This would be a lot cleaner IMO. Most users shouldn't know about
`DatabaseDefault`, which unsurprisingly isn't mentioned here,
https://docs.djangoproject.com/en/5.1/ref/models/fields/#db-default, or
anywhere else in the docs AFAICT.
--
Ticket URL: <https://code.djangoproject.com/ticket/35731>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,
Sep 4, 2024, 8:43:58 PM9/4/24
to django-...@googlegroups.com
#35731: Fields with db_default, and without default, are initialized to instance of
DatabaseDefault
-------------------------------------+-------------------------------------
Reporter: Kyle Bebak | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* cc: Simon Charette, Lily Foote (added)

Comment:

> IMO this is not a good design, because it's a case of an implementation
detail (setting a value for the field once it's saved to the DB) changing
the model's public interface (IMO a model instance's field values are part
of its public interface).

I don't agree that this is a change to the model's public interface.
Assigning `Expression` like objects to field attributes on model instances
has been a documented pattern for a while. See the section about
[https://docs.djangoproject.com/en/5.1/ref/models/instances/#updating-
attributes-based-on-existing-fields Updating attributes based on existing
fields] for example.

> Because DatabaseDefault is for bookkeeping, it seems like there's no
reason the model instance couldn't store its DatabaseDefault instances on
a "private" field which wouldn't affect the model's public interface.

A sentinel object has to be assigned to the attribute to distinguish from
`None` until its persisted and I'm afraid there is no way around that.
What exactly would you expect the model instance attribute accesses to
return when `db_default=TransactionNow()` or any other expression that is
not a simple literal value like in your example? I personally don't find
that returning a sentinel object that denotes this field's value is meant
to be assigned a database expression on creation a surprising behavior
when considering non-literal default values.

> This would be a lot cleaner IMO. Most users shouldn't know about
DatabaseDefault, which unsurprisingly isn't mentioned here,
The documentation you linked mentions

> If both `db_default` and `Field.default` are set, `default` will take
precedence **when creating instances in Python code**.

Given your report is specific to model instance initialization and
includes an example of a `db_default` composed of a literal that can be
expressed as `default` I believe that your expectations are met by
defining your field as `IntegerField(default=10, db_default=10)`. I
suspect that this feature that was specifically designed for the use case
you had in mind.

I'll let others chime in as I'm unsure if this should be closed as
''invalid'' (as specifying `default` achieves what you're after) or
accepted as a documentation improvement given
[https://github.com/django/django/pull/16092#issuecomment-1265398322 the
use case for defining both was brought up during the review phase] and
didn't make the cut.
--
Ticket URL: <https://code.djangoproject.com/ticket/35731#comment:1>

Django

unread,
Sep 5, 2024, 1:20:07 AM9/5/24
to django-...@googlegroups.com
#35731: Fields with db_default, and without default, are initialized to instance of
DatabaseDefault
-------------------------------------+-------------------------------------
Reporter: Kyle Bebak | Owner:
Type: | YashRaj1506
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by YashRaj1506):

* owner: (none) => YashRaj1506
* status: new => assigned

Comment:

I will try to take on this PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/35731#comment:2>
Reply all
Reply to author
Forward
0 new messages