[Jun3610]

변경 내용

설정 페이지용 API들을 추가했습니다. 카카오 닉네임 대신 본명을 쓰도록 이름을 조회·변경합니다.

• GET /api/member : 로그인한 유저의 이름·회원 구분 조회
• PATCH /api/member/name : 이름 변경
• Swagger에 "설정" 태그로 명세 추가

관련 이슈

Closed: #677

검증 결과

• 확인 방법: 설정 테스트 실행, bootJar 빌드, 로컬 서버의 Swagger에서 테스트 계정으로 직접 호출
• 결과: 테스트 31개 통과, 빌드 성공. 비로그인 401, 조회 200, 이름 변경 200, 규칙 위반 400 확인. Swagger에 두 API 표시 확인

리뷰 참고 사항

• memberType은 지금 항상 null입니다. 회원 명단 CSV 연동은 별도 이슈에서 진행합니다.
• 경로는 프론트 설정 화면(🔄 [Client] feat: setting page UI 구현 [Client] feat: setting page UI 구현 #674)의 호출에 맞췄습니다. 프론트는 fetch("/api/member") 앞에 REACT_APP_API_BASE_URL을 붙여야 서버로 요청이 갑니다.
• 기존 파일은 수정하지 않고 setting 패키지만 추가했습니다. DB 스키마 변경은 없습니다.
• dev 서버 Swagger에는 머지 후 배포되면 보입니다.

체크리스트

• 변경 내용을 직접 검토했습니다.
• 변경 범위에 맞게 검증하고, 위에 결과를 기록했습니다.
• 비밀 값이나 개인정보가 포함되지 않았는지 확인했습니다.

[github-actions]

코드 리뷰 결과

🔴 주요 문제점

1. @NotBlank와 커스텀 정규식의 검증 순서 문제 (DTO)

SettingUpdateRequest의 compact constructor에서 name = name.strip()으로 공백 제거를 수행합니다. 이는 Bean Validation이 실행되기 이전에 실행되므로 아래 케이스의 동작에 주의가 필요합니다.

• 입력 " " (공백만) → strip 후 "" → @NotBlank에 의해 거부됨. ✅ 의도대로 동작
• 하지만 compact constructor에서 재할당한 값이 실제 검증 대상이 아닐 수 있는 케이스가 있습니다. 레코드 compact constructor의 파라미터 재할당은 정상 동작하지만, @Pattern이 null에 대해 통과한다는 점(Bean Validation 스펙)을 인지해야 합니다. @NotBlank가 함께 있으므로 실질적으로는 방어되지만, 의도를 명확히 하려면 @NotBlank + @Pattern 조합이 적절한지 문서화 필요.

2. 정규식 관련 이슈

regexp = "^([가-힣]{1,10}|(?=.{1,15}$)[A-Za-z]+( [A-Za-z]+)*)$"

• 두 번째 대안 (?=.{1,15}$) : strip() 이후이므로 앞뒤 공백 없음. 하지만 전각 공백, NBSP 등은 strip()이 제거하지만 내부 공백은 [ ]만 허용하므로 다른 유니코드 공백은 거부됨. 이는 기획 의도에 따라 확인 필요.
• .{1,15}는 줄바꿈 문자(\n)를 포함하여 매칭될 수 있음. \n이 포함된 이름이 통과할 가능성 검토 필요 (예: "John\nDoe" → strip은 양끝만 제거, 중간 \n은 남음. [A-Za-z]+( [A-Za-z]+)* 부분에서 \n 불허로 최종 실패하지만, lookahead 자체는 통과하므로 의도한 것인지 명확화 필요).

3. 서비스 계층에서의 검증 의존

SettingService.updateSetting은 검증을 컨트롤러의 @Valid에만 의존합니다. 서비스가 다른 경로(배치, 내부 호출 등)에서 호출되면 잘못된 이름이 저장될 수 있습니다. 서비스 계층에서도 도메인 규칙 검증을 고려하세요.

4. User.setName() 직접 호출 (Setter 사용)

user.setName(request.name());

• 엔티티의 setter를 통한 변경은 도메인 무결성 관점에서 바람직하지 않습니다. user.changeName(name) 같은 의도 표현 메서드를 권장합니다.
• JPA dirty checking에 의존하고 있으므로 @Transactional은 올바르게 선언되어 있습니다.

5. 테스트 코드의 취약성

@Test
void 앞뒤_공백을_제거한다() {
    assertThat(new SettingUpdateRequest("  John Doe  ").name()).isEqualTo("John Doe");
}

• 이 테스트는 DTO의 생성자 로직만 확인합니다. 프레임워크(Jackson + Validation)를 통과한 실제 요청의 동작과 다를 수 있어, 컨트롤러 통합 테스트가 보완되어야 합니다.

🟡 개선 권고사항

6. SettingResponse.memberType의 null 처리

return new SettingResponse(user.getName(), null);

• "회원 명단 연동 전까지 null"이라고 문서화되어 있지만, 클라이언트가 이 필드를 어떻게 처리할지 명확하지 않습니다. Optional<String> 또는 명시적 sentinel 값(예: "미정")을 검토하거나, 필드 자체를 아예 응답에서 제외하는 것을 고려하세요.

7. 예외 메시지의 i18n/일관성

• 컨트롤러의 ErrorResponse 스키마는 COMMON_RESOURCE_NOT_FOUND 코드를 문서화하고 있으나, 서비스에서 던지는 ResourceNotFoundException("사용자를 찾을 수 없습니다.")가 이 코드로 매핑되는지 확인 필요. 다른 도메인의 메시지와 톤 일관성도 점검하세요.

8. Swagger 어노테이션 중복

• @ApiResponses를 클래스 레벨에 두고, @ApiResponse(200)을 메서드 레벨에 두는 구조는 유지보수 시 코드와 문서가 어긋나기 쉽습니다. 별도 인터페이스로 분리하는 패턴을 권장.

9. @Transactional(readOnly = true) 위치

• 현재 구조는 문제 없지만, findUser 헬퍼가 readOnly 트랜잭션 안에서만 호출되도록 강제되지 않습니다. 향후 쓰기 메서드에서도 안전하도록 주석이나 네이밍(findUserForRead 등)을 고려.

10. 이름 제약 중복 정의

• @Schema의 설명, @Operation의 description, @Pattern의 메시지, 실제 정규식이 네 곳에 중복되어 있습니다. 규칙 변경 시 동기화 실패 위험이 크므로 상수화를 권장합니다.

private static final String NAME_PATTERN = "^([가-힣]{1,10}|...)$";

11. 테스트 네이밍 스타일

• SettingServiceTest의 유저가_없으면_예외가_발생한다와 조회할_유저가_없으면_예외가_발생한다가 사실상 동일 시나리오(조회/수정)를 다룹니다. 조회시_유저가_없으면_예외가_발생한다 / 수정시_유저가_없으면_예외가_발생한다로 명확화.

🟢 보안 관점

12. 인증 정보 처리

• @CurrentUser UserPrincipal로 JWT에서 사용자 ID를 추출하고 요청 파라미터로 받지 않는 것은 ✅ 좋은 패턴입니다.
• 다만 userPrincipal이 null일 가능성에 대한 방어가 컨트롤러에 없습니다. 인터셉터/리졸버에서 보장되는지 확인하고, 그렇지 않다면 명시적 검증이 필요합니다.

13. XSS / 저장 값 이스케이프

• 이름이 화면에 그대로 렌더링될 가능성이 있다면, 저장 시 이스케이프 처리는 불필요하지만 출력 컨텍스트에서의 이스케이프가 보장되어야 합니다. 프론트엔드 책임인지 서버 책임인지 명확화 하세요.

요약

구분	항목
🔴 필수	정규식 lookahead의 \n 포함 가능성 검토, 서비스 계층 검증 보강, setter 대신 도메인 메서드 사용
🟡 권장	제약 조건 상수화, memberType null 처리 정책 명확화, Swagger 어노테이션 분리
🟢 양호	JWT 기반 사용자 식별, strip() + 검증 조합, 테스트 커버리지

전반적으로 구조는 깔끔하고 테스트도 잘 작성되었습니다. 위 항목 중 정규식의 줄바꿈 처리와 도메인 메서드 도입만 우선 반영해도 품질이 크게 향상됩니다.

[psvm203]

고생하셨습니다. 다음에는 API 엔드포인트를 합의해서 정하는 게 좋을 것 같습니다. '/api/...'인 다른 엔드포인트는 없으므로 프론트엔드 구현에 꼭 맞출 필요는 없을 것 같아요.