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

Conversation

timgraham
Copy link
Collaborator

No description provided.

@timgraham timgraham force-pushed the fix-trunc-tz branch 2 times, most recently from 7b00206 to a7bfe4a Compare May 3, 2025 00:35
@timgraham timgraham marked this pull request as ready for review May 3, 2025 01:37
@timgraham timgraham requested a review from WaVEV May 3, 2025 01:52
value = value.astimezone(self.tzinfo)
elif isinstance(value, datetime):
if value is None:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line isn't reachable I think. If value were None, cannot be instance of date

@@ -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

timgraham added 2 commits May 3, 2025 06:03
Add the same exception raising from TruncDate to TruncTime and add
tests for both functions.
@timgraham timgraham requested a review from WaVEV May 3, 2025 10:25
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.

@timgraham timgraham merged commit d04cbc5 into mongodb:main May 3, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants