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

Implement new Django time metadata interface where possible #1240

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TimoGuenther
Copy link
Contributor

@TimoGuenther TimoGuenther commented Apr 19, 2023

Closes #1233.

This PR normalizes the distribution of implementations of Django's newer get_*_time and its older *_time interfaces across the various storage backends. It does not add any new specific functionality, e.g., modified time on some backend that previously did not have this functionality.

On a technical level, this PR introduces a new TimeStorageMixin containing time metadata logic. This simplifies storage backends and their tests because they no longer have to deal with the specifics of the Django interface (i.e., converting timezones based on the USE_TZ setting). To do so, each subclass implements a custom _*_time interface that can return a naive or aware datetime, and TimeStorageMixin handles the rest.

Subclasses of TimeStorageMixin automatically support both the newer and the older interface if timezone information is present, else only the older *_time interface, by raising NotImplementedError where appropriate.

To ensure the new interface is definitely supported, subclasses may set _default_timezone. Azure, Dropbox, Google Cloud and S3 Boto3 all assume UTC datetime values by default, even if the underlying library should somehow fail to return aware datetime values. FTP and SFTP do not have a default timezone, but end users are free to subclass the storage and provide it if known. Apache Libcloud does not feature any time metadata methods.

Availability (✅ = implemented, ❌ = not implemented, ⚠️ = only old interface implemented but new interface easily implemented by providing a default timezone):

backend get_accessed_time get_created_time get_modified_time
Apache Libcloud
Azure
Dropbox
FTP ⚠️
Google Cloud
S3 Boto3
SFTP ⚠️ ⚠️

Breaking changes:

  • The implementations of the old interface may no longer return the same naive datetime where a default timezone is set, e.g., a naive datetime in the current timezone instead of a naive datetime in UTC. This was never specified by Django (hence the deprecation), but some end users might depend on it. Use the new interface instead and, if required, manually apply make_naive to its return value.

# Conflicts:
#	storages/backends/dropbox.py
#	storages/backends/s3boto3.py
@jschneier
Copy link
Owner

I suddenly got quite interested in this pull request when I read the following note from my colleague this morning about our attempt to upgrade from Django v4.1 to v4.2...

I haven't seen any breakages in displaying images from S3, but when trying to download a resource, it raises a 500 error: 'S3Storage' object has no attribute 'modified_time'.

@TimoGuenther Would it be possible to please rebase this PR given all the recent changes to the repo? @jschneier After it is rebased, could you please approve the GitHub Actions workflow so we can see if the tests pass?

The method modified_time was removed from Django several years ago so I removed support for it in this past version, properly it should have been migrated to get_modified_time.

@jschneier
Copy link
Owner

Just opened #1298 to mention the removal.

@TimoGuenther
Copy link
Contributor Author

TimoGuenther commented Sep 18, 2023

The method modified_time was removed from Django several years ago so I removed support for it in this past version, properly it should have been migrated to get_modified_time.

This PR would have accomplished that migration while maintaining some backward compatibility, so I'm surprised you simply deleted all those old time metadata methods without merging or at least reviewing this PR first. After all, I mentioned that the old time metadata methods have their purpose for FTP and SFTP due to lack of timezone information. How would we implement the new interface for these storages now that there are no means for accessing even naive time metadata? My use-case depends on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing implementations of the Django interface for time metadata due to missing timezone information
2 participants