[Feat] #593 - 플그관련 default 유저 삭제 Internal api 구현 #594
Conversation
|
| if (!internalApiKey.equals(apiKey)) { | ||
| throw new UnauthorizedException(ErrorCode.INVALID_INTERNAL_API_KEY); | ||
| } |
There was a problem hiding this comment.
간단한 로직이어서 이렇게 컨트롤러 메서드마다 호출해도 괜찮겠지만, 제가 작성한 유저 생성 부분과 중복되는 부분이니 메서드 추출을 하는 것은 어떨까요? 추후 api-key를 검증하는 부분에서 예외 발생 시 로그를 남기고 싶다~ 등의 공통적인 수정 사항이 발생했을 때 관리가 더 편할 것 같습니다~
There was a problem hiding this comment.
지훈님 이 부분 반영해주시면 좋을 것 같습니다!
There was a problem hiding this comment.
저도 말씀해주신 내용에 동의합니다 !
private으로 validate 메서드를 공통 메서드로 선언해줘서, 반영했습니다
+추가적으로 얘기했었던 내용이지만, 메모용으로 남겨놓겠습니다
사실 이전에 선행작업 해주신 재헌님 PR을 보면서도 들었던 생각이였지만
"컨트롤러단에서, 401을 검증해서 던져주는게 책임 분리 측면에서 맞는 방식일까?" 라는 생각이 들었었습니다
개인적인 생각으론 컨트롤러 이전에 필터나 인터셉터로 핸들러로 잡아서 처리해주는건 리소스 적으로 너무 크다고 판단했었고,
메서드를 별도의 서비스단으로 분리해주자니 다른 도메인들도 별도의 검증 서비스는 존재하지 않았던 상태여서 고민이 됐었습니다
인증중앙화가 진행 되면서 API 들이 확장될게 예상되지만, 계속 이번처럼 작업을 하게 된다면 유지보수가 힘들거같다는 판단도 들어서 다같의 의논해보면 더 좋을거같네요 !
| public void deleteUser(Long userId) { | ||
| userRepository.findUserById(userId) | ||
| .orElseThrow(() -> new NotFoundException(ErrorCode.USER_NOT_FOUND)); | ||
|
|
||
| userRepository.deleteById(userId); | ||
| } |
There was a problem hiding this comment.
just wonder인데요 ~~
userRepository.findUserById(userId)
.orElseThrow(() -> new NotFoundException(ErrorCode.USER_NOT_FOUND));
지훈님이 작성하신 부분들 말고도 기존 코드들에서도 Service 단에서 매번 이렇게 repository 에서 find를 하고 예외처리하는 부분들을 하나의 메서드에서 처리하더라구요. 이렇게 작성하면 어떤 장점이 있나요? 좀 더 세세한 컨트롤이 필요할 경우를 위해서인가요?
위 부분은 유저를 찾아오며 못찾은 경우 예외 발생이라는 플로우다보니 메서드에 책임이 너무 많아져버리진 않을까요? 이 부분을 따로 분리하는 것에 대해서는 어떻게 생각하세요??
There was a problem hiding this comment.
보통 케이스 분리를 위해 따로 분리하지 않고 명시하는 목적으로 사용하는데, findUserById는 반복부가 너무 많아서 분리하는 게 더 좋을 것 같아요! 이번 Pr에 바로 반영하실 필요는 없지만 이후 작업하면 좋겠네요~
There was a problem hiding this comment.
제가 이해한 바로는 따로 분리한다고 말씀해주신 부분이 아래 두가지 방식을 생각했었는데,
-
별도의 읽기/쓰기 레이어로 분리하자
-
UserGetService,UserUpdateService등으로 유스케이스 단위로 서비스 클래스를 나누어주자
만약 맞다면 위 방식으로 리팩토링을 진행했을때 작업 리소스랑 관련해서는 어떻게 생각하시는지도 궁금했습니다 !
아니라면 다른 더 좋은 방식이 있다면 의견 주시면 저도 같이 생각해볼게요 ~~
There was a problem hiding this comment.
제가 생각한 방식은 기존 레이어 자체를 건드리는 게 아니고 메서드 추출이었습니다 ~ 한 메서드에서 너무 많은 역할을 갖는 게 크게 좋지 않다고 봤고 (SRP), findUserById 같이 기본적으로 많이 쓰이는 부분은 중복이 너무 많아지는 것 같아서요!!
| @ApiResponse(responseCode = "401", description = "token error", content = @Content), | ||
| @ApiResponse(responseCode = "500", description = "server error", content = @Content) | ||
| }) | ||
| @DeleteMapping("/{userId}") |
There was a problem hiding this comment.
rollback delete api도 security whitelist에 추가해야할 것 같은데 엔드포인트가 api/v2/user/{userId}면 관리가 어려울 것 같아요!
api/v2/user/rollback/{userId}와 같은 엔드포인트는 어떠신가요?
There was a problem hiding this comment.
먼저 답변부터 드리면 api/v2/user/rollback/{userId}의 엔드포인트가 충분히 직관적이라고 생각해서 반영하도록 할게요 !
의도를 추가로 설명 드리자면, 엔드포인트와 같은 경우는 저도 고민을 많이 했던 부분이였지만
최대한 플그팀과 크로스 체크를 위해서 작업을 요청해주신 API 엔드포인트인 /api/v1/members/:{memberId} 와 비슷하게 설계하려고 해당 엔드포인트로 설계했었습니다
3 participants
Related issue 🛠
Work Description ✏️
DELETE메소드로default유저 삭제Internal api구현Uncompleted Tasks 😅
x
To Reviewers 📢
create.sql로 테이블 생성 시 안맞는 부분들이 조금 있었어서 해당 부분은 조금씩 수정하면서 테스트 진행했습니다DELETE메소드의 응답HTTP코드는 204가 원칙인걸로 알고있긴하지만, 컨벤션으로 200으로 통일된거같아, 다른API들이랑 동일하게HttpStatus.OK로 반환해주었습니다UserService단에서 던져주도록 하였습니다