Merged
Conversation
1 task
📝 WalkthroughWalkthrough공연 수정이 multipart/form-data로 전환되어 mainImage 업로드·삭제가 트랜잭션 시점에 맞춰 처리되고, schedules/stages/activeCategoryIds가 JSON 문자열로 수신되며, 제거된 무대 ID들에 대해 StageCongestionRepository의 일괄 삭제가 추가되었습니다. (≤50단어) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FestivalController
participant FestivalService
participant S3Service
participant Database
participant StageCongestionRepo
Client->>FestivalController: PUT /{festivalId} (multipart/form-data, `@ModelAttribute`)
FestivalController->>FestivalService: updateFestival(festivalId, request)
alt mainImage 포함
FestivalService->>S3Service: upload(mainImage)
S3Service-->>FestivalService: newImageUrl
FestivalService->>Database: 조회(기존 festival) -> oldImageUrl
Database-->>FestivalService: oldImageUrl
FestivalService->>Database: 업데이트(newImageUrl 포함)
alt 트랜잭션 커밋
FestivalService->>S3Service: extractKey(oldImageUrl)
S3Service-->>FestivalService: oldKey
FestivalService->>S3Service: delete(oldKey)
S3Service-->>FestivalService: deleteResult
else 트랜잭션 롤백
FestivalService->>S3Service: extractKey(newImageUrl)
S3Service-->>FestivalService: newKey
FestivalService->>S3Service: delete(newKey)
S3Service-->>FestivalService: deleteResult
end
end
FestivalService->>Database: schedules/stages/categories 동기화
Database-->>FestivalService: syncResult
FestivalService->>StageCongestionRepo: deleteByStageIdIn(removedStageIds)
StageCongestionRepo-->>Database: deleteQuery
Database-->>FestivalService: deleteResult
FestivalService-->>FestivalController: FestivalUpdateResponse
FestivalController-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60분 Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/festival/dto/request/FestivalUpdateRequest.java`:
- Around line 6-12: FestivalUpdateRequest's current contract cannot express an
"image delete only" intent because mainImage only covers keep-or-replace; add an
explicit indicator (e.g., Boolean removeMainImage or an enum imageAction) to
FestivalUpdateRequest so callers can signal deletion, update any consumers to
respect that flag in the update flow, and adjust Festival.mainImageUrl (nullable
constraint) and the persistence/update logic to allow null when removeMainImage
is true and to delete the file from storage accordingly; ensure update handler
checks the new flag before treating a null MultipartFile as "no change."
In
`@src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java`:
- Around line 189-202: In FestivalService, avoid deleting the existing S3 object
(s3Service.delete(oldKey)) before the DB transaction commits; instead, upload
the new image via uploadImage, set
festival.updateMainImage(s3Service.getPublicUrl(newKey)) inside the transaction,
and schedule deletion of the oldKey in an afterCommit callback (so deletion
happens only after successful commit); on failure or rollback only remove the
newly uploaded newKey (s3Service.delete(newKey)) in the catch/rollback path to
avoid DB/S3 divergence.
In `@src/main/java/com/amp/global/s3/S3Service.java`:
- Around line 85-91: The extractKey method currently uses String.replace which
silently returns the original URL when the base URL doesn't match; change
extractKey(String publicUrl) to first build the baseUrl (using
s3Properties.getBaseUrl(), s3Properties.getBucket(), s3Properties.getRegion())
and validate that publicUrl.startsWith(baseUrl); if it does, return
publicUrl.substring(baseUrl.length()) as the key, otherwise fail fast by
throwing an IllegalArgumentException (or a custom runtime exception) with a
clear message so callers (e.g., delete callers) won't proceed with an invalid
key.
In
`@src/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.java`:
- Around line 29-37: The current unit test helper requestWith(...) constructs
FestivalUpdateRequest directly and only exercises Bean Validation; add
service/controller-level tests that exercise the actual update flow (e.g., call
the controller endpoint or FestivalService.updateFestival) to validate JSON
string parsing and request-format handling for the
schedules/stages/activeCategoryIds fields; include at minimum tests for an empty
array string ("[]"), malformed JSON strings, and a full valid update request to
catch regressions in parsing and update behavior rather than only record
construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98fbdff1-b91a-41d5-b2b6-cad4b0ed0f52
📒 Files selected for processing (8)
src/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/global/s3/S3Service.javasrc/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.java
src/main/java/com/amp/domain/festival/dto/request/FestivalUpdateRequest.java
Show resolved
Hide resolved
src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java
Show resolved
Hide resolved
src/test/java/com/amp/domain/festival/dto/request/FestivalUpdateRequestValidationTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/festival/controller/organizer/FestivalController.java`:
- Around line 55-58: The HTTP method was accidentally changed to PUT which
breaks existing clients and the intended PATCH semantics; revert the mapping on
the updateFestival handler by replacing the `@PutMapping` annotation with the
appropriate `@PatchMapping` (keeping the value = "/{festivalId}" and consumes =
MediaType.MULTIPART_FORM_DATA_VALUE) so updateFestival continues to represent a
partial update while supporting multipart form data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10943663-404a-43ac-8fd8-9750866ce0f4
📒 Files selected for processing (1)
src/main/java/com/amp/domain/festival/controller/organizer/FestivalController.java
src/main/java/com/amp/domain/festival/controller/organizer/FestivalController.java
Show resolved
Hide resolved
- url이 baseUrl로 시작하지 않으면 예외
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/festival/service/organizer/FestivalService.java`:
- Around line 196-208: Wrap the s3Service.delete(...) calls inside the
TransactionSynchronization registered via
TransactionSynchronizationManager.registerSynchronization with try/catch blocks
in both the afterCommit and afterCompletion methods: catch any Exception thrown
by s3Service.delete(oldKey) in afterCommit and by s3Service.delete(newKey) in
afterCompletion (when status == STATUS_ROLLED_BACK), and log the full exception
using the existing logger so failures are recorded instead of propagating or
being silently ignored; keep the TransactionSynchronization and method names
(afterCommit, afterCompletion, STATUS_ROLLED_BACK, s3Service.delete, oldKey,
newKey) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfb64b2c-88d4-4106-874c-e1430480f4cb
📒 Files selected for processing (3)
src/main/java/com/amp/domain/festival/service/organizer/FestivalService.javasrc/main/java/com/amp/global/s3/S3ErrorCode.javasrc/main/java/com/amp/global/s3/S3Service.java
src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java
Show resolved
Hide resolved
dongchan0105
approved these changes
Mar 10, 2026
Contributor
dongchan0105
left a comment
dongchan0105
left a comment
There was a problem hiding this comment.
내가 왓슈,,, 딱히 문제되는 코드는 없어보이고 그냥 DTO에서 String 객체 쓰면서 검증하는거 빠진거같은데 이거 객체로 파싱하는과정에서라도 검증하는게 비즈니스 요구사항에 들어가 있으면 넣어주기만 하면 될거같아여 나머진 역시 매우 굿굿
Comment on lines
+9
to
+12
| MultipartFile mainImage, | ||
| @NotBlank(message = "공연 일시는 필수입니다.") String schedules, | ||
| @NotBlank(message = "1개 이상의 무대/부스 정보는 필수입니다.") String stages, | ||
| @NotBlank(message = "1개 이상의 카테고리 선택은 필수입니다.") String activeCategoryIds |
Contributor
There was a problem hiding this comment.
@Valid 가 빠지면서 내부 필드 값에 대한 검증이 더이상 진행되지 않는 걸로 보이는데 상관없는건가요??
Contributor
Author
There was a problem hiding this comment.
컨트롤러 파라미터에 FestivalUpdateRequest에 한 번에 붙여놨습니다. 현재 해당 클래스의 dto에는 중첩객체가 없고 단순 타입만 있어서 생략했습니다. 필요하다면 추가하겠습니다~!!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/controller/AudienceController.java`:
- Line 46: The controller mixes user identification methods:
AudienceController.getMyPage calls
AudienceService.getMyPage(principal.getEmail()) while getSavedNotices passes a
Long userId to AudienceNoticesService.getSavedAnnouncements, causing
inconsistency; pick and apply a single canonical identifier across the
controller (either email or userId) or add explicit conversion/lookup where
needed (e.g., resolve principal.getEmail() to userId before calling
getSavedAnnouncements or change AudienceService.getMyPage to accept userId),
update all calls (getMyPage, getSavedNotices, AudienceService.getMyPage,
AudienceNoticesService.getSavedAnnouncements) accordingly, and add a brief
comment documenting the chosen identifier strategy for future maintainers.
In
`@src/main/java/com/amp/domain/festival/service/organizer/FestivalService.java`:
- Around line 267-271: normalizeJsonArray currently wraps non-bracketed input
into an array string but converts empty or whitespace-only strings into "[]",
which later is filtered by checkNullField; to make intent clearer, change
normalizeJsonArray (method name) to return null when the trimmed value is empty
(i.e., treat "" or " " as null) instead of returning "[]", so callers like
checkNullField receive null explicitly—implement by checking trimmed.isEmpty()
and returning null before the startsWith("[") check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43954544-a2a1-40eb-829d-9e2413b0615a
📒 Files selected for processing (7)
src/main/java/com/amp/domain/audience/controller/AudienceController.javasrc/main/java/com/amp/domain/audience/service/AudienceService.javasrc/main/java/com/amp/domain/festival/dto/response/FestivalInfoResponse.javasrc/main/java/com/amp/domain/festival/entity/Festival.javasrc/main/java/com/amp/domain/festival/service/organizer/FestivalService.javasrc/main/java/com/amp/global/s3/S3Service.javasrc/test/java/com/amp/domain/congestion/service/CongestionCalculateServiceTest.java
| @AuthenticationPrincipal CustomUserPrincipal principal) { | ||
| Long userId = principal.getUserId(); | ||
| AudienceMyPageResponse response = audienceService.getMyPage(userId); | ||
| AudienceMyPageResponse response = audienceService.getMyPage(principal.getEmail()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: AudienceNoticesService가 userId를 사용하는지 확인하여 불일치가 의도적인지 검증
rg -n 'getSavedAnnouncements' --type java -A 5Repository: TEAM-DECIBEL/AMP-SERVER
Length of output: 1595
서비스 계층 변경에 맞춰 올바르게 수정되었습니다.
AudienceService.getMyPage(String email) 메서드 시그니처 변경에 따라 principal.getEmail()을 전달하는 것이 정확합니다.
다만, 동일 컨트롤러 내에서 사용자 식별 방식이 일관성 없습니다:
getMyPage()(46행):principal.getEmail()사용getSavedNotices()(73행):userId사용 (AudienceNoticesService.getSavedAnnouncements는 Long userId 필수)
각 서비스가 서로 다른 식별자를 요구하기 때문에 현재 구현은 필요에 따른 의도적 설계입니다. 다만 향후 유지보수 관점에서 사용자 식별 방식을 통일하거나, 각 선택 이유를 문서화할 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/amp/domain/audience/controller/AudienceController.java` at
line 46, The controller mixes user identification methods:
AudienceController.getMyPage calls
AudienceService.getMyPage(principal.getEmail()) while getSavedNotices passes a
Long userId to AudienceNoticesService.getSavedAnnouncements, causing
inconsistency; pick and apply a single canonical identifier across the
controller (either email or userId) or add explicit conversion/lookup where
needed (e.g., resolve principal.getEmail() to userId before calling
getSavedAnnouncements or change AudienceService.getMyPage to accept userId),
update all calls (getMyPage, getSavedNotices, AudienceService.getMyPage,
AudienceNoticesService.getSavedAnnouncements) accordingly, and add a brief
comment documenting the chosen identifier strategy for future maintainers.
Comment on lines
+267
to
+271
| private String normalizeJsonArray(String value) { | ||
| if (value == null) return null; | ||
| String trimmed = value.trim(); | ||
| return trimmed.startsWith("[") ? trimmed : "[" + trimmed + "]"; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Swagger의 배열 전송 방식 처리
normalizeJsonArray는 Swagger가 대괄호 없이 배열을 전송하는 경우(예: "1,2,3" → "[1,2,3]")를 처리합니다.
한 가지 고려사항: 빈 문자열("")이나 공백만 있는 경우(" ") "[]"로 변환되어 빈 리스트로 파싱됩니다. 현재는 checkNullField에서 빈 리스트를 걸러내므로 문제없지만, 의도를 더 명확히 하려면 빈 문자열일 때 null을 반환하는 방식도 고려할 수 있습니다:
선택적 개선안
private String normalizeJsonArray(String value) {
if (value == null) return null;
String trimmed = value.trim();
+ if (trimmed.isEmpty()) return null;
return trimmed.startsWith("[") ? trimmed : "[" + trimmed + "]";
}현재 구현도 정상 동작하므로 선택적 개선입니다.
📝 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.
Suggested change
| private String normalizeJsonArray(String value) { | |
| if (value == null) return null; | |
| String trimmed = value.trim(); | |
| return trimmed.startsWith("[") ? trimmed : "[" + trimmed + "]"; | |
| } | |
| private String normalizeJsonArray(String value) { | |
| if (value == null) return null; | |
| String trimmed = value.trim(); | |
| if (trimmed.isEmpty()) return null; | |
| return trimmed.startsWith("[") ? trimmed : "[" + trimmed + "]"; | |
| } |
🤖 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/service/organizer/FestivalService.java`
around lines 267 - 271, normalizeJsonArray currently wraps non-bracketed input
into an array string but converts empty or whitespace-only strings into "[]",
which later is filtered by checkNullField; to make intent clearer, change
normalizeJsonArray (method name) to return null when the trimmed value is empty
(i.e., treat "" or " " as null) instead of returning "[]", so callers like
checkNullField receive null explicitly—implement by checking trimmed.isEmpty()
and returning null before the startsWith("[") check.
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 #213
💻 Key Changes
1. 공연 이미지 수정 API 추가
스프린트 기능에 따라 공연 이미지 수정 API를 추가했습니다. 공연 생성의 경우 이미지가 필수라서 삭제 api는 구현되지 않았습니다.
mainImage필드를 optional로 추가하여 보내면 새로운 이미지로 교체, 안 보내면 기존 이미지 유지로 구현하였습니다.FestivalUpdateRequest에MultipartFile mainImage필드 추가PATCH /festivals/{id}엔드포인트를multipart/form-data로 변경 (@RequestBody→@ModelAttribute)Festival.updateMainImage()메서드 추가S3Service.extractKey()추가: 공개 URL에서 S3 키 추출2. 무대 삭제 시 관련 혼잡도 데이터도 삭제되도록 수정
무대 삭제 시 혼잡도 관련 데이터가 있는 경우 에러가 발생했습니다. 따라서 무대 삭제하는 경우 관련된 혼잡도 데이터도 삭제되도록 코드를 수정했습니다.
StageCongestionRepository.deleteByStageIdIn()추가StageService.syncStages(): 무대 제거 전 해당 혼잡도 데이터 먼저 삭제3. 공연 수정 요청 JSON 파싱 방식 통일
공연 생성 로직과 동일한 로직으로 교체했습니다. 또, 스웨거에서
activeCategoryIds가 [1,3] 로 입력해도 1,3으로 배열 괄호 없이 전달되는 문제가 있었습니다. postman이나 curl로 테스트할 때는 잘 되지만 프론트에서 테스트할 때 어려움이 있을 것 같아 파싱 로직을 추가했습니다.FestivalUpdateRequest의schedules,stages,activeCategoryIds타입을List→String(JSON)으로 변경updateFestival()에서 직접 JSON 파싱normalizeJsonArray()추가checkNullField()메서드로 추출💪🏻 To Reviewers
스웨거 문제는
@ModelAttribute랑 배열 조합이 좋지 않다고 하더라고요... 최대한으로 해결하는 방안이 직접 파싱해주는거라 이렇게 해결했습니다 ㅠ.ㅠ📎 ETC
Summary by CodeRabbit
새로운 기능
개선사항