Conversation
- 상세페이지를 조회한 일일 활성 사용자수를 조회합니다. 이때, 같은 ip는 1로 카운트 됩니다.
- ClubController -> ClubProfileController, ClubSearchController - ClubCommandService -> ClubProfileService - remove ClubDetailedPageService
- 봉사,종교,취미교양,학술,운동,공연 순서로 정렬
feat : 사이트 일일 활성사용자수 조회
[refactor] 컨트롤러 분할 등의 리팩토링 진행
[feature]검색 결과가 동아리 이름 오름차순으로 정렬되도록 변경
Zepelown
added
✨ Feature
|
""" Walkthrough이 변경사항은 기존의 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ClubProfileController
participant ClubProfileService
participant ClubRepository
Client->>ClubProfileController: POST /api/club (createClub)
ClubProfileController->>ClubProfileService: createClub(request)
ClubProfileService->>ClubRepository: save(club)
ClubProfileService-->>ClubProfileController: clubId
ClubProfileController-->>Client: Response(성공, clubId)
Client->>ClubProfileController: GET /api/club/{clubId}
ClubProfileController->>ClubProfileService: getClubDetail(clubId)
ClubProfileService->>ClubRepository: findById(clubId)
ClubProfileService-->>ClubProfileController: ClubDetailedResponse
ClubProfileController-->>Client: Response(ClubDetailedResponse)
sequenceDiagram
participant Client
participant ClubSearchController
participant ClubSearchService
participant ClubSearchRepository
Client->>ClubSearchController: GET /api/club/search?keyword=...
ClubSearchController->>ClubSearchService: searchClubsByKeyword(...)
ClubSearchService->>ClubSearchRepository: searchClubsByKeyword(...)
ClubSearchRepository-->>ClubSearchService: List<ClubSearchResult>
ClubSearchService-->>ClubSearchController: ClubSearchResponse (정렬된 결과)
ClubSearchController-->>Client: Response(ClubSearchResponse)
sequenceDiagram
participant Client
participant ClubMetricController
participant ClubMetricService
participant ClubMetricRepository
Client->>ClubMetricController: GET /api/club/metric/dau?n=7
ClubMetricController->>ClubMetricService: getDailyActiveUser(7)
ClubMetricService->>ClubMetricRepository: findAllByDateAfter(...)
ClubMetricRepository-->>ClubMetricService: List<ClubMetric>
ClubMetricService-->>ClubMetricController: int[] (일별 DAU)
ClubMetricController-->>Client: Response(int[])
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for moadong canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (12)
backend/src/main/java/moadong/club/controller/ClubMetricController.java (3)
31-31: 오타 수정 필요: "WitClub"메서드 이름
getDailyActiveUserWitClub에 오타가 있습니다. "Wit"을 "With"로 수정하는 것이 좋겠습니다.-public ResponseEntity<?> getDailyActiveUserWitClub(@PathVariable String clubId) { +public ResponseEntity<?> getDailyActiveUserWithClub(@PathVariable String clubId) {
41-41: 오타 수정 필요: "WitClub"메서드 이름
getWeeklyActiveUserWitClub에 오타가 있습니다. "Wit"을 "With"로 수정하는 것이 좋겠습니다.-public ResponseEntity<?> getWeeklyActiveUserWitClub(@PathVariable String clubId) { +public ResponseEntity<?> getWeeklyActiveUserWithClub(@PathVariable String clubId) {
51-51: 오타 수정 필요: "WitClub"메서드 이름
getMonthlyActiveUserWitClub에 오타가 있습니다. "Wit"을 "With"로 수정하는 것이 좋겠습니다.-public ResponseEntity<?> getMonthlyActiveUserWitClub(@PathVariable String clubId) { +public ResponseEntity<?> getMonthlyActiveUserWithClub(@PathVariable String clubId) {backend/src/main/java/moadong/club/service/ClubMetricService.java (3)
49-49: 오타 수정 필요: "WitClub"메서드 이름
getDailyActiveUserWitClub에 오타가 있습니다. "Wit"을 "With"로 수정하는 것이 좋겠습니다.-public int[] getDailyActiveUserWitClub(String clubId) { +public int[] getDailyActiveUserWithClub(String clubId) {
65-65: 오타 수정 필요: "WitClub"메서드 이름
getWeeklyActiveUserWitClub에 오타가 있습니다. "Wit"을 "With"로 수정하는 것이 좋겠습니다.-public int[] getWeeklyActiveUserWitClub(String clubId) { +public int[] getWeeklyActiveUserWithClub(String clubId) {
86-86: 오타 수정 필요: "WitClub"메서드 이름
getMonthlyActiveUserWitClub에 오타가 있습니다. "Wit"을 "With"로 수정하는 것이 좋겠습니다.-public int[] getMonthlyActiveUserWitClub(String clubId) { +public int[] getMonthlyActiveUserWithClub(String clubId) {backend/src/main/java/moadong/club/service/ClubSearchService.java (1)
32-41: 정렬 로직의 우선순위 처리가 잘 구현되었습니다만, 코드 가독성 개선이 필요합니다.정렬 로직이 모집 상태, 카테고리, 이름 순으로 우선순위에 따라 잘 구현되었습니다. 다만, 주석과 코드 포맷팅이 개선될 여지가 있습니다.
- // 정렬 - result = result.stream() - .sorted( - // - Comparator - .comparingInt((ClubSearchResult club) -> RecruitmentStatus.getPriorityFromString(club.recruitmentStatus())) - .thenComparingInt((ClubSearchResult club) -> ClubCategory.getPriorityFromString(club.category())) - .thenComparing(ClubSearchResult::name) - ) - .collect(Collectors.toList()); + // 우선순위 기준: 1) 모집상태 2) 카테고리 3) 이름 알파벳 + result = result.stream() + .sorted(Comparator + .comparingInt((ClubSearchResult club) -> RecruitmentStatus.getPriorityFromString(club.recruitmentStatus())) + .thenComparingInt((ClubSearchResult club) -> ClubCategory.getPriorityFromString(club.category())) + .thenComparing(ClubSearchResult::name)) + .collect(Collectors.toList());backend/src/main/java/moadong/club/enums/RecruitmentStatus.java (1)
34-37: getPriorityFromString 메서드가 효율적으로 구현되었습니다.getPriorityFromString 메서드가 fromString 메서드를 활용하여 코드 중복 없이 효율적으로 구현되었습니다.
다만, 일관성을 위해 ClubCategory 클래스의 유사한 메서드와 동일한 패턴을 사용하는 것이 좋습니다.
backend/src/main/java/moadong/club/controller/ClubSearchController.java (3)
23-23: URL 패턴에 후행 슬래시 확인 필요
/search/엔드포인트에 후행 슬래시가 있습니다. REST API 관례상 일반적으로 엔드포인트는 후행 슬래시 없이/search와 같이 사용합니다. 후행 슬래시가 의도적인 것인지 확인해 주세요.- @GetMapping("/search/") + @GetMapping("/search")
29-34: API 응답 타입을 명확하게 지정하는 것이 좋습니다현재
ResponseEntity<?>를 반환하고 있어 실제 반환 타입이 명확하지 않습니다. 구체적인 타입을 지정하면 API 문서화와 클라이언트 측 개발에 도움이 됩니다.- public ResponseEntity<?> searchClubsByKeyword( + public ResponseEntity<Response<ClubSearchResponse>> searchClubsByKeyword(
15-19: API 문서(Swagger)에 일관성 있는 설명 제공 필요현재 클래스 레벨
@Tag주석에는 "클럽 검색"으로 설명이 되어 있지만, 동일한/api/club경로를 공유하는ClubProfileController에서는 "클럽 API"로 설명되어 있습니다. 사용자가 API 문서를 볼 때 혼란을 방지하기 위해 일관된 네이밍 규칙을 사용하는 것이 좋습니다.- @Tag(name = "Club", description = "클럽 검색") + @Tag(name = "Club Search", description = "클럽 검색 API")backend/src/main/java/moadong/club/controller/ClubProfileController.java (1)
46-51: IP 주소 추출 로직의 유틸리티 메서드로 분리 권장IP 주소를 추출하는 로직이 컨트롤러 메서드에 직접 구현되어 있습니다. 이 로직은 다른 컨트롤러나 메서드에서도 필요할 수 있으므로, 유틸리티 클래스나 서비스로 분리하는 것이 코드 재사용성을 높이는 데 도움이 됩니다.
- String ip = request.getHeader("X-Forwarded-For"); - if (ip == null || ip.isEmpty()) { - ip = request.getRemoteAddr(); - } + String ip = ipAddressExtractor.extractIpAddress(request);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/src/main/java/moadong/club/controller/ClubController.java(0 hunks)backend/src/main/java/moadong/club/controller/ClubMetricController.java(1 hunks)backend/src/main/java/moadong/club/controller/ClubProfileController.java(1 hunks)backend/src/main/java/moadong/club/controller/ClubSearchController.java(1 hunks)backend/src/main/java/moadong/club/enums/ClubCategory.java(1 hunks)backend/src/main/java/moadong/club/enums/RecruitmentStatus.java(1 hunks)backend/src/main/java/moadong/club/payload/dto/ClubSearchResult.java(1 hunks)backend/src/main/java/moadong/club/repository/ClubMetricRepository.java(1 hunks)backend/src/main/java/moadong/club/repository/ClubRepository.java(0 hunks)backend/src/main/java/moadong/club/service/ClubDetailedPageService.java(0 hunks)backend/src/main/java/moadong/club/service/ClubMetricService.java(2 hunks)backend/src/main/java/moadong/club/service/ClubProfileService.java(2 hunks)backend/src/main/java/moadong/club/service/ClubSearchService.java(1 hunks)
💤 Files with no reviewable changes (3)
- backend/src/main/java/moadong/club/repository/ClubRepository.java
- backend/src/main/java/moadong/club/service/ClubDetailedPageService.java
- backend/src/main/java/moadong/club/controller/ClubController.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/src/main/java/moadong/club/controller/ClubSearchController.java (1)
backend/src/main/java/moadong/club/controller/ClubProfileController.java (1)
RestController(22-77)
backend/src/main/java/moadong/club/controller/ClubProfileController.java (1)
backend/src/main/java/moadong/club/controller/ClubSearchController.java (1)
RestController(15-43)
🔇 Additional comments (17)
backend/src/main/java/moadong/club/repository/ClubMetricRepository.java (1)
19-19: 새로운 메서드 추가가 적절합니다.
findAllByDateAfter메서드는 Spring Data 명명 규칙을 잘 따르고 있으며, 특정 날짜 이후의 모든 ClubMetric 객체를 조회하는 기능이 명확합니다. 이는 일일 활성 사용자 통계를 계산하는 새로운 기능에 적합합니다.backend/src/main/java/moadong/club/payload/dto/ClubSearchResult.java (2)
4-5: 임포트 순서 변경이 적절합니다.코드 기능에 영향을 주지 않는 형식적인 변경입니다.
9-17: 레코드 속성의 들여쓰기가 일관성 있게 적용되었습니다.코드 가독성을 향상시키는 형식적인 변경입니다.
backend/src/main/java/moadong/club/service/ClubSearchService.java (3)
3-14: 추가된 임포트가 코드 개선에 기여합니다.새로 추가된 ClubCategory와 RecruitmentStatus enum 클래스의 임포트와 Java 스트림 관련 임포트는 검색 결과 정렬 기능 구현을 위한 좋은 기반이 되었습니다.
21-31: 검색 파라미터 추가가 적절하게 구현되었습니다.검색 기능에 recruitmentStatus, division, category 파라미터를 추가하여 보다 세밀한 검색이 가능해졌습니다. 이는 사용자 경험 향상에 기여할 것입니다.
43-45: 응답 객체 구성이 단순하고 명확합니다.ClubSearchResponse 객체 생성 코드가 간결하고 명확하게 작성되었습니다.
backend/src/main/java/moadong/club/service/ClubProfileService.java (3)
3-14: 새로운 기능에 필요한 임포트가 적절히 추가되었습니다.ClubDetailedResult, ClubDetailedResponse, ObjectId 등 클럽 상세 정보 조회 기능 구현에 필요한 클래스들이 적절히 임포트되었습니다.
18-18: 클래스명 변경이 책임 분리에 도움이 됩니다.ClubCommandService에서 ClubProfileService로 클래스명이 변경되어 서비스의 주요 책임이 클럽 프로필 관리임을 명확하게 표현합니다.
34-41: 반환 타입 변경이 메서드의 목적에 더 적합합니다.updateClubInfo 메서드가 이제 String 대신 void를 반환하도록 변경되었습니다. 이는 업데이트 작업의 목적에 더 적합합니다.
backend/src/main/java/moadong/club/enums/ClubCategory.java (3)
3-12: 클럽 카테고리 열거형이 적절하게 정의되었습니다.클럽 카테고리가 우선순위와 함께 명확하게 정의되었습니다. 이는 검색 결과 정렬에 유용하게 사용됩니다.
13-21: 우선순위 관련 필드와 메서드가 잘 구현되었습니다.우선순위를 위한 필드와 getter 메서드가 적절히 구현되어 있습니다.
31-34: getPriorityFromString 메서드가 null 안전하게 처리되었습니다.getPriorityFromString 메서드가 null 값을 적절히 처리하여 Integer.MAX_VALUE를 반환합니다. 이는 정렬 시 유효하지 않은 카테고리를 마지막에 배치하는 데 도움이 됩니다.
backend/src/main/java/moadong/club/enums/RecruitmentStatus.java (5)
3-4: Arrays 유틸리티 임포트가 적절히 추가되었습니다.Arrays 클래스 임포트는 fromString 메서드 구현에 필요하며 적절히 추가되었습니다.
6-10: 모집 상태 열거형이 우선순위와 함께 확장되었습니다.ALWAYS("상시모집", 1) 상수가 추가되고 모든 상수에 우선순위가 할당되어 검색 정렬 기능이 개선되었습니다.
13-18: 우선순위 필드 추가와 생성자 업데이트가 적절히 이루어졌습니다.우선순위를 위한 필드가 추가되고 생성자가 업데이트되어 enum 상수에 우선순위를 부여할 수 있게 되었습니다.
24-26: 우선순위 getter 메서드 구현이 잘 되었습니다.getPriority 메서드가 간결하고 명확하게 구현되었습니다.
28-33: fromString 메서드 구현이 스트림을 활용하여 효율적입니다.fromString 메서드가 Java 스트림을 활용하여 가독성 좋게 구현되었습니다. 대소문자를 구분하지 않는 비교도 적절히 사용되었습니다.
| @GetMapping("/dau") | ||
| @Operation(summary = "일일 활성 사용자수 조회", description = "당일부터 n일 이내의 일일 활성 사용자수를 순서대로 조회합니다.<br>" | ||
| + "ip가 중복된 경우 1로 카운트합니다.<br>" | ||
| + "동아리 상세페이지를 조회한 기록을 활용합니다.") | ||
| public ResponseEntity<?> getDailyActiveUser(@RequestParam int n) { | ||
| int[] daus = clubMetricService.getDailyActiveUser(n); | ||
| return Response.ok(daus); | ||
| } |
There was a problem hiding this comment.
인증 검증 부재 및 파라미터 검증이 필요합니다.
이 API 엔드포인트는 다른 메트릭 엔드포인트들과 달리 @PreAuthorize("isAuthenticated()") 및 @SecurityRequirement(name = "BearerAuth") 어노테이션이 적용되어 있지 않습니다. 이는 의도적일 수 있으나, 동일한 컨트롤러 내의 다른 엔드포인트와 일관성이 없습니다. 또한 n 파라미터에 대한 검증이 없어 대량의 요청이 있을 경우 성능 문제가 발생할 수 있습니다.
아래와 같이 개선해보세요:
@GetMapping("/dau")
@Operation(summary = "일일 활성 사용자수 조회", description = "당일부터 n일 이내의 일일 활성 사용자수를 순서대로 조회합니다.<br>"
+ "ip가 중복된 경우 1로 카운트합니다.<br>"
+ "동아리 상세페이지를 조회한 기록을 활용합니다.")
+@PreAuthorize("isAuthenticated()")
+@SecurityRequirement(name = "BearerAuth")
public ResponseEntity<?> getDailyActiveUser(@RequestParam int n) {
+ if (n <= 0 || n > 365) {
+ return Response.badRequest("유효한 기간을 입력해주세요 (1-365)");
+ }
int[] daus = clubMetricService.getDailyActiveUser(n);
return Response.ok(daus);
}
There was a problem hiding this comment.
이걸 보면서 느낀게, 동아리 담당자가 아닌 사이트운영자들의 계정과 권한이 필요해보이네요.
하지만 아직 사이트 운영자의 역할이 어디까지 확장될지 예측할 수 없기에 고려하는 단계에서 그치지 싶습니다.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for Spring Security role configurations
echo "Looking for Spring Security role configurations..."
rg -A 5 -B 5 "hasRole|hasAuthority|@RolesAllowed|UserDetails|GrantedAuthority" --type java
# Specifically look at security configuration files
echo -e "\nChecking Security Configuration files..."
fd -e java "Security.*Config"
# Look for PreAuthorize annotations with role checks
echo -e "\nChecking PreAuthorize annotations with roles..."
rg -A 3 -B 3 "@PreAuthorize.*has(Role|Authority)" --type java
# Check existing CustomUserDetails implementation
echo -e "\nExamining CustomUserDetails implementation..."
cat backend/src/main/java/moadong/user/payload/CustomUserDetails.java
Length of output: 28402
🏁 Script executed:
#!/bin/bash
# Check the SecurityConfig class to understand the security configurations
echo "Examining SecurityConfig implementation..."
cat backend/src/main/java/moadong/global/config/SecurityConfig.java
# Check for any existing role or authority definitions in enums
echo -e "\nLooking for potential role definitions..."
fd -e java "Role|Authority" --type enum
# Understand the JwtProvider implementation
echo -e "\nExamining JwtProvider implementation..."
cat $(fd "JwtProvider.java")
Length of output: 6513
@Due-IT 네, 좋은 관찰이십니다! 코드를 살펴본 결과, 현재 시스템은 사용자가 인증되었는지만 확인하고 있으며(isAuthenticated()), 실제로 다양한 권한 수준이나 역할을 구분하지 않고 있습니다.
사이트 운영자 계정과 권한을 구현하려면 다음과 같이 접근할 수 있습니다:
User엔티티에 역할(Role) 필드 추가:
public enum UserRole {
CLUB_MANAGER, // 동아리 담당자
SITE_ADMIN // 사이트 관리자
}User클래스의getAuthorities()메서드 구현 업데이트:
@Override
public Collection<? extends GrantedAuthority> getAuthorities() {
return Collections.singletonList(new SimpleGrantedAuthority("ROLE_" + role.name()));
}- 특정 엔드포인트에 관리자 역할 제한 적용:
@GetMapping("/dau")
@PreAuthorize("hasRole('SITE_ADMIN')")
@SecurityRequirement(name = "BearerAuth")
public ResponseEntity<?> getDailyActiveUser(@RequestParam int n) {
// ...
}이렇게 하면 동아리 담당자와 사이트 운영자를 구분하여 적절한 권한 제어가 가능해질 것입니다.
🧬 Code Graph Analysis Results
Relevant snippets from backend/src/main/java/moadong/user/controller/UserController.java (lines 27-107):
@RestController
@RequestMapping("/auth/user")
@AllArgsConstructor
@Tag(name = "User", description = "동아리 담당자 계정 API")
public class UserController {
private final UserCommandService userCommandService;
private final JwtProvider jwtProvider;
@PostMapping("/register")
@Operation(
summary = UserSwaggerView.ADMIN_REGISTER_SUMMARY,
description = UserSwaggerView.ADMIN_PWD_ROLE_DESCRIPTION
)
public ResponseEntity<?> registerUser(@RequestBody @Validated UserRegisterRequest request) {
userCommandService.registerUser(request);
return Response.ok("success register");
}
@PostMapping("/login")
@Operation(
summary = UserSwaggerView.ADMIN_LOGIN_SUMMARY,
description = UserSwaggerView.ADMIN_LOGIN_DESCRIPTION
)
public ResponseEntity<?> loginUser(@RequestBody @Validated UserLoginRequest request,
HttpServletResponse response) {
LoginResponse loginResponse = userCommandService.loginUser(request, response);
return Response.ok(loginResponse);
}
//TODO : 토큰 회전 방식 + DB 리프레쉬 토큰 저장
@GetMapping("/logout")
@Operation(summary = "로그아웃", description = "클라이언트의 refresh token을 제거합니다.")
public ResponseEntity<?> logout(
@CookieValue(value = "refresh_token", required = false) String refreshToken,
HttpServletResponse response) {
userCommandService.logoutUser(refreshToken);
ResponseCookie cookie = ResponseCookie.from("refresh_token", "")
.path("/")
.maxAge(0)
.httpOnly(true)
.sameSite("None")
.secure(true)
.build();
response.addHeader("Set-Cookie", cookie.toString());
return Response.ok("success logout");
}
@PostMapping("/refresh")
@Operation(summary = "토큰 재발급", description = "refresh token을 이용하여 access token을 재발급합니다.")
public ResponseEntity<?> refresh(
@CookieValue(value = "refresh_token", required = false) String refreshToken,
HttpServletResponse response) {
RefreshResponse refreshResponse = userCommandService.refreshAccessToken(
refreshToken, response);
return Response.ok(refreshResponse);
}
@PutMapping("/")
@Operation(summary = "사용자 정보 수정", description = "사용자 정보를 수정합니다.")
@PreAuthorize("isAuthenticated()")
@SecurityRequirement(name = "BearerAuth")
public ResponseEntity<?> update(@CurrentUser CustomUserDetails user,
@RequestBody @Validated UserUpdateRequest userUpdateRequest,
HttpServletResponse response) {
userCommandService.update(user.getUserId(), userUpdateRequest, response);
return Response.ok("success update");
}
@PostMapping("/find/club")
@Operation(summary = "사용자 동아리 조회", description = "사용자의 동아리를 조회합니다.")
@PreAuthorize("isAuthenticated()")
@SecurityRequirement(name = "BearerAuth")
public ResponseEntity<?> findUserClub(@AuthenticationPrincipal CustomUserDetails userDetails) {
String clubId = userCommandService.findClubIdByUserId(userDetails.getId());
return Response.ok(new FindUserClubResponse(clubId));
}
}- This controller manages user accounts related to club administrators.
- It includes endpoints for user registration, login, logout, token refresh, user info update, and querying the user's associated club.
- Security annotations like
@PreAuthorize("isAuthenticated()")and@SecurityRequirement(name = "BearerAuth")are used to protect certain endpoints. - The presence of user registration and login suggests a system for managing user roles and permissions, potentially including site operators or administrators beyond club managers.
Relevant snippet from backend/src/main/java/moadong/gcs/controller/ClubImageController.java (lines 23-62):
@RestController
@RequestMapping("/api/club")
@RequiredArgsConstructor
@Tag(name = "ClubImage", description = "클럽 이미지 관련 API")
@PreAuthorize("isAuthenticated()")
@SecurityRequirement(name = "BearerAuth")
public class ClubImageController {
private final ClubImageService clubImageService;
@PostMapping(value = "/{clubId}/logo", consumes = MediaType.MULTIPART_FORM_DATA_VALUE)
@Operation(summary = "로고 이미지 업데이트", description = "로고 이미지를 업데이트합니다.")
public ResponseEntity<?> uploadLogo(@PathVariable String clubId,
@RequestPart("logo") MultipartFile file) {
String fileUrl = clubImageService.uploadLogo(clubId, file);
return Response.ok(fileUrl);
}
@DeleteMapping(value = "/{clubId}/logo")
@Operation(summary = "로고 이미지 삭제", description = "로고 이미지를 저장소에서 삭제합니다.")
public ResponseEntity<?> deleteLogo(@PathVariable String clubId) {
clubImageService.deleteLogo(clubId);
return Response.ok("success delete logo");
}
// TODO : Signed URL 을 통한 업로드로 추후 변경
@PostMapping(value = "/{clubId}/feed", consumes = MediaType.MULTIPART_FORM_DATA_VALUE)
@Operation(summary = "피드 이미지 업로드", description = "피드에 사용할 이미지를 업로드하고 주소를 반환받습니다.")
public ResponseEntity<?> uploadFeed(@PathVariable String clubId, @RequestPart("feed") MultipartFile file) {
return Response.ok(clubImageService.uploadFeed(clubId, file));
}
@PostMapping(value = "/{clubId}/feeds")
@Operation(summary = "저장된 피드 이미지 업데이트(순서, 삭제 등..)", description = "피드 이미지의 설정을 업데이트 합니다.")
public ResponseEntity<?> putFeeds(@PathVariable String clubId, @RequestBody FeedUpdateRequest feeds) {
clubImageService.updateFeeds(clubId, feeds.feeds());
return Response.ok("success put feeds");
}
}- This controller handles club image management, including uploading and deleting logos and feed images.
- It is secured with
@PreAuthorize("isAuthenticated()")and requires bearer token authentication. - This indicates that only authenticated users (likely club managers or authorized personnel) can modify club images.
Summary of backend/src/main/java/moadong/club/controller/ClubMetricController.java (provided file):
- Provides endpoints for retrieving club statistics such as daily, weekly, monthly active users, club ranking by views, and daily active users.
- Most endpoints require authentication (
@PreAuthorize("isAuthenticated()")), but there is no explicit role-based access control visible. - The endpoints return metrics related to clubs, presumably accessible by club managers or authenticated users.
These snippets collectively provide context on user roles, authentication, and authorization mechanisms related to club management and site operation. They are relevant to understanding the need for site operator accounts and permissions mentioned in the user comment.
| public int[] getDailyActiveUser(int n) { | ||
| LocalDate today = LocalDate.now(); | ||
| LocalDate fromDate = today.minusDays(n); | ||
| List<ClubMetric> metrics = clubMetricRepository.findAllByDateAfter(fromDate); | ||
|
|
||
| Map<LocalDate, Set<String>> daus = metrics.stream() | ||
| .collect(Collectors.groupingBy(ClubMetric::getDate, | ||
| Collectors.mapping(ClubMetric::getIp, Collectors.toSet()))); | ||
|
|
||
| int[] dausCount = new int[n]; | ||
| for (int i = 0; i < n; i++) { | ||
| LocalDate targetDate = today.minusDays(i); | ||
| dausCount[i] = daus.getOrDefault(targetDate, Collections.emptySet()).size(); | ||
| } | ||
|
|
||
| return dausCount; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
일일 활성 사용자 계산 로직에 최적화가 필요합니다.
새로운 getDailyActiveUser 메서드는 잘 구현되었지만, 몇 가지 개선이 필요합니다:
n파라미터에 대한 유효성 검사가 없습니다. 값이 매우 크면 성능 문제가 발생할 수 있습니다.- 모든 메트릭을 한 번에 가져오는 방식은 데이터가 많을 경우 효율적이지 않을 수 있습니다.
- 모든 날짜에 데이터가 있다고 가정하고 있지만, 실제로는 일부 날짜에 데이터가 없을 수 있습니다.
아래와 같이 개선해보세요:
public int[] getDailyActiveUser(int n) {
+ if (n <= 0 || n > 365) {
+ throw new IllegalArgumentException("유효한 기간을 입력해주세요 (1-365)");
+ }
LocalDate today = LocalDate.now();
LocalDate fromDate = today.minusDays(n);
List<ClubMetric> metrics = clubMetricRepository.findAllByDateAfter(fromDate);
Map<LocalDate, Set<String>> daus = metrics.stream()
.collect(Collectors.groupingBy(ClubMetric::getDate,
Collectors.mapping(ClubMetric::getIp, Collectors.toSet())));
int[] dausCount = new int[n];
for (int i = 0; i < n; i++) {
LocalDate targetDate = today.minusDays(i);
dausCount[i] = daus.getOrDefault(targetDate, Collections.emptySet()).size();
}
return dausCount;
}데이터가 많을 경우 페이징을 고려하시는 것도 좋습니다:
public int[] getDailyActiveUser(int n) {
if (n <= 0 || n > 365) {
throw new IllegalArgumentException("유효한 기간을 입력해주세요 (1-365)");
}
LocalDate today = LocalDate.now();
int[] dausCount = new int[n];
for (int i = 0; i < n; i++) {
LocalDate targetDate = today.minusDays(i);
LocalDate nextDate = targetDate.plusDays(1);
// 한 날짜씩 처리하여 메모리 사용량 최적화
List<ClubMetric> dailyMetrics = clubMetricRepository.findAllByDateGreaterThanEqualAndDateLessThan(
targetDate, nextDate);
Set<String> uniqueIps = dailyMetrics.stream()
.map(ClubMetric::getIp)
.collect(Collectors.toSet());
dausCount[i] = uniqueIps.size();
}
return dausCount;
}| public ClubDetailedResponse getClubDetail(String clubId) { | ||
| ObjectId objectId = new ObjectId(clubId); | ||
| Club club = clubRepository.findClubById(objectId) | ||
| .orElseThrow(() -> new RestApiException(ErrorCode.CLUB_NOT_FOUND)); | ||
|
|
||
| ClubDetailedResult clubDetailedResult = ClubDetailedResult.of( | ||
| club | ||
| ); | ||
| return new ClubDetailedResponse(clubDetailedResult); | ||
| } |
There was a problem hiding this comment.
getClubDetail 메서드에 예외 처리 개선이 필요합니다.
새로 추가된 getClubDetail 메서드는 잘 구현되었으나, ObjectId 변환 시 발생할 수 있는 예외 처리가 누락되었습니다. 잘못된 형식의 clubId가 전달될 경우 IllegalArgumentException이 발생할 수 있습니다.
public ClubDetailedResponse getClubDetail(String clubId) {
- ObjectId objectId = new ObjectId(clubId);
+ ObjectId objectId;
+ try {
+ objectId = new ObjectId(clubId);
+ } catch (IllegalArgumentException e) {
+ throw new RestApiException(ErrorCode.INVALID_INPUT_VALUE);
+ }
Club club = clubRepository.findClubById(objectId)
.orElseThrow(() -> new RestApiException(ErrorCode.CLUB_NOT_FOUND));
ClubDetailedResult clubDetailedResult = ClubDetailedResult.of(
club
);
return new ClubDetailedResponse(clubDetailedResult);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ClubDetailedResponse getClubDetail(String clubId) { | |
| ObjectId objectId = new ObjectId(clubId); | |
| Club club = clubRepository.findClubById(objectId) | |
| .orElseThrow(() -> new RestApiException(ErrorCode.CLUB_NOT_FOUND)); | |
| ClubDetailedResult clubDetailedResult = ClubDetailedResult.of( | |
| club | |
| ); | |
| return new ClubDetailedResponse(clubDetailedResult); | |
| } | |
| public ClubDetailedResponse getClubDetail(String clubId) { | |
| ObjectId objectId; | |
| try { | |
| objectId = new ObjectId(clubId); | |
| } catch (IllegalArgumentException e) { | |
| throw new RestApiException(ErrorCode.INVALID_INPUT_VALUE); | |
| } | |
| Club club = clubRepository.findClubById(objectId) | |
| .orElseThrow(() -> new RestApiException(ErrorCode.CLUB_NOT_FOUND)); | |
| ClubDetailedResult clubDetailedResult = ClubDetailedResult.of( | |
| club | |
| ); | |
| return new ClubDetailedResponse(clubDetailedResult); | |
| } |
| public static ClubCategory fromString(String category) { | ||
| for (ClubCategory c : values()) { | ||
| if (c.name().equals(category)) { | ||
| return c; | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
fromString 메서드에 대소문자 구분 없는 비교 기능이 필요합니다.
fromString 메서드가 대소문자를 구분하는 비교(equals)를 사용하고 있어, RecruitmentStatus의 구현과 일관성이 없습니다. 또한 null 매개변수에 대한 처리가 없습니다.
public static ClubCategory fromString(String category) {
+ if (category == null) {
+ return null;
+ }
for (ClubCategory c : values()) {
- if (c.name().equals(category)) {
+ if (c.name().equalsIgnoreCase(category)) {
return c;
}
}
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static ClubCategory fromString(String category) { | |
| for (ClubCategory c : values()) { | |
| if (c.name().equals(category)) { | |
| return c; | |
| } | |
| } | |
| return null; | |
| } | |
| public static ClubCategory fromString(String category) { | |
| if (category == null) { | |
| return null; | |
| } | |
| for (ClubCategory c : values()) { | |
| if (c.name().equalsIgnoreCase(category)) { | |
| return c; | |
| } | |
| } | |
| return null; | |
| } |
| ClubSearchResponse clubSearchResponse = clubSearchService.searchClubsByKeyword( | ||
| keyword, | ||
| recruitmentStatus, | ||
| division, | ||
| category | ||
| ); |
There was a problem hiding this comment.
서비스 호출 시 매개변수 순서가 일치하지 않습니다
searchClubsByKeyword 메서드 호출 시 파라미터 순서가 메서드 서명의 순서와 다릅니다. 메서드 서명에는 keyword, recruitmentStatus, category, division 순서이지만, 호출 시에는 keyword, recruitmentStatus, division, category 순서로 전달하고 있습니다. 이로 인해 예상치 못한 결과가 발생할 수 있습니다.
ClubSearchResponse clubSearchResponse = clubSearchService.searchClubsByKeyword(
keyword,
recruitmentStatus,
- division,
- category
+ category,
+ division
);또는 서비스 메서드의 매개변수 순서를 수정하세요.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ClubSearchResponse clubSearchResponse = clubSearchService.searchClubsByKeyword( | |
| keyword, | |
| recruitmentStatus, | |
| division, | |
| category | |
| ); | |
| ClubSearchResponse clubSearchResponse = clubSearchService.searchClubsByKeyword( | |
| keyword, | |
| recruitmentStatus, | |
| category, | |
| division | |
| ); |
There was a problem hiding this comment.
엇 이건 중요한 문제 같은데 다시 한 번 확인해봐야 할것 같아요.
There was a problem hiding this comment.
@Due-IT 네, 확인해보니 실제로 중요한 이슈가 맞습니다.
서비스 메서드의 파라미터 순서와 컨트롤러에서 호출할 때 파라미터를 전달하는 순서가 일치하지 않습니다:
ClubSearchService의 메서드 서명:
public ClubSearchResponse searchClubsByKeyword(String keyword, String recruitmentStatus, String division, String category)컨트롤러에서의 파라미터 순서:
@RequestParam(value = "keyword", required = false, defaultValue = "") String keyword,
@RequestParam(value = "recruitmentStatus", required = false, defaultValue = "all") String recruitmentStatus,
@RequestParam(value = "category", required = false, defaultValue = "all") String category,
@RequestParam(value = "division", required = false, defaultValue = "all") String division하지만 컨트롤러에서 서비스를 호출할 때는:
clubSearchService.searchClubsByKeyword(
keyword,
recruitmentStatus,
division,
category
);즉, 컨트롤러는 category와 division을 한 순서로 받고 있지만, 서비스를 호출할 때는 순서를 바꿔서 전달하고 있습니다. 이로 인해 사용자가 지정한 category와 division 값이 서로 바뀌어 검색에 사용될 수 있습니다.
수정 방법은 두 가지입니다:
- 컨트롤러의 파라미터 순서를 서비스 메서드와 맞추는 방법 (API 계약 변경)
- 서비스 호출 시 파라미터 순서를 컨트롤러 파라미터 순서와 맞추는 방법 (내부 구현 변경)
이 문제는 검색 결과에 직접적인 영향을 주므로 반드시 수정이 필요합니다.
✏️ Learnings added
Learnt from: Due-IT
PR: Moadong/moadong#329
File: backend/src/main/java/moadong/club/controller/ClubSearchController.java:0-0
Timestamp: 2025-05-01T08:32:09.386Z
Learning: ClubSearchController의 searchClubsByKeyword 메서드와 ClubSearchService의 searchClubsByKeyword 메서드 사이에 파라미터 순서 일치가 중요하다. 현재 컨트롤러의 매개변수 순서는 'keyword, recruitmentStatus, category, division'인 반면, 서비스 메서드의 순서는 'keyword, recruitmentStatus, division, category'이다. 컨트롤러에서 서비스 호출 시 'keyword, recruitmentStatus, division, category' 순서로 전달하면 category와 division 값이 바뀌어 검색 결과가 잘못될 수 있다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @PreAuthorize("isAuthenticated()") | ||
| @SecurityRequirement(name = "BearerAuth") | ||
| public ResponseEntity<?> createClub(@CurrentUser CustomUserDetails user, | ||
| @RequestBody ClubCreateRequest request) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
요청 유효성 검증 일관성 확보 필요
updateClubInfo 메서드에서는 @Validated 어노테이션을 사용하여 요청 객체의 유효성을 검증하고 있지만, createClub과 updateClubDescription 메서드에서는 유효성 검증 어노테이션이 누락되어 있습니다. 모든 요청 객체에 대해 일관되게 유효성 검증을 적용하는 것이 좋습니다.
- @RequestBody ClubCreateRequest request
+ @RequestBody @Validated ClubCreateRequest request
- @RequestBody ClubDescriptionUpdateRequest request
+ @RequestBody @Validated ClubDescriptionUpdateRequest requestAlso applies to: 63-63, 73-73
| public ResponseEntity<?> createClub(@CurrentUser CustomUserDetails user, | ||
| @RequestBody ClubCreateRequest request) { | ||
| String clubId = clubProfileService.createClub(request); | ||
| return Response.ok("success create club", "clubId : " + clubId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
인증된 사용자 정보 활용 확인 필요
createClub 메서드에서 @CurrentUser 어노테이션으로 주입된 user 객체를 사용하지 않고 있습니다. 클럽 생성 시 사용자 정보(예: 생성자 ID)를 저장하는 것이 일반적이므로, 주입된 사용자 정보를 서비스에 전달하는 것이 좋습니다.
public ResponseEntity<?> createClub(@CurrentUser CustomUserDetails user,
@RequestBody ClubCreateRequest request) {
- String clubId = clubProfileService.createClub(request);
+ String clubId = clubProfileService.createClub(request, user);
return Response.ok("success create club", "clubId : " + clubId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ResponseEntity<?> createClub(@CurrentUser CustomUserDetails user, | |
| @RequestBody ClubCreateRequest request) { | |
| String clubId = clubProfileService.createClub(request); | |
| return Response.ok("success create club", "clubId : " + clubId); | |
| public ResponseEntity<?> createClub(@CurrentUser CustomUserDetails user, | |
| @RequestBody ClubCreateRequest request) { | |
| String clubId = clubProfileService.createClub(request, user); | |
| return Response.ok("success create club", "clubId : " + clubId); | |
| } |
| public ResponseEntity<?> updateClubInfo(@RequestBody @Validated ClubInfoRequest request) { | ||
| clubProfileService.updateClubInfo(request); | ||
| return Response.ok("success update club"); |
There was a problem hiding this comment.
사용자 권한 검증 로직 필요
updateClubInfo 메서드에서 현재 사용자가 해당 클럽을 수정할 권한이 있는지 확인하는 로직이 없습니다. 인증된 사용자가 모든 클럽 정보를 수정할 수 있는 것은 보안 문제가 될 수 있습니다. 현재 사용자와 클럽 소유자/관리자 정보를 비교하여 권한 검증 로직을 추가하는 것이 좋습니다.
public ResponseEntity<?> updateClubInfo(@RequestBody @Validated ClubInfoRequest request) {
+ CustomUserDetails currentUser = (CustomUserDetails) SecurityContextHolder.getContext().getAuthentication().getPrincipal();
+ if (!clubProfileService.hasPermission(request.getClubId(), currentUser.getId())) {
+ return Response.forbidden("해당 클럽을 수정할 권한이 없습니다.");
+ }
clubProfileService.updateClubInfo(request);
return Response.ok("success update club");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ResponseEntity<?> updateClubInfo(@RequestBody @Validated ClubInfoRequest request) { | |
| clubProfileService.updateClubInfo(request); | |
| return Response.ok("success update club"); | |
| public ResponseEntity<?> updateClubInfo(@RequestBody @Validated ClubInfoRequest request) { | |
| CustomUserDetails currentUser = (CustomUserDetails) SecurityContextHolder | |
| .getContext() | |
| .getAuthentication() | |
| .getPrincipal(); | |
| if (!clubProfileService.hasPermission(request.getClubId(), currentUser.getId())) { | |
| return Response.forbidden("해당 클럽을 수정할 권한이 없습니다."); | |
| } | |
| clubProfileService.updateClubInfo(request); | |
| return Response.ok("success update club"); | |
| } |
| public ResponseEntity<?> updateClubDescription( | ||
| @RequestBody ClubDescriptionUpdateRequest request) { | ||
| clubProfileService.updateClubDescription(request); | ||
| return Response.ok("success update club"); |
There was a problem hiding this comment.
사용자 권한 검증 로직 필요
updateClubDescription 메서드에서도 동일하게 권한 검증 로직이 필요합니다. 현재 사용자가 해당 클럽의 설명을 수정할 권한이 있는지 확인하는 로직을 추가해야 합니다.
public ResponseEntity<?> updateClubDescription(
@RequestBody ClubDescriptionUpdateRequest request) {
+ CustomUserDetails currentUser = (CustomUserDetails) SecurityContextHolder.getContext().getAuthentication().getPrincipal();
+ if (!clubProfileService.hasPermission(request.getClubId(), currentUser.getId())) {
+ return Response.forbidden("해당 클럽을 수정할 권한이 없습니다.");
+ }
clubProfileService.updateClubDescription(request);
return Response.ok("success update club");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public ResponseEntity<?> updateClubDescription( | |
| @RequestBody ClubDescriptionUpdateRequest request) { | |
| clubProfileService.updateClubDescription(request); | |
| return Response.ok("success update club"); | |
| public ResponseEntity<?> updateClubDescription( | |
| @RequestBody ClubDescriptionUpdateRequest request) { | |
| CustomUserDetails currentUser = (CustomUserDetails) SecurityContextHolder | |
| .getContext() | |
| .getAuthentication() | |
| .getPrincipal(); | |
| if (!clubProfileService.hasPermission(request.getClubId(), currentUser.getId())) { | |
| return Response.forbidden("해당 클럽을 수정할 권한이 없습니다."); | |
| } | |
| clubProfileService.updateClubDescription(request); | |
| return Response.ok("success update club"); | |
| } |
| HttpServletRequest request, | ||
| @PathVariable String clubId) { | ||
| String ip = request.getHeader("X-Forwarded-For"); | ||
| if (ip == null || ip.isEmpty()) { | ||
| ip = request.getRemoteAddr(); | ||
| } | ||
| clubMetricService.patch(clubId, ip); | ||
| ClubDetailedResponse clubDetailedPageResponse = clubProfileService.getClubDetail( | ||
| clubId); | ||
| return Response.ok(clubDetailedPageResponse); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
클럽 ID 유효성 검증 로직 필요
getClubDetail 메서드에서 전달받은 clubId가 유효한지 검증하는 로직이 없습니다. 존재하지 않는 클럽 ID로 요청이 들어왔을 때 적절한 오류 응답을 반환하도록 예외 처리를 추가하는 것이 좋습니다.
public ResponseEntity<?> getClubDetail(
HttpServletRequest request,
@PathVariable String clubId) {
String ip = request.getHeader("X-Forwarded-For");
if (ip == null || ip.isEmpty()) {
ip = request.getRemoteAddr();
}
clubMetricService.patch(clubId, ip);
+ try {
ClubDetailedResponse clubDetailedPageResponse = clubProfileService.getClubDetail(
clubId);
return Response.ok(clubDetailedPageResponse);
+ } catch (NoSuchElementException e) {
+ return Response.notFound("존재하지 않는 클럽입니다.");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HttpServletRequest request, | |
| @PathVariable String clubId) { | |
| String ip = request.getHeader("X-Forwarded-For"); | |
| if (ip == null || ip.isEmpty()) { | |
| ip = request.getRemoteAddr(); | |
| } | |
| clubMetricService.patch(clubId, ip); | |
| ClubDetailedResponse clubDetailedPageResponse = clubProfileService.getClubDetail( | |
| clubId); | |
| return Response.ok(clubDetailedPageResponse); | |
| public ResponseEntity<?> getClubDetail( | |
| HttpServletRequest request, | |
| @PathVariable String clubId) { | |
| String ip = request.getHeader("X-Forwarded-For"); | |
| if (ip == null || ip.isEmpty()) { | |
| ip = request.getRemoteAddr(); | |
| } | |
| clubMetricService.patch(clubId, ip); | |
| try { | |
| ClubDetailedResponse clubDetailedPageResponse = clubProfileService.getClubDetail( | |
| clubId); | |
| return Response.ok(clubDetailedPageResponse); | |
| } catch (NoSuchElementException e) { | |
| return Response.notFound("존재하지 않는 클럽입니다."); | |
| } | |
| } |
- RecruitmentStatus -> ClubRecruitmentStatus로 이름 변경
|
코드레빗이 알려준 내용 보충해서 푸시하겠습니다. |
- ClubDescriptionUpdateRequest -> ClubRecruitmentInfoUpdateRequest
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/server_cd_release.yml (1)
113-113: 후행 공백 제거 권장
113, 116, 118행에 트레일링 스페이스가 포함되어 있습니다. YAML 파일의 일관성을 위해 제거해주세요.Also applies to: 116-116, 118-118
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 113-113: trailing spaces
(trailing-spaces)
.github/workflows/server_cd_develop.yml (1)
114-114: 후행 공백 제거 권장
114, 117, 119행에 트레일링 스페이스가 있습니다. 파일 일관성을 위해 제거해주세요.Also applies to: 117-117, 119-119
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 114-114: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/server_cd_develop.yml(2 hunks).github/workflows/server_cd_release.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/server_cd_release.yml
[error] 113-113: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 118-118: trailing spaces
(trailing-spaces)
.github/workflows/server_cd_develop.yml
[error] 114-114: trailing spaces
(trailing-spaces)
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy
🔇 Additional comments (7)
.github/workflows/server_cd_release.yml (3)
112-112: 동적 사용자 경로 적용 확인
gcloud auth activate-service-account명령에서 하드코딩된 사용자 경로를 시크릿${{ secrets.GCE_INSTANCE_USER }}로 바꾼 점은 적절합니다. 이 시크릿이 올바르게 설정되어 있는지 확인해주세요.
119-119: 배포 스크립트 실행 전 USERNAME 환경 변수 설정
export USERNAME을 사용해 동적 사용자명을 전달하는 것은 좋은 접근입니다.
121-122: 배포 스크립트 권한 부여 및 실행
sudo chmod +x후sudo -E로 환경 변수를 유지하며 스크립트를 실행하는 흐름이 적절합니다..github/workflows/server_cd_develop.yml (4)
9-9: 추가된 브랜치 필터링 검토
'fix/#346-fix_cicd'브랜치 트리거가 의도한 브랜치 명과 정확히 일치하는지 확인해주세요.
113-113: 동적 사용자 경로 적용 확인
gcloud auth activate-service-account경로를 시크릿${{ secrets.GCE_INSTANCE_USER }}로 수정한 것은 적절하지만, 해당 시크릿이 설정되어 있는지 검증이 필요합니다.
120-120: 배포 스크립트 사용자명 전달
export USERNAME=${{ secrets.GCE_INSTANCE_USER }}로 도커 컴포즈 내에서 사용자명을 전달하는 방식이 적절합니다.
122-123: 배포 스크립트 권한 및 실행
sudo chmod +x및sudo -E를 통해 배포 스크립트를 올바르게 실행하도록 설정한 점 확인했습니다.
[refactor] Club 조회 및 검색 로직에 예외 처리
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
backend/src/main/java/moadong/club/service/RecruitmentScheduler.java (2)
34-46:⚠️ Potential issue동일 key에 두 개의 스케줄을 저장하여 첫 번째 Future 가 유실됩니다
scheduledTasks.put(clubId, startFuture);이후 같은 key 에endFuture를 덮어써서 OPEN 상태 스케줄에 대한 참조가 Map 에서 사라집니다.
cancelScheduledTask호출 시 종료(Future)만 취소되고, 시작 스케줄은 메모리에 그대로 남아 잠재적 메모리 누수 및 예측 불가한 상태 변화를 일으킬 수 있습니다.아래와 같이 clubId → List<ScheduledFuture<?>> 구조나 start/end 를 구분한 key 를 사용해 두 Future 를 모두 추적하세요.
- private final Map<String, ScheduledFuture<?>> scheduledTasks = new ConcurrentHashMap<>(); + private final Map<String, List<ScheduledFuture<?>>> scheduledTasks = new ConcurrentHashMap<>(); ... - scheduledTasks.put(clubId, startFuture); - scheduledTasks.put(clubId, endFuture); + scheduledTasks.put(clubId, List.of(startFuture, endFuture));
48-53: 🛠️ Refactor suggestion취소 로직이 변경된 자료구조와 맞지 않습니다
위에서 제안한 다중 Future 저장 방식으로 전환하면, 취소 메서드도 모든 Future 를 순회하며 취소하도록 수정해야 합니다.
- ScheduledFuture<?> future = scheduledTasks.remove(clubId); - if (future != null) { - future.cancel(false); - } + List<ScheduledFuture<?>> futures = scheduledTasks.remove(clubId); + if (futures != null) { + futures.forEach(f -> f.cancel(false)); + }
🧹 Nitpick comments (4)
backend/src/main/java/moadong/global/util/ObjectIdConverter.java (1)
1-17: 유틸리티 클래스 설계가 잘 되었습니다!ObjectId 변환 로직을 중앙화하여 일관된 예외 처리를 제공하는 좋은 접근 방식입니다. 이 유틸리티 클래스는 여러 컴포넌트에서 문자열 ID를 MongoDB ObjectId로 변환하는 로직을 표준화하여 코드 중복을 줄이고 일관된 오류 처리를 보장합니다.
추가 제안:
- 장기적으로
CLUB_ID_INVALID대신 더 일반적인 에러 코드(예:INVALID_OBJECT_ID)를 사용하는 것을 고려해보세요. 이 유틸리티가 클럽 외의 다른 도메인에서도 사용될 가능성이 있습니다.- 메서드 이름을
convertToObjectId으로 명확하게 하는 것도 고려해볼 만합니다.backend/src/main/java/moadong/club/service/RecruitmentScheduler.java (1)
55-63: 트랜잭션 범위 내 save 호출이 필요할지 확인해 주세요
@Transactional이 붙어있으므로 변경 감지(dirty checking)만으로도 MongoTemplate/JPA 가 flush 할 수 있습니다.
clubRepository.save(club);호출을 생략해도 동일 동작을 기대할 수 있다면 제거해 성능을 미세하게나마 개선할 수 있습니다.backend/src/main/java/moadong/club/entity/ClubRecruitmentInformation.java (1)
72-77: null 병합 연산자를 사용해 불필요한 덮어쓰기를 방지할 수 있습니다
updateDescription에서 전달값이 null 일 경우 기존 값을 유지하려면 다음처럼 null-check 로직을 추가하는 것이 안전합니다.- this.description = request.description(); + if (request.description() != null) { + this.description = request.description(); + }특히 PATCH 성격의 API 라면 부분 업데이트 요구사항에 부합하도록 수정해 주세요.
backend/src/main/java/moadong/club/service/ClubProfileService.java (1)
67-74: 권한 검증 실패 시 상세 로그를 남기는 것을 권장합니다현재
USER_UNAUTHORIZED로 예외를 던지지만 어떤 clubId·userId 였는지 로그가 없으면 운영 중 원인 파악이 어렵습니다.
SLF4J logger 로 debug/info 수준의 로그를 남겨 추적성을 높여 주세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/src/main/java/moadong/club/controller/ClubProfileController.java(1 hunks)backend/src/main/java/moadong/club/controller/ClubSearchController.java(1 hunks)backend/src/main/java/moadong/club/entity/Club.java(2 hunks)backend/src/main/java/moadong/club/entity/ClubRecruitmentInformation.java(3 hunks)backend/src/main/java/moadong/club/enums/ClubCategory.java(1 hunks)backend/src/main/java/moadong/club/enums/ClubRecruitmentStatus.java(1 hunks)backend/src/main/java/moadong/club/payload/dto/ClubDetailedResult.java(1 hunks)backend/src/main/java/moadong/club/payload/request/ClubDescriptionUpdateRequest.java(0 hunks)backend/src/main/java/moadong/club/payload/request/ClubRecruitmentInfoUpdateRequest.java(1 hunks)backend/src/main/java/moadong/club/service/ClubProfileService.java(1 hunks)backend/src/main/java/moadong/club/service/ClubSearchService.java(1 hunks)backend/src/main/java/moadong/club/service/RecruitmentScheduler.java(3 hunks)backend/src/main/java/moadong/gcs/service/ClubImageService.java(5 hunks)backend/src/main/java/moadong/global/exception/ErrorCode.java(2 hunks)backend/src/main/java/moadong/global/util/ObjectIdConverter.java(1 hunks)backend/src/main/java/moadong/user/payload/CustomUserDetails.java(1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/main/java/moadong/club/payload/request/ClubDescriptionUpdateRequest.java
✅ Files skipped from review due to trivial changes (3)
- backend/src/main/java/moadong/user/payload/CustomUserDetails.java
- backend/src/main/java/moadong/club/payload/request/ClubRecruitmentInfoUpdateRequest.java
- backend/src/main/java/moadong/global/exception/ErrorCode.java
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/src/main/java/moadong/club/enums/ClubCategory.java
- backend/src/main/java/moadong/club/service/ClubSearchService.java
- backend/src/main/java/moadong/club/controller/ClubSearchController.java
- backend/src/main/java/moadong/club/controller/ClubProfileController.java
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/src/main/java/moadong/gcs/service/ClubImageService.java (1)
backend/src/main/java/moadong/global/util/ObjectIdConverter.java (1)
ObjectIdConverter(7-17)
backend/src/main/java/moadong/club/service/RecruitmentScheduler.java (1)
backend/src/main/java/moadong/global/util/ObjectIdConverter.java (1)
ObjectIdConverter(7-17)
backend/src/main/java/moadong/club/service/ClubProfileService.java (2)
backend/src/main/java/moadong/club/entity/ClubRecruitmentInformation.java (1)
AllArgsConstructor(23-108)backend/src/main/java/moadong/global/util/ObjectIdConverter.java (1)
ObjectIdConverter(7-17)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy
🔇 Additional comments (10)
backend/src/main/java/moadong/gcs/service/ClubImageService.java (5)
15-15: 적절한 유틸리티 클래스 임포트입니다.새로 추가된 ObjectIdConverter 유틸리티 클래스를 임포트하여 일관된 ID 변환 로직을 적용하고 있습니다.
36-36: 안전한 ObjectId 변환 방식으로 개선되었습니다.직접적인
new ObjectId(clubId)대신ObjectIdConverter.convertString(clubId)를 사용하여 일관된 예외 처리를 보장합니다. 이렇게 하면 유효하지 않은 ID 형식에 대해 더 명확한 오류 메시지를 제공할 수 있습니다.
51-51: 안전한 ObjectId 변환 방식으로 개선되었습니다.
deleteLogo메서드에서도 동일하게 ObjectIdConverter를 사용하여 일관된 예외 처리를 보장합니다.
61-61: 안전한 ObjectId 변환 방식으로 개선되었습니다.
uploadFeed메서드에서도 ObjectIdConverter를 사용하여 일관된 예외 처리를 구현하고 있습니다.
73-73: 안전한 ObjectId 변환 방식으로 개선되었습니다.
updateFeeds메서드에서도 동일하게 ObjectIdConverter 유틸리티를 활용하여 일관된 예외 처리를 보장합니다.backend/src/main/java/moadong/club/payload/dto/ClubDetailedResult.java (1)
52-53: ClubRecruitmentStatus 열거형 사용으로 업데이트되었습니다.이전
getRecruitmentStatus()대신getClubRecruitmentStatus()를 사용하여 새로운 열거형 구조와 일관성을 유지합니다. 이는 프로젝트 내 모집 상태 처리를 일관되게 유지하는 데 중요합니다.backend/src/main/java/moadong/club/entity/Club.java (2)
13-13: 적절한 임포트 변경입니다.
ClubDescriptionUpdateRequest대신ClubRecruitmentInfoUpdateRequest를 임포트하여 새로운 요청 모델 구조를 반영합니다.
80-80: 메서드 매개변수 타입이 적절하게 업데이트되었습니다.
update메서드가 이제ClubRecruitmentInfoUpdateRequest타입을 매개변수로 받아들이도록 변경되었습니다. 이는 새로운 요청 모델 구조와 일치합니다.backend/src/main/java/moadong/club/enums/ClubRecruitmentStatus.java (1)
9-12: 우선순위 값이 실제 노출 의도와 일치하는지 재확인 필요현재 priority:
ALWAYS(1) < OPEN(2) < CLOSED(3) < UPCOMING(4)입니다.
일반적으로 “모집중(OPEN)” 이 “상시모집(ALWAYS)” 보다 상위에 노출되길 원하는 경우도 있으니, 검색/정렬 요구사항과 일치하는지 제품 측에 다시 확인하시기 바랍니다.backend/src/main/java/moadong/club/service/ClubProfileService.java (1)
50-53: 스케줄 재등록 시 기존 두 개의 Future 모두 취소되지 않을 수 있습니다
RecruitmentScheduler.scheduleRecruitment내부 버그(동일 key 덮어쓰기)로 인해 start/end 중 하나만 취소됩니다.
위 Scheduler 개선이 완료되면 이곳에서도 정상 동작을 확인해 주세요.
| public static ClubRecruitmentStatus fromString(String status) { | ||
| return Arrays.stream(values()) | ||
| .filter(rs -> rs.name().equalsIgnoreCase(status)) | ||
| .findFirst() | ||
| .orElse(null); | ||
| } | ||
| public static int getPriorityFromString(String status) { | ||
| ClubRecruitmentStatus rs = fromString(status); | ||
| return (rs != null) ? rs.getPriority() : Integer.MAX_VALUE; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
null 반환 대신 명시적 예외 또는 Optional 사용을 고려하세요
fromString 이 일치하지 않을 경우 null 을 반환하고, 이어서 getPriorityFromString 에서 Integer.MAX_VALUE 로 대체합니다.
호출자가 null 을 간과할 가능성이 높으므로 다음과 같은 개선이 안전합니다.
- public static ClubRecruitmentStatus fromString(String status) {
- return Arrays.stream(values())
- .filter(rs -> rs.name().equalsIgnoreCase(status))
- .findFirst()
- .orElse(null);
- }
+ public static Optional<ClubRecruitmentStatus> fromString(String status) {
+ return Arrays.stream(values())
+ .filter(rs -> rs.name().equalsIgnoreCase(status))
+ .findFirst();
+ }그 후 getPriorityFromString 에서 orElseThrow 또는 기본값을 선택하는 편이 null-safety 측면에서 더 낫습니다.
Committable suggestion skipped: line range outside the PR's diff.
| public void updateRecruitmentStatus(ClubRecruitmentStatus status) { | ||
| this.clubRecruitmentStatus = status; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
상태 전이 규칙 검증이 없습니다
updateRecruitmentStatus 가 아무 검증 없이 상태를 덮어쓰고 있습니다.
모집 시작/종료 시점과 연동하여 유효한 상태 전이(예: UPCOMING → OPEN → CLOSED)를 강제하지 않으면 데이터가 불일치할 수 있습니다.
상태 전이 표를 정의하고, 잘못된 전이가 들어올 경우 예외를 던지도록 추가 검증을 권장합니다.
| public String createClub(ClubCreateRequest request) { | ||
| Club club = Club.builder() | ||
| .name(request.name()) | ||
| .category(request.category()) | ||
| .division(request.division()) | ||
| .build(); | ||
| clubRepository.save(club); | ||
|
|
||
| return club.getId(); |
There was a problem hiding this comment.
생성 시 userId 누락으로 이후 권한 검사가 실패합니다
validateClubUpdateRequest 에서 user.getId() 와 club.getUserId() 를 비교하지만, createClub 에서는 userId 필드를 세팅하지 않아 null 비교가 발생합니다.
생성 시점에 소유자 정보를 저장하도록 수정해 주세요.
- Club club = Club.builder()
- .name(request.name())
- .category(request.category())
- .division(request.division())
- .build();
+ Club club = Club.builder()
+ .name(request.name())
+ .category(request.category())
+ .division(request.division())
+ .userId(request.userId()) // 또는 user.getId()
+ .build();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String createClub(ClubCreateRequest request) { | |
| Club club = Club.builder() | |
| .name(request.name()) | |
| .category(request.category()) | |
| .division(request.division()) | |
| .build(); | |
| clubRepository.save(club); | |
| return club.getId(); | |
| public String createClub(ClubCreateRequest request) { | |
| Club club = Club.builder() | |
| .name(request.name()) | |
| .category(request.category()) | |
| .division(request.division()) | |
| .userId(request.userId()) // 또는 user.getId() | |
| .build(); | |
| clubRepository.save(club); | |
| return club.getId(); | |
| } |
3 participants
📝작업 내용
Summary by CodeRabbit
신규 기능
기능 개선
버그 수정
리팩터링