MOSU-46 feat: 관리자 문의 기능 구현(리뷰 오네가이시마스) #54
Conversation
jbh010204
changed the title
|
@jbh010204 닫고싶어요 싹싹김치 |
jbh010204
changed the title
polyglot-k
left a comment
polyglot-k
left a comment
There was a problem hiding this comment.
대체로 사이드 이펙트가 발생할 부분은 적어보이며 깔끔하게 잘 진행해주신거 같습니다.
체크한 부분 확인 후, 댓글 작성하고 재요청하면 Approve 해드리겠습니다.
| @Value("${aws.s3.presigned-url-expiration-minutes}") | ||
| private int durationTime; |
There was a problem hiding this comment.
이거는 그냥 개인 컨벤션 차이인데,
Value 자체를 Properties 객체에서 주입해서 Properties를 생성자 주입받는 방식도 있어서, 한번 그런게 있구나 생각하보셔도 좋을거 같아요
There was a problem hiding this comment.
아 저도 이 부분에 대해서 처리를 하고 싶었는데 몰라서 일단 적었는데, 좋은 방법 알려주셔서 감사합니다!
| requests.forEach(req -> { | ||
| fileUploadHelper.updateTag(req.s3Key()); | ||
| attachmentReposiory.save(req.toInquiryAnswerAttachmentEntity( | ||
| req.fileName(), req.s3Key(), inquiryId | ||
| )); |
There was a problem hiding this comment.
이 것도 개인 취향인거 같은데. 이런거 내부 로직을 private 으로 묶어도 좋을거 같아요!
| return attachments.stream() | ||
| .map(attachment -> new InquiryDetailResponse.AttachmentResponse( | ||
| attachment.getFileName(), | ||
| s3Service.getPreSignedUrl( | ||
| attachment.getS3Key(), | ||
| Duration.ofMinutes(durationTime) | ||
| ) | ||
| )) | ||
| .toList(); |
There was a problem hiding this comment.
map 내부에서 다양한 메소드 가 분리되는거 같아요 차라리 블록 처리를 해서 변수에 담아서 넣어주는게 가독성 측면에서 나을거 같아요 :)
| answer, | ||
| answerAttachmentService.toAttachmentResponses(answer) | ||
| )) | ||
| .orElse(null); // 답변이 없으면 null 반환 |
| public void updateStatusToComplete() { | ||
| this.status = InquiryStatus.COMPLETED; | ||
| } | ||
|
|
||
| public void updateStatusToPending() { | ||
| this.status = InquiryStatus.PENDING; | ||
| } |
There was a problem hiding this comment.
| public void updateStatusToComplete() { | |
| this.status = InquiryStatus.COMPLETED; | |
| } | |
| public void updateStatusToPending() { | |
| this.status = InquiryStatus.PENDING; | |
| } | |
| public void updateStatusBy(InquiryStatus status){ | |
| this.status = status | |
| } |
로 하고, Domain Service 에서 이를 구체화해도 좋을거 같아요!
There was a problem hiding this comment.
음 저 부분이 살짝 고민되었는데 메서드 행동을 명시적으로 보여주는 게 좋을지 아니면 상태 값을 받아서 변경할 지
태현님은 혹시 이 부분에 대해서 어떻게 생각하시나요?
| @PostMapping("/{postId}/answer") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> inquiryAnswer( | ||
| @PathVariable Long postId, | ||
| @RequestBody InquiryAnswerRequest request) { | ||
| inquiryAnswerService.createInquiryAnswer(postId, request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "답변 등록 성공")); | ||
| } | ||
|
|
||
| @DeleteMapping("/{postId}/answer") |
There was a problem hiding this comment.
answer 보다 answers 가 RESTful 한 URL 설계에 맞아보이는데, 보형님은 어떠세요?
There was a problem hiding this comment.
음 지금은 일단 답변 하나만을 생각했어서 저렇게 달았긴합니다..!
| public static InquiryAnswerDetailResponse empty() { | ||
| return new InquiryAnswerDetailResponse(null, null, null, null, List.of()); | ||
| } |
There was a problem hiding this comment.
해당 선언의 의도가 어떤지 설명 가능할까요?
There was a problem hiding this comment.
아 처음에 답변이 없을 시 빈 객체로 반환하려고 저렇게 했었다가, 그냥 빈 배열 주면 되는 걸 알아서
안쓰는데 삭제하겠습니당
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an inquiry feature for administrators. The review identified a critical bug in the inquiry deletion logic, naming inconsistencies, and non-standard use of HTTP status codes. Suggestions are provided to address these points.
| InquiryAnswerJpaEntity answerEntity = inquiryAnswerRepository.findByInquiryId(postId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.INQURIY_ANSWER_NOT_FOUND)); | ||
|
|
||
| inquiryAnswerRepository.delete(answerEntity); | ||
| answerAttachmentService.deleteAttachment(answerEntity); | ||
| inquiryEntity.updateStatusToPending(); |
There was a problem hiding this comment.
The current implementation throws an exception if an answer is not found, causing a critical issue when InquiryService.deleteInquiry calls this method for inquiries without answers. Consider modifying this method to use ifPresent to avoid the exception and ensure the inquiry deletion flow works correctly.
| InquiryAnswerJpaEntity answerEntity = inquiryAnswerRepository.findByInquiryId(postId) | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.INQURIY_ANSWER_NOT_FOUND)); | |
| inquiryAnswerRepository.delete(answerEntity); | |
| answerAttachmentService.deleteAttachment(answerEntity); | |
| inquiryEntity.updateStatusToPending(); | |
| inquiryAnswerRepository.findByInquiryId(postId) | |
| .ifPresent(answerEntity -> { | |
| inquiryAnswerRepository.delete(answerEntity); | |
| answerAttachmentService.deleteAttachment(answerEntity); | |
| inquiryEntity.updateStatusToPending(); | |
| }); |
| public interface InquiryAnswerAttachmentReposiory extends | ||
| JpaRepository<InquiryAnswerAttachmentEntity, Long> { |
There was a problem hiding this comment.
The interface name InquiryAnswerAttachmentReposiory contains a typo. Correct it to InquiryAnswerAttachmentRepository for consistency.
| public interface InquiryAnswerAttachmentReposiory extends | |
| JpaRepository<InquiryAnswerAttachmentEntity, Long> { | |
| public interface InquiryAnswerAttachmentRepository extends | |
| JpaRepository<InquiryAnswerAttachmentEntity, Long> { |
There was a problem hiding this comment.
안녕하세요! Reposiory는 Repository의 오타입니다. Repository가 올바른 영어 철자이며, 일반적으로 데이터 저장소(Repository)를 나타낼 때 사용되는 표준 명칭입니다. 오타를 수정하면 코드의 가독성과 일관성이 향상됩니다. 제안드린 코드는 이 오타를 수정한 것입니다.
| public String getCreatedAt() { | ||
| return createdAt != null ? createdAt.format( | ||
| DateTimeFormatter.ofPattern("yyyy-MM-dd")) : null; | ||
| } |
There was a problem hiding this comment.
Date formatting logic within the BaseTimeEntity mixes data representation with presentation. Move this formatting logic to the DTO layer to maintain a clear separation of concerns.
|
|
||
| // 문의 관련 에러 | ||
| INQUIRY_NOT_FOUND(HttpStatus.NOT_FOUND, "문의 내역을 찾을 수 없습니다."), | ||
| INQURIY_ANSWER_NOT_FOUND(HttpStatus.NOT_FOUND, "문의 답변을 찾을 수 없습니다."); |
There was a problem hiding this comment.
The enum constant INQURIY_ANSWER_NOT_FOUND contains a typo. Correct INQURIY to INQUIRY.
| INQURIY_ANSWER_NOT_FOUND(HttpStatus.NOT_FOUND, "문의 답변을 찾을 수 없습니다."); | |
| INQUIRY_ANSWER_NOT_FOUND(HttpStatus.NOT_FOUND, "문의 답변을 찾을 수 없습니다."); |
| public ResponseEntity<ApiResponseWrapper<Void>> create( | ||
| @RequestBody InquiryCreateRequest request) { | ||
| inquiryService.createInquiry(request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공")); |
There was a problem hiding this comment.
The create method should return 201 Created upon successful resource creation. Use ResponseEntity.status(HttpStatus.CREATED) to set the correct status code.
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공")); | |
| return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공")); |
| @PathVariable Long postId, | ||
| @RequestBody InquiryAnswerRequest request) { | ||
| inquiryAnswerService.createInquiryAnswer(postId, request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "답변 등록 성공")); |
There was a problem hiding this comment.
The inquiryAnswer method should return 201 Created upon successful resource creation. Use ResponseEntity.status(HttpStatus.CREATED) to set the correct status code.
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "답변 등록 성공")); | |
| return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "답변 등록 성공")); |
There was a problem hiding this comment.
Code Review
The code changes introduce a comprehensive inquiry and answer feature for administrators. The implementation is well-structured, leveraging services, repositories, and DTOs effectively. The review focuses on domain model design, correctness, consistency, naming, and API design to enhance code quality and maintainability.
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.INQUIRY_NOT_FOUND)); | ||
|
|
||
| InquiryAnswerJpaEntity answerEntity = inquiryAnswerRepository.findByInquiryId(postId) | ||
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.INQURIY_ANSWER_NOT_FOUND)); |
There was a problem hiding this comment.
The code uses ErrorCode.INQURIY_ANSWER_NOT_FOUND, which contains a typo. Please correct the enum constant in ErrorCode.java to INQUIRY_ANSWER_NOT_FOUND and update its usage here to ensure consistency and prevent confusion.
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.INQURIY_ANSWER_NOT_FOUND)); | |
| .orElseThrow(() -> new CustomRuntimeException(ErrorCode.INQUIRY_ANSWER_NOT_FOUND)); |
| inquiryRepository.delete(inquiryEntity); | ||
| inquiryAttachmentService.deleteAttachment(inquiryEntity); | ||
|
|
||
| inquiryAnswerService.deleteInquiryAnswer(postId); |
There was a problem hiding this comment.
The current implementation of deleteInquiry deletes the InquiryJpaEntity before its associated InquiryAnswerJpaEntity. If there are foreign key constraints, this will fail. The dependent entity (answer) should be deleted first. Also, inquiryAnswerService.deleteInquiryAnswer(postId) is called unconditionally, but this method throws an exception if no answer is found, which will cause the entire deletion to fail for inquiries that haven't been answered yet.
// The associated answer, if it exists, must be deleted first.
inquiryAnswerService.deleteInquiryAnswer(postId);
inquiryAttachmentService.deleteAttachment(inquiryEntity);
inquiryRepository.delete(inquiryEntity);| public String getCreatedAt() { | ||
| return createdAt != null ? createdAt.format( | ||
| DateTimeFormatter.ofPattern("yyyy-MM-dd")) : null; | ||
| } |
There was a problem hiding this comment.
Overriding getCreatedAt() to return a formatted String directly from an entity is not recommended. It violates the separation of concerns principle by mixing presentation logic into the domain layer. It's better to handle date formatting in the presentation layer (e.g., when mapping to DTOs).
| public interface InquiryAnswerAttachmentReposiory extends | ||
| JpaRepository<InquiryAnswerAttachmentEntity, Long> { |
There was a problem hiding this comment.
There's a typo in the interface name: InquiryAnswerAttachmentReposiory. It should be InquiryAnswerAttachmentRepository. Renaming both the file and the interface to use the correct spelling will improve code consistency and maintainability.
| public interface InquiryAnswerAttachmentReposiory extends | |
| JpaRepository<InquiryAnswerAttachmentEntity, Long> { | |
| public interface InquiryAnswerAttachmentRepository extends | |
| JpaRepository<InquiryAnswerAttachmentEntity, Long> { |
|
|
||
| // 문의 관련 에러 | ||
| INQUIRY_NOT_FOUND(HttpStatus.NOT_FOUND, "문의 내역을 찾을 수 없습니다."), | ||
| INQURIY_ANSWER_NOT_FOUND(HttpStatus.NOT_FOUND, "문의 답변을 찾을 수 없습니다."); |
There was a problem hiding this comment.
There's a typo in the enum constant INQURIY_ANSWER_NOT_FOUND. It should be INQUIRY_ANSWER_NOT_FOUND. Correcting this will improve code clarity.
| INQURIY_ANSWER_NOT_FOUND(HttpStatus.NOT_FOUND, "문의 답변을 찾을 수 없습니다."); | |
| INQUIRY_ANSWER_NOT_FOUND(HttpStatus.NOT_FOUND, "문의 답변을 찾을 수 없습니다."); |
| private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd"); | ||
|
|
||
| private final JPAQueryFactory queryFactory; | ||
| QInquiryJpaEntity inquiry = QInquiryJpaEntity.inquiryJpaEntity; |
There was a problem hiding this comment.
It's a common best practice to declare QueryDSL Q-class instances as private static final because they are stateless and thread-safe. This avoids creating a new instance for every repository instance and is a slight performance improvement.
| QInquiryJpaEntity inquiry = QInquiryJpaEntity.inquiryJpaEntity; | |
| private static final QInquiryJpaEntity inquiry = QInquiryJpaEntity.inquiryJpaEntity; |
| public FaqAttachmentJpaEntity toFaqAttachmentEntity(String fileName, String s3Key, Long faqId) { | ||
| return FaqAttachmentJpaEntity.builder() | ||
| .fileName(fileName) | ||
| .s3Key(s3Key) | ||
| .visibility(Visibility.PUBLIC) | ||
| .faqId(faqId) | ||
| .build(); | ||
| .fileName(fileName) | ||
| .s3Key(s3Key) | ||
| .visibility(Visibility.PUBLIC) | ||
| .faqId(faqId) | ||
| .build(); | ||
| } | ||
|
|
||
| public InquiryAttachmentJpaEntity toInquiryAttachmentEntity(String fileName, String s3Key, | ||
| Long inquiryId) { | ||
| return InquiryAttachmentJpaEntity.builder() | ||
| .fileName(fileName) | ||
| .s3Key(s3Key) | ||
| .visibility(Visibility.PRIVATE) | ||
| .inquiryId(inquiryId) | ||
| .build(); | ||
| } | ||
|
|
||
| public AdmissionTicketImageJpaEntity toAdmissionTicketImageEntity(String fileName, String s3Key, Long applicationId) { | ||
|
|
||
| public AdmissionTicketImageJpaEntity toAdmissionTicketImageEntity(String fileName, String s3Key, | ||
| Long applicationId) { | ||
| return AdmissionTicketImageJpaEntity.builder() | ||
| .fileName(fileName) | ||
| .s3Key(s3Key) | ||
| .visibility(Visibility.PRIVATE) | ||
| .applicationId(applicationId) | ||
| .build(); | ||
| .fileName(fileName) | ||
| .s3Key(s3Key) | ||
| .visibility(Visibility.PRIVATE) | ||
| .applicationId(applicationId) | ||
| .build(); | ||
| } | ||
|
|
||
| public InquiryAnswerAttachmentEntity toInquiryAnswerAttachmentEntity(String fileName, | ||
| String s3Key, | ||
| Long inquiryAnswerId) { | ||
| return InquiryAnswerAttachmentEntity.builder() | ||
| .fileName(fileName) | ||
| .s3Key(s3Key) | ||
| .visibility(Visibility.PRIVATE) | ||
| .inquiryAnswerId(inquiryAnswerId) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
The FileRequest record has taken on multiple responsibilities by containing factory methods (to...Entity) for several different types of attachment entities. This violates the Single Responsibility Principle (SRP) and can make the class harder to maintain. A better design would be to move the entity creation logic into the respective services or the parent request DTOs that use FileRequest.
| public ResponseEntity<ApiResponseWrapper<Void>> create( | ||
| @RequestBody InquiryCreateRequest request) { | ||
| inquiryService.createInquiry(request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공")); |
There was a problem hiding this comment.
For creating a resource, it's a REST convention to return an HTTP 201 Created status code instead of 200 OK. Adhering to this convention improves the semantic clarity of your API.
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공")); | |
| return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공")); |
WalkthroughThis update introduces a comprehensive inquiry management feature, including domain models, repositories, services, and REST controllers for handling inquiries and their answers, with support for attachments and status tracking. The changes also refactor file attachment handling to use centralized configuration properties and utility classes, and add new DTOs and error codes to support the new features. Minor refactoring and formatting improvements are made across related files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InquiryController
participant InquiryService
participant InquiryRepository
participant InquiryAttachmentService
participant InquiryAnswerService
participant InquiryAnswerRepository
participant InquiryAnswerAttachmentService
Client->>InquiryController: POST /inquiry (create inquiry)
InquiryController->>InquiryService: createInquiry(request)
InquiryService->>InquiryRepository: save(inquiry)
InquiryService->>InquiryAttachmentService: createAttachment(attachments, inquiry)
InquiryController-->>Client: 201 Created
Client->>InquiryController: GET /inquiry/list
InquiryController->>InquiryService: getInquiries(status, sort, asc, pageable)
InquiryService->>InquiryRepository: searchInquiries(...)
InquiryController-->>Client: 200 OK (paged inquiry list)
Client->>InquiryController: POST /inquiry/{postId}/answer
InquiryController->>InquiryAnswerService: createInquiryAnswer(postId, request)
InquiryAnswerService->>InquiryRepository: findById(postId)
InquiryAnswerService->>InquiryAnswerRepository: save(answer)
InquiryAnswerService->>InquiryAnswerAttachmentService: createAttachment(attachments, answer)
InquiryController-->>Client: 200 OK
Client->>InquiryController: GET /inquiry/{postId}
InquiryController->>InquiryService: getInquiryDetail(postId)
InquiryService->>InquiryRepository: findById(postId)
InquiryService->>InquiryAttachmentService: toAttachmentResponses(inquiry)
InquiryService->>InquiryAnswerService: getInquiryAnswerDetail(postId)
InquiryController-->>Client: 200 OK (inquiry detail)
Poem
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (9)
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerAttachmentReposiory.java (1)
6-7: Fix the typo in the interface name.The interface name contains a typo:
InquiryAnswerAttachmentReposioryshould beInquiryAnswerAttachmentRepository. This issue has been flagged multiple times in previous reviews.src/main/java/life/mosu/mosuserver/domain/base/BaseTimeEntity.java (1)
26-29: This change violates separation of concerns as previously noted.The same issue identified in past reviews remains: overriding
getCreatedAt()to return formatted string mixes presentation logic into the domain layer.src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
62-65: LGTM! Well-structured error codes for inquiry functionality.The new inquiry-related error codes are well-structured and follow the existing pattern. The typo issue mentioned in previous reviews has been properly addressed.
src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryJpaEntity.java (1)
51-57: Consider using a generic status update method.The specific status update methods work correctly, but a more flexible approach would be to use a generic method as suggested in the past review comment.
Consider implementing:
- public void updateStatusToComplete() { - this.status = InquiryStatus.COMPLETED; - } - - public void updateStatusToPending() { - this.status = InquiryStatus.PENDING; - } + public void updateStatusBy(InquiryStatus status) { + this.status = status; + }This provides more flexibility and follows the DRY principle.
src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryQueryRepositoryImpl.java (1)
26-26: Make Q-class instance static final for better performance.As mentioned in the previous review, Q-class instances should be declared as
private static finalsince they are stateless and thread-safe.Apply this change:
- private final QInquiryJpaEntity inquiry = QInquiryJpaEntity.inquiryJpaEntity; + private static final QInquiryJpaEntity inquiry = QInquiryJpaEntity.inquiryJpaEntity;src/main/java/life/mosu/mosuserver/presentation/faq/dto/FileRequest.java (1)
14-53: Address Single Responsibility Principle violation.The
FileRequestrecord has accumulated too many entity creation responsibilities, making it harder to maintain and violating SRP as noted in the previous review.Consider moving the entity creation logic to the respective services or parent request DTOs:
InquiryServicecould handletoInquiryAttachmentEntityInquiryAnswerServicecould handletoInquiryAnswerAttachmentEntityFaqServicecould handletoFaqAttachmentEntityThis would make
FileRequesta simple data container and improve maintainability.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java (1)
57-59: Good fix for the deletion order issue!The conditional check before calling
deleteInquiryAnswerproperly addresses the past review comment about avoiding exceptions for inquiries without answers.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java (1)
42-51: Handle missing answers gracefully to avoid breaking inquiry deletion.The current implementation throws an exception if an answer is not found, which will cause issues when
InquiryService.deleteInquirycalls this method for inquiries without answers.@Transactional public void deleteInquiryAnswer(Long postId) { InquiryJpaEntity inquiryEntity = getInquiryOrThrow(postId); - InquiryAnswerJpaEntity answerEntity = inquiryAnswerRepository.findByInquiryId(postId) - .orElseThrow(() -> new CustomRuntimeException(ErrorCode.INQUIRY_ANSWER_NOT_FOUND)); - - inquiryAnswerRepository.delete(answerEntity); - answerAttachmentService.deleteAttachment(answerEntity); - inquiryEntity.updateStatusToPending(); + inquiryAnswerRepository.findByInquiryId(postId) + .ifPresent(answerEntity -> { + inquiryAnswerRepository.delete(answerEntity); + answerAttachmentService.deleteAttachment(answerEntity); + inquiryEntity.updateStatusToPending(); + }); }src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java (1)
40-40: Fix HTTP status codes for resource creation.The creation endpoints should return
201 Createdinstead of200 OKfor proper REST semantics.Apply this fix to both methods:
- return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공")); + return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공"));- return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "답변 등록 성공")); + return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "답변 등록 성공"));Also applies to: 76-76
🧹 Nitpick comments (8)
src/main/resources/application.yml (1)
69-69: Add missing newline at end of file.YAML files should end with a newline character for proper formatting.
Add a newline at the end of the file:
api: base-url: https://api.tosspayments.com/v1/payments +src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java (1)
16-23: Consider moving entity conversion to application layer.While the current implementation works, having the presentation layer directly convert to domain entities via
toEntity()might violate separation of concerns. Consider moving this conversion logic to the application service layer.The fact that
attachmentsare not included in the entity conversion suggests they're handled separately, which is a reasonable approach for complex object creation.Example refactor:
- public InquiryJpaEntity toEntity() { - return InquiryJpaEntity.builder() - .title(title) - .content(content) - .userId(userId) - .author(author) - .build(); - }Move this logic to
InquiryServicewhere it belongs conceptually.src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java (1)
66-66: Remove unnecessary empty line.- - private InquiryResponse toInquiryResponse(InquiryJpaEntity inquiry) {src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java (1)
84-91: Update parameter names for consistency.The parameter names should be updated to reflect that this is for answer attachments, not inquiry attachments.
-private void saveAttachments(List<FileRequest> requests, Long inquiryId) { +private void saveAttachments(List<FileRequest> requests, Long answerId) { requests.forEach(req -> { fileUploadHelper.updateTag(req.s3Key()); attachmentReposiory.save(req.toInquiryAnswerAttachmentEntity( - req.fileName(), req.s3Key(), inquiryId + req.fileName(), req.s3Key(), answerId )); }); }src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryDetailResponse.java (3)
36-38: Remove unused AttachmentResponse record.The
AttachmentResponserecord appears to be unused throughout the codebase. Based on the past review comments, this was likely an artifact from earlier implementation decisions.- public record AttachmentResponse(String fileName, String url) { - - }
14-14: Consider using proper date/time types instead of String.The
createdAtfields are declared asStringwhich can lead to inconsistent date formatting and makes date operations more difficult.Consider using
LocalDateTimeorInstantfor better type safety:- String createdAt, + LocalDateTime createdAt,You'll need to add the appropriate import and handle the conversion in the factory methods.
Also applies to: 48-48
65-66: Remove unnecessary blank lines.Extra blank lines at the end of the file should be cleaned up for better code consistency.
- -src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java (1)
93-94: Remove unnecessary blank lines.Extra blank lines at the end of the file should be cleaned up for consistency.
- -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/main/java/life/mosu/mosuserver/MosuServerApplication.java(1 hunks)src/main/java/life/mosu/mosuserver/application/applicationschool/ApplicationSchoolService.java(3 hunks)src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java(2 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java(1 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/domain/base/BaseTimeEntity.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/faq/FaqJpaEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryAttachmentJpaEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryAttachmentRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryJpaEntity.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryQueryRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryQueryRepositoryImpl.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryStatus.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerAttachmentEntity.java(2 hunks)src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerAttachmentReposiory.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerJpaEntity.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/property/S3Properties.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/storage/FileUploadHelper.java(1 hunks)src/main/java/life/mosu/mosuserver/infra/storage/application/AttachmentService.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/faq/dto/FileRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerUpdateRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryDetailResponse.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryResponse.java(1 hunks)src/main/resources/application.yml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerAttachmentEntity.java (3)
src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryAttachmentJpaEntity.java (1)
Getter(16-37)src/main/java/life/mosu/mosuserver/domain/faq/FaqAttachmentJpaEntity.java (1)
Getter(11-30)src/main/java/life/mosu/mosuserver/domain/notice/NoticeAttachmentJpaEntity.java (1)
Getter(11-29)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java (2)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java (1)
Service(17-74)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java (1)
Service(18-84)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerService.java (1)
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java (1)
Service(17-93)
🪛 Gitleaks (8.26.0)
src/main/resources/application.yml
67-67: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 YAMLlint (1.37.1)
src/main/resources/application.yml
[error] 69-69: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (32)
src/main/java/life/mosu/mosuserver/infra/storage/FileUploadHelper.java (2)
1-1: LGTM: Package refactoring improves code organization.The package move from
life.mosu.mosuserver.infra.storage.applicationtolife.mosu.mosuserver.infra.storageis a good refactoring that aligns the helper class with the broader storage infrastructure organization.
5-5: LGTM: S3Service import addition is necessary.The import addition properly supports the S3Service dependency injection and aligns with the centralized S3 configuration mentioned in the AI summary.
src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryAttachmentJpaEntity.java (3)
3-8: LGTM: Explicit imports improve code clarity.Switching from wildcard imports to explicit imports for Jakarta Persistence annotations is a best practice that improves code readability and helps avoid potential naming conflicts.
14-14: LGTM: Soft delete annotation is appropriate for attachments.The
@SoftDeleteannotation is a good choice for attachment entities as it enables logical deletion, which is typically preferred for file attachments to maintain data integrity and audit trails.Also applies to: 20-20
32-36: LGTM: Constructor formatting improved.The constructor parameter formatting enhances readability without changing the logic or functionality.
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerAttachmentEntity.java (3)
3-8: LGTM: Consistent explicit imports across attachment entities.The change from wildcard to explicit imports maintains consistency with
InquiryAttachmentJpaEntityand follows best practices for import management.
14-14: LGTM: Soft delete annotation maintains consistency.The
@SoftDeleteannotation is consistently applied across attachment entities, which is appropriate for logical deletion of file attachments.Also applies to: 21-21
33-37: LGTM: Constructor formatting aligns with other entities.The constructor parameter formatting is consistent with the similar changes made to
InquiryAttachmentJpaEntityand improves readability.src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerAttachmentReposiory.java (1)
9-9: LGTM: Repository method follows JPA naming conventions.The
findAllByInquiryAnswerIdmethod follows Spring Data JPA naming conventions and will correctly generate the query to find attachments by inquiry answer ID.src/main/java/life/mosu/mosuserver/domain/faq/FaqJpaEntity.java (1)
3-8: LGTM: Import style consistency across entities.The change from wildcard to explicit imports for Jakarta Persistence annotations maintains consistency with the import style changes applied to other entity classes in this PR and follows Java best practices.
src/main/resources/application.yml (1)
67-69: LGTM on indentation consistency.The indentation adjustment from 4 spaces to 2 spaces aligns with the overall YAML formatting style in the file.
src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryStatus.java (1)
1-6: LGTM! Clean enum implementation.The enum is well-designed with clear, meaningful status values that effectively represent the inquiry lifecycle states. The implementation follows Java naming conventions and integrates well with the domain model.
src/main/java/life/mosu/mosuserver/MosuServerApplication.java (1)
5-7: LGTM! Proper configuration properties scanning setup.The addition of
@ConfigurationPropertiesScanenables automatic discovery of@ConfigurationPropertiesclasses, which is a clean approach for managing configuration properties like the new S3Properties class.src/main/java/life/mosu/mosuserver/infra/storage/application/AttachmentService.java (1)
5-10: LGTM! Improved generic type naming and formatting.The rename from
<T, R>to<E, R>makes the generic parameters more descriptive (E likely representing Entity), and the added spacing between method declarations improves readability.src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerRepository.java (1)
1-9: LGTM! Well-designed repository interface.The repository follows Spring Data JPA conventions with proper entity typing and a logical query method. The use of
Optional<InquiryAnswerJpaEntity>for the return type provides good null safety.src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryRepository.java (1)
5-6: LGTM! Standard repository interface implementation.The interface correctly extends both
JpaRepositoryfor standard CRUD operations andInquiryQueryRepositoryfor custom queries, following Spring Data JPA best practices.src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryAttachmentRepository.java (1)
6-10: LGTM! Proper repository implementation with custom query method.The repository correctly extends
JpaRepositoryand uses Spring Data JPA naming conventions for the custom query methodfindAllByInquiryId(Long id), which will be automatically implemented.src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerUpdateRequest.java (1)
6-10: LGTM! Clean and concise DTO record implementation.The record properly encapsulates the update request fields with appropriate types, following modern Java record best practices.
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerJpaEntity.java (3)
3-8: Good improvement: Using explicit imports over wildcards.Switching from wildcard imports to explicit imports improves code clarity and reduces potential naming conflicts.
26-27: LGTM! Proper JPA column definition.The title field is correctly annotated with appropriate constraints (non-nullable, length limit of 3000 characters).
51-54: LGTM! Clean update method implementation.The update method properly encapsulates the mutation logic for title and content fields, following domain entity best practices.
src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryResponse.java (1)
16-25: Approve: DTO mapping and date formatting verified
getCreatedAt()inBaseTimeEntityformats the timestamp as"yyyy-MM-dd", ensuring consistent API responses.src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java (1)
9-10: LGTM! Proper validation on required fields.The validation constraints are correctly applied to the required fields
titleandcontent.src/main/java/life/mosu/mosuserver/infra/property/S3Properties.java (1)
1-21: LGTM! Excellent configuration properties implementation.This implementation follows Spring Boot best practices:
- Uses
@ConfigurationPropertiesfor type-safe configuration- Constructor injection ensures immutability
@PostConstructlogging helps with debugging configuration loading- Clean separation of infrastructure concerns
This centralized approach for S3 configuration is much better than scattered
@Valueannotations across multiple service classes.src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerRequest.java (2)
8-13: LGTM! Well-structured DTO with appropriate validation.The record definition is clean and follows good practices with
@NotNullvalidation on required fields. The nullableuserIdfield is appropriate for cases where the answer might be provided by an admin without specific user context.
15-22: Conversion method correctly maps to entity.The
toEntitymethod properly converts the DTO toInquiryAnswerJpaEntity, correctly mappingpostIdtoinquiryId. The attachments are intentionally not handled here, which is appropriate as they're likely processed separately by the service layer.src/main/java/life/mosu/mosuserver/application/applicationschool/ApplicationSchoolService.java (1)
15-15: Excellent refactoring to centralize S3 configuration.The transition from
@Valueinjection toS3Propertiesdependency is a good architectural improvement that centralizes configuration management. This makes the code more maintainable and consistent across services.Also applies to: 39-39, 121-121
src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryJpaEntity.java (2)
3-10: Good practice using explicit imports.Replacing wildcard imports with explicit imports improves code readability and reduces the risk of import conflicts.
36-39: Well-designed entity enhancements.The addition of
authorandstatusfields with appropriate default initialization enhances the inquiry tracking capabilities. The constructor properly initializes the status toPENDINGby default.Also applies to: 42-49
src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryQueryRepositoryImpl.java (2)
29-50: Well-structured query implementation with good separation of concerns.The
searchInquiriesmethod properly handles pagination, filtering, and sorting. The use of helper methods makes the code readable and maintainable.
72-80: Robust sorting implementation with fallback.The switch expression for handling different sort fields is clean and includes a sensible default fallback to
idsorting.src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java (1)
8-9: Good refactoring to centralize S3 configuration!The migration from
@Valueannotations to the centralizedS3Propertiesclass improves maintainability and consistency across the codebase.Also applies to: 18-19, 26-26, 62-62
| secret-key: test_sk_kYG57Eba3GYBMGeobgbLrpWDOxmA | ||
| api: | ||
| base-url: https://api.tosspayments.com/v1/payments | ||
| secret-key: test_sk_kYG57Eba3GYBMGeobgbLrpWDOxmA |
There was a problem hiding this comment.
Remove hardcoded API key from configuration file.
Even though this appears to be a test key (based on the "test_sk_" prefix), hardcoded API keys should not be committed to version control. Use environment variables instead.
Apply this diff to use environment variables:
- secret-key: test_sk_kYG57Eba3GYBMGeobgbLrpWDOxmA
+ secret-key: ${TOSS_SECRET_KEY}📝 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.
| secret-key: test_sk_kYG57Eba3GYBMGeobgbLrpWDOxmA | |
| secret-key: ${TOSS_SECRET_KEY} |
🧰 Tools
🪛 Gitleaks (8.26.0)
67-67: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In src/main/resources/application.yml at line 67, the secret-key is hardcoded,
which is insecure. Replace the hardcoded API key with a reference to an
environment variable by using the appropriate syntax for environment variable
substitution in YAML, such as ${ENV_VARIABLE_NAME}, and ensure the actual key is
set in the environment where the application runs instead of in the file.
| @@ -0,0 +1,11 @@ | |||
| package life.mosu.mosuserver.domain.inquiry; | |||
|
|
|||
| import life.mosu.mosuserver.presentation.inquiry.dto.InquiryResponse; | |||
There was a problem hiding this comment.
Fix architectural layer violation: Domain depends on Presentation layer.
The domain layer interface InquiryQueryRepository depends on InquiryResponse from the presentation layer, which violates clean architecture principles. Dependencies should flow inward (presentation → application → domain → infrastructure).
Consider creating a domain-specific response object in the domain layer:
-import life.mosu.mosuserver.presentation.inquiry.dto.InquiryResponse;
+import life.mosu.mosuserver.domain.inquiry.InquirySearchResult;- Page<InquiryResponse> searchInquiries(InquiryStatus status, String sortField, boolean asc,
- Pageable pageable);
+ Page<InquirySearchResult> searchInquiries(InquiryStatus status, String sortField, boolean asc,
+ Pageable pageable);Then convert the domain objects to presentation DTOs in the application or presentation layer.
Also applies to: 9-10
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryQueryRepository.java
around lines 3 and 9-10, the domain layer interface improperly depends on the
presentation layer's InquiryResponse DTO, violating clean architecture
principles. To fix this, create a domain-specific response object within the
domain layer to represent inquiry data, and remove the import of InquiryResponse
from the presentation layer. Then, convert this domain response object to the
presentation DTO in the application or presentation layer, ensuring dependencies
flow inward and the domain layer remains independent of presentation concerns.
| private InquiryResponse toInquiryResponse(InquiryJpaEntity inquiry) { | ||
| return InquiryResponse.of(inquiry); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused method.
The toInquiryResponse method is defined but never used in this class.
-private InquiryResponse toInquiryResponse(InquiryJpaEntity inquiry) {
- return InquiryResponse.of(inquiry);
-}📝 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.
| private InquiryResponse toInquiryResponse(InquiryJpaEntity inquiry) { | |
| return InquiryResponse.of(inquiry); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java
around lines 67 to 69, the method toInquiryResponse is defined but never called
or used anywhere in the class. Remove this unused method entirely to clean up
the code and avoid unnecessary clutter.
| import life.mosu.mosuserver.infra.storage.FileUploadHelper; | ||
| import life.mosu.mosuserver.infra.storage.application.AttachmentService; | ||
| import life.mosu.mosuserver.infra.storage.application.S3Service; | ||
| import life.mosu.mosuserver.presentation.faq.dto.FileRequest; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix incorrect package import.
The FileRequest is imported from the faq package, which seems inconsistent for an inquiry service. Consider moving FileRequest to a shared package or creating an inquiry-specific version.
🏁 Script executed:
#!/bin/bash
# Description: Check if FileRequest is used consistently across different modules
# Search for FileRequest usage patterns
echo "=== FileRequest imports across the codebase ==="
rg "import.*FileRequest" -A 2
echo -e "\n=== FileRequest class definition ==="
fd -e java -x grep -l "class FileRequest\|record FileRequest" {} \; | head -5Length of output: 3480
Extract FileRequest into a Shared DTO Package
The FileRequest class is currently defined under presentation.faq.dto but is being reused in multiple feature modules (inquiry, application, FAQ). This creates an unintended coupling to the FAQ module for attachment handling across the application.
• Affected files:
- src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java
- src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java
- src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java
- src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java
- src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerRequest.java
- src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerUpdateRequest.java
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationRequest.java
- src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java
Suggested changes:
-
Move
FileRequest.javafrom
src/main/java/life/mosu/mosuserver/presentation/faq/dto/
to a new shared package, e.g.,
src/main/java/life/mosu/mosuserver/presentation/common/dto/. -
Update all imports accordingly:
- import life.mosu.mosuserver.presentation.faq.dto.FileRequest; + import life.mosu.mosuserver.presentation.common.dto.FileRequest;
-
If the inquiry attachment workflow evolves differently from the FAQ flow, consider defining an
InquiryFileRequestinpresentation.inquiry.dtoinstead of reusing the generic DTO.
This refactoring will decouple the generic file‐upload API from the FAQ feature and improve module cohesion.
📝 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.
| import life.mosu.mosuserver.presentation.faq.dto.FileRequest; | |
| // in src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java | |
| - import life.mosu.mosuserver.presentation.faq.dto.FileRequest; | |
| + import life.mosu.mosuserver.presentation.common.dto.FileRequest; |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java
at line 12 and related files, the FileRequest DTO is imported from the FAQ
module causing unwanted coupling. To fix this, move the FileRequest.java file
from presentation.faq.dto to a new shared package like presentation.common.dto,
then update all imports in the affected files to reference this new location. If
the inquiry attachment needs differ, create a separate InquiryFileRequest DTO in
presentation.inquiry.dto instead of reusing the generic one.
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java
Outdated
Show resolved
Hide resolved
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java
Outdated
Show resolved
Hide resolved
| private InquiryDetailResponse.AttachmentResponse createAttachResponse( | ||
| InquiryAnswerAttachmentEntity attachment) { | ||
| String presignedUrl = s3Service.getPreSignedUrl( | ||
| attachment.getS3Key(), | ||
| Duration.ofMinutes(s3Properties.getPresignedUrlExpirationMinutes()) | ||
| ); | ||
|
|
||
| return new InquiryDetailResponse.AttachmentResponse( | ||
| attachment.getFileName(), | ||
| presignedUrl | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused method.
The createAttachResponse method is defined but never used in this class.
-private InquiryDetailResponse.AttachmentResponse createAttachResponse(
- InquiryAnswerAttachmentEntity attachment) {
- String presignedUrl = s3Service.getPreSignedUrl(
- attachment.getS3Key(),
- Duration.ofMinutes(s3Properties.getPresignedUrlExpirationMinutes())
- );
-
- return new InquiryDetailResponse.AttachmentResponse(
- attachment.getFileName(),
- presignedUrl
- );
-}📝 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.
| private InquiryDetailResponse.AttachmentResponse createAttachResponse( | |
| InquiryAnswerAttachmentEntity attachment) { | |
| String presignedUrl = s3Service.getPreSignedUrl( | |
| attachment.getS3Key(), | |
| Duration.ofMinutes(s3Properties.getPresignedUrlExpirationMinutes()) | |
| ); | |
| return new InquiryDetailResponse.AttachmentResponse( | |
| attachment.getFileName(), | |
| presignedUrl | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java
around lines 57 to 68, the method createAttachResponse is defined but never
called or used anywhere in the class. Remove this unused method entirely to
clean up the code and avoid dead code.
|
|
||
| @GetMapping("/{postId}") | ||
| public ResponseEntity<ApiResponseWrapper<InquiryDetailResponse>> getInquiryDetail( | ||
| @PathVariable Long postId) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add path variable validation.
The postId path variables lack validation annotations, which could lead to invalid data being processed.
Add validation annotations:
- @PathVariable Long postId) {
+ @PathVariable @Positive Long postId) {Add the necessary import:
+import javax.validation.constraints.Positive;Also applies to: 66-66, 73-73, 81-81, 88-88
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java
at lines 58, 66, 73, 81, and 88, the @PathVariable parameters named postId lack
validation annotations. To fix this, add appropriate validation annotations such
as @Positive or @Min(1) to ensure postId is a valid positive number. Also,
import the necessary validation annotation classes from
javax.validation.constraints or jakarta.validation.constraints. This will
enforce validation on the path variables and prevent invalid data from being
processed.
| public ResponseEntity<ApiResponseWrapper<Void>> create( | ||
| @RequestBody InquiryCreateRequest request) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for request bodies.
The request body parameters lack validation annotations, which could lead to invalid data being processed.
Add validation annotations:
- public ResponseEntity<ApiResponseWrapper<Void>> create(
- @RequestBody InquiryCreateRequest request) {
+ public ResponseEntity<ApiResponseWrapper<Void>> create(
+ @RequestBody @Valid InquiryCreateRequest request) {Apply similar changes to other request body parameters and add the necessary import:
+import javax.validation.Valid;Also applies to: 73-74, 81-82
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java
at lines 37-38, 73-74, and 81-82, the request body parameters lack validation
annotations, risking invalid data processing. Add appropriate validation
annotations such as @Valid before the @RequestBody parameters to enable
validation. Also, ensure to import javax.validation.Valid or the relevant
validation package. Apply these changes consistently to all request body
parameters in the specified lines.
| @GetMapping("/list") | ||
| public ResponseEntity<ApiResponseWrapper<Page<InquiryResponse>>> getInquiryList( | ||
| @RequestParam(required = false) InquiryStatus status, | ||
| @RequestParam(required = false, defaultValue = "id") String sort, |
There was a problem hiding this comment.
Validate the sort parameter to prevent injection attacks.
The sort parameter accepts arbitrary field names without validation, which could lead to security vulnerabilities or runtime errors.
Add validation for the sort parameter:
- @RequestParam(required = false, defaultValue = "id") String sort,
+ @RequestParam(required = false, defaultValue = "id") String sort,Consider creating an enum for allowed sort fields or implement server-side validation in the service layer to ensure only valid field names are accepted.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java
at line 46, the sort parameter is accepted without validation, risking injection
attacks or runtime errors. Fix this by defining an enum of allowed sort fields
and changing the sort parameter type to this enum, or add server-side validation
in the service layer to check that the provided sort value matches one of the
allowed fields before processing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (13)
src/main/java/life/mosu/mosuserver/global/util/FileRequest.java (3)
23-31: Apply the same refactoring to this method.
34-42: Apply the same refactoring to this method.
44-53: Apply the same refactoring to this method.src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java (10)
38-42: Use HTTP 201 Created for resource creation.The endpoint creates a new inquiry resource but returns HTTP 200 OK instead of the more appropriate HTTP 201 Created status code.
Apply this fix:
- return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공")); + return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "질문 등록 성공"));
73-78: Use HTTP 201 Created for resource creation.The endpoint creates a new inquiry answer resource but returns HTTP 200 OK instead of the more appropriate HTTP 201 Created status code.
Apply this fix:
- return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "답변 등록 성공")); + return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "답변 등록 성공"));
59-59: Add path variable validation.The
postIdpath variable lacks validation annotations, which could lead to invalid data being processed.Add validation annotation:
- @PathVariable Long postId) { + @PathVariable @Positive Long postId) {Add the necessary import:
+import jakarta.validation.constraints.Positive;
67-67: Add path variable validation.
74-74: Add path variable validation.
82-82: Add path variable validation.
89-89: Add path variable validation.
47-47: Validate the sort parameter to prevent injection attacks.The sort parameter accepts arbitrary field names without validation, which could lead to security vulnerabilities or runtime errors.
Consider creating an enum for allowed sort fields or implementing server-side validation in the service layer to ensure only valid field names are accepted.
83-83: Add validation annotation for request body.The
InquiryAnswerUpdateRequestrequest body lacks validation annotation, which could lead to invalid data being processed.Add validation annotation:
- @RequestBody InquiryAnswerUpdateRequest request) { + @RequestBody @Valid InquiryAnswerUpdateRequest request) {
75-75: Add validation annotation for request body.The
InquiryAnswerRequestrequest body lacks validation annotation, which could lead to invalid data being processed.Add validation annotation:
- @RequestBody InquiryAnswerRequest request) { + @RequestBody @Valid InquiryAnswerRequest request) {
🧹 Nitpick comments (1)
src/main/java/life/mosu/mosuserver/global/util/FileRequest.java (1)
9-12: Consider adding validation for the record fields.The record fields lack validation, which could lead to invalid data being processed.
Consider adding validation annotations:
public record FileRequest( - String fileName, - String s3Key + @NotBlank String fileName, + @NotBlank String s3Key ) {Add the necessary import:
+import jakarta.validation.constraints.NotBlank;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java(2 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/domain/inquiryAnswer/InquiryAnswerAttachmentRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/FileRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqCreateRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/InquiryController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerUpdateRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/main/java/life/mosu/mosuserver/presentation/faq/dto/FaqCreateRequest.java
- src/main/java/life/mosu/mosuserver/presentation/application/dto/ApplicationRequest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerUpdateRequest.java
- src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryAnswerRequest.java
- src/main/java/life/mosu/mosuserver/presentation/inquiry/dto/InquiryCreateRequest.java
- src/main/java/life/mosu/mosuserver/application/faq/FaqAttachmentService.java
- src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAttachmentService.java
- src/main/java/life/mosu/mosuserver/application/inquiry/InquiryAnswerAttachmentService.java
🔇 Additional comments (2)
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerAttachmentRepository.java (1)
1-10: Clean and standard repository implementation.The repository interface follows Spring Data JPA conventions correctly. The custom query method
findAllByInquiryAnswerIduses proper naming conventions for automatic implementation by Spring Data JPA.src/main/java/life/mosu/mosuserver/application/application/ApplicationService.java (1)
14-14: Good refactoring to centralize file utilities.Moving
FileRequestto the global utility package improves code organization and reusability across different modules.
| public FaqAttachmentJpaEntity toFaqAttachmentEntity(String fileName, String s3Key, Long faqId) { | ||
| return FaqAttachmentJpaEntity.builder() | ||
| .fileName(fileName) | ||
| .s3Key(s3Key) | ||
| .visibility(Visibility.PUBLIC) | ||
| .faqId(faqId) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant parameters in conversion methods.
The conversion methods accept fileName and s3Key parameters that are already available as record fields. This creates redundancy and potential for inconsistency.
Consider simplifying the method signatures to use the record fields directly:
- public FaqAttachmentJpaEntity toFaqAttachmentEntity(String fileName, String s3Key, Long faqId) {
+ public FaqAttachmentJpaEntity toFaqAttachmentEntity(Long faqId) {
return FaqAttachmentJpaEntity.builder()
- .fileName(fileName)
- .s3Key(s3Key)
+ .fileName(this.fileName)
+ .s3Key(this.s3Key)
.visibility(Visibility.PUBLIC)
.faqId(faqId)
.build();
}📝 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 FaqAttachmentJpaEntity toFaqAttachmentEntity(String fileName, String s3Key, Long faqId) { | |
| return FaqAttachmentJpaEntity.builder() | |
| .fileName(fileName) | |
| .s3Key(s3Key) | |
| .visibility(Visibility.PUBLIC) | |
| .faqId(faqId) | |
| .build(); | |
| } | |
| public FaqAttachmentJpaEntity toFaqAttachmentEntity(Long faqId) { | |
| return FaqAttachmentJpaEntity.builder() | |
| .fileName(this.fileName) | |
| .s3Key(this.s3Key) | |
| .visibility(Visibility.PUBLIC) | |
| .faqId(faqId) | |
| .build(); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/global/util/FileRequest.java around lines
14 to 21, the toFaqAttachmentEntity method redundantly accepts fileName and
s3Key as parameters even though these values are already present as record
fields. Remove these parameters from the method signature and update the method
body to use the record's own fileName and s3Key fields directly to avoid
redundancy and potential inconsistency.
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation