Skip to content

feat: NestJS Task Manager API 초기 구현 (코드리뷰 비교용)#1

Open
Wade-Gabia wants to merge 5 commits into
developfrom
feat/task-manager-api
Open

feat: NestJS Task Manager API 초기 구현 (코드리뷰 비교용)#1
Wade-Gabia wants to merge 5 commits into
developfrom
feat/task-manager-api

Conversation

@Wade-Gabia

Copy link
Copy Markdown
Owner

Summary

  • NestJS + TypeScript 기반 Task Manager API 샘플 프로젝트
  • 코드리뷰 도구 3종(자체 플러그인, Claude, Alibaba) 비교용 테스트 대상

모듈 구성

  • User 모듈: 기본 CRUD (정상 구현)
  • Project 모듈: CRUD + 의도적 N+1 쿼리, 에러핸들링 누락
  • Task 모듈: CRUD + 의도적 N+1 쿼리, DTO 검증 미흡, 인덱스 누락, 에러핸들링 누락

의도적 결함 목록 (리뷰 비교 포인트)

위치 결함 유형
tasks.controller.ts ValidationPipe 없음, 404 미처리, update/delete 에러 미처리
projects.controller.ts null 반환 404 없음, delete 에러 미처리
tasks.service.ts N+1 assignee 루프 조회
projects.service.ts N+1 taskCount 루프 조회
create-task.dto.ts @IsNotEmpty() 누락, dueDate: any
update-task.dto.ts PartialType 미사용, projectId 노출
task.entity.ts status, assigneeId 인덱스 누락

@Wade-Gabia Wade-Gabia left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — NestJS Task Manager API

10개 findings (severity 순, 모두 CONFIRMED)

# 심각도 위치 요약
1 🔴 Critical tasks.service.ts:46 update() null 반환 → 200 OK
2 🔴 Critical tasks.controller.ts:22 findOne null → 200 OK (404 없음)
3 🔴 Critical projects.controller.ts:20 findOne null → 200 OK (404 없음)
4 🔴 Critical tasks.controller.ts:34 존재하지 않는 id 삭제 → 200 OK
5 🔴 Critical main.ts:5 ValidationPipe 미등록 — 모든 DTO 검증 무력화
6 🟠 High update-task.dto.ts:22 projectId 노출 — 무단 task 이동 가능
7 🟠 High app.module.ts:20 synchronize: true — 프로덕션 컬럼 DROP 위험
8 🟡 Medium create-task.dto.ts:25 dueDate: any — DB 타입 에러 유발 가능
9 🟡 Medium projects.service.ts:18 N+1 쿼리 — 프로젝트 수만큼 DB 쿼리 발생
10 🟡 Medium tasks.service.ts:21 N+1 쿼리 — 태스크 수만큼 DB 쿼리 발생

인라인 코멘트 참고.

});
}

async update(id: number, dto: UpdateTaskDto): Promise<Task> {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] update()가 null을 반환해 컨트롤러가 HTTP 200으로 응답

taskRepository.update(id, dto)는 존재하지 않는 id에 대해 조용히 no-op (affected rows = 0)합니다. 이후 findOne({ where: { id } })null을 반환하고, 컨트롤러는 이를 그대로 HTTP 200으로 응답합니다.

// PATCH /tasks/9999 → 200 OK, body: null
// 클라이언트는 성공/실패를 구분할 수 없음

// 수정:
async update(id: number, dto: UpdateTaskDto): Promise<Task> {
  const task = await this.taskRepository.findOne({ where: { id } });
  if (!task) throw new NotFoundException(`Task #${id} not found`);
  await this.taskRepository.update(id, dto);
  return this.taskRepository.findOne({ where: { id } });
}

}

// 결함: null 반환 시 404 처리 없음
@Get(':id')

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] 존재하지 않는 Task 조회 시 404 대신 200 null 반환

tasksService.findOne()는 row가 없으면 null을 반환합니다 (users.service.ts와 달리 NotFoundException을 throw하지 않음). 컨트롤러가 이를 그대로 반환하면 NestJS는 HTTP 200에 빈 body로 응답합니다.

// 수정: service에서 처리
async findOne(id: number): Promise<Task> {
  const task = await this.taskRepository.findOne({ where: { id }, relations: ['assignee', 'project'] });
  if (!task) throw new NotFoundException(`Task #${id} not found`);
  return task;
}

}

// 결함: 존재하지 않는 id 삭제 시 204만 반환, 실제 삭제 여부 확인 없음
@Delete(':id')

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] 존재하지 않는 Task 삭제 시 200 OK 반환 (silent no-op)

taskRepository.delete(id)는 해당 row가 없어도 에러를 던지지 않습니다 (DeleteResult.affected = 0). 서비스는 이를 무시하고 void 반환, 컨트롤러는 HTTP 200으로 응답합니다.

