Opened 4 months ago

Closed 2 months ago

Last modified 2 months ago

#35731 closed Cleanup/optimization (fixed)

Extend documentation about db_default and DatabaseDefault

Reported by: Kyle Bebak Owned by: YashRaj1506
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Kyle Bebak, Simon Charette, Lily Foote Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Natalia Bidart)

I would be helpful if the existing docs at ref/models/fields.txt, when describing db_default, would mention DatabaseDefault.
Also, the docs topics/db/models.txt describe most of the Field options but db_default is missing, so ideally we would add a section for it with examples, including some that would show how and when DatabaseDefault is returned/used.

Original report

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.

Change History (12)

comment:1 by Simon Charette, 4 months ago

Cc: Simon Charette Lily Foote added

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 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, ​https://docs.djangoproject.com/en/5.1/ref/models/fields/#db-default, or anywhere else in the docs AFAICT.

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 documenting the use case for defining both was brought up during the review phase and didn't make the cut.

Last edited 4 months ago by Simon Charette (previous) (diff)

comment:2 by YashRaj1506, 4 months ago

Owner: set to YashRaj1506
Status: newassigned
Last edited 4 months ago by YashRaj1506 (previous) (diff)

comment:3 by Natalia Bidart, 4 months ago

Description: modified (diff)
Summary: Fields with db_default, and without default, are initialized to instance of DatabaseDefaultExtend documentation about db_default and DatabaseDefault
Triage Stage: UnreviewedAccepted
Version: 5.0dev

Overall, I agree with Simon analysis. Specifically, the docs for db_default say:

The database-computed default value for this field.

So, when doing val = IntegerField(db_default=10); foo = Foo(); foo.val, I would expect anything but the value that was set as db_default (10), since the code "hasn't gone to the db yet". Intuitively I would have expected an exception such as "you can't have a value since DB hasn't been reached yet", but getting a instance of a "db value promise" is even better and clearer.

OTOH, I do agree that we may be lacking docs about this. In particular, I think we should:

  1. add a small note to the existing ref/models/fields.txt docs about db_default, and
  2. add a richer section to the topics/db/models.txt for :attr:~Field.db_default since most of the Field options are documented there but db_default is missing.

Accepting and re-purposing this ticket with that goal in mind.

comment:4 by Kyle Bebak, 4 months ago

Indeed, the way I solved this problem was setting both default and db_default. I'm guessing this is a fairly common use case, especially for BooleanField, IntegerField, CharField/TextField, etc, where you usually don't need a DB Func or Python function to set a default value for the field. E.g. I use Django as the SOT for the DB schema, but I don't only write to the DB with the Django ORM, so having both default and db_default is useful for me.

Also, I understand that the model instance needs a sentinel value so it knows to set foo.val after foo has been written to the DB. My proposal is that the sentinel value not be stored on foo.val, but rather in some "private" attribute not likely to be touched by client code. Before foo is written to the DB, I think foo.val should be None, not an instance of DatabaseDefault (which can be stored elsewhere on the model instance).

This probably seems like splitting hairs, and not a good reason for changing the implementation. I like the sentinel living elsewhere because it makes the model's type interface simpler.

If default is passed to the field, then val has a type of int. If it's not, then its type is int | None. In my proposal above, passing db_default wouldn't change the type of foo, whereas the current implementation means its type is int | DatabaseDefault, int | None, or int, depending on what combo of default and db_default is passed.

Anyway, I think a change in documentation would be a good outcome for this ticket, and that there should be an example of passing both default and db_default in the docs. Thank you for taking the time to look at this =)

comment:5 by Simon Charette, 4 months ago

I think foo.val should be None, not an instance of DatabaseDefault (which can be stored elsewhere on the model instance).

Without getting into all the details there are complications in doing that as None is already used to denote NULL and a field with a db_default could also be marked null=True and assigned None before creation to take precedence over db_default

class Author(models.Model):
    favorite_color = models.CharField(db_default="green", null=True)

author = Author()
assert isinstance(author.favorite_color, DatabaseDefault)
author.favorite_color = None
author.save()
author.refresh_from_db()
assert author.favorite_color is None

In other words we can't use None as a sentinel to denote a fallback to db_default as it might be explicitly assigned as a desired value.

If default is passed to the field, then val has a type of int. If it's not, then its type is int | None. In my proposal above, passing db_default wouldn't change the type of foo, whereas the current implementation means its type is int | DatabaseDefault, int | None, or int, depending on what combo of default and db_default is passed.

That's a bad assumption unfortunately, every property associated with a field are | Expression[F] where F is bound to the field type and represents its output_field and Expression is a protocol identifiable by callable(getattr(type, "resolve_expression", None)). In other words, IntegerField(null=True) results in a descriptor that is int | None | Expression[IntegerField] and not simply int | None. I know a few typing library simply this edge case but its pretty much an allowed assignment type.

Last edited 4 months ago by Simon Charette (previous) (diff)

comment:6 by YashRaj1506, 2 months ago

Has patch: set

comment:7 by YashRaj1506, 2 months ago

added a patch: https://github.com/django/django/pull/18682

open to suggestions!

comment:8 by Sarah Boyce, 2 months ago

Patch needs improvement: set

comment:9 by YashRaj1506, 2 months ago

Patch needs improvement: unset

comment:10 by Sarah Boyce, 2 months ago

Triage Stage: AcceptedReady for checkin

comment:11 by Sarah Boyce <42296566+sarahboyce@…>, 2 months ago

Resolution: fixed
Status: assignedclosed

In 35ab2e0:

Fixed #35731 -- Extended db_default docs.

This added a missing db_default reference in docs/topics/db/models.txt,
and added a reference to the DatabaseDefault object.

comment:12 by Sarah Boyce <42296566+sarahboyce@…>, 2 months ago

In 630c9e1:

[5.1.x] Fixed #35731 -- Extended db_default docs.

This added a missing db_default reference in docs/topics/db/models.txt,
and added a reference to the DatabaseDefault object.

Backport of 35ab2e018214479fa712d73f070198299ef670a1 from main.

Note: See TracTickets for help on using tickets.
Back to Top