[Feat/#216] 공지 수정 시 이미지 최대 20장 첨부 기능 추가#217
Merged
Conversation
- 한 장당 5MB, 한 번에 요청 가능한 최대 전송량 105MB
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough공지 이미지 처리를 단일 이미지에서 다중 이미지로 전환하고, NoticeImage 엔티티·저장소를 추가하며 컨트롤러·서비스·DTO를 멀티파트(다중 이미지) 흐름으로 변경했습니다. NoticeUpdateService는 제거되고 관련 로직은 NoticeService로 통합되었습니다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant NoticeCreateController
participant NoticeService
participant S3Service
participant NoticeImageRepository
participant Database
Client->>NoticeCreateController: POST /festivals/{id}/notices (multipart: noticeCreateRequest + images)
NoticeCreateController->>NoticeService: createNotice(festivalId, noticeCreateRequest, images)
NoticeService->>NoticeService: 이미지 개수 검증 (<=20)
alt images present
NoticeService->>S3Service: 병렬 업로드 요청 (각 이미지)
par 병렬 업로드
S3Service-->>NoticeService: 각 이미지 업로드 결과(URL)
and
Note over NoticeService: 업로드 실패 시 롤백(업로드된 키 삭제)
end
NoticeService->>Database: Notice 저장
NoticeService->>NoticeImageRepository: NoticeImage 엔티티 일괄 저장(순서 포함)
NoticeImageRepository-->>Database: 이미지 레코드 저장
else no images
NoticeService->>Database: Notice 저장 (이미지 없음)
end
NoticeService-->>NoticeCreateController: NoticeCreateResponse
NoticeCreateController-->>Client: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
Author
|
@CodeRabbit 이전 pr에 있는 내용 제외하고 이번 브랜치 내용에 대해서만 코드 리뷰해줘 |
|
이전 PR 내용은 제외하고, 이번 브랜치에서 새로 추가된 변경 사항(공지 수정 시 이미지 최대 20장 첨부 기능, NoticeImage 테이블 분리, S3 병렬 업로드, 소프트 삭제 시 S3 이미지 정리 등)에 대해서만 리뷰해주세요. ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 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/amp/domain/congestion/repository/StageCongestionRepository.java`:
- Line 30: The derived method deleteByStageIdIn(List<Long> stageIds) in
StageCongestionRepository does not guarantee a bulk DELETE; replace it with an
explicit bulk JPQL delete: add `@Modifying` and `@Query`("delete from
StageCongestion sc where sc.stageId in :stageIds") on a method signature like
void deleteByStageIdIn(`@Param`("stageIds") List<Long> stageIds), and ensure the
calling context/open transaction has `@Transactional` (or annotate the repository
method with `@Transactional` if appropriate) so the operation executes as a single
bulk delete rather than loading entities for individual deletes.
In
`@src/main/java/com/amp/domain/festival/controller/organizer/FestivalController.java`:
- Around line 55-58: The PATCH endpoint updateFestival currently restricts
consumes to MediaType.MULTIPART_FORM_DATA_VALUE which breaks clients still
sending application/json; add a second handler for the same path that accepts
application/json (e.g., another method annotated `@PatchMapping`(value =
"/{festivalId}", consumes = MediaType.APPLICATION_JSON_VALUE) that takes
`@PathVariable` Long festivalId and `@RequestBody` `@Valid` FestivalUpdateRequest and
returns the same ResponseEntity<BaseResponse<FestivalUpdateResponse>>), or
remove the consumes attribute to accept both; ensure both handlers delegate to
the same service logic (or refactor shared logic into a private method) so JSON
and multipart clients are both supported during rollout.
In
`@src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java`:
- Around line 189-202: The code deletes the old S3 key before the transaction
commits (in FestivalService where uploadImage, s3Service.delete and
festival.updateMainImage are used), risking a dangling/broken URL on failure;
change it to register a TransactionSynchronization via
TransactionSynchronizationManager.registerSynchronization(...) and perform
s3Service.delete(oldKey) in the afterCommit callback instead of immediately,
keeping the current behavior of deleting newKey on exceptions during upload and
still calling festival.updateMainImage(s3Service.getPublicUrl(newKey)) inside
the transaction so DB changes and S3 deletion occur only after a successful
commit (follow the same pattern used in CongestionProcessor).
- Around line 70-89: The parseJson flow currently only deserializes JSON into
objects (parseJson -> ScheduleRequest/StageRequest/activeCategoryIds) so
field-level constraints like `@NotNull` on
ScheduleRequest.festivalDate/festivalTime are not being validated, causing NPEs
later (see Comparator.comparing(ScheduleRequest::getFestivalDate)). After each
parseJson call for schedules/stages/activeCategoryIds, run Jakarta/Spring bean
validation against each element (e.g., Validator.validate(schedule)) and collect
ConstraintViolations; if any violations exist, map them to the appropriate
domain error (FestivalErrorCode.INVALID_SCHEDULE_FORMAT or similar) and throw a
domain validation exception instead of letting a 500 occur; update the same
pattern at the other locations noted (lines ~162-181, ~281-290) and remove
reliance on checkNullField as the sole validation.
In `@src/main/java/com/amp/domain/notice/dto/request/NoticeUpdateRequest.java`:
- Line 14: The current behavior deletes all images when
NoticeUpdateRequest.keepImageUrls is null because NoticeService.syncImages
treats null as an empty list; to prevent accidental deletion, either (A) make
keepImageUrls explicitly required by adding a validation annotation (e.g., add
`@NotNull` on the keepImageUrls field in NoticeUpdateRequest and ensure request
validation runs) or (B) change NoticeService.syncImages to treat a null
keepImageUrls as “no change” (i.e., if keepImageUrls == null, skip deletion and
preserve existing image URLs) — pick one approach and implement it consistently
so that keepImageUrls null no longer unintentionally removes all images,
referencing the keepImageUrls field and the syncImages method when making the
change.
In `@src/main/java/com/amp/domain/notice/dto/response/NoticeDetailResponse.java`:
- Line 14: The response DTO NoticeDetailResponse changed the field name/type
from imageUrl (single String) to imageUrls (List<String>), which breaks existing
clients; restore compatibility by exposing the old field name alongside the new
one (or add a versioned DTO) so both imageUrls (List<String>) and imageUrl
(String) are populated: update the NoticeDetailResponse class to include a
backwards-compatible imageUrl getter that returns the first element of imageUrls
(or null if empty), or create a v2 DTO and keep the original DTO unchanged, and
ensure serialization annotations (e.g., JSON property names) map imageUrl to the
old name while imageUrls remains for the new format.
In `@src/main/java/com/amp/domain/notice/entity/Notice.java`:
- Around line 48-50: The images field in the Notice entity is package-private;
change its access modifier to private to match other fields and encapsulation
standards: locate the List<NoticeImage> images declaration in the Notice class
(annotated with `@OneToMany`, `@BatchSize`) and make it private, keeping the
existing initialization, annotations, and behavior intact.
In `@src/main/java/com/amp/domain/notice/entity/NoticeImage.java`:
- Around line 19-21: The NoticeImage entity's notice association should be
non-nullable at the DB level; update the mapping on the NoticeImage class by
adding nullable = false to the `@JoinColumn` for the notice field (and optionally
set optional = false on the `@ManyToOne`) so the generated schema enforces NOT
NULL for the notice relationship.
In
`@src/main/java/com/amp/domain/notice/service/common/FestivalNoticeService.java`:
- Around line 58-61: Extract the repeated sorting-and-mapping logic into a
single reusable method (e.g., a static helper like
NoticeImageUtils.getSortedImageUrls(List<NoticeImage>) or an instance method
like Notice.getSortedImageUrls()) that takes List<NoticeImage> and returns a
List<String> of imageUrls using
Comparator.comparingInt(NoticeImage::getImageOrder) and
.map(NoticeImage::getImageUrl). Replace the inline chains in
FestivalNoticeService (the shown code), NoticeService.getNoticeDetail, and
SavedNoticesResponse.SavedAnnouncementDto.from() to call that new method so all
callers share the same ordering logic.
In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java`:
- Around line 297-327: Before deleting images, validate that each URL in
keepImageUrls actually belongs to the current notice: compute the set of keys
from imageMap (built from notice.getImages()) and if keepUrls contains any URL
not in that set, throw a 4xx NoticeException (e.g., new
NoticeException(NoticeErrorCode.INVALID_KEEP_IMAGE_URL) or add a suitable error
code) so unknown URLs are rejected instead of silently ignored; apply this check
in NoticeService right after building imageMap and before computing
imagesToDelete/keptImages.
- Around line 229-237: The current soft-delete flow calls notice.delete() which
only sets deletedAt, leaving NoticeImage rows in DB and relying on
cascade/orphanRemoval which won't run for soft deletes; instead, explicitly
remove child records and handle S3 failures: inside the same transaction where
you call notice.delete(), iterate notice.getImages(), attempt
s3Service.delete(s3Service.extractKey(image.getImageUrl())) and on success
remove the image from the collection (e.g., notice.getImages().remove(image) or
collect and call notice.getImages().clear()/removeAll(...)), and on S3 deletion
failure either throw or record the failure to retry (do not swallow exceptions
with only log.warn) so the transaction can roll back or a retry mechanism can be
scheduled; update logic around Notice.delete(), notice.getImages().clear(), and
S3 handling to ensure DB child rows (NoticeImage) are removed in the same
transaction when S3 deletion succeeds, and implement retry/reporting when S3
deletion fails.
- Around line 311-339: The code currently deletes S3 objects (imagesToDelete via
s3Service.delete) before uploading new images and persisting DB changes; move
the S3 deletion logic to after uploadImagesInParallel completes and after the DB
update/save succeeds so S3 removals only occur on full success. Concretely,
preserve imagesToDelete and their S3 keys, perform
uploadImagesInParallel(validNewImages, ...) and complete the DB updates that use
the returned keys (and update currentImages/keptImages/order), then once uploads
and DB save have succeeded, call s3Service.delete on the preserved keys and
finally remove images from currentImages; keep existing try/catch/log.warn
behavior around s3Service.delete.
- Around line 124-141: The createNotice and updateNotice flows in NoticeService
currently upload images via uploadImagesInParallel and/or syncImages inside a
`@Transactional` method without compensating cleanup on DB rollback, risking
orphaned S3 objects; change the flow to (1) perform image uploads but do not
immediately delete existing S3 images, (2) register a TransactionSynchronization
(e.g., TransactionSynchronizationManager.registerSynchronization or use
afterCommit/afterCompletion) that on afterCommit deletes the old images and on
afterCompletion(STATUS_ROLLED_BACK) deletes any newly uploaded images to clean
up, and (3) update syncImages to upload new images first and only schedule
deletion of old images after successful commit (or delete old images in
afterCommit) while referencing the methods uploadImagesInParallel, syncImages,
createNotice, updateNotice, and NoticeService to implement this transactional
cleanup.
In `@src/main/java/com/amp/global/s3/S3Service.java`:
- Around line 85-92: The extractKey method currently blindly does
publicUrl.replace(baseUrl, "") which can return the original string if publicUrl
doesn't start with the expected base URL; update extractKey to validate that
publicUrl.startsWith(baseUrl) (built from s3Properties.getBaseUrl(),
getBucket(), getRegion()) and handle the invalid case explicitly—either throw a
clear IllegalArgumentException or log an error via the class logger and return
null/empty—so callers (e.g., delete flows) won't attempt S3 operations with an
incorrect key; ensure the chosen behavior is documented in the method javadoc
and used consistently by callers.
In `@src/main/resources/application.yml`:
- Line 5: The YAML top-level property key "Jackson" should be renamed to the
lowercase "jackson" to follow Spring Boot's recommended kebab/lowercase
convention and ensure consistent relaxed binding; update the key in
application.yml (replace the "Jackson:" entry with "jackson:") so the
configuration follows the convention used by other properties.
In
`@src/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.java`:
- Around line 29-37: Add a positive unit test in
FestivalUpdateRequestValidationTest that asserts a FestivalUpdateRequest with
mainImage == null is considered valid; use the existing helper requestWith(...)
(which currently passes null for mainImage) to construct the request and assert
validation passes, ensuring the model allows "no new image" and keeps the
existing mainImageUrl rather than removing it or requiring a remove flag; this
guards against regressions like adding `@NotNull` to
FestivalUpdateRequest.mainImage and enforces the rule that updates without a new
image must preserve the current mainImageUrl.
In `@src/test/java/com/amp/domain/notice/service/NoticeServiceTest.java`:
- Around line 397-414: The test deleteNoticeS3DeleteFailStillSoftDeleted
currently forces s3Service.delete to throw and then asserts
notice.getDeletedAt() is set, which contradicts the PR intent; change the test
in NoticeServiceTest.deleteNoticeS3DeleteFailStillSoftDeleted to instead assert
the correct failure-handling behaviour of noticeService.deleteNotice: either (A)
expect an exception from noticeService.deleteNotice and verify
notice.getDeletedAt() remains null when s3Service.delete throws, or (B) verify
that a retry/recovery action is triggered by mocking and verifying the
retry/event publisher (e.g., an eventPublisher.publish... or
retryService.enqueue...) is called when s3Service.delete throws; update the
when/doThrow setup for s3Service.delete and adjust assertions/mocks accordingly
to reflect the chosen correct behaviour.
- Around line 576-592: The test updateNoticeImageTotalExceeds20ThrowException is
incorrect because setUp() leaves notice.images empty so the keepUrls are not
actually counted; modify the test to pre-populate the Notice returned by the
mock (the notice object used in your stubs created in setUp()) with two Image
entries whose URLs match the keepUrls before calling
noticeService.updateNotice(1L, request, newImages) so that existing kept images
+ newImages sum to 21 and trigger NoticeException; update the stubbing that
returns the notice (the mock used by your repository/service in setUp()) so it
includes those Image objects referenced by the NoticeUpdateRequest.keepUrls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ccd0467a-7856-47c8-b2da-5692e530ce19
📒 Files selected for processing (29)
.coderabbit.yamlsrc/main/java/com/amp/domain/audience/dto/response/SavedNoticesResponse.javasrc/main/java/com/amp/domain/congestion/repository/StageCongestionRepository.javasrc/main/java/com/amp/domain/congestion/service/StageService.javasrc/main/java/com/amp/domain/festival/controller/organizer/FestivalController.javasrc/main/java/com/amp/domain/festival/dto/request/FestivalUpdateRequest.javasrc/main/java/com/amp/domain/festival/entity/Festival.javasrc/main/java/com/amp/domain/festival/service/organizer/FestivalService.javasrc/main/java/com/amp/domain/notice/controller/organizer/NoticeCreateController.javasrc/main/java/com/amp/domain/notice/controller/organizer/NoticeUpdateController.javasrc/main/java/com/amp/domain/notice/dto/request/NoticeCreateRequest.javasrc/main/java/com/amp/domain/notice/dto/request/NoticeUpdateRequest.javasrc/main/java/com/amp/domain/notice/dto/response/FestivalNoticeListResponse.javasrc/main/java/com/amp/domain/notice/dto/response/NoticeDetailResponse.javasrc/main/java/com/amp/domain/notice/entity/Notice.javasrc/main/java/com/amp/domain/notice/entity/NoticeImage.javasrc/main/java/com/amp/domain/notice/exception/NoticeErrorCode.javasrc/main/java/com/amp/domain/notice/repository/NoticeImageRepository.javasrc/main/java/com/amp/domain/notice/repository/NoticeRepository.javasrc/main/java/com/amp/domain/notice/service/common/FestivalNoticeService.javasrc/main/java/com/amp/domain/notice/service/organizer/NoticeService.javasrc/main/java/com/amp/domain/notice/service/organizer/NoticeUpdateService.javasrc/main/java/com/amp/global/config/MultipartJackson2HttpMessageConverter.javasrc/main/java/com/amp/global/s3/S3Service.javasrc/main/resources/application-prod.ymlsrc/main/resources/application.ymlsrc/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.javasrc/test/java/com/amp/domain/notice/service/NoticeServiceTest.javasrc/test/java/com/amp/domain/notice/service/NoticeUpdateServiceTest.java
💤 Files with no reviewable changes (5)
- src/main/java/com/amp/domain/notice/repository/NoticeRepository.java
- src/main/resources/application-prod.yml
- src/main/java/com/amp/domain/notice/service/organizer/NoticeUpdateService.java
- src/main/java/com/amp/domain/notice/dto/request/NoticeCreateRequest.java
- src/test/java/com/amp/domain/notice/service/NoticeUpdateServiceTest.java
src/main/java/com/amp/domain/congestion/repository/StageCongestionRepository.java
Show resolved
Hide resolved
src/main/java/com/amp/domain/festival/controller/organizer/FestivalController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java
Show resolved
Hide resolved
src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java
Show resolved
Hide resolved
src/main/java/com/amp/domain/notice/dto/request/NoticeUpdateRequest.java
Show resolved
Hide resolved
src/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.java
Show resolved
Hide resolved
|
리뷰 범위:
이전 PR 내용은 제외하고, 이번 브랜치(PR 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
src/main/java/com/amp/domain/congestion/repository/StageCongestionRepository.java (1)
30-30:⚠️ Potential issue | 🟠 Major파생 삭제 메서드 대신 명시적 벌크 JPQL 삭제로 바꿔주세요.
Line 30의
deleteByStageIdIn(...)는 Spring Data JPA에서 단일 벌크DELETE가 아니라, 대상 엔티티를 조회한 뒤 개별 삭제로 처리됩니다. 지금처럼StageService.syncStages()에서 제거 대상 ID 목록을 한 번에 넘기는 흐름에서는 혼잡도 이력이 많을수록 불필요한 SELECT/개별 DELETE로 메모리 사용량과 처리 시간이 커질 수 있습니다.@Modifying+ JPQLDELETE FROM StageCongestion sc WHERE sc.stage.id IN :stageIds로 명시하는 편이 안전합니다. (docs.spring.io)수정 예시
+import org.springframework.data.jpa.repository.Modifying; import org.springframework.data.jpa.repository.Query; import org.springframework.data.repository.query.Param; @@ - void deleteByStageIdIn(List<Long> stageIds); + `@Modifying`(clearAutomatically = true, flushAutomatically = true) + `@Query`(""" + DELETE FROM StageCongestion sc + WHERE sc.stage.id IN :stageIds + """) + int deleteByStageIdIn(`@Param`("stageIds") List<Long> stageIds);Spring Data JPA reference documentation: do derived delete query methods like `deleteBy...In(...)` execute a single bulk DELETE, or do they first load matching entities and delete them one by one? Please include the official docs section about "Derived Delete Queries".Based on learnings: When writing dynamic queries in repository classes, prefer JPQL or QueryDSL over native SQL. Use native queries only when JPQL/QueryDSL cannot express the needed logic, and document the reason. This improves portability, maintainability, and type-safety of data access in Java repositories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/amp/domain/congestion/repository/StageCongestionRepository.java` at line 30, Replace the derived deleteByStageIdIn(List<Long> stageIds) in StageCongestionRepository with an explicit bulk JPQL delete: add a `@Modifying` `@Query`("DELETE FROM StageCongestion sc WHERE sc.stage.id IN :stageIds") method (accepting `@Param`("stageIds") List<Long> stageIds) and ensure it is executed within a transactional context (e.g., repository method or calling StageService.syncStages annotated/within a `@Transactional` boundary) so the deletion is performed as a single bulk DELETE rather than loading entities and deleting them one-by-one.src/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.java (1)
29-37: 🧹 Nitpick | 🔵 Trivial
mainImage == null이 유효함을 검증하는 양성 테스트를 추가하세요.현재 테스트들은 필드 누락/공백 시 검증 실패만 확인합니다. 모든 필수 필드가 유효하고
mainImage가null인 경우 검증이 통과하는지 확인하는 양성 케이스를 추가하면, 향후@NotNull같은 어노테이션이 실수로 추가되는 회귀를 방지할 수 있습니다.제안 코드
+ `@Test` + `@DisplayName`("모든 필수 필드가 유효하면 검증을 통과한다") + void passesWhenAllRequiredFieldsAreValid() { + Set<ConstraintViolation<FestivalUpdateRequest>> violations = validator.validate( + requestWith("테스트 공연", "고양시 일산서구", VALID_SCHEDULES, VALID_STAGES, VALID_CATEGORY_IDS) + ); + assertThat(violations).isEmpty(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.java` around lines 29 - 37, Add a positive test that verifies FestivalUpdateRequest validation passes when all required fields are valid and mainImage is null: create a test using the existing helper requestWith(...) to construct a FestivalUpdateRequest with valid title, location, schedules, stages, activeCategoryIds and explicitly set mainImage to null (via the constructor overload used in requestWith or by constructing directly), run the validator/assert that there are no constraint violations, and name the test to indicate it ensures mainImage==null is allowed; reference FestivalUpdateRequest and the requestWith(...) helper in the test.src/main/java/com/amp/domain/festival/controller/organizer/FestivalController.java (1)
55-58:⚠️ Potential issue | 🟠 Major기존
PATCH /festivals/{festivalId}JSON 클라이언트와의 호환성이 깨집니다.
application/json에서multipart/form-data로 Content-Type이 변경되어, 기존 클라이언트가 서버보다 늦게 배포되면 즉시 415(Unsupported Media Type) 또는 400 에러를 받게 됩니다.배포 전략에 따라 다음 중 하나를 고려하세요:
- 클라이언트와 서버 동시 배포가 보장되는 경우: 현재 구현 유지
- 점진적 배포가 필요한 경우: JSON과 multipart를 병행 지원하거나 새 엔드포인트(
/v2/festivals/{festivalId}) 분리🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/amp/domain/festival/controller/organizer/FestivalController.java` around lines 55 - 58, The current FestivalController.updateFestival PATCH mapping forces multipart/form-data and will break existing JSON clients; update the controller to support backward-compatible deployment by either (A) allowing both content types on the same endpoint (change `@PatchMapping` consumes to include MediaType.APPLICATION_JSON_VALUE and MediaType.MULTIPART_FORM_DATA_VALUE and branch inside updateFestival to accept `@RequestBody` FestivalUpdateRequest when content-type is application/json and `@ModelAttribute` when multipart is sent), or (B) create a new versioned endpoint (e.g., a new method mapped to /v2/festivals/{festivalId} that accepts multipart) while keeping the existing updateFestival signature for JSON clients; modify FestivalController.updateFestival (or add updateFestivalV2) accordingly and ensure validation and response type BaseResponse<FestivalUpdateResponse> remain consistent.src/main/java/com/amp/domain/notice/service/common/FestivalNoticeService.java (1)
58-61: 🧹 Nitpick | 🔵 Trivial이미지 URL 정렬 로직 중복: 공통 메서드로 추출을 권장합니다.
동일한
sorted(Comparator.comparingInt(NoticeImage::getImageOrder)).map(NoticeImage::getImageUrl).toList()패턴이FestivalNoticeService,NoticeService.getNoticeDetail,SavedNoticesResponse.SavedAnnouncementDto.from()에서 반복됩니다.정렬 규칙이 변경될 때 한 곳이라도 누락되면 응답 순서가 달라질 수 있으므로, 유지보수를 위해 공통 유틸리티로 추출하는 것이 좋습니다.
제안: Notice 엔티티에 헬퍼 메서드 추가
// Notice.java에 추가 public List<String> getSortedImageUrls() { return images.stream() .sorted(Comparator.comparingInt(NoticeImage::getImageOrder)) .map(NoticeImage::getImageUrl) .toList(); }사용 예시:
- List<String> imageUrls = notice.getImages().stream() - .sorted(Comparator.comparingInt(NoticeImage::getImageOrder)) - .map(NoticeImage::getImageUrl) - .toList(); + List<String> imageUrls = notice.getSortedImageUrls();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/amp/domain/notice/service/common/FestivalNoticeService.java` around lines 58 - 61, The image URL sorting logic is duplicated; extract it into a single helper on the Notice entity (e.g., add a method getSortedImageUrls() to Notice that returns images.stream().sorted(Comparator.comparingInt(NoticeImage::getImageOrder)).map(NoticeImage::getImageUrl).toList()), then replace the inline stream usages in FestivalNoticeService, NoticeService.getNoticeDetail, and SavedNoticesResponse.SavedAnnouncementDto.from() to call Notice.getSortedImageUrls(); ensure the new method is public and used everywhere the old pattern appeared to centralize the sort rule.src/test/java/com/amp/domain/notice/service/NoticeServiceTest.java (1)
575-592: 🧹 Nitpick | 🔵 Trivial테스트 fixture가 20장 초과 검증을 제대로 수행하지 못합니다.
syncImages()로직을 보면keepUrls.size() + validNewImages.size() > 20검사가 먼저 수행됩니다. 현재 테스트에서keepUrls2개 +newImages19개 = 21개이므로 이 검사에서 예외가 발생할 것으로 보입니다.그러나 이 테스트의 의도가 "기존 이미지 유지 + 신규 이미지가 20장 초과"를 검증하는 것이라면,
notice.images에 실제로 이미지를 세팅해야 합니다. 현재setUp()에서notice.images는 빈 리스트이므로,keepUrls에 해당하는 이미지가 실제로 존재하지 않습니다.♻️ 테스트 fixture 수정 제안
`@Test` `@DisplayName`("공지 수정 - keep 2개 + new 19개 = 21장이면 예외 발생") void updateNoticeImageTotalExceeds20ThrowException() { // given setAuth(organizer.getEmail()); stubCommonMocks(); + // fixture: notice에 실제 이미지 2장 추가 + List<NoticeImage> existingImages = new ArrayList<>(List.of( + NoticeImage.of(notice, "https://bucket/img1.jpg", 0), + NoticeImage.of(notice, "https://bucket/img2.jpg", 1) + )); + ReflectionTestUtils.setField(notice, "images", existingImages); List<String> keepUrls = List.of("https://bucket/img1.jpg", "https://bucket/img2.jpg");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/amp/domain/notice/service/NoticeServiceTest.java` around lines 575 - 592, The test fails to trigger the intended image-limit branch because the notice's existing images list (notice.images) in setUp() is empty, so keepUrls don't match any real images; update the fixture so the domain Notice used by noticeService contains image entries whose URLs match the keepUrls before calling noticeService.updateNotice (e.g., add NoticeImage objects with URLs "https://bucket/img1.jpg" and "https://bucket/img2.jpg" to notice.images in setUp), then call updateNotice to exercise syncImages() where the check keepUrls.size() + validNewImages.size() > 20 will throw the expected NoticeException.src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java (3)
71-144:⚠️ Potential issue | 🟠 MajorDB 저장 실패 시 업로드된 S3 이미지가 고아 상태로 남을 수 있습니다.
uploadImagesInParallel()은 업로드 중 실패 시에만 이미 업로드된 이미지를 정리합니다. 그러나 업로드 성공 후noticeImageRepository.save()또는eventPublisher.publishEvent()단계에서 예외가 발생하면 트랜잭션이 롤백되지만 S3에는 이미지가 남습니다.
TransactionSynchronization.afterCompletion()을 등록하여 롤백 시 업로드된 이미지를 정리하는 것을 권장합니다.,
♻️ 롤백 시 S3 정리 추가 제안
+ private void registerRollbackCleanup(String[] keys) { + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + `@Override` + public void afterCompletion(int status) { + if (status == STATUS_ROLLED_BACK) { + Arrays.stream(keys).filter(Objects::nonNull).forEach(key -> { + try { s3Service.delete(key); } catch (Exception ignored) {} + }); + } + } + }); + } String[] keys = uploadImagesInParallel(validImages, NoticeErrorCode.NOTICE_CREATE_FAIL); + registerRollbackCleanup(keys); for (int i = 0; i < keys.length; i++) {
203-238:⚠️ Potential issue | 🟡 Minor소프트 삭제 시 NoticeImage 레코드가 DB에 남습니다.
notice.delete()는deletedAt만 설정하는 소프트 삭제입니다.cascade = CascadeType.ALL, orphanRemoval = true는 하드 삭제(entityManager.remove())에서만 작동합니다.현재 구현에서
@SQLRestriction("deleted_at IS NULL")로 인해 삭제된 Notice는 조회되지 않으므로 실질적인 문제는 적지만, NoticeImage 레코드가 DB에 계속 누적됩니다. 주기적인 정리 작업이나 명시적 삭제를 고려하세요.,
♻️ 명시적 이미지 삭제 추가 제안
List<String> imageKeys = notice.getImages().stream() .map(img -> s3Service.extractKey(img.getImageUrl())) .toList(); + // 소프트 삭제 전 NoticeImage 레코드 명시적 제거 + notice.getImages().clear(); + notice.delete(); deleteS3AfterCommit(imageKeys);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java` around lines 203 - 238, The soft-delete flow leaves NoticeImage rows in DB because Notice.delete() only sets deletedAt and JPA orphanRemoval/cascade only applies on physical removes; update deleteNotice to explicitly remove or mark associated NoticeImage entities before/when soft-deleting the Notice: fetch notice.getImages(), for each image call imageRepository.delete(image) or set image.setDeletedAt(now) (matching your soft-delete pattern) and persist those changes, ensure S3 keys are still passed to deleteS3AfterCommit, and keep existing permission checks in deleteNotice and use the existing s3Service.extractKey logic to build imageKeys; reference Notice.delete(), Notice.getImages(), deleteS3AfterCommit(imageKeys), and the NoticeImage entity or imageRepository to locate where to apply the explicit deletion/marking.
294-325: 🧹 Nitpick | 🔵 TrivialkeepImageUrls에 존재하지 않는 URL이 포함되면 조용히 무시됩니다.
Line 322-325에서
imageMap에 없는 URL은filter(imageMap::containsKey)로 걸러지지만, 클라이언트에게 피드백 없이 무시됩니다. 클라이언트가 URL을 잘못 입력하면 유지하려던 이미지가 삭제될 수 있습니다.4xx 응답으로 거부하거나, 최소한 로깅을 추가하여 디버깅을 용이하게 하는 것을 권장합니다.
,
♻️ keepImageUrls 검증 추가 제안
List<NoticeImage> currentImages = notice.getImages(); Map<String, NoticeImage> imageMap = currentImages.stream() .collect(Collectors.toMap(NoticeImage::getImageUrl, img -> img)); + // keepUrls가 현재 이미지의 부분집합인지 검증 + List<String> invalidUrls = keepUrls.stream() + .filter(url -> !imageMap.containsKey(url)) + .toList(); + if (!invalidUrls.isEmpty()) { + log.warn("keepImageUrls에 존재하지 않는 URL 포함: {}", invalidUrls); + throw new NoticeException(NoticeErrorCode.INVALID_KEEP_IMAGE_URL); + } + List<NoticeImage> imagesToDelete = currentImages.stream()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java` around lines 294 - 325, In syncImages, validate keepImageUrls against the existing imageMap and fail fast instead of silently ignoring unknown URLs: compute invalidUrls = keepUrls.stream().filter(u -> !imageMap.containsKey(u)).toList(); if invalidUrls is non-empty, throw a 4xx NoticeException (e.g., new NoticeException(NoticeErrorCode.INVALID_KEEP_IMAGE_URL) or add a suitable error code) or at minimum log the invalidUrls and return an error to the client; update syncImages (and related callers/tests) to surface this validation so clients get immediate feedback when they pass unknown image URLs rather than having images removed silently.
🤖 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/amp/domain/audience/dto/response/SavedNoticesResponse.java`:
- Around line 37-40: 현재 DTO 매핑 시점에 LazyInitializationException 위험은 없지만 쿼리의 "N+1
방지를 위한 fetch join" 의도가 완전히 실현되지 않았습니다; SavedNoticesResponse에서
bookmark.getNotice().getImages()를 사용해 이미지 URL을 수집하는 부분(Notice.images)은
`@BatchSize로` 배치 로딩되긴 하나 일관성과 성능을 위해 해당 리포지토리/쿼리에서 Notice 엔티티를 조회할 때 명시적으로 LEFT
JOIN FETCH n.images를 추가하여 이미지를 한 번에 페치하도록 변경하고, 쿼리 이름 또는 메서드(예: findBy..., 또는
repository 메서드 that returns Bookmark/Notice used by SavedNoticesResponse)에서
fetch join을 적용해 DTO 매핑 시점에 추가 쿼리가 발생하지 않도록 하세요.
In
`@src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java`:
- Around line 252-256: normalizeJsonArray currently only wraps non-bracketed
input in brackets which breaks when input is a comma-separated list like "a,b"
or contains spaces; update normalizeJsonArray to detect comma-separated values
and build a valid JSON array: if value contains ',' split on ',', trim each
item, for each item decide if it is a pure number (keep as-is) or treat it as a
string (wrap in quotes and escape inner quotes), then join items with commas and
wrap with '[' and ']'; preserve current behavior for null and already-bracketed
inputs and reference the normalizeJsonArray method when implementing this
change.
In
`@src/main/java/com/amp/domain/notice/dto/response/FestivalNoticeListResponse.java`:
- Line 10: Breaking change: the DTO field changed from a single String imageUrl
to List<String> imageUrls in FestivalNoticeListResponse; update the DTO
consumer/producer contract, API docs, and notify clients. Modify
FestivalNoticeListResponse to use List<String> imageUrls (ensure JSON mapping
via `@JsonProperty` or equivalent if needed), update any code that constructs or
reads the old imageUrl (factory methods, builders, serializers/deserializers
like methods that reference imageUrl), update Swagger/OpenAPI annotations for
the response schema and regenerate docs, and coordinate/version the API with the
client teams before deployment.
In `@src/test/java/com/amp/domain/notice/service/NoticeServiceTest.java`:
- Around line 396-414: The test shows S3 deletion is attempted in
NoticeService.deleteS3AfterCommit via
TransactionSynchronizationManager.afterCommit and failures are currently
swallowed in the catch block that logs warn("S3 이미지 삭제 실패: {}", key, e); update
the service to add a compensating/observability mechanism: when
deleteS3AfterCommit catches an exception, emit a durable alert (e.g., publish a
message to a retry queue or DLQ, increment a metric, or create a persistent
CleanupTask record) containing the key and notice id so failed deletions can be
retried or monitored; ensure the catch still prevents transaction rollback but
reliably records the failure (use NoticeService.deleteS3AfterCommit, the
afterCommit callback, and the existing catch block to add the
DLQ/metric/persistent record call).
---
Duplicate comments:
In
`@src/main/java/com/amp/domain/congestion/repository/StageCongestionRepository.java`:
- Line 30: Replace the derived deleteByStageIdIn(List<Long> stageIds) in
StageCongestionRepository with an explicit bulk JPQL delete: add a `@Modifying`
`@Query`("DELETE FROM StageCongestion sc WHERE sc.stage.id IN :stageIds") method
(accepting `@Param`("stageIds") List<Long> stageIds) and ensure it is executed
within a transactional context (e.g., repository method or calling
StageService.syncStages annotated/within a `@Transactional` boundary) so the
deletion is performed as a single bulk DELETE rather than loading entities and
deleting them one-by-one.
In
`@src/main/java/com/amp/domain/festival/controller/organizer/FestivalController.java`:
- Around line 55-58: The current FestivalController.updateFestival PATCH mapping
forces multipart/form-data and will break existing JSON clients; update the
controller to support backward-compatible deployment by either (A) allowing both
content types on the same endpoint (change `@PatchMapping` consumes to include
MediaType.APPLICATION_JSON_VALUE and MediaType.MULTIPART_FORM_DATA_VALUE and
branch inside updateFestival to accept `@RequestBody` FestivalUpdateRequest when
content-type is application/json and `@ModelAttribute` when multipart is sent), or
(B) create a new versioned endpoint (e.g., a new method mapped to
/v2/festivals/{festivalId} that accepts multipart) while keeping the existing
updateFestival signature for JSON clients; modify
FestivalController.updateFestival (or add updateFestivalV2) accordingly and
ensure validation and response type BaseResponse<FestivalUpdateResponse> remain
consistent.
In
`@src/main/java/com/amp/domain/notice/service/common/FestivalNoticeService.java`:
- Around line 58-61: The image URL sorting logic is duplicated; extract it into
a single helper on the Notice entity (e.g., add a method getSortedImageUrls() to
Notice that returns
images.stream().sorted(Comparator.comparingInt(NoticeImage::getImageOrder)).map(NoticeImage::getImageUrl).toList()),
then replace the inline stream usages in FestivalNoticeService,
NoticeService.getNoticeDetail, and
SavedNoticesResponse.SavedAnnouncementDto.from() to call
Notice.getSortedImageUrls(); ensure the new method is public and used everywhere
the old pattern appeared to centralize the sort rule.
In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java`:
- Around line 203-238: The soft-delete flow leaves NoticeImage rows in DB
because Notice.delete() only sets deletedAt and JPA orphanRemoval/cascade only
applies on physical removes; update deleteNotice to explicitly remove or mark
associated NoticeImage entities before/when soft-deleting the Notice: fetch
notice.getImages(), for each image call imageRepository.delete(image) or set
image.setDeletedAt(now) (matching your soft-delete pattern) and persist those
changes, ensure S3 keys are still passed to deleteS3AfterCommit, and keep
existing permission checks in deleteNotice and use the existing
s3Service.extractKey logic to build imageKeys; reference Notice.delete(),
Notice.getImages(), deleteS3AfterCommit(imageKeys), and the NoticeImage entity
or imageRepository to locate where to apply the explicit deletion/marking.
- Around line 294-325: In syncImages, validate keepImageUrls against the
existing imageMap and fail fast instead of silently ignoring unknown URLs:
compute invalidUrls = keepUrls.stream().filter(u ->
!imageMap.containsKey(u)).toList(); if invalidUrls is non-empty, throw a 4xx
NoticeException (e.g., new
NoticeException(NoticeErrorCode.INVALID_KEEP_IMAGE_URL) or add a suitable error
code) or at minimum log the invalidUrls and return an error to the client;
update syncImages (and related callers/tests) to surface this validation so
clients get immediate feedback when they pass unknown image URLs rather than
having images removed silently.
In
`@src/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.java`:
- Around line 29-37: Add a positive test that verifies FestivalUpdateRequest
validation passes when all required fields are valid and mainImage is null:
create a test using the existing helper requestWith(...) to construct a
FestivalUpdateRequest with valid title, location, schedules, stages,
activeCategoryIds and explicitly set mainImage to null (via the constructor
overload used in requestWith or by constructing directly), run the
validator/assert that there are no constraint violations, and name the test to
indicate it ensures mainImage==null is allowed; reference FestivalUpdateRequest
and the requestWith(...) helper in the test.
In `@src/test/java/com/amp/domain/notice/service/NoticeServiceTest.java`:
- Around line 575-592: The test fails to trigger the intended image-limit branch
because the notice's existing images list (notice.images) in setUp() is empty,
so keepUrls don't match any real images; update the fixture so the domain Notice
used by noticeService contains image entries whose URLs match the keepUrls
before calling noticeService.updateNotice (e.g., add NoticeImage objects with
URLs "https://bucket/img1.jpg" and "https://bucket/img2.jpg" to notice.images in
setUp), then call updateNotice to exercise syncImages() where the check
keepUrls.size() + validNewImages.size() > 20 will throw the expected
NoticeException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa3cfe70-b979-4269-9a28-725649558226
📒 Files selected for processing (29)
.coderabbit.yamlsrc/main/java/com/amp/domain/audience/dto/response/SavedNoticesResponse.javasrc/main/java/com/amp/domain/congestion/repository/StageCongestionRepository.javasrc/main/java/com/amp/domain/congestion/service/StageService.javasrc/main/java/com/amp/domain/festival/controller/organizer/FestivalController.javasrc/main/java/com/amp/domain/festival/dto/request/FestivalUpdateRequest.javasrc/main/java/com/amp/domain/festival/entity/Festival.javasrc/main/java/com/amp/domain/festival/service/organizer/FestivalService.javasrc/main/java/com/amp/domain/notice/controller/organizer/NoticeCreateController.javasrc/main/java/com/amp/domain/notice/controller/organizer/NoticeUpdateController.javasrc/main/java/com/amp/domain/notice/dto/request/NoticeCreateRequest.javasrc/main/java/com/amp/domain/notice/dto/request/NoticeUpdateRequest.javasrc/main/java/com/amp/domain/notice/dto/response/FestivalNoticeListResponse.javasrc/main/java/com/amp/domain/notice/dto/response/NoticeDetailResponse.javasrc/main/java/com/amp/domain/notice/entity/Notice.javasrc/main/java/com/amp/domain/notice/entity/NoticeImage.javasrc/main/java/com/amp/domain/notice/exception/NoticeErrorCode.javasrc/main/java/com/amp/domain/notice/repository/NoticeImageRepository.javasrc/main/java/com/amp/domain/notice/repository/NoticeRepository.javasrc/main/java/com/amp/domain/notice/service/common/FestivalNoticeService.javasrc/main/java/com/amp/domain/notice/service/organizer/NoticeService.javasrc/main/java/com/amp/domain/notice/service/organizer/NoticeUpdateService.javasrc/main/java/com/amp/global/config/MultipartJackson2HttpMessageConverter.javasrc/main/java/com/amp/global/s3/S3Service.javasrc/main/resources/application-prod.ymlsrc/main/resources/application.ymlsrc/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.javasrc/test/java/com/amp/domain/notice/service/NoticeServiceTest.javasrc/test/java/com/amp/domain/notice/service/NoticeUpdateServiceTest.java
💤 Files with no reviewable changes (5)
- src/test/java/com/amp/domain/notice/service/NoticeUpdateServiceTest.java
- src/main/resources/application-prod.yml
- src/main/java/com/amp/domain/notice/repository/NoticeRepository.java
- src/main/java/com/amp/domain/notice/service/organizer/NoticeUpdateService.java
- src/main/java/com/amp/domain/notice/dto/request/NoticeCreateRequest.java
src/main/java/com/amp/domain/audience/dto/response/SavedNoticesResponse.java
Show resolved
Hide resolved
src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java
Show resolved
Hide resolved
src/main/java/com/amp/domain/notice/dto/response/FestivalNoticeListResponse.java
Show resolved
Hide resolved
Comment on lines
+344
to
+366
| private String[] uploadImagesInParallel(List<MultipartFile> images, NoticeErrorCode failCode) { | ||
| String[] keys = new String[images.size()]; | ||
|
|
||
| List<CompletableFuture<Void>> futures = IntStream.range(0, images.size()) | ||
| .mapToObj(i -> CompletableFuture.runAsync( | ||
| () -> keys[i] = s3Service.upload(images.get(i), "notices") | ||
| )) | ||
| .toList(); | ||
|
|
||
| CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])) | ||
| .exceptionally(ex -> null) | ||
| .join(); | ||
|
|
||
| boolean anyFailed = futures.stream().anyMatch(CompletableFuture::isCompletedExceptionally); | ||
| if (anyFailed) { | ||
| Arrays.stream(keys).filter(Objects::nonNull).forEach(key -> { | ||
| try { s3Service.delete(key); } catch (Exception ignored) {} | ||
| }); | ||
| throw new NoticeException(failCode); | ||
| } | ||
|
|
||
| return keys; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
병렬 업로드 구현이 적절합니다.
CompletableFuture를 사용한 병렬 업로드와 실패 시 롤백 로직이 잘 구현되어 있습니다. exceptionally(ex -> null).join()으로 모든 future가 완료될 때까지 대기하고, isCompletedExceptionally()로 실패 여부를 확인합니다.
단, CompletableFuture.runAsync()는 기본적으로 ForkJoinPool.commonPool()을 사용합니다. I/O 바운드 작업인 S3 업로드에는 별도의 스레드 풀 사용을 고려할 수 있습니다.
,
dongchan0105
approved these changes
Mar 10, 2026
Contributor
dongchan0105
left a comment
dongchan0105
left a comment
There was a problem hiding this comment.
MultipartJackson2HttpMessageConverter 만들어서 해결한거 좋네요 미친 실력 뽐내고 있는 서버리드;;
src/main/java/com/amp/domain/congestion/repository/StageCongestionRepository.java
Show resolved
Hide resolved
Comment on lines
+40
to
+41
| @RequestPart("noticeCreateRequest") @Valid NoticeCreateRequest noticeCreateRequest, | ||
| @RequestPart(value = "images", required = false) List<MultipartFile> images) { |
Contributor
There was a problem hiding this comment.
여기에는 requestPart적용해서 처리했군요 앞서 삭제기능도 이렇게 처리해도 좋을거같습니다
src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java
Show resolved
Hide resolved
Comment on lines
+58
to
59
| @ModelAttribute @Valid FestivalUpdateRequest request) { | ||
| FestivalUpdateResponse response = festivalService.updateFestival(festivalId, request); |
Contributor
There was a problem hiding this comment.
Notice는 @RequestPart 사용했잖아여 그게 더 좋을거 같아요 dto를 @ModelAttribute로 받는거랑 @RequestPart로 받는거랑 흐름도 한번 찾아보세요! @RequestPart 사용하면 json바로 객체로 파싱하고 검증을 스프링으로 넘길 수 있어요
src/main/java/com/amp/domain/notice/dto/request/NoticeUpdateRequest.java
Show resolved
Hide resolved
Comment on lines
210
to
212
| if (notice.getDeletedAt() != null) { | ||
| throw new NoticeException(NoticeErrorCode.NOTICE_ALREADY_DELETED); | ||
| } |
Contributor
There was a problem hiding this comment.
이거 이미 @SQLRestriction으로 Notice 테이블 자체에서 방어하고 있어서 중복검증 안해줘도 될거에요
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/test/java/com/amp/domain/notice/service/NoticeServiceTest.java (1)
575-592:⚠️ Potential issue | 🟡 Minor테스트 픽스처 설정이 불완전합니다.
현재
setUp()에서notice.images는 빈 리스트입니다.keepUrls에 2개 URL을 지정해도 실제로 유지되는 이미지가 없어서 "기존 0장 + 신규 19장 = 19장"이 되어 예외가 발생하지 않아야 하는데, 서비스 로직에서keepUrls.size() + validNewImages.size() > 20검증으로 예외가 발생합니다.이는 의도한 동작일 수 있지만, 테스트 의도("keep 2개 + new 19개 = 21장")와 실제 검증 내용이 다릅니다.
🔧 테스트 의도에 맞는 픽스처 설정
`@Test` `@DisplayName`("공지 수정 - keep 2개 + new 19개 = 21장이면 예외 발생") void updateNoticeImageTotalExceeds20ThrowException() { // given setAuth(organizer.getEmail()); stubCommonMocks(); + // 기존 이미지 2장을 notice에 추가 + List<NoticeImage> existingImages = new ArrayList<>(List.of( + NoticeImage.of(notice, "https://bucket/img1.jpg", 0), + NoticeImage.of(notice, "https://bucket/img2.jpg", 1) + )); + ReflectionTestUtils.setField(notice, "images", existingImages); List<String> keepUrls = List.of("https://bucket/img1.jpg", "https://bucket/img2.jpg");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/amp/domain/notice/service/NoticeServiceTest.java` around lines 575 - 592, The test fixture is incomplete: setUp() leaves notice.images empty so keepUrls aren't actually present, making the counted images incorrect; adjust the test setup so the existing Notice returned to NoticeService has two Image entries whose URLs match the keepUrls used in the test (e.g., ensure notice.getImages() contains two Image objects with "https://bucket/img1.jpg" and "https://bucket/img2.jpg") before calling noticeService.updateNotice(1L, request, newImages) so the service counts keepUrls + newImages as 21 and triggers NoticeException with NoticeErrorCode.NOTICE_IMAGE_LIMIT_EXCEEDED.src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java (1)
294-342:⚠️ Potential issue | 🟠 Major
keepImageUrls유효성 검증 누락현재
keepUrls에 포함된 URL이 실제로 해당 공지의 이미지인지 검증하지 않습니다. 클라이언트가 잘못된 URL을 전송하면 해당 URL은imageMap.containsKey()에서 필터링되어 무시되지만, 의도와 달리 기존 이미지가 삭제될 수 있습니다.예: 공지에
[A, B, C]이미지가 있을 때 클라이언트가keepUrls = [A, X]를 보내면,X는 무시되고B,C만 삭제 대상이 됩니다. 클라이언트는A,X두 장이 유지될 것으로 기대하지만 실제로는A한 장만 남습니다.🛡️ keepUrls 검증 추가 제안
private void syncImages(Notice notice, List<String> keepImageUrls, List<MultipartFile> newImages) { List<String> keepUrls = (keepImageUrls != null) ? keepImageUrls : List.of(); List<MultipartFile> validNewImages = (newImages != null) ? newImages.stream().filter(f -> f != null && !f.isEmpty()).toList() : List.of(); if (keepUrls.size() + validNewImages.size() > 20) { throw new NoticeException(NoticeErrorCode.NOTICE_IMAGE_LIMIT_EXCEEDED); } List<NoticeImage> currentImages = notice.getImages(); Map<String, NoticeImage> imageMap = currentImages.stream() .collect(Collectors.toMap(NoticeImage::getImageUrl, img -> img)); + // keepUrls가 현재 이미지의 부분집합인지 검증 + boolean hasInvalidUrl = keepUrls.stream() + .anyMatch(url -> !imageMap.containsKey(url)); + if (hasInvalidUrl) { + throw new NoticeException(NoticeErrorCode.INVALID_KEEP_IMAGE_URL); + } + List<NoticeImage> imagesToDelete = currentImages.stream()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java` around lines 294 - 342, syncImages currently doesn't validate that entries in keepImageUrls belong to the given notice, so clients can pass foreign/invalid URLs causing unintended deletions; add a validation step in syncImages that computes invalidKeepUrls = keepUrls.stream().filter(url -> !imageMap.containsKey(url)).toList() after building imageMap and if invalidKeepUrls is not empty throw a NoticeException (add or use an appropriate NoticeErrorCode, e.g. NOTICE_INVALID_KEEP_IMAGE_URL or define one) so invalid URLs are rejected before imagesToDelete/currentImages are modified. Ensure you reference keepImageUrls/keepUrls, imageMap (from currentImages), imagesToDelete, and the NoticeException/NoticeErrorCode in the change.
🤖 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/amp/domain/notice/dto/request/NoticeUpdateRequest.java`:
- Line 14: The syncImages() flow silently drops any keepImageUrls that don't
match existing images (filtered by imageMap::containsKey), causing unexpected
deletions; update validation so that NoticeUpdateRequest's keepImageUrls is
validated before syncImages() runs: in the request validation path, check each
URL in keepImageUrls against current imageMap (or repository lookup used by
syncImages()), and if any URL is not found, reject the request with a 4xx
validation error (include which URL(s) are invalid). Modify syncImages() to
assume validated input (or explicitly fail-fast if mismatch still occurs) and
add unit/integration tests that submit a keepImageUrls list containing a
non-existent URL and assert the request is rejected and no images are deleted.
Ensure the error message names the invalid URL(s) for clarity.
In `@src/main/java/com/amp/domain/notice/entity/NoticeImage.java`:
- Around line 35-36: The factory method NoticeImage.of currently constructs a
child without updating the parent's collection, causing potential
collection-desynchronization with Notice.getImages(); modify the flow so
creating/saving a NoticeImage also updates the parent collection—either by
having NoticeImage.of call notice.getImages().add(newImage) (or better:
implement a helper Notice.addImage(NoticeImage) and use that inside the factory)
and ensure syncImages() and noticeImageRepository.save(...) usage relies on that
helper so the parent collection and DB stay consistent.
In `@src/main/resources/application.yml`:
- Around line 11-12: application.yml에서 max-file-size와 max-request-size를
5MB/105MB로 올린 것만으로는 프록시(예: Nginx, Ingress controller, ALB)의 업로드 제한이 낮으면 413 에러가
발생하므로, 배포 전 프록시/로드밸런서의 body size 설정을 함께 맞추세요: 확인 및 조치 대상은 Nginx/Ingress의
client_max_body_size, ALB/ELB의 리미트 설정 또는 프론트엔드(프록시 서버)에서 적용된 최대 요청 크기이며, 배포 문서나
helm/ingress manifest의 해당 필드(예: annotations나 configMap)에서 값을
max-file-size/max-request-size에 맞춰 증가시키고 20장×5MB 같은 시나리오로 통합 테스트를 수행하세요.
---
Duplicate comments:
In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java`:
- Around line 294-342: syncImages currently doesn't validate that entries in
keepImageUrls belong to the given notice, so clients can pass foreign/invalid
URLs causing unintended deletions; add a validation step in syncImages that
computes invalidKeepUrls = keepUrls.stream().filter(url ->
!imageMap.containsKey(url)).toList() after building imageMap and if
invalidKeepUrls is not empty throw a NoticeException (add or use an appropriate
NoticeErrorCode, e.g. NOTICE_INVALID_KEEP_IMAGE_URL or define one) so invalid
URLs are rejected before imagesToDelete/currentImages are modified. Ensure you
reference keepImageUrls/keepUrls, imageMap (from currentImages), imagesToDelete,
and the NoticeException/NoticeErrorCode in the change.
In `@src/test/java/com/amp/domain/notice/service/NoticeServiceTest.java`:
- Around line 575-592: The test fixture is incomplete: setUp() leaves
notice.images empty so keepUrls aren't actually present, making the counted
images incorrect; adjust the test setup so the existing Notice returned to
NoticeService has two Image entries whose URLs match the keepUrls used in the
test (e.g., ensure notice.getImages() contains two Image objects with
"https://bucket/img1.jpg" and "https://bucket/img2.jpg") before calling
noticeService.updateNotice(1L, request, newImages) so the service counts
keepUrls + newImages as 21 and triggers NoticeException with
NoticeErrorCode.NOTICE_IMAGE_LIMIT_EXCEEDED.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d641e4ae-6916-4d0d-b947-45b07873007a
📒 Files selected for processing (21)
.coderabbit.yamlsrc/main/java/com/amp/domain/audience/dto/response/SavedNoticesResponse.javasrc/main/java/com/amp/domain/notice/controller/organizer/NoticeCreateController.javasrc/main/java/com/amp/domain/notice/controller/organizer/NoticeUpdateController.javasrc/main/java/com/amp/domain/notice/dto/request/NoticeCreateRequest.javasrc/main/java/com/amp/domain/notice/dto/request/NoticeUpdateRequest.javasrc/main/java/com/amp/domain/notice/dto/response/FestivalNoticeListResponse.javasrc/main/java/com/amp/domain/notice/dto/response/NoticeDetailResponse.javasrc/main/java/com/amp/domain/notice/entity/Notice.javasrc/main/java/com/amp/domain/notice/entity/NoticeImage.javasrc/main/java/com/amp/domain/notice/exception/NoticeErrorCode.javasrc/main/java/com/amp/domain/notice/repository/NoticeImageRepository.javasrc/main/java/com/amp/domain/notice/repository/NoticeRepository.javasrc/main/java/com/amp/domain/notice/service/common/FestivalNoticeService.javasrc/main/java/com/amp/domain/notice/service/organizer/NoticeService.javasrc/main/java/com/amp/domain/notice/service/organizer/NoticeUpdateService.javasrc/main/java/com/amp/global/config/MultipartJackson2HttpMessageConverter.javasrc/main/resources/application-prod.ymlsrc/main/resources/application.ymlsrc/test/java/com/amp/domain/notice/service/NoticeServiceTest.javasrc/test/java/com/amp/domain/notice/service/NoticeUpdateServiceTest.java
💤 Files with no reviewable changes (5)
- src/main/java/com/amp/domain/notice/dto/request/NoticeCreateRequest.java
- src/main/resources/application-prod.yml
- src/main/java/com/amp/domain/notice/service/organizer/NoticeUpdateService.java
- src/test/java/com/amp/domain/notice/service/NoticeUpdateServiceTest.java
- src/main/java/com/amp/domain/notice/repository/NoticeRepository.java
| @Size(max = 50, message = "공지 제목은 최대 50자까지 입력할 수 있습니다.") String title, | ||
| @NotNull(message = "공지 카테고리 값은 필수값입니다.") Long categoryId, | ||
| MultipartFile newImage, | ||
| List<String> keepImageUrls, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== syncImages 구현 =="
sed -n '294,342p' src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java
echo
echo "== invalid keepImageUrls 검증/테스트 검색 =="
rg -n -C2 'keepImageUrls|keepUrls|containsKey|NOTICE_.*IMAGE' \
src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java \
src/test/javaRepository: TEAM-DECIBEL/AMP-SERVER
Length of output: 11996
🏁 Script executed:
# Verify if there are any validation checks elsewhere or in DTOs
rg -n 'keepImageUrls|keepUrls' src/main/java/com/amp/domain/notice --type java -B2 -A2 | head -60Repository: TEAM-DECIBEL/AMP-SERVER
Length of output: 3754
keepImageUrls에 존재하지 않는 URL이 들어오면 조용히 삭제가 발생합니다
syncImages()는 keepImageUrls의 각 URL이 현재 이미지에 실제로 존재하는지 검증하지 않습니다. 존재하지 않는 URL은 314-317번 줄의 filter(imageMap::containsKey)에서 조용히 제거되고, 이렇게 필터링된 keepUrls에 포함되지 않는 기존 이미지들이 모두 삭제 대상으로 표시됩니다.
구체적 문제:
- 사용자가 오타가 난 URL을
keepImageUrls에 실수로 포함시키면 → 그 URL은 무시되고 → 유지하려던 다른 이미지들이 삭제됨 - 400 에러나 검증 오류 없이 조용히 발생 → 사용자 입력 실수가 의도하지 않은 데이터 손실로 이어짐
- 요청 DTO에
keepImageUrls필드에 대한 검증 로직이 없음 - 존재하지 않는 URL 시나리오를 테스트하는 케이스 부재
권장: 유효하지 않은 keepImageUrls URL이 하나라도 있으면 요청 자체를 거절하는 검증 로직을 추가하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/amp/domain/notice/dto/request/NoticeUpdateRequest.java` at
line 14, The syncImages() flow silently drops any keepImageUrls that don't match
existing images (filtered by imageMap::containsKey), causing unexpected
deletions; update validation so that NoticeUpdateRequest's keepImageUrls is
validated before syncImages() runs: in the request validation path, check each
URL in keepImageUrls against current imageMap (or repository lookup used by
syncImages()), and if any URL is not found, reject the request with a 4xx
validation error (include which URL(s) are invalid). Modify syncImages() to
assume validated input (or explicitly fail-fast if mismatch still occurs) and
add unit/integration tests that submit a keepImageUrls list containing a
non-existent URL and assert the request is rejected and no images are deleted.
Ensure the error message names the invalid URL(s) for clarity.
Comment on lines
+11
to
+12
| max-file-size: 5MB | ||
| max-request-size: 105MB |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
앞단 업로드 한도도 같이 맞춰 주세요.
애플리케이션 한도만 105MB로 올려도 Nginx/Ingress/ALB가 더 낮게 잡혀 있으면 운영에서는 그대로 413이 납니다. 20장 × 5MB 시나리오를 릴리즈할 때는 프록시/body-size 설정도 함께 점검하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/application.yml` around lines 11 - 12, application.yml에서
max-file-size와 max-request-size를 5MB/105MB로 올린 것만으로는 프록시(예: Nginx, Ingress
controller, ALB)의 업로드 제한이 낮으면 413 에러가 발생하므로, 배포 전 프록시/로드밸런서의 body size 설정을 함께
맞추세요: 확인 및 조치 대상은 Nginx/Ingress의 client_max_body_size, ALB/ELB의 리미트 설정 또는
프론트엔드(프록시 서버)에서 적용된 최대 요청 크기이며, 배포 문서나 helm/ingress manifest의 해당 필드(예:
annotations나 configMap)에서 값을 max-file-size/max-request-size에 맞춰 증가시키고 20장×5MB 같은
시나리오로 통합 테스트를 수행하세요.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java (2)
289-317:⚠️ Potential issue | 🟠 Major
keepImageUrls는 현재 이미지의 중복 없는 부분집합인지 먼저 검증해 주세요.
null을 빈 목록으로 보는 건 의도된 동작이지만, 비-null 입력에 현재 공지에 없는 URL이 섞여 있으면 지금은 조용히 무시되어 유지하려던 이미지가 삭제 대상으로 바뀝니다. 중복 URL도keptImages.size()를 실제 유지 수보다 키워 순서/개수 계산을 왜곡할 수 있으니,keepUrls가 현재imageMap.keySet()의 중복 없는 부분집합인지 먼저 4xx로 검증하는 편이 안전합니다.Based on learnings
NoticeUpdateRequest.keepImageUrls가null이면 전체 삭제로 해석하는 것은 의도된 동작입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java` around lines 289 - 317, Validate keepImageUrls (when non-null) in NoticeService before computing keptImages: ensure the list contains no duplicates (compare list size to new HashSet<>(keepImageUrls).size()) and that every URL exists in the current image set (imageMap.keySet().containsAll(keepImageUrls)). If either check fails, throw a NoticeException with a 4xx error code (add or reuse an appropriate NoticeErrorCode like NOTICE_INVALID_KEEP_IMAGE_URLS). Only proceed to build keptImages and delete others after this validation passes.
126-130:⚠️ Potential issue | 🟠 MajorS3 보상 정리가 아직 완결되지 않았습니다.
기존 이미지 삭제를
afterCommit로 미룬 건 좋지만, 새로 업로드한 키는 롤백 훅에 등록되지 않아 후속save/flush/commit실패 시 S3에 고아로 남습니다. 반대로 업로드 자체가 실패했을 때는s3Service.delete()예외를 삼켜 부분 정리 실패도 감지되지 않습니다. 새 업로드는afterCompletion(STATUS_ROLLED_BACK)로 정리하고, 실패한 보상 삭제는 로그/재시도 대상으로 남겨 주세요.다음 스크립트로 현재 파일의 롤백 정리 유무와 저장소의 기존 패턴을 함께 확인할 수 있습니다:
#!/bin/bash set -euo pipefail # NoticeService의 업로드/트랜잭션 정리 흐름 확인 rg -n -C4 'uploadImagesInParallel|deleteS3AfterCommit|registerSynchronization|afterCommit|afterCompletion' \ src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java # 저장소에 이미 있는 트랜잭션 동기화 패턴 확인 fd -t f 'CongestionProcessor.java' src/main/java | \ xargs -r rg -n -C4 'registerSynchronization|afterCommit|afterCompletion'기대 결과:
NoticeService에도 새 업로드 키를afterCompletion(STATUS_ROLLED_BACK)로 정리하는 코드가 있어야 하고, 보상 삭제 실패는 숨기지 않고 추적 가능해야 합니다.
Based on learnings S3 파일 정리는TransactionSynchronizationManager.registerSynchronization()의 커밋/롤백 콜백에서 처리해야 합니다.Also applies to: 329-357, 360-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java` around lines 126 - 130, The new S3 upload keys returned by uploadImagesInParallel are not registered for rollback cleanup and s3Service.delete exceptions are being swallowed; fix by registering a TransactionSynchronization (via TransactionSynchronizationManager.registerSynchronization) inside the method that performs uploads (where uploadImagesInParallel is called) to capture the uploaded keys and perform cleanup in afterCompletion when status == TransactionSynchronization.STATUS_ROLLED_BACK (delete each key via s3Service.delete), and ensure s3Service.delete failures are not silently ignored but logged and published to a retry/compensation queue or persisted as failed-deletions for later retry; also ensure uploadExceptions propagate so the transaction can roll back instead of hiding partial failures before calling noticeImageRepository.save.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java`:
- Around line 289-317: Validate keepImageUrls (when non-null) in NoticeService
before computing keptImages: ensure the list contains no duplicates (compare
list size to new HashSet<>(keepImageUrls).size()) and that every URL exists in
the current image set (imageMap.keySet().containsAll(keepImageUrls)). If either
check fails, throw a NoticeException with a 4xx error code (add or reuse an
appropriate NoticeErrorCode like NOTICE_INVALID_KEEP_IMAGE_URLS). Only proceed
to build keptImages and delete others after this validation passes.
- Around line 126-130: The new S3 upload keys returned by uploadImagesInParallel
are not registered for rollback cleanup and s3Service.delete exceptions are
being swallowed; fix by registering a TransactionSynchronization (via
TransactionSynchronizationManager.registerSynchronization) inside the method
that performs uploads (where uploadImagesInParallel is called) to capture the
uploaded keys and perform cleanup in afterCompletion when status ==
TransactionSynchronization.STATUS_ROLLED_BACK (delete each key via
s3Service.delete), and ensure s3Service.delete failures are not silently ignored
but logged and published to a retry/compensation queue or persisted as
failed-deletions for later retry; also ensure uploadExceptions propagate so the
transaction can roll back instead of hiding partial failures before calling
noticeImageRepository.save.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dca6014f-368b-4680-840f-6a2413c7e095
📒 Files selected for processing (1)
src/main/java/com/amp/domain/notice/service/organizer/NoticeService.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💭 Related Issue
closed #216
💻 Key Changes
1. 공지 수정 시 이미지 최대 20장 첨부 기능 추가
기존에 이미지가 최대 1장까지 첨부 가능했던 기능을 스프린트 기능 추가에 따라 20장까지 첨부 가능하도록 구현하였습니다.
2. NoticeUpdateService → NoticeService로 통합
서비스 클래스 분리 없이 단일 서비스로 관리로 변경하였습니다.
3. DB 마이그레이션
서버에 배포하기 전 아래의 수정 사항을 거쳐야합니다.
💪🏻 To Reviewers
앞에 올린 리뷰 머지 후 해당 pr 머지하도록 하겠습니다.
추가로, 코드래빗이 pr 본문에 자동으로 내용 요약해주는 항목 비활성화시켜놨습니다.
📎 ETC
Summary by CodeRabbit
New Features
Chores