feat : Mail 구현#48
Conversation
dh0304
left a comment
There was a problem hiding this comment.
이메일 보내는 기능들이 api 모듈에 존재하도록 구성했는데 이유가 있을까요?
굳이 분리하자면
core 모듈: 이메일 대상을 찾고, 제목과 내용을 구성하는 코드
api 모듈: SMTP 프로토콜과 관련된 코드, html형식을 구성하는 코드
이렇게 분리해볼 수 있을 것 같아요.
이것도 도메인 로직 관점에서 봤을 때 그런거고, 이메일을 구성하는 것을 도메인 로직으로 볼 것이냐, 다르게 볼것이냐에 따라서 달라질 것 같아요.
There was a problem hiding this comment.
local 파일에 auto ddl을 update를 create로 바꿔주세요~
There was a problem hiding this comment.
개인적인 의견으론 메일 모듈로 빼는 것도 아직은 시기상조라고 생각합니다.
api 모듈보단 core 모듈에 우선적으로 놓고 나중에 도메인이 좀 더 성숙해졌을 때 빼도 괜찮다고 생각했어요.
근데 이미 뺐으니까 일단 이렇게 진행하고 나중에 좀 더 도메인이 성숙해졌을 때 다시 리팩터링해도 될 것 같네요.
이것도 개인적인 의견이라, 승우님 의견이 듣고싶네요.
그리고 수정한 내용에 대해서는 확인하기 쉽게 커밋 내역 남겨주세요!
bc1d013
다른 코멘트에서 논의되고 있는 부분들 해결되면 메일 모듈 코드 한번에 확인할게요~
| } | ||
|
|
||
| @AllArgsConstructor(access = AccessLevel.PRIVATE) | ||
| public static class MailBuilder { |
There was a problem hiding this comment.
static class로 구성한 이유가 MailSendDto의 to 메서드를 시작점으로 사용하기 위해선가요?
There was a problem hiding this comment.
inner class 는 특별한 경우가 아니라면 static 을 붙여야합니다.
- 내부클래스는 외부클래스의 참조를 가진다. MailBuilder로 예를 들어보자면 MailBuilder를 생성할때 MailSendDto를 생성자로 받는다는 말이다.
- 참조를 하고있기 때문에 메모리를 더 사용한다.
- 참조하고있기때문에 GC 대상이 되지 않는다. (메모리누수)
There was a problem hiding this comment.
아 질문을 바꿀게요.
static을 붙여야하나요? 에 대한 질문이 아니라,
inner 클래스 없이 MailSendDto 클래스 하나로 처리 가능했을텐데, inner class를 사용한 이유에 대해 질문한거였어요!
There was a problem hiding this comment.
클래스 하나로 처리가 가능한데 -> 어떻게 가능하다는건지 무슨 의도로 클래스 하나의 처리와 inner class로의 처리에 대해서 질문을 하는건지 정확하게 말씀해주세요. 클래스 하나로 처리한다고했을때 어떻게 처리하면 좋을것같은지도 같이 적어주세요
static class를 사용한 이유는 매개변수가 너무 많고 또 3개의 매개변수가 모두 기본타입이기 때문입니다.
그리고 성질이 너무 달라요
보낼이메일 (한명에게만 보낼 수 있고, 여러명에게 보낼 수 있어야 함), 제목, 내용
모두 하나의 생성자또는 static 클래스 생성으로 만들기에는 복잡해보였습니다.
그래서 to 로 보낼사람으 이메일을 먼저 받고 builder에서 제목과 내용을 반드시 입력해야지만 객체를 생성할 수 있도록 강제한겁니다.
There was a problem hiding this comment.
확인이요~
별 의도는 없습니다! 그냥 궁금해서 물어본거예요
There was a problem hiding this comment.
MailSendRepository 인터페이스와 구현체가 Repository가 아니라 Service의 역할을 하고 있는 것 처럼 보여요.
Repository 계층은 데이터 접근 계층으로, DB와 상호작용이 일어나야하는데,
이렇게 구성한 이유가 있나요?
There was a problem hiding this comment.
MailSendRepository보단 MailSender와 같은 이름이 더 어울리지 않을까요?
컴포넌트로 선언하고 MailSenderImpl 클래스를 @Service계층으로 선언하는 게 자연스러워 보여요.
There was a problem hiding this comment.
계층은 중요하지 않아요 최종적으로 모듈화해서 사용자는 MailSender, MailSendDto의 사용만 허용하게 할 생각입니다. 원래 MailService와 같은 이름으로 하려다가 외부로 보이는 객체의 계층이 노출되는게 싫어서 이름을 MailSender로 바꾼거에요.
MailSendRepository와 MailSender의 역할이 불명확해진건 맞아요. 기존 계획을 변경했기 때문에 생긴 일인데,
기존 MailSendRepository 는 가장 원시적인 메일전송 기능을 제공하고, MailSender 에서 html, text 형식의 기능을 제공하는 메일전송기능을 개발하는게 원래 계획이었어요.
이렇게 되면 메일 전송 기능은 모두 구현이 되지만 이 MailSender를 조작하는 개발자 입장에서는 처리해야할 코드가 많아집니다.
인증번호 전송이란 단순한 기능을 외부에서 처음부터 끝까지 다 작성해야해요.
MailSender 객체를 보면 알겠지만 sendAuth 메소드가 있습니다. 메일주소와 인증번호만 있으면 쉽게 전송할 수 있어요.
그래서 기존 MailSender의 역할을 MailSenderRepository 로 이동시키고 MailSender 에는 편의기능을 넣기 위해서 이렇게 개발된겁니다.
sendAuth() -> 인증메일 편의기능, 이곳에 편의기능 추가개발
sendHtml(), sendText -> 기본적인 기능도 사용할 수 있어야함.
답이 좀 되었나요
There was a problem hiding this comment.
기능적인 부분은 이해가 됐습니다.
그런데 계층적인 부분은 짚고 넘어가야할 것 같아요.
계층이 중요하지 않다라는 의견은 반은 동의하고 반은 동의하지 않는데요.
동의하는 이유는 코드에는 유연성이 필요하기 때문이죠.
반면에 동의하지 않는 이유는 아래와 같아요.
계층을 정해놓은 이유는 유지보수 측면 관점에서 볼 수도 있지만, 그보다 더 중요한 것은 팀원을 위한 것이라고 생각해요.
즉, 프로토콜이라는 소리죠.
예를 들어 새로운 팀원이 들어왔을 때 Repository 라는 이름을 가진 클래스가 DB와 상호작용이 일어나지 않는다면 이는 큰 문제가 됩니다. 프로토콜에 정의된 대로 동작하지 않는다면 이는 유지보수에도 큰 영향을 미치게 됩니다. 팀원뿐만아니라 미래의 내가 봤을 때도 헷갈릴 가능성있을거예요.
Service의 역할을 하는 것 처럼 보여도, 이 클래스의 동작을 설명할 수 있는 다른 이름이라면 괜찮습니다.
우리가 정의한 4계층 관점에서 본다면 Component에 들어갈 클래스가 되겠죠?
MailSendRepository 또한 마찬가지예요. Repository 라는 이름 대신에 의도를 나타낼 수 있는 다른 이름이 필요해보여요,
There was a problem hiding this comment.
주석을 잘 달아주셔서 의도는 이해가 됐습니다.
@PostConsturct를 사용하여 애플리케이션 시작 시 템플릿에 대한 유효성을 검증하는데, 이 부분은 테스트 코드로 구성이 가능해보입니다.
github actions workflow에 배포시 빌드전에 테스트 코드가 동작하도록 구성되어있어, 테스트 코드로 충분해 보입니다.
테스트 코드 대신 이렇게 구성한 의도가 있을까요?
There was a problem hiding this comment.
테스트코드에 넣어도 괜찮을것같네요
There was a problem hiding this comment.
MailSendRepository보단 MailSender와 같은 이름이 더 어울리지 않을까요?
컴포넌트로 선언하고 MailSenderImpl 클래스를 @Service계층으로 선언하는 게 자연스러워 보여요.
There was a problem hiding this comment.
MailSendRepositoryImpl에도 코멘트를 남겼지만, MailSender 대신에 MailService와 같이 서비스 계층으로 선언하는 것이 더 낫지 않을까요?
MailService로 이름을 바꾸고 MailSendRepositoryImpl을 MailSender와 같은 이름으로 바꾸고 MailService에서 사용하는 것이 자연스러워 보여요.
There was a problem hiding this comment.
댓글 남겼어요~ 확인해주세요.
이 댓글에 답변이 달리지 않았는데, 확인하고 답변 부탁드려요~
추가 코멘트 남길게요.
로그관련 코드를 보니까 log.error 처럼 error 레벨의 로그가 많이 존재하더라구요.
error 대신에 warn 레벨로 바꿔야할 지 한번 더 체크해주세요.
error 레벨을 나중에 모니터링과 연결되어 즉시 알람이 발생하여 바로 문제가 해결되어야 하는 부분에 작성되어야 합니다.
기능이 동작하지 않으면 서비스에 치명적인 문제가 존재할 때 error 레벨을 사용하는 것이 좋아요.
이메일을 보내는 기능 또한 중요한 기능이지만, 제 생각에는 서비스에 치명적인 문제가 존재할 기능은 아니라고 판단되어요.(개인적인 의견입니다. 약간 모호한 느낌이 있어요. warn인거 같으면서 error인것같은?)
| } | ||
|
|
||
| @AllArgsConstructor(access = AccessLevel.PRIVATE) | ||
| public static class MailBuilder { |
There was a problem hiding this comment.
아 질문을 바꿀게요.
static을 붙여야하나요? 에 대한 질문이 아니라,
inner 클래스 없이 MailSendDto 클래스 하나로 처리 가능했을텐데, inner class를 사용한 이유에 대해 질문한거였어요!
There was a problem hiding this comment.
기능적인 부분은 이해가 됐습니다.
그런데 계층적인 부분은 짚고 넘어가야할 것 같아요.
계층이 중요하지 않다라는 의견은 반은 동의하고 반은 동의하지 않는데요.
동의하는 이유는 코드에는 유연성이 필요하기 때문이죠.
반면에 동의하지 않는 이유는 아래와 같아요.
계층을 정해놓은 이유는 유지보수 측면 관점에서 볼 수도 있지만, 그보다 더 중요한 것은 팀원을 위한 것이라고 생각해요.
즉, 프로토콜이라는 소리죠.
예를 들어 새로운 팀원이 들어왔을 때 Repository 라는 이름을 가진 클래스가 DB와 상호작용이 일어나지 않는다면 이는 큰 문제가 됩니다. 프로토콜에 정의된 대로 동작하지 않는다면 이는 유지보수에도 큰 영향을 미치게 됩니다. 팀원뿐만아니라 미래의 내가 봤을 때도 헷갈릴 가능성있을거예요.
Service의 역할을 하는 것 처럼 보여도, 이 클래스의 동작을 설명할 수 있는 다른 이름이라면 괜찮습니다.
우리가 정의한 4계층 관점에서 본다면 Component에 들어갈 클래스가 되겠죠?
MailSendRepository 또한 마찬가지예요. Repository 라는 이름 대신에 의도를 나타낼 수 있는 다른 이름이 필요해보여요,
작업 내용
Spring Mail 구현
MailSender로 Text, Html 형식으로 메일전송 가능
메일 형식은 MailSendDto 를 사용
MailSendDto
.to(String... address) // 보낼메일의 주소
.write(String subject, String content) // 제목과 내용
HTML은 직접 작성해서 보낼 수 있지만 사용 편의성을 위해
EmailTemplateType 에서 정의해 놓은 템플릿을 사용할 수 있음
사용되는 템플릿은 resources/email-template 패키지 안에 html 파일로 정의되어있음.
HTML 변수사용가능, {{변수명}} 으로 작성하고 Map 형식으로 key/value : 변수명/값 으로 넣어 사용할 수 있음
예) EmailTemplateType.AUTH.toHTMLString(Map.of("AuthKey", 12345));
-> AuthKey 변수를 12345로 치환
이메일인증은 고정된 템플릿을 사용하므로
MailSender에 void sendAuth(String toAddress,int authNumber); 로 정의 되어있음.
리뷰 요구사항