Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#23832 closed New feature (fixed)

Storage API should provide a timezone aware approach

Reported by: James Aylett Owned by: James Aylett
Component: File uploads/storage Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Per #23827, the Storage API is effectively (and once the patch there is merged, explicitly) timezone naive. We should provide a way of using Storage objects that returns suitable aware objects instead.

A suggestion by Russell Keith-Magee is to introduce an optional parameter to these calls, with a tri-state value to select between naive and aware returns, with a default of None initially meaning "naive", but a deprecation path for this through to None meaning "aware".

Change History (18)

comment:1 by James Aylett, 9 years ago

(I'm looking at this at the django-sprint post-DUTH.)

comment:2 by James Aylett, 9 years ago

Status: newassigned

comment:3 by James Aylett, 9 years ago

PR Going looking for someone to review this :)

comment:4 by James Aylett, 9 years ago

Owner: James Aylett removed
Status: assignednew

Removing myself as assignee; I'll attempt to pick up comments & changes from review, but can't guarantee after today that I can do so in a timely fashion.

comment:5 by James Aylett, 9 years ago

Has patch: set

comment:6 by Russell Keith-Magee, 9 years ago

I've just taken a look at this; I think the answer might be a lot simpler that I originally thought.

getatime, getmtime and getctime all return seconds past epoch - which *has* a timezone; we're just not using that information. So we're in a position to construct a non-naïve timestamp - we're just not doing so. If we make the call to datetime.fromtimestamp and immediately cast to UTC, the timestamp will be correct and non-naïve.

So - at the very least, the timezone guessing in _possibly_make_required isn't required.

However, what's the concern about backwards compatibility here? Is there any reason that changing those methods to return non-naive timezones will cause a problem?

comment:7 by Aymeric Augustin, 9 years ago

If you plan to cast to UTC, I believe you should use datetime.utcfromtimestamp.

The concern is -- if people compare the output with a naive datetime, and we change that output to an aware datetime, the comparison will raise TypeError.

comment:8 by James Aylett, 9 years ago

Patch needs improvement: set

I share Aymeric's concern about comparison — it was also Denis' in — and that's something that collectstatic has to do, so unless we ditch BC with third-party Storage implementers we'd have to convert naive to aware datetimes (assuming localtime) when we compare. collectstatic is the only time in core (well, contrib.staticfiles) that we use any of these methods, but again there may be third parties relying on the silent behaviour of naive timestamps and doing their own comparison or difference.

+1 on datetime.utcfromtimestamp, and I can update the patch for that if we still want to tackle things in this way. If not, it's a much easier patch, although the documentation becomes more important to communicate the urgency of anyone using or implementing this API to update (and users with third-party extensions to upgrade).

(Also noticed that there are more tests that should really be updated to implement the new API, so I've marked the patch as needing improvement anyway.)

comment:9 by James Aylett, 9 years ago

Oh, noting that if we take the aware=True route for say 1.8, for BC reasons I think the right approach would be for collectstatic to continue not to pass aware at 1.8, and move to aware=True at 1.9. (This will then hopefully flush out during 1.9 testing any third parties who haven't implemented the new API, ahead of killing off the old behaviour for users at 2.0.) [And renumber 1.8..2.0 as appropriate if this ships later.]

comment:10 by James Aylett, 9 years ago

I've updated the patch a little, although not to use datetime.utcfromtimestamp as yet. Bit stumped by the code in which doesn't look like it's capable of testing what the comment says it's for, but is the remaining place that would need updating. (The tests still pass if I update the custom Storage backend in there, making me think that there's a test lying around that isn't really testing what it should be — the existing code should throw an exception.)

Not going to take this further until there's consensus on the approach we want to take.

comment:11 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Version: 1.7master

Some form of the idea seems to be accepted.

comment:12 by James Aylett, 8 years ago

Concrete proposal for solving this:

I'm still intending to do this, when I can find some time.

comment:13 by Tim Graham, 8 years ago

Patch needs improvement: unset

comment:14 by Tim Graham, 8 years ago

Patch needs improvement: set

Left comments for improvement on the pull request.

comment:15 by James Aylett, 8 years ago

Owner: set to James Aylett
Status: newassigned

comment:16 by Tim Graham, 8 years ago

Patch needs improvement: unset

comment:17 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 1ff6e37:

Fixed #23832 -- Added timezone aware Storage API.

New Storage.get_{accessed,created,modified}_time() methods convert the
naive time from now-deprecated {accessed,created_modified}_time()
methods into aware objects in UTC if USE_TZ=True.

comment:18 by Tim Graham <timograham@…>, 7 years ago

In 2d7fb779:

Refs #23832 -- Removed deprecated non-timezone aware Storage API.

Note: See TracTickets for help on using tickets.
Back to Top