[UPLUS-29] api-message 초기 구현#1
Conversation
github-actions
Bot
added
size/XL
Summary of ChangesHello @swthewhite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 새로운 알림 서비스인 api-message를 도입합니다. 알림 이벤트를 소비하기 위한 Kafka 통합, 이메일-SMS 폴백 기능이 있는 알림 처리 파이프라인, 로깅 및 템플릿을 위한 데이터베이스 엔티티와 리포지토리, 모니터링을 위한 메트릭 등 상당한 변경 사항을 포함하고 있습니다. 전반적인 아키텍처는 견고하지만, 메시지 처리 신뢰성, 트랜잭션 관리, 보안과 관련된 몇 가지 중요하고 심각도가 높은 이슈들이 있어 해결이 필요합니다. 또한 코드 품질과 유지보수성을 개선하기 위한 몇 가지 제안 사항도 포함했습니다.
| org.hibernate.SQL: WARN | ||
|
|
||
| ureca: | ||
| secret-key: ${AES_SECRET_KEY:12345678901234567890123456789012} |
There was a problem hiding this comment.
설정 파일에 기본 하드코딩된 AES 비밀 키가 포함되어 있습니다. 이는 심각한 보안 취약점입니다. 기본 키는 절대로 사용해서는 안 되며, 특히 프로덕션 환경에서는 더욱 그렇습니다. 이 키는 소스 코드에서 제거하고 환경별 비밀값으로 안전하게 관리해야 합니다.
secret-key: ${AES_SECRET_KEY}| } catch (JsonProcessingException e) { | ||
| log.error("Failed to deserialize message. value: {}", record.value(), e); | ||
| } catch (Exception e) { | ||
| log.error("Failed to process notification. value: {}", record.value(), e); | ||
| } finally { | ||
| ack.acknowledge(); | ||
| } |
There was a problem hiding this comment.
현재 구현은 처리 실패 시에도 finally 블록에서 Kafka 메시지를 acknowledge 하고 있습니다. 이러한 "at-most-once" 전송 의미론은 처리 중 일시적인 오류(예: 데이터베이스 연결 문제)가 발생할 경우 메시지 손실로 이어질 수 있습니다. "at-least-once" 전송을 보장하고 메시지 손실을 방지하려면, 메시지가 성공적으로 처리된 후에만 acknowledge를 수행해야 합니다. 복구 불가능한 오류의 경우, 데드-레터 큐(DLQ) 전략을 고려할 수 있습니다.
| } catch (JsonProcessingException e) { | |
| log.error("Failed to deserialize message. value: {}", record.value(), e); | |
| } catch (Exception e) { | |
| log.error("Failed to process notification. value: {}", record.value(), e); | |
| } finally { | |
| ack.acknowledge(); | |
| } | |
| } catch (JsonProcessingException e) { | |
| log.error("Failed to deserialize message. value: {}", record.value(), e); | |
| ack.acknowledge(); // 복구 불가능한 오류는 acknowledge하여 무한 재시도를 방지합니다. | |
| } catch (Exception e) { | |
| log.error("Failed to process notification. value: {}", record.value(), e); | |
| // 일시적인 오류에 대해 Kafka가 메시지를 재전송하도록 여기서 acknowledge하지 않습니다. | |
| } |
| private final Timer emailProcessingTimer; | ||
| private final Timer smsProcessingTimer; | ||
|
|
||
| @Transactional |
There was a problem hiding this comment.
processNotification 메서드에 @Transactional 어노테이션이 있습니다. 이로 인해 기본 알림(이메일)이 실패하여 로그가 기록된 후, 후속 폴백(SMS) 처리도 실패하면 전체 트랜잭션이 롤백됩니다. 이 경우 초기 이메일 실패에 대한 로그 항목이 손실될 수 있습니다. 각 메시지 로그가 독립적으로 영속되도록 하려면, 별도의 트랜잭션에서 저장해야 합니다.
saveMessageLog 메서드(또는 로깅 전용 서비스의 새 메서드)에 @Transactional(propagation = Propagation.REQUIRES_NEW) 어노테이션을 추가하여, 부모 트랜잭션의 결과와 상관없이 각 로그가 자체 트랜잭션에 저장되도록 하는 것을 고려해 보세요.
| @KafkaListener( | ||
| id = "notification-consumer", | ||
| topics = "notification_topic", | ||
| groupId = "notification-consumer-group", | ||
| containerFactory = "kafkaListenerContainerFactory") |
There was a problem hiding this comment.
notification.consumer.concurrency 설정 속성이 정의되고 주입되었지만, @KafkaListener의 동시성을 설정하는 데 사용되지 않고 있습니다. 리스너는 기본 동시성 값인 1로 실행됩니다.
@KafkaListener(
id = "notification-consumer",
topics = "notification_topic",
groupId = "notification-consumer-group",
containerFactory = "kafkaListenerContainerFactory",
concurrency = "${notification.consumer.concurrency:1}")| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private NotificationRequestEvent parseEvent(Map<String, Object> rawPayload) { |
There was a problem hiding this comment.
parseEvent 메서드는 Map<String, Object>에서 수동으로 필드를 추출합니다. 이 방식은 깨지기 쉽고 타입-세이프하지 않습니다. 더 나은 접근 방식은 Kafka 메시지 페이로드를 NotificationRequestEvent DTO로 직접 역직렬화하는 것입니다. 이렇게 하면 코드가 더 깔끔하고 견고하며 유지보수하기 쉬워집니다.
ConcurrentKafkaListenerContainerFactory가 값에 대해 JsonDeserializer를 사용하도록 설정하고, 리스너의 시그니처를 consume(ConsumerRecord<String, NotificationRequestEvent> record, ...)로 변경할 수 있습니다. 이렇게 하면 parseEvent 메서드가 필요 없어집니다.
| @Query( | ||
| "SELECT tv FROM TemplateVersion tv " | ||
| + "WHERE tv.templateGroup.groupCode = :groupCode " | ||
| + "AND tv.channel = :channel " | ||
| + "AND tv.status = :status " | ||
| + "ORDER BY tv.version DESC " | ||
| + "LIMIT 1") | ||
| Optional<TemplateVersion> findLatestByGroupCodeAndChannelAndStatus( | ||
| @Param("groupCode") String groupCode, | ||
| @Param("channel") Channel channel, | ||
| @Param("status") TemplateStatus status); | ||
| } |
There was a problem hiding this comment.
쿼리에서 LIMIT 1을 사용하고 있는데, 이는 표준 JPQL이 아니므로 모든 JPA 프로바이더나 데이터베이스에서 호환되지 않을 수 있습니다. Spring Data JPA의 쿼리 파생 기능을 사용하는 것이 더 표준적이고 이식성 있는 방법입니다. @Query 어노테이션을 제거하고 쿼리 메서드를 정의하는 것이 좋습니다. 이렇게 하면 코드가 더 깔끔해지고 JPA 표준을 따르게 됩니다. 이 변경 사항을 적용하려면 TemplateVersionRepositoryImpl에서 이 메서드를 호출하도록 수정해야 합니다.
Optional<TemplateVersion> findFirstByTemplateGroupGroupCodeAndChannelAndStatusOrderByVersionDesc(
String groupCode, Channel channel, TemplateStatus status);| private String truncateBody(String body) { | ||
| if (body == null) { | ||
| return null; | ||
| } | ||
| if (body.length() > 100) { | ||
| return body.substring(0, 100) + "..."; | ||
| } | ||
| return body; | ||
| } |
There was a problem hiding this comment.
truncateBody 메서드가 EmailSender와 SmsSender 양쪽에 중복되어 있습니다. 이 중복된 로직은 DRY(Don't Repeat Yourself) 원칙에 따라 공통 유틸리티 클래스로 추출해야 합니다.
MessageUtils와 같은 유틸리티 클래스를 만들고 정적 truncate 메서드를 추가한 다음, 두 sender 클래스에서 이를 호출하도록 수정하세요.
| import io.micrometer.core.instrument.Timer; | ||
|
|
||
| @Configuration | ||
| public class MetricsConfig { |
There was a problem hiding this comment.
Counter와 Timer 빈을 생성하는 데 상당한 양의 보일러플레이트 코드가 있습니다. 이 코드는 중복을 줄이고 유지보수성을 향상시키기 위해 리팩토링할 수 있습니다.
메트릭 생성 및 등록 로직을 중앙에서 관리하는 팩토리 빈이나 헬퍼 클래스를 만드는 것을 고려해 보세요. 예를 들어, getSuccessCounter(Channel channel) 및 getProcessingTimer(Channel channel)와 같은 메서드를 노출하는 단일 NotificationMetrics 빈을 가질 수 있으며, 존재하지 않을 경우 동적으로 생성할 수 있습니다. 이는 NotificationService에서의 의존성 주입도 단순화할 것입니다.
| SecretKeySpec keySpec = | ||
| new SecretKeySpec(secretKey.getBytes(StandardCharsets.UTF_8), "AES"); |
There was a problem hiding this comment.
encrypt 및 decrypt를 호출할 때마다 비밀 키 문자열로부터 새로운 SecretKeySpec이 생성됩니다. 이는 비효율적입니다. SecretKeySpec은 초기화 시 한 번만 생성하여 필드로 저장하고 재사용할 수 있습니다.
init() 메서드에서 SecretKeySpec을 생성하여 private 필드에 저장하고, 암호화/복호화 시 이 필드를 재사용하세요.
1 participant
🍀 이슈 번호
UPLUS-29
✅ 작업 사항
api-message 초기 구현
📋 체크리스트
⌨ 기타