Skip to content

Issue/199/redis multi instance #200

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

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
19 changes: 19 additions & 0 deletions .docker/rabbitmq/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
version: '3'

services:
rabbitmq:
image: rabbitmq:3-management
container_name: rabbitmq
ports:
- '5672:5672' # AMQP 프로토콜 (앱들이 사용하는 포트)
- '15672:15672' # 관리 콘솔 (웹 UI) 포트
env_file: '../../env/.env.dev'
environment:
RABBITMQ_DEFAULT_USER: ${RABBITMQ_USER}
RABBITMQ_DEFAULT_PASS: ${RABBITMQ_PASS}
volumes:
- rabbitmq-data:/var/lib/rabbitmq
restart: unless-stopped

volumes:
rabbitmq-data:
1 change: 1 addition & 0 deletions .docker/rabbitmq/init-dev.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
docker compose --env-file=../../env/.env.dev up -d
12 changes: 12 additions & 0 deletions .docker/redis/redis-docker-compose.dev.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
version: '3.8'

services:
redis-MQ:
image: redis:7.2-alpine
container_name: redis
ports:
- '6379:6379'
volumes:
- ./redis.conf:/usr/local/etc/redis/redis.conf
- ./redis-data:/data
command: ['redis-server', '/usr/local/etc/redis/redis.conf']

Choose a reason for hiding this comment

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

코드 리뷰 코멘트

  1. 버전: version: '3.8'이 사용되었는데, 이는 Docker Compose v3.x의 버전입니다. 현재 후방 호환성을 위해 더 오래된 버전이 필요할 수 있습니다.

  2. 서비스 이름: redis-MQ라는 서비스 이름은 다소 혼동을 줄 수 있습니다. Redis는 메시지 큐로 사용되지만, 사용자에게 이를 명확히 하기 위해 간단하게 redis로 변경하는 것이 좋습니다.

  3. 컨테이너 이름: container_name: redis는 데이터베이스가 많은 환경에서 네임 충돌을 초래할 가능성이 있습니다. container_name을 고유하게 설정해주는 것이 좋습니다.

  4. 포트 설정: 포트를 노출할 때 보안 고려가 필요합니다. 만약 사용하지 않는 환경이라면 포트를 막는 것이 좋습니다.

  5. 볼륨 경로: ./redis.conf./redis-data는 상대 경로입니다. 이 경로가 정확한지 확인해야 하며, 운영 환경에서 이로 인해 발생할 수 있는 문제가 없는지 검토해야 합니다.

  6. 커맨드: command에 사용된 경로 /usr/local/etc/redis/redis.conf가 컨테이너 내에서 정확한 경로인지 확인이 필요합니다. 만약 잘못되면 Redis가 시작되지 않을 수 있습니다.

  7. Redis 설정 파일: redis.conf에서 추가적인 설정이 필요할 수 있습니다. 예를 들어, 보안과 관련된 설정(비밀번호 설정 등)이 필요할 수 있습니다.

다시 말해, 주요 문제는 서비스 이름과 경로 확인에 있으며, 더욱 안전한 설정을 적용하는 것이 필요합니다.

12 changes: 12 additions & 0 deletions .docker/redis/redis-mq-docker-compose.dev.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
version: '3.8'

services:
redis-MQ:
image: redis:7.2-alpine
container_name: redis-mq
ports:
- '6379:6379'
volumes:
- ./redis.conf:/usr/local/etc/redis/redis.conf
- ./redis-data:/data
command: ['redis-server', '/usr/local/etc/redis/redis.conf']

Choose a reason for hiding this comment

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

코드 패치 리뷰

  1. Redis 설정 파일: ./redis.conf 파일이 포함되어 있는 것으로 보이지만, 이 파일의 내용도 확인해야 합니다. 잘못된 설정이 있을 경우 실행 시 문제가 발생할 수 있습니다.

  2. 데이터 보존: ./redis-data 볼륨이 매핑되어 있는 것은 좋지만, 빈 디렉토리일 경우 Redis가 실행 중 데이터가 없거나 구성 잘못으로 인한 에러가 발생할 수 있습니다.

  3. 컨테이너 이름: container_nameredis-MQ로 명시되어 있으나, 일반적으로 컨테이너 이름은 소문자와 하이픈으로 구성하는 것이 좋습니다. redis-mq로 일관성 있게 사용하는 것이 더 바람직합니다.

  4. 포트 매핑: 6379:6379의 포트 매핑은 기본적으로 잘 설정되어 있으나, 다른 프로세스가 이미 해당 포트를 사용하고 있을 경우 충돌이 발생할 수 있습니다. 해당 포트 사용 여부를 사전에 확인하는 것이 좋습니다.

  5. 파일 경로: 상대 경로를 사용하고 있는데, 해당 경로가 존재하지 않거나 권한 문제가 발생하면 애플리케이션이 정상 작동하지 않을 수 있습니다. 실행 전에 파일 경로와 권한을 확인해야 합니다.

  6. 알파인 이미지: alpine 이미지를 사용하여 이미지 크기를 줄이고 성능을 개선하는 것은 긍정적입니다. 단, 필요한 패키지가 누락될 수 있으므로 애플리케이션에서 사용하는 모든 의존성이 잘 작동하는지 확인해야 합니다.

