-
Notifications
You must be signed in to change notification settings - Fork 6
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
[BE] Feat/#586 핀 수정 시 변경 이력 저장 이벤트 구현 #591
Conversation
- additivity-false는 기본 콘솔 로그와의 중복 출력 방지를 위함
- 회원 차단 시, 토픽 삭제 시, 핀 삭제 시 soft delete
- 핀의 정보 변경 이력을 관리하는데 update라는 수식어가 불필요함
- pin의 FK 외에도 변경 내용을 함께 저장해야 함
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.
도이... 멋집니다...
이벤트는 이렇게 쓰는 거란 걸 배울 수 있었습니다 👍👍👍
커밋 순서대로 확인하고 있는데
정책적인 부분도, 코드적인 측면도 도이가 고민하신 내용이 굉장히 질이 높다는 생각이 드는군요...
많이 배웠습니다.
그냥 PinHistory로 네이밍 변경하면 좋겠다고 생각했는데 해주셨군요... 굿굿..
전체적으로 제가 우리 코드에 리팩터링하면서 적용하고 싶었던거에 대한 가장 좋은 예시인 것 같습니다.
수고하셨습니다~!!
@@ -142,6 +146,26 @@ void addIfNotExistDuplicateLocation_Success() { | |||
assertThat(response.statusCode()).isEqualTo(HttpStatus.CREATED.value()); | |||
} | |||
|
|||
@Test |
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.
오 인수 테스트 먼저 진행하셨군요? ㄷㄷ
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.
이벤트 로그 찍히는 걸 인수 테스트에서 확인하려니 핀 수정 테스트가 없었더라고요 허헣
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.
구두로 말씀드린 부분들만 수정되면 바로 approve 드리도록 하겠습니다~!
@Modifying(clearAutomatically = true) | ||
@Query("delete from Bookmark b where b.member.id = :memberId") |
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.
flush 를 왜 다 지우셨나 했는데 @query 어노테이션도 추가적으로 붙여주시면서 @Modifying 어노테이션이 동작하게 해주셨군요! 굳굳
pinRepository.save(pin); | ||
eventPublisher.publishEvent(new PinUpdateEvent(pin, member)); |
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.
기가 막힙니다!
@@ -39,52 +38,42 @@ class AdminCommandServiceTest { | |||
|
|||
@Autowired | |||
private AdminCommandService adminCommandService; | |||
|
|||
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.
? ? ? ? ? ? ? !
assertThat(histories.get(0)).usingRecursiveComparison() | ||
.ignoringFields("id", "updatedAt", "createdAt") | ||
.isEqualTo(new PinHistory(pin, member)); |
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.
usingRecursiveComparison 최고!
ignoringFieldsOfTypes(LocalDateTime.class) 로도 할 수 있더라고요 호호
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.
오 좋네요! 반영 + 핀 변경 일시도 검증하도록 추가하겠습니다
@DisplayName("핀 정보 이력 저장 시 예외가 발생하면, 추가된 핀 정보도 저장하지 않는다.") | ||
void save_FailBySaveHistoryException() { | ||
// given | ||
doThrow(new RuntimeException()).when(pinHistoryCommandService).saveHistory(any(PinUpdateEvent.class)); |
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.
찾아낸 거 대바악
assertThatThrownBy(() -> pinCommandService.save(new Guest(), List.of(BASE_IMAGE_FILE), createRequest)) | ||
.isInstanceOf(RuntimeException.class); | ||
assertThat(pinRepository.findAll()).isEmpty(); | ||
} |
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.
여기서 pinCommandService.save
첫번째 인자로 Guest 가 들어가면 바로 Exception 이 터지지 않나용?
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.
맞네요!!
동일한 방식의 테스트들 모두 pinCommandService.save는 정상동작하도록 하고, 이벤트리스너에서 터지는 예외의 종류를 더 명시적으로 수정하겠습니다!
야무진 리뷰 감사합니당 ㅋㅋ
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.
해당 테스트를 기대한 방식대로 돌아가게 수정하면서, save가 롤백되지 않는 문제를 만났고
DataJpaTest를 통해서는 검증할 메서드(save)를 롤백시킬 수 없다는 것을 알았습니다..!! 이 코멘트 아니면 제대로 알지 못하고 넘어갈 뻔 했네요.
DataJpaTest는 @Transactional
을 전파속성 기본값(required)으로 지원해서, 트랜잭션이 테스트 메서드 하나로 묶이기 때문입니다.
이 메서드에 required new 를 붙여도, 부모 트랜잭션이 롤백된다고 자식 트랜잭션이 롤백되지는 않는다고 합니다.
그래서 PinIntegrationTest에 EventListenerTest 라는 내부 클래스를 만들어 '롤백에 대한 테스트'를 추가했습니다.
PinCommandServiceTest 입장에서 PinHistoryCommandService는 제어할 수없는 요소라는 점에서도 통합 테스트에서 하는 게 더 적절해 보이는 것 같아요.
assertThatThrownBy( | ||
() -> pinCommandService.update(new Guest(), illegalPinId, new PinUpdateRequest("name", "update")) | ||
).isInstanceOf(PinForbiddenException.class); | ||
|
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.
이!것!두!
@@ -54,6 +54,7 @@ public AdminCommandService( | |||
public void blockMember(Long memberId) { | |||
Member member = findMemberById(memberId); | |||
member.updateStatus(Status.BLOCKED); | |||
memberRepository.flush(); |
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.
도이가 쓴 글에 의하면 hardDelete
가 진행되기 이전에 flush
가 발생하면 되고, 그렇다면 permissionRepository.deleteAllByMemberId()
를 호출하게 되면서 flush
가 발생하게 되니까명시적인 flush 가 없어도 되지 않을까요??
근데, 해당 flush
를 호출하지 않으면 예상한 것과 같이 동작하지 않는다고 하셨으니 추후에 같이 탐구해봐야겠군요!
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.
맞아요. Repository에서 제공하는 flush를 통해 member의 update 쿼리도 같이 나갈 줄 알았는데,
그렇지 않아서 추가해주게 되었어요 ㅜㅜ
@Modifying
으로 실행되는 flush는 해당 repository 메서드의 쿼리만 flush해주는 걸까요?
원인은 더 탐구해보는 것으로..
- pinHistory의 createdAt과 저장 시 pin 의 updatedAt은 미세하게 다르므로 정확한 일시를 기록
- DataJpaTest에서는 내부 트랜잭션의 롤백을 검증할 수 없음
추가 push하면서 pinUpdatedAt 컬럼을 추가했습니다. |
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.
도이 고생하셨습니다 !!!!!! 테스트도 꼼꼼히 짜주셔서 좋았어여
리뷰가 늦어서 죄송해요.
늦었음에도 리뷰가 하나라 죄송해요.
리뷰가 하나임에도, 헷갈린다고 말해서 죄송해요.
그냥 죄송해요. 못하겠어요 ㅠㅠ
회의가 어떻게 진행되었는지는 잘 모르겠으나, 로그가 기록되지 않으면 변경되지 않아야한다는 쪽으로 결론이 난거겠죠 ?
한 편으로는, 변경 로그를 찍는 시스템(?)에 문제가 발생했을 때, 변경 자체를 수행하지 못하는 흐름이 괜찮을까 싶긴 하네요 !
@Column(name = "pin_updated_at", nullable = false) | ||
private LocalDateTime pinUpdatedAt; |
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.
BaseEntity
의 createdAt
과 동일하지 않나요 ?
헷갈리네요..
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.
BaseEntity의 createdAt은 해당 엔티티가 영속화될때 저장되는데
변경 이력 저장 시점에 따라 pinUpdatedAt과 미세하게 다를 수 있습니다.
@PrePersist 에서 해주기에는 Pin이 PinHistory를 몰라야 한다고 생각하고요!
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.
Pin을 업데이트 할 때에도, pin.updatedAt은 영속화 시점에 결정되지않나요 ?
같은 트랜잭션으로 묶여있는 현 상황에서도 서로 불일치할 수 있는건가요 ?
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.
createdAt, updatedAt 모두 종합해서 말씀드린거였어요!!
JpaAuditing은 해당하는 엔티티에 대해 persist를 호출하기 전
동작하기 때문에
Pin에 대한 persist가 호출되기 전
과 PinUpdateEvent에 대한 이벤트리스너가 동작-> PinHistory에 대한 persist가 호출되기 전
은 다르지 않나요..?! 혹시 제가 잘못 생각하고 있는 부분이 있을까요?
pinRepository.save(pin);
eventPublisher.publishEvent(new PinUpdateEvent(pin, member)); // pinHistoryRepository.save(...);
++ 그리고 같은 트랜잭션으로 묶여 있다 해도, 아래 코멘트에서 말씀해주신 것처럼 서로 다른 트랜잭션으로 가져가도록 변경할 여지가 있다면 별개로 관리하는 건 어떨까요? BaseEntity는 기본적으로 저장하는, '레코드'에 대한 시간 정보라는 생각도 들고요!
ㅠㅠ
저도 동일한 생각으로, 처음에는 |
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.
고생하셨습니다 또이또이!
assertSoftly(softly -> { | ||
assertThat(histories.get(0)).usingRecursiveComparison() | ||
.ignoringFields("id") | ||
.ignoringFieldsOfTypes(LocalDateTime.class) |
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.
야무지게 추가해주셨군요!
@DisplayName("Pin 저장 시 변경 이력 저장에 예외가 발생하면, 변경 사항을 함께 롤백한다.") | ||
void savePin_FailBySaveHistory_Rollback() { | ||
//given | ||
doThrow(new IllegalStateException()).when(pinHistoryCommandService).saveHistory(any(PinUpdateEvent.class)); |
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.
말씀대로 IntegrationTest 로 옮겨서 해결해주셨군요 굳굳
작업 대상
#586 참조부탁드립니다.
history 패키지를 새로 만들어 작업하였습니다.
핀 정보의 변경 이력이긴 하지만, 횡단 관심사인 이력 관리 - 'history'라는 분류로 관리하는 것이 좋다고 판단했습니다.
📄 작업 내용
기존 코드에 변경 없이 횡단 관심사를 도입하고자 Spring Event를 사용했는데요.
자세한 동작 원리는 잘 모르지만 사용법 위주로 적용해보았습니다!
이벤트 정의
그냥 class, record를 정의하는 것만으로도 이벤트로 사용할 수있습니다.
해당 클래스 타입으로 이벤트를 인식해요.
우리 코드에서는 이벤트로 사용할 PinUpdateEvent 라는 레코드를 정의했습니다.
이벤트 발행
Spring에서 제공하는 ApplicationEventPublisher를 주입받아 사용합니다.
서비스에서 주입받아, 아래와 같이 이벤트를 발행합니다.
이벤트 구독
이벤트가 발행되면, 해당 이벤트를 구독하는 EventListener가 발행한 이벤트 정보를 받아 로직을 수행합니다.
위 코드처럼, 구독할 이벤트를 메서드 파라미터에 정의하고
@EventListener
어노테이션을 붙여주면 됩니다.@TransactionalEventListener
를 사용하면 트랜잭션 커밋, 롤백을 기준으로 발행 시점을 정할 수 있는데,오늘 회의 때 논의한 것처럼
현재 변경 이력 저장 로직은 기존 트랜잭션과 함께 묶이는 것이 적절하다고 판단해 적용하지 않았습니다.
🙋🏻 주의 사항
테스트 내 일부 모킹
핵심 도메인에서 의존성이 분리된 로직이므로, PinCommandService에서는 해당 내용에 대해 테스트할 때 Mocking을 해주는 것이 적합하다고 판단했습니다. 따라서 행위만을 테스트하도록 했습니다.
대신 PinHistoryCommandService에서 실제 동작을 테스트합니다.
참조하는 핀이 삭제되거나 회원이 차단된다면?
상관 없이 변경 이력은 삭제 처리되지 않고 그처럼 계속 남아있습니다.
데이터가 너무 많이 쌓일 수도 있지만, 그건 사용자가 아주 많아졌을 때 고려해도 될 문제라고 생각합니다.
처음에는 참조하는 토픽, 핀이 삭제되거나 회원이 차단되면
즐겨찾기, 모아보기처럼 삭제해야 한다고 생각해서 열심히 작업하다가.. 롤백했습니다.
왜냐면 토픽, 핀, 회원은 모두 soft delete로 관리되어 실제로 삭제되지 않습니다.
그래서 FK 조건을 위배할 일도 없습니다.
그런 맥락에서 변경 이력에는 삭제 여부라는 개념이 없는 게 더 자연스러운 것 같아요.
만약 특정 회원이 참여한 모든 변경 이력을 조회한다고 해도, '삭제된 핀'에 대한 변경 이력을 보여주고 싶을 수도 있다고 생각해요.
예를 들면, 게시판 서비스에서 내가 작성한 덧글 목록을 볼 때 삭제된 글의 덧글도 보여주기도 하는 것처럼요.
스크린샷
📎 관련 이슈
closed #586
레퍼런스
참고 문건도 이슈에 링크해두었습니다!