feat(server): enforce pagination limits for Prisma findMany queries #101
Conversation
Adds shared pagination utility applying a default MAX_PAGE_SIZE (100) and sanitizing skip. Integrates utility into hotel, customer, room, and reservation services to cap unbounded queries. Includes unit and integration tests, documentation, and changelog entry. Resolves #84.
Completed Working on "Code Review v2.0"✅ Review submitted: REQUEST_CHANGES. Total comments: 5 across 3 files. |
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for the contribution — adding a shared pagination helper is a solid improvement.
Summary of findings (5 comments):
• 1 BLOCKER: Incorrect import path in pagination e2e test breaks compilation.
• 4 MAJOR: (a) Negative take values not sanitized; (b) Nested relation resolver bypasses pagination guard; (c) Wrong Prisma env var in tests; (d) Persistent file-based SQLite DB may cause flaky tests.
Key themes:
- Robust validation of pagination parameters (handle negative values).
- Consistent enforcement of pagination across all data access paths.
- Test reliability and environment configuration.
Next steps:
– Clamp take to non-negative bounds in applyPaginationDefaults.
– Ensure all data fetch paths (e.g., findRooms) call the new helper or service method.
– Fix the failing import path and use correct DATABASE_URL env var in tests.
– Switch test DB to file:memory: or ensure cleanup between runs.
Once these issues are addressed, I’ll be happy to re-review.
| const sanitized: T = { | ||
| ...args, | ||
| take: | ||
| args.take === undefined || args.take === null |
There was a problem hiding this comment.
Potential correctness issue – take is only clamped when it exceeds MAX_PAGE_SIZE, but negative values are left untouched. Passing a negative take to Prisma will throw an error and can be abused to trigger unnecessary exception handling or DOS the endpoint.
take:
args.take === undefined || args.take === null
? MAX_PAGE_SIZE
: Math.min(args.take, MAX_PAGE_SIZE),Consider additionally clamping the lower bound, e.g. Math.max(0, Math.min(args.take, MAX_PAGE_SIZE)), or throwing a validation error before hitting the database.
| super(prisma); | ||
| } | ||
|
|
||
| async hotels<T extends Prisma.HotelFindManyArgs>( |
There was a problem hiding this comment.
HotelService.hotels() correctly applies applyPaginationDefaults, but the nested resolver findRooms in hotel.controller.base.ts still forwards user-supplied args directly to prisma.room.findMany. That bypasses the new pagination guard and allows clients to request unbounded data. Please ensure nested relation resolvers wrap incoming args with applyPaginationDefaults (or delegate to RoomService) before calling Prisma.
| import { Test } from "@nestjs/testing"; | ||
| import { AppModule } from "../../../src/app.module"; | ||
| import { PrismaService } from "../../../src/prisma/prisma.service"; | ||
| import { applyPaginationDefaults, MAX_PAGE_SIZE } from "../../../src/pagination"; |
There was a problem hiding this comment.
Import path appears to be off by one directory: ../../../src/pagination resolves outside the service package and will fail when tests run from this workspace. It should likely be ../../src/pagination (or use the ts-config path alias) so that the test compiles.
|
|
||
| beforeAll(async () => { | ||
| // Override DB connection to sqlite for tests | ||
| process.env.DB_URL = "file:./test.db?connection_limit=1"; |
There was a problem hiding this comment.
process.env.DB_URL is set, but Prisma expects DATABASE_URL by default. Unless your PrismaService overrides the datasource env var, the test may silently connect to the production/ dev DB. Use process.env.DATABASE_URL or pass the datasource override in the PrismaClient constructor only (and drop the unused env var) to keep tests isolated.
| let app: INestApplication; | ||
| let prisma: PrismaClient; | ||
|
|
||
| beforeAll(async () => { |
There was a problem hiding this comment.
Persisting data to file:./test.db means the SQLite database remains between test runs, which can cause flaky tests (e.g., unique constraint violations on re-seed). Consider using an in-memory DB (file:memory:) or deleting the file / truncating tables in beforeEach.
Completed Working on "Auto-Changelog Reminder"✅ Reviewed PR #101 for changelog requirement: user-facing changes present; changelog entry detected in PR. Posted acknowledgment comment to PR. |
|
✅ Changelog entry detected. Thank you! |
|
This is a test comment to verify the update comment functionality. Initial version. |
|
✅ UPDATED: This comment has been successfully updated using the update_comment_on_pull_request tool. The update functionality is working correctly! |
|
✅ UPDATED - This PR comment has been successfully updated! The content has changed from v1 to v2. This demonstrates the update_comment_on_pull_request tool is working correctly. |
Adds shared pagination utility applying a default MAX_PAGE_SIZE (100) and sanitizing skip. Integrates utility into hotel, customer, room, and reservation services to cap unbounded queries. Includes unit and integration tests, documentation, and changelog entry. Resolves #84.