Conversation
WalkthroughThis update introduces a full Notice management feature, including entity, repository, service, controller, and DTOs for notices and their attachments. It also generalizes file attachment logic through a helper method, refactors attachment services for FAQ and Inquiry modules, and adds a new error code for missing notices. Minor code cleanups and import optimizations are included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NoticeController
participant NoticeService
participant NoticeRepository
participant NoticeAttachmentService
participant NoticeAttachmentRepository
participant FileUploadHelper
participant S3Service
Client->>NoticeController: POST /notice (NoticeCreateRequest)
NoticeController->>NoticeService: createNotice(request)
NoticeService->>NoticeRepository: save(noticeEntity)
NoticeService->>NoticeAttachmentService: createAttachment(request.attachments, noticeEntity)
NoticeAttachmentService->>FileUploadHelper: saveAttachments(requests, noticeId, repo, mapper, keyExtractor)
FileUploadHelper->>NoticeAttachmentRepository: save(entity)
FileUploadHelper->>S3Service: updateTag(s3Key, ACTIVE)
NoticeController-->>Client: 201 Created
Client->>NoticeController: GET /notice/{noticeId}
NoticeController->>NoticeService: getNoticeDetail(noticeId)
NoticeService->>NoticeRepository: findById(noticeId)
NoticeService->>NoticeAttachmentService: toDetailAttResponses(noticeEntity)
NoticeAttachmentService->>NoticeAttachmentRepository: findAllByNoticeId(noticeId)
NoticeAttachmentService->>S3Service: getPresignedUrl(s3Key, expiration)
NoticeController-->>Client: 200 OK (NoticeDetailResponse)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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 introduces a new 'Notice' feature to the application, enabling users to manage announcements with full CRUD functionality and integrated file attachments. A key part of this change involved a significant refactoring of the existing attachment handling mechanism, consolidating duplicated logic into a generic helper to improve code maintainability and establish a reusable pattern for future attachment-enabled features.
Highlights
- New 'Notice' Feature: I've implemented a comprehensive 'Notice' (공지) feature, providing full CRUD (Create, Read, Update, Delete) capabilities for notices, complete with dedicated API endpoints, service logic, and data models. This includes support for file attachments.
- Generic Attachment Handling Refactor: I've refactored the application's attachment saving logic by introducing a new generic
saveAttachmentsmethod inFileUploadHelper. This method centralizes the process of updating S3 tags and persisting attachment entities, significantly reducing code duplication across existing attachment services (FAQ, Inquiry, Inquiry Answer) and providing a consistent pattern for the new Notice feature.
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 the notice feature and refactors attachment handling logic. The review focuses on performance optimizations, security enhancements, and adherence to REST API conventions. A critical issue regarding orphaned S3 files was identified.
| public void deleteAttachment(NoticeJpaEntity entity) { | ||
| List<NoticeAttachmentJpaEntity> attachments = noticeAttachmentRepository.findAllByNoticeId( | ||
| entity.getId()); | ||
| noticeAttachmentRepository.deleteAll(attachments); | ||
| } |
There was a problem hiding this comment.
This method deletes attachment records from the database but doesn't delete the corresponding files from S3. This results in orphaned files in your S3 bucket, leading to unnecessary storage costs and data inconsistency. You should delete each file from S3 before deleting the attachment record.
There was a problem hiding this comment.
아 이거 아직 어떻게 파일 처리할 지 안정했어서 안넣긴했어요
| public List<NoticeResponse> getNoticeWithAttachments(int page, int size) { | ||
| Pageable pageable = PageRequest.of(page, size, Sort.by("id")); | ||
| Page<NoticeJpaEntity> noticePage = noticeRepository.findAll(pageable); | ||
|
|
||
| return noticePage.stream() | ||
| .map(this::toNoticeResponse) | ||
| .toList(); |
There was a problem hiding this comment.
This method has a potential N+1 query problem. The noticePage.stream().map(this::toNoticeResponse) triggers a separate database query for attachments for each notice on the page. Consider fetching all attachments in a single query.
| NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FILE_NOT_FOUND)); |
There was a problem hiding this comment.
This method uses ErrorCode.FILE_NOT_FOUND when a notice is not found. Use the more specific ErrorCode.NOTICE_NOT_FOUND for better error clarity. Also, consider reusing the getNoticeOrThrow helper method.
NoticeJpaEntity noticeEntity = getNoticeOrThrow(noticeId);| NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FILE_NOT_FOUND)); |
There was a problem hiding this comment.
This method uses ErrorCode.FILE_NOT_FOUND instead of ErrorCode.NOTICE_NOT_FOUND. Use the getNoticeOrThrow helper method here as well.
NoticeJpaEntity noticeEntity = getNoticeOrThrow(noticeId);|
|
||
| @NotNull String title, | ||
| @NotNull String content, | ||
| Long userId, |
There was a problem hiding this comment.
Including userId in the request body can be a security risk. Determine the user's identity from the security context on the server-side instead.
| .map(attachment -> new NoticeResponse.AttachmentResponse( | ||
| attachment.getFileName(), | ||
| s3Service.getPreSignedUrl( | ||
| attachment.getS3Key(), | ||
| Duration.ofMinutes(s3Properties.getPresignedUrlExpirationMinutes()) | ||
| ) | ||
| )) |
There was a problem hiding this comment.
Consider reducing code duplication by reusing the fileUrl method, which already encapsulates the logic for generating a pre-signed URL.
.map(attachment -> new NoticeResponse.AttachmentResponse(
attachment.getFileName(),
fileUrl(attachment.getS3Key())
))| public ResponseEntity<ApiResponseWrapper<Void>> createNotice( | ||
| @Valid @RequestBody NoticeCreateRequest request) { | ||
| noticeService.createNotice(request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); |
There was a problem hiding this comment.
For a resource creation endpoint, return an HTTP status code of 201 Created instead of 200 OK.
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); | |
| return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); |
| for (R req : requests) { | ||
| String s3Key = getKey.apply(req); | ||
| updateTag(s3Key); | ||
| E entity = toEntityMapper.apply(req, parentId); //DTO -> ToEntity | ||
| repository.save(entity); | ||
| } |
There was a problem hiding this comment.
Calling repository.save() inside a loop can lead to performance issues. Save all entities in a single batch operation using repository.saveAll() outside the loop.
List<E> entitiesToSave = new java.util.ArrayList<>(requests.size());
for (R req : requests) {
String s3Key = getKey.apply(req);
updateTag(s3Key);
E entity = toEntityMapper.apply(req, parentId);
entitiesToSave.add(entity);
}
repository.saveAll(entitiesToSave);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java (1)
10-13: Consider adding validation for attachments if needed.The
attachmentsfield lacks validation constraints. If there are business rules about attachment limits or requirements, consider adding appropriate validation annotations.src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java (1)
7-13: Consider adding a toEntity() method for consistency.Unlike
NoticeCreateRequest, this record doesn't have atoEntity()method. If the update logic would benefit from entity conversion, consider adding a similar method for consistency. Otherwise, the current structure is acceptable if updates are handled differently in the service layer.src/main/java/life/mosu/mosuserver/infra/storage/FileUploadHelper.java (2)
35-35: Use English for code comments.For consistency and better maintainability, code comments should be in English.
- E entity = toEntityMapper.apply(req, parentId); //DTO -> ToEntity + E entity = toEntityMapper.apply(req, parentId); // Convert DTO to Entity
21-38: Consider adding error handling for S3 and repository operations.The generic method design looks good and promotes code reuse. However, consider wrapping the S3 tag update and repository save operations in appropriate error handling to ensure partial failures are handled gracefully.
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java (1)
32-36: Simplify the lambda by using the provided ID parameter.The lambda receives an
idparameter but doesn't use it, instead callinginquiryEntity.getId()again.- (req, id) -> req.toInquiryAttachmentEntity( - req.fileName(), - req.s3Key(), - inquiryEntity.getId() - ), + (req, id) -> req.toInquiryAttachmentEntity( + req.fileName(), + req.s3Key(), + id + ),src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java (2)
65-66: Consider optimizing attachment updates.Currently, all attachments are deleted and recreated during updates. Consider implementing a more efficient approach that only modifies changed attachments.
Would you like me to suggest an implementation that compares existing attachments with the update request to minimize unnecessary deletions and creations?
36-36: Consider making the sort parameter configurable.The sort order is hardcoded to "id". Consider accepting sort parameters from the request for more flexibility.
src/main/java/life/mosu/mosuserver/application/notice/NoticeAttachmentService.java (3)
44-49: Consider adding error handling for attachment deletion.The method directly deletes all attachments without checking if the deletion was successful or handling potential database exceptions.
Consider wrapping the deletion in a try-catch block or adding logging:
@Override public void deleteAttachment(NoticeJpaEntity entity) { List<NoticeAttachmentJpaEntity> attachments = noticeAttachmentRepository.findAllByNoticeId( entity.getId()); - noticeAttachmentRepository.deleteAll(attachments); + try { + noticeAttachmentRepository.deleteAll(attachments); + log.info("Deleted {} attachments for notice {}", attachments.size(), entity.getId()); + } catch (Exception e) { + log.error("Failed to delete attachments for notice {}", entity.getId(), e); + throw e; + } }
51-65: Optimize database queries by extracting attachment retrieval.Both
toAttachmentResponsesandtoDetailAttResponsesmethods perform identical database queries. Consider extracting this logic to avoid duplication.Extract the common query logic:
+private List<NoticeAttachmentJpaEntity> getNoticeAttachments(Long noticeId) { + return noticeAttachmentRepository.findAllByNoticeId(noticeId); +} + public List<NoticeResponse.AttachmentResponse> toAttachmentResponses(NoticeJpaEntity notice) { - List<NoticeAttachmentJpaEntity> attachments = noticeAttachmentRepository.findAllByNoticeId( - notice.getId()); + List<NoticeAttachmentJpaEntity> attachments = getNoticeAttachments(notice.getId()); return attachments.stream() .map(attachment -> new NoticeResponse.AttachmentResponse(
67-81: Extract duplicate database query logic.This method has the same database query as
toAttachmentResponses. Apply the same refactoring suggestion to use the extracted helper method.public List<NoticeDetailResponse.AttachmentDetailResponse> toDetailAttResponses( NoticeJpaEntity notice) { - List<NoticeAttachmentJpaEntity> attachments = noticeAttachmentRepository.findAllByNoticeId( - notice.getId()); + List<NoticeAttachmentJpaEntity> attachments = getNoticeAttachments(notice.getId()); return attachments.stream() .map(attachment -> new NoticeDetailResponse.AttachmentDetailResponse(src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java (2)
39-46: Consider adding validation for pagination parameters.The pagination parameters have default values but no validation for maximum limits or negative values.
Add validation for pagination parameters:
@GetMapping("/list") public ResponseEntity<ApiResponseWrapper<List<NoticeResponse>>> getNotices( - @RequestParam(defaultValue = "0") int page, - @RequestParam(defaultValue = "10") int size + @RequestParam(defaultValue = "0") @Min(0) int page, + @RequestParam(defaultValue = "10") @Min(1) @Max(100) int size ) {
48-53: Consider adding path variable validation.The notice ID should be validated to ensure it's a positive value.
Add validation to the path variable:
@GetMapping("/{noticeId}") public ResponseEntity<ApiResponseWrapper<NoticeDetailResponse>> getNoticeDetail( - @PathVariable Long noticeId) { + @PathVariable @Min(1) Long noticeId) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notice/NoticeAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/notice/NoticeAttachmentRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/notice/NoticeJpaEntity.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/notice/NoticeRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/FileRequest.java(2 hunks)src/main/java/life/mosu/mosuserver/infra/storage/FileUploadHelper.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeDetailResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java(1 hunks)src/test/java/life/mosu/mosuserver/FaqServiceTest.java(1 hunks)
🔇 Additional comments (16)
src/test/java/life/mosu/mosuserver/FaqServiceTest.java (1)
24-27: LGTM! Clean import cleanup and proper mock formatting.The removal of unused imports and reformatting of mock annotations to separate lines improves code maintainability and readability.
src/main/java/life/mosu/mosuserver/domain/notice/NoticeRepository.java (1)
5-7: LGTM! Standard JPA repository implementation.Clean and conventional Spring Data JPA repository interface that provides the necessary CRUD operations for notice management.
src/main/java/life/mosu/mosuserver/global/util/FileRequest.java (1)
24-32: LGTM! Consistent implementation following established patterns.The new method follows the same pattern as existing attachment entity conversion methods, correctly using the builder pattern and setting appropriate visibility (PUBLIC for notices).
src/main/java/life/mosu/mosuserver/domain/notice/NoticeAttachmentRepository.java (1)
6-9: LGTM! Well-designed repository with appropriate custom query method.The repository follows Spring Data JPA conventions and the
findAllByNoticeIdmethod will be automatically implemented by Spring Data based on the method naming pattern.src/main/java/life/mosu/mosuserver/domain/notice/NoticeJpaEntity.java (2)
3-8: LGTM! Explicit imports improve code clarity.Using explicit imports instead of wildcards is a good practice that improves code readability and reduces potential naming conflicts.
29-29: LGTM! Appropriate column length for notice content.The increased column length of 3000 characters for content is reasonable for notice/announcement functionality.
src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java (1)
17-23: Confirm intentional separation of attachments handling.The
toEntity()method excludes theattachmentsfield from the entity conversion. This appears intentional, likely because attachments are handled separately by the service layer. This is a good separation of concerns.src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java (1)
31-41: Excellent refactoring to centralize attachment handling.The refactoring to use
fileUploadHelper.saveAttachmentsis a great improvement. It centralizes common attachment logic, reduces code duplication, and makes the code more maintainable. The lambda expressions are clean and readable.src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
74-77: Good addition of notice-related error code.The addition of
NOTICE_NOT_FOUNDfollows the established pattern with appropriate HTTP status, consistent Korean error message format, and proper enum syntax. The new comment section for notice-related errors maintains good code organization.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java (1)
29-39: Consistent refactoring improves code maintainability.This refactoring follows the same pattern as
FaqAttachmentService, using the centralizedfileUploadHelper.saveAttachmentsmethod. The consistency across attachment services is excellent and will make the codebase easier to maintain.src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeResponse.java (1)
1-25: LGTM!Clean and well-structured DTO implementation using records and factory method pattern.
src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeDetailResponse.java (1)
1-32: LGTM!Well-structured DTO with detailed attachment information, consistent with the overall DTO pattern.
src/main/java/life/mosu/mosuserver/application/notice/NoticeAttachmentService.java (3)
19-22: Good implementation of service class structure.The class follows Spring conventions with proper annotations and implements the generic
AttachmentServiceinterface, promoting consistency across different attachment types.
83-88: Good extraction of common URL generation logic.The private helper method promotes code reuse and maintainability.
29-42: Generic saveAttachments implementation verifiedThe
saveAttachmentsmethod inFileUploadHelper.javacorrectly guards against null or empty request lists, maps eachFileRequestto its entity using the provided lambdas, and persists them via the repository. Its usage inNoticeAttachmentServiceis consistent with other attachment services and poses no edge-case concerns. No changes required.src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java (1)
24-27: Controller structure follows Spring conventions.The controller is properly annotated and follows RESTful conventions with clear request mapping.
| public void update( | ||
| final String title, | ||
| final String content | ||
| ) { | ||
| this.title = title; | ||
| this.content = content; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to the update method.
The update method should validate that the provided parameters are not null or empty before updating the entity fields to prevent invalid state.
public void update(
final String title,
final String content
) {
+ if (title == null || title.trim().isEmpty()) {
+ throw new IllegalArgumentException("Title cannot be null or empty");
+ }
+ if (content == null || content.trim().isEmpty()) {
+ throw new IllegalArgumentException("Content cannot be null or empty");
+ }
this.title = title;
this.content = content;
}📝 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 update( | |
| final String title, | |
| final String content | |
| ) { | |
| this.title = title; | |
| this.content = content; | |
| } | |
| public void update( | |
| final String title, | |
| final String content | |
| ) { | |
| if (title == null || title.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Title cannot be null or empty"); | |
| } | |
| if (content == null || content.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Content cannot be null or empty"); | |
| } | |
| this.title = title; | |
| this.content = content; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/notice/NoticeJpaEntity.java around
lines 46 to 52, the update method lacks input validation for the title and
content parameters. Add checks to ensure that neither title nor content is null
or empty before assigning them to the entity fields. If validation fails, throw
an appropriate exception to prevent the entity from entering an invalid state.
| NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FILE_NOT_FOUND)); |
There was a problem hiding this comment.
Use NOTICE_NOT_FOUND error code for consistency.
Same issue as in the deleteNotice method - use the appropriate error code.
- NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId)
- .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FILE_NOT_FOUND));
+ NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId)
+ .orElseThrow(() -> new CustomRuntimeException(ErrorCode.NOTICE_NOT_FOUND));📝 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.
| NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FILE_NOT_FOUND)); | |
| NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.NOTICE_NOT_FOUND)); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java at
lines 61-62, replace the error code FILE_NOT_FOUND with NOTICE_NOT_FOUND in the
orElseThrow call to maintain consistency with the deleteNotice method. This
involves changing the argument passed to the CustomRuntimeException constructor
to ErrorCode.NOTICE_NOT_FOUND.
| NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FILE_NOT_FOUND)); |
There was a problem hiding this comment.
Use NOTICE_NOT_FOUND error code for consistency.
The method should throw NOTICE_NOT_FOUND instead of FILE_NOT_FOUND when a notice is not found.
- NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId)
- .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FILE_NOT_FOUND));
+ NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId)
+ .orElseThrow(() -> new CustomRuntimeException(ErrorCode.NOTICE_NOT_FOUND));📝 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.
| NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FILE_NOT_FOUND)); | |
| NoticeJpaEntity noticeEntity = noticeRepository.findById(noticeId) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.NOTICE_NOT_FOUND)); |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java at
lines 53-54, replace the error code FILE_NOT_FOUND with NOTICE_NOT_FOUND in the
orElseThrow call to ensure the exception thrown matches the context of a missing
notice.
| // TODO: 관리자 권한 체크 추가 | ||
| @DeleteMapping("/{noticeId}") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> deleteNotice(@PathVariable Long noticeId) { | ||
| noticeService.deleteNotice(noticeId); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 삭제 성공")); | ||
| } |
There was a problem hiding this comment.
Critical security issue: Missing admin permission checks for deletion.
The TODO comment indicates missing admin permission checks for the delete operation, which is a serious security vulnerability.
Implement proper authorization:
// TODO: 관리자 권한 체크 추가
@DeleteMapping("/{noticeId}")
+@PreAuthorize("hasRole('ADMIN')")
public ResponseEntity<ApiResponseWrapper<Void>> deleteNotice(@PathVariable Long noticeId) {🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java
around lines 55 to 60, the deleteNotice method lacks admin permission checks,
posing a critical security risk. Add authorization logic to verify that the user
has admin privileges before allowing deletion. This can be done by integrating
Spring Security annotations like @PreAuthorize("hasRole('ADMIN')") on the method
or by programmatically checking the user's roles within the method and returning
an unauthorized response if the user is not an admin.
| // TODO: 관리자 권한 체크 추가 | ||
| @PostMapping | ||
| public ResponseEntity<ApiResponseWrapper<Void>> createNotice( | ||
| @Valid @RequestBody NoticeCreateRequest request) { | ||
| noticeService.createNotice(request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); | ||
| } |
There was a problem hiding this comment.
Critical security issue: Missing admin permission checks.
The TODO comment indicates missing admin permission checks, which is a serious security vulnerability. Without proper authorization, anyone can create notices.
Implement proper authorization before the method executes:
// TODO: 관리자 권한 체크 추가
@PostMapping
+@PreAuthorize("hasRole('ADMIN')")
public ResponseEntity<ApiResponseWrapper<Void>> createNotice(
@Valid @RequestBody NoticeCreateRequest request) {Or implement a custom authorization mechanism if Spring Security is not used.
📝 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.
| // TODO: 관리자 권한 체크 추가 | |
| @PostMapping | |
| public ResponseEntity<ApiResponseWrapper<Void>> createNotice( | |
| @Valid @RequestBody NoticeCreateRequest request) { | |
| noticeService.createNotice(request); | |
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); | |
| } | |
| // TODO: 관리자 권한 체크 추가 | |
| @PostMapping | |
| @PreAuthorize("hasRole('ADMIN')") | |
| public ResponseEntity<ApiResponseWrapper<Void>> createNotice( | |
| @Valid @RequestBody NoticeCreateRequest request) { | |
| noticeService.createNotice(request); | |
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java
around lines 31 to 37, the createNotice method lacks admin permission checks,
posing a critical security risk. Add an authorization check to ensure only users
with admin privileges can access this endpoint, for example by annotating the
method with @PreAuthorize("hasRole('ADMIN')") if using Spring Security, or
implement a custom authorization mechanism to verify admin rights before
executing the method logic.
| // TODO: 관리자 권한 체크 추가 | ||
| @PutMapping("/{noticeId}") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> updateNotice( | ||
| @PathVariable Long noticeId, | ||
| @Valid @RequestBody NoticeUpdateRequest request | ||
| ) { | ||
| noticeService.updateNotice(noticeId, request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 수정 성공")); | ||
| } |
There was a problem hiding this comment.
Critical security issue: Missing admin permission checks for updates.
The TODO comment indicates missing admin permission checks for the update operation, which is a serious security vulnerability.
Implement proper authorization:
// TODO: 관리자 권한 체크 추가
@PutMapping("/{noticeId}")
+@PreAuthorize("hasRole('ADMIN')")
public ResponseEntity<ApiResponseWrapper<Void>> updateNotice(
@PathVariable Long noticeId,
@Valid @RequestBody NoticeUpdateRequest request
) {📝 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.
| // TODO: 관리자 권한 체크 추가 | |
| @PutMapping("/{noticeId}") | |
| public ResponseEntity<ApiResponseWrapper<Void>> updateNotice( | |
| @PathVariable Long noticeId, | |
| @Valid @RequestBody NoticeUpdateRequest request | |
| ) { | |
| noticeService.updateNotice(noticeId, request); | |
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 수정 성공")); | |
| } | |
| // TODO: 관리자 권한 체크 추가 | |
| @PutMapping("/{noticeId}") | |
| @PreAuthorize("hasRole('ADMIN')") | |
| public ResponseEntity<ApiResponseWrapper<Void>> updateNotice( | |
| @PathVariable Long noticeId, | |
| @Valid @RequestBody NoticeUpdateRequest request | |
| ) { | |
| noticeService.updateNotice(noticeId, request); | |
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 수정 성공")); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java
around lines 62 to 70, the updateNotice method lacks admin permission checks,
posing a critical security risk. Add authorization logic to verify that the user
has admin privileges before allowing the update operation. This can be done by
integrating a security annotation like @PreAuthorize("hasRole('ADMIN')") on the
method or by programmatically checking the user's roles within the method and
returning an appropriate error response if the user is unauthorized.
| // TODO: 관리자 권한 체크 추가 | ||
| @PostMapping | ||
| public ResponseEntity<ApiResponseWrapper<Void>> createNotice( | ||
| @Valid @RequestBody NoticeCreateRequest request) { | ||
| noticeService.createNotice(request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); | ||
| } | ||
|
|
||
| @GetMapping("/list") | ||
| public ResponseEntity<ApiResponseWrapper<List<NoticeResponse>>> getNotices( | ||
| @RequestParam(defaultValue = "0") int page, | ||
| @RequestParam(defaultValue = "10") int size | ||
| ) { | ||
| List<NoticeResponse> notices = noticeService.getNoticeWithAttachments(page, size); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 조회 성공", notices)); | ||
| } | ||
|
|
||
| @GetMapping("/{noticeId}") | ||
| public ResponseEntity<ApiResponseWrapper<NoticeDetailResponse>> getNoticeDetail( | ||
| @PathVariable Long noticeId) { | ||
| NoticeDetailResponse notice = noticeService.getNoticeDetail(noticeId); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 상세 조회 성공", notice)); | ||
| } | ||
|
|
||
| // TODO: 관리자 권한 체크 추가 | ||
| @DeleteMapping("/{noticeId}") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> deleteNotice(@PathVariable Long noticeId) { | ||
| noticeService.deleteNotice(noticeId); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 삭제 성공")); | ||
| } | ||
|
|
||
| // TODO: 관리자 권한 체크 추가 | ||
| @PutMapping("/{noticeId}") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> updateNotice( | ||
| @PathVariable Long noticeId, | ||
| @Valid @RequestBody NoticeUpdateRequest request | ||
| ) { | ||
| noticeService.updateNotice(noticeId, request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 수정 성공")); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address all TODO comments before production deployment.
Multiple TODO comments indicate incomplete security implementation. This creates significant security risks where unauthorized users could manage notices.
Do you want me to generate a comprehensive authorization solution or open a separate issue to track these security requirements?
🏁 Script executed:
#!/bin/bash
# Description: Check if similar authorization patterns exist in other controllers
# Expected: Find existing authorization implementations to follow the same pattern
# Search for authorization annotations in other controllers
rg -A 3 -B 3 "@PreAuthorize\|@Secured\|@RolesAllowed" --type java
# Search for similar TODO comments about authorization
rg -A 2 -B 2 "TODO.*권한\|TODO.*auth\|TODO.*permission" --type javaLength of output: 135
🏁 Script executed:
#!/bin/bash
# Verify presence of any existing Spring Security configuration or method-level security usage
# Look for a SecurityConfig class
rg -n "class .*SecurityConfig" --type java
# Check for global method security annotation
rg -n "@EnableGlobalMethodSecurity" --type java
# Look for WebSecurityConfigurerAdapter (older style) or SecurityFilterChain (new style)
rg -n "WebSecurityConfigurerAdapter" --type java
rg -n "SecurityFilterChain" --type java
# Scan for HttpSecurity usage or authorizeRequests calls
rg -n "HttpSecurity" --type java
rg -n "authorizeRequests" --type javaLength of output: 974
Secure Notice endpoints with proper authorization
The TODO markers indicate that create/update/delete operations in NoticeController are unprotected. Although you have a SecurityConfig with a SecurityFilterChain, no method-level security is enabled and no role restrictions are applied to /notices/**. To lock down these endpoints before production:
• In SecurityConfig.java (src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java):
- Enable method‐level annotations by adding
@EnableGlobalMethodSecurity(prePostEnabled = true) - Or configure HTTP restrictions directly in your
securityFilterChainbean, for example:http .authorizeRequests() .antMatchers(HttpMethod.POST, "/notices/**").hasRole("ADMIN") .antMatchers(HttpMethod.PUT, "/notices/**").hasRole("ADMIN") .antMatchers(HttpMethod.DELETE, "/notices/**").hasRole("ADMIN") .anyRequest().authenticated() // … other config ;
• In NoticeController.java (src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java):
- If using method‐level security, add annotations such as
@PreAuthorize("hasRole('ADMIN')")to the@PostMapping,@PutMapping, and@DeleteMappingmethods. - Remove the
// TODO: 관리자 권한 체크 추가comments once enforcement is in place.
Implement one of these approaches to mitigate unauthorized notice management.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java
lines 31 to 70, the create, update, and delete notice endpoints lack
authorization checks. To fix this, enable method-level security by adding
@EnableGlobalMethodSecurity(prePostEnabled = true) in SecurityConfig.java or
configure HTTP security to restrict POST, PUT, DELETE requests on /notices/** to
users with ADMIN role. Then, in NoticeController.java, add
@PreAuthorize("hasRole('ADMIN')") annotations to the createNotice, updateNotice,
and deleteNotice methods and remove the TODO comments indicating missing admin
checks.
Docstrings generation was requested by @jbh010204. * #58 (comment) The following files were modified: * `src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java` * `src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java` * `src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java` * `src/main/java/life/mosu/mosuserver/application/notice/NoticeAttachmentService.java` * `src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java` * `src/main/java/life/mosu/mosuserver/domain/notice/NoticeAttachmentRepository.java` * `src/main/java/life/mosu/mosuserver/domain/notice/NoticeJpaEntity.java` * `src/main/java/life/mosu/mosuserver/global/util/FileRequest.java` * `src/main/java/life/mosu/mosuserver/infra/storage/FileUploadHelper.java` * `src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java` * `src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java` * `src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeDetailResponse.java` * `src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeResponse.java`
|
Note Generated docstrings for this pull request at #59 |
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores