feat(mail): implement mail reservation #731

woowabrie · 2023-10-05T06:28:03Z

Resolves #715

해결하려는 문제가 무엇인가요?

메일 기능 고도화를 위해 메일 예약 기능을 추가해야 합니다.

어떻게 해결했나요?

MailMessage(메일 내용), MailReservation(예약), MailHistory(전송 이력) 의 객체로 확장하였습니다.
머지 이후 어드민에서 사용할 Service 메서드와 전송 호출에 사용할 Controller 메서드를 작성했습니다.

어떤 부분에 집중하여 리뷰해야 할까요?

이전에 논의한 객체 설계에 맞게 구현이 되어있는지 확인해 주세요. Entity 설정과 연관관계 매핑이 잘 되어 있는지 확인해 주세요.
- 커밋 단위로 보기보다는 최종 커밋을 기준으로 리뷰하는게 더 편할 것 같습니다.
TODO로 남겨둔 부분은 이후 보완하려 합니다.

RCA 룰

r: 꼭 반영해 주세요. 적극적으로 고려해 주세요. (Request changes)
c: 웬만하면 반영해 주세요. (Comment)
a: 반영해도 좋고 넘어가도 좋습니다. 그냥 사소한 의견입니다. (Approve)

- update functionality is replaced with deleting and creating

woowabrie · 2023-10-10T08:33:23Z

최신 develop 기반으로 rebase

woowabrie · 2023-10-10T08:33:28Z

제이슨과의 오프라인 코드 리뷰 후 변경한 사항 정리합니다.

도메인

정적 팩터리 메서드 제거 (463c5d9, 00ee05f)
Mail 객체 간의 객체 참조 -> Id 참조 변경 (3fab767, 2e9e70b)

예약 관련

SendBcc 동기 메서드 만들기 (7804385)
MailSent 이벤트 제거 -> MailHistory 저장은 SendBcc에서 처리 (5c6368a)

컨벤션

도메인의 경우 1파일에 1클래스만 갖도록 변경 (eb4280d)
Controller 메서드의 annotation 순서 맞추기 (9c4e58b)
ReservationStatus의 메서드와 상태의 이름 맞추기 (eeec842)

- update functionality is replaced with deleting and creating

…eference

…id reference

woowabrie · 2023-10-10T08:40:16Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+    fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) {
+        val reservations = mailReservationRepository.findByReservationTimeBetweenAndStatus(
+            standardTime.minusMinutes(1),
+            standardTime.plusMinutes(1),
+            MailReservationStatus.WAITING
+        )
+        val messagesById = findMessageMapById(reservations.map { it.mailMessageId })
+
+        reservations.forEach { mailReservation ->
+            mailReservation.send()
+            mailReservationRepository.save(mailReservation)
+            mailService.sendMailsByBccSynchronous(MailData(messagesById.getValue(mailReservation.id)), emptyMap())
+            mailReservation.finish()
+        }
+    }


회의 시간에 제이슨이 언급하셨던 예약 메일 전송 시 트랜잭션 처리와 관련된 코드입니다.

기존의 메일 전송은 Async로 동작하지만, 이번에는 문제를 단순화하여 동기 방식으로 처리하기로 했어요. (현재 설정 상 초당 300건의 메일 전송이 가능)
설계 시에는 멀티 환경에서의 전송을 생각하고 WATING(대기), SENDING(발송 중), FINISHED(처리 완료) 세 가지의 메일 예약 상태를 가지도록 했는데요.
쓰면서 생각해보니 한 번의 요청으로 하나의 서버에서 동기로 처리를 한다면 중간 상태 변경이 없어도 괜찮지 않나 싶기도 하네요.

혹시 제가 잘못 생각하고 있거나 아니면 더 좋은 동작 아이디어가 있다면 의견 부탁드립니다!

지금은 의미가 없어 보이긴 하네요.. 하지만 비동기로 전환할 것을 염두에 두고 남겨둬도 좋을 것 같아요. 다른 메일 발송 기능은 비동기로 처리하고 있기도 하구요.

kang-hyungu

브리 몇가지 의견 남겼습니다~!
그리고 이번에 AwsMailSender에서 예외 처리와 로그도 남기면 좋을 것 같아요!

kang-hyungu · 2023-10-11T01:21:51Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+        mailMessageRepository.deleteById(mailReservation.mailMessageId)
+    }
+
+    fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) {


r: @ Transactional 을 추가하고 타임아웃을 정해두시죠!

kang-hyungu · 2023-10-11T01:22:42Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+
+    fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) {
+        val reservations = mailReservationRepository.findByReservationTimeBetweenAndStatus(
+            standardTime.minusMinutes(1),


1분 범위에 예약된 메일을 모두 발송하기 위해 1분 추가, 빼기를 한게 맞을까요?

네 맞습니다! 트리거가 정확하게 시작하는걸 보장하기 어렵기도 하고, 메일 예약이 15분 단위로 설정되니 다른 시간의 예약을 조회할일은 없어 앞뒤로 1분을 넣었어요.

r: 1시 10분이 예약되어 있다고 했을 때 1시 9분, 1시 10분, 1시 11분에 요청이 안오면 해당 메일은 발송을 못하는 케이스가 존재할 것 같은데요.
standardTime보다 이전에 발송되어야 할 메시지 중 상태가 WAITING인 애들을 조회해서 발송하면 문제가 없어질 것 같은데 어떻게 생각하시나용

오 그렇네요. 아마 이전에 코드를 짤 때 예약시간 수정을 생각하다 보니 사이값을 검색하도록 만들었던 것 같아요.
현재 로직은 예약 시간/내용 수정 대신 기존 예약을 삭제 후 재생성하도록 하고있어서, 말씀주신 것처럼 예약 시간이 현재보다 이전인 대기 상태의 모든 예약을 조회하는게 더 좋을 것 같습니다!

kang-hyungu · 2023-10-11T01:26:31Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+
+        reservations.forEach { mailReservation ->
+            mailReservation.send()
+            mailReservationRepository.save(mailReservation)


c: 메일 예약을 개별 저장하는 것보다 한 번에 처리하는건 어떨까요
배치 사이즈를 지정했다면 더 효율적으로 처리할 수 있을 것 같아요

mailReservationRepository.saveAll(reservations)

kang-hyungu · 2023-10-11T01:38:43Z

src/main/kotlin/apply/application/mail/MailService.kt

+        saveMailHistories(mailMessage.id, succeeded, failed)
+    }
+
+    fun sendMailsByBccSynchronous(request: MailData, files: Map<String, ByteArrayResource>) {


단건으로 보내는 거라 s는 빼는게 맞겠네요.

Suggested change

fun sendMailsByBccSynchronous(request: MailData, files: Map<String, ByteArrayResource>) {

fun sendMailByBccSynchronous(request: MailData, files: Map<String, ByteArrayResource>) {

files도 MailData에 합치는게 좋을 것 같은데 스프링 리소스를 쓰고 있어서 따로 분리했나보네요. 저는 인터페이스 추가해서 하나로 합치는게 나을 것 같은데 다른 분들 어떠신가요. 따로 다루는게 나을까요?

첨부 파일이 없는 경우가 많을거라 기본값으로 emptyMap()을 설정하는게 좋을 것 같습니다.

이번에는 메일 예약에서 파일 첨부 기능을 지원하지 않기로 해서 크게 신경쓰지는 않았는데, 장기적인 관점으로는 구구 말씀처럼 합치는 방향으로 가는게 좋아보여요 ㅎㅎ

kang-hyungu · 2023-10-11T01:50:51Z

src/main/kotlin/apply/application/mail/MailService.kt

@@ -24,11 +26,13 @@ private const val MAIL_SENDING_UNIT: Int = 50
 class MailService(


c: MailMessageService가 있고 MailService가 있으니까 헷갈리네요;;;
MailService는 실제 메일 발송과 관련된 작업만 존재하니 SedingMailService 같은 이름으로 바꾸는건 어떨까요?

저도 만들면서 메일 관련 서비스들이 헷갈렸습니다... (MailService, MailHistoryService, MailMessageService)
현재 MailService의 역할에 공감해서 좋은 네이밍 인 것 같아 반영하겠습니다!
MailHistoryService는 현재 어드민에서 사용중이라 이후 어드민 작업 하면서 필요성을 보고 함께 정리할게요.

kang-hyungu · 2023-10-11T01:52:12Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+        mailMessageRepository.deleteById(mailReservation.mailMessageId)
+    }
+
+    fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) {


c: 예약된 메일들을 보내는거라 s 붙이는게 좋겠네요

Suggested change

fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) {

fun sendReservedMails(standardTime: LocalDateTime = LocalDateTime.now()) {

kang-hyungu · 2023-10-11T02:13:52Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+            standardTime.plusMinutes(1),
+            MailReservationStatus.WAITING
+        )
+        val messagesById = findMessageMapById(reservations.map { it.mailMessageId })


MailReservation이랑 MailMessage를 한 번에 불러오면 쿼리 호출 횟수도 줄이고 코드 복잡도도 줄어들거 같아요
아래 예시 쿼리로 작성했어요. JPQL 문법이 맞는지 확인이 필요합니다 ㅎㅎ;

@Query("select m" + " from MailMessage m" + " join MailReservation ms on ms.mailMessageId = m.id" + " where ms.reservationTime between :from and :to" + " and ms.status = :status") fun findByReservationTimeBetweenAndStatus( from: LocalDateTime, to: LocalDateTime, status: MailReservationStatus ): List<MailMessage>

오 이렇게하면 바로 예약 MailMessage 를 가져올 수 있군요! 넘 좋습니다 ㅎㅎㅎ
한 가지 궁금한게 이 때 조회한 MailReservation도 함께 받을 수 있나요? 메일 전송만 생각하면 MailMessage만 있어도 되는데, 상태 변경이 필요해서요!

쿼리에서 select에 ms를 추가하면 될거에요. 그런데 MailMessage가 MailReservation를 간접참조 하고 있으면 좀 더 수정해야 해요. dto를 새로 만들거나 객체를 참조하도록 바꾸는 방법이 있겠네요

kang-hyungu · 2023-10-11T02:17:36Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+        reservations.forEach { mailReservation ->
+            mailReservation.send()
+            mailReservationRepository.save(mailReservation)
+            mailService.sendMailsByBccSynchronous(MailData(messagesById.getValue(mailReservation.id)), emptyMap())


c: MailService를 인프라용 객체가 아닌 도메인용으로 본다면 MailMessage 객체를 전달하는게 나을 것 같아요.

val mailMessage = messagesById.getValue(mailReservation.id) mailService.sendMailsByBccSynchronous(mailMessage)

어드민에서 직접 사용하는 비동기 방식의 메일 전송 메서드와 맞추긴 했는데, 저도 고민되긴 하더라구요 ㅋㅋ
MailMessage를 만들테고 실제로 전송할 때 MailService(SendingMailService) 를 사용한다면, MailMessage를 전달하는게 좋아보이긴 해요.
이건 변경 여파가 어드민에 미쳐서 작업을 안했었는데, 이부분도 이후에 어드민 작업과 함께 정리해볼게요.

kang-hyungu · 2023-10-11T02:18:43Z

src/main/kotlin/apply/domain/mail/MailReservation.kt

+
+    @Column(nullable = false)
+    val reservationTime: LocalDateTime,
+    id: Long = 0L


a: id를 하단에 위치시키니 mailMessageId랑 헷갈리네요. 아이디 생략하고 기본값으로 0을 설정하려고 마지막에 배치한걸까요?

이건 다른 엔티티들을 보니 컨벤션으로 보여 맞추긴 했습니다. ㅎㅎ 의도는 @woowahan-pjs 에게 토스,,,

kang-hyungu · 2023-10-11T02:20:22Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+    fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) {
+        val reservations = mailReservationRepository.findByReservationTimeBetweenAndStatus(
+            standardTime.minusMinutes(1),
+            standardTime.plusMinutes(1),
+            MailReservationStatus.WAITING
+        )
+        val messagesById = findMessageMapById(reservations.map { it.mailMessageId })
+
+        reservations.forEach { mailReservation ->
+            mailReservation.send()
+            mailReservationRepository.save(mailReservation)
+            mailService.sendMailsByBccSynchronous(MailData(messagesById.getValue(mailReservation.id)), emptyMap())
+            mailReservation.finish()
+        }
+    }


지금은 의미가 없어 보이긴 하네요.. 하지만 비동기로 전환할 것을 염두에 두고 남겨둬도 좋을 것 같아요. 다른 메일 발송 기능은 비동기로 처리하고 있기도 하구요.

woowahan-neo

고생 많으셨습니다 :)

woowahan-neo · 2023-10-12T04:26:28Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+        )
+        val messagesById = findMessageMapById(reservations.map { it.mailMessageId })
+
+        reservations.forEach { mailReservation ->


c: 해당 메일 발송 작업이 하나의 트랜잭션으로 묶여야 할 이유가 있을까요? 개별적으로 성공 실패가 나뉘는게 더 자연스럽지 않을까 합니다.

woowahan-neo · 2023-10-12T04:29:45Z

src/main/kotlin/apply/ui/api/MailRestController.kt

+
+    @PostMapping("/reserved")
+    fun sendMail(
+        @Accessor("lambda") ignored: Unit


r: 채점을 위한 token이 변경되면 같이 변경이 되야 하는 문제가 있을 것 같은데, 동일한 token으로 가는 것 보다 다른 토큰으로 분리가 되면 어떨까요?

몰래 같이 쓰려다 걸렸네요... 🙄
mail-scheduler 로 분리했어요 ㅋㅋㅋ

woowahan-neo · 2023-10-12T04:31:39Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+        val messagesById = findMessageMapById(reservations.map { it.mailMessageId })
+
+        reservations.forEach { mailReservation ->
+            mailReservation.send()


질문) 지금 트랜잭션으로 묶여 있는데 MailReservation.status가 SENDING로 DB에 존재하는 케이스가 생길 수 있나요?

woowahan-neo · 2023-10-12T04:34:33Z

src/main/kotlin/apply/application/mail/MailMessageService.kt

+
+    fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) {
+        val reservations = mailReservationRepository.findByReservationTimeBetweenAndStatus(
+            standardTime.minusMinutes(1),


r: 1시 10분이 예약되어 있다고 했을 때 1시 9분, 1시 10분, 1시 11분에 요청이 안오면 해당 메일은 발송을 못하는 케이스가 존재할 것 같은데요.
standardTime보다 이전에 발송되어야 할 메시지 중 상태가 WAITING인 애들을 조회해서 발송하면 문제가 없어질 것 같은데 어떻게 생각하시나용

woowabrie · 2023-10-19T00:46:45Z

@kang-hyungu @woowahan-neo @woowahan-pjs
세분 모두 꼼꼼하게 리뷰해 주셔서 감사합니다. 🙏
제안주신 사항들 코드에 반영했으니 마지막으로 검토 부탁드려요.

그리고 (말도 많고 탈도 많던...) 예약 메시지 발송 관련 로직은 트랜잭션 걸지 않게 변경했어요. (0fe7829)

MailReservationStatus.SENDING 은 멀티 환경에서의 중복 전송을 고려한 값
- => 현재 구조에서는 처리 시 해당 상태로 변경할 필요가 없음
MailReservationStatus.FINISHED 는 발송 시도 했음을 나타내며 발송 결과(성공/실패) 여부와는 상관 없음. (성공/실패 여부는 History에 반영)
- => 서로다른 메일 예약이 하나의 트랜잭션으로 묶일 필요 없음

어제 프리코스 안내 메일 처리 속도를 보니 메일 전송 요청을 비동기로 변경해도 괜찮을 것� 같은데, 이건 AwsMailSender에 RateLimit이 걸려있어서 실제 요청 수치들을 더 확인하고 변경하는게 좋아보여 이번에 반영하지 않았습니다.

woowabrie self-assigned this Oct 5, 2023

woowabrie marked this pull request as ready for review October 5, 2023 07:00

woowabrie requested a review from woowahan-pjs October 5, 2023 07:00

woowahan-pjs force-pushed the develop branch from 0f0de6c to 57b8114 Compare October 9, 2023 12:00

woowabrie requested review from kang-hyungu and woowahan-neo October 10, 2023 08:24

woowabrie added 17 commits October 10, 2023 17:28

feat: add MailMessage

f89b43c

feat: add MailReservation

559f00a

feat: add MailHistory

faa24f5

feat: mapping relationships with mail message and reservation

81f6188

feat: mapping relationships with mail message and history

ad730d6

feat: using new MailHistory

742c8a6

refactor: delete old MailHistory

1d6032b

feat: Add functionality for scheduling email reservations

d09c01b

feat: add functionality for updating mail messages

b8168e6

feat: add functionality for updating mail's reservation time

f12edfa

feat: add functionality for deleting mail's reservation

f5777a4

feat: add functionality for finding mail's reservation

9e29d51

refactor: create MailHistory when MailSentEvent is published

501928e

feat: add endpoint to send reservation mail

e914ed2

refactor: migrate sql relating with mail tables

f766d5d

fix: edit searching conditions for mail reservations

3b6cd96

feat: remove functions relates with updating mail message

ba9d7b0

- update functionality is replaced with deleting and creating

woowabrie force-pushed the feature/mail-reservation branch from dd76881 to 8bb541f Compare October 10, 2023 08:31

woowabrie added 4 commits October 10, 2023 17:38

feat: remove functions relates with updating reservationTime of mail

16e0a74

- update functionality is replaced with deleting and creating

refactor: extract MailReservationStatus into another file

e2d7e91

refactor: adjust order of annotation on MailReservationController

0a673d4

refactor: rename functions changing status of reservation

ee17685

woowabrie added 11 commits October 10, 2023 17:38

refactor: delete factory method from MailHistory

1d11f1b

test: edit typo for creatorId of MailMessage

2c3341f

refactor: delete factory method from MailMessage

f0257c5

refactor: make relationships with MailHistory and MailMessage by id r…

fb64635

…eference

refactor: make relationships with MailReservation and MailMessage by …

237ba8c

…id reference

refactor: add synchronous function to send mail

4bf23cb

refactor: delete MailSentEvent and Listener

19cee58

test: add test for sending reservation mail

8bf6500

refactor: extract functions to get reservation time

f2760b4

refactor: move mails functions into MailMessageService

49f91f6

feat: use lambda accessor to authenticate API caller

6f23fa7

woowabrie force-pushed the feature/mail-reservation branch from 8bb541f to 6f23fa7 Compare October 10, 2023 08:38

woowabrie commented Oct 10, 2023

View reviewed changes

kang-hyungu requested changes Oct 11, 2023

View reviewed changes

woowabrie added 2 commits October 11, 2023 15:46

fix: use mailMessageId for getting MailMessage Information

7cdbeda

refactor: rename MailService to SendingMailService

3e96c58

woowahan-neo requested changes Oct 12, 2023

View reviewed changes

woowabrie added 5 commits October 12, 2023 14:27

refactor: edit searching condition for MailReservation

4ba0c6f

refactor: add accessor key for mail scheduler

1c21188

refactor: edit name of function that send mail

56a5115

refactor: edit name of function that send reservation mails

5c5c180

refactor: edit name of function that send mail

dd427ec

woowabrie force-pushed the feature/mail-reservation branch from e77a87d to 9f0e2a2 Compare October 18, 2023 23:49

refactor: change function to send reserved mail

0fe7829

woowabrie force-pushed the feature/mail-reservation branch from 9f0e2a2 to 0fe7829 Compare October 19, 2023 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mail): implement mail reservation #731

feat(mail): implement mail reservation #731

woowabrie commented Oct 5, 2023 •

edited

Loading

woowabrie commented Oct 10, 2023

woowabrie commented Oct 10, 2023

woowabrie Oct 10, 2023

kang-hyungu Oct 11, 2023

kang-hyungu left a comment

kang-hyungu Oct 11, 2023

kang-hyungu Oct 11, 2023

woowabrie Oct 11, 2023

woowahan-neo Oct 12, 2023

woowabrie Oct 12, 2023

kang-hyungu Oct 11, 2023

kang-hyungu Oct 11, 2023

woowabrie Oct 11, 2023

kang-hyungu Oct 11, 2023

woowabrie Oct 11, 2023

kang-hyungu Oct 11, 2023

kang-hyungu Oct 11, 2023

woowabrie Oct 18, 2023

kang-hyungu Oct 19, 2023

kang-hyungu Oct 11, 2023

woowabrie Oct 11, 2023

kang-hyungu Oct 11, 2023

woowabrie Oct 11, 2023

kang-hyungu Oct 11, 2023

woowahan-neo left a comment

woowahan-neo Oct 12, 2023

woowahan-neo Oct 12, 2023

woowabrie Oct 12, 2023

woowahan-neo Oct 12, 2023

woowahan-neo Oct 12, 2023

woowabrie commented Oct 19, 2023

	fun sendMailsByBccSynchronous(request: MailData, files: Map<String, ByteArrayResource>) {
	fun sendMailByBccSynchronous(request: MailData, files: Map<String, ByteArrayResource>) {

		@@ -24,11 +26,13 @@ private const val MAIL_SENDING_UNIT: Int = 50
		class MailService(

	fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) {
	fun sendReservedMails(standardTime: LocalDateTime = LocalDateTime.now()) {

feat(mail): implement mail reservation #731

Are you sure you want to change the base?

feat(mail): implement mail reservation #731

Conversation

woowabrie commented Oct 5, 2023 • edited Loading

해결하려는 문제가 무엇인가요?

어떻게 해결했나요?

어떤 부분에 집중하여 리뷰해야 할까요?

RCA 룰

woowabrie commented Oct 10, 2023

woowabrie commented Oct 10, 2023

도메인

예약 관련

컨벤션

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kang-hyungu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowahan-neo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woowabrie commented Oct 19, 2023

woowabrie commented Oct 5, 2023 •

edited

Loading