Refactor : FCM 리펙토링 및 PostController Mapping 관계 재설정 #95
Conversation
WalkthroughReplaces user-scoped FCM token handling with a PushToken domain (entity, repo, service, controllers), removes legacy FCMService and user fcmToken APIs, adds FcmSender and push-send controllers, refactors NotificationService to post-commit dispatch, and updates comment notification flow and some PostController routes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant PushMessageController as PushMessageController
participant FcmSender as FcmSender
participant Firebase as FirebaseMessaging
Client->>PushMessageController: POST /api/push/token (token, title, body, data, highPriority, clickAction)
PushMessageController->>FcmSender: sendToToken(...)
FcmSender->>FcmSender: build Message + AndroidConfig
FcmSender->>Firebase: FirebaseMessaging.send(message)
Firebase-->>FcmSender: messageId
FcmSender-->>PushMessageController: messageId
PushMessageController-->>Client: 200 OK (messageId)
sequenceDiagram
autonumber
participant Commenter as User
participant CommentSvc as CommentCommandServiceImpl
participant NotificationSvc as NotificationService
participant Repo as PushTokenRepository
participant Fcm as FcmSender
Commenter->>CommentSvc: create comment
CommentSvc->>CommentSvc: determine receivers (post writer, parent writer)
loop per receiver
CommentSvc->>NotificationSvc: saveAndDispatch(notification, receiverId, title, body, data, highPriority, clickAction)
NotificationSvc->>Repo: findActiveTokensByUserId(receiverId) [post-commit]
NotificationSvc->>Fcm: sendToTokens(tokens,...)
Fcm-->>NotificationSvc: BatchResult
NotificationSvc->>Repo: bulkDeactivateByTokens(failedTokens) [REQUIRES_NEW]
end
CommentSvc-->>Commenter: comment created response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 12
🧹 Nitpick comments (14)
src/main/java/naughty/tuzamate/domain/post/controller/PostController.java (1)
40-83: Consider the consistency impact with other REST endpoints.The routing changes create an inconsistency within the same controller: some endpoints still use
/boards/{boardType}/posts(create and list operations) while others now use/boards/posts/{postId}(get, update, delete). This mixed pattern could be confusing for API consumers.Looking at the CommentController (from relevant code snippets), it uses a consistent
/posts/{postId}/commentspattern. Consider whether the PostController should adopt a similar consistent approach.Apply this diff to maintain routing consistency (if the business logic supports it):
- @PostMapping("/boards/{boardType}/posts") + @PostMapping("/posts") @Operation(summary = "게시글 생성", description = "게시판에 맞는 게시글을 생성합니다.") public CustomResponse<PostResDTO.CreatePostResponseDTO> createPost( - @PathVariable BoardType boardType, + @RequestParam BoardType boardType, @RequestBody PostReqDTO.CreatePostRequestDTO reqDTO, @AuthenticationPrincipal PrincipalDetails principalDetails ) { - @GetMapping("/boards/{boardType}/posts") + @GetMapping("/posts") @Operation(summary = "게시글 목록 조회", description = "각 BoardType(FREE, INFO)에 맞는 게시글 목록을 조회합니다.") public CustomResponse<PostResDTO.PostPreviewListDTO> getPostList( - @PathVariable BoardType boardType, + @RequestParam BoardType boardType, @RequestParam(required = false) Long cursor, @RequestParam(defaultValue = "10") @Max(10) int size) {Alternatively, revert the simplified paths to maintain the original consistent structure:
- @GetMapping("/boards/posts/{postId}") + @GetMapping("/boards/{boardType}/posts/{postId}") - @PatchMapping("/boards/posts/{postId}") + @PatchMapping("/boards/{boardType}/posts/{postId}") - @DeleteMapping("/boards/posts/{postId}") + @DeleteMapping("/boards/{boardType}/posts/{postId}")src/main/java/naughty/tuzamate/domain/comment/service/command/CommentCommandServiceImpl.java (2)
23-25: Consider using HashSet instead of LinkedHashSet.Since the order of notification recipients doesn't matter for processing and you only iterate through the collection once, HashSet would be generally faster than LinkedHashSet due to its simpler structure and LinkedHashSet is slightly slower than HashSet due to the added expense of maintaining the linked list.
Apply this diff to use HashSet:
-import java.util.LinkedHashSet; +import java.util.HashSet; import java.util.Map; import java.util.Set;- Set<Long> receivers = new LinkedHashSet<>(); + Set<Long> receivers = new HashSet<>();
82-87: Consider externalizing notification content and deeplink format.The hardcoded notification title, content format, and deeplink URL pattern should be moved to configuration or constants for better maintainability and internationalization support.
Create a constants class or configuration properties:
public class NotificationConstants { public static final String NEW_COMMENT_TITLE = "새 댓글이 달렸습니다"; public static final String DEEPLINK_PATTERN = "myapp://post/%d?commentId=%d"; public static final String CLICK_ACTION_OPEN_POST = "OPEN_POST"; }Then update the usage:
- Map<String, String> data = Map.of( - "type", "COMMENT", - "postId", String.valueOf(post.getId()), - "commentId", String.valueOf(comment.getId()), - "deeplink", "myapp://post/" + post.getId() + "?commentId=" + comment.getId() - ); + Map<String, String> data = Map.of( + "type", "COMMENT", + "postId", String.valueOf(post.getId()), + "commentId", String.valueOf(comment.getId()), + "deeplink", String.format(NotificationConstants.DEEPLINK_PATTERN, post.getId(), comment.getId()) + );src/main/java/naughty/tuzamate/domain/pushToken/dto/PushTokenReqDTO.java (2)
6-10: Remove unnecessary Lombok @DaTa on container.
PushTokenReqDTOholds only records;@Datais redundant. Make the container non-instantiable for clarity.-import lombok.Data; +// no Lombok needed here -@Data public class PushTokenReqDTO { + private PushTokenReqDTO() {}
12-22: Add basic size guards to inputs.Prevent pathological payloads; keeps validation meaningful with @Valid.
+import jakarta.validation.constraints.Size; @@ public record RegisterPushTokenReqDTO( - @NotBlank - String token, + @NotBlank @Size(max = 1024) + String token, @@ - @NotBlank - String deviceId + @NotBlank @Size(max = 256) + String deviceId ) {}src/main/java/naughty/tuzamate/domain/pushToken/repository/PushTokenRepository.java (1)
19-21: Return updated count and guard empty lists upstream.Returning
inthelps telemetry; JPAIN ()with empty list is invalid—callers should short‑circuit.- @Modifying(clearAutomatically = true) + @Modifying(clearAutomatically = true) @Query("update PushToken pt set pt.isActive=false where pt.token in :tokens") - void bulkDeactivateByTokens(@Param("tokens") List<String> tokens); + int bulkDeactivateByTokens(@Param("tokens") List<String> tokens);src/main/java/naughty/tuzamate/domain/pushToken/controller/PushTokenController.java (1)
41-48: Heartbeat endpoint is fine; consider a lighter DTO later.src/main/java/naughty/tuzamate/domain/pushToken/entity/PushToken.java (3)
35-37: Make platform non-null.Tokens without platform complicate targeting/cleanup logic.
- @Enumerated(EnumType.STRING) + @Enumerated(EnumType.STRING) + @Column(nullable = false) private Platform platform;
41-45: Optional: use primitive boolean to avoid null semantics.
Boolean+@Builder.Defaultworks, butbooleanremoves the possibility of accidental nulls in custom builders/mappers.- private Boolean isActive = false; + private boolean isActive = false;Note: Keep
@Column(name = "is_active", nullable = false)as suggested above.
46-52: Clarify time basis for lastSeenAt (UTC vs server TZ).
LocalDateTime.now()uses server TZ. Prefer UTC (Instant) or inject aClockfor determinism; alternatively ensure service layer updates in UTC.src/main/java/naughty/tuzamate/domain/pushToken/FcmSender.java (4)
58-58: Continue on chunk failure to improve resilience.Wrap per‑chunk send to prevent one bad batch from aborting the rest.
- BatchResponse response = FirebaseMessaging.getInstance().sendEachForMulticast(mb.build()); + BatchResponse response; + try { + response = FirebaseMessaging.getInstance().sendEachForMulticast(mb.build()); + } catch (FirebaseMessagingException ex) { + log.warn("FCM multicast batch failed size={}, code={}", chunk.size(), ex.getMessagingErrorCode(), ex); + totalFailure += chunk.size(); + continue; + }
28-28: Inject FirebaseMessaging bean instead of static access (testability/config).Use DI to allow mocking and central config.
- return FirebaseMessaging.getInstance().send(mb.build()); + return firebaseMessaging.send(mb.build()); @@ - BatchResponse response; + BatchResponse response; try { - response = FirebaseMessaging.getInstance().sendEachForMulticast(mb.build()); + response = firebaseMessaging.sendEachForMulticast(mb.build());Add a field (class already has
@RequiredArgsConstructor):private final FirebaseMessaging firebaseMessaging;Also applies to: 58-58
100-110: Avoid setting null title/body on Notification.Guard per field to prevent nulls in payload.
- if (title != null || body != null) { - b.setNotification(Notification.builder().setTitle(title).setBody(body).build()); - } + if (title != null || body != null) { + Notification.Builder nb = Notification.builder(); + if (title != null && !title.isBlank()) nb.setTitle(title); + if (body != null && !body.isBlank()) nb.setBody(body); + b.setNotification(nb.build()); + }Apply similarly to
MulticastMessage.Builder.
119-126: Optional: add APNs/WebPush configs when targeting iOS/Web.Mirror priority/click handling for APNs/WebPush if those platforms are used.
Would you like a follow‑up patch adding
ApnsConfig(withapns-priority=10,apsfields) andWebpushConfig?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/java/naughty/tuzamate/domain/comment/service/FCMService.java(0 hunks)src/main/java/naughty/tuzamate/domain/comment/service/command/CommentCommandServiceImpl.java(2 hunks)src/main/java/naughty/tuzamate/domain/post/controller/PostController.java(3 hunks)src/main/java/naughty/tuzamate/domain/pushToken/FcmSender.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/code/PushTokenErrorCode.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/controller/PushMessageController.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/controller/PushTokenController.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/dto/PushTokenReqDTO.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/dto/PushTokenSendDTO.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/entity/PushToken.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/enums/Platform.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/repository/PushTokenRepository.java(1 hunks)src/main/java/naughty/tuzamate/domain/pushToken/service/PushTokenService.java(1 hunks)src/main/java/naughty/tuzamate/domain/user/controller/UserController.java(0 hunks)src/main/java/naughty/tuzamate/domain/user/entity/User.java(0 hunks)src/main/java/naughty/tuzamate/domain/user/service/UserService.java(0 hunks)src/main/java/naughty/tuzamate/domain/user/service/UserServiceImpl.java(0 hunks)
💤 Files with no reviewable changes (5)
- src/main/java/naughty/tuzamate/domain/user/service/UserServiceImpl.java
- src/main/java/naughty/tuzamate/domain/user/service/UserService.java
- src/main/java/naughty/tuzamate/domain/comment/service/FCMService.java
- src/main/java/naughty/tuzamate/domain/user/controller/UserController.java
- src/main/java/naughty/tuzamate/domain/user/entity/User.java
🧰 Additional context used
🧬 Code graph analysis (7)
src/main/java/naughty/tuzamate/domain/pushToken/enums/Platform.java (1)
src/main/java/naughty/tuzamate/domain/user/enums/SocialType.java (1)
SocialType(3-5)
src/main/java/naughty/tuzamate/domain/pushToken/dto/PushTokenSendDTO.java (3)
src/main/java/naughty/tuzamate/domain/user/dto/FcmRequestDTO.java (1)
FcmRequestDTO(5-8)src/main/java/naughty/tuzamate/domain/post/dto/PostReqDTO.java (3)
PostReqDTO(5-18)CreatePostRequestDTO(7-11)UpdatePostRequestDTO(13-17)src/main/java/naughty/tuzamate/domain/user/dto/UserInitProfileRequestDTO.java (1)
UserInitProfileRequestDTO(6-12)
src/main/java/naughty/tuzamate/domain/pushToken/dto/PushTokenReqDTO.java (6)
src/main/java/naughty/tuzamate/domain/annuity/dto/AnnuityRequestDTO.java (1)
AnnuityRequestDTO(7-32)src/main/java/naughty/tuzamate/domain/post/dto/PostReqDTO.java (1)
PostReqDTO(5-18)src/main/java/naughty/tuzamate/domain/deposit/dto/DepositRequestDTO.java (1)
DepositRequestDTO(7-30)src/main/java/naughty/tuzamate/domain/savings/dto/SavingsRequestDTO.java (1)
SavingsRequestDTO(7-30)src/main/java/naughty/tuzamate/domain/notification/dto/NotificationResDTO.java (2)
NotificationResDTO(8-24)toNotificationResDTO(10-16)src/main/java/naughty/tuzamate/domain/notification/dto/NotificationReqDTO.java (1)
NotificationReqDTO(5-12)
src/main/java/naughty/tuzamate/domain/pushToken/repository/PushTokenRepository.java (1)
src/main/java/naughty/tuzamate/auth/repository/RefreshTokenRepository.java (2)
RefreshTokenRepository(8-15)findByToken(10-10)
src/main/java/naughty/tuzamate/domain/post/controller/PostController.java (1)
src/main/java/naughty/tuzamate/domain/comment/controller/CommentController.java (1)
RestController(16-87)
src/main/java/naughty/tuzamate/domain/pushToken/controller/PushTokenController.java (2)
src/main/java/naughty/tuzamate/domain/user/controller/UserController.java (2)
UserController(25-61)saveFcmToken(52-60)src/main/java/naughty/tuzamate/domain/notification/controller/NotificationController.java (1)
NotificationController(16-73)
src/main/java/naughty/tuzamate/domain/pushToken/controller/PushMessageController.java (1)
src/main/java/naughty/tuzamate/domain/pushToken/dto/PushTokenSendDTO.java (1)
PushTokenSendDTO(6-26)
🪛 GitHub Actions: Java CI with Gradle
src/main/java/naughty/tuzamate/domain/comment/service/command/CommentCommandServiceImpl.java
[error] 89-89: cannot find symbol: method saveAndDispatch(Notification,Long,String,String,Map<String,String>,boolean,String) in NotificationService
🔇 Additional comments (11)
src/main/java/naughty/tuzamate/domain/post/controller/PostController.java (3)
74-83: Approve — removing {boardType} from delete endpoint is safe.deletePost(Long postId, PrincipalDetails) loads the Post, enforces owner-only deletion (throws GeneralErrorCode.FORBIDDEN_403 on mismatch), then deletes; no boardType-dependent authorization/validation found.
Locations: src/main/java/naughty/tuzamate/domain/post/service/command/PostCommandService.java (deletePost signature), src/main/java/naughty/tuzamate/domain/post/service/command/PostCommandServiceImpl.java (ownership check + delete), src/main/java/naughty/tuzamate/domain/post/controller/PostController.java (DELETE mapping).
40-49: No action needed — removing {boardType} from single-post route is safe.PostQueryService.getPost(Long postId, PrincipalDetails) does not accept BoardType and PostQueryServiceImpl.getPost(...) loads the Post by ID and does not use boardType (checks read/liked/scraped). Controller still uses /boards/{boardType}/posts for lists and /boards/posts/{postId} for single-post operations; no routing conflict was found. See: src/main/java/naughty/tuzamate/domain/post/service/query/PostQueryService.java (getPost signature), src/main/java/naughty/tuzamate/domain/post/service/query/PostQueryServiceImpl.java (getPost impl), src/main/java/naughty/tuzamate/domain/post/controller/PostController.java (mappings).
62-72: No action required — removing {boardType} from update/delete endpoints is safe.updatePost/deletePost load the Post by postId and validate ownership; neither reads a BoardType path variable and no update/delete logic depends on a boardType param. createPost/getPostList still require BoardType.
See: src/main/java/naughty/tuzamate/domain/post/service/command/PostCommandServiceImpl.java (updatePost), src/main/java/naughty/tuzamate/domain/post/entity/Post.java (boardType), src/main/java/naughty/tuzamate/domain/post/repository/PostRepository.java (findByBoardTypeAndCursor), src/main/java/naughty/tuzamate/domain/post/controller/PostController.java (create/getList mappings).
src/main/java/naughty/tuzamate/domain/comment/service/command/CommentCommandServiceImpl.java (3)
58-70: LGTM! Well-designed notification recipient logic.The logic correctly identifies notification recipients while avoiding duplicate notifications:
- Post author gets notified for new comments (unless they're the commenter)
- Parent comment author gets notified for replies (unless they're the commenter or post author)
- LinkedHashSet ensures no duplicates while preserving insertion order
73-94: LGTM! Clean refactoring from direct FCM to notification service.The notification dispatch logic is well-structured:
- Properly constructs notification entities with required fields
- Includes comprehensive data payload for frontend navigation
- Uses consistent notification title and content format
- Handles batch processing correctly for multiple recipients
89-93: Confirm NotificationService.saveAndDispatch signature — invocation uses 7 argsSearch of the repo did not locate a definition for NotificationService.saveAndDispatch; the call at src/main/java/naughty/tuzamate/domain/comment/service/command/CommentCommandServiceImpl.java:89-93 passes seven arguments. Add a matching overload to NotificationService or change this call to match the actual method signature.
src/main/java/naughty/tuzamate/domain/pushToken/service/PushTokenService.java (2)
59-71: Heartbeat flow looks correct.
21-44: DB uniqueness verified — no action required
PushToken.java's @table declares a unique constraint (uk_push_token_token) on column 'token', so the database enforces token uniqueness.src/main/java/naughty/tuzamate/domain/pushToken/controller/PushTokenController.java (1)
21-29: Register endpoint looks good.src/main/java/naughty/tuzamate/domain/pushToken/enums/Platform.java (1)
3-3: Persist enum as STRING — verified.
PushToken.platform is annotated with @Enumerated(EnumType.STRING) in src/main/java/naughty/tuzamate/domain/pushToken/entity/PushToken.java (line 35); no change required.src/main/java/naughty/tuzamate/domain/pushToken/entity/PushToken.java (1)
32-34: Reduce token column length to avoid MySQL/utf8mb4 unique-index failures.UniqueConstraint on token exists; length=2048 can exceed MySQL/utf8mb4 index limits — change to 512.
- @Column(nullable = false, length = 2048) + @Column(nullable = false, length = 512) private String token;Could not find DB config/build files in the repo to confirm engine; confirm whether you’re using MySQL with utf8mb4 before merging.
| TOKEN_NOT_FOUND(HttpStatus.NOT_FOUND, "TOKEN40401", "해당 토큰이 존재하지 않습니다."), | ||
| TOKEN_NOT_OWNER(HttpStatus.BAD_REQUEST, "TOKEN402", "토큰 소유자가 아닙니다."); | ||
|
|
||
| private final HttpStatus status; | ||
| private final String code; | ||
| private final String message; |
There was a problem hiding this comment.
HTTP status/code mismatch for NOT_OWNER.
Use 403 (Forbidden) and align the app code string with your 404 scheme.
- TOKEN_NOT_FOUND(HttpStatus.NOT_FOUND, "TOKEN40401", "해당 토큰이 존재하지 않습니다."),
- TOKEN_NOT_OWNER(HttpStatus.BAD_REQUEST, "TOKEN402", "토큰 소유자가 아닙니다.");
+ TOKEN_NOT_FOUND(HttpStatus.NOT_FOUND, "TOKEN40401", "해당 토큰이 존재하지 않습니다."),
+ TOKEN_NOT_OWNER(HttpStatus.FORBIDDEN, "TOKEN40301", "토큰 소유자가 아닙니다.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TOKEN_NOT_FOUND(HttpStatus.NOT_FOUND, "TOKEN40401", "해당 토큰이 존재하지 않습니다."), | |
| TOKEN_NOT_OWNER(HttpStatus.BAD_REQUEST, "TOKEN402", "토큰 소유자가 아닙니다."); | |
| private final HttpStatus status; | |
| private final String code; | |
| private final String message; | |
| TOKEN_NOT_FOUND(HttpStatus.NOT_FOUND, "TOKEN40401", "해당 토큰이 존재하지 않습니다."), | |
| TOKEN_NOT_OWNER(HttpStatus.FORBIDDEN, "TOKEN40301", "토큰 소유자가 아닙니다."); | |
| private final HttpStatus status; | |
| private final String code; | |
| private final String message; |
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/domain/pushToken/code/PushTokenErrorCode.java
around lines 11 to 16, the TOKEN_NOT_OWNER enum entry uses
HttpStatus.BAD_REQUEST and an inconsistent code string; change the HttpStatus to
HttpStatus.FORBIDDEN and update the application code string to follow the
existing scheme (e.g. "TOKEN40301") so it matches the 403 status, leaving the
message text intact.
| // test 용 | ||
| @RestController | ||
| @RequiredArgsConstructor | ||
| @RequestMapping("/api/push") | ||
| // @PreAuthorize("hasRole('ADMIN')") // 운영/관리자 전용 권장 | ||
| public class PushMessageController { |
There was a problem hiding this comment.
Lock down “test” push endpoints.
These endpoints can spam arbitrary tokens if exposed. Gate them.
+import org.springframework.security.access.prepost.PreAuthorize;
@@
-// test 용
@RestController
@RequiredArgsConstructor
@RequestMapping("/api/push")
-// @PreAuthorize("hasRole('ADMIN')") // 운영/관리자 전용 권장
+@PreAuthorize("hasRole('ADMIN')")
public class PushMessageController {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // test 용 | |
| @RestController | |
| @RequiredArgsConstructor | |
| @RequestMapping("/api/push") | |
| // @PreAuthorize("hasRole('ADMIN')") // 운영/관리자 전용 권장 | |
| public class PushMessageController { | |
| import org.springframework.security.access.prepost.PreAuthorize; | |
| @RestController | |
| @RequiredArgsConstructor | |
| @RequestMapping("/api/push") | |
| @PreAuthorize("hasRole('ADMIN')") | |
| public class PushMessageController { |
🤖 Prompt for AI Agents
In
src/main/java/naughty/tuzamate/domain/pushToken/controller/PushMessageController.java
around lines 13 to 18, the controller is left open for "test" use and can send
push messages to arbitrary tokens; re-lock it by restoring a security annotation
and ensuring method security is enabled: add @PreAuthorize("hasRole('ADMIN')")
(or equivalent role check) to the controller or each endpoint, ensure your
application has method security enabled (e.g., @EnableMethodSecurity or
@EnableGlobalMethodSecurity in your security config), and remove or restrict any
test-only endpoints/mappings (or move them behind an internal-only path/profile)
so only authorized admin users can call these APIs.
| package naughty.tuzamate.domain.pushToken.controller; | ||
|
|
||
| import jakarta.validation.Valid; | ||
| import lombok.RequiredArgsConstructor; | ||
| import naughty.tuzamate.auth.principal.PrincipalDetails; | ||
| import naughty.tuzamate.domain.pushToken.dto.PushTokenReqDTO; | ||
| import naughty.tuzamate.domain.pushToken.entity.PushToken; | ||
| import naughty.tuzamate.domain.pushToken.service.PushTokenService; | ||
| import naughty.tuzamate.global.apiPayload.CustomResponse; | ||
| import naughty.tuzamate.global.success.GeneralSuccessCode; | ||
| import org.springframework.security.core.annotation.AuthenticationPrincipal; | ||
| import org.springframework.web.bind.annotation.*; | ||
|
|
||
| @RestController | ||
| @RequiredArgsConstructor | ||
| @RequestMapping("/api/push-tokens") | ||
| public class PushTokenController { | ||
|
|
||
| private final PushTokenService pushTokenService; | ||
|
|
||
| @PostMapping | ||
| public CustomResponse<PushToken> register( | ||
| @Valid @RequestBody PushTokenReqDTO.RegisterPushTokenReqDTO request, | ||
| @AuthenticationPrincipal PrincipalDetails principal | ||
| ) { | ||
| PushToken token = pushTokenService.upsert(request, principal.getId()); | ||
|
|
||
| return CustomResponse.onSuccess(GeneralSuccessCode.OK, token); | ||
| } | ||
|
|
||
| // 토큰 해제(로그아웃 -> deactivate = false, 탈퇴 -> deactivate = false) | ||
| @DeleteMapping | ||
| public CustomResponse<?> unbind( | ||
| @Valid @RequestBody PushTokenReqDTO.UnbindPushTokenReqDTO request | ||
| ) { | ||
| pushTokenService.unbind(request.token(), request.deactivate()); | ||
|
|
||
| return CustomResponse.onSuccess(GeneralSuccessCode.OK); | ||
| } | ||
|
|
||
| @PostMapping("/heartbeat") | ||
| public CustomResponse<?> heartbeat( | ||
| @Valid @RequestBody PushTokenReqDTO.RegisterPushTokenReqDTO req, | ||
| @AuthenticationPrincipal PrincipalDetails principal | ||
| ) { | ||
| pushTokenService.touch(req.token(), principal.getId()); | ||
| return CustomResponse.onSuccess(GeneralSuccessCode.OK); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clean up legacy User fcmToken APIs to avoid drift.
Verify old UserController.saveFcmToken, userService.updateFcmToken, and FcmRequestDTO are removed or deprecated per PR goal.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 -e 'saveFcmToken' -e 'updateFcmToken' -e '\bfcmToken\b' -e 'FcmRequestDTO' --type=javaLength of output: 1075
Remove legacy User FCM APIs (delete or deprecate FcmRequestDTO and controller usage)
FcmRequestDTO and its import in UserController are still present — remove or mark deprecated and migrate calls to the new PushTokenController (upsert/unbind/heartbeat).
- src/main/java/naughty/tuzamate/domain/user/dto/FcmRequestDTO.java
- src/main/java/naughty/tuzamate/domain/user/controller/UserController.java (imports FcmRequestDTO)
| // 토큰 해제(로그아웃 -> deactivate = false, 탈퇴 -> deactivate = false) | ||
| @DeleteMapping | ||
| public CustomResponse<?> unbind( | ||
| @Valid @RequestBody PushTokenReqDTO.UnbindPushTokenReqDTO request | ||
| ) { | ||
| pushTokenService.unbind(request.token(), request.deactivate()); | ||
|
|
||
| return CustomResponse.onSuccess(GeneralSuccessCode.OK); | ||
| } |
There was a problem hiding this comment.
Unbind must be authenticated and owner-checked. Also fix comment.
Currently anyone can unbind any token. Require principal and pass userId to service. Comment says 탈퇴=false; should be true.
- // 토큰 해제(로그아웃 -> deactivate = false, 탈퇴 -> deactivate = false)
+ // 토큰 해제(로그아웃 -> deactivate = false, 탈퇴 -> deactivate = true)
@DeleteMapping
public CustomResponse<?> unbind(
- @Valid @RequestBody PushTokenReqDTO.UnbindPushTokenReqDTO request
+ @Valid @RequestBody PushTokenReqDTO.UnbindPushTokenReqDTO request,
+ @AuthenticationPrincipal PrincipalDetails principal
) {
- pushTokenService.unbind(request.token(), request.deactivate());
+ pushTokenService.unbind(request.token(), request.deactivate(), principal.getId());
return CustomResponse.onSuccess(GeneralSuccessCode.OK);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/naughty/tuzamate/domain/pushToken/controller/PushTokenController.java
around lines 31-39, the unbind endpoint is currently unauthenticated and allows
anyone to unbind any token, and the comment incorrectly states 탈퇴=false; change
the method to require authentication (add a Principal or
@AuthenticationPrincipal parameter), extract the authenticated user's id (or
username->id) and pass that userId into pushTokenService.unbind(...) so the
service can perform an owner check, and update the comment to reflect that
탈퇴=true for account deletion; ensure the service signature is updated if
necessary to accept the userId and perform the owner verification.
| public record SendToTokenRequest ( | ||
| String token, | ||
| String title, | ||
| String body, | ||
| Map<String, String> data, | ||
| Boolean highPriority, // null 허용 → 기본 false | ||
| String clickAction | ||
| ) {} |
There was a problem hiding this comment.
@Valid has no effect without constraints (single token).
Add constraints so empty/blank tokens are rejected at the API boundary.
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.NotEmpty;
@@
public record SendToTokenRequest (
- String token,
+ @NotBlank String token,
String title,
String body,
Map<String, String> data,
Boolean highPriority, // null 허용 → 기본 false
String clickAction
) {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record SendToTokenRequest ( | |
| String token, | |
| String title, | |
| String body, | |
| Map<String, String> data, | |
| Boolean highPriority, // null 허용 → 기본 false | |
| String clickAction | |
| ) {} | |
| import jakarta.validation.constraints.NotBlank; | |
| import jakarta.validation.constraints.NotEmpty; | |
| public record SendToTokenRequest ( | |
| @NotBlank String token, | |
| String title, | |
| String body, | |
| Map<String, String> data, | |
| Boolean highPriority, // null 허용 → 기본 false | |
| String clickAction | |
| ) {} |
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/domain/pushToken/dto/PushTokenSendDTO.java
around lines 8-15, the record's token component lacks validation so @Valid has
no effect; annotate the token component with a javax.validation constraint
(e.g., @NotBlank) and add the corresponding import so empty or blank tokens are
rejected at the API boundary, and ensure the controller method receiving this
DTO uses @Valid (and the controller is @Validated) so Bean Validation runs.
| ) throws Exception { | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Narrow throws to FirebaseMessagingException.
Avoid throws Exception in public API.
- ) throws Exception {
+ ) throws FirebaseMessagingException {
@@
- ) throws Exception {
+ ) throws FirebaseMessagingException {Also applies to: 39-40
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/domain/pushToken/FcmSender.java around lines
20-21 and 39-40, the public methods currently declare "throws Exception"; narrow
these to the specific FirebaseMessagingException: change the method signatures
to "throws FirebaseMessagingException", add the appropriate import, and remove
the generic Exception from the throws clause. Update any internal try/catch to
handle other checked exceptions locally (wrap or convert to runtime) or
translate them into FirebaseMessagingException as appropriate, and adjust
callers/tests to handle the narrower checked exception.
| try { | ||
| return FirebaseMessaging.getInstance().send(mb.build()); | ||
| } catch (FirebaseMessagingException e) { | ||
| log.warn("FCM single send failed token={}, code={}", token, e.getMessagingErrorCode(), e); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
Mask tokens in logs (PII/sensitive).
Avoid logging raw FCM tokens.
Apply:
- log.warn("FCM single send failed token={}, code={}", token, e.getMessagingErrorCode(), e);
+ log.warn("FCM single send failed token={}, code={}", maskToken(token), e.getMessagingErrorCode(), e);
@@
- log.warn("FCM multicast failed token={}, code={}", t, code, fme);
+ log.warn("FCM multicast failed token={}, code={}", maskToken(t), code, fme);
@@
- log.warn("FCM multicast failed token={} (non-Firebase exception)", t, ex);
+ log.warn("FCM multicast failed token={} (non-Firebase exception)", maskToken(t), ex);Add helper inside the class:
private String maskToken(String t) {
if (t == null) return "null";
int n = t.length();
if (n <= 10) return "****" + t;
return t.substring(0, 4) + "..." + t.substring(n - 4);
}Also applies to: 72-82
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/domain/pushToken/FcmSender.java around lines
27-33 (and also apply same change to lines 72-82), the code logs raw FCM tokens
which is sensitive; add a private helper method maskToken(String t) that returns
"null" for null, "****"+t for length <=10, otherwise first 4 chars + "..." +
last 4 chars, then replace occurrences of token in log calls with
maskToken(token) (and any other token variables in those log lines) so logs do
not contain the full token.
| if (tokens == null || tokens.isEmpty()) { | ||
| return new BatchResult(0, 0, List.of()); | ||
| } | ||
|
|
||
| int totalSuccess = 0; | ||
| int totalFailure = 0; | ||
|
|
||
| List<String> permanentFailed = new ArrayList<>(); | ||
|
|
||
| for (int start = 0; start < tokens.size(); start += 500) { | ||
| List<String> chunk = tokens.subList(start, Math.min(start + 500, tokens.size())); | ||
|
|
There was a problem hiding this comment.
Filter null/blank tokens before chunking to avoid runtime errors.
addAllTokens with null/blank entries can fail.
- if (tokens == null || tokens.isEmpty()) {
+ if (tokens == null || tokens.isEmpty()) {
return new BatchResult(0, 0, List.of());
}
+ // filter invalid entries; keep order and duplicates if desired
+ tokens = tokens.stream()
+ .filter(t -> t != null && !t.isBlank())
+ .toList();
+ if (tokens.isEmpty()) {
+ return new BatchResult(0, 0, List.of());
+ }
+
int totalSuccess = 0;
int totalFailure = 0;
List<String> permanentFailed = new ArrayList<>();
- for (int start = 0; start < tokens.size(); start += 500) {
+ for (int start = 0; start < tokens.size(); start += 500) {
List<String> chunk = tokens.subList(start, Math.min(start + 500, tokens.size()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (tokens == null || tokens.isEmpty()) { | |
| return new BatchResult(0, 0, List.of()); | |
| } | |
| int totalSuccess = 0; | |
| int totalFailure = 0; | |
| List<String> permanentFailed = new ArrayList<>(); | |
| for (int start = 0; start < tokens.size(); start += 500) { | |
| List<String> chunk = tokens.subList(start, Math.min(start + 500, tokens.size())); | |
| if (tokens == null || tokens.isEmpty()) { | |
| return new BatchResult(0, 0, List.of()); | |
| } | |
| // filter invalid entries; keep order and duplicates if desired | |
| tokens = tokens.stream() | |
| .filter(t -> t != null && !t.isBlank()) | |
| .toList(); | |
| if (tokens.isEmpty()) { | |
| return new BatchResult(0, 0, List.of()); | |
| } | |
| int totalSuccess = 0; | |
| int totalFailure = 0; | |
| List<String> permanentFailed = new ArrayList<>(); | |
| for (int start = 0; start < tokens.size(); start += 500) { | |
| List<String> chunk = tokens.subList(start, Math.min(start + 500, tokens.size())); |
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/domain/pushToken/FcmSender.java around lines
41 to 52, filter the incoming tokens list to remove nulls and
blank/whitespace-only strings before doing the 500-size chunking to prevent
runtime errors; create a new filtered list (e.g., trim and exclude null/empty
values), replace references to the original tokens with the filtered list, and
update the early-return to check the filtered list is empty so the chunk loop
never processes invalid entries.
| public PushToken upsert(PushTokenReqDTO.RegisterPushTokenReqDTO request, Long userId) { | ||
| PushToken entity = pushTokenRepository.findByToken(request.token()) | ||
| // 이미 존재하는 경우 -> update | ||
| .map(exist -> { | ||
| exist.setUserId(userId); | ||
| exist.setPlatform(request.platform()); | ||
| exist.setDeviceId(request.deviceId()); | ||
| exist.setIsActive(true); | ||
| exist.setLastSeenAt(LocalDateTime.now()); | ||
| return exist; | ||
| }) | ||
| // 없는 경우 -> insert | ||
| .orElseGet(() -> PushToken.builder() | ||
| .userId(userId) | ||
| .token(request.token()) | ||
| .platform(request.platform()) | ||
| .deviceId(request.deviceId()) | ||
| .isActive(true) | ||
| .lastSeenAt(LocalDateTime.now()) | ||
| .build() | ||
| ); | ||
|
|
||
| return pushTokenRepository.save(entity); | ||
| } |
There was a problem hiding this comment.
Prevent silent rebind across users.
Current upsert reassigns an existing token to a different user without checks. Enforce ownership or same deviceId.
+import java.util.Objects;
@@
PushToken entity = pushTokenRepository.findByToken(request.token())
// 이미 존재하는 경우 -> update
.map(exist -> {
+ if (exist.getUserId() != null
+ && !exist.getUserId().equals(userId)
+ && !Objects.equals(exist.getDeviceId(), request.deviceId())) {
+ throw new CustomException(PushTokenErrorCode.TOKEN_NOT_OWNER);
+ }
exist.setUserId(userId);
exist.setPlatform(request.platform());
exist.setDeviceId(request.deviceId());
exist.setIsActive(true);
exist.setLastSeenAt(LocalDateTime.now());
return exist;
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public PushToken upsert(PushTokenReqDTO.RegisterPushTokenReqDTO request, Long userId) { | |
| PushToken entity = pushTokenRepository.findByToken(request.token()) | |
| // 이미 존재하는 경우 -> update | |
| .map(exist -> { | |
| exist.setUserId(userId); | |
| exist.setPlatform(request.platform()); | |
| exist.setDeviceId(request.deviceId()); | |
| exist.setIsActive(true); | |
| exist.setLastSeenAt(LocalDateTime.now()); | |
| return exist; | |
| }) | |
| // 없는 경우 -> insert | |
| .orElseGet(() -> PushToken.builder() | |
| .userId(userId) | |
| .token(request.token()) | |
| .platform(request.platform()) | |
| .deviceId(request.deviceId()) | |
| .isActive(true) | |
| .lastSeenAt(LocalDateTime.now()) | |
| .build() | |
| ); | |
| return pushTokenRepository.save(entity); | |
| } | |
| import java.util.Objects; | |
| public PushToken upsert(PushTokenReqDTO.RegisterPushTokenReqDTO request, Long userId) { | |
| PushToken entity = pushTokenRepository.findByToken(request.token()) | |
| // 이미 존재하는 경우 -> update | |
| .map(exist -> { | |
| if (exist.getUserId() != null | |
| && !exist.getUserId().equals(userId) | |
| && !Objects.equals(exist.getDeviceId(), request.deviceId())) { | |
| throw new CustomException(PushTokenErrorCode.TOKEN_NOT_OWNER); | |
| } | |
| exist.setUserId(userId); | |
| exist.setPlatform(request.platform()); | |
| exist.setDeviceId(request.deviceId()); | |
| exist.setIsActive(true); | |
| exist.setLastSeenAt(LocalDateTime.now()); | |
| return exist; | |
| }) | |
| // 없는 경우 -> insert | |
| .orElseGet(() -> PushToken.builder() | |
| .userId(userId) | |
| .token(request.token()) | |
| .platform(request.platform()) | |
| .deviceId(request.deviceId()) | |
| .isActive(true) | |
| .lastSeenAt(LocalDateTime.now()) | |
| .build() | |
| ); | |
| return pushTokenRepository.save(entity); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/domain/pushToken/service/PushTokenService.java
around lines 21 to 44, the upsert currently reassigns an existing token to any
provided userId; change the update path to prevent silent rebind: when
pushTokenRepository.findByToken(...) returns a value and its userId differs from
the incoming userId, only allow the update if the existing deviceId equals
request.deviceId (treat as same device) otherwise throw a suitable exception
(e.g., IllegalStateException or a domain-specific exception) or return a
validation error; if userIds match (or deviceIds match), proceed to update
platform/isActive/lastSeenAt as now; implement this logic inside the map(...)
before mutating and ensure the repository.save(entity) still occurs for allowed
updates.
| // FCM 토큰을 서버에서 해제 | ||
| @Transactional | ||
| public void unbind(String token, boolean deactivate) { | ||
| PushToken pt = pushTokenRepository.findByToken(token) | ||
| .orElseThrow(() -> new CustomException(PushTokenErrorCode.TOKEN_NOT_FOUND)); | ||
|
|
||
| pt.setUserId(null); | ||
|
|
||
| if (deactivate) pt.setIsActive(false); | ||
|
|
||
| pt.setLastSeenAt(LocalDateTime.now()); | ||
| } |
There was a problem hiding this comment.
Unbind is unauthenticated/ownerless → anyone can unbind any token.
Require the requester to be the owner (or design a secure device-proof). Update signature to include userId and check.
- public void unbind(String token, boolean deactivate) {
+ public void unbind(String token, boolean deactivate, Long userId) {
PushToken pt = pushTokenRepository.findByToken(token)
.orElseThrow(() -> new CustomException(PushTokenErrorCode.TOKEN_NOT_FOUND));
+ if (pt.getUserId() == null || !pt.getUserId().equals(userId)) {
+ throw new CustomException(PushTokenErrorCode.TOKEN_NOT_OWNER);
+ }
pt.setUserId(null);
if (deactivate) pt.setIsActive(false);
pt.setLastSeenAt(LocalDateTime.now());
}Follow-up: wire userId from controller (see suggested diff there).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // FCM 토큰을 서버에서 해제 | |
| @Transactional | |
| public void unbind(String token, boolean deactivate) { | |
| PushToken pt = pushTokenRepository.findByToken(token) | |
| .orElseThrow(() -> new CustomException(PushTokenErrorCode.TOKEN_NOT_FOUND)); | |
| pt.setUserId(null); | |
| if (deactivate) pt.setIsActive(false); | |
| pt.setLastSeenAt(LocalDateTime.now()); | |
| } | |
| // FCM 토큰을 서버에서 해제 | |
| @Transactional | |
| public void unbind(String token, boolean deactivate, Long userId) { | |
| PushToken pt = pushTokenRepository.findByToken(token) | |
| .orElseThrow(() -> new CustomException(PushTokenErrorCode.TOKEN_NOT_FOUND)); | |
| if (pt.getUserId() == null || !pt.getUserId().equals(userId)) { | |
| throw new CustomException(PushTokenErrorCode.TOKEN_NOT_OWNER); | |
| } | |
| pt.setUserId(null); | |
| if (deactivate) pt.setIsActive(false); | |
| pt.setLastSeenAt(LocalDateTime.now()); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/domain/pushToken/service/PushTokenService.java
around lines 46 to 57, the unbind method currently allows
unauthenticated/ownerless unbinding; change the signature to accept a userId
parameter and enforce ownership by verifying that the found PushToken's userId
matches the supplied userId (throw CustomException with an appropriate error
code if not), then proceed to setUserId(null), optionally deactivate, and update
lastSeenAt; also ensure callers (controller) are updated to pass the
authenticated user's id.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/java/naughty/tuzamate/domain/notification/service/NotificationService.java (4)
38-78: Avoid blocking request thread on after-commit FCM sendafterCommit runs on the caller thread; network I/O and token deactivation here will extend request latency and couple UX to FCM. Offload to an executor or use an event listener with @async.
Minimal change (offload the whole block):
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { @Override public void afterCommit() { - List<String> tokens = pushTokenRepository.findActiveTokensByUserId(receiverId); - if (tokens.isEmpty()) { - log.info("No active tokens for receiverId={}", receiverId); - return; - } - - try { - FcmSender.BatchResult br = fcmSender.sendToTokens( - tokens, title, body, data, highPriority, clickAction - ); - if (!br.failedTokens().isEmpty()) { - deactivateTokensRequiresNew(br.failedTokens()); - log.info("Deactivated {} tokens (receiverId={})", - br.failedTokens().size(), receiverId); - } - log.info("FCM sent: success={}, failure={}, receiverId={}", - br.success(), br.failure(), receiverId); - } catch (Exception e) { - log.warn("FCM send failed afterCommit. receiverId={}", receiverId, e); - } + sendExecutor.execute(() -> { + List<String> tokens = pushTokenRepository.findActiveTokensByUserId(receiverId); + if (tokens.isEmpty()) { + log.info("No active tokens for receiverId={}", receiverId); + return; + } + try { + FcmSender.BatchResult br = fcmSender.sendToTokens( + tokens, title, body, data, highPriority, clickAction + ); + if (!br.failedTokens().isEmpty()) { + deactivateTokensRequiresNew(br.failedTokens()); + log.info("Deactivated {} tokens (receiverId={})", + br.failedTokens().size(), receiverId); + } + log.info("FCM sent: success={}, failure={}, receiverId={}", + br.success(), br.failure(), receiverId); + } catch (Exception e) { + log.warn("FCM send failed afterCommit. receiverId={}", receiverId, e); + } + }); } });Additional code outside this hunk:
// field private final org.springframework.core.task.TaskExecutor sendExecutor;
44-46: Null-safe data payload before sendGuard against null data to avoid NPEs in the sender or serialization.
// 알림 저장 notificationRepository.save(notification); + // FCM data payload null-guard + data = (data == null) ? Map.of() : data;
80-100: Programmatic REQUIRES_NEW is fine; consider TransactionTemplate for brevityCurrent txManager usage is correct and avoids self-invocation, but TransactionTemplate improves readability and ensures proper cleanup.
Example outside this hunk:
private final org.springframework.transaction.support.TransactionTemplate txTemplate; private void deactivateTokensRequiresNew(List<String> tokens) { if (tokens == null || tokens.isEmpty()) return; List<String> distinct = tokens.stream().distinct().collect(Collectors.toList()); txTemplate.execute(status -> { pushTokenRepository.bulkDeactivateByTokens(distinct); return null; }); }
114-121: Reuse converter to remove duplicate mapping logicPrefer the existing NotificationConverter to keep mapping in one place.
- List<NotificationResDTO.toNotificationResDTO> list = result.stream() - .map(n -> NotificationResDTO.toNotificationResDTO.builder() - .title(n.getTitle()) - .content(n.getContent()) - .isRead(n.isRead()) - .targetId(n.getTargetId()) - .build()) - .toList(); + List<NotificationResDTO.toNotificationResDTO> list = result.stream() + .map(NotificationConverter::toNotificationDTO) + .toList();src/main/java/naughty/tuzamate/domain/notification/entity/Notification.java (1)
32-34: Stray semicolonDouble semicolon after field initializer; harmless but noisy.
- @Builder.Default - private boolean isRead = false;; + @Builder.Default + private boolean isRead = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/naughty/tuzamate/domain/notification/entity/Notification.java(2 hunks)src/main/java/naughty/tuzamate/domain/notification/service/NotificationService.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/naughty/tuzamate/domain/notification/service/NotificationService.java (3)
src/main/java/naughty/tuzamate/domain/notification/converter/NotificationConverter.java (1)
NotificationConverter(6-17)src/main/java/naughty/tuzamate/domain/notification/dto/NotificationResDTO.java (1)
NotificationResDTO(8-24)src/main/java/naughty/tuzamate/domain/comment/service/command/CommentCommandServiceImpl.java (1)
Service(27-130)
🔇 Additional comments (4)
src/main/java/naughty/tuzamate/domain/notification/service/NotificationService.java (2)
146-146: LGTM: state change API is clearerReplacing toggle-style update with explicit notification.markAsRead() improves readability and intent.
58-67: Verified — FcmSender already enforces FCM per-request limits and only returns permanent failuressendToTokens() chunks tokens in 500-sized sublists and only collects tokens flagged by isPermanentFailure(...) into the BatchResult, so deactivateTokensRequiresNew(...) will only get permanent failures (see src/main/java/naughty/tuzamate/domain/pushToken/FcmSender.java, loop around lines ~50–79).
src/main/java/naughty/tuzamate/domain/notification/entity/Notification.java (2)
17-19: Index name suggests DESC but JPA @Index can’t declare orderThe name includes “_desc” but columnList lacks ordering. Verify desired query uses ORDER BY receiver_id, id DESC and that your DB can use the index in reverse order; otherwise create it via a migration.
42-45: LGTM: explicit read/unread mutatorsClearer than a toggle; avoids accidental double-toggles.
| @Column(nullable = false) | ||
| private String title; | ||
|
|
||
| @NotBlank | ||
| @Column(nullable = false) | ||
| private String content; |
There was a problem hiding this comment.
Regression risk: @notblank removed; DB-only null check allows empty strings
@column(nullable = false) prevents null but permits "". If non-empty title/content are required, reintroduce Bean Validation (and keep DB nullability).
- @Column(nullable = false)
- private String title;
+ @NotBlank
+ @Column(nullable = false)
+ private String title;
- @Column(nullable = false)
- private String content;
+ @NotBlank
+ @Column(nullable = false)
+ private String content;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Column(nullable = false) | |
| private String title; | |
| @NotBlank | |
| @Column(nullable = false) | |
| private String content; | |
| @NotBlank | |
| @Column(nullable = false) | |
| private String title; | |
| @NotBlank | |
| @Column(nullable = false) | |
| private String content; |
🤖 Prompt for AI Agents
In src/main/java/naughty/tuzamate/domain/notification/entity/Notification.java
around lines 26 to 30, the fields title and content only have @Column(nullable =
false) which prevents nulls but allows empty strings; reintroduce Bean
Validation by annotating both fields with @NotBlank (from
javax.validation.constraints or jakarta.validation.constraints depending on
project), keep the existing nullable = false on the @Column, and ensure the
entity is validated on persist/update (e.g., via @Valid at service/controller
boundaries or enabling JPA validation) so empty strings are rejected before
hitting the DB.
📍 PR 타입 (하나 이상 선택)
🏷️ 관련 이슈
Close #94
📌 개요
🔁 변경 사항
📸 스크린샷
✅ 체크리스트
💡 추가 사항 (리뷰어가 참고해야 할 것)
Summary by CodeRabbit
New Features
Refactor