[Feature] User Device domain & fcm token insert #72
Conversation
π WalkthroughWalkthroughAdds FCM token registration: new API endpoint, request/response DTOs, domain model, service/appender/reader components, repository contract, JPA entity/repository, and a DB-backed repository implementation enforcing max 5 devices per user and duplicate-token checks. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as UserController
participant Service as UserDeviceService
participant Appender as UserDeviceAppender
participant Reader as UserDeviceReader
participant Repo as UserDeviceCoreRepository
participant DB as Database
Client->>Controller: PUT /users/fcm-token (body, X-DEVICE-ID)
Controller->>Controller: Map request -> UserDevice.Create
Controller->>Service: append(create)
Service->>Appender: append(create)
Appender->>Reader: readAllByUserKey(userKey)
Reader->>Repo: findAllByUserKey(userKey)
Repo->>DB: SELECT non-deleted devices
DB-->>Repo: device list
Repo-->>Reader: List<UserDevice.Info>
Reader-->>Appender: List<UserDevice.Info>
alt Duplicate fcmToken found
Appender-->>Service: return (no-op)
else Too many devices (>=5)
Appender->>Repo: softDeleteBy(oldestId)
Repo->>DB: UPDATE deletedAt
DB-->>Repo: OK
end
Appender->>Repo: save(create)
Repo->>DB: INSERT device row
DB-->>Repo: persisted entity
Repo-->>Appender: UserDevice.Info
Appender-->>Service: complete
Service-->>Controller: success
Controller-->>Client: UserMobileDeviceResponse
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 minutes Suggested reviewers
Poem
π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing touches
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: 11
π€ Fix all issues with AI agents
In
@pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/request/UserMobileDeviceRequest.kt:
- Around line 13-22: The toCreate function currently generates a new UUID when
deviceId is null, which prevents upserts; change to require a stable deviceId
instead: remove the fallback UUID in UserMobileDeviceRequest.toCreate so it
passes deviceId directly into UserDevice.Create (make deviceId non-nullable in
the request or validate/throw earlier), and ensure callers/validation reject
requests without a deviceId so existing devices can be found and FCM tokens
upserted.
- Around line 10-11: Add Jakarta validation to the UserMobileDeviceRequest
property fcmToken: annotate the fcmToken property (val fcmToken: String) with
@field:NotBlank to reject empty/blank values and @field:Size(max = 4096) to
guard against excessively long tokens, and add the corresponding
jakarta.validation.constraints imports; ensure the annotations are placed on the
Kotlin field level (using @field:) so validation frameworks will pick them up.
In
@pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/UserController.kt:
- Line 66: The parameter declaration for user: User in UserController is missing
the @Parameter(hidden = true, required = false) annotation used elsewhere;
update the function signature to add @Parameter(hidden = true, required = false)
to the user: User parameter so Swagger/OpenAPI documentation is consistent with
the other endpoints (match the annotation usage found on the user parameters at
lines where other handlers declare user: User).
In @pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDevice.kt:
- Around line 13-18: The Create and Info data classes in UserDevice currently
store both userId and userKey, creating redundant, potentially inconsistent
state; update these models (e.g., the data class Create and the Info class
inside UserDevice) to persist only one canonical identifier (choose either
userId or userKey), remove the duplicate field from the other class, and update
any constructors, factory methods, mappers, and DAO/Repository code that
construct or read UserDevice (references: Create, Info, UserDevice) to join to
the User domain to resolve the other identifier when needed so lookups remain
correct.
- Line 11: The X-DEVICE-ID header is treated as optional and can be null/blank,
but a random UUID is silently generated; update the handling so the header is
validated instead: either make the controller parameter non-nullable (remove
nullable String? for @RequestHeader("X-DEVICE-ID")) to require the header, or
add explicit validation inside UserMobileDeviceRequest.toCreate() (or a
dedicated validator) to check that the provided deviceId is non-blank and return
a 4xx error when invalid; only fall back to generating UUID when your business
logic allows missing device IDs and document that behavior.
In
@pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceAppender.kt:
- Around line 9-18: The append method in UserDeviceAppender has a TOCTOU race
and lacks a transaction: add a DB-level unique constraint on (userKey, fcmToken)
and make append transactional (e.g., annotate the append function or call within
a transaction) so findAllByUserKey, softDeleteBy and save are atomic; eliminate
the check-then-act race by attempting to save and handling a duplicate-key
exception from the DB instead of relying solely on exists, and ensure
softDelete+save happen in one transaction so a failed save cannot leave the old
device deleted; fix the early-return logic around minByOrNull (do not silently
return β either throw or ensure a fallback) and replace minByOrNull { it.id }
with ordering by a createdAt timestamp (or add a repository method like
deleteOldestByUserKeyOrderByCreatedAt) to reliably pick the oldest device.
In
@pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceRepository.kt:
- Line 8: The method name findLastByUserId(UserDevice.Info?) is ambiguous about
ordering; update the API to make ordering explicit by either renaming the method
(e.g., findMostRecentByUserIdOrderByCreatedAtDesc or
findLatestUpdatedByUserIdOrderByUpdatedAtDesc) or adding KDoc above
findLastByUserId that clearly states the ordering criterion (for example
"returns the user device with the most recent createdAt timestamp"); then update
all implementations of UserDeviceRepository and any callers/tests to match the
new method name or rely on the documented ordering, ensuring return type
UserDevice.Info remains correct.
In
@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceCoreRepository.kt:
- Around line 29-35: The method findLastByUserId in UserDeviceCoreRepository
uses
userDeviceJpaRepository.findAllByUserIdAndDeletedAtIsNull(...).lastOrNull(),
which is non-deterministic; update the JPA repository to provide an explicit
ordered query (e.g., add findAllByUserIdAndDeletedAtIsNullOrderByCreatedAtDesc
or OrderByIdDesc) or a method that returns the first record in descending order,
then change findLastByUserId to call that ordered repository method and use
firstOrNull() (or take the first element) so the returned device is
deterministic.
- Around line 51-58: The softDeleteBy method in UserDeviceCoreRepository
currently no-ops if the entity isn't found, allowing UserDeviceAppender to
proceed and potentially violate the 5-device limit; change softDeleteBy to
either throw when the id is missing or return a Boolean success flag and update
the caller to check it: in UserDeviceCoreRepository.update softDeleteBy(Long id)
either retrieve with userDeviceJpaRepository.findById(id).orElseThrow {
EntityNotFoundException("UserDevice $id not found") } and then call softDelete()
and save, or change its signature to boolean softDeleteBy(Long id) that returns
true when entity was found/soft-deleted and false otherwise; then modify
UserDeviceAppender to call the new softDeleteBy and abort/throw if it returns
false (or let the thrown exception propagate) so deletion is validated before
creating a new device.
In
@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceEntity.kt:
- Around line 8-15: The entity t_user_device (UserDeviceEntity) lacks a DB-level
unique constraint on (userId, deviceId), allowing duplicate device rows and
breaking upsert/limit logic; add a unique constraint annotation to
UserDeviceEntity (on table name "t_user_device") enforcing uniqueness of the
userId and deviceId columns and create a corresponding migration that adds a
unique index/constraint on (user_id, device_id) in the schema so concurrent
inserts cannot create duplicates.
In
@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceJpaRepository.kt:
- Around line 6-14: The repository lacks DB-level queries for upsert and
device-limit enforcement: add methods to UserDeviceJpaRepository such as
findByUserIdAndDeviceIdAndDeletedAtIsNull(userId: Long, deviceId: String):
UserDeviceEntity? to locate an existing device for upsert, and
countByUserIdAndDeletedAtIsNull(userId: Long): Long to get the device count for
enforcing the 5-device limit; use these methods instead of fetching and
filtering all devices in memory.
π§Ή Nitpick comments (3)
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceEntity.kt (1)
23-31: Use safer null handling for id field.The
id!!force unwrap on line 25 will throwNullPointerExceptioniftoUserDevice()is accidentally called on a non-persisted entity. While this should not happen in normal operation, it makes debugging harder.π‘οΈ Proposed safer approach
fun toUserDevice() = UserDevice.Info( - id = id!!, + id = requireNotNull(id) { "Cannot convert non-persisted UserDeviceEntity to UserDevice.Info" }, userId = userId, userKey = userKey, fcmToken = fcmToken, deviceId = deviceId, createdAt = createdAt, )This provides a clearer error message if the precondition is violated.
pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceRepository.kt (1)
1-15: Consider adding KDoc for public API contracts.This repository interface lacks documentation for its methods. Adding KDoc comments would clarify expected behavior (e.g., what
softDeleteBydoes, whether methods throw exceptions on missing data, etc.).pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/UserController.kt (1)
64-72: Consider POST instead of PUT for token registration.The endpoint uses PUT, which conventionally implies idempotency and update semantics. However, the underlying
appendlogic (inUserDeviceAppender) performs a create-if-not-exists operation with a device limit enforcer. This behavior aligns more naturally with POST semantics, especially since:
- Duplicate tokens are silently ignored rather than updated
- The operation has side effects (soft-deleting old devices when limit is reached)
While not strictly incorrect, POST might better communicate the endpoint's intent.
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
π Files selected for processing (11)
pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/UserController.ktpida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/request/UserMobileDeviceRequest.ktpida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/response/UserMobileDeviceResponse.ktpida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDevice.ktpida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceAppender.ktpida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceReader.ktpida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceRepository.ktpida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceService.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceCoreRepository.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceEntity.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceJpaRepository.kt
π§° Additional context used
𧬠Code graph analysis (2)
pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDevice.kt (7)
pida-core/core-domain/src/main/kotlin/com/pida/notification/NewFcmToUser.kt (1)
userKey(3-7)pida-core/core-domain/src/main/kotlin/com/pida/user/NewUser.kt (1)
name(12-19)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/user/SocialUser.kt (1)
id(5-11)pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/UserCoreRepository.kt (1)
userJpaRepository(16-114)pida-core/core-domain/src/main/kotlin/com/pida/user/UserRepository.kt (1)
create(3-50)pida-core/core-domain/src/main/kotlin/com/pida/user/UserProfile.kt (1)
id(5-12)
pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceService.kt (4)
pida-core/core-domain/src/main/kotlin/com/pida/user/UserService.kt (1)
userAppender(5-59)pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/UserCoreRepository.kt (1)
userJpaRepository(16-114)pida-core/core-domain/src/main/kotlin/com/pida/user/UserUpdater.kt (1)
userRepository(5-23)pida-core/core-domain/src/main/kotlin/com/pida/user/UserRepository.kt (1)
create(3-50)
β° 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 (9)
pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/response/UserMobileDeviceResponse.kt (1)
1-9: LGTM!Clean and simple response DTO with appropriate OpenAPI documentation.
pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceReader.kt (1)
1-16: LGTM: Follows established architectural pattern.This reader component follows the same Service β Component β Repository layering pattern used elsewhere in the codebase (see
UserService,UserReader,UserUpdater), maintaining architectural consistency.pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceService.kt (1)
1-21: LGTM: Clean service layer delegation.This service class correctly delegates to
UserDeviceAppenderandUserDeviceReader, following the established architectural pattern seen inUserService. The method signatures are clear and the separation of concerns is appropriate.pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/UserController.kt (1)
67-70: No action needed β null deviceId is handled gracefully.The
toCreatemethod explicitly handles nulldeviceIdusing the Elvis operator:deviceId ?: UUID.randomUUID().toString(). When no device ID is provided, a random UUID is generated as a default, ensuring the downstreamUserDevice.Createalways has a valid value.Likely an incorrect or invalid review comment.
pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceCoreRepository.kt (5)
8-11: LGTM!Standard Spring repository implementation with proper constructor injection.
12-20: LGTM!Transaction handling and entity persistence are implemented correctly.
22-27: LGTM!Standard read operation with proper soft-delete filtering.
37-42: LGTM!Standard query implementation with proper filtering and mapping.
44-49: LGTM!Standard query implementation with proper filtering and mapping.
| @Schema(description = "FCM ν ν°", example = "fcm_token") | ||
| val fcmToken: String, |
There was a problem hiding this comment.
Add validation for fcmToken.
The fcmToken field accepts any string without validation. Empty, blank, or malformed tokens will be stored but cannot be used for push notifications, leading to failed delivery attempts and poor data quality.
β Proposed validation
Add Jakarta validation annotations:
+import jakarta.validation.constraints.NotBlank
+
@Schema(description = "FCM ν ν° λ±λ‘ μμ² Json")
data class UserMobileDeviceRequest(
@Schema(description = "FCM ν ν°", example = "fcm_token")
+ @field:NotBlank(message = "FCM ν ν°μ νμμ
λλ€")
val fcmToken: String,
)π 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.
| @Schema(description = "FCM ν ν°", example = "fcm_token") | |
| val fcmToken: String, | |
| @Schema(description = "FCM ν ν°", example = "fcm_token") | |
| @field:NotBlank(message = "FCM ν ν°μ νμμ λλ€") | |
| val fcmToken: String, |
π€ Prompt for AI Agents
In
@pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/request/UserMobileDeviceRequest.kt
around lines 10 - 11, Add Jakarta validation to the UserMobileDeviceRequest
property fcmToken: annotate the fcmToken property (val fcmToken: String) with
@field:NotBlank to reject empty/blank values and @field:Size(max = 4096) to
guard against excessively long tokens, and add the corresponding
jakarta.validation.constraints imports; ensure the annotations are placed on the
Kotlin field level (using @field:) so validation frameworks will pick them up.
| fun toCreate( | ||
| user: User, | ||
| deviceId: String?, | ||
| ): UserDevice.Create = | ||
| UserDevice.Create( | ||
| userId = user.id, | ||
| userKey = user.key, | ||
| fcmToken = fcmToken, | ||
| deviceId = deviceId ?: UUID.randomUUID().toString(), | ||
| ) |
There was a problem hiding this comment.
Critical: UUID generation prevents device upsert and breaks the 5-device limit.
Generating a new UUID when deviceId is null creates a new device record on every request from the same physical device, preventing token updates and device limit enforcement.
According to the PR objectives and commit message, this feature should support FCM token upsert (insert or update). However, without a stable deviceId, the system cannot identify existing devices to update them.
Consider one of these approaches:
- Make
deviceIdrequired β reject requests without a device identifier - Use fcmToken as the natural key β look up existing devices by fcmToken before creating new ones
- Return the generated deviceId to the client so it can be reused in subsequent requests
π§ Proposed fix: require deviceId
fun toCreate(
user: User,
- deviceId: String?,
+ deviceId: String,
): UserDevice.Create =
UserDevice.Create(
userId = user.id,
userKey = user.key,
fcmToken = fcmToken,
- deviceId = deviceId ?: UUID.randomUUID().toString(),
+ deviceId = deviceId,
)π€ Prompt for AI Agents
In
@pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/request/UserMobileDeviceRequest.kt
around lines 13 - 22, The toCreate function currently generates a new UUID when
deviceId is null, which prevents upserts; change to require a stable deviceId
instead: remove the fallback UUID in UserMobileDeviceRequest.toCreate so it
passes deviceId directly into UserDevice.Create (make deviceId non-nullable in
the request or validate/throw earlier), and ensure callers/validation reject
requests without a deviceId so existing devices can be found and FCM tokens
upserted.
pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/UserController.kt
Outdated
Show resolved
Hide resolved
| * | ||
| * @property userId μ¬μ©μ μμ΄λ | ||
| * @property fcmToken FCM ν ν° | ||
| * @property deviceId X-DEVICE-ID μμ΄λ |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
#!/bin/bash
# Search for X-DEVICE-ID header extraction patterns in controller files
rg -n -C5 -g '*.kt' 'X-DEVICE-ID|X_DEVICE_ID|@RequestHeader.*device|fcm-token' pida-core/core-api/Repository: Team-PIDA/Pida-Server
Length of output: 3358
π Script executed:
#!/bin/bash
# Find and inspect the controller that handles user device requests
fd -g '*.kt' -type f | xargs rg -l 'UserMobileDeviceRequest|UserDevice' pida-core/core-api/ | head -20Repository: Team-PIDA/Pida-Server
Length of output: 435
π Script executed:
#!/bin/bash
# Search for UserMobileDeviceRequest class and its toCreate method
rg -n -A 10 'fun toCreate|class UserMobileDeviceRequest' pida-core/Repository: Team-PIDA/Pida-Server
Length of output: 2024
The X-DEVICE-ID header is properly extracted, but validation is missing.
The controller correctly extracts the header via @RequestHeader("X-DEVICE-ID") and passes it to UserMobileDeviceRequest.toCreate(). However:
- The header is declared as nullable (
String?), making it optional - No validation exists to ensure the header is non-blank when provided
- When the header is missing or null, a random UUID is silently generated instead
Consider validating the header value or making it required if a device identifier is mandatory for the application's logic.
π€ Prompt for AI Agents
In @pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDevice.kt at
line 11, The X-DEVICE-ID header is treated as optional and can be null/blank,
but a random UUID is silently generated; update the handling so the header is
validated instead: either make the controller parameter non-nullable (remove
nullable String? for @RequestHeader("X-DEVICE-ID")) to require the header, or
add explicit validation inside UserMobileDeviceRequest.toCreate() (or a
dedicated validator) to check that the provided deviceId is non-blank and return
a 4xx error when invalid; only fall back to generating UUID when your business
logic allows missing device IDs and document that behavior.
| data class Create( | ||
| val userId: Long, | ||
| val userKey: String, | ||
| val fcmToken: String, | ||
| val deviceId: String, | ||
| ) |
There was a problem hiding this comment.
Consider storing only userId or userKey, not both.
Both Create and Info store userId and userKey, creating data redundancy. If these values become misaligned (e.g., through incomplete updates or inconsistent initialization), queries and lookups may return incorrect results.
Since userId and userKey have a 1:1 relationship in the User domain, consider:
- Store only
userIdand join to the User table whenuserKeyis needed - Store only
userKeyand join whenuserIdis needed
This eliminates duplication and prevents consistency issues.
π€ Prompt for AI Agents
In @pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDevice.kt
around lines 13 - 18, The Create and Info data classes in UserDevice currently
store both userId and userKey, creating redundant, potentially inconsistent
state; update these models (e.g., the data class Create and the Info class
inside UserDevice) to persist only one canonical identifier (choose either
userId or userKey), remove the duplicate field from the other class, and update
any constructors, factory methods, mappers, and DAO/Repository code that
construct or read UserDevice (references: Create, Info, UserDevice) to join to
the User domain to resolve the other identifier when needed so lookups remain
correct.
|
|
||
| fun findAll(): List<UserDevice.Info> | ||
|
|
||
| fun findLastByUserId(userId: Long): UserDevice.Info? |
There was a problem hiding this comment.
π οΈ Refactor suggestion | π Major
Clarify "last" ordering criteria.
The method name findLastByUserId is ambiguousβdoes "last" mean the most recent by createdAt, by updatedAt, or by id? This ambiguity can lead to incorrect usage or mismatched expectations between the interface and implementation.
π Recommended fix
Rename to clarify the ordering:
- fun findLastByUserId(userId: Long): UserDevice.Info?
+ fun findLatestByUserId(userId: Long): UserDevice.Info? // if ordered by createdAt DESCOr add KDoc:
+ /**
+ * Finds the most recently created device for the given user.
+ * @return the device with the latest createdAt timestamp, or null if none exist
+ */
fun findLastByUserId(userId: Long): UserDevice.Info?π 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 findLastByUserId(userId: Long): UserDevice.Info? | |
| fun findLatestByUserId(userId: Long): UserDevice.Info? // if ordered by createdAt DESC |
π€ Prompt for AI Agents
In
@pida-core/core-domain/src/main/kotlin/com/pida/user/device/UserDeviceRepository.kt
at line 8, The method name findLastByUserId(UserDevice.Info?) is ambiguous about
ordering; update the API to make ordering explicit by either renaming the method
(e.g., findMostRecentByUserIdOrderByCreatedAtDesc or
findLatestUpdatedByUserIdOrderByUpdatedAtDesc) or adding KDoc above
findLastByUserId that clearly states the ordering criterion (for example
"returns the user device with the most recent createdAt timestamp"); then update
all implementations of UserDeviceRepository and any callers/tests to match the
new method name or rely on the documented ordering, ensuring return type
UserDevice.Info remains correct.
| override fun findLastByUserId(userId: Long): UserDevice.Info? = | ||
| Tx.readable { | ||
| userDeviceJpaRepository | ||
| .findAllByUserIdAndDeletedAtIsNull(userId) | ||
| .lastOrNull() | ||
| ?.toUserDevice() | ||
| } |
There was a problem hiding this comment.
Add explicit ordering to ensure deterministic results.
The lastOrNull() call operates on an unordered list, making the "last" device selection non-deterministic. The result depends on the database query execution plan, which may vary.
For reliable device selection in FCM token management, add an explicit ORDER BY clause to the JPA query (e.g., by createdAt DESC, updatedAt DESC, or id DESC).
π‘ Suggested approach
Update UserDeviceJpaRepository to include an ordering method:
// In UserDeviceJpaRepository
fun findAllByUserIdAndDeletedAtIsNullOrderByCreatedAtDesc(userId: Long): List<UserDeviceEntity>
// or
fun findAllByUserIdAndDeletedAtIsNullOrderByIdDesc(userId: Long): List<UserDeviceEntity>Then update this method to use the ordered query:
override fun findLastByUserId(userId: Long): UserDevice.Info? =
Tx.readable {
userDeviceJpaRepository
- .findAllByUserIdAndDeletedAtIsNull(userId)
+ .findAllByUserIdAndDeletedAtIsNullOrderByCreatedAtDesc(userId)
.lastOrNull()
?.toUserDevice()
}Or better yet, use .firstOrNull() with descending order to avoid fetching all records:
override fun findLastByUserId(userId: Long): UserDevice.Info? =
Tx.readable {
userDeviceJpaRepository
- .findAllByUserIdAndDeletedAtIsNull(userId)
- .lastOrNull()
+ .findAllByUserIdAndDeletedAtIsNullOrderByCreatedAtDesc(userId)
+ .firstOrNull()
?.toUserDevice()
}Committable suggestion skipped: line range outside the PR's diff.
π€ Prompt for AI Agents
In
@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceCoreRepository.kt
around lines 29 - 35, The method findLastByUserId in UserDeviceCoreRepository
uses
userDeviceJpaRepository.findAllByUserIdAndDeletedAtIsNull(...).lastOrNull(),
which is non-deterministic; update the JPA repository to provide an explicit
ordered query (e.g., add findAllByUserIdAndDeletedAtIsNullOrderByCreatedAtDesc
or OrderByIdDesc) or a method that returns the first record in descending order,
then change findLastByUserId to call that ordered repository method and use
firstOrNull() (or take the first element) so the returned device is
deterministic.
| override fun softDeleteBy(id: Long) { | ||
| Tx.writeable { | ||
| userDeviceJpaRepository.findById(id).ifPresent { entity -> | ||
| entity.softDelete() | ||
| userDeviceJpaRepository.save(entity) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
rg -n -C3 'softDeleteBy\s*\(' --type=kotlinRepository: Team-PIDA/Pida-Server
Length of output: 2410
π Script executed:
# Find other soft delete implementations to understand the pattern
rg -n 'softDelete\(\)' --type=kotlin -C2
# Check for any error handling or validation around device limits
rg -n 'size\s*>=\s*5|deviceLimit|maxDevices' --type=kotlin -B3 -A3Repository: Team-PIDA/Pida-Server
Length of output: 2957
Add error handling or return value validation for device deletion.
The softDeleteBy method silently succeeds when the entity is not found, but the caller in UserDeviceAppender (line 15) doesn't validate whether deletion actually occurred before adding a new device. This breaks the 5-device limit enforcement: if the deletion fails silently, the new device is added anyway, potentially exceeding the limit.
Compare this to UserCoreRepository (line 110-113), which throws an exception when an entity is not found. Align softDeleteBy with this pattern by either:
- Throwing an exception when the entity doesn't exist
- Returning a Boolean and validating success before proceeding with device creation
π€ Prompt for AI Agents
In
@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceCoreRepository.kt
around lines 51 - 58, The softDeleteBy method in UserDeviceCoreRepository
currently no-ops if the entity isn't found, allowing UserDeviceAppender to
proceed and potentially violate the 5-device limit; change softDeleteBy to
either throw when the id is missing or return a Boolean success flag and update
the caller to check it: in UserDeviceCoreRepository.update softDeleteBy(Long id)
either retrieve with userDeviceJpaRepository.findById(id).orElseThrow {
EntityNotFoundException("UserDevice $id not found") } and then call softDelete()
and save, or change its signature to boolean softDeleteBy(Long id) that returns
true when entity was found/soft-deleted and false otherwise; then modify
UserDeviceAppender to call the new softDeleteBy and abort/throw if it returns
false (or let the thrown exception propagate) so deletion is validated before
creating a new device.
| @Entity | ||
| @Table(name = "t_user_device") | ||
| class UserDeviceEntity( | ||
| private val userId: Long, | ||
| private val userKey: String, | ||
| private val fcmToken: String, | ||
| private val deviceId: String, | ||
| ) : BaseEntity() { |
There was a problem hiding this comment.
Add unique constraint on (userId, deviceId) to prevent duplicate device records.
Without a database-level unique constraint, the same device can be registered multiple times for a user, especially under concurrent requests. This breaks the upsert semantics and makes the 5-device limit unenforceable.
π§ Proposed fix: add unique constraint
+import jakarta.persistence.UniqueConstraint
+
@Entity
-@Table(name = "t_user_device")
+@Table(
+ name = "t_user_device",
+ uniqueConstraints = [
+ UniqueConstraint(
+ name = "uk_user_device_user_id_device_id",
+ columnNames = ["user_id", "device_id"]
+ )
+ ]
+)
class UserDeviceEntity(
private val userId: Long,
private val userKey: String,
private val fcmToken: String,
private val deviceId: String,
) : BaseEntity() {Also ensure the corresponding database migration adds this constraint.
π 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.
| @Entity | |
| @Table(name = "t_user_device") | |
| class UserDeviceEntity( | |
| private val userId: Long, | |
| private val userKey: String, | |
| private val fcmToken: String, | |
| private val deviceId: String, | |
| ) : BaseEntity() { | |
| import jakarta.persistence.UniqueConstraint | |
| @Entity | |
| @Table( | |
| name = "t_user_device", | |
| uniqueConstraints = [ | |
| UniqueConstraint( | |
| name = "uk_user_device_user_id_device_id", | |
| columnNames = ["user_id", "device_id"] | |
| ) | |
| ] | |
| ) | |
| class UserDeviceEntity( | |
| private val userId: Long, | |
| private val userKey: String, | |
| private val fcmToken: String, | |
| private val deviceId: String, | |
| ) : BaseEntity() { |
π€ Prompt for AI Agents
In
@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceEntity.kt
around lines 8 - 15, The entity t_user_device (UserDeviceEntity) lacks a
DB-level unique constraint on (userId, deviceId), allowing duplicate device rows
and breaking upsert/limit logic; add a unique constraint annotation to
UserDeviceEntity (on table name "t_user_device") enforcing uniqueness of the
userId and deviceId columns and create a corresponding migration that adds a
unique index/constraint on (user_id, device_id) in the schema so concurrent
inserts cannot create duplicates.
| interface UserDeviceJpaRepository : | ||
| JpaRepository<UserDeviceEntity, Long>, | ||
| KotlinJdslJpqlExecutor { | ||
| fun findAllByDeletedAtIsNull(): List<UserDeviceEntity> | ||
|
|
||
| fun findAllByUserKeyAndDeletedAtIsNull(userKey: String): List<UserDeviceEntity> | ||
|
|
||
| fun findAllByUserIdAndDeletedAtIsNull(userId: Long): List<UserDeviceEntity> | ||
| } |
There was a problem hiding this comment.
Add query methods to support upsert and device limit enforcement.
The repository is missing key query methods needed for the FCM token upsert feature:
- For upsert: Need to find an existing device by
(userId, deviceId)to determine whether to insert or update - For device limit: Need to count devices per user to enforce the 5-device maximum mentioned in the AI summary
Without these methods, the implementation must fetch all devices for a user and filter/count in memory, which is inefficient and error-prone.
β Proposed additional methods
interface UserDeviceJpaRepository :
JpaRepository<UserDeviceEntity, Long>,
KotlinJdslJpqlExecutor {
fun findAllByDeletedAtIsNull(): List<UserDeviceEntity>
fun findAllByUserKeyAndDeletedAtIsNull(userKey: String): List<UserDeviceEntity>
fun findAllByUserIdAndDeletedAtIsNull(userId: Long): List<UserDeviceEntity>
+
+ fun findByUserIdAndDeviceIdAndDeletedAtIsNull(userId: Long, deviceId: String): UserDeviceEntity?
+
+ fun countByUserIdAndDeletedAtIsNull(userId: Long): Long
}π 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.
| interface UserDeviceJpaRepository : | |
| JpaRepository<UserDeviceEntity, Long>, | |
| KotlinJdslJpqlExecutor { | |
| fun findAllByDeletedAtIsNull(): List<UserDeviceEntity> | |
| fun findAllByUserKeyAndDeletedAtIsNull(userKey: String): List<UserDeviceEntity> | |
| fun findAllByUserIdAndDeletedAtIsNull(userId: Long): List<UserDeviceEntity> | |
| } | |
| interface UserDeviceJpaRepository : | |
| JpaRepository<UserDeviceEntity, Long>, | |
| KotlinJdslJpqlExecutor { | |
| fun findAllByDeletedAtIsNull(): List<UserDeviceEntity> | |
| fun findAllByUserKeyAndDeletedAtIsNull(userKey: String): List<UserDeviceEntity> | |
| fun findAllByUserIdAndDeletedAtIsNull(userId: Long): List<UserDeviceEntity> | |
| fun findByUserIdAndDeviceIdAndDeletedAtIsNull(userId: Long, deviceId: String): UserDeviceEntity? | |
| fun countByUserIdAndDeletedAtIsNull(userId: Long): Long | |
| } |
π€ Prompt for AI Agents
In
@pida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/user/device/UserDeviceJpaRepository.kt
around lines 6 - 14, The repository lacks DB-level queries for upsert and
device-limit enforcement: add methods to UserDeviceJpaRepository such as
findByUserIdAndDeviceIdAndDeletedAtIsNull(userId: Long, deviceId: String):
UserDeviceEntity? to locate an existing device for upsert, and
countByUserIdAndDeletedAtIsNull(userId: Long): Long to get the device count for
enforcing the 5-device limit; use these methods instead of fetching and
filtering all devices in memory.
There was a problem hiding this comment.
Actionable comments posted: 1
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
pida-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/UserController.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-core/core-api/src/main/kotlin/com/pida/presentation/v1/user/UserController.kt (2)
6-7: LGTM! Dependencies properly added.The new imports and service injection are correctly structured for the FCM token endpoint.
Also applies to: 13-13, 21-21, 28-28
67-67: The nullabledeviceIdis intentionally handled. When null,UserMobileDeviceRequest.toCreate()generates a UUID viadeviceId ?: UUID.randomUUID().toString(), which is the expected behavior. Additionally, theappendFcmTokenendpoint lacks thesuspendmodifier becauseUserDeviceService.append()is a synchronous operation, unlikeme()andupdateNickname()which call async operations. No action is needed.Likely an incorrect or invalid review comment.
| userDeviceService.append(request.toCreate(user, deviceId)) | ||
| return UserMobileDeviceResponse("FcmToken λ±λ‘ μλ£") |
There was a problem hiding this comment.
Hardcoded Korean message and missing async handling.
Two concerns:
-
Minor i18n issue (line 71): The response message is hardcoded in Korean ("FcmToken λ±λ‘ μλ£"), which prevents internationalization. Consider using a message resource bundle or an English default message.
-
Optional consistency improvement (line 65): The function is not marked
suspend, unlike other endpoints in this controller (me,updateNickname). IfuserDeviceService.append()performs I/O operations (database writes), consider making this functionsuspendfor consistency and non-blocking behavior.
π Proposed fix for i18n
- return UserMobileDeviceResponse("FcmToken λ±λ‘ μλ£")
+ return UserMobileDeviceResponse("FCM token registered successfully")For the suspend modifier:
- fun appendFcmToken(
+ suspend fun appendFcmToken(π 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.
| userDeviceService.append(request.toCreate(user, deviceId)) | |
| return UserMobileDeviceResponse("FcmToken λ±λ‘ μλ£") | |
| userDeviceService.append(request.toCreate(user, deviceId)) | |
| return UserMobileDeviceResponse("FCM token registered successfully") |
π± κ΄λ ¨ μ΄μ
π μμ λ΄μ© λ° νΉμ΄ μ¬ν
π μ°Έκ³
π μ²΄ν¬ λ¦¬μ€νΈ
Summary by CodeRabbit
βοΈ Tip: You can customize this high-level summary in your review settings.