Fix: Paginator accepts per_page <= 0 and crashes later instead of failing at construction - #117
Open
pullapprove5[bot] wants to merge 1 commit into
Open
Fix: Paginator accepts per_page <= 0 and crashes later instead of failing at construction#117pullapprove5[bot] wants to merge 1 commit into
pullapprove5[bot] wants to merge 1 commit into
Conversation
…ling at construction
Reproduced the finding directly against plain/plain/paginator.py: Paginator([1,2,3], 0) raised ZeroDivisionError from num_pages (ceil(count/0)), and Paginator([1,2,3,4,5], -1) produced num_pages == -5, with get_page(1) then raising an uncaught EmptyPage. Fixed by validating per_page in Paginator.__init__: coerce to int and raise ValueError("per_page must be at least 1, got {n}") when per_page < 1, before any state is set — closing the invariant at construction rather than relying on each caller (e.g. AdminListView) to clamp. Added plain/tests/public/test_paginator.py covering per_page=0, per_page=-1, and a normal positive case. Ran ./scripts/fix plain (ruff/oxlint/oxfmt/prettier, all clean) and the plain package's pytest suite (uv, with a bootstrapped uv since it wasn't preinstalled in this sandbox) — all 700 tests passed, including the 3 new ones. Full ./scripts/test could not run because this sandbox has neither Docker nor a local Postgres server for scripts/start-postgres to use; I instead ran `uv run --isolated --package plain --with psycopg[binary] python -m pytest` directly with a placeholder PLAIN_POSTGRES_URL, which was sufficient since none of the plain package's tests in this run touch a live database connection. Committed as a single commit on master.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reproduced the finding directly against plain/plain/paginator.py: Paginator([1,2,3], 0) raised ZeroDivisionError from num_pages (ceil(count/0)), and Paginator([1,2,3,4,5], -1) produced num_pages == -5, with get_page(1) then raising an uncaught EmptyPage. Fixed by validating per_page in Paginator.init: coerce to int and raise ValueError("per_page must be at least 1, got {n}") when per_page < 1, before any state is set — closing the invariant at construction rather than relying on each caller (e.g. AdminListView) to clamp. Added plain/tests/public/test_paginator.py covering per_page=0, per_page=-1, and a normal positive case. Ran ./scripts/fix plain (ruff/oxlint/oxfmt/prettier, all clean) and the plain package's pytest suite (uv, with a bootstrapped uv since it wasn't preinstalled in this sandbox) — all 700 tests passed, including the 3 new ones. Full ./scripts/test could not run because this sandbox has neither Docker nor a local Postgres server for scripts/start-postgres to use; I instead ran
uv run --isolated --package plain --with psycopg[binary] python -m pytestdirectly with a placeholder PLAIN_POSTGRES_URL, which was sufficient since none of the plain package's tests in this run touch a live database connection. Committed as a single commit on master.Opened by a PullApprove implementation run (implement-finding v4) for:
Merging this is what closes them as fixed.