From b34d7e1d9b53f393aa673cc431a26bc640164c39 Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Wed, 12 Dec 2012 16:29:14 +0100 Subject: [PATCH 1/3] Implement delete_dir in the FileStorage plus options for deleting only empty directories and deleting them recursively. Not sure how cloudfile storage is or should be handled here. Are things such as a "directory" even a concept there? --- mediagoblin/storage/__init__.py | 16 +++++++++++++++- mediagoblin/storage/filestorage.py | 26 ++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/mediagoblin/storage/__init__.py b/mediagoblin/storage/__init__.py index 2db4c37d..77e4212e 100644 --- a/mediagoblin/storage/__init__.py +++ b/mediagoblin/storage/__init__.py @@ -101,14 +101,28 @@ class StorageInterface(object): def delete_file(self, filepath): """ - Delete or dereference the file at filepath. + Delete or dereference the file (not directory) at filepath. + TODO: is the below comment correct? AFAIK, we won't clean up empty directories... This might need to delete directories, buckets, whatever, for cleanliness. (Be sure to avoid race conditions on that though) """ # Subclasses should override this method. self.__raise_not_implemented() + def delete_dir(self, dirpath, recursive=False): + """Delete the directory at dirpath + + :param recursive: Usually, a directory must not contain any + files for the delete to succeed. If True, containing files + and subdirectories within dirpath will be recursively + deleted. + + :returns: True in case of success, False otherwise. + """ + # Subclasses should override this method. + self.__raise_not_implemented() + def file_url(self, filepath): """ Get the URL for this file. This assumes our storage has been diff --git a/mediagoblin/storage/filestorage.py b/mediagoblin/storage/filestorage.py index 00d6335e..c86315f1 100644 --- a/mediagoblin/storage/filestorage.py +++ b/mediagoblin/storage/filestorage.py @@ -62,10 +62,32 @@ class BasicFileStorage(StorageInterface): return open(self._resolve_filepath(filepath), mode) def delete_file(self, filepath): - # TODO: Also delete unused directories if empty (safely, with - # checks to avoid race conditions). + """Delete file at filepath + + Raises OSError in case filepath is a directory.""" + #TODO: log error os.remove(self._resolve_filepath(filepath)) + def delete_dir(self, dirpath, recursive=False): + """returns True on succes, False on failure""" + + dirpath = self._resolve_filepath(dirpath) + + # Shortcut the default and simple case of nonempty=F, recursive=F + if recursive: + try: + shutil.rmtree(dirpath) + except OSError as e: + #TODO: log something here + return False + else: # recursively delete everything + try: + os.rmdir(dirpath) + except OSError as e: + #TODO: log something here + return False + return True + def file_url(self, filepath): if not self.base_url: raise NoWebServing( From 36ae6bcbbb0fc3ab0dbbc8dcba0664b3d7c5096f Mon Sep 17 00:00:00 2001 From: Sebastian Spaeth Date: Wed, 12 Dec 2012 16:32:43 +0100 Subject: [PATCH 2/3] Convert media processing backends to delete the queue directory (#254) We never deleted our queue directory which were created per submission. With the FileStorage backend being able to delete directories now, we can request the deletion of the task directory too. It will only be deleted if it is completely empty. --- mediagoblin/media_types/ascii/processing.py | 8 +++++++- mediagoblin/media_types/audio/processing.py | 8 +++++++- mediagoblin/media_types/image/processing.py | 8 ++++++-- mediagoblin/media_types/stl/processing.py | 8 ++++++-- mediagoblin/media_types/video/processing.py | 8 +++++++- 5 files changed, 33 insertions(+), 7 deletions(-) diff --git a/mediagoblin/media_types/ascii/processing.py b/mediagoblin/media_types/ascii/processing.py index 254717eb..dbc9661e 100644 --- a/mediagoblin/media_types/ascii/processing.py +++ b/mediagoblin/media_types/ascii/processing.py @@ -127,8 +127,14 @@ def process_ascii(entry, workbench=None): 'ascii', 'xmlcharrefreplace')) - mgg.queue_store.delete_file(queued_filepath) + # Remove queued media file from storage and database. + # queued_filepath is in the task_id directory which should + # be removed too, but fail if the directory is not empty to be on + # the super-safe side. + mgg.queue_store.delete_file(queued_filepath) # rm file + mgg.queue_store.delete_dir(queued_filepath[:-1]) # rm dir entry.queued_media_file = [] + media_files_dict = entry.setdefault('media_files', {}) media_files_dict['thumb'] = thumb_filepath media_files_dict['unicode'] = unicode_filepath diff --git a/mediagoblin/media_types/audio/processing.py b/mediagoblin/media_types/audio/processing.py index e12cefe6..a89d6634 100644 --- a/mediagoblin/media_types/audio/processing.py +++ b/mediagoblin/media_types/audio/processing.py @@ -147,4 +147,10 @@ def process_audio(entry, workbench=None): else: entry.media_files['thumb'] = ['fake', 'thumb', 'path.jpg'] - mgg.queue_store.delete_file(queued_filepath) + # Remove queued media file from storage and database. + # queued_filepath is in the task_id directory which should + # be removed too, but fail if the directory is not empty to be on + # the super-safe side. + mgg.queue_store.delete_file(queued_filepath) # rm file + mgg.queue_store.delete_dir(queued_filepath[:-1]) # rm dir + entry.queued_media_file = [] diff --git a/mediagoblin/media_types/image/processing.py b/mediagoblin/media_types/image/processing.py index e6a34ca0..2a5c463e 100644 --- a/mediagoblin/media_types/image/processing.py +++ b/mediagoblin/media_types/image/processing.py @@ -128,8 +128,12 @@ def process_image(entry, workbench=None): entry, name_builder.fill('{basename}{ext}')) mgg.public_store.copy_local_to_storage(queued_filename, original_filepath) - # Remove queued media file from storage and database - mgg.queue_store.delete_file(queued_filepath) + # Remove queued media file from storage and database. + # queued_filepath is in the task_id directory which should + # be removed too, but fail if the directory is not empty to be on + # the super-safe side. + mgg.queue_store.delete_file(queued_filepath) # rm file + mgg.queue_store.delete_dir(queued_filepath[:-1]) # rm dir entry.queued_media_file = [] # Insert media file information into database diff --git a/mediagoblin/media_types/stl/processing.py b/mediagoblin/media_types/stl/processing.py index 3089f295..12b87317 100644 --- a/mediagoblin/media_types/stl/processing.py +++ b/mediagoblin/media_types/stl/processing.py @@ -164,8 +164,12 @@ def process_stl(entry, workbench=None): with open(queued_filename, 'rb') as queued_file: model_file.write(queued_file.read()) - # Remove queued media file from storage and database - mgg.queue_store.delete_file(queued_filepath) + # Remove queued media file from storage and database. + # queued_filepath is in the task_id directory which should + # be removed too, but fail if the directory is not empty to be on + # the super-safe side. + mgg.queue_store.delete_file(queued_filepath) # rm file + mgg.queue_store.delete_dir(queued_filepath[:-1]) # rm dir entry.queued_media_file = [] # Insert media file information into database diff --git a/mediagoblin/media_types/video/processing.py b/mediagoblin/media_types/video/processing.py index 4c9f0131..68d14148 100644 --- a/mediagoblin/media_types/video/processing.py +++ b/mediagoblin/media_types/video/processing.py @@ -121,4 +121,10 @@ def process_video(entry, workbench=None): mgg.public_store.copy_local_to_storage(queued_filename, original_filepath) entry.media_files['original'] = original_filepath - mgg.queue_store.delete_file(queued_filepath) + # Remove queued media file from storage and database. + # queued_filepath is in the task_id directory which should + # be removed too, but fail if the directory is not empty to be on + # the super-safe side. + mgg.queue_store.delete_file(queued_filepath) # rm file + mgg.queue_store.delete_dir(queued_filepath[:-1]) # rm dir + entry.queued_media_file = [] From 0ac8317ddceee6d705c175e69c0825078246c7b9 Mon Sep 17 00:00:00 2001 From: Christopher Allan Webber Date: Tue, 22 Jan 2013 14:32:00 -0600 Subject: [PATCH 3/3] Removing docstring bit about delete_file possibly deleting directories in the future I agree that delete_dir as a separate operation is a better way to do things, especially since there is a non-recursive deletion option that will politely fail if the directory is not empty. --- mediagoblin/storage/__init__.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/mediagoblin/storage/__init__.py b/mediagoblin/storage/__init__.py index 77e4212e..57218eb5 100644 --- a/mediagoblin/storage/__init__.py +++ b/mediagoblin/storage/__init__.py @@ -102,10 +102,6 @@ class StorageInterface(object): def delete_file(self, filepath): """ Delete or dereference the file (not directory) at filepath. - - TODO: is the below comment correct? AFAIK, we won't clean up empty directories... - This might need to delete directories, buckets, whatever, for - cleanliness. (Be sure to avoid race conditions on that though) """ # Subclasses should override this method. self.__raise_not_implemented()