Assume naive datetimes are UTC in the msgpack and cbor2 converters - #775
Assume naive datetimes are UTC in the msgpack and cbor2 converters#775onk3sh wants to merge 3 commits into
Conversation
`datetime.timestamp` interprets a naive datetime as local time, so the value these two converters wrote to the wire depended on the timezone of the machine unstructuring it. The same object serialized in two timezones produced two different payloads, and neither round-tripped back to the input. The structure hooks already read timestamps back as UTC, so the unstructure hooks now pin naive datetimes to UTC before converting, which makes the two ends agree and the output machine-independent. This is what the neighbouring `date` hook in the msgpack converter already did. The preconf tests never exercised naive datetimes: the shared strategy pins `timezones=just(timezone.utc)`, and the strategy that would generate them is disabled at every call site. The regression test therefore drives the timezone explicitly, since it would otherwise pass vacuously on a UTC host.
|
This is technically a backwards-compatibility break, but maybe it's warranted. We should probaby document how to restore the old behavior in migrations.md. Also don't forget to add the appropriate |
…ehavior Add a migrations.md entry for the msgpack and cbor2 converters, with the recipe for restoring `datetime.timestamp` on a converter, and versionchanged clauses in the preconf docs for both converters. Also replace the branchy timezone helper in the regression test with a monkeypatch fixture, so the teardown path is exercised and the test file stays at 100% coverage.
|
Pushed It is a break, and I think it's warranted because the old behavior isn't a behavior anyone can depend on. The value it produced depended on the And the round-trip inside cattrs was already wrong: the structure hook reads timestamps back as UTC, so The other two:
The same commit also fixes the coverage failure. The timezone helper in the test had a branch that never ran, which tripped |
Reported in #774.
The
msgpackandcbor2converters unstructuredatetimewithdatetime.timestamp(). On a naive datetime that reads the value as local time, so the number written to the wire depends on the timezone of the machine doing the unstructuring:TZdumpsoutputloadsreturnsUTCcb41daa362b20000002026-08-25 12:30:00+00:00Asia/Tokyocb41daa3430e0000002026-08-25 03:30:00+00:00America/Torontocb41daa370c20000002026-08-25 16:30:00+00:00An aware datetime is the control: it yields
cb41daa362b2000000in all three timezones.The change
The structure hooks already declare the wire contract —
datetime.fromtimestamp(v, timezone.utc)— so the unstructure hooks now pin naive datetimes to UTC before converting, and the two ends agree. Thedatehook sitting immediately below thedatetimehook inmsgpack.pyalready did exactly this withtime(tzinfo=timezone.utc); this brings thedatetimehook in line with it.The shared helper lives next to
validate_datetimeinpreconf/__init__.py, since both converters need it and both already import from there.A naive input still comes back aware, because a bare float has nowhere to record awareness. What changes is that the value survives and the payload no longer depends on the host.
Note on the alternative
Raising on naive input instead of assuming UTC is the stricter option, and I would understand preferring it — it refuses to guess rather than guessing well. I went with the UTC assumption because it keeps working code working and because the structure hook had already committed to UTC as the format's meaning. Happy to switch it if you would rather have the exception.
Tests
tests/test_preconf.pynever generated naive datetimes: the shared strategy pinstimezones=just(timezone.utc)(L155-158), and the strategy that would produce them is passedinclude_datetimes=Falseat all seven of its call sites.The regression test sets
TZexplicitly rather than relying on the host, because on a UTC machine — including the ubuntu CI runners — a naive-datetime assertion passes whether or not the bug is present. It restores the previousTZand callstzset()again on the way out, and skips wheretzsetis unavailable.Verified locally: the two new tests fail on unpatched source (
assert 1787628600.0 == 1787661000.0, the 9-hour JST offset) and pass with the change. Full suite is 992 passed, 15 xfailed.ruff checkandruff format --checkclean under the pinned lint group.