이러한 이유들로 인해 현재 이 코드는 병합되기에 위험 요소가 존재합니다.

4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,6 @@ lerna-debug.log*

apps/server/volumes
volumes
.clinic

# redis
.docker/redis/redis-data

Choose a reason for hiding this comment

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

이 코드 수정사항은 몇 가지 문제를 안고 있습니다. 첫째, .docker/redis/redis-data 라인을 추가했으나, 이 경로가 실제로 올바른 경로인지 확인이 필요합니다. 둘째, 수정 후에도 아무 줄도 추가되지 않은 채로 파일 끝에 개행 문자가 없는 상태입니다. 이는 코드의 일관성을 해치고, 특히 여러 플랫폼에서 파일을 사용할 때 문제가 될 수 있습니다. 마지막으로, 주석이 없기 때문에 이 추가 사항의 목적이나 사용 이유를 명확하게 전달하지 못합니다. 간단한 주석을 추가하여 추후 유지보수를 쉽게 할 수 있도록 하는 것이 좋습니다. 이러한 사항들을 해결한 후에야 이 코드를 병합하는 것이 바람직합니다.

16 changes: 15 additions & 1 deletion apps/server/src/app.module.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import { createKeyv } from '@keyv/redis'
import { CacheModule } from '@nestjs/cache-manager'
import { Module } from '@nestjs/common'
import { APP_GUARD } from '@nestjs/core'
import { APP_GUARD, APP_INTERCEPTOR } from '@nestjs/core'
import { JwtService } from '@nestjs/jwt'
import { ClsPluginTransactional } from '@nestjs-cls/transactional'
import { TransactionalAdapterPrisma } from '@nestjs-cls/transactional-adapter-prisma'
import { ClsModule } from 'nestjs-cls'

import { LoggingInterceptor } from '@otl/common/logger/logging.interceptor'

import { PrismaModule } from '@otl/prisma-client/prisma.module'
import { PrismaService } from '@otl/prisma-client/prisma.service'

Expand Down Expand Up @@ -65,6 +69,12 @@ import settings from './settings'
}),
],
}),
CacheModule.registerAsync({
useFactory: async () => ({
stores: [createKeyv(settings().getRedisConfig().url)],
}),
isGlobal: true,
}),
],
controllers: [AppController],
providers: [
Expand All @@ -77,6 +87,10 @@ import settings from './settings'
},
inject: [AuthConfig],
},
{
provide: APP_INTERCEPTOR,
useClass: LoggingInterceptor,
},
JwtCookieGuard,
MockAuthGuard,
AppService,

Choose a reason for hiding this comment

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

코드 리뷰

  1. 의존성 추가: @keyv/redis@nestjs/cache-manager 모듈을 추가하는 변경이 있습니다. 이로 인해 새로운 의존성을 관리해야 하므로, 문서화된 설치 및 설정 과정이 필요할 수 있습니다.

  2. 전역 메모리 캐시: CacheModule.registerAsync 메소드에서는 Redis 캐시를 설정하고 있습니다. 그러나, Redis가 서버에서 제대로 실행 중이지 않거나 잘못된 설정이 있을 경우 발생할 수 있는 잠재적인 오류를 처리하는 부분이 없습니다. 이는 런타임 중 예외를 일으킬 수 있습니다.

    • 개선 제안: Redis 연결이 실패할 경우의 예외 처리를 추가하는 것이 좋습니다.
  3. 로깅 인터셉터 추가: LoggingInterceptor를 전역 인터셉터로 등록하고 있지만, 이 인터셉터 내부 구현체가 어떻게 되어 있는지 확인이 필요합니다. 로깅이 과하게 발생할 경우 퍼포먼스에 영향을 미칠 수 있습니다.

    • 개선 제안: 로깅의 상세 수준이나 조건에 대한 설정을 고려하세요.
  4. 코드 포맷팅: 코드의 가독성을 높이기 위해 일관된 포맷팅을 적용하는 것이 좋습니다. 특히 의존성 부분에서 공백 라인의 활용이 일관되지 않습니다.

  5. 테스트: 기능이 추가되었으므로, 새로운 캐시 메커니즘과 로깅 인터셉터가 예상대로 작동하는지 확인하기 위한 테스트 케이스를 추가하시기 바랍니다.

