Conversation
|
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check | Most changes align with admin page implementation goals (controllers, services, DTOs), but WebConfiguration CORS changes and Member.modifyMemberRole() appear tangential to core admin page feature and lack clear justification in PR description. | Clarify whether CORS configuration and member role modification are essential to admin page functionality, or consider separating them into a separate PR for better code organization and clear scope boundaries. | |
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Description check | ❓ Inconclusive | The PR description follows the template structure with a linked issue reference, but key sections (작업 내용, 주요 변경사항) are empty with only placeholders, lacking concrete implementation details. | Fill in the '작업 내용' and '주요 변경사항' sections with specific details about the admin page features implemented (controllers, services, DTOs, database queries). |
| Linked Issues check | ❓ Inconclusive | The linked issue #1107 contains only placeholder sections without concrete tasks, requirements, or acceptance criteria. Cannot fully validate whether code changes meet specific requirements due to empty issue content. | The linked issue #1107 needs concrete task specifications with acceptance criteria to properly validate code changes against requirements. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title '[BE][feat]: 어드민 페이지 구현' clearly summarizes the main feature: implementing an admin page backend functionality. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (25)
backend/src/main/java/backend/mulkkam/common/config/WebConfiguration.java-71-71 (1)
71-71: 허용된 Origin이 하드코딩되어 있습니다.
http://localhost:5173이 코드에 직접 작성되어 있어 다음과 같은 문제가 있습니다:
- 환경별 배포 불가: 개발/스테이징/프로덕션 환경에서 각각 다른 Origin을 사용해야 하는데 코드 수정 없이는 불가능합니다.
- 보안 위험: 프로덕션 환경에서 localhost를 허용하는 것은 의미가 없으며, 실제 도메인을 추가하려면 재배포가 필요합니다.
개선 방안:
application.yml에 환경별 설정을 외부화하고@Value로 주입받는 방식을 권장합니다.♻️ 제안 코드
application.yml
cors: allowed-origins: http://localhost:5173 # application-prod.yml cors: allowed-origins: https://your-production-domain.comWebConfiguration.java
+ @Value("${cors.allowed-origins}") + private String allowedOrigins; @Bean public FilterRegistrationBean<CorsFilter> corsFilter() { UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource(); CorsConfiguration config = new CorsConfiguration(); config.setAllowCredentials(true); - config.addAllowedOrigin("http://localhost:5173"); + config.setAllowedOrigins(List.of(allowedOrigins.split(","))); config.addAllowedHeader("*"); config.addAllowedMethod("*"); source.registerCorsConfiguration("/**", config); FilterRegistrationBean<CorsFilter> filterBean = new FilterRegistrationBean<>(new CorsFilter(source)); - filterBean.setOrder(Ordered.HIGHEST_PRECEDENCE); // 가장 먼저 실행 + filterBean.setOrder(Ordered.HIGHEST_PRECEDENCE); return filterBean; }참고: 여러 Origin을 지원하려면 쉼표로 구분하여 설정할 수 있습니다 (예:
http://localhost:5173,https://staging.example.com).Also applies to: 82-82
backend/src/main/java/backend/mulkkam/common/config/WebConfiguration.java-68-90 (1)
68-90: CORS 설정이 중복되어 있습니다.현재
addCorsMappings(Spring MVC 레벨)과corsFilter(Filter 레벨) 두 가지 방식으로 CORS를 동시에 설정하고 있습니다. 이렇게 되면 다음과 같은 문제가 발생할 수 있습니다:
- 헤더 중복: 요청이 Filter와 MVC CORS 처리를 모두 거치면서 CORS 헤더가 중복으로 추가될 수 있습니다.
- 설정 충돌: 두 설정이 불일치할 경우 예측하기 어려운 동작이 발생할 수 있습니다.
- 유지보수 부담: 설정 변경 시 두 곳을 모두 수정해야 합니다.
대안:
Option 1 (권장):
CorsFilterBean만 사용
- 장점: Filter 체인에서 가장 먼저 실행되어 모든 요청에 일관되게 적용, Spring Security와도 잘 통합됨
- 단점: 없음
Option 2:
addCorsMappings만 사용
- 장점: 간결하고 Spring MVC의 표준 방식
- 단점: Filter보다 늦게 실행되므로, 인증 실패 등으로 MVC에 도달하지 못하면 CORS 헤더가 누락될 수 있음
어드민 페이지처럼 인증이 필요한 엔드포인트가 많다면 Option 1을 권장합니다.
addCorsMappings메서드는 제거하고corsFilter만 유지하세요.♻️ Option 1 적용 시 제안 코드
- @Override - public void addCorsMappings(CorsRegistry registry) { - registry.addMapping("/**") - .allowedOrigins("http://localhost:5173") - .allowedMethods("GET", "POST", "PUT", "DELETE", "PATCH", "OPTIONS") - .allowedHeaders("*") - .allowCredentials(true); - } - @Bean public FilterRegistrationBean<CorsFilter> corsFilter() { UrlBasedCorsConfigurationSource source = new UrlBasedCorsConfigurationSource();backend/src/main/java/backend/mulkkam/admin/service/AdminDeviceService.java-24-29 (1)
24-29: 예외 처리 일관성 개선 필요Admin 패키지 내 예외 처리 전략이 불일치합니다.
AdminDeviceService에서는IllegalArgumentException을 사용하지만,AdminMemberService는CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)패턴을 따르고 있습니다.현황:
AdminMemberService:CommonException사용 (권장 패턴)AdminDeviceService,AdminCupService,AdminIntakeHistoryService,AdminFriendRelationService,AdminNotificationService:IllegalArgumentException사용 (비일관적)문제점:
- API 응답 형식이 일관되지 않음
- 에러 처리 로직의 예측 가능성 감소
- i18n 지원 어려움
개선 방향:
모든 Admin 서비스를CommonException+NotFoundErrorCode패턴으로 통일해야 합니다. 다행히NotFoundErrorCode.NOT_FOUND_DEVICE가 이미 정의되어 있어 바로 적용 가능합니다.@Transactional public void deleteDevice(Long deviceId) { Device device = deviceRepository.findById(deviceId) - .orElseThrow(() -> new IllegalArgumentException("Device not found: " + deviceId)); + .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_DEVICE)); deviceRepository.delete(device); }이와 함께
AdminCupService,AdminIntakeHistoryService,AdminFriendRelationService,AdminNotificationService도 동일한 패턴으로 일관성 있게 수정되어야 합니다.backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java-63-66 (1)
63-66: 에러 로그에 stacktrace가 남도록log.error(..., e)형태로 남기는 게 좋습니다.
지금은e.getMessage()만 찍혀서 운영 장애 시 원인 추적이 어렵습니다.제안 diff
- } catch (Exception e) { - log.error("[Admin SQL] ERROR - adminId: {}, sql: {}, error: {}", adminMemberId, sql, e.getMessage()); - return ExecuteAdminSqlResponse.error(e.getMessage()); - } + } catch (Exception e) { + log.error("[Admin SQL] ERROR - adminId: {}, sql: {}", adminMemberId, sql, e); + return ExecuteAdminSqlResponse.error(e.getMessage()); + }backend/src/main/java/backend/mulkkam/admin/dto/request/ExecuteAdminSqlRequest.java-6-12 (1)
6-12: 요청 검증에 길이 제한(@SiZe)도 추가하는 게 안전합니다.
@NotBlank만 있으면 매우 긴 SQL(수십~수백 KB)로 인해 로그/DB 부하, 응답 지연을 유발할 수 있습니다. 보안 성격의 엔드포인트라서 “입력 크기”를 명시적으로 제한하는 편이 운영 리스크가 낮습니다.
- 선택지 A: DTO에
@Size(max = …)추가 → 가장 단순, 장점: 즉시 방어 / 단점: 운영 중 필요 시 상향 조정 필요- 선택지 B: 서버 레벨(max request size) + Rate limit로 제어 → 장점: 전역 정책 / 단점: 엔드포인트별 세밀 제어는 어려움
제안 diff
import io.swagger.v3.oas.annotations.media.Schema; import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Size; @Schema(description = "어드민 SQL 쿼리 실행 요청") public record ExecuteAdminSqlRequest( @Schema(description = "실행할 SQL 쿼리", example = "SELECT * FROM member LIMIT 10") @NotBlank(message = "SQL 쿼리는 필수입니다.") + @Size(max = 5000, message = "SQL 쿼리는 5000자 이하여야 합니다.") String sql ) { }backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java-29-30 (1)
29-30: 예외 처리 방식의 팀 내 일관성을 확인해주세요.현재
IllegalArgumentException을 사용하고 있는데, 동일한 admin 패키지 내AdminMemberService는CommonException과NotFoundErrorCode.NOT_FOUND_MEMBER를 사용합니다.각 접근 방식의 장단점:
IllegalArgumentException 사용 (현재 방식)
- 장점: 간단하고 빠르게 구현 가능
- 단점: 클라이언트가 에러 타입을 구분하기 어려움, 일관된 에러 응답 형식 제공 불가
CommonException 사용 (AdminMemberService 방식)
- 장점: 팀 전체에서 정의한 에러 코드 체계 활용, 클라이언트 친화적인 에러 응답, 중앙 집중식 예외 관리
- 단점: 에러 코드 정의 필요
제안:
AdminMemberService가CommonException을 사용하는 것으로 보아, 팀의 표준 예외 처리 방식은 커스텀 예외 체계인 것으로 보입니다. 다른 admin 서비스들(AdminDeviceService,AdminFriendRelationService등)도 함께CommonException으로 통일하는 것을 권장합니다.♻️ 제안하는 수정
먼저
NotFoundErrorCode에NOT_FOUND_INTAKE_HISTORY추가가 필요합니다:// NotFoundErrorCode.java에 추가 NOT_FOUND_INTAKE_HISTORY("섭취 기록을 찾을 수 없습니다.")그 다음 Service 코드 수정:
+import backend.mulkkam.common.exception.CommonException; +import backend.mulkkam.common.exception.errorcode.NotFoundErrorCode; @Transactional(readOnly = true) public GetAdminIntakeHistoryDetailResponse getIntakeHistory(Long intakeHistoryId) { IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) - .orElseThrow(() -> new IllegalArgumentException("IntakeHistory not found: " + intakeHistoryId)); + .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_INTAKE_HISTORY)); return GetAdminIntakeHistoryDetailResponse.from(intakeHistory); } @Transactional public void updateIntakeHistory(Long intakeHistoryId, UpdateAdminIntakeHistoryRequest request) { IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) - .orElseThrow(() -> new IllegalArgumentException("IntakeHistory not found: " + intakeHistoryId)); + .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_INTAKE_HISTORY)); intakeHistory.modifyTargetAmount(new TargetAmount(request.targetAmount())); } @Transactional public void deleteIntakeHistory(Long intakeHistoryId) { IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) - .orElseThrow(() -> new IllegalArgumentException("IntakeHistory not found: " + intakeHistoryId)); + .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_INTAKE_HISTORY)); intakeHistoryRepository.delete(intakeHistory); }참고:
AdminMemberService의 예외 처리 방식을 기준으로 제안드렸습니다.Also applies to: 36-37, 43-44
backend/src/main/java/backend/mulkkam/admin/dto/response/GetAdminIntakeHistoryDetailItemResponse.java-14-22 (1)
14-22: 메서드 체이닝 규칙(one dot per line)을 준수해주세요.Lines 19-20에서
detail.getIntakeAmount().value(),detail.getCupEmojiUrl().value()형태로 메서드가 체이닝되어 있습니다.팀의 "one dot per line" 규칙을 따르면:
- 중간 값을 변수로 추출하여 각 단계를 명확히 구분
- NPE 발생 시 정확한 원인 지점 파악 용이
- 디버깅 및 유지보수성 향상
♻️ 제안하는 리팩토링
public static GetAdminIntakeHistoryDetailItemResponse from(IntakeHistoryDetail detail) { + IntakeAmount intakeAmountObj = detail.getIntakeAmount(); + Integer intakeAmount = intakeAmountObj != null ? intakeAmountObj.value() : null; + + CupEmojiUrl cupEmojiUrlObj = detail.getCupEmojiUrl(); + String cupEmojiUrl = cupEmojiUrlObj != null ? cupEmojiUrlObj.value() : null; + return new GetAdminIntakeHistoryDetailItemResponse( detail.getId(), detail.getIntakeTime(), detail.getIntakeType(), - detail.getIntakeAmount() != null ? detail.getIntakeAmount().value() : null, - detail.getCupEmojiUrl() != null ? detail.getCupEmojiUrl().value() : null + intakeAmount, + cupEmojiUrl ); }참고: 코딩 가이드라인에 명시된 내용입니다.
backend/src/main/java/backend/mulkkam/admin/dto/response/GetAdminIntakeHistoryDetailResponse.java-18-35 (1)
18-35: 메서드 체이닝 규칙(one dot per line)을 준수해주세요.현재 코드에서 여러 곳에서 메서드 체이닝이 한 줄에 여러 개의 점(
.)으로 연결되어 있습니다(예:history.getMember().getMemberNickname(),history.getIntakeHistoryDetails().stream()).팀 코딩 가이드라인에 따르면 "한 줄에 하나의 점(.)"만 사용해야 하는데, 이 규칙을 따르지 않으면:
- 디버깅 시 중간 값을 확인하기 어렵습니다
- NullPointerException 발생 시 정확한 원인 파악이 어렵습니다
- 코드 가독성이 떨어집니다
중간 변수를 추출하여 각 단계를 명확히 분리하면 위 문제들을 해결할 수 있습니다.
♻️ 제안하는 리팩토링
public static GetAdminIntakeHistoryDetailResponse from(IntakeHistory history) { - List<GetAdminIntakeHistoryDetailItemResponse> details = history.getIntakeHistoryDetails() != null - ? history.getIntakeHistoryDetails().stream() - .map(GetAdminIntakeHistoryDetailItemResponse::from) - .toList() - : List.of(); + List<GetAdminIntakeHistoryDetailItemResponse> details = List.of(); + List<IntakeHistoryDetail> historyDetails = history.getIntakeHistoryDetails(); + if (historyDetails != null) { + details = historyDetails.stream() + .map(GetAdminIntakeHistoryDetailItemResponse::from) + .toList(); + } + + Member member = history.getMember(); + MemberNickname memberNicknameObj = member.getMemberNickname(); + String memberNickname = memberNicknameObj != null ? memberNicknameObj.value() : null; + + TargetAmount targetAmountObj = history.getTargetAmount(); + Integer targetAmount = targetAmountObj != null ? targetAmountObj.value() : null; return new GetAdminIntakeHistoryDetailResponse( history.getId(), - history.getMember().getId(), - history.getMember().getMemberNickname() != null ? history.getMember().getMemberNickname().value() : null, + member.getId(), + memberNickname, history.getHistoryDate(), - history.getTargetAmount() != null ? history.getTargetAmount().value() : null, + targetAmount, history.getStreak(), details, history.getCreatedAt() ); }참고: 코딩 가이드라인에 명시된 내용입니다.
backend/src/main/java/backend/mulkkam/admin/dto/response/GetAdminIntakeHistoryListResponse.java-17-28 (1)
17-28: 메서드 체이닝 규칙(one dot per line)을 준수해주세요.Lines 21, 23, 25에서 메서드 체이닝이 한 줄에 여러 개의 점으로 연결되어 있습니다. 이는 다른 response DTO들과 동일한 패턴의 문제입니다.
중간 변수를 추출하여 각 단계를 명확히 분리하는 것을 권장합니다.
♻️ 제안하는 리팩토링
public static GetAdminIntakeHistoryListResponse from(IntakeHistory history) { + Member member = history.getMember(); + MemberNickname memberNicknameObj = member.getMemberNickname(); + String memberNickname = memberNicknameObj != null ? memberNicknameObj.value() : null; + + TargetAmount targetAmountObj = history.getTargetAmount(); + Integer targetAmount = targetAmountObj != null ? targetAmountObj.value() : null; + + List<IntakeHistoryDetail> historyDetails = history.getIntakeHistoryDetails(); + int detailCount = historyDetails != null ? historyDetails.size() : 0; + return new GetAdminIntakeHistoryListResponse( history.getId(), - history.getMember().getId(), - history.getMember().getMemberNickname() != null ? history.getMember().getMemberNickname().value() : null, + member.getId(), + memberNickname, history.getHistoryDate(), - history.getTargetAmount() != null ? history.getTargetAmount().value() : null, + targetAmount, history.getStreak(), - history.getIntakeHistoryDetails() != null ? history.getIntakeHistoryDetails().size() : 0, + detailCount, history.getCreatedAt() ); }참고: 코딩 가이드라인에 명시된 내용입니다.
backend/src/main/java/backend/mulkkam/admin/controller/AdminIntakeHistoryController.java-31-73 (1)
31-73: 인수 테스트 케이스 작성이 필요합니다가이드라인에 따르면 컨트롤러는 인수 테스트로 검증해야 합니다. 다음 테스트 케이스들을 고려해보세요:
1. 인증/인가 테스트
@RequireAuth(level = ADMIN)검증: ADMIN 권한 없이 접근 시 403 반환- 일반 사용자(USER 레벨)로 접근 시 차단 확인
2. 목록 조회 테스트 (
GET /admin/intake-histories)
- 페이징 파라미터에 따른 정상 조회 (size=10 기본값 포함)
- 빈 목록 조회 시 응답 구조 확인
- 잘못된 페이징 파라미터 처리 (음수 page, 과도한 size 등)
3. 상세 조회 테스트 (
GET /admin/intake-histories/{intakeHistoryId})
- 존재하는 ID로 조회 성공
- 존재하지 않는 ID로 조회 시 적절한 에러 응답 (현재는 500, 권장은 404)
4. 수정 테스트 (
PUT /admin/intake-histories/{intakeHistoryId})
- 유효한 targetAmount로 수정 성공 및 204 응답
- Validation 실패 케이스 (@Valid 검증): 음수, null 등
- 존재하지 않는 ID로 수정 시 에러 응답
5. 삭제 테스트 (
DELETE /admin/intake-histories/{intakeHistoryId})
- 존재하는 데이터 삭제 성공 및 204 응답
- 존재하지 않는 ID로 삭제 시 에러 응답
- 삭제 후 조회 시 Not Found 확인 (idempotency)
테스트 작성 예시:
@SpringBootTest @AutoConfigureMockMvc class AdminIntakeHistoryControllerTest { @Autowired private MockMvc mockMvc; @Test @WithMockUser(authorities = "USER") // ADMIN이 아닌 경우 void 일반_사용자는_어드민_API_접근_불가() throws Exception { mockMvc.perform(get("/admin/intake-histories")) .andExpect(status().isForbidden()); } @Test @WithMockUser(authorities = "ADMIN") void 섭취기록_목록_조회_성공() throws Exception { mockMvc.perform(get("/admin/intake-histories") .param("page", "0") .param("size", "10")) .andExpect(status().isOk()) .andExpect(jsonPath("$.content").isArray()); } // ... 추가 테스트 케이스들 }가이드라인 기준입니다.
backend/src/main/java/backend/mulkkam/admin/controller/AdminIntakeHistoryController.java-26-31 (1)
26-31: 어노테이션 순서와 클래스 선언 전 공백 라인 추가가 필요합니다코딩 가이드라인에 따르면 두 가지 규칙을 위반하고 있습니다:
어노테이션 순서: 현재
@RequiredArgsConstructor(Lombok)가@RequireAuth,@RestController,@RequestMapping(Spring 관련) 사이에 위치하고 있습니다. 가이드라인에서는 "logs, Lombok, Spring meta, Spring components (most important, placed last)" 순서를 요구합니다.클래스 선언 전 공백: Line 25(import 끝)와 Line 26(@tag) 사이에 공백 라인이 필요합니다.
♻️ 제안하는 수정안
import org.springframework.web.bind.annotation.RestController; + @Tag(name = "어드민 - 섭취 기록 관리", description = "어드민 섭취 기록 관리 API") +@RequiredArgsConstructor @RequireAuth(level = AuthLevel.ADMIN) -@RequiredArgsConstructor @RestController @RequestMapping("/admin/intake-histories") public class AdminIntakeHistoryController {가이드라인 기준입니다.
backend/src/main/java/backend/mulkkam/admin/controller/AdminIntakeHistoryController.java-47-51 (1)
47-51: CommonException과 ErrorCode 패턴으로 예외 처리 일관성 맞추기현재 서비스 레이어에서
IllegalArgumentException을 사용하고 있는데, 이는 GlobalExceptionHandler의 일반 Exception 핸들러에 의해 500 에러(INTER_SERVER_ERROR_CODE)로 변환됩니다. 이로 인해:
- HTTP 상태 코드 부정확: "찾을 수 없음"이 500 서버 에러로 응답되어 클라이언트가 잘못된 정보를 받습니다.
- 일관성 부족: 프로젝트의 예외 처리 패턴(CommonException + ErrorCode)과 맞지 않습니다.
- 로깅 부정확: 클라이언트 에러(404)를 서버 에러 로그로 기록합니다.
개선 방향 (기존 패턴 적용):
프로젝트는
CommonException과ErrorCode열거형으로 구성된 예외 처리 인프라를 이미 갖추고 있습니다. NotFoundErrorCode에 필요한 에러 코드들이 이미 정의되어 있으므로, 다음과 같이 수정하면 됩니다:// 현재 (문제) IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) .orElseThrow(() -> new IllegalArgumentException("IntakeHistory not found: " + intakeHistoryId)); // 개선 (기존 패턴 적용) IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_INTAKE_HISTORY));이렇게 하면 GlobalExceptionHandler에서 자동으로 404 상태 코드로 응답하고, 구조화된 에러 응답(error code 포함)을 클라이언트에 전달할 수 있습니다.
backend/src/main/java/backend/mulkkam/admin/controller/AdminNotificationController.java-43-51 (1)
43-51: 전체 회원 브로드캐스트가 동기 처리되어 확장성 문제가 발생할 수 있습니다.현재 구현은
200 OK를 반환하여 동기 처리를 의미하는데,AdminNotificationService.sendBroadcastNotification()을 보면 청크 단위로 전체 회원을 순회하며 알림을 저장하고 발송합니다. 회원 수가 많아질 경우:
- 요청 타임아웃: 웹 요청이 장시간 블로킹되어 타임아웃 발생 가능
- 트랜잭션 보유 시간:
@Transactional내에서 긴 작업이 실행되어 커넥션 풀 고갈 위험- 사용자 경험: 어드민이 요청 완료까지 기다려야 함
개선 방안:
옵션 1 (권장): 비동기 작업으로 전환
@Async를 사용하거나 별도 작업 큐(예: Spring Batch, 메시지 큐)로 처리202 Accepted+ 작업 ID 반환- 진행 상황 조회 API 제공
옵션 2: 현재 구조 유지하되 제한 조건 명시
- 소규모 서비스에서만 사용 가능
- 타임아웃 설정 증가
- 문서에 제약사항 명시
현재 회원 규모와 예상 증가율에 따라 판단하시되, 확장성을 고려하면 옵션 1을 권장합니다.
backend/src/main/java/backend/mulkkam/admin/service/AdminNotificationService.java-37-41 (1)
37-41: 메서드 체이닝과 네이밍 컨벤션을 개선해주세요.두 가지 코딩 가이드라인 위반이 있습니다:
한 줄에 하나의 점 규칙: Line 39-40에서
notificationRepository.findAll(pageable).map(...)로 체이닝되어 있습니다.메서드 네이밍: 가이드라인에 따르면 복수 결과를 반환하는 경우
read를 사용해야 합니다. 현재getNotifications는readNotifications가 더 적합합니다.♻️ 개선 제안
@Transactional(readOnly = true) -public Page<GetAdminNotificationListResponse> getNotifications(Pageable pageable) { - return notificationRepository.findAll(pageable) - .map(GetAdminNotificationListResponse::from); +public Page<GetAdminNotificationListResponse> readNotifications(Pageable pageable) { + Page<Notification> notifications = notificationRepository.findAll(pageable); + return notifications.map(GetAdminNotificationListResponse::from); }Controller에서도 메서드명 변경이 필요합니다.
backend/src/main/java/backend/mulkkam/admin/dto/response/GetAdminNotificationListResponse.java-16-26 (1)
16-26: 팩토리 메서드에서 메서드 체이닝과 널 안전성 개선이 필요합니다.현재 구현에서 여러 가지 개선 가능한 부분이 있습니다:
메서드 체이닝 위반: 코딩 가이드라인에 따르면 "한 줄에 하나의 점(.)"을 사용해야 합니다. Line 19-20에서
notification.getMember().getId(),notification.getMember().getMemberNickname()같은 체이닝이 반복됩니다.널 안전성 문제: Line 20에서
getMemberNickname() != null체크 후.value()를 호출하는데,getMemberNickname()이null이 아닌 빈 Optional을 반환하거나, 체크와 호출 사이에 상태가 변경될 가능성이 있다면 NPE 위험이 있습니다. 또한getMember()자체가null일 가능성도 검증되지 않았습니다.개선 방안:
- 중간 변수를 활용해 메서드 체이닝을 분리하고 널 체크를 명확히 하세요.
Optional을 사용한다면map(),orElse()등을 활용해 안전하게 처리하세요.♻️ 개선 제안
public static GetAdminNotificationListResponse from(Notification notification) { + Member member = notification.getMember(); + String nickname = (member.getMemberNickname() != null) + ? member.getMemberNickname().value() + : null; + return new GetAdminNotificationListResponse( notification.getId(), - notification.getMember().getId(), - notification.getMember().getMemberNickname() != null ? notification.getMember().getMemberNickname().value() : null, + member.getId(), + nickname, notification.getNotificationType(), notification.getContent(), notification.isRead(), notification.getCreatedAt() ); }backend/src/main/java/backend/mulkkam/admin/service/AdminNotificationService.java-100-105 (1)
100-105: 예외 처리를 팀의 기존 패턴에 맞춰CommonException과 에러코드로 개선하세요.현재
IllegalArgumentException을 사용하고 있는데, 이는 몇 가지 문제가 있습니다:
팀 컨벤션 불일치: AdminMemberService에서 동일한 not-found 상황을
CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)로 처리하고 있습니다. 이것이 팀의 기존 패턴입니다.글로벌 예외 핸들러 미지원: GlobalExceptionHandler에는
CommonException에 대한 핸들러가 있지만,IllegalArgumentException에 대한 핸들러가 없어 예상치 못한 에러 응답이 발생할 수 있습니다.일관성 문제: AdminIntakeHistoryService, AdminCupService 등 다른 Admin 서비스들도 같은 문제를 갖고 있어 팀 전체의 예외 처리 일관성이 떨어집니다.
개선 방향:
@Transactional public void deleteNotification(Long notificationId) { Notification notification = notificationRepository.findById(notificationId) .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_NOTIFICATION)); notificationRepository.delete(notification); }필요하면
NotFoundErrorCode.NOT_FOUND_NOTIFICATION을 에러코드에 추가하거나, 기존NOT_FOUND_*패턴을 재사용하세요. 이렇게 하면 GlobalExceptionHandler에서 올바르게 처리되고, 팀 컨벤션도 따를 수 있습니다.다른 Admin 서비스들(AdminCupService, AdminIntakeHistoryService, AdminDeviceService 등)도 동일하게 수정 필요합니다.
backend/src/main/java/backend/mulkkam/admin/controller/AdminFriendRelationController.java-21-47 (1)
21-47: 테스트 커버리지 제안 - 어드민 API 인수 테스트 필요현재 어드민 친구 관계 관리 기능에 대한 테스트 코드가 제공되지 않았습니다. 가이드라인에서 제안하는 대로 컨트롤러는 인수 테스트(Acceptance Test)를 작성하는 것을 권장합니다.
필수 테스트 시나리오:
1. 친구 관계 목록 조회 테스트 (GET /admin/friend-relations)
// 성공 케이스 - 관리자 권한으로 친구 관계 목록 조회 시 200 OK와 페이징된 데이터 반환 - 페이지 크기(size) 파라미터가 올바르게 적용되는지 확인 - 빈 목록일 때도 정상 응답 (empty page) // 실패 케이스 - 비인증 요청 시 401 Unauthorized - 일반 사용자 권한으로 요청 시 403 Forbidden - 잘못된 페이지 파라미터(음수 등) 시 400 Bad Request2. 친구 관계 삭제 테스트 (DELETE /admin/friend-relations/{friendRelationId})
// 성공 케이스 - 존재하는 친구 관계 삭제 시 204 No Content 반환 - 삭제 후 실제로 DB에서 제거되었는지 확인 // 실패 케이스 - 존재하지 않는 ID로 삭제 시 적절한 에러 응답 (현재 500이 아닌 404 권장) - 비인증 요청 시 401 Unauthorized - 일반 사용자 권한으로 요청 시 403 Forbidden - 잘못된 ID 형식(문자열 등) 시 400 Bad Request3. 서비스 계층 단위 테스트
AdminFriendRelationService에 대해서도:
- Repository 모킹을 통한 getFriendRelations 테스트
- 존재하지 않는 ID 삭제 시 예외 발생 확인
- 트랜잭션 동작 검증
추가 고려사항:
- 동시성 테스트: 동일한 친구 관계를 동시에 삭제하는 경우
- 데이터 정합성: 삭제 후 연관 데이터에 영향이 없는지 확인
테스트 작성을 통해 리팩토링 안정성을 확보하고, 향후 예외 처리 전략 변경 시에도 기능 보장이 가능합니다.
Do you want me to generate sample test code or open a new issue to track this task?
backend/src/main/java/backend/mulkkam/admin/service/AdminFriendRelationService.java-24-29 (1)
24-29: 공통 예외 처리 체계로의 통일 필요 - IllegalArgumentException의 HTTP 상태 코드 문제 포함현재
IllegalArgumentException을 사용하고 있는데, 이는 두 가지 측면에서 문제가 있습니다:1. 코드베이스 내 일관성 문제:
- AdminDeviceService, AdminCupService, AdminIntakeHistoryService →
IllegalArgumentException사용- AdminMemberService →
CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)사용2. HTTP 상태 코드 오류 (더 심각):
- 현재
IllegalArgumentException은 GlobalExceptionHandler에 정의된 핸들러가 없어서, 제네릭@ExceptionHandler(Exception.class)로 처리됨- 결과적으로 404(NOT_FOUND)가 아닌 500(INTERNAL_SERVER_ERROR) 응답을 반환
- 리소스 없음 상황이 서버 오류로 왜곡되어 클라이언트 혼란 야기
권장 방안:
두 가지 선택지가 있습니다. FriendRelation 도메인의 특성을 고려하여 선택하면 됩니다:선택지 1: 전용 에러 코드 생성 (권장)
- NotFoundErrorCode에
NOT_FOUND_FRIEND_RELATION추가- 도메인 리소스마다 구체적인 에러 코드로 디버깅 용이
선택지 2: 기존 에러 코드 재사용
- 기존
NOT_FOUND_FRIEND사용 (FriendRelation이 Friend 관계를 표현하는 엔티티라면)- 에러 코드 증가를 최소화하되, FriendRelation의 의미가 반영되는지 검토 필요
둘 다 다음 형태로 구현됩니다:
@Transactional public void deleteFriendRelation(Long friendRelationId) { FriendRelation friendRelation = friendRelationRepository.findById(friendRelationId) .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_FRIEND_RELATION)); // 또는 .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_FRIEND)) friendRelationRepository.delete(friendRelation); }효과:
- GlobalExceptionHandler에서 일관된 404 응답 처리
- 에러 코드 기반 클라이언트 처리 및 i18n 지원
- 다른 어드민 서비스(Device, Cup, IntakeHistory)도 함께 마이그레이션하여 전체 예외 처리 전략 통일 필요
backend/src/main/java/backend/mulkkam/admin/dto/response/GetAdminMemberListResponse.java-17-27 (1)
17-27: 정적 팩토리 메서드와 메서드 체이닝 규칙 위반
GetAdminMemberDetailResponse와 동일한 컨벤션 위반이 발생하고 있습니다:
정적 팩토리 메서드: 가이드라인에서 DTO 생성 시 정적 팩토리 메서드를 사용하지 않도록 명시하고 있습니다.
메서드 체이닝 규칙: Lines 20, 21, 22, 23에서 한 줄에 두 개 이상의 메서드 호출이 있어 "one dot per line" 규칙을 위반하고 있습니다.
추가 고려사항:
GetAdminMemberDetailResponse와 이 클래스 간에 Member 필드를 매핑하는 로직이 중복되고 있습니다. 만약 매핑 로직을 유지해야 한다면, 공통 매핑 유틸리티나 Mapper 클래스를 별도로 분리하는 것을 고려해볼 수 있습니다.장점:
- 중복 코드 제거
- 매핑 로직의 일관성 보장
- 변경 시 한 곳만 수정하면 됨
단점:
- 추가 클래스가 필요하여 구조가 약간 복잡해질 수 있음
Based on coding guidelines.
backend/src/main/java/backend/mulkkam/admin/dto/response/GetAdminMemberDetailResponse.java-20-33 (1)
20-33: 정적 팩토리 메서드 사용을 피하고, 메서드 체이닝 규칙을 준수해 주세요현재 코드는 두 가지 코딩 컨벤션을 위반하고 있습니다:
정적 팩토리 메서드 사용: 코딩 가이드라인에서 "Do not use static factory methods; use new operator for domain and DTO creation"을 명시하고 있습니다.
from()메서드 대신 생성자를 직접 호출하는 방식으로 변경이 필요합니다.메서드 체이닝 규칙 위반: "Use one dot per line" 가이드라인을 위반하고 있습니다 (Lines 23, 24, 25, 26). 각 메서드 호출을 별도의 줄로 분리하여 가독성을 높여야 합니다.
문제점:
- 정적 팩토리 메서드는 팀 컨벤션과 맞지 않아 일관성이 떨어집니다
- 메서드 체이닝은 디버깅 시 어느 단계에서 문제가 발생했는지 파악하기 어렵고, 가독성이 떨어집니다
대안:
호출하는 측에서 직접 생성자를 사용하도록 변경하거나, 만약 매핑 로직이 복잡하여 헬퍼가 필요하다면 별도의 Mapper 클래스를 고려할 수 있습니다.♻️ 메서드 체이닝 규칙을 준수하는 수정안
만약 팩토리 메서드를 유지해야 한다면, 최소한 메서드 체이닝 규칙은 준수해야 합니다:
public static GetAdminMemberDetailResponse from(Member member) { + MemberNickname memberNickname = member.getMemberNickname(); + String nickname = memberNickname.value(); + + PhysicalAttributes physicalAttributes = member.getPhysicalAttributes(); + Gender gender = physicalAttributes.getGender(); + Double weight = physicalAttributes.getWeight(); + + TargetAmount targetAmount = member.getTargetAmount(); + Integer targetAmountValue = targetAmount.value(); + return new GetAdminMemberDetailResponse( member.getId(), - member.getMemberNickname().value(), - member.getPhysicalAttributes().getGender(), - member.getPhysicalAttributes().getWeight(), - member.getTargetAmount().value(), + nickname, + gender, + weight, + targetAmountValue, member.isMarketingNotificationAgreed(), member.isNightNotificationAgreed(), member.isReminderEnabled(), member.getMemberRole(), member.getCreatedAt() ); }Based on coding guidelines.
backend/src/main/java/backend/mulkkam/admin/dto/response/CheckAdminResponse.java-11-18 (1)
11-18: 정적 팩토리 메서드 사용과 DTO 내 비즈니스 로직 배치 검토다음 사항들을 개선할 필요가 있습니다:
정적 팩토리 메서드: 다른 Response DTO들과 동일하게, 코딩 가이드라인에서 DTO 생성 시 정적 팩토리 메서드 사용을 지양하도록 명시하고 있습니다.
DTO 내 비즈니스 로직: Line 16에서
finishedOnboarding = (role != MemberRole.NONE)으로 온보딩 완료 여부를 결정하고 있습니다.문제점:
- DTO는 데이터 전송 객체로, 변환 로직만 포함해야 합니다. 비즈니스 규칙(온보딩 완료 판단)은 도메인이나 서비스 레이어에 있어야 합니다.
- 온보딩 완료 조건이 변경되면(예: 추가 정보 입력 필요) DTO를 수정해야 하는데, 이는 단일 책임 원칙에 위배됩니다.
- 현재 로직으로는
MemberRole과 온보딩 상태가 강하게 결합되어 있어, 향후 온보딩 상태를 별도로 추적해야 할 경우 문제가 될 수 있습니다.대안:
MemberDetails또는Member도메인 객체에isFinishedOnboarding()메서드를 추가하고, 여기서 해당 값을 가져오도록 변경- 온보딩 상태를 별도의 필드로 관리하여 명확하게 분리
장점:
- 비즈니스 로직이 적절한 계층에 위치
- 온보딩 조건 변경 시 도메인 레이어만 수정하면 됨
- 테스트가 용이해짐
♻️ 수정 제안
MemberDetails나 Member에 메서드 추가:
// MemberDetails 또는 Member 클래스에 public boolean isFinishedOnboarding() { return this.memberRole != MemberRole.NONE; }그리고 DTO에서는 단순히 매핑만:
public static CheckAdminResponse from(MemberDetails memberDetails) { - MemberRole role = memberDetails.memberRole(); return new CheckAdminResponse( memberDetails.isAdmin(), - role, - role != MemberRole.NONE + memberDetails.memberRole(), + memberDetails.isFinishedOnboarding() ); }Based on coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
backend/src/main/java/backend/mulkkam/admin/dto/request/UpdateAdminMemberRequest.java-7-31 (1)
7-31: Bean Validation 애노테이션 추가 및 파라미터 명명 규칙 적용검증 로직 구현과 코드 규칙 적용이 필요합니다:
1. 검증 애노테이션 부재
현재
@Schema에는 제약사항이 명시되어 있지만(닉네임 2~10자,목표 섭취량 200~5000ml등), 실제 검증을 수행하는 Bean Validation 애노테이션이 없습니다. 더욱이 컨트롤러(line 81)에서@RequestBody에@Valid가 없어 검증이 실행되지 않습니다.문제점:
- 클라이언트가 전송한 잘못된 데이터가 서비스 레이어에 도달하므로, 비즈니스 로직에서 방어 코드가 필요해집니다
- 검증 규칙이 분산되면 누락되거나 불일치할 가능성이 높습니다
- API 문서의 제약과 실제 동작이 맞지 않을 수 있습니다
2. 타입 일관성 및 파라미터 명명
targetAmount는 primitiveint인데,weight는 wrapperDouble입니다. DTO에서는 null 여부를 명확히 하기 위해 일반적으로 wrapper 타입을 사용합니다- 컨트롤러 line 81의 파라미터명
request는 DTO 클래스명과 일치해야 합니다 →updateAdminMemberRequest로 변경 필요♻️ 개선 예시
+import jakarta.validation.constraints.Max; +import jakarta.validation.constraints.Min; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Size; public record UpdateAdminMemberRequest( @Schema(description = "닉네임 (2~10자)", example = "홍길동") + @NotNull + @Size(min = 2, max = 10) String nickname, @Schema(description = "성별", example = "MALE") + @NotNull Gender gender, @Schema(description = "체중 (kg)", example = "70.0") + @NotNull + @Min(0) Double weight, @Schema(description = "목표 섭취량 (200~5000ml)", example = "2000") + @Min(200) + @Max(5000) - int targetAmount, + Integer targetAmount, @Schema(description = "회원 권한", example = "MEMBER") + @NotNull MemberRole memberRole ) { }컨트롤러:
@PutMapping("/members/{memberId}") public GetAdminMemberDetailResponse updateMember( @PathVariable Long memberId, - @RequestBody UpdateAdminMemberRequest request + @Valid @RequestBody UpdateAdminMemberRequest updateAdminMemberRequest ) { - return adminMemberService.updateMember(memberId, request); + return adminMemberService.updateMember(memberId, updateAdminMemberRequest); }Committable suggestion skipped: line range outside the PR's diff.
backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java-41-55 (1)
41-55: 회원 역할 변경의 보안 및 정책 검증 필요Line 52에서
member.modifyMemberRole(request.memberRole())을 통해 회원의 역할을 변경하고 있습니다. 이는 매우 민감한 작업으로, 다음과 같은 보안 및 정책 검증이 필요합니다:보안 우려 사항:
- 어드민 권한 상승 공격: 일반 회원을 ADMIN으로 승격시킬 수 있어, 잘못된 요청이나 의도하지 않은 조작으로 권한 상승 가능
- 마지막 어드민 문제: 시스템의 마지막 ADMIN을 MEMBER로 강등시키면 어드민이 없어져 시스템 관리 불가
- 자기 자신의 권한 변경: 현재 로그인한 ADMIN이 자기 자신의 권한을 실수로 변경할 가능성
- 감사 로그 누락: 이런 중요한 권한 변경 작업에 대한 로깅이나 감사 추적이 있는지 불명확
권장 사항:
정책 검증 추가:
- 자기 자신의 권한은 변경할 수 없도록 제한
- 시스템에 최소 1명의 ADMIN은 항상 유지되도록 보장
- ADMIN 권한 부여 시 2차 인증이나 별도 승인 프로세스 고려
감사 로그 추가:
- 누가, 언제, 누구의 권한을 무엇에서 무엇으로 변경했는지 기록
역할 변경을 별도 엔드포인트로 분리:
- 일반 회원 정보 수정과 역할 변경을 분리하여 더 엄격한 검증 적용
🔒 역할 변경 보안 강화 예시
@Transactional - public GetAdminMemberDetailResponse updateMember(Long memberId, UpdateAdminMemberRequest request) { + public GetAdminMemberDetailResponse updateMember( + Long memberId, + UpdateAdminMemberRequest request, + Long currentAdminId + ) { Member member = memberRepository.findById(memberId) .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)); + // 자기 자신의 권한은 변경할 수 없음 + if (memberId.equals(currentAdminId) && !member.getMemberRole().equals(request.memberRole())) { + throw new CommonException(/* 적절한 에러 코드 */); + } + + // ADMIN을 MEMBER로 강등 시, 다른 ADMIN이 존재하는지 확인 + if (member.getMemberRole() == MemberRole.ADMIN && request.memberRole() == MemberRole.MEMBER) { + long adminCount = memberRepository.countByMemberRole(MemberRole.ADMIN); + if (adminCount <= 1) { + throw new CommonException(/* 마지막 ADMIN 삭제 불가 에러 */); + } + } member.updateNickname(new MemberNickname(request.nickname())); member.updatePhysicalAttributes(new PhysicalAttributes(request.gender(), request.weight())); member.updateTargetAmount(new TargetAmount(request.targetAmount())); member.modifyIsMarketingNotificationAgreed(request.isMarketingNotificationAgreed()); member.modifyIsNightNotificationAgreed(request.isNightNotificationAgreed()); member.modifyIsReminderEnabled(request.isReminderEnabled()); member.modifyMemberRole(request.memberRole()); + // TODO: 역할 변경 감사 로그 추가 + // auditLogService.logRoleChange(currentAdminId, memberId, oldRole, newRole); return GetAdminMemberDetailResponse.from(member); }또는 역할 변경을 완전히 별도 메서드/엔드포인트로 분리:
@Transactional public void changeMemberRole( Long targetMemberId, MemberRole newRole, Long currentAdminId ) { // 더 엄격한 검증 로직 validateRoleChange(targetMemberId, newRole, currentAdminId); Member member = memberRepository.findById(targetMemberId) .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)); MemberRole oldRole = member.getMemberRole(); member.modifyMemberRole(newRole); auditLogService.logRoleChange(currentAdminId, targetMemberId, oldRole, newRole); }Committable suggestion skipped: line range outside the PR's diff.
backend/src/main/java/backend/mulkkam/admin/controller/AdminMemberController.java-72-84 (1)
72-84: 요청 검증 누락 및 응답 방식 불일치Line 81에서
@Valid어노테이션이 누락되어 요청 데이터 검증이 수행되지 않습니다. 다른 어드민 컨트롤러들(AdminCupController,AdminIntakeHistoryController)은 모두@Valid를 사용하고 있어 일관성이 떨어집니다.또한 업데이트 작업의 응답 방식도 불일치합니다:
- 이 컨트롤러:
GetAdminMemberDetailResponse반환 (200 OK)- 다른 어드민 컨트롤러들:
ResponseEntity<Void>반환 (204 No Content)문제점:
@Valid누락으로 잘못된 요청 데이터가 서비스 계층까지 전달될 수 있음- 응답 방식 불일치로 API 일관성 저하 및 클라이언트 구현 복잡도 증가
장단점 비교:
- 200 OK + 응답 바디: 클라이언트가 업데이트 결과를 즉시 확인 가능하지만, 추가 조회 없이도 최신 상태를 받을 수 있어 편리함. 단, RESTful 관례상 PUT 업데이트는 204를 주로 사용
- 204 No Content: RESTful 관례에 부합하고 응답 크기 감소. 클라이언트가 최신 상태 필요 시 별도 조회 필요
🐛 @Valid 어노테이션 추가 및 응답 방식 통일 제안
필수: @Valid 어노테이션 추가
@PutMapping("/members/{memberId}") public GetAdminMemberDetailResponse updateMember( @PathVariable Long memberId, - @RequestBody UpdateAdminMemberRequest request + @Valid @RequestBody UpdateAdminMemberRequest request ) {선택: 다른 어드민 컨트롤러와 동일하게 204 반환
@Operation(summary = "회원 정보 수정", description = "특정 회원의 정보를 수정합니다.") - @ApiResponse(responseCode = "200", description = "수정 성공") + @ApiResponse(responseCode = "204", description = "수정 성공") @ApiResponse(responseCode = "401", description = "인증 실패") @ApiResponse(responseCode = "403", description = "권한 없음") @ApiResponse(responseCode = "404", description = "회원을 찾을 수 없음") @RequireAuth(level = AuthLevel.ADMIN) @PutMapping("/members/{memberId}") - public GetAdminMemberDetailResponse updateMember( + public ResponseEntity<Void> updateMember( @PathVariable Long memberId, - @RequestBody UpdateAdminMemberRequest request + @Valid @RequestBody UpdateAdminMemberRequest request ) { - return adminMemberService.updateMember(memberId, request); + adminMemberService.updateMember(memberId, request); + return ResponseEntity.noContent().build(); }As per coding guidelines, ...
backend/src/main/java/backend/mulkkam/admin/controller/AdminMemberController.java-28-32 (1)
28-32: 어노테이션 순서 및 클래스 레벨 인증 설정 개선 필요현재 어노테이션 순서가 팀 컨벤션과 일치하지 않습니다. 코딩 가이드라인에 따르면 "Annotation order: logs, Lombok, Spring meta, Spring components (most important, placed last)"를 따라야 합니다.
또한 다른 어드민 컨트롤러들(
AdminDeviceController,AdminCupController,AdminIntakeHistoryController)은 클래스 레벨에@RequireAuth(level = AuthLevel.ADMIN)을 선언하고 있는데, 이 컨트롤러만 메서드 레벨에서 개별적으로 인증을 처리하고 있어 일관성이 떨어집니다.문제점:
- 어노테이션 순서 불일치로 코드 가독성 저하
/admin/check엔드포인트는MEMBER레벨, 나머지는ADMIN레벨로 혼재되어 있어 의도를 파악하기 어려움- 다른 어드민 컨트롤러와 패턴이 달라 유지보수 시 혼란 가능
대안:
- 현재 구조 유지 + 어노테이션 순서만 수정 (추천):
/admin/check가 의도적으로MEMBER레벨이라면, 메서드 레벨 인증을 유지하되 어노테이션 순서를 수정- 클래스 레벨 인증 + 별도 컨트롤러 분리:
checkAdmin을 별도 컨트롤러로 분리하고 이 컨트롤러는 클래스 레벨에ADMIN적용♻️ 대안 1: 어노테이션 순서 수정 (checkAdmin이 의도적으로 MEMBER 레벨인 경우)
@Tag(name = "어드민 - 회원 관리", description = "어드민 회원 관리 API") @RequiredArgsConstructor @RestController @RequestMapping("/admin") + public class AdminMemberController {어노테이션 순서를 다음과 같이 변경:
-@Tag(name = "어드민 - 회원 관리", description = "어드민 회원 관리 API") -@RequiredArgsConstructor -@RestController -@RequestMapping("/admin") +@RequiredArgsConstructor +@Tag(name = "어드민 - 회원 관리", description = "어드민 회원 관리 API") +@RestController +@RequestMapping("/admin") + public class AdminMemberController {대안 2: 다른 어드민 컨트롤러와 동일하게 클래스 레벨 인증 적용 (checkAdmin도 ADMIN 레벨로 통일)
+@Tag(name = "어드민 - 회원 관리", description = "어드민 회원 관리 API") +@RequireAuth(level = AuthLevel.ADMIN) @RequiredArgsConstructor -@Tag(name = "어드민 - 회원 관리", description = "어드민 회원 관리 API") @RestController @RequestMapping("/admin") + public class AdminMemberController {그리고
checkAdmin메서드에서@RequireAuth제거:@Operation(summary = "어드민 권한 확인", description = "현재 로그인한 사용자가 어드민인지 확인합니다.") @ApiResponse(responseCode = "200", description = "성공 응답") @ApiResponse(responseCode = "401", description = "인증 실패") - @RequireAuth(level = AuthLevel.MEMBER) @GetMapping("/check")Based on coding guidelines, ...
🟡 Minor comments (5)
backend/src/main/java/backend/mulkkam/admin/controller/AdminSqlController.java-20-47 (1)
20-47: 애노테이션 순서를 팀 컨벤션에 맞춰 정렬해주세요.현재 코드:
@Tag(...) @RequireAuth(level = AuthLevel.ADMIN) @RequiredArgsConstructor @RestController @RequestMapping("/admin/sql") public class AdminSqlController {팀 가이드라인(logs → Lombok → Spring meta → Spring components 순)에 따르면
@RequiredArgsConstructor가 Lombok 애노테이션이므로 맨 앞에 와야 합니다.@RestController와@RequestMapping같은 Spring 컴포넌트는 그 뒤에, 가장 중요한@RestController는 클래스 선언에 더 가까워야 합니다:수정 제안
@RequiredArgsConstructor @Tag(name = "어드민 - SQL 실행", description = "어드민 SQL 직접 실행 API (주의: 강력한 권한)") @RequireAuth(level = AuthLevel.ADMIN) @RestController @RequestMapping("/admin/sql") public class AdminSqlController {
(보안) 역할 체크만으로는 부족할 수 있어 추가 가드레일을 강조합니다.
현재 구현은 ADMIN 역할 확인과 위험한 SQL 명령어(DROP, TRUNCATE, ALTER 등) 차단 및 로깅은 잘 되어 있으나, 역할이 뚫리거나 계정이 탈취되는 순간 피해가 치명적입니다. 실무에서는 다층 방어를 권장합니다:
선택지 A: 운영 환경 자체에서 차단 (권장)
- 프로덕션 환경에서는 엔드포인트를 비활성화하거나 특정 IP/VPN/점프 호스트만 허용
- 장점: 근본적으로 악용 반경을 제거
- 단점: 운영 중 긴급 데이터 수정 시 절차 필요
선택지 B: 2차 인증 + 강화된 감사 추적
- 기능 플래그나 OTP/토큰 기반 승인 프로세스 추가
- 감사 로그를 별도 저장소(예: 읽기 전용 DB, 외부 감시 시스템)에 기록
- 장점: 운영에서도 제어된 사용 가능
- 단점: 구현과 운영 복잡도 증가
선택지 C: 읽기 전용(SELECT만)으로 축소
- 장점: 데이터 파괴 가능성 원천 차단
- 단점: 긴급 데이터 수정 목적 상실
팀의 보안 정책과 운영 필요성을 고려해 선택해주세요.
테스트 케이스 추가를 제안합니다.
컨트롤러 레벨의 인수 테스트를 통해 다음을 검증해주세요:
- ADMIN 권한 없이 엔드포인트 접근 거부
- 유효한 SELECT/INSERT/UPDATE/DELETE 쿼리 실행 성공
- 차단된 명령어(DROP, TRUNCATE 등) 거부 및 에러 메시지 확인
- @Valid 검증 실패 시 에러 응답
backend/src/main/java/backend/mulkkam/admin/dto/response/GetAdminCupDetailResponse.java-29-29 (1)
29-29: 메서드 체이닝 규칙 위반 (한 줄에 하나의 점)
cup.getCupEmoji().getCode().getValue()는 한 줄에 세 개의 점이 있어 팀 코딩 컨벤션을 위배합니다. 이는 가독성을 저하시키고 디버깅을 어렵게 만듭니다.🔧 수정 제안
- cup.getCupEmoji() != null && cup.getCupEmoji().getCode() != null ? cup.getCupEmoji().getCode().getValue() : null, + extractCupEmojiCode(cup),그리고 헬퍼 메서드 추가:
private static String extractCupEmojiCode(Cup cup) { if (cup.getCupEmoji() == null) { return null; } CupEmoji emoji = cup.getCupEmoji(); if (emoji.getCode() == null) { return null; } return emoji.getCode().getValue(); }팀 코딩 가이드라인 기준입니다.
Committable suggestion skipped: line range outside the PR's diff.
backend/src/main/java/backend/mulkkam/admin/controller/AdminCupController.java-56-59 (1)
56-59: RequestBody 파라미터 변수명을 DTO 클래스명과 일치시켜야 함팀의 기존 컨벤션에 따르면, Controller에서 @RequestBody 파라미터의 변수명은 해당 DTO 클래스명과 일치시켜야 합니다. 현재
request로 되어 있지만,updateAdminCupRequest로 변경해야 합니다.이유:
- 코드 일관성 유지 - 다른 컨트롤러들과 동일한 네이밍 규칙 적용
- 가독성 향상 - 변수명만 보고도 어떤 DTO인지 명확히 파악 가능
- IDE 자동완성 효율성 - DTO 타입과 변수명이 연관되어 검색이 용이
🔧 수정 제안
public ResponseEntity<Void> updateCup( @PathVariable Long cupId, - @Valid @RequestBody UpdateAdminCupRequest request + @Valid @RequestBody UpdateAdminCupRequest updateAdminCupRequest ) { - adminCupService.updateCup(cupId, request); + adminCupService.updateCup(cupId, updateAdminCupRequest); return ResponseEntity.noContent().build(); }Based on learnings, 팀의 기존 PR #716에서 확립된 컨벤션입니다.
backend/src/main/java/backend/mulkkam/admin/service/AdminNotificationService.java-84-89 (1)
84-89: 메서드 네이밍에 오타가 있습니다.Line 84의
savedNotifications는 과거형이 아닌 기본형 동사saveNotifications가 되어야 합니다. 코딩 가이드라인에 따르면 생성/저장 작업은create,save,register,add중 하나를 사용하며, 여기서는save가 적절합니다.🔧 수정 제안
-private void savedNotifications(List<Long> memberIds, NotificationMessageTemplate template) { +private void saveNotifications(List<Long> memberIds, NotificationMessageTemplate template) { List<NotificationInsertDto> notificationInsertDtos = memberIds.stream() .map(memberId -> new NotificationInsertDto(template, memberId)) .toList(); notificationBatchRepository.batchInsert(notificationInsertDtos, CHUNK_SIZE); }Line 80에서 호출하는 부분도 함께 수정하세요.
Committable suggestion skipped: line range outside the PR's diff.
backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java-57-60 (1)
57-60: AdminMemberService.deleteMember의 불필요한 역할 지정 코드 정리 필요Line 59에서
MemberRole.MEMBER를 하드코딩하여 전달하고 있는데, 이 값이MemberService.delete()내부에서 전혀 사용되지 않습니다. 실제로 delete 메서드는memberDetails.id()만 활용하여 회원을 조회하고 삭제하며, 역할 정보는 무시됩니다.더 중요한 점은 권한 검증이 이미
AdminMemberController의@RequireAuth(level = AuthLevel.ADMIN)어노테이션으로 컨트롤러 레벨에서 적절히 처리되고 있다는 것입니다. 따라서 서비스 레벨에서 하드코딩된 역할은 코드의 의도를 혼란스럽게 만드는 불필요한 요소입니다.개선 방안:
회원을 조회하여 실제 역할을 전달하거나, 역할 정보가 필요 없다면 코드를 단순화하는 것이 좋습니다:
♻️ 실제 회원 역할 전달 (일관성 개선)
@Transactional public void deleteMember(Long memberId) { - memberService.delete(new MemberDetails(memberId, MemberRole.MEMBER)); + Member member = memberRepository.findById(memberId) + .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)); + memberService.delete(new MemberDetails(member)); }이 방식은 MemberDetails의 설계 의도(Member 객체를 받는 생성자)와도 일치하며, 테스트 코드의 패턴과도 일관됩니다.
Committable suggestion skipped: line range outside the PR's diff.
| @Transactional | ||
| public void sendBroadcastNotification(SendAdminBroadcastNotificationRequest request) { | ||
| NotificationMessageTemplate template = new NotificationMessageTemplate( | ||
| request.title(), | ||
| request.body(), | ||
| Action.GO_HOME, | ||
| NotificationType.NOTICE | ||
| ); | ||
|
|
||
| Long lastId = null; | ||
|
|
||
| while (true) { | ||
| List<Long> memberIds = getAllMemberIds(lastId); | ||
|
|
||
| if (memberIds.isEmpty()) { | ||
| break; | ||
| } | ||
|
|
||
| saveAndSendNotifications(memberIds, template); | ||
|
|
||
| if (isLastChunk(memberIds)) { | ||
| break; | ||
| } | ||
|
|
||
| lastId = memberIds.getLast(); | ||
| } | ||
| } |
There was a problem hiding this comment.
브로드캐스트 메서드의 트랜잭션 범위와 에러 처리 전략에 문제가 있습니다.
현재 구현의 여러 위험 요소:
1. 트랜잭션 범위 문제 (Critical):
- 전체 회원 대상 알림을 하나의
@Transactional안에서 처리합니다. - 회원이 10만 명이라면 100개 청크를 순회하며 DB 커넥션을 장시간 점유합니다.
- 데이터베이스 커넥션 풀 고갈 및 타임아웃 위험이 큽니다.
2. 부분 실패 시나리오 (Critical):
- 청크 50/100에서 FCM 발송이 실패하면?
- 이미 저장된 49개 청크의 알림은 롤백되지만, FCM은 이미 발송되었을 수 있습니다.
- 재시도 시 중복 발송 문제가 발생합니다.
3. 이벤트 발행 시점 (Major):
- Line 61에서
saveAndSendNotifications는 트랜잭션 내에서 이벤트를 발행합니다. - 이벤트 리스너가 동기적으로 실패하면 전체 롤백, 비동기라면 트랜잭션 커밋 전 처리될 수 있습니다.
개선 방안:
옵션 1 (강력 권장): 청크별 독립 트랜잭션
@Transactional
public void sendBroadcastNotification(SendAdminBroadcastNotificationRequest request) {
NotificationMessageTemplate template = createTemplate(request);
Long lastId = null;
while (true) {
List<Long> memberIds = getAllMemberIds(lastId);
if (memberIds.isEmpty()) break;
// 별도 트랜잭션으로 분리
sendBroadcastChunk(memberIds, template);
if (isLastChunk(memberIds)) break;
lastId = memberIds.getLast();
}
}
@Transactional(propagation = Propagation.REQUIRES_NEW)
public void sendBroadcastChunk(List<Long> memberIds, NotificationMessageTemplate template) {
savedNotifications(memberIds, template);
sendNotifications(memberIds, template);
}옵션 2: 완전 비동기 작업 큐 전환
- Spring Batch 또는 메시지 큐 활용
- 재시도 및 실패 처리 메커니즘 내장
- 진행 상황 추적 가능
현재 구조는 소규모 서비스에서만 안전합니다. 확장을 고려한다면 옵션 1 또는 2를 적용하세요.
| // 위험한 SQL 명령어 차단 | ||
| private static final Set<String> BLOCKED_COMMANDS = Set.of( | ||
| "DROP", "TRUNCATE", "ALTER", "CREATE", "GRANT", "REVOKE", | ||
| "RENAME", "REPLACE", "LOAD", "CALL", "EXECUTE" | ||
| ); | ||
|
|
||
| // SQL 타입 판별용 패턴 | ||
| private static final Pattern SELECT_PATTERN = Pattern.compile("^\\s*SELECT\\s+", Pattern.CASE_INSENSITIVE); | ||
| private static final Pattern INSERT_PATTERN = Pattern.compile("^\\s*INSERT\\s+", Pattern.CASE_INSENSITIVE); | ||
| private static final Pattern UPDATE_PATTERN = Pattern.compile("^\\s*UPDATE\\s+", Pattern.CASE_INSENSITIVE); | ||
| private static final Pattern DELETE_PATTERN = Pattern.compile("^\\s*DELETE\\s+", Pattern.CASE_INSENSITIVE); | ||
|
|
||
| public ExecuteAdminSqlResponse executeSql(ExecuteAdminSqlRequest request, Long adminMemberId) { | ||
| String sql = request.sql().trim(); | ||
|
|
||
| // 위험한 명령어 체크 | ||
| String upperSql = sql.toUpperCase(); | ||
| for (String blocked : BLOCKED_COMMANDS) { | ||
| if (upperSql.contains(blocked)) { | ||
| log.warn("[Admin SQL] BLOCKED - adminId: {}, sql: {}", adminMemberId, sql); | ||
| return ExecuteAdminSqlResponse.error("차단된 명령어입니다: " + blocked); | ||
| } | ||
| } | ||
|
|
||
| // 로그 기록 | ||
| log.info("[Admin SQL] EXECUTE - adminId: {}, sql: {}", adminMemberId, sql); | ||
|
|
||
| long startTime = System.currentTimeMillis(); | ||
|
|
||
| try { | ||
| if (SELECT_PATTERN.matcher(sql).find()) { | ||
| return executeSelect(sql, startTime); | ||
| } else if (INSERT_PATTERN.matcher(sql).find()) { | ||
| return executeModify(sql, "INSERT", startTime); | ||
| } else if (UPDATE_PATTERN.matcher(sql).find()) { | ||
| return executeModify(sql, "UPDATE", startTime); | ||
| } else if (DELETE_PATTERN.matcher(sql).find()) { | ||
| return executeModify(sql, "DELETE", startTime); | ||
| } else { | ||
| return ExecuteAdminSqlResponse.error("지원하지 않는 쿼리 타입입니다. SELECT, INSERT, UPDATE, DELETE만 가능합니다."); | ||
| } | ||
| } catch (Exception e) { | ||
| log.error("[Admin SQL] ERROR - adminId: {}, sql: {}, error: {}", adminMemberId, sql, e.getMessage()); | ||
| return ExecuteAdminSqlResponse.error(e.getMessage()); | ||
| } |
There was a problem hiding this comment.
(치명적) 차단 로직(contains) + 원문 SQL 로깅은 우회/정보유출 리스크가 큽니다.
현재 방식은 (1) DR/**/OP 같은 주석/공백/DB별 문법으로 키워드 우회 가능성이 있고, (2) 반대로 컬럼명/문자열 리터럴에 포함된 단어로 오탐 가능성이 있으며, (3) 무엇보다 SQL 원문을 info로 남기면 PII/비밀값이 로그로 유출될 수 있습니다.
권장 가드레일(조합 추천):
- 선택지 A(강력): “단일 statement”만 허용(세미콜론 금지), 주석 토큰(
--,/* */) 금지 + 첫 토큰 allowlist(SELECT/INSERT/UPDATE/DELETE)로만 분기- 장점: 우회 난이도↑ / 단점: 복잡한 쿼리(주석 포함) 제한
- 선택지 B: 로그는 queryType/해시/길이/실행시간만 남기고 원문은 별도 안전한 감사 저장소(접근통제)로 분리
- 장점: 운영 추적 + 로그 유출 리스크↓ / 단점: 감사 저장소 설계 필요
- 선택지 C: SELECT만 허용(앞 코멘트와 동일)
- 장점: 파괴적 변경 방지 / 단점: 기능 축소
최소한으로는 log.info에서 SQL 원문을 제거/마스킹하고, 차단/허용 규칙을 “문자열 포함”이 아니라 “토큰 기반”으로 바꾸는 것을 권합니다.
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In @backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java:
- Around line 30-33: The getCups method currently chains two calls on one line
(cupRepository.findAll(pageable).map(GetAdminCupListResponse::from)); split this
into two statements to follow the "one dot per line" rule: first call
cupRepository.findAll(pageable) and assign the Page result to a local variable,
then call .map(GetAdminCupListResponse::from) on that variable and return the
mapped Page; update getCups accordingly so each method invocation (findAll and
map) appears on its own line.
- Line 41: The method declaration updateCup(Long cupId, UpdateAdminCupRequest
request) in AdminCupService violates the parameter-formatting rule for 2+
parameters; reformat the method signature so each parameter is on its own line
(or parameters are line-broken) in the declaration (not the call site) — locate
the updateCup method and adjust its signature formatting accordingly to match
the project's multiline-parameter style.
- Around line 42-44: The code currently chains
repository.findById(...).orElseThrow(...) for both cupRepository and
cupEmojiRepository which violates the method-chaining rule; in AdminCupService
split each into two statements: first call findById(...) and store the Optional
(e.g., Optional<Cup> cupOpt = cupRepository.findById(cupId)), then call
cupOpt.orElseThrow(() -> new CommonException(NOT_FOUND_CUP)) to obtain the Cup;
do the same for cupEmojiRepository (use Optional<CupEmoji> cupEmojiOpt and then
cupEmojiOpt.orElseThrow(...)); this makes debugging easier and clarifies intent
while keeping the same exceptions and repository/findById/orElseThrow usage.
- Around line 35-38: The chained repository call in getCup should follow the
"one method per line" rule: split cupRepository.findById(cupId).orElseThrow(...)
into two lines so the .orElseThrow(...) is on its own line; locate the getCup
method and replace the single-line chain that uses
cupRepository.findById(cupId).orElseThrow(() -> new
CommonException(NOT_FOUND_CUP)) with a two-line chain that preserves the same
behavior and exception (CommonException with NOT_FOUND_CUP).
In
@backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java:
- Around line 25-28: getIntakeHistories currently chains two calls on one line
(intakeHistoryRepository.findAll(pageable).map(GetAdminIntakeHistoryListResponse::from)),
violating the "one dot per line" rule; refactor by breaking the chain into
separate statements so each dot is on its own line—e.g., assign
intakeHistoryRepository.findAll(pageable) to a local Page variable and then call
.map(GetAdminIntakeHistoryListResponse::from) on the next line—or place the .map
call on its own line, keeping method names getIntakeHistories,
intakeHistoryRepository.findAll, and GetAdminIntakeHistoryListResponse::from to
locate the code.
- Line 23: Remove the final modifier from the IntakeHistoryRepository field
declaration in AdminIntakeHistoryService: change the field "private final
IntakeHistoryRepository intakeHistoryRepository;" to be non-final and adjust any
constructors or usages if necessary (constructor injection can remain as-is);
ensure the field style matches other admin services like AdminDeviceService and
AdminCupService for consistency across the codebase.
- Around line 30-34: The method getIntakeHistory violates the one-dot-per-line
chaining rule; fix it by breaking the chained calls into separate statements:
call intakeHistoryRepository.findById(intakeHistoryId) and store the Optional in
a variable, call optional.orElseThrow(() -> new
CommonException(NOT_FOUND_INTAKE_HISTORY)) on the next line to get
IntakeHistory, then call GetAdminIntakeHistoryDetailResponse.from(intakeHistory)
on its own line and return the result, referencing getIntakeHistory,
intakeHistoryRepository.findById, orElseThrow, CommonException,
NOT_FOUND_INTAKE_HISTORY, and GetAdminIntakeHistoryDetailResponse.from.
- Around line 36-41: updateIntakeHistory 메서드에서 한 줄에 두 개 이상의 점 연산자가 사용된
체이닝(intakeHistoryRepository.findById(intakeHistoryId).orElseThrow(...))을 분리하세요:
먼저 intakeHistoryRepository.findById(intakeHistoryId) 호출 결과를 로컬 변수(Optional<
IntakeHistory >)에 할당한 다음 별도 라인에서 .orElseThrow(...)를 호출하여 IntakeHistory
intakeHistory를 얻고, 그 뒤에 intakeHistory.modifyTargetAmount(new
TargetAmount(request.targetAmount())) 를 호출하도록 updateIntakeHistory를 수정하세요; 관련
식별자는 updateIntakeHistory, intakeHistoryRepository.findById, orElseThrow,
IntakeHistory 및 modifyTargetAmount입니다.
In @backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java:
- Line 56: The local variable TIMEOUT_SEC in AdminSqlService should be a
class-level constant; move its declaration out of the method and declare it as a
static final int TIMEOUT_SEC = 10 at the top of the AdminSqlService class,
update any references to the local variable to use this constant, and remove the
original local declaration to follow constant naming conventions and avoid
per-request allocations.
- Line 23: The class-level @Transactional(readOnly = true) on AdminSqlService
conflicts with methods that perform writes; remove the class-level annotation
and apply method-level transactions instead: add @Transactional(readOnly = true)
to executeSelect, and add @Transactional (or @Transactional(readOnly = false))
to the write methods (executeModify and executeSql) so INSERT/UPDATE/DELETE run
in a writable transaction; update imports accordingly and ensure method
signatures remain unchanged.
- Around line 58-90: Currently a new ExecutorService is created per request via
Executors.newSingleThreadExecutor() and shutdownNow(), causing resource churn
and OOM risk; move to a shared, class-level ExecutorService field (e.g., private
final ExecutorService adminSqlExecutor = Executors.newFixedThreadPool(...)) and
reuse it for submit(...) calls in executeAdminSql, add a @PreDestroy method to
shutdown the pool cleanly, remove per-request shutdownNow(), and keep the same
timeout logic (future.get(TIMEOUT_SEC, TimeUnit.SECONDS)) and cancellation
(future.cancel(true)) so symbols to update are
Executors.newSingleThreadExecutor(), executor.submit(...), future.get(...),
future.cancel(true), executor.shutdownNow() and add @PreDestroy shutdown that
calls adminSqlExecutor.shutdownNow() or shutdown+awaitTermination.
- Around line 41-44: The chained normalization call that builds normalizedSql in
AdminSqlService should be split into single-method-per-line statements or
extracted to a dedicated method (e.g., normalizeSql or sanitizeSql) to follow
the "one dot per line" rule; break the replaceAll calls into separate statements
(or move the entire sequence into a new private method like normalizeSql(String
sql) and call it from the current method), so each transformation (remove line
comments, remove block comments, collapse whitespace, trim) is on its own line
and exceptions are easier to trace.
- Around line 38-103: executeSql lacks persistent audit logging, has no
result-size limits, weak SQL-injection defenses, and violates method-chaining
style; fix by: 1) persist each execution into AdminAuditLog (capture
adminMemberId, sql, normalizedSql, startTime, executionTime, affectedRows or
resultCount, success/failure, and error message) from executeSql and update
AdminAuditLog in executeSelect/executeModify paths; 2) prevent OOM by limiting
SELECT results (e.g., enforce a maxRows cap before calling queryForList or
switch to a streaming/limited query and record resultCount) in executeSelect; 3)
harden SQL checks beyond BLOCKED_COMMANDS by applying stricter normalization of
normalizedSql, adding pattern checks for UNION/OR 1=1/comment-escape attempts or
integrate an SQL parser/whitelist approach and reject suspicious queries in
executeSql before submission; and 4) refactor the normalizedSql construction to
avoid multi-dot chaining (split replaceAll/trim into separate statements) to
satisfy style rules.
🧹 Nitpick comments (5)
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java (2)
22-25: 클래스 선언 전 공백 라인 누락코딩 가이드라인에 따르면 클래스 선언 전에 한 줄의 공백이 필요합니다. 현재
@Transactional(readOnly = true)어노테이션 바로 다음 줄에 클래스가 선언되어 있습니다.♻️ 가독성 개선을 위한 수정안
@RequiredArgsConstructor @Service @Transactional(readOnly = true) + public class AdminCupService {Based on coding guidelines, line 22-25.
25-60: 테스트 코드 작성 권장 사항AdminCupService에 대한 단위 테스트가 필요합니다. 다음 테스트 케이스들을 고려해주세요:
getCups 메서드:
- 페이지네이션이 올바르게 동작하는지 검증
- 빈 결과 처리 확인
getCup 메서드:
- 정상 조회 케이스
- cupId가 존재하지 않을 때 NOT_FOUND_CUP 예외 발생 검증
updateCup 메서드:
- 정상 업데이트 케이스 (nickname, cupAmount, intakeType, emoji 모두 변경 확인)
- cupId가 존재하지 않을 때 NOT_FOUND_CUP 예외 발생 검증
- cupEmojiId가 존재하지 않을 때 NOT_FOUND_CUP_EMOJI 예외 발생 검증
- 트랜잭션 롤백 동작 확인
deleteCup 메서드:
- 정상 삭제 케이스
- cupId가 존재하지 않을 때 NOT_FOUND_CUP 예외 발생 검증
- 삭제 후 실제로 DB에서 제거되었는지 검증
테스트 작성 팁:
@ExtendWith(MockitoExtension.class) class AdminCupServiceTest { @Mock private CupRepository cupRepository; @Mock private CupEmojiRepository cupEmojiRepository; @InjectMocks private AdminCupService adminCupService; // 각 테스트 케이스 작성... }컨트롤러 레이어(AdminCupController)에 대해서는 인수 테스트(Acceptance Test)를 작성하여 전체 요청-응답 흐름을 검증하는 것을 권장합니다.
도움이 필요하시면 구체적인 테스트 코드 예시를 작성해드릴 수 있습니다.
Based on coding guidelines, line 25-60.
backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java (1)
43-48: 삭제 로직 최적화 및 메서드 체이닝 규칙 위반문제점 1 (성능):
현재 삭제 로직은 엔티티를 먼저 조회한 후 삭제하는 2단계로 구성되어 있습니다. 이는 불필요한 SELECT 쿼리를 발생시킵니다.문제점 2 (코딩 규칙):
45-46라인에서 메서드 체이닝 규칙을 위반하고 있습니다.대안 1 (권장):
deleteById사용으로 단순화
- Spring Data JPA의
deleteById를 사용하면 내부적으로 존재 여부를 확인한 후 삭제- 장점: 코드 간결화, 불필요한 SELECT 제거
- 단점: 존재하지 않는 ID에 대해 EmptyResultDataAccessException 발생 (CommonException으로 변환 필요)
대안 2: 현재 패턴 유지하되 메서드 체이닝 수정
- 명시적 예외 처리를 유지하면서 코딩 규칙만 준수
- 장점: 일관된 예외 처리, 다른 Admin 서비스와 동일한 패턴
- 단점: SELECT 쿼리 오버헤드
AdminDeviceService, AdminFriendRelationService 등 다른 서비스도 동일한 패턴을 사용하고 있어, 팀 전체적으로 통일된 접근이 필요합니다.
♻️ 대안 1: deleteById 사용 (권장)
@Transactional public void deleteIntakeHistory(Long intakeHistoryId) { - IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) - .orElseThrow(() -> new CommonException(NOT_FOUND_INTAKE_HISTORY)); - intakeHistoryRepository.delete(intakeHistory); + if (!intakeHistoryRepository.existsById(intakeHistoryId)) { + throw new CommonException(NOT_FOUND_INTAKE_HISTORY); + } + intakeHistoryRepository.deleteById(intakeHistoryId); }♻️ 대안 2: 현재 패턴 유지하되 메서드 체이닝 수정
@Transactional public void deleteIntakeHistory(Long intakeHistoryId) { - IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) - .orElseThrow(() -> new CommonException(NOT_FOUND_INTAKE_HISTORY)); + IntakeHistory intakeHistory = intakeHistoryRepository + .findById(intakeHistoryId) + .orElseThrow(() -> new CommonException(NOT_FOUND_INTAKE_HISTORY)); intakeHistoryRepository.delete(intakeHistory); }backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java (2)
61-70: else-if 체인을 전략 패턴이나 Map 기반 디스패칭으로 개선할 수 있습니다.현재 구조는 SQL 타입에 따라 분기하는데, 새로운 SQL 타입(MERGE, REPLACE 등)이 추가되면 계속 else-if가 늘어납니다. 코딩 가이드라인에서도 else 사용을 지양하도록 권장하고 있습니다.
개선 방안:
- 전략 패턴: SqlExecutor 인터페이스와 구현체들
- Map 기반: Pattern을 키로, BiFunction을 값으로 하는 매핑
장단점:
- 전략 패턴: OCP 준수, 테스트 용이, 하지만 클래스 수 증가
- Map 기반: 간결하고 동적 추가 가능, 하지만 타입 안정성 약간 감소
♻️ 제안하는 수정안 (Map 기반)
+ private static final Map<Pattern, BiFunction<String, Long, ExecuteAdminSqlResponse>> SQL_EXECUTORS = Map.of( + SELECT_PATTERN, (sql, startTime) -> executeSelect(sql, startTime), + INSERT_PATTERN, (sql, startTime) -> executeModify(sql, "INSERT", startTime), + UPDATE_PATTERN, (sql, startTime) -> executeModify(sql, "UPDATE", startTime), + DELETE_PATTERN, (sql, startTime) -> executeModify(sql, "DELETE", startTime) + ); + public ExecuteAdminSqlResponse executeSql(ExecuteAdminSqlRequest request, Long adminMemberId) {Future<ExecuteAdminSqlResponse> future = executor.submit(() -> { try { - if (SELECT_PATTERN.matcher(normalizedSql).find()) { - return executeSelect(sql, startTime); - } else if (INSERT_PATTERN.matcher(normalizedSql).find()) { - return executeModify(sql, "INSERT", startTime); - } else if (UPDATE_PATTERN.matcher(normalizedSql).find()) { - return executeModify(sql, "UPDATE", startTime); - } else if (DELETE_PATTERN.matcher(normalizedSql).find()) { - return executeModify(sql, "DELETE", startTime); - } else { - return ExecuteAdminSqlResponse.error("지원하지 않는 쿼리 타입입니다. SELECT, INSERT, UPDATE, DELETE만 가능합니다."); - } + return SQL_EXECUTORS.entrySet() + .stream() + .filter(entry -> entry.getKey().matcher(normalizedSql).find()) + .findFirst() + .map(entry -> entry.getValue().apply(sql, startTime)) + .orElse(ExecuteAdminSqlResponse.error("지원하지 않는 쿼리 타입입니다. SELECT, INSERT, UPDATE, DELETE만 가능합니다.")); } catch (Exception e) {참고: 이 방법도 stream 체이닝으로 한 줄에 여러 점이 있으므로, 가이드라인을 엄격히 따른다면 변수로 분리 필요합니다.
38-103: 테스트 코드가 필요합니다.이 기능은 보안에 매우 민감하고 잘못된 동작 시 데이터 손실/유출로 이어질 수 있으므로, 철저한 테스트가 필수입니다.
제안하는 테스트 케이스:
단위 테스트 (AdminSqlServiceTest):
차단 명령어 테스트
- DROP, TRUNCATE 등 각 차단 명령어 실행 시 에러 응답 확인
- 대소문자 혼합, 공백 추가 등 우회 시도 테스트
- 주석 내 차단 명령어는 허용되어야 함 (false positive 방지)
SQL 타입 감지 테스트
- SELECT, INSERT, UPDATE, DELETE 각각 정상 실행 확인
- 복잡한 쿼리 (서브쿼리, JOIN, CTE 등) 정상 처리 확인
- 지원하지 않는 타입(MERGE, CALL 등) 에러 확인
타임아웃 테스트
- 느린 쿼리 시뮬레이션 (예:
SELECT SLEEP(15))- 타임아웃 시 적절한 에러 응답 확인
- Future가 취소되는지 확인
예외 처리 테스트
- 문법 오류 SQL 실행 시 에러 응답
- 존재하지 않는 테이블 접근 시 에러 처리
- DB 연결 실패 시나리오
트랜잭션 테스트
- INSERT/UPDATE/DELETE가 실제로 커밋되는지 확인
- 에러 발생 시 롤백되는지 확인
통합 테스트 (AdminSqlControllerTest - 인수 테스트):
- 권한 없는 사용자 접근 시 403 응답
- 정상 관리자의 SELECT 실행 및 결과 확인
- 차단 명령어 실행 시 에러 응답
테스트 코드 작성에 도움이 필요하시면, 구체적인 테스트 케이스 구현 예시를 제공해드릴 수 있습니다. 새로운 이슈로 분리해서 진행하시겠습니까?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminDeviceService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminFriendRelationService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminNotificationService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/src/main/java/backend/mulkkam/admin/service/AdminFriendRelationService.java
- backend/src/main/java/backend/mulkkam/admin/service/AdminDeviceService.java
- backend/src/main/java/backend/mulkkam/admin/service/AdminNotificationService.java
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.java
📄 CodeRabbit inference engine (backend/code-style.md)
backend/**/*.java: Use verb-starting method names without entity name, e.g., memberService.findById(Long id)
Avoid else statements; use alternative control flow patterns
Use one dot per line (method chaining rule)
Avoid abbreviations in code
Avoid getters and setters; use behavior-focused methods instead
Annotation order: logs, Lombok, Spring meta, Spring components (most important, placed last)
Add one blank line before class declaration
When method has 2 or more parameters, use line breaks for parameters (does not apply to method calls)
Do not use wildcard imports; apply IntelliJ code conventions for imports
Use method names: create, save, register, add for creation operations
Use method names: read for multiple results, get for always-existing values, find for Optional results
Use method names: update for full replacement (PUT), modify for partial updates (PATCH)
Use method names: delete, remove for deletion operations
Do not use final keyword on class or field declarations
Use uppercase with underscores for constant names, e.g., CONSTANT_NAME
Order class members and methods by access level: public, protected, private; arrange by call order
Files:
backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminCupService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java
backend/**
⚙️ CodeRabbit configuration file
backend/**: - 1. 팀 및 공식 컨벤션, 가독성, 예외처리, 테스트/확장/유지보수성, 모듈화, API/DB/보안 설계 기준을 기반으로 리뷰해주세요.
- 최신 트렌드, 불필요한 로직, 클린코드, 리팩토링, 서비스/도메인 설계, 공통 예외 처리, 확장성도 함께 확인해주세요.
- 각 피드백은 문제점·대안·장단점을 짧고 논리적으로, 예시 코드가 있다면 간결히 포함해 주세요.
- 팀 내 스타일 통일성도 확인해주세요.
- 미작성한 테스트 코드 케이스가 있다면, 어떤 테스트가 필요한지 제안해주세요. (예: 컨트롤러는 인수 테스트, 나머지는 단위 테스트)
- 리뷰 남겨주는 부분은 해당 라인 범위의 코멘트에 작성해주세요.
Files:
backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminCupService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java
🧬 Code graph analysis (2)
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java (3)
backend/src/main/java/backend/mulkkam/admin/service/AdminDeviceService.java (1)
RequiredArgsConstructor(15-33)backend/src/main/java/backend/mulkkam/admin/service/AdminFriendRelationService.java (1)
RequiredArgsConstructor(15-33)backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java (1)
RequiredArgsConstructor(22-61)
backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java (4)
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java (1)
RequiredArgsConstructor(22-60)backend/src/main/java/backend/mulkkam/admin/service/AdminDeviceService.java (1)
RequiredArgsConstructor(15-33)backend/src/main/java/backend/mulkkam/admin/service/AdminFriendRelationService.java (1)
RequiredArgsConstructor(15-33)backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java (1)
RequiredArgsConstructor(22-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java (1)
54-59: 메서드 체이닝 규칙 위반56-57번 라인에서 메서드 체이닝이 발생하고 있습니다.
🔧 수정안
@Transactional public void deleteCup(Long cupId) { Cup cup = cupRepository.findById(cupId) .orElseThrow(() -> new CommonException(NOT_FOUND_CUP)); cupRepository.delete(cup); }참고 - 현재 구현 방식의 장점:
cupRepository.deleteById(cupId)를 사용할 수도 있지만, 현재처럼 먼저 조회 후 삭제하는 방식은 다음과 같은 이점이 있습니다:
- 명시적인 존재 여부 검증으로 일관된 에러 처리 (CommonException)
- JPA의 deleteById 구현체마다 다를 수 있는 예외 처리 방식을 통일
- AdminDeviceService, AdminFriendRelationService와 동일한 패턴으로 일관성 유지
이 설계 선택은 합리적이므로, 메서드 체이닝 규칙만 준수하면 될 것 같습니다.
Based on coding guidelines, line 54-59.
⛔ Skipped due to learnings
Learnt from: CR Repo: woowacourse-teams/2025-mul-kkam PR: 0 File: backend/code-style.md:0-0 Timestamp: 2025-12-15T16:41:54.690Z Learning: Applies to backend/**/*.java : Use method names: delete, remove for deletion operations
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java
Show resolved
Hide resolved
| public class AdminIntakeHistoryService { | ||
|
|
||
| private final IntakeHistoryRepository intakeHistoryRepository; | ||
|
|
||
| public Page<GetAdminIntakeHistoryListResponse> getIntakeHistories(Pageable pageable) { | ||
| return intakeHistoryRepository.findAll(pageable) | ||
| .map(GetAdminIntakeHistoryListResponse::from); | ||
| } | ||
|
|
||
| public GetAdminIntakeHistoryDetailResponse getIntakeHistory(Long intakeHistoryId) { | ||
| IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) | ||
| .orElseThrow(() -> new CommonException(NOT_FOUND_INTAKE_HISTORY)); | ||
| return GetAdminIntakeHistoryDetailResponse.from(intakeHistory); | ||
| } | ||
|
|
||
| @Transactional | ||
| public void updateIntakeHistory(Long intakeHistoryId, UpdateAdminIntakeHistoryRequest request) { | ||
| IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) | ||
| .orElseThrow(() -> new CommonException(NOT_FOUND_INTAKE_HISTORY)); | ||
| intakeHistory.modifyTargetAmount(new TargetAmount(request.targetAmount())); | ||
| } | ||
|
|
||
| @Transactional | ||
| public void deleteIntakeHistory(Long intakeHistoryId) { | ||
| IntakeHistory intakeHistory = intakeHistoryRepository.findById(intakeHistoryId) | ||
| .orElseThrow(() -> new CommonException(NOT_FOUND_INTAKE_HISTORY)); | ||
| intakeHistoryRepository.delete(intakeHistory); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
테스트 코드 작성 제안
현재 서비스 레이어에 대한 단위 테스트가 필요합니다. 다음과 같은 테스트 케이스를 권장합니다:
AdminIntakeHistoryServiceTest (단위 테스트):
-
getIntakeHistories테스트- 페이지네이션이 올바르게 동작하는지 확인
- 빈 결과 처리 확인
-
getIntakeHistory테스트- 정상 조회 시나리오
- 존재하지 않는 ID로 조회 시 NOT_FOUND_INTAKE_HISTORY 예외 발생 확인
-
updateIntakeHistory테스트- 정상 수정 시나리오
- targetAmount가 올바르게 변경되는지 확인
- 존재하지 않는 ID로 수정 시 예외 발생 확인
- 유효하지 않은 targetAmount 값에 대한 검증 (경계값 테스트)
-
deleteIntakeHistory테스트- 정상 삭제 시나리오
- 존재하지 않는 ID로 삭제 시 예외 발생 확인
- 삭제 후 실제로 레포지토리에서 제거되었는지 확인
AdminIntakeHistoryControllerTest (인수 테스트):
컨트롤러 레이어는 별도의 인수 테스트로 엔드투엔드 시나리오를 검증하는 것이 좋습니다.
Based on coding guidelines.
| private static final Set<String> BLOCKED_COMMANDS = Set.of( | ||
| "DROP", "TRUNCATE", "ALTER", "CREATE", "GRANT", "REVOKE", | ||
| "RENAME", "REPLACE", "LOAD", "CALL", "EXECUTE" | ||
| ); | ||
|
|
||
| private static final Pattern SELECT_PATTERN = Pattern.compile("^\\s*SELECT\\s+", Pattern.CASE_INSENSITIVE); | ||
| private static final Pattern INSERT_PATTERN = Pattern.compile("^\\s*INSERT\\s+", Pattern.CASE_INSENSITIVE); | ||
| private static final Pattern UPDATE_PATTERN = Pattern.compile("^\\s*UPDATE\\s+", Pattern.CASE_INSENSITIVE); | ||
| private static final Pattern DELETE_PATTERN = Pattern.compile("^\\s*DELETE\\s+", Pattern.CASE_INSENSITIVE); | ||
|
|
||
| public ExecuteAdminSqlResponse executeSql(ExecuteAdminSqlRequest request, Long adminMemberId) { | ||
| String sql = request.sql().trim(); | ||
|
|
||
| String normalizedSql = sql.replaceAll("--.*?\n", " ") | ||
| .replaceAll("/\\*.*?\\*/", " ") | ||
| .replaceAll("\\s+", " ") | ||
| .trim(); | ||
| String upperSql = normalizedSql.toUpperCase(); | ||
|
|
||
| for (String blocked : BLOCKED_COMMANDS) { | ||
| if (upperSql.contains(blocked)) { | ||
| log.warn("[Admin SQL] BLOCKED - adminId: {}, sql: {}", adminMemberId, sql); | ||
| return ExecuteAdminSqlResponse.error("차단된 명령어입니다: " + blocked); | ||
| } | ||
| } |
There was a problem hiding this comment.
SQL 차단 메커니즘이 우회 가능합니다.
현재 구현은 단순 키워드 매칭으로 위험한 명령어를 차단하는데, 여러 방법으로 우회될 수 있습니다:
- 중첩 주석:
DR/**/OP,DR--comment\nOP형태로 키워드를 분리하면 정규식으로 제거한 후에도 실행 가능 - 대소문자 혼합 후 인코딩:
DrOp같은 경우 normalizedSql은 공백으로 치환되지만 원본 sql은 그대로 전달 - 동적 SQL:
PREPARE문이나 프로시저 호출을 통한 간접 실행
근본적인 문제점:
- 블랙리스트 방식은 새로운 우회 기법에 취약합니다
- 원본
sql변수가jdbcTemplate에 그대로 전달되므로,normalizedSql검증이 우회됩니다 (Line 62, 64, 66, 68)
개선 방안:
- 화이트리스트 방식: 허용된 테이블/컬럼만 접근 가능하도록 제한
- SQL 파서 사용: JSqlParser 같은 라이브러리로 AST 분석 후 허용 여부 판단
- 검증된 sql 사용:
normalizedSql또는 파싱 후 재구성한 SQL을 실행
🔒 제안하는 수정안 (검증된 SQL 사용)
public ExecuteAdminSqlResponse executeSql(ExecuteAdminSqlRequest request, Long adminMemberId) {
- String sql = request.sql().trim();
+ String originalSql = request.sql().trim();
- String normalizedSql = sql.replaceAll("--.*?\n", " ")
+ String sql = originalSql.replaceAll("--.*?\n", " ")
.replaceAll("/\\*.*?\\*/", " ")
.replaceAll("\\s+", " ")
.trim();
- String upperSql = normalizedSql.toUpperCase();
+ String upperSql = sql.toUpperCase();
for (String blocked : BLOCKED_COMMANDS) {
if (upperSql.contains(blocked)) {
- log.warn("[Admin SQL] BLOCKED - adminId: {}, sql: {}", adminMemberId, sql);
+ log.warn("[Admin SQL] BLOCKED - adminId: {}, sql: {}", adminMemberId, originalSql);
return ExecuteAdminSqlResponse.error("차단된 명령어입니다: " + blocked);
}
}
- log.info("[Admin SQL] EXECUTE - adminId: {}, sql: {}", adminMemberId, sql);
+ log.info("[Admin SQL] EXECUTE - adminId: {}, sql: {}", adminMemberId, originalSql);그리고 실행 시에는 정규화된 sql 사용:
Future<ExecuteAdminSqlResponse> future = executor.submit(() -> {
try {
- if (SELECT_PATTERN.matcher(normalizedSql).find()) {
+ if (SELECT_PATTERN.matcher(sql).find()) {
return executeSelect(sql, startTime);Committable suggestion skipped: line range outside the PR's diff.
| public ExecuteAdminSqlResponse executeSql(ExecuteAdminSqlRequest request, Long adminMemberId) { | ||
| String sql = request.sql().trim(); | ||
|
|
||
| String normalizedSql = sql.replaceAll("--.*?\n", " ") | ||
| .replaceAll("/\\*.*?\\*/", " ") | ||
| .replaceAll("\\s+", " ") | ||
| .trim(); | ||
| String upperSql = normalizedSql.toUpperCase(); | ||
|
|
||
| for (String blocked : BLOCKED_COMMANDS) { | ||
| if (upperSql.contains(blocked)) { | ||
| log.warn("[Admin SQL] BLOCKED - adminId: {}, sql: {}", adminMemberId, sql); | ||
| return ExecuteAdminSqlResponse.error("차단된 명령어입니다: " + blocked); | ||
| } | ||
| } | ||
| log.info("[Admin SQL] EXECUTE - adminId: {}, sql: {}", adminMemberId, sql); | ||
|
|
||
| long startTime = System.currentTimeMillis(); | ||
| int TIMEOUT_SEC = 10; | ||
|
|
||
| ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
| Future<ExecuteAdminSqlResponse> future = executor.submit(() -> { | ||
| try { | ||
| if (SELECT_PATTERN.matcher(normalizedSql).find()) { | ||
| return executeSelect(sql, startTime); | ||
| } else if (INSERT_PATTERN.matcher(normalizedSql).find()) { | ||
| return executeModify(sql, "INSERT", startTime); | ||
| } else if (UPDATE_PATTERN.matcher(normalizedSql).find()) { | ||
| return executeModify(sql, "UPDATE", startTime); | ||
| } else if (DELETE_PATTERN.matcher(normalizedSql).find()) { | ||
| return executeModify(sql, "DELETE", startTime); | ||
| } else { | ||
| return ExecuteAdminSqlResponse.error("지원하지 않는 쿼리 타입입니다. SELECT, INSERT, UPDATE, DELETE만 가능합니다."); | ||
| } | ||
| } catch (Exception e) { | ||
| log.error("[Admin SQL] ERROR - adminId: {}, sql: {}, error: {}", adminMemberId, sql, e.getMessage()); | ||
| return ExecuteAdminSqlResponse.error(e.getMessage()); | ||
| } | ||
| }); | ||
| try { | ||
| ExecuteAdminSqlResponse result = future.get(TIMEOUT_SEC, TimeUnit.SECONDS); | ||
| log.info("[Admin SQL] SUCCESS - adminId: {}, sql: {}", adminMemberId, sql); | ||
| return result; | ||
| } catch (TimeoutException te) { | ||
| future.cancel(true); | ||
| log.error("[Admin SQL] TIMEOUT - adminId: {}, sql: {}", adminMemberId, sql); | ||
| return ExecuteAdminSqlResponse.error("쿼리 실행 시간이 초과되었습니다(" + TIMEOUT_SEC + "초)"); | ||
| } catch (Exception e) { | ||
| log.error("[Admin SQL] ERROR - adminId: {}, sql: {}, error: {}", adminMemberId, sql, e.getMessage()); | ||
| return ExecuteAdminSqlResponse.error(e.getMessage()); | ||
| } finally { | ||
| executor.shutdownNow(); | ||
| } | ||
| } | ||
|
|
||
| private ExecuteAdminSqlResponse executeSelect(String sql, long startTime) { | ||
| List<Map<String, Object>> rows = jdbcTemplate.queryForList(sql); | ||
| long executionTime = System.currentTimeMillis() - startTime; | ||
| return ExecuteAdminSqlResponse.selectSuccess(rows, executionTime); | ||
| } | ||
|
|
||
| private ExecuteAdminSqlResponse executeModify(String sql, String queryType, long startTime) { | ||
| int affectedRows = jdbcTemplate.update(sql); | ||
| long executionTime = System.currentTimeMillis() - startTime; | ||
| return ExecuteAdminSqlResponse.modifySuccess(queryType, affectedRows, executionTime); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*AdminSqlService*" -o -name "*AdminAudit*" | head -20Repository: woowacourse-teams/2025-mul-kkam
Length of output: 149
🏁 Script executed:
# Search for the AdminSqlService and related files
fd -t f "Admin.*Service|AdminController" --max-results 20Repository: woowacourse-teams/2025-mul-kkam
Length of output: 700
🏁 Script executed:
# Check the full AdminSqlService file to see the BLOCKED_COMMANDS
cat -n backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java 2>/dev/null || fd -t f "*AdminSqlService*"Repository: woowacourse-teams/2025-mul-kkam
Length of output: 5664
🏁 Script executed:
# Find AdminSqlController and related controller files
fd -t f "AdminSqlController|AdminController" --max-results 20Repository: woowacourse-teams/2025-mul-kkam
Length of output: 153
🏁 Script executed:
# Search for audit log entities
rg "AuditLog|audit.*log" -t java --max-count 10Repository: woowacourse-teams/2025-mul-kkam
Length of output: 57
🏁 Script executed:
# Check for permission validation in admin context
rg "AdminMember|@PreAuthorize|@Secured" -t java -A 2 | head -50Repository: woowacourse-teams/2025-mul-kkam
Length of output: 5275
🏁 Script executed:
# View the AdminSqlController to check permission validation
cat -n backend/src/main/java/backend/mulkkam/admin/controller/AdminSqlController.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 2373
🏁 Script executed:
# Search for custom auth annotation
fd -t f "AuthLevel|RequireAuth" | head -10Repository: woowacourse-teams/2025-mul-kkam
Length of output: 228
🏁 Script executed:
# Look for existing audit/logging patterns in admin module
rg "log\.(info|warn|error|debug)" backend/src/main/java/backend/mulkkam/admin/ -A 1 | head -40Repository: woowacourse-teams/2025-mul-kkam
Length of output: 1887
🏁 Script executed:
# Check the RequireAuth and AuthLevel implementation
cat -n backend/src/main/java/backend/mulkkam/common/auth/annotation/RequireAuth.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 543
🏁 Script executed:
cat -n backend/src/main/java/backend/mulkkam/common/auth/annotation/AuthLevel.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 1102
🏁 Script executed:
# Search for the interceptor/aspect that validates @RequireAuth
rg "RequireAuth|AuthLevel" -t java --max-count 20 | grep -E "(class|implements|extends|if|check|validate)"Repository: woowacourse-teams/2025-mul-kkam
Length of output: 2725
🏁 Script executed:
# Check the AuthorizationInterceptor implementation
cat -n backend/src/main/java/backend/mulkkam/common/interceptor/AuthorizationInterceptor.java | head -100Repository: woowacourse-teams/2025-mul-kkam
Length of output: 1387
🏁 Script executed:
# Check if there are any audit log entities or tables
find . -type f -name "*Audit*" -o -name "*Log*" | grep -i "backend.*java" | head -20Repository: woowacourse-teams/2025-mul-kkam
Length of output: 233
🏁 Script executed:
# Search for database schema or entity definitions related to audit/logging
rg "@Entity|@Table" -t java | grep -i "audit\|log" | head -10Repository: woowacourse-teams/2025-mul-kkam
Length of output: 57
🏁 Script executed:
# Check AuthContext to understand the permission validation
cat -n backend/src/main/java/backend/mulkkam/common/auth/AuthContext.java | head -50Repository: woowacourse-teams/2025-mul-kkam
Length of output: 2427
🏁 Script executed:
# Look for any service-to-service calls or internal calls to AdminSqlService
rg "adminSqlService|AdminSqlService" -t javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 592
🏁 Script executed:
# Check if AdminSqlService is publicly accessible or exposed elsewhere
rg "AdminSqlService|executeSql" -t java -B 2 -A 2Repository: woowacourse-teams/2025-mul-kkam
Length of output: 3477
🏁 Script executed:
# Check the coding guidelines compliance - one dot per line rule for lines 41-44
cat -n backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java | sed -n '38,50p'Repository: woowacourse-teams/2025-mul-kkam
Length of output: 809
🏁 Script executed:
# Verify if there's any existing audit logging pattern in the codebase
rg "audit|감사" -t java -i | head -20Repository: woowacourse-teams/2025-mul-kkam
Length of output: 57
어드민 SQL 실행 기능의 감사 추적 및 보안 강화 필요
현재 구현은 기본적인 보안은 갖추었으나, 민감한 작업의 특성상 보완이 필요합니다:
1. 영구적 감사 로그 부재:
- 현재 SLF4J 로그는 로그 로테이션 시 삭제되어 컴플라이언스 요구사항(특히 금융/의료 산업) 충족 불가
- AdminAuditLog 엔티티를 통한 영구 저장 권장 (실행 시간, 영향받은 행 수, 성공/실패 여부 기록)
2. SQL Injection 위험:
BLOCKED_COMMANDS는 DDL(DROP, ALTER 등)만 방어하며, 다음 공격은 미방어:- UNION을 이용한 정보 유출:
SELECT * FROM users UNION SELECT password ... - WHERE절 조작:
SELECT * FROM users WHERE id=1 OR 1=1 - 주석 우회:
DELETE FROM users WHERE id=1--(정규식으로 제거하나, 방언별 우회 가능)
- UNION을 이용한 정보 유출:
- 임의 쿼리 특성상 PreparedStatement 활용이 제한적이므로, 추가 제어 필요:
- 선택지 A: 테이블/컬럼 화이트리스트 검증 (가장 안전하나 유연성 낮음)
- 선택지 B: 더 정교한 정규식 필터링 + SQL 파서 라이브러리 활용 (중간 수준 보안)
- 선택지 C: 현재 상태 유지 + 상세 감사 로그 (위험 인수, 추적성 강화)
3. 결과 크기 제한 부재:
queryForList()반환값 제한 없음 → 대용량 조회 시 OOM 발생 가능- 예:
SELECT * FROM large_table실행 시 메모리 폭발
4. 코드 스타일 - 메서드 체이닝 규칙 위반:
메서드 체이닝은 라인 당 하나의 점(.)만 사용하도록 가이드됩니다.
// 현재 (위반)
String normalizedSql = sql.replaceAll("--.*?\n", " ")
.replaceAll("/\\*.*?\\*/", " ")
.replaceAll("\\s+", " ")
.trim();
// 권장
String normalizedSql = sql.replaceAll("--.*?\n", " ");
normalizedSql = normalizedSql.replaceAll("/\\*.*?\\*/", " ");
normalizedSql = normalizedSql.replaceAll("\\s+", " ");
normalizedSql = normalizedSql.trim();개선 우선순위:
- 감사 로그 테이블 추가 (필수 - 컴플라이언스)
- 결과 행 수 제한 추가 (중요 - 안정성)
- SQL 주입 방어 강화 (권장 - 보안 심화)
- 코드 스타일 정정 (필수 - 팀 컨벤션)
🤖 Prompt for AI Agents
In @backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java
around lines 38 - 103, executeSql lacks persistent audit logging, has no
result-size limits, weak SQL-injection defenses, and violates method-chaining
style; fix by: 1) persist each execution into AdminAuditLog (capture
adminMemberId, sql, normalizedSql, startTime, executionTime, affectedRows or
resultCount, success/failure, and error message) from executeSql and update
AdminAuditLog in executeSelect/executeModify paths; 2) prevent OOM by limiting
SELECT results (e.g., enforce a maxRows cap before calling queryForList or
switch to a streaming/limited query and record resultCount) in executeSelect; 3)
harden SQL checks beyond BLOCKED_COMMANDS by applying stricter normalization of
normalizedSql, adding pattern checks for UNION/OR 1=1/comment-escape attempts or
integrate an SQL parser/whitelist approach and reject suspicious queries in
executeSql before submission; and 4) refactor the normalizedSql construction to
avoid multi-dot chaining (split replaceAll/trim into separate statements) to
satisfy style rules.
| String normalizedSql = sql.replaceAll("--.*?\n", " ") | ||
| .replaceAll("/\\*.*?\\*/", " ") | ||
| .replaceAll("\\s+", " ") | ||
| .trim(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
메서드 체이닝이 한 줄에 여러 개 연결되어 있습니다.
코딩 가이드라인의 "한 줄에 한 개의 점(.)" 규칙을 위반하고 있습니다. 디버깅 시 어느 메서드에서 예외가 발생했는지 파악하기 어렵고, 가독성이 떨어집니다.
♻️ 제안하는 수정안
- String normalizedSql = sql.replaceAll("--.*?\n", " ")
- .replaceAll("/\\*.*?\\*/", " ")
- .replaceAll("\\s+", " ")
- .trim();
+ String normalizedSql = sql.replaceAll("--.*?\n", " ");
+ normalizedSql = normalizedSql.replaceAll("/\\*.*?\\*/", " ");
+ normalizedSql = normalizedSql.replaceAll("\\s+", " ");
+ normalizedSql = normalizedSql.trim();또는 정규화 로직을 별도 메서드로 추출:
+ private String normalizeSql(String sql) {
+ String result = sql.replaceAll("--.*?\n", " ");
+ result = result.replaceAll("/\\*.*?\\*/", " ");
+ result = result.replaceAll("\\s+", " ");
+ return result.trim();
+ }
+
public ExecuteAdminSqlResponse executeSql(ExecuteAdminSqlRequest request, Long adminMemberId) {
String sql = request.sql().trim();
- String normalizedSql = sql.replaceAll("--.*?\n", " ")
- .replaceAll("/\\*.*?\\*/", " ")
- .replaceAll("\\s+", " ")
- .trim();
+ String normalizedSql = normalizeSql(sql);📝 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.
| String normalizedSql = sql.replaceAll("--.*?\n", " ") | |
| .replaceAll("/\\*.*?\\*/", " ") | |
| .replaceAll("\\s+", " ") | |
| .trim(); | |
| String normalizedSql = sql.replaceAll("--.*?\n", " "); | |
| normalizedSql = normalizedSql.replaceAll("/\\*.*?\\*/", " "); | |
| normalizedSql = normalizedSql.replaceAll("\\s+", " "); | |
| normalizedSql = normalizedSql.trim(); |
🤖 Prompt for AI Agents
In @backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java
around lines 41 - 44, The chained normalization call that builds normalizedSql
in AdminSqlService should be split into single-method-per-line statements or
extracted to a dedicated method (e.g., normalizeSql or sanitizeSql) to follow
the "one dot per line" rule; break the replaceAll calls into separate statements
(or move the entire sequence into a new private method like normalizeSql(String
sql) and call it from the current method), so each transformation (remove line
comments, remove block comments, collapse whitespace, trim) is on its own line
and exceptions are easier to trace.
| log.info("[Admin SQL] EXECUTE - adminId: {}, sql: {}", adminMemberId, sql); | ||
|
|
||
| long startTime = System.currentTimeMillis(); | ||
| int TIMEOUT_SEC = 10; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
로컬 변수명이 상수 네이밍 컨벤션을 따르지 않습니다.
TIMEOUT_SEC는 대문자와 언더스코어를 사용했지만, 로컬 변수로 선언되어 있습니다. 코딩 가이드라인에 따르면 상수는 클래스 레벨에서 static final로 선언해야 합니다.
문제점:
- 매 요청마다 불필요하게 변수가 생성됩니다
- 컨벤션 불일치로 코드 일관성이 떨어집니다
- 다른 개발자가 값을 변경 가능한 변수로 오해할 수 있습니다
♻️ 제안하는 수정안
private final JdbcTemplate jdbcTemplate;
+ private static final int TIMEOUT_SEC = 10;
+
private static final Set<String> BLOCKED_COMMANDS = Set.of(
"DROP", "TRUNCATE", "ALTER", "CREATE", "GRANT", "REVOKE", log.info("[Admin SQL] EXECUTE - adminId: {}, sql: {}", adminMemberId, sql);
long startTime = System.currentTimeMillis();
- int TIMEOUT_SEC = 10;
ExecutorService executor = Executors.newSingleThreadExecutor();📝 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.
| int TIMEOUT_SEC = 10; | |
| private final JdbcTemplate jdbcTemplate; | |
| private static final int TIMEOUT_SEC = 10; | |
| private static final Set<String> BLOCKED_COMMANDS = Set.of( | |
| "DROP", "TRUNCATE", "ALTER", "CREATE", "GRANT", "REVOKE", | |
| // ... rest of blocked commands | |
| ); | |
| // ... other class members ... | |
| // In the method: | |
| log.info("[Admin SQL] EXECUTE - adminId: {}, sql: {}", adminMemberId, sql); | |
| long startTime = System.currentTimeMillis(); | |
| ExecutorService executor = Executors.newSingleThreadExecutor(); |
🤖 Prompt for AI Agents
In @backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java at
line 56, The local variable TIMEOUT_SEC in AdminSqlService should be a
class-level constant; move its declaration out of the method and declare it as a
static final int TIMEOUT_SEC = 10 at the top of the AdminSqlService class,
update any references to the local variable to use this constant, and remove the
original local declaration to follow constant naming conventions and avoid
per-request allocations.
| ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
| Future<ExecuteAdminSqlResponse> future = executor.submit(() -> { | ||
| try { | ||
| if (SELECT_PATTERN.matcher(normalizedSql).find()) { | ||
| return executeSelect(sql, startTime); | ||
| } else if (INSERT_PATTERN.matcher(normalizedSql).find()) { | ||
| return executeModify(sql, "INSERT", startTime); | ||
| } else if (UPDATE_PATTERN.matcher(normalizedSql).find()) { | ||
| return executeModify(sql, "UPDATE", startTime); | ||
| } else if (DELETE_PATTERN.matcher(normalizedSql).find()) { | ||
| return executeModify(sql, "DELETE", startTime); | ||
| } else { | ||
| return ExecuteAdminSqlResponse.error("지원하지 않는 쿼리 타입입니다. SELECT, INSERT, UPDATE, DELETE만 가능합니다."); | ||
| } | ||
| } catch (Exception e) { | ||
| log.error("[Admin SQL] ERROR - adminId: {}, sql: {}, error: {}", adminMemberId, sql, e.getMessage()); | ||
| return ExecuteAdminSqlResponse.error(e.getMessage()); | ||
| } | ||
| }); | ||
| try { | ||
| ExecuteAdminSqlResponse result = future.get(TIMEOUT_SEC, TimeUnit.SECONDS); | ||
| log.info("[Admin SQL] SUCCESS - adminId: {}, sql: {}", adminMemberId, sql); | ||
| return result; | ||
| } catch (TimeoutException te) { | ||
| future.cancel(true); | ||
| log.error("[Admin SQL] TIMEOUT - adminId: {}, sql: {}", adminMemberId, sql); | ||
| return ExecuteAdminSqlResponse.error("쿼리 실행 시간이 초과되었습니다(" + TIMEOUT_SEC + "초)"); | ||
| } catch (Exception e) { | ||
| log.error("[Admin SQL] ERROR - adminId: {}, sql: {}, error: {}", adminMemberId, sql, e.getMessage()); | ||
| return ExecuteAdminSqlResponse.error(e.getMessage()); | ||
| } finally { | ||
| executor.shutdownNow(); | ||
| } |
There was a problem hiding this comment.
요청마다 ExecutorService를 생성하여 리소스가 낭비됩니다.
매 요청마다 Executors.newSingleThreadExecutor()를 생성하면:
- 스레드 생성/소멸 오버헤드 발생
- 동시 요청이 많을 경우 스레드 수가 급증하여 OOM 위험
shutdownNow()후에도 스레드가 즉시 정리되지 않을 수 있음
개선 방안:
- 클래스 레벨 ExecutorService:
@PreDestroy로 생명주기 관리 - Virtual Threads (Java 21+): 경량 스레드로 오버헤드 최소화
- CompletableFuture:
orTimeout()활용하여 executor 불필요
장단점:
- 방안 1: 전통적이고 안정적, 하지만 스레드 풀 크기 튜닝 필요
- 방안 2: 최신 기술, 오버헤드 최소, Java 버전 의존성
- 방안 3: 간결한 코드, 하지만 timeout 발생 시 쿼리는 계속 실행될 수 있음 (DB connection timeout과 별개)
♻️ 제안하는 수정안 (방안 1 - 클래스 레벨 풀)
+import jakarta.annotation.PreDestroy;
+
@Slf4j
@RequiredArgsConstructor
@Service
public class AdminSqlService {
private final JdbcTemplate jdbcTemplate;
+ private final ExecutorService sqlExecutor = Executors.newFixedThreadPool(5);
+
+ private static final int TIMEOUT_SEC = 10;
private static final Set<String> BLOCKED_COMMANDS = Set.of( log.info("[Admin SQL] EXECUTE - adminId: {}, sql: {}", adminMemberId, sql);
long startTime = System.currentTimeMillis();
- int TIMEOUT_SEC = 10;
- ExecutorService executor = Executors.newSingleThreadExecutor();
- Future<ExecuteAdminSqlResponse> future = executor.submit(() -> {
+ Future<ExecuteAdminSqlResponse> future = sqlExecutor.submit(() -> { return ExecuteAdminSqlResponse.error(e.getMessage());
- } finally {
- executor.shutdownNow();
}
}
+
+ @PreDestroy
+ public void destroy() {
+ sqlExecutor.shutdown();
+ try {
+ if (!sqlExecutor.awaitTermination(5, TimeUnit.SECONDS)) {
+ sqlExecutor.shutdownNow();
+ }
+ } catch (InterruptedException e) {
+ sqlExecutor.shutdownNow();
+ Thread.currentThread().interrupt();
+ }
+ }🤖 Prompt for AI Agents
In @backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java
around lines 58 - 90, Currently a new ExecutorService is created per request via
Executors.newSingleThreadExecutor() and shutdownNow(), causing resource churn
and OOM risk; move to a shared, class-level ExecutorService field (e.g., private
final ExecutorService adminSqlExecutor = Executors.newFixedThreadPool(...)) and
reuse it for submit(...) calls in executeAdminSql, add a @PreDestroy method to
shutdown the pool cleanly, remove per-request shutdownNow(), and keep the same
timeout logic (future.get(TIMEOUT_SEC, TimeUnit.SECONDS)) and cancellation
(future.cancel(true)) so symbols to update are
Executors.newSingleThreadExecutor(), executor.submit(...), future.get(...),
future.cancel(true), executor.shutdownNow() and add @PreDestroy shutdown that
calls adminSqlExecutor.shutdownNow() or shutdown+awaitTermination.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@backend/src/main/java/backend/mulkkam/admin/dto/response/ExecuteAdminSqlResponse.java:
- Around line 42-44: The ExecuteAdminSqlResponse.error(String errorMessage)
currently accepts raw messages; stop passing database exception messages
directly from AdminSqlService into this factory—catch exceptions in
AdminSqlService, log the full exception detail there (including SQLException)
and pass only a sanitized user-facing message or an error code to
ExecuteAdminSqlResponse.error; alternatively add a new factory like
ExecuteAdminSqlResponse.errorWithCode(String code) and use that from
AdminSqlService so sensitive schema/table/connection details are never returned
to clients while full details remain in server logs.
In
@backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java:
- Around line 18-21: The class AdminIntakeHistoryService has annotations ordered
as @RequiredArgsConstructor, @Service, @Transactional(readOnly = true) which
violates our convention; move @Service to be the last annotation so it reads
@RequiredArgsConstructor, @Transactional(readOnly = true), @Service on the
AdminIntakeHistoryService class declaration, and apply the same ordering to
other Admin*Service classes to keep consistency.
In @backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java:
- Around line 60-63: The admin deleteMember currently delegates to
memberService.delete with a hardcoded MemberRole.MEMBER, which is misleading
because MemberService.delete only needs the ID; change deleteMember to follow
the pattern of other AdminMemberService methods by using memberRepository to
load the Member and pass its actual id/role to memberService.delete (use
memberRepository.findById(...) and throw
CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER) if absent) or alternatively
modify MemberService.delete to accept just an ID; prefer the first option to
keep AdminMemberService consistent with getMember/updateMember and avoid
hardcoding MemberRole.MEMBER.
- Around line 41-58: The updateMember flow accepts UpdateAdminMemberRequest
without validation annotations and the controller call to updateMember lacks
@Valid on the @RequestBody, risking invalid data reaching the domain; add
JSR-303 annotations (e.g., @NotBlank, @Size, @NotNull, @Positive, @Min/@Max) to
fields in UpdateAdminMemberRequest (nickname, gender, weight, targetAmount,
etc.) and annotate the controller parameter with @Valid @RequestBody
UpdateAdminMemberRequest so Spring validates before hitting
AdminMemberService.updateMember, and optionally add audit logging around
updateMember to record who/when/what changed.
🧹 Nitpick comments (7)
backend/src/main/java/backend/mulkkam/admin/dto/response/ExecuteAdminSqlResponse.java (2)
8-26:queryType을 String 대신 Enum으로 정의하는 것을 고려해보세요.현재
queryType이 String으로 선언되어 있어, "SELECT", "INSERT", "UPDATE", "DELETE" 외의 잘못된 값이 들어올 가능성이 있습니다. Enum을 사용하면 컴파일 타임에 타입 안전성을 확보할 수 있고, 허용된 쿼리 타입을 명시적으로 제한할 수 있습니다.장점:
- 유효하지 않은 queryType 값 방지
- IDE 자동완성 및 리팩토링 지원
- API 문서에서 허용 값이 명확히 드러남
단점:
- 새로운 쿼리 타입 추가 시 Enum 수정 필요 (하지만 이는 의도적인 제한으로 볼 수 있음)
♻️ Enum 도입 예시
public enum AdminSqlQueryType { SELECT, INSERT, UPDATE, DELETE }public record ExecuteAdminSqlResponse( - @Schema(description = "쿼리 타입 (SELECT, INSERT, UPDATE, DELETE)") - String queryType, + @Schema(description = "쿼리 타입") + AdminSqlQueryType queryType, // ... 나머지 필드 ) {
27-44: 팩토리 메서드에 대한 단위 테스트를 추가하는 것을 권장합니다.각 팩토리 메서드가 올바른 필드 값으로 응답 객체를 생성하는지 검증하는 단위 테스트가 필요합니다.
테스트 케이스 제안:
selectSuccess:queryType이 "SELECT",success가 true,rows가 전달된 값과 일치하는지 검증modifySuccess: 전달된queryType,affectedRows,executionTimeMs가 올바르게 설정되는지 검증error:success가 false,errorMessage가 전달된 값과 일치하는지, 나머지 필드가 null/0인지 검증backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java (2)
25-28: 메서드 네이밍을 컨벤션(read/get/find) + “서비스명에 이미 엔티티가 있으니 중복 제거”로 맞추는 걸 추천해요.
AdminIntakeHistoryService안에서getIntakeHistories()는 “다건 조회”라서 가이드대로면read...가 더 자연스럽고,IntakeHistory라는 엔티티명은 서비스명에 이미 포함되어 중복입니다.
- 선택지 A) 컨벤션 우선(추천):
readHistories(Pageable pageable)
- 장점: 팀 가이드에 정합, 의미가 더 정확
- 단점: 컨트롤러/호출부 시그니처 변경 필요
- 선택지 B) 현 Admin*Service들과 통일: 다른 서비스들이 전부
getXxx()라면 유지
- 장점: 일관성 유지
- 단점: 팀 가이드와는 불일치 유지
46-51: deleteIntakeHistory는 현재 2-step(조회 후 삭제)인데, 패턴을 서비스들끼리 통일하면 더 좋아요.지금 구현은 “없으면 NOT_FOUND를 명확히 낸다”는 장점이 있습니다. 다만 매번
findById → delete(entity)패턴이 반복될 가능성이 높아서,
- 선택지 A) 현재 방식 유지 +
private IntakeHistory getOrThrow(Long id)로 중복 제거
- 장점: NOT_FOUND 유지, 중복 감소
- 단점: 여전히 조회 쿼리 1번 필요
- 선택지 B)
existsById확인 후deleteById(리포지토리 지원 시)
- 장점: 의도가 단순(삭제)
- 단점: 여전히 2번 쿼리, 동시성 경쟁(존재 확인 후 삭제 사이) 고려 필요
개인적으론 A가 제일 무난해 보입니다(다른 Admin*Service들과도 패턴 맞추기 좋음).
backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java (3)
22-24: 어노테이션 순서가 코딩 가이드라인과 다릅니다.팀 컨벤션에 따르면 어노테이션 순서는
Lombok → Spring meta → Spring components순이어야 합니다. 현재@Service가@Transactional보다 앞에 위치해 있어요. 일관된 코드 스타일을 위해 순서를 맞춰주시면 좋겠습니다.♻️ 수정 제안
@RequiredArgsConstructor -@Service @Transactional(readOnly = true) +@Service public class AdminMemberService {
35-39: 중복되는findById로직을 헬퍼 메서드로 추출하는 것을 고려해보세요.
getMember와updateMember에서 동일한 패턴(findById+orElseThrow)이 반복됩니다. DRY 원칙을 따라 private 헬퍼 메서드로 추출하면 코드 중복을 줄이고 유지보수성을 높일 수 있습니다.장점:
- 예외 메시지나 에러 코드 변경 시 한 곳만 수정
- 테스트 시 mock 설정이 간결해짐
단점:
- 매우 간단한 로직이라 과도한 추상화로 볼 수도 있음
♻️ 수정 제안
+ private Member findMemberById(Long memberId) { + return memberRepository.findById(memberId) + .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)); + } + public GetAdminMemberDetailResponse getMember(Long memberId) { - Member member = memberRepository.findById(memberId) - .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)); + Member member = findMemberById(memberId); return GetAdminMemberDetailResponse.from(member); }
25-64: 테스트 코드 작성이 필요합니다.어드민 서비스는 중요한 데이터 변경 작업을 수행하므로 다음과 같은 테스트 케이스를 권장드립니다:
단위 테스트 케이스:
getMembers: 페이지네이션 동작 확인, 빈 결과 처리getMember: 존재하는 회원 조회, 존재하지 않는 회원 조회 시CommonException발생updateMember: 각 필드 업데이트 정상 동작, 존재하지 않는 회원 업데이트 시 예외 발생deleteMember: 정상 삭제 위임 확인고려할 엣지 케이스:
nickname이 null이거나 빈 문자열인 경우weight,targetAmount가 음수인 경우memberRole이 null인 경우필요하시면 테스트 코드 템플릿을 생성해드릴 수 있습니다. 원하시면 말씀해주세요!
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/main/java/backend/mulkkam/admin/dto/response/ExecuteAdminSqlResponse.javabackend/src/main/java/backend/mulkkam/admin/service/AdminCupService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java
- backend/src/main/java/backend/mulkkam/admin/service/AdminSqlService.java
🧰 Additional context used
📓 Path-based instructions (3)
backend/**/*.java
📄 CodeRabbit inference engine (backend/code-style.md)
backend/**/*.java: Use verb-starting method names without entity name, e.g., memberService.findById(Long id)
Avoid else statements; use alternative control flow patterns
Use one dot per line (method chaining rule)
Avoid abbreviations in code
Avoid getters and setters; use behavior-focused methods instead
Annotation order: logs, Lombok, Spring meta, Spring components (most important, placed last)
Add one blank line before class declaration
When method has 2 or more parameters, use line breaks for parameters (does not apply to method calls)
Do not use wildcard imports; apply IntelliJ code conventions for imports
Use method names: create, save, register, add for creation operations
Use method names: read for multiple results, get for always-existing values, find for Optional results
Use method names: update for full replacement (PUT), modify for partial updates (PATCH)
Use method names: delete, remove for deletion operations
Do not use final keyword on class or field declarations
Use uppercase with underscores for constant names, e.g., CONSTANT_NAME
Order class members and methods by access level: public, protected, private; arrange by call order
Files:
backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.javabackend/src/main/java/backend/mulkkam/admin/dto/response/ExecuteAdminSqlResponse.java
backend/**
⚙️ CodeRabbit configuration file
backend/**: - 1. 팀 및 공식 컨벤션, 가독성, 예외처리, 테스트/확장/유지보수성, 모듈화, API/DB/보안 설계 기준을 기반으로 리뷰해주세요.
- 최신 트렌드, 불필요한 로직, 클린코드, 리팩토링, 서비스/도메인 설계, 공통 예외 처리, 확장성도 함께 확인해주세요.
- 각 피드백은 문제점·대안·장단점을 짧고 논리적으로, 예시 코드가 있다면 간결히 포함해 주세요.
- 팀 내 스타일 통일성도 확인해주세요.
- 미작성한 테스트 코드 케이스가 있다면, 어떤 테스트가 필요한지 제안해주세요. (예: 컨트롤러는 인수 테스트, 나머지는 단위 테스트)
- 리뷰 남겨주는 부분은 해당 라인 범위의 코멘트에 작성해주세요.
Files:
backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.javabackend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.javabackend/src/main/java/backend/mulkkam/admin/dto/response/ExecuteAdminSqlResponse.java
backend/**/dto/**/*.java
📄 CodeRabbit inference engine (backend/code-style.md)
Use Request/Response suffixes for DTOs; use Register/Update etc. for specific operations, e.g., PostRegisterRequest
Files:
backend/src/main/java/backend/mulkkam/admin/dto/response/ExecuteAdminSqlResponse.java
🧠 Learnings (4)
📚 Learning: 2025-09-10T04:51:38.314Z
Learnt from: CheChe903
Repo: woowacourse-teams/2025-mul-kkam PR: 739
File: backend/src/main/java/backend/mulkkam/auth/infrastructure/OauthJwtTokenHandler.java:56-64
Timestamp: 2025-09-10T04:51:38.314Z
Learning: CheChe903 사용자는 테스트 코드에서 BeforeEach를 활용해서 가독성을 향상시킬 수 있는 부분이 있으면 리뷰해달라고 요청했습니다. 앞으로 테스트 코드 리뷰 시 중복되는 객체 생성, 테스트 데이터 셋업, Mock 설정 등을 BeforeEach로 추출할 수 있는 리팩터링 기회를 적극적으로 제안해야 합니다.
Applied to files:
backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java
📚 Learning: 2025-12-15T16:41:54.690Z
Learnt from: CR
Repo: woowacourse-teams/2025-mul-kkam PR: 0
File: backend/code-style.md:0-0
Timestamp: 2025-12-15T16:41:54.690Z
Learning: Applies to backend/**/*.java : Do not use final keyword on class or field declarations
Applied to files:
backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java
📚 Learning: 2025-12-15T16:41:54.690Z
Learnt from: CR
Repo: woowacourse-teams/2025-mul-kkam PR: 0
File: backend/code-style.md:0-0
Timestamp: 2025-12-15T16:41:54.690Z
Learning: Applies to backend/**/*.java : Use one dot per line (method chaining rule)
Applied to files:
backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java
📚 Learning: 2025-12-15T16:41:54.690Z
Learnt from: CR
Repo: woowacourse-teams/2025-mul-kkam PR: 0
File: backend/code-style.md:0-0
Timestamp: 2025-12-15T16:41:54.690Z
Learning: Applies to backend/**/*.java : Use verb-starting method names without entity name, e.g., memberService.findById(Long id)
Applied to files:
backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java
🧬 Code graph analysis (1)
backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java (4)
backend/src/main/java/backend/mulkkam/admin/service/AdminCupService.java (1)
RequiredArgsConstructor(22-63)backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java (1)
RequiredArgsConstructor(22-64)backend/src/main/java/backend/mulkkam/admin/service/AdminDeviceService.java (1)
RequiredArgsConstructor(15-33)backend/src/main/java/backend/mulkkam/admin/service/AdminFriendRelationService.java (1)
RequiredArgsConstructor(15-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
backend/src/main/java/backend/mulkkam/admin/dto/response/ExecuteAdminSqlResponse.java (1)
27-40: 정적 팩토리 메서드 패턴이 잘 적용되었습니다.
selectSuccess,modifySuccess,error세 가지 팩토리 메서드로 응답 생성 의도를 명확히 표현하고 있어, 호출하는 쪽에서 어떤 종류의 응답을 만드는지 쉽게 파악할 수 있습니다. 생성자를 직접 호출하는 것보다 가독성이 높고, 잘못된 조합의 필드 값이 설정되는 것을 방지하는 효과도 있습니다.backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java (2)
30-34: 단건 조회(get) + not-found 처리 흐름은 깔끔합니다.
findById().orElseThrow(CommonException(...))로 “항상 존재해야 하는 값(get)”을 보장하고, 응답 DTO 변환 책임도from()으로 모여 있어 유지보수성이 좋아요.
36-44: updateIntakeHistory는 실제로 ‘부분 변경’인데 update(=PUT)로 보일 수 있어요. 엔드포인트 의미를 확인해 주세요.현재 변경은
targetAmount1개 필드만 수정이라 “PATCH 성격(=modify)”에 가깝습니다. 컨트롤러가 PUT이면 지금처럼update...가 맞고, PATCH면modify...가 더 정확해요. 또한request.targetAmount()가 null/범위 밖일 때의 방어는 DTO validation에 위임하는 편이 일반적이니,@Valid/제약(@NotNull, 최소/최대 등) 적용 여부도 같이 확인되면 좋겠습니다.
| public static ExecuteAdminSqlResponse error(String errorMessage) { | ||
| return new ExecuteAdminSqlResponse(null, null, null, 0, false, errorMessage); | ||
| } |
There was a problem hiding this comment.
에러 메시지 노출 시 보안에 유의해야 합니다.
error(String errorMessage) 팩토리 메서드가 에러 메시지를 그대로 전달하고 있습니다. 만약 SQLException이나 DB 관련 예외의 원본 메시지가 그대로 노출된다면, 테이블명, 컬럼명, 스키마 정보, 심지어 연결 문자열까지 외부로 유출될 수 있습니다.
권장 사항:
- 서비스 레이어에서 에러 메시지 정제: AdminSqlService에서 예외를 catch할 때, 사용자에게 보여줄 메시지와 로깅용 메시지를 분리하세요.
- 민감 정보 마스킹: 테이블명, 컬럼명 등이 포함된 경우 일반화된 메시지로 변환하거나, 에러 코드만 노출하는 방식을 고려하세요.
어드민 전용 기능이라 해도, 어드민 계정 탈취 시 내부 스키마 정보가 공격자에게 노출되면 추가 공격 벡터를 제공할 수 있습니다.
#!/bin/bash
# AdminSqlService에서 error() 호출 시 어떤 메시지가 전달되는지 확인
ast-grep --pattern $'ExecuteAdminSqlResponse.error($_)'🤖 Prompt for AI Agents
In
@backend/src/main/java/backend/mulkkam/admin/dto/response/ExecuteAdminSqlResponse.java
around lines 42 - 44, The ExecuteAdminSqlResponse.error(String errorMessage)
currently accepts raw messages; stop passing database exception messages
directly from AdminSqlService into this factory—catch exceptions in
AdminSqlService, log the full exception detail there (including SQLException)
and pass only a sanitized user-facing message or an error code to
ExecuteAdminSqlResponse.error; alternatively add a new factory like
ExecuteAdminSqlResponse.errorWithCode(String code) and use that from
AdminSqlService so sensitive schema/table/connection details are never returned
to clients while full details remain in server logs.
| @RequiredArgsConstructor | ||
| @Service | ||
| @Transactional(readOnly = true) | ||
| public class AdminIntakeHistoryService { |
There was a problem hiding this comment.
어노테이션 순서가 컨벤션과 반대예요 (@service는 마지막 권장).
가이드(“logs → Lombok → Spring meta → Spring components(마지막)”) 기준이면 @Service를 마지막으로 두는 편이 팀 스타일에 더 맞습니다. 현재는 @Service가 @Transactional보다 위에 있어요. (다른 Admin*Service들도 같은 패턴이면 함께 정리하는 게 베스트)
✅ 제안 diff
@RequiredArgsConstructor
-@Service
@Transactional(readOnly = true)
+@Service
public class AdminIntakeHistoryService {📝 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.
| @RequiredArgsConstructor | |
| @Service | |
| @Transactional(readOnly = true) | |
| public class AdminIntakeHistoryService { | |
| @RequiredArgsConstructor | |
| @Transactional(readOnly = true) | |
| @Service | |
| public class AdminIntakeHistoryService { |
🤖 Prompt for AI Agents
In
@backend/src/main/java/backend/mulkkam/admin/service/AdminIntakeHistoryService.java
around lines 18 - 21, The class AdminIntakeHistoryService has annotations
ordered as @RequiredArgsConstructor, @Service, @Transactional(readOnly = true)
which violates our convention; move @Service to be the last annotation so it
reads @RequiredArgsConstructor, @Transactional(readOnly = true), @Service on the
AdminIntakeHistoryService class declaration, and apply the same ordering to
other Admin*Service classes to keep consistency.
| @Transactional | ||
| public GetAdminMemberDetailResponse updateMember( | ||
| Long memberId, | ||
| UpdateAdminMemberRequest request | ||
| ) { | ||
| Member member = memberRepository.findById(memberId) | ||
| .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)); | ||
|
|
||
| member.updateNickname(new MemberNickname(request.nickname())); | ||
| member.updatePhysicalAttributes(new PhysicalAttributes(request.gender(), request.weight())); | ||
| member.updateTargetAmount(new TargetAmount(request.targetAmount())); | ||
| member.modifyIsMarketingNotificationAgreed(request.isMarketingNotificationAgreed()); | ||
| member.modifyIsNightNotificationAgreed(request.isNightNotificationAgreed()); | ||
| member.modifyIsReminderEnabled(request.isReminderEnabled()); | ||
| member.modifyMemberRole(request.memberRole()); | ||
|
|
||
| return GetAdminMemberDetailResponse.from(member); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "UpdateAdminMemberRequest.java" -x cat {}Repository: woowacourse-teams/2025-mul-kkam
Length of output: 1007
🏁 Script executed:
rg "UpdateAdminMemberRequest|audit|logging" backend/ --type java -A 3 -B 1Repository: woowacourse-teams/2025-mul-kkam
Length of output: 3668
🏁 Script executed:
rg "@Valid|@NotNull|@NotEmpty|validation" backend/src/main/java/backend/mulkkam/admin -A 2Repository: woowacourse-teams/2025-mul-kkam
Length of output: 7333
🏁 Script executed:
rg -A 10 "updateMember" backend/src/main/java/backend/mulkkam/admin/controller/AdminMemberController.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 758
입력 검증과 @Valid 어노테이션 추가 필수
UpdateAdminMemberRequest DTO에 검증 어노테이션이 부재하고, 컨트롤러의 @RequestBody에 @Valid 어노테이션도 없습니다. 팀 내 다른 어드민 DTO(UpdateAdminIntakeHistoryRequest, UpdateAdminCupRequest 등)는 모두 @NotNull, @NotBlank, @Positive 등의 검증을 적용하고 있으며, 해당 컨트롤러들도 @Valid를 사용하고 있습니다.
특히 nickname, weight, targetAmount 같은 필드는 도메인 제약이 있을 텐데(예: 닉네임 210자, 체중 양수, 목표량 2005000ml), 이를 DTO 레벨에서 명시해야 부정확한 요청이 DB까지 전파되는 것을 방지할 수 있습니다. 현재는 도메인 객체 생성 시점에서만 검증되고 있어 불필요한 예외 처리와 로직 중복을 초래합니다.
개선 방향:
public record UpdateAdminMemberRequest(
@NotBlank(message = "닉네임은 필수입니다.")
@Size(min = 2, max = 10, message = "닉네임은 2~10자여야 합니다.")
String nickname,
@NotNull(message = "성별은 필수입니다.")
Gender gender,
@NotNull(message = "체중은 필수입니다.")
@Positive(message = "체중은 양수여야 합니다.")
Double weight,
@NotNull(message = "목표 섭취량은 필수입니다.")
@Min(value = 200, message = "목표 섭취량은 200ml 이상이어야 합니다.")
@Max(value = 5000, message = "목표 섭취량은 5000ml 이하여야 합니다.")
int targetAmount,
// ...
) { }그리고 컨트롤러에서:
public GetAdminMemberDetailResponse updateMember(
@PathVariable Long memberId,
@Valid @RequestBody UpdateAdminMemberRequest request
) { ... }추가 제안: 어드민 회원 정보 수정은 민감한 작업이므로 누가 언제 무엇을 변경했는지 기록하는 감사 로그를 추가하면 책임성과 추적성이 향상됩니다.
🤖 Prompt for AI Agents
In @backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java
around lines 41 - 58, The updateMember flow accepts UpdateAdminMemberRequest
without validation annotations and the controller call to updateMember lacks
@Valid on the @RequestBody, risking invalid data reaching the domain; add
JSR-303 annotations (e.g., @NotBlank, @Size, @NotNull, @Positive, @Min/@Max) to
fields in UpdateAdminMemberRequest (nickname, gender, weight, targetAmount,
etc.) and annotate the controller parameter with @Valid @RequestBody
UpdateAdminMemberRequest so Spring validates before hitting
AdminMemberService.updateMember, and optionally add audit logging around
updateMember to record who/when/what changed.
| @Transactional | ||
| public void deleteMember(Long memberId) { | ||
| memberService.delete(new MemberDetails(memberId, MemberRole.MEMBER)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 1. MemberService 클래스 찾기
find . -type f -name "*.java" | xargs grep -l "class MemberService" | head -5Repository: woowacourse-teams/2025-mul-kkam
Length of output: 206
🏁 Script executed:
# 2. MemberService.delete 메서드 구현 확인
ast-grep --pattern 'public void delete($_) {
$$$
}' | head -100Repository: woowacourse-teams/2025-mul-kkam
Length of output: 666
🏁 Script executed:
# 3. MemberDetails 클래스 확인
find . -type f -name "*.java" | xargs grep -l "class MemberDetails"Repository: woowacourse-teams/2025-mul-kkam
Length of output: 59
🏁 Script executed:
# 4. AdminMemberService 전체 내용과 다른 메서드들 확인
cat backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java 2>/dev/null || find . -type f -name "AdminMemberService.java" | xargs cat 2>/dev/null | head -150Repository: woowacourse-teams/2025-mul-kkam
Length of output: 2874
🏁 Script executed:
# 5. MemberRole enum 확인
find . -type f -name "*.java" | xargs grep -l "enum MemberRole"Repository: woowacourse-teams/2025-mul-kkam
Length of output: 147
🏁 Script executed:
# MemberService.delete 메서드 찾기 및 구현 확인
rg -n -A 30 'public.*delete.*MemberDetails' --type javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 3701
🏁 Script executed:
# MemberRole enum 내용 확인
cat ./backend/src/main/java/backend/mulkkam/member/domain/vo/MemberRole.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 246
🏁 Script executed:
# MemberService 클래스 전체 또는 delete 관련 메서드 확인
find . -type f -name "MemberService.java" -path "*/member/service/*" | xargs cat | head -200Repository: woowacourse-teams/2025-mul-kkam
Length of output: 8820
🏁 Script executed:
# AdminMemberService에서 memberRepository를 직접 사용하는 다른 패턴 확인
rg -B 5 -A 10 'memberRepository\.' backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 1509
어드민 삭제 메서드의 구현 패턴을 다른 메서드들과 일치시키세요.
deleteMember는 MemberService로 위임하고 있지만, getMember와 updateMember는 memberRepository를 직접 사용합니다. MemberService.delete를 살펴보면 memberDetails의 role 필드는 실제로 삭제 로직에서 사용되지 않으며, ID만 필요합니다. 따라서 MemberRole.MEMBER를 hardcode하는 것은 의도 없이 혼란을 야기합니다.
대안:
-
memberRepository 직접 사용 (권장):
AdminMemberService의 다른 메서드들처럼 일관성 있게 처리@Transactional public void deleteMember(Long memberId) { Member member = memberRepository.findById(memberId) .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)); memberService.delete(new MemberDetails(member.getId(), member.getMemberRole())); }
-
MemberService.delete 시그니처 개선: ID만 받도록 변경하거나, 역할이 필요하면 명시적으로 어드민 전용 메서드 추가
첫 번째 방식이 더 간단하며 서비스 간 책임을 명확히 할 수 있습니다.
🤖 Prompt for AI Agents
In @backend/src/main/java/backend/mulkkam/admin/service/AdminMemberService.java
around lines 60 - 63, The admin deleteMember currently delegates to
memberService.delete with a hardcoded MemberRole.MEMBER, which is misleading
because MemberService.delete only needs the ID; change deleteMember to follow
the pattern of other AdminMemberService methods by using memberRepository to
load the Member and pass its actual id/role to memberService.delete (use
memberRepository.findById(...) and throw
CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER) if absent) or alternatively
modify MemberService.delete to accept just an ID; prefer the first option to
keep AdminMemberService consistent with getMember/updateMember and avoid
hardcoding MemberRole.MEMBER.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| return new CheckAdminResponse( | ||
| memberDetails.isAdmin(), | ||
| role, | ||
| role != MemberRole.NONE |
There was a problem hiding this comment.
MemverRole 내에서 검사하도록 책임을 이전하는 건 어떤가요??
| String cupEmojiCode, | ||
| LocalDateTime createdAt | ||
| ) { | ||
| public static GetAdminCupDetailResponse from(Cup cup) { |
There was a problem hiding this comment.
현재 코드 내에서 null 검증을 DTO에서 해주고 있는데요,
이 필드 값들은 비지니스상의 필수값이기 때문에
null일 가능성을 염두해두기 보다는, 값이 존재한다는 전제하에 사용하는 것이 더 적절하다고 생각해서요!
따로 null 검증 고려하지 않아도 된다고 생각하는 편입니다.
혹시 체체가 다른 의견이라면 편하게 말씀해주세요~
| Member member = memberRepository.findById(memberId) | ||
| .orElseThrow(() -> new CommonException(NotFoundErrorCode.NOT_FOUND_MEMBER)); | ||
|
|
||
| member.updateNickname(new MemberNickname(request.nickname())); |
| @Transactional | ||
| public void deleteMember(Long memberId) { | ||
| memberService.delete(new MemberDetails(memberId, MemberRole.MEMBER)); | ||
| } |
| source.registerCorsConfiguration("/**", config); | ||
|
|
||
| FilterRegistrationBean<CorsFilter> filterBean = new FilterRegistrationBean<>(new CorsFilter(source)); | ||
| filterBean.setOrder(Ordered.HIGHEST_PRECEDENCE); // 가장 먼저 실행 |
There was a problem hiding this comment.
잘 몰라가지구 궁금해서 여쭤봅니다
위에 64번째 줄 우선순위랑 겹치지는 않는 건가요 둘다 Order 우선순위가 동일해서요!
|
고생했어요 채채채 |
2 participants
🔗 관련 이슈
📝 작업 내용
주요 변경사항
📸 스크린샷 (Optional)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.