Add throttling to table row retrieval endpoints#1707
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRate limiting decorators were added to two table-row retrieval endpoints in the TableController. The Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to apply explicit throttling configuration to the table row retrieval endpoints in the TableController.
Changes:
- Add
@nestjs/throttler@Throttle()usage to the GET and POST “find rows” endpoints. - Add the
Throttleimport to the table controller.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| IImportCSVFinTable, | ||
| IUpdateRowInTable, | ||
| } from './use-cases/table-use-cases.interface.js'; | ||
| import { Throttle } from '@nestjs/throttler'; | ||
|
|
There was a problem hiding this comment.
Throttle import is out of the usual import grouping/order (external @nestjs/* imports are grouped at the top in other controllers). Consider moving import { Throttle } from '@nestjs/throttler'; up with the other Nest imports for consistency (e.g., backend/src/entities/user/user.controller.ts:18).
| @ApiQuery({ name: 'search', required: false }) | ||
| @Timeout(TimeoutDefaults.EXTENDED) | ||
| @Throttle({ default: { limit: 300, ttl: 60000 } }) | ||
| @Get('/table/rows/:connectionId') | ||
| async findAllRows( |
There was a problem hiding this comment.
These endpoints are already rate-limited by the global ThrottlerGuard (configured in backend/src/app.module.ts:55-62 with limit: 200, ttl: 60000). Adding @Throttle({ default: { limit: 300, ttl: 60000 } }) here overrides that default and actually increases the allowed rate. If the intent is to “add/tighten throttling” for row retrieval, consider removing this decorator (keep global defaults) or setting a stricter per-route limit; also consider following the existing pattern of relaxing limits under isTest() to avoid test flakiness (see backend/src/entities/user/user.controller.ts:148).
| @UseGuards(TableReadGuard) | ||
| @Timeout(TimeoutDefaults.EXTENDED) | ||
| @Throttle({ default: { limit: 300, ttl: 60000 } }) | ||
| @HttpCode(HttpStatus.OK) | ||
| @Post('/table/rows/find/:connectionId') |
There was a problem hiding this comment.
This @Throttle({ default: { limit: 300, ttl: 60000 } }) overrides the global throttling defaults (backend/src/app.module.ts:55-62 uses limit: 200, ttl: 60000) and increases the permitted request rate for this route. If the goal is to add/tighten throttling on row retrieval, consider removing the override or choosing a lower per-route limit; optionally match the repo pattern of higher limits in tests via isTest() (e.g., backend/src/entities/company-info/company-info.controller.ts:214).
Summary by CodeRabbit