After 94 days of building production NestJS applications, I've learned that a good code review separates systems that barely work from platforms that actually scale. My NestJS code review checklist has become the backbone of every pull request I evaluate—it catches the subtle issues that don't show up until 3 AM when the database is locked or a user gains access to someone else's data. Today I'm sharing exactly what I look for.
Validation — Every input has a DTO with validation. No any.
The first thing I check is whether every endpoint has a DTO with proper validation decorators. I've seen too many endpoints that accept any or skip validation entirely, and the fallout is always the same: malformed data gets deeper into the system than it should.
Here's what I expect to see:
import { IsEmail, IsString, IsNotEmpty, MinLength, IsOptional } from 'class-validator';
export class CreateUserDto {
@IsEmail()
email: string;
@IsString()
@IsNotEmpty()
@MinLength(8)
password: string;
@IsString()
@IsOptional()
firstName?: string;
@IsString()
@IsOptional()
lastName?: string;
}
Then in the controller, I verify the DTO is actually used:
@Post('users')
@HttpCode(HttpStatus.CREATED)
async createUser(@Body() createUserDto: CreateUserDto) {
return this.userService.create(createUserDto);
}
The key here is that NestJS's built-in validation pipe automatically validates the shape and constraints before the handler runs. No sneaking past with any, no hand-rolling regex checks. One thing I watch for: custom validators. If a field needs domain-specific validation (like checking if an email is already registered), I want that in a custom validator decorator, not scattered in the service.
@ValidatorConstraint({ name: 'isEmailUnique', async: true })
@Injectable()
export class IsEmailUniqueConstraint implements ValidatorConstraintInterface {
constructor(private readonly userService: UserService) {}
async validate(email: string): Promise {
const existing = await this.userService.findByEmail(email);
return !existing;
}
defaultMessage(): string {
return 'Email is already registered';
}
}
Authorization — Guard plus permission check. Ownership verified for resources.
Authentication isn't the same as authorization, and I check both meticulously. A JWT guard proves you are who you say you are, but does it verify you can actually do what you're asking to do?
I look for guards on protected routes:
@UseGuards(JwtAuthGuard)
@Get('posts/:id')
async getPost(@Param('id') postId: string, @Request() req) {
return this.postService.getPost(postId);
}
But the real question is whether ownership is verified. If I'm fetching a post, I need to confirm the post actually belongs to me or is public. This is where it gets tricky:
@UseGuards(JwtAuthGuard)
@Patch('posts/:id')
async updatePost(
@Param('id') postId: string,
@Body() updatePostDto: UpdatePostDto,
@Request() req
) {
const post = await this.postService.findById(postId);
if (post.userId !== req.user.id) {
throw new ForbiddenException('You cannot modify this post');
}
return this.postService.update(postId, updatePostDto);
}
I've seen pull requests where this check was missing entirely, or worse, where it was only done on specific operations. For resources with ownership semantics, every read/write that isn't explicitly public needs that verification. I also prefer moving this into a custom guard or using a resolver that handles it automatically, so it's not left to chance:
@Injectable()
export class PostOwnershipGuard implements CanActivate {
constructor(
private readonly postService: PostService,
private readonly reflector: Reflector,
) {}
async canActivate(context: ExecutionContext): Promise {
const request = context.switchToHttp().getRequest();
const postId = request.params.id;
const userId = request.user.id;
const post = await this.postService.findById(postId);
if (!post) throw new NotFoundException();
return post.userId === userId;
}
}
Data access — No N+1, selects limited, transactions where needed.
Performance problems hide in plain sight during code review. I always check the query patterns, especially in services where data is loaded.
N+1 is the classic culprit. Here's what I don't want to see:
async getUsersWithPosts(userId: string) {
const users = await this.usersRepository.find({ where: { id: userId } });
// This loops and fires a query per user
for (const user of users) {
user.posts = await this.postsRepository.find({ where: { userId: user.id } });
}
return users;
}
Instead, eager load with relations:
async getUsersWithPosts(userId: string) {
return this.usersRepository.find({
where: { id: userId },
relations: ['posts'],
});
}
I also check column selection. Fetching entire entities when you only need an ID or a name wastes memory and bandwidth:
async getUserId(email: string): Promise {
const user = await this.usersRepository.findOne({
where: { email },
select: ['id'], // Only fetch ID
});
return user?.id;
}
Transactions matter when multiple operations must succeed or fail together. If a payment service charges a card and then updates an order status in two separate queries, a crash between them leaves data corrupt:
async processOrder(orderId: string) {
const queryRunner = this.dataSource.createQueryRunner();
await queryRunner.connect();
await queryRunner.startTransaction();
try {
await queryRunner.manager.update(Order, { id: orderId }, { status: 'processing' });
await this.paymentService.charge(orderId);
await queryRunner.commitTransaction();
} catch (error) {
await queryRunner.rollbackTransaction();
throw error;
} finally {
await queryRunner.release();
}
}
Errors — Domain errors mapped, no stack traces leaked.
How you handle errors tells me a lot about whether this code is ready for production. Every error response that leaves your API should be intentional and sanitized.
I expect custom domain exceptions, not generic errors:
export class InsufficientFundsException extends BadRequestException {
constructor(available: number, required: number) {
super(`Insufficient funds. Available: ${available}, Required: ${required}`);
}
}
export class UserAlreadyExistsException extends ConflictException {
constructor(email: string) {
super(`User with email ${email} already exists`);
}
}
Then map these cleanly in an exception filter:
@Catch()
export class GlobalExceptionFilter implements ExceptionFilter {
catch(exception: unknown, host: ArgumentsHost) {
const ctx = host.switchToHttp();
const response = ctx.getResponse();
if (exception instanceof HttpException) {
return response.status(exception.getStatus()).json({
statusCode: exception.getStatus(),
message: exception.getResponse(),
});
}
// Never expose stack trace to client
console.error(exception); // Log it server-side
response.status(HttpStatus.INTERNAL_SERVER_ERROR).json({
statusCode: 500,
message: 'Internal server error',
});
}
}
The critical rule: stack traces stay in logs, not responses. A leaked stack trace is a free attack surface for someone trying to exploit your system.
Tests — Business rules unit-tested; critical flows covered end to end.
I don't expect 100% code coverage, but I do expect business logic to be tested. A utility function that formats dates doesn't need tests. A calculation that determines if a user can withdraw funds absolutely does.
Unit tests for services:
describe('AccountService', () => {
let service: AccountService;
let repository: Repository;
beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
providers: [
AccountService,
{
provide: getRepositoryToken(Account),
useValue: {
findOne: jest.fn(),
save: jest.fn(),
},
},
],
}).compile();
service = module.get(AccountService);
repository = module.get>(getRepositoryToken(Account));
});
it('should throw when withdrawing more than available balance', async () => {
const account = { balance: 100 };
(repository.findOne as jest.Mock).mockResolvedValue(account);
await expect(
service.withdraw('acc-1', 150),
).rejects.toThrow(InsufficientFundsException);
});
it('should deduct amount and save when balance is sufficient', async () => {
const account = { id: 'acc-1', balance: 500 };
(repository.findOne as jest.Mock).mockResolvedValue(account);
(repository.save as jest.Mock).mockResolvedValue({ ...account, balance: 450 });
const result = await service.withdraw('acc-1', 50);
expect(result.balance).toBe(450);
expect(repository.save).toHaveBeenCalledWith(expect.objectContaining({ balance: 450 }));
});
});
End-to-end tests for critical flows (like payments, authentication, or data export):
describe('Payment Flow (e2e)', () => {
it('should create order, charge card, and update status atomically', async () => {
const response = await request(app.getHttpServer())
.post('/orders')
.set('Authorization', `Bearer ${validToken}`)
.send({ amount: 5000, cardId: 'card-123' })
.expect(HttpStatus.CREATED);
const orderId = response.body.id;
const order = await orderRepository.findOne({ where: { id: orderId } });
expect(order.status).toBe('completed');
expect(order.paidAmount).toBe(5000);
});
});
Common mistakes
- Skipping DTO validation on "simple" endpoints. Every input is an opportunity for bad data. No exceptions.
- Mixing authentication and authorization. A JWT guard doesn't prove permission. Verify ownership and roles explicitly.
- Loading entire entities when you only need a field. Use
selectto be specific. It compounds at scale. - Catching all exceptions with a generic handler. You lose information about what actually failed. Be specific.
- Skipping tests for "obvious" logic. The business rules are where bugs live. Test them.
- Leaking stack traces in error responses. It's an attack surface. Log server-side, respond with a message.
Key takeaways
- Validation is your first line of defense. Use DTOs with decorators, never
any. - Authorization is separate from authentication. Verify ownership and permissions on every protected operation.
- Query patterns matter. Watch for N+1, use eager loading, and select only what you need.
- Handle errors intentionally. Map domain exceptions and never expose stack traces to clients.
- Test business rules and critical flows. Unit tests for logic, e2e tests for journeys.
- Code review is about catching what tests miss—ownership checks, transaction boundaries, and error handling edge cases.
Comments
Loading comments…