Skip to content

Fix: Paginator accepts per_page <= 0 and crashes later instead of failing at construction - #117

Open
pullapprove5[bot] wants to merge 1 commit into
masterfrom
pullapprove/pa-5-20871b
Open

Fix: Paginator accepts per_page <= 0 and crashes later instead of failing at construction#117
pullapprove5[bot] wants to merge 1 commit into
masterfrom
pullapprove/pa-5-20871b

Conversation

@pullapprove5

@pullapprove5 pullapprove5 Bot commented Sep 4, 2026

Copy link
Copy Markdown

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.


Opened by a PullApprove implementation run (implement-finding v4) for:

  • PA-5 — Paginator accepts per_page <= 0 and crashes later instead of failing at construction

Merging this is what closes them as fixed.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants