Skip to content

Fix Trunc database function with tzinfo parameter #296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions django_mongodb_backend/features.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,6 @@ class DatabaseFeatures(BaseDatabaseFeatures):
# Pattern lookups that use regexMatch don't work on JSONField:
# Unsupported conversion from array to string in $convert
"model_fields.test_jsonfield.TestQuerying.test_icontains",
# Truncating in another timezone doesn't work becauase MongoDB converts
# the result back to UTC.
"db_functions.datetime.test_extract_trunc.DateFunctionWithTimeZoneTests.test_trunc_func_with_timezone",
"db_functions.datetime.test_extract_trunc.DateFunctionWithTimeZoneTests.test_trunc_timezone_applied_before_truncation",
# Unexpected alias_refcount in alias_map.
"queries.tests.Queries1Tests.test_order_by_tables",
# The $sum aggregation returns 0 instead of None for null.
Expand Down Expand Up @@ -278,6 +274,7 @@ def django_test_expected_failures(self):
"update.tests.AdvancedTests.test_update_annotated_multi_table_queryset",
"update.tests.AdvancedTests.test_update_ordered_by_m2m_annotation",
"update.tests.AdvancedTests.test_update_ordered_by_m2m_annotation_desc",
"update.tests.AdvancedTests.test_update_values_annotation",
},
"QuerySet.dates() is not supported on MongoDB.": {
"admin_changelist.tests.ChangeListTests.test_computed_list_display_localization",
Expand Down
35 changes: 35 additions & 0 deletions django_mongodb_backend/functions.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
from datetime import datetime

from django.conf import settings
from django.db import NotSupportedError
from django.db.models import DateField, DateTimeField, TimeField
from django.db.models.expressions import Func
from django.db.models.functions import JSONArray
from django.db.models.functions.comparison import Cast, Coalesce, Greatest, Least, NullIf
Expand Down Expand Up @@ -196,6 +200,33 @@ def trunc(self, compiler, connection):
return {"$dateTrunc": lhs_mql}


def trunc_convert_value(self, value, expression, connection):
if connection.vendor == "mongodb":
# A custom TruncBase.convert_value() for MongoDB.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic could be refactored
But dont know how, at least If value none, return

Copy link
Collaborator

@WaVEV WaVEV May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part does two things, astimezone and time (one or none)
To check if they must be applied, an code like

if convert_to_tz and isinstance(value, datetime):
    if isinstance(self.output_type, DatetimeField | DateField | TimeField):
        value = value.astimezone(self.tzinfo)
    if isinstance(self.output_type, DateField):
        value = value.date()
    elif isinstance(self.output_field, TimeField):
        value = value.time()
return value
        

Does this code make sense?
I am assuming that datetimefield implies a datetime value

Copy link
Collaborator Author

@timgraham timgraham May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that looks logically correct, but it doesn't short-circuit as much and therefore has some extra isinstance() calls in some cases. (By the way, here's the original method: https://github.com/django/django/blob/9d93e35c207a001de1aa9ca9165bdec824da9021/django/db/models/functions/datetime.py#L345-L364. As you said, there's at least the issue of the unneeded None check under isinstance(value, datetime) there.)

edit: Actually, it's not correct, .date() and .time() truncations are needed even when convert_to_tz=False.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. I think it could be "untabbed" but in that way the code isn't much simple

if value is None:
return None
convert_to_tz = settings.USE_TZ and self.get_tzname() != "UTC"
if isinstance(self.output_field, DateTimeField):
if convert_to_tz:
# Unlike other databases, MongoDB returns the value in UTC,
# so rather than setting the time zone equal to self.tzinfo,
# the value must be converted to tzinfo.
value = value.astimezone(self.tzinfo)
elif isinstance(value, datetime):
if isinstance(self.output_field, DateField):
if convert_to_tz:
value = value.astimezone(self.tzinfo)
# Truncate for Trunc(..., output_field=DateField)
value = value.date()
elif isinstance(self.output_field, TimeField):
if convert_to_tz:
value = value.astimezone(self.tzinfo)
# Truncate for Trunc(..., output_field=TimeField)
value = value.time()
return value
return self.convert_value(value, expression, connection)


def trunc_date(self, compiler, connection):
# Cast to date rather than truncate to date.
lhs_mql = process_lhs(self, compiler, connection)
Expand All @@ -218,6 +249,9 @@ def trunc_date(self, compiler, connection):


def trunc_time(self, compiler, connection):
tzname = self.get_tzname()
if tzname and tzname != "UTC":
raise NotSupportedError(f"TruncTime with tzinfo ({tzname}) isn't supported on MongoDB.")
lhs_mql = process_lhs(self, compiler, connection)
return {
"$dateFromString": {
Expand Down Expand Up @@ -256,6 +290,7 @@ def register_functions():
Substr.as_mql = substr
Trim.as_mql = trim("trim")
TruncBase.as_mql = trunc
TruncBase.convert_value = trunc_convert_value
TruncDate.as_mql = trunc_date
TruncTime.as_mql = trunc_time
Upper.as_mql = preserve_null("toUpper")
14 changes: 11 additions & 3 deletions django_mongodb_backend/operations.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from django.db.backends.base.operations import BaseDatabaseOperations
from django.db.models import TextField
from django.db.models.expressions import Combinable, Expression
from django.db.models.functions import Cast
from django.db.models.functions import Cast, Trunc
from django.utils import timezone
from django.utils.regex_helper import _lazy_re_compile

Expand Down Expand Up @@ -97,7 +97,11 @@ def get_db_converters(self, expression):
]
)
elif internal_type == "DateField":
converters.append(self.convert_datefield_value)
# Trunc(... output_field="DateField") values must remain datetime
# until Trunc.convert_value() so they can be converted from UTC
# before truncation.
if not isinstance(expression, Trunc):
converters.append(self.convert_datefield_value)
elif internal_type == "DateTimeField":
if settings.USE_TZ:
converters.append(self.convert_datetimefield_value)
Expand All @@ -106,7 +110,11 @@ def get_db_converters(self, expression):
elif internal_type == "JSONField":
converters.append(self.convert_jsonfield_value)
elif internal_type == "TimeField":
converters.append(self.convert_timefield_value)
# Trunc(... output_field="TimeField") values must remain datetime
# until Trunc.convert_value() so they can be converted from UTC
# before truncation.
if not isinstance(expression, Trunc):
converters.append(self.convert_timefield_value)
elif internal_type == "UUIDField":
converters.append(self.convert_uuidfield_value)
return converters
Expand Down
2 changes: 2 additions & 0 deletions docs/source/releases/5.1.x.rst
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ Django MongoDB Backend 5.1.x
- Added support for a field's custom lookups and transforms in
``EmbeddedModelField``, e.g. ``ArrayField``’s ``contains``,
``contained__by``, ``len``, etc.
- Fixed the results of queries that use the ``tzinfo`` parameter of the
``Trunc`` database functions.

.. _django-mongodb-backend-5.1.0-beta-2:

Expand Down
2 changes: 2 additions & 0 deletions docs/source/releases/5.2.x.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,5 @@ Bug fixes
- Added support for a field's custom lookups and transforms in
``EmbeddedModelField``, e.g. ``ArrayField``’s ``contains``,
``contained__by``, ``len``, etc.
- Fixed the results of queries that use the ``tzinfo`` parameter of the
``Trunc`` database functions.
7 changes: 4 additions & 3 deletions docs/source/topics/known-issues.rst
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,10 @@ Database functions
:class:`~django.db.models.functions.SHA512`
- :class:`~django.db.models.functions.Sign`

- The ``tzinfo`` parameter of the :class:`~django.db.models.functions.Trunc`
database functions doesn't work properly because MongoDB converts the result
back to UTC.
- The ``tzinfo`` parameter of the
:class:`~django.db.models.functions.TruncDate` and
:class:`~django.db.models.functions.TruncTime` database functions isn't
supported.

Transaction management
======================
Expand Down
5 changes: 5 additions & 0 deletions tests/db_functions_/models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from django.db import models


class DTModel(models.Model):
start_datetime = models.DateTimeField(null=True, blank=True)
26 changes: 26 additions & 0 deletions tests/db_functions_/test_datetime.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
from zoneinfo import ZoneInfo

from django.db import NotSupportedError
from django.db.models.functions import TruncDate, TruncTime
from django.test import TestCase, override_settings

from .models import DTModel


@override_settings(USE_TZ=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this setting needed to raise the exception?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class TruncTests(TestCase):
melb = ZoneInfo("Australia/Melbourne")

def test_truncdate_tzinfo(self):
msg = "TruncDate with tzinfo (Australia/Melbourne) isn't supported on MongoDB."
with self.assertRaisesMessage(NotSupportedError, msg):
DTModel.objects.annotate(
melb_date=TruncDate("start_datetime", tzinfo=self.melb),
).get()

def test_trunctime_tzinfo(self):
msg = "TruncTime with tzinfo (Australia/Melbourne) isn't supported on MongoDB."
with self.assertRaisesMessage(NotSupportedError, msg):
DTModel.objects.annotate(
melb_date=TruncTime("start_datetime", tzinfo=self.melb),
).get()