Conversation
- 유저에게 전달할 정보를 나타내는 추상 클래스 AbstractNotificationData 정의 및 추가
- 사용자에게 정보를 보내는 추상 클래스 NotificationSender 클래스 정의 및 추가
- NotificationData 구현체에 맞는 NotificiationSender 구현체를 매핑하고 전송하는 서비스 레이어 클래스 추가
- json 역직렬화시 필요한 기본 생성자 및 setter 추가를 위해 row mapper 클래스 추가
- 메시지 큐의 NotificationData를 소비하는 클래스 추가
| public abstract class AbstractNotificationData { | ||
|
|
||
| private final String notificationMessageContent; | ||
| private final LocalDateTime scheduledAt; | ||
|
|
There was a problem hiding this comment.
사용자에게 정보를 전달한다는 맥락에서 사용되는 추상 메시지인데 사용자와 관련된 정보가 없네요. 어떤 사용자에게 보내는지를 식별할 수 있는 member id를 속성으로 가져도 되지 않을까요?
| public boolean isScheduled() { | ||
| return scheduledAt != null && scheduledAt.isAfter(LocalDateTime.now()); | ||
| } |
There was a problem hiding this comment.
사용자에게 정보를 전달한다의 맥락에 메시지를 언제 보낼 것이다와 같은 맥락이 포함되는 건 단순히 누군가에게 메시지를 전달한다를 표현하는 모델에 구체적 동작이 포함된 구조이지 않나요?
There was a problem hiding this comment.
예약 시간을 필드로 관리하는 ScheduledNotification 클래스를 추가했습니다. 이 클래스는 별도의 테이블로 생성하여 관리하도록 수정했습니다. 또한 주기적으로 폴링하면서 알림 데이터의 예약 시간을 확인한 후 NotificationService에 작업을 위임하는 ScheduledNotificationProcessService를 생성했는데, 이런 방식으로 설계하는 것이 적절할지 궁금합니다!
There was a problem hiding this comment.
그렇게 구현해도 될 것 같고 알림 발송 기능을 사용자에게 정보를 언제 전달할지 결정 -> 어떻게 전달할지 결정 -> 알림 전달 과 같은 플로우로 설계하고 싶다면 스케줄이 가능한 타입을 따로 두어서 해당 타입을 확장하고 있는 구현체면 DB에 저장했다가 주기적으로 폴링하여 전송해도 될 것 같네요.
아니면 좀 더 단순하게 구현하고 싶으면 예약 발송 기능을 일반 알림 발송 기능과 아예 분리해서 구현하는 것도 괜찮을 것 같습니다.
| public void send(AbstractNotificationData notificationData) { | ||
| if(!(notificationData instanceof MailNotificationData mailNotification)) | ||
| throw new ClassCastException("Expected MailNotification, but received: " + notificationData.getClass().getSimpleName()); | ||
|
|
||
| MimeMessage mimeMessage = javaMailSender.createMimeMessage(); | ||
| try { | ||
| MimeMessageHelper mimeMessageHelper = new MimeMessageHelper(mimeMessage, false, "UTF-8"); | ||
| mimeMessageHelper.setTo(mailNotification.getRecipientEmailAddress()); | ||
| mimeMessageHelper.setSubject(mailNotification.getSubject()); | ||
| mimeMessageHelper.setText(mailNotification.getNotificationMessageContent()); | ||
|
|
||
| javaMailSender.send(mimeMessage); | ||
| } catch (MessagingException e) { | ||
| throw new RuntimeException(e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
메시지를 send 하는 로직을 보면 NotificationData의 타입이 Sender가 올바르게 처리할 수 있는 타입이 아니면 예외를 던지고 이후에 구체적인 mail client를 사용해서 메일을 보내는 단계로 이루어져있는데
이때 JavaMailSender가 아닌 Aws의 Ses로 변경된다면? 네이버 email 서비스로 변경된다면? local 환경에서는 실제 메일을 보내는 것이 아닌 로그를 남기는 형태로 모킹을 하고 싶다면?
NotificationData의 타입 체크를 Sender가 하고 실제 클라이언트를 사용해서 메일을 전송하는 책임은 다른 객체에게 위임해도 되지 않을까요?
| private final List<NotificationSender> notificationSenders; | ||
| private final Map<Class<? extends AbstractNotificationData>, NotificationSender> senderMap = new HashMap<>(); | ||
|
|
||
| public NotificationService(List<NotificationSender> notificationSenders) { | ||
| this.notificationSenders = notificationSenders; | ||
| initializeSenderMap(); | ||
| } | ||
|
|
There was a problem hiding this comment.
HashMap과 생성자의 매개변수로 전달받은 List를 그대로 할당하면 어떤 문제가 발생할 수 있을까요? final 선언만으로 불변임을 보장할 수 있을까요?
There was a problem hiding this comment.
final 선언은 재할당을 방지하지만, 참조하고 있는 원본 리스트가 변경될 경우 불변성을 보장하지 못합니다. 이를 해결하기 위해 우선 List.copyOf를 사용했지만 리스트에 담긴 객체가 불변 클래스가 아닌 경우 여전히 문제가 발생할 수 있습니다.
현재 리스트에 인터페이스를 담고 있는데, 전달받은 인터페이스 리스트의 불변성을 확실히 보장하기 위해 별도의 처리 로직이 필요할지 궁급합니다..!
There was a problem hiding this comment.
아뇨 리스트가 담고 있는 Sender는 서비스 성경의 무상태 객체이기 때문에 불변을 굳이 보장해주지 않아도 될 것 같습니다.
다만 자바의 List, Map, Set 같은 컬렉션 타입의 큰 단점 중 하나인데 런타임 시점에 add, remove, put 과 같은 추가, 삭제 행위를 List.copyOf와 같은 코드로 방지할 수 있지만 여전히 타입 자체에 해당 메소드를 사용할 수 있는 문제가 있습니다.
즉, 해당 컬렉션 객체가 추가나 삭제 행위가 불가능하다는 사실을 컴파일타임 수준에서는 알 수 없어요.
이런 문제를 해결하기 위해서 Kotlin에서는 Map, List, Set 타입과 MutableMap, MutableList 타입으로 아예 나눠놨습니다. 추후에 컬렉션 객체가 수정이 가능한지 불가능한지 컴파일타임 수준에서의 보장이 필요하다면 Kotlin을 도입해보는 것도 고려해보면 좋겠네요.
|
|
||
| AbstractNotificationData mailNotification = | ||
| new MailNotificationData(content, null, subject, event.getUserEmail()); | ||
| notificationRepository.save(mailNotification); |
There was a problem hiding this comment.
회원가입 이벤트를 소비할 때 바로 NotificationService에 위임하지 않고 영속화시켰다가 다시 소비할 필요가 있나요?
There was a problem hiding this comment.
현재는 불필요해서 바로 NotificationService에 위임하도록 수정했습니다.
| class StudyRoomServiceTest { | ||
|
|
||
| private final StudyRoomRepository studyRoomRepository = Mockito.mock(StudyRoomRepository.class); | ||
|
|
||
|
|
||
| @Test |
There was a problem hiding this comment.
지금 스터디룸 관련 테스트 코드를 다 삭제하셨는데 이유가 뭘까요?
There was a problem hiding this comment.
StudyRoom 도메인이 더이상 불필요해서 테스트 코드를 삭제했습니다.
There was a problem hiding this comment.
음 이 작업은 엄밀히 따지면 알림 기능 구현과는 다른 맥락인 것 같네요. 그럼 해당 테스트 코드를 삭제하는 작업을 다른 PR로 분리했어도 됐을 것 같습니다.
작업 단위를 작게 정의하여 PR을 올려주면 협업하는 동료들의 인지 부하를 줄일 수 있거든요.
- 수신자를 나타내는 memberId 필드 추가
- 알림을 받는 멤버 아이디라는 의미를 명확히 나타내고자 변경 - 정보를 추상화한 클래스에 scheduledAt은 구체적 맥락이 들어간것이므로 삭제
| public interface NotificationRepository { | ||
| void save(AbstractNotificationData notification); | ||
| } |
There was a problem hiding this comment.
이 코드는 더이상 사용하지 않는 코드인 것 같네요
| public interface MailClient { | ||
| void sendMail(String subject, String content, String receiverEmailAddress); | ||
| } |
There was a problem hiding this comment.
매개변수를 전달할 때 MailNotificationData 를 그대로 전달해도 되지 않을까요? 이러면 필드가 추가될 때마다 해당 타입과 구현체 모두 코드 변경이 필요할 것 같아요
2 participants
1. 알림 기능 정의
알림 기능의 핵심 : 사용자에게 정보를 전달하는 것.
(1) 정보 전달의 추상화:
AbstractNotificationData알림 데이터를 정의하는 추상 클래스입니다.
공통 필드 :
String notificationMessageContent: 알림 메시지의 내용LocalDateTime scheduledAt: 예약 전송 시간추가로 필요한 필드는 각 구현체에서 개별적으로 정의.
(2) 전달 방식의 추상화:
NotificationSender알림을 전송하는 방법을 추상화한 클래스입니다.
주요 메서드 :
public abstract Class<? extends AbstractNotificationData> getSupportedDataTypeClass(): 지원하는 데이터 타입 반환public abstract void send(AbstractNotificationData notificationData): 알림 데이터를 전송2. NotificaionService 설계
NotificationService는 AbstractNotificationData의 구현체를 특정 NotificationSender 구현체와 매핑하여 알림을 전송하는 역할을 담당합니다. 이 과정에서 추상화된 타입만 사용하도록 설계되었습니다.
3. DB와의 동적 매핑
DB에 저장된 AbstractNotificationData를 올바른 구현체로 매핑하기 위한 매핑 클래스를 정의했습니다.
4. 데이터베이스 테이블 설계 :
NOTIFICATION