ContextId Changes In List API#7
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for filtering responses by context IDs in the findAll method. The changes involve updating the controller and service to accept an optional array of context IDs and applying a filter to the database query using TypeORM's In operator. A reviewer recommended migrating the inline request body type to a dedicated DTO to improve type safety and enable validation via class-validator.
| public async findAll( | ||
| @Req() request: Request, | ||
| @Body() body: { page?: number; limit?: number; sortBy?: string; sortOrder?: 'ASC' | 'DESC' }, | ||
| @Body() body: { page?: number; limit?: number; sortBy?: string; sortOrder?: 'ASC' | 'DESC'; contextIds?: string[] }, |
There was a problem hiding this comment.
Using an inline type for the request body bypasses the ValidationPipe. This means properties like page, limit, and the new contextIds are not validated for their type or format, which could lead to unexpected errors.
It's recommended to create a dedicated Data Transfer Object (DTO) class for the request body. This allows you to leverage class-validator decorators to enforce type and format constraints.
For example, you could create a ListResponsesDto class:
import { IsOptional, IsInt, IsString, IsIn, IsArray, IsUUID, Min } from 'class-validator';
import { Type } from 'class-transformer';
export class ListResponsesDto {
@IsOptional()
@Type(() => Number)
@IsInt()
@Min(1)
page?: number;
@IsOptional()
@Type(() => Number)
@IsInt()
@Min(1)
limit?: number;
@IsOptional()
@IsString()
sortBy?: string;
@IsOptional()
@IsString()
@IsIn(['ASC', 'DESC'])
sortOrder?: 'ASC' | 'DESC';
@IsOptional()
@IsArray()
@IsUUID('all', { each: true })
contextIds?: string[];
}You can then use this DTO in the controller (@Body() body: ListResponsesDto) and in the service (pagination: ListResponsesDto) for better type safety and validation.
No description provided.