Chore/swagger language header #308
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request standardizes language code representation across the codebase by migrating from 'kr' to the ISO-compliant 'ko' language code for Korean. The changes include refactoring language handling to use a centralized Language type and the @GetLanguage() decorator, replacing direct header extraction.
Key changes:
- Replaced 'kr' with 'ko' throughout the codebase to use the correct ISO 639-1 language code for Korean
- Introduced a centralized
Languagetype ('en' | 'ko') exported from the get-language decorator - Enhanced the
GetLanguagedecorator to properly parse accept-language headers with fallback logic - Updated OpenAPI/Swagger documentation to reflect the new language header parameter specification
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/server/src/swagger.json | Updated Swagger spec with correct absolute paths and new accept-language header parameters |
| apps/server/docs/swagger.json | Mirror of swagger.json updates for documentation |
| apps/server/src/common/decorators/get-language.decorator.ts | Enhanced decorator logic and exported Language type |
| apps/server/docs/openapi-generator.ts | Added detection for @GetLanguage decorator to auto-generate accept-language header params |
| apps/server/src/modules/timetables/v2/timetables.controller.ts | Replaced @headers with @GetLanguage decorator |
| apps/server/src/modules/timetables/v2/timetables.service.ts | Updated to use Language type and removed manual language parsing |
| apps/server/src/modules/lectures/v2/lectures.v2.controller.ts | Imported Language type from decorator |
| apps/server/src/modules/lectures/v2/lectures.v2.service.ts | Changed parameter names from 'kr'/'lang' to 'language' and updated variable naming |
| apps/server/src/modules/courses/v2/course.v2.controller.ts | Updated type annotations to use Language type |
| apps/server/src/modules/courses/v2/courses.v2.service.ts | Changed local type definition from 'kr' to 'ko' |
| apps/server/src/modules/reviews/v2/reviews.v2.service.ts | Updated default language parameter from 'kr' to 'ko' |
| apps/server/src/modules/user/v2/user.v2.service.ts | Updated default language parameter from 'kr' to 'ko' |
| apps/server/src/modules/semesters/semesters.service.ts | Updated default language parameter from 'kr' to 'ko' |
| apps/server/src/modules/share/share.service.ts | Changed language code from 'kr' to 'ko' in conditional expressions |
| apps/server/src/common/serializer/v2/timetable.serializer.ts | Updated type annotation to use Language type |
| apps/server/src/common/serializer/v2/review.v2.serializer.ts | Updated default language parameter from 'kr' to 'ko' |
| apps/server/src/common/interfaces/ICourseV2.ts | Updated comments from 'kr' to 'ko' |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/larrykwon/sparcs/nest/otl-nest/apps/server/src/modules/notification/domain/notification\").NotificationRequest" | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" |
There was a problem hiding this comment.
The schema contains an absolute file system path specific to a local machine ('/Users/sciberbee/Documents/otl/otlplus-server'). This should be a relative path or resolved dynamically to avoid breaking on different development machines and CI/CD environments.
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/import(\"/Users/larrykwon/sparcs/nest/otl-nest/apps/server/src/modules/notification/domain/notification\").NotificationRequest" | ||
| "$ref": "#/components/schemas/import(\"/Users/sciberbee/Documents/otl/otlplus-server/apps/server/src/modules/notification/domain/notification\").NotificationRequest" |
There was a problem hiding this comment.
The schema contains an absolute file system path specific to a local machine ('/Users/sciberbee/Documents/otl/otlplus-server'). This should be a relative path or resolved dynamically to avoid breaking on different development machines and CI/CD environments.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| if (!['query', 'param', 'body'].includes(dName)) { | ||
| return | ||
| } |
There was a problem hiding this comment.
이 코드 패치에서 몇 가지 문제가 있습니다:
-
헤더 매개변수 중복 추가:
accept-language헤더가 이미 추가된 상태에서 다시 추가되는 것을 방지하는 로직이 없습니다. 이 경우 중복된 헤더가 생길 위험이 있습니다. 예를 들어,deco.getName()이 여러 번 호출되면 매개변수가 중복될 수 있습니다. 이를 방지하기 위해parameters배열에서 이미 추가된 헤더가 있는지 확인하는 로직을 추가해야 합니다. -
반환 조건:
if (deco.getName() === 'GetLanguage')검사를 통과한 후 코드가return으로 종료됩니다. 이로 인해 다른 프로세스가 완전히 중단되고 나머지 검증이 무시될 수 있습니다. 필요하지 않은 경우에만 무시하도록 조건을 재구성하는 것이 좋습니다. -
enum 값의 설명:
enum값으로 'en'과 'ko'가 사용되고 있지만, 이 외의 언어 코드들이 필요할 경우 추가적인 검토가 필요합니다. 현재는 필요한 언어에 대한 확장이 쉽지 않습니다. -
주석의 명확성: 주석은 유용하지만, 각 조건문에 대해 무엇을 의미하는지에 대한 추가 주석을 달면 코드를 이해하는 데 더 도움이 될 수 있습니다. 특히
if (!['query', 'param', 'body'].includes(dName))와 같은 조건문의 의도를 명확히 해 주면 좋습니다.
| "lang" | ||
| ], | ||
| "additionalProperties": false | ||
| }, |
There was a problem hiding this comment.
코드 리뷰
-
경로 하드코딩: 코드에서 참조하는 경로가 사용자 특정 디렉터리에 하드코딩되어 있습니다. 이는 다른 개발자가 동일한 코드를 실행할 때 문제가 발생할 수 있습니다. 이러한 하드코딩 대신 상대 경로나 환경 변수 사용을 고려해야 합니다.
-
발견되지 않은 의존성: 코드에서 참조하는 모델들이 특정 사용자의 경로에 있기 때문에, 누군가가 지역적으로 이 코드를 실행할 수 없는 상황이 발생할 수 있습니다. 모델의 경로를 재설정할 필요가 있습니다.
-
유효성 검사 필요: 헤더에 추가한 "accept-language" 필드에 대한 유효성 검사가 서버에서 이루어지는지 확인해야 합니다. 필드가 "en" 또는 "ko" 중 하나가 아닐 경우 적절한 에러 메시지를 반환해야 합니다.
-
문서화 부족: "accept-language" 헤더에 대한 설명은 있지만, 이 값을 처리하는 방식이나 그에 따른 시스템의 응답 변화에 대한 문서가 부족해 보입니다. 그런 내용을 문서화하여 후속 개발 및 유지보수에 도움을 주는 것이 좋습니다.
-
enum 사용: "accept-language"의 enum에서 기본값을 명시하는 것은 좋지만, 이 값을 서버 측에서 사용자에게 어떻게 반영하는지에 대한 코드가 필요합니다. 기본값이 설정될 수 있도록 기존 로직을 검토해보셔야 합니다.
-
리팩토링 제안: 반복적으로 사용되는 스키마 참조 경로는 상수로 추출하고 재사용하는 방식을 고려하여 코드 중복을 줄이는 것이 바람직합니다. 이는 유지보수를 용이하게 할 수 있습니다.
이러한 점들을 수정하거나 보완하지 않으면, 코드의 안정성과 재사용성에 문제가 생길 수 있습니다.
| return 'ko' | ||
| }) | ||
|
|
||
| export type Language = 'en' | 'ko' |
There was a problem hiding this comment.
이 코드 변경은 몇 가지 위험 요소와 개선 사항이 있습니다.
-
국가 코드 반환: 'kr' 대신 'ko'를 반환하도록 변경된 것은 국제 ISO 639-1 코드를 준수하는 좋은 조치입니다. 그러나 기존 코드와의 하위 호환성이 보장되지 않으면 연결된 클라이언트 또는 서비스에 문제가 발생할 수 있습니다.
-
미지원 언어 처리: 코드에서 'ko'와 'en'만 처리하고 나머지 언어에 대해서는 기본적으로 'ko'를 반환하는 로직이 있습니다. 다양한 언어를 지원하는 애플리케이션에서는 이 방식이 문제를 일으킬 수 있습니다. 예를 들어, 지원되지 않는 언어를 받을 경우 여전히 적절한 처리를 해줄 필요가 있습니다.
-
테스트 부족: 코드 변경에 대한 유닛 테스트가 있으면 버그를 방지하는 데 도움이 될 수 있습니다. 다양한 Accept-Language 헤더 케이스에 대해 테스트해야 합니다.
-
주석 추가: 함수가 어떻게 작동하는지에 대한 주석이 없으면 나중에 유지보수가 어려울 수 있습니다. 각 언어 코드에 대한 설명을 추가하는 것이 좋습니다.
-
타입 안정성: 반환 타입이
Language로 변경되었는데, Accept-Language 헤더에서 'en'과 'ko' 외의 값을 무시하는 로직이 있어서 타입 오류가 발생할 수 있습니다. 반환 값을 안정적으로 관리하기 위해 이러한 경우를 명확히 처리해야 합니다.
| type: string // ko, en 구분 | ||
| department: IDepartmentV2.Basic | ||
| professors: IProfessorV2.Basic[] | ||
| summary: string |
There was a problem hiding this comment.
코드 패치에는 몇 가지 잠재적인 문제가 있습니다. 첫째, 주석에서 'kr'을 'ko'로 변경하였는데, 이는 한국어를 나타내는 ISO 언어 코드입니다. 다른 팀원들이 이 변경 사항을 이해할 수 있도록 설명을 추가하는 것이 좋습니다. 둘째, 주석 수정이 실제 코드의 의미에 영향을 주지 않지만, 코드의 가독성을 위해 주석을 'title'과 'title_en'의 맥락을 더 분명히 설명하는 방식으로 개선할 수 있습니다. 예를 들어, "ko : 이수과목명 / en : Course Title"과 같이 수정하는 것이 더 효과적일 수 있습니다. 추가로, 'type' 필드에 대한 주석을 'ko, en 구분' 대신 'ko: 한국어, en: 영어'와 같이 더 명확히 표현할 수 있습니다. 전반적으로, 이러한 변경 사항은 문제가 없지만 명확성과 팀원 간의 소통을 위해 개선할 여지가 있습니다.
| language: string = 'ko', | ||
| ): IReviewV2.Basic => { | ||
| let isLiked = false | ||
| if (user && review.review_reviewvote) { |
There was a problem hiding this comment.
코드 패치에 대해 몇 가지 잠재적인 문제와 개선 사항을 제안합니다.
-
언어 코드 변경:
language매개변수를 'kr'에서 'ko'로 변경했습니다. 이는 한국어를 나타내는 올바른 변경사항이지만, 기존 코드와의 일관성을 고려해야 합니다. 만약 다른 곳에서 'kr'을 사용하고 있다면, 변경된 언어 코드가 제대로 반영되고 있는지를 확인해야 합니다. -
타입 확인:
user의 타입을session_userprofile | null로 설정했는데,session_userprofile의 구조가 코드의 다른 부분에서 기대하는 속성들을 포함하고 있는지 확인해야 합니다. 속성이 없을 경우 접근할 때 문제가 발생할 수 있습니다. -
리뷰 및 투표 체크:
user와review.review_reviewvote의 조합을 체크하고 있습니다. 이 로직이 올바른지 확인해야 하며,review_reviewvote가 항상 기대한 형태로 존재하는지도 검토해야 합니다. -
에러 핸들링:
user또는review객체가 예상과 다를 경우 적절한 에러 핸들링 로직이 필요할 수 있습니다. 사용자 정의 에러 메시지를 제공하면 디버깅에 도움이 될 수 있습니다. -
주석 추가: 함수에 대한 설명 주석이 없기 때문에, 다른 개발자들이 이를 이해하기 쉽게 주석을 추가하는 것이 좋습니다. 이는 유지보수성을 높일 수 있습니다.
위의 사항들을 고려하여 변경 사항을 다시 검토해야 할 것입니다.
| const semesterName = await this.semestersService.getSemesterName(semesterObject, isEnglish ? 'en' : 'ko') | ||
|
|
||
| const timetableIcalData = { | ||
| name: `[OTL] ${semesterName}`, |
There was a problem hiding this comment.
코드 리뷰 코멘트
-
코드 스타일 개선:
getTimetableEntries호출할 때, 인라인으로 파라미터를 나열하는 것은 가독성을 떨어뜨릴 수 있습니다. 원래처럼 여러 줄로 나누어 작성하는 것이 좋습니다.
-
언어 코드 수정:
semesterName을 가져오는 부분에서'kr'대신'ko'로 замен 하였습니다. 이는 정확한 수정인지 확인이 필요합니다. 만약 변환이 의도된 것이라면, 관련 문서 또는 코드베이스에 업데이트를 반영했는지 점검해야 합니다.
-
예외 처리:
createTimetableIcal메소드에서lectures가 없거나 비어있을 경우 예외를 던지지만, 언급된 처리 이후에semesterObject에 대한 유효성 검사 등이 없습니다. 해당 변수가 적절하게 초기화되었는지 확인이 필요합니다.
-
타입 안정성:
query.language의 존재 여부 확인을 위해!!query.language를 사용하는 것은 좋지만, 이로 인해 예외를 피할 수 있지만, 나중에 해당 값이null이나undefined가 아닌 다른 타입일 경우 문제가 발생할 수 있습니다.typeof query.language === 'string'와 같은 보다 명확한 검사를 고려해 보세요.
이러한 사항을 반영하여 코드를 지켜보는 것이 중요합니다. 안정성과 가독성을 높이기 위해서는 작은 개선도 큰 도움이 될 수 있습니다.
| return await this.timetablesService.getTimetable(timetableId, user, language) | ||
| } | ||
|
|
||
| @Patch('/:timetableId') |
There was a problem hiding this comment.
코드 변경이 하신 내용을 고려할 때 몇 가지 우려 사항이 있습니다:
-
의존성 변경:
@Headers('Accept-Language')를 제거하고@GetLanguage()를 추가했습니다. 이 변경이 하위 호환성을 깨트릴 수 있는지 확인해야 합니다. 기존 클라이언트에서 Accept-Language 헤더를 보내는 방식에 의존하고 있었다면, 이 코드는 예상치 못한 오류를 초래할 수 있습니다. -
언어 타입 확인:
Language타입의 정의와 값들이 잘 설정되어 있는지 검토해야 합니다. 이 타입이Accept-Language와 호환될 수 없는 경우가 있을 수 있습니다. -
테스트 필요성: 변경된 기능이 잘 작동하는지 보장하기 위해 단위 테스트 및 통합 테스트를 보강하는 것이 좋습니다. 특히
@GetLanguage()데코레이터가 적절하게 작동하는지 확인해야 합니다. -
네이밍 및 가독성:
language라는 변수명이 언어가 아닌 다른 정보를 포함할 수 있는 경우, 가독성을 위해 명확한 이름으로 바꾸는 것을 고려해볼 수 있습니다. 예를 들어userPreferredLanguage와 같은 이름이 더 명확할 수 있습니다. -
에러 처리:
this.timetablesService메서드의 반환 값에 대한 예외 처리가 없으면, 서비스 내에서 발생하는 모든 오류가 적절히 처리되지 않을 수 있습니다. 서비스 내에서 발생할 수 있는 예외를 명확히 처리하거나, 호출하는 쪽에서 적절한 에러 핸들링을 하는 것이 좋습니다.
|
|
||
| @Transactional() | ||
| async updateTimetableLecture( | ||
| user: session_userprofile, |
There was a problem hiding this comment.
코드 리뷰
-
언어 처리 방식:
createTimetable및getTimetable함수에서language파라미터를 받도록 변경되었지만, 이 값이 어떻게 설정되는지 명확하지 않습니다. 이 값을 외부에서 전달받거나 확인하는 로직이 필요할 수 있습니다. -
기존 코드 삭제:
parseAcceptLanguage메서드가 제거됨에 따라, Accept-Language 헤더를 기반으로 언어를 설정하는 기능이 상실되었습니다. 이로 인해 이러한 정보가 누락된 경우 기본값(현재는 없을 수도 있음)이 적용되거나 언어 처리에 혼란이 생길 수 있습니다. 따라서 이 기능을 복원하는 것이 좋습니다. -
예외 처리:
getTimetable함수에서 예외 발생 시 언어 정보가 사용되고 있으나, 이를 어떻게 처리할지 명확하지 않습니다. 에러 발생 시에도 적절한 언어 정보가 전달되어야 할지 고려할 필요가 있습니다. -
주석 및 로깅: 언어 로그를 제거한 부분 (예:
console.log('language', language))에 대해 다시 추가할 필요가 있는지 검토해야 합니다. 디버그 및 유지보수를 위해 유용할 수 있습니다. -
에러 메시지 명료성: 'id of timetable is required'와 같은 에러 메시지에 좀 더 정교한 설명을 추가하면 사용자가 문제를 더 쉽게 이해할 수 있습니다.
결론적으로, 이 패치에는 여러 가지 리스크가 있으며, 언어 처리 로직과 그에 따른 예외 처리를 재검토해야 합니다.
| async getUserLikedReviews(user: session_userprofile, language: string = 'ko'): Promise<IReviewV2.Basic[]> { | ||
| const DEFAULT_ORDER = ['-written_datetime', '-id'] | ||
| const MAX_LIMIT = 100 | ||
| const likedRaw = await this.reviewsRepository.getLikedReviews(user.id, DEFAULT_ORDER, 0, MAX_LIMIT) |
There was a problem hiding this comment.
변경된 코드에서 language 매개변수가 'kr'에서 'ko'로 변경되었습니다. 이로 인해 시스템이 예상하는 언어 코드와의 불일치를 초래할 수 있습니다. 특히, 메서드가 언어를 기반으로 다르게 동작하는 경우 문제가 발생할 수 있습니다.
개선 제안:
1. 변경된 언어 코드 ('ko')가 시스템 내 다른 코드나 문서에서 일관되게 사용되고 있는지 확인하세요.
2. 추후 유지보수 및 가독성을 높이기 위해 언어 코드를 상수로 정의하고 사용하세요.
3. 만약 'kr'와 'ko'가 동일한 의미를 갖고 있다면, 주석을 추가하여 이 사실을 설명하는 것이 좋습니다.
| "lang" | ||
| ], | ||
| "additionalProperties": false | ||
| }, |
There was a problem hiding this comment.
코드 리뷰를 진행하였습니다. 주요 문제점 및 개선 사항은 다음과 같습니다:
-
하드코딩된 경로: 코드에서 사용되는 JSON 스키마의 경로가 하드코딩되어 있으며, 이는 환경에 따라 다를 수 있습니다. 따라서 이 경로를 개선하여 상대 경로를 사용하거나, 환경 변수를 이용하여 동적으로 설정하는 방식으로 변경할 것을 권장합니다.
-
유지보수성: 여러 개의
$ref경로가 변경되었는데, 이 변경이 다른 개발자의 작업에 영향을 줄 수 있습니다. 코드의 가독성과 유지 보수를 위해 문서화가 필요합니다. -
응답 코드는 변경되지 않았지만: 추가된
accept-language헤더는 좋지만, 이를 처리하는 로직이 있는지 확인이 필요합니다. 서버에서 이 헤더를 어떻게 사용할 것인지에 대한 명세가 필요합니다. -
성능 이슈: 현재 스키마에서
import를 사용하는 방식은 번들링 시 성능에 영향을 줄 수 있습니다. 이를 최적화하는 방안을 고려해 보세요. -
테스트 필요성: 이러한 경로 변경에 대해 기존의 테스트들이 여전히 통과하는지 확인이 필요합니다. 추가적인 유닛 테스트나 통합 테스트를 통해 변경 사항의 영향을 명확히 해야 합니다.
따라서, 본 패치는 환경에 따라 오류가 발생할 수 있는 리스크가 있으며, 추가적인 고려가 필요합니다. 다시 제출하기 전에 위의 사항들을 검토하시길 권장합니다.
getLanguage() 헤더를 swagger에서 감지해서 en/ko 둘 중 하나를 선택할 수 있게 했습니다.
추가로 기존에 언어가 'kr'로 잘못 적혀있던 게 있어서 'ko'로 수정했습니다.