Performance: cut redundant per-document lookups in bulk modify_custom_fields (#13807)

* Perf: batch the repeated lookups in modify_custom_fields

`modify_custom_fields()` re-resolved the same objects inside its
per-document loop: `custom_fields.get(id=field_id)` re-ran a CustomField
query for every document, and doc link fields called
`Document.objects.get(id=doc_id)` a second time for a document that was
already known.

Resolve both up front with `in_bulk()` and hand the resolved objects to
`update_or_create()` rather than bare ids. Passing the objects also
populates the FK cache on the newly created instance, so auditlog's
post_save receiver touching `.document`/`.field` no longer costs a reload
per row. The document map defers `content`, the one field here that is
both large and unused. The symmetrical-link removal pass and
`remove_doclink()` get `select_related()` for the same auditlog reason.

Measured over 50 documents, sqlite, audit log enabled:

                        before   after
  add 3 string fields    1502    1054
  update 1 string field   451     403
  add doc link            851     653
  remove doc link         604     354

`update_or_create()` is kept as-is. Dropping it for a hand-rolled
get-or-construct loop removes a further ~4 statements per row, but those
are the SAVEPOINT/RELEASE pairs of its `transaction.atomic()`, and the
`select_for_update()` and IntegrityError fallback that go with them. The
(document, field) unique constraint depends on that when two bulk edits
overlap, and the wall clock did not move to pay for it (331 ms vs 310 ms
for the string case above).

Also normalises the field ids to int once at the top so the old dict API,
whose keys may arrive as strings, indexes the resolved map correctly.

The `if custom_field:` branch it replaces was dead: `.get()` raises
DoesNotExist, it never returns None.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Fixes the comment

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Trenton H
2026-09-09 23:47:07 +00:00
committed by GitHub
co-authored by Claude Opus 5
parent 1c96819625
commit 60709b8319
+40 -35
View File
@@ -299,53 +299,55 @@ def modify_custom_fields(
) -> Literal["OK"]:
qs = Document.objects.filter(id__in=doc_ids).only("pk")
affected_docs = list(qs.values_list("pk", flat=True))
# Ensure add_custom_fields is a list of tuples, supports old API
# Ensure add_custom_fields is a list of (int, value) tuples, supports old API
add_custom_fields = (
add_custom_fields.items()
[(int(field), value) for field, value in add_custom_fields.items()]
if isinstance(add_custom_fields, dict)
else [(field, None) for field in add_custom_fields]
else [(int(field), None) for field in add_custom_fields]
)
custom_fields = CustomField.objects.filter(
id__in=[int(field) for field, _ in add_custom_fields],
).distinct()
# Resolved once, instead of re-querying the same field for every document
custom_fields_by_id: dict[int, CustomField] = CustomField.objects.in_bulk(
[field_id for field_id, _ in add_custom_fields],
)
# Passed to update_or_create() below rather than a bare id, so the FK is
# cached on the created instance and auditlog's post_save receiver does
# not reload it per row. Only needed for additions. content is deferred:
# the one field here that is both large and unused.
docs_by_id: dict[int, Document] = (
Document.objects.defer("content").in_bulk(affected_docs)
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[
custom_field.data_type
]
is_doclink = custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK
for doc_id in affected_docs:
defaults = {}
custom_field = custom_fields.get(id=field_id)
if custom_field:
value_field = CustomFieldInstance.TYPE_TO_DATA_STORE_NAME_MAP[
custom_field.data_type
]
defaults[value_field] = value
if (
custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK
and value
and doc_id in value
):
# Prevent self-linking
continue
if is_doclink and value and doc_id in value:
# Prevent self-linking
continue
CustomFieldInstance.objects.update_or_create(
document_id=doc_id,
field_id=field_id,
defaults=defaults,
document=docs_by_id[doc_id],
field=custom_field,
defaults={value_field: value},
)
if custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK:
doc = Document.objects.get(id=doc_id)
reflect_doclinks(doc, custom_field, value)
if is_doclink:
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 that are being removed, remove symmetrical links.
# select_related avoids a per-instance reload of the document and field.
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", "document"):
for target_doc_id in doclink_being_removed_instance.value:
remove_doclink(
document=Document.objects.get(
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,
)
@@ -1178,10 +1180,13 @@ def remove_doclink(
"""
Removes a 'symmetrical' link to `document` from the target document's existing custom field instance
"""
target_doc_field_instance = CustomFieldInstance.objects.filter(
document_id=target_doc_id,
field=field,
).first()
# select_related: a signal receiver (auditlog) touches .document/.field on
# the save() below, without this that is a per-call reload query
target_doc_field_instance = (
CustomFieldInstance.objects.filter(document_id=target_doc_id, field=field)
.select_related("document", "field")
.first()
)
if (
target_doc_field_instance is not None
and document.id in target_doc_field_instance.value