refactor: 키워드 알림 EventListener 책임 분리#2237
Merged
Soundbar91 merged 29 commits intoMay 25, 2026
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 26 minutes and 39 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughRefactors keyword notification: replaces old per-user event with per-article and lost-item events, simplifies keyword extraction to title matching, adds ArticleKeywordUserMatcher, introduces dedicated notification services and listeners, updates repository queries and DTOs, and removes prior unit tests. ChangesKeyword Notification System Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
src/main/java/in/koreatech/koin/domain/community/keyword/service/ArticleKeywordUserMatcher.java (2)
24-49: 💤 Low valueAdd an empty-input short-circuit.
When
matchedKeywordsis empty, the repository query still executes (... IN ()), incurring a needless roundtrip and, depending on JPA provider settings, potentially generating an awkward SQL. Returning an empty map immediately is cheaper and explicit.♻️ Suggested guard
public Map<String, List<Integer>> findUserIdsByMatchedKeyword( KeywordCategory category, List<String> matchedKeywords ) { + if (matchedKeywords == null || matchedKeywords.isEmpty()) { + return Map.of(); + } Map<Integer, ArticleKeyword> keywordByUserId = new LinkedHashMap<>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/domain/community/keyword/service/ArticleKeywordUserMatcher.java` around lines 24 - 49, Add a short-circuit at the start of findUserIdsByMatchedKeyword: if the matchedKeywords list is null or empty, return an empty LinkedHashMap immediately to avoid calling articleKeywordUserMapRepository.findAllByArticleKeywordCategoryAndArticleKeywordKeywordIn with an empty collection; ensure you check the matchedKeywords parameter (and treat null as empty) before any repository call so the method returns quickly when there's nothing to query.
29-49: ⚖️ Poor tradeoffConsider letting
KeywordExtractorreturnArticleKeywordentities to avoid double-fetching.
KeywordExtractor.matchKeywordspaginates and loadsArticleKeywordentities, then returns only theirkeywordtext. This service then re-fetches the sameArticleKeywordrows via the newIN :keywordsquery (joined toArticleKeywordUserMap). For categories with many keywords or high publish frequency, this is a redundant DB hit per article.If you keep the current shape, this is fine; otherwise consider returning
List<ArticleKeyword>(or IDs) from the extractor and querying the user-map byarticleKeyword.id IN :ids, which is also a more selective index path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/domain/community/keyword/service/ArticleKeywordUserMatcher.java` around lines 29 - 49, KeywordExtractor.matchKeywords currently returns List<String> causing ArticleKeyword rows to be refetched via articleKeywordUserMapRepository.findAllByArticleKeywordCategoryAndArticleKeywordKeywordIn; change the extractor to return List<ArticleKeyword> (or List<Long> ids) so the service can use those directly and avoid the redundant lookup, then update the repository call to use articleKeyword IDs (e.g., findAllByArticleKeywordCategoryAndArticleKeywordIdIn) or compare ArticleKeyword entities directly (findAllByArticleKeywordCategoryAndArticleKeywordIn) and adapt the loop that builds keywordByUserId to use ArticleKeyword objects instead of re-querying by keyword text.src/main/java/in/koreatech/koin/domain/notification/model/NotificationFactory.java (1)
235-240: ⚡ Quick winURL-encode
keywordwhen building the scheme URI.
keywordis interpolated raw into the deep-link query string. If a user's keyword contains&,=,?,#, whitespace, or non-ASCII characters, the resulting URI is malformed and the downstreamLIKE :schemeUriPatterndedupe inNotificationRepository.findUserIdsBySchemeUriLikeAndUserIdInmay also under-/over-match. The same concern applies to the existinggenerateKeywordSchemeUrion line 232.♻️ Suggested fix
+ import java.net.URLEncoder; + import java.nio.charset.StandardCharsets; ... private String generateLostItemKeywordSchemeUri(MobileAppPath path, Integer eventId, String keyword) { if (keyword == null) { return generateSchemeUri(path, eventId); } - return String.format("%s?id=%d&keyword=%s", path.getPath(), eventId, keyword); + String encoded = URLEncoder.encode(keyword, StandardCharsets.UTF_8); + return String.format("%s?id=%d&keyword=%s", path.getPath(), eventId, encoded); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/domain/notification/model/NotificationFactory.java` around lines 235 - 240, The methods generateLostItemKeywordSchemeUri and generateKeywordSchemeUri interpolate raw keyword into the deep-link query and must URL-encode keyword before building the scheme URI; update both functions to return generateSchemeUri(path,eventId) when keyword is null, otherwise encode the keyword using UTF-8 (e.g., via URLEncoder.encode(keyword, StandardCharsets.UTF_8.name()) or equivalent) and use the encoded value in the String.format call so reserved characters and non-ASCII text produce a valid query string.src/main/java/in/koreatech/koin/common/event/LostItemKeywordEvent.java (1)
12-16: 💤 Low valueConsider sharing
MatchedKeywordUserswithKoreatechArticleKeywordEventif the shape is identical.Per the PR description, both events carry a
Map<String, List<Integer>> userIdsByKeyword. IfKoreatechArticleKeywordEventdefines its own nestedMatchedKeywordUsers, the duplication will drift over time. Hoisting the value object toin.koreatech.koin.common.event(or a sibling shared type) keeps both events aligned and lets cross-cutting concerns (validation, defensive copy) live in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/common/event/LostItemKeywordEvent.java` around lines 12 - 16, The nested record MatchedKeywordUsers in LostItemKeywordEvent duplicates the same shape used by KoreatechArticleKeywordEvent; hoist MatchedKeywordUsers out of the nested class into a shared top-level type in package in.koreatech.koin.common.event (or a sibling shared package), remove the nested declarations from LostItemKeywordEvent and KoreatechArticleKeywordEvent, update both classes to import and reference the new shared MatchedKeywordUsers type, and consolidate any validation or defensive-copy logic into that shared record so both events reuse the single canonical implementation.src/main/java/in/koreatech/koin/domain/notification/repository/NotificationSubscribeRepository.java (1)
31-43: ⚡ Quick winMethod name embeds "ArticleKeyword" but the query is reused for lost-item subscribes.
The query is fully parameterized over
subscribeType, so it serves bothARTICLE_KEYWORDandLOST_ITEM_KEYWORDflows. Naming itfindArticleKeywordSubscribesByUserIdInwill mislead future readers and grep-based audits when the lost-item service calls the same method.♻️ Suggested rename
- List<NotificationSubscribe> findArticleKeywordSubscribesByUserIdIn( + List<NotificationSubscribe> findActiveSubscribesByTypeAndUserIdIn( `@Param`("subscribeType") NotificationSubscribeType subscribeType, `@Param`("userIds") List<Integer> userIds );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/domain/notification/repository/NotificationSubscribeRepository.java` around lines 31 - 43, The repository method NotificationSubscribeRepository.findArticleKeywordSubscribesByUserIdIn is misnamed because the JPQL is generic over subscribeType; rename the method to a neutral, accurate name like findKeywordSubscribesByUserIdIn or findBySubscribeTypeAndUserIds (keeping the same parameters `@Param`("subscribeType") NotificationSubscribeType subscribeType and `@Param`("userIds") List<Integer> userIds) and update all call sites that reference findArticleKeywordSubscribesByUserIdIn (e.g., services handling ARTICLE_KEYWORD and LOST_ITEM_KEYWORD flows) to the new method name so behavior and query remain unchanged but the name reflects its generic use.src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java (2)
29-42: 💤 Low valuePagination index can be simplified.
PageRequest.of(offset / KEYWORD_BATCH_SIZE, KEYWORD_BATCH_SIZE)re-derives a page number from a manual offset counter. A directint page = 0; ... page++;is clearer and avoids the implicit invariant thatoffsetmust always be a multiple ofKEYWORD_BATCH_SIZE.♻️ Suggested simplification
- int offset = 0; + int page = 0; while (true) { - Pageable pageable = PageRequest.of(offset / KEYWORD_BATCH_SIZE, KEYWORD_BATCH_SIZE); + Pageable pageable = PageRequest.of(page, KEYWORD_BATCH_SIZE); List<ArticleKeyword> keywords = articleKeywordRepository.findAllByCategory(category, pageable); if (keywords.isEmpty()) { break; } for (ArticleKeyword keyword : keywords) { if (title.contains(keyword.getKeyword())) { matchedKeywords.add(keyword.getKeyword()); } } - offset += KEYWORD_BATCH_SIZE; + page++; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java` around lines 29 - 42, The pagination loop in KeywordExtractor uses an offset integer and computes the page as PageRequest.of(offset / KEYWORD_BATCH_SIZE, KEYWORD_BATCH_SIZE), which is confusing and fragile; change the loop to use an explicit page counter (e.g., int page = 0) and create the pageable with PageRequest.of(page, KEYWORD_BATCH_SIZE), incrementing page++ each iteration and removing the offset arithmetic; keep the same stop condition on keywords.isEmpty() and leave KEYWORD_BATCH_SIZE and articleKeywordRepository.findAllByCategory intact.
25-45: 💤 Low valueGuard against null
title.
title.contains(...)will NPE if a caller ever passes a null title. Adding a defensive early-return keeps this utility robust against upstream changes (e.g., a new article publisher path) and avoids paginating the keyword table just to throw.🛡️ Suggested guard
public List<String> matchKeywords(String title, KeywordCategory category) { + if (title == null || title.isBlank()) { + return List.of(); + } List<String> matchedKeywords = new ArrayList<>(); int offset = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java` around lines 25 - 45, In matchKeywords (KeywordExtractor) add a defensive null-check for the title parameter and return an empty List<String> immediately if title is null (to avoid NPE from title.contains(...)); locate the matchKeywords method and put the guard before any pagination or repository calls (before PageRequest.of(...) / articleKeywordRepository.findAllByCategory(...)) so the method exits early when title is null and avoids unnecessary DB paging using the existing matchedKeywords empty list as the return.src/main/java/in/koreatech/koin/domain/notification/service/ArticleKeywordNotificationService.java (1)
67-69: ⚡ Quick winBuild the dedupe pattern from
KEYWORD, not a string literal.
createNotification()derives the target URI fromKEYWORD, but this lookup hardcodes"keyword". If those ever diverge, duplicate suppression breaks and users can receive repeated notifications for the same article.Suggested change
private Set<Integer> getAlreadyNotifiedUserIds(Integer articleId, List<Integer> userIds) { return new HashSet<>(notificationRepository - .findUserIdsBySchemeUriLikeAndUserIdIn("keyword?id=%d&%%".formatted(articleId), userIds)); + .findUserIdsBySchemeUriLikeAndUserIdIn( + "%s?id=%d&%%".formatted(KEYWORD.getPath(), articleId), + userIds + )); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/domain/notification/service/ArticleKeywordNotificationService.java` around lines 67 - 69, getAlreadyNotifiedUserIds currently hardcodes "keyword" when building the scheme-uri LIKE pattern which can diverge from the KEYWORD used in createNotification; change the pattern construction to use the KEYWORD constant (e.g., build "%s?id=%d&%%" with KEYWORD and articleId) so the call to notificationRepository.findUserIdsBySchemeUriLikeAndUserIdIn uses the same source of truth as createNotification, preventing duplicate notifications.src/main/java/in/koreatech/koin/common/event/KoreatechArticleKeywordEvent.java (1)
24-28: ⚡ Quick winSnapshot the matched users before publishing the async event.
MatchedKeywordUserscurrently wraps the original mutableMap<String, List<Integer>>by reference. Because the listener runsAFTER_COMMITand@Async, later mutations can leak into delivery or trigger concurrent modification issues. Copy the map and inner lists when constructing the event.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/in/koreatech/koin/common/event/KoreatechArticleKeywordEvent.java` around lines 24 - 28, The event is passing the original mutable Map into MatchedKeywordUsers by reference, risking concurrent mutation during async AFTER_COMMIT delivery; update the construction so MatchedKeywordUsers receives a defensive deep copy (copy the outer Map and for each entry copy the inner List) instead of the original reference—either modify where new MatchedKeywordUsers(userIdsByKeyword) is called in KoreatechArticleKeywordEvent or add copying logic inside MatchedKeywordUsers' constructor to produce an immutable/independent Map<String,List<Integer>> snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/in/koreatech/koin/common/event/KoreatechArticleKeywordEvent.java`:
- Around line 6-10: KoreatechArticleKeywordEvent is missing authorId so
ArticleKeywordNotificationService cannot exclude notifying the post author; add
an Integer authorId field to the KoreatechArticleKeywordEvent record (same
pattern as LostItemKeywordEvent), update all places that construct the
KoreatechArticleKeywordEvent to pass the article's authorId, and update any
consumers (e.g., ArticleKeywordNotificationService) to read event.authorId() to
preserve the "exclude self" logic; ensure compilation by adjusting imports and
tests that construct or pattern-match the record.
In
`@src/main/java/in/koreatech/koin/domain/notification/eventlistener/LostItemKeywordEventListener.java`:
- Around line 13-15: The LostItemKeywordEventListener class is always active and
must be disabled for the test profile; add the same profile restriction used on
ArticleKeywordEventListener by annotating the LostItemKeywordEventListener class
with `@Profile`("!test") (and import
org.springframework.context.annotation.Profile) so the async AFTER_COMMIT
listener is not active during tests.
---
Nitpick comments:
In
`@src/main/java/in/koreatech/koin/common/event/KoreatechArticleKeywordEvent.java`:
- Around line 24-28: The event is passing the original mutable Map into
MatchedKeywordUsers by reference, risking concurrent mutation during async
AFTER_COMMIT delivery; update the construction so MatchedKeywordUsers receives a
defensive deep copy (copy the outer Map and for each entry copy the inner List)
instead of the original reference—either modify where new
MatchedKeywordUsers(userIdsByKeyword) is called in KoreatechArticleKeywordEvent
or add copying logic inside MatchedKeywordUsers' constructor to produce an
immutable/independent Map<String,List<Integer>> snapshot.
In `@src/main/java/in/koreatech/koin/common/event/LostItemKeywordEvent.java`:
- Around line 12-16: The nested record MatchedKeywordUsers in
LostItemKeywordEvent duplicates the same shape used by
KoreatechArticleKeywordEvent; hoist MatchedKeywordUsers out of the nested class
into a shared top-level type in package in.koreatech.koin.common.event (or a
sibling shared package), remove the nested declarations from
LostItemKeywordEvent and KoreatechArticleKeywordEvent, update both classes to
import and reference the new shared MatchedKeywordUsers type, and consolidate
any validation or defensive-copy logic into that shared record so both events
reuse the single canonical implementation.
In
`@src/main/java/in/koreatech/koin/domain/community/keyword/service/ArticleKeywordUserMatcher.java`:
- Around line 24-49: Add a short-circuit at the start of
findUserIdsByMatchedKeyword: if the matchedKeywords list is null or empty,
return an empty LinkedHashMap immediately to avoid calling
articleKeywordUserMapRepository.findAllByArticleKeywordCategoryAndArticleKeywordKeywordIn
with an empty collection; ensure you check the matchedKeywords parameter (and
treat null as empty) before any repository call so the method returns quickly
when there's nothing to query.
- Around line 29-49: KeywordExtractor.matchKeywords currently returns
List<String> causing ArticleKeyword rows to be refetched via
articleKeywordUserMapRepository.findAllByArticleKeywordCategoryAndArticleKeywordKeywordIn;
change the extractor to return List<ArticleKeyword> (or List<Long> ids) so the
service can use those directly and avoid the redundant lookup, then update the
repository call to use articleKeyword IDs (e.g.,
findAllByArticleKeywordCategoryAndArticleKeywordIdIn) or compare ArticleKeyword
entities directly (findAllByArticleKeywordCategoryAndArticleKeywordIn) and adapt
the loop that builds keywordByUserId to use ArticleKeyword objects instead of
re-querying by keyword text.
In `@src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java`:
- Around line 29-42: The pagination loop in KeywordExtractor uses an offset
integer and computes the page as PageRequest.of(offset / KEYWORD_BATCH_SIZE,
KEYWORD_BATCH_SIZE), which is confusing and fragile; change the loop to use an
explicit page counter (e.g., int page = 0) and create the pageable with
PageRequest.of(page, KEYWORD_BATCH_SIZE), incrementing page++ each iteration and
removing the offset arithmetic; keep the same stop condition on
keywords.isEmpty() and leave KEYWORD_BATCH_SIZE and
articleKeywordRepository.findAllByCategory intact.
- Around line 25-45: In matchKeywords (KeywordExtractor) add a defensive
null-check for the title parameter and return an empty List<String> immediately
if title is null (to avoid NPE from title.contains(...)); locate the
matchKeywords method and put the guard before any pagination or repository calls
(before PageRequest.of(...) / articleKeywordRepository.findAllByCategory(...))
so the method exits early when title is null and avoids unnecessary DB paging
using the existing matchedKeywords empty list as the return.
In
`@src/main/java/in/koreatech/koin/domain/notification/model/NotificationFactory.java`:
- Around line 235-240: The methods generateLostItemKeywordSchemeUri and
generateKeywordSchemeUri interpolate raw keyword into the deep-link query and
must URL-encode keyword before building the scheme URI; update both functions to
return generateSchemeUri(path,eventId) when keyword is null, otherwise encode
the keyword using UTF-8 (e.g., via URLEncoder.encode(keyword,
StandardCharsets.UTF_8.name()) or equivalent) and use the encoded value in the
String.format call so reserved characters and non-ASCII text produce a valid
query string.
In
`@src/main/java/in/koreatech/koin/domain/notification/repository/NotificationSubscribeRepository.java`:
- Around line 31-43: The repository method
NotificationSubscribeRepository.findArticleKeywordSubscribesByUserIdIn is
misnamed because the JPQL is generic over subscribeType; rename the method to a
neutral, accurate name like findKeywordSubscribesByUserIdIn or
findBySubscribeTypeAndUserIds (keeping the same parameters
`@Param`("subscribeType") NotificationSubscribeType subscribeType and
`@Param`("userIds") List<Integer> userIds) and update all call sites that
reference findArticleKeywordSubscribesByUserIdIn (e.g., services handling
ARTICLE_KEYWORD and LOST_ITEM_KEYWORD flows) to the new method name so behavior
and query remain unchanged but the name reflects its generic use.
In
`@src/main/java/in/koreatech/koin/domain/notification/service/ArticleKeywordNotificationService.java`:
- Around line 67-69: getAlreadyNotifiedUserIds currently hardcodes "keyword"
when building the scheme-uri LIKE pattern which can diverge from the KEYWORD
used in createNotification; change the pattern construction to use the KEYWORD
constant (e.g., build "%s?id=%d&%%" with KEYWORD and articleId) so the call to
notificationRepository.findUserIdsBySchemeUriLikeAndUserIdIn uses the same
source of truth as createNotification, preventing duplicate notifications.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 441d18df-9053-4a73-95db-3417d872b94e
📒 Files selected for processing (22)
src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.javasrc/main/java/in/koreatech/koin/common/event/KoreatechArticleKeywordEvent.javasrc/main/java/in/koreatech/koin/common/event/LostItemKeywordEvent.javasrc/main/java/in/koreatech/koin/domain/community/article/repository/ArticleRepository.javasrc/main/java/in/koreatech/koin/domain/community/article/service/LostItemArticleService.javasrc/main/java/in/koreatech/koin/domain/community/keyword/dto/KeywordNotificationRequest.javasrc/main/java/in/koreatech/koin/domain/community/keyword/model/ArticleKeyword.javasrc/main/java/in/koreatech/koin/domain/community/keyword/repository/ArticleKeywordRepository.javasrc/main/java/in/koreatech/koin/domain/community/keyword/repository/ArticleKeywordUserMapRepository.javasrc/main/java/in/koreatech/koin/domain/community/keyword/service/ArticleKeywordUserMatcher.javasrc/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.javasrc/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.javasrc/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.javasrc/main/java/in/koreatech/koin/domain/notification/eventlistener/LostItemKeywordEventListener.javasrc/main/java/in/koreatech/koin/domain/notification/model/NotificationFactory.javasrc/main/java/in/koreatech/koin/domain/notification/repository/NotificationRepository.javasrc/main/java/in/koreatech/koin/domain/notification/repository/NotificationSubscribeRepository.javasrc/main/java/in/koreatech/koin/domain/notification/service/ArticleKeywordNotificationService.javasrc/main/java/in/koreatech/koin/domain/notification/service/LostItemKeywordNotificationService.javasrc/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.javasrc/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.javasrc/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java
💤 Files with no reviewable changes (5)
- src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java
- src/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.java
- src/main/java/in/koreatech/koin/domain/community/keyword/repository/ArticleKeywordRepository.java
- src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java
- src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java
Contributor
There was a problem hiding this comment.
Pull request overview
키워드 알림 처리에서 EventListener에 섞여 있던 비즈니스 로직(구독 조회/중복 제거/기발송 제외/알림 생성)을 역할별 서비스로 분리하고, 공지(KOREATECH)와 분실물(LOST_ITEM) 키워드 알림 흐름을 이벤트/서비스 단위로 분리한 PR입니다.
Changes:
- 게시글(공지) 키워드 알림:
KoreatechArticleKeywordEvent+ArticleKeywordNotificationService로 책임 분리 - 분실물 키워드 알림:
LostItemKeywordEvent+LostItemKeywordNotificationService및 전용 리스너 추가 - 키워드 매칭/구독 유저 매핑 로직 재구성(
KeywordExtractor단순화,ArticleKeywordUserMatcher도입) 및 관련 Repository 쿼리 추가
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java | 기존 리스너 중심 단위 테스트 삭제 |
| src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java | 기존 KeywordExtractor 이벤트 생성 테스트 삭제 |
| src/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.java | 기존 KeywordService 이벤트 발행/이력 저장 테스트 삭제 |
| src/main/java/in/koreatech/koin/domain/notification/service/LostItemKeywordNotificationService.java | 분실물 키워드 알림 비즈니스 로직 서비스로 분리 |
| src/main/java/in/koreatech/koin/domain/notification/service/ArticleKeywordNotificationService.java | 공지(게시글) 키워드 알림 비즈니스 로직 서비스로 분리 |
| src/main/java/in/koreatech/koin/domain/notification/repository/NotificationSubscribeRepository.java | 매칭 유저 ID 기반 구독 조회 쿼리 추가 |
| src/main/java/in/koreatech/koin/domain/notification/repository/NotificationRepository.java | schemeUri LIKE 기반 기발송 사용자 조회 쿼리 추가 |
| src/main/java/in/koreatech/koin/domain/notification/model/NotificationFactory.java | 키워드 알림 생성 API 변경 및 분실물 전용 생성 메서드 추가 |
| src/main/java/in/koreatech/koin/domain/notification/eventlistener/LostItemKeywordEventListener.java | 분실물 키워드 이벤트 리스너 신규 추가 |
| src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java | 리스너를 단순 위임 구조로 리팩터링 및 이벤트 타입 분리 |
| src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java | 이벤트 생성 책임 제거, 제목 기반 매칭 키워드 리스트 반환으로 단순화 |
| src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java | 공지 키워드 알림 이벤트 발행 흐름을 신규 구성 요소에 맞게 변경 |
| src/main/java/in/koreatech/koin/domain/community/keyword/service/ArticleKeywordUserMatcher.java | 매칭 키워드에 대한 구독 유저 매핑/우선순위(더 긴 키워드) 선정 로직 분리 |
| src/main/java/in/koreatech/koin/domain/community/keyword/repository/ArticleKeywordUserMapRepository.java | 카테고리/키워드 IN 기반 유저 매핑 조회 쿼리 추가 및 미사용 메서드 제거 |
| src/main/java/in/koreatech/koin/domain/community/keyword/repository/ArticleKeywordRepository.java | 미사용 deleteById 제거 |
| src/main/java/in/koreatech/koin/domain/community/keyword/model/ArticleKeyword.java | “더 긴 키워드 우선” 비교 유틸 메서드 추가 |
| src/main/java/in/koreatech/koin/domain/community/keyword/dto/KeywordNotificationRequest.java | updateNotification 타입을 List → Set으로 변경 |
| src/main/java/in/koreatech/koin/domain/community/article/service/LostItemArticleService.java | 분실물 키워드 이벤트 발행 흐름을 신규 구성 요소에 맞게 변경 |
| src/main/java/in/koreatech/koin/domain/community/article/repository/ArticleRepository.java | findAllByIdIn 파라미터를 Collection으로 확장 |
| src/main/java/in/koreatech/koin/common/event/LostItemKeywordEvent.java | 분실물 키워드 이벤트 신규 추가 |
| src/main/java/in/koreatech/koin/common/event/KoreatechArticleKeywordEvent.java | 공지(게시글) 키워드 이벤트 신규 추가 |
| src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java | 기존 통합 이벤트 삭제 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
BaeJinho4028
approved these changes
May 24, 2026
Collaborator
BaeJinho4028
left a comment
BaeJinho4028
left a comment
There was a problem hiding this comment.
ArticleKeywordNotificationService와 LostItemKeywordNotificationService의 흐름이 상당히 유사해 보여, 중복 로직을 공통화할 수 있을지 고민해봐도 좋을 것 같습니다.
| @Query(""" | ||
| SELECT DISTINCT n.user.id | ||
| FROM Notification n | ||
| WHERE n.schemeUri LIKE :schemeUriPattern |
Collaborator
There was a problem hiding this comment.
위험해보이긴합니담. schemeUri 문자열 파싱 대신 다른 방식이 있나 고려해보시면 좋을 것 같습니다.
Collaborator
Author
There was a problem hiding this comment.
배치 코드를 확인했는데, 새롭게 생성된 게시글의 ID값들을 요청값으로 보내고 수정/삭제된 게시글에 대해서는 보내지 않고 있음을 확인했습니다. 해당 로직이 필요없을 거 같아요 !
schemeUri 문자열 파싱이 위험하다가 생각하신 이유가 궁금합니당
제 생각은 서버에서 schemeUri를 관리하고 있기 때문에 DB에서 직접 수정을 가하는 것 아닌이상 문제가 될일은 없다고 생각했습니다. user_notification_status 테이블의 값을 직접 수정하는 거랑 동일한 맥락으로 확인했습니다.
별도의 방법을 더 고민을 했는데 마땅히 방법이 없다고 결론을 내렸습니다. Notification 도메인에서 Keyword 모델인 UserNotificationStatus를 참조하는 것을 풀고 싶었는데 방법이 마땅히 없었습니다. Keyword 도메인에서 구독자를 필터링을 하자니 그럼 Keyword 도메인에서 Notification 도메인을 의존하는 상황이 발생하게 되고, 이벤트 처리로 풀어볼려 했으나 이 또한 방법이 잘 안보였습니다.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/ArticleKeywordNotificationService.java`:
- Around line 38-39: The call to createNotifications(event, userIdsByKeyword,
subscribesByUserId) no longer filters out users who have already been notified
for the same article; restore the prior "already-notified" filtering by
computing the set/map of previously sent notifications (from notification
history) and pass that into createNotifications (or reintroduce an overloaded
createNotifications signature) so it excludes those userIds before building
Notification objects; update createNotifications (and its callers) to accept and
use that notified set/map (referencing createNotifications, userIdsByKeyword,
subscribesByUserId) to suppress duplicates.
In
`@src/main/java/in/koreatech/koin/domain/notification/service/LostItemKeywordNotificationService.java`:
- Around line 39-40: The notification assembly in
LostItemKeywordNotificationService now calls createNotifications(event,
userIdsByKeyword, subscribesByUserId) without filtering out items users were
already notified about, which can cause duplicate pushes; restore
duplicate-history suppression by modifying createNotifications (or its caller)
to accept and use the already-notified tracking (e.g., a Set or Map of
previously notified lostItem IDs per user) and filter out any lost-item entries
present in that structure before creating Notification objects; update the
createNotifications signature and its internal filtering logic (referencing
createNotifications, event, userIdsByKeyword, subscribesByUserId) and ensure the
caller populates the already-notified data from the notification history lookup
used elsewhere in LostItemKeywordNotificationService.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76446264-69fe-4965-b4f1-b897f9bd194c
📒 Files selected for processing (2)
src/main/java/in/koreatech/koin/domain/notification/service/ArticleKeywordNotificationService.javasrc/main/java/in/koreatech/koin/domain/notification/service/LostItemKeywordNotificationService.java
Comment on lines
+38
to
+39
| List<Notification> notifications = createNotifications(event, userIdsByKeyword, subscribesByUserId); | ||
|
|
There was a problem hiding this comment.
Restore already-notified filtering before building notifications.
Line 38 now calls createNotifications(...) without notification-history filtering, and Line 66 removed that input entirely. This regresses duplicate suppression for the same article and can resend pushes on retries/replays.
💡 Suggested direction
+ private final NotificationRepository notificationRepository;
public void notifyArticleKeyword(KoreatechArticleKeywordEvent event) {
Map<String, List<Integer>> userIdsByKeyword = event.matchedKeywordUsers().userIdsByKeyword();
List<Integer> matchedUserIds = getMatchedUserIds(userIdsByKeyword);
if (matchedUserIds.isEmpty()) {
return;
}
Map<Integer, NotificationSubscribe> subscribesByUserId = findSubscribesByUserId(matchedUserIds);
- List<Notification> notifications = createNotifications(event, userIdsByKeyword, subscribesByUserId);
+ Set<Integer> alreadyNotifiedUserIds = getAlreadyNotifiedUserIds(event.articleId(), matchedUserIds);
+ List<Notification> notifications = createNotifications(
+ event, userIdsByKeyword, subscribesByUserId, alreadyNotifiedUserIds
+ );
notificationService.pushNotificationsWithResult(notifications);
}
private List<Notification> createNotifications(
KoreatechArticleKeywordEvent event,
Map<String, List<Integer>> userIdsByKeyword,
- Map<Integer, NotificationSubscribe> subscribesByUserId
+ Map<Integer, NotificationSubscribe> subscribesByUserId,
+ Set<Integer> alreadyNotifiedUserIds
) {
List<Notification> notifications = new ArrayList<>();
for (Map.Entry<String, List<Integer>> entry : userIdsByKeyword.entrySet()) {
String keyword = entry.getKey();
for (Integer userId : entry.getValue()) {
+ if (alreadyNotifiedUserIds.contains(userId)) {
+ continue;
+ }
NotificationSubscribe subscribe = subscribesByUserId.get(userId);
if (subscribe == null) {
continue;
}
notifications.add(createNotification(event, keyword, subscribe));
}
}
return notifications;
}Also applies to: 63-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/in/koreatech/koin/domain/notification/service/ArticleKeywordNotificationService.java`
around lines 38 - 39, The call to createNotifications(event, userIdsByKeyword,
subscribesByUserId) no longer filters out users who have already been notified
for the same article; restore the prior "already-notified" filtering by
computing the set/map of previously sent notifications (from notification
history) and pass that into createNotifications (or reintroduce an overloaded
createNotifications signature) so it excludes those userIds before building
Notification objects; update createNotifications (and its callers) to accept and
use that notified set/map (referencing createNotifications, userIdsByKeyword,
subscribesByUserId) to suppress duplicates.
Comment on lines
+39
to
+40
| List<Notification> notifications = createNotifications(event, userIdsByKeyword, subscribesByUserId); | ||
|
|
There was a problem hiding this comment.
Reintroduce duplicate-history filtering in lost-item notification assembly.
Line 39 now builds notifications without prior “already notified” filtering, and Line 67 removed that parameter from createNotifications(...). Author filtering still exists, but duplicate suppression is missing, so repeated processing can resend the same lost-item keyword push.
💡 Suggested direction
+ private final NotificationRepository notificationRepository;
public void notifyLostItemKeyword(LostItemKeywordEvent event) {
Map<String, List<Integer>> userIdsByKeyword = event.matchedKeywordUsers().userIdsByKeyword();
List<Integer> matchedUserIds = getMatchedUserIds(userIdsByKeyword);
if (matchedUserIds.isEmpty()) {
return;
}
Map<Integer, NotificationSubscribe> subscribesByUserId = findSubscribesByUserId(matchedUserIds);
- List<Notification> notifications = createNotifications(event, userIdsByKeyword, subscribesByUserId);
+ Set<Integer> alreadyNotifiedUserIds = getAlreadyNotifiedUserIds(event.articleId(), matchedUserIds);
+ List<Notification> notifications = createNotifications(
+ event, userIdsByKeyword, subscribesByUserId, alreadyNotifiedUserIds
+ );
notificationService.pushNotificationsWithResult(notifications);
}
private List<Notification> createNotifications(
LostItemKeywordEvent event,
Map<String, List<Integer>> userIdsByKeyword,
- Map<Integer, NotificationSubscribe> subscribesByUserId
+ Map<Integer, NotificationSubscribe> subscribesByUserId,
+ Set<Integer> alreadyNotifiedUserIds
) {
List<Notification> notifications = new ArrayList<>();
for (Map.Entry<String, List<Integer>> entry : userIdsByKeyword.entrySet()) {
String keyword = entry.getKey();
for (Integer userId : entry.getValue()) {
if (isMyArticle(event, userId)) {
continue;
}
+ if (alreadyNotifiedUserIds.contains(userId)) {
+ continue;
+ }
NotificationSubscribe subscribe = subscribesByUserId.get(userId);
if (subscribe == null) {
continue;
}
notifications.add(createNotification(event, keyword, subscribe));
}
}
return notifications;
}Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/in/koreatech/koin/domain/notification/service/LostItemKeywordNotificationService.java`
around lines 39 - 40, The notification assembly in
LostItemKeywordNotificationService now calls createNotifications(event,
userIdsByKeyword, subscribesByUserId) without filtering out items users were
already notified about, which can cause duplicate pushes; restore
duplicate-history suppression by modifying createNotifications (or its caller)
to accept and use the already-notified tracking (e.g., a Set or Map of
previously notified lostItem IDs per user) and filter out any lost-item entries
present in that structure before creating Notification objects; update the
createNotifications signature and its internal filtering logic (referencing
createNotifications, event, userIdsByKeyword, subscribesByUserId) and ensure the
caller populates the already-notified data from the notification history lookup
used elsewhere in LostItemKeywordNotificationService.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔍 개요
🚀 주요 변경 내용
게시글/분실물 키워드 알림 처리 흐름을 역할별 서비스로 분리했습니다.
게시글 키워드 알림 API 처리 흐름
KeywordService가 알림 대상 게시글 제목에서KOREATECH카테고리 키워드를 추출합니다.ArticleKeywordUserMatcher가 매칭된 키워드별 구독 유저 ID를 조회합니다.KoreatechArticleKeywordEvent를 발행합니다.ArticleKeywordEventListener가ArticleKeywordNotificationService에 처리를 위임합니다.ArticleKeywordNotificationService처리 흐름userIdsByKeyword에서 중복을 제거한 전체 매칭 유저 ID 목록을 만듭니다.ARTICLE_KEYWORD타입으로 알림 수신 가능한 구독 정보를 조회합니다.분실물 키워드 알림 API 처리 흐름
LostItemArticleService가 제목에서LOST_ITEM카테고리 키워드를 추출합니다.ArticleKeywordUserMatcher가 매칭된 키워드별 구독 유저 ID를 조회합니다.LostItemKeywordEvent를 발행합니다.LostItemKeywordEventListener가LostItemKeywordNotificationService에 처리를 위임합니다.LostItemKeywordNotificationService처리 흐름userIdsByKeyword에서 중복을 제거한 전체 매칭 유저 ID 목록을 만듭니다.LOST_ITEM_KEYWORD타입으로 알림 수신 가능한 구독 정보를 조회합니다.기타 정리
💬 참고 사항
✅ Checklist (완료 조건)
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Style