이러한 사항들을 고려한 후 다시 검토할 것을 권장드립니다.

Expand Down
29 changes: 4 additions & 25 deletions apps/server/src/bootstrap/bootstrap.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import { ValidationPipe, VersioningType } from '@nestjs/common'
import { HttpException, ValidationPipe, VersioningType } from '@nestjs/common'
import { NestFactory } from '@nestjs/core'
import cookieParser from 'cookie-parser'
import csrf from 'csurf'
import { json } from 'express'
import session from 'express-session'
import fs from 'fs'
import morgan from 'morgan'
import * as v8 from 'node:v8'
import { join } from 'path'
import * as swaggerUi from 'swagger-ui-express'

import { HttpExceptionFilter, UnexpectedExceptionFilter } from '@otl/common/exception/exception.filter'

import { AppModule } from '../app.module'
import settings from '../settings'

Expand Down Expand Up @@ -43,30 +44,7 @@ async function bootstrap() {
ignoreMethods: ['GET', 'HEAD', 'OPTIONS', 'DELETE', 'PATCH', 'PUT', 'POST'],
}),
)
// Logs requests
app.use(
morgan(':method :url OS/:req[client-os] Ver/:req[client-api-version]', {
// https://github.com/expressjs/morgan#immediate
immediate: true,
stream: {
write: (message) => {
console.info(message.trim())
},
},
}),
)

