#7 Sse알림 기능에 Redis 도입 #8
Conversation
f-lab-moony
left a comment
f-lab-moony
left a comment
There was a problem hiding this comment.
고생 많으셨습니다!
피드백을 좀 남겨 두었는데, 확인 후에 머지 하도록 할게요 ~
| // @OneToOne 참조 관계를 설정하였습니다 | ||
| @OneToOne | ||
| @JoinColumn(name = "NOTIFICATION_ID") | ||
| Notification notification; |
There was a problem hiding this comment.
요거는 제가 그림이 약간 안그려지는데, 답변 하나에 알람이 하나만 만들어질 수 있는걸까요 ?
There was a problem hiding this comment.
음 제가 잘못한거 같네요. @OnetoOne이 아니라 @onetomany가 맞을 것 같습니다
| // Notification 엔티티와 | ||
| // @OneToMany 참조 관계를 설정하였습니다 | ||
| @OneToMany(mappedBy = "receiver") | ||
| @JsonManagedReference |
There was a problem hiding this comment.
이 어노테이션 사용하신 이유가 있을까요 ?
There was a problem hiding this comment.
순환 참조가 발생해서, 순환 참조를 방지하기 위해서 사용했습니다!
| @Getter | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
|
|
||
| @Setter |
There was a problem hiding this comment.
넵 삭제하겠습니다.
지양해야 하는 이유에 대해서도 알 수 있을까요?
| @Entity | ||
| @Getter | ||
| @Builder | ||
| @RequiredArgsConstructor |
There was a problem hiding this comment.
@RequiredArgsConstructor 와 @AllArgsConstructor를 같이 쓰신 이유가 뭘까요 ?
그거와 별개로 두 어노테이션은 사용이 지양되는 녀석들이라 생성자 직접 생성을 권장드립니다.
There was a problem hiding this comment.
넵 삭제하겠습니다.
사용을 지양해야 하는 이유에 대해서도 알 수 있을까요?
| List<Notification> notifications = redisDao.getValuesForNotification(SseId); | ||
| if(notifications == null){ | ||
| redisDao.setValuesForNotification(SseId); | ||
| }else{ |
There was a problem hiding this comment.
넵 이 부분 참고해보겠습니다
| if(notifications == null){ | ||
| redisDao.setValuesForNotification(SseId); | ||
| }else{ | ||
| for(Notification notification: notifications){ |
There was a problem hiding this comment.
이 작업을 null이 아닐 때만 하게 하면 setValue 부분은 분기문 밖에서 공통으로 진행될 수 있지 않을까요 ?
| // emitter가 null이 아닌 경우, | ||
| // 즉, receiver가 현재 로그인된 경우, receiver에게 바로 알림을 전송한다 | ||
| if(emitter.isPresent()){ | ||
| System.out.println("emitterId는?" + id); |
There was a problem hiding this comment.
이런 내부 로직들은 메서드로 추출해서 이름을 부여해주면 가독성을 더 높일 수 있어요!
| // emitter가 null인 경우, | ||
| // 즉, receiver가 현재 로그인되지 않은 경우, | ||
| // redisDao에서 SseId에 해당하는 notifications를 찾고 거기에 업데이트 한다 | ||
| else{ |
There was a problem hiding this comment.
else문은 기본적으로 인덴트가 깊어지고 위의 조건을 인지하고 있어야 하기 때문에 가독성을 떨어뜨려요.
위의 if문 안의 로직에서 return으로 플로우를 끊고 아래 로직은 else문 바깥에서 진행하는 게 어떨까요 ?
| .content(content) | ||
| .isRead(false) | ||
| .build(); | ||
| return notification; |
There was a problem hiding this comment.
이 부분에서 버그가 발생해서 테스트 도중 이렇게 작성해서 커밋한것 같습니다.
현재 Notification Entity의 @id 칼럼에 @GeneratedValue(strategy = GenerationType.IDENTITY)라고 설정되어 있는데,
notificationRepository.save(notification)으로
생성된 notification 객체를 저장하려고 할 때, id가 null이라는 문제가 발생하더라고요
제 생각에는 h2 데이터베이스에서 auto increment로 자동으로 생성해줘야 하는게 아닌가 싶었는데, 안되는 것 같아서 문제 해결중입니다
| } | ||
|
|
||
| @Bean | ||
| public RedisTemplate<String, List<Notification>> redisTemplateForNotification() { |
| public abstract class BaseTimeEntity { | ||
|
|
||
| @CreatedDate | ||
| @JsonIgnore |
There was a problem hiding this comment.
@JsonIgnore 는 왜 추가가 됐을까요 ?
There was a problem hiding this comment.
저 부분 때문에 Redis에서 직렬화, 역직렬화 할 때 에러가 발생하더라고요
그래서 추가했습니다!
Uh oh!
There was an error while loading. Please reload this page.