diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index fbc7f178ad..8f400fd012 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to the **Prowler API** are documented in this file. ### 🐞 Fixed +- `GET /api/v1/findings` N+1 query loading `resources__tags` when listing findings [(#11420)](https://github.com/prowler-cloud/prowler/pull/11420) - Clean up the scan tmp output directory when `scan-report` fails so partial files do not accumulate and fill the worker disk (`No space left on device`) [(#11421)](https://github.com/prowler-cloud/prowler/pull/11421) --- diff --git a/api/src/backend/api/tests/test_views.py b/api/src/backend/api/tests/test_views.py index 9e0b4362a3..02a83a997c 100644 --- a/api/src/backend/api/tests/test_views.py +++ b/api/src/backend/api/tests/test_views.py @@ -24,9 +24,11 @@ from conftest import ( today_after_n_days, ) from django.conf import settings +from django.db import connection from django.db.models import Count from django.http import JsonResponse from django.test import RequestFactory +from django.test.utils import CaptureQueriesContext from django.urls import reverse from django_celery_results.models import TaskResult from rest_framework import status @@ -64,6 +66,7 @@ from api.models import ( ProviderSecret, Resource, ResourceFindingMapping, + ResourceTag, Role, RoleProviderGroupRelationship, SAMLConfiguration, @@ -7054,6 +7057,80 @@ class TestFindingViewSet: == findings_fixture[0].status ) + def test_findings_list_resource_tags_no_n_plus_one( + self, authenticated_client, findings_fixture + ): + """Listing findings must load every resource's tags in a constant + number of queries, no matter how many findings/resources are returned. + + This guards ``FindingViewSet._optimize_tags_loading`` against + regressions that would reintroduce one extra query per resource (the + N+1 the prefetch was added to remove). + """ + scan = findings_fixture[0].scan + tenant_id = findings_fixture[0].tenant_id + provider = scan.provider + + def _create_finding_with_tagged_resource(index): + resource = Resource.objects.create( + tenant_id=tenant_id, + provider=provider, + uid=f"arn:aws:ec2:us-east-1:123456789012:instance/n-plus-one-{index}", + name=f"N+1 Instance {index}", + region="us-east-1", + service="ec2", + type="prowler-test", + ) + resource.upsert_or_delete_tags( + [ + ResourceTag.objects.create( + tenant_id=tenant_id, + key=f"key-{index}", + value=f"value-{index}", + ) + ] + ) + finding = Finding.objects.create( + tenant_id=tenant_id, + uid=f"n_plus_one_finding_{index}", + scan=scan, + status=Status.FAIL, + status_extended="n+1 status", + impact=Severity.medium, + severity=Severity.medium, + check_id="test_check_id", + check_metadata={"CheckId": "test_check_id", "servicename": "ec2"}, + first_seen_at="2024-01-02T00:00:00Z", + ) + finding.add_resources([resource]) + return finding + + params = {"filter[inserted_at]": TODAY, "include": "resources"} + + # Baseline: the two findings provided by the fixture. + with CaptureQueriesContext(connection) as baseline: + response = authenticated_client.get(reverse("finding-list"), params) + assert response.status_code == status.HTTP_200_OK + + # Add more findings, each with its own resource carrying tags. + extra_findings = 5 + for index in range(extra_findings): + _create_finding_with_tagged_resource(index) + + with CaptureQueriesContext(connection) as scaled: + response = authenticated_client.get(reverse("finding-list"), params) + assert response.status_code == status.HTTP_200_OK + assert len(response.json()["data"]) == len(findings_fixture) + extra_findings + + # The query count must not grow with the number of findings/resources. + assert len(scaled.captured_queries) == len(baseline.captured_queries), ( + "Resource tags are not being prefetched: " + f"{len(baseline.captured_queries)} queries for {len(findings_fixture)} " + f"findings vs {len(scaled.captured_queries)} for " + f"{len(findings_fixture) + extra_findings}. Likely an N+1 regression " + "in FindingViewSet._optimize_tags_loading." + ) + @pytest.mark.parametrize( "include_values, expected_resources", [ diff --git a/api/src/backend/api/v1/views.py b/api/src/backend/api/v1/views.py index bf5cdcc54b..4a09a04838 100644 --- a/api/src/backend/api/v1/views.py +++ b/api/src/backend/api/v1/views.py @@ -3761,6 +3761,16 @@ class FindingViewSet(PaginateByPkMixin, BaseRLSViewSet): return queryset return super().filter_queryset(queryset) + def _optimize_tags_loading(self, queryset): + """Prefetch resource tags to avoid N+1 queries when serializing findings""" + return queryset.prefetch_related( + Prefetch( + "resources__tags", + queryset=ResourceTag.objects.filter(tenant_id=self.request.tenant_id), + to_attr="prefetched_tags", + ) + ) + def list(self, request, *args, **kwargs): filtered_queryset = self.filter_queryset(self.get_queryset()) return self.paginate_by_pk(