mirror of
https://github.com/ansible/awx.git
synced 2026-08-04 12:00:03 -02:30
AAP-81173 — Replace 4-way OR with UNION in unified job list RBAC query (#16555)
* 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 <noreply@anthropic.com>
This commit is contained in:
@@ -13,7 +13,7 @@ from rest_framework.utils.urls import replace_query_param
|
|||||||
from rest_framework.settings import api_settings
|
from rest_framework.settings import api_settings
|
||||||
from django.utils.translation import gettext_lazy as _
|
from django.utils.translation import gettext_lazy as _
|
||||||
|
|
||||||
from awx.main.models import ActivityStream
|
from awx.main.models import ActivityStream, UnifiedJob
|
||||||
|
|
||||||
|
|
||||||
class DisabledPaginator(DjangoPaginator):
|
class DisabledPaginator(DjangoPaginator):
|
||||||
@@ -27,18 +27,21 @@ class DisabledPaginator(DjangoPaginator):
|
|||||||
|
|
||||||
|
|
||||||
class ActivityStreamPaginator(DjangoPaginator):
|
class ActivityStreamPaginator(DjangoPaginator):
|
||||||
"""Use unfiltered table count for activity stream pagination (AAP-83773).
|
"""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.
|
|
||||||
"""
|
|
||||||
|
|
||||||
@cached_property
|
@cached_property
|
||||||
def count(self):
|
def count(self):
|
||||||
return ActivityStream.objects.count()
|
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):
|
class Pagination(pagination.PageNumberPagination):
|
||||||
page_size_query_param = 'page_size'
|
page_size_query_param = 'page_size'
|
||||||
max_page_size = settings.MAX_PAGE_SIZE
|
max_page_size = settings.MAX_PAGE_SIZE
|
||||||
@@ -91,6 +94,10 @@ class ActivityStreamPagination(Pagination):
|
|||||||
django_paginator_class = ActivityStreamPaginator
|
django_paginator_class = ActivityStreamPaginator
|
||||||
|
|
||||||
|
|
||||||
|
class UnifiedJobPagination(Pagination):
|
||||||
|
django_paginator_class = UnifiedJobPaginator
|
||||||
|
|
||||||
|
|
||||||
class LimitPagination(pagination.BasePagination):
|
class LimitPagination(pagination.BasePagination):
|
||||||
default_limit = api_settings.PAGE_SIZE
|
default_limit = api_settings.PAGE_SIZE
|
||||||
limit_query_param = 'limit'
|
limit_query_param = 'limit'
|
||||||
|
|||||||
@@ -129,7 +129,7 @@ from awx.api.views.mixin import (
|
|||||||
NoTruncateMixin,
|
NoTruncateMixin,
|
||||||
UnifiedJobExcludeMixin,
|
UnifiedJobExcludeMixin,
|
||||||
)
|
)
|
||||||
from awx.api.pagination import ActivityStreamPagination, UnifiedJobEventPagination
|
from awx.api.pagination import ActivityStreamPagination, UnifiedJobEventPagination, UnifiedJobPagination
|
||||||
from awx.main.utils import set_environ
|
from awx.main.utils import set_environ
|
||||||
|
|
||||||
logger = logging.getLogger('awx.api.views')
|
logger = logging.getLogger('awx.api.views')
|
||||||
@@ -4573,6 +4573,7 @@ class UnifiedJobList(UnifiedJobExcludeMixin, ListAPIView):
|
|||||||
model = models.UnifiedJob
|
model = models.UnifiedJob
|
||||||
serializer_class = serializers.UnifiedJobListSerializer
|
serializer_class = serializers.UnifiedJobListSerializer
|
||||||
search_fields = ('description', 'name', 'job__playbook')
|
search_fields = ('description', 'name', 'job__playbook')
|
||||||
|
pagination_class = UnifiedJobPagination
|
||||||
resource_purpose = 'unified jobs'
|
resource_purpose = 'unified jobs'
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -2508,21 +2508,38 @@ class UnifiedJobAccess(BaseAccess):
|
|||||||
|
|
||||||
def filtered_queryset(self):
|
def filtered_queryset(self):
|
||||||
inv_pk_qs = Inventory.access_ids_qs(self.user, 'view')
|
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'))
|
by_template = (
|
||||||
| Q(
|
self.model.objects.filter(unified_job_template_id__in=UnifiedJobTemplate.accessible_pk_qs(self.user, 'read_role'))
|
||||||
pk__in=InventoryUpdate.objects.filter(
|
.order_by()
|
||||||
|
.values_list('pk', flat=True)
|
||||||
|
)
|
||||||
|
|
||||||
|
by_inventory_update = (
|
||||||
|
InventoryUpdate.objects.filter(
|
||||||
inventory_source__inventory__id__in=inv_pk_qs,
|
inventory_source__inventory__id__in=inv_pk_qs,
|
||||||
).values('pk')
|
|
||||||
)
|
)
|
||||||
| Q(
|
.order_by()
|
||||||
pk__in=AdHocCommand.objects.filter(
|
.values_list('pk', flat=True)
|
||||||
|
)
|
||||||
|
|
||||||
|
by_adhoc = (
|
||||||
|
AdHocCommand.objects.filter(
|
||||||
inventory__id__in=inv_pk_qs,
|
inventory__id__in=inv_pk_qs,
|
||||||
).values('pk')
|
|
||||||
)
|
)
|
||||||
| Q(organization__in=Organization.access_ids_qs(self.user, 'audit_organization'))
|
.order_by()
|
||||||
|
.values_list('pk', flat=True)
|
||||||
)
|
)
|
||||||
return qs
|
|
||||||
|
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):
|
def get_queryset(self):
|
||||||
return super(UnifiedJobAccess, self).get_queryset().filter(workflowapproval__isnull=True)
|
return super(UnifiedJobAccess, self).get_queryset().filter(workflowapproval__isnull=True)
|
||||||
|
|||||||
112
awx/main/tests/functional/dab_rbac/test_unified_job_access.py
Normal file
112
awx/main/tests/functional/dab_rbac/test_unified_job_access.py
Normal file
@@ -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
|
||||||
@@ -1,6 +1,6 @@
|
|||||||
from unittest.mock import patch, MagicMock
|
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:
|
class TestActivityStreamPaginator:
|
||||||
@@ -61,3 +61,63 @@ class TestActivityStreamPagination:
|
|||||||
|
|
||||||
assert captured_class['during'] is DisabledPaginator
|
assert captured_class['during'] is DisabledPaginator
|
||||||
assert pagination.django_paginator_class is ActivityStreamPaginator
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user