Skip to content

Commit

Permalink
Merge branch 'master' into jenkins/upgrade-python-requirements-9c03ca5
Browse files Browse the repository at this point in the history
  • Loading branch information
macdiesel authored Feb 6, 2024
2 parents d8d06c4 + 79e3bfd commit fef9492
Show file tree
Hide file tree
Showing 6 changed files with 388 additions and 23 deletions.
2 changes: 0 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,6 @@ private.py

docs/_build/

scripts/

.vscode/

.dev/
Expand Down
72 changes: 72 additions & 0 deletions license_manager/apps/api/pagination.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
"""
Defines custom paginators used by subscription viewsets.
"""
from django.core.paginator import Paginator as DjangoPaginator
from django.utils.functional import cached_property
from rest_framework.pagination import PageNumberPagination


class PageNumberPaginationWithCount(PageNumberPagination):
"""
A PageNumber paginator that adds the total number of pages to the paginated response.
"""

def get_paginated_response(self, data):
""" Adds a ``num_pages`` field into the paginated response. """
response = super().get_paginated_response(data)
response.data['num_pages'] = self.page.paginator.num_pages
return response


class LicensePagination(PageNumberPaginationWithCount):
"""
A PageNumber paginator that allows the client to specify the page size, up to some maximum.
"""
page_size_query_param = 'page_size'
max_page_size = 500


class EstimatedCountDjangoPaginator(DjangoPaginator):
"""
A lazy paginator that determines it's count from
the upstream `estimated_count`
"""
def __init__(self, *args, estimated_count=None, **kwargs):
self.estimated_count = estimated_count
super().__init__(*args, **kwargs)

@cached_property
def count(self):
if self.estimated_count is None:
return super().count
return self.estimated_count


class EstimatedCountLicensePagination(LicensePagination):
"""
Allows the caller (probably the `paginator()` property
of an upstream Viewset) to provided an `estimated_count`,
which means the downstream django paginator does *not*
perform an additional query to get the count of the queryset.
"""
def __init__(self, *args, estimated_count=None, **kwargs):
"""
Optionally stores an `estimated_count` to pass along
to `EstimatedCountDjangoPaginator`.
"""
self.estimated_count = estimated_count
super().__init__(*args, **kwargs)

def django_paginator_class(self, queryset, page_size):
"""
This only works because the implementation of `paginate_queryset`
treats `self.django_paginator_class` as if it is simply a callable,
and not necessarily a class, that returns a Django Paginator instance.
It also (safely) relies on `self` having an instance variable called `estimated_count`.
"""
if self.estimated_count is not None:
return EstimatedCountDjangoPaginator(
queryset, page_size, estimated_count=self.estimated_count,
)
return DjangoPaginator(queryset, page_size)
29 changes: 29 additions & 0 deletions license_manager/apps/api/v1/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@
LEARNER_ROLES,
SUBSCRIPTION_RENEWAL_DAYS_OFFSET,
)
from license_manager.apps.api.v1.views import (
ESTIMATED_COUNT_PAGINATOR_THRESHOLD,
)
from license_manager.apps.core.models import User
from license_manager.apps.subscriptions import constants
from license_manager.apps.subscriptions.exceptions import LicenseRevocationError
Expand Down Expand Up @@ -788,6 +791,32 @@ def test_license_list_staff_user_200_custom_page_size(api_client, staff_user):
assert response.data['next'] is not None


@pytest.mark.django_db
def test_license_list_staff_user_200_estimated_license_count(api_client, staff_user):
subscription, _, _, _, _ = _subscription_and_licenses()
_assign_role_via_jwt_or_db(
api_client,
staff_user,
subscription.enterprise_customer_uuid,
True,
)
subscription.desired_num_licenses = ESTIMATED_COUNT_PAGINATOR_THRESHOLD + 1
subscription.save()

response = _licenses_list_request(
api_client, subscription.uuid, page_size=1,
status=','.join([constants.UNASSIGNED, constants.ASSIGNED, constants.ACTIVATED]),
)

