Uh oh!
There was an error while loading. Please reload this page.
refactor: 관리자 대학 이미지 업로드 경로 식별자 개선 - #782
Conversation
- 관리자 대학 생성/수정 API에서 multipart로 대학 정보와 이미지 파일을 함께 받도록 변경 - 이미지 URL을 요청 DTO에서 제거하고 서버에서 S3 업로드 결과를 저장하도록 변경 - 영문명 slug와 한글명 hash를 조합해 중복 영문명 간 S3 경로 충돌을 방지 - 업로드 또는 DB 저장 실패 시 새로 업로드된 이미지가 남지 않도록 보상 삭제 처리
- 대학 생성/수정 시 이미지 업로드 URL 저장과 기존 이미지 유지 동작 검증 - 한글명 기반 S3 경로 식별자와 중복 영문명 충돌 방지 검증 - 업로드 실패 보상 삭제와 삭제용 key 사용 여부 검증
🚥 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 docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b029f6fae4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @RequestPart("logoFile") MultipartFile logoFile, | ||
| @RequestPart("backgroundFile") MultipartFile backgroundFile |
There was a problem hiding this comment.
This create endpoint now requires logoFile and backgroundFile in the same multipart request, but application.yml still caps spring.servlet.multipart.max-request-size at 10MB while each individual file is allowed up to 10MB. A logo and background that are each valid on their own, e.g. two ~6MB images, will be rejected by the servlet container before reaching this controller, so admins cannot create a university with two otherwise acceptable images unless the aggregate request limit is raised or uploads remain separate.
Useful? React with 👍 / 👎.
| if (uploadedFile == null) { | ||
| return; | ||
| } | ||
| deleteFile(uploadedFile.deletionKey()); |
There was a problem hiding this comment.
For images at or above the resize threshold, uploadFile stores deletionKey as original/... but returns fileUrl as resize/...webp; this compensation path deletes only the original key. If a later background upload or DB flush fails after the resize Lambda has already produced the resized logo/background, AdminHostUniversityService.deleteUploadedImages leaves the resize/... object orphaned even though the university was not saved. Delete both keys when they differ, or otherwise remove the generated resize key too.
Useful? React with 👍 / 👎.
| ); | ||
| hostUniversityRepository.flush(); | ||
| evictUnivApplyInfoDetailCaches(id); | ||
| return AdminHostUniversityDetailResponse.from(hostUniversity); |
There was a problem hiding this comment.
이미지 교체 성공 시 기존 S3 객체는 삭제하지 않는 걸로 보입니다!
실패 보상 삭제는 잘 들어갔지만, 수정 API에서 새 로고/배경 업로드가 성공하면 DB URL만 교체하고 이전 이미지는 남습니다. 기존 정책이 “고아 파일 허용”이면 괜찮지만, 비용/정리 관점에서는 누수입니다.
이 부분 확인 부탁드립니다!
There was a problem hiding this comment.
확인했습니다. 기존 이미지 교체 성공 후 old S3 객체 삭제는 현재 대학 이미지에는 적용되어 있지 않습니다.
다만 기존 로직을 확인해보니 프로필, 뉴스, 게시글 쪽은 old image 삭제를 수행하고 있지만, 모두 DB 트랜잭션 커밋 전 삭제하고 있습니다. 이 경우 DB 업데이트가 rollback되면 DB는 기존 URL을 유지하는데 S3 객체는 이미 삭제되는 문제가 생길 수 있습니다.
따라서 이 부분은 대학 이미지에만 즉시 추가하기보다, S3 객체 삭제 정책을 공통으로 정리하는 별도 PR에서 처리하는 것이 맞다고 판단했습니다. 후속 작업에서는 기존 이미지 삭제를 AFTER_COMMIT 기준으로 수행하도록 프로필/뉴스/게시글/대학 이미지 교체 로직을 함께 정리하겠습니다.
| backgroundFile, | ||
| UploadPath.ADMIN_UNIVERSITY_BACKGROUND, | ||
| directoryName | ||
| ); |
There was a problem hiding this comment.
이름 변경만 하고 이미지를 안 올리면 URL 경로와 새 이름 기반 디렉터리가 불일치할 수 있을 것 같습니다!
수정 시 이미지가 없으면 기존 URL을 유지합니다. 기능적으로는 자연스럽지만, 이번 PR 제목처럼 “경로 식별자 개선”이 목적이면 운영 데이터에서 같은 대학의 이름과 이미지 경로 식별자가 엇갈릴 수 있을 것으로 보이네요.. 확인 부탁드립니다!
There was a problem hiding this comment.
확인했습니다. 현재 경로 식별자는 이미지 업로드 시점의 저장 경로를 안정적으로 분리하기 위한 값이고 대학명 변경 시 기존 이미지를 자동 이동하지는 않습니다. 이름만 수정하는 경우 기존 이미지 URL을 유지하는 것이 의도된 동작입니다. S3 객체 rename은 실제로 copy+delete 작업이고 DB 커밋 이후 처리해야 하므로 수정 API에 암묵적으로 포함하면 실패 지점이 커질 수 있습니다. 이미지 경로를 새 이름 기준으로 맞추려면 새 이미지를 함께 업로드하도록 운영 정책을 두거나 별도 마이그레이션/정리 작업으로 다루는 편이 안전하다고 봅니다.
There was a problem hiding this comment.
확인했습니다! 그럼 단순 이름 변경에 대해서는 해당 API를 요청하지 않도록 방어로직을 짤 필요는 없는 걸까요? 정책으로 다룬다면 해당 부분에 대해서 명확히 FE 쪽과 협의가 필요해 보이긴 합니다!
There was a problem hiding this comment.
말씀하신 것처럼 FE/운영 정책은 명확히 맞춰야 할 것 같습니다. FE에서는 이름만 변경하는 경우 이미지 파일 없이 수정 API를 호출해도 되고 이미지까지 교체하려는 경우에만 새 파일을 함께 전달하는 방향으로 협의하겠습니다. PR 특이사항에도 이 정책을 명시하겠습니다.
Hexeong
left a comment
There was a problem hiding this comment.
고생하셨습니다! 추가 작업 내용은 까먹지 않게 이슈로 만들어주세요!
Uh oh!
There was an error while loading. Please reload this page.
관련 이슈
작업 내용
관리자 대학 이미지 업로드를 대학 생성/수정 API에 통합
multipart/form-data로 대학 정보 JSON(request)과 로고/배경 이미지 파일을 함께 받도록 변경했습니다.multipart/form-data로 변경하고, 로고/배경 이미지 파일은 선택 입력으로 처리했습니다./file/admin/university/logo,/file/admin/university/background)는 제거했습니다.이유
이미지 URL 요청 필드 제거
AdminHostUniversityCreateRequest,AdminHostUniversityUpdateRequest에서logoImageUrl,backgroundImageUrl을 제거했습니다.HostUniversity이미지 필드에 저장하도록 변경했습니다.이유
대학별 S3 경로 식별자 개선
UploadDirectoryName.fromUniversityNames(englishName, koreanName)을 추가했습니다.이유
HostUniversity.englishName은 unique 제약이 없어 서로 다른 대학이 같은 영문명을 가질 수 있습니다.업로드 실패 보상 삭제 처리
warn로그만 남기고 원래 예외를 유지했습니다.이유
S3 삭제 API 캡슐화
S3Service.deleteFile(String)은 내부 구현으로 유지하고, 외부에서는deleteUploadedFile(UploadedFileUrlResponse)를 사용하도록 변경했습니다.UploadedFileUrlResponse에 내부 삭제용deletionKey를 추가하고, API 응답에는 노출되지 않도록@JsonIgnore를 적용했습니다.이유
S3Service내부로 캡슐화했습니다.테스트 보강
deleteUploadedFile이fileUrl이 아니라deletionKey로 삭제 요청을 보내는지 검증했습니다.이유
특이 사항
multipart/form-data로 변경됩니다.request,logoFile,backgroundFilepart가 필요합니다.requestpart가 필요하고,logoFile,backgroundFilepart는 선택입니다.리뷰 요구사항 (선택)
AdminHostUniversityService가 S3 업로드까지 담당하도록 변경한 결합도가 현재 유스케이스 기준에서 적절한지 확인 부탁드립니다.