Api/feature/65 #66
Api/feature/65 #66
Conversation
| @DeleteMapping(value = "/books/{id}", produces = MediaType.APPLICATION_JSON_VALUE) | ||
| public ResponseEntity deleteBook(@PathVariable Long id, | ||
| @CurrentLoginMember LoginMember loginMember) { | ||
| try { | ||
| bookService.delete(id, loginMember.getId()); | ||
|
|
||
| } catch (PermissionDeniedException e) { | ||
| handleNoAuthorizationException(); | ||
| } | ||
|
|
||
| return new ResponseEntity<>(HttpStatus.OK); | ||
| } |
There was a problem hiding this comment.
try catch 문으로 에러 핸들링을 하셨는데 Exception 이 났을때 호출하는 메서드는 ExceptionHandler 어노테이션을 사용하는 메서드네요
Spring ExceptionHandler 을 키워드로 검색해보시면 아시겠지만 ExceptionHandler 는 어노테이션에 지정된 예외가 발생했을때 스프링 프레임워크가 호출해주는 메서드 입니다. 굳이 try catch 문을 사용할 필요가 없습니다.
There was a problem hiding this comment.
아아 그렇군요! Exception Handler에 대해 공부가 부족했습니다. 공부해서 보완할게요 😀
| package com.squadb.workassistantapi.service.exception; | ||
|
|
||
| public class PermissionDeniedException extends RuntimeException { | ||
|
|
||
| public PermissionDeniedException(String message) { | ||
| super(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 NoAuthorizationException 라는 커스텀 예외 클래스가 있는데요.
의미가 비슷해 보이는데 해당 예외 클래스를 따로 만드신 이유가 있으실까요?
There was a problem hiding this comment.
네 맞습니다. 이 부분이 좀 고민이었는데요. exception이 domain package와 service package 로 나누신 이유가 패키지간 의존성을 낮추기 위함이라고 생각했습니다. 그래서 권한 문제에 대한 예외를 서비스 레이어에서 처리할 때 service package scope 내 사용 가능한 예외 클래스가 있으면 좋겠다고 생각했습니다.
사실상 NoAuthorizationException을 사용해도 상관없습니다. 그런데 그럴거면 상위 패키지를 하나 만들고 여러 서브 Exception 패키지를 모아놓는 게 낫지 않나요? 특별히 exception만 나눠놓는 이유가 궁금합니다.
There was a problem hiding this comment.
domain 패키지 안에도 exceptions 패키지가 있고, service 패키지 안에도 KeyDuplicationExcetpion 이 있고 그러네요.
권한이 없다는 Exception (NoAuthorizationException) 은 domain 패키지 안에 코드 안에서 이루어져야 한다고 생각해요. 왜냐면 권한이 없어서 뭔가 행위를 할 수 없다는것은 비즈니스 로직이기 때문이에요. 비즈니스 로직은 도메인 클래스의 코드안에 있어야 해요.
근데 KeyDuplicationExcetpion 은 도메인 로직에서 커버할 수 없는 영역이에요. DB 라는 외부 세계에 의존하기 때문입니다. 그래서 service 패키지에 exceptions 패키지를 또 만들어서 거기에 둔거 같아요.
그런데 그럴거면 상위 패키지를 하나 만들고 여러 서브 Exception 패키지를 모아놓는 게 낫지 않나요?
만약 service 패키지에 exception 패키지를 모아 뒀다고 가정할께요, 그럼 현재 domain 패키지의 로직에서 service.exception 에 있는 Exception 들을 가져다 쓰지 않을까요? 그럼 그 순간 domain 패키지가 service 패키지에 의존하게 되는 것 이라고 생각합니다.
service 패키지에서도 domain 패키지에 있는 코드를 가져다 쓰고, domain 패키지에서도 service 패키지에 있는 코드를 가져다 쓰는 상황이 일어나게 되는 것입니다 (의존성 사이클), 데이터는 최대한 단방향으로 흐르는게 유지보수에 좋고 의존성도 단방향 의존성만 있는 것이 유지보수에 좋습니다. 아예 없으면 best 고요
사실 지금 의존성 사이클이 생기면 큰 문제가 되는건 아니지만, 연습 할 때 좀 빡세게 연습 (이런 패키지 의존성 까지 잘 고려를 해서 연습) 하면 실무를 할때 확실히 도움이 될거 같아요.
There was a problem hiding this comment.
말씀해주신 부분 곱씹어보면서 많은 부분을 배울 수 있었습니다. 감사합니다!!!
| public <X extends Throwable> Member orElseThrow(Supplier<? extends X> exceptionSupplier) throws X { | ||
| if (this != null) { | ||
| return this; | ||
| } else { | ||
| throw (X) exceptionSupplier.get(); | ||
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
이 메서드도 Member 객체의 역할과 책임에 어긋나는 메서드 인거 같습니다.
Member 객체가 null 일때 뭔가 처리해줘야 하는 로직은 Member 를 사용하는 클라이언트에 있어야 하는 로직이라고 생각해요.
Member 객체안에는 Member(회원) 에 대한 비즈니스 로직이 들어가야 간다고 생각합니다.
| public Member ifAdmin(Consumer<Member> action) { | ||
| if (isAdmin()) { | ||
| action.accept(this); | ||
| return this; | ||
| } else { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
'Member 가 admin 일때 특정 Member 에 대한 action 을 해주겠다' 라는 의도의 메서드 인가요?
그렇다면 '실행 하려고 하는 Member 에 대한 action' 을 명확한 input 과 ouput 을 가지는 메서드로 표현하는게 더 좋을거 같습니다. 그 메서드 안에서 admin 검사를 하면 되는 것이고요.
그리고 public 메서드에서 null 을 리턴해버리면. 해당 메서드를 사용하는 불특정 클라이언트에서 계속 null 체크를 해줘야 합니다. 그런데 딱히 이 메서드에서는 리턴값도 필요가 없어보이네요ㅜ
There was a problem hiding this comment.
이 메소드에서 의도한 바는 관리자만 할 수 있는 여러 action 들을 추상화해서 좀 더 유연하게 사용하게 하기 위함이었습니다. 제가 제대로 이해했는 지 모르겠지만 input과 output를 고정시켜 놓으면 하나의 비즈니스 로직만을 위한 메소드가 되어버리는 데 그러면 해당 메소드의 필요성이 희석되는 것같습니다. action을 추상화시키면 안 된다는 말씀이실까요?
- 정확하게 지적하셨듯이 null을 나중에 오는 orElse 메소드를 사용하기 위한 장치로 만들어주었습니다ㅠㅠ
Optional 형태의 메소드 구조를 참고했는데 이 부분은 제가 잘못 설계한 것같네요ㅠ 다시 고민해보겠습니다.
There was a problem hiding this comment.
이 메소드에서 의도한 바는 관리자만 할 수 있는 여러 action 들을 추상화해서 좀 더 유연하게 사용하게 하기 위함이었습니다. 제가 제대로 이해했는 지 모르겠지만 input과 output를 고정시켜 놓으면 하나의 비즈니스 로직만을 위한 메소드가 되어버리는 데 그러면 해당 메소드의 필요성이 희석되는 것같습니다. action을 추상화시키면 안 된다는 말씀이실까요?
우리가 추상화 시킬 것은, 요구사항에서 변하지 않는 것과 변하는 것을 잘 구분해 변하지 않는 것을 추상화 시켜야 합니다.
'책은 관리자만 삭제할 수 있어야 한다.' 에서 변하지 않는 것과 변하는 것은 무엇일까요?
제 생각에는 잘 변하지 않는 것은 '책을 삭제 할 때 뭔지는 몰라도 뭔가 권한이 있어야 삭제할 수 있다.' 이고, 잘 변하는 것은 '책을 삭제할 때의 구체적인 권한' 입니다. 지금은 관리자만 책을 지울 수 있지만, 나중에는 관리자가 아닌 등록자만 지울 수 있게 될 수도 있고, 특별한 사람에게 권한을 주어 책을 지울 수도 있게 할 수도 있습니다. 그래서 저는 관리자가 할 수 있는 액션이 아닌 권한을 검사하는 것 을 추상화 시켜야 한다고 생각합니다.
그리고 관리자만 책 이름을 수정할 수 있다는 요구사항이 있다고 하면.
위의 메서드를 이용하면 아래와 같은 코드가 나올거 같아요
if (member.ifAdmin(m -> book.changeTitle(title)) == null) {
throw ...
}
저는 책을 수정하는 로직 안에 수정 권한을 체크하는 로직이 있어야 한다고 생각합니다. 그게 다른 개발자가 책 이름을 수정하는 메서드 (Book.changeTitle) 를 봤을때, '아 수정할때는 특정 권한이 필요하구나' 라는 걸 바로 알 수 있겠죠.
위의 코드가 많아지면 책 이름을 수정하는 메서드도 보고 그 메서드가 어떻게 호출 되는지 까지 봐야 요구사항이 파악이 됩니다.
There was a problem hiding this comment.
아아 그런 관점에서 보고 계셨군요. 그러면 관리자의 액션보다 삭제하는 사람의 권한에 확장성을 주는 게 맞겠습니다. 그러한 측면에서 리팩토링해보겠습니다. 좋은 의견 감사드립니다!
|
그리고 책을 삭제할때, 이미 대여중인 책인지 아닌지도 확인을 해봐야 할 거 같네용 |
| } | ||
|
|
||
| public <X extends Throwable> Member orElseThrow(Supplier<? extends X> exceptionSupplier) throws X { | ||
| if (this != null) { |
|
|
||
| memberService.findById(registrantId) | ||
| .ifAdmin((m) -> bookRepository.deleteById(bookId)) | ||
| .orElseThrow(() -> new PermissionDeniedException("Authorization Required")); |
There was a problem hiding this comment.
ifAdmin 메서드가 관리자가 아닌 경우에 null을 반환하는것 같은데, NPE 나지 않나요?? 🤔
의도대로 동작하는지 테스트코드 짜보시는 것도 좋을 것 같아요 :-)
There was a problem hiding this comment.
네네ㅠㅠㅠ 테스트 코드 짜는 법을 공부해서 한 번 재확인해보겠습니다! 좋은 지적 감사합니다ㅎㅎㅎㅎ
4 participants
등록된 65번 이슈에 대한 PR 입니다.