-
Notifications
You must be signed in to change notification settings - Fork 2
MOSU-70 refactor: faq, notice response 형식 변경 및 Swagger Docs 적용 #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b92ba05
422a7d7
36242f3
b80b671
373abbe
96642f7
6e8ae18
9ce4c6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,22 +32,29 @@ public class NoticeJpaEntity extends BaseTimeEntity { | |
| @Column(name = "user_id", nullable = false) | ||
| private Long userId; | ||
|
|
||
| @Column(name = "author", nullable = false) | ||
| private String author; | ||
|
Comment on lines
+35
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify 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:
🤖 Prompt for AI Agents |
||
|
|
||
| @Builder | ||
| public NoticeJpaEntity( | ||
| final String title, | ||
| final String content, | ||
| final Long userId | ||
| final Long userId, | ||
| final String author | ||
| ) { | ||
| this.title = title; | ||
| this.content = content; | ||
| this.userId = userId; | ||
| this.author = author; | ||
| } | ||
|
|
||
| public void update( | ||
| final String title, | ||
| final String content | ||
| final String content, | ||
| final String author | ||
| ) { | ||
| this.title = title; | ||
| this.content = content; | ||
| this.author = author; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,18 +1,18 @@ | ||||||||||||||||||||||||
| package life.mosu.mosuserver.infra.property; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import jakarta.annotation.PostConstruct; | ||||||||||||||||||||||||
| import lombok.Getter; | ||||||||||||||||||||||||
| import lombok.RequiredArgsConstructor; | ||||||||||||||||||||||||
| import lombok.Data; | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainSignificant 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:
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 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 |
||||||||||||||||||||||||
| import lombok.extern.slf4j.Slf4j; | ||||||||||||||||||||||||
|
Comment on lines
+4
to
5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using
Suggested change
|
||||||||||||||||||||||||
| import org.springframework.boot.context.properties.ConfigurationProperties; | ||||||||||||||||||||||||
| import org.springframework.stereotype.Component; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Getter | ||||||||||||||||||||||||
| @RequiredArgsConstructor | ||||||||||||||||||||||||
| @Data | ||||||||||||||||||||||||
| @Component | ||||||||||||||||||||||||
| @ConfigurationProperties(prefix = "aws.s3") | ||||||||||||||||||||||||
| @Slf4j | ||||||||||||||||||||||||
| public class S3Properties { | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| private final int presignedUrlExpirationMinutes; | ||||||||||||||||||||||||
| private int presignedUrlExpirationMinutes; | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider preserving immutability for configuration properties. The change from 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
🤖 Prompt for AI Agents |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @PostConstruct | ||||||||||||||||||||||||
| public void init() { | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,20 @@ | ||
| package life.mosu.mosuserver.presentation.faq; | ||
|
|
||
| import jakarta.validation.Valid; | ||
| import java.util.List; | ||
| import life.mosu.mosuserver.application.faq.FaqService; | ||
| import life.mosu.mosuserver.global.util.ApiResponseWrapper; | ||
| import life.mosu.mosuserver.presentation.faq.dto.FaqCreateRequest; | ||
| import life.mosu.mosuserver.presentation.faq.dto.FaqResponse; | ||
| import life.mosu.mosuserver.presentation.faq.dto.FaqUpdateRequest; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.DeleteMapping; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.PostMapping; | ||
| import org.springframework.web.bind.annotation.PutMapping; | ||
| import org.springframework.web.bind.annotation.RequestBody; | ||
| import org.springframework.web.bind.annotation.RequestMapping; | ||
| import org.springframework.web.bind.annotation.RequestParam; | ||
|
|
@@ -21,13 +24,15 @@ | |
| @RestController | ||
| @RequiredArgsConstructor | ||
| @RequestMapping("/faq") | ||
| public class FaqController { | ||
| public class FaqController implements FaqControllerDocs { | ||
|
|
||
| private final FaqService faqService; | ||
|
|
||
| //TODO: 관리자 권한 체크 추가 | ||
| @PostMapping | ||
| public ResponseEntity<ApiResponseWrapper<Void>> create(@RequestBody FaqCreateRequest request) { | ||
| // @PreAuthorize("isAuthenticated() and hasRole('ADMIN')") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainAddress the commented security annotations. The 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
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 |
||
| public ResponseEntity<ApiResponseWrapper<Void>> createFaq( | ||
| @Valid @RequestBody FaqCreateRequest request) { | ||
| faqService.createFaq(request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); | ||
|
Comment on lines
+34
to
37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return ResponseEntity.status(HttpStatus.CREATED).body(ApiResponseWrapper.success(HttpStatus.CREATED, "게시글 등록 성공")); |
||
| } | ||
|
|
@@ -41,9 +46,27 @@ public ResponseEntity<ApiResponseWrapper<List<FaqResponse>>> getFaqs( | |
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 조회 성공", responses)); | ||
| } | ||
|
|
||
| @GetMapping("/{faqId}") | ||
| public ResponseEntity<ApiResponseWrapper<FaqResponse>> getFaqDetail( | ||
| @PathVariable Long faqId) { | ||
| FaqResponse faq = faqService.getFaqDetail(faqId); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 상세 조회 성공", faq)); | ||
| } | ||
|
|
||
| @PutMapping("/{faqId}") | ||
| // @PreAuthorize("isAuthenticated() and hasRole('ADMIN')") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> updateFaq( | ||
| @PathVariable Long faqId, | ||
| @Valid @RequestBody FaqUpdateRequest request | ||
| ) { | ||
| faqService.update(request, faqId); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 수정 성공")); | ||
| } | ||
|
|
||
| //TODO: 관리자 권한 체크 추가 | ||
| @DeleteMapping("/{faqId}") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> delete(@PathVariable Long faqId) { | ||
| // @PreAuthorize("isAuthenticated() and hasRole('ADMIN')") | ||
| public ResponseEntity<ApiResponseWrapper<Void>> deleteFaq(@PathVariable Long faqId) { | ||
| faqService.deleteFaq(faqId); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "게시글 삭제 성공")); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| package life.mosu.mosuserver.presentation.faq; | ||
|
|
||
| import io.swagger.v3.oas.annotations.Operation; | ||
| import io.swagger.v3.oas.annotations.Parameter; | ||
| import io.swagger.v3.oas.annotations.enums.ParameterIn; | ||
| import io.swagger.v3.oas.annotations.media.ArraySchema; | ||
| import io.swagger.v3.oas.annotations.media.Content; | ||
| import io.swagger.v3.oas.annotations.media.Schema; | ||
| import io.swagger.v3.oas.annotations.responses.ApiResponse; | ||
| import io.swagger.v3.oas.annotations.responses.ApiResponses; | ||
| import io.swagger.v3.oas.annotations.tags.Tag; | ||
| import jakarta.validation.Valid; | ||
| import java.util.List; | ||
| import life.mosu.mosuserver.global.util.ApiResponseWrapper; | ||
| import life.mosu.mosuserver.presentation.faq.dto.FaqCreateRequest; | ||
| import life.mosu.mosuserver.presentation.faq.dto.FaqResponse; | ||
| import life.mosu.mosuserver.presentation.faq.dto.FaqUpdateRequest; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.PathVariable; | ||
| import org.springframework.web.bind.annotation.RequestBody; | ||
| import org.springframework.web.bind.annotation.RequestParam; | ||
|
|
||
| @Tag(name = "Faq API", description = "FAQ 관련 API 명세") | ||
| public interface FaqControllerDocs { | ||
|
|
||
| @Operation(summary = "FAQ 등록", description = "관리자가 새로운 FAQ를 등록합니다.") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "201", description = "FAQ 등록 성공") | ||
| }) | ||
| ResponseEntity<ApiResponseWrapper<Void>> createFaq( | ||
| @Parameter(description = "FAQ 등록 요청 데이터") @RequestBody @Valid FaqCreateRequest request | ||
| ); | ||
|
|
||
| @Operation(summary = "FAQ 목록 조회", description = "전체 FAQ 목록을 페이징하여 조회합니다.") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "FAQ 목록 조회 성공", | ||
| content = @Content(mediaType = "application/json", | ||
| array = @ArraySchema(schema = @Schema(implementation = FaqResponse.class)) | ||
| ) | ||
| ) | ||
|
Comment on lines
+35
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| }) | ||
| ResponseEntity<ApiResponseWrapper<List<FaqResponse>>> getFaqs( | ||
| @Parameter(name = "page", description = "페이지 번호", in = ParameterIn.QUERY) | ||
| @RequestParam(defaultValue = "0") int page, | ||
|
|
||
| @Parameter(name = "size", description = "페이지 크기", in = ParameterIn.QUERY) | ||
| @RequestParam(defaultValue = "10") int size | ||
| ); | ||
|
|
||
| @Operation(summary = "FAQ 상세 조회", description = "FAQ ID를 기반으로 상세 내용을 조회합니다.") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "FAQ 상세 조회 성공") | ||
| }) | ||
| ResponseEntity<ApiResponseWrapper<FaqResponse>> getFaqDetail( | ||
| @Parameter(name = "faqId", description = "FAQ ID", in = ParameterIn.PATH) | ||
| @PathVariable Long faqId | ||
| ); | ||
|
|
||
| @Operation(summary = "FAQ 삭제", description = "FAQ ID를 기반으로 게시글을 삭제합니다.") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "FAQ 삭제 성공") | ||
| }) | ||
| ResponseEntity<ApiResponseWrapper<Void>> deleteFaq( | ||
| @Parameter(name = "faqId", description = "삭제할 FAQ ID", in = ParameterIn.PATH) | ||
| @PathVariable Long faqId | ||
| ); | ||
|
|
||
| @Operation(summary = "FAQ 수정", description = "기존 FAQ 내용을 수정합니다.") | ||
| @ApiResponses(value = { | ||
| @ApiResponse(responseCode = "200", description = "FAQ 수정 성공") | ||
| }) | ||
| ResponseEntity<ApiResponseWrapper<Void>> updateFaq( | ||
| @Parameter(name = "faqId", description = "수정할 FAQ ID", in = ParameterIn.PATH) | ||
| @PathVariable Long faqId, | ||
|
|
||
| @Parameter(description = "FAQ 수정 요청 데이터") | ||
| @RequestBody @Valid FaqUpdateRequest request | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,32 @@ | ||
| package life.mosu.mosuserver.presentation.faq.dto; | ||
|
|
||
| import io.swagger.v3.oas.annotations.media.Schema; | ||
| import java.util.List; | ||
| import life.mosu.mosuserver.domain.faq.FaqJpaEntity; | ||
|
|
||
| public record FaqResponse( | ||
| Long id, | ||
| String title, | ||
| String content, | ||
| List<AttachmentResponse> attachments | ||
| @Schema(description = "FAQ ID") Long id, | ||
| @Schema(description = "질문") String question, | ||
| @Schema(description = "답변") String answer, | ||
| @Schema(description = "작성 일자 (yyyy-MM-dd)") String createdAt, | ||
| @Schema(description = "첨부파일 리스트") List<AttachmentResponse> attachments | ||
| ) { | ||
|
|
||
| public static FaqResponse of(FaqJpaEntity faq, List<AttachmentResponse> attachments) { | ||
| return new FaqResponse( | ||
| faq.getId(), | ||
| faq.getQuestion(), | ||
| faq.getAnswer(), | ||
| attachments | ||
| faq.getId(), | ||
| faq.getQuestion(), | ||
| faq.getAnswer(), | ||
| faq.getCreatedAt().substring(0, 10), // 일자만 반환 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Fragile date extraction logic - consider using proper date formatting. The Recommended improvement: - faq.getCreatedAt().substring(0, 10), // 일자만 반환
+ faq.getCreatedAt().toLocalDate().toString(), // 안전한 날짜 추출Or if - faq.getCreatedAt().substring(0, 10), // 일자만 반환
+ LocalDateTime.parse(faq.getCreatedAt()).format(DateTimeFormatter.ofPattern("yyyy-MM-dd")),
🤖 Prompt for AI Agents |
||
| attachments | ||
| ); | ||
| } | ||
|
|
||
| public record AttachmentResponse(String fileName, String url) {} | ||
| public record AttachmentResponse( | ||
| @Schema(description = "파일명") String fileName, | ||
| @Schema(description = "파일 URL") String url, | ||
| @Schema(description = "S3 키") String s3Key | ||
| ) { | ||
|
|
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within a
@Transactionalmethod, an explicit call tofaqRepository.save(faqEntity)is redundant. JPA's dirty checking mechanism will automatically detect changes to thefaqEntityand persist them to the database when the transaction commits. Removing this line will make the code cleaner.