feat: Discord Webhook URL 동적 로딩 기능 구현 완료 #20
Conversation
- serviceType에 따라 적절한 NotificationService 구현체를 심플 팩토리에서 주입 - NotificationContext에서 전략 실행 로직 위임
- SlackConstant와 중복되는 에러 메시지가 포함되어 있으나, 향후 서비스별 커스터마이징 가능성을 고려해 중복을 허용하고 별도로 정의함
- Sentry 이벤트 정보를 기반으로 Discord 메시지 생성 및 전송 - 메시지 생성 실패, 직렬화 실패, 전송 실패에 대한 예외 처리 로직 포함 - 알림 내용에는 에러 수준, 발생 시각, 팀, 서버 유형 등의 정보 포함 - Slack 구조를 참고하되 Discord 형식에 맞게 임베드로 구성
- 환경별로 구현체를 분리해 주입하는 방법도 고려했지만, 해당 방식은 인스턴스 생성을 전제로 하므로 유틸성 클래스의 성격과는 맞지 않다고 판단하여, setter를 통해 주입하는 방식으로 개선하였습니다.
- 실제 Sentry 웹훅 페이로드를 사용하여 이벤트 정보가 정확히 추출되는지 검증
|
Summary by CodeRabbit
Summary by CodeRabbit
Walkthrough이 변경사항은 Discord 웹훅 알림 기능을 새롭게 도입하고, Slack/Discord 등 다양한 알림 전략을 지원하는 구조로 리팩토링합니다. 전략 패턴과 팩토리 메서드 패턴을 적용하여 서비스 확장성을 높였으며, 환경 변수의 동적 로딩과 테스트 코드도 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant API_Gateway
participant SentryWebhookHandler
participant SentryEventExtractorService
participant NotificationProcessorService
participant NotificationServiceFactory
participant NotificationContext
participant NotificationService
API_Gateway->>SentryWebhookHandler: handleRequest(request)
SentryWebhookHandler->>SentryEventExtractorService: extractEvent(requestBody)
SentryEventExtractorService-->>SentryWebhookHandler: SentryEventDetail
SentryWebhookHandler->>NotificationProcessorService: processNotification(request, event)
NotificationProcessorService->>EnvUtil: getWebhookUrl(serviceType, team, stage, type)
NotificationProcessorService->>NotificationServiceFactory: createService(serviceType, objectMapper)
NotificationServiceFactory-->>NotificationProcessorService: NotificationService
NotificationProcessorService->>NotificationContext: new(NotificationService)
NotificationContext->>NotificationService: sendNotification(team, type, stage, event, webhookUrl)
NotificationService-->>NotificationContext: (void)
NotificationContext-->>NotificationProcessorService: (void)
NotificationProcessorService-->>SentryWebhookHandler: (void)
SentryWebhookHandler-->>API_Gateway: APIGatewayProxyResponseEvent
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/test/java/org/sopt/makers/global/util/EnvUtilTest.java (1)
75-103: 🛠️ Refactor suggestion테스트 케이스에 Discord 서비스 유형 테스트 추가 필요
현재 테스트 케이스들은 모두 Slack 서비스 유형에 대해서만 테스트하고 있습니다. Discord 웹훅 기능이 추가되었으므로, Discord 서비스 유형에 대한 테스트 케이스도 추가하는 것이 좋겠습니다.
@DisplayName("대소문자 구분 없이 키를 인식할 수 있어야 한다") @ParameterizedTest(name = "service={0}, team={1}, stage={2}, type={3}") @CsvSource({ "SLACK, CREW, DEV, BE", "slack, crew, dev, be", "SlAcK, CrEw, DeV, Be", "slack, APP, PROD, FE", "SLACK, app, prod, fe", "SlAcK, ApP, PrOd, Fe" + "DISCORD, CREW, DEV, BE", + "discord, crew, dev, be", + "DiScOrD, CrEw, DeV, Be" }) void testGetWebhookUrl_caseInsensitive(String service, String team, String stage, String type) { + // Discord 테스트 케이스 추가 + if (service.equalsIgnoreCase("discord")) { + String actualUrl = EnvUtil.getWebhookUrl(service, team, stage, type); + assertEquals("https://discord.com/api/webhooks/crew/dev/be", actualUrl); + return; + } if (team.equalsIgnoreCase("app")) { String actualUrl = EnvUtil.getWebhookUrl(service, team, stage, type); assertEquals("https://hooks.slack.com/services/app/prod/fe", actualUrl); return; } String actualUrl = EnvUtil.getWebhookUrl(service, team, stage, type); assertEquals("https://hooks.slack.com/services/crew/dev/be", actualUrl); } +@DisplayName("Discord 웹훅 URL을 정상적으로 가져온다") +@Test +void testGetWebhookUrl_discord() { + // 테스트 환경에 Discord 웹훅 URL 설정 필요 + String expectedUrl = "https://discord.com/api/webhooks/crew/dev/be"; + String actualUrl = EnvUtil.getWebhookUrl("discord", "crew", "dev", "be"); + + assertEquals(expectedUrl, actualUrl); +}
🧹 Nitpick comments (15)
src/test/java/org/sopt/makers/global/util/EnvUtilTest.java (1)
30-33: 테스트 환경 변수 설정이 명확합니다.테스트에 필요한 환경 변수를 명시적으로 설정하는 방식으로 변경되었습니다. Slack 웹훅 URL 환경 변수가 테스트를 위해 적절히 설정되어 있습니다. 그러나 Discord 웹훅 URL에 대한 테스트 설정도 추가하는 것을 고려해 볼 수 있습니다.
String content = """ SLACK_WEBHOOK_CREW_DEV_BE=https://hooks.slack.com/services/crew/dev/be SLACK_WEBHOOK_APP_PROD_FE=https://hooks.slack.com/services/app/prod/fe + DISCORD_WEBHOOK_CREW_DEV_BE=https://discord.com/api/webhooks/crew/dev/be + DISCORD_WEBHOOK_APP_PROD_FE=https://discord.com/api/webhooks/app/prod/fe """;src/main/java/org/sopt/makers/service/NotificationService.java (1)
5-6: 메소드 시그니처와 JavaDoc 일관성 개선 필요JavaDoc에서는 구체적인 예외 타입(
MessageBuildException,SendException)을 명시하고 있으나, 메소드 시그니처에서는 상위 타입인SentryCheckedException만 선언하고 있습니다. 가독성과 명확성을 위해 메소드 시그니처도 JavaDoc과 일치시키는 것이 좋을 것 같습니다.void sendNotification(String team, String type, String stage, SentryEventDetail eventDetail, String webhookUrl) - throws SentryCheckedException; + throws MessageBuildException, SendException;Also applies to: 17-18, 20-21
src/main/java/org/sopt/makers/global/constant/DiscordConstant.java (1)
1-25: 상수 클래스 개선 제안Discord 관련 상수 클래스가 잘 구조화되어 있습니다. 상수들이 논리적으로 그룹화되고 명확하게 주석 처리된 것이 좋습니다. 다음 사항을 고려해보세요:
Discord 임베드 제한(MAX_TITLE_LENGTH, MAX_DESCRIPTION_LENGTH)의 출처를 주석으로 남기면 좋을 것 같습니다. Discord API 문서의 참조 링크가 있다면 더 좋겠습니다.
SENTRY_ICON_URL과 같은 외부 URL은 변경될 수 있으므로 프로퍼티나 환경 변수로 구성 가능하게 만드는 것을 고려해보세요.
DATE_FORMAT_PATTERN과 TIMEZONE_SEOUL은 애플리케이션의 다른 부분에서도 사용될 수 있습니다. 이미 공통 상수가 있다면 그것을 재사용하거나, 없다면 더 일반적인 상수 클래스로 이동을 고려해보세요.
src/main/java/org/sopt/makers/vo/discord/message/DiscordMessage.java (1)
9-18: 클래스 구현이 간결하고 명확합니다.자바 레코드를 사용한 불변 데이터 클래스 구현이 적절합니다. 몇 가지 제안:
- 클래스와 메서드에 Javadoc 주석을 추가하여 클래스의 목적과 Discord API와의 통합 방법을 설명하는 것이 도움이 될 것 같습니다.
newInstance정적 팩토리 메서드에 매개변수 유효성 검사를 추가하는 것을 고려해보세요.- 자주 사용되는 Discord 메시지 패턴을 위한 추가 팩토리 메서드를 제공하는 것도 유용할 수 있습니다.
src/main/java/org/sopt/makers/service/NotificationContext.java (1)
8-16: 전략 패턴을 적절하게 사용했습니다.
NotificationContext클래스가 다양한 알림 서비스를 위한 전략 패턴을 효과적으로 구현했습니다. 몇 가지 개선 제안:
- 클래스와 메서드에 Javadoc 주석을 추가하여 이 클래스의 역할과 전체 알림 시스템에서의 위치를 설명하는 것이 좋을 것 같습니다.
- 매개변수 유효성 검사를 추가하여 null 값이 전달되지 않도록 하는 것을 고려해보세요.
src/main/java/org/sopt/makers/service/SentryEventExtractorService.java (1)
18-34: 메서드 구현이 잘되었으나 문서화가 필요합니다.이벤트 추출 로직이 잘 구현되어 있으며 에러 처리와 로깅도 적절합니다. 그러나 다음과 같은 개선 사항을 제안합니다:
- 메서드에 Javadoc 주석을 추가하여 메서드의 목적, 매개변수, 반환 값, 발생 가능한 예외를 문서화하는 것이 좋을 것 같습니다.
- Sentry 이벤트 구조에 대한 간략한 설명도 추가하면 코드 이해에 도움이 될 것 같습니다.
src/main/java/org/sopt/makers/service/DiscordNotificationService.java (2)
16-16: 사용되지 않는 import 제거 필요
ObjectMapperConfig를 import했으나 클래스 내에서 사용되지 않고 있습니다. 불필요한 의존성은 코드 가독성을 떨어뜨리고, 리팩터링 시 혼란을 줄 수 있으므로 삭제해 주세요.-import org.sopt.makers.global.config.ObjectMapperConfig;
65-67: 전체 페이로드 로깅은 log level 조정 권장
jsonPayload전체를info레벨로 기록하면 대용량 메시지 및 PII 유출 위험이 있습니다.
디버깅 목적이라면debug레벨로 낮추거나, 필요한 필드만 로그에 남기는 방식을 권장드립니다.- log.info("Discord 메시지: {}", jsonPayload); + log.debug("Discord 메시지 페이로드: {}", jsonPayload);src/main/java/org/sopt/makers/service/NotificationProcessorService.java (1)
16-26: 로깅 및 입력값 검증 강화 제안
processNotification에 진입했을 때 요청 파라미터를info/debug레벨로 남기면 추후 운영 모니터링에 도움이 됩니다.NotificationServiceFactory.createService가null을 반환하거나UnsupportedServiceTypeException(unchecked)을 던질 경우, 현재 메서드에서 잡히지 않습니다. 서비스 타입 검증을 별도로 수행해 두면 예외 원인 파악이 더 용이합니다.src/main/java/org/sopt/makers/global/util/EnvUtil.java (1)
55-57: 테스트 편의용 setter 공개로 인한 전역 상태 오염 가능성 주의
setDotenv로 전역dotenv를 교체하면 멀티스레드 환경에서 예측 불가능한 사이드이펙트가 생길 수 있습니다.
– 테스트 전용이라면@VisibleForTesting(Guava) 또는 패키지-프라이빗 접근자로 제한하는 방안을 고려해 주세요.
– 또는 DI 컨테이너를 통해 주입하도록 구조 개선을 권장합니다.src/main/java/org/sopt/makers/handler/SentryWebhookHandler.java (1)
28-31: 의존성 주입(or 싱글턴 패턴)으로 서비스 재사용성을 높이세요
SentryEventExtractorService와NotificationProcessorService를 핸들러 내부에서 직접new로 생성하면
- 테스트 시 목(mock) 교체가 어렵고
- 람다 컨테이너가 재사용될 때마다 불필요하게 객체가 중복 생성될 수 있습니다.
스프링 DI 없이도, 다음과 같이
static final싱글턴으로 올리거나 외부 팩토리/빌더를 주입받는 방식으로 개선할 수 있습니다.-private final SentryEventExtractorService eventExtractorService = new SentryEventExtractorService(objectMapper); -private final NotificationProcessorService notificationProcessorService = new NotificationProcessorService(objectMapper); +private static final SentryEventExtractorService EVENT_EXTRACTOR = + new SentryEventExtractorService(ObjectMapperConfig.getInstance()); +private static final NotificationProcessorService NOTIFICATION_PROCESSOR = + new NotificationProcessorService(ObjectMapperConfig.getInstance());src/test/java/org/sopt/makers/handler/FakeSentryPayload.java (2)
1-4: 테스트 전용 유틸 패키지로 이동 권장
org.sopt.makers.handler패키지는 프로덕션 코드와 섞여 있습니다.
src/test/java/.../fixture혹은util패키지로 옮기면 실제 핸들러 클래스 검색 시 충돌을 방지할 수 있습니다.
5-420: 대용량 문자열 상수는 리소스 파일로 분리하면 메모리∙가독성을 개선할 수 있습니다30 KB 이상의 JSON 을 소스 코드에 그대로 포함하면
- 클래스 로딩 시 필요 없는 메모리를 점유하고
- IDE 탐색성이 떨어집니다.
src/test/resources/sentry/sample_payload.json로 옮기고Files.readString(...)으로 읽어오는 방식을 고려해 주세요.src/main/java/org/sopt/makers/vo/discord/embed/DiscordEmbed.java (2)
12-15: Discord 제한 길이 검증 로직 추가 제안Discord Embed 은
title256자,description4096자,fields25개 등의 제한이 있습니다.
newInstance안에서 길이 검증을 해두면 서비스 계층에서 중복 검증할 필요가 없어집니다.
17-24: EmbedField 인라인 플래그의 기본값을 오버로드로 제공하면 사용성이 좋아집니다대부분
inline=false로 쓰이는 경우가 많으니, 아래와 같이 인라인 파라미터를 생략할 수 있는 팩토리 메서드를 추가해 보세요.public record DiscordEmbedField( String name, String value, boolean inline ) { + public static DiscordEmbedField newInstance(String name, String value) { + return new DiscordEmbedField(name, value, false); + } public static DiscordEmbedField newInstance(String name, String value, boolean inline) { return new DiscordEmbedField(name, value, inline); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
build.gradle(1 hunks)src/main/java/org/sopt/makers/global/constant/DiscordConstant.java(1 hunks)src/main/java/org/sopt/makers/global/exception/checked/MessageBuildException.java(1 hunks)src/main/java/org/sopt/makers/global/exception/checked/SendException.java(1 hunks)src/main/java/org/sopt/makers/global/exception/checked/SlackMessageBuildException.java(0 hunks)src/main/java/org/sopt/makers/global/exception/checked/SlackSendException.java(0 hunks)src/main/java/org/sopt/makers/global/exception/message/ErrorMessage.java(1 hunks)src/main/java/org/sopt/makers/global/util/EnvUtil.java(2 hunks)src/main/java/org/sopt/makers/global/util/FakeEnvUtil.java(1 hunks)src/main/java/org/sopt/makers/handler/SentryWebhookHandler.java(1 hunks)src/main/java/org/sopt/makers/service/DiscordNotificationService.java(1 hunks)src/main/java/org/sopt/makers/service/NotificationContext.java(1 hunks)src/main/java/org/sopt/makers/service/NotificationProcessorService.java(1 hunks)src/main/java/org/sopt/makers/service/NotificationService.java(2 hunks)src/main/java/org/sopt/makers/service/NotificationServiceFactory.java(1 hunks)src/main/java/org/sopt/makers/service/SentryEventExtractorService.java(1 hunks)src/main/java/org/sopt/makers/service/SlackNotificationService.java(4 hunks)src/main/java/org/sopt/makers/vo/discord/embed/DiscordEmbed.java(1 hunks)src/main/java/org/sopt/makers/vo/discord/message/DiscordMessage.java(1 hunks)src/test/java/org/sopt/makers/global/util/EnvUtilTest.java(6 hunks)src/test/java/org/sopt/makers/handler/FakeSentryPayload.java(1 hunks)src/test/java/org/sopt/makers/service/SentryEventExtractorServiceTest.java(1 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/org/sopt/makers/global/exception/checked/SlackMessageBuildException.java
- src/main/java/org/sopt/makers/global/exception/checked/SlackSendException.java
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/test/java/org/sopt/makers/service/SentryEventExtractorServiceTest.java (1)
src/test/java/org/sopt/makers/handler/FakeSentryPayload.java (1)
FakeSentryPayload(3-424)
src/main/java/org/sopt/makers/service/NotificationService.java (2)
src/main/java/org/sopt/makers/global/exception/checked/MessageBuildException.java (1)
MessageBuildException(5-13)src/main/java/org/sopt/makers/global/exception/checked/SendException.java (1)
SendException(5-13)
src/main/java/org/sopt/makers/service/SentryEventExtractorService.java (1)
src/main/java/org/sopt/makers/global/exception/unchecked/InvalidSlackPayloadException.java (1)
InvalidSlackPayloadException(5-13)
src/main/java/org/sopt/makers/service/NotificationServiceFactory.java (2)
src/main/java/org/sopt/makers/global/exception/unchecked/UnsupportedServiceTypeException.java (1)
UnsupportedServiceTypeException(5-13)src/main/java/org/sopt/makers/service/SlackNotificationService.java (1)
Slf4j(38-163)
src/main/java/org/sopt/makers/global/util/EnvUtil.java (1)
src/main/java/org/sopt/makers/global/exception/unchecked/InvalidEnvParameterException.java (1)
InvalidEnvParameterException(5-14)
🔇 Additional comments (23)
src/main/java/org/sopt/makers/global/util/FakeEnvUtil.java (3)
10-10: 클래스 이름이 적절하게 변경되었습니다.
TestEnvUtil에서FakeEnvUtil로 이름 변경은 테스트 목적으로 사용되는 유틸리티 클래스의 의도를 더 명확하게 표현합니다. 'Fake'라는 네이밍은 테스트 더블 패턴에서 실제 환경 대신 사용되는 목적을 더 정확히 나타냅니다.
28-47: 웹훅 URL을 가져오는 로직이 적절합니다.
getWebhookUrl메소드는 서비스 유형, 팀, 스테이지, 타입에 따라 적절한 웹훅 URL을 반환하며, 유효하지 않은 매개변수, 지원되지 않는 서비스 타입, URL을 찾을 수 없는 경우에 대한 예외 처리가 잘 구현되어 있습니다.
49-55: Discord 웹훅 지원이 적절하게 구현되어 있습니다.
resolvePrefix메소드에서 Slack과 Discord 서비스 유형을 모두 지원하고 있어, Discord 웹훅 기능 추가 목적에 부합합니다. 지원되지 않는 서비스 타입에 대한 예외 처리도 적절히 구현되어 있습니다.build.gradle (2)
40-40: JUnit Platform Launcher 의존성 추가가 적절합니다.JUnit Platform Launcher 추가는 JUnit 테스트의 실행과 결과 처리를 위한 API를 제공하여 테스트 자동화와 IDE 통합을 향상시킵니다. 이는 새로운 서비스 클래스와 유틸리티를 위한 테스트 환경을 개선하는 데 도움이 됩니다.
42-42: Mockito JUnit Jupiter 의존성 추가가 적절합니다.Mockito JUnit Jupiter는 JUnit 5와 Mockito를 통합하는 확장 기능을 제공합니다. 이는 모킹 프레임워크를 사용하여 Discord 웹훅 기능과 같은 새로운 서비스를 효과적으로 테스트하는 데 필요합니다.
src/test/java/org/sopt/makers/global/util/EnvUtilTest.java (3)
22-22: Dotenv 임포트 추가가 적절합니다.테스트에서 환경 변수를 명시적으로 설정하기 위해 Dotenv 라이브러리를 임포트한 것은 적절합니다.
37-40: Dotenv 구성 및 로드가 적절합니다.테스트 리소스 디렉토리에서 Dotenv를 구성하고 로드하는 방식이 명확합니다.
52-52: EnvUtil 직접 호출로 변경이 일관되게 적용되었습니다.모든 테스트 케이스에서
FakeEnvUtil대신EnvUtil.getWebhookUrl을 직접 호출하도록 일관되게 변경되었습니다. 이는EnvUtil의 리팩토링과 잘 맞추어져 있습니다.Also applies to: 61-61, 71-71, 80-80, 96-97, 101-101
src/main/java/org/sopt/makers/global/exception/checked/MessageBuildException.java (3)
1-14: 전략 패턴을 위한 일반화된 예외 클래스 구현이 적절합니다.
MessageBuildException클래스는 Slack 특정 예외를 대체하는 일반화된 예외로, 전략 패턴을 사용한 다양한 알림 서비스에서 메시지 빌드 오류를 처리하는 데 적합합니다. 생성자와 팩토리 메서드 패턴(from)을 통해 코드 재사용성과 가독성이 향상되었습니다.
5-9: 명확한 예외 계층 구조 설계가 잘 되었습니다.
SentryCheckedException을 상속받아 예외 계층 구조가 명확하게 설계되었으며,BaseErrorCode를 파라미터로 받는 생성자를 통해 오류 코드를 체계적으로 관리할 수 있습니다.
10-12: 팩토리 메서드 패턴 적용이 적절합니다.정적 팩토리 메서드
from을 제공하여 예외 생성을 캡슐화하고, 코드의 가독성과 사용성을 향상시켰습니다.src/main/java/org/sopt/makers/global/exception/checked/SendException.java (1)
1-13: 적절한 예외 클래스 구현예외 처리 일반화를 위한
SendException클래스가 적절히 구현되었습니다.SentryCheckedException을 확장하고BaseErrorCode를 받는 생성자와 정적 팩토리 메소드를 제공하여 코드 일관성을 유지했습니다. Slack과 Discord 등 여러 알림 서비스에서 재사용 가능한 방식으로 설계된 것이 좋습니다.src/main/java/org/sopt/makers/service/NotificationServiceFactory.java (2)
19-32: 팩토리 패턴 개선이 잘 되었습니다.
ObjectMapper를 주입받을 수 있도록 팩토리 패턴을 개선한 것이 좋습니다. 서비스 인스턴스 생성을 공급자 함수로 변경하여 유연성을 높였습니다. Discord 서비스 등록도 잘 되어 있습니다.몇 가지 제안:
- 클래스와 메서드에 Javadoc 주석을 추가하여 팩토리의 역할과 사용법을 설명하는 것이 좋을 것 같습니다.
- 추후 더 많은 의존성이 필요해질 경우를 대비하여 확장성을 고려해보세요.
34-36: 헬퍼 메서드 도입이 좋습니다.서비스 등록을 위한 헬퍼 메서드
registerService를 도입한 것이 코드의 가독성과 유지보수성을 향상시켰습니다.src/main/java/org/sopt/makers/service/DiscordNotificationService.java (1)
64-74:Details
❌ Incorrect review comment
InterruptedException누락 및 공통 예외 래핑 제안
HttpClientUtil.sendPost가java.net.http.HttpClient를 내부적으로 사용한다면InterruptedException이 발생할 수 있습니다.
현재IOException만 캐치하고 있어 스레드 인터럽트 시 상위로 unchecked 예외가 전파될 가능성이 있습니다.
또한IOException→SendException매핑이 중복되므로 공통 처리 메서드로 추출하면 가독성이 개선됩니다.- } catch (IOException e) { - throw SendException.from(ErrorMessage.DISCORD_SERIALIZATION_FAILED); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); // 인터럽트 상태 복구 + throw SendException.from(ErrorMessage.DISCORD_SEND_FAILED); + } catch (IOException e) { + throw SendException.from(ErrorMessage.DISCORD_SERIALIZATION_FAILED); }
🏁 Script executed:
#!/bin/bash # Locate HttpClientUtil class and inspect its sendPost method signature rg --context 3 "class HttpClientUtil" -n rg --context 3 "sendPost" -n srcLength of output: 3271
잘못된 리뷰:
InterruptedException은 이미HttpClientUtil.sendPost에서 처리됩니다.
HttpClientUtil.sendPost(String, String, String)내부에서IOException과InterruptedException을 함께 캐치하여
HttpRequestException으로 래핑하고 있습니다.- 따라서 서비스 레이어(
DiscordNotificationService)에서 별도로InterruptedException을 잡아Thread.currentThread().interrupt()
상태 복구를 할 필요가 없습니다.리뷰어 제안대로
InterruptedException을 추가로 처리하면 예외가 중복 래핑되거나 불필요한 코드가 추가됩니다.
공통 예외 래핑 메서드 추출 제안은 유지할 수 있으나, 인터럽트 처리 제안은 무시해 주세요.Likely an incorrect or invalid review comment.
src/main/java/org/sopt/makers/handler/SentryWebhookHandler.java (1)
38-41: Request Body NPE 및 추출 실패 케이스를 명시적으로 처리하세요
apiGatewayEvent.getBody()가null이거나eventExtractorService.extractEvent()가 예외 없이null을 반환하는 경우를 대비한 방어 로직이 없습니다.
잘못된/빈 페이로드가 들어오면 200 OK 가 아닌 400 Bad Request 로 응답하도록 한번 더 검증해 주세요.String body = Objects.requireNonNullElse(apiGatewayEvent.getBody(), ""); SentryEventDetail sentryEvent = eventExtractorService.extractEvent(body); if (sentryEvent == null) { throw new SentryUncheckedException(ErrorMessage.INVALID_SENTRY_PAYLOAD); }src/main/java/org/sopt/makers/service/SlackNotificationService.java (5)
19-20: 의존성 수정이 적절하게 이루어졌습니다.Slack 특정 예외 대신 더 일반적인
MessageBuildException과SendException을 사용함으로써 여러 알림 서비스에 일관된 예외 처리 방식을 적용할 수 있게 되었습니다.
35-39: Lombok 애노테이션을 적절히 사용하였습니다.
@RequiredArgsConstructor를 사용하여 코드가 간결해졌습니다. 명시적인 생성자 대신 Lombok 기능을 활용하는 것은 좋은 접근 방식입니다.
57-57: 예외 처리 개선이 잘 이루어졌습니다.Slack 특정 예외에서 일반적인
MessageBuildException으로 변경되어 다양한 알림 서비스를 위한 확장성이 개선되었습니다.
69-72: 예외 처리 흐름이 개선되었습니다.이전의 Slack 특정 예외에서 더 일반적인 예외로 변경하면서 적절한 오류 메시지를 전달하고 있습니다. 이 변경은 다양한 알림 서비스를 지원하는 아키텍처에 적합합니다.
76-82: 응답 처리 메서드의 예외 타입이 적절히 업데이트되었습니다.
handleSlackResponse메서드의 예외 타입이 일반적인SendException으로 변경되었습니다. 응답 처리 로직은 그대로 유지하면서 예외 타입만 변경한 것이 좋습니다.src/main/java/org/sopt/makers/global/exception/message/ErrorMessage.java (2)
23-29: Discord 관련 오류 메시지가 적절히 추가되었습니다.Slack 오류 메시지 패턴과 일치하도록 Discord 관련 오류 메시지를 추가한 것이 좋습니다. 에러 코드 접두사를 'D'로 설정하여 서비스 유형을 명확하게 구분했습니다.
32-32: 공통 오류 섹션으로 서비스 타입 관련 오류를 이동한 것이 적절합니다.
UNSUPPORTED_SERVICE_TYPE오류를 공통 오류 섹션으로 이동하고 코드 접두사를 'C'로 변경한 것은 논리적입니다. 이는 특정 서비스에 한정되지 않는 오류이므로 공통 섹션에 위치하는 것이 적합합니다.
src/test/java/org/sopt/makers/service/SentryEventExtractorServiceTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/sopt/makers/service/SentryEventExtractorService.java
Outdated
Show resolved
Hide resolved
- 테스트 환경에서 dotenv 경로를 setter로 주입하도록 변경하며 관련 유틸 클래스를 제거합니다
- Discord 웹훅 알림 전송 테스트 추가 (embeds 필드 검증) - Slack 웹훅 알림 전송 테스트 추가 (attachments 필드 검증) - 다양한 Sentry 레벨별 이벤트 처리 테스트 추가 (fatal, warning, info) - API 에러 응답 및 Rate Limiting 처리 테스트 추가 - 네트워크 타임아웃 및 잘못된 파라미터 처리 테스트 추가
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/sopt/makers/service/DiscordNotificationService.java (1)
175-178:⚠️ Potential issueHEX → int 변환 시 예외 처리 누락
과거 리뷰에서 이미 동일한 지적이 있었지만(
NumberFormatException→MessageBuildException래핑), 아직 반영되지 않았습니다. 잘못된 HEX 문자열이 들어오면 서비스 전체가 런타임 예외로 중단될 수 있습니다. 아래와 같이 예외를 잡아서MessageBuildException으로 변환해 주세요.-private int convertHexColorToInt(String hexColor) { - String cleanHex = hexColor.startsWith("#") ? hexColor.substring(1) : hexColor; - return Integer.parseInt(cleanHex, 16); +private int convertHexColorToInt(String hexColor) throws MessageBuildException { + try { + String cleanHex = hexColor.startsWith("#") ? hexColor.substring(1) : hexColor; + return Integer.parseInt(cleanHex, 16); + } catch (NumberFormatException e) { + log.error("HEX → int 변환 실패: {}", hexColor, e); + throw MessageBuildException.from(ErrorMessage.DISCORD_MESSAGE_BUILD_FAILED); + } }
🧹 Nitpick comments (5)
src/main/java/org/sopt/makers/service/DiscordNotificationService.java (2)
64-68: 전체 Payload 로그는 개인정보·용량 문제를 일으킬 수 있습니다
jsonPayload전체를 INFO 레벨로 남기면
- Sentry 이벤트에 포함된 사용자 데이터가 로그로 노출될 위험
- 대용량 메시지로 인한 로그 저장 비용 증가
가 있습니다. 필요하다면 DEBUG 레벨로 낮추거나, 필터링/마스킹 후 부분만 기록하는 방식을 고려해 주세요.
124-135: 제목 길이 계산 식 가독성 개선 제안현재
substring길이 계산이maxContentLength - EMOJI_PREFIX.length() - 1과 같이 한 줄에 섞여 있어 가독성이 떨어집니다. 의미-있는 상수(예:EMOJI_WITH_SPACE_LENGTH)를 두면 의도를 더 명확히 전달할 수 있습니다.src/test/java/org/sopt/makers/handler/SentryWebhookHandlerTest.java (3)
286-301: 10 초 고정 지연은 테스트 속도를 과도하게 증가시킵니다
withFixedDelay(10000)로 인해 네트워크 타임아웃 시나리오 테스트마다 10 초를 소모합니다.
- CI 파이프라인 병목
- 로컬 테스트 불편
chunkedDribbleDelay또는 1-2 초 지연 + 커스텀 타임아웃 설정으로도 충분히 시나리오를 재현할 수 있습니다. 시간을 대폭 줄여 보세요.
285-303: 테스트 클래스 구분이 실제 호출 대상과 불일치
SlackNotificationTests내부에서 Discord API(/api/webhooks/...) 타임아웃 시나리오를 다루고 있어 독자가 혼란스럽습니다.
- 위치를
DiscordNotificationTests로 이동- 또는 메서드명을
shouldHandleNetworkTimeoutWithDiscord등으로 변경해당 테스트가 어떤 서비스 경로를 검증하는지 명확히 해 주세요.
461-468:String#replace사용은 레벨 필드가 중복될 때 오작동 가능
"\"level\": \"error\""문자열을 단순 치환하면 JSON 안에 동일 패턴이 여럿 있을 경우 전부 변경됩니다. 안전하게 파싱-직렬화 방식으로 수정하세요. 예)ObjectMapper로 Map → 수정 → 다시 String.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
build.gradle(1 hunks)src/main/java/org/sopt/makers/global/constant/DiscordConstant.java(1 hunks)src/main/java/org/sopt/makers/global/exception/message/ErrorMessage.java(1 hunks)src/main/java/org/sopt/makers/service/DiscordNotificationService.java(1 hunks)src/test/java/org/sopt/makers/handler/SentryWebhookHandlerTest.java(1 hunks)src/test/java/org/sopt/makers/service/SentryEventExtractorServiceTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- build.gradle
- src/test/java/org/sopt/makers/service/SentryEventExtractorServiceTest.java
- src/main/java/org/sopt/makers/global/constant/DiscordConstant.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/org/sopt/makers/handler/SentryWebhookHandlerTest.java (1)
src/test/java/org/sopt/makers/handler/FakeSentryPayload.java (1)
FakeSentryPayload(3-424)
Related issue 🛠
Work Description ✏️
신규 기능
Consideration 🤔
전략패턴과 팩토리패턴 도입 배경: 멀티 플랫폼 알림 지원 요구사항
초기 상황 및 확장 필요성
전략패턴 도입
팩토리패턴 도입 과정
설계 패턴 선택의 고민
실용성을 고려한 최종 결정
결론
WireMock 도입 배경: 실제 외부 API 호출 문제 해결
기존 테스트의 한계
WireMock 도입 결정
결론
Result 🏆
Trouble Shooting ⚽️