[Refactor&Feature] FCM Sender & Notification Domain #70
Conversation
๐ WalkthroughWalkthroughThis PR introduces a notification storage system with cursor-based pagination, enhances FCM push notifications with destination metadata propagation, refactors transaction management into a centralized Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant NotificationService
participant NotificationStoredAppender
participant NotificationStoredRepository
participant NotificationStoredCoreRepository
participant Tx
participant Database as NotificationStoredEntity
Client->>NotificationService: appendAll(List<CreateNotificationStoredCommand>)
NotificationService->>NotificationService: Generate key for each command
NotificationService->>NotificationService: Map to NotificationStored.Create
NotificationService->>NotificationStoredAppender: appendAll(List<Create>)
NotificationStoredAppender->>NotificationStoredRepository: saveAll(List<Create>)
NotificationStoredRepository->>NotificationStoredCoreRepository: saveAll(List<Create>)
NotificationStoredCoreRepository->>Tx: writeable { ... }
Tx->>Database: Persist NotificationStoredEntity records
Database-->>Tx: Entities saved
Tx-->>NotificationStoredCoreRepository: Success
NotificationStoredCoreRepository-->>NotificationStoredRepository: List<NotificationStored.Info>
NotificationStoredRepository-->>NotificationStoredAppender: List<NotificationStored.Info>
NotificationStoredAppender-->>NotificationService: List<NotificationStored.Info>
NotificationService-->>Client: List<NotificationStored.Info>
rect rgb(230, 240, 255)
Note over Client,Database: Cursor-based retrieval flow
Client->>NotificationService: findAllNotifications(userId, CursorRequest)
NotificationService->>NotificationStoredReader: findAllBy(userId, CursorRequest)
NotificationStoredReader->>NotificationStoredRepository: findAllBy(userId, CursorRequest)
NotificationStoredRepository->>NotificationStoredCoreRepository: findAllBy(userId, CursorRequest)
NotificationStoredCoreRepository->>Tx: readable { ... }
Tx->>Database: Query with cursor pagination (excludes REGULAR type)
Database-->>Tx: Cursor<NotificationStored.Info>
Tx-->>NotificationStoredCoreRepository: Cursor<NotificationStored.Info>
NotificationStoredCoreRepository-->>NotificationStoredRepository: Cursor<NotificationStored.Info>
NotificationStoredRepository-->>NotificationStoredReader: Cursor<NotificationStored.Info>
NotificationStoredReader-->>NotificationService: Cursor<NotificationStored.Info>
NotificationService-->>Client: Cursor<NotificationStored.Info>
end
rect rgb(240, 255, 240)
Note over Client,Database: Mark as read flow
Client->>NotificationService: markAsRead(notificationId)
NotificationService->>NotificationStoredUpdater: markAsRead(notificationId)
NotificationStoredUpdater->>NotificationStoredRepository: markAsRead(notificationId)
NotificationStoredRepository->>NotificationStoredCoreRepository: markAsRead(notificationId)
NotificationStoredCoreRepository->>Tx: writeable { ... }
Tx->>Database: Update readStatus to READ
Database-->>Tx: Success
Tx-->>NotificationStoredCoreRepository: void
NotificationStoredCoreRepository-->>NotificationStoredRepository: void
NotificationStoredRepository-->>NotificationStoredUpdater: void
NotificationStoredUpdater-->>NotificationService: void
NotificationService-->>Client: void
end
Estimated code review effort๐ฏ 4 (Complex) | โฑ๏ธ ~65 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touchesโ Failed checks (1 warning)
โ Passed checks (4 passed)
โจ Finishing touches
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: 9
Caution
Some comments are outside the diff and canโt be posted inline due to platform limitations.
โ ๏ธ Outside diff range comments (1)
pida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmClientRepository.kt (1)
24-30: Critical: Batch send uses only first message's content.The multicast send on lines 26-28 uses
fcmRequests[0].title,fcmRequests[0].body, andfcmRequests[0].destinationfor all messages in the batch. If the list contains messages with different content, all recipients will receive the first message's content instead of their intended messages.๐ Recommended fix
If all messages in a batch should have identical content, validate this assumption:
val tokens = fcmRequests.map { it.fcmToken } + // Validate all messages have identical content for multicast + require(fcmRequests.all { it.title == fcmRequests[0].title && + it.body == fcmRequests[0].body && + it.destination == fcmRequests[0].destination }) { + "Multicast requires all messages to have identical content" + } val batchResult = firebaseCloudMessageSender.sendEachForMulticastAll(Or, if messages can differ, send them individually instead of as a multicast.
๐งน Nitpick comments (16)
pida-core/core-domain/src/main/kotlin/com/pida/support/tx/Tx.kt (2)
50-73: Design inconsistency: templates unused by synchronous methods.The
writeTemplate,requiresNewTemplate, andreadOnlyTemplateare created but only used by the coroutine methods (coWrite,coRequiresNew,coReadOnly). The synchronous methods (write,requiresNew,readOnly) rely solely on@Transactionalannotations.This works, but the two approaches could diverge in behavior (e.g., isolation level, timeout settings). Consider unifying:
- Either use
TransactionTemplate.execute()for synchronous methods too, or- Document why the dual approach is intentional.
13-23: The thread safety concern is overstated; however, there is a code duplication issue requiring attention.The
lateinit var txAdvicepattern is not as risky as initially suggested. Spring creates a singleTxbean (singleton by default), so the init block executes once, after whichtxAdviceis only read. The actual risk is minimal: a thread callingTx.writeable()before the Spring context fully initializes could encounterUninitializedPropertyAccessException, but since these methods are called from repository beans (which won't execute until the context is ready), this is unlikely in practice.However, there is a significant design issue:
TxAdviceis defined in two placesโboth as a nested class inTx.ktand as a standalone@ComponentinTxAdvice.kt. These are identical and will both be instantiated by Spring, creating redundancy and potential confusion. Remove one of these definitions to eliminate duplication.The
coExecute()extension is properly defined inTransactionTemplates.ktand the templates are used in the coroutine methods, so those earlier concerns do not apply.pida-core/core-domain/src/main/kotlin/com/pida/notification/FcmToUserCommand.kt (1)
3-7: Consider consolidating withNewFcmToUseror clarifying the distinction.
FcmToUserCommandappears to be a duplicate ofNewFcmToUser(both haveuserKey,title,bodyfields). If these serve different purposes in the message flow, consider renaming them to clarify their roles (e.g.,FcmUserMessageCommandvsFcmUserMessageDto). Otherwise, consolidate to avoid duplication.pida-core/core-domain/src/main/kotlin/com/pida/support/cursor/CursorRequest.kt (1)
5-5: Consider usingIntforsizeinstead ofLong.Since
sizeis constrained to 1..100, usingLongis unnecessary.Intis more appropriate for count-based pagination parameters and aligns with typical JVM collection APIs.๐ Proposed refactor
data class CursorRequest( val lastId: Long?, - val size: Long, + val size: Int, ) { init { require(size in 1..100) { "ํฌ๊ธฐ๋ 1 ~ 100 ์ฌ์ด์ฌ์ผ ํฉ๋๋ค." } } }pida-core/core-domain/src/main/kotlin/com/pida/support/page/PageResponse.kt (1)
8-14: The factory method adds no value over the constructor.The
of()method simply delegates to the constructor without validation or transformation. While this may provide consistency withPage.of()andCursor.of()(which do validate), direct constructor usage with default parameters is clearer when no validation is needed.pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationType.kt (1)
8-15: Simplify using Kotlin's built-in enum utilities.The manual
whenexpression duplicatesvalueOf()functionality and requires updates when adding new enum values. Consider usingvalueOf()orentries.find().๐ Proposed refactor options
Option 1: Use
valueOf()with try-catchfun of(type: String): NotificationType = - when (type) { - "ADMIN_PUSH" -> ADMIN_PUSH - "REGULAR" -> REGULAR - else -> throw IllegalArgumentException("Unknown NotificationType: $type") - } + try { + valueOf(type) + } catch (e: IllegalArgumentException) { + throw IllegalArgumentException("Unknown NotificationType: $type", e) + }Option 2: Use
entries.find()for custom error handlingfun of(type: String): NotificationType = - when (type) { - "ADMIN_PUSH" -> ADMIN_PUSH - "REGULAR" -> REGULAR - else -> throw IllegalArgumentException("Unknown NotificationType: $type") - } + entries.find { it.name == type } + ?: throw IllegalArgumentException("Unknown NotificationType: $type")pida-core/core-domain/src/main/kotlin/com/pida/support/page/SortType.kt (1)
3-7: Clarify sort direction in displayName.The enum entry
NEW("id")doesn't clearly indicate the sort direction. Consider using a more descriptive displayName like"id_desc"or"newest"to clarify that this represents newest-first (descending ID) ordering.pida-core/core-domain/src/main/kotlin/com/pida/support/page/OffsetLimit.kt (1)
14-21: Consider removing the redundant factory method.The
of()factory method duplicates the primary constructor exactlyโsame parameters, same defaults, same behavior. In Kotlin, named/default arguments make such companion factories unnecessary unless they provide additional logic (e.g., caching, validation, or alternative construction paths).pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStored.kt (2)
3-39: Nested classes do not extend the sealed class.The data classes
Create,Info, andResultare nested insideNotificationStoredbut don't extend it. This meansNotificationStoredas a sealed class provides no polymorphic benefitsโyou cannot usewhenexhaustiveness checks or treat these as subtypes. If polymorphism isn't needed, consider using a simple object or companion object as a namespace instead.If you intend to leverage sealed class benefits:
sealed class NotificationStored { - data class Create( + data class Create(...) : NotificationStored()Otherwise, a namespace object suffices:
object NotificationStored { data class Create(...) data class Info(...) data class Result(...) }
23-23: Consider using a temporal type forcreatedAt.Using
StringforcreatedAtloses type safety and requires parsing at usage sites. If this is a domain model, considerjava.time.InstantorLocalDateTime. If it's intentionally a formatted string for API responses, this is acceptable but worth documenting.Also applies to: 37-37
pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredAppender.kt (1)
11-11: Add explicit return type for consistency.
append()explicitly declares: NotificationStored.Info, butappendAll()relies on type inference. For public API consistency and clarity, consider adding the explicit return type.๐ Proposed fix
- fun appendAll(createAll: List<NotificationStored.Create>) = notificationStoredRepository.saveAll(createAll) + fun appendAll(createAll: List<NotificationStored.Create>): List<NotificationStored.Info> = notificationStoredRepository.saveAll(createAll)pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredKeyGenerator.kt (1)
18-18: Consider using explicit timezone for consistency.
LocalDate.now()uses the system default timezone, which can vary across servers or environments. For consistent key generation in distributed systems, consider using an explicit timezone:๐ Proposed fix
- private fun generateDate(): String = FORMAT_YYYYMMDD.format(LocalDate.now()) + private fun generateDate(): String = FORMAT_YYYYMMDD.format(LocalDate.now(java.time.ZoneOffset.UTC))pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredRepository.kt (1)
6-29: Consider adding KDoc comments to the repository interface.The repository interface lacks documentation for its public methods. Adding KDoc comments would clarify:
- Expected behavior when entities are not found (e.g.,
findById)- The semantics of
countByUserIdAndReadStatuswhenreadStatusis null- Whether
markAsReadis idempotent and what happens if the notification doesn't existpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredEntity.kt (2)
16-23: Specify explicit column lengths for String fields.The String fields
notificationStoredKey,parameterValue,topic, andcontentslack explicit@Column(length = ...)annotations. This can lead to:
- Database portability issues (different defaults across DB vendors)
- Potential VARCHAR(255) truncation for longer content
- Unclear schema intent for other developers
Consider adding explicit length constraints based on expected data sizes, or use
@Lobfor large text fields likecontents.๐ Example with explicit lengths
+ @Column(length = 100) val notificationStoredKey: String, val userId: Long, @Enumerated(value = EnumType.STRING) @Column(length = 30) val type: NotificationType, + @Column(length = 500) val parameterValue: String, + @Column(length = 100) val topic: String, + @Lob val contents: String,Adjust lengths based on actual requirements.
48-48: Consider using a consistent date format for createdAt.Line 48 converts
createdAtusing.toString(), which produces JVM-default formatting (e.g.,2025-12-28T14:04:40.123). For API consistency, consider:
- Using ISO-8601 format explicitly (
DateTimeFormatter.ISO_DATE_TIME)- Converting to a specific timezone (UTC)
- Keeping it as a structured type rather than String
This ensures predictable serialization across different JVM locales and timezones.
pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationService.kt (1)
21-34: Consider batch key generation for better performance.Lines 23-33 call
notificationStoredKeyGenerator.generate()inside amapoperation, generating keys one-by-one for bulk operations. If the key generator has any I/O, synchronization, or expensive operations, this could be inefficient for large batches.Consider adding a batch key generation method to
NotificationStoredKeyGenerator:interface NotificationStoredKeyGenerator { fun generate(): String fun generateBatch(count: Int): List<String> = List(count) { generate() } }Then use it:
fun appendAll(createAll: List<CreateNotificationStoredCommand>): List<NotificationStored.Info> { val keys = notificationStoredKeyGenerator.generateBatch(createAll.size) return notificationStoredAppender.appendAll( createAll.mapIndexed { index, notification -> NotificationStored.Create( notificationStoredKey = keys[index], // ... rest of fields ) } ) }This is a minor optimization and may not be necessary if key generation is already lightweight.
๐ Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (32)
pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/s3/AwsS3Client.ktpida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmClientRepository.ktpida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmSendRequest.ktpida-clients/notification/src/main/kotlin/com/pida/client/notification/FirebaseCloudMessageSender.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/CreateNotificationStoredCommand.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/FcmSender.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/FcmToUserCommand.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/FirebaseCloudMessage.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/FirebaseCloudMessageCommand.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NewFirebaseCloudMessage.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationService.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStored.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredAppender.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredKeyGenerator.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredReader.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredRepository.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredUpdater.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationType.ktpida-core/core-domain/src/main/kotlin/com/pida/notification/ReadStatus.ktpida-core/core-domain/src/main/kotlin/com/pida/support/cursor/Cursor.ktpida-core/core-domain/src/main/kotlin/com/pida/support/cursor/CursorRequest.ktpida-core/core-domain/src/main/kotlin/com/pida/support/page/OffsetLimit.ktpida-core/core-domain/src/main/kotlin/com/pida/support/page/OffsetPageRequest.ktpida-core/core-domain/src/main/kotlin/com/pida/support/page/Page.ktpida-core/core-domain/src/main/kotlin/com/pida/support/page/PageResponse.ktpida-core/core-domain/src/main/kotlin/com/pida/support/page/Sort.ktpida-core/core-domain/src/main/kotlin/com/pida/support/page/SortType.ktpida-core/core-domain/src/main/kotlin/com/pida/support/tx/Tx.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCoreRepository.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredEntity.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredJpaRepository.kt
๐งฐ Additional context used
๐งฌ Code graph analysis (2)
pida-core/core-domain/src/main/kotlin/com/pida/notification/NewFirebaseCloudMessage.kt (4)
pida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmSendResponse.kt (1)
fcmToken(3-8)pida-core/core-domain/src/main/kotlin/com/pida/notification/NewFcmToUser.kt (1)
userKey(3-7)pida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmProperties.kt (1)
prefix(5-8)pida-core/core-domain/src/main/kotlin/com/pida/notification/FcmToUserKey.kt (1)
key(3-5)
pida-core/core-domain/src/main/kotlin/com/pida/notification/FirebaseCloudMessage.kt (4)
pida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmProperties.kt (1)
prefix(5-8)pida-core/core-domain/src/main/kotlin/com/pida/notification/NewFcmToUser.kt (1)
userKey(3-7)pida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmSendResponse.kt (1)
fcmToken(3-8)pida-core/core-domain/src/main/kotlin/com/pida/notification/FcmToUserKey.kt (1)
key(3-5)
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (21, 2.1.0, corretto)
๐ Additional comments (26)
pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/s3/AwsS3Client.kt (1)
5-9: LGTM! Explicit imports improve code clarity.Replacing the wildcard import with explicit imports is a good practice that makes dependencies clearer and reduces namespace pollution. All imported classes are properly used throughout the file.
Minor note: This change appears unrelated to the PR's FCM notification improvements. If this was an incidental cleanup or IDE auto-fix, consider grouping unrelated refactors separately in future PRs for clearer change tracking.
pida-core/core-domain/src/main/kotlin/com/pida/support/page/OffsetPageRequest.kt (1)
3-11: LGTM! Well-structured pagination request object.The data class design is clean and follows good practices:
- Appropriate validation bounds (page >= 0, size 1..100)
- Fail-fast validation in the init block
- Consistent with the existing
CursorRequestpatternpida-core/core-domain/src/main/kotlin/com/pida/support/tx/Tx.kt (4)
24-44: LGTM on the companion object API.The static-style helper methods provide a clean, ergonomic API for transaction management. Defaulting coroutine variants to
Dispatchers.IOis appropriate for blocking JDBC calls.
60-64:PROPAGATION_SUPPORTSmay not provide transactional guarantees.With
PROPAGATION_SUPPORTS, if no outer transaction exists, the block runs non-transactionally. This means:
- No snapshot isolation or repeatable reads
- No connection reuse within the block
If consistent reads are required, consider using
PROPAGATION_REQUIREDwithisReadOnly = trueinstead.
66-73: LGTM on@Transactionalmethod design.The synchronous transactional methods are called through the injected
TxAdvicebean reference, ensuring Spring's AOP proxy intercepts the calls correctly.
75-88: TheTransactionTemplate.coExecuteextension function is properly defined inpida-core/core-domain/src/main/kotlin/com/pida/support/tx/TransactionTemplates.kt(lines 35-41) and is widely used throughout the codebase. The code will compile without issues.pida-core/core-domain/src/main/kotlin/com/pida/support/page/Sort.kt (1)
3-6: LGTM!Standard sorting direction enum with clear, conventional naming.
pida-core/core-domain/src/main/kotlin/com/pida/notification/CreateNotificationStoredCommand.kt (1)
3-11: LGTM!The command object structure is clean and the default value
ReadStatus.UNREADfor new notifications is appropriate.pida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmSendRequest.kt (1)
7-7: LGTM!The destination field addition enables propagation of destination information through FCM flows.
pida-core/core-domain/src/main/kotlin/com/pida/notification/FcmSender.kt (1)
25-25: LGTM!The destination field is correctly propagated during the mapping from
NewFirebaseCloudMessagetoFirebaseCloudMessage.pida-core/core-domain/src/main/kotlin/com/pida/notification/ReadStatus.kt (1)
3-16: LGTM!The enum structure and factory method follow best practices with proper error handling for invalid input.
pida-clients/notification/src/main/kotlin/com/pida/client/notification/FcmClientRepository.kt (3)
39-39: LGTM!The destination field is correctly propagated in the result construction for batch sends.
53-53: LGTM!The destination field is correctly included in the single-send request construction.
61-61: LGTM!The destination field is correctly propagated in the result construction for single sends.
pida-core/core-domain/src/main/kotlin/com/pida/notification/NewFirebaseCloudMessage.kt (1)
7-7: LGTM!The destination field addition aligns with the broader refactoring to propagate destination information throughout the FCM messaging flow.
pida-clients/notification/src/main/kotlin/com/pida/client/notification/FirebaseCloudMessageSender.kt (2)
39-45: Verify intentional platform inconsistency in notification display.The
Notificationbuilder sets bothtitleandbody(Lines 32-33), which Android will display. However, the APNS alert is set to onlyrequest.body(Line 42), meaning iOS users see only the body text without the title. This creates an inconsistent cross-platform experience.If this is intentional (e.g., iOS design preference), consider adding a comment. Otherwise, restore the title for iOS:
๐ Potential fix to include title in iOS alert
.setAps( Aps .builder() - .setAlert(request.body) + .setAlert("${request.title}\n${request.body}") .setBadge(1)
70-76: Same platform inconsistency applies to multicast messages.The multicast path also sets APNS alert to only
body(Line 73) while theNotificationincludes both title and body. Ensure this aligns with the intended UX for iOS.pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredUpdater.kt (1)
9-9: Verify authorization is handled upstream.
markAsReadaccepts onlynotificationIdwithoutuserId, which means any notification can be marked as read if the ID is known. Ensure the calling service layer validates that the requesting user owns the notification before invoking this method.pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredReader.kt (1)
7-15: LGTM!Clean delegation to the repository with explicit types. Follows the established pattern of the other storage components.
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredJpaRepository.kt (1)
15-18: This method appears to be unusedโno callers found in the codebase.The method
countByUserIdAndReadStatusis defined across the repository layers but has no invocations. If this is dead code, consider removing it. If it's intended for future use, document the purpose and expected behavior (particularly regarding how nullreadStatusshould be handled in the derived query).Note: There's also an inconsistency with
findByUserIdAndReadStatus, which uses non-nullablereadStatus: ReadStatus, whilecountByUserIdAndReadStatusacceptsReadStatus?. If both methods are retained, clarify the contractโespecially whether null means "count all statuses" or "count only IS NULL rows" in Spring Data's derived query interpretation.pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationStoredRepository.kt (1)
28-28: LGTM! Idempotent markAsRead design is appropriate.The
markAsReadmethod returns Unit, which is acceptable given the implementation inNotificationStoredCoreRepositoryshows it's idempotent (early returns if already read). This design simplifies callers who don't need to handle the already-read case.pida-core/core-domain/src/main/kotlin/com/pida/notification/NotificationService.kt (2)
14-19: Verify key overwrite behavior in append method.Line 16-17 unconditionally overwrites the
notificationStoredKeyfield using.copy(), even if the caller already set one. This could surprise callers who expect their key to be preserved.If this is intentional (enforcing that all keys are server-generated), consider:
- Documenting this behavior in method KDoc
- Or, alternatively, only generate a key if not already present
Current design enforces central key generation, which is generally good for consistency. Confirm this aligns with the intended contract.
36-42: LGTM! Clean delegation pattern.The service properly delegates to specialized reader and updater components, maintaining clear separation of concerns. The delegation pattern is appropriate for this orchestration layer.
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCoreRepository.kt (2)
67-78: LGTM! Excellent idempotency optimization.The
markAsReadimplementation includes an early-return guard (lines 73-75) that prevents unnecessary write operations when the notification is already marked as read. This optimization:
- Reduces database writes
- Makes the operation naturally idempotent
- Avoids unnecessary entity version increments (if using optimistic locking)
Well done!
17-78: Transaction management is properly implemented.The
Tx.writeableandTx.readablewrappers delegate to Spring's@Transactionalannotation framework, which correctly handles:
- Transaction lifecycle:
@Transactionalopens a transaction on method entry and commits on successful return- Read-only optimization:
@ReadOnlyTransactionalmarks transactions as read-only withisReadOnly=true, allowing database optimizations- Exception handling: Spring's AOP automatically rolls back on unchecked exceptions
- Entity persistence: The
notificationStored.read()mutation at line 77 occurs within the transaction context, so JPA's dirty checking detects and persists the changes on commitpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.kt (1)
15-20: The exclusion of REGULAR notification type is intentional and appropriate.
NotificationType.REGULARrepresents scheduled/automated notifications (regular push every Tuesday at 9 AM), which are correctly excluded from the user's notification inbox query. ThefindAllBymethod retrieves user-facing notifications (ADMIN_PUSH), while automated scheduled notifications are handled separately.
| val fcmToken: String, | ||
| val title: String, | ||
| val body: String, | ||
| val destination: String, |
There was a problem hiding this comment.
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
# First, find the FirebaseCloudMessage file and examine its structure
echo "=== FirebaseCloudMessage.kt file structure ==="
fd -t f "FirebaseCloudMessage.kt" --exec wc -l {} \;
echo ""
# Read the file to understand the class definition
echo "=== FirebaseCloudMessage.kt contents ==="
fd -t f "FirebaseCloudMessage.kt" --exec cat -n {} \;
echo ""
# Search for destination field usage in message construction and validation
echo "=== Destination field usage patterns ==="
rg -nP --type=kt -C3 'destination\s*[=:]' -g '!*Test.kt' || echo "No matches found"
echo ""
# Search for empty/blank checks on destination
echo "=== Empty/blank checks on destination ==="
rg -nP --type=kt 'destination\.(isNotBlank|isNotEmpty|isEmpty|isBlank)' -g '!*Test.kt' || echo "No matches found"
echo ""
# Search for where FirebaseCloudMessage is instantiated
echo "=== FirebaseCloudMessage instantiation ==="
rg -nP --type=kt 'FirebaseCloudMessage\s*\(' -C2 -g '!*Test.kt' | head -50Repository: Team-PIDA/Pida-Server
Length of output: 1132
๐ Script executed:
#!/bin/bash
# Search for destination field usage without file type restriction
echo "=== Destination field usage in codebase ==="
rg -n 'destination' --type-list | grep -i kotlin || echo "Checking Kotlin type support..."
echo ""
# Try broader search
echo "=== All destination references ==="
rg -n '\.destination' -g '*.kt' -C2 | head -100
echo ""
# Search for FirebaseCloudMessage usage
echo "=== FirebaseCloudMessage instantiation/usage ==="
rg -n 'FirebaseCloudMessage(' -g '*.kt' -C3 | head -100
echo ""
# Search for validation in notification package
echo "=== Validation in notification package ==="
rg -n 'require\|check\|assert' pida-core/core-domain/src/main/kotlin/com/pida/notification/ | head -50
echo ""
# Check if there are any constructor validations
echo "=== Constructor or init blocks ==="
rg -n 'init\s*\{' pida-core/core-domain/src/main/kotlin/com/pida/notification/ -A5Repository: Team-PIDA/Pida-Server
Length of output: 5667
๐ Script executed:
#!/bin/bash
# Examine the FirebaseCloudMessageSender implementation
echo "=== FirebaseCloudMessageSender.kt contents ==="
cat -n pida-clients/notification/src/main/kotlin/com/pida/client/notification/FirebaseCloudMessageSender.kt
echo ""
# Look for any custom data validation in related files
echo "=== NewFirebaseCloudMessage request creation ==="
rg -n 'NewFirebaseCloudMessage' pida-clients/notification/src/main/kotlin/com/pida/client/notification/ -A3 -B1
echo ""
# Check if there are any tests that show validation expectations
echo "=== Test files for destination ==="
find . -name '*Test.kt' -o -name '*Tests.kt' | xargs grep -l 'destination' 2>/dev/null | head -10
echo ""
# Examine any validation in request/domain models
echo "=== FcmRequest or related request models ==="
rg -n 'data class.*Request' pida-clients/notification/src/main/kotlin/com/pida/client/notification/ -A5Repository: Team-PIDA/Pida-Server
Length of output: 4347
Add validation to ensure destination is not empty.
The destination field is sent directly as custom data in Firebase messages but has no validation. An empty string would be silently accepted and sent, causing routing failures on the client side. Add validation such as require(destination.isNotBlank()) in the constructor.
๐ค Prompt for AI Agents
In
pida-core/core-domain/src/main/kotlin/com/pida/notification/FirebaseCloudMessage.kt
around line 8, the destination property lacks validation and may be empty; add a
constructor check such as require(destination.isNotBlank()) (or
require(destination.isNotEmpty()) depending on style) to enforce
non-empty/non-blank values and throw an IllegalArgumentException if invalid,
ensuring the validation runs during object creation and prevents empty
destination values from being sent to Firebase.
pida-core/core-domain/src/main/kotlin/com/pida/notification/FirebaseCloudMessageCommand.kt
Outdated
Show resolved
Hide resolved
| init { | ||
| require(offset >= 0) { "offset์ 0๋ณด๋ค ์ปค์ผ ํฉ๋๋ค. " } | ||
| require(limit > 0) { "limit์ 0๋ณด๋ค ์ปค์ผ ํฉ๋๋ค. " } |
There was a problem hiding this comment.
Error message inconsistent with validation logic.
Line 10: The condition offset >= 0 allows offset to be 0, but the Korean message "offset์ 0๋ณด๋ค ์ปค์ผ ํฉ๋๋ค" translates to "offset must be greater than 0". The message should reflect that offset can be 0.
๐ Proposed fix
init {
- require(offset >= 0) { "offset์ 0๋ณด๋ค ์ปค์ผ ํฉ๋๋ค. " }
+ require(offset >= 0) { "offset์ 0 ์ด์์ด์ด์ผ ํฉ๋๋ค." }
require(limit > 0) { "limit์ 0๋ณด๋ค ์ปค์ผ ํฉ๋๋ค. " }
}๐ค Prompt for AI Agents
In pida-core/core-domain/src/main/kotlin/com/pida/support/page/OffsetLimit.kt
around lines 9 to 11, the require check uses `require(offset >= 0)` but the
error message says "offset์ 0๋ณด๋ค ์ปค์ผ ํฉ๋๋ค." (must be greater than 0); update the
message to reflect the actual validation (e.g. "offset์ 0๋ณด๋ค ํฌ๊ฑฐ๋ ๊ฐ์์ผ ํฉ๋๋ค." or
"offset์ 0 ์ด์์ด์ด์ผ ํฉ๋๋ค.") so the text matches the condition, keeping the existing
condition as-is.
| val size: Int, | ||
| ) { | ||
| init { | ||
| require(page >= 0) { "ํ์ด์ง๋ 0 ์ด์์ด์ผ ํฉ๋๋ค." } |
There was a problem hiding this comment.
Fix typo in error message.
The Korean error message contains a grammatical error: "์ด์ผ ํฉ๋๋ค" should be "์ด์ด์ผ ํฉ๋๋ค".
๐ Proposed fix
- require(page >= 0) { "ํ์ด์ง๋ 0 ์ด์์ด์ผ ํฉ๋๋ค." }
+ require(page >= 0) { "ํ์ด์ง๋ 0 ์ด์์ด์ด์ผ ํฉ๋๋ค." }๐ 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.
| require(page >= 0) { "ํ์ด์ง๋ 0 ์ด์์ด์ผ ํฉ๋๋ค." } | |
| require(page >= 0) { "ํ์ด์ง๋ 0 ์ด์์ด์ด์ผ ํฉ๋๋ค." } |
๐ค Prompt for AI Agents
In
pida-core/core-domain/src/main/kotlin/com/pida/support/page/OffsetPageRequest.kt
around line 8, the Korean error message in the require() call contains a typo
("์ด์ผ ํฉ๋๋ค"); update the string to the correct grammar ("์ด์ด์ผ ํฉ๋๋ค") so the message
reads "ํ์ด์ง๋ 0 ์ด์์ด์ด์ผ ํฉ๋๋ค." and keep the require condition unchanged.
| require(totalCount >= 0) { "totalCount ($totalCount) cannot be negative" } | ||
| require(totalCount >= content.size) { | ||
| "totalCount ($totalCount) cannot be smaller than content.size (${content.size})" | ||
| } | ||
| val count = if (content.isEmpty()) 0 else totalCount | ||
| return Page(content, count) |
There was a problem hiding this comment.
Inconsistent validation and count assignment logic.
Lines 12-15 validate that totalCount >= content.size, allowing totalCount > 0 when content.isEmpty(). However, Line 16 forces count = 0 when content is empty, overriding the validated totalCount. This creates inconsistency:
- If
totalCount=10with empty content is allowed by validation, why override it to 0? - If
count=0for empty content is intended, validation should rejecttotalCount > 0when content is empty.
Clarify the intended behavior and align validation with the assignment logic.
๐ Proposed fix: Align validation with assignment logic
Option 1: Remove the override (if totalCount should be preserved)
- val count = if (content.isEmpty()) 0 else totalCount
- return Page(content, count)
+ return Page(content, totalCount)Option 2: Strengthen validation (if count must be 0 for empty content)
require(totalCount >= 0) { "totalCount ($totalCount) cannot be negative" }
+ require(content.isNotEmpty() || totalCount == 0L) {
+ "totalCount must be 0 when content is empty, but got $totalCount"
+ }
require(totalCount >= content.size) {
"totalCount ($totalCount) cannot be smaller than content.size (${content.size})"
}
- val count = if (content.isEmpty()) 0 else totalCount
- return Page(content, count)
+ return Page(content, totalCount)๐ 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.
| require(totalCount >= 0) { "totalCount ($totalCount) cannot be negative" } | |
| require(totalCount >= content.size) { | |
| "totalCount ($totalCount) cannot be smaller than content.size (${content.size})" | |
| } | |
| val count = if (content.isEmpty()) 0 else totalCount | |
| return Page(content, count) | |
| require(totalCount >= 0) { "totalCount ($totalCount) cannot be negative" } | |
| require(totalCount >= content.size) { | |
| "totalCount ($totalCount) cannot be smaller than content.size (${content.size})" | |
| } | |
| return Page(content, totalCount) |
| require(totalCount >= 0) { "totalCount ($totalCount) cannot be negative" } | |
| require(totalCount >= content.size) { | |
| "totalCount ($totalCount) cannot be smaller than content.size (${content.size})" | |
| } | |
| val count = if (content.isEmpty()) 0 else totalCount | |
| return Page(content, count) | |
| require(totalCount >= 0) { "totalCount ($totalCount) cannot be negative" } | |
| require(content.isNotEmpty() || totalCount == 0L) { | |
| "totalCount must be 0 when content is empty, but got $totalCount" | |
| } | |
| require(totalCount >= content.size) { | |
| "totalCount ($totalCount) cannot be smaller than content.size (${content.size})" | |
| } | |
| return Page(content, totalCount) |
๐ค Prompt for AI Agents
In pida-core/core-domain/src/main/kotlin/com/pida/support/page/Page.kt around
lines 12 to 17, the code validates totalCount >= content.size but then forces
count = 0 when content.isEmpty(), causing inconsistency; to fix, stop overriding
a validated totalCount: assign count = totalCount unconditionally (remove the
empty-content special-case) so the validated totalCount is preserved, and leave
the existing require checks as-is (or add a brief comment explaining that
totalCount may be >0 even when content is empty).
| fun findAllBy( | ||
| userId: Long, | ||
| cursorRequest: CursorRequest, | ||
| ): Cursor<NotificationStored.Info> { | ||
| val pageable = PageRequest.ofSize(cursorRequest.size.toInt()) | ||
| val notifications = | ||
| notificationStoredJpaRepository.findPage(JDSLExtensions, pageable) { | ||
| select(entity(NotificationStoredEntity::class)) | ||
| .from(entity(NotificationStoredEntity::class)) | ||
| .whereAnd( | ||
| path(NotificationStoredEntity::userId).equal(userId), | ||
| path(NotificationStoredEntity::type).notIn(EXCLUDED_TYPES), | ||
| cursorRequest.lastId?.let { | ||
| path(NotificationStoredEntity::id).lessThan(cursorRequest.lastId) | ||
| }, | ||
| ).orderBy( | ||
| path(NotificationStoredEntity::id).desc(), | ||
| ) | ||
| } | ||
| val content = | ||
| notifications.content | ||
| .filterNotNull() | ||
| .map { it.toNotificationStoredInfo() } | ||
| val nextCursor = if (content.size < cursorRequest.size) null else notifications.lastOrNull()?.id | ||
|
|
||
| return Cursor.of( | ||
| nextCursor = nextCursor, | ||
| size = notifications.totalElements, | ||
| content = content, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix cursor pagination logic to handle edge case.
The nextCursor calculation at line 45 has a subtle bug: when content.size == cursorRequest.size, the code assumes there are more pages and sets nextCursor = notifications.lastOrNull()?.id. However, if the query returned exactly cursorRequest.size items and there are no more items in the database, the client will make an unnecessary additional request that returns empty results.
Standard cursor pagination patterns typically:
- Fetch
size + 1items to detect if more pages exist, or - Use a separate count query (though less efficient)
๐ Proposed fix using size+1 approach
- val pageable = PageRequest.ofSize(cursorRequest.size.toInt())
+ val pageable = PageRequest.ofSize(cursorRequest.size.toInt() + 1)
val notifications =
notificationStoredJpaRepository.findPage(JDSLExtensions, pageable) {
select(entity(NotificationStoredEntity::class))
.from(entity(NotificationStoredEntity::class))
.whereAnd(
path(NotificationStoredEntity::userId).equal(userId),
path(NotificationStoredEntity::type).notIn(EXCLUDED_TYPES),
cursorRequest.lastId?.let {
path(NotificationStoredEntity::id).lessThan(cursorRequest.lastId)
},
).orderBy(
path(NotificationStoredEntity::id).desc(),
)
}
- val content =
- notifications.content
+ val allItems = notifications.content
.filterNotNull()
.map { it.toNotificationStoredInfo() }
+ val hasMore = allItems.size > cursorRequest.size
+ val content = if (hasMore) allItems.take(cursorRequest.size.toInt()) else allItems
- val nextCursor = if (content.size < cursorRequest.size) null else notifications.lastOrNull()?.id
+ val nextCursor = if (hasMore) content.lastOrNull()?.id else null
return Cursor.of(
nextCursor = nextCursor,
- size = notifications.totalElements,
+ size = content.size.toLong(),
content = content,
)Note: Also consider whether size in the Cursor should represent the current page size or total count across all pages. The current implementation uses totalElements which may be inefficient for large datasets.
๐ 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.
| fun findAllBy( | |
| userId: Long, | |
| cursorRequest: CursorRequest, | |
| ): Cursor<NotificationStored.Info> { | |
| val pageable = PageRequest.ofSize(cursorRequest.size.toInt()) | |
| val notifications = | |
| notificationStoredJpaRepository.findPage(JDSLExtensions, pageable) { | |
| select(entity(NotificationStoredEntity::class)) | |
| .from(entity(NotificationStoredEntity::class)) | |
| .whereAnd( | |
| path(NotificationStoredEntity::userId).equal(userId), | |
| path(NotificationStoredEntity::type).notIn(EXCLUDED_TYPES), | |
| cursorRequest.lastId?.let { | |
| path(NotificationStoredEntity::id).lessThan(cursorRequest.lastId) | |
| }, | |
| ).orderBy( | |
| path(NotificationStoredEntity::id).desc(), | |
| ) | |
| } | |
| val content = | |
| notifications.content | |
| .filterNotNull() | |
| .map { it.toNotificationStoredInfo() } | |
| val nextCursor = if (content.size < cursorRequest.size) null else notifications.lastOrNull()?.id | |
| return Cursor.of( | |
| nextCursor = nextCursor, | |
| size = notifications.totalElements, | |
| content = content, | |
| ) | |
| } | |
| fun findAllBy( | |
| userId: Long, | |
| cursorRequest: CursorRequest, | |
| ): Cursor<NotificationStored.Info> { | |
| val pageable = PageRequest.ofSize(cursorRequest.size.toInt() + 1) | |
| val notifications = | |
| notificationStoredJpaRepository.findPage(JDSLExtensions, pageable) { | |
| select(entity(NotificationStoredEntity::class)) | |
| .from(entity(NotificationStoredEntity::class)) | |
| .whereAnd( | |
| path(NotificationStoredEntity::userId).equal(userId), | |
| path(NotificationStoredEntity::type).notIn(EXCLUDED_TYPES), | |
| cursorRequest.lastId?.let { | |
| path(NotificationStoredEntity::id).lessThan(cursorRequest.lastId) | |
| }, | |
| ).orderBy( | |
| path(NotificationStoredEntity::id).desc(), | |
| ) | |
| } | |
| val allItems = notifications.content | |
| .filterNotNull() | |
| .map { it.toNotificationStoredInfo() } | |
| val hasMore = allItems.size > cursorRequest.size | |
| val content = if (hasMore) allItems.take(cursorRequest.size.toInt()) else allItems | |
| val nextCursor = if (hasMore) content.lastOrNull()?.id else null | |
| return Cursor.of( | |
| nextCursor = nextCursor, | |
| size = content.size.toLong(), | |
| content = content, | |
| ) | |
| } |
๐ค Prompt for AI Agents
In
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.kt
around lines 22 to 52, the nextCursor logic assumes more pages exist when
content.size == cursorRequest.size which can cause an extra empty request;
change the query to fetch cursorRequest.size + 1 items, determine hasMore by
whether more than requested items were returned, set nextCursor only when
hasMore (use the id of the extra item or the last of the requested items as
appropriate), trim the returned content to cursorRequest.size, and set the
Cursor.size to the actual returned page size (or content.size) instead of
notifications.totalElements to avoid expensive total count.
| } | ||
| val content = | ||
| notifications.content | ||
| .filterNotNull() |
There was a problem hiding this comment.
๐งฉ Analysis chain
๐ Script executed:
# First, let's read the file to understand the context around line 43
cat -n pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.kt | sed -n '30,60p'Repository: Team-PIDA/Pida-Server
Length of output: 1209
๐ Script executed:
# Search for the complete method/class containing line 43
rg -nP --type=kt -B 20 -A 5 'filterNotNull' pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.ktRepository: Team-PIDA/Pida-Server
Length of output: 91
๐ Script executed:
# Check JDSL query patterns in the codebase to understand if null entities are possible
rg -nP --type=kt 'findPage.*filterNotNull' --max-count=10Repository: Team-PIDA/Pida-Server
Length of output: 91
๐ Script executed:
# Look for JDSL documentation or type signatures
rg -nP --type=kt 'import.*jdsl' --max-count=20Repository: Team-PIDA/Pida-Server
Length of output: 91
๐ Script executed:
# Search for findPage implementation and return type
rg -n 'fun findPage' pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/ -A 5Repository: Team-PIDA/Pida-Server
Length of output: 47
๐ Script executed:
# Look for other filterNotNull usages in the codebase
rg -n 'filterNotNull' pida-storage/db-core/src/main/kotlin/ --max-count=15Repository: Team-PIDA/Pida-Server
Length of output: 213
๐ Script executed:
# Check JDSL imports to understand the library
rg -n 'import.*jdsl' pida-storage/db-core/src/main/kotlin/ --max-count=10Repository: Team-PIDA/Pida-Server
Length of output: 1730
๐ Script executed:
# Look for the complete method signature to see return type
cat -n pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.kt | sed -n '20,52p'Repository: Team-PIDA/Pida-Server
Length of output: 1640
๐ Script executed:
# Find the JDSLExtensions implementation
cat -n pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/support/JDSLExtensions.ktRepository: Team-PIDA/Pida-Server
Length of output: 763
๐ Script executed:
# Find the findPage extension function definition
rg -n 'fun.*findPage' pida-storage/db-core/src/main/kotlin/ -A 10Repository: Team-PIDA/Pida-Server
Length of output: 47
๐ Script executed:
# Check if there are other similar repository patterns without filterNotNull
rg -n 'findPage.*\.content' pida-storage/db-core/src/main/kotlin/ -A 3Repository: Team-PIDA/Pida-Server
Length of output: 47
๐ Script executed:
# Search for findPage in all files including XML or Gradle configs
find pida-storage -name "*.kt" -type f | xargs grep -l 'fun.*findPage' 2>/dev/nullRepository: Team-PIDA/Pida-Server
Length of output: 49
๐ Script executed:
# Check the JpaRepository interface or extension
rg -n 'findPage|KotlinJdslJpqlExecutor' pida-storage/db-core/src/main/kotlin/ -B 2 -A 8Repository: Team-PIDA/Pida-Server
Length of output: 11240
๐ Script executed:
# Check if there are any null checks or Optional handling in the codebase
rg -n 'notifications\.content' pida-storage/db-core/src/main/kotlin/ -B 2 -A 3Repository: Team-PIDA/Pida-Server
Length of output: 1023
๐ Script executed:
# Look at the NotificationStoredJpaRepository to understand its interface
cat pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredJpaRepository.ktRepository: Team-PIDA/Pida-Server
Length of output: 659
๐ Script executed:
# Search for how findPage is used in other custom repositories
find pida-storage -name "*CustomRepository.kt" -type f -exec grep -l "findPage" {} \;Repository: Team-PIDA/Pida-Server
Length of output: 177
๐ Script executed:
# Look at BloomingCustomRepository to see if it uses similar patterns
cat pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/blooming/BloomingCustomRepository.ktRepository: Team-PIDA/Pida-Server
Length of output: 2650
๐ Script executed:
# Check Spring Data Page documentation or similar usage in codebase
rg -n '\.content' pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/ -B 2 -A 2Repository: Team-PIDA/Pida-Server
Length of output: 1495
๐ Script executed:
# Search for any type annotations or method signatures that might hint at nullable content
rg -n 'Page<.*\?>' pida-storage/ -A 2Repository: Team-PIDA/Pida-Server
Length of output: 47
๐ Script executed:
# Check the complete method signature including imports to understand return type
cat pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.kt | head -20Repository: Team-PIDA/Pida-Server
Length of output: 726
๐ Script executed:
# Verify if JDSL is documented in build files or dependencies
find pida-storage -name "build.gradle*" -o -name "pom.xml" | xargs cat 2>/dev/null | grep -i "jdsl\|kotlin.*jdsl" -A 1 -B 1Repository: Team-PIDA/Pida-Server
Length of output: 208
๐ Script executed:
# Check if there are any comments or documentation about why filterNotNull is used
grep -B 5 -A 1 'filterNotNull' pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.ktRepository: Team-PIDA/Pida-Server
Length of output: 311
๐ Script executed:
# Verify the Spring Data Page type signature by checking library usage
rg -n 'Page<' pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/ -A 1Repository: Team-PIDA/Pida-Server
Length of output: 47
Remove unnecessary filterNotNull() call.
The JDSL query explicitly selects complete entities (select(entity(NotificationStoredEntity::class))), and Spring Data's Page<T>.content property returns List<T> (non-nullable). JPQL entity selections cannot produce null values, making filterNotNull() unnecessary.
๐ค Prompt for AI Agents
In
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/notification/NotificationStoredCustomRepository.kt
around line 43, remove the unnecessary .filterNotNull() call from the pipeline:
the JDSL query selects full entities and Page<T>.content is non-nullable so
.filterNotNull() is redundant; simply drop that call and adjust any chained
calls accordingly (no replacement needed).
| fun toNotificationStoredInfo(): NotificationStored.Info = | ||
| NotificationStored.Info( | ||
| id = id!!, | ||
| notificationStoredKey = notificationStoredKey, | ||
| userId = userId, | ||
| type = type, | ||
| parameterValue = parameterValue, | ||
| topic = topic, | ||
| contents = contents, | ||
| readStatus = readStatus, | ||
| createdAt = createdAt.toString(), | ||
| ) |
There was a problem hiding this comment.
Fix potential NPE from force-unwrapping id.
Line 40 uses id!! which will throw a NPE if toNotificationStoredInfo() is called on a non-persisted entity (before save() completes). While the current usage pattern may ensure entities are always persisted first, this creates a fragile contract.
๐ Proposed fix with safer null handling
fun toNotificationStoredInfo(): NotificationStored.Info =
NotificationStored.Info(
- id = id!!,
+ id = requireNotNull(id) { "Cannot convert unpersisted entity to Info" },
notificationStoredKey = notificationStoredKey,
userId = userId,
type = type,
parameterValue = parameterValue,
topic = topic,
contents = contents,
readStatus = readStatus,
createdAt = createdAt.toString(),
)This provides a clearer error message if the contract is violated.
๐ 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.
| fun toNotificationStoredInfo(): NotificationStored.Info = | |
| NotificationStored.Info( | |
| id = id!!, | |
| notificationStoredKey = notificationStoredKey, | |
| userId = userId, | |
| type = type, | |
| parameterValue = parameterValue, | |
| topic = topic, | |
| contents = contents, | |
| readStatus = readStatus, | |
| createdAt = createdAt.toString(), | |
| ) | |
| fun toNotificationStoredInfo(): NotificationStored.Info = | |
| NotificationStored.Info( | |
| id = requireNotNull(id) { "Cannot convert unpersisted entity to Info" }, | |
| notificationStoredKey = notificationStoredKey, | |
| userId = userId, | |
| type = type, | |
| parameterValue = parameterValue, | |
| topic = topic, | |
| contents = contents, | |
| readStatus = readStatus, | |
| createdAt = createdAt.toString(), | |
| ) |
LeeHanEum
left a comment
LeeHanEum
left a comment
There was a problem hiding this comment.
์์ฃผ ๊น๋ํ๊ณ ์ฑ
์์ด ๋ช
ํํ๊ฒ ๋ถ๋ฆฌ๋ ์ข์ ์ฝ๋ ์๋ดค์ต๋๋ค~
๊ถ๊ธํ ์ ๋ค ๋ช๊ฐ์ง ๋จ๊ฒจ๋ดค์ด์ :)
There was a problem hiding this comment.
์ ํ์ ์ ์ฐ๋ด์์๋ ์์ฐ์ด๊ณ ์๋๋ฐ, ์ด๋ค ์ญํ ์ ํ๋๊ฑด๊ฐ์?
There was a problem hiding this comment.
์ ๋ฐ์ดํฐ ํด๋์ค๋ FcmSender ๋จ์์ ์ฌ์ฉ์์๊ฒ ํธ์๋ฅผ ๋ณด๋ด๊ธฐ ์ํ ๊ฐ์ฒด๋ก ์์ฑ์ ํ์๋๋ฐ, ์ด๋ฏธ ์ฌ์ฉ์ค์ธ๊ฒ ์๋ค์..! ์ญ์ ์กฐ์นํ ๊ฒ์!
| data class Result( | ||
| val id: Long, | ||
| val userId: Long, | ||
| val nickname: String?, | ||
| val spotName: String?, | ||
| val site: String?, | ||
| val type: String, | ||
| val parameterValue: String, | ||
| val topic: String, | ||
| val contents: String, | ||
| val readStatus: ReadStatus, | ||
| val createdAt: String, | ||
| ) |
There was a problem hiding this comment.
์๊ธฐ Result๋ ์ด๋ค ์ญํ ์ธ๊ฐ์~
There was a problem hiding this comment.
์ฌ๊ธฐ Result๋ Facade ๋ ์ด์ด์์ User ๋๋ฉ์ธ๊ณผ Notification ๋๋ฉ์ธ์์ ๋น์ฆ๋์ค ๋ก์ง์ ๊ฒฐํฉํ ๋ฐ์ดํฐ ๋ชจ๋ธ์ ๊ฒฐ๊ณผ๋ผ๊ณ ๋ด์ฃผ์ฌ ๋ ๊ฑฐ ๊ฐ์ต๋๋ค!
| val fcmToken: String, | ||
| val title: String, | ||
| val body: String, | ||
| val destination: String, |
There was a problem hiding this comment.
์ด๊ฑด ์ ๊ฐ FCM์ ๋ํ ์ดํด๊ฐ ๋ถ์กฑํด์ ์ผ ์๋ ์๋๋ฐ, destination๊ณผ topic์ ์ด๋ค ์ฐจ์ด๊ฐ ์๋์?
๋๋ค ์ ์ก ๊ฒฝ๋ก๋ผ๋ ๋น์ทํ ์๋ฏธ๋ก ์๊ณ ์์ด์ ์ฌ์ญค๋ด
๋๋ค!
There was a problem hiding this comment.
destination์ FCM ๊ด๋ จํ ํ๋๊ฐ ์๋๋๋ค!
putCustomData์ผ๋ก APNS์์ ์ ํฌ๊ฐ ์์๋ก ์ปค์คํ
ํ๋๋ฅผ ์ง์ ํ ์ ์๊ณ , ํธ์ ์๋ฆผ์์ UI ์ ํ์ ์ํ ์ฑ์คํค๋ง ์ปจํ
์คํธ๋ผ๊ณ ๋ด์ฃผ์ฌ ์ข์ ๊ฑฐ ๊ฐ์ต๋๋ค!
There was a problem hiding this comment.
์ํ ๊ทธ๋ ๊ตฐ์ ๋ต๋ณ ๊ฐ์ฌํฉ๋๋ค!
| nextCursor: Long?, | ||
| size: Long, | ||
| ): Cursor<T> { | ||
| require(size >= 0) { "size ($size) must be greater than or equal to 0" } |
There was a problem hiding this comment.
๋๋จธ์ง๋ ๋ค ํ๊ธ์ธ๊ฑฐ ๊ฐ์๋ฐ ์๊ฑฐ๋ ์์ด๋ค์~
| content: List<T>, | ||
| totalCount: Long, | ||
| ): Page<T> { | ||
| require(totalCount >= 0) { "totalCount ($totalCount) cannot be negative" } |
There was a problem hiding this comment.
์๊ฒ๋ ์์ด๋ค์~ ์ผ๊ด์ฑ์ด ์์ผ๋ฉด ์ข์๊ฑฐ ๊ฐ์์!
There was a problem hiding this comment.
์ด๊ฑฐ๋ ์ ๊ฐ ์ดํด๊ฐ ์ข ํ์ํ ๊ฑฐ ๊ฐ์์ Tx์ TXAdvice ๊ฐ๊ฐ ์ด๋ค ์ญํ ์ ํ๋์ง ๊ฐ๋จํ๊ฒ ์ค๋ช
๋ถํ๋๋ ค๋ ๋ ๊น์?
There was a problem hiding this comment.
์ ์ ๊ฐ ์ค์ํ ๋ถ๋ถ์ด ์๋ค์..
์ด์ ์ฐ๋ด ๋ ์์ ์์๋ spring.main.lazy-initialization ์ต์ ์ ๋นผ๊ณ ์งํํ์ฌ ๊ตณ์ด txAdvice๋ฅผ ์์กด์ฑ ์ฃผ์ ํ์ง ์์๋ ํ์ฅํจ์๋ก ์ถฉ๋ถํ ํธ๋์ญ์ ๋ฒ์๋ฅผ ์ก์ ์ ์์์ต๋๋ค.
๊ทธ๋ฌ๋ ์ ๋ฒ ํผ๋ค ๊ฐ๋ฐํ ๋น์์ lazy-initialization์ ์๋ฒ์ ์ผ๋ก ์ฌ์ฉํ์๊ณ , txAdvice๋ฅผ ์์กด์ฑ ๋ฐ์ ์ฒ๋ฆฌํ์์ต๋๋ค. ์ lazy-initialization์ด true ์ต์ ์์ ๊ตณ์ด TxAdvice ํด๋์ค๋ฅผ ์์กด๋ฐ๋๋ ํ๋ฉด, ์คํ๋ง ์ ํ๋ฆฌ์ผ์ด์ ์คํ ์ ๋น ์ด๊ธฐํ๊ฐ ์ด๋ค์ง๋ ๊ณผ์ ์์ Tx๋ lazy-initialization ์ต์ ์ผ๋ก ์ธํด ๋ฆ์ ์ด๊ธฐํ๊ฐ ๋๊ธฐ์ ํธ๋์ญ์ ๋ฒ์๋ฅผ ์ก์ง ๋ชปํ๋ ํ์์ด ์์ด ์์กด์ฑ ์ฃผ์ ์ ๋ฐ์ ์ฒ๋ฆฌ๋ฅผ ํ์์ต๋๋ค.
๊ทธ๋์ lazy-initialization์ด ๊ตณ์ด ์ง๊ธ์ผ๋ก์จ ํ์ํ๋๊ฐ์ ๋ํด์ ๊ณ ๋ฏผ์ ํด๋ณด๋ฉด ๊ทธ๋ ์ง๋ ์๋ค๊ณ ์๊ฐ๋๊ธฐ์ lazy-initialization ์ ๊ฑฐ ํ Tx ํ์ฅํจ์๋ฅผ ์ฌ์ฉํ๋ ์ชฝ์ผ๋ก ์์ ํด๋๊ฒ ์ต๋๋ค...!
There was a problem hiding this comment.
์ ๋ค๋ค ํ์ธํ์ต๋๋ค. ๋๋ถ์ lazy-initialization์ ๋ํด ์์๋ณผ ์ ์์์ด์! ๊ฐ์ฌํฉ๋๋ค~
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and canโt be posted inline due to platform limitations.
โ ๏ธ Outside diff range comments (2)
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/blooming/BloomingCoreRepository.kt (2)
25-31: Inconsistent transaction wrapper for suspend functions.These suspend functions use
Tx.readableinstead ofTx.coReadable, which differs from the pattern inUserCoreRepositorywhere all suspend read operations useTx.coReadable.
Method Current Expected (per UserCoreRepository pattern) findTopByUserIdAndSpotIdDecsTx.readableTx.coReadablefindAllByUserIdTx.readableTx.coReadablefindAllByFlowerSpotIdTx.readableTx.coReadableCompare with
findRecentlyBySpotId(line 43-46) which correctly usesTx.coReadablefor a suspend function.๐ Proposed fix for consistency
override suspend fun findTopByUserIdAndSpotIdDecs( userId: Long, flowerSpotId: Long, ): Blooming? = - Tx.readable { + Tx.coReadable { bloomingJpaRepository.findTopByUserIdAndFlowerSpotIdOrderByCreatedAtDesc(userId, flowerSpotId)?.toBlooming() } override suspend fun findAllByUserId(userId: Long): List<Blooming> = - Tx.readable { + Tx.coReadable { bloomingJpaRepository.findAllByUserId(userId).map { it.toBlooming() } } override suspend fun findAllByFlowerSpotId(flowerSpotId: Long): List<Blooming> = - Tx.readable { + Tx.coReadable { bloomingJpaRepository.findAllByFlowerSpotId(flowerSpotId).map { it.toBlooming() } }Also applies to: 33-36, 38-41
25-25: Typo in method name:DecsโDesc.The method name
findTopByUserIdAndSpotIdDecsappears to have a typo. It should likely befindTopByUserIdAndSpotIdDesc(for "descending").
๐ Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (3)
pida-core/core-domain/src/main/kotlin/com/pida/support/tx/TxAdvice.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/blooming/BloomingCoreRepository.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/UserCoreRepository.kt
๐ค Files with no reviewable changes (1)
- pida-core/core-domain/src/main/kotlin/com/pida/support/tx/TxAdvice.kt
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (21, 2.1.0, corretto)
๐ Additional comments (2)
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/blooming/BloomingCoreRepository.kt (1)
43-59: Correct transaction wrapper usage for remaining methods.These methods correctly apply the transaction pattern:
findRecentlyBySpotId(suspend) โTx.coReadableโfindRecentBySpotIds(non-suspend) โTx.readableโfindTodayBloomingByUserId(non-suspend) โTx.readableโpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/UserCoreRepository.kt (1)
6-6: Clean migration to the Tx API.The refactoring consistently applies the new transaction abstraction pattern:
- Non-suspend operations use
Tx.writeable/Tx.readable- Suspend operations use
Tx.coWriteable/Tx.coReadableThe constructor simplification aligns with the centralized
Txutility approach, which is properly configured as a Spring-managed singleton with access to the transaction manager through itsTxAdvicecomponent.
There was a problem hiding this comment.
Actionable comments posted: 0
โป๏ธ Duplicate comments (1)
pida-core/core-domain/src/main/kotlin/com/pida/support/cursor/Cursor.kt (1)
17-20: Past issue resolved: error message now correctly references "size".The validation logic is correct, and the error message accurately refers to the
sizeparameter. The past review concern about using "totalCount" instead of "size" has been addressed.
๐งน Nitpick comments (2)
pida-core/core-domain/src/main/kotlin/com/pida/support/cursor/Cursor.kt (2)
8-10: Consider type consistency for DEFAULT_SIZE.
DEFAULT_SIZEis defined asIntwhile thesizeparameter in the factory method isLong. While Kotlin handles the conversion automatically, using consistent types improves clarity.๐ Proposed fix for type consistency
companion object { const val DEFAULT_CURSOR = 0L - const val DEFAULT_SIZE = 20 + const val DEFAULT_SIZE = 20L
3-7: Consider clarifying the semantic purpose of thesizeproperty.The
sizeproperty in a pagination response is semantically unusual. Typical cursor-based pagination responses contain the data (content) and navigation metadata (nextCursor), but don't typically echo back the requested page size. The property namesizeis also ambiguousโit could be interpreted as the content size, the page size limit, or even a total count.If the
sizeproperty serves a specific purpose in your API design, consider renaming it to clarify intent (e.g.,pageSize,limit, orrequestedSize). Alternatively, if it's not needed by consumers, consider removing it from the response.
๐ Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (3)
pida-core/core-api/src/main/resources/application.ymlpida-core/core-domain/src/main/kotlin/com/pida/support/cursor/Cursor.ktpida-core/core-domain/src/main/kotlin/com/pida/support/page/Page.kt
๐ค Files with no reviewable changes (1)
- pida-core/core-api/src/main/resources/application.yml
๐ง Files skipped from review as they are similar to previous changes (1)
- pida-core/core-domain/src/main/kotlin/com/pida/support/page/Page.kt
โฐ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (21, 2.1.0, corretto)
๐ฑ ๊ด๋ จ ์ด์
๐ ์์ ๋ด์ฉ ๋ฐ ํน์ด ์ฌํญ
๐ ์ฐธ๊ณ
๐ ์ฒดํฌ ๋ฆฌ์คํธ
Summary by CodeRabbit
Release Notes
New Features
Refactor
โ๏ธ Tip: You can customize this high-level summary in your review settings.