Skip to content
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 : 이메일 인증 기능 구현 #57

Merged
merged 68 commits into from
Feb 25, 2024

Conversation

binary-ho
Copy link
Member

@binary-ho binary-ho commented Feb 11, 2024

🌱 관련 이슈

📌 작업 내용 및 특이사항

  • 12일에 유진님과 얘기해서 gmail password 채워 넣고 조금 더 수정해야 완성이지만, 리뷰를 위해 미리 OPEN합니다
  • 네이밍 맘에 안 드는 부분이 많은데 같이 봐주시면 감사하겠습니다.
  • 패키지 이동은 develop pull 받은 이후 옮기겠습니다!

📝 참고사항

📚 기타

@binary-ho binary-ho requested a review from a team as a code owner February 11, 2024 19:46
Copy link
Member

@uwoobeat uwoobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR, 커밋 컨벤션 맞춰주세요~

Copy link
Member

@Sangwook02 Sangwook02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

몇가지 제안과 오타 수정사항 등 확인했습니다!

Comment on lines 17 to 19
gmail:
id: ${GMAIL_ID}
password: ${GMAIL_PASSWORD}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redis, storage처럼 이메일은 yaml 파일을 분리하지 않는게 일반적인가요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일반적이라기 보다는 상황에 따라 다를거 같아요 ㅎㅎ
한번 고민해볼게요 감사합니다!


private static final String HONGIK_UNIV_MAIL_DOMAIN = "@g.hongik.ac.kr";

private static final String EMAIL_REGEX = "^[^\\W&=_'-+,<>]+(\\.[^\\W&=_'-+,<>]+)*@g\\.hongik\\.ac\\.kr$";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 정규표현식을 모두 RegexConstant에서 관리하고 있는데 이 경우에는 RegexConstant에서 관리하지 않는 특별한 이유가 있을까요?

Copy link
Member Author

@binary-ho binary-ho Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 필요해서 가지고 있었어요!
패키지 정리하면서 RegexConstant로 옮기겠습니다! ㅎㅎ

Comment on lines 10 to 13
+ "<h1> 안녕하세요 GDSC Hongik 이메일 인증 메일입니다. </h1> <br>"
+ "<h3> 아래의 인증 코드를 %d분 안에 입력해주세요. </h3> <br>"
+ "<h3> 감사힙니다. </h3> <br>"
+ "CODE : <strong>";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. "이메일 인증 메일입니다." 대신 "재학생 인증 메일입니다." 어떤가요?

  2. '감사힙니다' 오타있습니다!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

놓쳐서 죄송합니다!
꼼꼼하게 봐주셔서 감사합니다!

@Sangwook02 Sangwook02 added the ✨ feature 새로운 기능 추가 및 수정 label Feb 12, 2024
Copy link
Member

@uwoobeat uwoobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨트롤러 클래스가 없는 것 같은데 체크 부탁드립니다~

@@ -0,0 +1,26 @@
package com.gdschongik.gdsc.domain.integration;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

유틸리티로 빼는 게 맞을 것 같습니다~

