diff --git a/src/documents/signals/handlers.py b/src/documents/signals/handlers.py index cfd2f185b..023962fab 100644 --- a/src/documents/signals/handlers.py +++ b/src/documents/signals/handlers.py @@ -19,6 +19,7 @@ from django.db import DatabaseError from django.db import close_old_connections from django.db import connections from django.db import models +from django.db import transaction from django.db.models import Q from django.dispatch import receiver from django.utils import timezone @@ -453,62 +454,94 @@ def update_filename_and_move_files( # This will in turn cause this logic to move the file where it belongs. return - with FileLock(settings.MEDIA_LOCK): - try: - # If this was waiting for the lock, the filename or archive_filename - # of this document may have been updated. This happens if multiple updates - # get queued from the UI for the same document - # So freshen up the data before doing anything - instance.refresh_from_db() + move_original = False + move_archive = False + old_filename = None + old_archive_filename = None + old_source_path = None + old_archive_path = None - old_filename = instance.filename - old_source_path = instance.source_path + try: + with transaction.atomic(): + Document.global_objects.select_for_update().get(pk=instance.pk) + with FileLock(settings.MEDIA_LOCK): + # If this was waiting for the lock, the filename or archive_filename + # of this document may have been updated. This happens if multiple updates + # get queued from the UI for the same document + # So freshen up the data before doing anything + instance.refresh_from_db() - candidate_filename = generate_filename(instance) - candidate_source_path = ( - settings.ORIGINALS_DIR / candidate_filename - ).resolve() - if candidate_filename == Path(old_filename): - new_filename = Path(old_filename) - elif ( - candidate_source_path.exists() - and candidate_source_path != old_source_path - ): - # Only fall back to unique search when there is an actual conflict - new_filename = generate_unique_filename(instance) - else: - new_filename = candidate_filename + old_filename = instance.filename + old_source_path = instance.source_path - # Need to convert to string to be able to save it to the db - instance.filename = str(new_filename) - move_original = old_filename != instance.filename - - old_archive_filename = instance.archive_filename - old_archive_path = instance.archive_path - - if instance.has_archive_version: - archive_candidate = generate_filename(instance, archive_filename=True) - archive_candidate_path = ( - settings.ARCHIVE_DIR / archive_candidate + candidate_filename = generate_filename(instance) + candidate_source_path = ( + settings.ORIGINALS_DIR / candidate_filename ).resolve() - if archive_candidate == Path(old_archive_filename): - new_archive_filename = Path(old_archive_filename) + if candidate_filename == Path(old_filename): + new_filename = Path(old_filename) elif ( - archive_candidate_path.exists() - and archive_candidate_path != old_archive_path + candidate_source_path.exists() + and candidate_source_path != old_source_path ): - new_archive_filename = generate_unique_filename( + # Only fall back to unique search when there is an actual conflict + new_filename = generate_unique_filename(instance) + else: + new_filename = candidate_filename + + # Need to convert to string to be able to save it to the db + instance.filename = str(new_filename) + move_original = old_filename != instance.filename + + old_archive_filename = instance.archive_filename + old_archive_path = instance.archive_path + + if instance.has_archive_version: + archive_candidate = generate_filename( instance, archive_filename=True, ) + archive_candidate_path = ( + settings.ARCHIVE_DIR / archive_candidate + ).resolve() + if archive_candidate == Path(old_archive_filename): + new_archive_filename = Path(old_archive_filename) + elif ( + archive_candidate_path.exists() + and archive_candidate_path != old_archive_path + ): + new_archive_filename = generate_unique_filename( + instance, + archive_filename=True, + ) + else: + new_archive_filename = archive_candidate + + instance.archive_filename = str(new_archive_filename) + + move_archive = old_archive_filename != instance.archive_filename else: - new_archive_filename = archive_candidate + move_archive = False - instance.archive_filename = str(new_archive_filename) + if move_original: + validate_move( + instance, + old_source_path, + instance.source_path, + settings.ORIGINALS_DIR, + ) + create_source_path_directory(instance.source_path) + shutil.move(old_source_path, instance.source_path) - move_archive = old_archive_filename != instance.archive_filename - else: - move_archive = False + if move_archive: + validate_move( + instance, + old_archive_path, + instance.archive_path, + settings.ARCHIVE_DIR, + ) + create_source_path_directory(instance.archive_path) + shutil.move(old_archive_path, instance.archive_path) if not move_original and not move_archive: # Just update modified. Also, don't save() here to prevent infinite recursion. @@ -517,26 +550,6 @@ def update_filename_and_move_files( ) return - if move_original: - validate_move( - instance, - old_source_path, - instance.source_path, - settings.ORIGINALS_DIR, - ) - create_source_path_directory(instance.source_path) - shutil.move(old_source_path, instance.source_path) - - if move_archive: - validate_move( - instance, - old_archive_path, - instance.archive_path, - settings.ARCHIVE_DIR, - ) - create_source_path_directory(instance.archive_path) - shutil.move(old_archive_path, instance.archive_path) - # Don't save() here to prevent infinite recursion. Document.global_objects.filter(pk=instance.pk).update( filename=instance.filename, @@ -546,22 +559,24 @@ def update_filename_and_move_files( # Clear any caching for this document. Slightly overkill, but not terrible clear_document_caches(instance.pk) - except (OSError, DatabaseError, CannotMoveFilesException) as e: - logger.warning(f"Exception during file handling: {e}") - # 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. + except (OSError, DatabaseError, CannotMoveFilesException) as e: + logger.warning(f"Exception during file handling: {e}") + # 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. + if move_original or move_archive: # Try to move files to their original location. try: - if move_original and instance.source_path.is_file(): - logger.info("Restoring previous original path") - shutil.move(instance.source_path, old_source_path) + with FileLock(settings.MEDIA_LOCK): + if move_original and instance.source_path.is_file(): + logger.info("Restoring previous original path") + shutil.move(instance.source_path, old_source_path) - if move_archive and instance.archive_path.is_file(): - logger.info("Restoring previous archive path") - shutil.move(instance.archive_path, old_archive_path) + if move_archive and instance.archive_path.is_file(): + logger.info("Restoring previous archive path") + shutil.move(instance.archive_path, old_archive_path) except Exception: # This is fine, since: @@ -574,23 +589,29 @@ def update_filename_and_move_files( # anyway. pass - # restore old values on the instance + # restore old values on the instance + if old_filename is not None: instance.filename = old_filename + if old_archive_filename is not None: instance.archive_filename = old_archive_filename - # finally, remove any empty sub folders. This will do nothing if - # something has failed above. - if not old_source_path.is_file(): - delete_empty_directories( - Path(old_source_path).parent, - root=settings.ORIGINALS_DIR, - ) + # finally, remove any empty sub folders. This will do nothing if + # something has failed above. + if old_source_path and not old_source_path.is_file(): + delete_empty_directories( + Path(old_source_path).parent, + root=settings.ORIGINALS_DIR, + ) - if instance.has_archive_version and not old_archive_path.is_file(): - delete_empty_directories( - Path(old_archive_path).parent, - root=settings.ARCHIVE_DIR, - ) + if ( + instance.has_archive_version + and old_archive_path + and not old_archive_path.is_file() + ): + delete_empty_directories( + Path(old_archive_path).parent, + root=settings.ARCHIVE_DIR, + ) @shared_task