Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add: logging middleware #135

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
258 changes: 256 additions & 2 deletions package-lock.json

Large diffs are not rendered by default.

6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,10 @@
"passport-jwt": "^4.0.1",
"reflect-metadata": "^0.1.13",
"rimraf": "^3.0.2",
"rxjs": "^7.2.0"
"rxjs": "^7.2.0",
"uuid": "^10.0.0",
"winston": "^3.14.2",
"winston-daily-rotate-file": "^5.0.0"
},
"devDependencies": {
"@nestjs/cli": "^9.0.0",
Expand All @@ -88,6 +91,7 @@
"@types/jest": "28.1.4",
"@types/node": "^16.18.23",
"@types/supertest": "^6.0.2",
"@types/uuid": "^10.0.0",
"@typescript-eslint/eslint-plugin": "^5.0.0",
"@typescript-eslint/parser": "^5.0.0",
"cross-env": "^7.0.3",

Choose a reason for hiding this comment

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

코드 패치에 대한 간략한 리뷰를 하겠습니다.

  1. 버전 호환성: 새로운 패키지(uuid, winston, winston-daily-rotate-file)의 버전이 기존 프로젝트 의존성과 잘 맞는지 확인해야 합니다. 특히 winston과 관련된 패키지는 서로 호환성을 검토하는 것이 좋습니다.

  2. @types 추가: @types/uuid를 추가한 것은 좋지만, 나머지 새로 추가된 패키지에도 타입 정의가 필요한 경우, 해당 패키지의 타입을 추가하는지 확인하십시오.

  3. 사용하지 않는 의존성: 추가된 패키지가 실제 코드에서 사용되는지 점검해야 합니다. 필요 없는 의존성이 포함되지 않도록 주의하세요.

  4. 패키지 보안: uuid, winston 등 새로 추가된 패키지의 보안 취약점 여부를 확인할 필요가 있습니다. 정기적으로 보안 스캐닝 도구를 활용하는 것을 추천합니다.

  5. 문서화: 새로 추가된 패키지의 사용법에 대한 문서를 업데이트해야 다른 개발자들이 이해하는 데 도움이 됩니다.

이 외에도 전반적인 테스트 케이스가 잘 작성되어 있는지 점검하는 것도 중요합니다.

Expand Down
3 changes: 2 additions & 1 deletion src/app.module.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Module } from '@nestjs/common';
import { MiddlewareConsumer, Module, NestModule } from '@nestjs/common';

Check warning on line 1 in src/app.module.ts

View workflow job for this annotation

GitHub Actions / Lint

'MiddlewareConsumer' is defined but never used

Check warning on line 1 in src/app.module.ts

View workflow job for this annotation

GitHub Actions / Lint

'NestModule' is defined but never used
import { APP_GUARD } from '@nestjs/core';
import { JwtService } from '@nestjs/jwt';
import { AppController } from './app.controller';
Expand Down Expand Up @@ -29,6 +29,7 @@
import { ClsPluginTransactional } from '@nestjs-cls/transactional';
import { PrismaService } from '@src/prisma/prisma.service';
import { TransactionalAdapterPrisma } from '@nestjs-cls/transactional-adapter-prisma';
// import { LoggingMiddleware } from "@src/common/middleware/http.logging.middleware";

@Module({
imports: [

Choose a reason for hiding this comment

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

코드 패치에 대한 간단한 검토를 하겠습니다.

개선 제안 및 버그 위험

  1. 미사용 코드 주석 처리:

    • LoggingMiddleware 임포트 문이 주석 처리되어 있는데, 현재 코드에서 사용되지 않는 경우 지워버리는 것이 깔끔합니다. 불필요한 코드가 프로젝트 관리에 혼란을 줄 수 있습니다.
  2. NestModule 구현:

    • NestModule 인터페이스가 추가되었으나, 실제로 이를 구현하는 코드가 없습니다. 필요하다면 configure() 메서드를 추가하여 미들웨어를 설정할 수 있습니다. 그렇지 않으면 불필요한 의존성이 될 수 있습니다.
  3. 주석의 용도:

    • 주석으로 남겨진 코드는 나중에 사용할 계획이 없는 것이라면 삭제하는 게 좋습니다. 주석은 종종 혼란을 초래할 수 있기 때문입니다.
  4. 모듈 구조:

    • 모듈 내 의존성을 명확히 하고, 필요하지 않은 모듈은 제거하도록 검토해야 합니다. 예를 들어, ClsModule이나 PrismaService의 목적이 명확하지 않다면 검토해보는 것이 좋습니다.
  5. 코드 일관성:

    • 전체적인 코드 스타일과 일관성을 유지하는 것이 중요합니다. 다른 파일들과 비교하여 변화를 적용하는 것이 좋습니다.

이러한 점들을 고려하여 코드를 개선하면 더 견고하고 이해하기 쉬운 모듈을 만들 수 있을 것입니다.

Expand Down Expand Up @@ -83,7 +84,7 @@
{
provide: APP_GUARD,
useFactory: () => {
const env = process.env.NODE_ENV;

Check warning on line 87 in src/app.module.ts

View workflow job for this annotation

GitHub Actions / Lint

'env' is assigned a value but never used
},
},
JwtCookieGuard,
Expand Down
4 changes: 4 additions & 0 deletions src/bootstrap/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import session from 'express-session';
import { AppModule } from '../app.module';
import settings from '../settings';
import morgan = require('morgan');
import { LoggingInterceptor } from '@src/common/middleware/http.logging.middleware';
import { HttpExceptionFilter } from '@src/common/filter/http.exception.filter';

async function bootstrap() {
const app = await NestFactory.create(AppModule);
Expand Down Expand Up @@ -53,6 +55,8 @@ async function bootstrap() {
}),
);

app.useGlobalInterceptors(new LoggingInterceptor());
app.useGlobalFilters(new HttpExceptionFilter());
app.enableShutdownHooks();
return app.listen(8000);
}

Choose a reason for hiding this comment

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

코드 리뷰를 위해 제공된 패치 내용을 살펴보았습니다. 다음은 몇 가지 버그 위험 요소와 개선 제안입니다:

  1. 버그 위험:

    • LoggingInterceptorHttpExceptionFilter가 제대로 작동하기 위해서는 이들이 올바르게 구현되어 있어야 합니다. 해당 클래스들이 의존하는 외부 모듈이나 설정이 올바르게 구성되어 있는지 확인해야 합니다.
  2. 성능:

    • 글로벌 인터셉터 및 필터를 추가함에 따라 애플리케이션의 성능에 영향을 줄 수 있습니다. 실제로 로그를 남기거나 예외를 처리할 때 성능 저하가 발생하지 않도록 주의해야 합니다.
  3. 예외 처리 개선:

    • HttpExceptionFilter가 전역적으로 설정되었으므로, 필터 내부에서 모든 에러를 카운트하거나 로깅하는 로직을 추가하여 문제 해결에 도움이 될 수 있습니다.
  4. 테스트:

    • 새로 추가된 인터셉터와 필터에 대한 유닛 테스트를 작성하여 그 동작을 검증하는 것이 좋습니다. 실행 중인 애플리케이션에서의 예상치 못한 동작을 방지할 수 있습니다.
  5. 주석 추가:

    • 코드의 의도를 명확히 하기 위해 주요 변경 사항에 대한 주석을 추가하면 향후 유지보수에 도움이 될 것입니다.
  6. 환경 변수 관리:

    • 포트 번호(8000)가 하드 코드되어 있는데, 환경 변수를 사용해 더 유연하게 처리하는 방법을 고려해 볼 수 있습니다.

이러한 점들을 참고하여 코드를 보완하면 보다 안정적이고 효율적인 애플리케이션이 될 것입니다.

Expand Down
93 changes: 93 additions & 0 deletions src/common/filter/http.exception.filter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import {
ExceptionFilter,
Catch,
ArgumentsHost,
HttpException,
HttpStatus,
} from '@nestjs/common';
import { Request, Response } from 'express';
import * as winston from 'winston';
import 'winston-daily-rotate-file';
import { v4 as uuidv4 } from 'uuid';

// JSON 포맷을 커스터마이징하여 메시지와 속성 순서를 설정
const customFormat = winston.format.printf(({ message }) => {
return JSON.stringify(message);
});

// 파일 핸들러 설정
const fileTransport = new winston.transports.DailyRotateFile({
filename: 'logs/response-%DATE%.log', // 파일 이름 패턴
datePattern: 'YYYY-MM-DD', // 날짜 패턴
zippedArchive: true, // 압축 여부
maxSize: '10m', // 파일 최대 크기
maxFiles: '10d', // 백업 파일 수
handleExceptions: true, // 예외 처리
format: winston.format.combine(customFormat),
});

const logger = winston.createLogger({
transports: [fileTransport],
exitOnError: false, // 예외 발생 시 프로세스 종료하지 않음
});

@Catch(HttpException)
export class HttpExceptionFilter implements ExceptionFilter {
catch(exception: HttpException, host: ArgumentsHost) {
const ctx = host.switchToHttp();
const response = ctx.getResponse();
const request = ctx.getRequest();
const status = exception.getStatus
? exception.getStatus()
: HttpStatus.INTERNAL_SERVER_ERROR;

const startTime = Date.now();
const requestId = request.headers['uuid'] || uuidv4();

const { method, headers, query } = request;

// 요청 데이터 처리
let requestData = {};
try {
if (method === 'GET') {
requestData = { ...query };
} else if (method === 'POST') {
requestData = { ...request.body };
} else {
requestData = { ...request.body };
}
} catch (error) {
requestData = {}; // 예외 발생 시 빈 객체로 처리
}
const requestLog = {
method,
path: request.path, // URI만 포함하도록 수정
UUID: requestId,
data: requestData,
user: request?.user?.sid ?? '',
// meta,
};

// 예외 로그 기록
const errorResponseLog = {
status: status,
data: response?.body ?? '',
};

// 전체 로그 객체
const logData = {
request: requestLog,
response: errorResponseLog,
};

// 로그 출력
logger.error(logData);

// 응답 생성
response.status(status).json({
statusCode: status,
timestamp: new Date().toISOString(),
path: request.url,
});
}
}

Choose a reason for hiding this comment

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

코드 리뷰 결과는 다음과 같습니다:

  1. 예외 처리:

    • requestData를 처리할 때, method가 'PUT' 또는 'DELETE'와 같은 다른 HTTP 메소드에 대한 처리가 누락되었습니다. 해당 경우도 고려하여 로직을 추가하는 것이 좋습니다.
  2. 로깅 정보:

    • response.body를 사용하고 있으나, Express의 응답 객체에는 body 프로퍼티가 존재하지 않습니다. 제대로 된 오류 메시지를 기록하기 위해서는 예외에서 발생한 내용을 대신 로깅해야 합니다.
  3. UUID 생성:

    • 요청 헤더에서 UUID를 가져오고 없는 경우 새로 생성하는 방식은 괜찮지만, 실패한 예외에 대해서만 이런 UUID를 사용하는 것도 한 방법입니다. 더 명확한 트래킹을 위해 이 방식을 고민해볼 수 있습니다.
  4. 변수명 일관성:

    • requestLogerrorResponseLog와 같이 변수명을 좀 더 직관적으로 직무에 맞게 지정하면 가독성이 향상될 것입니다. 예를 들면, errorResponseLogexceptionLog로 변경할 수 있습니다.
  5. 불필요한 주석:

    • // meta와 같은 불필요한 주석은 삭제하세요. 이는 코드 가독성을 떨어뜨립니다.
  6. 로그 형식:

    • JSON으로 로그를 작성하고 있지만, customFormat에서 단순히 문자열 변환을 통해 출력하는 것은 가독성을 낮출 수 있습니다. 필요한 필드를 명시적으로 추출하여 logs 쿼리를 개선할 수 있습니다.
  7. 성능:

    • 매 호출마다 UUID를 생성하는 것은 성능 저하를 초래할 수 있습니다. Kubernetes 등 환경에서도 고유한 로그를 위한 ID 생성 전략을 신중히 고려하세요.
  8. 타입 안전:

    • request?.user?.sid ?? ''와 같은 옵셔널 체이닝은 자바스크립트의 버전을 고려하여 잘 사용하세요. 모든 서버 환경에서 지원되지 않을 수 있습니다.

이 외에도 전반적으로 구조가 잘 잡혀있고, 기본적인 에러 핸들링 로직은 적절합니다. 그러나 위 사항들을 고려하여 다듬으면 더욱 안정성과 가독성을 높일 수 있을 것입니다.

Choose a reason for hiding this comment

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

이 코드 패치는 NestJS의 예외 필터를 사용하여 HTTP 예외를 처리하는 기능을 추가합니다. 전반적으로 잘 구성되어 있지만 몇 가지 주의해야 할 점과 개선 사항이 있습니다.

잠재적인 버그 리스크

  1. UUID 처리: requestIdrequest.headers['uuid']에서 가져오는데, 이 값을 확인하지 않고 바로 사용합니다. 존재하지 않을 경우 undefined가 될 수 있으므로, 로그에 명시적으로 "not provided"와 같은 기본값을 설정하는 것이 좋습니다.

  2. 요청 데이터 처리: 메서드별로 요청 데이터를 처리하긴 하지만, PUT이나 DELETE 등의 메서드를 다루지 않고 있습니다. 이러한 메서드도 로그에 포함할 필요가 있을 수 있습니다.

  3. 응답 데이터: response.body에서 예외 발생 시 응답 데이터를 가져오고 있는데, 이는 일반적으로 존재하지 않습니다. 대신 예외 객체에서 메시지를 추출하는 것이 좋습니다.

개선 제안

  1. 일관된 데이터 포맷: 모든 요청 메서드(GET, POST 등)에서 요청 본문을 처리하는 부분을 통합할 수 있습니다. 함수로 분리하여 유지보수를 쉽게 할 수 있습니다.

  2. 로그 레벨 조정: 현재 오류 로그(logger.error)만 출력하고 있는데, 다양한 상황에 따라 정보 로그(logger.info)나 경고 로그(logger.warn)도 사용할 수 있도록 더 세분화할 수 있습니다.

  3. 타입 정의: requestData와 같은 변수에 대한 타입을 명시하면 코드 가독성이 향상될 수 있습니다. TypeScript의 장점을 활용하여 더욱 엄격한 타입 체크를 적용하세요.

  4. 예외 처리 추가: 예외 필터 내에서 예외가 발생했을 때(예: JSON 파싱 오류 등)를 처리할 방법을 추가하십시오. 이를 통해 안정성을 높일 수 있습니다.

  5. 환경별 로깅 설정: 프로덕션 환경에서는 로그 레벨이나 저장 방식 등을 변경할 수 있도록 환경에 따라 다르게 설정할 수 있는 구조로 만드는 것도 고려해 볼 만합니다.

위 사항들을 반영하면 코드의 안정성과 가독성을 높이는 데 도움이 될 것입니다.

145 changes: 145 additions & 0 deletions src/common/middleware/http.logging.middleware.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
import {
CallHandler,
ExecutionContext,
Injectable,
NestInterceptor,
HttpException,
HttpStatus,
} from '@nestjs/common';
import { Observable, throwError } from 'rxjs';
import { tap, catchError } from 'rxjs/operators';
import { v4 as uuidv4 } from 'uuid';
import * as winston from 'winston';
import 'winston-daily-rotate-file';

// JSON 포맷을 커스터마이징하여 메시지와 속성 순서를 설정
const customFormat = winston.format.printf(({ message }) => {
return JSON.stringify(message);
});

// 파일 핸들러 설정
const fileTransport = new winston.transports.DailyRotateFile({
filename: 'logs/response-%DATE%.log', // 파일 이름 패턴
datePattern: 'YYYY-MM-DD', // 날짜 패턴
zippedArchive: true, // 압축 여부
maxSize: '10m', // 파일 최대 크기
maxFiles: '10d', // 백업 파일 수
handleExceptions: true, // 예외 처리
format: winston.format.combine(customFormat),
});

export const logger = winston.createLogger({
transports: [fileTransport],
exitOnError: false, // 예외 발생 시 프로세스 종료하지 않음
});

@Injectable()
export class LoggingInterceptor implements NestInterceptor {
intercept(context: ExecutionContext, next: CallHandler): Observable<any> {
const req = context.switchToHttp().getRequest();
const res = context.switchToHttp().getResponse();

const startTime = Date.now();
const requestId = req.headers['uuid'] || uuidv4();

const { method, headers, query } = req;

// 요청 데이터 처리
let requestData = {};
try {
if (method === 'GET') {
requestData = { ...query };
} else if (method === 'POST') {
requestData = { ...req.body };
} else {
requestData = { ...req.body };
}
} catch (error) {
requestData = {}; // 예외 발생 시 빈 객체로 처리
}

const meta = {
tz: new Date().toLocaleString('en-US', {
timeZone: 'Asia/Seoul',
timeZoneName: 'short',
}),
remote_host: req.hostname,
content_length: headers['content-length'] || '',
path_info: req.path,
remote_addr: req.ip,
content_type: headers['content-type'] || '',
http_host: headers['host'] || '',
http_user_agent: headers['user-agent'] || '',
};

const requestLog = {
method,
path: req.path, // URI만 포함하도록 수정
UUID: requestId,
data: requestData,
user: req?.user?.sid ?? '',
// meta,
};

return next.handle().pipe(
tap((responseBody) => {
res.on('finish', () => {
const duration = Date.now() - startTime;
const { statusCode } = res;

const responseLog = {
status: statusCode,
// headers: {
// 'Content-Type': res.getHeader('Content-Type') || '',
// // ...res.getHeaders(), // 모든 응답 헤더를 포함
// },
// charset: res.charset || 'utf-8',
data: responseBody, // Response Body 저장
};

// request와 response를 하나의 객체로 묶어서 로깅
const logData = {
request: requestLog,
response: responseLog,
duration: duration,
};

logger.info(logData);
});
}),
catchError((err) => {
res.on('finish', () => {
const duration = Date.now() - startTime;
const statusCode =
err instanceof HttpException
? err.getStatus()
: HttpStatus.INTERNAL_SERVER_ERROR;

const errorResponseLog = {
status: statusCode,
headers: {
'Content-Type': res.getHeader('Content-Type') || '',
...res.getHeaders(), // 모든 응답 헤더를 포함
},
charset: res.charset || 'utf-8',
data: {
message: err.message || 'Internal Server Error',
stack: err.stack || '', // Stack trace 포함 (원하는 경우)
},
duration: `${duration}ms`,
};

// 예외 발생 시에도 request는 그대로 남기고, error response를 로깅
const logData = {
request: requestLog,
response: errorResponseLog,
};

logger.error(logData);
});

return throwError(() => err);
}),
);
}
}

Choose a reason for hiding this comment

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

코드 리뷰 결과는 다음과 같습니다:

장점:

  1. 명확한 역할: Middleware가 HTTP 요청 로그를 기록하는 역할을 명확히 하고 있습니다.
  2. Logger 사용: NestJS의 Logger 클래스를 활용하여 일관된 로그 형식을 제공합니다.

잠재적 문제:

  1. Content-Length 처리: content-length 헤더가 비어있을 경우 로그에 null 또는 undefined가 나타날 수 있습니다. 이를 기본값으로 처리하면 좋습니다.

    const contentLength = response.get('content-length') || '0';
  2. 비동기 로깅: 현재 로그를 기록하는 과정이 비동기적으로 요청이 끝날 때까지 기다립니다. 응답이 완료되는 시점에서 메시지를 기록하기 때문에, 간혹 동시성이 문제가 될 수 있는 고부하 환경에서는 로그가 유실될 가능성이 있습니다. 일반적으로, 로그는 finish 이벤트로 처리하는 것이 더 안전합니다.

개선 제안:

  • 포맷팅 개선: 로그 출력을 좀 더 읽기 쉽게 포맷팅할 수 있습니다. 예를 들어, JSON 형식으로 묶어서 출력하는 방법을 고려할 수 있습니다.
this.logger.log(JSON.stringify({
  method,
  url,
  statusCode,
  contentLength,
  userAgent,
  ip,
}));
  • 에러 핸들링: 가능하면 에러 상황도 로그에 포함시키는 것으로 확장하면 유용할 것입니다. 예를 들어 유효하지 않은 요청 시 로그를 남기는 것도 좋은 접근입니다.

이와 같은 점들을 개선하면 더욱 안정적이고 사용자 친화적인 로그 시스템이 될 것입니다.

Choose a reason for hiding this comment

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

코드 리뷰 결과는 다음과 같습니다:

버그 리스크

  1. req.body 처리: POST 외의 메소드에 대한 처리가 모두 req.body로 설정되어 있습니다. 이 부분은 다른 HTTP 메소드에 대해 적절한 로그를 남기지 못할 수 있습니다.
  2. requestId 처리: 기본값으로 UUID 생성이 되어 있지만, 만약 클라이언트가 유효한 uuid를 제공하지 않을 경우 여전히 UUID가 생략될 수 있습니다.
  3. res.on('finish') 사용: 모든 파이프라인에서 finish 이벤트를 등록하고 있으며, 이는 여러 번 호출될 위험이 있습니다. 단일 호출로 제한하는 로직이 필요합니다.

개선 제안

  1. 로깅 포맷 통일화: 요청 및 응답 로깅에서 사용하는 필드들을 일관되게 구성하면 나중에 분석하기 용이합니다.
  2. 예외 처리 향상: 예외가 발생했을 때, 클라이언트에게 더 구체적인 정보를 보내기 위해 res.status().json()을 활용할 수 있습니다.
  3. 메타데이터 포함: 요청의 메타데이터를 더 추가하여 나중에 로그를 통해 유용한 정보(예: 사용자 IP)에 접근하기 쉽게 할 수 있습니다.
  4. 리듀서 추가: 요청 데이터를 간단하게 정리해서 로그에 남길 수 있도록 리듀서를 구현하면 가독성이 개선됩니다.

총평

로깅 인터셉터로써 기능은 잘 유지되고 있으나, 약간의 예외 처리와 코드 정리가 필요합니다. 전반적으로 안정성과 가독성을 높이는 방향으로 개선할 수 있을 것입니다.

Choose a reason for hiding this comment

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

이 코드 패치에 대한 간단한 코드 리뷰를 제공하겠습니다.

버그 위험 요소:

  1. UUID가 없는 경우: requestIdundefined일 수 있지만, 로깅 데이터에서는 항상 UUID가 존재해야 한다고 가정하고 사용합니다. 기본값을 제공하는 것이 좋습니다.
  2. POST와 PUT 요청 처리 중복: POST와 기타 모든 메서드에 대해 동일한 조건 처리하는 부분(else 대신 else if (method === 'PUT'))이 있으며, 코드를 더 명확하게 하기 위해 두 개의 분기를 나누는 것이 좋습니다.

개선 제안:

  1. 리퀘스트 및 레스폰스 로깅 구조화: 로그 데이터를 더 구조적으로 만들기 위해 인터페이스를 사용하면 명확성 증가.
  2. 로깅 포맷: 특정 속성을 JSON 형태로 가지도록 포맷을 커스터마이징 하지만, 이 부분을 좀 더 직관적으로 개선할 수 있습니다. 예를 들어, 필요한 정보만 선택적으로 로그에 포함시킬 수 있습니다.
  3. 수동 에러 핸들링 제거: req.body를 읽을 때 발생할 수 있는 오류를 자동으로 처리하도록 NestJS의 유효성 검사 기능을 사용하는 것도 고려해 보세요.
  4. 응답 시간 계산: finish 이벤트 리스너 내에서 응답 시간이 매우 정확하게 계산될 수 있지만, 비동기 실행 흐름에서 로그가 누락될 가능성은 다소 존재합니다. 즉, 모든 로깅이 완벽히 기록되도록 처리할지 검토하는 것이 좋습니다.
  5. 시간대 관리: 서버 머신의 지역적 시간대를 고려할 경우, 로그 기록 시 고정된 시간대 설정이 필요할 수 있습니다.

이러한 점들을 고려하여 코드의 안정성과 가독성을 높일 수 있습니다.

Copy link

Choose a reason for hiding this comment

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

코드 리뷰를 진행하겠습니다.

코드 리뷰 요약

  1. 로깅 구현:

    • Winston을 이용하여 요청 및 응답 로깅을 잘 설정했습니다. DailyRotateFile 사용으로 인해 로그 파일 관리도 용이합니다.
  2. 예외 처리:

    • 예외 발생 시 빈 객체를 기본으로 설정하는 부분은 좋지만, 구체적인 로그를 남기는 것이 더 유용할 수 있습니다. console.error() 같은 추가 로깅을 고려해 보세요.
  3. 요청 데이터 처리:

    • POST 방식의 요청에서 조건을 잘 나누고 처리는 적절하지만, PUT, PATCH와 같은 다른 HTTP 메서드를 통일감 있게 처리해야 할 수도 있습니다.
  4. 타임존 처리:

    • 타임존을 서울로 하여 좋은 선택입니다. 하지만 UTC로 스탬프를 찍고 클라이언트 쪽에서 변환하는 방법도 고려해보세요.
  5. requestId 처리:

    • UUID를 헤더에서 가져오도록 했는데, 값이 없을 경우의 비즈니스 요구사항에 맞춰 기본값을 설정하는 것도 좋습니다.
  6. 로그 포맷 일관성:

    • 요청 및 응답의 로그 포맷이 통일성을 가지지 않으면 나중에 로그 분석이 어려울 수 있습니다. 필요하다면 포맷팅을 개선해보세요.
  7. 내부 서버 오류 핸들링:

    • HttpException 외에도 다른 종류의 오류 처리를 추가해서 더 많은 세부정보를 로그에 남길 수 있습니다.
  8. 주석 처리된 코드 제거:

    • 주석 처리된 부분은 불필요한 코드를 깔끔하게 정리하여 가독성을 높이는 것이 좋습니다. 필요하다면 명확한 의도를 가진 주석만 유지하세요.

버그 위험

  • req.user.sid가 존재하지 않을 경우 undefined를 빈 문자열로 대체하는 방식은 생각보다 안전하지만 경우에 따라서 값이 더 복잡할 수 있으므로 확인이 필요합니다.
  • 로거의 동시성 문제도 고려하세요. 멀티스레드 컨텍스트에서 여러 요청이 동시에 로깅될 수 있기 때문에 경합 상태(race condition) 발생 가능성이 있습니다.

개선 사항

  • API 요청/응답 구조에 대해서 스키마 검증을 추가하여 로그하기 전에 데이터를 검증하는 방법을 생각해볼 수 있습니다.
  • 테스트 가능한 단위로 로깅 기능을 나누어 각 컴포넌트가 독립적으로 테스트 가능한 구조로 개발하는 것도 도움이 됩니다.

요약하자면, 매우 잘 작성된 코드이며 몇 가지 세부적인 개선을 통해 더욱 견고한 시스템으로 발전할 수 있을 거라 생각됩니다.

4 changes: 3 additions & 1 deletion src/modules/auth/auth.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { JwtCommand } from './command/jwt.command';
import { SidCommand } from './command/sid.command';
import { IsPublicCommand } from './command/isPublic.command';
import { AuthChain } from './auth.chain';
import { SessionCommand } from '@src/modules/auth/command/session.command';

@Injectable()
export class AuthConfig {
Expand All @@ -14,6 +15,7 @@ export class AuthConfig {
private readonly jwtCommand: JwtCommand,
private readonly sidCommand: SidCommand,
private readonly isPublicCommand: IsPublicCommand,
private readonly sessionCommand: SessionCommand,
) {}

public async config(env: string) {
Expand All @@ -34,7 +36,7 @@ export class AuthConfig {
return this.authChain
.register(this.isPublicCommand)
.register(this.sidCommand)
.register(this.jwtCommand);
.register(this.sessionCommand);
};

private getProdGuardConfig = () => {

Choose a reason for hiding this comment

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

코드 리뷰를 다음과 같이 진행하겠습니다.

  1. 버그 리스크:

    • sessionCommand가 추가됐는데, 다른 부분에서 이 커맨드를 사용하는 로직이 정의되어 있는지 확인해야 합니다. 만약 sessionCommand에 필요한 의존성이 제대로 설정되지 않았다면 런타임 오류가 발생할 수 있습니다.
    • .register(this.sessionCommand);로 인해 기존의 JWT 커맨드 등록이 누락되었습니다. 이 코드가 의도된 것인지 검토가 필요합니다. 모든 인증 관련 로직을 제대로 처리하는지 확인하세요.
  2. 개선 제안:

    • 변수와 메소드에 대해 일관된 네이밍 규칙을 유지하는 것이 좋습니다. 예를 들어 getProdGuardConfig 같은 메소드는 명명 규칙을 통일하면 더 읽기 쉬울 것입니다.
    • 에러 핸들링 로직이 필요할 수 있습니다. 특히 비동기 작업에서는 실패 상황에 대한 처리가 중요합니다.
    • SessionCommand 클래스에서 어떤 기능을 제공하는지를 명확히 문서화하거나 코멘트 추가를 추천합니다. 이를 통해 팀원들이 이해하기 쉬워집니다.

전반적으로 잘 구성된 코드이나, 몇 가지 확인해야 할 사항들이 존재합니다.

Expand Down
4 changes: 4 additions & 0 deletions src/modules/auth/auth.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,8 @@ export class AuthService {
): Promise<session_userprofile> {
return await this.userRepository.updateUser(userId, user);
}

async findBySessionKey(session_key: string) {
return await this.userRepository.findBySessionKey(session_key);
}
}
44 changes: 44 additions & 0 deletions src/modules/auth/command/session.command.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { ExecutionContext, Injectable } from '@nestjs/common';
import { Reflector } from '@nestjs/core';
import { Request } from 'express';
import { AuthService } from '../auth.service';
import { JwtService } from '@nestjs/jwt';
import { AuthCommand, AuthResult } from '../auth.command';

@Injectable()
export class SessionCommand implements AuthCommand {
constructor(
private reflector: Reflector,
private authService: AuthService,
private jwtService: JwtService,
) {}

public async next(
context: ExecutionContext,
prevResult: AuthResult,
): Promise<AuthResult> {
const request = context.switchToHttp().getRequest<Request>();
const response = context.switchToHttp().getResponse<Response>();
const accessToken = this.extractTokenFromCookie(request, 'accessToken');
const cookie = request.cookies['sessionid'];
if (!cookie) {
return prevResult;
}
const user = await this.authService.findBySessionKey(cookie);
request['user'] = user;
prevResult.authentication = true;
prevResult.authorization = true;
return prevResult;
}

private extractTokenFromCookie(
request: Request,
type: 'accessToken' | 'refreshToken',
): string | undefined {
const cookie = request.cookies[type];
if (cookie) {
return cookie;
}
return undefined;
}
}
2 changes: 2 additions & 0 deletions src/modules/semesters/semesters.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ import { Controller, Get, Query } from '@nestjs/common';
import { ISemester } from 'src/common/interfaces/ISemester';
import { toJsonSemester } from '../../common/interfaces/serializer/semester.serializer';
import { SemestersService } from './semesters.service';
import { Public } from '@src/common/decorators/skip-auth.decorator';

@Controller('api/semesters')
export class SemestersController {
constructor(private readonly semestersService: SemestersService) {}

@Get()
@Public()
async getSemesters(@Query() query: ISemester.QueryDto) {
const semesters = await this.semestersService.getSemesters(query);
return semesters.map((semester) => toJsonSemester(semester));

Choose a reason for hiding this comment

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

이 코드 패치는 전반적으로 잘 작성되어 있지만, 몇 가지 점검할 만한 사항과 개선 제안을 드립니다.

버그 리스크

  1. Query DTO 검증: @Query() 데코레이터로 전달된 ISemester.QueryDto에 대한 검증이 필요합니다. 입력 값이 유효하지 않을 경우 오류를 처리하는 로직을 추가해야 합니다.

  2. 비동기 오류 처리: 현재 async/await 블록에서 getSemesters 호출 시 오류가 발생하면 이를 처리할 수 있는 로직이 없습니다. 적절한 예외 처리가 필요합니다.

개선 제안

  1. TypeScript 타입 명시: 반환 타입을 명시하여 가독성을 높일 수 있습니다. 예를 들면, async getSemesters(@Query() query: ISemester.QueryDto): Promise<ISemester[]>와 같이 타입을 지정해주면 좋습니다.

  2. 불필요한 import 줄이기: 만약 Public 데코레이터가 이 컨트롤러에서만 사용하는 것이 아니라면, 사용 여부를 재검토하고 불필요한 import는 제거하는 것이 좋습니다.

  3. 세미스터 데이터 형식 변환: map 함수 안의 toJsonSemester가 어떤 형태로 데이터를 변환하는지 명확히 하기 위해 주석이나 문서화를 고려하세요.

이러한 사항들을 기억하고 반영하시면 더욱 안정적이고 가독성 좋은 코드를 만들 수 있을 것입니다.

Expand Down
Loading
Loading