assert status.HTTP_200_OK == response.status_code
results_by_uuid = {item['uuid']: item for item in response.data['results']}
# We test for content in the test above,
# we're only worried about the response count matching `desired_num_licenses` here.
assert len(results_by_uuid) == 1
assert response.data['count'] == ESTIMATED_COUNT_PAGINATOR_THRESHOLD + 1
assert response.data['next'] is not None


@pytest.mark.django_db
def test_license_list_ignore_null_emails_query_param(api_client, staff_user, boolean_toggle):
"""
Expand Down
59 changes: 38 additions & 21 deletions license_manager/apps/api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
from rest_framework.decorators import action
from rest_framework.exceptions import ParseError
from rest_framework.mixins import ListModelMixin
from rest_framework.pagination import PageNumberPagination
from rest_framework.response import Response
from rest_framework.views import APIView
from rest_framework_csv.renderers import CSVRenderer
Expand Down Expand Up @@ -68,11 +67,15 @@
localized_utcnow,
)

from ..pagination import EstimatedCountLicensePagination, LicensePagination


logger = logging.getLogger(__name__)

ASSIGNMENT_LOCK_TIMEOUT_SECONDS = 300

ESTIMATED_COUNT_PAGINATOR_THRESHOLD = 10000


class CustomerAgreementViewSet(
PermissionRequiredForListingMixin,
Expand Down Expand Up @@ -485,26 +488,6 @@ def base_queryset(self):
return licenses


class PageNumberPaginationWithCount(PageNumberPagination):
"""
A PageNumber paginator that adds the total number of pages to the paginated response.
"""

def get_paginated_response(self, data):
""" Adds a ``num_pages`` field into the paginated response. """
response = super().get_paginated_response(data)
response.data['num_pages'] = self.page.paginator.num_pages
return response


class LicensePagination(PageNumberPaginationWithCount):
"""
A PageNumber paginator that allows the client to specify the page size, up to some maximum.
"""
page_size_query_param = 'page_size'
max_page_size = 500


class BaseLicenseViewSet(PermissionRequiredForListingMixin, viewsets.ReadOnlyModelViewSet):
"""
Base Viewset for read operations on individual licenses in a given subscription plan.
Expand Down Expand Up @@ -609,6 +592,40 @@ class LicenseAdminViewSet(BaseLicenseViewSet):

pagination_class = LicensePagination

@property
def paginator(self):
# pylint: disable=line-too-long
"""
If the caller has requested all usable licenses, we want to fall
back to grabbing the paginator's count from ``SubscriptionPlan.desired_num_licenses``
for large plans, as determining the count dynamically is an expensive query.
This is the only way to dynamically select a pagination class in DRF.
https://github.com/encode/django-rest-framework/issues/6397
Underlying implementation of the paginator() property:
https://github.com/encode/django-rest-framework/blob/7749e4e3bed56e0f3e7775b0b1158d300964f6c0/rest_framework/generics.py#L156
"""
if hasattr(self, '_paginator'):
return self._paginator

# If we don't have a subscription plan, or the requested
# status values aren't for all usable licenses, fall back to
# the normal LicensePagination class
self._paginator = super().paginator # pylint: disable=attribute-defined-outside-init

usable_license_states = {constants.UNASSIGNED, constants.ASSIGNED, constants.ACTIVATED}
if value := self.request.query_params.get('status'):
status_values = value.strip().split(',')
if set(status_values) == usable_license_states:
if subscription_plan := self._get_subscription_plan():
estimated_count = subscription_plan.desired_num_licenses
if estimated_count is not None and estimated_count > ESTIMATED_COUNT_PAGINATOR_THRESHOLD:
# pylint: disable=attribute-defined-outside-init
self._paginator = EstimatedCountLicensePagination(estimated_count=estimated_count)

return self._paginator

@property
def active_only(self):
return int(self.request.query_params.get('active_only', 0))
Expand Down
Loading

0 comments on commit fef9492

Please sign in to comment.