Preserve the instant when deserializing a timestamp into AwareDateTime

[yousaf-360]

AwareDateTime(format="timestamp", default_timezone=tz) returns a different moment in time than the one it was given.

import datetime as dt
from marshmallow import fields

central = dt.timezone(dt.timedelta(hours=-6), "central")
field = fields.AwareDateTime(format="timestamp", default_timezone=central)

field.deserialize(1384043025)
# datetime.datetime(2013, 11, 10, 0, 23, 45, tzinfo=central)

field.deserialize(1384043025).timestamp()
# 1384064625.0   <- six hours after the timestamp that went in

1384043025 is 2013-11-10 00:23:45+00:00. What comes back is 2013-11-10 00:23:45-06:00, which is 06:23:45 UTC. The value silently identifies a different instant, and the drift is whatever the configured zone's offset happens to be.

Cause

utils.from_timestamp resolves the timestamp against UTC and then drops the tzinfo, leaving UTC wall time in a naive object:

# Load a timestamp with utc as timezone to prevent using system timezone.
# Then set timezone to None, to let the Field handle adding timezone info.
return dt.datetime.fromtimestamp(value, tz=dt.timezone.utc).replace(tzinfo=None)

AwareDateTime._deserialize then relabels whatever naive value it gets:

ret = ret.replace(tzinfo=self.default_timezone)

For iso, rfc and strftime formats that is the correct reading of default_timezone: the parsed value really is a bare wall time, and the caller is supplying the zone it should be interpreted in. For the timestamp formats it is not — the instant was never in doubt, and the naive object is UTC wall time. Relabelling it reinterprets a UTC reading as a local one.

Change

DateTime now records which deserialization formats yield UTC wall time, and AwareDateTime converts rather than relabels for those:

UTC_FORMATS = frozenset({"timestamp", "timestamp_ms"})

The string formats are untouched, and so are DateTime and NaiveDateTime. The guard also skips values that arrive as datetime objects rather than through the wire format, since those did not come from a timestamp and the existing relabelling behaviour is right for them.

After the change the instant survives, expressed in the requested zone:

field.deserialize(1384043025)
# datetime.datetime(2013, 11, 9, 18, 23, 45, tzinfo=central)
field.deserialize(1384043025).timestamp()
# 1384043025.0

Tests

Added test_aware_timestamp_deserialization_preserves_instant, which asserts the round trip directly for both timestamp formats. It fails on main and passes with the change.

I also had to amend the existing assertion in test_timestamp_field_deserialization, which pinned the old behaviour:

expected_aware = expected.replace(tzinfo=central)

It now checks that the instant matches and that the result carries the requested zone. I want to flag that clearly, since it means this PR changes behaviour that was deliberately covered — but the covered behaviour returns the wrong instant, so I do not think it can be kept as is.

Full suite: 1189 passed. The one failure, test_from_timestamp_with_overflow_value, is #2999 and fails the same way on a clean checkout here (Windows message mismatch). ruff check and ruff format are clean on both files.

Two things I left alone

AwareDateTime(format="timestamp") with no default_timezone still rejects every input, because the value it is handed is naive. Since a POSIX timestamp is always UTC, defaulting to UTC there would make the field usable rather than guaranteed to fail, but that is a larger behaviour question than the bug above and the current behaviour is at least not wrong. Happy to fold it in if you would like it.

I have not added a CHANGELOG entry, since entries reference the PR number. Glad to push one under "Bug fixes" now that this has a number.

[lafrech]

Thanks @yousaf-360 for the detailed report. Indeed, there's a bug, here.

Maybe we shouldn't have mixed timestamp and datetime this way, the logic differs too much.

A timestamp is neither naive or aware, but it points to a specific moment in time, so it can be converted to an aware or naive datetime (given a timezone).

In our current implementation, the default timezone is meant for naive datetimes, it doesn't replace the timezone on an aware datetime. We could do that with some e.g. convert_to parameter but this has never been asked for AFAIK.

In the fix you propose, you use the default timezone to convert the datetime from UTC. This is not really in line with the behaviour with datetimes. If we say a timestamp is UTC (which is debatable), it should be returned as UTC.

