Conversation
WalkthroughThis change refactors attachment handling across notices and inquiries by adding a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NoticeController
participant NoticeService
participant NoticeAttachmentService
participant S3Service
Client->>NoticeController: createNotice(request, principalDetails)
NoticeController->>NoticeService: createNotice(request, user)
NoticeService->>NoticeAttachmentService: updateAttachment(files, noticeEntity)
NoticeAttachmentService->>NoticeAttachmentService: deleteAttachment(noticeEntity)
NoticeAttachmentService->>NoticeAttachmentService: createAttachment(files, noticeEntity)
NoticeAttachmentService->>S3Service: getPublicUrl(s3Key)
NoticeService-->>NoticeController: void
NoticeController-->>Client: ResponseEntity
Client->>NoticeController: updateNotice(id, request, principalDetails)
NoticeController->>NoticeService: updateNotice(id, request, user)
NoticeService->>NoticeAttachmentService: updateAttachment(files, noticeEntity)
NoticeAttachmentService->>NoticeAttachmentService: deleteAttachment(noticeEntity)
NoticeAttachmentService->>NoticeAttachmentService: createAttachment(files, noticeEntity)
NoticeService-->>NoticeController: void
NoticeController-->>Client: ResponseEntity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 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 refactors the handling of notice attachments by transitioning from using S3 pre-signed URLs to public S3 URLs. Additionally, it includes general code cleanup by removing commented-out sections and renaming a method for better clarity and maintainability.
Highlights
- S3 URL Handling: The core change involves switching from generating S3 pre-signed URLs to using public S3 URLs for notice attachments, simplifying how these files are accessed.
- Code Refactoring: The
getNoticeOrThrowmethod inNoticeServicehas been renamed togetNoticefor improved readability and consistency across the codebase. - Code Cleanup: Unused and commented-out code related to attachment responses has been removed from
NoticeAttachmentServiceand theNoticeResponseDTOs, contributing to a cleaner codebase.
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. ↩
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
…ng and attachment updates
…lete and create operations into update method
…type mismatch and resource not found responses
…cking and update attachment handling
…cessary request path logging
jbh010204
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java (1)
4-4: Remove unused import.The
HttpServletRequestimport is not used anywhere in the class.-import jakarta.servlet.http.HttpServletRequest;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java(5 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/notice/NoticeAttachmentService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java(4 hunks)src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/entity/InquiryAnswerJpaEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java(5 hunks)src/main/java/life/mosu/mosuserver/infra/persistence/s3/AttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/admin/AdminInquiryController.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/admin/docs/AdminInquiryControllerDocs.java(4 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java(4 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/NoticeControllerDocs.java(3 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java(0 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeUpdateRequest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/life/mosu/mosuserver/application/notice/NoticeAttachmentService.java
- src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java
🔇 Additional comments (27)
src/main/java/life/mosu/mosuserver/infra/persistence/s3/AttachmentService.java (1)
11-11: LGTM! Clean interface extension.The new
updateAttachmentmethod provides a unified interface for attachment updates, which improves consistency across implementations and simplifies client code that previously had to call delete + create separately.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java (1)
73-73: LGTM! Proper attachment cleanup.Good change to uncomment the attachment deletion - this ensures complete cleanup of inquiry-related resources when an inquiry is deleted.
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java (1)
56-63: LGTM! Clean implementation of the update pattern.The
updateAttachmentmethod correctly implements the delete-then-create pattern for updating attachments. This approach ensures all old attachments are removed before adding new ones, preventing orphaned records.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java (1)
41-48: LGTM! Consistent update implementation.The
updateAttachmentmethod implementation is clean and consistent with the pattern used in other attachment services. The delete-then-create approach ensures proper cleanup of old attachments.src/main/java/life/mosu/mosuserver/presentation/notice/dto/NoticeCreateRequest.java (1)
23-29: Excellent security improvement!The refactoring to extract user information from the authenticated
UserJpaEntityinstead of accepting it from the client request is a significant security enhancement. This prevents clients from spoofing author identity and ensures user context is controlled by the authentication system.src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/entity/InquiryAnswerJpaEntity.java (3)
37-38: LGTM! Proper JPA field addition.The new
authorfield is correctly defined with appropriate JPA annotations and constraints.
43-57: LGTM! Constructor properly updated.The constructor correctly includes the new
authorparameter and assigns it to the field, maintaining proper initialization order.
59-63: LGTM! Update method enhanced correctly.The update method properly includes the author parameter and updates the field accordingly.
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerRequest.java (1)
18-26: LGTM! Excellent security improvement.Removing
userIdfrom the request payload and using the authenticated user context is a security best practice. This prevents users from impersonating others by manipulating the request.src/main/java/life/mosu/mosuserver/presentation/notice/NoticeController.java (2)
37-42: LGTM! Proper authentication context integration.The controller correctly accepts the authenticated principal and passes the user context to the service layer while maintaining proper authorization checks.
68-76: LGTM! Consistent authentication pattern.The update method follows the same authentication pattern as the create method, ensuring consistency across the API.
src/main/java/life/mosu/mosuserver/presentation/notice/NoticeControllerDocs.java (2)
32-38: LGTM! Excellent API documentation practice.The Swagger documentation correctly hides the authentication parameter from public docs while providing clear descriptions for internal use.
75-82: LGTM! Consistent documentation pattern.The updateNotice method documentation follows the same pattern as createNotice, maintaining consistency in API documentation.
src/main/java/life/mosu/mosuserver/presentation/admin/AdminInquiryController.java (2)
54-61: LGTM! Proper authentication integration.The inquiry answer creation method correctly accepts and uses the authenticated principal, maintaining security while adding user context.
65-72: LGTM! Consistent authentication pattern.The update method follows the same authentication pattern, ensuring consistency across admin inquiry operations.
src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java (4)
129-140: LGTM! Proper 404 error handling added.The new handler correctly handles
NoResourceFoundExceptionwith appropriate status code and user-friendly Korean message.
142-152: LGTM! Type mismatch handling with clear error code.The handler provides clear error messaging for type mismatch issues and includes a specific error code "TYPE_MISMATCH" for better client-side handling.
168-170: LGTM! Code formatting improvement.The method signature reformatting improves readability without changing functionality.
185-185: LGTM! Null-safe exception cause handling.The null check prevents potential NPE when
ex.getCause()returns null, providing a fallback message "Unknown Cause".src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java (6)
32-32: LGTM! User context added to service method.The method signature now properly accepts user context for authorization and audit trail purposes.
39-39: LGTM! User entity passed to DTO conversion.The
toEntitymethod now receives the user entity, enabling proper author tracking in the inquiry answer.
60-60: LGTM! Proper attachment cleanup added.The addition of
deleteAttachmentcall ensures proper cleanup of associated attachments when deleting an inquiry answer.
75-75: LGTM! User context added for updates.The method signature consistently includes user context for update operations.
79-79: LGTM! Author tracking in updates.The update method now properly tracks the author using
user.getName(), maintaining audit information.
82-82: LGTM! Simplified attachment update handling.The new
updateAttachmentmethod provides a cleaner abstraction for managing attachment updates, replacing the previous delete-and-create pattern.src/main/java/life/mosu/mosuserver/presentation/admin/docs/AdminInquiryControllerDocs.java (2)
47-48: LGTM! Authentication principal properly documented.The
@AuthenticationPrincipal PrincipalDetailsparameter is correctly added with appropriate Swagger annotations. The parameter is hidden from public API docs but includes a clear description about admin privilege requirements.
60-61: LGTM! Consistent authentication parameter.The authentication principal parameter is consistently applied to the update method with the same documentation patterns as the create method.
| inquiryAnswerJpaRepository.findByInquiryId(postId).ifPresent(answer -> { | ||
| inquiryAnswerService.deleteInquiryAnswer(postId); | ||
| }); | ||
| inquiryAnswerService.deleteInquiryAnswer(postId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that deleteInquiryAnswer handles missing answers gracefully.
The unconditional call to deleteInquiryAnswer simplifies the deletion logic, but ensure that this method can handle cases where no inquiry answer exists without throwing exceptions.
🏁 Script executed:
#!/bin/bash
# Description: Check if deleteInquiryAnswer handles missing answers gracefully
# Expected: Method should handle cases where no answer exists
ast-grep --pattern 'deleteInquiryAnswer($$$) {
$$$
}'Length of output: 57
🏁 Script executed:
#!/bin/bash
# Locate the InquiryAnswerService class and the implementation of deleteInquiryAnswer
rg -n "class InquiryAnswerService" -n src/main/java
rg -n "deleteInquiryAnswer" -n src/main/javaLength of output: 950
Further inspecting deleteInquiryAnswer implementation to confirm it handles missing entries without exceptions.
🏁 Script executed:
#!/bin/bash
# Show implementation of deleteInquiryAnswer with surrounding context
rg -n -A20 "public void deleteInquiryAnswer" src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.javaLength of output: 1161
Ensure deleteInquiryAnswer handles missing answers without throwing
The current implementation in InquiryAnswerService.java (lines 54–56) will throw a CustomRuntimeException when no answer exists, causing deleteInquiry to fail for inquiries that simply have no answers. To make deletion idempotent, update the method to only delete if an answer is found:
• File: src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java
• Method: public void deleteInquiryAnswer(Long postId)
Suggested change:
- InquiryAnswerJpaEntity answerEntity = inquiryAnswerJpaRepository.findByInquiryId(postId)
- .orElseThrow(() -> new CustomRuntimeException(ErrorCode.INQUIRY_ANSWER_NOT_FOUND));
-
- inquiryAnswerJpaRepository.delete(answerEntity);
- inquiryEntity.updateStatusToPending();
- answerAttachmentService.deleteAttachment(answerEntity);
+ inquiryAnswerJpaRepository.findByInquiryId(postId)
+ .ifPresent(answerEntity -> {
+ inquiryAnswerJpaRepository.delete(answerEntity);
+ inquiryEntity.updateStatusToPending();
+ answerAttachmentService.deleteAttachment(answerEntity);
+ });This ensures that deleting an inquiry without an existing answer will not throw an exception but simply no-op the answer cleanup.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java
around lines 54 to 56, the method deleteInquiryAnswer currently throws a
CustomRuntimeException if no answer exists for the given postId. To make
deletion idempotent and avoid exceptions when no answer is found, modify
deleteInquiryAnswer to first check if an answer exists for the postId and only
perform the delete operation if it does. If no answer is found, simply return
without throwing an exception.
…entities and requests
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/life/mosu/mosuserver/domain/inquiry/entity/InquiryJpaEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/entity/InquiryAnswerJpaEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/life/mosu/mosuserver/domain/inquiry/entity/InquiryJpaEntity.java
🔇 Additional comments (6)
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java (2)
5-5: LGTM!The
@Sizeimport addition is appropriate and necessary for the validation constraints added to the title and content fields.
17-17: LGTM!The content field validation is correctly implemented with consistent constraint and error message (both 1000 characters maximum).
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/entity/InquiryAnswerJpaEntity.java (4)
31-31: Content length constraint added.The content field now has a 1000 character limit. Ensure this aligns with business requirements and UI constraints.
37-38: New author field added appropriately.The new author field is properly defined as non-nullable and will store the author's name directly in the entity, which aligns with the refactoring goals mentioned in the AI summary.
48-50: Constructor updated correctly for new author field.The constructor properly includes the new author parameter and assigns it to the field. The parameter ordering and assignment logic look correct.
Also applies to: 56-56
59-59: Update method enhanced with author parameter.The update method now accepts and updates the author field, which maintains consistency with the entity's new structure and supports the refactored user context handling.
Also applies to: 62-62
| private Long id; | ||
|
|
||
| @Column(name = "title", nullable = false, length = 3000) | ||
| @Column(name = "title", nullable = false, length = 300) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the title length reduction impact.
The title column length was reduced from 3000 to 300 characters. This is a significant reduction that could cause data truncation if existing records exceed the new limit.
🏁 Script executed:
#!/bin/bash
# Description: Check for existing inquiry answer titles that exceed 300 characters
# Expected: Should return any titles longer than 300 characters that would be truncated
# Search for the table structure and any existing data concerns
rg -A 5 -B 5 "inquiry_answer" --type sql
# Look for any migration files that handle this change
fd -e sql -e java | xargs rg -l "ALTER.*inquiry_answer.*title|title.*300"Length of output: 435
Add a DB migration for title length reduction
We didn’t detect any SQL migration altering the inquiry_answer.title column from 3000 → 300 characters. Without a dedicated Flyway/Liquibase (or equivalent) script, existing records > 300 chars will either be truncated or cause errors in production.
Suggested fixes:
- Add a migration in
resources/db/migration/...(or your project’s convention) to alter the column, for example:ALTER TABLE inquiry_answer ALTER COLUMN title TYPE VARCHAR(300);
- Audit or migrate existing data so no titles exceed 300 characters (or decide on truncation strategy).
- Confirm your CI/integration tests cover reads/writes around the 300-char boundary.
- Reference the JPA mapping in
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/entity/InquiryAnswerJpaEntity.java(line 28).
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/entity/InquiryAnswerJpaEntity.java
at line 28, the title column length was reduced from 3000 to 300 characters in
the JPA entity but no corresponding database migration exists. To fix this,
create a new SQL migration script in the resources/db/migration directory that
alters the inquiry_answer.title column to VARCHAR(300). Additionally, audit
existing data to ensure no titles exceed 300 characters or implement a
truncation strategy. Finally, verify that your tests cover scenarios with title
lengths near the 300-character limit.
|
|
||
| public record InquiryCreateRequest( | ||
|
|
||
| @Size(max = 100, message = "제목은 최대 300자까지 입력 가능합니다.") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix inconsistency between validation constraint and error message.
The validation constraint allows maximum 100 characters (max = 100) but the error message states "최대 300자까지 입력 가능합니다" (maximum 300 characters). This creates user confusion and inconsistent behavior.
Please clarify the intended maximum length and apply one of these fixes:
Option 1: If 100 characters is correct:
-@Size(max = 100, message = "제목은 최대 300자까지 입력 가능합니다.")
+@Size(max = 100, message = "제목은 최대 100자까지 입력 가능합니다.")Option 2: If 300 characters is correct (aligning with database entity):
-@Size(max = 100, message = "제목은 최대 300자까지 입력 가능합니다.")
+@Size(max = 300, message = "제목은 최대 300자까지 입력 가능합니다.")Let me verify the database entity constraint:
🏁 Script executed:
#!/bin/bash
# Description: Check the InquiryJpaEntity title column constraint to determine the intended maximum length
# Expected: Find the actual column length definition in the entity class
ast-grep --pattern 'class InquiryJpaEntity {
$$$
@Column($$$)
$$$
String title;
$$$
}'
# Also search for any column annotations on title field
rg -A 3 -B 1 "String title" --type javaLength of output: 18300
Align title validation with database constraint
The InquiryJpaEntity defines the title column with length = 300, so the DTO’s @Size must allow up to 300 characters.
• File to update:
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java
• Apply this diff:
- @Size(max = 100, message = "제목은 최대 300자까지 입력 가능합니다.")
+ @Size(max = 300, message = "제목은 최대 300자까지 입력 가능합니다.")📝 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.
| @Size(max = 100, message = "제목은 최대 300자까지 입력 가능합니다.") | |
| @Size(max = 300, message = "제목은 최대 300자까지 입력 가능합니다.") |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java
at line 13, the @Size annotation on the title field incorrectly limits the
length to 100 characters, while the database column allows up to 300. Update the
max attribute in the @Size annotation to 300 and adjust the validation message
accordingly to reflect the correct maximum length.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
Refactor
Bug Fixes
Style