// Logs responses
// app.use(
// morgan(':method :url :status :res[content-length] :response-time ms', {
// stream: {
// write: (message) => {
// // console.log(formatMemoryUsage())
// console.info(message.trim());
// },
// },
// }),
// );
if (process.env.NODE_ENV !== 'prod') {
const swaggerJsonPath = join(__dirname, '..', '..', 'docs', 'swagger.json')
const swaggerDocument = JSON.parse(fs.readFileSync(swaggerJsonPath, 'utf-8'))
Expand All @@ -83,6 +61,7 @@ async function bootstrap() {

app.use('/api/sync', json({ limit: '50mb' }))
app.use(json({ limit: '100kb' }))
app.useGlobalFilters(new UnexpectedExceptionFilter(), new HttpExceptionFilter<HttpException>())
console.log(v8.getHeapStatistics().heap_size_limit / 1024 / 1024)

app.enableShutdownHooks()

Choose a reason for hiding this comment

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

다음과 같은 몇 가지 사항을 고려해야 합니다:

  1. 의존성 제거: 원래 로그 요청 및 응답을 처리하던 morgan 의존성이 삭제되었습니다. 이로 인해 요청 및 응답을 로깅하는 기능이 사라집니다. 이는 디버깅과 모니터링에 문제를 일으킬 수 있습니다. 필요하다면 적절한 로깅 방법을 마련해야 합니다.

  2. 예외 필터: 새로운 예외 필터(HttpExceptionFilter, UnexpectedExceptionFilter)가 추가되었습니다. 이 필터들이 실제로 예외를 어떻게 처리하는지는 코드에서 확인할 수 없으므로, 이에 대한 충분한 검토가 필요합니다. 예외 처리가 원하는 대로 작동하는지 확인해야 합니다.

  3. 환경 설정: if (process.env.NODE_ENV !== 'prod') 블록 내에서 Swagger 문서를 로딩하는 로직이 있습니다. 이 조건이 의도한 대로 작동하는지 확인하십시오. 프로덕션 환경에서 Swagger 문서가 불필요한 경우가 많으므로, 이 점을 다시 고려해야 합니다.

  4. 메모리 사용: v8.getHeapStatistics().heap_size_limit 값을 공용 로그에 출력하고 있습니다. 이는 메모리 사용량에 대한 중요한 정보를 제공하지만, 클라이언트에게 부적절한 정보로 노출될 우려가 있습니다. 로그에 포함된 데이터가 안전한지 확인하십시오.

해결해야 할 이슈와 함께 코드 품질을 향상시키기 위한 방법을 고려하십시오.

Expand Down
43 changes: 43 additions & 0 deletions apps/server/src/common/interceptor/http.cache.interceptor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { CacheInterceptor } from '@nestjs/cache-manager'
import { ExecutionContext, Injectable } from '@nestjs/common'

/**
* @Todo
* 예시로 일단 인터넷에 있는거 긁어온거임.
* 적절히 구현 변형 필요
*/

const excludePaths = [
// 캐시가 적용되지 않아야 할 path 목록 ()
/(\/sample2\/)(.*)/i,
]

@Injectable()
export class HttpCacheInterceptor extends CacheInterceptor {
trackBy(context: ExecutionContext): string | undefined {
const request = context.switchToHttp().getRequest()
const { query } = request
const { httpAdapter } = this.httpAdapterHost

// Get Request가 아닌 request 처리
const isGetRequest = httpAdapter.getRequestMethod(request) === 'GET'
if (!isGetRequest) {
return undefined
}

// noCache=true query parameter 처리
const noCache = query.noCache && query.noCache.toLowerCase() === 'true'
if (noCache) {
return undefined
}

// 설정된 캐시 예외 path 처리
const requestPath = httpAdapter.getRequestUrl(request).split('?')[0]
const exclude = excludePaths.find((path) => path.test(requestPath))
if (exclude) {
return undefined
}

return httpAdapter.getRequestUrl(request)
}
}

Choose a reason for hiding this comment

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

이 코드 패치는 HttpCacheInterceptor를 구현하였으나, 몇 가지 문제점과 개선 사항이 있습니다.

  1. 주석 및 TODO: 코드의 상단에 TODO가 있지만, 어떤 부분을 구체적으로 수정해야 하는지에 대한 설명이 부족합니다. 더 자세한 정보를 추가하여 나중에 작업하기 쉽게 만들 필요가 있습니다.

  2. excludePaths: 현재 excludePaths 배열에 정의된 정규표현식이 하나뿐입니다. 다양한 경로를 처리해야 한다면, 더 많은 경로를 추가하거나 이를 동적으로 채울 방법을 고려해야 합니다. 또한, 경우에 따라 경로 예외 처리를 위치 기반으로 실행할 수 있는 방법도 고민해볼 필요가 있습니다.

  3. noCache 처리: noCache 쿼리 파라미터의 경우 문자열로 처리하고 있지만, 추가적인 데이터 유형 확인 및 변환이 필요할 수 있습니다(예: Boolean 타입으로 사용하기 위해).

  4. 캐시 키 생성 로직: 캐시 키를 단순히 요청 URL로 생성하는 것이 적절한지 검토해야 합니다. 쿼리 파라미터가 포함되지 않을 경우, 같은 경로에 대해 여러 캐시 키가 생성될 수 있습니다. 이로 인해 캐시 효율성이 떨어질 수 있습니다.

  5. 테스트: 이 코드 패치에 대한 단위 테스트가 누락되어 있습니다. 예외적인 경로 및 noCache가 설정된 경우에 대한 테스트 케이스가 필요합니다.

  6. 상수화: 캐시 예외 경로 목록을 상수로 만드는 것이 코드 가독성을 높이고 향후 수정 용이성을 증대시킬 수 있습니다.

위 사항들을 종합적으로 고려하여 수정 후 다시 검토하는 것이 좋겠습니다.

13 changes: 12 additions & 1 deletion apps/server/src/modules/courses/courses.controller.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
import { CacheInterceptor, CacheTTL } from '@nestjs/cache-manager'
import {
BadRequestException, Controller, Get, Param, Post, Query,
BadRequestException, Controller, Get, Param, Post, Query, UseInterceptors,
} from '@nestjs/common'
import { GetUser } from '@otl/server-nest/common/decorators/get-user.decorator'
import { Public } from '@otl/server-nest/common/decorators/skip-auth.decorator'
import { ICourse, ILecture, IReview } from '@otl/server-nest/common/interfaces'
import { CourseIdPipe } from '@otl/server-nest/common/pipe/courseId.pipe'
import { session_userprofile } from '@prisma/client'

import { RedisTTL } from '@otl/common/enum/time'

import { CoursesService } from './courses.service'

@Controller('api/courses')
export class CourseController {
constructor(private readonly coursesService: CoursesService) {}

@Public()
@CacheTTL(RedisTTL.HOUR)
@UseInterceptors(CacheInterceptor)
@Get()
async getCourses(
@Query() query: ICourse.Query,
Expand All @@ -24,12 +29,16 @@ export class CourseController {
}

@Public()
@CacheTTL(RedisTTL.HOUR)
@UseInterceptors(CacheInterceptor)
@Get('autocomplete')
async getCourseAutocomplete(@Query() query: ICourse.AutocompleteQueryDto): Promise<string | undefined> {
return await this.coursesService.getCourseAutocomplete(query)
}

@Public()
@CacheTTL(RedisTTL.HOUR)
@UseInterceptors(CacheInterceptor)
@Get(':id')
async getCourseById(
@Param('id', CourseIdPipe) id: number,
Expand All @@ -40,6 +49,8 @@ export class CourseController {
}

@Public()
@CacheTTL(RedisTTL.HOUR)
@UseInterceptors(CacheInterceptor)
@Get(':id/lectures')
async getLecturesByCourseId(
@Query() query: ICourse.LectureQueryDto,

Choose a reason for hiding this comment

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

코드 리뷰 코멘트

이 코드 패치는 전반적으로 잘 작성되었으나 몇 가지 고려할 점과 잠재적인 위험이 있습니다. 다음은 세부 사항입니다:

  1. 캐시 설정:

    • 모든 엔드포인트에 대해 CacheTTLCacheInterceptor를 사용하고 있습니다. 이를 통해 성능을 높일 수 있지만, 캐시된 데이터의 업데이트 주기와 무효화 정책을 고려해야 합니다. 데이터가 자주 변경되는 경우 캐시로 인해 오래된 정보가 사용자에게 전달될 위험이 있습니다.
  2. 예외 처리:

    • 현재 코드에서 BadRequestException을 포함하고 있지만, 각 함수 내에서 예외를 처리하는 로직이 없습니다. Query나 Param 검증에 대한 별도의 유효성 검사가 필요할 수 있습니다.
  3. 일관성 있는 응답 형식:

    • 각 API 요청의 응답 형식에 대한 명시적인 정의가 필요합니다. 예를 들면, 오류 발생 시 어떤 형태의 응답을 클라이언트가 받을 수 있는지 서류화할 필요성이 있습니다.
  4. 주석 및 문서화:

    • 함수의 목적과 입력, 출력에 대한 주석이 추가되면 유지보수성과 가독성을 높일 수 있습니다. 특히 API 문서화가 필요합니다.
  5. 미사용 import:

    • session_userprofile이 import 되었으나 사용되지 않고 있습니다. 이러한 불필요한 코드는 제거하는 것이 좋습니다.
  6. 함수 비동기 처리:

    • async/await를 사용하는 경우, 오류를 처리하기 위해 try-catch를 사용하는 것이 좋습니다. 그러면 비동기 작업 중 발생하는 예외를 잡을 수 있습니다.

이러한 고려 사항을 해결하면 코드의 품질과 안정성을 향상시킬 수 있습니다.

Expand Down
11 changes: 10 additions & 1 deletion apps/server/src/modules/lectures/lectures.controller.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,39 @@
import { CacheInterceptor, CacheTTL } from '@nestjs/cache-manager'
import {
Controller, Get, Param, Query,
Controller, Get, Param, Query, UseInterceptors,
} from '@nestjs/common'
import { GetUser } from '@otl/server-nest/common/decorators/get-user.decorator'
import { Public } from '@otl/server-nest/common/decorators/skip-auth.decorator'
import { ILecture, IReview } from '@otl/server-nest/common/interfaces'
import { session_userprofile } from '@prisma/client'

import { RedisTTL } from '@otl/common/enum/time'

import { LecturesService } from './lectures.service'

@Controller('api/lectures')
export class LecturesController {
constructor(private readonly LectureService: LecturesService) {}

@Public()
@CacheTTL(RedisTTL.HOUR)
@UseInterceptors(CacheInterceptor)
@Get()
async getLectures(@Query() query: ILecture.QueryDto): Promise<ILecture.Detail[]> {
return await this.LectureService.getLectureByFilter(query)
}

@Public()
@CacheTTL(RedisTTL.HOUR)
@UseInterceptors(CacheInterceptor)
@Get('autocomplete')
async getLectureAutocomplete(@Query() query: ILecture.AutocompleteQueryDto): Promise<string | undefined> {
return await this.LectureService.getLectureAutocomplete(query)
}

@Public()
@CacheTTL(RedisTTL.HOUR)
@UseInterceptors(CacheInterceptor)
@Get(':id')
async getLectureById(@Param('id') id: number): Promise<ILecture.Detail> {
return await this.LectureService.getLectureById(id)

Choose a reason for hiding this comment

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

코드 리뷰

  1. 캐시 보관 시간 선언의 위치: @CacheTTL(RedisTTL.HOUR) 애너테이션이 모든 내장 메서드에서 일관되게 사용되고 있는데, 만약 RedisTTL.HOUR의 값이 예상보다 긴 시간이라면 동적 데이터에 대한 사용자 경험에 부정적인 영향을 미칠 수 있습니다. 이를 확인하고 적절한 TTL 값을 지정해야 합니다.

  2. 에러 핸들링: 현재 각 메서드에서 서비스 호출 후 결과를 바로 반환하고 있지만, 서비스 호출 중 발생할 수 있는 오류를 처리하는 로직이 없습니다. 예를 들어, getLectureById 메서드에서 id가 유효하지 않은 경우 서버가 예외를 던질 수 있습니다. 각 메서드에 적절한 에러 핸들링을 추가하는 것이 좋습니다.

  3. 타입 안전성: id 파라미터를 직접 number로 받고 있는데, 클라이언트 요청 시 :id가 숫자가 아닐 경우 에러가 발생할 수 있습니다. 파라미터 검증 로직을 추가하여 형식이 유효한지 확인할 필요가 있습니다.

  4. 존재하지 않는 강의 요청 처리: getLectureById와 같은 메서드는 사용자에게 보다 친절한 오류 메시지를 제공하기 위해, 요청된 ID에 해당하는 강의가 존재하지 않는 경우 예외 처리를 구현해야 합니다.

  5. 코드 일관성: 메소드 이름이 getLectureByFilter, getLectureAutocomplete 등으로 일관성이 있지만, getById에 대한 명칭을 getLectureById로 통일할 수도 있습니다. 명명 규칙을 통일하는 것이 가독성에 도움이 됩니다.

  6. 불필요한 await: async 메서드 내부에서 바로 반환하는 경우 await 키워드를 사용할 필요가 없습니다. 예를 들어, return await this.LectureService.getLectureById(id) 대신 return this.LectureService.getLectureById(id)로 작성하는 것이 더 깔끔합니다.

이렇듯 코드에 수정할 부분이 존재하므로, 현재 상태에서는 머지할 준비가 되었다고 보기 어렵습니다.

Expand Down
5 changes: 5 additions & 0 deletions apps/server/src/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ const getPrismaConfig = (): Prisma.PrismaClientOptions => ({
],
})

const getRedisConfig = () => ({
url: process.env.REDIS_URL,
})

const getReplicatedPrismaConfig = (): Prisma.PrismaClientOptions => ({})

const getAWSConfig = () => ({})
Expand Down Expand Up @@ -106,6 +110,7 @@ export default () => ({
ormconfig: () => getPrismaConfig(),
ormReplicatedConfig: () => getReplicatedPrismaConfig(),
awsconfig: () => getAWSConfig(),
getRedisConfig: () => getRedisConfig(),
getJwtConfig: () => getJwtConfig(),
getSsoConfig: () => getSsoConfig(),
getCorsConfig: () => getCorsConfig(),

Choose a reason for hiding this comment

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

이 코드 패치에는 몇 가지 잠재적인 문제가 있습니다.

  1. 환경 변수 확인: getRedisConfig 함수에서 process.env.REDIS_URL을 직접 사용하는데, 이 환경 변수가 정의되지 않았을 경우에 대한 처리가 없습니다. 정의되지 않은 값이 전달될 경우 애플리케이션이 오류를 발생시킬 수 있습니다. 환경 변수가 유효한지 확인하는 로직을 추가하는 것이 좋습니다.

  2. 타입 안전성: getRedisConfig의 반환 타입이 명시되지 않았습니다. TypeScript를 사용하는 것이라면, 반환 타입을 명확히 정의하여 타입 안전성을 높이는 것이 좋습니다.

  3. 상태 관리: Redis와 같은 외부 서비스의 연결 설정을 코드에 하드코딩하기 보다는, 설정을 별도의 환경 파일이나 설정 관리 시스템으로 분리하는 것이 좋습니다. 이는 코드 가독성을 높이고 설정을 관리하기 쉽게 만듭니다.

  4. 각 함수의 역할 분리: getRedisConfig 함수가 단순히 환경 변수에서 URL을 가져오는 기능만을 수행하고 있습니다. 만약 Redis에 대한 추가 설정이 필요해진다면 함수가 비대해질 수 있습니다. 이는 코드의 유지보수를 더욱 어렵게 만들 수 있습니다.

따라서, 위의 사항들을 고려하여 수정한다면 코드의 안정성과 가독성을 향상시킬 수 있을 것입니다.

Expand Down
2 changes: 2 additions & 0 deletions deploy/scholar-sync/docker/Dockerfile.server
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ RUN mkdir -p /var/www/otlplus
RUN mkdir -p /var/www/otlplus/libs
RUN mkdir -p /var/www/otlplus/apps
WORKDIR /var/www/otlplus
RUN mkdir -p /var/www/otlplus/apps/scholar-sync/logs


COPY package.json yarn.lock* package-lock.json* tsconfig.json tsconfig.build.json ./
COPY nest-cli.json ./

Choose a reason for hiding this comment

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

이 패치에서 몇 가지 우려사항이 있습니다:

  1. 디렉토리 생성: RUN mkdir -p /var/www/otlplus/apps/scholar-sync/logs 명령어를 추가했는데, 이 경로가 실제로 필요한지 확인해야 합니다. 필요하지 않은 디렉토리를 미리 생성하는 것은 불필요한 리소스 낭비로 이어질 수 있습니다.

  2. 의존성 검사: COPY package.json yarn.lock* package-lock.json* tsconfig.json tsconfig.build.json ./ 명령어에서 yarn.lockpackage-lock.json 파일 중 하나만 사용해야 할 것 같습니다. 두 파일을 동시에 사용하는 것은 혼란을 초래할 수 있으며, 패키지 버전 충돌을 유발할 수 있습니다. 하나의 패키지 관리 도구를 선택해야 합니다.

  3. 가독성: 각 명령어 사이에 빈 줄을 넣어 가독성을 높이면 더 좋습니다. 예를 들어, WORKDIRRUN mkdir 명령어 사이에 빈 줄을 넣으면 읽기 쉬워집니다.

  4. 로그 디렉토리 권한: 로그 디렉토리에 대해 적절한 권한이 설정되어 있는지 확인하세요. 애플리케이션이 로그를 쓸 수 없기 때문에 오류가 발생할 수 있습니다.

따라서, 위의 문제들을 수정한 후에 다시 검토하는 것이 좋습니다.

Expand Down
2 changes: 2 additions & 0 deletions deploy/server/docker/Dockerfile.server
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ RUN mkdir -p /var/www/otlplus-server
RUN mkdir -p /var/www/otlplus-server/libs
RUN mkdir -p /var/www/otlplus-server/apps
WORKDIR /var/www/otlplus-server
RUN mkdir -p /var/www/otlplus-server/apps/server/logs


COPY package.json yarn.lock* package-lock.json* tsconfig.json tsconfig.build.json ./
COPY nest-cli.json ./

Choose a reason for hiding this comment

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

코드 리뷰

  1. 디렉토리 생성: mkdir -p 커맨드를 이용해 로그 디렉토리를 생성한 것은 좋은 접근이지만, 특정 디렉토리가 존재하지 않을 경우에만 생성하도록 하는 로직이 없어서, 이로 인해 불필요한 여유 공간을 차지할 수 있습니다.

  2. 로그 파일 관리: logs 디렉토리를 생성함으로써, 로그 파일을 관리할 수 있는 공간을 마련한 것은 긍정적입니다. 그러나, 로그 파일의 로테이션 및 관리 로직이 명시되어 있지 않아서, 로그 파일이 커져서 배포할 이미지의 용량에 영향을 줄 수 있습니다. 이에 대한 관리 방안을 생각해보는 것이 좋습니다.

  3. COPY 명령어: COPY 명령어에서 yarn.lock*package-lock.json*의 사용이 다소 의심스럽습니다. 만약 두 파일이 모두 존재하지 않는 경우, Docker 빌드가 실패할 수 있습니다. 이를 명확하게 관리하기 위해 Dockerfile에서 존재 여부를 체크하는 추가적인 로직이 필요할 수 있습니다.

  4. 주석 또는 문서화 부족: 향후 유지보수성을 높이기 위해 각 단계에 대한 주석을 추가하는 것이 좋습니다. 특히, 각 디렉토리를 만드는 목적이 무엇인지 주석으로 남겨두면, 다른 개발자가 이해하기 쉬워집니다.

위의 문제를 고려했을 때, 코드에 약간의 위험 요소가 포함되어 있어 현재 상태에서는 병합에 주의가 필요합니다.

Expand Down
1 change: 1 addition & 0 deletions deploy/server/docker/docker-compose.dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ services:
- '8000'
volumes:
- '/etc/timezone:/etc/timezone:ro'
- './apps/server/logs:/var/www/otlplus-server/apps/server/logs'
working_dir: /var/www/otlplus-server
command: pm2-runtime start ecosystem.config.js --only @otl/server-nest --node-args="max-old-space-size=40920"

Choose a reason for hiding this comment

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

수정 사항에 대해 몇 가지 잠재적인 문제가 있습니다. 추가된 로그 디렉토리 마운트 경로 './apps/server/logs'가 실제로 존재하는지 확인해야 합니다. 존재하지 않는 경로를 마운트하려고 하면 런타임 에러가 발생할 수 있습니다. 이 경로의 존재 여부를 확인하고, 필요한 경우 사전 생성하도록 문서화하는 것이 좋습니다. 또한, 볼륨 마운트는 보안 및 성능 측면에서 주의해야 하므로, 로그 데이터의 크기와 저장 정책에 대해 검토할 필요가 있습니다. 마지막으로, command 줄에서 메모리 제한을 40920MB로 설정하는 부분은 일반적으로 비정상적으로 큰 값입니다. 이 값을 실제 필요에 따라 조정해야 할 수 있습니다.

1 change: 1 addition & 0 deletions deploy/server/docker/docker-compose.local.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ services:
- '8000'
volumes:
- '/etc/timezone:/etc/timezone:ro'
- './apps/server/logs:/var/www/otlplus-server/apps/server/logs'
working_dir: /var/www/otlplus-server
command: pm2-runtime start ecosystem.config.js --only @otl/server-nest --node-args="max-old-space-size=40920"

Choose a reason for hiding this comment

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

다음 사항들을 고려할 필요가 있습니다:

  1. 로그 파일 경로 검토: 로그 파일이 호스트 시스템의 상대 경로 (./apps/server/logs)에 저장됩니다. 해당 디렉터리가 존재하지 않거나 적절한 권한이 없는 경우, 애플리케이션 실행 중에 에러가 발생할 수 있습니다. 이를 위해 실행하기 전에 확인하는 스크립트를 추가하는 것이 좋습니다.

  2. 환경 설정 상태: 새로운 볼륨을 추가함으로써 이전 설정에 영향을 줄 수 있습니다. 이 변경이 기존 서비스의 로그 파일에 영향을 미치지 않는지 테스트가 필요합니다.

  3. 리소스 할당: --node-args="max-old-space-size=40920" 옵션에서 할당된 메모리 양(40920MB)은 일부 시스템에서 너무 많을 수 있습니다. 사용자의 환경에 따라 조정해야 할 수 있습니다.

  4. 주석 추가: 새로 추가된 볼륨의 목적이나 사용 이유에 대한 주석을 추가해 문서화를 하는 것이 향후 유지 보수에 유리할 것입니다.

1 change: 1 addition & 0 deletions deploy/server/docker/docker-compose.prod.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ services:
- '58000:8000'
volumes:
- '/etc/timezone:/etc/timezone:ro'
- './apps/server/logs:/var/www/otlplus-server/apps/server/logs'
working_dir: /var/www/otlplus-server
command: pm2-runtime start ecosystem.config.js --only @otl/server-nest --node-args="max-old-space-size=40920"

Choose a reason for hiding this comment

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

코드 검토 코멘트

  1. 로그 볼륨 추가: 새로운 로그 파일 시스템을 추가하는 것은 적절해 보입니다, 그러나 애플리케이션이 이 경로를 올바르게 사용하고 있는지 확인해야 합니다. 또한, 로그 파일이 ad-hoc으로 생성될 경우도 염두에 두어야합니다. 로그 파일이 디스크 공간을 차지할 수 있으므로, 관리 전략(예: 로그 회전)을 도입하는 것이 좋습니다.

  2. 경로의 유효성 검사: ./apps/server/logs 경로가 실제로 존재하는지 확인해야 합니다. 경로가 없거나 잘못 설정되어 있다면, 애플리케이션에서 로그 기록에 실패할 것입니다.

  3. 보안 고려사항: 로그를 노출시키는 것은 잠재적인 보안 위험이 될 수 있습니다. 로그 파일에 민감한 정보가 포함되어 있을 경우, 이를 잘 관리하고 적절한 권한 설정을 하는 것이 중요합니다.

  4. 코드 가독성: 전체적인 접근 경로 및 설정을 문서화하여 다른 개발자들이 이해하기 쉽게 하는 것도 중요합니다.

결론적으로, 제안한 변경 사항은 적절해 보이나, 경로 확인 및 보안 측면에서 추가 검토가 필요합니다.

8 changes: 8 additions & 0 deletions libs/common/src/enum/time.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export const RedisTTL = {
DAY: 3600 * 24 * 1000,
WEEK: 3600 * 24 * 7 * 1000,
HOUR: 3600 * 1000,
HALF_HOUR: 1800 * 1000,
MIN: 60 * 1000,
} as const
export type TTL = (typeof RedisTTL)[keyof typeof RedisTTL]

Choose a reason for hiding this comment

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

코드에서 몇 가지 잠재적인 문제가 발견되었습니다. 아래는 개선 제안입니다:

  1. 단위 일관성 문제: RedisTTL 객체에서 각 값은 밀리초로 표현되고 있지만, 이는 DAY, WEEK, HOUR, HALF_HOUR, MIN에 대한 사용에서 혼란을 초래할 수 있습니다. 주석을 통해 각 상수가 무엇을 의미하는지 명확히 해주세요.

  2. 타입 안전성: TTL 타입을 사용하는 곳에서 오타나 잘못된 값을 전파할 위험이 있습니다. 사용할 수 있는 타임아웃을 명시적으로 나열하는 enum 대신 사용하는 것은 더 안전할 수 있습니다.

  3. 수학적 오류 확인: 모든 값을 재검토하여 올바른 값인지 확인하십시오. 특히 'DAY'와 'WEEK' 계산에서 밀리초로 변환하는 것이 정확한지 검증할 필요가 있습니다.

이러한 주요 사항을 확인한 후에는 병합이 가능할 것입니다.

Loading