We could change your fix to force UTC and keep it as is whatever the default timezone:

        if not utils.is_aware(ret):
            if (
                self.format or self.DEFAULT_FORMAT
            ) in self.UTC_FORMATS and not isinstance(value, dt.datetime):
                ret = ret.replace(tzinfo=tz.UTC)

And we could add that conversion parameter I suggest above, if needed.

Not sure that's the way to go, just thinking out loud. This needs a bit of thought.

(Also, I'm not sure the case of value being a datetime is meant to be supported. I remember discussions about allowing already deserialized data to be deserialized, but I thought we decided not to.)

[yousaf-360]

That's a better framing than mine, and I've pushed it as 88e6386.

You're right that using default_timezone as a conversion target was out of step. It's documented as supplying a timezone that a naive value is missing, and it deliberately doesn't re-express aware values — so making it mean "convert to" only for timestamps would have given the same parameter two different jobs depending on the format. Agreed that a separate convert_to is the right home for that if it's ever wanted.

Two things worth knowing before you decide.

It settles the no-default case too. AwareDateTime(format="timestamp") with no default_timezone previously rejected every input it could ever be given — the field was unusable in that configuration, since the value handed to it is always naive. It now returns UTC. That's a bigger behaviour change than my first version, but in the direction of the field doing something rather than always failing.

default_timezone becomes a silent no-op for timestamp formats. With your version, AwareDateTime(format="timestamp", default_timezone=central) returns UTC and ignores what was asked for. That's defensible — nothing is missing for it to supply — but it does quietly discard a parameter the caller set. Options if you'd rather it not be silent: raise in __init__ when both are given, or warn. I've left it silent to match your snippet; say the word.

Test impact: the block in test_timestamp_field_deserialization that pinned the old behaviour now asserts UTC for both the default and non-default cases. Full suite is 1191 passed, with test_from_timestamp_with_overflow_value (#2999) failing the same way on a clean checkout here. ruff check and ruff format clean.

I'm not sure the case of value being a datetime is meant to be supported.

It works today — _TemporalField._deserialize returns the value untouched when it's already an instance of the field's type, so a datetime passed to a DateTime field short-circuits before any format handling. I couldn't find a test covering it, so it looks incidental rather than intended. The not isinstance(value, dt.datetime) guard only matters because of that path: a naive datetime passed in directly didn't come from a timestamp, so stamping UTC on it would be wrong. If you decide that path shouldn't be supported, the guard can go with it.

Happy to go back to the conversion version, or to split the no-default change out, if either lands better.

[lafrech]

I'm not sure the case of value being a datetime is meant to be supported.

I gave it a quick look and from a few other fields, it looks like it was meant to work this way. I guess we decided the cost was low so we might as well support that. Custom fields may not. Let's keep this as it is. Sorry for bringing this up.

---

Regarding the issue, we could go further and treat timestamps as explicit UTC. It doesn't make much sense to turn a timestamp into a naive datetime without a default datetime to convert to (unless we assume UTC is the default default datetime).

• DateTime would return an aware datetime instead of a naive one. Breaking change but people who care should use specific field.

• NaiveDateTime would error, or convert if timezone is supplied. Breaking change. Current behaviour kept by setting timezone=tz.UTC. Conversion to other TZ possible if setting another TZ.

• AwareDateTime would return UTC aware whatever the supplied TZ. Better than current buggy behaviour.

Basically, this means removing .replace(tzinfo=None) in from_timestamp.

This implementation seems more "pure" to me.

We may have to balance ideal solution vs. breaking changes. I like to think of the ideal solution first, then figure out whether we need to find a less breaking fix for current stable version.

(Now, I'm wondering about serialization of naive datetimes. Should we keep it UTC or use the field timezone so that it round trips correctly? The timezone is optional so it would fail if none is provided. We could raise at __init__ if using a timestamp func and no timezone is provided. Really not sure about this. This parameter was not meant to be symmetrical.)

(Another point that we may want to take into account it default Python behaviour, which is generally sensible and can be the ground for user expectations. In this case, I don't find the use of the local system TZ sensible. It may make sense for local use, but not in an API served by a server for which the system TZ is irrelevant. This is the reason why I had to overload timestamp/fromtimestamp in utils.py. I'd rather stick to that.)