Skip to content

Issue/185/noti server#278

Draft
sciberbee wants to merge 13 commits intomainfrom
issue/185/noti-server
Draft

Issue/185/noti server#278
sciberbee wants to merge 13 commits intomainfrom
issue/185/noti-server

Conversation

@sciberbee
Copy link
Contributor

No description provided.

@sciberbee sciberbee linked an issue Sep 15, 2025 that may be closed by this pull request
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT

apps/noti-server/env/*

.venv

Choose a reason for hiding this comment

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

코드 패치에서 'apps/noti-server/env/*' 경로가 추가되었습니다. 이 경로가 의도한 대로 추가된 것인지, 해당 디렉토리에 민감한 정보나 비밀 정보가 포함되어 있지 않은지 확인해야 합니다. 만약 비밀 정보가 있다면, 이 경로가 올바르게 무시되도록 설정했는지 검토해야 합니다. 또한, 다른 팀원들과의 커뮤니케이션을 통해 이 변경 사항의 필요성에 대한 피드백을 받는 것이 좋습니다. 향후 유지보수를 고려하여 더 이상의 경로를 추가할 필요가 있을 경우, 문서화하여 팀이 변경 사항을 쉽게 이해할 수 있도록 하는 것이 중요합니다.

# or
yarn install
yarn workspace @otlplus/scholar-sync run start:local
```

Choose a reason for hiding this comment

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

코드 패치 리뷰

  1. 환경변수 설정: env/.env.example에서 env/.env.local로 복사하는 과정이 명확하게 설명되어 있으나, 파일 생성 후 해당 파일이 .gitignore에 포함되어야 함을 강조해야 합니다. 공개 리포지토리에서 비밀번호와 같은 민감 정보를 포함할 가능성이 있습니다.

  2. DB 설정: 기본값으로 제공되는 DB 비밀번호(password)는 매우 약하므로, 사용자에게 강력한 비밀번호 사용을 권장하는 메시지를 추가해야 합니다. 이는 보안상의 문제를 방지하는 데 중요합니다.

  3. 도커 사용 안내: sudo docker compose up 명령어는 사용자 권한 문제를 일으킬 수 있습니다. 사용자가 루트 권한을 요구하게 되며, 이는 보안에 리스크가 있을 수 있습니다. 가능하다면, 도커를 실행하기 위한 권장 설정이나 사용자 권한에 대한 설명을 추가하는 것이 좋습니다.

  4. Node.js 설치 및 버전 관리: Node.js의 설치 및 버전 관리 방법이 잘 설명되어 있으나, brew를 이용한 설치에 대한 추가적인 오해를 피하기 위해 macOS 환경이 아닌 다른 OS(Windows, Linux) 사용자를 위해 대안을 제시할 필요가 있습니다.

  5. NestJS 서버 실행: yarn workspace 부분에서 제대로 설정되지 않은 워크스페이스가 있는 경우 실패할 수 있습니다. 명령어를 주기 전에 워크스페이스 설정을 확인하는 방법이나, yarn install이 성공적으로 완료되었는지 확인하도록 유도하는 메시지를 추가할 필요가 있습니다.

  6. 없는 패키지 설치 여부: brew install npm 명령어는 일반적으로 필요하지 않습니다. npm은 Node.js 설치 시 자동으로 설치되므로, 중복 설치에 대한 안내를 제거하거나 설명을 명확히 해야 합니다.

전체적으로 설명이 잘 되어 있지만, 보안과 관련된 부분 및 다양한 환경을 고려한 추가적인 설명 개선이 필요합니다.

'@typescript-eslint/no-empty-interface': 'off',
},
},
];

Choose a reason for hiding this comment

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

코드 리뷰 코멘트

  1. ESLint 설정: 현재 설정에서 여러 개의 TypeScript 관련 규칙들이 'off'로 설정되어 있습니다. 이는 코드 품질 저하의 위험이 있으며, TypeScript의 강력한 타입 시스템을 무시할 수 있습니다. 필요한 규칙을 'warn'으로 변경하거나 정말로 필요하지 않은 경우에만 'off'로 설정하는 것이 좋습니다.

  2. ECMA 버전: ecmaVersion: 5가 설정되어 있는데, 이는 최신 ES 기능을 사용할 수 없게 만듭니다. 버전을 2021 또는 최신으로 업데이트하는 것을 고려하십시오.

  3. Parser 관련 설정: project: './tsconfig.json' 설정이 코드의 최상위 디렉토리에 tsconfig.json 파일이 존재한다고 가정하고 있습니다. 만약 존재하지 않으면 ESLint가 작동하지 않을 것입니다. 파일의 존재 여부를 확인하는 로직을 추가하면 좋습니다.

  4. 존재하지 않는 플러그인: typescriptEslintEslintPlugin이 잘못된 이름일 가능성이 있습니다. 알맞은 플러그인 이름인지 확인하고, 올바른 이름으로 수정하십시오.

  5. 글로벌 변수 설정: globals.nodeglobals.jest를 모두 사용하는 것은 좋지만, 우리 프로젝트에서 사용되지 않는 글로벌 변수가 추가될 위험이 있습니다. 필요한 퍼리미터들만 포함시키는 것이 좋습니다.

  6. 인덴트 및 코딩 스타일: 전체적인 인덴트를 통일시키고, 필요 없는 공백이나 비어 있는 줄을 정리해 코드 가독성을 향상시킬 수 있습니다.

이 외에도, ESLint의 설정은 프로젝트에 맞게 조정되어야 하며, 팀원과의 논의를 통해 합의된 규칙을 반영하는 것도 중요합니다.

"coverageDirectory": "../coverage",
"testEnvironment": "node"
}
}

Choose a reason for hiding this comment

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

다음은 코드 패치에 대한 리뷰입니다:

  1. 버전 관리: @otl/api-interface와 같이 버전이 아닌 '*'로 지정된 의존성은 잠재적인 위험 요소입니다. 이는 의존성이 업데이트 되었을 때, 예기치 않은 오류를 초래할 수 있습니다. 추천하는 방법은 구체적인 버전을 사용하거나, 범위(예: ^1.0.0)로 지정하여 가능한 최소한의 변경에 대한 예측이 가능하도록 하는 것입니다.

  2. 문서화 부족: descriptionauthor와 같은 필드가 비어 있습니다. 이는 프로젝트 관리 및 유지보수에 있어 좋지 않은 접근이며, 다른 개발자가 프로젝트를 이해하는 데에 방해가 될 수 있습니다. 필요한 정보를 추가하는 것을 추천합니다.

  3. 의존성 유지 관리: 의존성 중 일부는 상당히 오래된 버전으로 보입니다(예: @prisma/clientprisma는 버전 6.1.0입니다). 의존성 업데이트는 보안 취약점을 줄이고 성능을 향상시키는 데 도움이 됩니다. 문제가 없을 경우 최신 안정화 버전으로 업데이트하시기 바랍니다.

  4. 스크립트 설정: ts-node 스크립트가 환경 변수를 local로 설정하고 있는데, 이는 개발 환경에서는 괜찮지만 배포 환경에서는 위험할 수 있습니다. 따라서, 운영 환경에 맞게 적절한 NODE_ENV 값을 설정하길 권장합니다.

  5. Linter 및 테스트 설정: 설정의 전반적인 구조는 괜찮지만, 특정 라이브러리의 버전이 서로 호환되지 않는 경우들이 있을 수 있습니다. 이러한 경우가 발생하지 않도록 각 의존성의 변경 로그를 주의 깊게 살펴보시기 바랍니다.

  6. PR 리뷰 및 CI 설정: 이 패치에는 코드 리뷰 또는 CI/CD 설정에 대한 언급이 없습니다. 이러한 자동화 과정은 코드 품질을 보장하고 실수를 줄이는 데 도움이 됩니다. 이를 위한 설정을 추가하는 것이 좋습니다.

path: envFilePath,
};

export { dotEnvOptions };

Choose a reason for hiding this comment

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

이 코드 패치는 환경 변수를 로드하기 위한 경로 설정을 포함하고 있습니다. 그러나 몇 가지 잠재적인 문제와 개선 사항이 보입니다:

  1. 파일 존재 여부 확인: 지정된 경로의 .env 파일이 존재하지 않을 경우 프로그램이 에러를 발생시킬 수 있습니다. 파일 존재 여부를 확인하고 없을 경우 적절한 오류 처리를 추가하는 것이 좋습니다.

  2. 환경 변수 유효성 검사: process.env.NODE_ENV의 값이 'local' 외의 값일 경우 (예: 오타가 있을 경우 잘못된 경로를 생성할 수 있습니다). 유효한 환경 변수 목록을 확인하고 이에 대한 기본값을 설정하는 것이 좋습니다.

  3. 개인 정보 노출 우려: console.log를 통해 파일 경로를 출력하는 것은 보안 상의 이유로 좋지 않습니다. 이는 경로가 민감할 수 있고, 로깅이 잘못된 환경에서 발생할 경우 사용자에게 노출될 수 있습니다. 대신 로깅 레벨 조정을 통해 정보의 출력 여부를 제어하는 것이 좋습니다.

  4. 모듈 구조: dotEnvOptions를 export 하기 전에 해당 객체의 사용처가 분명하지 않습니다. 이를 사용하는 컴포넌트에서 구체적인 사용 사례를 보강하면 더 좋습니다.

이러한 이유로, 현재 상태에서는 이 코드가 병합되는 것이 바람직하지 않습니다.

RABBITMQ_URL: amqp://admin:admin@rabbitmq:5672
RABBITMQ_API: http://rabbitmq:15672/api/queues
RABBITMQ_USER: admin
RABBITMQ_PASS: admin

Choose a reason for hiding this comment

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

코드 리뷰

  1. 보안 문제: RABBITMQ_DEFAULT_USERRABBITMQ_DEFAULT_PASS 환경 변수가 코드에 하드코딩 되어 있습니다. 이는 보안상 좋지 않으며, 외부에 노출될 경우 위험할 수 있습니다. 대신, 이 정보를 환경 변수 관리 도구(예: Docker secrets)나 안전한 비밀 저장소에 저장하는 것이 좋습니다.

  2. 환경 파일: push_notification_workerauto_scale_schedulerenv_file 경로가 하드코딩 되어 있어, 다른 환경에서 사용할 때 문제가 발생할 수 있습니다. 상대 경로 대신 환경 변수나 외부 설정 파일을 사용하는 것이 도움이 될 수 있습니다.

  3. 의존성 관리: 서비스가 rabbitmq에 의존하고 있지만, 해당 서비스가 시작되고 준비가 될 때까지 token_lookup_worker, push_notification_workerauto_scale_scheduler가 시작되지 않을 수 있습니다. 여러 서비스가 의존하고 있는 경우, 실제로 사용할 수 있는지 확인하기 위한 건강 체크를 추가하는 것이 좋습니다.

  4. 이미지 태그: rabbitmq:3-management 이미지 버전이 고정되어 있습니다. 이것이 특정 버전의 기능을 지원하는 경우에는 괜찮지만, 장기적으로는 최신 버전으로 자동 업데이트가 일어나지 않으므로, 명시적으로 버전을 관리할 필요가 있습니다.

  5. 환경 변수 명명: FCM_SERVER_KEY와 같은 환경 변수는 주석이나 문서화를 통해 그 의미를 명확하게 해야 합니다. 추가 문서가 필요할 수 있습니다.

개선 제안

  • 환경 변수를 안전하게 관리하는 방법을 고려하십시오.
  • 상대 경로 대신 외부 구성 솔루션을 사용하여 레거시 문제를 피하십시오.
  • 서비스의 건강 체크를 추가하여 의존성 문제를 방지하십시오.
  • Docker 이미지의 버전 관리를 강화하십시오.

COPY . .

# Push Notification Worker 실행
CMD ["node", "worker.js"]

Choose a reason for hiding this comment

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

이 코드 패치에 대한 검토 결과, 몇 가지 잠재적인 문제와 개선 사항이 있습니다.

  1. 의존성 관리: npm install 후에 package-lock.json을 복사합니다. 그러나 이 파일이 변경된 후 npm install이 실제로 필요한지 확인하는 것이 중요합니다. COPY 순서를 조정하여 캐시를 더욱 효율적으로 사용할 수 있습니다.

    COPY package.json package-lock.json ./
    RUN npm ci # 더 안전한 설치 방법으로 변경
  2. 보안: Docker 이미지에서 알려진 취약점을 방지하기 위해 npm audit를 추가하여 의존성을 점검하는 것이 좋습니다.

    RUN npm audit 
  3. 파일 복사: COPY . .는 모든 파일을 복사합니다. 필요하지 않은 파일까지 복사되어 이미지 크기가 커질 수 있습니다. .dockerignore 파일을 사용하여 불필요한 파일을 제외하는 것을 고려해 보십시오.

  4. RUN 명령어: RUN npm install을 실행하는 위치에서 최소한의 종속성만 설치하도록 조정하여 이미지를 가볍게 할 수 있습니다.

  5. CMD 명령어: CMD에 지정된 worker.js 파일이 실제로 존재하지 않는 경우 런타임 오류가 발생할 수 있습니다. 해당 파일 경로를 확인하거나 Dockerfile에서 이 파일이 올바르게 캠퍼스되는지 확인하는 것이 좋습니다.

이러한 점을 수정하면 더 나은 품질과 보안을 가진 Dockerfile이 될 것입니다.

COPY . .

# Token Lookup Worker 실행
CMD ["node", "worker.js"]

Choose a reason for hiding this comment

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

이 코드 패치는 기본적으로 Node.js 애플리케이션을 Docker 컨테이너에서 실행하기 위한 설정으로 보입니다. 그러나 다음과 같은 몇 가지 문제점과 개선 방안을 고려해야 합니다:

  1. 의존성 설치 시 취약점: npm install을 실행할 때, 취약점이 있는 패키지가 설치될 수 있습니다. 대신 npm ci를 사용하는 것이 좋습니다. 이 명령어는 package-lock.json을 기반으로 하여 안전하게 의존성을 설치합니다.

  2. 이미지 크기 최적화: 현재 설정에서는 모든 파일이 컨테이너에 복사됩니다. 필요한 파일만 복사하도록 .dockerignore 파일을 사용하는 것이 좋습니다. 이는 이미지 크기를 줄이고 빌드 시간을 단축하는 데 도움이 됩니다.

  3. 포트 설정: 컨테이너가 어떤 포트를 사용할 것인지 명시되지 않았습니다. EXPOSE 명령어를 추가하여 해당 포트를 지정해 주는 것이 좋습니다.

  4. 환경 변수: Worker의 실행에 필요한 환경 변수를 Dockerfile에서 직접 설정하거나 .env 파일로 관리하는 것이 좋습니다.

  5. CMD의 성격: CMD는 Dockerfile에서 명령어를 실행하는 방법을 정의합니다. 특정 Node.js 인자를 전달해야 할 경우가 있을 수 있으므로, 필요에 따라 수정이 필요합니다.

이 습관은 향후 배포 및 유지 관리에 중요한 요소가 될 것입니다.

},
},
);
)

Choose a reason for hiding this comment

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

코드 패치는 전반적으로 간단한 수정사항(세미콜론 제거)으로 보입니다. 그러나 다음과 같은 잠재적인 문제와 개선 사항이 있습니다:

  1. 세미콜론 제거: 세미콜론을 제거하는 것은 일부 코드 스타일 가이드에서 권장되지 않을 수 있으며, 경우에 따라 나중에 오류를 발생시킬 수 있습니다. JavaScript에서는 세미콜론 자동 삽입(ASI) 때문에, 거부된 위치에 ASI가 작동하지 않으면 심각한 오류를 초래할 수 있습니다. 따라서 세미콜론을 유지하는 것이 좋습니다.

  2. 코드 가독성: 김작은 코드가 임시로 사용되고 있다는 주석이 있지만, 이 주석이 더 구체적이면 좋습니다. 예를 들어 "airbnb 스타일을 지원하기 위한 임시 변환" 대신에 "현재 airbnb 스타일을 사용하고 있으니 추후에 이를 업데이트할 필요가 있음" 같은 내용이 추가된다면 코드 유지보수에 도움이 될 것입니다.

  3. Lint 및 타입 검사: 이 패치가 코드 스타일 및 linting 규칙에 영향을 미칠 가능성이 있으므로, 코드가 제대로 작동하는지 회귀 테스트를 수행하는 것이 좋습니다. 즉, ESLint 및 TypeScript 체크가 정상적으로 작동하는지 확인하세요.

  4. 에러 핸들링: FlatCompat를 사용하기 위해 new FlatCompat({})와 같이 빈 객체를 전달하고 있지만, 추가적인 옵션이 필요한지 여부를 검토해야 합니다. 해당 패키지의 문서를 참조하여 필요 시 적절한 설정을 추가하는 것이 좋습니다.


enum SyncType {
DEPARTMENT
COURSE

Choose a reason for hiding this comment

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

이 코드 패치에는 다음과 같은 문제점과 위험 요소가 있습니다:

  1. 병합 충돌 마커: 코드 내에 <<<<<<< HEAD, =======, >>>>>>> main와 같은 병합 충돌 마커가 존재합니다. 이는 수정 사항이 제대로 병합되지 않았다는 것을 나타내며, 현재 상태로는 코드가 실행되지 않을 것입니다. 병합 충돌을 해결해야 합니다.

  2. 모델 주석 부족: session_userprofile_fcm_token, session_userprofile_term, 및 session_term 모델에 대한 설명이나 주석이 없습니다. 이러한 모델들이 하는 일과 그 관계를 설명하는 주석이 필요합니다.

  3. 데이터베이스 종속성: 여러 관계가 설정되어 있으며, 업데이트 제약 조건(onUpdate: Restrict)이 있습니다. 필요에 따라 관계 및 종속성을 재검토해야 합니다. 특히 데이터 무결성을 보장하기 위한 적절한 설정인지 확인하십시오.

  4. 타입 검토: 각 필드의 데이터 유형에 대해 다시 검토하십시오. 예를 들어, String의 최대 길이를 정의할 필요가 있으며, 각 필드가 Null을 허용할지 여부를 확실히 해야 합니다.

  5. 인덱스 및 유니크 제약 조건: 인덱스와 유니크 제약 조건이 명확하게 정의되어 있습니다. 하지만 각 모델의 데이터 및 쿼리 패턴에 대해 이를 다시 평가하여 성능을 극대화할 수 있습니다.

이러한 이유로 이 코드는 현재 상태로는 병합할 수 없습니다. 먼저 위의 지적 사항들을 해결해야 합니다.

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.

알림 서버 구축

3 participants