Skip to content

Commit 99df6c4

Browse files
Maffoochclaude
andauthored
perf(jira,product): fix finding-group push N+1 and add case-insensitive product-name index (#15122)
* perf(jira): prefetch finding group's findings before push to fix N+1 Fixes DJANGO-42P8. push_finding_group_to_jira loaded the group without prefetching its findings, so the JIRA helpers it fans out to (jira_description, jira_priority, get_sla_deadline, get_labels, get_tags, ...) each re-ran finding_group.findings.all(), producing an N+1 on dojo_finding_group_findings. Load the group with prefetch_related('findings') so those reads hit the cache. Guarded by a query-count regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(product): add functional Upper(name) index for case-insensitive lookup Fixes DJANGO-D2M. Filtering findings by product name issues WHERE UPPER(dojo_product.name) = UPPER(%s), which the plain unique btree on name can't serve, causing a slow sequential scan. Add a functional Upper(name) index (created CONCURRENTLY). Guarded by a schema regression test asserting the index exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style: satisfy ruff on perf regression tests Fix docstring summary placement (D213), drop unused noqa directives, and move the jira helper import to top-level (the Django test runner has apps loaded, so the lazy import is unnecessary). No behavior change; both tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: use versioned_fixtures for jira finding-group perf test dojo_testdata.json can't load under V3_FEATURE_LOCATIONS (Endpoint model deprecated), which failed setUpClass on the V3 CI matrix variant. Apply the standard @versioned_fixtures decorator so it loads dojo_testdata_locations.json under V3. Verified passing under both V3_FEATURE_LOCATIONS true and false. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 210fe88 commit 99df6c4

5 files changed

Lines changed: 105 additions & 1 deletion

File tree

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
from django.contrib.postgres.operations import AddIndexConcurrently
2+
from django.db import migrations, models
3+
from django.db.models.functions import Upper
4+
5+
6+
class Migration(migrations.Migration):
7+
# CREATE INDEX CONCURRENTLY cannot run inside a transaction block, and avoids
8+
# an ACCESS EXCLUSIVE lock on the dojo_product table.
9+
atomic = False
10+
11+
dependencies = [
12+
("dojo", "0272_reencrypt_tool_config_credentials_aes_gcm"),
13+
]
14+
15+
operations = [
16+
AddIndexConcurrently(
17+
model_name="product",
18+
index=models.Index(Upper("name"), name="dojo_product_upper_name_idx"),
19+
),
20+
]

dojo/jira/helper.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -789,7 +789,11 @@ def push_finding_to_jira(finding_id, *args, **kwargs) -> tuple[str, bool]:
789789

790790
@app.task
791791
def push_finding_group_to_jira(finding_group_id, *args, **kwargs) -> tuple[str, bool]:
792-
finding_group = get_object_or_none(Finding_Group, id=finding_group_id)
792+
# Prefetch the group's findings: the JIRA helpers below (jira_description,
793+
# jira_priority, get_sla_deadline, get_labels, ...) each call
794+
# finding_group.findings.all(), which would otherwise re-query per call and
795+
# N+1 on dojo_finding_group_findings (Sentry DJANGO-42P8).
796+
finding_group = get_object_or_none(Finding_Group.objects.prefetch_related("findings"), id=finding_group_id)
793797
if not finding_group:
794798
message = f"Finding_Group with id {finding_group_id} does not exist, skipping push_finding_group_to_jira"
795799
logger.warning(message)

dojo/product/models.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from django.core.validators import MinValueValidator
44
from django.db import models
5+
from django.db.models.functions import Upper
56
from django.urls import reverse
67
from django.utils import timezone
78
from django.utils.functional import cached_property
@@ -124,6 +125,12 @@ class Product(BaseModel):
124125

125126
class Meta:
126127
ordering = ("name",)
128+
indexes = [
129+
# Serves the case-insensitive product-name lookup
130+
# (WHERE UPPER(name) = UPPER(%s)) used when filtering findings by
131+
# product name; the plain unique btree on name can't (Sentry DJANGO-D2M).
132+
models.Index(Upper("name"), name="dojo_product_upper_name_idx"),
133+
]
127134

128135
def __str__(self):
129136
return self.name
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
"""
2+
Query-count regression test for pushing a finding group to JIRA.
3+
4+
Regression: DJANGO-42P8 — push_finding_group_to_jira loaded the group without
5+
prefetching its findings, so the JIRA helpers it fans out to (jira_description,
6+
jira_priority, get_sla_deadline, get_labels, get_tags, ...) each re-ran
7+
finding_group.findings.all(), producing an N+1 on dojo_finding_group_findings.
8+
"""
9+
from unittest.mock import patch
10+
11+
from django.test import TestCase
12+
13+
from dojo.jira.helper import push_finding_group_to_jira
14+
from dojo.models import Dojo_User, Finding, Finding_Group, Test
15+
from unittests.dojo_test_case import versioned_fixtures
16+
17+
18+
@versioned_fixtures
19+
class JiraFindingGroupPushQueryCountTest(TestCase):
20+
fixtures = ["dojo_testdata.json"]
21+
22+
def _make_group(self, num_findings: int) -> Finding_Group:
23+
test = Test.objects.first()
24+
admin = Dojo_User.objects.get(username="admin")
25+
group = Finding_Group.objects.create(test=test, name="perf group", creator=admin)
26+
group.findings.add(*list(Finding.objects.all()[:num_findings]))
27+
return group
28+
29+
@patch("dojo.jira.helper.add_jira_issue")
30+
@patch("dojo.jira.helper.update_jira_issue")
31+
def test_group_findings_prefetched_before_jira_helpers(self, mock_update, mock_add):
32+
"""
33+
The group handed to the JIRA helpers must have its findings prefetched.
34+
35+
The real helpers read finding_group.findings.all() several times; if the
36+
group is prefetched those reads hit the prefetch (0 queries). Without the
37+
fix each read is a fresh dojo_finding_group_findings query (N+1).
38+
"""
39+
group = self._make_group(5)
40+
41+
def _simulate_jira_helpers(obj, *args, **kwargs):
42+
with self.assertNumQueries(0):
43+
for _ in range(5):
44+
list(obj.findings.all())
45+
return "ok", True
46+
47+
mock_add.side_effect = _simulate_jira_helpers
48+
push_finding_group_to_jira(group.id)
49+
mock_add.assert_called_once()
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
"""
2+
Schema regression test for the case-insensitive product-name index.
3+
4+
Regression: DJANGO-D2M — filtering findings by product name issues
5+
``WHERE UPPER(dojo_product.name) = UPPER(%s)``. The plain unique btree on
6+
dojo_product.name can't serve that predicate, so a functional Upper(name) index
7+
is required to keep the lookup fast.
8+
"""
9+
from django.db import connection
10+
from django.test import TestCase
11+
12+
INDEX_NAME = "dojo_product_upper_name_idx"
13+
14+
15+
class ProductUpperNameIndexTest(TestCase):
16+
def test_upper_name_functional_index_exists(self):
17+
with connection.cursor() as cursor:
18+
cursor.execute(
19+
"SELECT indexdef FROM pg_indexes WHERE tablename = 'dojo_product' AND indexname = %s",
20+
[INDEX_NAME],
21+
)
22+
row = cursor.fetchone()
23+
self.assertIsNotNone(row, f"expected functional index {INDEX_NAME} on dojo_product")
24+
self.assertIn("upper", row[0].lower(), f"index {INDEX_NAME} is not on UPPER(name): {row[0]}")

0 commit comments

Comments
 (0)