From 15cf23fd6a57c436443ed0f5b315cd3ea757a893 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Thu, 27 Nov 2025 09:37:45 -0800 Subject: [PATCH] Performance: change async handling of select custom field updates to --- src/documents/signals/handlers.py | 44 ++++++++++------- src/documents/tests/test_api_custom_fields.py | 47 +++++++++++++++++++ src/documents/tests/test_file_handling.py | 7 +-- 3 files changed, 77 insertions(+), 21 deletions(-) diff --git a/src/documents/signals/handlers.py b/src/documents/signals/handlers.py index 0e53c9357..48df739d8 100644 --- a/src/documents/signals/handlers.py +++ b/src/documents/signals/handlers.py @@ -396,7 +396,6 @@ class CannotMoveFilesException(Exception): @receiver(models.signals.post_save, sender=CustomFieldInstance, weak=False) @receiver(models.signals.m2m_changed, sender=Document.tags.through, weak=False) @receiver(models.signals.post_save, sender=Document, weak=False) -@shared_task def update_filename_and_move_files( sender, instance: Document | CustomFieldInstance, @@ -533,34 +532,43 @@ def update_filename_and_move_files( ) -# should be disabled in /src/documents/management/commands/document_importer.py handle -@receiver(models.signals.post_save, sender=CustomField) -def check_paths_and_prune_custom_fields(sender, instance: CustomField, **kwargs): +@shared_task +def process_cf_select_update(custom_field: CustomField): """ - When a custom field is updated: + Update documents tied to a select custom field: + 1. 'Select' custom field instances get their end-user value (e.g. in file names) from the select_options in extra_data, which is contained in the custom field itself. So when the field is changed, we (may) need to update the file names of all documents that have this custom field. 2. If a 'Select' field option was removed, we need to nullify the custom field instances that have the option. """ + select_options = { + option["id"]: option["label"] + for option in custom_field.extra_data.get("select_options", []) + } + + for cf_instance in custom_field.fields.select_related("document").iterator(): + # Check if the current value is still a valid option + if cf_instance.value not in select_options: + cf_instance.value_select = None + cf_instance.save(update_fields=["value_select"]) + + # Update the filename and move files if necessary + update_filename_and_move_files(CustomFieldInstance, cf_instance) + + +# should be disabled in /src/documents/management/commands/document_importer.py handle +@receiver(models.signals.post_save, sender=CustomField) +def check_paths_and_prune_custom_fields(sender, instance: CustomField, **kwargs): + """ + When a custom field is updated, check if we need to update any documents. Done async to avoid slowing down the save operation. + """ if ( instance.data_type == CustomField.FieldDataType.SELECT and instance.fields.count() > 0 and instance.extra_data ): # Only select fields, for now - select_options = { - option["id"]: option["label"] - for option in instance.extra_data.get("select_options", []) - } - - for cf_instance in instance.fields.all(): - # Check if the current value is still a valid option - if cf_instance.value not in select_options: - cf_instance.value_select = None - cf_instance.save(update_fields=["value_select"]) - - # Update the filename and move files if necessary - update_filename_and_move_files.delay(sender, cf_instance) + process_cf_select_update.delay(instance) @receiver(models.signals.post_delete, sender=CustomField) diff --git a/src/documents/tests/test_api_custom_fields.py b/src/documents/tests/test_api_custom_fields.py index b6e6c1342..8cc8f2cb2 100644 --- a/src/documents/tests/test_api_custom_fields.py +++ b/src/documents/tests/test_api_custom_fields.py @@ -1,5 +1,6 @@ import json from datetime import date +from unittest import mock from unittest.mock import ANY from django.contrib.auth.models import Permission @@ -276,6 +277,52 @@ class TestCustomFieldsAPI(DirectoriesMixin, APITestCase): doc.refresh_from_db() self.assertEqual(doc.custom_fields.first().value, None) + @mock.patch("documents.signals.handlers.process_cf_select_update.delay") + def test_custom_field_update_offloaded_once(self, mock_delay): + """ + GIVEN: + - A select custom field attached to multiple documents + WHEN: + - The select options are updated + THEN: + - The async update task is enqueued once + """ + cf_select = CustomField.objects.create( + name="Select Field", + data_type=CustomField.FieldDataType.SELECT, + extra_data={ + "select_options": [ + {"label": "Option 1", "id": "abc-123"}, + {"label": "Option 2", "id": "def-456"}, + ], + }, + ) + + documents = [ + Document.objects.create( + title="WOW", + content="the content", + checksum=f"{i}", + mime_type="application/pdf", + ) + for i in range(3) + ] + for document in documents: + CustomFieldInstance.objects.create( + document=document, + field=cf_select, + value_select="def-456", + ) + + cf_select.extra_data = { + "select_options": [ + {"label": "Option 1", "id": "abc-123"}, + ], + } + cf_select.save() + + mock_delay.assert_called_once_with(cf_select) + def test_custom_field_select_old_version(self): """ GIVEN: diff --git a/src/documents/tests/test_file_handling.py b/src/documents/tests/test_file_handling.py index 117a964ba..a1fd3d674 100644 --- a/src/documents/tests/test_file_handling.py +++ b/src/documents/tests/test_file_handling.py @@ -530,6 +530,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): @override_settings( FILENAME_FORMAT="{{title}}_{{custom_fields|get_cf_value('test')}}", + CELERY_TASK_ALWAYS_EAGER=True, ) @mock.patch("documents.signals.handlers.update_filename_and_move_files") def test_select_cf_updated(self, m): @@ -569,7 +570,7 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): self.assertEqual(generate_filename(doc), Path("document_apple.pdf")) # handler should not have been called - self.assertEqual(m.delay.call_count, 0) + self.assertEqual(m.call_count, 0) cf.extra_data = { "select_options": [ {"label": "aubergine", "id": "abc123"}, @@ -579,8 +580,8 @@ class TestFileHandling(DirectoriesMixin, FileSystemAssertsMixin, TestCase): } cf.save() self.assertEqual(generate_filename(doc), Path("document_aubergine.pdf")) - # handler should have been called via delay - self.assertEqual(m.delay.call_count, 1) + # handler should have been called once via the async task + self.assertEqual(m.call_count, 1) class TestFileHandlingWithArchive(DirectoriesMixin, FileSystemAssertsMixin, TestCase):