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

Add load_as_tz_aware (and dump_as_tz_aware ?) parameter to DateTime #520

Closed
lafrech opened this issue Aug 31, 2016 · 9 comments
Closed

Add load_as_tz_aware (and dump_as_tz_aware ?) parameter to DateTime #520

lafrech opened this issue Aug 31, 2016 · 9 comments

Comments

@lafrech
Copy link
Member

lafrech commented Aug 31, 2016

As said in the docs,

Schema.load returns datetime objects that are timezone-aware.

I'm using webargs to parse input to an API and the code expects naive datetimes. I would like to get naive datetimes, rather than having to remove the TZ from every date attribute in each resource method.

Maybe I'm wrong working with naive datetimes in the first place. I have no opinion about it. But sometimes you're working with a lib that expects naive datetimes, and you don't want to patch it.

I guess the most flexible approach would be to provide a field parameter allowing to choose which format a DateTime field should output.

Could it be possible to have this configured with a flag/metadata in the DateTime field?

Or is there a design reason not to allow this?

(Apparently, I'm not the first one falling into this: #309.)

@maximkulkin
Copy link
Contributor

I wonder if the problem is in Marshmallow DateTime field implementation or in third-party software that in 2016 still uses naive datetimes and can't handle time zone aware ones. Maybe you need to be knocking their door to finally support that.

Anyways, it is easy to implement:

import marshmallow.fields as mf

class MyNaiveDateTime(mf.DateTime):
    def _deserialize(self, value, attr, data):
        result = super(MyNaiveDateTime, self)\
            ._deserialize(value, attr, data)
        return result.replace(tzinfo=None)

@lafrech
Copy link
Member Author

lafrech commented Sep 1, 2016

I was wrong, as explained here: DateTime deserializes a naive datetime string as a naive datetime. The doc is a bit misleading.

Now, assuming

  • my API exchanges datetimes as UTC according to ISO_8601 (e.g. 1966-05-24T00:00:00+00:00)
  • my business code assumes everything is UTC and works with naive datetimes

I still think it would be nice to be able to enforce a format for deserialization.

And in fact, I could extend this request to serialization as well.

Maybe my business code should be all TZ aware datetimes (Is this what you suggest?).

Thanks for the subclass hint.

Maybe you need to be knocking their door to finally support that.

Done: https://github.com/lepture/flask-oauthlib/issues/294

@lafrech
Copy link
Member Author

lafrech commented Sep 23, 2016

I ended up subclassing DateTime in a custom Field with a load_as_tz_aware boolean parameter. (Discussion here.)

Basically, it deserializes like this:

        date = super()._deserialize(value, attr, data)
        if self.load_as_tz_aware:
            # If datetime is TZ naive, set UTC timezone
            if date.tzinfo is None or date.tzinfo.utcoffset(date) is None:
                date = date.replace(tzinfo=tzutc())
        else:
            # If datetime is TZ aware, convert it to UTC and remove TZ info
            if date.tzinfo is not None and date.tzinfo.utcoffset(date) is not None:
                date = date.astimezone(tzutc())
            date = date.replace(tzinfo=None)
        return date

I still think this could be addressed in Marshmallow.

Using Marshmallow + webargs to parse API inputs is a typical use case, and currently, you can't rely on it to ensure the awareness of deserialized dates. This is an issue, especially since comparisons of aware and non-aware dates generate an exception: a user can make the application crash by providing wrong input data. Unless of course the application has specific code to cope with both formats, or ensures awareness by itself, but isn't this Marshmallow's purpose?

My subclass only addresses deserialization, but there may also be a use case for a dump_as_tz_aware parameter (which I think is a bit harder to do in a subclass).

@lafrech lafrech changed the title Allow DateTime to return a naive datetime Add load_as_tz_aware (and dump_as_tz_aware ?) parameter to DateTime Sep 23, 2016
@lafrech
Copy link
Member Author

lafrech commented Apr 3, 2018

I'd be tempted to make it symmetrical and enforce serialization awareness as well.

Also, I realize this lacks the possibility to choose the TZ (UTC is hardcoded). The user might want to enforce another TZ.

@sloria, any interest in adding this feature to DateTime in MA3? Depending on the implementation, it could be a breaking change.

Alternatively, we could add NaiveDateTime and AwareDateTime and keep DateTime as is. I'm not sure what's best.

@lafrech
Copy link
Member Author

lafrech commented Apr 3, 2018

Example:

class AwareDateTime(DateTime):
    def __init__(self, timezone):
        self.tz = dateutil.tz.gettz(timezone)
        [...]

utc_field = fields.AwareDateTime(timezone='UTC')
paris_field = fields.AwareDateTime(timezone='Europe/Paris')

The problem with allowing non-UTC timezones is that it requires dateutil.

@lafrech
Copy link
Member Author

lafrech commented May 14, 2018

Adding 3.0 milestone as I think this is an important pitfall. Feel free to postpone if you think it is not.

Also, wouldn't datetime stuff be easier if dateutil was not optional? Do you have a strong stand against adding it as required? Is it just to limit the dependencies to the bare minimum (which I can understand) or is there something specific about this one? umongo ended up adding it as required (Scille/umongo#17) for consistency.

@lafrech
Copy link
Member Author

lafrech commented May 14, 2018

Also, wouldn't datetime stuff be easier if dateutil was not optional?

Looks like it is more complicated than I thought: #497.

Also just noticed #349.

@lafrech
Copy link
Member Author

lafrech commented Sep 2, 2018

I've begun work on this (#933). Serialization is a bit more complicated than I expected.

Hopefully, this could be achieved with two new fields ([Aware|Naive]DateTime) in a non-breaking way, so I remove the "backwards incompat" tag and the 3.0 milestone.

@lafrech
Copy link
Member Author

lafrech commented Jul 3, 2019

Closing in favor of #1234.

@lafrech lafrech closed this as completed Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants