Storage API should provide a timezone aware approach

[James Aylett]

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".

[James Aylett]

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

[James Aylett]

PR ​https://github.com/django/django/pull/3530. Going looking for someone to review this :)

[James Aylett]

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.

[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?

[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.

[James Aylett]

I share Aymeric's concern about comparison — it was also Denis' in https://code.djangoproject.com/ticket/22567 — 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.)

[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.]

[James Aylett]

I've updated the patch a little, although not to use datetime.utcfromtimestamp as yet. Bit stumped by the code in tests.staticfiles_tests.storage 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.

[Tim Graham]

Some form of the idea seems to be accepted.

[James Aylett]

Concrete proposal for solving this: ​https://groups.google.com/d/msg/django-developers/e6qY_qtjUDE/sFENDCYtAgAJ

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

[Tim Graham]

​PR

[Tim Graham]

Left comments for improvement on the pull request.

[Tim Graham <timograham@…>]

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.

[Tim Graham <timograham@…>]

In 2d7fb779:

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