๐ Email Send #27
๐ Email Send #27
Conversation
Walkthrough์ด ๋ณ๊ฒฝ ์ฌํญ์ Spring Boot ํ๋ก์ ํธ์ ์ด๋ฉ์ผ ์ธ์ฆ ๊ธฐ๋ฅ์ ์ถ๊ฐํฉ๋๋ค. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthController
participant MailService
participant JavaMailSender
participant RateLimiter
Client->>AuthController: Request email verification
AuthController->>RateLimiter: checkRateLimit(email)
RateLimiter-->>AuthController: Rate limit check
AuthController->>MailService: sendMail(email)
MailService->>MailService: createVerificationCode()
MailService->>JavaMailSender: Send verification email
MailService-->>AuthController: Return verification code
AuthController-->>Client: Return verification code
Client->>AuthController: Verify code
AuthController->>MailService: verifyCode(email, code)
MailService-->>AuthController: Return verification result
AuthController-->>Client: Return verification result
Possibly related PRs
Suggested labels
Poem
๐ชง TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
๐งน Nitpick comments (1)
build.gradle (1)
62-62: ๋ฉ์ผ ์์กด์ฑ ๋ฒ์ ๊ด๋ฆฌ ๊ฐ์ ํ์Spring Boot Mail Starter์ ๋ฒ์ ์ด ๋ช ์๋์ด ์์ง ์์ต๋๋ค. ์์กด์ฑ ๊ด๋ฆฌ๋ฅผ ์ํด ๋ค์๊ณผ ๊ฐ์ด ์์ ํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค:
- implementation 'org.springframework.boot:spring-boot-starter-mail' + implementation 'org.springframework.boot:spring-boot-starter-mail:3.3.4'
๐ Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (4)
build.gradle(1 hunks)src/main/java/com/mycom/socket/auth/controller/AuthController.java(2 hunks)src/main/java/com/mycom/socket/auth/service/AuthService.java(0 hunks)src/main/java/com/mycom/socket/auth/service/MailService.java(1 hunks)
๐ค Files with no reviewable changes (1)
- src/main/java/com/mycom/socket/auth/service/AuthService.java
| public MimeMessage CreateMail(String mail) { | ||
| createNumber(); | ||
| MimeMessage message = javaMailSender.createMimeMessage(); | ||
|
|
||
| try { | ||
| message.setFrom(senderEmail); | ||
| message.setRecipients(MimeMessage.RecipientType.TO, mail); | ||
| message.setSubject("์ด๋ฉ์ผ ์ธ์ฆ"); | ||
| String body = ""; | ||
| body += "<h3>" + "์์ฒญํ์ ์ธ์ฆ ๋ฒํธ์ ๋๋ค." + "</h3>"; | ||
| body += "<h1>" + number + "</h1>"; | ||
| body += "<h3>" + "๊ฐ์ฌํฉ๋๋ค." + "</h3>"; | ||
| message.setText(body,"UTF-8", "html"); | ||
| } catch (MessagingException e) { | ||
| e.printStackTrace(); | ||
| } | ||
|
|
||
| return message; | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ด๋ฉ์ผ ์์ฑ ๋ก์ง ๊ฐ์ ํ์
๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ด ํ์ํฉ๋๋ค:
- ์์ธ ์ฒ๋ฆฌ๊ฐ ๋ฏธํกํฉ๋๋ค.
- ์ด๋ฉ์ผ ์ฃผ์ ์ ํจ์ฑ ๊ฒ์ฆ์ด ์์ต๋๋ค.
- HTML ํ ํ๋ฆฟ ๋ถ๋ฆฌ๊ฐ ํ์ํฉ๋๋ค.
public MimeMessage CreateMail(String mail) {
+ if (!isValidEmail(mail)) {
+ throw new InvalidEmailException("์ ํจํ์ง ์์ ์ด๋ฉ์ผ ์ฃผ์์
๋๋ค.");
+ }
- createNumber();
+ int verificationCode = createVerificationCode();
+ verificationCodes.put(mail, verificationCode);
MimeMessage message = javaMailSender.createMimeMessage();
try {
message.setFrom(senderEmail);
message.setRecipients(MimeMessage.RecipientType.TO, mail);
message.setSubject("์ด๋ฉ์ผ ์ธ์ฆ");
- String body = "";
- body += "<h3>" + "์์ฒญํ์ ์ธ์ฆ ๋ฒํธ์
๋๋ค." + "</h3>";
- body += "<h1>" + number + "</h1>";
- body += "<h3>" + "๊ฐ์ฌํฉ๋๋ค." + "</h3>";
+ String body = emailTemplateService.getVerificationEmailTemplate(verificationCode);
message.setText(body,"UTF-8", "html");
} catch (MessagingException e) {
- e.printStackTrace();
+ throw new EmailSendException("์ด๋ฉ์ผ ์์ฑ ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค.", e);
}
return message;
}Committable suggestion skipped: line range outside the PR's diff.
| // ๋๋ค์ผ๋ก ์ซ์ ์์ฑ | ||
| public static void createNumber() { | ||
| number = (int)(Math.random() * (90000)) + 100000; //(int) Math.random() * (์ต๋๊ฐ-์ต์๊ฐ+1) + ์ต์๊ฐ | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ธ์ฆ ๋ฒํธ ์์ฑ ๋ก์ง ๊ฐ์ ํ์
์ธ์ฆ ๋ฒํธ ์์ฑ ๋ฉ์๋์ ๋ค์๊ณผ ๊ฐ์ ๋ฌธ์ ๊ฐ ์์ต๋๋ค:
- ์ ์ ๋ฉ์๋๋ก ๊ตฌํ๋์ด ํ ์คํธ๊ฐ ์ด๋ ต์ต๋๋ค.
- ๋ณด์์ฑ์ด ๋ฎ์ ๋์ ์์ฑ ๋ฐฉ์์ ์ฌ์ฉํฉ๋๋ค.
๋ค์๊ณผ ๊ฐ์ด ๊ฐ์ ํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค:
- public static void createNumber() {
- number = (int)(Math.random() * (90000)) + 100000;
+ private int createVerificationCode() {
+ SecureRandom secureRandom = new SecureRandom();
+ return 100000 + secureRandom.nextInt(900000);Committable suggestion skipped: line range outside the PR's diff.
| private static final String senderEmail= "[email protected]"; | ||
| private static int number; |
There was a problem hiding this comment.
์ ์ ๋ณ์ ์ฌ์ฉ ๋ฐ ์ด๋ฉ์ผ ์ฃผ์ ํ๋์ฝ๋ฉ ๋ฌธ์
๋ค์๊ณผ ๊ฐ์ ์ฌ๊ฐํ ๋ฌธ์ ์ ๋ค์ด ์์ต๋๋ค:
number๋ฅผ ์ ์ ๋ณ์๋ก ์ฌ์ฉํ๋ฉด ๋์์ฑ ๋ฌธ์ ๊ฐ ๋ฐ์ํ ์ ์์ต๋๋ค.- ์ด๋ฉ์ผ ์ฃผ์๊ฐ ์ฝ๋์ ํ๋์ฝ๋ฉ๋์ด ์์ต๋๋ค.
๋ค์๊ณผ ๊ฐ์ด ์์ ํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค:
- private static final String senderEmail= "[email protected]";
- private static int number;
+ @Value("${mail.sender.address}")
+ private String senderEmail;
+ private final Map<String, Integer> verificationCodes = new ConcurrentHashMap<>();Committable suggestion skipped: line range outside the PR's diff.
| public int sendMail(String mail) { | ||
| MimeMessage message = CreateMail(mail); | ||
| javaMailSender.send(message); | ||
|
|
||
| return number; | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ด๋ฉ์ผ ์ ์ก ๋ฉ์๋ ๊ฐ์ ํ์
๋ค์๊ณผ ๊ฐ์ ๋ณด์ ๋ฐ ์์ ์ฑ ๊ฐ์ ์ด ํ์ํฉ๋๋ค:
- ์๋ ์ ํ(rate limiting) ๊ตฌํ์ด ํ์ํฉ๋๋ค.
- ์ด๋ฉ์ผ ์ ์ก ์คํจ ์ ์์ธ ์ฒ๋ฆฌ๊ฐ ๋ฏธํกํฉ๋๋ค.
public int sendMail(String mail) {
+ rateLimiter.checkRateLimit(mail);
MimeMessage message = CreateMail(mail);
- javaMailSender.send(message);
+ try {
+ javaMailSender.send(message);
+ return verificationCodes.get(mail);
+ } catch (MailException e) {
+ throw new EmailSendException("์ด๋ฉ์ผ ์ ์ก ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค.", e);
+ }
- return number;
}Committable suggestion skipped: line range outside the PR's diff.
| private final MailService mailService; | ||
| private int number; |
There was a problem hiding this comment.
์ปจํธ๋กค๋ฌ์ ์ํ ์ ์ฅ ๋ฐฉ์ ๊ฐ์ ํ์
์ธ์ฆ ๋ฒํธ๋ฅผ ์ปจํธ๋กค๋ฌ ์ธ์คํด์ค ๋ณ์๋ก ์ ์ฅํ๋ ๊ฒ์ ๋ค์๊ณผ ๊ฐ์ ๋ฌธ์ ๊ฐ ์์ต๋๋ค:
- ์ฌ๋ฌ ์ฌ์ฉ์์ ์ธ์ฆ ์์ฒญ์ ์ฒ๋ฆฌํ ์ ์์ต๋๋ค.
- ์๋ฒ๊ฐ ์ฌ๋ฌ ์ธ์คํด์ค๋ก ์คํ๋ ๊ฒฝ์ฐ ๋์ํ์ง ์์ต๋๋ค.
์ธ์ฆ ๋ฒํธ ์ ์ฅ์๋ฅผ ์๋น์ค ๊ณ์ธต์ผ๋ก ์ด๋ํ๊ณ Redis์ ๊ฐ์ ๋ถ์ฐ ์บ์๋ฅผ ์ฌ์ฉํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค.
- private int number;๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private final MailService mailService; | |
| private int number; | |
| private final MailService mailService; |
| @GetMapping("/mail-check") | ||
| public Boolean mailCheck(@RequestParam(name = "userNumber") String userNumber) { | ||
| return userNumber.equals(String.valueOf(number)); | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ธ์ฆ ๋ฒํธ ํ์ธ ์๋ํฌ์ธํธ ๊ฐ์ ํ์
๋ค์๊ณผ ๊ฐ์ ๋ณด์ ๋ฐ ๊ธฐ๋ฅ ๊ฐ์ ์ด ํ์ํฉ๋๋ค:
- ์๋ต ํ์์ด ํ์คํ๋์ด ์์ง ์์ต๋๋ค.
- ์ธ์ฆ ์๋ ํ์ ์ ํ์ด ์์ต๋๋ค.
- ์ธ์ฆ ๋ง๋ฃ ์๊ฐ ์ค์ ์ด ์์ต๋๋ค.
@GetMapping("/mail-check")
- public Boolean mailCheck(@RequestParam(name = "userNumber") String userNumber) {
- return userNumber.equals(String.valueOf(number));
+ public ApiResponse<VerificationResult> verifyCode(
+ @RequestParam(name = "mail") @Email String mail,
+ @RequestParam(name = "code") @Pattern(regexp = "^[0-9]{6}$") String code) {
+ boolean isValid = mailService.verifyCode(mail, code);
+ return ApiResponse.success(new VerificationResult(isValid));
}Committable suggestion skipped: line range outside the PR's diff.
| @PostMapping("/mail-send") | ||
| public Integer mailSend(@RequestParam(name = "mail") String mail) { | ||
| try { | ||
| number = mailService.sendMail(mail); | ||
| return number; | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to send mail: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ด๋ฉ์ผ ์ ์ก ์๋ํฌ์ธํธ ๊ฐ์ ํ์
๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ด ํ์ํฉ๋๋ค:
- ์๋ต ํ์์ด ํ์คํ๋์ด ์์ง ์์ต๋๋ค.
- ์ ๋ ฅ๊ฐ ๊ฒ์ฆ์ด ๋ถ์กฑํฉ๋๋ค.
- ์์ธ ์ฒ๋ฆฌ๊ฐ ๊ตฌ์ฒด์ ์ด์ง ์์ต๋๋ค.
@PostMapping("/mail-send")
- public Integer mailSend(@RequestParam(name = "mail") String mail) {
+ public ApiResponse<VerificationResponse> mailSend(
+ @RequestParam(name = "mail") @Email(message = "์ ํจํ ์ด๋ฉ์ผ ์ฃผ์๋ฅผ ์
๋ ฅํด์ฃผ์ธ์") String mail) {
try {
- number = mailService.sendMail(mail);
- return number;
+ int verificationCode = mailService.sendMail(mail);
+ return ApiResponse.success(new VerificationResponse(verificationCode));
} catch (Exception e) {
- throw new RuntimeException("Failed to send mail: " + e.getMessage());
+ throw new EmailServiceException("์ด๋ฉ์ผ ์ ์ก์ ์คํจํ์ต๋๋ค", e);
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 3
๐งน Nitpick comments (2)
src/main/java/com/mycom/socket/auth/service/RateLimiter.java (1)
22-34: ์์ธ ์ฒ๋ฆฌ ๊ฐ์ ํ์ํ์ฌ ๊ตฌํ์ ์ผ๋ฐ์ ์ธ
BaseException์ ์ฌ์ฉํ๊ณ ์์ต๋๋ค. ๋ ๊ตฌ์ฒด์ ์ธ ์์ธ ํด๋์ค๋ฅผ ์ฌ์ฉํ๋ฉด ํด๋ผ์ด์ธํธ๊ฐ ๋ ์ ์ ํ๊ฒ ๋์ํ ์ ์์ต๋๋ค.๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ ์ ์ํฉ๋๋ค:
+public class RateLimitExceededException extends BaseException { + public RateLimitExceededException(String message) { + super(message, HttpStatus.TOO_MANY_REQUESTS); + } +} if (requests.size() >= MAX_REQUESTS) { - throw new BaseException("๋๋ฌด ๋ง์ ์์ฒญ์ ๋๋ค. ์ ์ ํ ๋ค์ ์๋ํด์ฃผ์ธ์.", HttpStatus.TOO_MANY_REQUESTS); + throw new RateLimitExceededException("๋๋ฌด ๋ง์ ์์ฒญ์ ๋๋ค. ์ ์ ํ ๋ค์ ์๋ํด์ฃผ์ธ์."); }src/main/java/com/mycom/socket/auth/service/MailService.java (1)
44-49: ์ด๋ฉ์ผ ํ ํ๋ฆฟ ์ธ๋ถํ ํ์HTML ์ด๋ฉ์ผ ํ ํ๋ฆฟ์ด ์ฝ๋์ ํ๋์ฝ๋ฉ๋์ด ์์ต๋๋ค. ์ ์ง๋ณด์์ ๋ค๊ตญ์ด ์ง์์ ์ํด ์ธ๋ถ ํ์ผ๋ก ๋ถ๋ฆฌํด์ผ ํฉ๋๋ค.
resources/templates/email/verification.htmlํ์ผ์ ์์ฑํ๊ณ Thymeleaf ํ ํ๋ฆฟ ์์ง์ ์ฌ์ฉํ๋ ๊ฒ์ ์ ์ํฉ๋๋ค:-String body = String.format(""" - <h3>์์ฒญํ์ ์ธ์ฆ ๋ฒํธ์ ๋๋ค.</h3> - <h1>%d</h1> - <h3>๊ฐ์ฌํฉ๋๋ค.</h3> - """, verificationCode); -message.setText(body, "UTF-8", "html"); +Context context = new Context(); +context.setVariable("code", verificationCode); +String body = templateEngine.process("email/verification", context); +message.setText(body, "UTF-8", "html");
๐ Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (3)
src/main/java/com/mycom/socket/auth/controller/AuthController.java(2 hunks)src/main/java/com/mycom/socket/auth/service/MailService.java(1 hunks)src/main/java/com/mycom/socket/auth/service/RateLimiter.java(1 hunks)
๐ Additional comments (2)
src/main/java/com/mycom/socket/auth/controller/AuthController.java (2)
41-49: ๐ ๏ธ Refactor suggestion์ ๋ ฅ ์ ํจ์ฑ ๊ฒ์ฆ ๋ฐ ์์ธ ์ฒ๋ฆฌ ๊ฐ์ ํ์
์ด๋ฉ์ผ ๊ฒ์ฆ ์๋ํฌ์ธํธ์ ๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ด ํ์ํฉ๋๋ค:
- ์ด๋ฉ์ผ ์ฃผ์ ํ์ ๊ฒ์ฆ
- ๊ตฌ์ฒด์ ์ธ ์์ธ ์ฒ๋ฆฌ
๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ ์ ์ํฉ๋๋ค:
@PostMapping("/email/verification") -public Integer mailSend(@RequestParam(name = "mail") String mail) { +public Integer mailSend( + @RequestParam(name = "mail") + @Email(message = "์ ํจํ ์ด๋ฉ์ผ ์ฃผ์๋ฅผ ์ ๋ ฅํด์ฃผ์ธ์") + String mail) { try { rateLimiter.checkRateLimit(mail); // ์์ฒญ ์ ํ ์ฒดํฌ return mailService.sendMail(mail); - } catch (Exception e) { - throw new BaseException("์ด๋ฉ์ผ ์ ์ก์ ์คํจํ์ต๋๋ค.", HttpStatus.BAD_REQUEST); + } catch (RateLimitExceededException e) { + throw e; + } catch (MessagingException e) { + throw new BaseException("์ด๋ฉ์ผ ์ ์ก์ ์คํจํ์ต๋๋ค: " + e.getMessage(), + HttpStatus.BAD_REQUEST); } }Likely invalid or redundant comment.
51-59: ๐ ๏ธ Refactor suggestion์ธ์ฆ ์ฝ๋ ๊ฒ์ฆ ๋ก์ง ๊ฐ์ ํ์
์ธ์ฆ ์ฝ๋ ๊ฒ์ฆ ์๋ํฌ์ธํธ์ ๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ด ํ์ํฉ๋๋ค:
- ์ธ์ฆ ์ฝ๋ ํ์ ๊ฒ์ฆ
- ์๋ต ํ์ ํ์คํ
๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ ์ ์ํฉ๋๋ค:
@GetMapping("/email/verify") -public Boolean mailCheck(@RequestParam(name = "mail") String mail, - @RequestParam(name = "code") String code) { +public ApiResponse<VerificationResult> mailCheck( + @RequestParam(name = "mail") @Email String mail, + @RequestParam(name = "code") @Pattern(regexp = "^[0-9]{6}$") String code) { try { - return mailService.verifyCode(mail, code); + boolean isValid = mailService.verifyCode(mail, code); + return ApiResponse.success(new VerificationResult(isValid)); } catch (Exception e) { throw new BaseException("์ธ์ฆ์ฝ๋ ๊ฒ์ฆ์ ์คํจํ์ต๋๋ค.", HttpStatus.BAD_REQUEST); } }Likely invalid or redundant comment.
| private final Map<String, List<LocalDateTime>> requestMap = new ConcurrentHashMap<>(); | ||
| private static final int MAX_REQUESTS = 3; // 1๋ถ๋น ์ต๋ 3๋ฒ | ||
| private static final Duration WINDOW_SIZE = Duration.ofMinutes(1); // 1๋ถ์ ์๊ฐ ๊ฐ๊ฒฉ |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
๋ฉ๋ชจ๋ฆฌ ๋์ ๊ฐ๋ฅ์ฑ ๊ฐ์ ํ์
requestMap์ ์ ์ฅ๋ ์ด๋ฉ์ผ ์ฃผ์์ ์์ฒญ ์๊ฐ์ด ์ง์์ ์ผ๋ก ๋์ ๋์ด ๋ฉ๋ชจ๋ฆฌ ๋์๊ฐ ๋ฐ์ํ ์ ์์ต๋๋ค. ์ฃผ๊ธฐ์ ์ผ๋ก ์ค๋๋ ๋ฐ์ดํฐ๋ฅผ ์ ๋ฆฌํ๋ ๋ฉ์ปค๋์ฆ์ด ํ์ํฉ๋๋ค.
๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ ์ ์ํฉ๋๋ค:
private final Map<String, List<LocalDateTime>> requestMap = new ConcurrentHashMap<>();
private static final int MAX_REQUESTS = 3; // 1๋ถ๋น ์ต๋ 3๋ฒ
private static final Duration WINDOW_SIZE = Duration.ofMinutes(1); // 1๋ถ์ ์๊ฐ ๊ฐ๊ฒฉ
+private static final Duration CLEANUP_INTERVAL = Duration.ofHours(1);
+
+@Scheduled(fixedRate = 3600000) // 1์๊ฐ๋ง๋ค ์คํ
+public void cleanup() {
+ LocalDateTime threshold = LocalDateTime.now().minus(WINDOW_SIZE);
+ requestMap.entrySet().removeIf(entry ->
+ entry.getValue().stream().allMatch(time -> time.isBefore(threshold)));
+}Committable suggestion skipped: line range outside the PR's diff.
| public boolean verifyCode(String email, String code) { | ||
| Integer savedCode = verificationCodes.get(email); | ||
| return savedCode != null && String.valueOf(savedCode).equals(code); | ||
| } |
There was a problem hiding this comment.
์ธ์ฆ ์ฝ๋ ๊ฒ์ฆ ๋ก์ง ๊ฐ์ ํ์
ํ์ฌ ๊ตฌํ์์๋ ์ธ์ฆ ์ฝ๋ ๋ง๋ฃ ์ฌ๋ถ๋ฅผ ํ์ธํ์ง ์์ต๋๋ค. ๋ํ, ๊ฒ์ฆ ํ์๋ ์ฝ๋๊ฐ ๊ณ์ ์ ํจํ ์ํ๋ก ๋จ์์์ต๋๋ค.
๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ ์ ์ํฉ๋๋ค:
public boolean verifyCode(String email, String code) {
- Integer savedCode = verificationCodes.get(email);
- return savedCode != null && String.valueOf(savedCode).equals(code);
+ VerificationData data = verificationCodes.get(email);
+ if (data == null || LocalDateTime.now().isAfter(data.expiryTime)) {
+ return false;
+ }
+ boolean isValid = String.valueOf(data.code).equals(code);
+ if (isValid) {
+ verificationCodes.remove(email); // ์ฌ์ฉ๋ ์ฝ๋ ์ ๊ฑฐ
+ }
+ return isValid;
}Committable suggestion skipped: line range outside the PR's diff.
| public class MailService { | ||
|
|
||
| private final JavaMailSender javaMailSender; | ||
| private final Map<String, Integer> verificationCodes = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
์ธ์ฆ ์ฝ๋ ๋ง๋ฃ ์๊ฐ ์ค์ ํ์
ํ์ฌ ๊ตฌํ์์๋ ์ธ์ฆ ์ฝ๋๊ฐ ๋ง๋ฃ๋์ง ์์ต๋๋ค. ๋ณด์์ ์ํด ๋ง๋ฃ ์๊ฐ์ ์ค์ ํด์ผ ํฉ๋๋ค.
๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ ์ ์ํฉ๋๋ค:
-private final Map<String, Integer> verificationCodes = new ConcurrentHashMap<>();
+private final Map<String, VerificationData> verificationCodes = new ConcurrentHashMap<>();
+
+@Value("${verification.code.expiry-minutes:5}")
+private int codeExpiryMinutes;
+
+private static class VerificationData {
+ private final int code;
+ private final LocalDateTime expiryTime;
+
+ public VerificationData(int code, LocalDateTime expiryTime) {
+ this.code = code;
+ this.expiryTime = expiryTime;
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
๐งน Nitpick comments (2)
src/main/java/com/mycom/socket/auth/service/RateLimiter.java (1)
23-28: cleanup ๋ฉ์๋์ ์คํ ์ฃผ๊ธฐ ์ค์ ๊ฐ์ ํ์ํ์ฌ ํ๋์ฝ๋ฉ๋ ์คํ ์ฃผ๊ธฐ(3600000)๋ฅผ ์ค์ ํ์ผ๋ก ๋ถ๋ฆฌํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
- @Scheduled(fixedRate = 3600000) // 1์๊ฐ๋ง๋ค ์คํ + @Value("${rate.limiter.cleanup.interval:3600000}") + private long cleanupInterval; + @Scheduled(fixedRateString = "${rate.limiter.cleanup.interval:3600000}") public void cleanup() {src/main/java/com/mycom/socket/auth/service/MailService.java (1)
37-42: ๋์ ์์ฑ ์ต์ ํ ํ์SecureRandom ์ธ์คํด์ค๋ฅผ ๋งค๋ฒ ์์ฑํ์ง ๋ง๊ณ ํด๋์ค ํ๋๋ก ์ ์ธํ์ฌ ์ฌ์ฌ์ฉํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
+ private final SecureRandom secureRandom = new SecureRandom(); + private int createVerificationCode() { - SecureRandom secureRandom = new SecureRandom(); return 100000 + secureRandom.nextInt(900000); }
๐ Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (3)
src/main/java/com/mycom/socket/GoSocketBeApplication.java(1 hunks)src/main/java/com/mycom/socket/auth/service/MailService.java(1 hunks)src/main/java/com/mycom/socket/auth/service/RateLimiter.java(1 hunks)
๐ Additional comments (4)
src/main/java/com/mycom/socket/GoSocketBeApplication.java (1)
7-8: ์ค์ผ์ค๋ง ๊ธฐ๋ฅ ํ์ฑํ ์ ์ ํ ๊ตฌํ๋จRateLimiter์ cleanup ์์ ์ ์ํ @EnableScheduling ์ด๋ ธํ ์ด์ ์ด ์ฌ๋ฐ๋ฅด๊ฒ ์ถ๊ฐ๋์์ต๋๋ค.
src/main/java/com/mycom/socket/auth/service/RateLimiter.java (1)
19-21: ๋์์ฑ ์ฒ๋ฆฌ๊ฐ ์ ๊ตฌํ๋์์ผ๋ ๋ฉ๋ชจ๋ฆฌ ๊ด๋ฆฌ ๊ฐ์ ํ์ConcurrentHashMap์ ์ฌ์ฉํ ๋์์ฑ ์ฒ๋ฆฌ๋ ์ ์ ํ๋, ์ด์ ๋ฆฌ๋ทฐ์์ ์ง์ ๋ ๋ฉ๋ชจ๋ฆฌ ๋์ ๋ฌธ์ ๊ฐ ์ฌ์ ํ ์กด์ฌํฉ๋๋ค.
src/main/java/com/mycom/socket/auth/service/MailService.java (2)
92-112: ๐ ๏ธ Refactor suggestion๊ฒ์ฆ ๋ก์ง ์ต์ ํ ํ์
๊ฒ์ฆ ๋ก์ง์ด ๋ ๊ฐ์ Map์ ์กฐํํ๊ณ ์์ด ๋นํจ์จ์ ์ ๋๋ค. ์ ์๋ VerificationData ๊ตฌ์กฐ๋ฅผ ์ฌ์ฉํ์ฌ ๊ฐ์ ํด์ผ ํฉ๋๋ค.
public boolean verifyCode(String email, String code) { - Integer savedCode = verificationCodes.get(email); - LocalDateTime expiryTime = expiryTimes.get(email); + VerificationData data = verificationData.get(email); - if (savedCode == null || - expiryTime == null || - LocalDateTime.now().isAfter(expiryTime)) { + if (data == null || LocalDateTime.now().isAfter(data.expiryTime())) { return false; } - boolean isValid = String.valueOf(savedCode).equals(code); + boolean isValid = String.valueOf(data.code()).equals(code); if (isValid) { - verificationCodes.remove(email); - expiryTimes.remove(email); + verificationData.remove(email); } return isValid; }Likely invalid or redundant comment.
75-84:โ ๏ธ Potential issue์ด๋ฉ์ผ ์ ์ก ์คํจ ์ฒ๋ฆฌ ๋๋ฝ
JavaMailSender.send() ๋ฉ์๋์ ์์ธ ์ฒ๋ฆฌ๊ฐ ํ์ํฉ๋๋ค.
public int sendMail(String mail) { rateLimiter.checkRateLimit(mail); MimeMessage message = createMail(mail); - javaMailSender.send(message); + try { + javaMailSender.send(message); + } catch (MailException e) { + throw new BaseException("์ด๋ฉ์ผ ์ ์ก ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค: " + e.getMessage(), + HttpStatus.INTERNAL_SERVER_ERROR, e); + }Likely invalid or redundant comment.
| public void checkRateLimit(String email) { | ||
| List<LocalDateTime> requests = requestMap.computeIfAbsent(email, k -> new ArrayList<>()); | ||
| LocalDateTime now = LocalDateTime.now(); | ||
|
|
||
| requests.removeIf(requestTime -> | ||
| requestTime.plus(WINDOW_SIZE).isBefore(now)); | ||
|
|
||
| if (requests.size() >= MAX_REQUESTS) { | ||
| throw new BaseException("๋๋ฌด ๋ง์ ์์ฒญ์ ๋๋ค. ์ ์ ํ ๋ค์ ์๋ํด์ฃผ์ธ์.", HttpStatus.TOO_MANY_REQUESTS); | ||
| } | ||
|
|
||
| requests.add(now); | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์์ฒญ ์ ํ ๋ก์ง ๋ณด์ ํ์
๋ค์ ์ฌํญ๋ค์ ๊ณ ๋ คํด์ผ ํฉ๋๋ค:
- ์์ธ ๋ฉ์์ง ๊ตญ์ ํ
- ๋จ์ ๋๊ธฐ ์๊ฐ ์ ๋ณด ์ ๊ณต
if (requests.size() >= MAX_REQUESTS) {
- throw new BaseException("๋๋ฌด ๋ง์ ์์ฒญ์
๋๋ค. ์ ์ ํ ๋ค์ ์๋ํด์ฃผ์ธ์.", HttpStatus.TOO_MANY_REQUESTS);
+ LocalDateTime oldestRequest = requests.get(0);
+ Duration waitTime = WINDOW_SIZE.minus(Duration.between(oldestRequest, now));
+ throw new BaseException(
+ String.format("๋๋ฌด ๋ง์ ์์ฒญ์
๋๋ค. %d์ด ํ์ ๋ค์ ์๋ํด์ฃผ์ธ์.",
+ waitTime.getSeconds()),
+ HttpStatus.TOO_MANY_REQUESTS);
}๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void checkRateLimit(String email) { | |
| List<LocalDateTime> requests = requestMap.computeIfAbsent(email, k -> new ArrayList<>()); | |
| LocalDateTime now = LocalDateTime.now(); | |
| requests.removeIf(requestTime -> | |
| requestTime.plus(WINDOW_SIZE).isBefore(now)); | |
| if (requests.size() >= MAX_REQUESTS) { | |
| throw new BaseException("๋๋ฌด ๋ง์ ์์ฒญ์ ๋๋ค. ์ ์ ํ ๋ค์ ์๋ํด์ฃผ์ธ์.", HttpStatus.TOO_MANY_REQUESTS); | |
| } | |
| requests.add(now); | |
| } | |
| public void checkRateLimit(String email) { | |
| List<LocalDateTime> requests = requestMap.computeIfAbsent(email, k -> new ArrayList<>()); | |
| LocalDateTime now = LocalDateTime.now(); | |
| requests.removeIf(requestTime -> | |
| requestTime.plus(WINDOW_SIZE).isBefore(now)); | |
| if (requests.size() >= MAX_REQUESTS) { | |
| LocalDateTime oldestRequest = requests.get(0); | |
| Duration waitTime = WINDOW_SIZE.minus(Duration.between(oldestRequest, now)); | |
| throw new BaseException( | |
| String.format("๋๋ฌด ๋ง์ ์์ฒญ์ ๋๋ค. %d์ด ํ์ ๋ค์ ์๋ํด์ฃผ์ธ์.", | |
| waitTime.getSeconds()), | |
| HttpStatus.TOO_MANY_REQUESTS); | |
| } | |
| requests.add(now); | |
| } |
| } catch (MessagingException e) { | ||
| throw new BaseException("์ด๋ฉ์ผ ์์ฑ ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค.", HttpStatus.BAD_REQUEST); | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์์ธ ์ฒ๋ฆฌ ๊ฐ์ ํ์
์์ธ ์์ธ๋ฅผ ํฌํจํ์ฌ ๋ ์์ธํ ๋๋ฒ๊น ์ด ๊ฐ๋ฅํ๋๋ก ํด์ผ ํฉ๋๋ค.
- throw new BaseException("์ด๋ฉ์ผ ์์ฑ ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค.", HttpStatus.BAD_REQUEST);
+ throw new BaseException("์ด๋ฉ์ผ ์์ฑ ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค: " + e.getMessage(),
+ HttpStatus.BAD_REQUEST, e);๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (MessagingException e) { | |
| throw new BaseException("์ด๋ฉ์ผ ์์ฑ ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค.", HttpStatus.BAD_REQUEST); | |
| } | |
| } catch (MessagingException e) { | |
| throw new BaseException("์ด๋ฉ์ผ ์์ฑ ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค: " + e.getMessage(), | |
| HttpStatus.BAD_REQUEST, e); | |
| } |
| private final JavaMailSender javaMailSender; | ||
| private final RateLimiter rateLimiter; // ์ธ์ฆ ๋ฒํธ ์์ฒญ ์ ํ | ||
| private final Map<String, Integer> verificationCodes = new ConcurrentHashMap<>(); // ์ด๋ฉ์ผ๋ณ ์ธ์ฆ๋ฒํธ ์ ์ฅ | ||
| private final Map<String, LocalDateTime> expiryTimes = new ConcurrentHashMap<>(); // ์ด๋ฉ์ผ๋ณ ์ธ์ฆ๋ฒํธ ๋ง๋ฃ ์๊ฐ ์ ์ฅ | ||
|
|
||
| private static final Duration CODE_VALID_DURATION = Duration.ofMinutes(5); | ||
|
|
||
| @Value("${spring.mail.username}") | ||
| private String senderEmail; |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
ํ๋ ๊ตฌ์กฐ ๊ฐ์ ํ์
๋ ๊ฐ์ Map์ ์ฌ์ฉํ๋ ๋์ ๋จ์ผ Map๊ณผ ๋ณตํฉ ๊ฐ์ฒด๋ฅผ ์ฌ์ฉํ๋ ๊ฒ์ด ๋ ํจ์จ์ ์ ๋๋ค.
- private final Map<String, Integer> verificationCodes = new ConcurrentHashMap<>();
- private final Map<String, LocalDateTime> expiryTimes = new ConcurrentHashMap<>();
+ private final Map<String, VerificationData> verificationData = new ConcurrentHashMap<>();
+
+ @Value("${mail.verification.expiry-minutes:5}")
+ private int expiryMinutes;
+
+ private record VerificationData(
+ int code,
+ LocalDateTime expiryTime
+ ) {}๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private final JavaMailSender javaMailSender; | |
| private final RateLimiter rateLimiter; // ์ธ์ฆ ๋ฒํธ ์์ฒญ ์ ํ | |
| private final Map<String, Integer> verificationCodes = new ConcurrentHashMap<>(); // ์ด๋ฉ์ผ๋ณ ์ธ์ฆ๋ฒํธ ์ ์ฅ | |
| private final Map<String, LocalDateTime> expiryTimes = new ConcurrentHashMap<>(); // ์ด๋ฉ์ผ๋ณ ์ธ์ฆ๋ฒํธ ๋ง๋ฃ ์๊ฐ ์ ์ฅ | |
| private static final Duration CODE_VALID_DURATION = Duration.ofMinutes(5); | |
| @Value("${spring.mail.username}") | |
| private String senderEmail; | |
| private final JavaMailSender javaMailSender; | |
| private final RateLimiter rateLimiter; // ์ธ์ฆ ๋ฒํธ ์์ฒญ ์ ํ | |
| private final Map<String, VerificationData> verificationData = new ConcurrentHashMap<>(); | |
| @Value("${mail.verification.expiry-minutes:5}") | |
| private int expiryMinutes; | |
| private record VerificationData( | |
| int code, | |
| LocalDateTime expiryTime | |
| ) {} | |
| @Value("${spring.mail.username}") | |
| private String senderEmail; |
There was a problem hiding this comment.
Actionable comments posted: 2
๐งน Nitpick comments (1)
src/main/java/com/mycom/socket/auth/service/MailService.java (1)
61-66: HTML ํ ํ๋ฆฟ ๋ถ๋ฆฌ ํ์HTML ๋ด์ฉ์ด ์ฝ๋์ ์ง์ ํฌํจ๋์ด ์์ด ์ ์ง๋ณด์๊ฐ ์ด๋ ต์ต๋๋ค. ๋ณ๋์ ํ ํ๋ฆฟ ํ์ผ๋ก ๋ถ๋ฆฌํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
Thymeleaf๋ ๋ค๋ฅธ ํ ํ๋ฆฟ ์์ง์ ์ฌ์ฉํ์ฌ HTML ๋ด์ฉ์ ๋ถ๋ฆฌํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค.
๐ Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (3)
src/main/java/com/mycom/socket/auth/service/MailService.java(1 hunks)src/main/java/com/mycom/socket/auth/service/RateLimiter.java(1 hunks)src/main/java/com/mycom/socket/auth/service/data/VerificationData.java(1 hunks)
โ Files skipped from review due to trivial changes (1)
- src/main/java/com/mycom/socket/auth/service/data/VerificationData.java
๐ง Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/mycom/socket/auth/service/RateLimiter.java
๐ Additional comments (3)
src/main/java/com/mycom/socket/auth/service/MailService.java (3)
37-42: ๊ตฌํ์ด ์ ์ ํฉ๋๋ค!SecureRandom์ ์ฌ์ฉํ ์์ ํ ๋์ ์์ฑ๊ณผ ๋ช ํํ ์ฃผ์ ์ฒ๋ฆฌ๊ฐ ์ ๋์ด์์ต๋๋ค.
67-70: ๐ ๏ธ Refactor suggestion์์ธ ์ฒ๋ฆฌ ๊ฐ์ ํ์
BaseException์ ์์ธ ์์ธ๋ฅผ ํฌํจ์ํค์ง ์์ ๋๋ฒ๊น ์ด ์ด๋ ค์ธ ์ ์์ต๋๋ค.
๋ค์๊ณผ ๊ฐ์ด ์์ ํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค:
- throw new BaseException("์ด๋ฉ์ผ ์์ฑ ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค: " + e.getMessage(), - HttpStatus.BAD_REQUEST); + throw new BaseException("์ด๋ฉ์ผ ์์ฑ ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค: " + e.getMessage(), + HttpStatus.BAD_REQUEST, e);Likely invalid or redundant comment.
79-85:โ ๏ธ Potential issue๋ฉ์ผ ์ ์ก ์์ธ ์ฒ๋ฆฌ ํ์
JavaMailSender.send() ๋ฉ์๋์ ์์ธ ์ฒ๋ฆฌ๊ฐ ๋๋ฝ๋์์ต๋๋ค. ๋ฉ์ผ ์๋ฒ ์ฐ๊ฒฐ ์คํจ ๋ฑ์ ์ํฉ์ ๋ํ ์ฒ๋ฆฌ๊ฐ ํ์ํฉ๋๋ค.
๋ค์๊ณผ ๊ฐ์ด ์์ ํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค:
public int sendMail(String mail) { rateLimiter.checkRateLimit(mail); MimeMessage message = createMail(mail); - javaMailSender.send(message); + try { + javaMailSender.send(message); + } catch (MailException e) { + throw new BaseException("์ด๋ฉ์ผ ์ ์ก ์ค ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค", + HttpStatus.INTERNAL_SERVER_ERROR, e); + } return verificationDataMap.get(mail).code(); }Likely invalid or redundant comment.
| private final JavaMailSender javaMailSender; | ||
| private final RateLimiter rateLimiter; // ์ธ์ฆ ๋ฒํธ ์์ฒญ ์ ํ | ||
|
|
||
| private final Map<String, VerificationData> verificationDataMap = new ConcurrentHashMap<>(); | ||
| private static final Duration CODE_VALID_DURATION = Duration.ofMinutes(5); | ||
|
|
||
| @Value("${spring.mail.username}") | ||
| private String senderEmail; |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ด๋ฉ์ผ ์ ํจ์ฑ ๊ฒ์ฆ ๋ก์ง ์ถ๊ฐ ํ์
์ด๋ฉ์ผ ์ฃผ์์ ์ ํจ์ฑ์ ๊ฒ์ฆํ๋ ๋ก์ง์ด ์์ต๋๋ค. ์๋ชป๋ ์ด๋ฉ์ผ ์ฃผ์๋ก ์ธํ ์ค๋ฅ๋ฅผ ๋ฐฉ์งํ๊ธฐ ์ํด ์ ํจ์ฑ ๊ฒ์ฆ์ด ํ์ํฉ๋๋ค.
๋ค์๊ณผ ๊ฐ์ ์ ํธ๋ฆฌํฐ ๋ฉ์๋๋ฅผ ์ถ๊ฐํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค:
private boolean isValidEmail(String email) {
String emailRegex = "^[A-Za-z0-9+_.-]+@(.+)$";
return email != null && email.matches(emailRegex);
}| public boolean verifyCode(String email, String code) { | ||
| VerificationData data = verificationDataMap.get(email); | ||
|
|
||
| if (data == null || LocalDateTime.now().isAfter(data.expiryTime())) { | ||
| return false; | ||
| } | ||
|
|
||
| boolean isValid = String.valueOf(data.code()).equals(code); | ||
| if (isValid) { | ||
| verificationDataMap.remove(email); | ||
| } | ||
| return isValid; | ||
|
|
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ ๋ ฅ๊ฐ ๊ฒ์ฆ ๊ฐํ ํ์
code ํ๋ผ๋ฏธํฐ์ ๋ํ null ์ฒดํฌ๊ฐ ๋๋ฝ๋์์ต๋๋ค. ๋ํ ์ฝ๋ ํ์(์ซ์ 6์๋ฆฌ)์ ๋ํ ๊ฒ์ฆ๋ ํ์ํฉ๋๋ค.
๋ค์๊ณผ ๊ฐ์ด ์์ ํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค:
public boolean verifyCode(String email, String code) {
+ if (code == null || !code.matches("\\d{6}")) {
+ return false;
+ }
VerificationData data = verificationDataMap.get(email);๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public boolean verifyCode(String email, String code) { | |
| VerificationData data = verificationDataMap.get(email); | |
| if (data == null || LocalDateTime.now().isAfter(data.expiryTime())) { | |
| return false; | |
| } | |
| boolean isValid = String.valueOf(data.code()).equals(code); | |
| if (isValid) { | |
| verificationDataMap.remove(email); | |
| } | |
| return isValid; | |
| public boolean verifyCode(String email, String code) { | |
| if (code == null || !code.matches("\\d{6}")) { | |
| return false; | |
| } | |
| VerificationData data = verificationDataMap.get(email); | |
| if (data == null || LocalDateTime.now().isAfter(data.expiryTime())) { | |
| return false; | |
| } | |
| boolean isValid = String.valueOf(data.code()).equals(code); | |
| if (isValid) { | |
| verificationDataMap.remove(email); | |
| } | |
| return isValid; |
There was a problem hiding this comment.
Actionable comments posted: 1
๐งน Nitpick comments (4)
src/main/java/com/mycom/socket/auth/service/data/VerificationData.java (1)
9-9: ๋ง๋ฃ ์๊ฐ์ ๊ตฌ์ฑ ๊ฐ๋ฅํ๋๋ก ๊ฐ์ ํ์๋ง๋ฃ ์๊ฐ์ด ํ๋์ฝ๋ฉ๋์ด ์์ต๋๋ค. ํ๊ฒฝ์ ๋ฐ๋ผ ์ ์ฐํ๊ฒ ์กฐ์ ํ ์ ์๋๋ก ๊ตฌ์ฑ ํ์ผ์์ ์ค์ ํ ์ ์๊ฒ ๊ฐ์ ํ๋ ๊ฒ์ด ์ข์ต๋๋ค.
- private static final Duration CODE_VALID_DURATION = Duration.ofMinutes(5); + @Value("${verification.code.expiry-minutes:5}") + private static final int CODE_EXPIRY_MINUTES; + private static final Duration CODE_VALID_DURATION = Duration.ofMinutes(CODE_EXPIRY_MINUTES);src/main/java/com/mycom/socket/auth/service/MailService.java (3)
55-60: ์ด๋ฉ์ผ ํ ํ๋ฆฟ ์ธ๋ถํ ํ์HTML ํ ํ๋ฆฟ์ด ์ฝ๋์ ํ๋์ฝ๋ฉ๋์ด ์์ต๋๋ค. ๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ด ํ์ํฉ๋๋ค:
- ํ ํ๋ฆฟ์ ์ธ๋ถ ํ์ผ๋ก ๋ถ๋ฆฌ
- ๋ค๊ตญ์ด ์ง์์ ์ํ ๋ฉ์์ง ์ธ๋ถํ
- String body = String.format(""" - <h3>์์ฒญํ์ ์ธ์ฆ ๋ฒํธ์ ๋๋ค.</h3> - <h1>%s</h1> - <h3>๊ฐ์ฌํฉ๋๋ค.</h3> - """, verificationCode); - message.setText(body, "UTF-8", "html"); + Context context = new Context(); + context.setVariable("code", verificationCode); + String body = templateEngine.process("mail/verification-template", context); + message.setText(body, "UTF-8", "html");
49-66: MimeMessage ์์ฑ ๋ก์ง ๊ฐ์ ํ์MimeMessage ์์ฑ ๋ก์ง์ ๋น๋ ํจํด์ ์ฌ์ฉํ์ฌ ๊ฐ์ ํ๋ฉด ์ข์ ๊ฒ ๊ฐ์ต๋๋ค:
- ๊ฐ๋ ์ฑ ํฅ์
- ์ ์ง๋ณด์์ฑ ๊ฐ์
- ํ ์คํธ ์ฉ์ด์ฑ ํฅ์
+ @Builder + private static class EmailBuilder { + private String from; + private String to; + private String subject; + private String content; + + public MimeMessage build(JavaMailSender mailSender) throws MessagingException { + MimeMessage message = mailSender.createMimeMessage(); + message.setFrom(from); + message.setRecipients(MimeMessage.RecipientType.TO, to); + message.setSubject(subject); + message.setText(content, "UTF-8", "html"); + return message; + } + }
83-99: ์ด๋ฉ์ผ ๋ฐ์ก ์คํจ ์ ์ฌ์๋ ๋ก์ง ์ถ๊ฐ ๊ณ ๋ คํ์ฌ ๊ตฌํ์์๋ ์ด๋ฉ์ผ ๋ฐ์ก ์คํจ ์ ์ฆ์ ์์ธ๋ฅผ ๋์ง๊ณ ์์ต๋๋ค. ๋ค์๊ณผ ๊ฐ์ ๊ฐ์ ์ ๊ณ ๋ คํด๋ณด์ธ์:
- ์ฌ์๋ ๋ก์ง ์ถ๊ฐ
- ์คํจ ๋ก๊ทธ ๊ธฐ๋ก
- ๋น๋๊ธฐ ์ฒ๋ฆฌ ๊ณ ๋ ค
๐ Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (3)
src/main/java/com/mycom/socket/auth/controller/AuthController.java(2 hunks)src/main/java/com/mycom/socket/auth/service/MailService.java(1 hunks)src/main/java/com/mycom/socket/auth/service/data/VerificationData.java(1 hunks)
๐ Additional comments (2)
src/main/java/com/mycom/socket/auth/controller/AuthController.java (1)
51-59: ๐ ๏ธ Refactor suggestion์ธ์ฆ ์ฝ๋ ๊ฒ์ฆ ์๋ํฌ์ธํธ ๊ฐ์ ํ์
๊ฒ์ฆ ์๋ํฌ์ธํธ์ ๋ค์ ๊ฐ์ ์ฌํญ์ด ํ์ํฉ๋๋ค:
- ์ฝ๋ ํ์ ๊ฒ์ฆ (
@Pattern์ด๋ ธํ ์ด์ ์ฌ์ฉ)- ํ์คํ๋ ์๋ต ํ์ ์ฌ์ฉ
@GetMapping("/email/verify") - public Boolean mailCheck(@RequestParam(name = "mail") String mail, - @RequestParam(name = "code") String code) { + public ApiResponse<EmailVerificationResponse> mailCheck( + @RequestParam(name = "mail") @Email(message = "์ ํจํ ์ด๋ฉ์ผ ์ฃผ์๋ฅผ ์ ๋ ฅํด์ฃผ์ธ์") String mail, + @RequestParam(name = "code") @Pattern(regexp = "^\\d{6}$", message = "6์๋ฆฌ ์ซ์๋ฅผ ์ ๋ ฅํด์ฃผ์ธ์") String code) { try { - return mailService.verifyCode(mail, code); + boolean verified = mailService.verifyCode(mail, code); + return ApiResponse.success(new EmailVerificationResponse(verified)); } catch (Exception e) { throw new BaseException("์ธ์ฆ์ฝ๋ ๊ฒ์ฆ์ ์คํจํ์ต๋๋ค.", HttpStatus.BAD_REQUEST); } }Likely invalid or redundant comment.
src/main/java/com/mycom/socket/auth/service/MailService.java (1)
32-42: ์ธ์ฆ๋ฒํธ ์์ฑ ๋ก์ง์ ๋ํ ๋จ์ ํ ์คํธ ํ์SecureRandom์ ์ฌ์ฉํ ์ธ์ฆ๋ฒํธ ์์ฑ ๋ก์ง์ ๋ํ ๋จ์ ํ ์คํธ๊ฐ ํ์ํฉ๋๋ค:
- ์์ฑ๋ ์ฝ๋๊ฐ 6์๋ฆฌ์ธ์ง ํ์ธ
- ์์ฑ๋ ์ฝ๋๊ฐ ์ซ์๋ก๋ง ๊ตฌ์ฑ๋์ด ์๋์ง ํ์ธ
- ์ค๋ณต ์ฝ๋ ์์ฑ ํ๋ฅ ํ ์คํธ
๋จ์ ํ ์คํธ ์ฝ๋๋ฅผ ์์ฑํด๋๋ฆด๊น์?
| @PostMapping("/email/verification") | ||
| public Boolean mailSend(@RequestParam(name = "mail") String mail) { | ||
| try { | ||
| rateLimiter.checkRateLimit(mail); // ์์ฒญ ์ ํ ์ฒดํฌ | ||
| return mailService.sendMail(mail); | ||
| } catch (Exception e) { | ||
| throw new BaseException("์ด๋ฉ์ผ ์ ์ก์ ์คํจํ์ต๋๋ค.", HttpStatus.BAD_REQUEST); | ||
| } | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ ๋ ฅ๊ฐ ๊ฒ์ฆ ๋ฐ ์๋ต ํ์ ํ์คํ ํ์
์ด๋ฉ์ผ ๊ฒ์ฆ ์๋ํฌ์ธํธ์ ๋ค์ ๊ฐ์ ์ฌํญ์ด ํ์ํฉ๋๋ค:
@Email์ด๋ ธํ ์ด์ ์ผ๋ก ์ด๋ฉ์ผ ํ์ ๊ฒ์ฆ- ํ์คํ๋ ์๋ต ํ์ ์ฌ์ฉ
@PostMapping("/email/verification")
- public Boolean mailSend(@RequestParam(name = "mail") String mail) {
+ public ApiResponse<EmailVerificationResponse> mailSend(
+ @RequestParam(name = "mail") @Email(message = "์ ํจํ ์ด๋ฉ์ผ ์ฃผ์๋ฅผ ์
๋ ฅํด์ฃผ์ธ์") String mail) {
try {
rateLimiter.checkRateLimit(mail); // ์์ฒญ ์ ํ ์ฒดํฌ
- return mailService.sendMail(mail);
+ boolean sent = mailService.sendMail(mail);
+ return ApiResponse.success(new EmailVerificationResponse(sent));
} catch (Exception e) {
throw new BaseException("์ด๋ฉ์ผ ์ ์ก์ ์คํจํ์ต๋๋ค.", HttpStatus.BAD_REQUEST);
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
๐งน Nitpick comments (5)
src/main/java/com/mycom/socket/global/handler/GlobalExceptionHandler.java (1)
11-13: ๋ถํ์ํ๊ฑฐ๋ ์๋ชป๋ ์์ธ ํด๋์ค ์ํฌํธ
NoResourceFoundException์ ์ฃผ๋ก ๋ฆฌ์์ค ํธ๋ค๋ง ์ ์ฌ์ฉ๋๋ ์์ธ๋ก, ํด๋น ํด๋์ค๋ฅผ ์ํฌํธํ๋ ๊ฒ์ด ํ์ํ์ง ํ์ธํด ์ฃผ์๊ธฐ ๋ฐ๋๋๋ค. ๋ง์ฝ ์ฌ์ฉ๋์ง ์๋๋ค๋ฉด ๋ถํ์ํ ์ํฌํธ๋ฅผ ์ ๊ฑฐํ๋ ๊ฒ์ด ์ข์ต๋๋ค.src/main/java/com/mycom/socket/auth/dto/request/EmailVerificationRequestDto.java (1)
7-14: ์ฝ๋ ๊ตฌํ์ด ์ ๋์์ต๋๋ค!์ด๋ฉ์ผ ๊ฒ์ฆ์ ์ํ DTO ๊ตฌํ์ด ๊น๋ํ๊ณ ์ ์ ํฉ๋๋ค. ์ ํจ์ฑ ๊ฒ์ฌ ์ ๋ ธํ ์ด์ ๊ณผ ํ๊ธ ์๋ฌ ๋ฉ์์ง๊ฐ ์ ๊ตฌ์ฑ๋์ด ์์ต๋๋ค.
์ธ์ฆ ์ฝ๋ ์ ๊ท์ ํจํด์ ๋ํ ์ ์
ํ์ฌ ์ ๊ท์
\\d{6}์ ์ ํํ 6์๋ฆฌ ์ซ์๋ง์ ํ์ฉํฉ๋๋ค. ๋ณด์์ฑ์ ๋์ฑ ๊ฐํํ๊ธฐ ์ํด ๋ค์๊ณผ ๊ฐ์ ํจํด์ ๊ณ ๋ คํด๋ณด์ธ์:-@Pattern(regexp = "\\d{6}", message = "์ธ์ฆ ์ฝ๋๋ 6์๋ฆฌ ์ซ์์ฌ์ผ ํฉ๋๋ค.") +@Pattern(regexp = "^[0-9]{6}$", message = "์ธ์ฆ ์ฝ๋๋ 6์๋ฆฌ ์ซ์์ฌ์ผ ํฉ๋๋ค.")src/main/java/com/mycom/socket/auth/dto/response/EmailVerificationResponseDto.java (1)
5-14: ์๋ต DTO ๊ตฌํ์ด ๋ช ํํฉ๋๋ค.์ฑ๊ณต๊ณผ ์คํจ ์ผ์ด์ค๋ฅผ ์ํ ์ ์ ํฉํ ๋ฆฌ ๋ฉ์๋๊ฐ ์ ๊ตฌํ๋์ด ์์ต๋๋ค.
๋ฌธ์ํ ๊ฐ์ ์ ์
๋ฉ์๋์ Javadoc์ ์ถ๊ฐํ์ฌ ์ฌ์ฉ ๋ฐฉ๋ฒ๊ณผ ๋ฐํ๊ฐ์ ๋ช ํํ ์ค๋ช ํ๋ฉด ์ข์ ๊ฒ ๊ฐ์ต๋๋ค:
/** * ์ด๋ฉ์ผ ์ ์ก ์ฑ๊ณต ์๋ต์ ์์ฑํฉ๋๋ค. * @return ์ฑ๊ณต ๋ฉ์์ง๊ฐ ํฌํจ๋ ์๋ต ๊ฐ์ฒด */ public static EmailVerificationResponseDto createSuccessResponse() { // ... existing code } /** * ์ด๋ฉ์ผ ์ ์ก ์คํจ ์๋ต์ ์์ฑํฉ๋๋ค. * @param errorMessage ์คํจ ์์ธ์ ์ค๋ช ํ๋ ์๋ฌ ๋ฉ์์ง * @return ์๋ฌ ๋ฉ์์ง๊ฐ ํฌํจ๋ ์คํจ ์๋ต ๊ฐ์ฒด */ public static EmailVerificationResponseDto createFailureResponse(String errorMessage) { // ... existing code }src/main/java/com/mycom/socket/auth/dto/response/EmailVerificationCheckResponseDto.java (1)
5-14: ์๋ต ๊ตฌ์กฐ๊ฐ ์ผ๊ด์ฑ ์๊ฒ ๊ตฌํ๋์์ต๋๋ค.์ฑ๊ณต/์คํจ ์๋ต ์์ฑ ๋ก์ง์ด ์ ๊ตฌํ๋์ด ์์ผ๋ฉฐ, Boolean ํ์ ์ฌ์ฉ์ด ์ ์ ํฉ๋๋ค.
๋ฉ์์ง ์์ํ ์ ์
์ฑ๊ณต/์คํจ ๋ฉ์์ง๋ฅผ ์์๋ก ๋ถ๋ฆฌํ๋ฉด ์ฌ์ฌ์ฉ์ฑ๊ณผ ์ ์ง๋ณด์์ฑ์ด ํฅ์๋ ๊ฒ ๊ฐ์ต๋๋ค:
public record EmailVerificationCheckResponseDto(ApiResponse<Boolean> apiResponse) { private static final String SUCCESS_MESSAGE = "์ด๋ฉ์ผ ์ธ์ฆ ์ฑ๊ณต"; public static EmailVerificationCheckResponseDto createSuccessResponse() { return new EmailVerificationCheckResponseDto( ApiResponse.success(SUCCESS_MESSAGE, true)); } // ... rest of the code }src/main/java/com/mycom/socket/auth/controller/AuthController.java (1)
41-61: ์ํคํ ์ฒ ๊ฐ์ ์ ์ํ์ฌ ๊ตฌํ์ ํ์ฅ์ฑ๊ณผ ๋ณด์์ ์ํด ๋ค์๊ณผ ๊ฐ์ ์ํคํ ์ฒ ๊ฐ์ ์ ์ ์ํฉ๋๋ค:
- ์ธ์ฆ ์ฝ๋ ์ ์ฅ์๋ฅผ Redis์ ๊ฐ์ ๋ถ์ฐ ์บ์๋ก ์ด๋
- ์ด๋ฉ์ผ ๋ฐ์ก์ ๋น๋๊ธฐ๋ก ์ฒ๋ฆฌํ์ฌ ์๋ต ์๊ฐ ๊ฐ์
- ๋ณด์์ ์ํ ์ด๋ฉ์ผ ํ ํ๋ฆฟ ์ฌ์ฉ
- ์คํจํ ์ธ์ฆ ์๋์ ๋ํ ๋ก๊น ์ถ๊ฐ
์ด๋ฌํ ๊ฐ์ ์ฌํญ๋ค์ ๊ตฌํํ์๊ฒ ์ต๋๊น? ํ์ํ ๊ฒฝ์ฐ ์์ธํ ๊ตฌํ ๊ฐ์ด๋๋ฅผ ์ ๊ณตํด๋๋ฆฌ๊ฒ ์ต๋๋ค.
๐ Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (9)
src/main/java/com/mycom/socket/auth/controller/AuthController.java(2 hunks)src/main/java/com/mycom/socket/auth/dto/request/EmailRequestDto.java(1 hunks)src/main/java/com/mycom/socket/auth/dto/request/EmailVerificationRequestDto.java(1 hunks)src/main/java/com/mycom/socket/auth/dto/response/EmailVerificationCheckResponseDto.java(1 hunks)src/main/java/com/mycom/socket/auth/dto/response/EmailVerificationResponseDto.java(1 hunks)src/main/java/com/mycom/socket/auth/service/MailService.java(1 hunks)src/main/java/com/mycom/socket/global/dto/ApiResponse.java(1 hunks)src/main/java/com/mycom/socket/global/handler/GlobalExceptionHandler.java(2 hunks)src/main/java/com/mycom/socket/global/handler/ResponseHandler.java(1 hunks)
๐ง Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/mycom/socket/auth/service/MailService.java
๐ Additional comments (6)
src/main/java/com/mycom/socket/global/handler/ResponseHandler.java (1)
26-26: API ์๋ต ํ์์ ๋ณ๊ฒฝ์ผ๋ก ์ธํ ์ํฅ ํ์ธ ํ์
beforeBodyWrite๋ฉ์๋์์ ์๋ต ๋ฐ๋๋ฅผ ๊ทธ๋๋ก ๋ฐํํ๋๋ก ๋ณ๊ฒฝ๋์์ต๋๋ค. ์ด์ ์๋ ๋ชจ๋ ์๋ต์ดApiResponse๋ก ๋ํ๋์ด ์ผ๊ด๋ ์๋ต ํ์์ ์ ๊ณตํ์ ์ ์์ต๋๋ค. ์ด ๋ณ๊ฒฝ์ผ๋ก ์ธํด ํด๋ผ์ด์ธํธ ์ธก์์ ์๋ต ํ์์ ๋ณํ๊ฐ ๋ฐ์ํ ์ ์์ผ๋ฏ๋ก, ๊ธฐ์กด API๋ฅผ ์ฌ์ฉํ๋ ํด๋ผ์ด์ธํธ์ ๋ฏธ์น๋ ์ํฅ๊ณผ ์๋ต์ ์ผ๊ด์ฑ์ ํ์ธํด ์ฃผ์๊ธฐ ๋ฐ๋๋๋ค.src/main/java/com/mycom/socket/auth/dto/request/EmailRequestDto.java (1)
6-11: Java Record ์ฌ์ฉ์ ๋ฐ๋ฅธ ํธํ์ฑ ํ์ธ ํ์
EmailRequestDto๋ฅผrecord๋ก ์ ์ํ์ จ์ต๋๋ค.recordํค์๋๋ Java 16๋ถํฐ ์ง์๋๋ฏ๋ก, ํ๋ก์ ํธ์ Java ๋ฒ์ ์ด 16 ์ด์์ธ์ง ํ์ธํด ์ฃผ์๊ธฐ ๋ฐ๋๋๋ค. ๋ง์ฝ Java 16 ๋ฏธ๋ง์ ์ฌ์ฉํ๊ณ ์๋ค๋ฉด, ์ผ๋ฐ ํด๋์ค๋ก ๋ณ๊ฒฝํ๋ ๊ฒ์ด ํ์ํฉ๋๋ค.src/main/java/com/mycom/socket/global/dto/ApiResponse.java (2)
11-15: ์๋ต ๊ตฌ์กฐ์ฒด์ ํ๋ ๊ตฌ์ฑ์ด ์ ์ ํฉ๋๋ค.success ํ๋ ์ถ๊ฐ๋ก ์๋ต ์ํ๋ฅผ ๋ช ํํ ํํํ ์ ์๊ฒ ๋์์ต๋๋ค.
17-19: Builder ํจํด ๊ตฌํ์ด ๊น๋ํฉ๋๋ค.Record์ Builder ํจํด์ ์กฐํฉ์ด ๋ถ๋ณ์ฑ๊ณผ ์ ์ฐ์ฑ์ ์ ์ ๊ณตํฉ๋๋ค.
src/main/java/com/mycom/socket/auth/controller/AuthController.java (2)
24-24: ์์กด์ฑ ์ฃผ์ ํจํด ์น์ธ
@RequiredArgsConstructor์ ํจ๊ปfinalํ๋๋กMailService๋ฅผ ์ฃผ์ ํ ๊ฒ์ด ์ข์ ๋ฐฉ์์ ๋๋ค.
42-50:โ ๏ธ Potential issue์ด๋ฉ์ผ ๊ฒ์ฆ ์๋ํฌ์ธํธ ๋ณด์ ํ์
๋ค์ ์ฌํญ๋ค์ ๊ฐ์ ์ด ํ์ํฉ๋๋ค:
- PR ๋ชฉํ์์ ์ธ๊ธ๋ ๋ถ๋น 3ํ ์์ฒญ ์ ํ์ด ๊ตฌํ๋์ง ์์์ต๋๋ค.
- ์ด๋ฉ์ผ ํ์ ๊ฒ์ฆ์ ์ํ
- ์๋ต ํ์์ด ์ผ๊ด๋์ง ์์ต๋๋ค.
๋ค์๊ณผ ๊ฐ์ด ์์ ํ๋ ๊ฒ์ ์ ์ํฉ๋๋ค:
@PostMapping("/verification") - public EmailVerificationResponseDto mailSend(@Valid @RequestBody EmailRequestDto emailRequestDto) { + public EmailVerificationResponseDto mailSend( + @Valid @Email(message = "์ ํจํ ์ด๋ฉ์ผ ํ์์ด ์๋๋๋ค") + @RequestBody EmailRequestDto emailRequestDto) { try { + rateLimiter.checkRateLimit(emailRequestDto.email()); // ์์ฒญ ์ ํ ํ์ธ boolean isSuccess = mailService.sendMail(emailRequestDto.email()); return isSuccess ? EmailVerificationResponseDto.createSuccessResponse() : EmailVerificationResponseDto.createFailureResponse("์ด๋ฉ์ผ ์ ์ก์ ์คํจํ์ต๋๋ค."); } catch (BaseException e) { return EmailVerificationResponseDto.createFailureResponse(e.getMessage()); } }Likely invalid or redundant comment.
| // ์ผ๋ฐ์ ์ธ ์์ธ ์ฒ๋ฆฌ (IllegalArgumentException, NoSuchElementException ๋ฑ) | ||
| @ExceptionHandler({IllegalArgumentException.class, NoSuchElementException.class, NoResourceFoundException.class}) | ||
| protected ResponseEntity<ApiResponse<?>> handleCommonException(Exception e) { | ||
| HttpStatus status = (e instanceof NoResourceFoundException) ? HttpStatus.NOT_FOUND : HttpStatus.BAD_REQUEST; | ||
| log.error("Common Exception : {}", e.getMessage()); | ||
| return ResponseEntity | ||
| .status(status) | ||
| .body(ApiResponse.error(e.getMessage())); | ||
| } |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
๊ณตํต ์์ธ ์ฒ๋ฆฌ์ ๋ํ ์ ์ ์ฑ ๊ฒํ ํ์
handleCommonException ๋ฉ์๋์์ IllegalArgumentException, NoSuchElementException, NoResourceFoundException์ ํ๊บผ๋ฒ์ ์ฒ๋ฆฌํ๊ณ ์์ต๋๋ค. ์ด๋ฌํ ์์ธ๋ค์ ์ ์ญ์ ์ผ๋ก ์ฒ๋ฆฌํ๋ ๊ฒ์ด ์ ์ ํ์ง ๊ฒํ ํด ์ฃผ์๊ธฐ ๋ฐ๋๋๋ค. ํนํ, NoSuchElementException๊ณผ NoResourceFoundException์ ๊ตฌ์ฒด์ ์ธ ์ํฉ์ ๋ฐ๋ผ ๋ค๋ฅธ HTTP ์ํ ์ฝ๋๋ฅผ ๋ฐํํด์ผ ํ ์ ์์ต๋๋ค.
| protected ResponseEntity<ApiResponse<?>> handleAllException(Exception e) { | ||
| log.error("Internal Server Error", e); | ||
| return ResponseEntity | ||
| .status(HttpStatus.INTERNAL_SERVER_ERROR) | ||
| .body(ApiResponse.error("์๋ฒ ๋ด๋ถ ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค")); | ||
| .body(ApiResponse.error(e.getMessage() != null ? e.getMessage() : "์๋ฒ ๋ด๋ถ ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค.")); |
There was a problem hiding this comment.
์์ธ ๋ฉ์์ง์ ์ง์ ๋ ธ์ถ๋ก ์ธํ ๋ณด์ ์ํ
handleAllException ๋ฉ์๋์์ ์์ธ์ ์์ธ ๋ฉ์์ง๋ฅผ ํด๋ผ์ด์ธํธ์๊ฒ ์ง์ ๋ฐํํ๊ณ ์์ต๋๋ค. ์ด๋ ๋ด๋ถ ์ ๋ณด๊ฐ ๋
ธ์ถ๋์ด ๋ณด์ ์ทจ์ฝ์ ์ด ๋ ์ ์์ต๋๋ค. ํด๋ผ์ด์ธํธ์๊ฒ๋ ์ผ๋ฐ์ ์ธ ์ค๋ฅ ๋ฉ์์ง๋ฅผ ์ ๊ณตํ๊ณ , ์์ธํ ์์ธ ์ ๋ณด๋ ๋ก๊ทธ๋ก๋ง ๋จ๊ธฐ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค.
์์ ์ ์:
- .body(ApiResponse.error(e.getMessage() != null ? e.getMessage() : "์๋ฒ ๋ด๋ถ ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค."));
+ .body(ApiResponse.error("์๋ฒ ๋ด๋ถ ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค."));๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected ResponseEntity<ApiResponse<?>> handleAllException(Exception e) { | |
| log.error("Internal Server Error", e); | |
| return ResponseEntity | |
| .status(HttpStatus.INTERNAL_SERVER_ERROR) | |
| .body(ApiResponse.error("์๋ฒ ๋ด๋ถ ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค")); | |
| .body(ApiResponse.error(e.getMessage() != null ? e.getMessage() : "์๋ฒ ๋ด๋ถ ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค.")); | |
| protected ResponseEntity<ApiResponse<?>> handleAllException(Exception e) { | |
| log.error("Internal Server Error", e); | |
| return ResponseEntity | |
| .status(HttpStatus.INTERNAL_SERVER_ERROR) | |
| .body(ApiResponse.error("์๋ฒ ๋ด๋ถ ์ค๋ฅ๊ฐ ๋ฐ์ํ์ต๋๋ค.")); |
| public static <T> ApiResponse<T> success(String message) { | ||
| return new ApiResponse<>(message, null, LocalDateTime.now()); | ||
| return ApiResponse.<T>builder() | ||
| .success(true) | ||
| .message(message) | ||
| .timestamp(LocalDateTime.now()) | ||
| .build(); | ||
| } | ||
|
|
||
| public static <T> ApiResponse<T> success(String message, T data) { | ||
| return new ApiResponse<>(message, data, LocalDateTime.now()); | ||
| return ApiResponse.<T>builder() | ||
| .success(true) | ||
| .message(message) | ||
| .data(data) | ||
| .timestamp(LocalDateTime.now()) | ||
| .build(); | ||
| } | ||
|
|
||
| public static <T> ApiResponse<T> error(String message) { | ||
| return new ApiResponse<>(message, null, LocalDateTime.now()); | ||
| return ApiResponse.<T>builder() | ||
| .success(false) | ||
| .message(message) | ||
| .timestamp(LocalDateTime.now()) | ||
| .build(); | ||
| } | ||
|
|
||
| public static <T> ApiResponse<T> error(String message, T data) { | ||
| return new ApiResponse<>(message, data, LocalDateTime.now()); | ||
| return ApiResponse.<T>builder() | ||
| .success(false) | ||
| .message(message) | ||
| .data(data) | ||
| .timestamp(LocalDateTime.now()) | ||
| .build(); |
There was a problem hiding this comment.
๐ ๏ธ Refactor suggestion
์ ์ ํฉํ ๋ฆฌ ๋ฉ์๋์ Null ์์ ์ฑ ๊ฐ์ ํ์
๋ฉ์์ง์ ๋ฐ์ดํฐ์ null ์ฒ๋ฆฌ๋ฅผ ๊ฐ์ ํ๋ฉด ์ข์ ๊ฒ ๊ฐ์ต๋๋ค:
public static <T> ApiResponse<T> success(String message) {
+ Objects.requireNonNull(message, "๋ฉ์์ง๋ null์ผ ์ ์์ต๋๋ค.");
return ApiResponse.<T>builder()
.success(true)
.message(message)
.timestamp(LocalDateTime.now())
.build();
}
public static <T> ApiResponse<T> success(String message, T data) {
+ Objects.requireNonNull(message, "๋ฉ์์ง๋ null์ผ ์ ์์ต๋๋ค.");
return ApiResponse.<T>builder()
.success(true)
.message(message)
.data(data)
.timestamp(LocalDateTime.now())
.build();
}๋ํ timestamp ์์ฑ์ ์ค์ํํ๋ฉด ์ผ๊ด์ฑ ์๋ ์๊ฐ ์ฒ๋ฆฌ๊ฐ ๊ฐ๋ฅํฉ๋๋ค:
private static LocalDateTime getCurrentTimestamp() {
return LocalDateTime.now();
}| @GetMapping("/email/verify") | ||
| public EmailVerificationCheckResponseDto mailCheck(@Valid @RequestBody EmailVerificationRequestDto emailRequestDto) { | ||
| try { | ||
| boolean isVerified = mailService.verifyCode(emailRequestDto.email(), emailRequestDto.code()); | ||
| return isVerified ? EmailVerificationCheckResponseDto.createSuccessResponse() : | ||
| EmailVerificationCheckResponseDto.createFailureResponse("์ด๋ฉ์ผ ์ธ์ฆ์ ์คํจํ์ต๋๋ค."); | ||
| } catch (BaseException e) { | ||
| return EmailVerificationCheckResponseDto.createFailureResponse(e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
์ด๋ฉ์ผ ์ธ์ฆ ํ์ธ ์๋ํฌ์ธํธ ๊ฐ์ ํ์
ํ์ฌ ๊ตฌํ์ ๋ค์๊ณผ ๊ฐ์ ๋ฌธ์ ๊ฐ ์์ต๋๋ค:
- GET ๋ฉ์๋์ @RequestBody๋ฅผ ์ฌ์ฉํ๋ ๊ฒ์ REST ๊ท์ฝ์ ๋ง์ง ์์ต๋๋ค.
- ์ธ์ฆ ์ฝ๋ ๊ธธ์ด ๊ฒ์ฆ์ด ์์ต๋๋ค.
- ์ธ์ฆ ์๋ ํ์ ์ ํ์ด ์์ต๋๋ค.
๋ค์๊ณผ ๊ฐ์ด ์์ ํ๋ ๊ฒ์ ์ ์ํฉ๋๋ค:
- @GetMapping("/email/verify")
- public EmailVerificationCheckResponseDto mailCheck(@Valid @RequestBody EmailVerificationRequestDto emailRequestDto) {
+ @PostMapping("/email/verify")
+ public EmailVerificationCheckResponseDto mailCheck(
+ @Valid @Email(message = "์ ํจํ ์ด๋ฉ์ผ ํ์์ด ์๋๋๋ค")
+ @Pattern(regexp = "^[0-9]{6}$", message = "์ธ์ฆ๋ฒํธ๋ 6์๋ฆฌ ์ซ์์ฌ์ผ ํฉ๋๋ค")
+ @RequestBody EmailVerificationRequestDto emailRequestDto) {
try {
+ verificationAttemptLimiter.checkAttemptLimit(emailRequestDto.email()); // ์๋ ํ์ ์ ํ ํ์ธ
boolean isVerified = mailService.verifyCode(emailRequestDto.email(), emailRequestDto.code());
return isVerified ? EmailVerificationCheckResponseDto.createSuccessResponse() :
EmailVerificationCheckResponseDto.createFailureResponse("์ด๋ฉ์ผ ์ธ์ฆ์ ์คํจํ์ต๋๋ค.");
} catch (BaseException e) {
return EmailVerificationCheckResponseDto.createFailureResponse(e.getMessage());
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
โป๏ธ Duplicate comments (2)
src/main/java/com/mycom/socket/global/handler/GlobalExceptionHandler.java (2)
47-54:โ ๏ธ Potential issue์์ธ ๋ฉ์์ง๋ฅผ ์ง์ ๋ ธ์ถํ๋ฉด ๋ณด์ ์ทจ์ฝ์ ์ด ๋ฐ์ํ ์ ์์ต๋๋ค
IllegalArgumentException์ ๋ฉ์์ง๋ฅผ ํด๋ผ์ด์ธํธ์๊ฒ ์ง์ ๋ฐํํ๊ณ ์์ต๋๋ค. ์ด๋ ๋ด๋ถ ์ ๋ณด๊ฐ ๋ ธ์ถ๋์ด ๋ณด์ ์ทจ์ฝ์ ์ ์ด๋ํ ์ ์์ต๋๋ค. ํด๋ผ์ด์ธํธ์๊ฒ๋ ์ผ๋ฐ์ ์ธ ์ค๋ฅ ๋ฉ์์ง๋ฅผ ์ ๊ณตํ๊ณ ์์ธํ ์์ธ ์ ๋ณด๋ ๋ก๊ทธ๋ก๋ง ๊ธฐ๋กํ๋ ๊ฒ์ด ์ข์ต๋๋ค.์ฝ๋ ์์ ์ ์:
- .body(ApiResponse.error(e.getMessage())); + .body(ApiResponse.error("์๋ชป๋ ์์ฒญ์ ๋๋ค."));
56-63:โ ๏ธ Potential issue์์ธ ๋ฉ์์ง๋ฅผ ์ง์ ๋ ธ์ถํ๋ฉด ๋ณด์ ์ทจ์ฝ์ ์ด ๋ฐ์ํ ์ ์์ต๋๋ค
NoSuchElementException์ ๋ฉ์์ง๋ฅผ ํด๋ผ์ด์ธํธ์๊ฒ ์ง์ ๋ฐํํ๊ณ ์์ต๋๋ค. ์ด๋ ๋ด๋ถ ์ ๋ณด๊ฐ ๋ ธ์ถ๋์ด ๋ณด์ ์ทจ์ฝ์ ์ ์ด๋ํ ์ ์์ต๋๋ค. ํด๋ผ์ด์ธํธ์๊ฒ๋ ์ผ๋ฐ์ ์ธ ์ค๋ฅ ๋ฉ์์ง๋ฅผ ์ ๊ณตํ๊ณ ์์ธํ ์์ธ ์ ๋ณด๋ ๋ก๊ทธ๋ก๋ง ๊ธฐ๋กํ๋ ๊ฒ์ด ์ข์ต๋๋ค.์ฝ๋ ์์ ์ ์:
- .body(ApiResponse.error(e.getMessage())); + .body(ApiResponse.error("์์ฒญํ์ ์์์ ์ฐพ์ ์ ์์ต๋๋ค."));
๐งน Nitpick comments (2)
src/main/java/com/mycom/socket/auth/controller/AuthController.java (2)
44-52: ์์ธ ์ฒ๋ฆฌ ๋ก์ง์ ์ค๋ณต์ ์ค์ด๊ณ ์ฝ๋์ ์ฌ์ฌ์ฉ์ฑ์ ๋์ผ ์ ์์ต๋๋ค
mailSend๋ฉ์๋์์ ์์ธ ์ฒ๋ฆฌ ๋ก์ง์ด ๋ฐ๋ณต๋๊ณ ์์ต๋๋ค. ์์ธ ์ฒ๋ฆฌ๋ฅผ ๊ณตํต ๋ฉ์๋๋ก ์ถ์ถํ์ฌ ์ฝ๋ ์ค๋ณต์ ์ค์ด๊ณ ์ผ๊ด๋ ์์ธ ์ฒ๋ฆฌ๋ฅผ ๊ตฌํํ๋ ๊ฒ์ ๊ถ์ฅํฉ๋๋ค.์ฝ๋ ์์ ์ ์:
// ๊ณตํต ์์ธ ์ฒ๋ฆฌ ๋ฉ์๋ ์ถ๊ฐ private <T> T handleException(Supplier<T> supplier, Function<String, T> failureResponseCreator) { try { return supplier.get(); } catch (BaseException e) { return failureResponseCreator.apply(e.getMessage()); } } // ๊ธฐ์กด ๋ฉ์๋ ์์ @PostMapping("/verification") public EmailVerificationResponseDto mailSend(@Valid @RequestBody EmailRequestDto emailRequestDto) { return handleException(() -> { boolean isSuccess = mailService.sendMail(emailRequestDto.email()); return isSuccess ? EmailVerificationResponseDto.createSuccessResponse() : EmailVerificationResponseDto.createFailureResponse("์ด๋ฉ์ผ ์ ์ก์ ์คํจํ์ต๋๋ค."); }, EmailVerificationResponseDto::createFailureResponse); }
54-64: ์์ธ ์ฒ๋ฆฌ ๋ก์ง์ ์ค๋ณต์ ์ค์ด๊ณ ์ฝ๋์ ์ฌ์ฌ์ฉ์ฑ์ ๋์ผ ์ ์์ต๋๋ค
mailCheck๋ฉ์๋์์๋ ์์ธ ์ฒ๋ฆฌ ๋ก์ง์ด ๋ฐ๋ณต๋๊ณ ์์ต๋๋ค. ์์ ์ ์ํ ๊ณตํต ์์ธ ์ฒ๋ฆฌ ๋ฉ์๋๋ฅผ ํ์ฉํ์ฌ ์ฝ๋ ์ค๋ณต์ ์ค์ผ ์ ์์ต๋๋ค.์ฝ๋ ์์ ์ ์:
@PostMapping("/email/verify") public EmailVerificationCheckResponseDto mailCheck(@Valid @RequestBody EmailVerificationRequestDto emailRequestDto) { return handleException(() -> { rateLimiter.checkRateLimit(emailRequestDto.email()); // ์๋ ํ์ ์ ํ boolean isVerified = mailService.verifyCode(emailRequestDto.email(), emailRequestDto.code()); return isVerified ? EmailVerificationCheckResponseDto.createSuccessResponse() : EmailVerificationCheckResponseDto.createFailureResponse("์ด๋ฉ์ผ ์ธ์ฆ์ ์คํจํ์ต๋๋ค."); }, EmailVerificationCheckResponseDto::createFailureResponse); }
๐ Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (4)
src/main/java/com/mycom/socket/auth/controller/AuthController.java(2 hunks)src/main/java/com/mycom/socket/auth/dto/request/EmailVerificationRequestDto.java(1 hunks)src/main/java/com/mycom/socket/global/dto/ApiResponse.java(1 hunks)src/main/java/com/mycom/socket/global/handler/GlobalExceptionHandler.java(2 hunks)
๐ง Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/mycom/socket/auth/dto/request/EmailVerificationRequestDto.java
๐ Additional comments (1)
src/main/java/com/mycom/socket/global/dto/ApiResponse.java (1)
19-20: LGTM!
ApiResponseํด๋์ค์successํ๋๋ฅผ ์ถ๊ฐํ๊ณ ๋น๋ ํจํด์ ์ฌ์ฉํ์ฌ ์์ฑ์๋ฅผ ์ ๋ฆฌํ ๋ถ๋ถ์ด ์ ๊ตฌํ๋์์ต๋๋ค.
๐ ๊ด๋ จ ์ด์
#23 ์ด๋ฉ์ผ ์ธ์ฆ ๊ธฐ๋ฅ
โจ ๊ณผ์ ๋ด์ฉ
์ด๋ฉ์ผ ์ธ์ฆ ๋ฒํธ๋ฅผ ํตํด ์ด๋ฉ์ผ์ ์ธ์ฆํ๋ ๊ธฐ๋ฅ์ ๊ตฌํํ์์ต๋๋ค.
๐ธ ์คํฌ๋ฆฐ์ท(์ ํ)
Summary by CodeRabbit
Summary by CodeRabbit
์๋ก์ด ๊ธฐ๋ฅ
์ข ์์ฑ ๋ณ๊ฒฝ
์๋น์ค ๊ฐ์
๋ฐ์ดํฐ ๋ชจ๋ธ ์ถ๊ฐ
์์ธ ์ฒ๋ฆฌ ๊ฐ์
์๋ต ์ฒ๋ฆฌ ๊ฐ์