Opened 9 months ago

Closed 9 months ago

Last modified 9 months ago

#35223 closed Cleanup/optimization (fixed)

Fields with db_default raise ValidationErrors when full_clean() called

Reported by: Brian Ibbotson Owned by: bcail
Component: Database layer (models, ORM) Version: 5.0
Severity: Release blocker Keywords:
Cc: Brian Ibbotson, bcail 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 Brian Ibbotson)

Starting to migrate models in my (large) project to use Django 5’s new db_default attribute for fields (using Django 5.0.2), encountering behavior contrary to my expectations.

A field with db_default raises a ValidationError when full_clean() called, if that field has been omitted from the create() call.

This is (to me) unexpected behavior. Would expect that no error would be raised, the instance would be saved successfully, with the specified db_default value set at the database level.

Has been explained to me in the Django forums that the ValidationError is correct, that I should instead either

(1) explicitly choose to exclude the missing fields from full_clean() call,
(2) write a custom clean method for each field, or
(3) simply save the instance rather than calling full_clean()

Had always been impressed on me that it is best practice to always call full_clean() on instance creation and/or update.

In either case, having to determine the missing fields and annotate the full_clean() call or write a custom clean method per field seem to work against the usual conception of a default value, which should propagate... well, by default and allow for simpler operation where possible.

I would suggest having fields with db_default be excluded by default from full_clean()

If the current behavior is re-affirmed, would suggest incorporating the suggested 3 workaround steps into the Django documentation, as I suspect most users would have similar expectations as myself when implementing this in future code.

Change History (13)

comment:1 by Brian Ibbotson, 9 months ago

Description: modified (diff)

comment:2 by Brian Ibbotson, 9 months ago

Description: modified (diff)

comment:3 by Mariusz Felisiak, 9 months ago

I would suggest having fields with db_default be excluded by default from full_clean()

db_default is just a default value, we cannot always exclude it from full_clean(). What about not valid values provided explicitly in create()? What about None when field is non-nullable? etc.

comment:4 by Simon Charette, 9 months ago

I believe that we should find a way to have db_default behave the same way as generated fields when no value has been assigned to the instance when full_clean is called (#35127). Requiring users to explicitly pass it to exclude is simply bad ergonomics IMO.

One thing that the solution should take into account is that the value should only be skipped when the value is not assigned. In other words

class Article(models.Model):
   upvotes = models.PositiveIntegerField(db_default=0)

a1 = Article(upvotes=-1)
a1.full_clean()  # should raise a ValidationError

a2 = Article(upvotes=42)
a2.full_clean()  # should *not* raise a ValidationError

a1 = Article()
a1.full_clean()  # should not raise a ValidationError

In other words, when a value is present it should be validated otherwise if db_default is provided it should be assumed to be valid just like it's the case with generated fields.

A workaround in the mean time is to have both default and db_default assigned but obviously that kind of defeats the purpose of using db_default in the first place.

in reply to:  4 comment:5 by Mariusz Felisiak, 9 months ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Replying to Simon Charette:

I believe that we should find a way to have db_default behave the same way as generated fields when no value has been assigned to the instance when full_clean is called (#35127). Requiring users to explicitly pass it to exclude is simply bad ergonomics IMO.

Agreed, I think we should treat this as a cleanup not bug.

comment:6 by Damir Nafikov, 9 months ago

Owner: changed from nobody to Damir Nafikov
Status: newassigned

comment:7 by bcail, 9 months ago

Cc: bcail added

@Damir Nafikov, are you still working on this ticket?

comment:8 by bcail, 9 months ago

Has patch: set

I opened a PR.

comment:9 by Mariusz Felisiak, 9 months ago

Severity: NormalRelease blocker

Bumping to a release blocker as Model.full_clean() crashes on some expressions, e.g.

  File "/django/tests/field_defaults/tests.py", line 175, in test_full_clean
    obj.full_clean()
  File "/django/django/db/models/base.py", line 1586, in full_clean
    self.clean_fields(exclude=exclude)
  File "/django/django/db/models/base.py", line 1641, in clean_fields
    setattr(self, f.attname, f.clean(raw_value, self))
  File "/django/django/db/models/fields/__init__.py", line 830, in clean
    value = self.to_python(value)
  File "/django/django/db/models/fields/__init__.py", line 1620, in to_python
    parsed = parse_datetime(value)
  File "/django/django/utils/dateparse.py", line 114, in parse_datetime
    return datetime.datetime.fromisoformat(value)
TypeError: fromisoformat: argument must be str

comment:10 by Mariusz Felisiak, 9 months ago

Last edited 9 months ago by Mariusz Felisiak (previous) (diff)

comment:11 by Mariusz Felisiak, 9 months ago

Owner: changed from Damir Nafikov to bcail
Triage Stage: AcceptedReady for checkin

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

Resolution: fixed
Status: assignedclosed

In 1570ef0:

Fixed #35223 -- Made Model.full_clean() ignore fields with db_default when validating empty values.

Thanks Brian Ibbotson for the report.

Regression in 7414704e88d73dafbcfbb85f9bc54cb6111439d3.

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

In 5f07460a:

[5.0.x] Fixed #35223 -- Made Model.full_clean() ignore fields with db_default when validating empty values.

Thanks Brian Ibbotson for the report.

Regression in 7414704e88d73dafbcfbb85f9bc54cb6111439d3.

Backport of 1570ef02f34037d32218d463342592debccf915c from main.

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