MOSU-155 refactor: 엔티티 softDelete 적용 및 cursor 기반 페이지네이션 구현 #164
MOSU-155 refactor: 엔티티 softDelete 적용 및 cursor 기반 페이지네이션 구현 #164chominju02 merged 10 commits intodevelopfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces cursor-based pagination for event listings, adds soft-delete support to numerous entities, and removes or disables attachment-related logic for events and notices. Several domain classes are refactored to change their inheritance hierarchy, and new classes and annotations are added to support cursor pagination. Attachment entities and services for events are commented out or deleted, and security is tightened for exam-related endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EventController
participant EventService
participant EventQueryRepository
participant CursorResponse
Client->>EventController: GET /events?id={cursor}
EventController->>EventService: getEvents(cursor.id)
EventService->>EventQueryRepository: findAllByCursorId(cursor.id)
EventQueryRepository-->>EventService: Slice<EventJpaEntity>
EventService->>CursorResponse: of(slice, nextCursor)
CursorResponse-->>EventService: CursorResponse<EventResponse>
EventService-->>EventController: CursorResponse<EventResponse>
EventController-->>Client: ApiResponseWrapper<CursorResponse<EventResponse>>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (51)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 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 @chominju02, 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!
I've refactored the application to introduce a comprehensive soft delete mechanism across various entities, centralizing the logic for better maintainability. Concurrently, I've implemented cursor-based pagination for event listings, which significantly improves performance for large datasets. Additionally, I've streamlined the handling of event attachments by embedding file information directly into the event entity, simplifying the data model and related service interactions. These changes contribute to a more robust and efficient backend system.
Highlights
- Centralized Soft Delete Implementation: I've implemented soft deletion across many core entities by centralizing the logic in a new
BaseDeleteEntity. This entity now handles both thedeletedflag and auditing timestamps (createdAt,updatedAt), allowingBaseTimeEntityto focus solely on general timestamps. This change impacts entities likeApplication,Event,Exam,Inquiry,Notice,Payment,Profile,Refund, andUser. - Migration to Cursor-Based Pagination for Events: I've refactored the event listing functionality to use cursor-based pagination instead of traditional offset-based pagination. This involved introducing new
Cursor,CursorResponse, andCursorParamcomponents, along with a dedicatedEventQueryRepositoryImplusing QueryDSL for efficient data retrieval. This approach is generally more performant for large datasets as it avoids the performance issues associated withOFFSETclauses. - Streamlined Event Attachment Management: I've streamlined how event attachments are handled. Instead of using separate
EventAttachmentJpaEntityandEventAttachmentService, the file name and S3 key for event images are now directly embedded within theEventJpaEntity. This simplifies the data model and reduces the overhead of managing separate attachment entities for events. - Enhanced Admin Functionality for Exams: I've added a new
DELETEendpoint for exams, allowing administrators to remove exam entries. This endpoint is secured with@PreAuthorizeto ensure only authenticated users with the 'ADMIN' role can perform this operation. - Code Cleanup and Refinement: I've performed a general cleanup of various attachment-related services and DTOs across the application, commenting out or removing deprecated code paths that are no longer needed due to the new attachment handling strategy and soft delete implementation.
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 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 or fill out our survey 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 is a significant refactoring that introduces soft-deletes across many entities and implements cursor-based pagination for the events list, which are great improvements. The overall implementation is solid, but there are a few critical areas that need attention. Several entities that were updated to support soft-deletes are missing the required @SoftDelete annotation, which will cause them to be hard-deleted. There is also a critical bug in the new CursorArgumentResolver that will prevent the cursor-based pagination from working correctly. Additionally, a number of files and code blocks have been commented out instead of being removed, which affects code clarity. I've provided specific comments and suggestions to address these points.
| String cursorJson = webRequest.getParameter("id"); | ||
|
|
||
| if (cursorJson == null || cursorJson.isBlank()) { | ||
| return Cursor.empty(); | ||
| } | ||
|
|
||
| return objectMapper.readValue(cursorJson, Cursor.class); |
There was a problem hiding this comment.
The implementation of resolveArgument is incorrect and will cause a runtime exception. webRequest.getParameter("id") returns a raw string value (e.g., "123"), not a JSON string. objectMapper.readValue will fail when trying to parse this plain string into a Cursor object. The implementation should parse the string to a Long and then create the Cursor object.
String cursorIdStr = webRequest.getParameter("id");
if (cursorIdStr == null || cursorIdStr.isBlank()) {
return Cursor.empty();
}
try {
return Cursor.of(Long.parseLong(cursorIdStr));
} catch (NumberFormatException e) {
throw new IllegalArgumentException("Invalid cursor 'id' format. Must be a number.", e);
}| // answerAttachmentService.deleteAttachment(answerEntity); | ||
| answerAttachmentService.createAttachment(request.attachments(), answerEntity); |
There was a problem hiding this comment.
The logic for updating attachments is incorrect after the changes. By commenting out answerAttachmentService.deleteAttachment(answerEntity), old attachments will no longer be removed when new ones are provided during an update. This will lead to an accumulation of attachments for the same answer over multiple updates, creating data inconsistencies. The line to delete old attachments should be restored to ensure that only the latest attachments are associated with the answer.
| // answerAttachmentService.deleteAttachment(answerEntity); | |
| answerAttachmentService.createAttachment(request.attachments(), answerEntity); | |
| answerAttachmentService.deleteAttachment(answerEntity); | |
| answerAttachmentService.createAttachment(request.attachments(), answerEntity); |
| @Entity | ||
| @Table(name = "application") | ||
| @Getter | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @SoftDelete | ||
| public class ApplicationJpaEntity extends BaseTimeEntity { | ||
| public class ApplicationJpaEntity extends BaseDeleteEntity { |
There was a problem hiding this comment.
The @SoftDelete annotation is missing from this entity. While it now extends BaseDeleteEntity, which provides the deleted flag, Hibernate's soft delete mechanism is only activated by the @SoftDelete annotation on the entity class itself. Without it, any delete operation on this entity will result in a hard delete from the database, which goes against the main goal of this pull request.
| @Entity | |
| @Table(name = "application") | |
| @Getter | |
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | |
| @SoftDelete | |
| public class ApplicationJpaEntity extends BaseTimeEntity { | |
| public class ApplicationJpaEntity extends BaseDeleteEntity { | |
| @Entity | |
| @Table(name = "application") | |
| @Getter | |
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | |
| @SoftDelete | |
| public class ApplicationJpaEntity extends BaseDeleteEntity { |
| public static String formatDate(LocalDateTime dateTime) { | ||
| return dateTime != null ? dateTime.toLocalDate().toString() : null; | ||
| } | ||
|
|
||
| public String getCreatedAt() { | ||
| return createdAt != null ? createdAt.format( | ||
| DateTimeFormatter.ofPattern("yyyy-MM-dd")) : null; | ||
| } |
There was a problem hiding this comment.
The formatDate and getCreatedAt methods are responsible for date formatting, which is a presentation-layer concern. To maintain a clean architecture and good separation of concerns, this logic should be moved out of the domain entity. Consider placing formatting logic within DTOs or a dedicated utility class.
| //package life.mosu.mosuserver.domain.event; | ||
| // | ||
| //import jakarta.persistence.Column; | ||
| //import jakarta.persistence.Entity; | ||
| //import jakarta.persistence.GeneratedValue; | ||
| //import jakarta.persistence.GenerationType; | ||
| //import jakarta.persistence.Id; | ||
| //import jakarta.persistence.Table; | ||
| //import life.mosu.mosuserver.domain.file.File; | ||
| //import life.mosu.mosuserver.domain.file.Visibility; | ||
| //import lombok.Builder; | ||
| //import lombok.Getter; | ||
| //import lombok.RequiredArgsConstructor; | ||
| // | ||
| //@Entity | ||
| //@RequiredArgsConstructor | ||
| //@Getter | ||
| //@Table(name = "event_attachment") | ||
| //public class EventAttachmentJpaEntity extends File { | ||
| // | ||
| // @Id | ||
| // @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| // @Column(name = "event_attachment_id", nullable = false) | ||
| // private Long id; | ||
| // | ||
| // @Column(name = "event_id", nullable = false) | ||
| // private Long eventId; | ||
| // | ||
| // @Builder | ||
| // public EventAttachmentJpaEntity(final String fileName, final String s3Key, | ||
| // final Visibility visibility, final Long eventId) { | ||
| // super(fileName, s3Key, visibility); | ||
| // this.eventId = eventId; | ||
| // } | ||
| // | ||
| // protected void updateFile() | ||
| // | ||
| //} |
There was a problem hiding this comment.
This entire file has been commented out. If this class is no longer needed as part of the refactoring, the file should be deleted from the project instead of being left as commented-out code. This improves codebase cleanliness and maintainability.
| //package life.mosu.mosuserver.domain.event; | ||
| // | ||
| //import java.util.Optional; | ||
| //import org.springframework.data.jpa.repository.JpaRepository; | ||
| // | ||
| //public interface EventAttachmentRepository extends JpaRepository<EventAttachmentJpaEntity, Long> { | ||
| // | ||
| // void deleteByEventId(Long eventId); | ||
| // | ||
| // Optional<EventAttachmentJpaEntity> findByEventId(Long eventId); | ||
| //} | ||
| // |
There was a problem hiding this comment.
This repository interface is fully commented out. It should be deleted from the source tree if it's no longer in use to keep the project clean.
| // @Query(""" | ||
| // select new life.mosu.mosuserver.domain.event.projection.EventWithAttachmentProjection( | ||
| // e.id, | ||
| // e.title, | ||
| // e.duration.endDate, | ||
| // e.eventLink, | ||
| // new life.mosu.mosuserver.domain.event.projection.AttachmentProjection( | ||
| // ea.fileName, | ||
| // ea.s3Key | ||
| // ) | ||
| // ) | ||
| // from EventJpaEntity e | ||
| // left join EventAttachmentJpaEntity ea | ||
| // on e.id = ea.eventId | ||
| // """) | ||
| // List<EventWithAttachmentProjection> findAllWithAttachment(); | ||
|
|
||
| @Query(""" | ||
| select new life.mosu.mosuserver.domain.event.projection.EventWithAttachmentProjection( | ||
| e.id, | ||
| e.title, | ||
| e.duration.endDate, | ||
| e.eventLink, | ||
| new life.mosu.mosuserver.domain.event.projection.AttachmentProjection( | ||
| ea.fileName, | ||
| ea.s3Key | ||
| ) | ||
| ) | ||
| from EventJpaEntity e | ||
| left join EventAttachmentJpaEntity ea | ||
| on e.id = ea.eventId | ||
| WHERE e.id = :id | ||
| """) | ||
| Optional<EventWithAttachmentProjection> findWithAttachmentById(Long id); | ||
| // @Query(""" | ||
| // select new life.mosu.mosuserver.domain.event.projection.EventWithAttachmentProjection( | ||
| // e.id, | ||
| // e.title, | ||
| // e.duration.endDate, | ||
| // e.eventLink, | ||
| // new life.mosu.mosuserver.domain.event.projection.AttachmentProjection( | ||
| // ea.fileName, | ||
| // ea.s3Key | ||
| // ) | ||
| // ) | ||
| // from EventJpaEntity e | ||
| // left join EventAttachmentJpaEntity ea | ||
| // on e.id = ea.eventId | ||
| // WHERE e.id = :id | ||
| // """) | ||
| // Optional<EventWithAttachmentProjection> findWithAttachmentById(Long id); |
There was a problem hiding this comment.
The commented-out query methods are no longer used. They should be removed completely to improve code readability and maintainability.
| //package life.mosu.mosuserver.domain.event.projection; | ||
| // | ||
| //import java.time.LocalDate; | ||
| // | ||
| //public record EventWithAttachmentProjection( | ||
| // Long eventId, | ||
| // String title, | ||
| // LocalDate endDate, | ||
| // String eventLink, | ||
| // AttachmentProjection attachment | ||
| //) { | ||
| // | ||
| //} No newline at end of file |
There was a problem hiding this comment.
This record is no longer used and its contents have been commented out. The file should be deleted from the project to avoid clutter.
| @RequiredArgsConstructor | ||
| @RequestMapping("/event") | ||
| public class EventController implements EventControllerDocs { | ||
| public class EventController { |
There was a problem hiding this comment.
The implements EventControllerDocs clause has been removed from the class definition. If this was unintentional, it should be restored. Its absence could lead to incomplete or missing API documentation if you are using a tool like Springdoc or Swagger that relies on such interfaces for documentation generation.
| public class EventController { | |
| public class EventController implements EventControllerDocs { |
| // @Schema(description = "첨부파일 목록") | ||
| // List<AttachmentResponse> attachments | ||
| ) { | ||
|
|
||
| public static NoticeResponse of(NoticeJpaEntity notice, List<AttachmentResponse> attachments) { | ||
| public static NoticeResponse of(NoticeJpaEntity notice) { | ||
| return new NoticeResponse( | ||
| notice.getId(), | ||
| notice.getTitle(), | ||
| notice.getContent(), | ||
| notice.getAuthor(), | ||
| notice.getCreatedAt(), | ||
| attachments | ||
| notice.getCreatedAt() | ||
| // attachments | ||
| ); | ||
| } | ||
|
|
||
| public record AttachmentResponse( | ||
|
|
||
| @Schema(description = "파일 이름", example = "service_guide.pdf") | ||
| String fileName, | ||
|
|
||
| @Schema(description = "S3 Presigned URL", example = "https://bucket.s3.amazonaws.com/.../service_guide.pdf") | ||
| String url | ||
| ) { | ||
|
|
||
| } | ||
| // public record AttachmentResponse( | ||
| // | ||
| // @Schema(description = "파일 이름", example = "service_guide.pdf") | ||
| // String fileName, | ||
| // | ||
| // @Schema(description = "S3 Presigned URL", example = "https://bucket.s3.amazonaws.com/.../service_guide.pdf") | ||
| // String url | ||
| // ) { | ||
| // | ||
| // } | ||
| } No newline at end of file |
There was a problem hiding this comment.
The attachments field and the nested AttachmentResponse record have been commented out. This dead code should be removed to clean up the DTO definition and improve readability.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Removals
Bug Fixes
Other Changes