-
Notifications
You must be signed in to change notification settings - Fork 1
Api/feature/65 #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Api/feature/65 #66
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,13 +8,17 @@ | |
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
|
|
||
| import com.squadb.workassistantapi.service.exception.PermissionDeniedException; | ||
| import com.squadb.workassistantapi.util.HashUtil; | ||
| import com.squadb.workassistantapi.web.exception.LoginFailedException; | ||
|
|
||
| import lombok.AccessLevel; | ||
| import lombok.Getter; | ||
| import lombok.NoArgsConstructor; | ||
|
|
||
| import java.util.function.Consumer; | ||
| import java.util.function.Supplier; | ||
|
|
||
| @Getter | ||
| @Entity | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
|
|
@@ -50,6 +54,25 @@ public boolean isAdmin() { | |
| return type.isAdmin(); | ||
| } | ||
|
|
||
| // 관리자 권한을 가진 사람에 대한 커스텀 액션 실행 메소드 | ||
| public Member ifAdmin(Consumer<Member> action) { | ||
| if (isAdmin()) { | ||
| action.accept(this); | ||
| return this; | ||
| } else { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| public <X extends Throwable> Member orElseThrow(Supplier<? extends X> exceptionSupplier) throws X { | ||
| if (this != null) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this가 null 일수가 있나요?? 🤔
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 진짜 다시 보니 코드가 개판이군여.... |
||
| return this; | ||
| } else { | ||
| throw (X) exceptionSupplier.get(); | ||
| } | ||
|
|
||
| } | ||
|
|
||
|
Comment on lines
+67
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 메서드도 Member 객체의 역할과 책임에 어긋나는 메서드 인거 같습니다. |
||
| public void checkEqualPassword(String passwordInput) { | ||
| if (!HashUtil.equalPassword(passwordInput, passwordHash)) { | ||
| throw LoginFailedException.wrongPassword(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import java.util.List; | ||
|
|
||
| import com.squadb.workassistantapi.service.exception.PermissionDeniedException; | ||
| import org.springframework.data.domain.Sort; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.transaction.annotation.Transactional; | ||
|
|
@@ -37,6 +38,14 @@ public Long register(final Book book, final Long registrantId) { | |
| return saveBook.getId(); | ||
| } | ||
|
|
||
| @Transactional | ||
| public void delete(Long bookId, Long registrantId) { | ||
|
|
||
| memberService.findById(registrantId) | ||
| .ifAdmin((m) -> bookRepository.deleteById(bookId)) | ||
| .orElseThrow(() -> new PermissionDeniedException("Authorization Required")); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ifAdmin 메서드가 관리자가 아닌 경우에 null을 반환하는것 같은데, NPE 나지 않나요?? 🤔
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 네네ㅠㅠㅠ 테스트 코드 짜는 법을 공부해서 한 번 재확인해보겠습니다! 좋은 지적 감사합니다ㅎㅎㅎㅎ |
||
| } | ||
|
|
||
| private void checkIsbnDuplication(final String isbn) { | ||
| if (bookRepository.findByIsbn(isbn).isPresent()) { | ||
| throw new KeyDuplicationException("key duplication book : [" + isbn + "]"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package com.squadb.workassistantapi.service.exception; | ||
|
|
||
| public class PermissionDeniedException extends RuntimeException { | ||
|
|
||
| public PermissionDeniedException(String message) { | ||
| super(message); | ||
| } | ||
| } | ||
|
Comment on lines
+1
to
+8
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 현재 NoAuthorizationException 라는 커스텀 예외 클래스가 있는데요.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 네 맞습니다. 이 부분이 좀 고민이었는데요. exception이 domain package와 service package 로 나누신 이유가 패키지간 의존성을 낮추기 위함이라고 생각했습니다. 그래서 권한 문제에 대한 예외를 서비스 레이어에서 처리할 때 service package scope 내 사용 가능한 예외 클래스가 있으면 좋겠다고 생각했습니다.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. domain 패키지 안에도 exceptions 패키지가 있고, service 패키지 안에도 KeyDuplicationExcetpion 이 있고 그러네요. 근데 KeyDuplicationExcetpion 은 도메인 로직에서 커버할 수 없는 영역이에요. DB 라는 외부 세계에 의존하기 때문입니다. 그래서 service 패키지에 exceptions 패키지를 또 만들어서 거기에 둔거 같아요.
만약 service 패키지에 exception 패키지를 모아 뒀다고 가정할께요, 그럼 현재 domain 패키지의 로직에서 service.exception 에 있는 Exception 들을 가져다 쓰지 않을까요? 그럼 그 순간 domain 패키지가 service 패키지에 의존하게 되는 것 이라고 생각합니다. service 패키지에서도 domain 패키지에 있는 코드를 가져다 쓰고, domain 패키지에서도 service 패키지에 있는 코드를 가져다 쓰는 상황이 일어나게 되는 것입니다 (의존성 사이클), 데이터는 최대한 단방향으로 흐르는게 유지보수에 좋고 의존성도 단방향 의존성만 있는 것이 유지보수에 좋습니다. 아예 없으면 best 고요 사실 지금 의존성 사이클이 생기면 큰 문제가 되는건 아니지만, 연습 할 때 좀 빡세게 연습 (이런 패키지 의존성 까지 잘 고려를 해서 연습) 하면 실무를 할때 확실히 도움이 될거 같아요.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 말씀해주신 부분 곱씹어보면서 많은 부분을 배울 수 있었습니다. 감사합니다!!! |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,16 +3,12 @@ | |
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| import com.squadb.workassistantapi.service.exception.PermissionDeniedException; | ||
| import org.springframework.data.domain.Sort; | ||
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.http.MediaType; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.ExceptionHandler; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.PostMapping; | ||
| import org.springframework.web.bind.annotation.RequestBody; | ||
| import org.springframework.web.bind.annotation.RestController; | ||
| import org.springframework.web.bind.annotation.*; | ||
|
|
||
| import com.squadb.workassistantapi.domain.Book; | ||
| import com.squadb.workassistantapi.domain.exceptions.NoAuthorizationException; | ||
|
|
@@ -51,6 +47,19 @@ public ResponseEntity<BookRegisterResponseDto> registerBook(@RequestBody BookReg | |
| return new ResponseEntity<>(BookRegisterResponseDto.success(bookId), HttpStatus.OK); | ||
| } | ||
|
|
||
| @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); | ||
| } | ||
|
Comment on lines
+50
to
+61
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. try catch 문으로 에러 핸들링을 하셨는데 Exception 이 났을때 호출하는 메서드는 ExceptionHandler 어노테이션을 사용하는 메서드네요 Spring ExceptionHandler 을 키워드로 검색해보시면 아시겠지만 ExceptionHandler 는 어노테이션에 지정된 예외가 발생했을때 스프링 프레임워크가 호출해주는 메서드 입니다. 굳이 try catch 문을 사용할 필요가 없습니다.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 아아 그렇군요! Exception Handler에 대해 공부가 부족했습니다. 공부해서 보완할게요 😀 |
||
|
|
||
| @GetMapping(value = "/books", produces = MediaType.APPLICATION_JSON_VALUE) | ||
| public ResponseEntity<List<BookListResponseDto>> findAll() { | ||
| List<Book> bookList = bookService.findAll(Sort.by(Sort.Direction.DESC, "id")); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Member 가 admin 일때 특정 Member 에 대한 action 을 해주겠다' 라는 의도의 메서드 인가요?
그렇다면 '실행 하려고 하는 Member 에 대한 action' 을 명확한 input 과 ouput 을 가지는 메서드로 표현하는게 더 좋을거 같습니다. 그 메서드 안에서 admin 검사를 하면 되는 것이고요.
그리고 public 메서드에서 null 을 리턴해버리면. 해당 메서드를 사용하는 불특정 클라이언트에서 계속 null 체크를 해줘야 합니다. 그런데 딱히 이 메서드에서는 리턴값도 필요가 없어보이네요ㅜ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메소드에서 의도한 바는 관리자만 할 수 있는 여러 action 들을 추상화해서 좀 더 유연하게 사용하게 하기 위함이었습니다. 제가 제대로 이해했는 지 모르겠지만 input과 output를 고정시켜 놓으면 하나의 비즈니스 로직만을 위한 메소드가 되어버리는 데 그러면 해당 메소드의 필요성이 희석되는 것같습니다. action을 추상화시키면 안 된다는 말씀이실까요?
Optional 형태의 메소드 구조를 참고했는데 이 부분은 제가 잘못 설계한 것같네요ㅠ 다시 고민해보겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우리가 추상화 시킬 것은, 요구사항에서 변하지 않는 것과 변하는 것을 잘 구분해 변하지 않는 것을 추상화 시켜야 합니다.
'책은 관리자만 삭제할 수 있어야 한다.' 에서 변하지 않는 것과 변하는 것은 무엇일까요?
제 생각에는
잘 변하지 않는 것은 '책을 삭제 할 때 뭔지는 몰라도 뭔가 권한이 있어야 삭제할 수 있다.'이고,잘 변하는 것은 '책을 삭제할 때의 구체적인 권한'입니다. 지금은 관리자만 책을 지울 수 있지만, 나중에는 관리자가 아닌 등록자만 지울 수 있게 될 수도 있고, 특별한 사람에게 권한을 주어 책을 지울 수도 있게 할 수도 있습니다. 그래서 저는관리자가 할 수 있는 액션이 아닌권한을 검사하는 것을 추상화 시켜야 한다고 생각합니다.그리고
관리자만 책 이름을 수정할 수 있다는 요구사항이 있다고 하면.위의 메서드를 이용하면 아래와 같은 코드가 나올거 같아요
저는 책을 수정하는 로직 안에 수정 권한을 체크하는 로직이 있어야 한다고 생각합니다. 그게 다른 개발자가 책 이름을 수정하는 메서드 (Book.changeTitle) 를 봤을때, '아 수정할때는 특정 권한이 필요하구나' 라는 걸 바로 알 수 있겠죠.
위의 코드가 많아지면 책 이름을 수정하는 메서드도 보고 그 메서드가 어떻게 호출 되는지 까지 봐야 요구사항이 파악이 됩니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아아 그런 관점에서 보고 계셨군요. 그러면 관리자의 액션보다 삭제하는 사람의 권한에 확장성을 주는 게 맞겠습니다. 그러한 측면에서 리팩토링해보겠습니다. 좋은 의견 감사드립니다!