MOSU-70 refactor: faq, notice response 형식 변경 및 Swagger Docs 적용 #73
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces author tracking and enhanced API documentation for FAQ and notice entities. It adds author fields to domain, DTO, and response classes, updates service and controller logic for CRUD operations, and integrates detailed Swagger/OpenAPI annotations. The FAQ API now supports retrieval and update of individual entries, with expanded validation and response metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FaqController
participant FaqService
participant FaqJpaEntity
participant FaqAttachmentService
Client->>FaqController: POST /faqs (createFaq)
FaqController->>FaqService: create(request)
FaqService->>FaqJpaEntity: build (question, answer, userId, author)
FaqService->>FaqAttachmentService: createAttachments(...)
FaqController-->>Client: 201 Created
Client->>FaqController: GET /faqs/{faqId} (getFaqDetail)
FaqController->>FaqService: getFaqDetail(faqId)
FaqService->>FaqJpaEntity: findById(faqId)
FaqService->>FaqAttachmentService: getAttachments(faqId)
FaqController-->>Client: 200 OK (FaqResponse)
Client->>FaqController: PUT /faqs/{faqId} (updateFaq)
FaqController->>FaqService: update(request, faqId)
FaqService->>FaqJpaEntity: findById(faqId)
FaqService->>FaqJpaEntity: update(question, answer, author)
FaqService->>FaqAttachmentService: deleteAttachments(faqId)
FaqService->>FaqAttachmentService: createAttachments(...)
FaqController-->>Client: 200 OK
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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 significantly refactors the FAQ and Notice modules, primarily focusing on integrating comprehensive Swagger documentation for all related API endpoints. It also expands the functionality of these modules by introducing new API operations for FAQ detail retrieval and updates, and by standardizing response formats to include more complete data like author information and S3 keys for attachments. These changes aim to improve API clarity, maintainability, and feature completeness.
Highlights
- Swagger Documentation Integration: Introduced dedicated documentation interfaces (
FaqControllerDocs,NoticeControllerDocs) and extensively applied@Schemaannotations across DTOs to provide comprehensive Swagger/OpenAPI documentation for FAQ and Notice APIs, enhancing API discoverability and usability. - FAQ Module Enhancements: Expanded FAQ functionality by adding an 'author' field to the data model and DTOs, introducing new API endpoints for retrieving a single FAQ by ID (
GET /faq/{faqId}), and enabling full update capabilities for existing FAQs (PUT /faq/{faqId}), including content and associated attachments. - Notice Module Updates: Enhanced the Notice module by incorporating an 'author' field into the data model and DTOs, allowing the author information to be created and updated alongside other notice details.
- Standardized Attachment Responses: Modified attachment response DTOs for both FAQs and Notices to consistently include the S3 key (
s3Key) in addition to the file name and pre-signed URL, providing more complete file metadata. - Configuration and DTO Refinements: Performed minor refactorings, including the removal of
@ConfigurationPropertiesScanfrom the main application class, adjustments toS3Propertiesfor better Spring integration, and standardization of date formats in response DTOs.
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. ↩
| public ResponseEntity<ApiResponseWrapper<Void>> createFaq( | ||
| @Valid @RequestBody FaqCreateRequest request) { | ||
| faqService.createFaq(request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); |
There was a problem hiding this comment.
The createFaq method returns ResponseEntity.ok(), which results in a 200 OK HTTP status. For resource creation, the standard practice is to return a 201 Created status. Using ResponseEntity.status(HttpStatus.CREATED) will align the implementation with REST conventions and the Swagger documentation.
return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공"));| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.FAQ_NOT_FOUND)); | ||
|
|
||
| faqEntity.update(request.question(), request.answer(), request.author()); | ||
| faqRepository.save(faqEntity); |
There was a problem hiding this comment.
Within a @Transactional method, an explicit call to faqRepository.save(faqEntity) is redundant. JPA's dirty checking mechanism will automatically detect changes to the faqEntity and persist them to the database when the transaction commits. Removing this line will make the code cleaner.
| import lombok.Data; | ||
| import lombok.extern.slf4j.Slf4j; |
There was a problem hiding this comment.
Consider using @Getter and @Setter instead of @Data to have more control over which methods are generated. @Data generates equals(), hashCode(), and toString() methods, which might not be desirable for configuration properties classes.
| import lombok.Data; | |
| import lombok.extern.slf4j.Slf4j; | |
| import lombok.Getter; | |
| import lombok.Setter; | |
| import lombok.extern.slf4j.Slf4j; | |
| import org.springframework.boot.context.properties.ConfigurationProperties; | |
| import org.springframework.stereotype.Component; | |
| @Getter | |
| @Setter | |
| @Component |
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "FAQ 목록 조회 성공", | ||
| content = @Content(mediaType = "application/json", | ||
| array = @ArraySchema(schema = @Schema(implementation = FaqResponse.class)) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The @ApiResponse for getFaqs seems to incorrectly describe the response body. The array attribute suggests the response is a direct JSON array of FaqResponse objects. However, the actual return type is ResponseEntity<ApiResponseWrapper<List<FaqResponse>>>. This will generate misleading Swagger documentation. To fix this, describe the ApiResponseWrapper object itself.
| faq.getId(), | ||
| faq.getQuestion(), | ||
| faq.getAnswer(), | ||
| faq.getCreatedAt().substring(0, 10), // 일자만 반환 |
There was a problem hiding this comment.
Using substring(0, 10) to format the date is fragile. If the format of the string from getCreatedAt() changes, this could lead to a StringIndexOutOfBoundsException. A more robust approach would be to parse the string into a Temporal type and then format it to the desired yyyy-MM-dd format using a DateTimeFormatter.
| String author, | ||
|
|
||
| @Schema(description = "문의 상태 (WAITING: 답변 대기, COMPLETED: 답변 완료)", example = "WAITING") | ||
| @Schema(description = "문의 상태 (PENDING: 답변 대기, COMPLETED: 답변 완료)", example = "PENDING") |
There was a problem hiding this comment.
Consider aligning the status description with the actual enum value. Change WAITING to PENDING to match the code.
| @Schema(description = "문의 상태 (PENDING: 답변 대기, COMPLETED: 답변 완료)", example = "PENDING") | |
| @Schema(description = "문의 상태 (PENDING: 답변 대기, COMPLETED: 답변 완료)", example = "PENDING") |
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "공지사항 목록 조회 성공", | ||
| content = @Content(schema = @Schema(implementation = NoticeResponse.class))) | ||
| }) |
There was a problem hiding this comment.
The @ApiResponse for getNotices incorrectly describes the response body as a single NoticeResponse object. The actual return type is ApiResponseWrapper<List<NoticeResponse>>, so the documentation is misleading. To correctly document a wrapped list, describe the ApiResponseWrapper.
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "공지사항 상세 조회 성공", | ||
| content = @Content(schema = @Schema(implementation = NoticeDetailResponse.class))) | ||
| }) |
There was a problem hiding this comment.
The @ApiResponse for getNoticeDetail incorrectly describes the response body as a NoticeDetailResponse object. The actual return type is ApiResponseWrapper<NoticeDetailResponse>, which means the documentation is misleading. To fix this, describe the ApiResponseWrapper object itself in the @Content annotation.
| @Schema(description = "작성자 ID (토큰에서 추출 예정)", example = "42") | ||
| Long userId, |
There was a problem hiding this comment.
The userId field is included in the NoticeUpdateRequest, but it's not used in the NoticeService.updateNotice method. Consider removing it to make the API contract clearer.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryResponse.java (1)
24-25: Prefer strong date-time typing over plainStringExposing
createdAtas a rawStringforfeits format validation and can lead to inconsistent payloads. Consider switching tojava.time.LocalDateTime(orOffsetDateTime) and declare the OpenAPI format explicitly:- @Schema(description = "문의 등록일", example = "2025-07-10") - String createdAt + @Schema(description = "문의 등록일", + type = "string", + format = "date-time", + example = "2025-07-10T15:30:00") + LocalDateTime createdAtThis keeps the contract self-describing and removes the need for manual parsing down-stream.
src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java (1)
31-31: Address the TODO comments for admin authorization.The TODO comments indicate missing admin authorization checks for create, update, and delete operations. These are critical security requirements that should be implemented before production deployment.
Would you like me to help generate the admin authorization implementation or create an issue to track this security requirement?
Also applies to: 55-55, 62-62
src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java (1)
17-18: Consider adding length validation for the author field.While
@NotNullvalidation is present, consider adding@Sizeannotation to prevent database constraint violations if the author column has a maximum length limit.@Schema(description = "작성자 이름", example = "관리자") -@NotNull String author, +@NotNull @Size(max = 100) String author,src/main/java/life/mosu/mosuserver/domain/notice/NoticeJpaEntity.java (1)
51-59: Consider consistency in the update method.The
updatemethod updatestitle,content, andauthorbut notuserId. This might be intentional for security reasons, but it creates an inconsistency between the constructor parameters and update method parameters.If
userIdshould not be updatable, consider documenting this design decision with a comment.src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeDetailResponse.java (1)
18-22: Consider using LocalDateTime for createdAt instead of String.The
createdAtfield is defined as String, which requires date formatting logic. Consider usingLocalDateTimeorLocalDatefor type safety and letting Jackson handle the serialization format.-@Schema(description = "작성일시 (yyyy-MM-dd)", example = "2025-07-08") -String createdAt, +@Schema(description = "작성일시", example = "2025-07-08T10:30:00") +LocalDateTime createdAt,src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java (3)
22-23: Clarify the purpose of the optional userId field.The
userIdfield is commented as "토큰에서 추출 예정" (to be extracted from token). This suggests future authentication integration. Consider:
- Whether this field should be in the request DTO or extracted from security context
- Adding validation if this becomes a required field
- Documenting the intended usage pattern
25-26: Consider adding validation constraints for attachments.The
attachmentsfield lacks validation constraints. Consider adding:
- Maximum list size validation
- Individual file validation (if not handled by FileRequest itself)
+@Size(max = 10, message = "첨부파일은 최대 10개까지 가능합니다") @Schema(description = "첨부파일 목록") List<FileRequest> attachments
22-23: Consider clarifying userId field usage.The userId field is optional but the documentation mentions "토큰에서 추출 예정" (to be extracted from token). Consider adding more detailed documentation about when this field is used versus when it's extracted from authentication context.
src/main/java/life/mosu/mosuserver/application/faq/FaqService.java (1)
53-63: Consider optimizing attachment handling for better performance.The current implementation deletes all existing attachments and recreates them, which might be inefficient for large attachment lists. Consider implementing a more granular approach that only updates changed attachments.
However, the current approach ensures data consistency and follows the existing transaction patterns correctly.
Alternative approach for attachment handling:
- attachmentService.deleteAttachment(faqEntity); - attachmentService.createAttachment(request.attachments(), faqEntity); + attachmentService.updateAttachments(faqEntity, request.attachments());This would require implementing a more sophisticated update logic in the attachment service that compares existing vs. new attachments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/main/java/life/mosu/mosuserver/MosuServerApplication.java(0 hunks)src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/faq/FaqService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/faq/FaqJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/notice/NoticeJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/property/S3Properties.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/faq/FaqController.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/faq/FaqControllerDocs.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqCreateRequest.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqUpdateRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/NoticeControllerDocs.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeDetailResponse.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeResponse.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/life/mosu/mosuserver/MosuServerApplication.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/life/mosu/mosuserver/infra/property/S3Properties.java (2)
src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java (1)
Slf4j(18-70)src/main/java/life/mosu/mosuserver/infra/storage/FileUploadHelper.java (1)
Component(13-50)
🔇 Additional comments (73)
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryResponse.java (1)
21-22: Enum value change must be propagated everywhereThe description now advertises
PENDING, while older contracts usedWAITING. Double-check that
InquiryStatusenum actually containsPENDING,- database values / migration scripts are aligned, and
- any clients or tests referencing
WAITINGare updated.A silent mismatch will surface as 400/500 errors once Jackson tries to deserialize an unmapped value.
src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java (1)
27-27: LGTM! Good addition of Swagger documentation.Implementing the
NoticeControllerDocsinterface enhances API documentation consistency across the project.src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java (1)
62-62: LGTM! Consistent with domain model updates.The addition of the
authorparameter to theupdatemethod call correctly aligns with the enhancedNoticeJpaEntitydomain model.src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java (2)
3-3: LGTM! Excellent addition of Swagger documentation.The comprehensive Swagger annotations significantly improve API documentation with clear descriptions and examples.
Also applies to: 11-11, 14-14, 20-20, 23-23
33-33: LGTM! Correctly integrated author field in entity creation.The
toEntity()method properly includes the newauthorfield when building theNoticeJpaEntity.src/main/java/life/mosu/mosuserver/domain/notice/NoticeJpaEntity.java (1)
35-36: LGTM! Properly implemented author field.The
authorfield is correctly defined as a non-nullable column with appropriate JPA annotations.src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeResponse.java (3)
3-3: LGTM! Comprehensive Swagger documentation.The added Swagger annotations provide clear API documentation with appropriate descriptions and examples.
Also applies to: 9-9, 12-12, 15-15, 18-18, 21-21, 24-24
39-48: LGTM! Well-documented nested record.The
AttachmentResponserecord is properly enhanced with Swagger annotations for improved API documentation.
34-34: Ignore the createdAt type mismatch concern
BaseTimeEntity#getCreatedAt returns a formatted String, so mapping it to theString createdAtfield in NoticeResponse is fully compatible.
• domain/base/BaseTimeEntity.java:26 –public String getCreatedAt()
• presentation/notice/dto/NoticeResponse.java:22 –String createdAtLikely an incorrect or invalid review comment.
src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeDetailResponse.java (14)
3-3: LGTM: Swagger import added correctly.The Swagger import is properly added for API documentation support.
9-25: LGTM: Comprehensive Swagger annotations with good examples.The @Schema annotations are well-structured with clear descriptions and realistic examples. This will generate excellent API documentation.
44-51: LGTM: Nested record properly enhanced with Swagger annotations.The AttachmentDetailResponse record is correctly enhanced with comprehensive Swagger annotations including realistic examples for S3 integration.
36-37: Verification complete: entity getters return String as expectedBoth
NoticeJpaEntity.getAuthor()andBaseTimeEntity.getCreatedAt()returnString, matching your DTO’s expectations:
NoticeJpaEntitydeclaresprivate String author, and its getter (generated via Lombok) returnsString.BaseTimeEntity.getCreatedAt()is explicitly defined to format and return aString.No changes are necessary here.
3-3: LGTM: Swagger import added for API documentation.Good addition of Swagger support for enhanced API documentation.
9-25: LGTM: Comprehensive Swagger annotations added to all fields.The Swagger annotations are well-structured with clear descriptions and realistic examples. The new
authorandcreatedAtfields are properly documented.
36-37: LGTM: Factory method updated with new fields.The static factory method correctly incorporates the new
authorandcreatedAtfields from the entity.
44-51: LGTM: Nested record enhanced with Swagger documentation.The
AttachmentDetailResponserecord now has proper Swagger annotations with clear descriptions and examples.
21-22: Confirmed date format consistency forcreatedAtfield
The DTO’screatedAtexample (“2025-07-08”) matches theBaseTimeEntity#getCreatedAt()implementation, which formatsLocalDateTimeasyyyy-MM-dd. No change is required to keep formats aligned. You may optionally addformat = "date"to the@Schemaannotation or switch the DTO type tojava.time.LocalDatefor stronger typing, but the current String representation is consistent.
3-3: LGTM: Swagger import added for API documentation.Good addition of Swagger annotations for enhanced API documentation.
9-25: Excellent Swagger documentation with comprehensive field descriptions.The @Schema annotations provide clear descriptions in Korean with appropriate examples, enhancing the API documentation quality.
18-22: New author and createdAt fields properly documented.The new fields are well-documented with appropriate examples. Note that
createdAtis using String type with "yyyy-MM-dd" format - ensure this format is consistently used across the application.
36-37: Factory method correctly updated to include new fields.The static factory method properly populates the new author and createdAt fields from the NoticeJpaEntity.
44-51: Nested AttachmentDetailResponse enhanced with Swagger annotations.Consistent application of Swagger documentation to the nested record with clear descriptions and examples.
src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java (9)
3-6: LGTM: Proper imports for validation and documentation.All necessary imports are correctly added for Swagger documentation, validation, and file handling.
10-26: LGTM: Comprehensive field annotations with proper validation.The @Schema annotations provide clear descriptions and realistic examples. The @NotNull validation is correctly applied to required fields while keeping optional fields (userId, attachments) without validation constraints.
25-26: FileRequest record verified and properly importedThe
FileRequesttype is defined as arecordin
- src/main/java/life/mosu/mosuserver/global/util/FileRequest.java
and is imported in NoticeUpdateRequest.java:
import life.mosu.mosuserver.global.util.FileRequest;Since the record exists and is already used elsewhere for attachment handling, no further changes are needed here.
3-3: LGTM: Swagger import added for API documentation.Good addition of Swagger support for enhanced API documentation.
10-26: LGTM: Comprehensive field annotations with validation.The addition of Swagger annotations and validation constraints is well-implemented. The new
authorfield properly has@NotNullvalidation.
3-3: LGTM: Swagger import added for API documentation.Proper addition of Swagger annotations for enhanced API documentation.
10-16: Comprehensive Swagger documentation for existing fields.Good addition of @Schema annotations with Korean descriptions and validation annotations for title and content fields.
18-20: Author field properly implemented with validation.The new author field is correctly marked as required with @NotNull validation and proper Swagger documentation.
25-26: Attachments field properly configured for file uploads.The attachments field supports file uploads using the FileRequest type, which aligns with the service layer file handling capabilities.
src/main/java/life/mosu/mosuserver/presentation/notice/NoticeControllerDocs.java (20)
1-22: LGTM: Comprehensive imports and package structure.All necessary Swagger/OpenAPI imports are correctly included for building comprehensive API documentation.
23-24: LGTM: Proper API grouping with descriptive tag.The @tag annotation correctly groups all notice-related endpoints under "Notice API" with a clear Korean description.
26-32: LGTM: Well-documented create operation.The create operation is properly documented with:
- Clear summary and description
- Appropriate 201 response code
- Proper parameter validation annotation
34-44: LGTM: Comprehensive list operation with pagination.The list operation properly documents:
- Pagination parameters with default values
- Response schema reference
- Clear parameter descriptions in both name and description fields
46-54: LGTM: Detailed retrieval operation.The detail operation correctly documents:
- Path parameter extraction
- Response schema reference
- Clear operation purpose
56-63: LGTM: Proper delete operation documentation.The delete operation is well-documented with appropriate authorization context mentioning admin permissions.
65-75: LGTM: Comprehensive update operation.The update operation properly documents:
- Path parameter for resource identification
- Request body validation
- Admin permission requirements
- Clear parameter descriptions
30-74: ApiResponseWrapper generic implementation is correct.
Verified thatApiResponseWrapper<T>properly wraps status, message, and data across all endpoints—no changes needed.
1-22: LGTM: Well-structured imports and interface setup.The imports are comprehensive and the interface is properly structured for Swagger documentation.
23-24: LGTM: Proper API documentation tagging.The
@Tagannotation provides clear grouping for the Notice API endpoints.
26-32: LGTM: Clear create operation documentation.The create operation is well-documented with appropriate response codes and parameter descriptions.
34-44: LGTM: Comprehensive list operation documentation.The list operation includes proper pagination parameter documentation and response schema specification.
46-54: LGTM: Clear detail operation documentation.The detail operation properly documents the path parameter and response schema.
1-22: Excellent imports and package structure for Swagger documentation.Comprehensive import statements covering all necessary Swagger annotations and Spring components for API documentation.
23-24: Good use of @tag for API grouping.The @tag annotation with Korean description properly groups the notice-related endpoints in the API documentation.
26-32: Create notice endpoint well-documented.Comprehensive documentation with proper operation summary, description, and response specification for the create notice endpoint.
34-44: List notices endpoint with proper pagination documentation.Excellent documentation including pagination parameters with default values and proper response schema specification.
46-54: Detail notice endpoint properly documented.Clear documentation for the notice detail retrieval with proper path parameter specification and response schema.
56-63: Delete notice endpoint with admin privilege indication.Good documentation that clearly indicates this is an admin-only operation with proper parameter specification.
65-74: Update notice endpoint comprehensively documented.Complete documentation for the update operation with proper parameter descriptions and admin privilege indication.
src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqUpdateRequest.java (1)
8-24: LGTM! Well-structured DTO with comprehensive validation and documentation.The record implementation follows best practices with:
- Proper validation annotations on required fields
- Comprehensive Swagger documentation with examples
- Consistent structure with other DTOs in the codebase
- Appropriate handling of optional attachments field
src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java (1)
60-65: No duplicated s3Key parameter in getPreSignedUrl()The call to
s3Service.getPreSignedUrlinFaqAttachmentService.javaonly passes two arguments (attachment.getS3Key()and theDuration), which matches its signature:public String getPreSignedUrl(String s3Key, Duration expireDuration)The subsequent
attachment.getS3Key()on line 64 is the third argument to theFaqResponse.AttachmentResponseconstructor—not togetPreSignedUrl. No changes are needed here.Likely an incorrect or invalid review comment.
src/main/java/life/mosu/mosuserver/domain/faq/FaqJpaEntity.java (3)
31-32: LGTM! Properly implemented author field with appropriate JPA annotations.The author field is correctly defined with proper database column mapping and non-null constraint.
38-44: LGTM! Constructor updated consistently with new author parameter.The constructor properly includes the author parameter and initializes the field correctly.
46-50: LGTM! Well-implemented update method for entity modification.The update method provides a clean way to modify the entity's core fields and follows proper encapsulation principles.
src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqCreateRequest.java (3)
3-3: LGTM! Comprehensive Swagger documentation added to all fields.The Swagger schema annotations provide excellent API documentation with clear descriptions and examples for all fields.
Also applies to: 11-11, 14-14, 17-18, 20-20, 23-23
17-18: LGTM! Author field properly added with validation.The author field is correctly implemented with proper validation and documentation.
32-32: LGTM! toEntity() method updated consistently.The method properly includes the author field when building the entity, maintaining consistency with the domain model.
src/main/java/life/mosu/mosuserver/application/faq/FaqService.java (1)
45-51: LGTM: Well-implemented detail retrieval method.The method correctly uses read-only transaction with SUPPORTS propagation for optimal performance and follows the established error handling pattern.
src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqResponse.java (2)
8-12: LGTM: Excellent Swagger documentation and field naming.The Swagger annotations provide clear API documentation, and the field name changes (
question/answer) are more descriptive than the previoustitle/content.
25-31: Well-structured AttachmentResponse enhancement.The addition of the
s3Keyfield with proper Swagger annotations provides useful metadata for API consumers.src/main/java/life/mosu/mosuserver/presentation/faq/FaqController.java (4)
27-27: LGTM: Good practice implementing the documentation interface.Implementing
FaqControllerDocsensures consistency between API documentation and actual endpoints.
34-35: LGTM: Proper validation and method naming.The addition of
@Validannotation and descriptive method namecreateFaqimproves code clarity and ensures request validation.
49-54: LGTM: Well-implemented detail retrieval endpoint.The new endpoint follows established patterns and provides proper error handling with consistent response wrapping.
56-64: LGTM: Consistent update endpoint implementation.The update endpoint properly validates input and follows the established response pattern. Good use of
@Validannotation for request validation.src/main/java/life/mosu/mosuserver/presentation/faq/FaqControllerDocs.java (6)
23-24: LGTM: Excellent API documentation interface design.The interface provides a clean separation between API documentation and implementation, following good architectural practices.
26-32: LGTM: Comprehensive and well-structured API documentation.The Swagger annotations provide clear, detailed documentation with appropriate Korean descriptions and proper parameter definitions.
34-49: LGTM: Detailed pagination documentation.The list endpoint documentation properly describes pagination parameters and response structure using
ArraySchema.
51-58: LGTM: Clear detail retrieval documentation.The documentation clearly describes the detail retrieval endpoint with proper parameter and response definitions.
60-67: LGTM: Consistent delete operation documentation.The delete endpoint documentation follows the established pattern with clear parameter descriptions.
69-79: LGTM: Complete update operation documentation.The update endpoint documentation properly describes both path parameters and request body with appropriate validation annotations.
| @Column(name = "author", nullable = false) | ||
| private String author; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify database migration for the new non-nullable author field.
Adding a non-nullable field to an existing entity may cause issues if there are existing records in the database. Ensure that a proper database migration script is in place to handle existing data.
🏁 Script executed:
#!/bin/bash
# Check if there are any database migration files for the author field
fd -e sql -e xml migration | xargs grep -l "author\|notice" | head -10Length of output: 72
To locate any existing migration scripts, let’s list all SQL and XML files in the repo:
🏁 Script executed:
#!/bin/bash
# List all SQL and XML files to check for migration scripts
fd -e sql -e xmlLength of output: 56
Add a database migration for the new non-nullable author field
I didn’t find any SQL or XML migration scripts in the repo, so you need to:
- Create a migration (Flyway, Liquibase, etc.) to
• Add theauthorcolumn to thenoticetable
• Backfill existing records (e.g. with a default or placeholder)
• Alter the column toNOT NULL - Place it under your migrations folder (e.g.
src/main/resources/db/migration) with a proper versioned filename (e.g.V2__add_author_to_notice.sql)
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/notice/NoticeJpaEntity.java at
lines 35-36, you added a new non-nullable 'author' field but did not create a
corresponding database migration. To fix this, create a new migration script
(e.g., Flyway SQL) in the migrations folder (like
src/main/resources/db/migration) with a versioned filename such as
V2__add_author_to_notice.sql. The script should add the 'author' column to the
'notice' table, backfill existing rows with a default or placeholder value, and
then alter the column to be NOT NULL to match the entity definition.
| public class S3Properties { | ||
|
|
||
| private final int presignedUrlExpirationMinutes; | ||
| private int presignedUrlExpirationMinutes; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider preserving immutability for configuration properties.
The change from private final int to private int removes the immutability guarantee. Configuration properties are typically immutable to prevent accidental modification and ensure thread safety.
Consider reverting to the immutable design pattern:
-@Data
-@Component
+@Getter
+@RequiredArgsConstructor
@ConfigurationProperties(prefix = "aws.s3")
@Slf4j
public class S3Properties {
- private int presignedUrlExpirationMinutes;
+ private final int presignedUrlExpirationMinutes;If Spring component management is required, consider using @ConfigurationProperties with @EnableConfigurationProperties instead.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/infra/property/S3Properties.java at line
15, the field presignedUrlExpirationMinutes was changed from final to non-final,
removing immutability. Revert this field to be private final to preserve
immutability and thread safety. If you need Spring to manage this configuration
property, use @ConfigurationProperties on the class and enable it with
@EnableConfigurationProperties in your configuration, rather than making the
field mutable.
| import jakarta.annotation.PostConstruct; | ||
| import lombok.Getter; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.Data; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Significant architectural change from immutable to mutable design.
The class has been changed from an immutable configuration properties class to a mutable Spring component. This introduces several concerns:
- Thread Safety: The field is no longer
final, making the class potentially unsafe for concurrent access - Design Intent: Configuration properties are typically immutable by design
- Dependency Injection: The change from constructor injection to field injection may impact testability
Please verify that this change is intentional and consider the thread safety implications, especially since this class is used in services that may be accessed concurrently.
Also applies to: 7-7, 9-10
🏁 Script executed:
#!/bin/bash
# Search for all usages of S3Properties to understand the impact
rg -A 3 -B 3 "S3Properties"Length of output: 12305
Use constructor binding for S3Properties to enforce immutability and thread-safety
The class is now a Spring singleton bound via setter injection (Lombok’s @DaTa + non-final field), which makes it mutable at runtime. Configuration properties are safer when immutable:
• File: src/main/java/life/mosu/mosuserver/infra/property/S3Properties.java
– Remove Lombok’s @DaTa; use @Getter (or @value) to eliminate setters
– Change private int presignedUrlExpirationMinutes; → private final int presignedUrlExpirationMinutes;
– Add Spring Boot’s @ConstructorBinding on the class (requires Spring Boot 2.2+)
– Provide a single constructor and remove any no-args constructor or setters
If you prefer setter binding, please confirm that no code invokes setters at runtime and document this choice to avoid unexpected mutations.
This will restore immutability, preserve thread safety for your singleton bean, and align with best practices for configuration properties.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/infra/property/S3Properties.java at line
4, replace Lombok's @Data annotation with @Getter or @Value to remove setters
and enforce immutability. Change the field presignedUrlExpirationMinutes to be
final. Add Spring Boot's @ConstructorBinding annotation to the class and provide
a single constructor initializing all fields, removing any no-args constructor
or setters to ensure the class is immutable and thread-safe as a configuration
properties singleton.
| faq.getId(), | ||
| faq.getQuestion(), | ||
| faq.getAnswer(), | ||
| faq.getCreatedAt().substring(0, 10), // 일자만 반환 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fragile date extraction logic - consider using proper date formatting.
The substring(0, 10) approach assumes a specific string format and could break if the date format changes.
Recommended improvement:
- faq.getCreatedAt().substring(0, 10), // 일자만 반환
+ faq.getCreatedAt().toLocalDate().toString(), // 안전한 날짜 추출Or if getCreatedAt() returns a string, consider using DateTimeFormatter:
- faq.getCreatedAt().substring(0, 10), // 일자만 반환
+ LocalDateTime.parse(faq.getCreatedAt()).format(DateTimeFormatter.ofPattern("yyyy-MM-dd")),Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqResponse.java at
line 20, the code uses substring(0, 10) on the date string returned by
getCreatedAt(), which is fragile and format-dependent. To fix this, parse the
date string into a LocalDate or LocalDateTime object using an appropriate
formatter, then format it back to a string with DateTimeFormatter to reliably
extract the date portion. This ensures the code handles date formats robustly
and avoids errors if the input format changes.
| //TODO: 관리자 권한 체크 추가 | ||
| @PostMapping | ||
| public ResponseEntity<ApiResponseWrapper<Void>> create(@RequestBody FaqCreateRequest request) { | ||
| // @PreAuthorize("isAuthenticated() and hasRole('ADMIN')") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the commented security annotations.
The @PreAuthorize annotations are commented out across multiple endpoints. This represents a security concern that should be addressed.
Please verify if these security annotations should be enabled or if there's a planned timeline for implementing proper authorization.
Also applies to: 57-57, 68-68
🏁 Script executed:
#!/bin/bash
# Search for other security patterns in the codebase
rg -A 3 -B 3 "PreAuthorize.*ADMIN"Length of output: 6548
Enable security checks on admin endpoints
The @PreAuthorize("isAuthenticated() and hasRole('ADMIN')") annotations are currently commented out on several critical endpoints, leaving them unprotected. Please re-enable these annotations or implement an equivalent authorization mechanism for the following methods:
-
FaqController (
src/main/java/life/mosu/mosuserver/presentation/faq/FaqController.java):
• createFaq (formerly at line 33)
• updateFaq (formerly at line 57)
• deleteFaq (formerly at line 68) -
AdminController (
src/main/java/life/mosu/mosuserver/presentation/admin/AdminController.java):
• getStudents
• downloadStudentInfo
• getLunchCounts
• getApplications
• downloadApplicationInfo
If you’re planning to secure these via another approach (e.g., custom filter, AOP interceptor), please outline the timeline or provide details of the planned solution.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/faq/FaqController.java at
lines 33, 57, and 68, the @PreAuthorize("isAuthenticated() and
hasRole('ADMIN')") annotations are commented out, leaving admin endpoints
unprotected. To fix this, uncomment these annotations on the createFaq,
updateFaq, and deleteFaq methods to enforce security checks ensuring only
authenticated users with the ADMIN role can access them. If using another
security mechanism, provide details or timeline for its implementation.
| @Schema(description = "작성자 ID (토큰에서 추출 예정)", example = "42") | ||
| Long userId, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the purpose and usage of userId field.
The userId field is described as "작성자 ID (토큰에서 추출 예정)" but remains nullable. Consider:
- If it's extracted from token, should it be removed from the request DTO?
- If it's needed for validation, should it have appropriate constraints?
- Document when this field should be populated vs. when it's null.
🏁 Script executed:
#!/bin/bash
# Check how userId is used in the service layer
ast-grep --pattern 'updateNotice($_, $request) {
$$$
}'Length of output: 59
🏁 Script executed:
#!/bin/bash
# Search for service method handling updates and where NoticeUpdateRequest is referenced
rg -n "updateNotice" -n src
rg -n "NoticeUpdateRequest" -n srcLength of output: 1652
🏁 Script executed:
#!/bin/bash
# Display the updateNotice method implementation for context
rg -n "public void updateNotice" -n src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java -A20
# Check for any references to request.userId in the codebase
rg -n "request.userId" -n src
# Inspect if the security context or token extraction is happening anywhere
rg -n "SecurityContextHolder" -n srcLength of output: 1717
Remove or Properly Wire the Unused userId Field
The userId property in NoticeUpdateRequest is never consumed by your service layer—NoticeService.updateNotice only uses request.title(), request.content(), and request.author(). Keeping an unused, nullable field in the DTO can be confusing.
Please choose one of the following:
- Remove the
userIdfield (and update any related OpenAPI docs) if you don’t need it in the request body. - Inject the author’s ID via your
@UserIdargument resolver in the controller instead of via JSON. For example:Then update the service signature to accept the extractedpublic ResponseEntity<Void> updateNotice( @PathVariable Long noticeId, @Valid @RequestBody NoticeUpdateRequest request, @UserId Long userId ) { noticeService.updateNotice(noticeId, request, userId); … }
userId.
File needing changes:
- src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java:22-23
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java
at lines 22-23, the userId field is unused in the service layer and should be
removed to avoid confusion. Delete the userId property from the DTO and update
any related OpenAPI documentation accordingly. Alternatively, if userId is
needed, remove it from the DTO and instead inject it in the controller method
using the @UserId argument resolver, then update the service method signature to
accept this userId parameter.
| @Operation(summary = "공지사항 수정", description = "특정 공지사항을 수정합니다. (관리자 권한)") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "공지사항 수정 성공") | ||
| }) | ||
| ResponseEntity<ApiResponseWrapper<Void>> updateNotice( | ||
| @Parameter(name = "noticeId", description = "수정할 공지사항 ID", in = ParameterIn.PATH) | ||
| @PathVariable Long noticeId, | ||
| @Parameter(description = "수정할 공지사항 정보") | ||
| @Valid @RequestBody NoticeUpdateRequest request | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding security annotations for admin operations.
Similar to the delete operation, the update operation mentions "(관리자 권한)" but lacks security annotations. Consider adding @PreAuthorize or similar security annotations to enforce admin-only access.
@Operation(summary = "공지사항 수정", description = "특정 공지사항을 수정합니다. (관리자 권한)")
+@PreAuthorize("hasRole('ADMIN')")
@ApiResponses(value = {📝 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.
| @Operation(summary = "공지사항 수정", description = "특정 공지사항을 수정합니다. (관리자 권한)") | |
| @ApiResponses(value = { | |
| @ApiResponse(responseCode = "200", description = "공지사항 수정 성공") | |
| }) | |
| ResponseEntity<ApiResponseWrapper<Void>> updateNotice( | |
| @Parameter(name = "noticeId", description = "수정할 공지사항 ID", in = ParameterIn.PATH) | |
| @PathVariable Long noticeId, | |
| @Parameter(description = "수정할 공지사항 정보") | |
| @Valid @RequestBody NoticeUpdateRequest request | |
| ); | |
| @Operation(summary = "공지사항 수정", description = "특정 공지사항을 수정합니다. (관리자 권한)") | |
| @PreAuthorize("hasRole('ADMIN')") | |
| @ApiResponses(value = { | |
| @ApiResponse(responseCode = "200", description = "공지사항 수정 성공") | |
| }) | |
| ResponseEntity<ApiResponseWrapper<Void>> updateNotice( | |
| @Parameter(name = "noticeId", description = "수정할 공지사항 ID", in = ParameterIn.PATH) | |
| @PathVariable Long noticeId, | |
| @Parameter(description = "수정할 공지사항 정보") | |
| @Valid @RequestBody NoticeUpdateRequest request | |
| ); |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/notice/NoticeControllerDocs.java
around lines 65 to 74, the updateNotice method lacks security annotations
enforcing admin-only access despite mentioning admin privileges. Add a security
annotation such as @PreAuthorize with an expression that restricts access to
admin roles, similar to the delete operation, to ensure only authorized
administrators can perform the update.
| @Operation(summary = "공지사항 삭제", description = "특정 공지사항을 삭제합니다. (관리자 권한)") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "공지사항 삭제 성공") | ||
| }) | ||
| ResponseEntity<ApiResponseWrapper<Void>> deleteNotice( | ||
| @Parameter(name = "noticeId", description = "삭제할 공지사항 ID", in = ParameterIn.PATH) | ||
| @PathVariable Long noticeId | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding security annotations for admin operations.
The delete operation mentions "(관리자 권한)" but lacks security annotations. Consider adding @PreAuthorize or similar security annotations to enforce admin-only access.
@Operation(summary = "공지사항 삭제", description = "특정 공지사항을 삭제합니다. (관리자 권한)")
+@PreAuthorize("hasRole('ADMIN')")
@ApiResponses(value = {📝 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.
| @Operation(summary = "공지사항 삭제", description = "특정 공지사항을 삭제합니다. (관리자 권한)") | |
| @ApiResponses(value = { | |
| @ApiResponse(responseCode = "200", description = "공지사항 삭제 성공") | |
| }) | |
| ResponseEntity<ApiResponseWrapper<Void>> deleteNotice( | |
| @Parameter(name = "noticeId", description = "삭제할 공지사항 ID", in = ParameterIn.PATH) | |
| @PathVariable Long noticeId | |
| ); | |
| @Operation(summary = "공지사항 삭제", description = "특정 공지사항을 삭제합니다. (관리자 권한)") | |
| @PreAuthorize("hasRole('ADMIN')") | |
| @ApiResponses(value = { | |
| @ApiResponse(responseCode = "200", description = "공지사항 삭제 성공") | |
| }) | |
| ResponseEntity<ApiResponseWrapper<Void>> deleteNotice( | |
| @Parameter(name = "noticeId", description = "삭제할 공지사항 ID", in = ParameterIn.PATH) | |
| @PathVariable Long noticeId | |
| ); |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/notice/NoticeControllerDocs.java
around lines 56 to 63, the deleteNotice method lacks security annotations to
enforce admin-only access despite the description indicating it requires admin
privileges. Add a security annotation such as @PreAuthorize("hasRole('ADMIN')")
above the method to restrict access to administrators only.
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor