Skip to content
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

[WIP] fields.DateTime timestamp, timestamp_ms, timezone, timezone_naive added #1003

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

vgavro
Copy link
Contributor

@vgavro vgavro commented Oct 15, 2018

UPDATE: [ THIS MERGE REQUEST SHOULD BE CANCELED WHEN WE HAVE SOME PROGRESS WITH NEW #1009 ]


Please see related comment #612 (comment)

TODO: (if this concept will be approved)

  • tests
  • timezone conversion from string needed
  • timestamp overflow raises ValueError/OverflowError/OSError
  • fields.DateTime subclasses doesn't need timezone and timezone_naive, but may want timestamp format (some api are storing dates in timestamps for some reason, maybe timezone replace for proper date from timestamp may be needed there?)

@lafrech
Copy link
Member

lafrech commented Jul 17, 2019

Using a format is the right approach for this. Typically in marshmallow, the name of the field class refers to the object type, not the serialized type. We're dealing here with datetimes serialized as timestamps so this PR is the right way and I'm closing #1009.

From a quick glance, I think what you're trying to achieve here could be done relatively easily and in a non-breaking change now that #1278 is merged.

@lafrech lafrech mentioned this pull request Jul 17, 2019
@sloria
Copy link
Member

sloria commented Sep 7, 2019

Are you still interested in this? If so, could you please get this up to date with dev?

@vgavro
Copy link
Contributor Author

vgavro commented Sep 9, 2019

@sloria This merge request (and idea behind it) is with conflict with current AwareDateTime/NaiveDateTime implementation, so decisions should be made before I can work further on timestamp functionality.
The problem with current decision with Naive/Aware DateTime fields
that we're forcing naivness or awareness only for deserialization,
not expecting that serialization target may implicitly assume timezone.
For timestamp format cases this is crucial, because some API (serialization target) timestamps are in their local timezones.

# Consider problems with current iso format
AwareDateTime("iso", default_tz=UTC)
deserialize("2017-12-12 17:00:00")  # dt(2017,12,12,17,0,0,tzinfo=UTC)
serialize(...)  # "2017-12-12 17:00:00+Z"
# problem here - what if serialization target (api) is not expecting +X part?

NaiveDateTime("iso", tz=UTC)
deserialize("2017-12-12 17:00:00+01")  # dt(2017,12,12,16,0,0)
serialize(...)  # "2017-12-12 16:00:00"
# problem here - what if serialization target (api) has "default_tz" set to UTC+01,
# so next time we can get 2017-12-12 16:00:00+01 ?

# Let's assume we want to add timestamp format with other than UTC timezone..
AwareDateTime("timestamp", default_tz=UTC+01)
deserialize(0)  # dt(1970,1,1,0,0,0, tzinfo=UTC+01)
serialize(...)  # -3600  # because timestamps expected to be in UTC...

SO - there are 4 ways to go:

  1. Not to break current Naive/Aware/Datetime logic:
    to rely that timestamps are always UTC, but in such cases we must not allow
    NaiveDateTime(timezone) and AwareDateTime(timezone) be other than UTC.
    ValueError on __init__ and None defaults to UTC on timestamp formats?
  2. Make "timestamp" formats SPECIAL usecases (AwareDateTime converts to default_timezone on timestamp serialization, NaiveDateTime converts to "timezone" on timestamp serialization). [bad idea, as we don't solve problems for other formats + breaking consistency and adding not obvious behaviour]
  3. Make TimestampField.
  4. [MY VOTE GOES HERE] (and this was the idea behind this PR before there was other decision in related ticket, because of that I started to work on separate field) Consider DateTimeField refactoring, which will be working AS CLOSE AS EXPECTED with all formats, but timestamp will be "special case" for serialization anyway.
class SmartDateTime:
# timezone=None - default timezone for naive datetimes on serialization/deserialization;
# naive=False - should datetimes be serialized/deserialized without timezone,
# if true - aware-datetimes will be converted to "timezone" before serialization/deserialization.

def deserialize(value):
    if is_aware(value):
        if self.naive:
            assert self.timezone
            return value.astimezone(self.timezone).replace(tzinfo=None)
        else:
            return value
    else:
        if self.naive:
            return value
        else:
            assert self.timezone
            return value.replace(self.timezone)

def serialize(value):
    if is_aware(value):
        if self.naive or self.format in timestamp_formats:
            assert self.timezone
            return value.astimezone(self.timezone).replace(tzinfo=None)
        return value
    else:
        if self.naive or self.format in timestamp_formats:
            return value
        else:
            assert self.timezone
            return value.replace(self.timezone)

@sloria
Copy link
Member

sloria commented Oct 31, 2019

Sorry I haven't had time to review this. @deckar01 or @lafrech can you take a look?

@lafrech
Copy link
Member

lafrech commented Nov 15, 2019

Thans @vgavro for investigating this.

Here's my understanding for now.

You're right about the API being incomplete. One could want to enforce awareness in the app and naiveness in the API and this is not possible currently, as you shown.

A timestamp is TZ aware (https://stackoverflow.com/questions/23062515/do-unix-timestamps-change-across-timezones) so it would be rather like

AwareDateTime("timestamp", default_tz=UTC+01)
deserialize(0)  # dt(1970,1,1,0,0,0, tzinfo=UTC)  # default_tz is not used since timestamps is aware
serialize(...)  # 0

However it fails in the NaiveDateTime case

NaiveDateTime("timestamp", tz=UTC-01)
deserialize(3600)  # dt(1970,1,1,0,0,0)
serialize(...)  # 0

We could add specific timestamp logic in the field or in timestamp dedicated util functions, but it would be better to address the root cause, which is the API not allowing to specify awareness on serialization.

@@ -317,11 +317,38 @@ def to_iso_date(date, *args, **kwargs):
return datetime.date.isoformat(date)


def from_timestamp(timestamp, tzinfo=UTC):
return datetime.fromutctimestamp(float(timestamp)).replace(tzinfo=tzinfo)

Choose a reason for hiding this comment

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

Per the documentation, datetime.fromtimestamp(value, tz=datetime.timezone.utc) is preferred over datetime.fromutctimestamp(value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants