Skip to content

Query for less data on pagination count #3106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rhysyngsun wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Query for less data on pagination count #3106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3042eef
Update DRF pagination to only use limited fields
rhysyngsun Mar 25, 2026
c84beab
Fix the summary endpoint
rhysyngsun Mar 26, 2026
03f664d
Feedback
rhysyngsun Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions articles/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
)
from rest_framework import status, viewsets
from rest_framework.decorators import action
from rest_framework.pagination import LimitOffsetPagination
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView
Expand All @@ -24,17 +23,6 @@
from .permissions import CanEditArticle, CanViewArticle, is_article_group_user
from .serializers import ArticleImageUploadSerializer

# Create your views here.


class DefaultPagination(LimitOffsetPagination):
"""
Pagination class for learning_resources viewsets which gets default_limit and max_limit from settings
""" # noqa: E501

default_limit = 10
max_limit = 100


@extend_schema_view(
list=extend_schema(
Expand Down Expand Up @@ -66,7 +54,6 @@ class ArticleViewSet(viewsets.ModelViewSet):

serializer_class = RichTextArticleSerializer
queryset = Article.objects.all()
pagination_class = DefaultPagination

permission_classes = [CanViewArticle, CanEditArticle]
http_method_names = VALID_HTTP_METHODS
Expand Down
4 changes: 2 additions & 2 deletions channels/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
ChannelSerializer,
ChannelWriteSerializer,
)
from learning_resources.views import DefaultPagination
from main.constants import VALID_HTTP_METHODS
from main.permissions import AnonymousAccessReadonlyPermission
from main.utils import cache_page_for_all_users
Expand Down Expand Up @@ -68,7 +67,6 @@ class ChannelViewSet(
or organizations at MIT and are a high-level categorization of content.
"""

pagination_class = DefaultPagination
permission_classes = (HasChannelPermission,)
http_method_names = VALID_HTTP_METHODS
lookup_field = "id"
Expand Down Expand Up @@ -164,6 +162,7 @@ class ChannelModeratorListView(ListCreateAPIView):

permission_classes = (ChannelModeratorPermissions,)
serializer_class = ChannelModeratorSerializer
pagination_class = None

def get_queryset(self):
"""
Expand Down Expand Up @@ -211,6 +210,7 @@ class ChannelCountsView(mixins.ListModelMixin, viewsets.GenericViewSet):

serializer_class = ChannelCountsSerializer
permission_classes = (AnonymousAccessReadonlyPermission,)
pagination_class = None

def get_queryset(self):
"""
Expand Down
32 changes: 9 additions & 23 deletions learning_resources/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
from rest_framework.decorators import action
from rest_framework.filters import OrderingFilter
from rest_framework.generics import get_object_or_404
from rest_framework.pagination import LimitOffsetPagination
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework_nested.viewsets import NestedViewSetMixin
Expand Down Expand Up @@ -104,6 +103,7 @@
)
from main.constants import VALID_HTTP_METHODS
from main.filters import MultipleOptionsFilterBackend
from main.pagination import LargePagination
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing DefaultPagination/LargePagination from learning_resources.views breaks existing imports (e.g., testimonials/views.py imports LargePagination from learning_resources.views). Update those imports/usages to point to main.pagination (or re-export from learning_resources.views) to avoid ImportError at runtime.

Suggested change
from main.pagination import LargePagination
from main.pagination import DefaultPagination, LargePagination

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@zamanafzal zamanafzal Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make this change at

mit-learn/testimonials/views.py

Line 7 in 088cac9

from learning_resources.views import LargePagination

from main.permissions import (
AnonymousAccessReadonlyPermission,
is_admin_user,
Expand All @@ -130,22 +130,6 @@ def show_content_file_content(user):
log = logging.getLogger(__name__)


class DefaultPagination(LimitOffsetPagination):
"""
Pagination class for learning_resources viewsets which gets default_limit and max_limit from settings
""" # noqa: E501

default_limit = 10
max_limit = 100


class LargePagination(DefaultPagination):
"""Large pagination for small resources, e.g., topics."""

default_limit = 1000
max_limit = 1000


@extend_schema_view(
list=extend_schema(
summary="List",
Expand All @@ -162,7 +146,6 @@ class BaseLearningResourceViewSet(viewsets.ReadOnlyModelViewSet):
"""

permission_classes = (AnonymousAccessReadonlyPermission,)
pagination_class = DefaultPagination
filter_backends = [MultipleOptionsFilterBackend]
filterset_class = LearningResourceFilter
lookup_field = "id"
Expand Down Expand Up @@ -334,7 +317,11 @@ def summary(self, request, **kwargs): # noqa: ARG002
Intended to be performant with large page sizes.
"""
queryset = self.filter_queryset(
self.get_queryset().values("id", "last_modified")
# we don't use `self.get_queryset()` here because there are incomplatible
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in comment: "incomplatible" should be "incompatible".

Suggested change
# we don't use `self.get_queryset()` here because there are incomplatible
# we don't use `self.get_queryset()` here because there are incompatible

Copilot uses AI. Check for mistakes.
# `select_related()` invocations and we don't need related data anyway
LearningResource.objects.filter(published=True)
.only("id", "last_modified")
.distinct()
)
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary action now builds its own base queryset without .distinct(). Because LearningResource filters include many-to-many relations (e.g., topics/departments), filtering can introduce duplicate rows unless the queryset is made distinct. Consider applying .distinct() after filter_queryset (or otherwise ensuring uniqueness) to preserve correct counts/results.

Suggested change
)
).distinct()

Copilot uses AI. Check for mistakes.
page = self.paginate_queryset(queryset)

Expand Down Expand Up @@ -507,6 +494,7 @@ class LearningPathMembershipViewSet(viewsets.ReadOnlyModelViewSet):

serializer_class = MicroLearningPathRelationshipSerializer
permission_classes = (permissions.HasLearningPathMembershipPermissions,)
pagination_class = None
http_method_names = ["get"]

def get_queryset(self):
Expand Down Expand Up @@ -552,7 +540,6 @@ class ResourceListItemsViewSet(NestedViewSetMixin, viewsets.ReadOnlyModelViewSet
parent_lookup_kwargs = {"learning_resource_id": "parent_id"}
permission_classes = (AnonymousAccessReadonlyPermission,)
serializer_class = LearningResourceRelationshipSerializer
pagination_class = DefaultPagination
queryset = (
LearningResourceRelationship.objects.select_related("child")
.prefetch_related(
Expand Down Expand Up @@ -603,6 +590,7 @@ class LearningResourceListRelationshipViewSet(viewsets.GenericViewSet):
"""

permission_classes = (AnonymousAccessReadonlyPermission,)
pagination_class = None
filter_backends = [MultipleOptionsFilterBackend]
queryset = LearningResourceRelationship.objects.select_related("parent", "child")
http_method_names = ["patch"]
Expand Down Expand Up @@ -861,7 +849,6 @@ class ContentFileViewSet(viewsets.ReadOnlyModelViewSet):
.filter(published=True)
.order_by("-created_on")
)
pagination_class = DefaultPagination
filter_backends = [MultipleOptionsFilterBackend]
filterset_class = ContentFileFilter
private_fields = ["content"]
Expand Down Expand Up @@ -912,7 +899,6 @@ class UserListViewSet(viewsets.ModelViewSet):
"""

serializer_class = UserListSerializer
pagination_class = DefaultPagination
permission_classes = (HasUserListPermissions,)
http_method_names = VALID_HTTP_METHODS
lookup_url_kwarg = "id"
Expand Down Expand Up @@ -974,7 +960,6 @@ class UserListItemViewSet(NestedViewSetMixin, viewsets.ModelViewSet):
"position"
)
serializer_class = UserListRelationshipSerializer
pagination_class = DefaultPagination
permission_classes = (HasUserListItemPermissions,)
http_method_names = VALID_HTTP_METHODS
parent_lookup_kwargs = {"userlist_id": "parent"}
Expand Down Expand Up @@ -1020,6 +1005,7 @@ class UserListMembershipViewSet(viewsets.ReadOnlyModelViewSet):

serializer_class = MicroUserListRelationshipSerializer
permission_classes = (IsAuthenticated,)
pagination_class = None
http_method_names = ["get"]

def get_queryset(self):
Expand Down
1 change: 1 addition & 0 deletions learning_resources_search/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ class UserSearchSubscriptionViewSet(mixins.ListModelMixin, viewsets.GenericViewS

permission_classes = (IsAuthenticated,)
serializer_class = PercolateQuerySerializer
pagination_class = None
http_method_names = ["get", "post", "delete"]

def get_queryset(self):
Expand Down
24 changes: 24 additions & 0 deletions main/pagination.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
from rest_framework.pagination import LimitOffsetPagination


class DefaultPagination(LimitOffsetPagination):
"""
Default pagination class for rest APIs
"""

count_fields = ("pk",)

default_limit = 10
max_limit = 100

def get_count(self, queryset):
"""Get the count of objects in the queryset"""
# we additionally filter this down to a subset of fields
return queryset.only(*self.count_fields).count()


class LargePagination(DefaultPagination):
"""Large pagination for small resources, e.g., topics."""

default_limit = 1000
max_limit = 1000
1 change: 1 addition & 0 deletions main/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,7 @@ def get_all_config_keys():
"DEFAULT_AUTHENTICATION_CLASSES": (
"rest_framework.authentication.SessionAuthentication",
),
"DEFAULT_PAGINATION_CLASS": "main.pagination.DefaultPagination",
Copy link
Copy Markdown
Contributor

@zamanafzal zamanafzal Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're now setting DEFAULT_PAGINATION_CLASS in settings, some endpoints might switch from returning a plain list ([]) to a full paginated response ({count, next, previous, results}).
Even with the pagination_class = None you added in a few places, do you think we should do a quick audit of all list endpoints to make sure we haven't accidentally changed the response shape for any frontend consumers?

Copy link
Copy Markdown
Contributor

@ChristopherChudzicki ChristopherChudzicki Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(doesn't hurt to check, but this is the sort of thing our OpenAPI CI check should actualy be very good at catching)

"EXCEPTION_HANDLER": "main.exceptions.api_exception_handler",
"TEST_REQUEST_DEFAULT_FORMAT": "json",
"TEST_REQUEST_RENDERER_CLASSES": [
Expand Down
13 changes: 0 additions & 13 deletions news_events/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from django.utils.decorators import method_decorator
from drf_spectacular.utils import extend_schema, extend_schema_view
from rest_framework import viewsets
from rest_framework.pagination import LimitOffsetPagination

from main.filters import MultipleOptionsFilterBackend
from main.permissions import AnonymousAccessReadonlyPermission
Expand All @@ -16,16 +15,6 @@
from news_events.serializers import FeedItemSerializer, FeedSourceSerializer


class DefaultPagination(LimitOffsetPagination):
"""
Pagination class for news/events viewsets which gets
default_limit and max_limit from settings
"""

default_limit = 10
max_limit = 100


@extend_schema_view(
list=extend_schema(
description="Get a paginated list of feed items.",
Expand All @@ -42,7 +31,6 @@ class FeedItemViewSet(viewsets.ReadOnlyModelViewSet):
resource_type_name_plural = "News and Events"
serializer_class = FeedItemSerializer
permission_classes = (AnonymousAccessReadonlyPermission,)
pagination_class = DefaultPagination
filter_backends = [MultipleOptionsFilterBackend]
filterset_class = FeedItemFilter
queryset = (
Expand Down Expand Up @@ -80,7 +68,6 @@ class FeedSourceViewSet(viewsets.ReadOnlyModelViewSet):
"""

permission_classes = (AnonymousAccessReadonlyPermission,)
pagination_class = DefaultPagination
resource_type_name_plural = "News & Events Sources"
serializer_class = FeedSourceSerializer
filter_backends = [MultipleOptionsFilterBackend]
Expand Down
2 changes: 1 addition & 1 deletion profiles/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class UserViewSet(viewsets.ModelViewSet):
"""View for users"""

permission_classes = (IsAuthenticated, IsStaffPermission)

pagination_class = None
serializer_class = UserSerializer

queryset = get_user_model().objects.filter(is_active=True)
Expand Down
2 changes: 1 addition & 1 deletion testimonials/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
from drf_spectacular.utils import extend_schema, extend_schema_view
from rest_framework.viewsets import ReadOnlyModelViewSet

from learning_resources.views import LargePagination
from main.filters import MultipleOptionsFilterBackend
from main.pagination import LargePagination
from main.permissions import AnonymousAccessReadonlyPermission
from main.utils import now_in_utc
from testimonials.filters import AttestationFilter
Expand Down
5 changes: 2 additions & 3 deletions video_shorts/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,20 @@
from django.utils.decorators import method_decorator
from drf_spectacular.utils import extend_schema, extend_schema_view
from rest_framework import viewsets
from rest_framework.pagination import LimitOffsetPagination

from main.pagination import DefaultPagination
from main.permissions import AnonymousAccessReadonlyPermission
from main.utils import cache_page_for_all_users
from video_shorts.models import VideoShort
from video_shorts.serializers import VideoShortSerializer


class VideoShortPagination(LimitOffsetPagination):
class VideoShortPagination(DefaultPagination):
"""
Pagination class for video shorts viewset with a default limit of 12
"""

default_limit = 12
max_limit = 100


@extend_schema_view(
Expand Down
Loading