refactor: Reinstated a configurable upper bound on pagination limit. - #109
Merged
Conversation
8f08c48 removed MaxLimit to allow flexibility, leaving `limit` unbounded on the five Organizations list endpoints. A single request for `?limit=1000000` makes the database materialise the whole table, holds a pooled connection open for the duration, and forces the API process to serialise the result — a denial-of-service vector reachable by any authenticated caller. The flexibility that motivated removing the cap no longer needs an unbounded limit. b702bd2 added unpaginated GetAll… methods at the repository, service and plugin API layers, so embedders who need a whole collection have a first-class escape hatch that is Go-only and never reachable over HTTP. The ceiling is back, but configurable rather than constant: - pagination.DefaultMaxLimit = 100 is the fallback, and Clamp now takes the maximum as a parameter. A maximum of zero or less falls back to DefaultMaxLimit, so a caller with nothing configured still gets a bounded query rather than an unbounded one. - The floor is applied before the ceiling, so a maximum below DefaultLimit also caps the default: with max_page_limit 5, a request with no limit returns 5 rows. - OrganizationsPluginConfig gains MaxPageLimit *int, guarded in ApplyDefaults the same way InvitationsLimit is. - ServiceUtils carries the configured maximum and exposes ClampPagination. It is the one collaborator all five list services already share, and it is constructed once in plugin.go. Because a zero maxPageLimit degrades to DefaultMaxLimit, the existing &ServiceUtils{...} literals across the service tests keep compiling and behave as cap-100. Clamp's signature changed rather than gaining a ClampWithMax sibling, so that every un-migrated caller is a compile error instead of silently returning unbounded results. That is how the direct pagination.Clamp call inside an assertion in organization_team_member_service_test.go surfaced. The repository-layer pageLimit deliberately keeps its floor and gains no ceiling. Repositories take flat page/limit ints and cannot see plugin config, so capping to a constant there would silently override an operator who raised max_page_limit and make the response envelope lie. Every HTTP path reaches a repository through a service that has already clamped; a direct caller is a Go embedder, on the same footing as the GetAll… methods. The reasoning is recorded as a doc comment. Corrects the five list endpoint descriptions, which have documented "no upper bound on limit" since b702bd2, and adds `default` to the page and limit query parameter schemas. No `minimum` or `maximum`: both are JSON Schema assertions, and a validating gateway would reject `?page=0` outright — precisely the 400 that clamping exists to avoid. The ceiling is also deployment-configurable, so a fixed `maximum` in a static spec would be wrong. Bounds stay in prose, where they are advisory. openapi.json is regenerated. Adds docs/pagination.md covering the envelope, the clamping rules, max_page_limit, the created_at DESC, id DESC ordering and its tiebreaker, and when to reach for the unpaginated GetAll… methods. Tests cover the clamp at every layer: the new maximum and its fallbacks in core/pagination, ClampPagination including a nil receiver, ApplyDefaults, and the repository mock expectations in all five service list tests. The SQL-backed cases seed 120 members and prove the configured value — not the constant — reaches the real LIMIT and the response envelope, and that GetAllMembers ignores the ceiling entirely. The handler test asserting that an absurd limit is forwarded unclamped still passes: clamping stays a service concern and handlers stay parse-only. BREAKING CHANGE: pagination.Clamp takes a maxLimit argument and services.NewServiceUtils takes a maxPageLimit argument. Both are exported; embedders calling them must update their call sites, which will fail to compile rather than change behaviour silently. Over HTTP, a client that relied on an unbounded limit to fetch a whole collection in one request now receives at most max_page_limit rows, with the effective value echoed in pagination.limit. Such a client should page on has_more, use GetAll… if embedding in Go, or run against a deployment configured with a higher ceiling.
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.
8f08c48 removed MaxLimit to allow flexibility, leaving
limitunbounded on the five Organizations list endpoints. A single request for?limit=1000000makes the database materialise the whole table, holds a pooled connection open for the duration, and forces the API process to serialise the result — a denial-of-service vector reachable by any authenticated caller.The flexibility that motivated removing the cap no longer needs an unbounded limit. b702bd2 added unpaginated GetAll… methods at the repository, service and plugin API layers, so embedders who need a whole collection have a first-class escape hatch that is Go-only and never reachable over HTTP.
The ceiling is back, but configurable rather than constant:
Clamp's signature changed rather than gaining a ClampWithMax sibling, so that every un-migrated caller is a compile error instead of silently returning unbounded results. That is how the direct pagination.Clamp call inside an assertion in organization_team_member_service_test.go surfaced.
The repository-layer pageLimit deliberately keeps its floor and gains no ceiling. Repositories take flat page/limit ints and cannot see plugin config, so capping to a constant there would silently override an operator who raised max_page_limit and make the response envelope lie. Every HTTP path reaches a repository through a service that has already clamped; a direct caller is a Go embedder, on the same footing as the GetAll… methods. The reasoning is recorded as a doc comment.
Corrects the five list endpoint descriptions, which have documented "no upper bound on limit" since b702bd2, and adds
defaultto the page and limit query parameter schemas. Nominimumormaximum: both are JSON Schema assertions, and a validating gateway would reject?page=0outright — precisely the 400 that clamping exists to avoid. The ceiling is also deployment-configurable, so a fixedmaximumin a static spec would be wrong. Bounds stay in prose, where they are advisory. openapi.json is regenerated.Adds docs/pagination.md covering the envelope, the clamping rules, max_page_limit, the created_at DESC, id DESC ordering and its tiebreaker, and when to reach for the unpaginated GetAll… methods.
Tests cover the clamp at every layer: the new maximum and its fallbacks in core/pagination, ClampPagination including a nil receiver, ApplyDefaults, and the repository mock expectations in all five service list tests. The SQL-backed cases seed 120 members and prove the configured value — not the constant — reaches the real LIMIT and the response envelope, and that GetAllMembers ignores the ceiling entirely. The handler test asserting that an absurd limit is forwarded unclamped still passes: clamping stays a service concern and handlers stay parse-only.
BREAKING CHANGE: pagination.Clamp takes a maxLimit argument and services.NewServiceUtils takes a maxPageLimit argument. Both are exported; embedders calling them must update their call sites, which will fail to compile rather than change behaviour silently. Over HTTP, a client that relied on an unbounded limit to fetch a whole collection in one request now receives at most max_page_limit rows, with the effective value echoed in pagination.limit. Such a client should page on has_more, use GetAll… if embedding in Go, or run against a deployment configured with a higher ceiling.