mirror of
https://github.com/ansible/awx.git
synced 2026-07-31 09:59:55 -02:30
[devel][forwardport] AAP-83773 — Use unfiltered count for activity stream pagination (#16557)
* AAP-83773 — Use unfiltered count for activity stream pagination Forward-port of tower#7606 (stable-2.6). The RBAC-filtered COUNT(*) on activity_stream takes ~36 min per call on large tables (713K rows) due to the pk__in subquery shape introduced by the AAP-81860 LEFT JOIN fix. Replace with an unfiltered table count for pagination -- an approximate over-count is harmless for UI page navigation while the actual page results remain RBAC-filtered. * AAP-83773 — Add unit tests for ActivityStreamPagination Cover ActivityStreamPaginator and ActivityStreamPagination to satisfy SonarCloud coverage gate on new lines in awx/api/pagination.py. * AAP-83773 — Save/restore django_paginator_class in base Pagination Address review feedback from lallen92: use a save/restore pattern in Pagination.paginate_queryset() so subclasses only need to set the class attribute. Remove the now-redundant paginate_queryset override from ActivityStreamPagination. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Liam Allen <lallen@redhat.com>
This commit is contained in:
@@ -6,12 +6,15 @@ from collections import OrderedDict
|
||||
# Django REST Framework
|
||||
from django.conf import settings
|
||||
from django.core.paginator import Paginator as DjangoPaginator
|
||||
from django.utils.functional import cached_property
|
||||
from rest_framework import pagination
|
||||
from rest_framework.response import Response
|
||||
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
|
||||
|
||||
|
||||
class DisabledPaginator(DjangoPaginator):
|
||||
@property
|
||||
@@ -23,6 +26,19 @@ class DisabledPaginator(DjangoPaginator):
|
||||
return 200
|
||||
|
||||
|
||||
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.
|
||||
"""
|
||||
|
||||
@cached_property
|
||||
def count(self):
|
||||
return ActivityStream.objects.count()
|
||||
|
||||
|
||||
class Pagination(pagination.PageNumberPagination):
|
||||
page_size_query_param = 'page_size'
|
||||
max_page_size = settings.MAX_PAGE_SIZE
|
||||
@@ -57,12 +73,13 @@ class Pagination(pagination.PageNumberPagination):
|
||||
|
||||
def paginate_queryset(self, queryset, request, **kwargs):
|
||||
self.count_disabled = 'count_disabled' in request.query_params
|
||||
original_paginator = self.django_paginator_class
|
||||
try:
|
||||
if self.count_disabled:
|
||||
self.django_paginator_class = DisabledPaginator
|
||||
return super(Pagination, self).paginate_queryset(queryset, request, **kwargs)
|
||||
finally:
|
||||
self.django_paginator_class = DjangoPaginator
|
||||
self.django_paginator_class = original_paginator
|
||||
|
||||
def get_paginated_response(self, data):
|
||||
if self.count_disabled:
|
||||
@@ -70,6 +87,10 @@ class Pagination(pagination.PageNumberPagination):
|
||||
return super(Pagination, self).get_paginated_response(data)
|
||||
|
||||
|
||||
class ActivityStreamPagination(Pagination):
|
||||
django_paginator_class = ActivityStreamPaginator
|
||||
|
||||
|
||||
class LimitPagination(pagination.BasePagination):
|
||||
default_limit = api_settings.PAGE_SIZE
|
||||
limit_query_param = 'limit'
|
||||
|
||||
@@ -129,7 +129,7 @@ from awx.api.views.mixin import (
|
||||
NoTruncateMixin,
|
||||
UnifiedJobExcludeMixin,
|
||||
)
|
||||
from awx.api.pagination import UnifiedJobEventPagination
|
||||
from awx.api.pagination import ActivityStreamPagination, UnifiedJobEventPagination
|
||||
from awx.main.utils import set_environ
|
||||
|
||||
logger = logging.getLogger('awx.api.views')
|
||||
@@ -4818,6 +4818,7 @@ class ActivityStreamList(SimpleListAPIView):
|
||||
model = models.ActivityStream
|
||||
serializer_class = serializers.ActivityStreamSerializer
|
||||
search_fields = ('changes',)
|
||||
pagination_class = ActivityStreamPagination
|
||||
resource_purpose = 'audit trail entries for tracking system changes'
|
||||
|
||||
@extend_schema_if_available(
|
||||
|
||||
@@ -139,6 +139,29 @@ def test_stream_queryset_hides_shows_items(
|
||||
assert access.can_read(activity_stream_entry)
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_activity_stream_pagination_uses_unfiltered_count(get, organization, project, user, settings):
|
||||
"""The pagination count should reflect total activity stream rows, not
|
||||
the RBAC-filtered subset. The RBAC-filtered COUNT is catastrophically
|
||||
slow on large tables (AAP-83773); an approximate over-count from an
|
||||
unfiltered SELECT COUNT(*) is acceptable for pagination UI."""
|
||||
settings.ACTIVITY_STREAM_ENABLED = True
|
||||
|
||||
no_access_user = user('no-access-user', False)
|
||||
|
||||
total_entries = ActivityStream.objects.count()
|
||||
assert total_entries > 0
|
||||
|
||||
url = reverse('api:activity_stream_list')
|
||||
response = get(url, no_access_user)
|
||||
|
||||
assert response.status_code == 200
|
||||
visible_results = len(response.data['results'])
|
||||
pagination_count = response.data['count']
|
||||
assert pagination_count == total_entries
|
||||
assert visible_results < pagination_count
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_stream_user_direct_role_updates(get, post, organization_factory):
|
||||
objects = organization_factory('test_org', superusers=['admin'], users=['test'], inventories=['inv1'])
|
||||
|
||||
63
awx/main/tests/unit/api/test_pagination.py
Normal file
63
awx/main/tests/unit/api/test_pagination.py
Normal file
@@ -0,0 +1,63 @@
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
from awx.api.pagination import ActivityStreamPaginator, ActivityStreamPagination, DisabledPaginator
|
||||
|
||||
|
||||
class TestActivityStreamPaginator:
|
||||
def test_count_uses_unfiltered_table_count(self):
|
||||
with patch('awx.api.pagination.ActivityStream') as mock_as:
|
||||
mock_as.objects.count.return_value = 713000
|
||||
paginator = ActivityStreamPaginator(object_list=[], per_page=25)
|
||||
assert paginator.count == 713000
|
||||
mock_as.objects.count.assert_called_once()
|
||||
|
||||
def test_count_is_cached(self):
|
||||
with patch('awx.api.pagination.ActivityStream') as mock_as:
|
||||
mock_as.objects.count.return_value = 500
|
||||
paginator = ActivityStreamPaginator(object_list=[], per_page=25)
|
||||
_ = paginator.count
|
||||
_ = paginator.count
|
||||
mock_as.objects.count.assert_called_once()
|
||||
|
||||
|
||||
class TestActivityStreamPagination:
|
||||
def test_default_paginator_class(self):
|
||||
pagination = ActivityStreamPagination()
|
||||
assert pagination.django_paginator_class is ActivityStreamPaginator
|
||||
|
||||
def test_normal_request_preserves_activity_stream_paginator(self):
|
||||
pagination = ActivityStreamPagination()
|
||||
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 ActivityStreamPaginator
|
||||
|
||||
def test_count_disabled_restores_activity_stream_paginator(self):
|
||||
pagination = ActivityStreamPagination()
|
||||
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 ActivityStreamPaginator
|
||||
|
||||
def test_count_disabled_temporarily_uses_disabled_paginator(self):
|
||||
pagination = ActivityStreamPagination()
|
||||
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 ActivityStreamPaginator
|
||||
Reference in New Issue
Block a user