[feature] Added OpenWispPagination class with suitable default values #587
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAdds a new reusable Django REST Framework pagination class at openwisp_utils/api/pagination.py: OpenWispPagination (subclasses DRF PageNumberPagination) with import-time validation for DRF and configurable defaults read from Django settings (OPENWISP_PAGINATION_PAGE_SIZE default 10, OPENWISP_PAGINATION_MAX_PAGE_SIZE default 100, OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM default 'page_size'). Properties allow per-instance overrides. Also adds documentation to docs/developer/other-utilities.rst describing defaults, settings, usage, and subclassing. Sequence Diagram(s)sequenceDiagram
participant Client
participant DRF_View as "DRF View"
participant OpenWispPagination as "OpenWispPagination"
participant Database
Client->>DRF_View: HTTP GET /api/resources?page=1&page_size=10
DRF_View->>OpenWispPagination: request resolved pagination params
OpenWispPagination-->>DRF_View: page_size, max_page_size, query_param
DRF_View->>Database: query with limit/offset
Database-->>DRF_View: result set
DRF_View->>OpenWispPagination: paginate queryset
OpenWispPagination-->>DRF_View: paginated data + metadata
DRF_View-->>Client: HTTP 200 with paginated payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/api/pagination.py`:
- Around line 21-25: The class-level attributes page_size, max_page_size, and
page_size_query_param are read at import time so `@override_settings` won't affect
tests; change them to properties that read from django.conf.settings at access
time and add a setter for page_size so DRF's paginate_queryset can assign to
self.page_size at runtime. Specifically, replace the class attributes in the
pagination class with: a `@property` page_size that returns getattr(settings,
"OPENWISP_PAGINATION_PAGE_SIZE", 10) and a `@page_size.setter` that stores an
instance attribute (e.g., self._page_size) when assigned; implement `@property`
max_page_size and `@property` page_size_query_param that return getattr(settings,
"...", default) so tests using override_settings observe changes. Ensure
references in methods use
self.page_size/self.max_page_size/self.page_size_query_param.
🧹 Nitpick comments (2)
openwisp_utils/api/pagination.py (2)
4-10: Useraise ... from Noneto suppress exception chaining.The
raiseinside theexcept ImportErrorblock should usefrom Noneto produce a clean traceback focused on theImproperlyConfigurederror, rather than showing the originalImportErroralongside it. This is also flagged by Ruff B904.♻️ Proposed fix
try: from rest_framework.pagination import PageNumberPagination -except ImportError: # pragma: nocover +except ImportError: # pragma: nocover raise ImproperlyConfigured( "Django REST Framework is required to use " "this feature but it is not installed" - ) + ) from None
13-19: Docstring formatting is collapsed into a single hard-to-read line.The settings list in the docstring is intended to be multi-line but renders as a single run-on sentence. Consider using proper reStructuredText or plain-text formatting.
📝 Suggested docstring formatting
class OpenWispPagination(PageNumberPagination): - """Reusable pagination class with sensible defaults. - - Configurable via Django settings: - OPENWISP_PAGINATION_PAGE_SIZE - (default: 10) - OPENWISP_PAGINATION_MAX_PAGE_SIZE (default: 100) - - OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM (default: 'page_size') - """ + """Reusable pagination class with sensible defaults. + + Configurable via Django settings: + + - OPENWISP_PAGINATION_PAGE_SIZE (default: 10) + - OPENWISP_PAGINATION_MAX_PAGE_SIZE (default: 100) + - OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM (default: 'page_size') + """
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/developer/other-utilities.rstopenwisp_utils/api/pagination.py
🧰 Additional context used
🪛 Ruff (0.14.14)
openwisp_utils/api/pagination.py
[warning] 7-10: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 7-10: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (1)
docs/developer/other-utilities.rst (1)
86-155: Documentation looks comprehensive and well-structured.The documentation clearly covers defaults, settings configuration, view-level usage, subclassing, and global configuration. The code examples are accurate and follow established patterns in the file.
One minor suggestion: consider adding a note that when using the global configuration (Line 153-155), the
PAGE_SIZEinREST_FRAMEWORKsettings is not needed becauseOpenWispPaginationalready defines its ownpage_sizevia theOPENWISP_PAGINATION_PAGE_SIZEsetting. This could prevent confusion for developers familiar with DRF's standard setup where both are typically configured together.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pushpitkamboj
force-pushed
the
feature/reusable_pagination
branch
from
97eff50 to
8b19ec1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/api/pagination.py`:
- Around line 4-10: Change the bare except ImportError to capture the exception
and re-raise ImproperlyConfigured with explicit exception chaining: use "except
ImportError as err" and re-raise the ImproperlyConfigured(...) using "from err"
so the original ImportError traceback is preserved; this applies to the import
of PageNumberPagination and the subsequent raise of ImproperlyConfigured in
openwisp_utils/api/pagination.py.
🧹 Nitpick comments (2)
openwisp_utils/api/pagination.py (2)
14-19: Docstring readability: consider a proper list format.The settings are concatenated on a single wrapped line. A structured list would be easier to scan and render better in documentation tools.
Proposed fix
"""Reusable pagination class with sensible defaults. - Configurable via Django settings: - OPENWISP_PAGINATION_PAGE_SIZE - (default: 10) - OPENWISP_PAGINATION_MAX_PAGE_SIZE (default: 100) - - OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM (default: 'page_size') + Configurable via Django settings: + + - OPENWISP_PAGINATION_PAGE_SIZE (default: 10) + - OPENWISP_PAGINATION_MAX_PAGE_SIZE (default: 100) + - OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM (default: 'page_size') """
32-41: Minor: inconsistent docstrings across properties.
page_sizehas a docstring on its getter (Line 23), whilemax_page_sizehas one on its setter (Line 40), andpage_size_query_paramhas none at all. Consider either adding brief docstrings consistently to all getters (or none) to keep the class uniform.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/developer/other-utilities.rstopenwisp_utils/api/pagination.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/developer/other-utilities.rst
🧰 Additional context used
🪛 Ruff (0.14.14)
openwisp_utils/api/pagination.py
[warning] 7-10: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 7-10: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (3)
openwisp_utils/api/pagination.py (3)
1-2: LGTM!Clean, minimal imports.
21-30: LGTM — property-based approach correctly addresses the import-time binding issue.The getter falls back to settings at runtime, and the setter accommodates DRF's internal
self.page_size = ...assignment inpaginate_queryset. Since DRF creates a new paginator instance per request, the instance-level override won't leak across requests.
43-53: LGTM!Consistent property pattern with the other two attributes. No issues.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/developer/other-utilities.rst`:
- Around line 132-150: The example is missing the import for ModelViewSet which
prevents copy-paste usage; add the import for ModelViewSet (from
rest_framework.viewsets import ModelViewSet) at the top of the snippet alongside
the OpenWispPagination import so the classes LargePagination and ReportViewSet
(which subclasses ModelViewSet and references OpenWispPagination) work as shown.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/developer/other-utilities.rst
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pushpitkamboj
force-pushed
the
feature/reusable_pagination
branch
from
0eaa83e to
469ee65
Compare
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
Thanks for picking up this issue @pushpitkamboj. The implementation can be greatly simplified. See my comments below.
| @property | ||
| def page_size(self): | ||
| """Return the page size from settings or default.""" | ||
| if hasattr(self, "_page_size"): | ||
| return self._page_size | ||
| return getattr(settings, "OPENWISP_PAGINATION_PAGE_SIZE", 10) | ||
|
|
||
| @page_size.setter | ||
| def page_size(self, value): | ||
| self._page_size = value | ||
|
|
||
| @property | ||
| def max_page_size(self): | ||
| if hasattr(self, "_max_page_size"): | ||
| return self._max_page_size | ||
| return getattr(settings, "OPENWISP_PAGINATION_MAX_PAGE_SIZE", 100) | ||
|
|
||
| @max_page_size.setter | ||
| def max_page_size(self, value): | ||
| """Allow setting max_page_size.""" | ||
| self._max_page_size = value | ||
|
|
||
| @property | ||
| def page_size_query_param(self): | ||
| if hasattr(self, "_page_size_query_param"): | ||
| return self._page_size_query_param | ||
| return getattr( | ||
| settings, "OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM", "page_size" | ||
| ) | ||
|
|
||
| @page_size_query_param.setter | ||
| def page_size_query_param(self, value): | ||
| self._page_size_query_param = value |
There was a problem hiding this comment.
@pushpitkamboj why do we need to use Python properties here? As far as I know, we can simple do the following:
class OpenWispPagination(PageNumberPagination):
page_size = 20
max_page_size = 100
page_size_query_param = page_size | - ``OPENWISP_PAGINATION_PAGE_SIZE`` (default: ``10``): Default number of | ||
| items per page | ||
| - ``OPENWISP_PAGINATION_MAX_PAGE_SIZE`` (default: ``100``): Maximum | ||
| allowed page size | ||
| - ``OPENWISP_PAGINATION_PAGE_SIZE_QUERY_PARAM`` (default: | ||
| ``"page_size"``): Query parameter name for page size |
There was a problem hiding this comment.
While creating the issue, I didn't think we can benefit from having this configurable.
Since you have already implemented this, I will ask you to implement this properly:
- All settings are defined in openwisp_utils/settings.py
- The docs for the settings lives at docs/user/settings.rst. Pay attention to how other settings are defined.
- Use a hyperlink on this page to the settings.rst section to inform the users about these settings. E..g.
You can configure different properties of OpenWispPagination class by using `OPENWISP_PAGINATION_ATTRIBUTES <link to the section>`.
Instead of adding three settings, can we just add one setting which is a dictionary?| For view-specific pagination requirements, you can subclass | ||
| ``OpenWispPagination`` and override the properties: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| from openwisp_utils.api.pagination import OpenWispPagination | ||
| from rest_framework.viewsets import ModelViewSet | ||
|
|
||
|
|
||
| class LargePagination(OpenWispPagination): | ||
| @property | ||
| def page_size(self): | ||
| return 50 | ||
|
|
||
| @property | ||
| def max_page_size(self): | ||
| return 500 | ||
|
|
||
|
|
||
| class ReportViewSet(ModelViewSet): | ||
| queryset = Report.objects.all() | ||
| serializer_class = ReportSerializer | ||
| pagination_class = LargePagination | ||
|
|
There was a problem hiding this comment.
After implementing above changes, this example can be largely simplified.
| **Global Configuration:** | ||
|
|
||
| To use ``OpenWispPagination`` as the default pagination class for all DRF | ||
| views in your project, add it to ``REST_FRAMEWORK`` settings: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| REST_FRAMEWORK = { | ||
| "DEFAULT_PAGINATION_CLASS": "openwisp_utils.api.pagination.OpenWispPagination", | ||
| } | ||
|
|
There was a problem hiding this comment.
Let's avoid mentioning this here. This is a DRF setting which is documented in the DRF docs.
| **API Request Examples:** | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| # Returns first 10 items (default page size) | ||
| GET /api/devices/ | ||
|
|
||
| # Returns items 11-20 (second page) | ||
| GET /api/devices/?page=2 | ||
|
|
||
| # Returns first 25 items (custom page size) | ||
| GET /api/devices/?page_size=25 | ||
|
|
||
| # Returns items 26-50 with custom page size | ||
| GET /api/devices/?page=2&page_size=25 | ||
|
|
There was a problem hiding this comment.
These examples are good, but let's use a real URL, e.g. /api/v1/controller/device/.
There was a problem hiding this comment.
Can we create a simple DRF view in the test project for any model and then test the pagination on that view? The current tests look very synthetic and do not model real world scenario.
I will suggest you to look for test in other modules, e.g. openwisp-notification / openwisp-radius on how we write tests for pagination. Here's one quick instance.
|
Hi, sorry for late response I was working on other PR which was a priority. Will work on this ASAP. |
4 participants
Checklist
Reference to Existing Issue
Closes #586
Description of Changes
OpenWispPaginationclass inheriting from DRF'sPageNumberPagination.