Use modified_time method provided by Storage interface instead of assuming os.path compatible storages when getting mod times #180

Merged
merged 1 commit into from Oct 24, 2013

Conversation

Projects
None yet
5 participants

As you're aware, checking file modified times is currently critical for the thumbnail_exists method (and much of the framework) to operate correctly. Currently, get_source_modtime & get_thumbnail_modtime, in files.py (line 461 & 470), are hardcoded to use os.path.getmtime to get the modtimes for thumbnails and sources. However, this assumes an os.path compatible Storage backend and neglects the fact that the Django Storage interface defines a "modified_time" method for this purpose.

As it stands, easy-thumbnails doesn't work out of the box with some custom storages because of the os.path assumption. However, there was no reason why easy-thumbnails couldn't work with any storage backend that implements the modified_time interface which Django's Storage prescribes. FileSystemStorage (and by extension ThumbnailFileSystemStorage) already implement modified_time by doing basically the same thing easy-thumbnails manually does (see django.core.files.storage.FileSystemStorage):

def modified_time(self, name): return datetime.fromtimestamp(os.path.getmtime(self.path(name)))

In sum, it seems more correct to use the storage's modified_time method rather than manually getting the modified time, and this enables different types of backends.

My commit swaps out the hardcoded call to os.path and calls the storage's modified_time method instead. Also, the call to utils.fromtimestamp at lines 437 and 449 become redundant, since the modified_time method in FileSystemStorage performs time stamp conversion already for file-based storages.

All tests pass, and I don't really see room for new tests beyond the current ones, since this is using the modified_time method of Django Storage or FileSystemStorage (or of a custom storage) which are tested in their respective packages. But i'd be happy to write a test if you can describe to me what it would do. Anecdotally, I've been using this code in production for a few months and it works great.

Owner

SmileyChris commented Nov 21, 2012

Also consider that modified_time is a relatively new addition to the Storage class, so we'll need to work around older versions not having this.

Probably some kind of "try the storage modified first, then fallback to the hard-coded os.path.getmtime) -- maybe as some util.

I see you commented out the timezone mtime fixes too, it'd be good to have some explanation of why that's necessary too.

You're right, looks like the modified_time was added in 1.3. I can take a stab at making that backward compatible.

W/r/t to the timezones, that was carelessness on my part. I made the change because FileSystemStorage.modified_time returns a datetime, not a timestamp, converted using datetime.datetime.fromtimestamp and didn't look closely at the utils method. We'd probably need to either modify the utils.fromtimestamp method to skip the conversion step if a datetime object is passed into it (at which point it would be somewhat confusingly named). Or, we could modify ThumbnailFileSystemStorage to provide a modified_time method that utilizes the utils.fromtimestamp and return a TZ aware datetime. (this would also reduce the likelihood of the backwards incompatibility issue actually coming into play). Would one of these approaches be preferred?

If you think this looks like a worthwhile update, I'd be happy to do a second round that addresses these issues.

Owner

SmileyChris commented Jan 14, 2013

The update is worthwhile.

We can't fix it in ThumbnailFileSystemStorage - that means that it's only backwards compatible for users using this storage.

Lets just make the get_source_modtime and get_thumbnail_modtime methods smarter, and make the utils skip conversion on datetime objects. Maybe even abstract these two similar methods to a single util if it makes sense?

Sounds good. I'll update the pull request later this week, and we'll see where it ends up at that point, since you have a better sense for the backwards-compatibility requirements.

@nebstrebor nebstrebor Gets modified_time from storage, if available. Moved common functiona…
…lity for getting thumbnail and source mod times to get_modified_time in utils.py

Fixes # 180

Since storages' modified_time functions return a datetime and not a timestamp, I changed the utils.py "fromtimestamp" function, which assumed a timestamp and performed the conversion and then did timezone-aware conversion, to "make_time_zone_aware" which assumes a datetime. To deal with the fallback case where a storage doesn't implement modified_time and we use os.path directly, the new utils.get_modified_time converts the timestamp to a datetime and return that for consistency.
6da1c28

Sorry if that took me a while... you know how it goes. I implemented what we had discussed. Let me know if you spot any problems.

1 Test fails (test_standard_filefield) but it fails before this commit. The new code works as expected. As a side note, is there an easy way to run tests for the easy_thumbnails project on its own? (without adding it to an existing django project and running ./manage.py test easy_thumbnails)

👍 for this. I can't seem to use easy_thumbnails with django-storages and s3boto without this commit. The thumbnail works once, but on the second view, easy_thumbnails tries to check the modtime and fails.

I tried using this branch and it did work for me.

Contributor

nebstrebor commented Aug 11, 2013

I'd be happy to see a merge here so I can ditch my fork! Let me know if there's anything holding it up.

pplante commented Oct 23, 2013

This just bit me today. We upgraded from Django 1.4 to 1.5.4 and it seems the changes in timezone handling changed what this library is expecting.

@SmileyChris SmileyChris added a commit that referenced this pull request Oct 24, 2013

@SmileyChris SmileyChris Merge pull request #180 from roberts81/master
Use modified_time method provided by Storage interface instead of assuming os.path compatible storages when getting mod times

Conflicts:
	easy_thumbnails/utils.py
e9bfad6

@SmileyChris SmileyChris merged commit 6da1c28 into SmileyChris:master Oct 24, 2013

Owner

SmileyChris commented Oct 24, 2013

@pplante - mind testing your code against easy-thumbnails master to ensure this fix is working?

Owner

SmileyChris commented Oct 24, 2013

Also note that I changed the code to only use the modification times from the storage if it's a local FS based storage engine (otherwise this would introduce more overhead)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment