Conversation
* add console.log to confirm * clear sso_state when logout or callback stage * log * log * log * log * log * log * log * log
* add: api-doc gen * add: index.ts
* restructure yarn based monorepo to nestjs builtin monorepo * remove console.log the query
- logger (need to checks the logs is located in proper directory) - exception filter - logging interceptor - nestjs cache manager setting - temporarily implement cache interceptor, need to customize later - nestjs docker compose, need to require password - add redis cache for get course, lecture, session/info Todo - need to implement local cache synchronization
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| volumes: | ||
| - ./redis.conf:/usr/local/etc/redis/redis.conf | ||
| - ./redis-data:/data | ||
| command: ['redis-server', '/usr/local/etc/redis/redis.conf'] |
There was a problem hiding this comment.
코드 리뷰 코멘트
-
버전:
version: '3.8'이 사용되었는데, 이는 Docker Compose v3.x의 버전입니다. 현재 후방 호환성을 위해 더 오래된 버전이 필요할 수 있습니다. -
서비스 이름:
redis-MQ라는 서비스 이름은 다소 혼동을 줄 수 있습니다. Redis는 메시지 큐로 사용되지만, 사용자에게 이를 명확히 하기 위해 간단하게redis로 변경하는 것이 좋습니다. -
컨테이너 이름:
container_name: redis는 데이터베이스가 많은 환경에서 네임 충돌을 초래할 가능성이 있습니다.container_name을 고유하게 설정해주는 것이 좋습니다. -
포트 설정: 포트를 노출할 때 보안 고려가 필요합니다. 만약 사용하지 않는 환경이라면 포트를 막는 것이 좋습니다.
-
볼륨 경로:
./redis.conf와./redis-data는 상대 경로입니다. 이 경로가 정확한지 확인해야 하며, 운영 환경에서 이로 인해 발생할 수 있는 문제가 없는지 검토해야 합니다. -
커맨드:
command에 사용된 경로/usr/local/etc/redis/redis.conf가 컨테이너 내에서 정확한 경로인지 확인이 필요합니다. 만약 잘못되면 Redis가 시작되지 않을 수 있습니다. -
Redis 설정 파일:
redis.conf에서 추가적인 설정이 필요할 수 있습니다. 예를 들어, 보안과 관련된 설정(비밀번호 설정 등)이 필요할 수 있습니다.
다시 말해, 주요 문제는 서비스 이름과 경로 확인에 있으며, 더욱 안전한 설정을 적용하는 것이 필요합니다.
| volumes: | ||
| - ./redis.conf:/usr/local/etc/redis/redis.conf | ||
| - ./redis-data:/data | ||
| command: ['redis-server', '/usr/local/etc/redis/redis.conf'] |
There was a problem hiding this comment.
코드 패치 리뷰
-
Redis 설정 파일:
./redis.conf파일이 포함되어 있는 것으로 보이지만, 이 파일의 내용도 확인해야 합니다. 잘못된 설정이 있을 경우 실행 시 문제가 발생할 수 있습니다. -
데이터 보존:
./redis-data볼륨이 매핑되어 있는 것은 좋지만, 빈 디렉토리일 경우 Redis가 실행 중 데이터가 없거나 구성 잘못으로 인한 에러가 발생할 수 있습니다. -
컨테이너 이름:
container_name은redis-MQ로 명시되어 있으나, 일반적으로 컨테이너 이름은 소문자와 하이픈으로 구성하는 것이 좋습니다.redis-mq로 일관성 있게 사용하는 것이 더 바람직합니다. -
포트 매핑:
6379:6379의 포트 매핑은 기본적으로 잘 설정되어 있으나, 다른 프로세스가 이미 해당 포트를 사용하고 있을 경우 충돌이 발생할 수 있습니다. 해당 포트 사용 여부를 사전에 확인하는 것이 좋습니다. -
파일 경로: 상대 경로를 사용하고 있는데, 해당 경로가 존재하지 않거나 권한 문제가 발생하면 애플리케이션이 정상 작동하지 않을 수 있습니다. 실행 전에 파일 경로와 권한을 확인해야 합니다.
-
알파인 이미지:
alpine이미지를 사용하여 이미지 크기를 줄이고 성능을 개선하는 것은 긍정적입니다. 단, 필요한 패키지가 누락될 수 있으므로 애플리케이션에서 사용하는 모든 의존성이 잘 작동하는지 확인해야 합니다.
이러한 이유들로 인해 현재 이 코드는 병합되기에 위험 요소가 존재합니다.
There was a problem hiding this comment.
이 코드 수정사항은 몇 가지 문제를 안고 있습니다. 첫째, .docker/redis/redis-data 라인을 추가했으나, 이 경로가 실제로 올바른 경로인지 확인이 필요합니다. 둘째, 수정 후에도 아무 줄도 추가되지 않은 채로 파일 끝에 개행 문자가 없는 상태입니다. 이는 코드의 일관성을 해치고, 특히 여러 플랫폼에서 파일을 사용할 때 문제가 될 수 있습니다. 마지막으로, 주석이 없기 때문에 이 추가 사항의 목적이나 사용 이유를 명확하게 전달하지 못합니다. 간단한 주석을 추가하여 추후 유지보수를 쉽게 할 수 있도록 하는 것이 좋습니다. 이러한 사항들을 해결한 후에야 이 코드를 병합하는 것이 바람직합니다.
| }, | ||
| JwtCookieGuard, | ||
| MockAuthGuard, | ||
| AppService, |
There was a problem hiding this comment.
코드 리뷰
-
의존성 추가:
@keyv/redis및@nestjs/cache-manager모듈을 추가하는 변경이 있습니다. 이로 인해 새로운 의존성을 관리해야 하므로, 문서화된 설치 및 설정 과정이 필요할 수 있습니다. -
전역 메모리 캐시:
CacheModule.registerAsync메소드에서는 Redis 캐시를 설정하고 있습니다. 그러나, Redis가 서버에서 제대로 실행 중이지 않거나 잘못된 설정이 있을 경우 발생할 수 있는 잠재적인 오류를 처리하는 부분이 없습니다. 이는 런타임 중 예외를 일으킬 수 있습니다.- 개선 제안: Redis 연결이 실패할 경우의 예외 처리를 추가하는 것이 좋습니다.
-
로깅 인터셉터 추가:
LoggingInterceptor를 전역 인터셉터로 등록하고 있지만, 이 인터셉터 내부 구현체가 어떻게 되어 있는지 확인이 필요합니다. 로깅이 과하게 발생할 경우 퍼포먼스에 영향을 미칠 수 있습니다.- 개선 제안: 로깅의 상세 수준이나 조건에 대한 설정을 고려하세요.
-
코드 포맷팅: 코드의 가독성을 높이기 위해 일관된 포맷팅을 적용하는 것이 좋습니다. 특히 의존성 부분에서 공백 라인의 활용이 일관되지 않습니다.
-
테스트: 기능이 추가되었으므로, 새로운 캐시 메커니즘과 로깅 인터셉터가 예상대로 작동하는지 확인하기 위한 테스트 케이스를 추가하시기 바랍니다.
이러한 사항들을 고려한 후 다시 검토할 것을 권장드립니다.
| app.useGlobalFilters(new UnexpectedExceptionFilter(), new HttpExceptionFilter<HttpException>()) | ||
| console.log(v8.getHeapStatistics().heap_size_limit / 1024 / 1024) | ||
|
|
||
| app.enableShutdownHooks() |
There was a problem hiding this comment.
다음과 같은 몇 가지 사항을 고려해야 합니다:
-
의존성 제거: 원래 로그 요청 및 응답을 처리하던
morgan의존성이 삭제되었습니다. 이로 인해 요청 및 응답을 로깅하는 기능이 사라집니다. 이는 디버깅과 모니터링에 문제를 일으킬 수 있습니다. 필요하다면 적절한 로깅 방법을 마련해야 합니다. -
예외 필터: 새로운 예외 필터(
HttpExceptionFilter,UnexpectedExceptionFilter)가 추가되었습니다. 이 필터들이 실제로 예외를 어떻게 처리하는지는 코드에서 확인할 수 없으므로, 이에 대한 충분한 검토가 필요합니다. 예외 처리가 원하는 대로 작동하는지 확인해야 합니다. -
환경 설정:
if (process.env.NODE_ENV !== 'prod')블록 내에서 Swagger 문서를 로딩하는 로직이 있습니다. 이 조건이 의도한 대로 작동하는지 확인하십시오. 프로덕션 환경에서 Swagger 문서가 불필요한 경우가 많으므로, 이 점을 다시 고려해야 합니다. -
메모리 사용:
v8.getHeapStatistics().heap_size_limit값을 공용 로그에 출력하고 있습니다. 이는 메모리 사용량에 대한 중요한 정보를 제공하지만, 클라이언트에게 부적절한 정보로 노출될 우려가 있습니다. 로그에 포함된 데이터가 안전한지 확인하십시오.
해결해야 할 이슈와 함께 코드 품질을 향상시키기 위한 방법을 고려하십시오.
| path: request.url, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
이 코드는 예외 처리에 대한 것을 구현하고 있지만 몇 가지 문제와 개선 사항이 있습니다.
-
예외 클래스:
@Catch()와@Catch(HttpException)을 사용하는 것은 좋지만, 각 필터가 어떤 예외를 처리하는지 명확히 하기 위해 BaseException을 상속받는 사용자 정의 예외 클래스를 명시적으로 문서화하는 것이 좋습니다. -
로깅: 로깅 부분에서
logger.error(exception)로 전체 예외 객체를 로깅하고 있습니다. 이는 예외 객체의 정보를 노출할 수 있으며, 민감한 정보를 포함할 수 있기 때문에 조심해야 합니다. 조치로, 특정 정보만 로깅하는 것이 좋습니다. -
response 형식 결정: 'todo'로 남겨둔 주석 부분은 구현 시 누락될 위험이 있습니다. 가능한 경우, exception의 응답 형식을 명확히 지정하거나, 고정된 형식을 사용하는 것이 좋습니다.
-
HTTP 상태 코드 처리:
HttpExceptionFilter에서exception.getResponse()로 반환된 메시지를 그대로 사용하고 있습니다. 명확한 사용자 경험을 위해 이 메시지의 형식을 효율적으로 다룰 필요가 있습니다. 현재 예외 처리에 대한 통합된 형식을 제공하는 것이 좋습니다. -
성능 고려: timestamp를 매번 새로운 Date 객체로 생성하는 것이 성능에 미미하게 영향을 줄 수 있으므로, 필요에 따라 캐시를 고려해볼 수 있습니다.
주석 및 코드 문서화가 부족하여 가독성이 떨어질 수 있으므로, 더 많은 문서화와 명확성을 추가하는 것이 좋습니다.
| }) | ||
| } | ||
|
|
||
| export default logger |
There was a problem hiding this comment.
일부 잠재적인 버그 및 개선 사항이 있습니다.
-
환경 변수가 없음:
NODE_ENV가 정의되지 않은 경우, 마지막 else 블록이 실행됩니다. 그러나 이는 'prod'와 'dev' 이외의 환경에서는 완전히 색상화된 콘솔 로거를 사용하므로, 개발 또는 생산 외의 환경에서 로깅이 불완전할 수 있습니다.NODE_ENV에 대해 기본값을 설정하거나, 해당 변수에 대한 유효성 검사를 추가하는 것이 좋습니다. -
로그 파일 과용 가능성:
maxSize를 설정했지만, 로그 파일이 과도하게 생성되거나 디스크 공간을 초과할 수 있습니다. 일정 기간 내에 일정 수의 로그 파일만 유지하는 로테이션 정책을 구현하는 것도 좋습니다. -
업데이트된 패키지 버전 확인 필요: 사용 중인
winston및 관련 패키지들이 최신인지 확인하고, 의존성을 주기적으로 업데이트하는 것을 고려하여 보안 문제를 예방해야 합니다. -
타임스탬프 포맷: 타임스탬프 포맷에서 'UTCZ'가 잘못될 수 있습니다. 'Z'는 UTC 오프셋 정보로, 올바른 형식은 'Z'를 포함하는 'YYYY-MM-DDTHH:mm:ss.sssZ' 형태일 수 있습니다.
-
메시지의 오류 형식: 오류 메시지 및 스택 정보는 기본적으로 문자열에 포함되지만, 이는 로그를 파싱하는 데 어려움을 겪을 수 있습니다. JSON 형식을 유지하는 것이 디버깅에 유용할 수 있습니다.
-
예외 처리:
exceptionHandlers배열의 각각의 핸들러에서 기록만 수행되고 종료 등 명확한 실행 후 처리가 없습니다. 필요시에 프로세스를 종료하는 로직을 추가하는 것이 좋습니다.
| }), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 검토:
-
Null 체크:
request?.user?.id에 대한 null 체크는 사용자의 ID가 없을 때 'Anonymous'로 설정되어야 하는 로직입니다. 이 부분은 괜찮지만,request자체가 null일 경우를 고려해야 합니다.request가 null일 가능성이 있을 경우 예외 처리를 추가하는 것이 좋습니다. -
헤더 값 기본값 설정:
client-os와client-api-version헤더를 가져올 때 기본값을 '-'로 설정하고 있습니다. 이 방식은 괜찮지만, 이러한 헤더가 없어질 경우 로깅의 신뢰성에 영향을 미칠 수 있습니다. 더욱 명확한 기본값이나 예외 처리를 고려해볼 수 있습니다. -
성능 문제:
Date.now()는 단순한 시간 계산을 위한 것이지만, 성능에 민감한 어플리케이션에서는 타임스탬프 계산을 과도하게 사용하지 않도록 주의해야 합니다. 예를 들어,tap에서 로그를 기록하는 것보다는finalize혹은map에 포함시키는 것이 더 나을 수 있습니다. -
로그 레벨: 현재
logger.info를 사용하고 있습니다. 로그 레벨이 적절한지 확인할 필요가 있습니다. 이 정보가 중요하다면logger.warn또는logger.debug와 같은 다른 레벨을 고려해볼 수 있습니다. -
의존성 관리:
@otl/common/logger/logger모듈이 적절하게 로드되었는지, 또는 사이드 이펙트가 있는지 확인이 필요합니다. 이 부분에 대한 문서화도 필요할 수 있습니다. -
단위 테스트: 이 인터셉터에 대한 단위 테스트는 현재 존재하는지 확인하고, 없다면 추가하는 것이 좋습니다. 특히, 로그 메시지가 제대로 생성되는지 확인하는 테스트가 필요합니다.
이러한 점들을 보완하면 더 강하고 안정적인 로깅 인터셉터가 될 것 같습니다.
| "cache-manager": "^6.4.2", | ||
| "canvas": "^3.0.1", | ||
| "class-transformer": "^0.5.1", | ||
| "class-validator": "^0.14.1", |
There was a problem hiding this comment.
이 코드 패치에는 몇 가지 주의사항과 개선 제안이 있습니다:
-
의존성 충돌 가능성: 추가된 의존성 (
@keyv/redis,@nestjs/cache-manager,cache-manager)에 주의해야 합니다.cache-manager및@nestjs/cache-manager는 서로 의존성이 있을 수 있으며, 이로 인해 충돌이 발생할 수 있습니다. 가장 추천하는 방식은 필요한 경우 각 버전이 호환되는지 확인한 후 의존성을 추가하는 것입니다. -
버전 호환성 문제: 추가된 의존성들의 버전이 다른 패키지와 충돌할 가능성이 있습니다. 특히, NestJS 및 Redis 관련 패키지는 버전이 유형별로 일관성이 있어야 합니다. 이를 확인하기 위해 전체 패키지 의존성을 다시 검토하는 것이 좋습니다.
-
리팩토링 또는 제거 고려: 만약 만약
@keyv/redis를 사용하지 않거나 과거의 코드에서 이미 사용되었다면, 불필요한 의존성을 계속 유지하지 않도록 이 코드를 재검토해야 합니다. -
테스트 커버리지: 신규 추가 패키지들에 대한 테스트가 포함되어 있는지 확인하십시오. 어떤 의존성이든 변경이 있습니다면, 이에 대한 적절한 테스트 케이스를 만들어서 이후의 테스트 실행 시 실패가 발생하지 않도록 해야 합니다.
종합적으로, 버전 확인과 테스트가 필요하므로 병합을 권장하지 않습니다.
|
|
||
| yaml@^2.2.1, yaml@^2.5.0: | ||
| version "2.7.1" | ||
| resolved "https://registry.npmjs.org/yaml/-/yaml-2.7.1.tgz#44a247d1b88523855679ac7fa7cda6ed7e135cf6" |
There was a problem hiding this comment.
코드 패치 리뷰
-
의존성 버전 관리: 새로운 패키지가 추가되고 여러 의존성이 업데이트 되었습니다. 이로 인해 기존 코드와의 호환성 문제가 발생할 수 있습니다. 특히
@redis관련 패키지들은 여러 하위 의존성을 가지고 있으므로, 이들이 서로 충돌하지 않도록 주의해야 합니다. -
정합성 및 패키지 충돌:
keyv패키지의 버전이 업데이트 되었고, 그에 따라 의존성으로@keyv/serialize도 포함되었습니다. 두 패키지 간의 버전 불일치로 인한 충돌 가능성을 고려해야 합니다. -
의존성 체인: 새로 추가된 패키지들이 의존하는 다른 패키지들도 주의깊게 살펴보아야 합니다. 예를 들어,
buffer와yallist에 대한 의존성과 그 버전이 불일치 상태로 다수 명시되어 있습니다. 이는 의도하지 않은 버그의 원인이 될 수 있습니다. -
코드 활용 시나리오: 추가된 패키지들의 사용 방법이나 API 변화에 대한 문서화를 반드시 확인하고, 코드에 미치는 영향을 테스트해야 합니다. 특히, Redis와 관련된 작업이 포함되어 있는데, 이는 성능 및 데이터의 일관성에 큰 영향을 미칠 수 있습니다.
-
종속성 청소: 언급된 패키지 중
yallist의 중복이 발생하고 있습니다. 코드베이스에서 필요하지 않은 패키지를 제거하여 종속성을 최소화하는 것이 좋습니다.
패치를 머지하기 전에, 이러한 지점을 검토하고 적절한 대응을 하는 것이 필요합니다.
2 participants
close #199