neo4j / neo4j-python-driver

Neo4j Bolt driver for Python
https://neo4j.com/docs/api/python-driver/current/
Other
898 stars 186 forks source link

neo4j.time.DateTime.now() doesn't accept tzinfo #1103

Closed jackiedeng0 closed 3 days ago

jackiedeng0 commented 1 week ago

Hi, I'm running into the following bug where neo4j.time.DateTime.now() isn't accepting a tzinfo argument:

>>> neo4j.time.DateTime.now(datetime.UTC)
Traceback (most recent call last):
  File "/home/jack/Code/chance-backend/venv/lib/python3.12/site-packages/neo4j/time/__init__.py", line 2206, in now
    return tz.fromutc(  # type: ignore
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: fromutc: argument must be a datetime

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jack/Code/chance-backend/venv/lib/python3.12/site-packages/neo4j/time/__init__.py", line 2216, in now
    now_native = tz.fromutc(utc_now_native)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: fromutc: dt.tzinfo is not self

whereas ...Date.today() and ...Time.now() work fine:

>>> neo4j.time.Date.today(datetime.UTC)
neo4j.time.Date(2024, 10, 10)
>>> neo4j.time.Time.now(datetime.UTC)
neo4j.time.Time(12, 45, 8, 60610215, tzinfo=datetime.timezone.utc)

I'm currently using a workaround like this:

neo4j.time.DateTime.utc_now().replace(tzinfo=datetime.UTC)

It might just be caused by some legacy code in ...time/__init__.py, and perhaps some changes like this would make it work and match the other now() and today() functions:

2205,2224c2205,2207
<             try:
<                 return tz.fromutc(  # type: ignore
<                     cls.from_clock_time(  # type: ignore
<                         Clock().utc_time(), UnixEpoch
<                     ).replace(tzinfo=tz)
<                 )
<             except TypeError:
<                 # For timezone implementations not compatible with the custom
<                 # datetime implementations, we can't do better than this.
<                 utc_now = cls.from_clock_time(Clock().utc_time(), UnixEpoch)
<                 utc_now_native = utc_now.to_native()
<                 now_native = tz.fromutc(utc_now_native)
<                 now = cls.from_native(now_native)
<                 return now.replace(
<                     nanosecond=(
<                         now.nanosecond
<                         + utc_now.nanosecond
<                         - utc_now_native.microsecond * 1000
<                     )
<                 )
---
>             return cls.from_clock_time(Clock().local_time(), UnixEpoch)
>                 .replace(tzinfo=timezone.utc)
>                 .astimezone(tz)

I would make a pull request but I've just never worked on this project like this before, thanks.

My Environment

Python Version: 3. Driver Version: 5.25.0 Server Version and Edition: Neo4j 5 Operating System: Arch Linux

robsdedude commented 1 week ago

Hi and thanks for reaching out.

Looking at the API docs, I find

Temporal data types are implemented by the neo4j.time module.

It provides a set of types compliant with ISO-8601 and Cypher, which are similar to those found in the built-in datetime module. Sub-second values are measured to nanosecond precision and the types are compatible with pytz.

Now this is not very explicit, I find, and I think this should be updated, but it hints at the core of the problem: back when neo4j.time was designed, pytz was the way to go for IANA time zone DB based tzinfo implementations. Python's zoneinfo module was not a thing yet and datetime.timezone was insufficient (only supporting fixed UTC offset time zones).

I suppose making our custom temporal types work with datetime.timezone might be possible. However, it can never work with zoneinfo (new since Python 3.9) since that relies on date times having a fold attribute, which our custom date times don't have. As a matter of fact, I've observed Python segfault when using neo4j.time with zoneinfo. So I highly recommend sticking to pytz (at least for the time being). I still hope that Python manages to get nanosecond precision into its temporal types eventually, which would make all/most of our custom temporal types superfluous.

TL;DR: use pytz timezones and you'll have a much easier time. Other tzinfo implementations might work or might not. We've designed our temporal types with pytz in mind.

jackiedeng0 commented 6 days ago

I see, I appreciate the detailed explanation! I must've overlooked that part in the doc while looking at the table comparing the neo4j.time types to the built-in types. I've changed to pytz and it works fine now, thanks again.