Conversation
WalkthroughThis change introduces update and delete capabilities for school entities, adding corresponding service methods, controller endpoints, and a validated DTO for school edits. The domain entity gains an update method to apply changes from the DTO. The controller now exposes PUT and DELETE endpoints for modifying and removing schools. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SchoolController
participant SchoolService
participant SchoolJpaEntity
participant SchoolRepository
Client->>SchoolController: PUT /school/{id} (SchoolEditRequest)
SchoolController->>SchoolService: update(id, SchoolEditRequest)
SchoolService->>SchoolRepository: findById(id)
SchoolRepository-->>SchoolService: SchoolJpaEntity
SchoolService->>SchoolJpaEntity: updateInfo(SchoolEditRequest)
SchoolService-->>SchoolController: (void)
SchoolController-->>Client: 200 OK
Client->>SchoolController: DELETE /school/{id}
SchoolController->>SchoolService: deleteSchool(id)
SchoolService->>SchoolRepository: existsById(id)
alt School exists
SchoolService->>SchoolRepository: deleteById(id)
else School not found
SchoolService-->>SchoolController: throw CustomRuntimeException
end
SchoolController-->>Client: 200 OK
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jbh010204, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request implements new functionality for managing school records by introducing API endpoints and corresponding service and domain logic to allow for the update and deletion of existing school entities. It enhances the current school management capabilities by providing a structured way to modify school details and remove records from the system.
Highlights
- School Management API Endpoints: New REST API endpoints have been added to
SchoolControllerto support updating and deleting school records. Specifically, aPUT /{id}endpoint for updates and aDELETE /{id}endpoint for deletions are now available. - School Service Business Logic: The
SchoolServicenow includesdeleteSchool(Long id)andupdate(Long schoolId, SchoolEditRequest request)methods. These methods encapsulate the business logic for removing and modifying school entities, including validation to ensure the school exists before performing the operation. - School Entity Update Method: A new
updateInfo(SchoolEditRequest request)method has been added to theSchoolJpaEntity. This method allows the entity to update its own fields based on the providedSchoolEditRequestDTO, promoting encapsulation and domain-driven design principles. - New School Edit Request DTO: A new data transfer object,
SchoolEditRequest.java, has been introduced. This DTO defines the structure and validation rules for the data required when updating an existing school record, including fields for school name, address, exam date, lunch details, and capacity.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to update and delete school information. The changes include new controller endpoints, service methods, and an update method in the entity. The review focuses on improving efficiency in the delete operation, highlighting a critical security issue with disabled authorization, and suggesting improvements for validation consistency in the new DTO.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/java/life/mosu/mosuserver/application/school/SchoolService.java (1)
36-43: Optimize delete operation to avoid double database call.The current implementation makes two database calls - one to check existence and another to delete. This can be optimized to a single operation.
Consider using a single delete operation and checking the result:
@Transactional public void deleteSchool(Long id) { - if (!schoolJpaRepository.existsById(id)) { - throw new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND); - } - - schoolJpaRepository.deleteById(id); + SchoolJpaEntity school = schoolJpaRepository.findById(id) + .orElseThrow(() -> new CustomRuntimeException(ErrorCode.SCHOOL_NOT_FOUND)); + + schoolJpaRepository.delete(school); }Or use a custom repository method that returns the number of deleted entities and check if it's zero.
src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolEditRequest.java (1)
37-39: Add validation message for consistency.The
deadlineTimefield is missing a validation message for consistency with other fields.-@NotNull +@NotNull(message = "신청 마감 일시는 필수입니다.") @Schema(description = "신청 마감 일시", example = "2025-05-30T10:00:00") LocalDateTime deadlineTime,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/life/mosu/mosuserver/application/school/SchoolService.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/school/SchoolController.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolEditRequest.java(1 hunks)
🔇 Additional comments (3)
src/main/java/life/mosu/mosuserver/application/school/SchoolService.java (1)
45-52: Well-implemented update method with proper error handling.The update method correctly:
- Uses
@Transactionalfor proper transaction management- Handles entity not found cases with appropriate exceptions
- Leverages JPA dirty checking for automatic persistence
- Follows clean architecture principles
src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolEditRequest.java (1)
11-46: Well-structured DTO with comprehensive validation.The record effectively uses:
- Proper validation annotations with descriptive messages
- Comprehensive Swagger documentation
- Appropriate data types for each field
- Clean record syntax
src/main/java/life/mosu/mosuserver/presentation/school/SchoolController.java (1)
45-53: Well-implemented REST endpoints with proper HTTP semantics.The endpoints correctly:
- Use appropriate HTTP methods (PUT for update, DELETE for delete)
- Return proper HTTP status codes
- Follow consistent response structure with
ApiResponseWrapper- Handle path parameters appropriately
- Provide meaningful success messages
Also applies to: 55-62
| import java.time.LocalDate; | ||
| import java.time.LocalDateTime; | ||
| import life.mosu.mosuserver.domain.application.Lunch; | ||
| import life.mosu.mosuserver.presentation.school.dto.SchoolEditRequest; |
There was a problem hiding this comment.
Architectural violation: Domain layer depends on presentation layer.
The domain entity should not import DTOs from the presentation layer. This violates the dependency inversion principle and creates tight coupling between layers.
Consider creating a domain-specific update command or moving the update logic to the service layer:
-import life.mosu.mosuserver.presentation.school.dto.SchoolEditRequest;Move the update logic to the service layer or create a domain command object instead.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java at line
16, remove the import of SchoolEditRequest from the presentation layer to avoid
domain layer depending on presentation. Refactor by either moving the update
logic that uses this DTO into the service layer or create a domain-specific
command object within the domain layer to handle updates, ensuring the domain
entity does not directly depend on presentation DTOs.
| public void updateInfo(SchoolEditRequest request) { | ||
| this.schoolName = request.schoolName(); | ||
| this.area = Area.from(request.area()); | ||
| this.address = request.schoolAddress().toValueObject(); | ||
| this.examDate = request.examDate(); | ||
| this.lunch = Lunch.from(request.lunch()); | ||
| this.lunchPrice = request.lunchPrice(); | ||
| this.deadlineTime = request.deadlineTime(); | ||
| this.capacity = request.capacity(); | ||
| } |
There was a problem hiding this comment.
Missing validation and business logic in update method.
The updateInfo method performs direct field assignment without any validation, null checks, or business logic. This could lead to invalid entity states.
Consider adding validation and business logic:
public void updateInfo(SchoolEditRequest request) {
+ if (request == null) {
+ throw new IllegalArgumentException("Request cannot be null");
+ }
+
+ // Add business validation here
+ if (request.capacity() != null && request.capacity() <= 0) {
+ throw new IllegalArgumentException("Capacity must be positive");
+ }
+
this.schoolName = request.schoolName();
this.area = Area.from(request.area());
this.address = request.schoolAddress().toValueObject();
this.examDate = request.examDate();
this.lunch = Lunch.from(request.lunch());
this.lunchPrice = request.lunchPrice();
this.deadlineTime = request.deadlineTime();
this.capacity = request.capacity();
}Alternatively, consider moving this logic to the service layer to maintain better separation of concerns.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void updateInfo(SchoolEditRequest request) { | |
| this.schoolName = request.schoolName(); | |
| this.area = Area.from(request.area()); | |
| this.address = request.schoolAddress().toValueObject(); | |
| this.examDate = request.examDate(); | |
| this.lunch = Lunch.from(request.lunch()); | |
| this.lunchPrice = request.lunchPrice(); | |
| this.deadlineTime = request.deadlineTime(); | |
| this.capacity = request.capacity(); | |
| } | |
| public void updateInfo(SchoolEditRequest request) { | |
| if (request == null) { | |
| throw new IllegalArgumentException("Request cannot be null"); | |
| } | |
| // Add business validation here | |
| if (request.capacity() != null && request.capacity() <= 0) { | |
| throw new IllegalArgumentException("Capacity must be positive"); | |
| } | |
| this.schoolName = request.schoolName(); | |
| this.area = Area.from(request.area()); | |
| this.address = request.schoolAddress().toValueObject(); | |
| this.examDate = request.examDate(); | |
| this.lunch = Lunch.from(request.lunch()); | |
| this.lunchPrice = request.lunchPrice(); | |
| this.deadlineTime = request.deadlineTime(); | |
| this.capacity = request.capacity(); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/school/SchoolJpaEntity.java around
lines 71 to 80, the updateInfo method directly assigns fields from the request
without validation or business logic, risking invalid entity states. To fix
this, add appropriate null checks and validation for each field before
assignment, and enforce any business rules relevant to the entity's state.
Alternatively, refactor to move validation and business logic to the service
layer, keeping the entity update method focused on state changes only.
| @Schema(description = "도시락 메뉴") | ||
| @NotNull | ||
| String lunch, | ||
|
|
||
| @Schema(description = "도시락 가격") | ||
| @NotNull | ||
| Integer lunchPrice, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve validation for lunch and lunchPrice fields.
The lunch field should use @NotBlank instead of @NotNull for string validation, and lunchPrice should have @Positive constraint to prevent negative values.
Apply these validation improvements:
@Schema(description = "도시락 메뉴")
-@NotNull
+@NotBlank(message = "도시락 메뉴는 필수입니다.")
String lunch,
@Schema(description = "도시락 가격")
-@NotNull
+@NotNull(message = "도시락 가격은 필수입니다.")
+@Positive(message = "도시락 가격은 양수여야 합니다.")
Integer lunchPrice,📝 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 = "도시락 메뉴") | |
| @NotNull | |
| String lunch, | |
| @Schema(description = "도시락 가격") | |
| @NotNull | |
| Integer lunchPrice, | |
| @Schema(description = "도시락 메뉴") | |
| @NotBlank(message = "도시락 메뉴는 필수입니다.") | |
| String lunch, | |
| @Schema(description = "도시락 가격") | |
| @NotNull(message = "도시락 가격은 필수입니다.") | |
| @Positive(message = "도시락 가격은 양수여야 합니다.") | |
| Integer lunchPrice, |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/school/dto/SchoolEditRequest.java
around lines 29 to 35, replace the @NotNull annotation on the lunch field with
@NotBlank to ensure the string is not empty or just whitespace. For the
lunchPrice field, add the @Positive annotation to enforce that the price must be
a positive integer, preventing negative values. This improves validation by
ensuring lunch is a meaningful string and lunchPrice is a valid positive number.
| } | ||
|
|
||
| @PutMapping("/{id}") | ||
| // @PreAuthorize("isAuthenticated() and hasRole('ADMIN')") |
There was a problem hiding this comment.
Security vulnerability: Authentication and authorization disabled.
The security annotations are commented out, which means these endpoints are accessible without authentication or authorization. This is a critical security issue for update and delete operations.
Uncomment and verify the security configuration:
@PutMapping("/{id}")
-// @PreAuthorize("isAuthenticated() and hasRole('ADMIN')")
+@PreAuthorize("isAuthenticated() and hasRole('ADMIN')")
public ResponseEntity<ApiResponseWrapper<Void>> update(@DeleteMapping("/{id}")
-// @PreAuthorize("isAuthenticated() and hasRole('ADMIN')")
+@PreAuthorize("isAuthenticated() and hasRole('ADMIN')")
public ResponseEntity<ApiResponseWrapper<Void>> delete(Ensure that the security configuration is properly set up to handle these annotations.
Also applies to: 56-56
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/school/SchoolController.java
at lines 46 and 56, the @PreAuthorize annotations are commented out, disabling
authentication and authorization for critical endpoints. Uncomment these
annotations to enforce security checks, and verify that the overall security
configuration supports these annotations to properly restrict access to update
and delete operations.
| public ResponseEntity<ApiResponseWrapper<Void>> update( | ||
| @RequestBody SchoolEditRequest request, | ||
| @PathVariable Long id |
There was a problem hiding this comment.
Missing request validation annotation.
The @Valid annotation is missing from the request parameter, which means the validation constraints in SchoolEditRequest won't be enforced.
Add validation annotation:
public ResponseEntity<ApiResponseWrapper<Void>> update(
- @RequestBody SchoolEditRequest request,
+ @RequestBody @Valid SchoolEditRequest request,
@PathVariable Long id
) {Don't forget to add the import for @Valid:
+import jakarta.validation.Valid;🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/school/SchoolController.java
around lines 47 to 49, the @Valid annotation is missing on the SchoolEditRequest
parameter in the update method, so validation constraints won't be enforced. Add
the @Valid annotation before the @RequestBody annotation on the request
parameter and import javax.validation.Valid to enable validation of the incoming
request.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit