-
Notifications
You must be signed in to change notification settings - Fork 102
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
feat(mail): implement sending mail #352
Conversation
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.
마갸 정말 고생많으셨습니다 👍
메일 테스트해보니 정상적으로 잘 보내집니다 👍👍👍
비동기 처리에 첨부파일도 고려해야하고 고려할 요소가 많았을텐데 잘 구현해주신 것 같아요 🥲
단지 중간에 옥에 티(java 클래스)가 보이는 것 같네요 👀
그리고 한 가지 궁금한 점이 현재 AWS SES
가 아닌 JavaMailSender
로 기능을 구현하셨다고 하셨는데, 현재 이슈는 지금 처럼 구현하면 끝인건가요!? 아니면 AWS SES
를 사용하도록 변경해야 해당 이슈가 끝나는 걸까요!? (끝인지 아닌지 몰라서 approve는 한번 미루겠습니다 🤔)
interface MailSender { | ||
fun send(toAddress: String, subject: String, body: String) | ||
|
||
fun sendBCC(toAddresses: Array<String>, subject: String, body: String, files: List<Pair<String, ByteArrayResource>>) |
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.
마갸 여기서 BCC가 B.C.C (Blind carbon copy)
로 숨은 참조 의미하는게 맞을까요? 🤔
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.
넵 해당 기능을 말하는게 맞습니다. Github에 돌아다니는 코드를 많이 참고해보니 축약어로 쓰이는 경우가 더 많더라구요
|
||
@Async | ||
fun sendMailsByBCC(request: MailSendData, files: List<Pair<String, ByteArrayResource>>) { | ||
for (targetMailsPart in request.targetMails.chunked(MAIL_SENDING_UNIT)) { |
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.
동시에 50명의 인원까지 묶어서 전송하며, 50명 이상의 인원이 존재할 경우 50명 단위로 전송합니다.
의 의미가 여기서부터 시작되는 거군요? chunked
로 50명씩 나눠서 snedMailsByBCC를 수행하는거군요.... 오.... 😧
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.
넵. 사실 메일을 보내는 자체에는 제한이 없지만 AWS 스펙상50개로 제한을 두고 있어 그 내용을 반영했습니다
package apply.config; | ||
|
||
import org.springframework.context.annotation.Bean; | ||
import org.springframework.web.multipart.commons.CommonsMultipartResolver; | ||
|
||
public class MailConfig { | ||
private final int MAX_FILE_SIZE = 1024 * 1024 * 10; | ||
|
||
@Bean(name = "multipartResolver") | ||
public CommonsMultipartResolver multipartResolver() { | ||
CommonsMultipartResolver multipartResolver = new CommonsMultipartResolver(); | ||
multipartResolver.setMaxUploadSize(MAX_FILE_SIZE); | ||
return multipartResolver; | ||
} | ||
|
||
} |
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.
갑자기 친숙한 Java 냄새가...? 👀
package apply.config; | |
import org.springframework.context.annotation.Bean; | |
import org.springframework.web.multipart.commons.CommonsMultipartResolver; | |
public class MailConfig { | |
private final int MAX_FILE_SIZE = 1024 * 1024 * 10; | |
@Bean(name = "multipartResolver") | |
public CommonsMultipartResolver multipartResolver() { | |
CommonsMultipartResolver multipartResolver = new CommonsMultipartResolver(); | |
multipartResolver.setMaxUploadSize(MAX_FILE_SIZE); | |
return multipartResolver; | |
} | |
} | |
package apply.config | |
import org.springframework.context.annotation.Bean | |
import org.springframework.context.annotation.Configuration | |
import org.springframework.web.multipart.commons.CommonsMultipartResolver | |
@Configuration | |
class MailConfig { | |
@Bean | |
fun multipartResolver(): CommonsMultipartResolver? { | |
val multipartResolver = CommonsMultipartResolver() | |
multipartResolver.setMaxUploadSize(MAX_FILE_SIZE.toLong()) | |
return multipartResolver | |
} | |
companion object { | |
private const val MAX_FILE_SIZE = 1024 * 1024 * 10 | |
} | |
} | |
위와 같이 갓틀린 클래스로 변경하면 어떨까요 ?? 😃
추가적으로 Bean 이름을 직접 multipartResolver
라고 지정해줄 필요가 없을 것 같아요 😃 (설정하신 이유가 있다면 코틀린 코드에서도 그대로 둬도 될 것 같아요)
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.
샘플 코드로 가져왔는데 kotlin으로 바꾸는 걸 깜빡했네요. 이런 큰 실수를...
bean
과 관련해서는 샘플 코드에 명시가 되어 있어서 따라 했는데 기본 설정 자체가 메소드 이름을 따라가기 때문에 굳이 의미가 없기는 하네요. 자동으로 지정하는 방식으로 반영하도록 하겠습니다.
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.
사실 해당 클래스가 하는 역할을 10MB로 파일 사이즈를 제한하는 기능밖에 없어요. 곰곰히 생각해보니 그정도 기능을 따로 설정으로 뺄 필요는 없을 것 같더라구요. 설정 파일을 아예 삭제했습니다
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.
마갸 !!!
작업하신 내용 확인하고 코멘트 남겨보았어요 ㅎㅎ
까다로운 부분이 많았을텐데 메일 고수 답게 깔끔하게 잘 구현해주셨네요 !! 짱짱
메일 부분을 다룬적이 많이 없었는데 코드보면서 이렇게 하는거구나하고 많이 배웠습니다 👏👏
정말 수고 많으셨어요 🔥
아 pr에 적어주신 내용따라 테스트도 해봤는데, 잘 전송됩니다 !!
정리 감사해요 ~
) { | ||
val MAIL_SENDING_UNIT = 50 |
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.
상수는 companion object로 빼는게 어떨까요 ? ㅎㅎ
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.
상수로 빼지 않으면 컨벤션 위반입니다. 그리고 상수로 빼는 것이 더 적절해 보여요.
https://developer.android.com/kotlin/style-guide#constant_names
data class MailSendData( | ||
@field:NotNull | ||
val subject: String, | ||
@field:NotNull | ||
val content: String, |
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.
@NotNull
은 null만 검증해주니, String 타입은 @NotBlank
로 null, 빈 공백 문자열을 막아주는게 어떨까요 ?
그리고 컨트롤러에서 @Valid
어노테이션을 걸어주지 않아서 api로 이 요청을 보내면 해당 dto 검증을 거치지 않고 있네요.
엇 근데 지금 보니까 전반적으로 validation을 걸어준 dto를 쓰는 컨트롤러가 다 @Valid
가 걸려있지 않군요... ㅎㅎㅎㅎ
지금은 바딘에서 바로 서비스를 호출하지만 api들을 쓸 때는 컨트롤러에 꼭 어노테이션을 붙여주어야겠네요 !
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.
실제 validate
하기 위해 걸어 준 부분은 아니었는데 혼동이 오는 부부인 겉 같아요. 아직 예외 정책을 정하지 않았기 때문에 오히려 다 뺴는게 나을 수도 있을 것 같아요
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.
구현된 기능이 아니라서 혼동을 줄 수 있을 것 같네요. 아예 어노테이션을 삭제해 두었습니다!
val inputStreamFiles = | ||
files.map { Pair(it.originalFilename!!, ByteArrayResource(IOUtils.toByteArray(it.inputStream))) } |
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.
val inputStreamFiles = | |
files.map { Pair(it.originalFilename!!, ByteArrayResource(IOUtils.toByteArray(it.inputStream))) } | |
val inputStreamFiles = | |
files.associate { it.originalFilename!! to ByteArrayResource(IOUtils.toByteArray(it.inputStream)) } |
inputStreamFiles의 타입을 List<Pair<String, ByteArrayResource>>
대신 Map<String, ByteArrayResource>
으로 사용하는 것은 어떨까요 ?
(이렇게 되면 이 변수를 받는 메서드들의 파라미터 타입도 조금 변경되겠네요)
list안에 pair가 들어가서 조금 복잡해보이기도 하고, 저렇게 만들어진 map 안에서도 결국 pair를 쓰고 있으니
map 타입을 쓰는 것도 괜찮아보여요 ㅎㅎ
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.
"이름" 과 "내부 데이터" 를 연결시켜 주려고 했는데 이번 미션도 그렇고 map 을 쓰는게 보편적인 것 같더라구요. mail 에 파일 데이터를 쓰는 과정에서 foreach
를 쓰기 위해서 pair를 사용했는데 map 타입으로도 keyset
을 사용하지 않고 사용할 수 있더라구요
@PostMapping | ||
fun sendMail( | ||
@RequestPart(value = "request") request: MailSendData, | ||
@RequestPart(value = "files") files: Array<MultipartFile>, | ||
): ResponseEntity<Unit> { |
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.
@PostMapping | |
fun sendMail( | |
@RequestPart(value = "request") request: MailSendData, | |
@RequestPart(value = "files") files: Array<MultipartFile>, | |
): ResponseEntity<Unit> { | |
@PostMapping | |
fun sendMail( | |
@RequestPart request: MailSendData, | |
@RequestPart files: Array<MultipartFile>, | |
): ResponseEntity<Unit> { |
굳굳 !!
요청 키값과 변수명이 같다면 value를 생략해도 좋을 것 같아요.
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.
확인했습니다. RequestPart
부분을 처음 써서 명시해줬는데 없어도 상관없다면 지워줘도 되겠네요
val subject: String, | ||
@field:NotNull | ||
val content: String, | ||
@field:NotNull |
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.
리스트의 경우 NotEmpty가 되어야 하지 않을까 생각해 봅니다~ㅎㅎㅎ
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.
어노테이션을 모두 삭제했습니다! 어노테이션이 적용되고 있지 않은 상태였습니다
@@ -14,8 +16,10 @@ import org.thymeleaf.spring5.ISpringTemplateEngine | |||
class MailService( | |||
private val applicationProperties: ApplicationProperties, | |||
private val templateEngine: ISpringTemplateEngine, | |||
private val mailSender: MailSender | |||
private val mailSender: SimpleMailSender |
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.
AwsMailSender 의 @Component
를 제거하고 MailSender로 받아도 되지 않을까요?ㅎㅎㅎ
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.
제가 왜 여기를 SimpleMailSender
로 바꿨을까요? 과거의 제가 무슨짓을...
2021/09/28 3:43 1차 반영 모두 완료했습니다. |
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.
코드도 깔끔하고 메일도 잘 보내지는 것 확인했습니다👍
간단한 코멘트만 하나 남겼습니다~~
val subject: String, | ||
@field:NotNull | ||
val content: String, |
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.
데이터 클래스의 변수들을 바딘에서 사용하려면 변수타입이 var이어야 합니다~
변수타입을 바꿔야 로키의 pr과 충돌을 피할 수 있을 것 같아요 😊
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.
Java 클래스만 사라지면 당장 approve해도 이상이 없었는데, 제가 Request Change 드렸던 걸 깜빡했네요 🙄
마갸 정말 고생많으셨습니다 🙏
조금 궁금한 점은 이전의 MailConfig.java
역할을 하는 설정이나 클래스 파일이 안보이는 것 같은데 혹시 어디에 위치하는걸까요? 🤔
제 작업이 머지된 이후에 이어서 작업하실 것들이 있는 것으로 알고있으니 approve는 했지만 다시 찾아와서 코멘트 남기겠습니다 😃
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.
AwsMailSender
를 이어 구현하면 됩니다.
@@ -0,0 +1,7 @@ | |||
package apply.application.mail | |||
|
|||
data class MailSendData( |
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.
이제 아래 클래스를 사용하면 됩니다. 유효성 검사 애너테이션은 Vaadin에 필요하니 삭제하면 안 됩니다.
data class MailData(
@field:NotEmpty
var subject: String = "",
@field:NotEmpty
var body: String = "",
@field:NotEmpty
var recipients: List<String> = emptyList()
)
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.
확인했습니다
interface MailSender { | ||
fun send(toAddress: String, subject: String, body: String) | ||
|
||
fun sendBCC(toAddresses: Array<String>, subject: String, body: String, files: Map<String, ByteArrayResource>) |
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.
단체 메일은 숨은 참조가 기본이므로 send()
또는 sendBcc()
중 하나를 선택하면 됩니다.
https://developer.android.com/kotlin/style-guide?hl=ko#camel_case
fun sendBCC(toAddresses: Array<String>, subject: String, body: String, files: Map<String, ByteArrayResource>) | |
fun sendBcc(toAddresses: Array<String>, subject: String, body: String, files: Map<String, ByteArrayResource>) |
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.
일반적인 send와 MIME
타입의 String
데이터로 구성된 데이터를 send 로 쏠 수 있도록 오버로딩 할 수도 있고
snedBcc
를 통해서 bcc라는걸 명시해줄수도 있겠네요
숨은 참조가 기본이긴 해도 좀 더 정확히 명시하는게 더 좋아보입니다. 우선은 sendBcc로 분리하도록 할게요
|
2차로 구현했습니다! 테스트를 위한 환경은 아래와 같이 설정해주면 됩니다.
현재 남은 작업 목록
AWS SES 설정을 위해 참고한 문서는 아래와 같습니다 |
추가 기능 구현
버그
|
0d50365
to
b8aaf96
Compare
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.
마갸 이메일 구현하시느라 고생이 많으셨습니다 👑 다 구현된 내용을 이해하는데도 쉽지 않네요. 대부분 스타일과 관련된 코멘트이긴 한데 몇자 남겨보았습니다.
추가적으로 남겼었는데, 인텔리제이에서 누락되었네요..ㅠㅠ
- 프론트 측에서 템플릿 수정으로 email, title 은 코드에서 삭제되도 될까요?
@@ -36,6 +37,7 @@ class MailForm( | |||
private val subject: TextField = TextField("제목").apply { setWidthFull() } | |||
private val body: TextArea = createBody() | |||
private val mailTargets: MutableSet<MailTargetResponse> = mutableSetOf() | |||
private val uploadFile: MutableMap<String, ByteArrayResource> = LinkedHashMap() |
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.
자바 컬렉션이 아닌 코틀린의 mutableMapOf()
를 사용하는 건 어떨까요?
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.
LinkedHashMap 을 한 단계 포장한게 MutableMap 로 사용되고 있는데 순서 유지가 필요하다고 명시를 해주고 싶어 해당 방식대로 사용하였습니다. 둘 다 코틀린 컬렉션입니다.
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.
@SinceKotlin("1.1") public actual typealias LinkedHashMap<K, V> = java.util.LinkedHashMap<K, V>
이렇게 타고 들어가져서 이것은 결국 자바 패키지로 가지는데 혹시 제가 잘 못 알고 있는 걸까요??
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.
mutableMapOf()
를 들어가보시면 public inline fun <K, V> mutableMapOf(): MutableMap<K, V> = LinkedHashMap()
이와 같이 선언되어 있습니다. mutableMapOf 자체가 내부적으로 LinkedHashMap을 구현하는 방식입니다. 어색함을 느끼시는 것 같아 변경했습니다
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.
네 감사합니다. 하지만 저 부분에서 public inline fun <K, V> mutableMapOf(): MutableMap<K, V> = LinkedHashMap()
이 부분에서 LinkedHashMap()
을 타고 들어가면 코틀린 컬렉션이 나오기 때문에 다른 것이라고 생각했습니다. 혹시 잘못된 정보라면 알려주세요.
|
||
@Async | ||
fun sendMailsByBCC(request: MailData, files: Map<String, ByteArrayResource>) { | ||
request.recipients.plus(mailProperties.username) |
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.
request.recipients + mailProperties.username
를 사용해도 좋을 것 같아요. 오히려 plus를 사용하는게 읽기 좋다고 느껴지기도 해서 참고만 해주세요 :)
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.
저는 이 부분은 plus
로 두고싶네요. 굳이 한 값을 올리는 데 recipients
라는 그룹을 명시해주기에는 어색하다고 느낍니다
val inputStreamFiles = | ||
files.associate { (it.originalFilename!! to ByteArrayResource(it.bytes)) } |
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.
이 프로젝트에서 주로 =
뒤에 줄바꿈보단 인자를 기준으로 줄바꿈을 하더라고요.
val inputStreamFiles = files.associate {
(it.originalFilename!! to ByteArrayResource(IOUtils.toByteArray(it.inputStream)))
}
이런 방식은 어떨까 의견 내봅니다.
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.
IOUtlis 가 사라지고 bytes로 처리했습니다
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를 드렸던터라 의견 comment로 추가했습니다)
마갸 고생 많으셨습니다.
Data 클래스랑 상수쪽 코멘트 남겼는데, 확인해주시고 괜찮은 것 같다면 반영해주세요 🤗
@field:NotNull | ||
var id: Long = 0L | ||
) { | ||
|
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.
변경했습니다
|
||
var attachFiles: Map<String, ByteArrayResource> = emptyMap(), | ||
|
||
@field:NotNull | ||
var id: Long = 0L |
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.
TermSelectData
에서는 필드 파라미터 배치를 이렇게 두고 있더라구요!
id에 NotNull Validation을 붙인 이유가 있을까요!? 다른 xxxData 클래스들은 id에 별다른 Validation을 안붙였더라구요 🤔
var attachFiles: Map<String, ByteArrayResource> = emptyMap(), | |
@field:NotNull | |
var id: Long = 0L | |
var attachFiles: Map<String, ByteArrayResource> = emptyMap(), | |
var id: Long = 0L |
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.
음... 제 파트에서 검증 로직을 붙인 건 아니라서 정확히는 모르겠지만 mail history
관련해서 사용하고 있을거에요
ex.printStackTrace() | ||
} | ||
} | ||
|
||
private fun createContent(data: String): Content { | ||
return Content(data).withCharset("UTF-8") |
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.
return Content(data).withCharset("UTF-8") | |
return Content(data).withCharset(Charsets.UTF_8.name()) |
코틀린에서 제공하고있는 상수를 활용해보면 어떨까요? 🤔
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.
변경했습니다
|
||
@Async | ||
fun sendMailsByBCC(request: MailData, files: Map<String, ByteArrayResource>) { | ||
request.recipients.plus(mailProperties.username) |
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.
수고 많으셨습니다 !!!
낯선 부분이라 코드 이해하는것도 꽤 걸렸는데,
이걸 구현하신 마갸는... 진짜 수고 많으셨습니다 👏
확인하고 몇개 남겨보았는데 가볍게 참고만 부탁드려요~
fun message(lambda: MultipartMimeMessageBuilder.() -> Unit): MultipartMimeMessageBuilder { | ||
return MultipartMimeMessageBuilder().apply(lambda) | ||
} |
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.
fun message(lambda: MultipartMimeMessageBuilder.() -> Unit): MultipartMimeMessageBuilder { | |
return MultipartMimeMessageBuilder().apply(lambda) | |
} | |
fun message(lambda: MultipartMimeMessageBuilder.() -> Unit): MultipartMimeMessage { | |
return MultipartMimeMessageBuilder().apply(lambda).build() | |
} |
와우 dsl 스타일 👏👏
혹시 이렇게 MultipartMimeMessage를 넘기는 것은 어떨까요 ???
그러면 해당 함수를 쓰는 곳에서 build를 생략할 수 있어 깔끔할 거같다는 생각이 드는데
그냥 가볍게 참고만 해주세요 ㅎㅎㅎㅎ
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.
build()
를 붙여주는게 빌더패턴을 썻다고 명시할 수 있어서 이렇게 구현했는데 확실히 dsl 스타일이니 굳이 필요하진 않아보이네요. 변경했습니다.
@@ -125,6 +124,7 @@ class MailForm( | |||
override fun bindOrNull(): MailData? { | |||
return bindDefaultOrNull()?.apply { | |||
recipients = mailTargets.map { it.email }.toList() | |||
attachFiles = uploadFile |
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.
MailForm의 uploadFile 변수명을 attachFiles로 통일해 해당 코드를 생략하는 방법은 혹시 어떨까요 ??
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.
이 부분은 조금 명시해주고 싶은 부분입니다. uploadFile
을 이벤트 핸들링 할 때에는 uploadedFile
이라는 명칭을 쓰고(바딘에서) 메일로 보낼 때에는 attachFile
이라는 명칭을 씁니다.
"사용자가 업로드 한 파일" 과 "첨부 파일" 을 구별해주는게 더 낫지 않을까요?
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.
메일을 보내기 위한 화면이므로 attachments
도 괜찮다고 생각합니다. attachments
를 사용하지 않는다면 uploadedFiles
가 적절하겠네요.
attachFiles = uploadFile | |
attachments = uploadedFiles |
val fds: DataSource = ByteArrayDataSource( | ||
fileData.byteArray, |
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.
네 맞습니다. 다혀성 개념을 지켜주었다고 보시면 될 것 같아요
해당 변수인 fds는 설계상 바이트 형태의 값인 ByteArrayDataSource
가 올 수도 있지만 로컬 파일을 이용하기 위한 FiledataSource
가 올 수도 있습니다. aws에서 attach
를 하기 위해서는 DataSource
형태이기만 하면 되서 혼동을 주지 않기 위해 부모 타입으로 명시했습니다.
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 하겠습니다 ㅎㅎ
try { | ||
client.sendRawEmail(rawEmailRequest) | ||
} catch (ex: Exception) { | ||
ex.printStackTrace() |
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.
print 대신 로그를 남기는 것은 어떤가요?
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.
예외는 ExceptionHandler에서 잡아주고 있지 않나요?
return MimetypesFileTypeMap().getContentType(fileName) | ||
?: throw IllegalArgumentException("잘못된 확장자입니다.") |
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.
변경했습니다
var recipient: Recipient? = null, | ||
var body: String = "", | ||
var files: Map<String, ByteArrayResource>? = 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.
이 부분은 잘몰라서 질문드리는데 recipient와 files 만 nullable 한 타입으로 받는 이유가 있나요?
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.
음... 사실 두 부분은 코틀린 dsl을 쓰면서 만들었다면 null이 될 수 없습니다. 그러나 실제로 null이 없는데 저런식으로 넣는 건 좀 이상하게 보이긴 하네요. 해당 부분도 변경해 주었습니다.
코드리뷰 반영, 첨부파일 삭제시 정상적으로 삭제되어 메일 발송시 해당 부분을 제외하는 기능 추가했습니다 해당 PR이 길어졌으므로 처리한 내역들을 |
interface MailSender { | ||
fun send(toAddress: String, subject: String, body: String) | ||
|
||
fun sendBcc(toAddresses: Array<String>, subject: String, body: String, files: Map<String, ByteArrayResource>) |
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.
Array
대신 List
를 사용하는 건 어떨까요?
코틀린 개발자들이 Array
를 남겨둔건 Array
를 사용하는 자바 라이브러리와의 호환성 때문으로 알고 있어요.
저희가 코딩하는 부분에서는 List
를 사용하는 편이 굉장히 많은 장점을 누릴 수 있을 거 같네요 :)
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.
우선 array를 사용한 건 해당 코드 구현시에 AWS SES 키가 없어서 JavaSimpleMailSender
를 사용했는데 array로 해야하는 작업이 제법 많더라구요 마찬가지로 AWS
에서도 Array
를 사용해야 하는 부분이 제법 있어서 아예 Array로 사용하게 되었습니다.
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.
SimpleMailSender
를 제외하면 모두 List
로 사용할 수 있습니다.
fun sendBcc(toAddresses: Array<String>, subject: String, body: String, files: Map<String, ByteArrayResource>) | |
fun sendBcc(toAddresses: List<String>, subject: String, body: String, attachments: Map<String, ByteArrayResource>) |
try { | ||
client.sendRawEmail(rawEmailRequest) | ||
} catch (ex: Exception) { | ||
ex.printStackTrace() |
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.
예외는 ExceptionHandler에서 잡아주고 있지 않나요?
data class MultipartMimeMessageBuilder( | ||
var session: Session = Session.getDefaultInstance(Properties()), | ||
var applicationProperties: ApplicationProperties = ApplicationProperties(""), | ||
var templateEngine: ISpringTemplateEngine = SpringTemplateEngine(), | ||
var mimeMixedPart: MimeMultipart = MimeMultipart("mixed"), | ||
var subject: String = "", | ||
var userName: String = "", | ||
var recipient: Recipient = Recipient(), | ||
var body: String = "", | ||
var files: Map<String, ByteArrayResource> = mutableMapOf() | ||
) { | ||
fun build(): MultipartMimeMessage { | ||
return MultipartMimeMessage(session, mimeMixedPart, applicationProperties, templateEngine).apply { | ||
setSubject(subject) | ||
setFrom(userName) | ||
setRecipient(recipient) | ||
addBody(body) | ||
addAttachment(files) | ||
} | ||
} |
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.
얘기할만한 부분이 꽤 많은 거 같네요
- 개인적으로 코틀린에서 빌더는 큰 의미가 없다고 생각합니다. 자바에서 빌더가 주는 장점은 코틀린의 named argument나 기본값 지정 등으로 모두 커버가 가능하다고 생각해요.
- 코드를 보면 코틀린 DSL을 사용한 다른 라이브러리를 참고하지 않았나 싶습니다(맞나요?). 만약, DSL -> Builder -> Object 순으로 사용하고 있는 코드를 보셨다면 혹시 빌더가 자바코드인지 한 번 확인해보시는 것도 좋을거 같아요. 보통 자바 라이브러리에서 빌더로 만들어 둔 것들을 감싸서 DSL로 만드는 방식을 사용하더라구요. 그래서 제 생각엔 만약 기존 빌더가 없이 직접 구현할 생각이면 DSL -> Object의 빌더가 없는 구조가 더 괜찮다고 생각합니다.
- 빌더나 DSL을 따로 두려고 하면 Object가 가변일 필요가 있을까 싶어요.
setXX
,addXX
는 빌더가 제공하고 Object는 불변으로 만드는게 서로 책임을 분명히 나눌 수 있는 방법 아닐까싶네요. - subject, recipient 등 필수값에 빈 문자열 등을 기본값으로 넣는게 의미가 있을까요? 필수값이라면 기본값 없이 생성자를 통해 받고, 아니라면
addXX
,setXX
등을 통해 만드는 건 어떨까요? - 사실 이 빌더를 리뷰하는게 쉽지 않았는데, 아마 한군데서만 사용하고 있어서 그런 거 같아요. 적당한 기능을 제공하는지, 적당히 추상화가 됐는지는 사실 여러 군데서 사용해봐야 판단할 수 있는 거 같습니다. 그런 접근에서 보면 이 기능을 구현하기 위해 빌더나 DSL을 사용한 건 약간 오버스펙이 아닌가 싶기도 합니다 :)
- 하지만 DSL에 관심도 많으신거 같고, 잘 다루시는 거 같으니 apply에서 DSL을 적용할만한 부분을 찾자면 문서화 코드에 DSL을 적용해 볼 수 있을 거 같아요.
https://github.com/spring-projects/spring-restdocs/issues/677
. mockmvc는 kotlin dsl을 지원하지만 restdocs는 아직 지원하지 않거든요(apply에서도 문서화 테스트 코드는 기존 자바 스타일의 mockmvc 코드를 사용하고, 다른 컨트롤러 테스트 코드는 dsl 스타일의 mockmvc를 사용합니다). 시간이 남고 열정이 넘친다면 한 번 도전해볼만 한 거 같네요 ㅎㅎ
리뷰가 약간 의식의 흐름대로 나왔는데, 이상한 부분은 언제든지 답글 주세요. 저도 코드 보면서 공부가 많이 됐네요 :)
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.
네. 좋은 의견 감사합니다.
- 사용하는 부분이 큰 의미가 없다는 의견에는 동의합니다. 자바와 달리 코틀린은 기본값을 지정 할 수 있으니까요. 다만, 사용하는 측에서 조금 더 가독성을 높일 수 있어 사용했습니다
- 조금 더 공부해보고 나은 방향을 생각해보겠습니다
- 그렇네요. builder에서 사용하는 메소드도 private 해져야 하고 멤버 변수도 모두 val으로 처리가 가능하겠군요
- 3번과 같은 의견입니다. val로 처리하고 기본값을 모두 없애는게 나아 보입니다. 예외 처리시 좀 더 확실하게 확인할 수 있도록 구현한 부분인데 좋지 않은 방법인 것 같아요
- 오버스펙이라는 의견에는 동의합니다! 제가 한번 만들어놓은
BCC
를 차후에 건드릴 것 같지 않아서 차라리 내부의 값을 자유롭게 바꿀 수 있는 형태로 외부에 제공하려는 생각에서 DSL 을 사용하게 되었네요 - 좋은 레퍼런스 감사합니다
도움이 많이 되었어요. 차후 코드에 반영하도록 하겠습니다!
} | ||
|
||
private fun createRawMessage(message: MimeMessage): RawMessage { | ||
val outputStream = ByteArrayOutputStream() |
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.
Closable
은 .use()
로 닫아주는게 좋을 거 같네요
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.
제가 자원 사용하는 부분 찾아 볼 땐 안보이더니 찾아주셨네요.
꼼꼼한 리뷰 감사합니다
리뷰를 반영하다보니 이곳저곳 건드리게 돼서 중심적으로 구현한 부분과 봐줬으면 하는 부분을 따로 정리했습니다~
|
for (targetMailsPart in recipients.chunked(MAIL_SENDING_UNIT)) { | ||
mailSender.sendBcc(targetMailsPart, request.subject, body, files) | ||
} | ||
|
||
mailHistoryRepository.save( | ||
MailHistory( | ||
request.subject, | ||
request.body, | ||
request.sender, | ||
request.recipients, | ||
request.sentTime | ||
) | ||
) |
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.
염두에 두고 있는 구현 방식이 있을까요?
import javax.mail.internet.MimeBodyPart | ||
import javax.mail.internet.MimeMessage | ||
import javax.mail.internet.MimeMultipart | ||
import javax.mail.util.ByteArrayDataSource | ||
|
||
@Component | ||
class AwsMailSender( |
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.
현재 코드에서 일일 전송 제한은 보이지 않는 것 같은데 제한이 큰 의미가 없다고 판단해서일까요?
private fun sync(requestTime: Long) { | ||
while (available.isNotEmpty()) { | ||
val elapsedTime = requestTime - available.peek() | ||
if (elapsedTime.milliseconds < 1.seconds) { | ||
break | ||
} |
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.
지금 구현에서 문제되는 부분은 아니지만 혹시라도 나중에 시간 순서대로 queue에 쌓이지 않으면 RateLimiter가 잘못 동작할 수도 있을 거 같아요.
elapsedTime이 음수라면 예외처리하는 로직을 추가해도 좋을 것 같습니다 😄
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.
…gs of aws and spring
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.
늦은시간까지 페어프로그래밍 수고하셨습니다!!
ExperimentalCoroutinesApi
로 코루틴으로 예외를 잡는 부분,,, 잘 보고 배우고 갑니당👍
import kotlinx.coroutines.test.runTest | ||
import kotlin.time.Duration.Companion.milliseconds | ||
|
||
private val Int.ms: Long get() = milliseconds.inWholeMilliseconds |
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.
테스트해 보니 잘 작동하네요. 고생하셨습니다. 👍
고생하셨습니다! |
메일 발송을 위한 API입니다.
현재
AWS SES
가 아니라JavaMailSender
를 사용하고 있습니다. 차후 변경 과정에서 테스트 과정이나 스펙이 바뀔 수 있습니다.더 자세한 정보와 이미지는 #305 에서 확인할 수 있습니다.
테스트 방법
application.properties
의username
과password
에 자신의 메일 계정과 패스워드를 적습니다.이 과정에서
Gmail
을 사용중일 경우 해당 이슈 를 참고하며, 보안 수준이 낮은 앱의 엑세스를 허용해 주어야 합니다 링크 참고이후에
postman
혹은 기타 툴로request
툴로 요청을 보내면 테스트 할 수 있습니다postman 예시
아래 이미지와 같이 채워 넣을 수 있습니다.
json
예시text 타입으로 지정합니다
files
예시file 타입으로 지정합니다
Close #305