@RequiredArgsConstructor
public class JavaMailSender implements MailSender {

private static final String SENDER_PERSONAL = "GDSC Hongik 운영팀";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constant 클래스로 분리하면 더 좋을 것 같습니다~

public class JavaMailSenderConfig {

@Value("${gmail.id}")
private String id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MailProperty 로 분리해서 사용하면 좋을 것 같아요~

MimeMessage message = writeMimeMessage(recipient, content);
javaMailSender.send(message);
} catch (MessagingException e) {
throw new RuntimeException(e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커스텀 예외로 변경해주시면 좋을 것 같아요~

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 감사합니다!!

import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;

@Service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서비스 계층이라고 보는 것이 맞나? 라는 의문이 드네요.
도메인이랑 상호작용하는 부분이 없으니 유틸리티라고 보는 게 맞지 않을까 싶습니다~

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그게 맞는 것 같아요! 말씀 감사합니다 ㅎㅎ


public interface MailSender {

void send(String recipient, String content);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메일 전송 성공/실패를 전달받을 수 있는 방법은 존재하지 않나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인해보겠습니다!

@Component
public class VerificationMailContentWriter {

private static final String NOTIFICATION_MESSAGE = "<div style='margin:20px;'>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인증 코드 입력방식 말고, 인증 URL 클릭하는 방식으로 처리하는게 어떨까 싶습니다~
stoplight 명세 확인해보시면 좋을 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

죄송합니다 확인해보겠습니다!

@binary-ho binary-ho changed the title ✨ 이메일 인증 기능 구현 feat : 이메일 인증 기능 구현 Feb 12, 2024
Copy link
Member

@Sangwook02 Sangwook02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수랑 메서드명 suggestion 세 가지랑
로직에 대한 부분 2개 있습니다.

작업량이 많아서 한번에 리뷰를 못 드리네요.. 죄송합니다😢

@Operation(summary = "학교 인증 메일 발송 요청", description = "학교 인증 메일 발송을 요청합니다.")
@PostMapping
public ResponseEntity<Void> sendUnivEmailVerificationLink(
@Valid @RequestBody UnivEmailVerificationLinkSendRequest univEmailVerificationLinkSendRequest) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 controller에서는 모두 request로 사용하고 있습니다.
통일해야 할 필요는 없지만 너무 긴 것 같아서 suggestion 남깁니다!

Suggested change
@Valid @RequestBody UnivEmailVerificationLinkSendRequest univEmailVerificationLinkSendRequest) {
@Valid @RequestBody UnivEmailVerificationLinkSendRequest request) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 확인했어야 했는데, 못 했습니다.
좋은 것 같습니다! 감사합니다

Comment on lines 45 to 47
univEmailVerificationRepository.findByUnivEmail(univEmail).ifPresent(univEmailVerification -> {
throw new CustomException(ErrorCode.UNIV_EMAIL_ALREADY_VERIFIED);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redis에 univEmail이 present 하다는 것은 이미 이메일이 발송되었을 뿐, VERIFIED 상태는 아닌 것 같습니다!

이미 재학생 인증을 마친 회원인지를 검증하려는 목적이라면 MemberUtil로 Member를 가져와서 Member의 Requirement의 univStatus를 확인하는게 적절해보입니다!

Copy link
Member Author

@binary-ho binary-ho Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

머리 속으로는 그렇게 짠다고 생각했는데, 실제 코드는 저렇게 됐네요..
Member Repository에 저 로직을 위한 메서드까지 선언했었네요.
부끄럽습니다.. 봐주셔서 감사합니다!

꼼꼼히 체크하던, 테스트를 작성하던 해야겠군요
급한 작업들 다 끝나면 테스트 부터 작성해야겠습니다..

hongikUnivEmailValidator.validate(univEmail);
validateUnivEmailNotVerified(univEmail);

String verificationCode = verificationCodeGenerator.generate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verificationCode도 결국 verificationLink의 일부니까 send() 메서드보다는 VerificationLinkUtil.createLink에서 알아서 만들어 주는게 적절하지 않나하는 생각이 들어요!
어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 verificationCode를 verificationLinkUtil가 createLink 메서드 내부에서 스스로 generate해서 링크에 포함하는게 어떻냐고 말씀하신게 맞을까요?

현재 verificationCode는 Link의 일부이긴 하지만, 이메일 인증을 위해 Redis에 Key로써 따로 저장되는 값이기도 합니다!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redis에 저장해야 하는 문제 때문에 service에서 직접 호출할 수밖에 없는 상황이군요..
확인했습니다!

private final UnivEmailVerificationRepository univEmailVerificationRepository;

public void verifyMemberUnivEmail(String verificationCode) {
UnivEmailVerification univEmailByVerificationCode = getUnivEmailVerification(verificationCode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
UnivEmailVerification univEmailByVerificationCode = getUnivEmailVerification(verificationCode);
UnivEmailVerification univEmailVerification = getUnivEmailVerification(verificationCode);

@@ -43,4 +43,8 @@ public static Requirement createRequirement() {
public boolean isUnivPending() {
return this.univStatus == PENDING;
}

public void updateUnivState(RequirementStatus univStatus) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수명이랑 통일감 있게 가면 좋을 것 같습니다.

Suggested change
public void updateUnivState(RequirementStatus univStatus) {
public void updateUnivStatus(RequirementStatus univStatus) {

Copy link
Member

@uwoobeat uwoobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기능 코멘트에 대략적인 설명좀 부탁드려요.
지금 기능 사이즈가 커서 간단하게 설명 보면서 리뷰하는 게 좋을 것 같습니다.

++ 해당 기능 테스트로 정상작동하는거 체크하셨는지도 궁금합니다

@binary-ho
Copy link
Member Author

binary-ho commented Feb 16, 2024

기능 코멘트에 대략적인 설명좀 부탁드려요. 지금 기능 사이즈가 커서 간단하게 설명 보면서 리뷰하는 게 좋을 것 같습니다.

++ 해당 기능 테스트로 정상작동하는거 체크하셨는지도 궁금합니다

@uwoobeat

애초애 2개로 나누었어야 했는데 아쉬움이 많이 남습니다..

기본적으로 Service 클래스는 2개로 나뉩니다.

  1. 재학생 "인증링크"를 전송하는 UnivEmailVerificationLinkSendService
  2. 인증 코드를 통해 유저의 재학생 메일을 인증해주는 UnivEmailVerificationService

1. UnivEmailVerificationLinkSendService

univEmail을 받아 인증 메일 전송을 수행합니다.

  1. 홍익대학교 이메일이 맞는지 검증
  2. 해당 이메일로 인증한 이력이 없음을 검증
  3. 랜덤 인증 "코드" 발급
  4. 인증 "링크" 전송 (인증 링크는 유저가 클릭하게 될 인증 코드가 포함된 링크입니다.)
  5. 인증 코드를 메모리 저장소에 저장
public void send(String univEmail) {
    hongikUnivEmailValidator.validate(univEmail);
    validateUnivEmailNotVerified(univEmail);

    String verificationCode = verificationCodeGenerator.generate();
    sendVerificationLink(univEmail, verificationCode);

    saveUnivEmailVerification(univEmail, verificationCode);
}

4, 5번 메서드에 대해 추가로 설명하겠습니다.

1.1 sendVerificationLink와 MailSender

링크를 생성하고 초대 메일의 문구를 작성합니다.
MailSender를 이용해 메일을 전송합니다. (구현체 JavaMailSender)

private void sendVerificationLink(String univEmail, String verificationCode) {
    String verificationLink = verificationLinkUtil.createLink(verificationCode);
    String mailContent = verificationMailContentWriter.write(verificationLink, VERIFICATION_CODE_TIME_TO_LIVE);

    mailSender.send(univEmail, UnivEmailVerificationConstant.VERIFICATION_EMAIL_SUBJECT, mailContent);
}

1.2 saveUnivEmailVerification

MemberUtil을 통해 현재 로그인한 유저의 id를 가져옵니다.
인증 코드를 Key로, 인증 코드, 홍익대학교 이메일, 유저 아이디를 Redis에 저장합니다.

인증 코드가 Key인 이유는 나중에 유저가 링크를 눌렀을 때 빠르게 객체를 찾기 위함입니다.
홍익대학교 이메일이 Key인 경우 유저의 중복 발급을 막기 용이하나, 일단은 그런 경우는 일단 배제하기로 했습니다. 인증 코드가 Key일 떄의 이득이 더 있다고 생각했습니다. 필요한 경우 다른 방법으로 막는게 좋아 보입니다.
또한 매우 낮은 확률로 랜덤 인증 코드가 겹치는 경우도 배제했습니다. 추후 1번 정도의 리트라이 로직을 넣는 것도 괜찮을 것 같습니다.

private void saveUnivEmailVerification(String univEmail, String verificationCode) {
    Long currentMemberId = memberUtil.getCurrentMemberId();
    UnivEmailVerification univEmailVerification = new UnivEmailVerification(
            verificationCode, univEmail, currentMemberId, VERIFICATION_CODE_TIME_TO_LIVE.toSeconds());

    univEmailVerificationRepository.save(univEmailVerification);
}

이렇게 하면 전송과 발급 과정이 끝나게 됩니다.

2. UnivEmailVerificationService

유저가 verificationCode가 포함된 링크를 누르는 경우 인증을 해주는 Service입니다.
redis 저장소에서 verificationCode를 이용해 홍익대학교 이메일과, 유저 id를 가져옵니다.

이후 유저 id로 유저 객체를 가져옵니다. 이후 유저의 univEmail를 업데이트 해주고, 유저 Requirement의 univStatus 또한 VERIFIED로 업데이트 해줍니다.

public void verifyMemberUnivEmail(String verificationCode) {
    UnivEmailVerification univEmailVerification = getUnivEmailVerification(verificationCode);
    Member member = getMemberById(univEmailVerification.getMemberId());
    member.completeUnivEmailVerification(univEmailVerification.getUnivEmail());
}



맴버 안의 update 메서드

public class Member extends BaseTimeEntity {

    ...(생략)

    public void completeUnivEmailVerification(String univEmail) {
        this.univEmail = univEmail;
        requirement.updateUnivStatus(RequirementStatus.VERIFIED);
    }

    ....(생략)
}

3. 추가 설명이 필요한 유틸 클래스들

3.1 VerificationCodeGenerator

랜덤 코드의 발급은 간단하게 RandomStringUtils를 통해 생성합니다. 16자의 코드를 생성하고, 아스키 테이블의 '0' 부터 'z' 사이의 문제들로 생성됩니다.

random 메서드의 4, 5 번째 파라미터에 true를 넣어, 특수문자를 모두 제외하고 숫자와 알파벳 대소문자로만 코드를 만들도록 했습니다.

@Component
public class VerificationCodeGenerator {

    private static final int VERIFICATION_CODE_LENGTH = 16;
    private static final char RANGE_START_CHAR = '0';
    private static final char RANGE_END_CHAR = 'z';

    public String generate() {
        return RandomStringUtils.random(VERIFICATION_CODE_LENGTH, RANGE_START_CHAR, RANGE_END_CHAR, true, true);
    }
}

3.2 VerificationLinkUtil

EnvironmentUtil을 활용해 환경 상태에 따라 다른 Link를 생성합니다.

  private String getClientUrl() {
      return switch (environmentUtil.getCurrentProfile()) {
          case PROD -> PROD_CLIENT_URL;
          case DEV -> DEV_CLIENT_URL;
          default -> LOCAL_VITE_CLIENT_SECURE_URL;
      };
  }

4. 더 드릴 말씀

  1. 궁금한 것이 있다면 편하게 더 물어봐주세요
  2. env 값이 현재는 imhere의 메일 발송용 gmail로 메일을 보내고 있습니다. 최대한 빨리 gdsc 공식 메일 계정으로 바꾸겠스빈다. (노션에 업데이트 했습니다)
  3. 정상작동은 코드레벨의 테스트를 통해 확인했습니다. 코드를 올리지 못한 이유는 Redis 테스트 작성에 대해 아직 미숙해 코드를 작성하지 못했습니다. 직접 Redis 서버를 돌리며 테스트했고, 메일이 정상적으로 가고 레디스에 잘 저장되고 찾아지는 정도의 테스트를 마쳤습니다. (결론 : 서비스~dao 단까지는 테스트가 된 상태)

Copy link
Member

@Sangwook02 Sangwook02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service쪽에 하나 더 리뷰 남겼습니다!
미리 approve 해둘게요~
감사합니다👍👍

Comment on lines 52 to 57
private void sendVerificationLink(String univEmail, String verificationCode) {
String verificationLink = verificationLinkUtil.createLink(verificationCode);
String mailContent = verificationMailContentWriter.write(verificationLink, VERIFICATION_CODE_TIME_TO_LIVE);

mailSender.send(univEmail, UnivEmailVerificationConstant.VERIFICATION_EMAIL_SUBJECT, mailContent);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

send에서 또 다른 send를 부르는게 살짝 어색한 것 같아요
mailSender.send를 위에 있는 public send에서 호출하도록 하고 메서드 이름을 writeMailContent 같은 이름으로 바꿔봐도 좋을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다 ㅎㅎ

@RestController
@RequestMapping("/onboarding/verify-email")
@RequiredArgsConstructor
public class UnivEmailVerificationController {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 컨트롤러를 두 개로 쪼개야 하는 필요가 있나 싶습니다. 현재는 어드민과 온보딩 두 가지 서비스에 맞게 컨트롤러 쪼개주고 있으므로 해당 규칙 따라서 구현하면 될 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엔드포인트와 사용 Service가 다르기 떄문에 분리해 보았습니다.
패키지로 구분이 가능하다고 생각했었는데, 읽는 분 입장에서 불필요하게 느껴진시다면 말씀 주세요 ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨트롤러는 엔드포인트 단위로 구분되는 것이 좋지 사용 서비스에 따라서 구별할 필요는 없다고 봅니다.
현재 시점에서 굳이 성급하게 분리할 필요가 없을 것 같습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이러한 방식으로 컨트롤러 클래스를 구성하게 되면, 모든 이메일 관련 API에 대한 컨트롤러 클래스를 만들어줘야 합니다. 불필요하게 복잡해지는 문제가 생긴다는 겁니다.

컨트롤러를 쪼개야 하는 경우는 하나의 컨트롤러에 너무 많은 API가 존재해서, 서로 다른 도메인으로 분리해내야 하는 경우입니다. 필요해질 때 분리하고, 그렇지 않을 때는 합치는 것이 관리 포인트를 줄일 수 있는 방법입니다.

https://careerly.co.kr/comments/66030?utm_campaign=user-share

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

비슷한 맥락에서 컨텍스트 분리 전까지는 EmailController 와 같이 "이메일" 이라는 컨텍스트로만 관리해주고
그 외 클래스들도 의미만 잘 드러나는 선에서만 네이밍해주고 좀 과도하게 상세해진 부분은 수정하는 게 어떨까 싶습니다
가령 UnivEmail -> Email 같이요. '재학생' 이메일 인증과 그냥 이메일 인증이라는 컨텍스트를 구분할 필요성이 없기 때문에 저는 굳이? 싶다는 생각입니다. 다만 이 부분은 크리티컬하지는 않아서 그냥 Univ 붙여쓰든 안붙여쓰든 크게 문제 없을 것 같아요. 의미만 충분하다면 네이밍을 굳이 길게 쓸 필요는 없다는 게 제 생각이어서요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 글 감사합니다! 공감되는 내용이네요 ㅎㅎ

  1. 메일 발송은 /onboarding/send-verification-email을 사용하고, 메일 인증은 onboarding/verify-email을 사용하는데, 같은 엔드포인트로 보는게 맞을까요?
  2. 좋은 말씀입니다! 이 PR Merge이후에 시간이 되는 대로 고쳐볼게요

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 맞습니다. 하나의 컨트롤러로 통일하는 게 맞습니다.

이름은 조금 더 고민해보셔요. 제가 EmailController 로 합치는 게 좋다고 했지만 사실은 OnboardingXXXController / AdminXXXController 와 같이 분리해서 사용하고 있습니다. (나중에 온보딩 / 어드민 도메인 분리해두는 것을 염두에 두고 이렇게 작업하고 있습니다)

컨벤셔널하게 간다면 OnboardingEmailController 가 맞습니다. 하지만 EmailController 가 그렇게 틀렸냐 하면 그건 아닙니다. 예측가능성 vs 유지보수 포인트 줄이기 + 이메일 도메인의 특수성 고려 간 trade-off 잘 고려하셔서 진호님이 자율적으로 선택하시면 될 것 같아요. 언제까지나 제 생각입니다. 이런 문제에 정답은 없으니까요~

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조금 헷갈려서 다시 질문드립니다. 현재 메일 발송은 엔드포인트를 /onboarding/send-verification-email을 사용하고, 메일 인증은 엔드포인트로 /onboarding/verify-email 하는데 하나의 엔드포인트로 보신다는 말씀이실까요?

그래서 최상단에는 @RequestMapping("/onboarding") 이렇게 두고 두개의 메서드에 @PostMapping("send-verification-email"), @GetMapping("verify-email") 이렇게 쓰자고 말씀하신건가요?

아니면 "onboarding/verify-email"과 같은 하나의 엔드포인트로 통일하고 하나에 컨트롤러에 넣어 달라는 말씀이실까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨벤션 상으로는 컨트롤러를 AdminXXXController, OnboardingXXXController 와 같이 분리해서 사용하고 있습니다.

하지만 현재는 어드민에서 이메일 보내는 기능이 존재하지 않고,
해당 기능이 추가될 가능성이 그리 높다고 보기도 어려우므로,
AdminEmailController, OnboardingEmailController 로 분리해야만 하는 것은 아닙니다.
EmailController 와 같이 사용할 수도 있습니다.

컨벤셔널하게 vs 관리포인트를 작게 중 어느 것을 선택하냐에 대한 문제라는 것입니다.

OnboardingEmailController 를 사용한다면 클래스 레벨의 @RequestMapping("/onboarding") 이 있어야겠죠.
EmailController 를 사용한다면 클래스 레벨의 @RequestMapping("/onboarding") 을 사용하든 그냥 해당 어노테이션 없이 가든 자유롭게 선택 가능하겠죠.

결론은 컨트롤러 네이밍을 어떻게 가져가냐에 따라 질문주신 내용에 대한 방향성이 달라진다는 것입니다.

Comment on lines 47 to 49
memberRepository.findByUnivEmail(univEmail).ifPresent(univEmailVerification -> {
throw new CustomException(ErrorCode.UNIV_EMAIL_ALREADY_VERIFIED);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
memberRepository.findByUnivEmail(univEmail).ifPresent(univEmailVerification -> {
throw new CustomException(ErrorCode.UNIV_EMAIL_ALREADY_VERIFIED);
});
memberRepository.findByUnivEmail(univEmail).orElseThrow(
() -> new CustomException(ErrorCode.UNIV_EMAIL_ALREADY_VERIFIED));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find결과가 present일때 (객체가 DB에 있을 때) 예외를 발생생시키는 상황입니다!
orElseThrow는 객체가 없을 때 예외를 던지는 메서드로 알고 있는데 확인해주실 수 있을까요??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 잘못 적었네요. () -> new CustomException(...) 과 같이 작성하면 어떨까 싶었습니다. 람다 파라미터를 사용하지는 않으니까요~

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위 메서드 ifPresent는 Consumer가 요구되는 메서드로 보여주신 모습과 같이 작성하긴 어렵습니다.
그것과 별개로 ifPresent를 안 쓰는 쪽으로 바꿔보겠습니다.


private void sendVerificationLink(String univEmail, String verificationCode) {
String verificationLink = verificationLinkUtil.createLink(verificationCode);
String mailContent = verificationMailContentWriter.write(verificationLink, VERIFICATION_CODE_TIME_TO_LIVE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write() 는 과도하게 추상화되었다는 생각이 듭니다 + 로직도 굳이 유틸리티로 뺄 필요가 있나... 싶습니다

유틸리티로 분리하고 의존성 주입으로 처리하겠다는 건 재사용성과 확장 가능성을 고려하겠다는 건데,
애초에 재학생 인증이라는 컨텍스트에 종속적인 기능이고
그래서 인터페이스에서도 인증링크와 ttl만 받고 있는 것인데,
이 상황에서 유틸리티로 빼낸다고 해서 큰 이점을 누릴 수 있을 것 같지 않습니다

그냥 서비스 코드에서 String.format 호출하더라도 큰 문제 없었을 것 같습니다.
지금은 유연하다기보다는 오히려 서비스 복잡도만 증가시키는 것 같습니다

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생각해 봤는데, 과한 분리가 맞는 것 같습니다!
이 객체 안에서 처리해보겠습니다 짚어주셔서 감사합니다 ㅎㅎ

});
}

private void sendVerificationLink(String univEmail, String verificationCode) {
Copy link
Member

@uwoobeat uwoobeat Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sendVerificationLink 메서드에서 verificationMailContentWriter.write() 를 호출하여 메일의 내용(content)를 결정하는 것이 해당 메서드 구현부의 책임은 아닌 것 같습니다.

해당 메서드는 이름이 뜻하는 것처럼 링크를 전송하는 것이고 그 과정에서 인자로 1) 어떤 링크를 보낼지 2) 어떤 내용으로 보낼지 3) 어디로 보낼지는 모른 상태로 그냥 외부에서 전달받은 값을 잘 모아서 send만 해주면 됩니다.

1), 2), 3)에 대한 값을 결정하는 것은 외부여야 합니다.
이 메서드가 할 일은 메일을 보내는 거지 뭘 어떻게 누구한테 보내는 걸 결정하는 게 아니라고 생각합니다.

그런데 2) 어떤 내용 보낼지를 내부 구현에서 결정하고 있습니다. 해당 부분에 대한 개선이 필요해보입니다

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다!

import org.springframework.transaction.annotation.Transactional;

@Service
@Transactional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 컨벤션 상으로는 클래스 레벨에서는 readOnly 걸고 메서드에서 필요하면 다시 기본옵션으로 달아주는 쪽으로 작성하고 있습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다

Copy link
Member Author

@binary-ho binary-ho Feb 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 작성해주신 코드들을 읽으면서, Transactionsal이 readOnly가 필요한 public 메서드가 많고, 전역적인 Transactonal을 선언할 때 분명히 편한 부분이 있음에 공감했었습니다.

그러나 이 서비스의 경우 단 하나의 public method가 존재하고, readOnly 옵션이 필요하지 않은 Transactional이 필요한데 똑같이 적용하는 것이 괜찮은 것인지 고민됩니다.

저희의 경우엔 그런 케이스가 없었지만, 다른 프로젝트 하면서 Transactional이 필요한 곳임에도 깜빡하고 달지 않는 분들을 본 적이 있었습니다. 그러면서 직접적으로 Transactional이 필요한 메서드에 어노테이션을 붙여주면서 명시적으로 "이 메서드에서 어떤 트랜잭션이 필요하다고" 표시하는 것을 선호하기도 했었습니다. ("어떤"이라는 표현은readOnly 여부와 관련)

혹시 전역전인 Transactional과 readOnly true 옵션을 컨벤션으로 지정하게 된 배경을 들려주실 수 있을까요?
고치지 않겠다는 것은 아닙니다! 컨벤션인 만큼 말씀 주신 대로 변경하겠습니다 ㅎㅎ
.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readOnly = true를 클래스 레벨에서 선언할 것인가 메서드 레벨에서 선언할 것인가에 대한 이야기로 이해하면 될까요?

결국 메서드 레벨에서 @Transactional(readOnly = true) 다는 것을 실수하는 경우와 @Transactional 다는 것을 실수하는 경우에 대한 문제인 것 같습니다. 전자를 실수하면 조회 성능이 나빠지고 후자를 실수하면 쓰기 지연이 발생하지 않겠네요.

후자의 경우 테스트로 검증이 가능하지만 전자의 경우 검증이 불가능하다는 점에서 클래스 레벨에서는 @Transactional(readOnly = true) 을 달아주고 쓰기 (CUD) 작업인 경우 @Transaction 을 달아주는 방식을 사용하게 되는 것 같습니다.

그리고 메서드가 하나인 클래스라 하더라도 이러한 컨벤셔널한 부분은 통일되어야 한다고 생각합니다.
만약 우리가 readOnly 클래스 레벨에 달아주기로 해놓고 여기서만 그렇게 하지 않는다면,
추후 먼 미래 해당 클래스에 메서드가 추가되는 경우 기존 컨벤션에 적응되 작업자가 실수할 수도 있습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다 컨벤션에 맞게 수정해보겠습니다~
(저는 Transactional 자체를 클래스 레벨에 달 것이냐, 메서드 레벨에 달 것이냐에 대한 이야기였습니다!)

UNIV_NOT_VERIFIED(HttpStatus.CONFLICT, "재학생 인증이 되지 않았습니다."),

// Univ Email Verification
UNIV_EMAIL_ALREADY_VERIFIED(HttpStatus.CONFLICT, "이미 가입된 Univ Email 입니다."),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Univ email -> 학교 이메일로 변경하는 게 좋아보입니다

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다 감사합니다!

@@ -0,0 +1,3 @@
package com.gdschongik.gdsc.global.util;

public record Pair<T>(String key, T value) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map.Entry 를 고려해보시기 바랍니다. 패키지도 잘못되어 있는 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map의 Entry는 정말 Map의 내부에서 사용될 Entry로써 사용될 떄 사용하는 것이 옳다고 생각해서 직접 정의해 보았습니다! 결국 사용방법은 동일하지만, 그 의미가 다르다고 생각되서 사용을 꺼렸습니다.

혹시 Map.Entry 사용 고려를 권하신 이유를 들려주실 수 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

패키지는 어디에 두는 것이 좋을까요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map.Entry의 사용의미에 대해서 잘못 알고 계신 것 같습니다. 해당 클래스는 key-value 단일 쌍을 위해서도 사용될 수 있습니다.

굳이 자료구조를 새로 만들어서 관리 포인트를 늘릴 필요가 없습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 말씀주신 의미로 사용해도 좋다는 관련 자료가 있을까요??

Copy link
Member

@uwoobeat uwoobeat Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요약

  • key-value single pair에 대해 AbstractMap.SimpleEntry 를 사용할 것을 제안하는 여러 레퍼런스가 있습니다.
  • Map 내부의 Entry를 사용하기 싫다면 Apache Commons Lang 의존성을 추가해서 Pair 클래스를 사용하면 됩니다.
  • 외부 의존성을 추가하기 싫다면 그냥 Map.Entry 구현체를 쓰면 됩니다.
  • Pair 클래스는 객체지향적으로 좋지 않을 수도 있습니다.

본문

https://www.baeldung.com/java-pairs

먼저 질문에 대한 답부터 드리자면...
위 글에는 자바에서 key-value 쌍을 처리하기 위해서 AbstractMap.SimpleEntry 를 사용할 수 있다는 것을 소개하고 있습니다.

https://stackoverflow.com/questions/3110547/java-how-to-create-new-entry-key-value
https://stackoverflow.com/questions/2973041/a-keyvaluepair-in-java

그리고 위의 2개의 스택오버플로우 글은 진호님과 같은 생각을 한 사람들을 위한 글입니다.
2번째 레퍼런스의 댓글을 보시면 어느정도 답을 얻을 수 있습니다.

I prefer commons Pair personally - doesn't seem right to have to dig for a class inside of AbstractMap. But if you don't want to depend on commons lang for some reason, I could see that.

개인적으로는 Commons Pair를 선호합니다 - AbstractMap 내부에서 클래스를 찾아야 한다는 것이 올바르지 않은 것 같습니다. 하지만 어떤 이유로든 Commons Lang에 의존하고 싶지 않다면, 그것도 이해할 수 있습니다.

말씀하신대로 Entry가 Map에 종속적으로 사용될 것을 의도하고 만들어졌기에, Map과 무관하게 Entry를 사용하는 것에 불편함을 느낄 수도 있습니다. 이럴 경우 org.apache.commons.lang3.tuple 패키지의 pair 을 사용하면 됩니다. 어느 쪽이든 직접 구현하는 편보다는 낫습니다. 유지보수 포인트를 늘리는 것보다는 외부 의존성을 늘리는 것이 낫다고 생각합니다. 그리고 더 나아가, 외부 의존성을 늘리는 편보다는 자바의 기본 패키지 (java.util) 의 Map.Entry 를 사용하는 편이 낫습니다.

제 생각은 어떻게 믿든 Map.Entry가 낫다는 결론입니다.

그리고 Pair의 사용이 객체지향적으로 좋지 않다는 레퍼런스가 있습니다.
하지만 최근 라이브러리들은 Pair 구현체를 제공하는 편이긴 합니다. 심지어 안드로이드도 자체적으로 Pair 클래스를 제공합니다.
옛날 글이라는 점은 감안해주세요.

Google Guava에서는 Pair 도입이 그닥 좋지 않다고 이야기했습니다.
https://groups.google.com/g/guava-discuss/c/GF4QyBu0gsI

직접 정의한 Pair 클래스의 단점을 제네릭과 타입 안정성 관점에서 접근한 글입니다.
https://james-iry.blogspot.com/2010/05/anatomy-of-annoyance.html

Copy link
Member Author

@binary-ho binary-ho Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오! 첫번째 래퍼런스는 저도 이번에 구현하는 과정에서 봤던건데 반갑네요.

  1. 저 글에서는 Map의 Entry를 사용하는 법만 알려주고 맥락적으로 왜 사용해도 되는지 이유를 알려주지는 않는 것 같아요.. 아니면 같은 글에 등장하는 대안 5번과 같은 사용은 어떨까요? 지금의 Pair를 PropertyPair로 이름을 바꿔서 사용해 보는 것은 어떻게 생각하시나요?
  2. 사실 현재 Property를 위한 용도의 필드 2개인 record인 Pair가 유지보수 포인트를 많이 늘리는 지는 조금 와닿지 않는 것 같습니다... Wrapper Class에 가까운 record class 하나가 추가 되는 상황보다는 외부 의존성을 늘리는 것이 낫다고 생각하시는 부분은 이유를 여쭈어 봐도 될까요? js의 유명한 util용 라이브러리 lodash의 퇴출은 예전 부터 논쟁거리였습니다 You-Dont-Need-Lodash-Underscore 퇴출을 원하는 사람들은 대부분 작은 기능을 위해 라이브러리를 사용하면서 여러 문제를 마주하지 말고, 기본적인 언어의 기능을 활용하자는 것이 의견입니다.
  3. 그렇다고 Apache Commons Lang의 Pair 클래스를 쓰기 꺼리는 것은 아닙니다. 이걸 사용해볼까요?
  4. 또한 Pair가 객체지향적으로 좋지 않다는 점은 매우 동의합니다.
  5. 최하단의 글은 정말 재미있는 글이지만 저희의 케이스는 조금 다른 것 같습니다.. 활용처가 매우 다양하고 범용적인 Pair를 정의하면서 생긴 다양한 문제로 느껴집니다. 저희의 경우에는 Property 값을 ConfigurationProperties을 통해 가져올 때만 사용하고 있습니다. 물론 제가 이름을 그렇게 짓지 않았습니다.. 차라리 그런 의미를 가진 PropetyPair로 이름을 바꾸고 사용함은 어떨 것 같나요??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1-1)

지금 초점이 "왜 사용해야 하는지"에 대한 이야기인데, 반대로 "왜 사용하면 안되는지" 에 대해서 이야기해야 한다고 생각합니다. 사용하지 말아야 하는 것은 보통 사용하지 못하도록 접근 제어자 등을 통해 제한하고 있습니다. 만약 저희가 사용하지 말아야 하는 private 모듈을 reflection 등을 통해 강제로 사용하고 있다면 "왜 사용해야 하는지"에 대해서 이야기 하는 것이 맞습니다. 하지만 SimpleEntry 는 엄연히 public으로 열려있는 클래스이고 이것을 사용하지 않아야 한다면, 왜 그렇게 하지 말아야 하는지에 대한 레퍼런스를 찾아오시는게 논리적으로 맞습니다.

(1-2)

레퍼런스의 5번 솔루션에서는 이렇게 이야기하고 있습니다.

사용자 선호에 따라 또는 앞서 언급한 라이브러리가 없는 경우 쌍 기능에 대한 표준 해결 방법은 원하는 반환 값을 래핑하는 간단한 컨테이너 클래스를 만드는 것입니다.

이미 저희의 목적성에 맞는 라이브러리는 존재하고, 따라서 커스텀 컨테이너를 만들어야 한다면 그 이유는 개인적인 선호가 되어야 합니다. 그리고 제 개인적인 선호로는 "이미 존재하는 자료구조를 사용하지 않을 이유는 없다" 입니다.

(2)

제가 최종적으로 하고 싶은 말이 "외부 의존성을 늘리자" 가 아닙니다.
유지보수 포인트를 늘리는 것보다는 외부 의존성을 활용하는 편이 낫고, 외부 의존성을 늘리는 것보다는 이미 자바에 존재하는 기능을 활용하는 것이 낫다, 라는 것입니다.

대부분 작은 기능을 위해 라이브러리를 사용하면서 여러 문제를 마주하지 말고, 기본적인 언어의 기능을 활용하자는 것이 의견입니다.

제가 하고 싶은 말입니다. 자바 유틸 패키지의 Map.Entry 를 '절대' 사용하지 말아야 할 이유가 있나요? 이미 존재하는 솔루션을 회피하면서까지 새로운 컨테이너 클래스를 만들어야 할 이유가 있나요?

"메일에 들어갈 프로퍼티를 yml로부터 읽어오는" 그 한가지 역할만을 수행하는 자료구조를 만들어야 하는 필요성이 있나요?
재사용성이 없는, 오직 한 가지 usage만을 가지는 자료구조란 의미가 있나요?

(3)

마찬가지로 SimpleEntry 를 사용하지 않으면서까지 외부 의존성을 늘리는 것은 옳지 않다고 생각합니다.

(5)

(3)에서 말했던 것처럼 동의하지 않습니다.

  • "오직 한 가지 usage만을 가지는 자료구조" 를 만들고 싶지 않습니다. 다른 대안을 찾을 것 같습니다.
  • 설사 PropertyPair 가 "메일 프로퍼티를 yml로부터 읽어온다는" 단 한 가지의 사용목적만을 가진다 하더라도, 다른 개발자에게 PropertyPair 을 사용하지 않도록 강제할 수 있는 방법이 존재하나요? 최하단의 글처럼, 진호님이 만드신 PropertyPair 가 범용적인 Pair가 되지 않을 것이라는 보장이 있나요? 혹은, 그렇게 되지 않도록 강제할 수 있는 방법이 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일단 Pair 삭제하고 Map 활용하는 쪽으로 구현 변경했습니다

제가 방법을 못 찾았는지 몰라도 @ConfigurationProperties를 통해 값을 Map.Entry나 SimpleEntry, Commons Pair에 주입하는 방법을 찾지 못했습니다. 차라리 이름을 명시하기 위해 사용했던 Wrapper 클래스들을 전부 제거하고 Properties들을 받는 Map을 통해 yml 값들을 받아왔습니다.

import org.springframework.boot.context.properties.ConfigurationProperties;

@ConfigurationProperties(prefix = "email")
public record EmailProperty(Gmail gmail, String host, int port, String encoding, JavaMailProperty javaMailProperty) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

프로퍼티 내부 구성요소들은 inner record 클래스로 선언하여 사용 가능합니다. (참고링크)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다!


@Component
@RequiredArgsConstructor
public class HongikUnivEmailValidator {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

email 패키지의 유틸리티와 global 패키지의 email 유틸리티의 분류 기준이 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정말 "재학생 인증 메일"과 무관한 email에 관한 요소들을 globals에, "재학생 인증 메일과"유관한 요소들을 domain email 패키지에 두었습니다.

"재학생 인증 메일"이라는 context에 종속된 요소들이라도, global에 있는게 좋다고 생각하실까요??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일관된 분류기준만 있다면 무엇이든 상관없습니다.

그리고 HongikUnivEmailValidator는 "재학생 인증 메일" 의 컨텍스트와 무관하다고 생각하시는지, 그렇다면 왜 그렇게 생각하셨는지 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HongikUnivEmailValidator는 잘못 놓았네요..
이메일과 관련된 util 객체들은 global로 옮겨두겠습니다!

private final EmailProperty emailProperty;

@Bean
public JavaMailSender javaMailSenderBean() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빈 네이밍 컨벤션을 지키는 것이 좋아보입니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다!

@RestController
@RequestMapping("/onboarding/verify-email")
@RequiredArgsConstructor
public class UnivEmailVerificationController {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨트롤러는 엔드포인트 단위로 구분되는 것이 좋지 사용 서비스에 따라서 구별할 필요는 없다고 봅니다.
현재 시점에서 굳이 성급하게 분리할 필요가 없을 것 같습니다.

@RestController
@RequestMapping("/onboarding/verify-email")
@RequiredArgsConstructor
public class UnivEmailVerificationController {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이러한 방식으로 컨트롤러 클래스를 구성하게 되면, 모든 이메일 관련 API에 대한 컨트롤러 클래스를 만들어줘야 합니다. 불필요하게 복잡해지는 문제가 생긴다는 겁니다.

컨트롤러를 쪼개야 하는 경우는 하나의 컨트롤러에 너무 많은 API가 존재해서, 서로 다른 도메인으로 분리해내야 하는 경우입니다. 필요해질 때 분리하고, 그렇지 않을 때는 합치는 것이 관리 포인트를 줄일 수 있는 방법입니다.

https://careerly.co.kr/comments/66030?utm_campaign=user-share

@RestController
@RequestMapping("/onboarding/verify-email")
@RequiredArgsConstructor
public class UnivEmailVerificationController {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

비슷한 맥락에서 컨텍스트 분리 전까지는 EmailController 와 같이 "이메일" 이라는 컨텍스트로만 관리해주고
그 외 클래스들도 의미만 잘 드러나는 선에서만 네이밍해주고 좀 과도하게 상세해진 부분은 수정하는 게 어떨까 싶습니다
가령 UnivEmail -> Email 같이요. '재학생' 이메일 인증과 그냥 이메일 인증이라는 컨텍스트를 구분할 필요성이 없기 때문에 저는 굳이? 싶다는 생각입니다. 다만 이 부분은 크리티컬하지는 않아서 그냥 Univ 붙여쓰든 안붙여쓰든 크게 문제 없을 것 같아요. 의미만 충분하다면 네이밍을 굳이 길게 쓸 필요는 없다는 게 제 생각이어서요


private void saveUnivEmailVerification(String univEmail, String verificationCode) {
Long currentMemberId = memberUtil.getCurrentMemberId();
UnivEmailVerification univEmailVerification = new UnivEmailVerification(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 이미 DiscordVerificationCode 를 사용하고 있어서 비슷하게 EmailVerificationCode 를 쓰면 될 것 같습니다.
  • 현재 엔티티 생성 시 정적 팩토리 메서드를 사용하도록 컨벤션을 잡고 있어서, 생성자 쓸 때 참고해주세요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다!

import org.springframework.transaction.annotation.Transactional;

@Service
@Transactional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readOnly = true를 클래스 레벨에서 선언할 것인가 메서드 레벨에서 선언할 것인가에 대한 이야기로 이해하면 될까요?

결국 메서드 레벨에서 @Transactional(readOnly = true) 다는 것을 실수하는 경우와 @Transactional 다는 것을 실수하는 경우에 대한 문제인 것 같습니다. 전자를 실수하면 조회 성능이 나빠지고 후자를 실수하면 쓰기 지연이 발생하지 않겠네요.

후자의 경우 테스트로 검증이 가능하지만 전자의 경우 검증이 불가능하다는 점에서 클래스 레벨에서는 @Transactional(readOnly = true) 을 달아주고 쓰기 (CUD) 작업인 경우 @Transaction 을 달아주는 방식을 사용하게 되는 것 같습니다.

그리고 메서드가 하나인 클래스라 하더라도 이러한 컨벤셔널한 부분은 통일되어야 한다고 생각합니다.
만약 우리가 readOnly 클래스 레벨에 달아주기로 해놓고 여기서만 그렇게 하지 않는다면,
추후 먼 미래 해당 클래스에 메서드가 추가되는 경우 기존 컨벤션에 적응되 작업자가 실수할 수도 있습니다.

public class UnivEmailVerificationConstant {

public static final String VERIFY_EMAIL_API_ENDPOINT = "/onboarding/verify-email?%s=";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상수 클래스 멤버 개행 관련해서 스타일 컨벤션 한번만 체크해주세요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다!

@@ -0,0 +1,3 @@
package com.gdschongik.gdsc.global.util;

public record Pair<T>(String key, T value) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map.Entry의 사용의미에 대해서 잘못 알고 계신 것 같습니다. 해당 클래스는 key-value 단일 쌍을 위해서도 사용될 수 있습니다.

굳이 자료구조를 새로 만들어서 관리 포인트를 늘릴 필요가 없습니다.


@Component
@RequiredArgsConstructor
public class HongikUnivEmailValidator {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일관된 분류기준만 있다면 무엇이든 상관없습니다.

그리고 HongikUnivEmailValidator는 "재학생 인증 메일" 의 컨텍스트와 무관하다고 생각하시는지, 그렇다면 왜 그렇게 생각하셨는지 궁금합니다.

Copy link
Member

@uwoobeat uwoobeat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@binary-ho binary-ho merged commit 500216f into develop Feb 25, 2024
1 check passed
@binary-ho binary-ho deleted the feature/28-email-verification branch February 25, 2024 13:43
@binary-ho binary-ho restored the feature/28-email-verification branch February 25, 2024 15:23
@binary-ho binary-ho deleted the feature/28-email-verification branch February 26, 2024 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature 새로운 기능 추가 및 수정
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ 이메일 인증 기능 구현
3 participants