[BE] 질문 체크 기능 개선 #218
[BE] 질문 체크 기능 개선 #218
Conversation
- 데이터베이스에 조회한 뒤 업데이트하는 과정을 하나의 트랜잭션으로 처리하여 일관성 보장
WalkthroughThis pull request refactors the question and request handling logic. In the question domain, service methods have been modified to delegate validations and state updates to dedicated methods, with redundant private methods removed. In the request domain, a new service class has been introduced and transaction management added to repository operations. Controller methods have been updated to simplify parameter handling, and dependencies have been refactored to shift responsibilities across services. Changes
Sequence Diagram(s)sequenceDiagram
participant SC as StudentQuestionController
participant SS as StudentQuestionService
participant QS as QuestionService
participant DB as Database (QuestionRepository)
SC->>SS: sendQuestionCheck(courseId, questionId)
SS->>QS: checkQuestion(questionId)
QS->>DB: Retrieve question & validate course status
DB-->>QS: Return Question
QS-->>SS: Return updated Question
SS-->>SC: Complete operation
sequenceDiagram
participant SR as StudentRequestService
participant RS as RequestService
participant CR as CourseRepository
participant DB as RequestRepository
SR->>RS: sendRequest(requestPayload)
RS->>CR: getCourse(courseId)
CR-->>RS: Return Course
RS->>RS: checkIfOpen(Course)
RS->>DB: incrementCount(Course, type)
DB-->>RS: Return updated count
RS-->>SR: Acknowledge success
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
back-end/reacton/src/main/java/com/softeer/reacton/domain/request/RequestRepository.java (1)
13-20: Good addition of transaction management!The @transactional annotation ensures that the increment operation is executed atomically, preventing potential race conditions during concurrent updates. This aligns well with the PR's objective of improving transaction handling.
Note: Since this is a modifying query, the @transactional annotation is particularly important as it ensures:
- Atomic updates to prevent partial modifications
- Proper transaction boundaries for database consistency
- Automatic rollback on exceptions
back-end/reacton/src/main/java/com/softeer/reacton/domain/request/RequestService.java (2)
8-10: Consider aligning with Spring's standard transactional usage.You currently use the
jakarta.transaction.Transactionalannotation. If your project relies primarily on Spring's transaction management, adoptingorg.springframework.transaction.annotation.Transactionalthroughout can enhance consistency and avoid confusion.Also applies to: 13-13, 15-15
21-32: Review concurrency handling and fallback behavior.Since this method increments a counter, concurrent invocations might cause race conditions if the underlying DB logic doesn’t handle locking. Also, if no existing request is found, the current implementation throws an exception rather than inserting a new record, which could be intentional or may warrant a reconsideration of the fallback behavior.
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionService.java (1)
22-30: Potential concurrency consideration in checkQuestion method.
checkQuestionsetsisCompleteon the question within a transaction. If multiple calls attempt to check the same question concurrently, ensure consistency. A simple approach is optimistic locking or verifyingisCompleteis not already set.back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionController.java (1)
81-92: Update API documentation to reflect transaction behavior.Since this PR focuses on improving transaction handling during question checking, consider adding a note about transaction behavior in the API documentation.
Add transaction-related details to the API documentation:
@Operation( summary = "학생 질문 체크 전송", description = "학생이 교수에게 질문 체크를 전송합니다.", + description = "학생이 교수에게 질문 체크를 전송합니다. 모든 데이터베이스 작업은 단일 트랜잭션으로 처리됩니다.", responses = { @ApiResponse(responseCode = "200", description = "성공적으로 전송했습니다."), @ApiResponse(responseCode = "404", description = "수업을 찾을 수 없습니다."), @ApiResponse(responseCode = "404", description = "질문을 찾을 수 없습니다."), @ApiResponse(responseCode = "409", description = "아직 수업이 시작되지 않았습니다."), @ApiResponse(responseCode = "500", description = "서버와의 연결에 실패했습니다.") } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java(0 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionController.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionService.java(3 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/request/RequestRepository.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/request/RequestService.java(1 hunks)back-end/reacton/src/main/java/com/softeer/reacton/domain/request/StudentRequestService.java(1 hunks)
💤 Files with no reviewable changes (1)
- back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionRepository.java
🧰 Additional context used
🧠 Learnings (1)
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionService.java (1)
Learnt from: sunohkim
PR: softeer5th/Team3-PowerPenguin#174
File: back-end/reacton/src/main/java/com/softeer/reacton/domain/question/dto/QuestionSendRequest.java:0-0
Timestamp: 2025-02-19T08:11:33.101Z
Learning: QuestionSendRequest DTO requires validation: content must not be empty (@NotBlank) and must not exceed 1000 characters (@Size).
🔇 Additional comments (18)
back-end/reacton/src/main/java/com/softeer/reacton/domain/request/RequestRepository.java (1)
8-8: LGTM!Clean addition of the required import for @transactional annotation.
back-end/reacton/src/main/java/com/softeer/reacton/domain/request/RequestService.java (4)
3-7: Looks good.These imports appropriately reflect the new domain models and custom error codes, and no issues are apparent here.
18-19: Dependency injection approach is appropriate.Using Lombok's
@AllArgsConstructorwith final fields is clean and ensures proper dependency injection without requiring boilerplate constructors.
34-37: No issues found in getCourse.Implementing a straightforward lookup with a custom exception on absence follows best practices.
39-43: No issues found in checkIfOpen.This concise method cleanly enforces the active-course requirement.
back-end/reacton/src/main/java/com/softeer/reacton/domain/request/StudentRequestService.java (1)
17-17: Looks good, but ensure proper testing of the new service dependency.Replacing direct repository references with
RequestServiceimproves maintainability, but please confirm you have adequate unit tests covering this new dependency injection.back-end/reacton/src/main/java/com/softeer/reacton/domain/question/ProfessorQuestionService.java (1)
21-21: Consider explicit error handling for exceptions.
checkQuestionmay throw an exception if the question does not exist or the course is inactive. Ensure higher-level code gracefully handles these exceptions, returning meaningful error responses or logs for debugging.back-end/reacton/src/main/java/com/softeer/reacton/domain/question/QuestionService.java (6)
3-7: Imports aligned with new functionalities.These imports accurately reflect the new exception and domain references needed for
CourseandCourseErrorCode.
10-13: Good practice: logging enabled with @slf4j.Enabling detailed debug logs is beneficial for monitoring question workflows, especially for debugging transaction issues.
18-18: Additional dependency injection recognized.The injection of
CourseRepositoryensures thatQuestionServicecan manage both question and course data.
47-50: Clear handling of non-existent questions.Throwing a
BaseExceptionwith a specific error code is a good practice to standardize error handling.
52-55: Consistent approach for missing course.Similar to
getQuestion, throwing aBaseExceptionensures consistent error messages if the course is not found.
57-60: Blocking inactive course usage.Raising a
BaseExceptionfor inactive courses enforces the rule that only active courses can be used.back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionService.java (4)
29-29: Integration with QuestionService recognized.Delegating question handling to
QuestionServiceenhances maintainability and consolidates logic for retrieving and saving questions.
48-50: Well-structured response building.Constructing
CourseQuestionResponsedirectly from the saved question is clear and concise, avoiding duplication of fields.
61-61: Method signature clarified.The removal of
studentIdfrom method parameters simplifies the check flow, relying on internal lookups inQuestionService.
64-64: ReusingcheckQuestionlogic in StudentQuestionService.This centralizes question validation and completion logic, reducing code duplication and risk of inconsistent behavior.
back-end/reacton/src/main/java/com/softeer/reacton/domain/question/StudentQuestionController.java (1)
100-100:Details
❓ Verification inconclusive
LGTM! Architectural improvement in student identification.
The removal of
studentIdparameter fromsendQuestionCheckcall suggests a good architectural decision to handle student identification within the service layer, promoting better separation of concerns.Let's verify that the service layer properly handles student identification:
🏁 Script executed:
#!/bin/bash # Search for student identification handling in the service layer ast-grep --pattern $'class StudentQuestionService { $$$ sendQuestionCheck($$$) { $$$ } $$$ }'Length of output: 106
Let's try a broader search for the implementation of
sendQuestionCheckin the service layer. This will help us verify if the service method now handles student identification internally. Please run the following script, which usesrgto search across Java files for references tosendQuestionCheck:#!/bin/bash # Search for occurrences of "sendQuestionCheck" in Java files to locate its implementation and review context. rg "sendQuestionCheck" --type javaAfter getting the output from this command, we can assess whether the service layer properly integrates student identification internally or if further adjustments are needed. Please provide feedback on the output so we can finalize the review comment.
[BE] 질문 체크 기능 개선
#️⃣ 연관된 이슈
#182
📝 작업 내용
질문 체크 과정에서 발생한 트랜잭션 이슈 해결
기타 실시간 수업 중 SSE 통신을 포함하는 API에 대한 트랜잭션 이슈 해결
💬 리뷰 요구사항(선택)
Summary by CodeRabbit
New Features
Refactor