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 Changed 9 years ago by James Aylett

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

comment:2 Changed 9 years ago by James Aylett

Status: newassigned

comment:3 Changed 9 years ago by James Aylett

PR Going looking for someone to review this :)

comment:4 Changed 9 years ago by James Aylett

Owner: James Aylett deleted
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 Changed 9 years ago by James Aylett

Has patch: set

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

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 Changed 9 years ago by Aymeric Augustin

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 Changed 9 years ago by James Aylett

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 Changed 9 years ago by James Aylett

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 Changed 9 years ago by James Aylett

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 Changed 9 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Version: 1.7master

Some form of the idea seems to be accepted.

comment:12 Changed 8 years ago by James Aylett

Concrete proposal for solving this:

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

comment:13 Changed 8 years ago by Tim Graham

Patch needs improvement: unset

comment:14 Changed 8 years ago by Tim Graham

Patch needs improvement: set

Left comments for improvement on the pull request.

comment:15 Changed 8 years ago by James Aylett

Owner: set to James Aylett
Status: newassigned

comment:16 Changed 8 years ago by Tim Graham

Patch needs improvement: unset

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

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 Changed 7 years ago by Tim Graham <timograham@…>

In 2d7fb779:

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

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