feat/pagination: add paginated endpoints for rooms and vms #91
Conversation
|
""" WalkthroughPagination support for both rooms and virtual machines has been introduced across controllers, DTOs, use cases, repositories, and interfaces. New paginated response DTOs and use cases were added, old endpoints for fetching all entities were moved, and comprehensive tests and mocks were implemented to cover the new paginated flows. Additionally, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant UseCase
participant Repository
Client->>Controller: GET /room?page=1&limit=10&includeCounts=false (with JWT)
Controller->>UseCase: execute(page=1, limit=10, includeCounts=false)
UseCase->>Repository: paginate(page=1, limit=10)
Repository-->>UseCase: [Room[], totalCount]
UseCase-->>Controller: RoomListResponseDto (with optional counts)
Controller-->>Client: Paginated room list
Client->>Controller: GET /vm?page=1&limit=10 (with JWT)
Controller->>UseCase: execute(page=1, limit=10)
UseCase->>Repository: paginate(page=1, limit=10)
Repository-->>UseCase: [Vm[], totalCount]
UseCase-->>Controller: VmListResponseDto
Controller-->>Client: Paginated VM list
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/modules/rooms/domain/interfaces/room.repository.interface.ts (1)
11-17: Interface design looks good with clear documentation.The
paginatemethod signature follows common pagination patterns. The JSDoc clearly documents the 1-based page numbering and parameter meanings.Consider adding input validation in the implementation to handle edge cases like negative page numbers or zero/negative limits.
src/modules/rooms/application/dto/__tests__/room.list.response.dto.spec.ts (1)
5-21: Tests cover basic functionality well.The test cases verify correct property assignment and totalPages calculation. The math is correct:
Math.ceil(5/2) = 3for the first test andMath.ceil(0/10) = 0for the empty case.Consider adding edge case tests for boundary conditions like invalid page numbers or page sizes, though this may depend on validation handled by the base
PaginatedResponseDtoclass.src/modules/rooms/application/dto/room.list.response.dto.ts (1)
14-21: Remove unnecessary constructor.The constructor only calls the parent constructor without any additional logic, making it redundant. TypeScript will automatically handle this.
- constructor( - items: RoomResponseDto[], - totalItems: number, - currentPage: number, - pageSize: number, - ) { - super(items, totalItems, currentPage, pageSize); - }src/modules/rooms/application/use-cases/get-room-list.use-case.ts (1)
2-2: Fix typo in import path.There's an extra slash in the import path.
-import { RoomRepositoryInterface } from '../..//domain/interfaces/room.repository.interface'; +import { RoomRepositoryInterface } from '../../domain/interfaces/room.repository.interface';src/modules/vms/application/dto/vm.list.response.dto.ts (1)
14-21: Remove the unnecessary constructor.The constructor only passes parameters to the parent class without adding any logic. The parent
PaginatedResponseDtoalready handles these parameters appropriately.- constructor( - items: VmResponseDto[], - totalItems: number, - currentPage: number, - pageSize: number, - ) { - super(items, totalItems, currentPage, pageSize); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/modules/rooms/__mocks__/room.repository.mock.ts(1 hunks)src/modules/rooms/application/controllers/room.controller.spec.ts(4 hunks)src/modules/rooms/application/controllers/room.controller.ts(2 hunks)src/modules/rooms/application/dto/__tests__/room.list.response.dto.spec.ts(1 hunks)src/modules/rooms/application/dto/index.ts(1 hunks)src/modules/rooms/application/dto/room.list.response.dto.ts(1 hunks)src/modules/rooms/application/use-cases/__tests__/get-room-list.use-case.spec.ts(1 hunks)src/modules/rooms/application/use-cases/get-room-list.use-case.ts(1 hunks)src/modules/rooms/application/use-cases/index.ts(2 hunks)src/modules/rooms/domain/interfaces/room.repository.interface.ts(1 hunks)src/modules/rooms/infrastructure/repositories/room.typeorm.repository.ts(1 hunks)src/modules/vms/__mocks__/index.ts(1 hunks)src/modules/vms/__mocks__/vm.repository.mock.ts(1 hunks)src/modules/vms/application/controllers/__tests__/vm.controller.spec.ts(5 hunks)src/modules/vms/application/controllers/vm.controller.ts(3 hunks)src/modules/vms/application/dto/__tests__/vm.list.response.dto.spec.ts(1 hunks)src/modules/vms/application/dto/index.ts(1 hunks)src/modules/vms/application/dto/vm.list.response.dto.ts(1 hunks)src/modules/vms/application/use-cases/__tests__/get-vm-list.use-case.spec.ts(1 hunks)src/modules/vms/application/use-cases/get-vm-list.use-case.ts(1 hunks)src/modules/vms/application/use-cases/index.ts(2 hunks)src/modules/vms/domain/interfaces/vm.repository.interface.ts(1 hunks)src/modules/vms/infrastructure/repositories/vm.typeorm.repository.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/modules/rooms/application/dto/__tests__/room.list.response.dto.spec.ts (2)
src/modules/rooms/__mocks__/rooms.mock.ts (1)
createMockRoom(3-10)src/modules/rooms/application/dto/room.list.response.dto.ts (1)
RoomListResponseDto(7-22)
src/modules/rooms/application/use-cases/__tests__/get-room-list.use-case.spec.ts (3)
src/modules/rooms/domain/interfaces/room.repository.interface.ts (1)
RoomRepositoryInterface(4-18)src/modules/rooms/application/use-cases/index.ts (1)
GetRoomListUseCase(23-23)src/modules/rooms/__mocks__/rooms.mock.ts (1)
createMockRoom(3-10)
src/modules/vms/application/dto/__tests__/vm.list.response.dto.spec.ts (2)
src/modules/vms/__mocks__/index.ts (1)
createMockVm(1-1)src/modules/vms/application/dto/vm.list.response.dto.ts (1)
VmListResponseDto(7-22)
src/modules/vms/__mocks__/vm.repository.mock.ts (1)
src/modules/vms/__mocks__/index.ts (1)
mockVmRepository(2-2)
src/modules/vms/application/use-cases/__tests__/get-vm-list.use-case.spec.ts (3)
src/modules/vms/domain/interfaces/vm.repository.interface.ts (1)
VmRepositoryInterface(4-16)src/modules/vms/application/use-cases/index.ts (1)
GetVmListUseCase(23-23)src/modules/vms/__mocks__/index.ts (1)
createMockVm(1-1)
src/modules/vms/application/use-cases/get-vm-list.use-case.ts (3)
src/modules/vms/infrastructure/repositories/vm.typeorm.repository.ts (1)
Injectable(12-88)src/modules/vms/domain/interfaces/vm.repository.interface.ts (1)
VmRepositoryInterface(4-16)src/modules/vms/application/dto/vm.list.response.dto.ts (1)
VmListResponseDto(7-22)
src/modules/rooms/application/controllers/room.controller.spec.ts (1)
src/modules/rooms/application/use-cases/index.ts (1)
GetRoomListUseCase(23-23)
src/modules/vms/application/controllers/__tests__/vm.controller.spec.ts (3)
src/modules/vms/application/use-cases/index.ts (1)
GetVmListUseCase(23-23)src/modules/vms/__mocks__/index.ts (1)
createMockVm(1-1)src/modules/vms/__mocks__/vms.mock.ts (1)
createMockVm(4-11)
src/modules/rooms/application/controllers/room.controller.ts (2)
src/modules/rooms/application/use-cases/index.ts (1)
GetRoomListUseCase(23-23)src/modules/rooms/application/dto/room.list.response.dto.ts (1)
RoomListResponseDto(7-22)
🪛 Biome (1.9.4)
src/modules/vms/application/dto/vm.list.response.dto.ts
[error] 14-21: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/modules/rooms/application/dto/room.list.response.dto.ts
[error] 14-21: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (30)
src/modules/rooms/__mocks__/room.repository.mock.ts (1)
13-13: LGTM! Mock method correctly added.The addition of the
paginatemock method aligns perfectly with the interface extension and supports testing of the new pagination functionality.src/modules/rooms/application/use-cases/index.ts (1)
4-4: LGTM! Use case properly integrated.The
GetRoomListUseCaseis correctly imported, added to the array, and exported following the established pattern of other use cases in the module.Also applies to: 9-9, 23-23
src/modules/rooms/application/dto/index.ts (1)
3-3: LGTM! DTO properly exported.The
RoomListResponseDtois correctly imported and exported, making it available for use throughout the application.Also applies to: 5-5
src/modules/rooms/infrastructure/repositories/room.typeorm.repository.ts (1)
52-59: LGTM! Clean TypeORM pagination implementation.The pagination method correctly uses TypeORM's
findAndCountwith proper skip/take calculation and maintains consistency with the existingfindAllmethod by loading the same relations.src/modules/rooms/application/use-cases/__tests__/get-room-list.use-case.spec.ts (1)
1-35: LGTM! Comprehensive test coverage for pagination use case.The test suite properly covers the main scenarios including pagination parameters, empty results, and verifies the correct interaction with the repository interface.
src/modules/rooms/application/use-cases/get-room-list.use-case.ts (1)
13-17: LGTM! Clean use case implementation with proper pagination logic.The execute method correctly handles pagination parameters with sensible defaults and properly maps entities to DTOs before returning the paginated response.
src/modules/rooms/application/controllers/room.controller.spec.ts (4)
6-6: LGTM! Proper import addition for the new use case.The import correctly includes the new
GetRoomListUseCaseneeded for pagination functionality.
21-21: LGTM! Proper mock setup for the new use case.The mock is correctly initialized to support testing the pagination functionality.
39-39: LGTM! Proper test module provider configuration.The test module correctly provides the mocked
GetRoomListUseCasefor dependency injection.
59-73: LGTM! Comprehensive pagination test coverage.The test cases properly verify both explicit pagination parameters and default values, ensuring the controller correctly delegates to the use case with the right parameters.
src/modules/rooms/application/controllers/room.controller.ts (2)
10-10: LGTM: Clean imports and dependency injection setup.The imports and constructor dependency injection are properly implemented for the pagination functionality.
Also applies to: 17-17, 23-23, 27-27, 41-41
62-69: Verify intentional breaking change for GET /room endpointThe
getAllRoomsroute was moved fromGET /roomtoGET /room/all, removing the rootGET /roompath:
- No references to
GET /roomwere found in API docs or client code.- This will break existing API consumers relying on
GET /room.Please confirm this is intentional and that you’ve documented or provided a migration/deprecation strategy (e.g., redirect, client notifications) for downstream users.
src/modules/vms/__mocks__/vm.repository.mock.ts (1)
1-1: LGTM! Clean and focused mock implementation.The mock repository follows best practices with a simple factory function returning the necessary jest mock for the
paginatemethod. This aligns well with the interface extension shown in other files.src/modules/vms/application/use-cases/index.ts (3)
6-6: LGTM! Consistent import pattern.The import follows the established naming convention and module structure.
9-9: Good placement in the use case array.Adding
GetVmListUseCaseat the beginning of the array makes sense as it's the primary listing use case, withGetAllVmsUseCaseserving as a fallback for non-paginated scenarios.
23-23: LGTM! Complete export pattern.The export is properly included alongside other use cases, maintaining consistency with the module's export pattern.
src/modules/vms/application/dto/index.ts (1)
1-6: Excellent use of barrel export pattern.This index file properly centralizes all VM-related DTOs, making imports cleaner throughout the application. The inclusion of the new
VmListResponseDtoalongside existing DTOs maintains consistency and improves module organization.src/modules/vms/domain/interfaces/vm.repository.interface.ts (1)
9-15: Well-designed pagination interface.The
paginatemethod follows established patterns:
- Clear JSDoc documentation with parameter descriptions
- Standard tuple return type
[Vm[], number]for items and total count- Appropriate parameter types with meaningful names
The interface extension maintains consistency with the existing repository pattern.
src/modules/vms/__mocks__/index.ts (1)
1-2: Excellent centralization of mock utilities.This index file properly organizes mock exports, making it easier for tests to import the necessary mocks. The combination of existing VM mocks (
createMockVm,createMockVmDto) with the new repository mock (mockVmRepository) provides comprehensive testing support for the pagination feature.src/modules/vms/application/dto/__tests__/vm.list.response.dto.spec.ts (1)
5-21: LGTM! Comprehensive test coverage.The tests properly verify both populated and empty pagination scenarios, including correct
totalPagescalculation (5 items ÷ 2 per page = 3 pages).src/modules/vms/application/use-cases/__tests__/get-vm-list.use-case.spec.ts (1)
9-12: LGTM! Efficient mock setup.The minimal mock approach focusing only on the
paginatemethod is appropriate for unit testing this use case.src/modules/vms/application/use-cases/get-vm-list.use-case.ts (1)
6-18: LGTM! Clean implementation following established patterns.The use case correctly implements pagination logic with appropriate defaults, proper dependency injection, and correct DTO mapping.
src/modules/vms/application/controllers/__tests__/vm.controller.spec.ts (5)
7-7: LGTM! Proper test setup for new use case.The addition of
GetVmListUseCaseto the imports maintains consistency with the existing test structure.
21-21: LGTM! Consistent mock declaration pattern.The mock declaration follows the same pattern as other use case mocks in the test.
50-50: LGTM! Proper mock initialization.The mock initialization is consistent with other use case mocks.
60-60: LGTM! Correct dependency injection setup.The mock is properly provided in the testing module configuration.
95-111: LGTM! Comprehensive pagination endpoint testing.The tests properly verify:
- String-to-number parameter conversion ('1', '5' → 1, 5)
- Default parameter usage when no arguments provided (1, 10)
- Correct pass-through of use case results
src/modules/vms/application/dto/vm.list.response.dto.ts (1)
7-12: Well-structured paginated response DTO.The class properly extends the generic
PaginatedResponseDtoand includes appropriate validation decorators for the items array. The use of@ValidateNested({ each: true })ensures each VM response is validated individually.src/modules/vms/application/controllers/vm.controller.ts (2)
66-71: Good separation of paginated vs. non-paginated endpoints.Moving the original "get all" functionality to a dedicated
/allroute maintains backward compatibility while clearly distinguishing between paginated and non-paginated data retrieval.
55-58: Comprehensive API documentation for pagination.The Swagger documentation properly describes the pagination parameters and response type, which will help API consumers understand the expected behavior.
| @Get() | ||
| @ApiOperation({ | ||
| summary: 'Lister toutes les salles', | ||
| description: 'Renvoie la liste de toutes les salles disponibles.', | ||
| }) | ||
| @ApiQuery({ name: 'page', required: false, type: Number }) | ||
| @ApiQuery({ name: 'limit', required: false, type: Number }) | ||
| @ApiOperation({ summary: 'Lister les salles paginées' }) | ||
| @ApiBearerAuth() | ||
| @UseGuards(JwtAuthGuard) | ||
| @ApiResponse({ status: 200, type: RoomListResponseDto }) | ||
| async getRooms( | ||
| @Query('page') page = '1', | ||
| @Query('limit') limit = '10', | ||
| ): Promise<RoomListResponseDto> { | ||
| return this.getRoomListUseCase.execute(Number(page), Number(limit)); | ||
| } |
There was a problem hiding this comment.
Add input validation for pagination parameters.
The implementation lacks validation for the page and limit query parameters, which could lead to runtime issues if invalid values are passed.
Consider these potential problems:
Number('invalid')returnsNaN- Negative numbers or zero could cause repository issues
- Very large numbers could impact performance
Apply this diff to add proper validation:
+import { IsOptional, IsPositive, Transform } from 'class-validator';
+class PaginationQueryDto {
+ @IsOptional()
+ @Transform(({ value }) => parseInt(value, 10))
+ @IsPositive()
+ page?: number = 1;
+
+ @IsOptional()
+ @Transform(({ value }) => parseInt(value, 10))
+ @IsPositive()
+ limit?: number = 10;
+}
async getRooms(
- @Query('page') page = '1',
- @Query('limit') limit = '10',
+ @Query() query: PaginationQueryDto,
): Promise<RoomListResponseDto> {
- return this.getRoomListUseCase.execute(Number(page), Number(limit));
+ return this.getRoomListUseCase.execute(query.page, query.limit);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/rooms/application/controllers/room.controller.ts around lines 48
to 60, the getRooms method lacks validation for the page and limit query
parameters, which can lead to runtime errors or performance issues if invalid
values are passed. To fix this, add validation logic to ensure page and limit
are positive integers within acceptable ranges before calling
getRoomListUseCase.execute. If the values are invalid, set them to default safe
values or throw a validation error. This prevents NaN, negative, zero, or
excessively large numbers from causing problems.
| async paginate(page: number, limit: number): Promise<[Vm[], number]> { | ||
| return this.findAndCount({ | ||
| relations: ['permissions'], | ||
| skip: (page - 1) * limit, | ||
| take: limit, | ||
| order: { name: 'ASC' }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation.
The paginate method lacks error handling that exists in other repository methods like findAll. Additionally, consider validating input parameters to prevent issues with negative or zero values.
async paginate(page: number, limit: number): Promise<[Vm[], number]> {
+ if (page < 1 || limit < 1) {
+ throw new Error('Page and limit must be positive numbers');
+ }
+
+ try {
return this.findAndCount({
relations: ['permissions'],
skip: (page - 1) * limit,
take: limit,
order: { name: 'ASC' },
});
+ } catch (error) {
+ Logger.error('Error retrieving paginated VMs:', error);
+ throw new VmRetrievalException();
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/modules/vms/infrastructure/repositories/vm.typeorm.repository.ts around
lines 60 to 67, the paginate method currently lacks error handling and input
validation. Add validation to ensure page and limit are positive integers before
proceeding. Wrap the findAndCount call in a try-catch block to handle potential
errors gracefully, logging or rethrowing as appropriate to maintain consistency
with other repository methods.
| @@ -0,0 +1,21 @@ | |||
| import { createMockVm } from '@/modules/vms/__mocks__/vms.mock'; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify import path consistency.
The import path '@/modules/vms/__mocks__/vms.mock' may be inconsistent with other files. Based on the relevant code snippets, createMockVm appears to be exported from '@/modules/vms/__mocks__/index.ts'.
🏁 Script executed:
#!/bin/bash
# Check which file exports createMockVm and verify the correct import path
fd -t f "vms.mock.ts" src/modules/vms/__mocks__/
fd -t f "index.ts" src/modules/vms/__mocks__/
rg -A 3 "export.*createMockVm" src/modules/vms/__mocks__/Length of output: 962
Use the mock directory’s index export
The createMockVm helper is re-exported by src/modules/vms/__mocks__/index.ts. To stay consistent and leverage the directory index, update the import in your test:
• File: src/modules/vms/application/dto/tests/vm.list.response.dto.spec.ts
- import { createMockVm } from '@/modules/vms/__mocks__/vms.mock';
+ import { createMockVm } from '@/modules/vms/__mocks__';This ensures all mock utilities are imported via the central __mocks__/index.ts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { createMockVm } from '@/modules/vms/__mocks__/vms.mock'; | |
| -import { createMockVm } from '@/modules/vms/__mocks__/vms.mock'; | |
| +import { createMockVm } from '@/modules/vms/__mocks__'; |
🤖 Prompt for AI Agents
In src/modules/vms/application/dto/__tests__/vm.list.response.dto.spec.ts at
line 1, update the import statement to import createMockVm from the central mock
directory index file src/modules/vms/__mocks__/index.ts instead of directly from
vms.mock. This maintains consistency by using the directory’s index export for
all mock utilities.
| it('should handle empty list', async () => { | ||
| repo.paginate.mockResolvedValue([[], 0]); | ||
|
|
||
| const result = await useCase.execute(); | ||
|
|
||
| expect(result.items).toEqual([]); | ||
| expect(result.totalItems).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve default parameter verification.
The test should explicitly verify that the default values (page=1, limit=10) are used when no parameters are provided.
it('should handle empty list', async () => {
repo.paginate.mockResolvedValue([[], 0]);
const result = await useCase.execute();
+ expect(repo.paginate).toHaveBeenCalledWith(1, 10);
expect(result.items).toEqual([]);
expect(result.totalItems).toBe(0);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should handle empty list', async () => { | |
| repo.paginate.mockResolvedValue([[], 0]); | |
| const result = await useCase.execute(); | |
| expect(result.items).toEqual([]); | |
| expect(result.totalItems).toBe(0); | |
| }); | |
| it('should handle empty list', async () => { | |
| repo.paginate.mockResolvedValue([[], 0]); | |
| const result = await useCase.execute(); | |
| expect(repo.paginate).toHaveBeenCalledWith(1, 10); | |
| expect(result.items).toEqual([]); | |
| expect(result.totalItems).toBe(0); | |
| }); |
🤖 Prompt for AI Agents
In src/modules/vms/application/use-cases/__tests__/get-vm-list.use-case.spec.ts
around lines 26 to 33, the test for handling an empty list does not verify that
the default pagination parameters page=1 and limit=10 are used when no
parameters are passed. Update the test to explicitly check that the
useCase.execute method calls the repository's paginate function with these
default values, ensuring the default parameter behavior is correctly tested.
| @Get() | ||
| @ApiOperation({ | ||
| summary: 'Lister toutes les machines virtuelles', | ||
| description: | ||
| 'Renvoie toutes les machines virtuelles enregistrées dans le système.', | ||
| }) | ||
| @ApiQuery({ name: 'page', required: false, type: Number }) | ||
| @ApiQuery({ name: 'limit', required: false, type: Number }) | ||
| @ApiOperation({ summary: 'Lister les VMs paginées' }) | ||
| @ApiResponse({ status: 200, type: VmListResponseDto }) | ||
| async getVms( | ||
| @Query('page') page = '1', | ||
| @Query('limit') limit = '10', | ||
| ): Promise<VmListResponseDto> { | ||
| return this.getVmListUseCase.execute(Number(page), Number(limit)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for pagination parameters.
The pagination parameters lack validation to ensure they are positive integers and within reasonable bounds. Invalid values could cause unexpected behavior or performance issues.
Consider adding validation decorators to the query parameters:
async getVms(
- @Query('page') page = '1',
- @Query('limit') limit = '10',
+ @Query('page', new DefaultValuePipe(1), ParseIntPipe) page: number,
+ @Query('limit', new DefaultValuePipe(10), ParseIntPipe) limit: number,
): Promise<VmListResponseDto> {You'll also need to import the additional pipes:
ParseUUIDPipe,
Query,
+ DefaultValuePipe,
+ ParseIntPipe,
UseGuards,Additionally, consider adding validation to ensure reasonable limits:
// In the method body
const validatedLimit = Math.min(Math.max(limit, 1), 100); // Cap at 100 items per page
const validatedPage = Math.max(page, 1); // Ensure page >= 1
return this.getVmListUseCase.execute(validatedPage, validatedLimit);🤖 Prompt for AI Agents
In src/modules/vms/application/controllers/vm.controller.ts lines 54 to 64, the
pagination query parameters 'page' and 'limit' lack validation to ensure they
are positive integers and within reasonable bounds. Fix this by adding
validation decorators such as ParseIntPipe and Min(1) to the parameters, and
import these pipes from '@nestjs/common'. Also, inside the method, clamp 'limit'
to a maximum of 100 and ensure 'page' is at least 1 before passing them to
getVmListUseCase.execute.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/modules/rooms/application/dto/__tests__/room.response.dto.spec.ts (1)
8-21: Consider adding comprehensive test coverage for the new optional properties.While the current test validates the DTO with count fields, additional test cases would improve coverage:
- Test DTO validation without count fields (since they're optional)
- Test invalid count values (negative numbers, non-numbers)
- Test the static
frommethod withincludeCountstrue/false+ it('should be valid without count fields', async () => { + const dto = plainToInstance(RoomResponseDto, { + id: '7c3dda0a-e6fb-4fd5-a8be-23fd45c77c8c', + name: 'Salle 1', + servers: [], + ups: [], + }); + + const errors = await validate(dto); + expect(errors.length).toBe(0); + }); + + it('should be invalid with non-numeric count fields', async () => { + const dto = plainToInstance(RoomResponseDto, { + id: '7c3dda0a-e6fb-4fd5-a8be-23fd45c77c8c', + name: 'Salle 1', + servers: [], + ups: [], + serverCount: 'invalid', + upsCount: 'invalid', + }); + + const errors = await validate(dto); + expect(errors.length).toBeGreaterThan(0); + expect(errors.some((e) => e.property === 'serverCount')).toBe(true); + expect(errors.some((e) => e.property === 'upsCount')).toBe(true); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/modules/rooms/__mocks__/room.response.dto.mock.ts(1 hunks)src/modules/rooms/application/controllers/room.controller.spec.ts(4 hunks)src/modules/rooms/application/controllers/room.controller.ts(2 hunks)src/modules/rooms/application/dto/__tests__/room.response.dto.spec.ts(1 hunks)src/modules/rooms/application/dto/room.response.dto.ts(2 hunks)src/modules/rooms/application/use-cases/__tests__/get-room-list.use-case.spec.ts(1 hunks)src/modules/rooms/application/use-cases/get-room-list.use-case.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/modules/rooms/mocks/room.response.dto.mock.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/modules/rooms/application/use-cases/tests/get-room-list.use-case.spec.ts
- src/modules/rooms/application/use-cases/get-room-list.use-case.ts
- src/modules/rooms/application/controllers/room.controller.spec.ts
- src/modules/rooms/application/controllers/room.controller.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
src/modules/rooms/application/dto/room.response.dto.ts (4)
8-9: LGTM! Imports are correctly added for new validation decorators.The
IsOptionalandIsNumberimports are properly included to support the new optional count properties.
43-52: LGTM! Property declarations follow NestJS best practices.The new optional properties are properly decorated with:
@ApiProperty({ required: false })for Swagger documentation@IsOptional()for validation flexibility@IsNumber()for type safety
58-59: LGTM! Constructor properly handles the new optional parameters.The constructor signature and assignment logic correctly handle the new optional count parameters.
Also applies to: 65-66
69-83: Pagination eagerly loads related entities for accurate counts
TheRoomTypeormRepository.paginatemethod (src/modules/rooms/infrastructure/repositories/room.typeorm.repository.ts) already callsthis.repo.findAndCount({ relations: ['servers', 'servers.ilo', 'ups'], … });so when
RoomResponseDto.from(room, includeCounts)runs, bothroom.serversandroom.upsare fully loaded andservers.length/ups.lengthwill be correct. No further changes required.src/modules/rooms/application/dto/__tests__/room.response.dto.spec.ts (1)
15-16: LGTM! Test correctly includes new optional properties.The test appropriately includes
serverCount: 0andupsCount: 0to validate the complete DTO structure with the new optional fields.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/modules/servers/application/dto/server.list.response.dto.ts (1)
14-21: Remove unnecessary constructor.The static analysis correctly identifies that this constructor is redundant since it only passes parameters to the superclass without any additional logic.
Apply this diff to remove the unnecessary constructor:
export class ServerListResponseDto extends PaginatedResponseDto<ServerResponseDto> { @ApiProperty({ type: () => ServerResponseDto, isArray: true }) @IsArray() @ValidateNested({ each: true }) @Type(() => ServerResponseDto) readonly items: ServerResponseDto[]; - - constructor( - items: ServerResponseDto[], - totalItems: number, - currentPage: number, - pageSize: number, - ) { - super(items, totalItems, currentPage, pageSize); - } }The base class constructor will be called automatically with the same parameters when instantiating this class.
src/modules/servers/infrastructure/repositories/server.typeorm.repository.ts (1)
98-104: Consider making the ordering configurable.The implementation is correct, but the hardcoded
order: { name: 'ASC' }might not be flexible enough for different use cases.Consider adding an optional ordering parameter to the interface and method:
async findAllByFieldPaginated<T extends PrimitiveFields<Server>>( { field, value, disableThrow = false, relations = [], }: FindAllByFieldOptions<Server, T>, page: number, limit: number, + order?: { [key: string]: 'ASC' | 'DESC' }, ): Promise<[Server[], number]> { // ... return await this.findAndCount({ where: whereClause, relations, skip: (page - 1) * limit, take: limit, - order: { name: 'ASC' }, + order: order || { name: 'ASC' }, });src/modules/servers/application/use-cases/__tests__/get-user-servers.use-case.spec.ts (1)
115-117: Consider cleaning up the spy after each test.The
jest.spyOn(ServerResponseDto, 'fromEntity')is used in multiple tests but there's no visible cleanup. This could cause test pollution.Add cleanup in an
afterEachblock:+ afterEach(() => { + jest.restoreAllMocks(); + });Alternatively, consider using
mockImplementationOnceif the mock is only needed for a single assertion.Also applies to: 157-159, 194-196, 280-282
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/core/filters/__tests__/role.filter.spec.ts(1 hunks)src/modules/rooms/application/use-cases/__tests__/get-room-by-id.use-case.spec.ts(2 hunks)src/modules/servers/application/controllers/__tests__/server.controller.spec.ts(2 hunks)src/modules/servers/application/controllers/server.controller.ts(4 hunks)src/modules/servers/application/dto/server.list.response.dto.ts(1 hunks)src/modules/servers/application/use-cases/__tests__/get-user-servers.use-case.spec.ts(2 hunks)src/modules/servers/application/use-cases/get-user-servers.use-case.ts(5 hunks)src/modules/servers/domain/interfaces/server.repository.interface.ts(1 hunks)src/modules/servers/infrastructure/repositories/server.typeorm.repository.ts(1 hunks)src/modules/users/domain/exceptions/user.exception.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/core/filters/tests/role.filter.spec.ts
- src/modules/users/domain/exceptions/user.exception.ts
- src/modules/rooms/application/use-cases/tests/get-room-by-id.use-case.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/modules/servers/domain/interfaces/server.repository.interface.ts (2)
src/core/types/primitive-fields.interface.ts (1)
PrimitiveFields(1-11)src/core/utils/find-all-by-field.options.ts (1)
FindAllByFieldOptions(3-11)
src/modules/servers/application/controllers/server.controller.ts (3)
src/modules/servers/application/dto/server.list.response.dto.ts (1)
ServerListResponseDto(7-22)src/core/decorators/current-user.decorator.ts (1)
CurrentUser(3-9)src/core/types/jwt-payload.interface.ts (1)
JwtPayload(1-4)
src/modules/servers/infrastructure/repositories/server.typeorm.repository.ts (4)
src/core/types/primitive-fields.interface.ts (1)
PrimitiveFields(1-11)src/core/utils/find-all-by-field.options.ts (1)
FindAllByFieldOptions(3-11)src/core/exceptions/repository.exception.ts (1)
InvalidQueryValueException(1-6)src/modules/servers/domain/exceptions/server.exception.ts (1)
ServerRetrievalException(25-29)
src/modules/servers/application/controllers/__tests__/server.controller.spec.ts (3)
src/modules/servers/__mocks__/servers.mock.ts (1)
createMockServerDto(37-42)src/core/types/jwt-payload.interface.ts (1)
JwtPayload(1-4)src/modules/servers/application/dto/server.list.response.dto.ts (1)
ServerListResponseDto(7-22)
src/modules/servers/application/use-cases/get-user-servers.use-case.ts (1)
src/modules/servers/application/dto/server.list.response.dto.ts (1)
ServerListResponseDto(7-22)
src/modules/servers/application/dto/server.list.response.dto.ts (1)
src/modules/servers/application/dto/server.response.dto.ts (1)
ServerResponseDto(12-92)
🪛 Biome (1.9.4)
src/modules/servers/application/dto/server.list.response.dto.ts
[error] 14-21: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
src/modules/servers/domain/interfaces/server.repository.interface.ts (1)
12-16: LGTM! Clean interface extension for pagination support.The new
findAllByFieldPaginatedmethod follows the established pattern and correctly returns a tuple containing both the results and total count, which is the standard approach for paginated queries.src/modules/servers/application/use-cases/get-user-servers.use-case.ts (3)
24-28: LGTM! Well-designed pagination parameter handling.The method signature properly defines default values for pagination parameters, maintaining backward compatibility while enabling pagination functionality.
63-72: Excellent implementation of paginated repository call.The use of destructuring assignment to capture both the servers array and total count from the paginated repository method is clean and follows established patterns.
85-90: Consistent response handling with proper pagination metadata.The construction of
ServerListResponseDtocorrectly includes all pagination metadata (items, total count, current page, and page size), ensuring clients receive complete pagination information.src/modules/servers/infrastructure/repositories/server.typeorm.repository.ts (1)
74-83: LGTM! Solid method signature and parameter handling.The method signature correctly follows the established pattern and includes proper input validation for null/undefined values.
src/modules/servers/application/controllers/__tests__/server.controller.spec.ts (1)
216-288: Well-structured pagination tests!The test cases comprehensively cover default pagination, custom parameters, and empty results. The assertions properly verify all pagination metadata fields.
src/modules/servers/application/use-cases/__tests__/get-user-servers.use-case.spec.ts (3)
4-4: Import added for pagination support.The ServerListResponseDto import is correctly added to support the new paginated response format.
24-91: Excellent test organization for empty result scenarios.The tests comprehensively cover all edge cases where empty paginated responses should be returned. The logical grouping improves test maintainability.
216-258: Robust error handling tests.Good approach to return empty paginated responses on errors rather than throwing, ensuring consistent API responses. The tests verify that pagination parameters are preserved even in error cases.
| @Query('page') page = '1', | ||
| @Query('limit') limit = '10', | ||
| ): Promise<ServerListResponseDto> { | ||
| return this.getUserServersUseCase.execute( | ||
| user.userId, | ||
| Number(page), | ||
| Number(limit), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for pagination parameters.
The current implementation doesn't validate the page and limit parameters, which could lead to issues with invalid values (e.g., negative numbers, zero, or non-numeric strings).
Consider adding validation using NestJS validation pipes:
async getMyServers(
@CurrentUser() user: JwtPayload,
- @Query('page') page = '1',
- @Query('limit') limit = '10',
+ @Query('page', new DefaultValuePipe(1), ParseIntPipe) page: number,
+ @Query('limit', new DefaultValuePipe(10), ParseIntPipe) limit: number,
): Promise<ServerListResponseDto> {
- return this.getUserServersUseCase.execute(
- user.userId,
- Number(page),
- Number(limit),
- );
+ // Add bounds validation
+ const validatedPage = Math.max(1, page);
+ const validatedLimit = Math.min(Math.max(1, limit), 100); // Cap at 100
+
+ return this.getUserServersUseCase.execute(
+ user.userId,
+ validatedPage,
+ validatedLimit,
+ );
}Don't forget to import DefaultValuePipe and ParseIntPipe from @nestjs/common.
🤖 Prompt for AI Agents
In src/modules/servers/application/controllers/server.controller.ts around lines
104 to 111, the pagination parameters 'page' and 'limit' are currently accepted
as strings without validation, which can cause issues with invalid inputs. To
fix this, import DefaultValuePipe and ParseIntPipe from '@nestjs/common' and
apply them to the 'page' and 'limit' query parameters to ensure they are parsed
as integers with default values and validated to be positive numbers. This will
prevent negative, zero, or non-numeric values from being processed.
| skip: (page - 1) * limit, | ||
| take: limit, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add upper limit validation for performance protection.
Consider validating the limit parameter to prevent potential performance issues with very large page sizes.
Add validation at the beginning of the method:
async findAllByFieldPaginated<T extends PrimitiveFields<Server>>(
// ... parameters
): Promise<[Server[], number]> {
if (value === undefined || value === null) {
throw new InvalidQueryValueException(String(field), value);
}
+
+ // Validate pagination parameters
+ if (page < 1) {
+ throw new InvalidQueryValueException('page', page);
+ }
+ if (limit < 1 || limit > 1000) { // Cap at reasonable limit
+ throw new InvalidQueryValueException('limit', limit);
+ }
try {
// ... rest of implementation📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| skip: (page - 1) * limit, | |
| take: limit, | |
| async findAllByFieldPaginated<T extends PrimitiveFields<Server>>( | |
| field: T, | |
| value: Server[T], | |
| page: number, | |
| limit: number, | |
| ): Promise<[Server[], number]> { | |
| if (value === undefined || value === null) { | |
| throw new InvalidQueryValueException(String(field), value); | |
| } | |
| // Validate pagination parameters | |
| if (page < 1) { | |
| throw new InvalidQueryValueException('page', page); | |
| } | |
| if (limit < 1 || limit > 1000) { // Cap at reasonable limit | |
| throw new InvalidQueryValueException('limit', limit); | |
| } | |
| try { | |
| const [items, count] = await this.repository.findAndCount({ | |
| where: { [field]: value }, | |
| skip: (page - 1) * limit, | |
| take: limit, | |
| }); | |
| return [items, count]; | |
| } catch (error) { | |
| // ... existing error handling ... | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/modules/servers/infrastructure/repositories/server.typeorm.repository.ts
around lines 101 to 102, the pagination parameters skip and take use the limit
value directly without validation, which can cause performance issues if limit
is too large. Add validation at the start of the method to enforce an upper
limit on the limit parameter, such as capping it to a maximum allowed value, to
prevent excessively large page sizes from degrading performance.
| it('should handle zero limit gracefully', async () => { | ||
| userRepo.findOneByField.mockResolvedValue({ | ||
| id: 'user-zero', | ||
| roles: [{ id: 'role-zero' }], | ||
| }); | ||
| permissionRepo.findAllByField.mockResolvedValue([ | ||
| { serverId: 'srv-zero', bitmask: PermissionBit.READ }, | ||
| ]); | ||
|
|
||
| serverRepo.findAllByFieldPaginated.mockResolvedValue([[], 1]); | ||
|
|
||
| const result = await useCase.execute('user-zero', 1, 0); | ||
|
|
||
| expect(result).toBeInstanceOf(ServerListResponseDto); | ||
| expect(result.items).toEqual([]); | ||
| expect(result.totalItems).toBe(1); | ||
| expect(result.currentPage).toBe(1); | ||
| expect(result.totalPages).toBe(Infinity); | ||
| }); |
There was a problem hiding this comment.
Handle the zero limit edge case more gracefully.
When limit=0, the test expects totalPages=Infinity, which could cause issues with JSON serialization and frontend handling.
Consider handling this edge case in the use case implementation by either:
- Rejecting limit=0 with a validation error
- Defaulting to a minimum limit (e.g., 1)
- Returning totalPages=0 or totalPages=1 for this edge case
The current behavior with Infinity may not serialize properly in API responses.
🤖 Prompt for AI Agents
In
src/modules/servers/application/use-cases/__tests__/get-user-servers.use-case.spec.ts
around lines 293 to 311, the test expects totalPages to be Infinity when limit
is zero, which can cause serialization and frontend issues. Update the use case
implementation to handle limit=0 more gracefully by either rejecting it with a
validation error, defaulting the limit to a minimum value like 1, or returning
totalPages as 0 or 1 instead of Infinity. Adjust the test accordingly to reflect
this improved handling.
Summary by CodeRabbit
New Features
page,limit, and optionalincludeCountsquery parameters./room/alland/vm/all.Bug Fixes
Tests
Chores
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores