From c84575c215c7006f70180d1f3f535ec607714a90 Mon Sep 17 00:00:00 2001 From: Dirk Julich Date: Mon, 3 Aug 2026 16:27:46 +0200 Subject: [PATCH] =?UTF-8?q?AAP-81173=20=E2=80=94=20Replace=204-way=20OR=20?= =?UTF-8?q?with=20UNION=20in=20unified=20job=20list=20RBAC=20query=20(#165?= =?UTF-8?q?55)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * AAP-81173 — Replace 4-way OR with UNION in unified job list RBAC query The UnifiedJobAccess.filtered_queryset() method used a 4-way OR to determine which unified jobs a user can see. Under load, the OR forces PostgreSQL to evaluate all four branches in a single plan, preventing branch-specific index optimization and causing 35 hours of DB time per 30-minute Scale Lab window. Split each RBAC branch (template read_role, inventory update, ad-hoc command, org auditor) into separate querysets combined with UNION, giving the planner an independent optimal plan per branch. The UNION result is wrapped in pk__in= for compatibility with BaseAccess.get_queryset() prefetch_related and the workflowapproval filter. This follows the same pattern proven in AAP-83319 (team list UNION fix). * AAP-81173 — Add UnifiedJobPagination to prevent COUNT regression The pk__in UNION pattern used for RBAC filtering forces the large outer table as the driving table for COUNT(*), requiring PostgreSQL to materialize all subquery result sets. On large deployments this produces catastrophic query times (see AAP-83773 for the identical issue on activity_stream). Override the paginator count to use an unfiltered UnifiedJob.objects.count() — the over-count is acceptable for pagination UI. Also fix test_unified_job_list_rando_sees_nothing to assert on results length instead of count, since count is now unfiltered. * AAP-81173 — Add unit tests for UnifiedJobPagination coverage Cover the UnifiedJobPaginator.count cached property and the count_disabled branch in UnifiedJobPagination.paginate_queryset to satisfy SonarCloud's 90% new-code coverage gate. * AAP-81173 — Address review feedback: save/restore paginator class, format consistency - Fix Pagination.paginate_queryset() to save/restore django_paginator_class instead of hardcoding DjangoPaginator in the finally block. This lets subclasses set the class attribute without needing to override the method. - Remove UnifiedJobPagination.paginate_queryset() override — now only needs to set django_paginator_class = UnifiedJobPaginator as a class attribute. - Format by_org_auditor consistently with the other three UNION branches. --------- Co-authored-by: Claude Opus 4.6 --- awx/api/pagination.py | 21 ++-- awx/api/views/__init__.py | 3 +- awx/main/access.py | 45 ++++--- .../dab_rbac/test_unified_job_access.py | 112 ++++++++++++++++++ awx/main/tests/unit/api/test_pagination.py | 62 +++++++++- 5 files changed, 220 insertions(+), 23 deletions(-) create mode 100644 awx/main/tests/functional/dab_rbac/test_unified_job_access.py diff --git a/awx/api/pagination.py b/awx/api/pagination.py index 156a6c5ab1..fd84438e1f 100644 --- a/awx/api/pagination.py +++ b/awx/api/pagination.py @@ -13,7 +13,7 @@ from rest_framework.utils.urls import replace_query_param from rest_framework.settings import api_settings from django.utils.translation import gettext_lazy as _ -from awx.main.models import ActivityStream +from awx.main.models import ActivityStream, UnifiedJob class DisabledPaginator(DjangoPaginator): @@ -27,18 +27,21 @@ class DisabledPaginator(DjangoPaginator): class ActivityStreamPaginator(DjangoPaginator): - """Use unfiltered table count for activity stream pagination (AAP-83773). - - The RBAC-filtered COUNT query takes ~36 min on large tables due to the - pk__in subquery shape from AAP-81860. An unfiltered count is acceptable - for pagination UI -- an approximate over-count is harmless. - """ + """Use unfiltered table count for activity stream pagination (AAP-83773).""" @cached_property def count(self): return ActivityStream.objects.count() +class UnifiedJobPaginator(DjangoPaginator): + """Use unfiltered table count for unified job pagination.""" + + @cached_property + def count(self): + return UnifiedJob.objects.count() + + class Pagination(pagination.PageNumberPagination): page_size_query_param = 'page_size' max_page_size = settings.MAX_PAGE_SIZE @@ -91,6 +94,10 @@ class ActivityStreamPagination(Pagination): django_paginator_class = ActivityStreamPaginator +class UnifiedJobPagination(Pagination): + django_paginator_class = UnifiedJobPaginator + + class LimitPagination(pagination.BasePagination): default_limit = api_settings.PAGE_SIZE limit_query_param = 'limit' diff --git a/awx/api/views/__init__.py b/awx/api/views/__init__.py index 85d3ae9d00..02df14859c 100644 --- a/awx/api/views/__init__.py +++ b/awx/api/views/__init__.py @@ -129,7 +129,7 @@ from awx.api.views.mixin import ( NoTruncateMixin, UnifiedJobExcludeMixin, ) -from awx.api.pagination import ActivityStreamPagination, UnifiedJobEventPagination +from awx.api.pagination import ActivityStreamPagination, UnifiedJobEventPagination, UnifiedJobPagination from awx.main.utils import set_environ logger = logging.getLogger('awx.api.views') @@ -4573,6 +4573,7 @@ class UnifiedJobList(UnifiedJobExcludeMixin, ListAPIView): model = models.UnifiedJob serializer_class = serializers.UnifiedJobListSerializer search_fields = ('description', 'name', 'job__playbook') + pagination_class = UnifiedJobPagination resource_purpose = 'unified jobs' diff --git a/awx/main/access.py b/awx/main/access.py index 184f940614..0c228357e8 100644 --- a/awx/main/access.py +++ b/awx/main/access.py @@ -2508,21 +2508,38 @@ class UnifiedJobAccess(BaseAccess): def filtered_queryset(self): inv_pk_qs = Inventory.access_ids_qs(self.user, 'view') - qs = self.model.objects.filter( - Q(unified_job_template_id__in=UnifiedJobTemplate.accessible_pk_qs(self.user, 'read_role')) - | Q( - pk__in=InventoryUpdate.objects.filter( - inventory_source__inventory__id__in=inv_pk_qs, - ).values('pk') - ) - | Q( - pk__in=AdHocCommand.objects.filter( - inventory__id__in=inv_pk_qs, - ).values('pk') - ) - | Q(organization__in=Organization.access_ids_qs(self.user, 'audit_organization')) + + by_template = ( + self.model.objects.filter(unified_job_template_id__in=UnifiedJobTemplate.accessible_pk_qs(self.user, 'read_role')) + .order_by() + .values_list('pk', flat=True) ) - return qs + + by_inventory_update = ( + InventoryUpdate.objects.filter( + inventory_source__inventory__id__in=inv_pk_qs, + ) + .order_by() + .values_list('pk', flat=True) + ) + + by_adhoc = ( + AdHocCommand.objects.filter( + inventory__id__in=inv_pk_qs, + ) + .order_by() + .values_list('pk', flat=True) + ) + + by_org_auditor = ( + self.model.objects.filter( + organization__in=Organization.access_ids_qs(self.user, 'audit_organization'), + ) + .order_by() + .values_list('pk', flat=True) + ) + + return self.model.objects.filter(pk__in=by_template.union(by_inventory_update, by_adhoc, by_org_auditor)) def get_queryset(self): return super(UnifiedJobAccess, self).get_queryset().filter(workflowapproval__isnull=True) diff --git a/awx/main/tests/functional/dab_rbac/test_unified_job_access.py b/awx/main/tests/functional/dab_rbac/test_unified_job_access.py new file mode 100644 index 0000000000..eaec23de4c --- /dev/null +++ b/awx/main/tests/functional/dab_rbac/test_unified_job_access.py @@ -0,0 +1,112 @@ +import pytest + +from django.test.utils import CaptureQueriesContext +from django.db import connection + +from ansible_base.rbac.models import RoleDefinition + +from awx.api.versioning import reverse +from awx.main.models import ( + AdHocCommand, + InventorySource, + InventoryUpdate, + JobTemplate, + Organization, + Project, + UnifiedJob, +) + + +@pytest.mark.django_db +def test_unified_job_list_uses_union(user, organization, inventory, setup_managed_roles, get): + """The unified job list RBAC query uses UNION instead of OR to allow per-branch query planning.""" + org_admin = user('uj-org-admin') + RoleDefinition.objects.get(name='Organization Admin').give_permission(org_admin, organization) + + project = Project.objects.create(name='uj-test-project', organization=organization) + jt = JobTemplate.objects.create(name='uj-test-jt', project=project, inventory=inventory, organization=organization) + jt.create_unified_job() + + inv_src = InventorySource.objects.create(name='uj-test-invsrc', inventory=inventory, source='ec2') + InventoryUpdate.objects.create(inventory_source=inv_src, source=inv_src.source) + + AdHocCommand.objects.create(name='uj-test-adhoc', inventory=inventory) + + with CaptureQueriesContext(connection) as ctx: + response = get(reverse('api:unified_job_list'), org_admin) + + assert response.status_code == 200 + assert response.data['count'] >= 3 + + uj_rbac_queries = [q['sql'] for q in ctx.captured_queries if 'UNION' in q['sql'] and 'main_unifiedjob' in q['sql']] + assert uj_rbac_queries, "Expected at least one query using UNION for unified job RBAC filtering" + + +@pytest.mark.django_db +def test_unified_job_list_org_auditor_sees_jobs(user, setup_managed_roles, get): + """Org auditors see unified jobs in their org via the audit_organization RBAC branch.""" + org = Organization.objects.create(name='uj-audit-org') + auditor = user('uj-auditor') + RoleDefinition.objects.get(name='Organization Audit').give_permission(auditor, org) + + inventory = org.inventories.create(name='uj-audit-inv') + project = Project.objects.create(name='uj-audit-project', organization=org) + jt = JobTemplate.objects.create(name='uj-audit-jt', project=project, inventory=inventory, organization=org) + job = jt.create_unified_job() + + response = get(reverse('api:unified_job_list'), auditor) + assert response.status_code == 200 + result_ids = [r['id'] for r in response.data['results']] + assert job.pk in result_ids + + +@pytest.mark.django_db +def test_unified_job_list_inventory_viewer_sees_inventory_updates(user, setup_managed_roles, get): + """Users with inventory view permission see inventory updates via the inventory RBAC branch.""" + org = Organization.objects.create(name='uj-inv-org') + inventory = org.inventories.create(name='uj-inv-test') + inv_viewer = user('uj-inv-viewer') + RoleDefinition.objects.get(name='Inventory Admin').give_permission(inv_viewer, inventory) + + inv_src = InventorySource.objects.create(name='uj-inv-src', inventory=inventory, source='ec2') + inv_update = InventoryUpdate.objects.create(inventory_source=inv_src, source=inv_src.source) + + response = get(reverse('api:unified_job_list'), inv_viewer) + assert response.status_code == 200 + result_ids = [r['id'] for r in response.data['results']] + assert inv_update.pk in result_ids + + +@pytest.mark.django_db +def test_unified_job_list_rando_sees_nothing(rando, setup_managed_roles, get): + """Unprivileged user sees no unified jobs.""" + org = Organization.objects.create(name='uj-rando-org') + inventory = org.inventories.create(name='uj-rando-inv') + project = Project.objects.create(name='uj-rando-project', organization=org) + jt = JobTemplate.objects.create(name='uj-rando-jt', project=project, inventory=inventory, organization=org) + jt.create_unified_job() + AdHocCommand.objects.create(name='uj-rando-adhoc', inventory=inventory) + + response = get(reverse('api:unified_job_list'), rando) + assert response.status_code == 200 + assert len(response.data['results']) == 0 + + +@pytest.mark.django_db +def test_unified_job_list_pagination_uses_unfiltered_count(rando, setup_managed_roles, get): + """The pagination count should reflect total unified job rows, not + the RBAC-filtered subset. The RBAC-filtered COUNT is catastrophically + slow on large tables with pk__in UNION subqueries.""" + org = Organization.objects.create(name='uj-count-org') + inventory = org.inventories.create(name='uj-count-inv') + project = Project.objects.create(name='uj-count-project', organization=org) + jt = JobTemplate.objects.create(name='uj-count-jt', project=project, inventory=inventory, organization=org) + jt.create_unified_job() + + total_jobs = UnifiedJob.objects.count() + assert total_jobs > 0 + + response = get(reverse('api:unified_job_list'), rando) + assert response.status_code == 200 + assert len(response.data['results']) == 0 + assert response.data['count'] == total_jobs diff --git a/awx/main/tests/unit/api/test_pagination.py b/awx/main/tests/unit/api/test_pagination.py index 978c9e60df..32eca80af1 100644 --- a/awx/main/tests/unit/api/test_pagination.py +++ b/awx/main/tests/unit/api/test_pagination.py @@ -1,6 +1,6 @@ from unittest.mock import patch, MagicMock -from awx.api.pagination import ActivityStreamPaginator, ActivityStreamPagination, DisabledPaginator +from awx.api.pagination import ActivityStreamPaginator, ActivityStreamPagination, UnifiedJobPaginator, UnifiedJobPagination, DisabledPaginator class TestActivityStreamPaginator: @@ -61,3 +61,63 @@ class TestActivityStreamPagination: assert captured_class['during'] is DisabledPaginator assert pagination.django_paginator_class is ActivityStreamPaginator + + +class TestUnifiedJobPaginator: + def test_count_uses_unfiltered_table_count(self): + with patch('awx.api.pagination.UnifiedJob') as mock_uj: + mock_uj.objects.count.return_value = 42000 + paginator = UnifiedJobPaginator(object_list=[], per_page=25) + assert paginator.count == 42000 + mock_uj.objects.count.assert_called_once() + + def test_count_is_cached(self): + with patch('awx.api.pagination.UnifiedJob') as mock_uj: + mock_uj.objects.count.return_value = 500 + paginator = UnifiedJobPaginator(object_list=[], per_page=25) + _ = paginator.count + _ = paginator.count + mock_uj.objects.count.assert_called_once() + + +class TestUnifiedJobPagination: + def test_default_paginator_class(self): + pagination = UnifiedJobPagination() + assert pagination.django_paginator_class is UnifiedJobPaginator + + def test_normal_request_preserves_unified_job_paginator(self): + pagination = UnifiedJobPagination() + request = MagicMock() + request.query_params = {} + + with patch('rest_framework.pagination.PageNumberPagination.paginate_queryset', return_value=[]): + pagination.paginate_queryset(MagicMock(), request) + + assert pagination.count_disabled is False + assert pagination.django_paginator_class is UnifiedJobPaginator + + def test_count_disabled_restores_unified_job_paginator(self): + pagination = UnifiedJobPagination() + request = MagicMock() + request.query_params = {'count_disabled': 'true'} + + with patch('rest_framework.pagination.PageNumberPagination.paginate_queryset', return_value=[]): + pagination.paginate_queryset(MagicMock(), request) + + assert pagination.count_disabled is True + assert pagination.django_paginator_class is UnifiedJobPaginator + + def test_count_disabled_temporarily_uses_disabled_paginator(self): + pagination = UnifiedJobPagination() + request = MagicMock() + request.query_params = {'count_disabled': 'true'} + captured_class = {} + + def capture_paginator_class(self_inner, queryset, request, **kwargs): + captured_class['during'] = pagination.django_paginator_class + + with patch('rest_framework.pagination.PageNumberPagination.paginate_queryset', capture_paginator_class): + pagination.paginate_queryset(MagicMock(), request) + + assert captured_class['during'] is DisabledPaginator + assert pagination.django_paginator_class is UnifiedJobPaginator