diff --git a/src/documents/bulk_edit.py b/src/documents/bulk_edit.py index 7a99c61a3..963a0c5ed 100644 --- a/src/documents/bulk_edit.py +++ b/src/documents/bulk_edit.py @@ -311,17 +311,19 @@ def modify_custom_fields( id__in=[int(field) for field, _ in add_custom_fields], ) } - # Deferred, not `.only()`: these objects get cached onto the FK - # descriptor of newly-created CustomFieldInstance rows below, and - # downstream post_save receivers (e.g. the filename-generation signal) - # touch other Document fields -- `.only("pk")` would just turn that into - # a deferred-field reload per document, trading one N+1 for another. - # `content` is the one field guaranteed to be both large (full OCR text) - # and unused by anything this function or its receivers touch. - docs_by_id: dict[int, Document] = { - doc.id: doc - for doc in Document.objects.filter(id__in=affected_docs).defer("content") - } + # Deferred, not `.only()`: signal receivers touch other Document fields, + # and `.only("pk")` would just turn that into a per-document reload. + # `content` is the one field both large and unused here. Skipped + # entirely for a remove-only call -- the removal pass below resolves + # its own documents. + docs_by_id: dict[int, Document] = ( + { + doc.id: doc + for doc in Document.objects.filter(id__in=affected_docs).defer("content") + } + if add_custom_fields + else {} + ) for field_id, value in add_custom_fields: custom_field = custom_fields_by_id[field_id] value_field = CustomFieldInstance.TYPE_TO_DATA_STORE_NAME_MAP[ @@ -336,29 +338,41 @@ def modify_custom_fields( ): # Prevent self-linking continue - # Pass the already-resolved objects, not bare ids: this caches - # them on the FK descriptor of any newly-created instance, so a - # later `.field`/`.document` access (e.g. auditlog's post_save - # receiver calling `str(instance)`, which touches `.field.name`) - # doesn't trigger its own per-instance re-fetch. - CustomFieldInstance.objects.update_or_create( - document=docs_by_id[doc_id], - field=custom_field, - defaults=defaults, - ) + # Not update_or_create(): it fetches an existing row via plain + # `.get()` before calling .save(), so a signal receiver touching + # `.field`/`.document` (e.g. auditlog) on that save re-fetches + # per instance regardless of what's passed in as lookup kwargs. + # Assigning the cached objects ourselves before .save() avoids + # that for both the create and update case. + try: + instance = CustomFieldInstance.objects.get( + document=docs_by_id[doc_id], + field=custom_field, + ) + except CustomFieldInstance.DoesNotExist: + instance = CustomFieldInstance( + document=docs_by_id[doc_id], + field=custom_field, + ) + instance.document = docs_by_id[doc_id] + instance.field = custom_field + for attr, val in defaults.items(): + setattr(instance, attr, val) + instance.save() if custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK: reflect_doclinks(docs_by_id[doc_id], custom_field, value) - # For doc link fields that are being removed, remove symmetrical links + # For doc link fields being removed, remove symmetrical links. + # select_related here avoids resolving every affected document up front. for doclink_being_removed_instance in CustomFieldInstance.objects.filter( document_id__in=affected_docs, field__id__in=remove_custom_fields, field__data_type=CustomField.FieldDataType.DOCUMENTLINK, value_document_ids__isnull=False, - ).select_related("field"): + ).select_related("field", "document"): for target_doc_id in doclink_being_removed_instance.value: remove_doclink( - document=docs_by_id[doclink_being_removed_instance.document_id], + document=doclink_being_removed_instance.document, field=doclink_being_removed_instance.field, target_doc_id=target_doc_id, ) diff --git a/src/documents/tests/test_bulk_edit.py b/src/documents/tests/test_bulk_edit.py index 9a6158caa..9fae05e27 100644 --- a/src/documents/tests/test_bulk_edit.py +++ b/src/documents/tests/test_bulk_edit.py @@ -440,6 +440,110 @@ class TestBulkEdit(DirectoriesMixin, TestCase): [target.id], ) + def test_modify_custom_fields_update_caches_document_and_field(self) -> None: + """ + GIVEN: + - Several documents already have an instance of a custom field + WHEN: + - modify_custom_fields runs again for the same field, updating + the existing instances rather than creating new ones + THEN: + - No per-instance `.document`/`.field` reload query is issued + (e.g. by auditlog's post_save receiver touching them) + """ + docs = [ + Document.objects.create(checksum=f"update-{i}", title=f"update-{i}") + for i in range(6) + ] + field = CustomField.objects.create( + name="Update Field", + data_type=CustomField.FieldDataType.STRING, + ) + bulk_edit.modify_custom_fields( + [doc.id for doc in docs], + add_custom_fields=[field.id], + remove_custom_fields=[], + ) + + with CaptureQueriesContext(connection) as ctx: + bulk_edit.modify_custom_fields( + [doc.id for doc in docs], + add_custom_fields={field.id: "updated value"}, + remove_custom_fields=[], + ) + + single_row_reloads = [ + q + for q in ctx.captured_queries + if ('FROM "documents_document"' in q["sql"] and '."id" = ' in q["sql"]) + or ('FROM "documents_customfield"' in q["sql"] and '."id" = ' in q["sql"]) + ] + self.assertEqual( + single_row_reloads, + [], + "Expected no per-instance document/field reload queries when " + f"updating existing custom field instances, got: {single_row_reloads}", + ) + + for doc in docs: + self.assertEqual( + doc.custom_fields.get(field=field).value, + "updated value", + ) + + def test_modify_custom_fields_removes_symmetrical_doclinks_batched(self) -> None: + """ + GIVEN: + - Several source documents link to a shared target via a doc + link field + WHEN: + - The field is removed from all of them in one call + THEN: + - The symmetrical links are removed from the target + - The source documents come from one batched query, not one + lookup per source document (the target side, fetched inside + remove_doclink(), is untouched by this PR and out of scope) + """ + target = Document.objects.create(checksum="rm-target", title="rm-target") + docs = [ + Document.objects.create(checksum=f"rm-{i}", title=f"rm-{i}") + for i in range(6) + ] + field = CustomField.objects.create( + name="Related", + data_type=CustomField.FieldDataType.DOCUMENTLINK, + ) + bulk_edit.modify_custom_fields( + [doc.id for doc in docs], + add_custom_fields={field.id: [target.id]}, + remove_custom_fields=[], + ) + self.assertEqual( + target.custom_fields.get(field=field).value, + [d.id for d in docs], + ) + + with CaptureQueriesContext(connection) as ctx: + bulk_edit.modify_custom_fields( + [doc.id for doc in docs], + add_custom_fields=[], + remove_custom_fields=[field.id], + ) + + source_doc_lookups = [ + q + for q in ctx.captured_queries + if 'FROM "documents_document"' in q["sql"] + and any(f'."id" = {doc.id} ' in q["sql"] for doc in docs) + ] + self.assertEqual( + source_doc_lookups, + [], + "Expected source documents to come from a batched query, not " + f"per-document lookups, got: {source_doc_lookups}", + ) + self.assertEqual(target.custom_fields.get(field=field).value, []) + def test_modify_custom_fields_doclink_self_link(self) -> None: """ GIVEN: