Fix two bugs in _Timestamp.to_datetime and _Timestamp.from_datetime

[lukasbindreiter]

I've recently added some property based tests using hypothesis to a project using protobuf messages, which has lead me to discover two distinct bugs in the _Timestamp.to_datetime and _Timestamp.from_datetime conversion function.

I've narrowed it down to a basic test for the classic encode / decode invariant pattern, that looked like this:

from datetime import datetime, timezone

from hypothesis import given
from hypothesis.strategies import datetimes

from betterproto import _Timestamp

@given(datetimes())
def test_datetime_to_timestamp_and_back(dt: datetime):
    """
    Make sure converting a datetime to a protobuf timestamp message
    and then back again ends up with the same datetime.
    """
    dt = dt.astimezone(timezone.utc)
    assert _Timestamp.from_datetime(dt).to_datetime() == dt

This test found two failing examples:
datetime(2242, 12, 31, 23, 0, 0, 1, tzinfo=timezone.utc)
and
datetime(1969, 12, 31, 23, 0, 0, 1, tzinfo=timezone.utc)

Let's see whats going on:

Bug 1: Floating point precision error:

>>> dt = datetime(2242, 12, 31, 23, 0, 0, 1, tzinfo=timezone.utc)
>>> ts = _Timestamp.from_datetime(dt)
>>> ts  # looks correct still
_Timestamp(seconds=8615026800, nanos=1000)

>>> ts.to_datetime()  # but now we are off by one microseconds
datetime.datetime(2242, 12, 31, 23, 0, 0, 2, tzinfo=datetime.timezone.utc)

>>> # but why, lets take a look at the intermediate timestamp value
>>> ts.seconds + ts.nanos / 1e9
8615026800.000002

>>> # ah, it is a floating point issue, let's verify:
>>> 8615026800.000001
8615026800.000002

Bug 2: Negative timestamps:

>>> dt = datetime(1969, 12, 31, 23, 0, 0, 1, tzinfo=timezone.utc)
>>> ts = _Timestamp.from_datetime(dt)
>>> ts  # this is already wrong, should be seconds=-3600, nanos=1000
_Timestamp(seconds=-3599, nanos=1000)

>>> ts.to_datetime()  # we are off by one second now
datetime.datetime(1969, 12, 31, 23, 0, 1, 1, tzinfo=datetime.timezone.utc)

The _Timestamp we get from from_datetime should already be one with: _Timestamp(seconds=-3600, nanos=1000). That this is the correct way to represent negative timestamps can also be read up here: https://protobuf.dev/reference/protobuf/google.protobuf/#timestamp

nanos
Non-negative fractions of a second at nanosecond resolution. Negative second values with fractions must still have non-negative nanos values that count forward in time. Must be from 0 to 999,999,999 inclusive.

Fix

I've implemented a fixed version of the two methods by calculating a timedelta from the unix start epoch and then calculating the total_microseconds (not floating point seconds) of that timedelta.

I've also added a few test cases for this, but without using hypothesis (I didn't want to add a new dependency, even though property based testing could probably be a cool tool for this project). Instead I manually specified the failing examples from above.

[Gobot1234]

This does also seem like it negates the need for #415 could you also add the tests from that please?

[cetanu]

Ah crap I just merged #415 and saw this. If needed then please include a revert.

[Gobot1234]

Thanks for this!

[lukasbindreiter]

Thanks for finalizing this PR 👍

Just checked the other issue and yeah this should be resolved as well by this fix 😄

Thanks for this!

My pleasure :)