From 2106f066157f73886fc0198cea62afd935a5147e Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Tue, 24 Feb 2026 20:16:55 +0100 Subject: [PATCH 01/11] misc: add logging to cdh.files --- dev/dev_project/settings.py | 32 ++++++++++++++++++++++++++- src/cdh/files/db/descriptors.py | 39 +++++++++++++++++++++++++++++---- src/cdh/files/db/fields.py | 24 ++++++++++++++++++++ src/cdh/files/db/wrappers.py | 35 ++++++++++++++++++++++++++++- src/cdh/files/logger.py | 4 ++++ src/cdh/files/signals.py | 3 ++- 6 files changed, 130 insertions(+), 7 deletions(-) create mode 100644 src/cdh/files/logger.py diff --git a/dev/dev_project/settings.py b/dev/dev_project/settings.py index 73c945be..211d419e 100644 --- a/dev/dev_project/settings.py +++ b/dev/dev_project/settings.py @@ -7,7 +7,7 @@ For the full list of settings and their values, see https://docs.djangoproject.com/en/3.2/ref/settings/ """ - +import os from pathlib import Path # Build paths inside the project like this: BASE_DIR / 'subdir'. @@ -177,6 +177,36 @@ 'django.contrib.auth.hashers.PBKDF2PasswordHasher', ] +# Logging +# + + +LOGGING = { + 'version': 1, + 'disable_existing_loggers': False, + 'handlers': { + 'console': { + 'class': 'logging.StreamHandler', + }, + }, + 'root': { + 'handlers': ['console'], + 'level': 'DEBUG', + }, + 'loggers': { + 'django': { + 'handlers': ['console'], + 'level': os.getenv('DJANGO_LOG_LEVEL', 'INFO'), + 'propagate': False, + }, + 'cdh.files': { + 'handlers': ['console'], + 'level': os.getenv('DJANGO_LOG_LEVEL', 'DEBUG'), + 'propagate': False, + }, + }, +} + # Internationalization # https://docs.djangoproject.com/en/3.2/topics/i18n/ diff --git a/src/cdh/files/db/descriptors.py b/src/cdh/files/db/descriptors.py index 2d153ffe..970bc14f 100644 --- a/src/cdh/files/db/descriptors.py +++ b/src/cdh/files/db/descriptors.py @@ -8,7 +8,19 @@ from cdh.files.db.manager import create_tracked_file_manager from cdh.files.db.wrappers import FileWrapper, TrackedFileWrapper - +from cdh.files.logger import logger + +def _debug(message: str): + """Helper function to log debug messages with a consistent format""" + logger.debug(f"FileDescriptor: {message}") + +def _warning(message: str): + """Helper function to log warning messages with a consistent format""" + logger.warning(f"FileDescriptor: {message}") + +def _error(message: str): + """Helper function to log error messages with a consistent format""" + logger.error(f"FileDescriptor: {message}") class FileDescriptor(ForeignKeyDeferredAttribute): """FileDescriptor handles the {field_name}_id field of a FileField""" @@ -53,6 +65,8 @@ def __get__(self, instance, cls=None): if instance is None: return self + _debug(f"Getting {self.field.name} for {instance}") + try: # See if we have it in cache, raises KeyError if not file_wrapper = self.field.get_cached_value(instance) @@ -66,8 +80,11 @@ def __get__(self, instance, cls=None): if file_wrapper is None or file_wrapper._removed: return None + _debug(f"Found {self.field.name} in cache; returning it") + return file_wrapper except KeyError: + _debug(f"No cached value for {self.field.name}; fetching from DB") # Try to fetch it from the DB instead file_obj = self.get_object(instance) @@ -103,20 +120,27 @@ def __set__(self, instance, value): Please see the helper methods for detailed info on how they are handled """ + _debug(f"Setting {self.field.name} for {instance}") if isinstance(value, tuple): + _debug(f"Got tuple (from widget); processing") return_value = self._set_from_tuple(instance, value) elif isinstance( value, self.field.remote_field.model._meta.concrete_model # NoQA ): + _debug(f"Got File instance; processing") return_value = self._set_from_db_instance(instance, value) elif isinstance(value, self.field.attr_class): + _debug(f"Got FileWrapper; processing") return_value = self._set_from_file_wrapper(instance, value) elif isinstance(value, File): + _debug(f"Got Django/Python-native File; processing") return_value = self._set_from_file_like_object(instance, value) elif value is None: + _debug(f"Got None; processing") return_value = self._set_from_none(instance) else: + _error(f"Got unknown type {type(value)}; aborting") # We can't be completely inclusive :o raise ValueError( 'Cannot assign "%r": "%s.%s" must be either a "%s" instance, ' @@ -129,9 +153,11 @@ def __set__(self, instance, value): ) ) + _debug(f"Setting {self.field.name} for {instance} to {return_value}") self.field.set_cached_value(instance, return_value) if return_value is None or return_value._removed: + _debug(f"After processing, file is to be deleted. Removing ORM links") # If we got returned None, or a FileWrapper marked for deletion, we # need to clear the fields that the ORM uses to link objects. # (In other words, the {field_name}_id field) @@ -205,6 +231,7 @@ def _set_from_tuple(self, instance, value): # say in whether it's actually deleted. (It might be referenced by a # different FileField). if uuid: + _debug(f"Marking existing FileWrapper for potential deletion: {uuid}") old_obj = self.field.remote_field.model.objects.get(uuid=uuid) old_file_wrapper = old_obj.get_file_wrapper(self.field) old_file_wrapper._removed = True @@ -247,6 +274,7 @@ def _set_from_file_wrapper(self, instance, value): # removed. (It's going to be overriden by an older version # apparently) if current_fw != value: + _debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") current_fw._removed = True # Make sure we won't remove it value._removed = False @@ -258,12 +286,15 @@ def _set_from_file_wrapper(self, instance, value): # we are replacing it. We don't need to cache it, as __get__ would # have done that for us if current_fw and current_fw != value: + _debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") current_fw._removed = True elif current_fw == value: + _debug(f"We were given the same FileWrapper as before, removing any potential deletion marker") value._removed = False # Create a file instance if one isn't present if not value.file_instance: + _debug(f"Creating new file instance for FileWrapper {value.uuid}") value.file_instance = self._create_file_instance() # Make sure our new file_instance knows of this wrapper :) value.file_instance.set_file_wrapper(value, self.field) @@ -279,6 +310,7 @@ def _set_from_file_wrapper(self, instance, value): # the only reason you don't receive a FW with a file_instance is if a # programmer created an empty (non field-attached) FileWrapper) elif value.field != self.field: + _warning(f"We were given a FileWrapper for a different field ({value.field.name}); this is programmer error. Fixing by creating a new FileWrapper for this field.") file = value.file value = value.file_instance.get_file_wrapper(self.field, False) value.file = file @@ -297,6 +329,7 @@ def _set_from_file_like_object(self, instance, value): try: current_fw = self.__get__(instance) if current_fw is not None: + _debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") current_fw._removed = True except self.RelatedObjectDoesNotExist: pass @@ -316,9 +349,6 @@ def _set_from_none(self, instance): # First, see if we have a value try: obj = self.__get__(instance) - # If we got a result, mark it as to be removed - if obj: - obj._removed = True except self.RelatedObjectDoesNotExist: # If we get this exception, we are not allowed to set None because # the field does not allow it @@ -338,6 +368,7 @@ def _set_from_none(self, instance): # 2) It's against normal Django behaviour to alter ANY data before # calling .save() (or .remove() for that matter). Thus, we need to # wait till the programmer expects things to change. + _debug(f"Marking existing FileWrapper for potential deletion: {obj.uuid}") obj._removed = True return obj diff --git a/src/cdh/files/db/fields.py b/src/cdh/files/db/fields.py index e0479b90..bbaf5c3c 100644 --- a/src/cdh/files/db/fields.py +++ b/src/cdh/files/db/fields.py @@ -21,8 +21,18 @@ from .models import BaseFile, File from .wrappers import FileWrapper, TrackedFileWrapper +from cdh.files.logger import logger + NOT_PROVIDED = object() +def _debug(message: str): + """Helper function to log debug messages with a consistent format""" + logger.debug(f"DB Field: {message}") + +def _info(message: str): + """Helper function to log debug messages with a consistent format""" + logger.info(f"DB Field: {message}") + class FileFieldCacheMixin: """Provide an API for working with the model's fields value cache. @@ -197,6 +207,10 @@ def __init__(self, to=None, on_delete=None, to_field=None, db_constraint=True, self.db_constraint = db_constraint self.url_pattern = url_pattern + @property + def full_name(self): + return f"{self.model._meta.app_label}.{self.model._meta.model_name}.{self.name}" + def check(self, **kwargs): return [ *super().check(**kwargs), @@ -417,10 +431,12 @@ def pre_save(self, model_instance, add): """Make sure we save our file when saving the model this field is attached to""" ret = super().pre_save(model_instance, add) + _debug(f"pre_save called for FileField {self.full_name}") file = getattr(model_instance, self.name, None) # If we have a file and it's marked as not-saved (committed) if file is not None and not file._committed: + _debug(f"File not saved yet, committing it now: {file}") # Commit the file to storage prior to saving the model # This will also save the File instance file.save() @@ -429,8 +445,10 @@ def pre_save(self, model_instance, add): def post_save(self, sender, instance, created, **kwargs): """Handle deletion of a file""" + _debug(f"post_save called for FileField {self.full_name}") # If we have something in cache if self.is_cached(instance): + _debug(f"FileField {self.full_name} is cached, checking for any to-delete files") # Get all values # The cache can contain multiple values, in which case all but # the last of them should be marked for deletion @@ -441,6 +459,7 @@ def post_save(self, sender, instance, created, **kwargs): # referenced by a different FileField, in which case # value.delete() will simple stop execution if value is not None and value._removed: + _info(f"File marked for deletion, starting delete for {value}") # Terminate this file value.delete() # And clear our cached value @@ -455,15 +474,19 @@ def pre_delete(self, sender, instance, **kwargs): # we simply force our file into the cache if it exists so we can delete # it after the instance is deleted. getattr(instance, self.name, None) + _debug(f"pre_delete called for FileField {self.full_name}; prepopulating cache") def post_delete(self, sender, instance, **kwargs): # When our model was deleted, we should see if our cache contains files # that need to be deleted + _debug(f"post_delete called for FileField {self.full_name}; checking for any to-delete files") if self.is_cached(instance): + _debug(f"FileField {self.name} is cached, continue checking for any to-delete files") value = self.get_cached_value(instance, None) num_references = value.file_instance._num_child_instances # Delete the File if no other object is referencing it if value is not None and num_references == 0: + _info(f"FileField {self.full_name} has no more references, starting delete for {value}") value.delete() def contribute_to_class(self, *args, **kwargs): @@ -471,6 +494,7 @@ def contribute_to_class(self, *args, **kwargs): # Connect ourselves to some signals; # Fields really ought to have these methods themselves Django, # you already provide the pre_save method! + _debug(f"Connecting signals for FileField {self.full_name}") post_save.connect(self.post_save, sender=self.model) pre_delete.connect(self.pre_delete, sender=self.model) post_delete.connect(self.post_delete, sender=self.model) diff --git a/src/cdh/files/db/wrappers.py b/src/cdh/files/db/wrappers.py index 525a5a54..2ced87d7 100644 --- a/src/cdh/files/db/wrappers.py +++ b/src/cdh/files/db/wrappers.py @@ -9,13 +9,31 @@ from .. import settings from ..mime_names import get_name_from_mime from ..utils import get_storage - +from ..logger import logger if TYPE_CHECKING: from . import TrackedFileField from .models import BaseFile +def _debug(message: str): + """Helper function to log debug messages with a consistent format""" + logger.debug(f"FileWrapper: {message}") + + +def _info(message: str): + """Helper function to log warning messages with a consistent format""" + logger.info(f"FileWrapper: {message}") + +def _warning(message: str): + """Helper function to log warning messages with a consistent format""" + logger.warning(f"FileWrapper: {message}") + +def _error(message: str): + """Helper function to log error messages with a consistent format""" + logger.error(f"FileWrapper: {message}") + + class FileWrapper(File): """ """ @@ -114,6 +132,7 @@ def _get_file(self): if getattr(self, '_file', None) is None: self._file = self.storage.open(self.name_on_disk, 'rb') except FileNotFoundError: + _debug(f"File {self.name_on_disk} not found on disk") self._file = None return self._file @@ -174,7 +193,9 @@ def save(self, content=None, original_filename=None): # If we overwrite the file this instance represents, we need to first # delete the old one, as otherwise we would lose the new file if self.storage.exists(self.name_on_disk): + _info(f"Removing {self.name_on_disk} before saving new file") self.storage.delete(self.name_on_disk) + _info(f"Saving {self.name_on_disk}") self.storage.save( self.name_on_disk, content, @@ -186,6 +207,7 @@ def save(self, content=None, original_filename=None): # I just liked saying 'use MAGIC' with self.open() as file: mime = magic.from_buffer(file.read(2048), mime=True) + _debug(f"Detected MIME type {mime} for file {self.name_on_disk}") self.file_instance.content_type = mime if original_filename: @@ -216,7 +238,10 @@ def delete(self, save=True, force=False): :param force: Whether to force a deletion if multiple DB objects still refer to it, defaults to False """ + _debug(f"Deleting FileWrapper {self.uuid} (save={save}, force={force})") + if not self.storage.exists(self.name_on_disk): + _warning(f"File {self.name_on_disk} does not exist on disk, skipping deletion") return # By default, only delete if there are no references in the DB anymore @@ -227,27 +252,35 @@ def delete(self, save=True, force=False): if save and self.file_instance: model = self.file_instance.__class__ if model.objects.filter(pk=self.file_instance.pk).exists(): + _debug(f"File object for FileWrapper {self.uuid} still exists, allowing deletion with 1 reference") deletion_threshold += 1 + logger.debug(f"Deletion threshold for FileWrapper {self.uuid}: {deletion_threshold}") + logger.debug(f"FileWrapper {self.uuid} has {self.file_instance._num_child_instances} references") + # Check if we only have the allowed amount number of references or fewer # If we have more, and we're not forcing a deletion, stop right here! if self.file_instance and \ self.file_instance._num_child_instances > deletion_threshold and \ not force: + _warning(f"FileWrapper {self.uuid} still has more references than allowed ({deletion_threshold}), skipping deletion!") return # First, delete our metadata model. The check above _should_ make sure # we don't get integrity errors, but it's better to have this fail # because of those errors before we have actually deleted the file if save: + _debug(f"Deleting FileWrapper {self.uuid} from DB") self.file_instance.delete() # Only close the file if it's already open, which we know by the # presence of self._file if hasattr(self, '_file'): + _debug(f"Closing file handle for FileWrapper {self.uuid}") self.close() del self.file + _debug(f"Deleting file {self.name_on_disk} from disk") self.storage.delete(self.name_on_disk) self.original_filename = None diff --git a/src/cdh/files/logger.py b/src/cdh/files/logger.py new file mode 100644 index 00000000..93bf5f08 --- /dev/null +++ b/src/cdh/files/logger.py @@ -0,0 +1,4 @@ +import logging + +logger = logging.getLogger('cdh.files') + diff --git a/src/cdh/files/signals.py b/src/cdh/files/signals.py index 0ee3c8cc..a39acbad 100644 --- a/src/cdh/files/signals.py +++ b/src/cdh/files/signals.py @@ -2,7 +2,7 @@ from django.db.models.signals import pre_delete from cdh.files.db import BaseFile - +from cdh.files.logger import logger def delete_file_on_delete(sender, instance, **kwargs): """Deletes the file on disk when the corresponding File is deleted""" @@ -11,6 +11,7 @@ def delete_file_on_delete(sender, instance, **kwargs): # want to delete it prematurely) # force=True means we will ALWAYS delete the file, even if the ORM still # sees some references to it + logger.debug(f"Signals: pre_delete called for {sender.__name__}; deleting file for {instance}") instance.get_file_wrapper().delete(save=False, force=True) From 7debf091d0a72535bc4e7f657f6d7cdbeefa62d1 Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Tue, 24 Feb 2026 20:49:23 +0100 Subject: [PATCH 02/11] fix: fixed bug that allowed referenced files to be seen as safe-to-delete --- src/cdh/files/db/wrappers.py | 26 ++++---------------------- src/cdh/files/signals.py | 8 +++----- 2 files changed, 7 insertions(+), 27 deletions(-) diff --git a/src/cdh/files/db/wrappers.py b/src/cdh/files/db/wrappers.py index 2ced87d7..1d98f7c6 100644 --- a/src/cdh/files/db/wrappers.py +++ b/src/cdh/files/db/wrappers.py @@ -227,7 +227,7 @@ def save(self, content=None, original_filename=None): save.alters_data = True - def delete(self, save=True, force=False): + def delete(self, save=True): """Deletes the file on disk. If save = True, the metadata object will also be deleted. Note: only delete the metadata object if no other DB object is referencing it, otherwise you'll get nasty Integrity @@ -235,35 +235,17 @@ def delete(self, save=True, force=False): :param save: Whether to also delete the metadata in the DB, defaults to True - :param force: Whether to force a deletion if multiple DB objects still - refer to it, defaults to False """ - _debug(f"Deleting FileWrapper {self.uuid} (save={save}, force={force})") + _debug(f"Deleting FileWrapper {self.uuid} (save={save})") if not self.storage.exists(self.name_on_disk): _warning(f"File {self.name_on_disk} does not exist on disk, skipping deletion") return - # By default, only delete if there are no references in the DB anymore - deletion_threshold = 0 - - # If we are instructed to also destroy our file_instance and we still - # have a reference, we allow deletion with 1 more reference - if save and self.file_instance: - model = self.file_instance.__class__ - if model.objects.filter(pk=self.file_instance.pk).exists(): - _debug(f"File object for FileWrapper {self.uuid} still exists, allowing deletion with 1 reference") - deletion_threshold += 1 - - logger.debug(f"Deletion threshold for FileWrapper {self.uuid}: {deletion_threshold}") - logger.debug(f"FileWrapper {self.uuid} has {self.file_instance._num_child_instances} references") - # Check if we only have the allowed amount number of references or fewer # If we have more, and we're not forcing a deletion, stop right here! - if self.file_instance and \ - self.file_instance._num_child_instances > deletion_threshold and \ - not force: - _warning(f"FileWrapper {self.uuid} still has more references than allowed ({deletion_threshold}), skipping deletion!") + if self.file_instance and self.file_instance._num_child_instances > 0: + _warning(f"FileWrapper {self.uuid} still has references, skipping deletion!") return # First, delete our metadata model. The check above _should_ make sure diff --git a/src/cdh/files/signals.py b/src/cdh/files/signals.py index a39acbad..28ee1f6b 100644 --- a/src/cdh/files/signals.py +++ b/src/cdh/files/signals.py @@ -1,5 +1,5 @@ from django.apps import apps -from django.db.models.signals import pre_delete +from django.db.models.signals import pre_delete, post_delete from cdh.files.db import BaseFile from cdh.files.logger import logger @@ -9,12 +9,10 @@ def delete_file_on_delete(sender, instance, **kwargs): # save=False means we will only touch the file on disk, leaving the DB # object alone. (That will obviously be handled by the ORM, so we don't # want to delete it prematurely) - # force=True means we will ALWAYS delete the file, even if the ORM still - # sees some references to it logger.debug(f"Signals: pre_delete called for {sender.__name__}; deleting file for {instance}") - instance.get_file_wrapper().delete(save=False, force=True) + instance.get_file_wrapper().delete(save=False) for model in apps.get_models(): if issubclass(model, BaseFile): - pre_delete.connect(delete_file_on_delete, sender=model) + post_delete.connect(delete_file_on_delete, sender=model) From fd351754be66020f22da65ad7b1d9e15ae9b99fc Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Tue, 24 Feb 2026 20:55:08 +0100 Subject: [PATCH 03/11] fix: add sanity check to prevent deleting files with active DB references --- src/cdh/files/db/wrappers.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/cdh/files/db/wrappers.py b/src/cdh/files/db/wrappers.py index 1d98f7c6..1e84b9ec 100644 --- a/src/cdh/files/db/wrappers.py +++ b/src/cdh/files/db/wrappers.py @@ -3,7 +3,7 @@ from django.core.files import File import magic -from django.db.models import Manager +from django.db.models import Manager, Q from django.urls import reverse_lazy from .. import settings @@ -255,6 +255,17 @@ def delete(self, save=True): _debug(f"Deleting FileWrapper {self.uuid} from DB") self.file_instance.delete() + # This is a sanity check; at this point we should have no references to + # this file anymore, so we should be able to safely delete it from disk + # However, if for some reason this method was called with save=False, + # with a still existing file_instance, some code messed up. + # This catches that situation. + if self.file_instance: + model = self.file_instance._meta.model + if model.objects.filter(Q(pk=self.file_instance.pk) | Q(uuid=self.uuid)).exists(): + _error(f"FileWrapper {self.uuid} still has references in DB, this should not happen!") + return + # Only close the file if it's already open, which we know by the # presence of self._file if hasattr(self, '_file'): From e00899492ca6f70bebd62cd9920e0e9b879cabad Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Tue, 24 Feb 2026 20:57:50 +0100 Subject: [PATCH 04/11] feat: global kill-switch on file-deletion --- src/cdh/files/settings.py | 6 ++++++ src/cdh/files/storage.py | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/src/cdh/files/settings.py b/src/cdh/files/settings.py index 4d82ace9..364a1b0b 100644 --- a/src/cdh/files/settings.py +++ b/src/cdh/files/settings.py @@ -24,6 +24,12 @@ settings.FILE_UPLOAD_DIRECTORY_PERMISSIONS, ) +FILE_BLOCK_DELETION = getattr( + settings, + 'CDH_FILES_FILE_BLOCK_DELETION', + False, +) + _tlum_loaded = 'cdh.core.middleware.ThreadLocalUserMiddleware' in \ settings.MIDDLEWARE diff --git a/src/cdh/files/storage.py b/src/cdh/files/storage.py index 76ca84b9..294e2618 100644 --- a/src/cdh/files/storage.py +++ b/src/cdh/files/storage.py @@ -32,6 +32,13 @@ def file_permissions_mode(self): def directory_permissions_mode(self): return settings.FILE_UPLOAD_DIRECTORY_PERMISSIONS + def delete(self, name): + # Don't delete files if block deletion is enabled + if settings.FILE_BLOCK_DELETION: + return + + super().delete(name) + class DefaultStorage(LazyObject): def _setup(self): From d7baa8a785f09ae331fa4c5c0cb721c5932e097d Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Tue, 24 Feb 2026 21:33:07 +0100 Subject: [PATCH 05/11] docs: document the hard-kill switch on file deletion --- src/cdh/files/settings.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/cdh/files/settings.py b/src/cdh/files/settings.py index 364a1b0b..f6f8efec 100644 --- a/src/cdh/files/settings.py +++ b/src/cdh/files/settings.py @@ -24,6 +24,13 @@ settings.FILE_UPLOAD_DIRECTORY_PERMISSIONS, ) +# This is a hard-kill switch on any file deletion. It works by making the +# remove call on the storage backend a no-op. (So it will delete DB references, +# but not the actual file). +# This is intended for when you think the files-app is eating your files. +# It should be safe to enable; HOWEVER! If your code manipulates the underlying +# `FileWrapper` objects directly, you may encounter issues. +# (You should refrain from doing that, but be warned) FILE_BLOCK_DELETION = getattr( settings, 'CDH_FILES_FILE_BLOCK_DELETION', From d91a26b01f30bb7266387b37a87f9d209e2d4c15 Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Tue, 24 Feb 2026 21:43:33 +0100 Subject: [PATCH 06/11] fix: ensure file operations run post-transaction to prevent data loss --- src/cdh/files/db/wrappers.py | 39 ++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/cdh/files/db/wrappers.py b/src/cdh/files/db/wrappers.py index 1e84b9ec..8077733f 100644 --- a/src/cdh/files/db/wrappers.py +++ b/src/cdh/files/db/wrappers.py @@ -4,6 +4,7 @@ from django.core.files import File import magic from django.db.models import Manager, Q +from django.db import transaction from django.urls import reverse_lazy from .. import settings @@ -34,6 +35,26 @@ def _error(message: str): logger.error(f"FileWrapper: {message}") +def _save_file(storage, name_on_disk, content, max_length): + """Does the actual saving of the file to disk. This method is called by + FileWrapper.save() and should not be called directly. + + This is a separate function to allow it to run as a post-transaction + hook. (This is needed, doing it during a transaction may result in dataloss + when a transaction is rolled back) + """ + # If we overwrite the file this instance represents, we need to first + # delete the old one, as otherwise we would lose the new file + if storage.exists(name_on_disk): + _info(f"Removing {name_on_disk} before saving new file") + storage.delete(name_on_disk) + _info(f"Saving {name_on_disk}") + storage.save( + name_on_disk, + content, + max_length=max_length + ) + class FileWrapper(File): """ """ @@ -190,17 +211,11 @@ def save(self, content=None, original_filename=None): if original_filename is None and hasattr(content, 'name'): original_filename = content.name - # If we overwrite the file this instance represents, we need to first - # delete the old one, as otherwise we would lose the new file - if self.storage.exists(self.name_on_disk): - _info(f"Removing {self.name_on_disk} before saving new file") - self.storage.delete(self.name_on_disk) - _info(f"Saving {self.name_on_disk}") - self.storage.save( + transaction.on_commit(lambda: _save_file( + self.storage, self.name_on_disk, - content, - max_length=self.field.max_length - ) + content, self.field.max_length + )) self._committed = True # Use magic to determine the mime type. It's pretty obvious, I know @@ -274,7 +289,9 @@ def delete(self, save=True): del self.file _debug(f"Deleting file {self.name_on_disk} from disk") - self.storage.delete(self.name_on_disk) + # Run this as part of a post-transaction hook to make sure we only + # delete the file once the related database changes have been committed. + transaction.on_commit(lambda: self.storage.delete(self.name_on_disk)) self.original_filename = None self._committed = False From 37f47c6d88c5b8525ceca13aed7359f799be39f7 Mon Sep 17 00:00:00 2001 From: Michael Villeneuve Date: Mon, 23 Mar 2026 16:06:16 +0100 Subject: [PATCH 07/11] feat: Simplify logger summoning --- src/cdh/files/db/descriptors.py | 57 ++++++++++++++------------------- src/cdh/files/db/fields.py | 31 +++++++----------- src/cdh/files/db/wrappers.py | 44 ++++++++----------------- src/cdh/files/logger.py | 4 --- src/cdh/files/signals.py | 5 ++- 5 files changed, 54 insertions(+), 87 deletions(-) delete mode 100644 src/cdh/files/logger.py diff --git a/src/cdh/files/db/descriptors.py b/src/cdh/files/db/descriptors.py index 970bc14f..6dc7c938 100644 --- a/src/cdh/files/db/descriptors.py +++ b/src/cdh/files/db/descriptors.py @@ -8,19 +8,10 @@ from cdh.files.db.manager import create_tracked_file_manager from cdh.files.db.wrappers import FileWrapper, TrackedFileWrapper -from cdh.files.logger import logger - -def _debug(message: str): - """Helper function to log debug messages with a consistent format""" - logger.debug(f"FileDescriptor: {message}") - -def _warning(message: str): - """Helper function to log warning messages with a consistent format""" - logger.warning(f"FileDescriptor: {message}") - -def _error(message: str): - """Helper function to log error messages with a consistent format""" - logger.error(f"FileDescriptor: {message}") + +import logging +logger = logging.getLogger(__name__) + class FileDescriptor(ForeignKeyDeferredAttribute): """FileDescriptor handles the {field_name}_id field of a FileField""" @@ -65,7 +56,7 @@ def __get__(self, instance, cls=None): if instance is None: return self - _debug(f"Getting {self.field.name} for {instance}") + logger.debug(f"Getting {self.field.name} for {instance}") try: # See if we have it in cache, raises KeyError if not @@ -80,11 +71,11 @@ def __get__(self, instance, cls=None): if file_wrapper is None or file_wrapper._removed: return None - _debug(f"Found {self.field.name} in cache; returning it") + logger.debug(f"Found {self.field.name} in cache; returning it") return file_wrapper except KeyError: - _debug(f"No cached value for {self.field.name}; fetching from DB") + logger.debug(f"No cached value for {self.field.name}; fetching from DB") # Try to fetch it from the DB instead file_obj = self.get_object(instance) @@ -120,27 +111,27 @@ def __set__(self, instance, value): Please see the helper methods for detailed info on how they are handled """ - _debug(f"Setting {self.field.name} for {instance}") + logger.debug(f"Setting {self.field.name} for {instance}") if isinstance(value, tuple): - _debug(f"Got tuple (from widget); processing") + logger.debug(f"Got tuple (from widget); processing") return_value = self._set_from_tuple(instance, value) elif isinstance( value, self.field.remote_field.model._meta.concrete_model # NoQA ): - _debug(f"Got File instance; processing") + logger.debug(f"Got File instance; processing") return_value = self._set_from_db_instance(instance, value) elif isinstance(value, self.field.attr_class): - _debug(f"Got FileWrapper; processing") + logger.debug(f"Got FileWrapper; processing") return_value = self._set_from_file_wrapper(instance, value) elif isinstance(value, File): - _debug(f"Got Django/Python-native File; processing") + logger.debug(f"Got Django/Python-native File; processing") return_value = self._set_from_file_like_object(instance, value) elif value is None: - _debug(f"Got None; processing") + logger.debug(f"Got None; processing") return_value = self._set_from_none(instance) else: - _error(f"Got unknown type {type(value)}; aborting") + logger.error(f"Got unknown type {type(value)}; aborting") # We can't be completely inclusive :o raise ValueError( 'Cannot assign "%r": "%s.%s" must be either a "%s" instance, ' @@ -153,11 +144,11 @@ def __set__(self, instance, value): ) ) - _debug(f"Setting {self.field.name} for {instance} to {return_value}") + logger.debug(f"Setting {self.field.name} for {instance} to {return_value}") self.field.set_cached_value(instance, return_value) if return_value is None or return_value._removed: - _debug(f"After processing, file is to be deleted. Removing ORM links") + logger.debug(f"After processing, file is to be deleted. Removing ORM links") # If we got returned None, or a FileWrapper marked for deletion, we # need to clear the fields that the ORM uses to link objects. # (In other words, the {field_name}_id field) @@ -231,7 +222,7 @@ def _set_from_tuple(self, instance, value): # say in whether it's actually deleted. (It might be referenced by a # different FileField). if uuid: - _debug(f"Marking existing FileWrapper for potential deletion: {uuid}") + logger.debug(f"Marking existing FileWrapper for potential deletion: {uuid}") old_obj = self.field.remote_field.model.objects.get(uuid=uuid) old_file_wrapper = old_obj.get_file_wrapper(self.field) old_file_wrapper._removed = True @@ -274,7 +265,7 @@ def _set_from_file_wrapper(self, instance, value): # removed. (It's going to be overriden by an older version # apparently) if current_fw != value: - _debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") + logger.debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") current_fw._removed = True # Make sure we won't remove it value._removed = False @@ -286,15 +277,15 @@ def _set_from_file_wrapper(self, instance, value): # we are replacing it. We don't need to cache it, as __get__ would # have done that for us if current_fw and current_fw != value: - _debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") + logger.debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") current_fw._removed = True elif current_fw == value: - _debug(f"We were given the same FileWrapper as before, removing any potential deletion marker") + logger.debug(f"We were given the same FileWrapper as before, removing any potential deletion marker") value._removed = False # Create a file instance if one isn't present if not value.file_instance: - _debug(f"Creating new file instance for FileWrapper {value.uuid}") + logger.debug(f"Creating new file instance for FileWrapper {value.uuid}") value.file_instance = self._create_file_instance() # Make sure our new file_instance knows of this wrapper :) value.file_instance.set_file_wrapper(value, self.field) @@ -310,7 +301,7 @@ def _set_from_file_wrapper(self, instance, value): # the only reason you don't receive a FW with a file_instance is if a # programmer created an empty (non field-attached) FileWrapper) elif value.field != self.field: - _warning(f"We were given a FileWrapper for a different field ({value.field.name}); this is programmer error. Fixing by creating a new FileWrapper for this field.") + logger.warning(f"We were given a FileWrapper for a different field ({value.field.name}); this is programmer error. Fixing by creating a new FileWrapper for this field.") file = value.file value = value.file_instance.get_file_wrapper(self.field, False) value.file = file @@ -329,7 +320,7 @@ def _set_from_file_like_object(self, instance, value): try: current_fw = self.__get__(instance) if current_fw is not None: - _debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") + logger.debug(f"Marking existing FileWrapper for potential deletion: {current_fw.uuid}") current_fw._removed = True except self.RelatedObjectDoesNotExist: pass @@ -368,7 +359,7 @@ def _set_from_none(self, instance): # 2) It's against normal Django behaviour to alter ANY data before # calling .save() (or .remove() for that matter). Thus, we need to # wait till the programmer expects things to change. - _debug(f"Marking existing FileWrapper for potential deletion: {obj.uuid}") + logger.debug(f"Marking existing FileWrapper for potential deletion: {obj.uuid}") obj._removed = True return obj diff --git a/src/cdh/files/db/fields.py b/src/cdh/files/db/fields.py index bbaf5c3c..a9c1a067 100644 --- a/src/cdh/files/db/fields.py +++ b/src/cdh/files/db/fields.py @@ -21,18 +21,11 @@ from .models import BaseFile, File from .wrappers import FileWrapper, TrackedFileWrapper -from cdh.files.logger import logger +import logging +logger = logging.getLogger(__name__) NOT_PROVIDED = object() -def _debug(message: str): - """Helper function to log debug messages with a consistent format""" - logger.debug(f"DB Field: {message}") - -def _info(message: str): - """Helper function to log debug messages with a consistent format""" - logger.info(f"DB Field: {message}") - class FileFieldCacheMixin: """Provide an API for working with the model's fields value cache. @@ -431,12 +424,12 @@ def pre_save(self, model_instance, add): """Make sure we save our file when saving the model this field is attached to""" ret = super().pre_save(model_instance, add) - _debug(f"pre_save called for FileField {self.full_name}") + logger.debug(f"pre_save called for FileField {self.full_name}") file = getattr(model_instance, self.name, None) # If we have a file and it's marked as not-saved (committed) if file is not None and not file._committed: - _debug(f"File not saved yet, committing it now: {file}") + logger.debug(f"File not saved yet, committing it now: {file}") # Commit the file to storage prior to saving the model # This will also save the File instance file.save() @@ -445,10 +438,10 @@ def pre_save(self, model_instance, add): def post_save(self, sender, instance, created, **kwargs): """Handle deletion of a file""" - _debug(f"post_save called for FileField {self.full_name}") + logger.debug(f"post_save called for FileField {self.full_name}") # If we have something in cache if self.is_cached(instance): - _debug(f"FileField {self.full_name} is cached, checking for any to-delete files") + logger.debug(f"FileField {self.full_name} is cached, checking for any to-delete files") # Get all values # The cache can contain multiple values, in which case all but # the last of them should be marked for deletion @@ -459,7 +452,7 @@ def post_save(self, sender, instance, created, **kwargs): # referenced by a different FileField, in which case # value.delete() will simple stop execution if value is not None and value._removed: - _info(f"File marked for deletion, starting delete for {value}") + logger.info(f"File marked for deletion, starting delete for {value}") # Terminate this file value.delete() # And clear our cached value @@ -474,19 +467,19 @@ def pre_delete(self, sender, instance, **kwargs): # we simply force our file into the cache if it exists so we can delete # it after the instance is deleted. getattr(instance, self.name, None) - _debug(f"pre_delete called for FileField {self.full_name}; prepopulating cache") + logger.debug(f"pre_delete called for FileField {self.full_name}; prepopulating cache") def post_delete(self, sender, instance, **kwargs): # When our model was deleted, we should see if our cache contains files # that need to be deleted - _debug(f"post_delete called for FileField {self.full_name}; checking for any to-delete files") + logger.debug(f"post_delete called for FileField {self.full_name}; checking for any to-delete files") if self.is_cached(instance): - _debug(f"FileField {self.name} is cached, continue checking for any to-delete files") + logger.debug(f"FileField {self.name} is cached, continue checking for any to-delete files") value = self.get_cached_value(instance, None) num_references = value.file_instance._num_child_instances # Delete the File if no other object is referencing it if value is not None and num_references == 0: - _info(f"FileField {self.full_name} has no more references, starting delete for {value}") + logger.info(f"FileField {self.full_name} has no more references, starting delete for {value}") value.delete() def contribute_to_class(self, *args, **kwargs): @@ -494,7 +487,7 @@ def contribute_to_class(self, *args, **kwargs): # Connect ourselves to some signals; # Fields really ought to have these methods themselves Django, # you already provide the pre_save method! - _debug(f"Connecting signals for FileField {self.full_name}") + logger.debug(f"Connecting signals for FileField {self.full_name}") post_save.connect(self.post_save, sender=self.model) pre_delete.connect(self.pre_delete, sender=self.model) post_delete.connect(self.post_delete, sender=self.model) diff --git a/src/cdh/files/db/wrappers.py b/src/cdh/files/db/wrappers.py index 8077733f..dd48f252 100644 --- a/src/cdh/files/db/wrappers.py +++ b/src/cdh/files/db/wrappers.py @@ -10,31 +10,15 @@ from .. import settings from ..mime_names import get_name_from_mime from ..utils import get_storage -from ..logger import logger + +import logging +logger = logging.getLogger(__name__) if TYPE_CHECKING: from . import TrackedFileField from .models import BaseFile -def _debug(message: str): - """Helper function to log debug messages with a consistent format""" - logger.debug(f"FileWrapper: {message}") - - -def _info(message: str): - """Helper function to log warning messages with a consistent format""" - logger.info(f"FileWrapper: {message}") - -def _warning(message: str): - """Helper function to log warning messages with a consistent format""" - logger.warning(f"FileWrapper: {message}") - -def _error(message: str): - """Helper function to log error messages with a consistent format""" - logger.error(f"FileWrapper: {message}") - - def _save_file(storage, name_on_disk, content, max_length): """Does the actual saving of the file to disk. This method is called by FileWrapper.save() and should not be called directly. @@ -46,9 +30,9 @@ def _save_file(storage, name_on_disk, content, max_length): # If we overwrite the file this instance represents, we need to first # delete the old one, as otherwise we would lose the new file if storage.exists(name_on_disk): - _info(f"Removing {name_on_disk} before saving new file") + logger.info(f"Removing {name_on_disk} before saving new file") storage.delete(name_on_disk) - _info(f"Saving {name_on_disk}") + logger.info(f"Saving {name_on_disk}") storage.save( name_on_disk, content, @@ -153,7 +137,7 @@ def _get_file(self): if getattr(self, '_file', None) is None: self._file = self.storage.open(self.name_on_disk, 'rb') except FileNotFoundError: - _debug(f"File {self.name_on_disk} not found on disk") + logger.debug(f"File {self.name_on_disk} not found on disk") self._file = None return self._file @@ -222,7 +206,7 @@ def save(self, content=None, original_filename=None): # I just liked saying 'use MAGIC' with self.open() as file: mime = magic.from_buffer(file.read(2048), mime=True) - _debug(f"Detected MIME type {mime} for file {self.name_on_disk}") + logger.debug(f"Detected MIME type {mime} for file {self.name_on_disk}") self.file_instance.content_type = mime if original_filename: @@ -251,23 +235,23 @@ def delete(self, save=True): :param save: Whether to also delete the metadata in the DB, defaults to True """ - _debug(f"Deleting FileWrapper {self.uuid} (save={save})") + logger.debug(f"Deleting FileWrapper {self.uuid} (save={save})") if not self.storage.exists(self.name_on_disk): - _warning(f"File {self.name_on_disk} does not exist on disk, skipping deletion") + logger.warning(f"File {self.name_on_disk} does not exist on disk, skipping deletion") return # Check if we only have the allowed amount number of references or fewer # If we have more, and we're not forcing a deletion, stop right here! if self.file_instance and self.file_instance._num_child_instances > 0: - _warning(f"FileWrapper {self.uuid} still has references, skipping deletion!") + logger.warning(f"FileWrapper {self.uuid} still has references, skipping deletion!") return # First, delete our metadata model. The check above _should_ make sure # we don't get integrity errors, but it's better to have this fail # because of those errors before we have actually deleted the file if save: - _debug(f"Deleting FileWrapper {self.uuid} from DB") + logger.debug(f"Deleting FileWrapper {self.uuid} from DB") self.file_instance.delete() # This is a sanity check; at this point we should have no references to @@ -278,17 +262,17 @@ def delete(self, save=True): if self.file_instance: model = self.file_instance._meta.model if model.objects.filter(Q(pk=self.file_instance.pk) | Q(uuid=self.uuid)).exists(): - _error(f"FileWrapper {self.uuid} still has references in DB, this should not happen!") + logger.error(f"FileWrapper {self.uuid} still has references in DB, this should not happen!") return # Only close the file if it's already open, which we know by the # presence of self._file if hasattr(self, '_file'): - _debug(f"Closing file handle for FileWrapper {self.uuid}") + logger.debug(f"Closing file handle for FileWrapper {self.uuid}") self.close() del self.file - _debug(f"Deleting file {self.name_on_disk} from disk") + logger.debug(f"Deleting file {self.name_on_disk} from disk") # Run this as part of a post-transaction hook to make sure we only # delete the file once the related database changes have been committed. transaction.on_commit(lambda: self.storage.delete(self.name_on_disk)) diff --git a/src/cdh/files/logger.py b/src/cdh/files/logger.py deleted file mode 100644 index 93bf5f08..00000000 --- a/src/cdh/files/logger.py +++ /dev/null @@ -1,4 +0,0 @@ -import logging - -logger = logging.getLogger('cdh.files') - diff --git a/src/cdh/files/signals.py b/src/cdh/files/signals.py index 28ee1f6b..b46e01d7 100644 --- a/src/cdh/files/signals.py +++ b/src/cdh/files/signals.py @@ -2,7 +2,10 @@ from django.db.models.signals import pre_delete, post_delete from cdh.files.db import BaseFile -from cdh.files.logger import logger + +import logging +logger = logging.getLogger(__name__) + def delete_file_on_delete(sender, instance, **kwargs): """Deletes the file on disk when the corresponding File is deleted""" From 2a10aaf393e1707fc085ef652d911c7526e37642 Mon Sep 17 00:00:00 2001 From: Michael Villeneuve Date: Mon, 23 Mar 2026 16:06:50 +0100 Subject: [PATCH 08/11] feat: Add default formatter and make logging settings more generic --- dev/dev_project/settings.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/dev/dev_project/settings.py b/dev/dev_project/settings.py index 211d419e..6e31f46d 100644 --- a/dev/dev_project/settings.py +++ b/dev/dev_project/settings.py @@ -180,6 +180,7 @@ # Logging # +DJANGO_LOG_LEVEL = os.getenv('DJANGO_LOG_LEVEL', 'INFO') LOGGING = { 'version': 1, @@ -187,23 +188,18 @@ 'handlers': { 'console': { 'class': 'logging.StreamHandler', + 'formatter': 'default', }, }, + 'formatters': { + 'default': { + 'format': '{levelname} {name} {message}', + 'style': '{', + } + }, 'root': { 'handlers': ['console'], - 'level': 'DEBUG', - }, - 'loggers': { - 'django': { - 'handlers': ['console'], - 'level': os.getenv('DJANGO_LOG_LEVEL', 'INFO'), - 'propagate': False, - }, - 'cdh.files': { - 'handlers': ['console'], - 'level': os.getenv('DJANGO_LOG_LEVEL', 'DEBUG'), - 'propagate': False, - }, + 'level': DJANGO_LOG_LEVEL, }, } From 3a2e3723ce949809c8039f5ed18cec09e67b13c2 Mon Sep 17 00:00:00 2001 From: Michael Villeneuve Date: Mon, 23 Mar 2026 16:19:02 +0100 Subject: [PATCH 09/11] feat: Simplify logging for cdh.mail --- src/cdh/mail/classes.py | 4 +++- src/cdh/mail/logger.py | 4 ---- 2 files changed, 3 insertions(+), 5 deletions(-) delete mode 100644 src/cdh/mail/logger.py diff --git a/src/cdh/mail/classes.py b/src/cdh/mail/classes.py index eaf6b96c..6239a662 100644 --- a/src/cdh/mail/classes.py +++ b/src/cdh/mail/classes.py @@ -16,9 +16,11 @@ from django.utils.functional import keep_lazy_text from django.utils.html import _strip_once -from .logger import logger from .settings import CDH_EMAIL_THEME_SETTINGS, CDH_EMAIL_PLAIN_FALLBACK_TEMPLATE, CDH_EMAIL_HTML_FALLBACK_TEMPLATE, CDH_EMAIL_FAIL_SILENTLY +import logging +logger = logging.getLogger(__name__) + @keep_lazy_text def _strip_tags(value) -> str: diff --git a/src/cdh/mail/logger.py b/src/cdh/mail/logger.py deleted file mode 100644 index d7d767f0..00000000 --- a/src/cdh/mail/logger.py +++ /dev/null @@ -1,4 +0,0 @@ -import logging - -logger = logging.getLogger('cdh.mail') - From bf3d351890b2768129dab0ad3c550e86b6bf8576 Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Tue, 21 Apr 2026 15:06:45 +0200 Subject: [PATCH 10/11] fix: make sure tests fire on_commit --- dev/dev_files/tests.py | 325 ++++++++++++++++++++++++++--------------- 1 file changed, 210 insertions(+), 115 deletions(-) diff --git a/dev/dev_files/tests.py b/dev/dev_files/tests.py index f42b3726..92e84aa4 100644 --- a/dev/dev_files/tests.py +++ b/dev/dev_files/tests.py @@ -1,5 +1,5 @@ from django.core.files import File -from django.test import TestCase +from django.test import TestCase, TransactionTestCase from django.urls import reverse from cdh.files import settings @@ -65,9 +65,10 @@ def test_save_delete_single(self): MetadataModel = self.single_cls.required_file.field.related_model storage = get_storage() - obj = self.single_cls() - obj.required_file = File(open(self.file_cat, mode='rb')) - obj.save() + with self.captureOnCommitCallbacks(execute=True): + obj = self.single_cls() + obj.required_file = File(open(self.file_cat, mode='rb')) + obj.save() self.assertTrue( storage.exists(obj.required_file.name_on_disk) @@ -77,7 +78,8 @@ def test_save_delete_single(self): 1 ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() self.assertFalse( storage.exists(obj.required_file.name_on_disk) @@ -92,13 +94,16 @@ def test_save_delete_single(self): "Storage is not empty" ) + get_storage().clear() + def test_single_multiple_use(self): MetadataModel = self.single_cls.required_file.field.related_model storage = get_storage() - obj_1 = self.single_cls() - obj_1.required_file = File(open(self.file_cat, mode='rb')) - obj_1.save() + with self.captureOnCommitCallbacks(execute=True): + obj_1 = self.single_cls() + obj_1.required_file = File(open(self.file_cat, mode='rb')) + obj_1.save() self.assertTrue( storage.exists(obj_1.required_file.name_on_disk) @@ -108,9 +113,10 @@ def test_single_multiple_use(self): 1 ) - obj_2 = self.single_cls() - obj_2.required_file = obj_1.required_file - obj_2.save() + with self.captureOnCommitCallbacks(execute=True): + obj_2 = self.single_cls() + obj_2.required_file = obj_1.required_file + obj_2.save() self.assertTrue( storage.exists(obj_1.required_file.name_on_disk) @@ -134,7 +140,8 @@ def test_single_multiple_use(self): "Second object was not created" ) - obj_1.delete() + with self.captureOnCommitCallbacks(execute=True): + obj_1.delete() self.assertTrue( storage.exists(obj_2.required_file.name_on_disk), @@ -152,7 +159,8 @@ def test_single_multiple_use(self): "Somehow the object referencing the File was not deleted?" ) - obj_2.delete() + with self.captureOnCommitCallbacks(execute=True): + obj_2.delete() self.assertFalse( storage.exists(obj_2.required_file.name_on_disk), @@ -175,13 +183,16 @@ def test_single_multiple_use(self): "Storage is not empty" ) + get_storage().clear() + def test_tracked_save_delete(self): MetadataModel = self.tracked_cls.files.field.related_model storage = get_storage() - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) + with self.captureOnCommitCallbacks(execute=True): + obj = self.tracked_cls() + obj.save() + obj.files.add(File(open(self.file_cat, mode='rb'))) self.assertTrue( storage.exists(obj.files.current_file.name_on_disk) @@ -191,7 +202,8 @@ def test_tracked_save_delete(self): 1 ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() self.assertFalse( storage.exists(obj.files.current_file.name_on_disk) @@ -206,16 +218,19 @@ def test_tracked_save_delete(self): "Storage is not empty" ) + get_storage().clear() + def test_tracked_multiple_files(self): MetadataModel = self.tracked_cls.files.field.related_model storage = get_storage() - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - cat_uuid = obj.files.current_file.uuid - obj.files.add(File(open(self.file_dog, mode='rb'))) - dog_uuid = obj.files.current_file.uuid + with self.captureOnCommitCallbacks(execute=True): + obj = self.tracked_cls() + obj.save() + obj.files.add(File(open(self.file_cat, mode='rb'))) + cat_uuid = obj.files.current_file.uuid + obj.files.add(File(open(self.file_dog, mode='rb'))) + dog_uuid = obj.files.current_file.uuid self.assertNotEqual(cat_uuid, dog_uuid) @@ -235,7 +250,8 @@ def test_tracked_multiple_files(self): 2 ) - obj.files.delete(obj.files.current_file) + with self.captureOnCommitCallbacks(execute=True): + obj.files.delete(obj.files.current_file) # Current file should now be none self.assertIsNone( @@ -262,7 +278,9 @@ def test_tracked_multiple_files(self): ) # Try to set the old cat file as current - obj.files.set_as_current(cat_uuid) + with self.captureOnCommitCallbacks(execute=True): + obj.files.set_as_current(cat_uuid) + self.assertIsNotNone( obj.files.current_file ) @@ -272,7 +290,8 @@ def test_tracked_multiple_files(self): self.file_cat ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() self.assertFalse( storage.exists(obj.files.current_file.name_on_disk) @@ -290,17 +309,21 @@ def test_tracked_multiple_files(self): storage.num_files(), "Storage is not empty" ) + get_storage().clear() def test_tracked_delete_all(self): MetadataModel = self.tracked_cls.files.field.related_model storage = get_storage() - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - cat_uuid = obj.files.current_file.uuid - obj.files.add(File(open(self.file_dog, mode='rb'))) - dog_uuid = obj.files.current_file.uuid + with self.captureOnCommitCallbacks(execute=True): + obj = self.tracked_cls() + obj.save() + obj.files.add(File(open(self.file_cat, mode='rb'))) + cat_uuid = obj.files.current_file.uuid + + with self.captureOnCommitCallbacks(execute=True): + obj.files.add(File(open(self.file_dog, mode='rb'))) + dog_uuid = obj.files.current_file.uuid self.assertTrue( storage.exists(cat_uuid) @@ -321,7 +344,8 @@ def test_tracked_delete_all(self): 2 ) - obj.files.delete_all() + with self.captureOnCommitCallbacks(execute=True): + obj.files.delete_all() self.assertFalse( storage.exists(cat_uuid) @@ -341,19 +365,24 @@ def test_tracked_delete_all(self): storage.num_files(), 0 ) + get_storage().clear() def test_tracked_auto_delete(self): MetadataModel = self.tracked_cls.files.field.related_model storage = get_storage() - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - cat_uuid = obj.files.current_file.uuid - obj.files.add(File(open(self.file_dog, mode='rb'))) - dog_uuid = obj.files.current_file.uuid + with self.captureOnCommitCallbacks(execute=True): + obj = self.tracked_cls() + obj.save() + obj.files.add(File(open(self.file_cat, mode='rb'))) + cat_uuid = obj.files.current_file.uuid - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.files.add(File(open(self.file_dog, mode='rb'))) + dog_uuid = obj.files.current_file.uuid + + with self.captureOnCommitCallbacks(execute=True): + obj.delete() self.assertFalse( storage.exists(cat_uuid) @@ -373,17 +402,21 @@ def test_tracked_auto_delete(self): storage.num_files(), 0 ) + get_storage().clear() def test_tracked_set_current(self): storage = get_storage() - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - cat_uuid = obj.files.current_file.uuid - cat_wrapper = obj.files.current_file - obj.files.add(File(open(self.file_dog, mode='rb'))) - dog_uuid = obj.files.current_file.uuid + with self.captureOnCommitCallbacks(execute=True): + obj = self.tracked_cls() + obj.save() + obj.files.add(File(open(self.file_cat, mode='rb'))) + cat_uuid = obj.files.current_file.uuid + cat_wrapper = obj.files.current_file + + with self.captureOnCommitCallbacks(execute=True): + obj.files.add(File(open(self.file_dog, mode='rb'))) + dog_uuid = obj.files.current_file.uuid self.assertEqual( self.file_dog, @@ -398,7 +431,8 @@ def test_tracked_set_current(self): storage.num_files(), ) - obj.files.set_as_current(cat_wrapper) + with self.captureOnCommitCallbacks(execute=True): + obj.files.set_as_current(cat_wrapper) self.assertEqual( self.file_cat, @@ -417,7 +451,10 @@ def test_tracked_set_current(self): 2 ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() + + get_storage().clear() def test_single_set_from_django_file(self): storage = get_storage() @@ -425,9 +462,10 @@ def test_single_set_from_django_file(self): MetadataModel = self.single_cls.required_file.field.related_model data = File(open(self.file_cat, mode='rb')) - obj = self.single_cls() - obj.required_file = data - obj.save() + with self.captureOnCommitCallbacks(execute=True): + obj = self.single_cls() + obj.required_file = data + obj.save() self.assertTrue( storage.exists(obj.required_file.name_on_disk) @@ -441,7 +479,10 @@ def test_single_set_from_django_file(self): obj.required_file.name ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() + + get_storage().clear() def test_single_set_from_tuple(self): storage = get_storage() @@ -449,9 +490,10 @@ def test_single_set_from_tuple(self): MetadataModel = self.single_cls.required_file.field.related_model data = (File(open(self.file_cat, mode='rb')), None, True) - obj = self.single_cls() - obj.required_file = data - obj.save() + with self.captureOnCommitCallbacks(execute=True): + obj = self.single_cls() + obj.required_file = data + obj.save() self.assertTrue( storage.exists(obj.required_file.name_on_disk) @@ -465,20 +507,25 @@ def test_single_set_from_tuple(self): obj.required_file.name ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() + + get_storage().clear() def test_single_set_from_metadata_model(self): storage = get_storage() MetadataModel = self.single_cls.required_file.field.related_model - obj_1 = self.single_cls() - obj_1.required_file = File(open(self.file_cat, mode='rb')) - obj_1.save() + with self.captureOnCommitCallbacks(execute=True): + obj_1 = self.single_cls() + obj_1.required_file = File(open(self.file_cat, mode='rb')) + obj_1.save() - obj_2 = self.single_cls() - obj_2.required_file = obj_1.required_file.file_instance - obj_2.save() + with self.captureOnCommitCallbacks(execute=True): + obj_2 = self.single_cls() + obj_2.required_file = obj_1.required_file.file_instance + obj_2.save() self.assertTrue( storage.exists(obj_2.required_file.name_on_disk) @@ -500,8 +547,13 @@ def test_single_set_from_metadata_model(self): obj_2.required_file.name ) - obj_1.delete() - obj_2.delete() + with self.captureOnCommitCallbacks(execute=True): + obj_1.delete() + + with self.captureOnCommitCallbacks(execute=True): + obj_2.delete() + + get_storage().clear() def test_single_set_from_file_wrapper(self): storage = get_storage() @@ -509,13 +561,15 @@ def test_single_set_from_file_wrapper(self): MetadataModel = self.single_cls.required_file.field.related_model data = (File(open(self.file_cat, mode='rb')), None, True) - obj_1 = self.single_cls() - obj_1.required_file = data - obj_1.save() + with self.captureOnCommitCallbacks(execute=True): + obj_1 = self.single_cls() + obj_1.required_file = data + obj_1.save() - obj_2 = self.single_cls() - obj_2.required_file = obj_1.required_file - obj_2.save() + with self.captureOnCommitCallbacks(execute=True): + obj_2 = self.single_cls() + obj_2.required_file = obj_1.required_file + obj_2.save() self.assertTrue( storage.exists(obj_2.required_file.name_on_disk) @@ -537,18 +591,23 @@ def test_single_set_from_file_wrapper(self): obj_2.required_file.name ) - obj_1.delete() - obj_2.delete() + with self.captureOnCommitCallbacks(execute=True): + obj_1.delete() + with self.captureOnCommitCallbacks(execute=True): + obj_2.delete() + + get_storage().clear() def test_single_set_from_None(self): storage = get_storage() MetadataModel = self.single_cls.required_file.field.related_model - obj = self.single_cls() - obj.required_file = File(open(self.file_cat, mode='rb')) - obj.nullable_file = File(open(self.file_dog, mode='rb')) - obj.save() + with self.captureOnCommitCallbacks(execute=True): + obj = self.single_cls() + obj.required_file = File(open(self.file_cat, mode='rb')) + obj.nullable_file = File(open(self.file_dog, mode='rb')) + obj.save() nullable_uuid = obj.nullable_file.name_on_disk self.assertTrue( @@ -567,8 +626,9 @@ def test_single_set_from_None(self): obj.nullable_file.name ) - obj.nullable_file = None - obj.save() + with self.captureOnCommitCallbacks(execute=True): + obj.nullable_file = None + obj.save() self.assertFalse( storage.exists(nullable_uuid) @@ -585,20 +645,25 @@ def test_single_set_from_None(self): obj.nullable_file ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() + + get_storage().clear() def test_tracked_set_from_metadata_model(self): storage = get_storage() MetadataModel = self.single_cls.required_file.field.related_model - obj_1 = self.single_cls() - obj_1.required_file = File(open(self.file_cat, mode='rb')) - obj_1.save() + with self.captureOnCommitCallbacks(execute=True): + obj_1 = self.single_cls() + obj_1.required_file = File(open(self.file_cat, mode='rb')) + obj_1.save() - obj_2 = self.tracked_cls() - obj_2.save() - obj_2.files.current_file = obj_1.required_file.file_instance + with self.captureOnCommitCallbacks(execute=True): + obj_2 = self.tracked_cls() + obj_2.save() + obj_2.files.current_file = obj_1.required_file.file_instance self.assertTrue( storage.exists(obj_2.files.current_file.name_on_disk) @@ -620,8 +685,13 @@ def test_tracked_set_from_metadata_model(self): obj_2.files.current_file.name ) - obj_1.delete() - obj_2.delete() + with self.captureOnCommitCallbacks(execute=True): + obj_1.delete() + + with self.captureOnCommitCallbacks(execute=True): + obj_2.delete() + + get_storage().clear() def test_tracked_set_from_file_wrapper(self): storage = get_storage() @@ -629,13 +699,15 @@ def test_tracked_set_from_file_wrapper(self): MetadataModel = self.single_cls.required_file.field.related_model data = (File(open(self.file_cat, mode='rb')), None, True) - obj_1 = self.single_cls() - obj_1.required_file = data - obj_1.save() + with self.captureOnCommitCallbacks(execute=True): + obj_1 = self.single_cls() + obj_1.required_file = data + obj_1.save() - obj_2 = self.tracked_cls() - obj_2.save() - obj_2.files.current_file = obj_1.required_file + with self.captureOnCommitCallbacks(execute=True): + obj_2 = self.tracked_cls() + obj_2.save() + obj_2.files.current_file = obj_1.required_file self.assertTrue( storage.exists(obj_2.files.current_file.name_on_disk) @@ -657,17 +729,22 @@ def test_tracked_set_from_file_wrapper(self): obj_2.files.current_file.name ) - obj_1.delete() - obj_2.delete() + with self.captureOnCommitCallbacks(execute=True): + obj_1.delete() + with self.captureOnCommitCallbacks(execute=True): + obj_2.delete() + + get_storage().clear() def test_tracked_set_from_None(self): storage = get_storage() MetadataModel = self.single_cls.required_file.field.related_model - obj = self.tracked_cls() - obj.save() - obj.files.current_file = File(open(self.file_cat, mode='rb')) + with self.captureOnCommitCallbacks(execute=True): + obj = self.tracked_cls() + obj.save() + obj.files.current_file = File(open(self.file_cat, mode='rb')) self.assertTrue( storage.exists(obj.files.current_file.name_on_disk) @@ -685,7 +762,8 @@ def test_tracked_set_from_None(self): obj.files.current_file.name ) - obj.files.current_file = None + with self.captureOnCommitCallbacks(execute=True): + obj.files.current_file = None self.assertIsNone( obj.files.current_file @@ -703,7 +781,10 @@ def test_tracked_set_from_None(self): 0 ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() + + get_storage().clear() def test_filename_generator(self): static_name = "I love cats" @@ -713,9 +794,10 @@ def _static_generator(file_wrapper): self.single_cls._meta.get_field('required_file').filename_generator = \ _static_generator - obj = self.single_cls() - obj.required_file = File(open(self.file_cat, mode='rb')) - obj.save() + with self.captureOnCommitCallbacks(execute=True): + obj = self.single_cls() + obj.required_file = File(open(self.file_cat, mode='rb')) + obj.save() self.assertEqual( static_name, @@ -733,36 +815,46 @@ def _dynamic_generator(file_wrapper): obj.required_file.name ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() self.single_cls._meta.get_field('required_file').filename_generator = \ _default_filename_generator + get_storage().clear() + def test_single_url_generation(self): - obj = self.single_cls() - obj.required_file = File(open(self.file_cat, mode='rb')) - obj.save() + with self.captureOnCommitCallbacks(execute=True): + obj = self.single_cls() + obj.required_file = File(open(self.file_cat, mode='rb')) + obj.save() self.assertEqual( reverse(self.url_pattern_single, args=[obj.required_file.uuid]), obj.required_file.url, ) - obj._meta.get_field('required_file').url_pattern = None + with self.captureOnCommitCallbacks(execute=True): + obj._meta.get_field('required_file').url_pattern = None self.assertEqual( None, obj.required_file.url, ) - obj._meta.get_field('required_file').url_pattern = self.url_pattern_single + with self.captureOnCommitCallbacks(execute=True): + obj._meta.get_field('required_file').url_pattern = self.url_pattern_single - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() + + get_storage().clear() def test_tracked_url_generation(self): - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) + with self.captureOnCommitCallbacks(execute=True): + obj = self.tracked_cls() + obj.save() + obj.files.add(File(open(self.file_cat, mode='rb'))) self.assertEqual( reverse(self.url_pattern_tracked, args=[ @@ -771,13 +863,16 @@ def test_tracked_url_generation(self): obj.files.current_file.url, ) - obj.delete() + with self.captureOnCommitCallbacks(execute=True): + obj.delete() # We do not test the None case, as changing it at runtime isn't actually # supported. The single case works (although also not supported), so we # can actually test it there. If it works there, it should work here. # This test case should only test if setting url_pattern will actually # propagate it to the linking model. + get_storage().clear() + class CustomFileTests(FileTests): single_cls = CustomSingleFile From 77fbc679d3fe441cbf8dadbae3bb4a5a08e3bb15 Mon Sep 17 00:00:00 2001 From: "Mees, T.D. (Ty)" Date: Tue, 21 Apr 2026 15:09:48 +0200 Subject: [PATCH 11/11] misc: deprecate tracked file field and remove its tests --- dev/dev_files/tests.py | 854 ++++++++++++++++++------------------- src/cdh/files/db/fields.py | 2 + 2 files changed, 429 insertions(+), 427 deletions(-) diff --git a/dev/dev_files/tests.py b/dev/dev_files/tests.py index 92e84aa4..253e4392 100644 --- a/dev/dev_files/tests.py +++ b/dev/dev_files/tests.py @@ -185,276 +185,276 @@ def test_single_multiple_use(self): get_storage().clear() - def test_tracked_save_delete(self): - MetadataModel = self.tracked_cls.files.field.related_model - storage = get_storage() - - with self.captureOnCommitCallbacks(execute=True): - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - - self.assertTrue( - storage.exists(obj.files.current_file.name_on_disk) - ) - self.assertEqual( - MetadataModel.objects.count(), - 1 - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.delete() - - self.assertFalse( - storage.exists(obj.files.current_file.name_on_disk) - ) - self.assertEqual( - MetadataModel.objects.count(), - 0 - ) - self.assertEqual( - 0, - storage.num_files(), - "Storage is not empty" - ) - - get_storage().clear() - - def test_tracked_multiple_files(self): - MetadataModel = self.tracked_cls.files.field.related_model - storage = get_storage() - - with self.captureOnCommitCallbacks(execute=True): - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - cat_uuid = obj.files.current_file.uuid - obj.files.add(File(open(self.file_dog, mode='rb'))) - dog_uuid = obj.files.current_file.uuid - - self.assertNotEqual(cat_uuid, dog_uuid) - - self.assertTrue( - storage.exists(obj.files.current_file.name_on_disk) - ) - self.assertEqual( - obj.files.current_file.name, - self.file_dog - ) - self.assertEqual( - MetadataModel.objects.count(), - 2 - ) - self.assertEqual( - len(list(obj.files.all)), - 2 - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.files.delete(obj.files.current_file) - - # Current file should now be none - self.assertIsNone( - obj.files.current_file - ) - - self.assertFalse( - storage.exists(dog_uuid) - ) - self.assertTrue( - storage.exists(cat_uuid) - ) - self.assertEqual( - MetadataModel.objects.count(), - 1 - ) - self.assertEqual( - storage.num_files(), - 1 - ) - self.assertEqual( - len(list(obj.files.all)), - 1 - ) - - # Try to set the old cat file as current - with self.captureOnCommitCallbacks(execute=True): - obj.files.set_as_current(cat_uuid) - - self.assertIsNotNone( - obj.files.current_file - ) - - self.assertEqual( - obj.files.current_file.name, - self.file_cat - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.delete() - - self.assertFalse( - storage.exists(obj.files.current_file.name_on_disk) - ) - self.assertEqual( - MetadataModel.objects.count(), - 0 - ) - self.assertEqual( - self.tracked_cls.objects.count(), - 0 - ) - self.assertEqual( - 0, - storage.num_files(), - "Storage is not empty" - ) - get_storage().clear() - - def test_tracked_delete_all(self): - MetadataModel = self.tracked_cls.files.field.related_model - storage = get_storage() - - with self.captureOnCommitCallbacks(execute=True): - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - cat_uuid = obj.files.current_file.uuid - - with self.captureOnCommitCallbacks(execute=True): - obj.files.add(File(open(self.file_dog, mode='rb'))) - dog_uuid = obj.files.current_file.uuid - - self.assertTrue( - storage.exists(cat_uuid) - ) - self.assertTrue( - storage.exists(dog_uuid) - ) - self.assertEqual( - MetadataModel.objects.count(), - 2 - ) - self.assertEqual( - self.tracked_cls.objects.count(), - 1 - ) - self.assertEqual( - storage.num_files(), - 2 - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.files.delete_all() - - self.assertFalse( - storage.exists(cat_uuid) - ) - self.assertFalse( - storage.exists(dog_uuid) - ) - self.assertEqual( - MetadataModel.objects.count(), - 0 - ) - self.assertEqual( - self.tracked_cls.objects.count(), - 1 - ) - self.assertEqual( - storage.num_files(), - 0 - ) - get_storage().clear() - - def test_tracked_auto_delete(self): - MetadataModel = self.tracked_cls.files.field.related_model - storage = get_storage() - - with self.captureOnCommitCallbacks(execute=True): - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - cat_uuid = obj.files.current_file.uuid - - with self.captureOnCommitCallbacks(execute=True): - obj.files.add(File(open(self.file_dog, mode='rb'))) - dog_uuid = obj.files.current_file.uuid - - with self.captureOnCommitCallbacks(execute=True): - obj.delete() - - self.assertFalse( - storage.exists(cat_uuid) - ) - self.assertFalse( - storage.exists(dog_uuid) - ) - self.assertEqual( - MetadataModel.objects.count(), - 0 - ) - self.assertEqual( - self.tracked_cls.objects.count(), - 0 - ) - self.assertEqual( - storage.num_files(), - 0 - ) - get_storage().clear() - - def test_tracked_set_current(self): - storage = get_storage() - - with self.captureOnCommitCallbacks(execute=True): - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - cat_uuid = obj.files.current_file.uuid - cat_wrapper = obj.files.current_file - - with self.captureOnCommitCallbacks(execute=True): - obj.files.add(File(open(self.file_dog, mode='rb'))) - dog_uuid = obj.files.current_file.uuid - - self.assertEqual( - self.file_dog, - obj.files.current_file.name, - ) - self.assertEqual( - dog_uuid, - obj.files.current_file.uuid, - ) - self.assertEqual( - 2, - storage.num_files(), - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.files.set_as_current(cat_wrapper) - - self.assertEqual( - self.file_cat, - obj.files.current_file.name, - ) - self.assertEqual( - cat_uuid, - obj.files.current_file.uuid, - ) - self.assertEqual( - 2, - storage.num_files(), - ) - self.assertEqual( - len(list(obj.files.all)), - 2 - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.delete() - - get_storage().clear() + # def test_tracked_save_delete(self): + # MetadataModel = self.tracked_cls.files.field.related_model + # storage = get_storage() + # + # with self.captureOnCommitCallbacks(execute=True): + # obj = self.tracked_cls() + # obj.save() + # obj.files.add(File(open(self.file_cat, mode='rb'))) + # + # self.assertTrue( + # storage.exists(obj.files.current_file.name_on_disk) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 1 + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.delete() + # + # self.assertFalse( + # storage.exists(obj.files.current_file.name_on_disk) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 0 + # ) + # self.assertEqual( + # 0, + # storage.num_files(), + # "Storage is not empty" + # ) + # + # get_storage().clear() + + # def test_tracked_multiple_files(self): + # MetadataModel = self.tracked_cls.files.field.related_model + # storage = get_storage() + # + # with self.captureOnCommitCallbacks(execute=True): + # obj = self.tracked_cls() + # obj.save() + # obj.files.add(File(open(self.file_cat, mode='rb'))) + # cat_uuid = obj.files.current_file.uuid + # obj.files.add(File(open(self.file_dog, mode='rb'))) + # dog_uuid = obj.files.current_file.uuid + # + # self.assertNotEqual(cat_uuid, dog_uuid) + # + # self.assertTrue( + # storage.exists(obj.files.current_file.name_on_disk) + # ) + # self.assertEqual( + # obj.files.current_file.name, + # self.file_dog + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 2 + # ) + # self.assertEqual( + # len(list(obj.files.all)), + # 2 + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.files.delete(obj.files.current_file) + # + # # Current file should now be none + # self.assertIsNone( + # obj.files.current_file + # ) + # + # self.assertFalse( + # storage.exists(dog_uuid) + # ) + # self.assertTrue( + # storage.exists(cat_uuid) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 1 + # ) + # self.assertEqual( + # storage.num_files(), + # 1 + # ) + # self.assertEqual( + # len(list(obj.files.all)), + # 1 + # ) + # + # # Try to set the old cat file as current + # with self.captureOnCommitCallbacks(execute=True): + # obj.files.set_as_current(cat_uuid) + # + # self.assertIsNotNone( + # obj.files.current_file + # ) + # + # self.assertEqual( + # obj.files.current_file.name, + # self.file_cat + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.delete() + # + # self.assertFalse( + # storage.exists(obj.files.current_file.name_on_disk) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 0 + # ) + # self.assertEqual( + # self.tracked_cls.objects.count(), + # 0 + # ) + # self.assertEqual( + # 0, + # storage.num_files(), + # "Storage is not empty" + # ) + # get_storage().clear() + + # def test_tracked_delete_all(self): + # MetadataModel = self.tracked_cls.files.field.related_model + # storage = get_storage() + # + # with self.captureOnCommitCallbacks(execute=True): + # obj = self.tracked_cls() + # obj.save() + # obj.files.add(File(open(self.file_cat, mode='rb'))) + # cat_uuid = obj.files.current_file.uuid + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.files.add(File(open(self.file_dog, mode='rb'))) + # dog_uuid = obj.files.current_file.uuid + # + # self.assertTrue( + # storage.exists(cat_uuid) + # ) + # self.assertTrue( + # storage.exists(dog_uuid) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 2 + # ) + # self.assertEqual( + # self.tracked_cls.objects.count(), + # 1 + # ) + # self.assertEqual( + # storage.num_files(), + # 2 + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.files.delete_all() + # + # self.assertFalse( + # storage.exists(cat_uuid) + # ) + # self.assertFalse( + # storage.exists(dog_uuid) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 0 + # ) + # self.assertEqual( + # self.tracked_cls.objects.count(), + # 1 + # ) + # self.assertEqual( + # storage.num_files(), + # 0 + # ) + # get_storage().clear() + + # def test_tracked_auto_delete(self): + # MetadataModel = self.tracked_cls.files.field.related_model + # storage = get_storage() + # + # with self.captureOnCommitCallbacks(execute=True): + # obj = self.tracked_cls() + # obj.save() + # obj.files.add(File(open(self.file_cat, mode='rb'))) + # cat_uuid = obj.files.current_file.uuid + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.files.add(File(open(self.file_dog, mode='rb'))) + # dog_uuid = obj.files.current_file.uuid + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.delete() + # + # self.assertFalse( + # storage.exists(cat_uuid) + # ) + # self.assertFalse( + # storage.exists(dog_uuid) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 0 + # ) + # self.assertEqual( + # self.tracked_cls.objects.count(), + # 0 + # ) + # self.assertEqual( + # storage.num_files(), + # 0 + # ) + # get_storage().clear() + + # def test_tracked_set_current(self): + # storage = get_storage() + # + # with self.captureOnCommitCallbacks(execute=True): + # obj = self.tracked_cls() + # obj.save() + # obj.files.add(File(open(self.file_cat, mode='rb'))) + # cat_uuid = obj.files.current_file.uuid + # cat_wrapper = obj.files.current_file + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.files.add(File(open(self.file_dog, mode='rb'))) + # dog_uuid = obj.files.current_file.uuid + # + # self.assertEqual( + # self.file_dog, + # obj.files.current_file.name, + # ) + # self.assertEqual( + # dog_uuid, + # obj.files.current_file.uuid, + # ) + # self.assertEqual( + # 2, + # storage.num_files(), + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.files.set_as_current(cat_wrapper) + # + # self.assertEqual( + # self.file_cat, + # obj.files.current_file.name, + # ) + # self.assertEqual( + # cat_uuid, + # obj.files.current_file.uuid, + # ) + # self.assertEqual( + # 2, + # storage.num_files(), + # ) + # self.assertEqual( + # len(list(obj.files.all)), + # 2 + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.delete() + # + # get_storage().clear() def test_single_set_from_django_file(self): storage = get_storage() @@ -650,141 +650,141 @@ def test_single_set_from_None(self): get_storage().clear() - def test_tracked_set_from_metadata_model(self): - storage = get_storage() - - MetadataModel = self.single_cls.required_file.field.related_model - - with self.captureOnCommitCallbacks(execute=True): - obj_1 = self.single_cls() - obj_1.required_file = File(open(self.file_cat, mode='rb')) - obj_1.save() - - with self.captureOnCommitCallbacks(execute=True): - obj_2 = self.tracked_cls() - obj_2.save() - obj_2.files.current_file = obj_1.required_file.file_instance - - self.assertTrue( - storage.exists(obj_2.files.current_file.name_on_disk) - ) - self.assertEqual( - MetadataModel.objects.count(), - 1 - ) - self.assertEqual( - storage.num_files(), - 1 - ) - self.assertEqual( - obj_1.required_file.file_instance, - obj_2.files.current_file.file_instance, - ) - self.assertEqual( - self.file_cat, - obj_2.files.current_file.name - ) - - with self.captureOnCommitCallbacks(execute=True): - obj_1.delete() - - with self.captureOnCommitCallbacks(execute=True): - obj_2.delete() - - get_storage().clear() - - def test_tracked_set_from_file_wrapper(self): - storage = get_storage() - - MetadataModel = self.single_cls.required_file.field.related_model - data = (File(open(self.file_cat, mode='rb')), None, True) - - with self.captureOnCommitCallbacks(execute=True): - obj_1 = self.single_cls() - obj_1.required_file = data - obj_1.save() - - with self.captureOnCommitCallbacks(execute=True): - obj_2 = self.tracked_cls() - obj_2.save() - obj_2.files.current_file = obj_1.required_file - - self.assertTrue( - storage.exists(obj_2.files.current_file.name_on_disk) - ) - self.assertEqual( - MetadataModel.objects.count(), - 1 - ) - self.assertEqual( - storage.num_files(), - 1 - ) - self.assertEqual( - obj_1.required_file.file_instance, - obj_2.files.current_file.file_instance, - ) - self.assertEqual( - self.file_cat, - obj_2.files.current_file.name - ) - - with self.captureOnCommitCallbacks(execute=True): - obj_1.delete() - with self.captureOnCommitCallbacks(execute=True): - obj_2.delete() - - get_storage().clear() - - def test_tracked_set_from_None(self): - storage = get_storage() - - MetadataModel = self.single_cls.required_file.field.related_model - - with self.captureOnCommitCallbacks(execute=True): - obj = self.tracked_cls() - obj.save() - obj.files.current_file = File(open(self.file_cat, mode='rb')) - - self.assertTrue( - storage.exists(obj.files.current_file.name_on_disk) - ) - self.assertEqual( - MetadataModel.objects.count(), - 1 - ) - self.assertEqual( - storage.num_files(), - 1 - ) - self.assertEqual( - self.file_cat, - obj.files.current_file.name - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.files.current_file = None - - self.assertIsNone( - obj.files.current_file - ) - self.assertEqual( - MetadataModel.objects.count(), - 0 - ) - self.assertEqual( - storage.num_files(), - 0 - ) - self.assertEqual( - len(list(obj.files.all)), - 0 - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.delete() - - get_storage().clear() + # def test_tracked_set_from_metadata_model(self): + # storage = get_storage() + # + # MetadataModel = self.single_cls.required_file.field.related_model + # + # with self.captureOnCommitCallbacks(execute=True): + # obj_1 = self.single_cls() + # obj_1.required_file = File(open(self.file_cat, mode='rb')) + # obj_1.save() + # + # with self.captureOnCommitCallbacks(execute=True): + # obj_2 = self.tracked_cls() + # obj_2.save() + # obj_2.files.current_file = obj_1.required_file.file_instance + # + # self.assertTrue( + # storage.exists(obj_2.files.current_file.name_on_disk) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 1 + # ) + # self.assertEqual( + # storage.num_files(), + # 1 + # ) + # self.assertEqual( + # obj_1.required_file.file_instance, + # obj_2.files.current_file.file_instance, + # ) + # self.assertEqual( + # self.file_cat, + # obj_2.files.current_file.name + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj_1.delete() + # + # with self.captureOnCommitCallbacks(execute=True): + # obj_2.delete() + # + # get_storage().clear() + + # def test_tracked_set_from_file_wrapper(self): + # storage = get_storage() + # + # MetadataModel = self.single_cls.required_file.field.related_model + # data = (File(open(self.file_cat, mode='rb')), None, True) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj_1 = self.single_cls() + # obj_1.required_file = data + # obj_1.save() + # + # with self.captureOnCommitCallbacks(execute=True): + # obj_2 = self.tracked_cls() + # obj_2.save() + # obj_2.files.current_file = obj_1.required_file + # + # self.assertTrue( + # storage.exists(obj_2.files.current_file.name_on_disk) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 1 + # ) + # self.assertEqual( + # storage.num_files(), + # 1 + # ) + # self.assertEqual( + # obj_1.required_file.file_instance, + # obj_2.files.current_file.file_instance, + # ) + # self.assertEqual( + # self.file_cat, + # obj_2.files.current_file.name + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj_1.delete() + # with self.captureOnCommitCallbacks(execute=True): + # obj_2.delete() + # + # get_storage().clear() + + # def test_tracked_set_from_None(self): + # storage = get_storage() + # + # MetadataModel = self.single_cls.required_file.field.related_model + # + # with self.captureOnCommitCallbacks(execute=True): + # obj = self.tracked_cls() + # obj.save() + # obj.files.current_file = File(open(self.file_cat, mode='rb')) + # + # self.assertTrue( + # storage.exists(obj.files.current_file.name_on_disk) + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 1 + # ) + # self.assertEqual( + # storage.num_files(), + # 1 + # ) + # self.assertEqual( + # self.file_cat, + # obj.files.current_file.name + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.files.current_file = None + # + # self.assertIsNone( + # obj.files.current_file + # ) + # self.assertEqual( + # MetadataModel.objects.count(), + # 0 + # ) + # self.assertEqual( + # storage.num_files(), + # 0 + # ) + # self.assertEqual( + # len(list(obj.files.all)), + # 0 + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.delete() + # + # get_storage().clear() def test_filename_generator(self): static_name = "I love cats" @@ -850,28 +850,28 @@ def test_single_url_generation(self): get_storage().clear() - def test_tracked_url_generation(self): - with self.captureOnCommitCallbacks(execute=True): - obj = self.tracked_cls() - obj.save() - obj.files.add(File(open(self.file_cat, mode='rb'))) - - self.assertEqual( - reverse(self.url_pattern_tracked, args=[ - obj.files.current_file.uuid - ]), - obj.files.current_file.url, - ) - - with self.captureOnCommitCallbacks(execute=True): - obj.delete() - # We do not test the None case, as changing it at runtime isn't actually - # supported. The single case works (although also not supported), so we - # can actually test it there. If it works there, it should work here. - # This test case should only test if setting url_pattern will actually - # propagate it to the linking model. - - get_storage().clear() + # def test_tracked_url_generation(self): + # with self.captureOnCommitCallbacks(execute=True): + # obj = self.tracked_cls() + # obj.save() + # obj.files.add(File(open(self.file_cat, mode='rb'))) + # + # self.assertEqual( + # reverse(self.url_pattern_tracked, args=[ + # obj.files.current_file.uuid + # ]), + # obj.files.current_file.url, + # ) + # + # with self.captureOnCommitCallbacks(execute=True): + # obj.delete() + # # We do not test the None case, as changing it at runtime isn't actually + # # supported. The single case works (although also not supported), so we + # # can actually test it there. If it works there, it should work here. + # # This test case should only test if setting url_pattern will actually + # # propagate it to the linking model. + # + # get_storage().clear() class CustomFileTests(FileTests): diff --git a/src/cdh/files/db/fields.py b/src/cdh/files/db/fields.py index a9c1a067..81d921e2 100644 --- a/src/cdh/files/db/fields.py +++ b/src/cdh/files/db/fields.py @@ -685,6 +685,8 @@ def __init__(self, to=None, related_name=None, related_query_name=None, if file_kwargs is None: file_kwargs = {} + logger.warning("TrackedFileField is semi-deprecated; behavior is no longer tested") + self.url_pattern = url_pattern file_kwargs['url_pattern'] = url_pattern