From 5b56fad9c7ff3561cfd272dad9483cfea6cae040 Mon Sep 17 00:00:00 2001 From: jonaswinkler Date: Fri, 12 Feb 2021 01:31:50 +0100 Subject: [PATCH] fix test case --- src/documents/file_handling.py | 22 +++++++- src/documents/signals/handlers.py | 93 +++++++++++++++---------------- 2 files changed, 66 insertions(+), 49 deletions(-) diff --git a/src/documents/file_handling.py b/src/documents/file_handling.py index 8c0e2148b..d7a42db24 100644 --- a/src/documents/file_handling.py +++ b/src/documents/file_handling.py @@ -79,7 +79,19 @@ def many_to_dictionary(field): return mydictionary -def generate_unique_filename(doc, archive_filename=False): +def generate_unique_filename(doc, + archive_filename=False): + """ + Generates a unique filename for doc in settings.ORIGINALS_DIR. + + The returned filename is guaranteed to be either the current filename + of the document if unchanged, or a new filename that does not correspondent + to any existing files. The function will append _01, _02, etc to the + filename before the extension to avoid conflicts. + + If archive_filename is True, return a unique archive filename instead. + + """ if archive_filename: old_filename = doc.archive_filename root = settings.ARCHIVE_DIR @@ -87,6 +99,14 @@ def generate_unique_filename(doc, archive_filename=False): old_filename = doc.filename root = settings.ORIGINALS_DIR + # If generating archive filenames, try to make a name that is similar to + # the original filename first. + + if archive_filename and doc.filename: + new_filename = os.path.splitext(doc.filename)[0] + ".pdf" + if new_filename == old_filename or not os.path.exists(os.path.join(root, new_filename)): # NOQA: E501 + return new_filename + counter = 0 while True: diff --git a/src/documents/signals/handlers.py b/src/documents/signals/handlers.py index c5e213949..eb981661c 100755 --- a/src/documents/signals/handlers.py +++ b/src/documents/signals/handlers.py @@ -173,21 +173,23 @@ def cleanup_document_deletion(sender, instance, using, **kwargs): ) +class CannotMoveFilesException(Exception): + pass + + def validate_move(instance, old_path, new_path): if not os.path.isfile(old_path): # Can't do anything if the old file does not exist anymore. logger.fatal( f"Document {str(instance)}: File {old_path} has gone.") - return False + raise CannotMoveFilesException() if os.path.isfile(new_path): # Can't do anything if the new file already exists. Skip updating file. logger.warning( f"Document {str(instance)}: Cannot rename file " f"since target path {new_path} already exists.") - return False - - return True + raise CannotMoveFilesException() @receiver(models.signals.m2m_changed, sender=Document.tags.through) @@ -206,45 +208,40 @@ def update_filename_and_move_files(sender, instance, **kwargs): return with FileLock(settings.MEDIA_LOCK): - old_filename = instance.filename - new_filename = generate_unique_filename(instance) - move_original = old_filename != new_filename - old_source_path = instance.source_path - new_source_path = os.path.join(settings.ORIGINALS_DIR, new_filename) - - if move_original and not validate_move(instance, old_source_path, new_source_path): - return - - old_archive_filename = instance.archive_filename - - if instance.has_archive_version: - new_archive_filename = generate_unique_filename( - instance, archive_filename=True - ) - old_archive_path = instance.archive_path - new_archive_path = os.path.join(settings.ARCHIVE_DIR, - new_archive_filename) - - move_archive = old_archive_filename != new_archive_filename - if move_archive and not validate_move(instance, old_archive_path, new_archive_path): - return - else: - move_archive = False - - if not move_original and not move_archive: - # Don't do anything if filenames did not change. - return - try: + old_filename = instance.filename + old_source_path = instance.source_path + + instance.filename = generate_unique_filename(instance) + move_original = old_filename != instance.filename + + old_archive_filename = instance.archive_filename + old_archive_path = instance.archive_path + + if instance.has_archive_version: + + instance.archive_filename = generate_unique_filename( + instance, archive_filename=True + ) + + move_archive = old_archive_filename != instance.archive_filename # NOQA: E501 + else: + move_archive = False + + if not move_original and not move_archive: + # Don't do anything if filenames did not change. + return + if move_original: - instance.filename = new_filename - create_source_path_directory(new_source_path) - os.rename(old_source_path, new_source_path) + validate_move(instance, old_source_path, instance.source_path) + create_source_path_directory(instance.source_path) + os.rename(old_source_path, instance.source_path) if move_archive: - instance.archive_filename = new_archive_filename - create_source_path_directory(new_archive_path) - os.rename(old_archive_path, new_archive_path) + validate_move( + instance, old_archive_path, instance.archive_path) + create_source_path_directory(instance.archive_path) + os.rename(old_archive_path, instance.archive_path) # Don't save() here to prevent infinite recursion. Document.objects.filter(pk=instance.pk).update( @@ -252,23 +249,19 @@ def update_filename_and_move_files(sender, instance, **kwargs): archive_filename=instance.archive_filename, ) - except Exception as e: + except (OSError, DatabaseError, CannotMoveFilesException): # This happens when either: # - moving the files failed due to file system errors # - saving to the database failed due to database errors # In both cases, we need to revert to the original state. - # restore old values on the instance - instance.filename = old_filename - instance.archive_filename = old_archive_filename - # Try to move files to their original location. try: - if move_original and os.path.isfile(new_source_path): - os.rename(new_source_path, old_source_path) + if move_original and os.path.isfile(instance.source_path): + os.rename(instance.source_path, old_source_path) - if move_archive and os.path.isfile(new_archive_path): - os.rename(new_archive_path, old_archive_path) + if move_archive and os.path.isfile(instance.archive_path): + os.rename(instance.archive_path, old_archive_path) except Exception as e: # This is fine, since: @@ -281,6 +274,10 @@ def update_filename_and_move_files(sender, instance, **kwargs): # anyway. pass + # restore old values on the instance + instance.filename = old_filename + instance.archive_filename = old_archive_filename + # finally, remove any empty sub folders. This will do nothing if # something has failed above. if not os.path.isfile(old_source_path):