Conversation
✅ 테스트 결과 for PRBuild: success 🧪 테스트 실행 with Gradle |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
dev-ant
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request implements a user verification message filter feature for the HornBugle (뿔피리/horn bugle) messaging system. The changes introduce Kafka event publishing to detect and process user verification codes embedded in horn bugle messages, along with comprehensive import reorganization across the entire codebase.
Changes:
- Added Kafka integration for publishing user verification events when specific message patterns are detected
- Refactored Elasticsearch operations from repository pattern to direct ElasticsearchOperations usage
- Reorganized imports across 150+ files following a consistent pattern (static imports → java.* → third-party)
Reviewed changes
Copilot reviewed 154 out of 155 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| HornBugleKafkaProducerService.java | New Kafka producer service for user verification events |
| UserVerificationVerifyEvent.java | New DTO for verification event data |
| HornBugleScheduler.java | Added regex pattern matching and event publishing for verification codes |
| HornBugleService.java | Added method to return saved entities for event publishing |
| HornBugleIndexService.java | Replaced repository with direct ElasticsearchOperations calls |
| HornBugleElasticsearchConfig.java | Removed @EnableElasticsearchRepositories annotation |
| HornBugleIndexRunner.java | Updated conditional activation to require both ES flags |
| KafkaTopicConstant.java | New constants class for Kafka topic names |
| application.yml | Added Kafka configuration (bootstrap servers, serializers, topics) |
| application-sample.yml | Added sample Kafka configuration |
| docker-compose-local.yml | Added Kafka bootstrap servers and hostname resolution |
| build.gradle.kts | Added spring-kafka dependency |
| 140+ other files | Import reorganization (static first, then java.*, then third-party) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public class KafkaTopicConstant { | ||
|
|
||
| public static final String USER_VERIFICATION_VERIFY_EVENT = "USER_VERIFICATION_VERIFY_EVENT"; | ||
|
|
||
| private KafkaTopicConstant() {} | ||
| } |
There was a problem hiding this comment.
The constructor for KafkaTopicConstant is private, which is correct for a utility class. However, the class should be declared as final to prevent inheritance and make the design intent clearer that this is purely a constants holder class.
| for (HornBugleDocument document : batch) { | ||
| elasticsearchOperations.save(document, IndexCoordinates.of(INDEX_NAME)); | ||
| } |
There was a problem hiding this comment.
The batch indexing logic has changed from repository.saveAll(batch) to iterating and calling elasticsearchOperations.save() individually for each document. This is less efficient than a true bulk operation. Consider using elasticsearchOperations.bulkIndex() or elasticsearchOperations.save() with a collection parameter if available, to maintain the performance benefits of batch operations.
| public void sendUserVerificationVerifyEvent(UserVerificationVerifyEvent event) { | ||
| kafkaTemplate.send(userVerificationVerifyTopic, event); | ||
| } |
There was a problem hiding this comment.
The method sendUserVerificationVerifyEvent in HornBugleKafkaProducerService does not handle the asynchronous result returned by kafkaTemplate.send(). The send operation returns a CompletableFuture that should be checked for success or failure. Consider adding error handling with a callback to log failures, for example using .whenComplete() or .exceptionally().
| kafka: | ||
| bootstrap-servers: ${KAFKA_BOOTSTRAP_SERVERS} |
There was a problem hiding this comment.
The Kafka bootstrap servers configuration uses an environment variable without a default value. If KAFKA_BOOTSTRAP_SERVERS is not set, the application will fail to start. Consider providing a default value (e.g., localhost:9092) for local development or add validation to fail gracefully with a clear error message.
| # 로컬 개발 시 설정 파일 마운트 (선택사항) | ||
| # - ./config:/app/config:ro | ||
| extra_hosts: | ||
| - "devnogi.ddns.net:119.194.235.11" # Kafka advertised hostname 임시 해석 |
There was a problem hiding this comment.
The extra_hosts configuration maps devnogi.ddns.net to a specific IP address. This is hardcoded and environment-specific, which could cause issues in other deployment environments. Consider using environment variables for both the hostname and IP address to make this configuration more flexible and portable across different environments.
| - "devnogi.ddns.net:119.194.235.11" # Kafka advertised hostname 임시 해석 | |
| - "${KAFKA_ADVERTISED_HOSTNAME:-devnogi.ddns.net}:${KAFKA_ADVERTISED_IP:-119.194.235.11}" # Kafka advertised hostname 임시 해석 (env-overridable) |
| @Service | ||
| @RequiredArgsConstructor | ||
| public class HornBugleKafkaProducerService { | ||
|
|
||
| private final KafkaTemplate<String, Object> kafkaTemplate; | ||
|
|
||
| @Value( | ||
| "${app.kafka.topics.user-verification-verify:" | ||
| + KafkaTopicConstant.USER_VERIFICATION_VERIFY_EVENT | ||
| + "}") | ||
| private String userVerificationVerifyTopic; | ||
|
|
||
| public void sendUserVerificationVerifyEvent(UserVerificationVerifyEvent event) { | ||
| kafkaTemplate.send(userVerificationVerifyTopic, event); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new Kafka producer service HornBugleKafkaProducerService and the verification event publishing logic in HornBugleScheduler.publishVerificationEvents() lack test coverage. Given that other services in the codebase have comprehensive test coverage (e.g., AuctionHistoryServiceTest, ItemInfoServiceTest), these new critical features should also have unit tests to ensure reliability.
| /** Elasticsearch 설정 클래스. elasticsearch.enabled=true일 때만 관련 구성을 활성화한다. */ | ||
| @Configuration | ||
| @ConditionalOnProperty(name = "elasticsearch.enabled", havingValue = "true", matchIfMissing = false) | ||
| @EnableElasticsearchRepositories( | ||
| basePackages = "until.the.eternity.hornBugle.infrastructure.elasticsearch") | ||
| public class HornBugleElasticsearchConfig {} |
There was a problem hiding this comment.
The configuration annotation @EnableElasticsearchRepositories has been removed from this config class. Since HornBugleElasticsearchRepository is still present in the codebase (at src/main/java/until/the/eternity/hornBugle/infrastructure/elasticsearch/HornBugleElasticsearchRepository.java), it appears this repository interface should either be deleted or this configuration should remain. As the repository is no longer being used in HornBugleIndexService, the repository interface should be deleted to maintain consistency.
| private void publishVerificationEvents(List<HornBugleWorldHistory> savedHistories) { | ||
| for (HornBugleWorldHistory history : savedHistories) { | ||
| Matcher matcher = CERTIFICATE_PATTERN.matcher(history.getMessage()); | ||
|
|
||
| while (matcher.find()) { | ||
| String verificationValue = matcher.group(1); | ||
|
|
||
| UserVerificationVerifyEvent event = | ||
| new UserVerificationVerifyEvent( | ||
| history.getCharacterName(), | ||
| history.getServerName(), | ||
| verificationValue, | ||
| history.getMessage(), | ||
| history.getDateSend()); | ||
|
|
||
| hornBugleKafkaProducerService.sendUserVerificationVerifyEvent(event); | ||
|
|
||
| log.info( | ||
| "[HornBugle] Verification event published. characterName={}, serverName={}, verificationValue={}", | ||
| history.getCharacterName(), | ||
| history.getServerName(), | ||
| verificationValue); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new method publishVerificationEvents lacks error handling. If the Kafka producer fails to send an event, the exception will propagate and potentially disrupt the scheduler. Consider wrapping the sendUserVerificationVerifyEvent call in a try-catch block to handle Kafka failures gracefully and log errors without stopping the entire scheduler execution.
| private final HornBugleKafkaProducerService hornBugleKafkaProducerService; | ||
|
|
||
| private static final long RATE_LIMIT_DELAY_MS = 1000L; | ||
| private static final Pattern CERTIFICATE_PATTERN = Pattern.compile("메모노기_([A-Z0-9]{20})"); |
There was a problem hiding this comment.
The regex pattern 메모노기_([A-Z0-9]{20}) assumes verification codes are exactly 20 characters of uppercase letters and digits. Consider validating this assumption and documenting it. If the verification code format might change in the future, consider making the pattern configurable via application properties rather than hard-coding it.
| @ConditionalOnProperty( | ||
| name = "elasticsearch.index.enabled", | ||
| name = {"elasticsearch.enabled", "elasticsearch.index.enabled"}, | ||
| havingValue = "true", | ||
| matchIfMissing = false) |
There was a problem hiding this comment.
The @ConditionalOnProperty condition has been changed to require both elasticsearch.enabled and elasticsearch.index.enabled to be true. This is a breaking change in behavior. Previously, only elasticsearch.index.enabled was required. Now, if only elasticsearch.index.enabled is set to true without elasticsearch.enabled, this runner won't activate. This should be documented or reconsidered to maintain backward compatibility.
1 participant
📋 상세 설명
📊 체크리스트