Open
Conversation
taejinn
requested review from
BaeJinho4028,
DHkimgit,
ImTotem,
Soundbar91,
Copilot and
dh2906
📝 WalkthroughWalkthroughAggregates matched keywords per article into a userId→keyword map carried by Changes
Sequence DiagramsequenceDiagram
participant KS as KeywordService
participant AR as ArticleRepository
participant KE as KeywordExtractor
participant AKE as ArticleKeywordEventListener
participant UNR as UserNotificationStatusRepository
participant NS as NotificationService
participant FCM as FcmClient
KS->>KS: Deduplicate article IDs
alt has articles
KS->>AR: fetch articles by IDs
AR-->>KS: articles
KS->>KE: matchKeyword(articles)
KE->>KE: group matches per-article per-user (userId→keyword)
KE-->>KS: List<ArticleKeywordEvent>
loop per ArticleKeywordEvent
KS->>KS: publish ArticleKeywordEvent
KS->>AKE: ArticleKeywordEvent
AKE->>AKE: build per-user subscription map
AKE->>UNR: getAlreadyNotifiedUserIds
alt user is author or already-notified
AKE->>AKE: skip user
else
AKE->>NS: pushNotificationsWithResult(notifications)
NS->>FCM: sendMessageWithResult(...)
FCM-->>NS: boolean delivered
NS-->>AKE: NotificationDeliveryResult
AKE->>UNR: upsertLastNotifiedArticleId for delivered users
end
end
else no articles
KS->>KS: return (no-op)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
There was a problem hiding this comment.
Pull request overview
키워드 알림에서 동일 게시글이 여러 키워드에 매칭될 때 사용자에게 중복 발송되던 문제(#2164)를 해결하기 위해, 이벤트 생성/소비 흐름을 “게시글당 1건 이벤트 + 사용자당 1회 발송” 구조로 정리한 PR입니다.
Changes:
ArticleKeywordEvent를 단일 키워드 →matchedKeywords(다중 키워드)로 확장하고,KeywordExtractor가 게시글당 이벤트 1건만 생성하도록 변경ArticleKeywordEventListener에서 (1) 구독 중복 제거 (2) 사용자당 1회만 알림 생성 (3) 더 구체적인 키워드(문자열 길이) 우선 선택 로직 추가KeywordService에서 요청 articleId 중복 제거 및UserNotificationStatus저장을 update-or-insert 형태로 변경 + 관련 테스트 추가
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java | 이벤트 payload를 다중 키워드 매칭 구조로 변경 |
| src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java | 게시글당 이벤트 1건만 생성되도록 매칭 결과를 articleId로 그룹핑 |
| src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java | 요청 ID dedupe 및 알림 상태 저장 로직을 update-or-insert로 변경 |
| src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java | 구독/키워드 매칭 중복 제거 및 사용자당 1회 알림 생성으로 변경 |
| src/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.java | 불필요 import 정리 |
| src/main/java/in/koreatech/koin/domain/community/keyword/model/UserNotificationStatus.java | 상태 갱신 메서드 추가 |
| src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java | 다중 키워드 매칭 시 이벤트 1건 생성 테스트 추가 |
| src/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.java | 중복 articleId 요청 dedupe 테스트 추가 |
| src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java | 중복 구독/다중 키워드 매칭에도 사용자당 1회 발송 테스트 추가 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java
Outdated
Show resolved
Hide resolved
![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 (1)
src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java (1)
33-54: Good test coverage for the core fix.The test correctly verifies that multiple matching keywords for a single article produce only one event with all matched keywords aggregated. Consider adding edge case tests for better coverage:
- No keywords match any article (empty result)
- Multiple articles with varying keyword matches
- Empty keyword list from repository
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java` around lines 33 - 54, Add additional unit tests in KeywordExtractorTest targeting KeywordExtractor.matchKeyword to cover the suggested edge cases: (1) no keywords match any article — assert an empty result when repository returns keywords that don't appear in articles, (2) multiple articles with varying keyword matches — create multiple mocked Article instances with different titles and assert one event per article aggregating that article's matched keywords, and (3) empty keyword list from repository — mock articleKeywordRepository.findAll(...) to return an empty list and assert matchKeyword returns an empty list; use the same mocking style (when(...).thenReturn(...)) and assertions (assertThat(...)) as the existing test to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java`:
- Around line 202-209: The current createNotifiedArticleStatus method uses
userNotificationStatusRepository.findByUserId(...) followed by save which is not
atomic and can race; change this to an atomic upsert or guarded transaction:
implement a repository-level upsert (e.g., a custom repository method using
INSERT ... ON CONFLICT / ON DUPLICATE KEY UPDATE) or use a transactional
read-for-update (pessimistic lock) when loading the UserNotificationStatus and
then call updateNotifiedArticleId, or catch
unique-constraint/DataIntegrityViolation exceptions around save and retry the
lookup+update; reference createNotifiedArticleStatus,
userNotificationStatusRepository, findByUserId, save, UserNotificationStatus,
and updateNotifiedArticleId when making the change.
In
`@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java`:
- Around line 84-96: getMatchedKeywordByUserId triggers lazy loading via
ArticleKeyword#getArticleKeywordUserMaps causing LazyInitializationException
when run after commit; fix by ensuring ArticleKeyword.articleKeywordUserMaps is
eagerly loaded at retrieval: update the repository/query in KeywordExtractor
that returns the matched keywords to use a JOIN FETCH for articleKeywordUserMaps
(or create a new method like findWithUserMapsByIds(...) that uses JOIN FETCH) so
getMatchedKeywordByUserId can iterate without accessing the closed persistence
context; alternatively, if changing the fetch path is undesirable, annotate the
listener method that calls getMatchedKeywordByUserId with `@Transactional` to open
a session during execution.
---
Nitpick comments:
In
`@src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java`:
- Around line 33-54: Add additional unit tests in KeywordExtractorTest targeting
KeywordExtractor.matchKeyword to cover the suggested edge cases: (1) no keywords
match any article — assert an empty result when repository returns keywords that
don't appear in articles, (2) multiple articles with varying keyword matches —
create multiple mocked Article instances with different titles and assert one
event per article aggregating that article's matched keywords, and (3) empty
keyword list from repository — mock articleKeywordRepository.findAll(...) to
return an empty list and assert matchKeyword returns an empty list; use the same
mocking style (when(...).thenReturn(...)) and assertions (assertThat(...)) as
the existing test to keep consistency.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.javasrc/main/java/in/koreatech/koin/domain/community/keyword/model/UserNotificationStatus.javasrc/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.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/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 (1)
- src/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.java
src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java
Show resolved
Hide resolved
...in/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java
Outdated
Show resolved
Hide resolved
taejinn
force-pushed
the
fix/2164-notification-duplication
branch
from
5e78b18 to
89619ba
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...in/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java
Show resolved
Hide resolved
.../in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java (1)
5-9: Consider making the event payload map immutable.A defensive copy (e.g.,
Map.copyOf(...)) in the record constructor would prevent accidental mutation after publish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java` around lines 5 - 9, The record ArticleKeywordEvent currently exposes a mutable Map via the matchedKeywordByUserId component; make the payload immutable by adding a defensive copy in the record constructor (e.g., use Map.copyOf(...) or Collections.unmodifiableMap(...)) so the stored field is an immutable map; implement this in the record's canonical or compact constructor to replace matchedKeywordByUserId with the copied immutable map before assignment.src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java (1)
43-54: Potential over-fetch in batch loop.
findAllByArticleKeywordIdIn(keywordIds)runs before title matching, so user-map rows are loaded even for keywords that match no article title in that batch. Consider querying user maps only for matched keyword IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java` around lines 43 - 54, The current code fetches ArticleKeywordUserMap rows for all keywordIds via articleKeywordUserMapRepository.findAllByArticleKeywordIdIn(keywordIds) before filtering keywords by title, which can over-fetch; change the flow to first filter/match keywords by title (use the existing keywords stream and title-matching predicate to produce matchedKeywordIds or matchedKeywords list), then call findAllByArticleKeywordIdIn(matchedKeywordIds) and build userMapsByKeywordId (preserving grouping with LinkedHashMap::new) only for those matched IDs; update any downstream references from keywordIds to matchedKeywordIds and ensure ArticleKeywordUserMap filtering on getIsDeleted() remains.src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java (1)
158-161: Consider replacing per-IDgetByIdcalls with a batch fetch.The current loop issues one DB call per article ID. For crawler-triggered batches, a single repository batch query will scale better.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java` around lines 158 - 161, The loop in KeywordService that builds articles by calling articleRepository.getById for each id (using updateNotificationIds -> articles) causes N DB calls; replace it with a single batch fetch (e.g., articleRepository.findAllById or findByIdIn(updateNotificationIds)) to retrieve all Article entities in one query, then map the returned list back into the original updateNotificationIds order (and handle missing ids/nulls) before assigning to articles; ensure you use the repository batch method name that exists (or add one) and remove the per-id getById loop.src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java (1)
44-50: Move the empty-recipient short-circuit before loadingArticle/Board.Line [44]-Line [45] hits the DB before the early return at Line [48]. For empty
matchedKeywordByUserId, this is unnecessary work.♻️ Proposed change
- Article article = articleRepository.getById(event.articleId()); - Board board = article.getBoard(); Map<Integer, String> matchedKeywordByUserId = event.matchedKeywordByUserId(); if (matchedKeywordByUserId.isEmpty()) { return; } + + Article article = articleRepository.getById(event.articleId()); + Board board = article.getBoard();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java` around lines 44 - 50, In ArticleKeywordEventListener, avoid loading Article/Board when there are no recipients by moving the matched-recipient empty check before calling articleRepository.getById; specifically, check event.matchedKeywordByUserId().isEmpty() first and return early, then only call articleRepository.getById(event.articleId()) and use article.getBoard() when the map is non-empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java`:
- Around line 63-66: The stream in ArticleKeywordEventListener uses
isNewNotifiedArticleId per subscriber which triggers an N+1 exists query;
instead add a repository projection method (e.g.,
NotificationRepository.findUserIdsByNotifiedArticleIdAndUserIdIn(articleId,
userIds)) to fetch all userIds already notified for the article in one query,
call it once before building the stream, store the result as a Set<Long>, and
replace the isNewNotifiedArticleId(...) check with a simple contains check
against that set when filtering keywordSubscribersByUserId; keep
isNewNotifiedArticleId only if you refactor it to use the pre-fetched set or
remove it and reference the set directly.
- Around line 67-75: The code currently persists notification status inside
createAndRecordNotification while mapping subscribers
(ArticleKeywordEventListener) which causes permanent loss if
notificationService.pushNotifications (which uses fcmClient.sendMessage) fails;
change the flow so createAndRecordNotification no longer writes status to DB but
only constructs Notification DTOs, then call
notificationService.pushNotifications(notifications) to send, and only after
each send succeeds persist the delivered status (e.g., add a new method like
notificationService.recordNotificationStatus or use a separate
persistNotifications(List<Notification>, deliveryResults) that updates the
database). Ensure references: modify ArticleKeywordEventListener to stop calling
a persistence side-effect in createAndRecordNotification, update
createAndRecordNotification to be pure (construct Notification), and add logic
after notificationService.pushNotifications to persist statuses based on send
results from fcmClient.sendMessage.
---
Nitpick comments:
In `@src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java`:
- Around line 5-9: The record ArticleKeywordEvent currently exposes a mutable
Map via the matchedKeywordByUserId component; make the payload immutable by
adding a defensive copy in the record constructor (e.g., use Map.copyOf(...) or
Collections.unmodifiableMap(...)) so the stored field is an immutable map;
implement this in the record's canonical or compact constructor to replace
matchedKeywordByUserId with the copied immutable map before assignment.
In
`@src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java`:
- Around line 158-161: The loop in KeywordService that builds articles by
calling articleRepository.getById for each id (using updateNotificationIds ->
articles) causes N DB calls; replace it with a single batch fetch (e.g.,
articleRepository.findAllById or findByIdIn(updateNotificationIds)) to retrieve
all Article entities in one query, then map the returned list back into the
original updateNotificationIds order (and handle missing ids/nulls) before
assigning to articles; ensure you use the repository batch method name that
exists (or add one) and remove the per-id getById loop.
In `@src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java`:
- Around line 43-54: The current code fetches ArticleKeywordUserMap rows for all
keywordIds via
articleKeywordUserMapRepository.findAllByArticleKeywordIdIn(keywordIds) before
filtering keywords by title, which can over-fetch; change the flow to first
filter/match keywords by title (use the existing keywords stream and
title-matching predicate to produce matchedKeywordIds or matchedKeywords list),
then call findAllByArticleKeywordIdIn(matchedKeywordIds) and build
userMapsByKeywordId (preserving grouping with LinkedHashMap::new) only for those
matched IDs; update any downstream references from keywordIds to
matchedKeywordIds and ensure ArticleKeywordUserMap filtering on getIsDeleted()
remains.
In
`@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java`:
- Around line 44-50: In ArticleKeywordEventListener, avoid loading Article/Board
when there are no recipients by moving the matched-recipient empty check before
calling articleRepository.getById; specifically, check
event.matchedKeywordByUserId().isEmpty() first and return early, then only call
articleRepository.getById(event.articleId()) and use article.getBoard() when the
map is non-empty.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.javasrc/main/java/in/koreatech/koin/domain/community/keyword/model/UserNotificationStatus.javasrc/main/java/in/koreatech/koin/domain/community/keyword/repository/ArticleKeywordUserMapRepository.javasrc/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.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/repository/NotificationSubscribeRepository.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 skipped from review as they are similar to previous changes (2)
- src/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.java
- src/main/java/in/koreatech/koin/domain/community/keyword/model/UserNotificationStatus.java
...in/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java
Show resolved
Hide resolved
...in/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java (1)
39-67:⚠️ Potential issue | 🟠 Major
sendMessageWithResult가try이전 예외를 놓치고 있습니다.현재는
FirebaseMessaging.send(...)만try/catch로 감싸고 있어, 그 이전 단계에서 예외가 나면false반환 대신 메서드가 그대로 예외를 던집니다. 결과 기반 흐름에서는 여기서도 일관되게false를 반환해야 안전합니다.🔧 Proposed fix
public boolean sendMessageWithResult( String targetDeviceToken, String title, String content, String imageUrl, MobileAppPath path, String schemeUri, String type ) { if (targetDeviceToken == null) { return false; } log.info("call FcmClient sendMessage: title: {}, content: {}", title, content); - - ApnsConfig apnsConfig = generateAppleConfig(title, content, imageUrl, path, type, schemeUri); - AndroidConfig androidConfig = generateAndroidConfig(title, content, imageUrl, schemeUri, type); - - Message message = Message.builder() - .setToken(targetDeviceToken) - .setApnsConfig(apnsConfig) - .setAndroidConfig(androidConfig).build(); try { + ApnsConfig apnsConfig = generateAppleConfig(title, content, imageUrl, path, type, schemeUri); + AndroidConfig androidConfig = generateAndroidConfig(title, content, imageUrl, schemeUri, type); + + Message message = Message.builder() + .setToken(targetDeviceToken) + .setApnsConfig(apnsConfig) + .setAndroidConfig(androidConfig) + .build(); String result = FirebaseMessaging.getInstance().send(message); log.info("FCM 알림 전송 성공: {}", result); return true; } catch (Exception e) { log.warn("FCM 알림 전송 실패", e); return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java` around lines 39 - 67, The method sendMessageWithResult currently only wraps FirebaseMessaging.getInstance().send(...) in a try/catch, so exceptions from generateAppleConfig, generateAndroidConfig or Message.builder() escape; move or expand the try/catch to cover the code that builds ApnsConfig/AndroidConfig and the Message (everything after the null-check) so any thrown Exception is caught, logged via log.warn (include the exception) and the method returns false consistently; keep the early null check for targetDeviceToken outside the try if you prefer, but ensure all calls to generateAppleConfig, generateAndroidConfig, Message.builder() and FirebaseMessaging.getInstance().send are inside the try block.
🧹 Nitpick comments (3)
src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java (1)
151-185:delivered=false경로도 테스트에 추가하면 더 견고합니다.성공 시에만
createNotifiedArticleStatus(...)를 호출하는 로직이 핵심이므로, 실패 전송 케이스를 명시적으로 고정해두는 것을 권장합니다.🧪 Suggested test case
+ `@Test` + `@DisplayName`("푸시 전송 실패 시 기발송 상태를 기록하지 않는다.") + void onKeywordRequest_whenDeliveryFailed_doesNotRecordStatus() { + Integer articleId = 400; + Integer boardId = 13; + Integer userId = 4; + User subscriber = UserFixture.id_설정_코인_유저(userId); + subscriber.permitNotification("device-token"); + + NotificationSubscribe subscribe = createKeywordSubscribe(subscriber); + ArticleKeywordEvent event = new ArticleKeywordEvent(articleId, 999, Map.of(userId, "근로")); + + Article article = mock(Article.class); + Board board = mock(Board.class); + when(articleRepository.getById(articleId)).thenReturn(article); + when(article.getId()).thenReturn(articleId); + when(article.getTitle()).thenReturn("근로 공지"); + when(article.getBoard()).thenReturn(board); + when(board.getId()).thenReturn(boardId); + when(notificationSubscribeRepository.findAllBySubscribeTypeAndDetailTypeIsNullWithUser(ARTICLE_KEYWORD)) + .thenReturn(List.of(subscribe)); + when(userNotificationStatusRepository.findUserIdsByNotifiedArticleIdAndUserIdIn(eq(articleId), any())) + .thenReturn(List.of()); + + Notification notification = mock(Notification.class); + when(notification.getUser()).thenReturn(subscriber); + when(notificationFactory.generateKeywordNotification(any(), anyInt(), anyString(), anyString(), anyInt(), anyString(), any())) + .thenReturn(notification); + when(notificationService.pushNotificationsWithResult(any())) + .thenReturn(List.of(new NotificationService.NotificationDeliveryResult(notification, false))); + + articleKeywordEventListener.onKeywordRequest(event); + + verify(keywordService, never()).createNotifiedArticleStatus(anyInt(), anyInt()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java` around lines 151 - 185, Add a failing-delivery test to ArticleKeywordEventListenerTest (e.g., a new test alongside onKeywordRequest_whenAlreadyNotified_skipsNotification) that simulates notification push returning a result with delivered=false; set up the same Article/Board/subscribe mocks and have notificationService.pushNotificationsWithResult(...) return a list containing a NotificationResult (or equivalent) with delivered=false, then assert that notificationFactory.generateKeywordNotification is invoked (if applicable) but keywordService.createNotifiedArticleStatus(...) is NOT called and that pushNotificationsWithResult was called with the expected list—this explicitly verifies the non-creation path when delivered=false.src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java (1)
47-52: 결과 기반 전송 루프의 외부 I/O를 트랜잭션 경계 밖으로 분리하는 것을 권장합니다.현재는 FCM 호출이 DB 트랜잭션 내부에서 수행되어, 대상이 많을 때 트랜잭션 유지 시간이 길어질 수 있습니다.
♻️ Suggested minimal change
- `@Transactional` public List<NotificationDeliveryResult> pushNotificationsWithResult(List<Notification> notifications) { return notifications.stream() .map(this::pushNotificationWithResult) .toList(); }Also applies to: 69-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 47 - 52, The pushNotificationsWithResult method is currently annotated with `@Transactional` and calls pushNotificationWithResult inside the transaction, causing external I/O (FCM calls) to run within the DB transaction; refactor so that DB reads/writes remain inside a short transaction and the actual FCM send happens outside it — for example, have a transactional helper that loads/updates Notification entities (keep method like pushNotificationsWithResult or a new fetchAndMarkForSend) then collect payloads and call pushNotificationWithResult (or a new sendNotification) outside the `@Transactional` boundary; apply the same pattern to the other transactional send loop (lines 69-82) to ensure all network/FCM calls occur after the transaction commits.src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java (1)
65-68: 이미 발송 조회 대상을 매칭된 사용자로 먼저 축소하면 더 효율적입니다.현재는 디바이스 토큰이 있는 전체 키워드 구독자를 기준으로 조회해, 매칭되지 않은 사용자까지
IN대상에 포함됩니다.⚡ Proposed optimization
- Set<Integer> alreadyNotifiedUserIds = getAlreadyNotifiedUserIds( - event.articleId(), - keywordSubscribersByUserId.keySet() - ); + Set<Integer> targetUserIds = keywordSubscribersByUserId.keySet().stream() + .filter(matchedKeywordByUserId::containsKey) + .collect(Collectors.toSet()); + + Set<Integer> alreadyNotifiedUserIds = getAlreadyNotifiedUserIds( + event.articleId(), + targetUserIds + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java` around lines 65 - 68, The call to getAlreadyNotifiedUserIds currently passes keywordSubscribersByUserId.keySet() (all subscribers with device tokens), causing the DB IN query to include users who weren’t matched; narrow the IN list by first computing the set of matched user IDs and pass that to getAlreadyNotifiedUserIds. Concretely, derive a Set<Integer> matchedUserIds from keywordSubscribersByUserId (or from whatever matching result you have) and call getAlreadyNotifiedUserIds(event.articleId(), matchedUserIds) so only matched subscribers are queried for prior notifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java`:
- Around line 39-67: The method sendMessageWithResult currently only wraps
FirebaseMessaging.getInstance().send(...) in a try/catch, so exceptions from
generateAppleConfig, generateAndroidConfig or Message.builder() escape; move or
expand the try/catch to cover the code that builds ApnsConfig/AndroidConfig and
the Message (everything after the null-check) so any thrown Exception is caught,
logged via log.warn (include the exception) and the method returns false
consistently; keep the early null check for targetDeviceToken outside the try if
you prefer, but ensure all calls to generateAppleConfig, generateAndroidConfig,
Message.builder() and FirebaseMessaging.getInstance().send are inside the try
block.
---
Nitpick comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java`:
- Around line 65-68: The call to getAlreadyNotifiedUserIds currently passes
keywordSubscribersByUserId.keySet() (all subscribers with device tokens),
causing the DB IN query to include users who weren’t matched; narrow the IN list
by first computing the set of matched user IDs and pass that to
getAlreadyNotifiedUserIds. Concretely, derive a Set<Integer> matchedUserIds from
keywordSubscribersByUserId (or from whatever matching result you have) and call
getAlreadyNotifiedUserIds(event.articleId(), matchedUserIds) so only matched
subscribers are queried for prior notifications.
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 47-52: The pushNotificationsWithResult method is currently
annotated with `@Transactional` and calls pushNotificationWithResult inside the
transaction, causing external I/O (FCM calls) to run within the DB transaction;
refactor so that DB reads/writes remain inside a short transaction and the
actual FCM send happens outside it — for example, have a transactional helper
that loads/updates Notification entities (keep method like
pushNotificationsWithResult or a new fetchAndMarkForSend) then collect payloads
and call pushNotificationWithResult (or a new sendNotification) outside the
`@Transactional` boundary; apply the same pattern to the other transactional send
loop (lines 69-82) to ensure all network/FCM calls occur after the transaction
commits.
In
`@src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java`:
- Around line 151-185: Add a failing-delivery test to
ArticleKeywordEventListenerTest (e.g., a new test alongside
onKeywordRequest_whenAlreadyNotified_skipsNotification) that simulates
notification push returning a result with delivered=false; set up the same
Article/Board/subscribe mocks and have
notificationService.pushNotificationsWithResult(...) return a list containing a
NotificationResult (or equivalent) with delivered=false, then assert that
notificationFactory.generateKeywordNotification is invoked (if applicable) but
keywordService.createNotifiedArticleStatus(...) is NOT called and that
pushNotificationsWithResult was called with the expected list—this explicitly
verifies the non-creation path when delivered=false.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.javasrc/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.javasrc/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javasrc/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.javasrc/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java (1)
71-84: Notification is persisted before delivery confirmation.
notificationRepository.save(notification)at line 72 occurs before the FCM send at line 74. IfsendMessageWithResultreturnsfalse, the notification record remains in the database even though delivery failed. This could lead to orphaned notification records that the user never received.Consider either:
- Moving
save()after successful delivery, or- Accepting the current behavior as an audit trail of attempted notifications
If the intent is to record all attempted notifications (even failed ones), this is acceptable but should be documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 71 - 84, The NotificationService.pushNotificationWithResult currently persists the Notification before attempting delivery (notificationRepository.save called before fcmClient.sendMessageWithResult), which can leave failed delivery records in the DB; either move the save call to after a successful send (call notificationRepository.save(notification) only when fcmClient.sendMessageWithResult(...) returns true and then construct NotificationDeliveryResult), or if you intend to record attempted deliveries, add a clear comment and update documentation/tests to reflect that notificationRepository.save(notification) intentionally remains before the send; locate and update the pushNotificationWithResult method and the use of notificationRepository.save and fcmClient.sendMessageWithResult accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 71-84: The NotificationService.pushNotificationWithResult
currently persists the Notification before attempting delivery
(notificationRepository.save called before fcmClient.sendMessageWithResult),
which can leave failed delivery records in the DB; either move the save call to
after a successful send (call notificationRepository.save(notification) only
when fcmClient.sendMessageWithResult(...) returns true and then construct
NotificationDeliveryResult), or if you intend to record attempted deliveries,
add a clear comment and update documentation/tests to reflect that
notificationRepository.save(notification) intentionally remains before the send;
locate and update the pushNotificationWithResult method and the use of
notificationRepository.save and fcmClient.sendMessageWithResult accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.javasrc/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javasrc/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.javasrc/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java (1)
57-69:⚠️ Potential issue | 🟠 MajorLegacy send path still persists undelivered notifications.
Line 58 saves before delivery, and Line 165 still routes dining notifications through this path. If FCM send fails, the DB can still record it as sent, which conflicts with the success-only persistence behavior introduced in this PR.
Proposed fix
`@Transactional`(propagation = Propagation.NOT_SUPPORTED) public void pushNotification(Notification notification) { - notificationRepository.save(notification); - String deviceToken = notification.getUser().getDeviceToken(); - fcmClient.sendMessage( - deviceToken, - notification.getTitle(), - notification.getMessage(), - notification.getImageUrl(), - notification.getMobileAppPath(), - notification.getSchemeUri(), - notification.getType().toLowerCase() - ); + pushNotificationWithResult(notification); }Also applies to: 153-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 57 - 69, The current pushNotification method saves the Notification via notificationRepository.save(notification) before calling fcmClient.sendMessage, leaving undelivered notifications persisted; change the flow so the Notification is only persisted after a successful FCM send. Specifically, modify pushNotification to call fcmClient.sendMessage(...) first, check the send result/acknowledgement or catch exceptions from fcmClient, and only then call notificationRepository.save(notification); for failures, do not save and instead propagate or log the error. Also update any callers that route dining notifications through the legacy path (the block around the dining-notification routing that invokes pushNotification) to follow the same send-then-save pattern or to use the updated pushNotification behavior so failed sends are not persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 57-69: The current pushNotification method saves the Notification
via notificationRepository.save(notification) before calling
fcmClient.sendMessage, leaving undelivered notifications persisted; change the
flow so the Notification is only persisted after a successful FCM send.
Specifically, modify pushNotification to call fcmClient.sendMessage(...) first,
check the send result/acknowledgement or catch exceptions from fcmClient, and
only then call notificationRepository.save(notification); for failures, do not
save and instead propagate or log the error. Also update any callers that route
dining notifications through the legacy path (the block around the
dining-notification routing that invokes pushNotification) to follow the same
send-then-save pattern or to use the updated pushNotification behavior so failed
sends are not persisted.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javasrc/test/java/in/koreatech/koin/unit/domain/notification/service/NotificationServiceTest.java
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
44
to
46
| @TransactionalEventListener | ||
| public void onKeywordRequest(ArticleKeywordEvent event) { | ||
| Article article = articleRepository.getById(event.articleId()); |
There was a problem hiding this comment.
onKeywordRequest가 pushNotificationsWithResult를 호출하면서 FCM 전송 결과를 기다리게 되어(현재 sendMessageWithResult는 동기 호출), 이벤트 발행 스레드를 오래 점유할 수 있습니다. 다른 알림 이벤트 리스너들처럼 @Async + @TransactionalEventListener(phase = AFTER_COMMIT)로 비동기 처리해 요청 지연/스레드 고갈 위험을 줄이는 게 안전합니다.
Comment on lines
+48
to
+52
| if (targetDeviceToken == null) { | ||
| return; | ||
| return false; | ||
| } | ||
| log.info("call FcmClient sendMessage: title: {}, content: {}", title, content); | ||
| try { | ||
| log.info("call FcmClient sendMessage: title: {}, content: {}", title, content); |
There was a problem hiding this comment.
sendMessageWithResult는 targetDeviceToken == null만 필터링하고 있어 빈 문자열/공백 토큰이 들어오면 불필요한 Firebase 호출 및 warn 로그가 발생할 수 있습니다. StringUtils.isBlank(targetDeviceToken) 같은 체크로 조기 반환(false) 처리하는 편이 안전합니다.
Contributor
Author
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| @Transactional | ||
| @Transactional(propagation = Propagation.NOT_SUPPORTED) | ||
| public List<NotificationDeliveryResult> pushNotificationsWithResult(List<Notification> notifications) { | ||
| return notifications.stream() |
There was a problem hiding this comment.
pushNotificationsWithResult가 알림 개수만큼 sendMessageWithResult를 순차 호출하므로(네트워크 I/O), 대상자가 많을 때 처리 시간이 선형으로 증가합니다. Firebase Admin SDK의 batch 전송(sendAll/sendEachForMulticast 등)으로 묶어서 보내고 결과를 매핑하면 지연과 호출 수를 크게 줄일 수 있습니다.
Suggested change
| return notifications.stream() | |
| return notifications.parallelStream() |
…2168) * Initial plan * fix: use StringUtils.hasText to guard against blank FCM tokens Co-authored-by: taejinn <140797244+taejinn@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: taejinn <140797244+taejinn@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java (1)
56-69:⚠️ Potential issue | 🟠 MajorDelivery persistence semantics are inconsistent between old and new paths.
Line 58 always saves before sending (
pushNotification), but Line 82-84 saves only on successful delivery (pushNotificationWithResult). Existing callers onpushNotification(s)still record failed deliveries, while new callers won’t.🔧 Proposed alignment (single delivery policy)
`@Transactional`(propagation = Propagation.NOT_SUPPORTED) public void pushNotifications(List<Notification> notifications) { - for (Notification notification : notifications) { - pushNotification(notification); - } + notifications.forEach(this::pushNotificationWithResult); } `@Transactional`(propagation = Propagation.NOT_SUPPORTED) public void pushNotification(Notification notification) { - notificationRepository.save(notification); - String deviceToken = notification.getUser().getDeviceToken(); - fcmClient.sendMessage( - deviceToken, - notification.getTitle(), - notification.getMessage(), - notification.getImageUrl(), - notification.getMobileAppPath(), - notification.getSchemeUri(), - notification.getType().toLowerCase() - ); + pushNotificationWithResult(notification); }Also applies to: 71-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 56 - 69, pushNotification currently persists notifications before attempting delivery while pushNotificationWithResult persists only on success, causing inconsistent delivery records; update both pushNotification and pushNotificationWithResult to follow a single policy (choose one) — either persist only after fcmClient.sendMessage returns a successful result or persist immediately and update/save delivery status after send — and apply the change to both methods (referencing pushNotification, pushNotificationWithResult, notificationRepository.save, and fcmClient.sendMessage) so persistence timing and status handling are consistent across callers. Ensure you check the send result from fcmClient.sendMessage and only call notificationRepository.save or notificationRepository.save/updateStatus according to the chosen policy.
🧹 Nitpick comments (1)
src/test/java/in/koreatech/koin/unit/domain/notification/service/NotificationServiceTest.java (1)
51-81: Consider adding one mixed-batch test case.A
[true, false]batch in one call would validate per-item result mapping and selective saves in a single execution path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/in/koreatech/koin/unit/domain/notification/service/NotificationServiceTest.java` around lines 51 - 81, Add a mixed-batch unit test in NotificationServiceTest that calls NotificationService.pushNotificationsWithResult with two Notification instances and stubs fcmClient.sendMessageWithResult to return true for the first and false for the second (e.g., thenReturn(true, false) or use thenAnswer to map args). Assert the returned List has size 2 and that delivered() is true for the first and false for the second, then verify notificationRepository.save was invoked only for the delivered notification (verify(save).times(1) with the first notification and never() for the second). Use the existing helper createNotification and reference pushNotificationsWithResult, fcmClient.sendMessageWithResult, and notificationRepository.save in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 42-43: The methods in NotificationService currently use
`@Transactional`(propagation = Propagation.NOT_SUPPORTED) (e.g., pushNotifications
and the other NotificationService methods flagged) which suspends caller
transactions and causes saved notifications to persist even if callers roll
back; change these methods to participate in the caller transaction instead by
removing NOT_SUPPORTED and using the default transactional behavior or
explicitly `@Transactional`(propagation = Propagation.REQUIRED) so calls like
ChatMessageSendService.handle() and
NotificationScheduleService.sendDueNotifications() will roll back notification
persistence when their surrounding transaction is rolled back.
---
Outside diff comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 56-69: pushNotification currently persists notifications before
attempting delivery while pushNotificationWithResult persists only on success,
causing inconsistent delivery records; update both pushNotification and
pushNotificationWithResult to follow a single policy (choose one) — either
persist only after fcmClient.sendMessage returns a successful result or persist
immediately and update/save delivery status after send — and apply the change to
both methods (referencing pushNotification, pushNotificationWithResult,
notificationRepository.save, and fcmClient.sendMessage) so persistence timing
and status handling are consistent across callers. Ensure you check the send
result from fcmClient.sendMessage and only call notificationRepository.save or
notificationRepository.save/updateStatus according to the chosen policy.
---
Nitpick comments:
In
`@src/test/java/in/koreatech/koin/unit/domain/notification/service/NotificationServiceTest.java`:
- Around line 51-81: Add a mixed-batch unit test in NotificationServiceTest that
calls NotificationService.pushNotificationsWithResult with two Notification
instances and stubs fcmClient.sendMessageWithResult to return true for the first
and false for the second (e.g., thenReturn(true, false) or use thenAnswer to map
args). Assert the returned List has size 2 and that delivered() is true for the
first and false for the second, then verify notificationRepository.save was
invoked only for the delivered notification (verify(save).times(1) with the
first notification and never() for the second). Use the existing helper
createNotification and reference pushNotificationsWithResult,
fcmClient.sendMessageWithResult, and notificationRepository.save in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9ea39f4-440b-42d2-bda1-c6c83edef901
📒 Files selected for processing (3)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javasrc/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.javasrc/test/java/in/koreatech/koin/unit/domain/notification/service/NotificationServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java
Outdated
Show resolved
Hide resolved
…m/BCSDLab/KOIN_API_V2 into fix/2164-notification-duplication
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 49-53: pushNotificationsWithResult currently streams notifications
and calls pushNotificationWithResult which can throw and abort the stream and
transaction; change the mapping so each call is wrapped in a try-catch that
catches Exception, logs the failure, and returns a NotificationDeliveryResult
for that Notification with delivered=false (mirroring AdminNotificationService
behavior) so one failure doesn't stop the rest and the transaction won't roll
back for the whole batch; update the lambda in pushNotificationsWithResult to
call pushNotificationWithResult inside the try block and construct a failed
NotificationDeliveryResult in the catch.
- Around line 41-42: The FCM sends occur inside transaction boundaries (see
pushNotifications / pushNotification(), pushNotificationsWithResult() and
sendMessageWithResult()/sendMessage()) which risks delivering notifications when
the DB later rolls back; refactor by decoupling send logic to run AFTER_COMMIT:
publish an application event (e.g., NotificationCreatedEvent) instead of calling
sendMessage/sendMessageWithResult directly from ArticleKeywordEventListener,
ShopEventListener, ClubRecruitmentEventListener and
NotificationScheduleService.sendDueNotifications(), and implement an event
listener annotated with `@TransactionalEventListener`(phase =
TransactionPhase.AFTER_COMMIT) that performs the FCM send (or implement an
outbox pattern where pushNotifications persists outbox rows inside the
transaction and a separate dispatcher sends after commit). Ensure
pushNotificationsWithResult() persists the Notification(s) or outbox entry
before scheduling/sending so no sends occur prior to a successful commit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 807734e1-657f-4a8f-8c07-71a759290b43
📒 Files selected for processing (1)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java
Comment on lines
+41
to
42
| @Transactional | ||
| public void pushNotifications(List<Notification> notifications) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the NotificationService file
find . -name "NotificationService.java" -type fRepository: BCSDLab/KOIN_API_V2
Length of output: 149
🏁 Script executed:
# Check the NotificationService implementation, focusing on the flagged lines
cd src/main/java/in/koreatech/koin/domain/notification/service && wc -l NotificationService.javaRepository: BCSDLab/KOIN_API_V2
Length of output: 91
🏁 Script executed:
# Get an overview of the file structure
cd src/main/java/in/koreatech/koin/domain/notification/service && head -100 NotificationService.javaRepository: BCSDLab/KOIN_API_V2
Length of output: 4285
🏁 Script executed:
# Now run the verification checks from the review comment
#!/bin/bash
set -euo pipefail
echo "== NotificationService call sites and transaction context =="
rg -n -C3 --type java 'notificationService\.(pushNotificationsWithResult|pushNotifications|pushNotification)\(' src/main/java
echo
echo "== Check for AFTER_COMMIT handling =="
rg -n -C3 --type java '@TransactionalEventListener|TransactionPhase\.AFTER_COMMIT|afterCommit' src/main/javaRepository: BCSDLab/KOIN_API_V2
Length of output: 39392
🏁 Script executed:
# Read the rest of NotificationService file
sed -n '40,90p' src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javaRepository: BCSDLab/KOIN_API_V2
Length of output: 2021
🏁 Script executed:
# Check lines 152-153 mentioned in the review
tail -30 src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javaRepository: BCSDLab/KOIN_API_V2
Length of output: 1263
🏁 Script executed:
# Check one of the caller classes to understand transaction context
cat -n src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.javaRepository: BCSDLab/KOIN_API_V2
Length of output: 6663
🏁 Script executed:
# Check another important caller
cat -n src/main/java/in/koreatech/koin/domain/notification/eventlistener/ShopEventListener.javaRepository: BCSDLab/KOIN_API_V2
Length of output: 2259
🏁 Script executed:
# Check NotificationScheduleService which is a non-event-listener caller
cat -n src/main/java/in/koreatech/koin/domain/shop/service/NotificationScheduleService.javaRepository: BCSDLab/KOIN_API_V2
Length of output: 4318
🏁 Script executed:
# Check for any outbox or pending notification tables/patterns
rg -i 'outbox|pending.*notification' --type java src/main/java/in/koreatech/koin/domain/notification/Repository: BCSDLab/KOIN_API_V2
Length of output: 45
🏁 Script executed:
# Check if there's any event publishing in NotificationService or related services
rg -n 'ApplicationEventPublisher|publishEvent|sendEvent' --type java src/main/java/in/koreatech/koin/domain/notification/Repository: BCSDLab/KOIN_API_V2
Length of output: 45
🏁 Script executed:
# Look at FcmClient to understand what sendMessage does
find . -name "FcmClient.java" -o -name "*FcmClient*" | head -5Repository: BCSDLab/KOIN_API_V2
Length of output: 130
🏁 Script executed:
# Read FcmClient to understand the external call
cat -n src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.javaRepository: BCSDLab/KOIN_API_V2
Length of output: 5046
🏁 Script executed:
# Check if any transaction management or afterCommit patterns exist in the codebase
rg -n 'TransactionSynchronizationManager|registerSynchronization' --type java src/main/java | head -10Repository: BCSDLab/KOIN_API_V2
Length of output: 45
🏁 Script executed:
# Check how NotificationScheduleService caller is being used (from a scheduler or transactional context)
rg -n 'sendDueNotifications' --type java src/main/java -B 3 -A 3Repository: BCSDLab/KOIN_API_V2
Length of output: 2102
Separate FCM push notifications from transactional boundary using AFTER_COMMIT phase or event publishing.
FCM notifications are sent within transaction boundaries in pushNotification() (lines 59-67) and especially in pushNotificationsWithResult() (lines 72-80), creating an inconsistency risk: if the transaction rolls back, notifications are already delivered to users but their state is not persisted in the database.
In pushNotificationsWithResult(), sendMessageWithResult() executes synchronously before the conditional save(), so a rollback leaves the sent notification unrecorded. In pushNotification(), though sendMessage() is @Async, the task submission happens within the transaction, and if the transaction rolls back, the queued async task will likely still execute.
This affects all call paths including event listeners (ArticleKeywordEventListener, ShopEventListener, ClubRecruitmentEventListener, etc.) and the scheduler-driven NotificationScheduleService.sendDueNotifications().
Move notification sending to execute after transaction commit using @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) or an outbox pattern instead.
Also applies to: 48-53, 152-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`
around lines 41 - 42, The FCM sends occur inside transaction boundaries (see
pushNotifications / pushNotification(), pushNotificationsWithResult() and
sendMessageWithResult()/sendMessage()) which risks delivering notifications when
the DB later rolls back; refactor by decoupling send logic to run AFTER_COMMIT:
publish an application event (e.g., NotificationCreatedEvent) instead of calling
sendMessage/sendMessageWithResult directly from ArticleKeywordEventListener,
ShopEventListener, ClubRecruitmentEventListener and
NotificationScheduleService.sendDueNotifications(), and implement an event
listener annotated with `@TransactionalEventListener`(phase =
TransactionPhase.AFTER_COMMIT) that performs the FCM send (or implement an
outbox pattern where pushNotifications persists outbox rows inside the
transaction and a separate dispatcher sends after commit). Ensure
pushNotificationsWithResult() persists the Notification(s) or outbox entry
before scheduling/sending so no sends occur prior to a successful commit.
Comment on lines
+49
to
+53
| public List<NotificationDeliveryResult> pushNotificationsWithResult(List<Notification> notifications) { | ||
| return notifications.stream() | ||
| .map(this::pushNotificationWithResult) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the NotificationService class
fd 'NotificationService.java' -x cat -n {}Repository: BCSDLab/KOIN_API_V2
Length of output: 19027
🏁 Script executed:
#!/bin/bash
# Find FcmClient to understand what exceptions sendMessageWithResult can throw
fd 'FcmClient.java' -x head -100 {}Repository: BCSDLab/KOIN_API_V2
Length of output: 3205
🏁 Script executed:
#!/bin/bash
# Search for the pushNotificationWithResult method implementation (lines 54-85)
fd 'NotificationService.java' -x sed -n '49,90p' {}Repository: BCSDLab/KOIN_API_V2
Length of output: 5270
Exception handling needed for batch stability
pushNotificationsWithResult can fail partially due to exceptions from notificationRepository.save() at line 82. When an exception occurs, the stream stops processing remaining items and the @Transactional annotation causes the entire batch transaction to rollback, leaving an incomplete result list. Wrap the operation in try-catch to absorb exceptions and return delivered=false for failed items, similar to the pattern in AdminNotificationService.
💡 제안 패치
private NotificationDeliveryResult pushNotificationWithResult(Notification notification) {
String deviceToken = notification.getUser().getDeviceToken();
- boolean delivered = fcmClient.sendMessageWithResult(
- deviceToken,
- notification.getTitle(),
- notification.getMessage(),
- notification.getImageUrl(),
- notification.getMobileAppPath(),
- notification.getSchemeUri(),
- notification.getType().toLowerCase()
- );
- if (delivered) {
- notificationRepository.save(notification);
- }
- return new NotificationDeliveryResult(notification, delivered);
+ try {
+ boolean delivered = fcmClient.sendMessageWithResult(
+ deviceToken,
+ notification.getTitle(),
+ notification.getMessage(),
+ notification.getImageUrl(),
+ notification.getMobileAppPath(),
+ notification.getSchemeUri(),
+ notification.getType().toLowerCase()
+ );
+ if (delivered) {
+ notificationRepository.save(notification);
+ }
+ return new NotificationDeliveryResult(notification, delivered);
+ } catch (RuntimeException e) {
+ return new NotificationDeliveryResult(notification, false);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`
around lines 49 - 53, pushNotificationsWithResult currently streams
notifications and calls pushNotificationWithResult which can throw and abort the
stream and transaction; change the mapping so each call is wrapped in a
try-catch that catches Exception, logs the failure, and returns a
NotificationDeliveryResult for that Notification with delivered=false (mirroring
AdminNotificationService behavior) so one failure doesn't stop the rest and the
transaction won't roll back for the whole batch; update the lambda in
pushNotificationsWithResult to call pushNotificationWithResult inside the try
block and construct a failed NotificationDeliveryResult in the catch.
DHkimgit
approved these changes
Mar 6, 2026
Comment on lines
+159
to
+160
| for (Integer id : updateNotificationIds) { | ||
| articles.add(articleRepository.getById(id)); |
Collaborator
There was a problem hiding this comment.
articleRepository.findAllById(updateNotificationIds)
한번에 가져오는건 어떨까요
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.
🔍 개요
근로,근로장학/A,C) 사용자에게 동일 게시글 알림이 중복 발송되었습니다.🚀 주요 변경 내용
ArticleKeywordEvent를 단일 키워드에서matchedKeywords(다중 키워드) 구조로 변경KeywordExtractor에서 게시글당 이벤트를 1건만 생성하도록 변경KeywordService.sendKeywordNotification에서 중복 게시글 ID를 제거 후 처리하도록 변경ArticleKeywordEventListener에서UserNotificationStatus저장 로직을 upsert 방식(기존 row 갱신)으로 보완하여 충돌 가능성 제거💬 참고 사항
/articles/keyword/notification을 호출하는 흐름에서 재현되던 중복 알림을 기준으로 수정했습니다.KeywordExtractorTestKeywordServiceTestArticleKeywordEventListenerTest./gradlew test --rerun-tasks전체 테스트 통과✅ Checklist (완료 조건)
Summary by CodeRabbit
New Features
Bug Fixes
Tests