Conversation
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
github-actions
bot
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/src/main/java/backend/mulkkam/common/exception/errorCode/UnauthorizedErrorCode.java (2)
10-11: 온보딩 미완 에러코드 추가는 명확하고 👍도메인 시그널이 분리되어 핸들러/클라이언트 단에서 의도를 구분하기 쉬워졌습니다. 네이밍은 살짝 더 간결한
ONBOARDING_INCOMPLETE(또는ONBOARDING_NOT_COMPLETED)도 고려해볼 만합니다.
13-16: HTTP 상태 매핑 재검토: 인증 성공 후 미완 상태라면 401보단 403/409/428 제안
- 문제: 이 enum은 모든 케이스를 401(UNAUTHORIZED)로 반환합니다. 온보딩 미완은 “인증은 됐지만 권한/전제조건 미충족”에 가까워 403(Forbidden) 또는 409(Conflict)/428(Precondition Required)가 더 적합할 수 있습니다.
- 대안(선호): 상수별로 HttpStatus를 가질 수 있게 리팩터링.
- 장점: 의미 있는 상태코드 제공, 클라이언트 UX 개선(재로그인 유도 대신 온보딩 유도).
- 단점: enum 시그니처 변경(생성자/필드 추가)으로 소폭 파급.
예시 diff:
- public enum UnauthorizedErrorCode implements ErrorCode { - - UNAUTHORIZED, - MISSING_AUTHORIZATION_HEADER, - INVALID_AUTHORIZATION_HEADER, - ONBOARDING_HAS_NOT_FINISHED, - ; - - @Override - public HttpStatus getStatus() { - return HttpStatus.UNAUTHORIZED; - } - } + public enum UnauthorizedErrorCode implements ErrorCode { + UNAUTHORIZED(HttpStatus.UNAUTHORIZED), + MISSING_AUTHORIZATION_HEADER(HttpStatus.UNAUTHORIZED), + INVALID_AUTHORIZATION_HEADER(HttpStatus.UNAUTHORIZED), + ONBOARDING_HAS_NOT_FINISHED(HttpStatus.FORBIDDEN); + + private final HttpStatus status; + + UnauthorizedErrorCode(HttpStatus status) { + this.status = status; + } + + @Override + public HttpStatus getStatus() { + return status; + } + }온보딩 미완을 403/409/428 중 무엇으로 노출할지 API 문서/앱 클라이언트와 합의 필요.
backend/src/main/java/backend/mulkkam/common/resolver/MemberResolver.java (2)
47-47: 미세 최적화: 중복 dereference 제거(선택)같은 객체 접근을 한 번만 수행하도록 지역 변수로 담아두면 가독성이 소폭 좋아집니다.
- return new MemberDetails(account.getMember().getId()); + var member = account.getMember(); + return new MemberDetails(member.getId());
39-41: 캐스팅 안전성 확인됨:JwtAuthenticationFilter에서account_id를 항상Long으로 설정하므로(Long)캐스팅은 안전합니다.
선택적 개선:"account_id"문자열 키를AuthAttributes.ACCOUNT_ID등 상수로 중앙 관리하면 오타·분산 리스크를 줄일 수 있습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/src/main/java/backend/mulkkam/common/exception/errorCode/UnauthorizedErrorCode.java(1 hunks)backend/src/main/java/backend/mulkkam/common/resolver/MemberResolver.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
backend/**
⚙️ CodeRabbit configuration file
backend/**: - 1. 팀 및 공식 컨벤션, 가독성, 예외처리, 테스트/확장/유지보수성, 모듈화, API/DB/보안 설계 기준을 기반으로 리뷰해주세요.
- 최신 트렌드, 불필요한 로직, 클린코드, 리팩토링, 서비스/도메인 설계, 공통 예외 처리, 확장성도 함께 확인해주세요.
- 각 피드백은 문제점·대안·장단점을 짧고 논리적으로, 예시 코드가 있다면 간결히 포함해 주세요.
- 팀 내 스타일 통일성도 확인해주세요.
- 미작성한 테스트 코드 케이스가 있다면, 어떤 테스트가 필요한지 제안해주세요. (예: 컨트롤러는 인수 테스트, 나머지는 단위 테스트)
- 리뷰 남겨주는 부분은 해당 라인 범위의 코멘트에 작성해주세요.
Files:
backend/src/main/java/backend/mulkkam/common/exception/errorCode/UnauthorizedErrorCode.javabackend/src/main/java/backend/mulkkam/common/resolver/MemberResolver.java
🔇 Additional comments (2)
backend/src/main/java/backend/mulkkam/common/resolver/MemberResolver.java (2)
4-4: static import 적용 적절의도 전달이 분명해지고 예외 매핑이 깔끔해졌습니다. 팀 컨벤션과도 어긋나지 않습니다.
43-46: NPE 사전 방지 가드 추가 적절
findByIdWithMember이후member == null경로를 명시적으로 분기하여 NPE를 제거하고 도메인 상태(온보딩 미완)를 정확히 신호합니다.
- 제안(테스트): 아래 케이스 단위/슬라이스 테스트 추가 권장
- account 존재 + member null → CommonException(ONBOARDING_HAS_NOT_FINISHED)
- account 미존재 → CommonException(NOT_FOUND_OAUTH_ACCOUNT) (회귀 테스트)
- account 존재 + member 존재 → MemberDetails 정상 반환
원하시면 테스트 스켈레톤을 바로 생성해드릴게요.
리졸버를 왜 도입했나!이야기를 나누기 전에 먼저
따라서 리졸버도 굳이 따지자면? 컨트롤러와 동일한 계층에 해당한다고 생각해요~ 의문점1: 레파지토리를 직접 참조하는게 맞나?그리고 이에 따라서 컨트롤러에서의 토큰 추출 로직을 떠올려보면, 레이어드 아키텍처 구조상으로
의문점2: 서비스 계층에서 멤버 조회가 필수적임에도 불구하고, 리졸버에서 한번 더 조회하는 이점이 그만큼 큰가?지난번에
물론 멤버 관련 서비스가 모두
레파지토리 직접 참조 부분만 해소되어도 일단은 넘어갈 수 있을만한 문제라고 생각해서 어푸 해놓을게요 |
2Jin1031
left a comment
2Jin1031
left a comment
There was a problem hiding this comment.
넘 늦게 확인했네요 ㅜㅜ 죄송합니다,,
문서화도 꼼꼼히 해주셔서 덕분에 훨씬 빠르게 맥락 파악이 가능했어요 👍👍
현 구조를 유지하자는 의견의 동의하는 바입니다~
다만, resolver에서 DB 접근을 배제하려 했던 배경을 다시 짚어보자면,
OSIV가 꺼져 있다는 사실을 인지하지 못한 채 OSIV가 켜져 있다는 전제를 두고 원인을 파악하다보니 영속성 컨텍스트에 대한 개념적인 부분이 흔들렸고, resolver에서의 DB 접근을 배제해야 한다는 결론이 나왔던 것 같다는 생각이 들어요
소신 발언 하자면,, OSIV가 꺼져 있었던 것이 단순 실수였고, 원래 의도했던 방향대로라면 OSIV를 켜고 객체를 반환하는 방식도 충분히 고려할 수 있는 방법이라고 생각해요
3 participants
🔗 관련 이슈
📝 작업 내용
✅ 확인해야 하는 부분
기존에 태스크 분배 시 Resolver 내부에서 DB 접근하는 부분에 대한 우려가 있어, 구조 리팩터링을 하고자 했습니다.
그러나 새로운 구조에 대해 고려해본 결과,
현재 구조를 유지하는 것이 가장 낫다는 판단을 하게 됐습니다.이와 관련해서는 의사 결정 문서 에 더 자세히 정리해두었습니다.
해당 부분 확인해보시고,
현 PR 에 의견 남겨주세요.🤔 고민하고 있는 부분
현재 diff 에 찍히지는 않았지만, OauthAccountResolver 에서는 현재 DB 접근을 하지 않고 있습니다. 즉, OatuhAccount 가 존재하지 않더라도 해당 Resolver 에서 넘어가게 됩니다.
반면, MemberResolver 에서는 OauthAccount 에 대해 존재하는지 확인한 뒤에, Member 의 id 를 반환합니다.
통일성이 없다는 우려가 들기도 했으나, 다음과 같은 이유로 우선은 뒀습니다.
MemberResolver내부에서의 DB 접근 및 OauthAccount 의 유무 확인은 불가피이에 대해서도 의견 주세요~!