Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#32966 closed Bug (fixed)

Time-related _check_fix_default_value() methods can be optimized / simplified and have a bug

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: 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

I noticed that three of the _check_fix_default_value() method definitions in django/db/models/fields/__init__.py can be simplified. Here is one of them: https://github.com/django/django/blob/fe074c96a343530beea50fbdd0803d3e7b739e8e/django/db/models/fields/__init__.py#L1156-L1167

For example, in each of them, timezone.now() is called even when the return value isn't needed / won't be used.

Change History (11)

comment:1 Changed 10 months ago by Chris Jerdonek

Summary: time-related _check_fix_default_value() methods can be simplifiedtime-related _check_fix_default_value() methods can be optimized / simplified

comment:2 Changed 10 months ago by Mariusz Felisiak

Triage Stage: UnreviewedAccepted

comment:3 Changed 10 months ago by Chris Jerdonek

Type: Cleanup/optimizationBug

When I started looking at this, I noticed there is a bug on this line:
https://github.com/django/django/blob/fe074c96a343530beea50fbdd0803d3e7b739e8e/django/db/models/fields/__init__.py#L2216

It can be triggered by the following when USE_TZ = True:

from django.db import models
from django.utils.timezone import now

class MyModel(models.Model):
    tz = models.TimeField(default=now().timetz())
    
    class Meta:
        app_label = 'test'

field = MyModel._meta.get_field('tz')
field.check()

It results in:

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "/.../django/db/models/fields/__init__.py", line 1112, in check
    *self._check_fix_default_value(),
  File "/.../django/db/models/fields/__init__.py", line 2220, in _check_fix_default_value
    if lower <= value <= upper:
TypeError: '<=' not supported between instances of 'datetime.datetime' and 'datetime.time'

This case is not covered in the tests here:
https://github.com/django/django/blob/fe074c96a343530beea50fbdd0803d3e7b739e8e/tests/invalid_models_tests/test_ordinary_fields.py#L743-L750

Last edited 10 months ago by Chris Jerdonek (previous) (diff)

comment:4 Changed 10 months ago by Chris Jerdonek

Has patch: set

comment:5 Changed 10 months ago by Chris Jerdonek

Summary: time-related _check_fix_default_value() methods can be optimized / simplifiedTime-related _check_fix_default_value() methods can be optimized / simplified and have a bug

comment:6 Changed 10 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:7 Changed 10 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 542e7494:

Fixed #32966 -- Fixed TimeField.check() crash for timezone-aware times in default when USE_TZ = True.

comment:8 Changed 10 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In eebebfe0:

Refs #32966 -- Added _to_naive() and _get_naive_now() for use in DateTimeCheckMixin classes.

comment:9 Changed 10 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 6fa5d05d:

Refs #32966 -- Simplified the _check_fix_default_value() implementations.

comment:10 Changed 10 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 6f5e07a8:

Refs #32966 -- Refactored out DateTimeCheckMixin._check_if_value_fixed().

comment:11 Changed 10 months ago by Chris Jerdonek

For future reference, it turns out the bug fixed by this ticket was previously mentioned here, but never opened as a separate ticket: https://code.djangoproject.com/ticket/21905#comment:19

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