Conversation
Develop-KIM
added
the
refactoring
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
WalkthroughNotice 조회 서비스의 응답 구성 로직을 새로운 NoticeResponseAssembler 컴포넌트로 추출하고, Notice 응답 DTO의 authorId 필드를 NoticeAuthorResponse 객체로 변경하여 작성자 정보를 구조화했습니다. Admin 저장소에 배치 조회 메서드를 추가했고 SVG 파일 형식을 지원하도록 확장했습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NoticeQueryService
participant NoticeResponseAssembler
participant FileFinder
participant AdminRepository
participant Database
Client->>NoticeQueryService: findByIdWithFiles(noticeId)
NoticeQueryService->>NoticeResponseAssembler: assemble(notice)
NoticeResponseAssembler->>FileFinder: findFilesBy(NOTICE, noticeId)
FileFinder->>Database: query files
Database-->>FileFinder: List<FileResponse>
FileFinder-->>NoticeResponseAssembler: files list
NoticeResponseAssembler->>AdminRepository: findAllByIds([authorId])
AdminRepository->>Database: query admin by id
Database-->>AdminRepository: List<Admin>
AdminRepository-->>NoticeResponseAssembler: Admin objects
NoticeResponseAssembler->>NoticeResponseAssembler: toAuthorResponse(admin)
NoticeResponseAssembler->>NoticeResponseAssembler: toResponse(notice, filesMap, authorMap)
NoticeResponseAssembler-->>NoticeQueryService: NoticeResponse
NoticeQueryService-->>Client: NoticeResponse with nested author
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10ce91804
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private final ObjectStorageProperties properties; | ||
|
|
||
| private static final List<String> ALLOWED_EXTENSIONS = Arrays.asList(".jpg", ".jpeg", ".png", ".gif", ".webp"); | ||
| private static final List<String> ALLOWED_EXTENSIONS = Arrays.asList(".jpg", ".jpeg", ".png", ".gif", ".webp", ".svg"); |
There was a problem hiding this comment.
이번 변경으로 .svg가 허용되었는데, 현재 업로드 경로는 NcpStorage#createMetadata에서 file.getContentType()를 그대로 저장해 사용자 제공 콘텐츠를 그대로 브라우저에 노출합니다. SVG는 스크립트/foreignObject를 포함할 수 있는 active content라서, 업로드된 URL을 직접 열거나 임베드하는 환경에서 stored XSS가 발생할 수 있으므로(특히 관리자/운영자가 파일을 미리보기하는 경우) SVG를 계속 허용하려면 sanitize/변환 처리 또는 강제 다운로드 정책 같은 방어가 필요합니다.
Useful? React with 👍 / 👎.
| ) { | ||
| List<FileResponse> files = filesMap.getOrDefault(notice.getId(), List.of()); | ||
|
|
||
| NoticeAuthorResponse author = authorMap.get(notice.getAuthorId()); |
There was a problem hiding this comment.
NoticeResponseAssembler#toResponse는 authorMap.get(notice.getAuthorId()) 결과를 그대로 사용해, admin 조회에서 누락되면 author가 null로 직렬화됩니다. 이 코드베이스는 notice.author_id에 FK가 없고(V13__create_notice_table.sql), admin 삭제가 가능해(AdminManagementService#deleteAdmin) orphan notice가 현실적으로 생길 수 있으므로, 기존에는 항상 전달되던 작성자 식별 정보가 사라져 클라이언트의 author.authorId 접근이 깨집니다; 조회 실패 시 fallback author를 채우거나 명시적 예외로 계약을 유지해야 합니다.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR changes the notice (공지사항) response structure to replace the flat authorId: UUID field with a nested author: { authorId, username } object. A new NoticeResponseAssembler component is introduced to encapsulate the logic for fetching both files and author details (via AdminRepository) and composing them into NoticeResponse DTOs. NoticeQueryService is simplified to delegate all assembly to this new component. Additionally, .svg is added to the list of allowed file upload extensions.
Changes:
- Introduced
NoticeAuthorResponseDTO and replacedauthorIdflat field inNoticeResponsewith the new nestedauthorobject. - Introduced
NoticeResponseAssemblerto batch-fetch files and authors and composeNoticeResponse; refactoredNoticeQueryServiceto delegate to it. AddedfindAllByIdstoAdminRepository/AdminRepositoryImpl/AdminJpaRepositoryto support batch author lookups. - Updated all tests (
NoticeQueryServiceTest,NoticeApiTest,AdminNoticeApiTest) and REST Docs to reflect the new response structure; added.svgto allowed upload extensions inNcpStorage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
NoticeResponse.java |
Replaced flat UUID authorId with nested NoticeAuthorResponse author; updated from() factory method signature |
NoticeAuthorResponse.java |
New DTO record holding author UUID and username |
NoticeResponseAssembler.java |
New component encapsulating batch file + author fetching and response assembly |
NoticeQueryService.java |
Simplified; delegates all assembly to NoticeResponseAssembler |
AdminRepository.java / AdminRepositoryImpl.java / AdminJpaRepository.java |
Added findAllByIds for batch admin lookup |
NoticeQueryServiceTest.java |
Updated mocks and assertions to reflect assembler delegation |
NoticeApiTest.java / AdminNoticeApiTest.java |
Updated REST Docs and assertions for new author response shape |
NcpStorage.java |
Added .svg to allowed upload extensions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Map<Long, List<FileResponse>> filesMap = fetchFilesMap(List.of(notice)); | ||
| Map<UUID, NoticeAuthorResponse> authorMap = fetchAuthorMap(List.of(notice)); | ||
|
|
||
| return toResponse(notice, filesMap, authorMap); |
There was a problem hiding this comment.
The assemble(Notice notice) method creates single-element lists (List.of(notice)) just to call fetchFilesMap and fetchAuthorMap, which were designed for batching over a list of notices. This means for every single-notice lookup, there is still a separate DB query for files and a separate DB query for the author, even though the batching could be shortcut. More importantly, this is slightly wasteful compared to having single-entity variants of the fetch methods. This is a minor design concern but worth noting for future optimization.
| Map<Long, List<FileResponse>> filesMap = fetchFilesMap(List.of(notice)); | |
| Map<UUID, NoticeAuthorResponse> authorMap = fetchAuthorMap(List.of(notice)); | |
| return toResponse(notice, filesMap, authorMap); | |
| Map<Long, List<FileResponse>> filesByEntityId = | |
| fileFinder.findFilesByEntityIds(EntityType.NOTICE, List.of(notice.getId())); | |
| List<FileResponse> files = filesByEntityId.getOrDefault(notice.getId(), List.of()); | |
| NoticeAuthorResponse author = adminRepository.findAllByIds(List.of(notice.getAuthorId())) | |
| .stream() | |
| .findFirst() | |
| .map(this::toAuthorResponse) | |
| .orElse(null); | |
| return NoticeResponse.from(notice, author, files); |
| NoticeAuthorResponse author1 = NoticeAuthorResponse.of(TEST_ADMIN_ID, "admin"); | ||
| NoticeAuthorResponse author2 = NoticeAuthorResponse.of(TEST_ADMIN_ID, "admin"); |
There was a problem hiding this comment.
The two variables author1 and author2 are created separately but are identical (both are NoticeAuthorResponse.of(TEST_ADMIN_ID, "admin")). Using a single author variable (as done in all the other test methods in this PR) would be cleaner and less confusing for readers.
| public class NoticeResponseAssembler { | ||
|
|
||
| private final FileFinder fileFinder; | ||
| private final AdminRepository adminRepository; | ||
|
|
||
| public NoticeResponse assemble(Notice notice) { | ||
| Map<Long, List<FileResponse>> filesMap = fetchFilesMap(List.of(notice)); | ||
| Map<UUID, NoticeAuthorResponse> authorMap = fetchAuthorMap(List.of(notice)); | ||
|
|
||
| return toResponse(notice, filesMap, authorMap); | ||
| } | ||
|
|
||
| public List<NoticeResponse> assembleAll(List<Notice> notices) { | ||
| if (notices.isEmpty()) { | ||
| return List.of(); | ||
| } | ||
|
|
||
| Map<Long, List<FileResponse>> filesMap = fetchFilesMap(notices); | ||
| Map<UUID, NoticeAuthorResponse> authorMap = fetchAuthorMap(notices); | ||
|
|
||
| return notices.stream() | ||
| .map(n -> toResponse(n, filesMap, authorMap)) | ||
| .toList(); | ||
| } | ||
|
|
||
| private NoticeResponse toResponse( | ||
| Notice notice, | ||
| Map<Long, List<FileResponse>> filesMap, | ||
| Map<UUID, NoticeAuthorResponse> authorMap | ||
| ) { | ||
| List<FileResponse> files = filesMap.getOrDefault(notice.getId(), List.of()); | ||
|
|
||
| NoticeAuthorResponse author = authorMap.get(notice.getAuthorId()); | ||
|
|
||
| return NoticeResponse.from(notice, author, files); | ||
| } | ||
|
|
||
| private Map<Long, List<FileResponse>> fetchFilesMap(List<Notice> notices) { | ||
| List<Long> ids = notices.stream().map(Notice::getId).toList(); | ||
| if (ids.isEmpty()) { | ||
| return Collections.emptyMap(); | ||
| } | ||
| return fileFinder.findFilesByEntityIds(EntityType.NOTICE, ids); | ||
| } | ||
|
|
||
| private Map<UUID, NoticeAuthorResponse> fetchAuthorMap(List<Notice> notices) { | ||
| List<UUID> authorIds = notices.stream() | ||
| .map(Notice::getAuthorId) | ||
| .distinct() | ||
| .toList(); | ||
|
|
||
| if (authorIds.isEmpty()) { | ||
| return Collections.emptyMap(); | ||
| } | ||
|
|
||
| return adminRepository.findAllByIds(authorIds).stream() | ||
| .map(this::toAuthorResponse) | ||
| .collect(Collectors.toMap(NoticeAuthorResponse::authorId, Function.identity())); | ||
| } | ||
|
|
||
| private NoticeAuthorResponse toAuthorResponse(Admin admin) { | ||
| return NoticeAuthorResponse.of( | ||
| admin.getId(), | ||
| admin.getUsername().value() | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is no test class for NoticeResponseAssembler, which is new, non-trivial logic containing file fetching, author resolution, and combining of results. Similar application-layer services in the codebase (e.g., FileQueryServiceTest, FileModifyServiceTest, NoticeModifyServiceTest) all have dedicated unit tests. The assembler's behavior — including the null author case, the empty list short-circuit path in assembleAll, and the batching logic in fetchAuthorMap — should be covered by dedicated unit tests.
| import com.souzip.application.notice.provided.NoticeFinder; | ||
| import com.souzip.application.notice.provided.NoticeRegister; | ||
| import com.souzip.docs.RestDocsSupport; | ||
| import com.souzip.domain.admin.model.AdminRole; |
There was a problem hiding this comment.
The AdminRole import is unused in this file. No usage of AdminRole was found anywhere in AdminNoticeApiTest.java. This import was added in this PR but is never referenced in the test code.
| import com.souzip.domain.admin.model.AdminRole; |
| ) { | ||
| List<FileResponse> files = filesMap.getOrDefault(notice.getId(), List.of()); | ||
|
|
||
| NoticeAuthorResponse author = authorMap.get(notice.getAuthorId()); |
There was a problem hiding this comment.
In toResponse(), authorMap.get(notice.getAuthorId()) returns null when the author is not found in the map (e.g., if an admin was deleted after writing the notice, or if the findAllByIds query returns incomplete results). This null is then passed directly into NoticeResponse.from(notice, author, files), resulting in a NoticeResponse with a null author field. When this response is serialized or accessed (e.g., response.author().authorId()), it will cause a NullPointerException or produce null in the JSON output, which can break clients expecting the author object.
Consider using authorMap.getOrDefault(notice.getAuthorId(), ...) with a fallback value, or throwing a descriptive exception when the author cannot be resolved.
| NoticeAuthorResponse author = authorMap.get(notice.getAuthorId()); | |
| NoticeAuthorResponse author = authorMap.get(notice.getAuthorId()); | |
| if (author == null) { | |
| throw new IllegalStateException( | |
| "Author not found for notice. noticeId=" + notice.getId() | |
| + ", authorId=" + notice.getAuthorId() | |
| ); | |
| } |
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선 사항