[BE-Feat] 논의 탈퇴 기능구현 #388
[BE-Feat] 논의 탈퇴 기능구현 #388
Conversation
WalkthroughThis pull request introduces several new methods and modifications across repository, service, and test classes to improve discussion participant management. The changes add deletion and offset-retrieval methods in the participant repository, update participant addition logic in the service layer, and implement a new exit discussion flow that leverages pessimistic locking. Furthermore, a new Redis method is introduced for bitmap manipulation, along with corresponding tests to validate the changes. Changes
Sequence Diagram(s)Join Discussion FlowsequenceDiagram
participant C as Client
participant DS as DiscussionService
participant DR as DiscussionRepository
participant DPS as DiscussionParticipantService
C->>DS: joinDiscussion(discussionId, request)
DS->>DR: findByIdForUpdate(discussionId)
DR-->>DS: Discussion entity
DS->>DPS: addDiscussionParticipant(discussion, user)
DPS-->>DS: Assigned offset
DS-->>C: JoinDiscussionResponse
Exit Discussion FlowsequenceDiagram
participant C as Client
participant DS as DiscussionService
participant DR as DiscussionRepository
participant DPS as DiscussionParticipantService
participant DBS as DiscussionBitmapService
C->>DS: exitDiscussion(discussionId, userId)
DS->>DR: findByIdForUpdate(discussionId)
DR-->>DS: Discussion entity
DS->>DPS: getDiscussionParticipantOffset(discussionId, userId)
DPS-->>DS: Participant offset
DS->>DBS: deleteUsersFromDiscussion(discussionId, offset)
DBS-->>DS: Acknowledgement
DS->>DPS: deleteDiscussionParticipant(discussionId, userId)
DPS-->>DS: Deletion acknowledgement
DS-->>C: Exit confirmation
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| .isHost(false) | ||
| .userOffset(offset) | ||
| .build(); | ||
| long offset = 0L; |
There was a problem hiding this comment.
p3; offsets가 오름차순으로 정렬되어있는데 contains를 여러번 수행하지 않고 offsets에 대해 반복을 수행하며 찾는건 어떤가요?
offsets에 대해서만 반복을 수행하도록 수정
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java (1)
322-324: Consider adding validation before deletionThe new
deleteDiscussionParticipantmethod directly calls the repository's delete method without first verifying if the participant exists. Consider adding validation to provide better feedback when attempting to delete a non-existent participant.public void deleteDiscussionParticipant(Long discussionId, Long userId) { + // Validate if the participant exists before deletion + discussionParticipantRepository.findOffsetByDiscussionIdAndUserId(discussionId, userId) + .orElseThrow(() -> new ApiException(ErrorCode.DISCUSSION_PARTICIPANT_NOT_FOUND)); discussionParticipantRepository.deleteByDiscussionIdAndUserId(discussionId, userId); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java(3 hunks)
🔇 Additional comments (5)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java (5)
45-48: Good optimization in participant verificationChanged from using a maximum offset to retrieving a complete list of offsets, which allows for better reuse of previously assigned offsets. This approach efficiently handles cases where participants have left the discussion.
52-56: Efficient implementation of finding the first available offsetThe algorithm correctly identifies the first available offset by iterating through the sorted list of existing offsets. This aligns with the previous review feedback about avoiding redundant
containschecks.
58-65: Clean, simplified participant creation logicThe host determination is now more straightforward (checking if offset is 0), and the builder pattern for participant creation is clear and well-structured.
18-18:Details
❓ Verification inconclusive
Verify the new import's usage in this class
Added import for
DiscussionBitmapService, but I don't see any direct usage of this service in the current file. Is this needed for future functionality, or is it being used in one of the methods?
🏁 Script executed:
#!/bin/bash # Check if DiscussionBitmapService is used elsewhere in this class rg "discussionBitmapService" backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.javaLength of output: 120
Unused Import in DiscussionParticipantService Detected—Please Confirm Its Intent
After running a case-sensitive search, I found no references to the
DiscussionBitmapService(or any variant in casing) in the filebackend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java. This suggests that the newly added import is currently unused. Please verify whether it is reserved for future functionality or if it can be removed to keep the code clean.
- File:
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantService.java- Concern: The import for
DiscussionBitmapServicedoes not appear to be utilized anywhere in this class.- Action: Confirm if this is an intentional placeholder for upcoming features. If not, consider removing the import to reduce unused dependencies.
322-324:Details
❓ Verification inconclusive
Verify transaction handling for discussion withdrawal
The PR objectives mention implementing pessimistic locking for concurrency issues. I don't see any explicit locking mechanism in this method. Ensure that proper locking is implemented either at the repository level or in a higher-level service that calls this method.
🏁 Script executed:
#!/bin/bash # Check if there's pessimistic locking implemented in the repository or related service echo "Checking for pessimistic locking in repository methods:" rg -A 3 "Lock|lock" backend/src/main/java/endolphin/backend/domain/discussion/DiscussionParticipantRepository.java echo -e "\nChecking for transaction management with pessimistic locking in service layers:" rg -A 3 "Pessimistic|PESSIMISTIC" backend/src/main/java/endolphin/backend/domain/discussion/Length of output: 1400
Ensure Transactional Consistency for Discussion Withdrawal
The deletion method in
DiscussionParticipantService(lines 322–324) currently does not introduce explicit locking. However, our investigation shows that pessimistic locking is applied in the discussion flow via theDiscussionRepository(specifically, thefindByIdForUpdatemethod is annotated with@Lock(LockModeType.PESSIMISTIC_WRITE)). This indicates that the broader discussion withdrawal process is expected to enforce concurrency control at a higher level.
- Confirm that any workflow invoking
deleteDiscussionParticipantis executed within a transactional context that acquires the pessimistic lock (e.g., via a call tofindByIdForUpdate).- If the deletion can occur independently (outside that transactional boundary), consider either:
- Annotating the method (or its calling service) with
@Transactional, or- Integrating explicit locking at the repository level for participant deletion.
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
Summary by CodeRabbit
New Features
Bug Fixes
Tests