[FEAT] poster, notice, casting 이미지 수정, dto로 받은 공연관련 이미지의 imageUrl을 db에 저장안하던 기존 로직 수정 #120
Conversation
WalkthroughCentralizes amateur-show image handling into URL-aware ImageService methods, adds null/blank guards and S3-key validation, updates poster/notice/casting save/update/delete flows, extends FilePath with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant AmateurService as AmateurServiceImpl
participant ImageService
participant S3Service
participant DB as Database
participant EventPub as EventPublisher
Client->>AmateurService: enroll/update/delete show (includes image keyNames & optional imageUrls)
AmateurService->>ImageService: saveImageWithImageUrl(...) / updateShowImage(...) / delete image calls
ImageService->>S3Service: doesObjectExist(keyName)
alt S3 object exists
ImageService->>DB: find existing Image by (filePath, contentId)
alt keyName changed or new
ImageService->>DB: delete existing Image (if any)
ImageService->>DB: save new Image (with optional imageUrl)
else same keyName and imageUrl present
ImageService->>DB: update Image URL
end
else S3 object missing
ImageService-->>AmateurService: throw INVALID_S3_KEY / error
end
AmateurService->>DB: persist AmateurShow and related entities (castings, notices)
AmateurService->>EventPub: publish NewShowEvent (only when likers exist)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/java/cc/backend/image/service/ImageService.java (1)
63-85: Avoid persistingimageUrl=""; usenullto represent “no URL”
Right nowsaveImageWithImageUrlstores empty-string when absent (Line 72), while the existingsaveImage(...)path doesn’t setimageUrlat all (likelyNULL). This inconsistency will bite querying/serialization.- String imageUrl = imageUrlOpt.orElse(""); + String imageUrl = imageUrlOpt.orElse(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java(7 hunks)src/main/java/cc/backend/config/s3/S3Controller.java(1 hunks)src/main/java/cc/backend/image/service/ImageService.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/cc/backend/config/s3/S3Controller.java (1)
src/main/java/cc/backend/config/s3/S3Service.java (1)
doesObjectExist(188-211)
src/main/java/cc/backend/image/service/ImageService.java (2)
src/main/java/cc/backend/image/controller/ImageController.java (1)
Operation(25-31)src/main/java/cc/backend/image/entity/Image.java (1)
Entity(16-61)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (1)
src/main/java/cc/backend/image/DTO/ImageRequestDTO.java (1)
ImageRequestDTO(12-66)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
Show resolved
Hide resolved
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
Show resolved
Hide resolved
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
Show resolved
Hide resolved
| if(keyName == null){ | ||
| return ResponseEntity.ok(false); | ||
| } | ||
| return ResponseEntity.ok(s3Service.doesObjectExist(keyName, member.getId())); |
There was a problem hiding this comment.
/s3/exist should likely treat blank keyName the same as null
Returning false for null but throwing for "" / " " is surprising for an “exist” endpoint.
- if(keyName == null){
+ if (keyName == null || keyName.isBlank()) {
return ResponseEntity.ok(false);
}📝 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.
| if(keyName == null){ | |
| return ResponseEntity.ok(false); | |
| } | |
| return ResponseEntity.ok(s3Service.doesObjectExist(keyName, member.getId())); | |
| if (keyName == null || keyName.isBlank()) { | |
| return ResponseEntity.ok(false); | |
| } | |
| return ResponseEntity.ok(s3Service.doesObjectExist(keyName, member.getId())); |
🤖 Prompt for AI Agents
In src/main/java/cc/backend/config/s3/S3Controller.java around lines 86 to 89,
the endpoint returns false when keyName is null but will throw for empty or
whitespace-only strings; normalize the input by treating blank values the same
as null (e.g., check keyName == null || keyName.trim().isEmpty() or use
StringUtils.isBlank(keyName)) and return ResponseEntity.ok(false) for those
cases so empty/whitespace keys do not cause errors and behavior is consistent.
| @Transactional | ||
| public void updateImage( | ||
| Long memberId, | ||
| String keyName, | ||
| Optional<String> imageUrlOpt, | ||
| Long contentId, | ||
| FilePath filePath | ||
| ) { | ||
| if (keyName == null || keyName.isBlank()) { | ||
| return; // keyName 없으면 처리하지 않음 | ||
| } | ||
|
|
||
| String imageUrl = imageUrlOpt.orElse(""); | ||
|
|
||
| // 기존 이미지 조회 (filePath + contentId 기준) | ||
| Image existingImage = imageRepository | ||
| .findAllByFilePathAndContentId(filePath, contentId) | ||
| .stream() | ||
| .findFirst() | ||
| .orElse(null); | ||
|
|
||
| // 기존 이미지가 있고, keyName이 다르면 삭제 후 새로 저장 | ||
| if (existingImage != null && !existingImage.getKeyName().equals(keyName)) { | ||
| deleteImage(existingImage.getId(), memberId); | ||
| } | ||
|
|
||
| // 새 이미지 저장 | ||
| Image image = Image.builder() | ||
| .keyName(keyName) | ||
| .imageUrl(imageUrl) | ||
| .filePath(filePath) | ||
| .contentId(contentId) | ||
| .uploadedAt(LocalDateTime.now()) | ||
| .memberId(memberId) | ||
| .build(); | ||
|
|
||
| imageRepository.save(image); | ||
| } |
There was a problem hiding this comment.
updateImage creates duplicate DB rows and can corrupt “one image per (filePath, contentId)” semantics
- If
existingImage != nullandkeyNameis unchanged, you don’t delete, but you stillsave(image)(Line 225) ⇒ duplicates. - You also
findFirst()among potentially many rows (Line 206-208), so behavior becomes non-deterministic as duplicates accumulate. - No S3 existence check here, so you can persist a key that was never uploaded.
A minimally invasive fix (prevents duplicates, adds S3 validation, and avoids S3 delete on same-key rewrites):
public void updateImage(
Long memberId,
String keyName,
Optional<String> imageUrlOpt,
Long contentId,
FilePath filePath
) {
if (keyName == null || keyName.isBlank()) {
return; // keyName 없으면 처리하지 않음
}
- String imageUrl = imageUrlOpt.orElse("");
+ if (!s3Service.doesObjectExist(keyName, memberId)) {
+ throw new GeneralException(ErrorStatus.NOT_FOUND_IN_S3);
+ }
+
+ String imageUrl = imageUrlOpt.orElse(null);
// 기존 이미지 조회 (filePath + contentId 기준)
Image existingImage = imageRepository
.findAllByFilePathAndContentId(filePath, contentId)
.stream()
.findFirst()
.orElse(null);
- // 기존 이미지가 있고, keyName이 다르면 삭제 후 새로 저장
- if (existingImage != null && !existingImage.getKeyName().equals(keyName)) {
- deleteImage(existingImage.getId(), memberId);
- }
+ if (existingImage != null) {
+ // keyName 변경이면 S3/DB 모두 삭제
+ if (!existingImage.getKeyName().equals(keyName)) {
+ deleteImage(existingImage.getId(), memberId);
+ } else {
+ // 같은 keyName이면 DB row만 교체(중복 방지), S3는 유지
+ imageRepository.delete(existingImage);
+ }
+ }
// 새 이미지 저장
Image image = Image.builder()
.keyName(keyName)
.imageUrl(imageUrl)
.filePath(filePath)
.contentId(contentId)
.uploadedAt(LocalDateTime.now())
.memberId(memberId)
.build();
imageRepository.save(image);
}If you truly need multiple images (e.g., multiple casting images) under the same filePath+contentId, then this API/lookup key needs redesign (e.g., contentId = castingId, or add an ImageType column and query by (filePath, contentId, type)).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/cc/backend/image/service/ImageService.java around lines
189-226, the updateImage method currently can create duplicate DB rows, uses a
non-deterministic findFirst() over many rows, and does not validate S3
existence; change the flow so: 1) query for all images by filePath+contentId and
deterministically pick the canonical existingImage (preferably the one with
matching keyName or the newest), 2) if an existingImage exists and its keyName
equals the incoming keyName, do NOT insert a new row — update the existingImage
fields (imageUrl, uploadedAt, memberId) and save the entity instead, 3) if
existingImage exists and keyName differs, first validate that the new keyName
exists in S3 (or throw/return) then delete any other DB rows and their S3
objects for the old key(s) before inserting the new Image row, 4) if no
existingImage, validate the new keyName exists in S3 before creating and saving
a single Image row; ensure the repository enforces or is migrated toward a
uniqueness constraint on (filePath, contentId) or introduce an ImageType if
multiple images per key are required.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (1)
79-84: Poster DTO null check still missing—potential NPE.Line 82 calls
dto.getKeyName()butdtoitself (fromrequestDTO.getPosterImageRequestDTO()) can be null, causing an NPE.ImageRequestDTO.PosterImageRequestDTO dto = requestDTO.getPosterImageRequestDTO(); //poster 이미지는 없으면 에러 -if(dto.getKeyName() == null || dto.getKeyName().isBlank()){ +if(dto == null || dto.getKeyName() == null || dto.getKeyName().isBlank()){ throw new GeneralException(ErrorStatus.INVALID_S3_KEY); }
🧹 Nitpick comments (2)
src/main/java/cc/backend/image/service/ImageService.java (2)
63-88: Consider refactoring to reduce code duplication withsaveImage.
saveImageWithImageUrlduplicates most of the logic fromsaveImage(lines 42-61). Consider extracting common logic or havingsaveImagedelegate tosaveImageWithImageUrlwithOptional.empty().@Transactional public ImageResponseDTO.ImageResultWithPresignedUrlDTO saveImage(Long memberId, ImageRequestDTO.FullImageRequestDTO requestDTO) { - memberRepository.findById(memberId).orElseThrow(() -> new GeneralException(ErrorStatus.MEMBER_NOT_FOUND)); - //S3에 실제 존재하는 이미지인지 검증 - if(!s3Service.doesObjectExist(requestDTO.getKeyName(), memberId)) { - throw new GeneralException(ErrorStatus.NOT_FOUND_IN_S3); - } - - Image image = Image.builder() - .keyName(requestDTO.getKeyName()) - .filePath(requestDTO.getFilePath()) - .contentId(requestDTO.getContentId()) - .uploadedAt(LocalDateTime.now()) - .memberId(memberId) - .build(); - - Image newImage = imageRepository.save(image); - - return getImage(newImage.getKeyName(), memberId); + return saveImageWithImageUrl(memberId, requestDTO, Optional.empty()); }
192-233: Repository method correctly returns single Image; verify uniqueness is enforced elsewhere.The method
findByFilePathAndContentIdexists on line 15 ofImageRepository.javaand correctly returns a singleImageinstead of a list. The S3 existence check and duplicate-prevention logic inupdateShowImageare sound. However, note that the database index onfile_pathandcontent_id(line 23 ofImage.java) is for query performance only—it does not enforce uniqueness. If uniqueness is required, add@UniqueConstraint(columnNames = {"file_path", "content_id"})to the@Tableannotation on the Image entity to prevent duplicate entries at the database level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java(8 hunks)src/main/java/cc/backend/config/s3/S3Controller.java(3 hunks)src/main/java/cc/backend/config/s3/S3Service.java(1 hunks)src/main/java/cc/backend/image/FilePath.java(1 hunks)src/main/java/cc/backend/image/service/ImageService.java(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/cc/backend/config/s3/S3Service.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/cc/backend/config/s3/S3Controller.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (1)
src/main/java/cc/backend/image/DTO/ImageRequestDTO.java (1)
ImageRequestDTO(12-66)
🔇 Additional comments (4)
src/main/java/cc/backend/image/FilePath.java (1)
4-4: LGTM!The new
noticeandcastingenum constants follow the existing naming convention and support the new image handling flows.src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (3)
119-138: LGTM!The null/blank guard for
keyName(Lines 122-124) and usage ofcasting.getId()ascontentIdwithFilePath.castingare correct. Each casting image is properly associated with its casting entity.
200-211: LGTM!Proper null and blank validation before calling
updateShowImage, and the show'sposterImageUrlis updated accordingly.
96-105: LGTM!Good optimization to only publish
NewShowEventwhen there are actual likers to notify.
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
Show resolved
Hide resolved
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
Show resolved
Hide resolved
| //포스터 삭제 | ||
| Image posterImg = imageRepository.findByFilePathAndContentId(FilePath.amateurShow, amateurShowId); | ||
| imageService.deleteImage(posterImg.getId(), memberId); |
There was a problem hiding this comment.
Missing null check for poster image before deletion.
findByFilePathAndContentId may return null if no poster image exists, causing an NPE on Line 407. Add a null check for consistency with the notice image handling below.
//포스터 삭제
Image posterImg = imageRepository.findByFilePathAndContentId(FilePath.amateurShow, amateurShowId);
-imageService.deleteImage(posterImg.getId(), memberId);
+if (posterImg != null) {
+ imageService.deleteImage(posterImg.getId(), memberId);
+}🤖 Prompt for AI Agents
In
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
around lines 405 to 407, the code calls
imageRepository.findByFilePathAndContentId(...) and immediately uses
posterImg.getId(), which can NPE if no poster exists; add a null check like the
one used for the notice image: only call imageService.deleteImage(...) when
posterImg is not null (and handle/log accordingly) to avoid
NullPointerException.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/cc/backend/image/service/ImageService.java (1)
42-72:imageUrlOpt.orElse("")should likely benull(avoid empty-string “nullable” drift).
Right nowsaveImageWithImageUrlwrites""when absent (Line 59), whileupdateShowImagewritesnull(Line 191). This can break “nullable” semantics and any downstream “is null” queries.- String imageUrl = imageUrlOpt.orElse(""); + String imageUrl = imageUrlOpt.orElse(null);
♻️ Duplicate comments (3)
src/main/java/cc/backend/image/service/ImageService.java (1)
176-217: Consider “update-in-place” + “delete-on-blank” semantics for show images.
- If
keyNameis blank youreturn(Line 184-186), so callers cannot remove an existing notice/casting image (even though those are “nullable” per PR objectives). Consider: blank/null ⇒ delete existing(filePath, contentId)row (and S3 object) instead of no-op.- When
existingImagehas the samekeyName, you delete the row and re-insert (Line 202-204, 206-216). This avoids duplicates, but churns IDs and is less safe than updating fields on the existing entity (this is closely related to the earlier duplicate-row concern).Proposed direction (illustrative):
public void updateShowImage(...){ - if (keyName == null || keyName.isBlank()) { - return; // keyName 없으면 처리하지 않음 - } + Image existingImage = imageRepository.findByFilePathAndContentId(filePath, contentId); + if (keyName == null || keyName.isBlank()) { + if (existingImage != null) deleteImage(existingImage.getId(), memberId); + return; + } if (!s3Service.doesObjectExist(keyName, memberId)) { throw new GeneralException(ErrorStatus.NOT_FOUND_IN_S3); } String imageUrl = imageUrlOpt.orElse(null); - Image existingImage = imageRepository - .findByFilePathAndContentId(filePath, contentId); - - if (existingImage != null) { - if (!existingImage.getKeyName().equals(keyName)) { - deleteImage(existingImage.getId(), memberId);} - else { - imageRepository.delete(existingImage); - } - } - Image image = Image.builder()... - imageRepository.save(image); + if (existingImage == null) { + imageRepository.save(Image.builder()...build()); + return; + } + if (!existingImage.getKeyName().equals(keyName)) { + deleteImage(existingImage.getId(), memberId); + imageRepository.save(Image.builder()...build()); + return; + } + // same keyName: update in place + existingImage.updateImageUrl(imageUrl); + existingImage.updateUploadedAt(LocalDateTime.now()); + existingImage.updateMemberId(memberId); + imageRepository.save(existingImage); }(You’ll need setters / update methods on
Imageto avoid direct field mutation.)src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (2)
79-84: Poster DTO can be null → NPE ondto.getKeyName()(poster “required” validation still incomplete).
This matches the earlier review feedback:requestDTO.getPosterImageRequestDTO()can be null, so Line 82 can throw.- ImageRequestDTO.PosterImageRequestDTO dto = requestDTO.getPosterImageRequestDTO(); - //poster 이미지는 없으면 에러 - if(dto.getKeyName() == null || dto.getKeyName().isBlank()){ + ImageRequestDTO.PosterImageRequestDTO dto = requestDTO.getPosterImageRequestDTO(); + // poster 이미지는 없으면 에러 + if (dto == null || dto.getKeyName() == null || dto.getKeyName().isBlank()) { throw new GeneralException(ErrorStatus.INVALID_S3_KEY); }
413-435: Poster deletion can NPE when no poster image exists.
findByFilePathAndContentIdmay return null; Line 415 then crashes. This matches the earlier review feedback.//포스터 삭제 Image posterImg = imageRepository.findByFilePathAndContentId(FilePath.amateurShow, amateurShowId); - imageService.deleteImage(posterImg.getId(), memberId); + if (posterImg != null) { + imageService.deleteImage(posterImg.getId(), memberId); + }
🧹 Nitpick comments (2)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (2)
146-161: Notice image DTO null-guard looks good; confirmrequestDTO.getNotice()can’t be null here.
You guardnoticeImageDTO, but still dereferencerequestDTO.getNotice()(Line 146). IfrequestDTO.getNotice()is nullable at API level, consider caching it earlier and null-checking before use.
275-305: New casting save: addsavedCastingtoupdatedList(clarity/consistency).
Yousave(newCasting)but addnewCastingtoupdatedList(Line 288-290). Usually the saved instance is clearer/safer.- AmateurCasting savedCasting = amateurCastingRepository.save(newCasting); - updatedList.add(newCasting); + AmateurCasting savedCasting = amateurCastingRepository.save(newCasting); + updatedList.add(savedCasting); contentId = savedCasting.getId();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java(8 hunks)src/main/java/cc/backend/image/service/ImageService.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (2)
src/main/java/cc/backend/image/DTO/ImageRequestDTO.java (1)
ImageRequestDTO(12-66)src/main/java/cc/backend/amateurShow/converter/AmateurConverter.java (1)
AmateurConverter(17-281)
🔇 Additional comments (3)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (3)
93-105: New show event publish guarded by liker existence looks good.
Avoids firing events with empty target lists (Line 96-105).
120-137: Casting image save: good(FilePath.casting, contentId=castingId)+ null/blank guard.
This aligns “one image per casting” and prevents accidental overwrites.
249-263: Notice update: good null-guards + using persisted notice ID.
Fixes the earliernoticeDTONPE risk and avoids using request DTO IDs (Line 251-262).
| if (dto != null && dto.getKeyName() != null && !dto.getKeyName().isBlank()) { | ||
| // 현재 포스터 이미지 조회 (show당 1개) | ||
| Image existingImage = imageRepository | ||
| .findAllByFilePathAndContentId(FilePath.amateurShow, amateurShow.getId()) | ||
| .stream() | ||
| .findFirst() | ||
| .orElse(null); | ||
|
|
||
| // 기존 keyName과 다르면 기존 이미지 삭제 후 교체 | ||
| if (existingImage != null && !existingImage.getKeyName().equals(dto.getKeyName())) { | ||
| imageService.deleteImage(existingImage.getId(), memberId); | ||
| } | ||
| imageService.updateShowImage( | ||
| memberId, | ||
| dto.getKeyName(), | ||
| Optional.ofNullable(dto.getImageUrl()), | ||
| amateurShow.getId(), | ||
| FilePath.amateurShow | ||
| ); | ||
|
|
||
| ImageRequestDTO.FullImageRequestDTO fullImageRequestDTO = ImageRequestDTO.FullImageRequestDTO.builder() | ||
| .keyName(dto.getKeyName()) | ||
| .filePath(FilePath.amateurShow) | ||
| .contentId(amateurShow.getId()) | ||
| .memberId(memberId) | ||
| .build(); | ||
|
|
||
| imageService.saveImage(memberId, fullImageRequestDTO); | ||
| //amateurShow 엔티티 내 posterImageUrl 필드 수정 | ||
| amateurShow.updatePosterImageUrl(dto.getImageUrl()); | ||
|
|
||
| } |
There was a problem hiding this comment.
Poster update via updateShowImage is cleaner than delete+insert; watch for null imageUrl.
If FE ever sends keyName but omits imageUrl, you’ll set amateurShow.posterImageUrl to null (Line 210-211). If poster URL is required for reads, consider requiring imageUrl when keyName is provided (or derive URL server-side).
🤖 Prompt for AI Agents
In
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java
around lines 201-211, the code updates amateurShow.posterImageUrl with
dto.getImageUrl() even when dto.getKeyName() is present but imageUrl is null,
which can wipe out the stored URL; either validate and require dto.getImageUrl()
when dto.getKeyName() is provided (return a 4xx error), or else derive/resolve
the image URL server-side before updating and only call
amateurShow.updatePosterImageUrl(...) when a non-null imageUrl is available;
implement one of these fixes and add a unit-test or input validation to prevent
null poster URL updates.
| // 기존 이미지 조회 (filePath + contentId 기준) | ||
| Image existingImage = imageRepository | ||
| .findByFilePathAndContentId(filePath, contentId); | ||
|
|
||
| // 기존 이미지가 있고, keyName이 다르면 삭제 후 새로 저장 | ||
| if (existingImage != null) { | ||
| // keyName 변경이면 S3/DB 모두 삭제 | ||
| if (!existingImage.getKeyName().equals(keyName)) { | ||
| deleteImage(existingImage.getId(), memberId);} | ||
| else {// 같은 keyName이면 DB row만 교체(중복 방지), S3는 유지 | ||
| imageRepository.delete(existingImage); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find ImageRepository interface definition
echo "=== Searching for ImageRepository interface ==="
rg -n "interface ImageRepository|findByFilePathAndContentId" --type=java -A 5
# 2) Find Image entity class
echo "=== Searching for Image entity class ==="
rg -n "class Image\b" --type=java -B 3 -A 30 | head -100
# 3) Search for unique constraints or indexes in entity or annotations
echo "=== Searching for unique constraints/indexes ==="
rg -n "@UniqueConstraint|@Index|uniqueConstraints|@Column.*unique" --type=java -B 2 -A 2Repository: SeeATheater/CC_Backend
Length of output: 8773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for migration files or SQL schema definitions
echo "=== Searching for migrations or schema files ==="
fd -e 'sql' -e 'yml' -e 'yaml' | head -20
echo ""
echo "=== Searching for Flyway/Liquibase migrations ==="
find . -type f \( -name "V*.sql" -o -name "*.sql" \) 2>/dev/null | head -20
echo ""
echo "=== Checking for unique constraint in schema files ==="
rg -n "UNIQUE|unique|constraint" -g '*.sql' -C 2 | grep -i "file_path\|content_id\|image" -C 2 || echo "No matches found"Repository: SeeATheater/CC_Backend
Length of output: 565
Add unique constraint on (filePath, contentId) to prevent duplicate image rows.
findByFilePathAndContentId() at line 195 assumes a single row exists, but the database does not enforce uniqueness on this combination. The @Index(name = "idx_file_content", columnList = "file_path, content_id") on the Image entity is for query performance only and allows duplicates. Additionally, the repository defines both findByFilePathAndContentId() (singular) and findAllByFilePathAndContentId() (plural), indicating duplicates are possible. If duplicates occur, only the first result is returned with undefined ordering, risking data inconsistency.
Add uniqueConstraints = @UniqueConstraint(columnNames = {"file_path", "content_id"}) to the @Table annotation on the Image entity to enforce this constraint at the database level.
🤖 Prompt for AI Agents
In src/main/java/cc/backend/image/service/ImageService.java around lines 193 to
205, the code assumes (filePath, contentId) uniqueness but the DB lacks
enforcement; update the Image entity's @Table annotation (where Image is
defined) to include uniqueConstraints = @UniqueConstraint(columnNames =
{"file_path","content_id"}) so the database enforces uniqueness, create and run
a migration to add that unique constraint (cleaning or deduping existing rows
first as part of the migration), and ensure repository/logic continues to use
singular findByFilePathAndContentId() once the constraint is added.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (1)
237-264: updateNotice: image update may be skipped for newly created notice
When creating a new notice (Lines 243–246),AmateurNotice notice = amateurShow.getAmateurNotice()(Line 253) can still be null depending on how the association is managed, so the image update can silently skip. Prefer carrying the saved/new notice reference through and using that forcontentId.private void updateNotice(AmateurShow amateurShow, AmateurUpdateRequestDTO.UpdateNotice noticeDTO) { + AmateurNotice notice = null; if (noticeDTO != null) { AmateurNotice existing = amateurShow.getAmateurNotice(); if (existing != null) { existing.update(noticeDTO); + notice = existing; } else { AmateurNotice newNotice = AmateurConverter.toAmateurNoticeEntity(noticeDTO, amateurShow); if (newNotice != null) { - amateurNoticeRepository.save(newNotice); + notice = amateurNoticeRepository.save(newNotice); } } } //NoticeImage 수정 if (noticeDTO == null) return; - - AmateurNotice notice = amateurShow.getAmateurNotice(); ImageRequestDTO.NoticeImageRequestDTO dto = noticeDTO.getNoticeImageRequestDTO(); if (notice != null && dto != null && dto.getKeyName() != null && !dto.getKeyName().isBlank()){ imageService.updateShowImage( amateurShow.getMember().getId(), dto.getKeyName(), Optional.ofNullable(dto.getImageUrl()), notice.getId(), FilePath.notice ); } }
♻️ Duplicate comments (3)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (3)
79-85: Fix potential NPE: poster DTO can be null; also consider AFTER_COMMIT for NewShowEvent
requestDTO.getPosterImageRequestDTO()can be null, causing an NPE at Line 82. Also, publishingNewShowEventinside the transaction can be observed before commit if any listener is synchronous/reads DB.ImageRequestDTO.PosterImageRequestDTO dto = requestDTO.getPosterImageRequestDTO(); //poster 이미지는 없으면 에러 -if(dto.getKeyName() == null || dto.getKeyName().isBlank()){ +if (dto == null || dto.getKeyName() == null || dto.getKeyName().isBlank()) { throw new GeneralException(ErrorStatus.INVALID_S3_KEY); }Recommended: ensure the event listener uses
@TransactionalEventListener(phase = AFTER_COMMIT)(or equivalent) if it depends on committed data.Also applies to: 93-105
200-211: Poster update can wipe stored URL when imageUrl is null
If FE sendskeyNamewithoutimageUrl, Line 210 setsposterImageUrlto null. Either requireimageUrlwhenkeyNameis present, or only update the field whenimageUrlis non-null.if (dto != null && dto.getKeyName() != null && !dto.getKeyName().isBlank()) { imageService.updateShowImage( memberId, dto.getKeyName(), Optional.ofNullable(dto.getImageUrl()), amateurShow.getId(), FilePath.amateurShow ); - - amateurShow.updatePosterImageUrl(dto.getImageUrl()); + if (dto.getImageUrl() != null && !dto.getImageUrl().isBlank()) { + amateurShow.updatePosterImageUrl(dto.getImageUrl()); + } }
413-416: Fix critical NPE in deleteShow when poster image row is missing
findByFilePathAndContentId(...)can return null; Line 415 will NPE.//포스터 삭제 Image posterImg = imageRepository.findByFilePathAndContentId(FilePath.amateurShow, amateurShowId); -imageService.deleteImage(posterImg.getId(), memberId); +if (posterImg != null) { + imageService.deleteImage(posterImg.getId(), memberId); +}
🧹 Nitpick comments (1)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (1)
119-138: Casting image save: contentId/filePath split looks correct; avoid double-fetching keyName
Logic is fine (skip null/blank key;FilePath.casting,contentId = amateurCasting.getId()). Minor cleanup: reusekeyNamelocal when building DTO to avoid divergence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java(8 hunks)src/main/java/cc/backend/image/service/ImageService.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/cc/backend/image/service/ImageService.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (4)
src/main/java/cc/backend/image/DTO/ImageRequestDTO.java (1)
ImageRequestDTO(12-66)src/main/java/cc/backend/amateurShow/converter/AmateurConverter.java (1)
AmateurConverter(17-281)src/main/java/cc/backend/amateurShow/entity/AmateurNotice.java (1)
Entity(11-41)src/main/java/cc/backend/amateurShow/entity/AmateurShow.java (1)
updatePosterImageUrl(148-150)
🔇 Additional comments (2)
src/main/java/cc/backend/amateurShow/service/amateurShowService/AmateurServiceImpl.java (2)
146-161: Notice image save: null-guard is good; verify noticeImageKeyName is persisted if needed
Good:noticeImageDTOnull-checked before dereference andFilePath.notice,contentId = amateurNotice.getId(). If the domain needsAmateurNotice.noticeImageKeyNamepersisted too, confirm it’s set somewhere (entity update sets it, but enroll converter currently doesn’t).
266-316: updateCasting: good fix to persist new casting before image update; consider cleanup on removals
Nice: new castings are saved first socontentIdisn’t null, and images are keyed byFilePath.casting + castingId.
However, when castings are removed (Lines 308–311), their Image rows/S3 objects aren’t deleted here—verify you won’t leak orphan images on updates (depending on your Image cleanup strategy).
#이슈번호,#이슈번호Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.