// 수정:
async remove(id: number): Promise<void> {
  const result = await this.taskRepository.delete(id);
  if (result.affected === 0) throw new NotFoundException(`Task #${id} not found`);
}

Comment thread src/main.ts
import { AppModule } from './app.module';

async function bootstrap() {
const app = await NestFactory.create(AppModule);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] GlobalValidationPipe 미등록 — 모든 DTO validator가 무력화됨

app.useGlobalPipes(new ValidationPipe())가 없어서 CreateTaskDto, UpdateTaskDto의 모든 class-validator 데코레이터(@IsString, @IsEnum, @IsNumber 등)가 런타임에 전혀 동작하지 않습니다. 어떤 body라도 그대로 통과합니다.

// 수정:
async function bootstrap() {
  const app = await NestFactory.create(AppModule);
  app.useGlobalPipes(new ValidationPipe({ whitelist: true, forbidNonWhitelisted: true }));
  await app.listen(3000);
}

@Wade-Gabia Wade-Gabia left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인라인 코멘트 (findings 3, 6-10)

}

// 결함: findOne이 null을 반환해도 404를 던지지 않음
@Get(':id')

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] 존재하지 않는 Project 조회 시 404 대신 200 null 반환

projectsService.findOne()가 null을 반환해도 컨트롤러는 그대로 HTTP 200으로 응답합니다.

// 수정: service에서 처리
async findOne(id: number): Promise<Project> {
  const project = await this.projectRepository.findOne({ where: { id }, relations: ['tasks', 'owner'] });
  if (!project) throw new NotFoundException(`Project #${id} not found`);
  return project;
}

@IsNumber()
assigneeId?: number;

// 결함: projectId는 변경 불가여야 하는데 수정 가능하도록 노출됨

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High] projectId 노출 — 무단 task 프로젝트 이동 가능

UpdateTaskDtoprojectId가 optional 필드로 노출되어 있어 PATCH /tasks/:id { projectId: 99 }만으로 태스크를 다른 프로젝트로 이동할 수 있습니다. taskRepository.update(id, dto)는 DTO를 필터링 없이 그대로 전달합니다.

수정: DTO에서 projectId 제거하거나, 서비스에서 명시적으로 제외:

const { projectId, ...safeDto } = dto;
await this.taskRepository.update(id, safeDto);

Comment thread src/app.module.ts
password: 'password',
database: 'task_manager',
entities: [User, Project, Task],
synchronize: true,

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High] synchronize: true — 프로덕션에서 컬럼 DROP 위험

TypeORM의 synchronize: true는 앱 시작 시마다 엔티티 정의와 실제 DB 스키마를 비교해 자동으로 ALTER TABLE / DROP COLUMN을 실행합니다. 프로덕션 배포 시 데이터 손실이 발생할 수 있습니다.

// 수정: 환경변수로 제어
synchronize: process.env.NODE_ENV !== 'production',
// 또는 TypeORM Migration 사용

assigneeId?: number;

// 결함: dueDate 타입이 any — Date 검증 없음
@IsOptional()

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] dueDate 타입이 any — 잘못된 값이 DB에 그대로 저장됨

dueDate?: any"not-a-date", 0, {} 등 어떤 값도 통과시킵니다. DB DATE 컬럼 타입 불일치로 runtime 에러가 발생하거나 잘못된 데이터가 저장됩니다.

// 수정:
@IsOptional()
@IsDateString()
dueDate?: string;

return this.projectRepository.save(project);
}

// 결함: 프로젝트 목록을 가져온 뒤 각 프로젝트의 task 수를 루프에서 별도 쿼리로 조회 (N+1)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] N+1 쿼리 — 프로젝트 N개일 때 N+1번 DB 쿼리 발생

projectRepository.find()로 전체 프로젝트를 가져온 뒤, 각 프로젝트마다 별도 쿼리로 task count를 조회합니다. 프로젝트 500개 → 501번 쿼리.

// 수정: 단일 쿼리로 처리
async findAll() {
  return this.projectRepository
    .createQueryBuilder('project')
    .loadRelationCountAndMap('project.taskCount', 'project.tasks')
    .getMany();
}

}

// 결함: 태스크 목록 조회 후 각 태스크의 assignee를 루프에서 별도 쿼리로 조회 (N+1)
async findAll(): Promise<any[]> {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Medium] N+1 쿼리 — 태스크 N개일 때 N+1번 DB 쿼리 발생

taskRepository.find()로 태스크를 로드한 뒤, assigneeId가 있는 태스크마다 manager.findOne(User, ...)을 별도 호출합니다. 또한 dynamic require()는 TypeScript 모듈 시스템을 우회합니다.

// 수정: relations 옵션 사용
async findAll() {
  return this.taskRepository.find({ relations: ['assignee'] });
}

@Wade-Gabia

Copy link
Copy Markdown
Owner Author

@claude review once

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant