Conversation
Develop-KIM
added
documentation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (20)
Walkthrough공지사항(Notice) 기능 추가, 파일 entityType을 String→EntityType enum으로 리팩토링, Coordinate를 record→Lombok 클래스으로 전환, 관리자 문서 재구성 및 관련 테스트 대규모 추가가 포함된 변경입니다. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant AdminApi as AdminNoticeApi
participant NoticeReg as NoticeRegister
participant Repo as NoticeRepository
participant FileMod as FileModifier
participant FileStorage as FileStorage
Client->>AdminApi: POST /api/admin/notices (multipart)
AdminApi->>NoticeReg: register(registerRequest, files)
NoticeReg->>Repo: save(Notice)
Repo-->>NoticeReg: Notice(with id)
NoticeReg->>FileMod: register(userId, EntityType.NOTICE, noticeId, file, order)
FileMod->>FileStorage: upload(file)
FileStorage-->>FileMod: storageKey
FileMod-->>NoticeReg: File entity
NoticeReg-->>AdminApi: NoticeResponse
AdminApi-->>Client: 200 SuccessResponse<NoticeResponse>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: ce1dbd9189
ℹ️ 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".
| <entity class="com.souzip.domain.notice.Notice"> | ||
| <table name="notice"/> |
There was a problem hiding this comment.
Notice 엔티티 매핑을 추가했지만(orm.xml) src/main/resources/db/migration에는 notice 테이블을 생성하는 신규 Flyway SQL이 없습니다. 현재 설정상 dev/prod는 spring.jpa.hibernate.ddl-auto=validate이므로 배포 환경에서 애플리케이션 기동 시 스키마 검증이 즉시 실패합니다. 엔티티 추가와 함께 동일 커밋에 테이블 생성(및 필요한 인덱스) 마이그레이션을 포함해야 합니다.
Useful? React with 👍 / 👎.
|
|
||
| import java.util.List; | ||
|
|
||
| @RequestMapping("/api/notices") |
There was a problem hiding this comment.
이 컨트롤러는 일반 앱 공지 조회 API를 노출하지만, 현재 보안 설정(SecurityConfig)은 화이트리스트 경로만 permitAll이고 나머지는 anyRequest().authenticated()라서 /api/notices가 인증 없이 호출되지 않습니다. 즉, 문서에 명시된 공개 조회 동작과 달리 실제로는 401이 발생하므로, 경로를 명시적으로 permitAll에 추가하거나 공개 접근 정책을 적용해야 합니다.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,8 @@ | |||
| package com.souzip.application.notice; | |||
|
|
|||
| public class NoticeNotFoundException extends RuntimeException { | |||
There was a problem hiding this comment.
NoticeNotFoundException이 RuntimeException으로만 정의되어 있어 GlobalExceptionHandler의 BusinessException 핸들러를 타지 않고, 최종 Exception 핸들러에서 500으로 처리됩니다. 따라서 존재하지 않는 noticeId 조회가 클라이언트 오류(예: 404)가 아니라 서버 오류로 노출되므로, 도메인 에러 타입으로 매핑되도록 예외 계층 또는 핸들러를 맞춰야 합니다.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
src/test/java/com/souzip/domain/location/LocationTest.java (1)
29-55: 검증 실패 테스트 케이스 - LGTM
null필드와 범위 초과 위도에 대한 검증 테스트가 잘 작성되었습니다. 다만, 경도 범위 검증(-180 ~ 180) 테스트 케이스도 추가하면 더 완전한 커버리지를 확보할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/souzip/domain/location/LocationTest.java` around lines 29 - 55, Add a test assertion in the createFail test to verify longitude range validation: call Location.create with a LocationCreateRequest (use valid name and address and a valid latitude) but set longitude to an out-of-range value (e.g., BigDecimal.valueOf(200)) and assert it throws IllegalStateException with a message containing "경도는 -180 ~ 180 사이여야 합니다"; reference the existing createFail test, Location.create method, and LocationCreateRequest constructor to locate where to add this new assertion.src/main/java/com/souzip/domain/shared/Coordinate.java (1)
13-19: @NoArgsConstructor와 불변 검증 우회 가능성 확인 필요
@NoArgsConstructor(access = PROTECTED)를 사용하면 JPA/Hibernate가 리플렉션을 통해 객체를 생성할 때validateLatitude/validateLongitude검증을 우회하게 됩니다. 이는 JPA 엔티티 매핑을 위해 의도된 설계로 보이나, 데이터베이스에 잘못된 좌표값이 저장될 경우 검증 없이 로드될 수 있습니다.데이터베이스 레벨의 제약조건(CHECK constraint)이나
@PostLoad검증을 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/souzip/domain/shared/Coordinate.java` around lines 13 - 19, The protected no-args constructor on Coordinate (annotated with `@NoArgsConstructor`(access = AccessLevel.PROTECTED)) lets JPA/Hibernate instantiate objects without running validateLatitude/validateLongitude, so add a defensive validation step that runs after JPA loads the entity: implement a `@PostLoad` method in the Coordinate class that calls the existing validateLatitude and validateLongitude methods (or centralize validation into a private validate() invoked from the public constructors and `@PostLoad`) to ensure invalid coordinates cannot be silently loaded; alternatively add a DB CHECK constraint for latitude/longitude ranges if you prefer enforcement at the database level.src/test/java/com/souzip/application/file/FileQueryServiceTest.java (1)
111-129: 조기 반환 케이스에 외부 의존성 미호출 검증을 추가하면 더 안전합니다.null/empty 입력 및 빈 파일 목록 케이스에서 repository/storage 호출이 없음을 명시적으로 검증하면 성능/회귀 관점에서 테스트 신뢰도가 올라갑니다.
테스트 보강 예시 diff
import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; +import static org.mockito.BDDMockito.then; +import static org.mockito.Mockito.never; @@ void findThumbnailsByEntityIds_emptyList() { // when Map<Long, File> result = fileQueryService.findThumbnailsByEntityIds(EntityType.NOTICE, List.of()); // then assertThat(result).isEmpty(); + then(fileRepository).shouldHaveNoInteractions(); } @@ void findThumbnailsByEntityIds_nullList() { // when Map<Long, File> result = fileQueryService.findThumbnailsByEntityIds(EntityType.NOTICE, null); // then assertThat(result).isEmpty(); + then(fileRepository).shouldHaveNoInteractions(); } @@ void findFileResponsesByEntity_emptyFiles() { // given given(fileRepository.findByEntityTypeAndEntityIdOrderByDisplayOrderAsc(EntityType.NOTICE, 1L)) .willReturn(List.of()); // when List<FileResponse> result = fileQueryService.findFileResponsesByEntity(EntityType.NOTICE, 1L); // then assertThat(result).isEmpty(); + then(fileStorage).should(never()).generateUrl(eq("storage-key-1")); }Also applies to: 162-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/souzip/application/file/FileQueryServiceTest.java` around lines 111 - 129, The two tests in FileQueryServiceTest (findThumbnailsByEntityIds_emptyList and findThumbnailsByEntityIds_nullList) should assert that no external calls are made when input is empty/null: after calling fileQueryService.findThumbnailsByEntityIds and asserting result.isEmpty(), add Mockito verifications such as verifyNoInteractions(fileRepository, fileStorageService) or verify(fileRepository, never()).find*(...) and verify(fileStorageService, never()).*(...) to ensure repository/storage mocks are not invoked; apply the same verification to the related test(s) around lines 162-174 that cover the "empty file list" scenario so all early-return cases check for no interactions with fileRepository/fileStorageService.src/main/java/com/souzip/domain/file/EntityType.java (1)
19-24:from()메서드에서 null 입력 처리 고려
from(String value)메서드에서value가 null인 경우type.value.equals(value)비교 시 의도치 않은 동작이 발생할 수 있습니다.Objects.equals()를 사용하거나 명시적인 null 체크를 추가하면 더 안전합니다.♻️ 제안하는 수정 사항
public static EntityType from(String value) { + if (value == null) { + throw new IllegalArgumentException("EntityType value cannot be null"); + } return Arrays.stream(values()) .filter(type -> type.value.equals(value)) .findFirst() .orElseThrow(() -> new IllegalArgumentException("Unknown EntityType: " + value)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/souzip/domain/file/EntityType.java` around lines 19 - 24, The from(String value) method in EntityType risks NPE or incorrect comparisons when value is null; update EntityType.from to handle nulls explicitly by either returning the matching enum via Objects.equals(type.value, value) in the stream predicate or adding a null guard (if value == null) and comparing appropriately to type.value, ensuring the filter uses a null-safe comparison for type.value and the input value before calling orElseThrow.src/main/resources/META-INF/orm.xml (1)
75-92: 공지사항 조회 패턴에 맞는 인덱스도 함께 고려해 주세요.
status + created_at(활성 공지 최신순)와created_at(전체 최신순) 인덱스가 없으면 공지 데이터가 늘 때 조회 비용이 커질 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/META-INF/orm.xml` around lines 75 - 92, Add database index definitions to the Notice entity mapping so queries can use status + created_at and created_at indexes: update the <entity class="com.souzip.domain.notice.Notice"> mapping (table="notice") to include an index for the composite columns status and created_at (for "active notices ordered by newest") and a separate index on created_at (for overall newest queries); reference the column names "status" and "created_at" in the new index definitions in orm.xml so the JPA/DDL generator will create those indexes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/souzip/adapter/webapi/admin/AdminNoticeApi.java`:
- Around line 30-52: Add bean validation to the NoticeRequest record (annotate
its fields: title with `@NotBlank` and `@Size`(max=...) , content with `@NotBlank` and
`@Size`(max=...), and status with `@NotNull`) and then apply `@Valid` to the
controller request parts in the AdminNoticeApi.register(...) and
AdminNoticeApi.update(...) methods (i.e., annotate the `@RequestPart`("notice")
parameter with `@Valid`) so Spring will enforce these constraints at controller
entry; ensure the necessary javax.validation / jakarta.validation imports are
added and adjust maximum sizes to match project conventions.
In `@src/main/java/com/souzip/adapter/webapi/admin/dto/NoticeRequest.java`:
- Around line 9-13: The NoticeRequest record's fields (title, content, status)
lack validation; add Jakarta/hibernate validator annotations on the record
components: annotate title and content with `@NotBlank` and `@Size`(max=...) (choose
sensible max lengths, e.g. title 255, content 5000) and annotate status with
`@NotNull` (and/or a custom enum whitelist validator if you need strict enum code
checking). Ensure the controller uses `@Valid/`@Validated so these constraints on
NoticeRequest are enforced at the API boundary.
In `@src/main/java/com/souzip/application/file/FileNotFoundException.java`:
- Around line 11-13: The constructor FileNotFoundException(EntityType
entityType, Long entityId) uses String.format with "%d" which will NPE when
entityId is null; change the message formatting to be null-safe (e.g., use "%s"
or String.valueOf(entityId) / Objects.toString(entityId, "null")) so the
exception message can be built even when entityId is null and still include a
readable entityId representation.
In `@src/main/java/com/souzip/application/notice/NoticeModifyService.java`:
- Around line 49-53: The file storage calls deleteFiles(...) and
uploadFiles(...) in NoticeModifyService run inside the DB flow but are not
atomic with the DB transaction (so storage is not rolled back if the DB save
fails); fix by deferring external storage side-effects until after the DB
transaction commits: move the calls to run in a post-commit hook (e.g.,
TransactionSynchronizationManager.registerSynchronization or an
`@TransactionalEventListener`) so that noticeRepository.save(notice) is executed
inside the transaction and only after successful commit you call
deleteFiles(deleteFileIds) and uploadFiles(noticeId,
notice.getAuthorId().toString(), newFiles); alternatively upload to a temp area
and finalize/move files in the after-commit hook and only delete originals after
commit—use the methods deleteFiles, uploadFiles, and noticeRepository.save in
NoticeModifyService to implement this change.
- Around line 87-93: The deleteFiles method in NoticeModifyService currently
calls fileModifier.delete for each id without verifying ownership, risking
deletion of files not belonging to the target notice; change deleteFiles to
accept the current notice id (or access it in scope), load the File entities for
deleteFileIds via the file repository (e.g., fileRepository.findAllByIdIn or a
custom findByIds) and filter/verify that each file.getNoticeId() (or
file.getNotice().getId()) matches the notice id; only after verification call
fileModifier.delete for those that belong, and for any mismatches either skip
and log or throw an exception (e.g., IllegalArgumentException) to prevent
accidental cross-entity deletes. Ensure references to deleteFiles and
fileModifier.delete are updated accordingly.
In `@src/main/java/com/souzip/application/notice/NoticeQueryService.java`:
- Around line 50-60: findAllActiveWithFiles and findAllWithFiles cause N+1
queries because toResponseWithFiles calls fileFinder.findFileResponsesByEntity
per notice; instead, batch-fetch files for all notices at once (or use a
fetch-join repository method) and then attach them when mapping to responses.
Concretely: in findAllActiveWithFiles/findAllWithFiles collect all notice ids
from findAllActive()/findAll(), call a new or existing fileFinder method that
returns file responses keyed by owner id (e.g., findFileResponsesByEntities or
similar), then update toResponseWithFiles (or add an overload) to accept the
pre-fetched files map and build NoticeResponse without per-notice DB calls.
Ensure to deprecate/remove the per-entity fileFinder call in
toResponseWithFiles.
In `@src/main/java/com/souzip/domain/notice/Notice.java`:
- Around line 23-38: Add a null-check for the incoming request object at the
start of both Notice.register(NoticeRegisterRequest) and
Notice.update(NoticeUpdateRequest) to avoid an NPE when accessing request
fields; call requireNonNull(request, "요청 객체는 필수입니다.") (or a similar message)
before using request.title(), request.content(), request.authorId(), or
request.status() so the contract is explicit and failures produce a clear
message.
In `@src/main/java/com/souzip/domain/notice/NoticeRegisterRequest.java`:
- Around line 5-18: NoticeRegisterRequest lacks input validation for title,
content, authorId, and status; add validation by either adding Bean Validation
annotations on the record components (e.g., `@NotBlank/`@Size for title/content,
`@NotNull` for authorId/status) or implement a compact canonical constructor in
the record (the NoticeRegisterRequest(...) constructor) that enforces checks
(Objects.requireNonNull for authorId/status, reject blank/oversized
title/content with clear IllegalArgumentException messages) and ensure the
provided status is a valid NoticeStatus value if you need a whitelist. Update
the of(...) factory to rely on the validated constructor so invalid requests are
rejected early.
In `@src/main/java/com/souzip/domain/notice/NoticeUpdateRequest.java`:
- Around line 3-10: Add input validation to the NoticeUpdateRequest record so
null/blank titles or contents and null statuses cannot be created: annotate the
record components in NoticeUpdateRequest (title, content, status) with
validation constraints (e.g., `@NotBlank` and `@Size`(max=...) for title and
content, `@NotNull` for status) and ensure any enum whitelist/allowed values are
enforced (or validated) before constructing via the of(...) factory; also
trigger validation in the controller/service layer where NoticeUpdateRequest is
consumed so invalid requests are rejected.
---
Nitpick comments:
In `@src/main/java/com/souzip/domain/file/EntityType.java`:
- Around line 19-24: The from(String value) method in EntityType risks NPE or
incorrect comparisons when value is null; update EntityType.from to handle nulls
explicitly by either returning the matching enum via Objects.equals(type.value,
value) in the stream predicate or adding a null guard (if value == null) and
comparing appropriately to type.value, ensuring the filter uses a null-safe
comparison for type.value and the input value before calling orElseThrow.
In `@src/main/java/com/souzip/domain/shared/Coordinate.java`:
- Around line 13-19: The protected no-args constructor on Coordinate (annotated
with `@NoArgsConstructor`(access = AccessLevel.PROTECTED)) lets JPA/Hibernate
instantiate objects without running validateLatitude/validateLongitude, so add a
defensive validation step that runs after JPA loads the entity: implement a
`@PostLoad` method in the Coordinate class that calls the existing
validateLatitude and validateLongitude methods (or centralize validation into a
private validate() invoked from the public constructors and `@PostLoad`) to ensure
invalid coordinates cannot be silently loaded; alternatively add a DB CHECK
constraint for latitude/longitude ranges if you prefer enforcement at the
database level.
In `@src/main/resources/META-INF/orm.xml`:
- Around line 75-92: Add database index definitions to the Notice entity mapping
so queries can use status + created_at and created_at indexes: update the
<entity class="com.souzip.domain.notice.Notice"> mapping (table="notice") to
include an index for the composite columns status and created_at (for "active
notices ordered by newest") and a separate index on created_at (for overall
newest queries); reference the column names "status" and "created_at" in the new
index definitions in orm.xml so the JPA/DDL generator will create those indexes.
In `@src/test/java/com/souzip/application/file/FileQueryServiceTest.java`:
- Around line 111-129: The two tests in FileQueryServiceTest
(findThumbnailsByEntityIds_emptyList and findThumbnailsByEntityIds_nullList)
should assert that no external calls are made when input is empty/null: after
calling fileQueryService.findThumbnailsByEntityIds and asserting
result.isEmpty(), add Mockito verifications such as
verifyNoInteractions(fileRepository, fileStorageService) or
verify(fileRepository, never()).find*(...) and verify(fileStorageService,
never()).*(...) to ensure repository/storage mocks are not invoked; apply the
same verification to the related test(s) around lines 162-174 that cover the
"empty file list" scenario so all early-return cases check for no interactions
with fileRepository/fileStorageService.
In `@src/test/java/com/souzip/domain/location/LocationTest.java`:
- Around line 29-55: Add a test assertion in the createFail test to verify
longitude range validation: call Location.create with a LocationCreateRequest
(use valid name and address and a valid latitude) but set longitude to an
out-of-range value (e.g., BigDecimal.valueOf(200)) and assert it throws
IllegalStateException with a message containing "경도는 -180 ~ 180 사이여야 합니다";
reference the existing createFail test, Location.create method, and
LocationCreateRequest constructor to locate where to add this new assertion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
src/docs/asciidoc/admin/admin.adocsrc/docs/asciidoc/admin/auth.adocsrc/docs/asciidoc/admin/index.adocsrc/docs/asciidoc/admin/location.adocsrc/docs/asciidoc/admin/management.adocsrc/docs/asciidoc/admin/notice.adocsrc/docs/asciidoc/app/index.adocsrc/docs/asciidoc/app/notice.adocsrc/docs/도메인모델.mdsrc/docs/용어사전.mdsrc/main/java/com/souzip/adapter/integration/googlegeocoding/GoogleGeocoding.javasrc/main/java/com/souzip/adapter/storage/file/InvalidFileException.javasrc/main/java/com/souzip/adapter/storage/file/NcpStorage.javasrc/main/java/com/souzip/adapter/webapi/admin/AdminNoticeApi.javasrc/main/java/com/souzip/adapter/webapi/admin/dto/NoticeRequest.javasrc/main/java/com/souzip/adapter/webapi/location/dto/SearchResponse.javasrc/main/java/com/souzip/adapter/webapi/user/NoticeApi.javasrc/main/java/com/souzip/application/file/FileModifyService.javasrc/main/java/com/souzip/application/file/FileNotFoundException.javasrc/main/java/com/souzip/application/file/FileQueryService.javasrc/main/java/com/souzip/application/file/provided/FileFinder.javasrc/main/java/com/souzip/application/file/provided/FileModifier.javasrc/main/java/com/souzip/application/file/required/FileRepository.javasrc/main/java/com/souzip/application/file/required/FileStorage.javasrc/main/java/com/souzip/application/notice/NoticeModifyService.javasrc/main/java/com/souzip/application/notice/NoticeNotFoundException.javasrc/main/java/com/souzip/application/notice/NoticeQueryService.javasrc/main/java/com/souzip/application/notice/dto/NoticeResponse.javasrc/main/java/com/souzip/application/notice/provided/NoticeFinder.javasrc/main/java/com/souzip/application/notice/provided/NoticeRegister.javasrc/main/java/com/souzip/application/notice/required/NoticeRepository.javasrc/main/java/com/souzip/domain/file/EntityType.javasrc/main/java/com/souzip/domain/file/EntityTypeConverter.javasrc/main/java/com/souzip/domain/file/File.javasrc/main/java/com/souzip/domain/file/FileRegisterRequest.javasrc/main/java/com/souzip/domain/notice/Notice.javasrc/main/java/com/souzip/domain/notice/NoticeRegisterRequest.javasrc/main/java/com/souzip/domain/notice/NoticeStatus.javasrc/main/java/com/souzip/domain/notice/NoticeUpdateRequest.javasrc/main/java/com/souzip/domain/recommend/ai/service/AiRecommendationService.javasrc/main/java/com/souzip/domain/recommend/general/service/GeneralRecommendationService.javasrc/main/java/com/souzip/domain/shared/Coordinate.javasrc/main/java/com/souzip/domain/souvenir/service/SouvenirService.javasrc/main/java/com/souzip/domain/user/service/UserService.javasrc/main/resources/META-INF/orm.xmlsrc/main/resources/application-dev.yamlsrc/main/resources/application-local.yamlsrc/main/resources/application-prod.yamlsrc/test/java/com/souzip/adapter/integration/googleplaces/GooglePlacesTest.javasrc/test/java/com/souzip/adapter/storage/file/NcpStorageTest.javasrc/test/java/com/souzip/adapter/webapi/admin/AdminNoticeApiTest.javasrc/test/java/com/souzip/adapter/webapi/user/NoticeApiTest.javasrc/test/java/com/souzip/application/file/FileModifyServiceTest.javasrc/test/java/com/souzip/application/file/FileQueryServiceTest.javasrc/test/java/com/souzip/application/notice/NoticeModifyServiceTest.javasrc/test/java/com/souzip/application/notice/NoticeQueryServiceTest.javasrc/test/java/com/souzip/domain/file/EntityTypeTest.javasrc/test/java/com/souzip/domain/file/FileTest.javasrc/test/java/com/souzip/domain/location/LocationTest.javasrc/test/java/com/souzip/domain/notice/NoticeTest.java
💤 Files with no reviewable changes (7)
- src/main/resources/application-local.yaml
- src/main/resources/application-prod.yaml
- src/main/java/com/souzip/adapter/storage/file/NcpStorage.java
- src/docs/asciidoc/admin/admin.adoc
- src/main/java/com/souzip/application/file/required/FileStorage.java
- src/test/java/com/souzip/adapter/storage/file/NcpStorageTest.java
- src/main/resources/application-dev.yaml
| public SuccessResponse<NoticeResponse> register( | ||
| @CurrentAdminId UUID adminId, | ||
| @RequestPart("notice") NoticeRequest request, | ||
| @RequestPart(value = "files", required = false) Optional<List<MultipartFile>> files | ||
| ) { | ||
| Notice notice = noticeRegister.register( | ||
| request.toDomain(adminId), | ||
| files.orElse(List.of()) | ||
| ); | ||
|
|
||
| NoticeResponse response = noticeFinder.findByIdWithFiles(notice.getId()); | ||
|
|
||
| return SuccessResponse.of(response, "공지사항이 등록되었습니다."); | ||
| } | ||
|
|
||
| @AdminAccess | ||
| @PutMapping("/{noticeId}") | ||
| public SuccessResponse<NoticeResponse> update( | ||
| @PathVariable Long noticeId, | ||
| @RequestPart("notice") NoticeRequest request, | ||
| @RequestPart(value = "deleteFileIds", required = false) Optional<List<Long>> deleteFileIds, | ||
| @RequestPart(value = "newFiles", required = false) Optional<List<MultipartFile>> newFiles | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 목적: NoticeRequest 제약 정의 여부와 `@RequestPart` + `@Valid` 적용 상태 확인
set -e
echo "== NoticeRequest 정의 확인 =="
fd 'NoticeRequest.java$' src --exec cat -n {} \;
echo ""
echo "== src/main/java 내 Bean Validation 제약 애너테이션 위치 =="
rg -n --type=java '@(NotNull|NotBlank|Size|Pattern|Positive|PositiveOrZero|Email)' src/main/java 2>/dev/null | head -50
echo ""
echo "== AdminNoticeApi의 메서드 시그니처 확인 =="
sed -n '30,52p' src/main/java/com/souzip/adapter/webapi/admin/AdminNoticeApi.java
echo ""
echo "== AdminNoticeApi의 `@RequestPart` + `@Valid` 적용 상태 =="
rg -B2 -A5 '@RequestPart.*"notice"' src/main/java/com/souzip/adapter/webapi/admin/AdminNoticeApi.java
echo ""
echo "== `@Valid` 임포트 확인 =="
rg -n 'import.*Valid' src/main/java/com/souzip/adapter/webapi/admin/AdminNoticeApi.java
echo ""
echo "== 다른 컨트롤러의 `@Valid` + `@RequestPart` 사용 패턴 =="
rg -B1 -A2 '@Valid.*@RequestPart|@RequestPart.*@Valid' src/main/java --type=java 2>/dev/null | head -30Repository: souzip/souzip-api
Length of output: 8691
NoticeRequest에 검증 제약을 추가하고 @Valid 적용 필요
NoticeRequest 레코드에 검증 제약이 전혀 없습니다. 코드베이스의 다른 Request DTO(SouvenirRequest, OnboardingRequest 등)는 @NotBlank, @Size 등의 제약을 정의하고 있으며, 컨트롤러에서 @Valid를 사용하여 검증을 실행합니다. NoticeRequest도 동일한 패턴을 따라야 합니다.
title: 필수 값이고 길이 제한 필요 (예:@NotBlank,@Size)content: 필수 값이고 길이 제한 필요 (예:@NotBlank,@Size)status: 필수 값 (예:@NotNull)
그 다음, 컨트롤러의 register(), update() 메서드에서 @RequestPart 파라미터에 @Valid 추가:
적용 예시
+import jakarta.validation.Valid;
+
public record NoticeRequest(
+ `@NotBlank`(message = "공지사항 제목은 필수입니다.")
+ `@Size`(max = 200, message = "공지사항 제목은 200자 이하여야 합니다.")
String title,
+ `@NotBlank`(message = "공지사항 내용은 필수입니다.")
+ `@Size`(max = 5000, message = "공지사항 내용은 5000자 이하여야 합니다.")
String content,
+ `@NotNull`(message = "공지사항 상태는 필수입니다.")
NoticeStatus status
) { } public SuccessResponse<NoticeResponse> register(
`@CurrentAdminId` UUID adminId,
- `@RequestPart`("notice") NoticeRequest request,
+ `@Valid` `@RequestPart`("notice") NoticeRequest request,
`@RequestPart`(value = "files", required = false) Optional<List<MultipartFile>> files
) {
public SuccessResponse<NoticeResponse> update(
`@PathVariable` Long noticeId,
- `@RequestPart`("notice") NoticeRequest request,
+ `@Valid` `@RequestPart`("notice") NoticeRequest request,
`@RequestPart`(value = "deleteFileIds", required = false) Optional<List<Long>> deleteFileIds,
`@RequestPart`(value = "newFiles", required = false) Optional<List<MultipartFile>> newFiles
) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/souzip/adapter/webapi/admin/AdminNoticeApi.java` around
lines 30 - 52, Add bean validation to the NoticeRequest record (annotate its
fields: title with `@NotBlank` and `@Size`(max=...) , content with `@NotBlank` and
`@Size`(max=...), and status with `@NotNull`) and then apply `@Valid` to the
controller request parts in the AdminNoticeApi.register(...) and
AdminNoticeApi.update(...) methods (i.e., annotate the `@RequestPart`("notice")
parameter with `@Valid`) so Spring will enforce these constraints at controller
entry; ensure the necessary javax.validation / jakarta.validation imports are
added and adjust maximum sizes to match project conventions.
| deleteFiles(deleteFileIds); | ||
|
|
||
| uploadFiles(noticeId, notice.getAuthorId().toString(), newFiles); | ||
|
|
||
| return noticeRepository.save(notice); |
There was a problem hiding this comment.
파일 스토리지 작업과 DB 트랜잭션의 원자성이 보장되지 않습니다.
파일 삭제/업로드는 외부 부작용인데, 이후 DB 단계에서 예외가 나면 롤백되어도 스토리지는 복구되지 않아 데이터 불일치(끊어진 참조/고아 파일)가 발생할 수 있습니다.
Also applies to: 75-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/souzip/application/notice/NoticeModifyService.java` around
lines 49 - 53, The file storage calls deleteFiles(...) and uploadFiles(...) in
NoticeModifyService run inside the DB flow but are not atomic with the DB
transaction (so storage is not rolled back if the DB save fails); fix by
deferring external storage side-effects until after the DB transaction commits:
move the calls to run in a post-commit hook (e.g.,
TransactionSynchronizationManager.registerSynchronization or an
`@TransactionalEventListener`) so that noticeRepository.save(notice) is executed
inside the transaction and only after successful commit you call
deleteFiles(deleteFileIds) and uploadFiles(noticeId,
notice.getAuthorId().toString(), newFiles); alternatively upload to a temp area
and finalize/move files in the after-commit hook and only delete originals after
commit—use the methods deleteFiles, uploadFiles, and noticeRepository.save in
NoticeModifyService to implement this change.
src/main/java/com/souzip/application/notice/NoticeModifyService.java
Outdated
Show resolved
Hide resolved
| public List<NoticeResponse> findAllActiveWithFiles() { | ||
| return findAllActive().stream() | ||
| .map(this::toResponseWithFiles) | ||
| .toList(); | ||
| } | ||
|
|
||
| @Override | ||
| public List<NoticeResponse> findAllWithFiles() { | ||
| return findAll().stream() | ||
| .map(this::toResponseWithFiles) | ||
| .toList(); |
There was a problem hiding this comment.
목록 조회에서 파일 조회 N+1이 발생합니다.
Line 5153, Line 5860에서 공지마다 fileFinder.findFileResponsesByEntity(...)를 호출해 공지 건수만큼 추가 조회가 발생합니다. 데이터가 늘면 getAll/getAllActive 응답 지연이 커집니다.
♻️ 배치 조회 기반 개선 예시
`@Override`
public List<NoticeResponse> findAllActiveWithFiles() {
- return findAllActive().stream()
- .map(this::toResponseWithFiles)
- .toList();
+ return toResponsesWithFiles(findAllActive());
}
`@Override`
public List<NoticeResponse> findAllWithFiles() {
- return findAll().stream()
- .map(this::toResponseWithFiles)
- .toList();
+ return toResponsesWithFiles(findAll());
}
+private List<NoticeResponse> toResponsesWithFiles(List<Notice> notices) {
+ if (notices.isEmpty()) {
+ return List.of();
+ }
+ List<Long> noticeIds = notices.stream()
+ .map(Notice::getId)
+ .toList();
+ Map<Long, List<FileResponse>> filesByNoticeId =
+ fileFinder.findFileResponsesByEntityIds(EntityType.NOTICE, noticeIds);
+
+ return notices.stream()
+ .map(notice -> NoticeResponse.from(
+ notice,
+ filesByNoticeId.getOrDefault(notice.getId(), List.of())
+ ))
+ .toList();
+}Based on learnings: Applies to /{repository,service}//*.{java,kt} : Spot N+1 issues; prefer fetch joins or batch where appropriate.
Also applies to: 63-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/souzip/application/notice/NoticeQueryService.java` around
lines 50 - 60, findAllActiveWithFiles and findAllWithFiles cause N+1 queries
because toResponseWithFiles calls fileFinder.findFileResponsesByEntity per
notice; instead, batch-fetch files for all notices at once (or use a fetch-join
repository method) and then attach them when mapping to responses. Concretely:
in findAllActiveWithFiles/findAllWithFiles collect all notice ids from
findAllActive()/findAll(), call a new or existing fileFinder method that returns
file responses keyed by owner id (e.g., findFileResponsesByEntities or similar),
then update toResponseWithFiles (or add an overload) to accept the pre-fetched
files map and build NoticeResponse without per-notice DB calls. Ensure to
deprecate/remove the per-entity fileFinder call in toResponseWithFiles.
| public static Notice register(NoticeRegisterRequest request) { | ||
| Notice notice = new Notice(); | ||
|
|
||
| notice.title = requireNonNull(request.title(), "제목은 필수입니다."); | ||
| notice.content = requireNonNull(request.content(), "내용은 필수입니다."); | ||
| notice.authorId = requireNonNull(request.authorId(), "작성자는 필수입니다."); | ||
| notice.status = requireNonNull(request.status(), "상태는 필수입니다."); | ||
|
|
||
| return notice; | ||
| } | ||
|
|
||
| public void update(NoticeUpdateRequest request) { | ||
| this.title = requireNonNull(request.title(), "제목은 필수입니다."); | ||
| this.content = requireNonNull(request.content(), "내용은 필수입니다."); | ||
| this.status = requireNonNull(request.status(), "상태는 필수입니다."); | ||
| } |
There was a problem hiding this comment.
요청 객체 자체 null 검증을 추가해 주세요.
Line 23, Line 34에서 request가 null이면 필드 접근 시 메시지 없는 NPE가 발생합니다. 입력 계약을 명확히 하기 위해 객체 자체를 먼저 requireNonNull로 검증하는 편이 안전합니다.
수정 예시
public static Notice register(NoticeRegisterRequest request) {
+ requireNonNull(request, "공지사항 등록 요청은 필수입니다.");
Notice notice = new Notice();
notice.title = requireNonNull(request.title(), "제목은 필수입니다.");
notice.content = requireNonNull(request.content(), "내용은 필수입니다.");
notice.authorId = requireNonNull(request.authorId(), "작성자는 필수입니다.");
notice.status = requireNonNull(request.status(), "상태는 필수입니다.");
return notice;
}
public void update(NoticeUpdateRequest request) {
+ requireNonNull(request, "공지사항 수정 요청은 필수입니다.");
this.title = requireNonNull(request.title(), "제목은 필수입니다.");
this.content = requireNonNull(request.content(), "내용은 필수입니다.");
this.status = requireNonNull(request.status(), "상태는 필수입니다.");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/souzip/domain/notice/Notice.java` around lines 23 - 38, Add
a null-check for the incoming request object at the start of both
Notice.register(NoticeRegisterRequest) and Notice.update(NoticeUpdateRequest) to
avoid an NPE when accessing request fields; call requireNonNull(request, "요청 객체는
필수입니다.") (or a similar message) before using request.title(), request.content(),
request.authorId(), or request.status() so the contract is explicit and failures
produce a clear message.
| public record NoticeUpdateRequest( | ||
| String title, | ||
| String content, | ||
| NoticeStatus status | ||
| ) { | ||
| public static NoticeUpdateRequest of(String title, String content, NoticeStatus status) { | ||
| return new NoticeUpdateRequest(title, content, status); | ||
| } |
There was a problem hiding this comment.
요청 객체 입력 검증이 누락되었습니다.
title/content/status가 null/blank여도 생성되어, 업데이트 경로에서 잘못된 공지 상태가 저장될 수 있습니다.
🔧 제안 수정안
public record NoticeUpdateRequest(
String title,
String content,
NoticeStatus status
) {
+ public NoticeUpdateRequest {
+ if (title == null || title.isBlank()) {
+ throw new IllegalArgumentException("title must not be blank");
+ }
+ if (content == null || content.isBlank()) {
+ throw new IllegalArgumentException("content must not be blank");
+ }
+ if (status == null) {
+ throw new IllegalArgumentException("status must not be null");
+ }
+ }
+
public static NoticeUpdateRequest of(String title, String content, NoticeStatus status) {
return new NoticeUpdateRequest(title, content, status);
}
}As per coding guidelines, **/*{Request,Dto,Request}*.{java,kt}: Validate input: request DTO constraints, whitelists for enums/codes, length limits.
📝 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 record NoticeUpdateRequest( | |
| String title, | |
| String content, | |
| NoticeStatus status | |
| ) { | |
| public static NoticeUpdateRequest of(String title, String content, NoticeStatus status) { | |
| return new NoticeUpdateRequest(title, content, status); | |
| } | |
| public record NoticeUpdateRequest( | |
| String title, | |
| String content, | |
| NoticeStatus status | |
| ) { | |
| public NoticeUpdateRequest { | |
| if (title == null || title.isBlank()) { | |
| throw new IllegalArgumentException("title must not be blank"); | |
| } | |
| if (content == null || content.isBlank()) { | |
| throw new IllegalArgumentException("content must not be blank"); | |
| } | |
| if (status == null) { | |
| throw new IllegalArgumentException("status must not be null"); | |
| } | |
| } | |
| public static NoticeUpdateRequest of(String title, String content, NoticeStatus status) { | |
| return new NoticeUpdateRequest(title, content, status); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/souzip/domain/notice/NoticeUpdateRequest.java` around lines
3 - 10, Add input validation to the NoticeUpdateRequest record so null/blank
titles or contents and null statuses cannot be created: annotate the record
components in NoticeUpdateRequest (title, content, status) with validation
constraints (e.g., `@NotBlank` and `@Size`(max=...) for title and content, `@NotNull`
for status) and ensure any enum whitelist/allowed values are enforced (or
validated) before constructing via the of(...) factory; also trigger validation
in the controller/service layer where NoticeUpdateRequest is consumed so invalid
requests are rejected.
There was a problem hiding this comment.
Pull request overview
This PR implements the notice board feature ([SOU-143]). It introduces a Notice domain entity and full CRUD functionality for admin users, along with a read-only API for regular users. It also refactors the EntityType for files from a raw String to a type-safe EntityType enum, updates the Coordinate class from a Java record to a regular class (for JPA compatibility), moves exception classes to more appropriate packages, and reorganizes admin API documentation into separate AsciiDoc files.
Changes:
- Adds notice domain (
Notice,NoticeStatus,NoticeRegisterRequest,NoticeUpdateRequest), application services (NoticeQueryService,NoticeModifyService), and REST controllers (AdminNoticeApi,NoticeApi) with full CRUD for admin and read-only for users. - Refactors
EntityTypefrom raw strings to a type-safe enum with a JPAAttributeConverter, updating all callers throughout the codebase. - Converts
Coordinatefrom a Java record to a regular class for JPA embeddable compatibility and reorganizes admin API documentation.
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/com/souzip/domain/notice/*.java |
New domain model: Notice, NoticeStatus, NoticeRegisterRequest, NoticeUpdateRequest |
src/main/java/com/souzip/application/notice/*.java |
Application layer: NoticeQueryService, NoticeModifyService, NoticeNotFoundException, DTOs and interfaces |
src/main/java/com/souzip/adapter/webapi/admin/AdminNoticeApi.java |
Admin REST API for full notice CRUD |
src/main/java/com/souzip/adapter/webapi/user/NoticeApi.java |
User-facing read-only notice API |
src/main/java/com/souzip/domain/file/EntityType.java |
New type-safe enum replacing raw String entity type |
src/main/java/com/souzip/domain/file/EntityTypeConverter.java |
JPA converter for EntityType enum ↔ DB string |
src/main/java/com/souzip/domain/file/File.java |
Updated to use EntityType instead of String |
src/main/java/com/souzip/application/file/FileNotFoundException.java |
Relocated from domain package to application package |
src/main/java/com/souzip/application/file/FileQueryService.java |
Updated signatures + new findFileResponsesByEntity method |
src/main/java/com/souzip/domain/shared/Coordinate.java |
Converted from record to class for JPA compatibility |
src/main/resources/META-INF/orm.xml |
Added Notice entity mapping and EntityTypeConverter for File |
src/main/resources/application-*.yaml |
Removed redundant dialect config from profile-specific files |
src/docs/asciidoc/admin/*.adoc |
Reorganized admin docs into separate files + new notice documentation |
src/test/java/... |
Unit/slice tests for all new functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public List<NoticeResponse> findAllActiveWithFiles() { | ||
| return findAllActive().stream() | ||
| .map(this::toResponseWithFiles) | ||
| .toList(); | ||
| } | ||
|
|
||
| @Override | ||
| public List<NoticeResponse> findAllWithFiles() { | ||
| return findAll().stream() | ||
| .map(this::toResponseWithFiles) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
The findAllActiveWithFiles() and findAllWithFiles() methods call fileFinder.findFileResponsesByEntity() for each notice individually, resulting in N+1 database queries: 1 query to load all notices, then N additional queries to load files for each notice. This can be problematic as the number of notices grows. Consider loading all files for the relevant notice IDs in a single batch query (similar to how findThumbnailsByEntityIds works in UserService and other services).
| import com.souzip.application.notice.provided.NoticeFinder; | ||
| import com.souzip.global.common.dto.SuccessResponse; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.web.bind.annotation.*; |
There was a problem hiding this comment.
The wildcard import import org.springframework.web.bind.annotation.*; is used here, but the rest of the codebase consistently uses explicit imports for Spring MVC annotations (e.g., import org.springframework.web.bind.annotation.GetMapping;, import org.springframework.web.bind.annotation.RestController;, etc.). This deviates from the established convention.
| import org.springframework.web.bind.annotation.*; | |
| import org.springframework.web.bind.annotation.GetMapping; | |
| import org.springframework.web.bind.annotation.PathVariable; | |
| import org.springframework.web.bind.annotation.RequestMapping; | |
| import org.springframework.web.bind.annotation.RestController; |
|
|
||
| import java.util.List; | ||
| import java.util.UUID; | ||
| import org.springframework.web.bind.annotation.*; |
There was a problem hiding this comment.
The wildcard import import org.springframework.web.bind.annotation.*; is used here, but the rest of the codebase consistently uses explicit imports for Spring MVC annotations. This deviates from the established convention seen in files like AdminManagementController.java and other controllers.
| import org.springframework.web.bind.annotation.*; | |
| import org.springframework.web.bind.annotation.DeleteMapping; | |
| import org.springframework.web.bind.annotation.GetMapping; | |
| import org.springframework.web.bind.annotation.PatchMapping; | |
| 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.RequestMapping; | |
| import org.springframework.web.bind.annotation.RequestPart; | |
| import org.springframework.web.bind.annotation.RestController; |
|
|
||
| public static List<NoticeResponse> fromList(List<Notice> notices) { | ||
| return notices.stream() | ||
| .map(notice -> from(notice, List.of())) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
The fromList method creates NoticeResponse instances with empty file lists (List.of()), which may return incomplete data. More importantly, this method is not used anywhere in the codebase and appears to be dead code. It should either be removed or replaced with a variant that properly loads files.
| public static List<NoticeResponse> fromList(List<Notice> notices) { | |
| return notices.stream() | |
| .map(notice -> from(notice, List.of())) | |
| .toList(); | |
| } |
| <entity class="com.souzip.domain.notice.Notice"> | ||
| <table name="notice"/> | ||
| <attributes> | ||
| <basic name="title"> | ||
| <column name="title" nullable="false" length="200"/> | ||
| </basic> | ||
| <basic name="content"> | ||
| <column name="content" nullable="false" column-definition="TEXT"/> | ||
| </basic> | ||
| <basic name="authorId"> | ||
| <column name="author_id" nullable="false"/> | ||
| </basic> | ||
| <basic name="status"> | ||
| <column name="status" nullable="false" length="20"/> | ||
| <enumerated>STRING</enumerated> | ||
| </basic> | ||
| </attributes> | ||
| </entity> |
There was a problem hiding this comment.
A Flyway migration script for the notice table is missing. The project uses Flyway (with ddl-auto: validate in dev and prod environments), so the notice table must have a corresponding migration script (e.g., V13__create_notice_table.sql) or it will fail at startup on these environments. The ddl-auto: update in the default/local profile may mask this problem during development.
| public class NoticeNotFoundException extends RuntimeException { | ||
|
|
||
| public NoticeNotFoundException(Long noticeId) { | ||
| super("공지사항을 찾을 수 없습니다. id: " + noticeId); |
There was a problem hiding this comment.
NoticeNotFoundException extends RuntimeException but is not handled in the GlobalExceptionHandler. When a notice is not found, this exception will be caught by the catch-all Exception handler, returning an HTTP 500 Internal Server Error instead of an appropriate 404 Not Found response. For consistency with the API contract, this exception should either extend BusinessException with a suitable ErrorCode, or a dedicated @ExceptionHandler should be added to the global exception handler. FileNotFoundException has the same issue and was introduced in the same PR.
| public class NoticeNotFoundException extends RuntimeException { | |
| public NoticeNotFoundException(Long noticeId) { | |
| super("공지사항을 찾을 수 없습니다. id: " + noticeId); | |
| import com.souzip.global.error.ErrorCode; | |
| import com.souzip.global.error.exception.BusinessException; | |
| public class NoticeNotFoundException extends BusinessException { | |
| public NoticeNotFoundException(Long noticeId) { | |
| super(ErrorCode.NOTICE_NOT_FOUND); |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/test/java/com/souzip/domain/file/EntityTypeTest.java (2)
50-59: 중복 검증 제거를 고려해 주세요.
containsExactly는 이미 요소의 개수와 순서를 모두 검증하므로hasSize(2)호출은 중복됩니다. 제거해도 테스트의 의미는 동일합니다.♻️ 간결화 제안
`@DisplayName`("모든 EntityType이 정의되어 있다") `@Test` void all_types_defined() { // when EntityType[] values = EntityType.values(); // then - assertThat(values).hasSize(2); assertThat(values).containsExactly(EntityType.NOTICE, EntityType.SOUVENIR); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/souzip/domain/file/EntityTypeTest.java` around lines 50 - 59, The test all_types_defined duplicates checks: remove the redundant assertThat(values).hasSize(2) because assertThat(values).containsExactly(EntityType.NOTICE, EntityType.SOUVENIR) already asserts both size and order; update the test in EntityTypeTest (method all_types_defined) to keep only the containsExactly assertion.
27-40: null 입력에 대한 테스트 케이스 추가를 권장합니다.현재 유효하지 않은 문자열에 대한 테스트는 잘 구성되어 있습니다. 다만,
EntityType.from(null)호출 시의 동작도 검증하면 방어적 프로그래밍 관점에서 더 완전한 테스트가 됩니다.💡 null 테스트 케이스 추가 제안
`@DisplayName`("유효하지 않은 문자열은 예외가 발생한다") `@ParameterizedTest` `@CsvSource`({ "INVALID", "notice", "NOTICE", "Unknown", "''" }) void from_invalid(String value) { // when & then assertThatThrownBy(() -> EntityType.from(value)) .isInstanceOf(IllegalArgumentException.class); } + +@DisplayName("null 입력 시 예외가 발생한다") +@Test +void from_null() { + // when & then + assertThatThrownBy(() -> EntityType.from(null)) + .isInstanceOf(IllegalArgumentException.class); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/souzip/domain/file/EntityTypeTest.java` around lines 27 - 40, Add a null-input test for EntityType.from to ensure it throws the same IllegalArgumentException as other invalid inputs: either extend the existing parameterized test by adding `@NullSource` to the from_invalid test or add a new test method (e.g., from_null_throws) that calls EntityType.from(null) and asserts isInstanceOf(IllegalArgumentException.class); reference EntityType.from and the test class EntityTypeTest (method from_invalid) when making the change.src/main/resources/db/migration/V13__create_notice_table.sql (1)
5-5:author_id필드에 외래 키(FK) 제약이 없어 데이터 무결성 위험이 있습니다.현재 설계에서는 Notice 엔티티가
author_id를 단순 UUID 값으로만 저장하고 ORM 관계를 정의하지 않고 있습니다. 이는admin테이블(UUID PK)이나user테이블(UUID PK)과의 참조 무결성이 데이터베이스 수준에서 보장되지 않아, 작성자가 삭제되어도 고아 공지 데이터가 남을 수 있습니다.만약 작성자 참조 무결성이 필요하다면, 마이그레이션에 FK 제약을 추가하거나 Notice 엔티티에
@ManyToOne관계를 정의하는 것을 권장합니다. 아니면 현재의 단순 참조 설계가 의도된 것이라면 설계 결정을 문서화하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V13__create_notice_table.sql` at line 5, Notice 테이블의 author_id 컬럼에 외래키 제약이 없어 참조 무결성이 보장되지 않습니다; V13__create_notice_table.sql의 author_id 컬럼에 대해 admin(id) 또는 user(id) 중 적절한 테이블을 참조하는 FOREIGN KEY 제약을 추가하거나(ON DELETE 동작도 명시), 만약 의도적으로 단순 UUID만 저장하는 설계라면 Notice 엔티티의 필드명(author_id)과 설계 의도를 문서화하거나 엔티티에 `@ManyToOne` 관계를 명시해 데이터베이스 수준 또는 ORM 수준에서 참조 관계를 명확히 해 주세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/souzip/global/security/config/SecurityConfig.java`:
- Line 54: The SecurityConfig currently allows anonymous access to
"/api/notices/**" for all HTTP methods; change the matcher to only permit
anonymous GET requests (e.g., replace the broad antMatcher for "/api/notices/**"
with an antMatcher scoped to HttpMethod.GET such as antMatchers(HttpMethod.GET,
"/api/notices/**").permitAll()) and ensure other methods on the same path are
left to require authentication/authorization (e.g., keep subsequent
.authenticated() or role-based rules) so future POST/PUT/DELETE endpoints are
not exposed; update the rule in SecurityConfig where the antMatchers for
"/api/notices/**" is defined.
- Around line 63-65: SecurityConfig currently exposes sensitive endpoints
anonymously; remove "/api/test/**" from permitAll, and change the authorization
rules so "/api/migration/apple/prepare" and "/actuator/**" require admin
authority. Specifically, in SecurityConfig (the HttpSecurity/authorizeRequests
configuration) stop permitting those patterns and instead authorize them with
.hasRole("ADMIN") or .hasAuthority("ROLE_ADMIN") for the
AntMatchers/authorizeRequests entries that reference
"/api/migration/apple/prepare" and "/actuator/**"; also remove the dead
"/api/test/**" entry entirely. Ensure AppleMigrationController
(migrationPrepService.prepareAllAppleUsers) remains callable only when the
authenticated principal has the ADMIN role.
In `@src/main/resources/db/migration/V13__create_notice_table.sql`:
- Line 6: The status column in the new notice table (defined in
V13__create_notice_table.sql) lacks a DB-level constraint allowing invalid
values; update the migration to enforce allowed values for the status column by
either adding a PostgreSQL CHECK constraint (e.g., add CHECK (status IN
('ACTIVE','INACTIVE')) to the column definition) or create and use a PostgreSQL
ENUM type (e.g., CREATE TYPE notice_status AS ENUM('ACTIVE','INACTIVE') and
change status VARCHAR(20) to status notice_status NOT NULL), and ensure any
ALTER/CREATE statements in the migration are consistent and reversible.
- Around line 7-8: Change the created_at and updated_at columns in the notice
table from TIMESTAMP to TIMESTAMPTZ to store absolute times (update the column
types in the V13 migration or add an ALTER TABLE migration to ALTER COLUMN
created_at TYPE TIMESTAMPTZ and ALTER COLUMN updated_at TYPE TIMESTAMPTZ), and
add a foreign key constraint on author_id in notice to reference user.user_id
(UUID) to prevent orphaned authors—define the FK (notice.author_id →
user.user_id) with an appropriate ON DELETE behavior (RESTRICT or SET NULL)
consistent with the domain requirements.
---
Nitpick comments:
In `@src/main/resources/db/migration/V13__create_notice_table.sql`:
- Line 5: Notice 테이블의 author_id 컬럼에 외래키 제약이 없어 참조 무결성이 보장되지 않습니다;
V13__create_notice_table.sql의 author_id 컬럼에 대해 admin(id) 또는 user(id) 중 적절한 테이블을
참조하는 FOREIGN KEY 제약을 추가하거나(ON DELETE 동작도 명시), 만약 의도적으로 단순 UUID만 저장하는 설계라면 Notice
엔티티의 필드명(author_id)과 설계 의도를 문서화하거나 엔티티에 `@ManyToOne` 관계를 명시해 데이터베이스 수준 또는 ORM 수준에서
참조 관계를 명확히 해 주세요.
In `@src/test/java/com/souzip/domain/file/EntityTypeTest.java`:
- Around line 50-59: The test all_types_defined duplicates checks: remove the
redundant assertThat(values).hasSize(2) because
assertThat(values).containsExactly(EntityType.NOTICE, EntityType.SOUVENIR)
already asserts both size and order; update the test in EntityTypeTest (method
all_types_defined) to keep only the containsExactly assertion.
- Around line 27-40: Add a null-input test for EntityType.from to ensure it
throws the same IllegalArgumentException as other invalid inputs: either extend
the existing parameterized test by adding `@NullSource` to the from_invalid test
or add a new test method (e.g., from_null_throws) that calls
EntityType.from(null) and asserts isInstanceOf(IllegalArgumentException.class);
reference EntityType.from and the test class EntityTypeTest (method
from_invalid) when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/souzip/domain/file/EntityType.javasrc/main/java/com/souzip/global/security/config/SecurityConfig.javasrc/main/resources/db/migration/V13__create_notice_table.sqlsrc/test/java/com/souzip/domain/file/EntityTypeTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/souzip/domain/file/EntityType.java
| "/api/auth/refresh", | ||
| "/docs/**", | ||
| "/api/test/**", | ||
| "/api/notices/**", |
There was a problem hiding this comment.
/api/notices/**의 익명 허용 범위가 과도합니다.
Line [54]는 모든 HTTP 메서드를 익명 허용하므로, 추후 공지 수정/삭제 API가 추가되면 즉시 무권한 노출됩니다. 조회(GET)만 명시적으로 열어두는 편이 안전합니다.
🔧 제안 수정안
- .requestMatchers(
+ .requestMatchers(
"/api/auth/login/**",
"/api/auth/refresh",
- "/api/notices/**",
"/api/location/search",
"/api/countries/**",
"/api/categories",
"/api/search/**",
"/api/souvenirs/nearby",
"/api/discovery/**",
"/api/admin/auth/login",
"/api/admin/auth/refresh",
"/api/migration/apple/prepare",
"/api/test/**",
"/actuator/**",
"/docs/**"
).permitAll()
+ .requestMatchers(HttpMethod.GET, "/api/notices/**").permitAll()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/souzip/global/security/config/SecurityConfig.java` at line
54, The SecurityConfig currently allows anonymous access to "/api/notices/**"
for all HTTP methods; change the matcher to only permit anonymous GET requests
(e.g., replace the broad antMatcher for "/api/notices/**" with an antMatcher
scoped to HttpMethod.GET such as antMatchers(HttpMethod.GET,
"/api/notices/**").permitAll()) and ensure other methods on the same path are
left to require authentication/authorization (e.g., keep subsequent
.authenticated() or role-based rules) so future POST/PUT/DELETE endpoints are
not exposed; update the rule in SecurityConfig where the antMatchers for
"/api/notices/**" is defined.
| "/api/migration/apple/prepare", | ||
| "/api/test/**", | ||
| "/actuator/**", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] Examine SecurityConfig.java file structure around lines 63-65"
if [ -f "src/main/java/com/souzip/global/security/config/SecurityConfig.java" ]; then
wc -l "src/main/java/com/souzip/global/security/config/SecurityConfig.java"
echo "---"
sed -n '50,80p' "src/main/java/com/souzip/global/security/config/SecurityConfig.java"
else
echo "SecurityConfig.java not found at expected path"
find . -name "SecurityConfig.java" -type f 2>/dev/null | head -5
fiRepository: souzip/souzip-api
Length of output: 1980
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[2] Check for endpoint implementations of /api/test, /api/migration/apple/prepare"
rg -n '/api/test|/migration/apple/prepare' --type=java src/main/java 2>/dev/null | head -30 || echo "No matches found for test/migration endpoints"
echo
echo "[3] Check for actuator endpoint mappings"
rg -n '@.*Mapping.*actuator' --type=java src/main/java 2>/dev/null || echo "No actuator mappings found"
echo
echo "[4] Check management endpoint exposure settings"
find src/main/resources -name "*.properties" -o -name "*.yml" -o -name "*.yaml" 2>/dev/null | while read f; do
if grep -l "management\|actuator" "$f" 2>/dev/null; then
echo "File: $f"
grep -n "management\|actuator" "$f" || true
fi
doneRepository: souzip/souzip-api
Length of output: 987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[5] Full SecurityConfig.java content (if not too large)"
FILE="src/main/java/com/souzip/global/security/config/SecurityConfig.java"
LINES=$(wc -l < "$FILE")
echo "Total lines: $LINES"
if [ "$LINES" -le 250 ]; then
cat -n "$FILE"
else
echo "File too large, showing sections:"
echo "=== Lines 1-50 ==="
sed -n '1,50p' "$FILE" | cat -n
echo
echo "=== Lines 50-100 (containing lines 63-65) ==="
sed -n '50,100p' "$FILE" | cat -n
echo
echo "=== Last 50 lines ==="
tail -50 "$FILE" | cat -n
fiRepository: souzip/souzip-api
Length of output: 5789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[6] Check complete management endpoint exposure configuration"
for file in src/main/resources/application*.yaml src/main/resources/application*.properties; do
if [ -f "$file" ]; then
echo "=== $file ==="
cat "$file" | grep -A20 "^management:" || echo "No management config found"
echo
fi
doneRepository: souzip/souzip-api
Length of output: 1134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[7] Search for actual controller/endpoint implementations with test or migration paths"
rg -n -C5 '@RestController|@RequestMapping|@GetMapping|@PostMapping' --type=java src/main/java | grep -B5 -A5 'test\|migration' || echo "No test/migration endpoint implementations found"
echo
echo "[8] List all controller classes"
rg -n '@RestController|@RequestMapping.*controller' --type=java src/main/java | head -20Repository: souzip/souzip-api
Length of output: 5193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[9] Check if these paths actually exist as endpoints - search for any mention in code"
echo "Searching for 'api/test' occurrences:"
rg -n 'api/test' . --type=java --type=yaml --type=properties 2>/dev/null | grep -v SecurityConfig || echo "Only found in SecurityConfig"
echo
echo "Searching for 'migration/apple' occurrences:"
rg -n 'migration/apple' . --type=java --type=yaml --type=properties 2>/dev/null || echo "Only found in SecurityConfig"Repository: souzip/souzip-api
Length of output: 288
민감한 마이그레이션/관리 엔드포인트가 익명 접근으로 노출되어 있습니다.
검증 결과, 다음 문제가 확인되었습니다:
-
/api/migration/apple/prepare(Line 63):AppleMigrationController에서 실제로 구현된 엔드포인트로,migrationPrepService.prepareAllAppleUsers()를 호출합니다. 모든 Apple 사용자에 대한 마이그레이션 작업을 수행하는 민감한 비즈니스 로직이 완전히 익명 접근 가능 상태입니다. -
/actuator/**(Line 65): 프로덕션 환경(application-prod.yaml)에서 health, info, metrics, prometheus 엔드포인트를 노출하고 있어 시스템 정보 유출 위험이 있습니다. -
/api/test/**(Line 64): 실제 구현된 엔드포인트가 없는 데드 코드이지만, 보안 설정에 포함되어 있습니다.
이 세 경로 모두 즉시 관리자 권한으로 제한되어야 합니다.
제안 수정안
.requestMatchers(
"/api/auth/login/**",
"/api/auth/refresh",
"/api/notices/**",
"/api/location/search",
"/api/countries/**",
"/api/categories",
"/api/search/**",
"/api/souvenirs/nearby",
"/api/discovery/**",
"/api/admin/auth/login",
"/api/admin/auth/refresh",
- "/api/migration/apple/prepare",
- "/api/test/**",
- "/actuator/**",
+ "/actuator/health",
+ "/actuator/info",
"/docs/**"
).permitAll()
+ .requestMatchers("/api/migration/apple/prepare", "/api/test/**", "/actuator/**").hasRole("ADMIN")
.requestMatchers(HttpMethod.GET, "/api/souvenirs/*").permitAll()
.anyRequest().authenticated()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/souzip/global/security/config/SecurityConfig.java` around
lines 63 - 65, SecurityConfig currently exposes sensitive endpoints anonymously;
remove "/api/test/**" from permitAll, and change the authorization rules so
"/api/migration/apple/prepare" and "/actuator/**" require admin authority.
Specifically, in SecurityConfig (the HttpSecurity/authorizeRequests
configuration) stop permitting those patterns and instead authorize them with
.hasRole("ADMIN") or .hasAuthority("ROLE_ADMIN") for the
AntMatchers/authorizeRequests entries that reference
"/api/migration/apple/prepare" and "/actuator/**"; also remove the dead
"/api/test/**" entry entirely. Ensure AppleMigrationController
(migrationPrepService.prepareAllAppleUsers) remains callable only when the
authenticated principal has the ADMIN role.
| title VARCHAR(200) NOT NULL, | ||
| content TEXT NOT NULL, | ||
| author_id UUID NOT NULL, | ||
| status VARCHAR(20) NOT NULL, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find NoticeStatus enum and check migration constraints
fd 'NoticeStatus.java' src/main/java -x cat {} | head -50Repository: souzip/souzip-api
Length of output: 149
🏁 Script executed:
# Check the complete migration file for status column definition and constraints
cat src/main/resources/db/migration/V13__create_notice_table.sqlRepository: souzip/souzip-api
Length of output: 546
🏁 Script executed:
# Search for any CHECK or ENUM constraints related to status
rg -n "status|CHECK|CREATE TYPE" src/main/resources/db/migration/V13__create_notice_table.sqlRepository: souzip/souzip-api
Length of output: 148
status 컬럼에 데이터베이스 무결성 제약이 필요합니다.
현재 status VARCHAR(20) NOT NULL은 제약이 없어 애플리케이션 버그나 직접 SQL을 통해 enum 외 값(ACTIVE, INACTIVE 이외)이 저장될 수 있습니다. 다음 중 하나로 DB 레벨에서 제약을 추가하세요:
- PostgreSQL CHECK 제약:
CHECK (status IN ('ACTIVE', 'INACTIVE')) - 또는 PostgreSQL ENUM 타입 사용:
CREATE TYPE notice_status AS ENUM ('ACTIVE', 'INACTIVE');
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/V13__create_notice_table.sql` at line 6, The
status column in the new notice table (defined in V13__create_notice_table.sql)
lacks a DB-level constraint allowing invalid values; update the migration to
enforce allowed values for the status column by either adding a PostgreSQL CHECK
constraint (e.g., add CHECK (status IN ('ACTIVE','INACTIVE')) to the column
definition) or create and use a PostgreSQL ENUM type (e.g., CREATE TYPE
notice_status AS ENUM('ACTIVE','INACTIVE') and change status VARCHAR(20) to
status notice_status NOT NULL), and ensure any ALTER/CREATE statements in the
migration are consistent and reversible.
| created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 프로젝트 구조 파악 및 Notice 관련 파일 탐색
find . -type f -name "*.java" -o -name "*.kt" | head -20Repository: souzip/souzip-api
Length of output: 1502
🏁 Script executed:
# 원본 comment에 포함된 명령어 실행
rg -n "createdAt|updatedAt|LocalDateTime|OffsetDateTime|Instant" src/main/java/com/souzip/domain/notice
rg -n "hibernate\.jdbc\.time_zone|spring\.jackson\.time-zone|timezone|time-zone" src/main/resourcesRepository: souzip/souzip-api
Length of output: 43
🏁 Script executed:
# 마이그레이션 파일 전체 내용 확인
cat src/main/resources/db/migration/V13__create_notice_table.sqlRepository: souzip/souzip-api
Length of output: 546
🏁 Script executed:
# NoticeStatus enum 확인
fd -e java -e kt -path "*/notice/*" | xargs grep -l "NoticeStatus" 2>/dev/nullRepository: souzip/souzip-api
Length of output: 231
🏁 Script executed:
# author_id 관련 외래 키 및 참조 대상 확인
rg -n "author|Author|userId|UserId" src/main/resources/db/migration/Repository: souzip/souzip-api
Length of output: 272
🏁 Script executed:
# Notice 엔티티 전체 검색 (경로 제한 없이)
fd Notice . --type f | grep -E "\.(java|kt)$"Repository: souzip/souzip-api
Length of output: 1362
🏁 Script executed:
# createdAt, updatedAt 필드 검색 (broader search)
rg -i "createdAt|updatedAt" src/main/java --type java -A 3Repository: souzip/souzip-api
Length of output: 14152
🏁 Script executed:
# application 설정 파일 찾기
fd "application\.(yml|yaml|properties)" src/main/resourcesRepository: souzip/souzip-api
Length of output: 96
🏁 Script executed:
# 타임존 관련 설정 전체 검색
rg -i "timezone|time-zone|hibernate.*time|jackson.*time" src/main/resourcesRepository: souzip/souzip-api
Length of output: 43
🏁 Script executed:
# NoticeStatus enum 검색
rg "enum NoticeStatus|NoticeStatus.*{" src/main/java --type java -A 10Repository: souzip/souzip-api
Length of output: 190
🏁 Script executed:
# 모든 마이그레이션 파일 확인 (user/account 테이블 구조 이해)
ls -la src/main/resources/db/migration/ | head -20Repository: souzip/souzip-api
Length of output: 1245
🏁 Script executed:
# Notice.java 파일 확인
cat -n src/main/java/com/souzip/domain/notice/Notice.javaRepository: souzip/souzip-api
Length of output: 1824
🏁 Script executed:
# NoticeStatus.java 파일 확인
cat -n src/main/java/com/souzip/domain/notice/NoticeStatus.javaRepository: souzip/souzip-api
Length of output: 191
🏁 Script executed:
# application.yaml 파일 확인
cat -n src/main/resources/application.yamlRepository: souzip/souzip-api
Length of output: 2711
🏁 Script executed:
# V1 마이그레이션에서 user/account 테이블 구조 확인
head -100 src/main/resources/db/migration/V1__init_schema.sql | grep -A 20 "CREATE TABLE.*user\|CREATE TABLE.*account"Repository: souzip/souzip-api
Length of output: 43
🏁 Script executed:
# BaseEntity 파일도 확인 (Notice가 상속하는 부모 클래스)
cat -n src/main/java/com/souzip/domain/shared/BaseEntity.javaRepository: souzip/souzip-api
Length of output: 1135
🏁 Script executed:
# V1 마이그레이션 전체 내용 (user/account 테이블 정의 부분)
grep -A 15 "CREATE TABLE.*user\|CREATE TABLE.*account" src/main/resources/db/migration/V1__init_schema.sql | head -40Repository: souzip/souzip-api
Length of output: 1239
🏁 Script executed:
# V1 마이그레이션 파일의 user 테이블 부분만 추출
sed -n '/CREATE TABLE.*user/,/);/p' src/main/resources/db/migration/V1__init_schema.sqlRepository: souzip/souzip-api
Length of output: 1168
TIMESTAMP 대신 TIMESTAMPTZ 사용을 권장합니다.
Line 7, Line 8의 타임존 정보 없는 TIMESTAMP는 hibernate.jdbc.time_zone 설정으로 변환이 처리되고 있으나, 운영 환경 변경이나 타임존 설정 수정 시 예기치 않은 오류가 발생할 수 있습니다. TIMESTAMPTZ로 변경하면 절대 시간을 저장하여 이식성과 안정성이 향상됩니다.
또한 author_id에 외래 키 제약을 추가하세요. user 테이블의 user_id (UUID)를 참조하도록 설정하지 않으면 고아 데이터(존재하지 않는 사용자를 참조)가 발생할 수 있습니다.
제안 변경안
created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
+ author_id UUID NOT NULL REFERENCES "user"(user_id),-- TIMESTAMP 변경
ALTER TABLE notice ALTER COLUMN created_at TYPE TIMESTAMPTZ;
ALTER TABLE notice ALTER COLUMN updated_at TYPE TIMESTAMPTZ;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/V13__create_notice_table.sql` around lines 7
- 8, Change the created_at and updated_at columns in the notice table from
TIMESTAMP to TIMESTAMPTZ to store absolute times (update the column types in the
V13 migration or add an ALTER TABLE migration to ALTER COLUMN created_at TYPE
TIMESTAMPTZ and ALTER COLUMN updated_at TYPE TIMESTAMPTZ), and add a foreign key
constraint on author_id in notice to reference user.user_id (UUID) to prevent
orphaned authors—define the FK (notice.author_id → user.user_id) with an
appropriate ON DELETE behavior (RESTRICT or SET NULL) consistent with the domain
requirements.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4efe8cb08
ℹ️ 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".
| return; | ||
| } | ||
|
|
||
| deleteFileIds.forEach(fileModifier::delete); |
There was a problem hiding this comment.
공지 수정 시 deleteFileIds를 그대로 fileModifier.delete에 전달해 파일의 entityType/entityId가 현재 noticeId와 일치하는지 검증하지 않습니다. 이 상태에서는 요청 본문에 다른 공지나 Souvenir의 파일 ID가 섞여 들어오면 해당 파일과 스토리지 객체가 실제로 삭제되어 데이터 무결성이 깨질 수 있으므로, 삭제 전에 공지 소속 파일인지 확인하거나 EntityType.NOTICE + noticeId 범위로만 삭제하는 메서드로 제한해야 합니다.
Useful? React with 👍 / 👎.
| @Override | ||
| public List<NoticeResponse> findAllActiveWithFiles() { | ||
| return findAllActive().stream() | ||
| .map(this::toResponseWithFiles) |
There was a problem hiding this comment.
findAllActiveWithFiles/findAllWithFiles는 공지 목록을 순회하며 toResponseWithFiles를 호출하고, 각 호출마다 fileFinder.findFileResponsesByEntity가 실행됩니다. 공지가 N건이면 파일 조회가 N번 추가되는 1+N query 패턴이 되어 공지 수가 늘어날수록 응답 지연이 커지므로, 공지 ID 집합 기준으로 파일을 한 번에 조회해 메모리에서 매핑하도록 바꾸는 것이 필요합니다.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1d5178368
ℹ️ 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".
|
|
||
| @GetMapping("/{noticeId}") | ||
| public SuccessResponse<NoticeResponse> getById(@PathVariable Long noticeId) { | ||
| NoticeResponse response = noticeFinder.findByIdWithFiles(noticeId); |
There was a problem hiding this comment.
/api/notices/{noticeId}는 인증 없이 호출되는데, 여기서 noticeFinder.findByIdWithFiles를 그대로 사용해 상태 검증 없이 ID로 조회합니다. 그 결과 목록에서는 숨겨진 INACTIVE 공지도 ID만 알면 상세 API로 그대로 노출되어 비공개/임시 공지 은닉 정책이 깨집니다(특히 ID가 순번이라 추측이 쉬운 환경에서 재현됩니다). 사용자 API 경로에서는 ACTIVE 상태만 허용하도록 별도 조회 로직(예: status 필터 또는 isActive() 검증)을 분리해야 합니다.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private Notice findNoticeById(Long noticeId) { | ||
| return noticeFinder.findById(noticeId); | ||
| } |
There was a problem hiding this comment.
The update() method calls noticeFinder.findById(noticeId) directly on line 45, while activate(), deactivate(), and delete() use the private findNoticeById() helper. Both calls are equivalent since findNoticeById() is just a one-line wrapper. This inconsistency makes it unclear which approach is intended. Either the update() method should also use findNoticeById(), or the private helper should be removed and all methods should call noticeFinder.findById() directly.
| assertThat(values).hasSize(2); | ||
| assertThat(values).containsExactly(EntityType.NOTICE, EntityType.SOUVENIR); |
There was a problem hiding this comment.
The test asserts exactly 2 EntityType values exist, hardcoding the expected count. This test will fail automatically whenever a new EntityType constant is added to the enum, without the test author necessarily having intended to restrict the count. This creates unnecessary maintenance burden.
Consider using containsExactlyInAnyOrder without asserting the exact size, or removing this test, since the values() check and containsExactly already fail if the enum has unexpected values.
| @GetMapping("/{noticeId}") | ||
| public SuccessResponse<NoticeResponse> getById(@PathVariable Long noticeId) { | ||
| NoticeResponse response = noticeFinder.findByIdWithFiles(noticeId); | ||
|
|
||
| return SuccessResponse.of(response); | ||
| } |
There was a problem hiding this comment.
The public /api/notices/{noticeId} endpoint calls findByIdWithFiles() which retrieves any notice regardless of its status (ACTIVE or INACTIVE). This means unauthenticated users can retrieve INACTIVE notices if they know the ID, bypassing the intent of the status field.
The user-facing endpoint should validate that the retrieved notice is active, or use a dedicated service method that filters by active status before returning the result.
| private NoticeResponse toResponseWithFiles(Notice notice) { | ||
| List<FileResponse> files = fileFinder.findFileResponsesByEntity(EntityType.NOTICE, notice.getId()); | ||
|
|
||
| return NoticeResponse.from(notice, files); | ||
| } |
There was a problem hiding this comment.
Both findAllActiveWithFiles() and findAllWithFiles() issue one file-query per notice (N+1 pattern): for N notices, there will be N+1 DB queries (1 to fetch notices + N to fetch files for each notice). As the number of notices grows, this will cause performance degradation.
Consider adding a batch-fetch method to FileRepository/FileFinder (e.g., findFileResponsesByEntityIds) that loads all files for a given list of entity IDs in a single query, then groups them by entityId in memory — similar to how findThumbnailsByEntityIds works.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97082b45f3
ℹ️ 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".
| "/api/discovery/**", | ||
| "/actuator/**", | ||
| "/api/migration/apple/prepare" | ||
| "/api/admin/auth/login", | ||
| "/api/admin/auth/refresh", | ||
| "/docs/**" | ||
| ).permitAll() |
There was a problem hiding this comment.
여기서 /actuator/**가 화이트리스트에서 빠지면서 actuator endpoint가 전부 anyRequest().authenticated()로 보호됩니다. 그런데 배포/모니터링 스크립트는 인증 없이 curl -f http://localhost:8080/actuator/health를 호출하고 있어(deploy/dev/scripts/deploy.sh 103행, deploy/dev/scripts/rollback.sh 56행, deploy/shared/system-monitor.sh 33행), 401이 나면 배포가 실패·롤백되거나 상태 알림이 오탐으로 전환됩니다. 헬스체크 경로는 기존처럼 permitAll로 유지하거나 스크립트 인증 처리를 함께 반영해야 합니다.
Useful? React with 👍 / 👎.
| deleteFileIds.stream() | ||
| .filter(validFileIds::contains) | ||
| .forEach(fileModifier::delete); |
There was a problem hiding this comment.
이 로직은 요청의 deleteFileIds를 그대로 순회해 삭제하므로 동일한 파일 ID가 중복으로 들어오면 첫 삭제 이후 두 번째 호출에서 fileModifier.delete가 FileNotFoundException을 던져 공지 수정 전체가 실패할 수 있습니다(예: [10,10]). 실제 삭제 전에 distinct()/Set으로 중복을 제거해 idempotent하게 처리하는 것이 안전합니다.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
새로운 기능
문서화
데이터/마이그레이션
보안/설정
테스트