[Django] #23832: Storage API should provide a timezone aware approach
Django
--------------------------------------+---------------------
Reporter: jaylett | Owner: jaylett
Type: New feature | Status: new
Component: File uploads/storage | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+---------------------
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".
--
Ticket URL: <https://code.djangoproject.com/ticket/23832>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jaylett | Owner: jaylett
Type: New feature | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jaylett):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
(I'm looking at this at the django-sprint post-DUTH.)
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:1>
Django
Reporter: jaylett | Owner: jaylett
Type: New feature | Status: assigned
Component: File | Version: 1.7
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jaylett):
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:2>
Django
Reporter: jaylett | Owner: jaylett
Type: New feature | Status: assigned
Component: File | Version: 1.7
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jaylett):
PR https://github.com/django/django/pull/3530. Going looking for someone
to review this :)
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:3>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jaylett):
* status: assigned => new
* owner: jaylett =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:4>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jaylett):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:5>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by freakboy3742):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:6>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:7>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jaylett):
* needs_better_patch: 0 => 1
Comment:
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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:8>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by jaylett):
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.]
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:9>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by jaylett):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:10>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by timgraham):
* version: 1.7 => master
* stage: Unreviewed => Accepted
Comment:
Some form of the idea seems to be accepted.
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:11>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by jaylett):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:12>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/6108 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:13>
Django
Reporter: jaylett | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left comments for improvement on the pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:14>
Django
Reporter: jaylett | Owner: jaylett
Type: New feature | Status: assigned
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by jaylett):
* status: new => assigned
* owner: => jaylett
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:15>
Django
Reporter: jaylett | Owner: jaylett
Type: New feature | Status: assigned
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:16>
Django
Reporter: jaylett | Owner: jaylett
Type: New feature | Status: closed
Component: File uploads/storage | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1ff6e37de46f0cbf271a287a0ca67678e741a90a" 1ff6e37]:
{{{
#!CommitTicketReference repository=""
revision="1ff6e37de46f0cbf271a287a0ca67678e741a90a"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:17>
Django
Reporter: James Aylett | Owner: James
| Aylett
Type: New feature | Status: closed
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2d7fb779870583f8c6ee003f81bf67c4d7c21227" 2d7fb779]:
{{{
#!CommitTicketReference repository=""
revision="2d7fb779870583f8c6ee003f81bf67c4d7c21227"
Refs #23832 -- Removed deprecated non-timezone aware Storage API.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23832#comment:18>