-
Notifications
You must be signed in to change notification settings - Fork 8
refactor: 관리자 대학 이미지 업로드 경로 식별자 개선 #782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,25 +18,33 @@ | |
| import com.example.solidconnection.location.country.repository.CountryRepository; | ||
| import com.example.solidconnection.location.region.domain.Region; | ||
| import com.example.solidconnection.location.region.repository.RegionRepository; | ||
| import com.example.solidconnection.s3.domain.UploadDirectoryName; | ||
| import com.example.solidconnection.s3.domain.UploadPath; | ||
| import com.example.solidconnection.s3.dto.UploadedFileUrlResponse; | ||
| import com.example.solidconnection.s3.service.S3Service; | ||
| import com.example.solidconnection.university.domain.HostUniversity; | ||
| import com.example.solidconnection.university.repository.HostUniversityRepository; | ||
| import com.example.solidconnection.university.repository.UnivApplyInfoRepository; | ||
| import java.util.List; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.data.domain.Page; | ||
| import org.springframework.data.domain.Pageable; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.transaction.annotation.Transactional; | ||
| import org.springframework.web.multipart.MultipartFile; | ||
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
| @Slf4j | ||
| public class AdminHostUniversityService { | ||
|
|
||
| private final HostUniversityRepository hostUniversityRepository; | ||
| private final CountryRepository countryRepository; | ||
| private final RegionRepository regionRepository; | ||
| private final UnivApplyInfoRepository univApplyInfoRepository; | ||
| private final CustomCacheManager cacheManager; | ||
| private final S3Service s3Service; | ||
|
|
||
| @Transactional(readOnly = true) | ||
| public Page<AdminHostUniversityResponse> getHostUniversities( | ||
|
|
@@ -65,29 +73,51 @@ public AdminHostUniversityDetailResponse getHostUniversity(Long id) { | |
| cacheManager = "customCacheManager", | ||
| prefix = true | ||
| ) | ||
| public AdminHostUniversityDetailResponse createHostUniversity(AdminHostUniversityCreateRequest request) { | ||
| public AdminHostUniversityDetailResponse createHostUniversity( | ||
| AdminHostUniversityCreateRequest request, | ||
| MultipartFile logoFile, | ||
| MultipartFile backgroundFile | ||
| ) { | ||
| validateKoreanNameNotExists(request.koreanName()); | ||
|
|
||
| Country country = findCountryByCode(request.countryCode()); | ||
| Region region = findRegionByCode(request.regionCode()); | ||
| String directoryName = UploadDirectoryName.fromUniversityNames(request.englishName(), request.koreanName()); | ||
| UploadedFileUrlResponse logoImage = null; | ||
| UploadedFileUrlResponse backgroundImage = null; | ||
|
|
||
| HostUniversity hostUniversity = new HostUniversity( | ||
| null, | ||
| request.koreanName(), | ||
| request.englishName(), | ||
| request.formatName(), | ||
| request.homepageUrl(), | ||
| request.englishCourseUrl(), | ||
| request.accommodationUrl(), | ||
| request.logoImageUrl(), | ||
| request.backgroundImageUrl(), | ||
| request.detailsForLocal(), | ||
| country, | ||
| region | ||
| ); | ||
| try { | ||
| logoImage = uploadUniversityImage( | ||
| logoFile, | ||
| UploadPath.ADMIN_UNIVERSITY_LOGO, | ||
| directoryName | ||
| ); | ||
| backgroundImage = uploadUniversityImage( | ||
| backgroundFile, | ||
| UploadPath.ADMIN_UNIVERSITY_BACKGROUND, | ||
| directoryName | ||
| ); | ||
|
|
||
| HostUniversity savedHostUniversity = hostUniversityRepository.save(hostUniversity); | ||
| return AdminHostUniversityDetailResponse.from(savedHostUniversity); | ||
| HostUniversity hostUniversity = new HostUniversity( | ||
| null, | ||
| request.koreanName(), | ||
| request.englishName(), | ||
| request.formatName(), | ||
| request.homepageUrl(), | ||
| request.englishCourseUrl(), | ||
| request.accommodationUrl(), | ||
| logoImage.fileUrl(), | ||
| backgroundImage.fileUrl(), | ||
| request.detailsForLocal(), | ||
| country, | ||
| region | ||
| ); | ||
| HostUniversity savedHostUniversity = hostUniversityRepository.saveAndFlush(hostUniversity); | ||
| return AdminHostUniversityDetailResponse.from(savedHostUniversity); | ||
| } catch (RuntimeException e) { | ||
| deleteUploadedImages(logoImage, backgroundImage); | ||
| throw e; | ||
| } | ||
| } | ||
|
|
||
| private void validateKoreanNameNotExists(String koreanName) { | ||
|
|
@@ -103,32 +133,97 @@ private void validateKoreanNameNotExists(String koreanName) { | |
| cacheManager = "customCacheManager", | ||
| prefix = true | ||
| ) | ||
| public AdminHostUniversityDetailResponse updateHostUniversity(Long id, AdminHostUniversityUpdateRequest request) { | ||
| public AdminHostUniversityDetailResponse updateHostUniversity( | ||
| Long id, | ||
| AdminHostUniversityUpdateRequest request, | ||
| MultipartFile logoFile, | ||
| MultipartFile backgroundFile | ||
| ) { | ||
| HostUniversity hostUniversity = hostUniversityRepository.findById(id) | ||
| .orElseThrow(() -> new CustomException(UNIVERSITY_NOT_FOUND)); | ||
|
|
||
| validateKoreanNameNotDuplicated(request.koreanName(), id); | ||
|
|
||
| Country country = findCountryByCode(request.countryCode()); | ||
| Region region = findRegionByCode(request.regionCode()); | ||
| String directoryName = UploadDirectoryName.fromUniversityNames(request.englishName(), request.koreanName()); | ||
| UploadedFileUrlResponse logoImage = null; | ||
| UploadedFileUrlResponse backgroundImage = null; | ||
|
|
||
| hostUniversity.update( | ||
| request.koreanName(), | ||
| request.englishName(), | ||
| request.formatName(), | ||
| request.homepageUrl(), | ||
| request.englishCourseUrl(), | ||
| request.accommodationUrl(), | ||
| request.logoImageUrl(), | ||
| request.backgroundImageUrl(), | ||
| request.detailsForLocal(), | ||
| country, | ||
| region | ||
| ); | ||
| try { | ||
| logoImage = uploadUniversityImageIfExists( | ||
| logoFile, | ||
| UploadPath.ADMIN_UNIVERSITY_LOGO, | ||
| directoryName | ||
| ); | ||
| backgroundImage = uploadUniversityImageIfExists( | ||
| backgroundFile, | ||
| UploadPath.ADMIN_UNIVERSITY_BACKGROUND, | ||
| directoryName | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이름 변경만 하고 이미지를 안 올리면 URL 경로와 새 이름 기반 디렉터리가 불일치할 수 있을 것 같습니다!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 확인했습니다. 현재 경로 식별자는 이미지 업로드 시점의 저장 경로를 안정적으로 분리하기 위한 값이고 대학명 변경 시 기존 이미지를 자동 이동하지는 않습니다. 이름만 수정하는 경우 기존 이미지 URL을 유지하는 것이 의도된 동작입니다. S3 객체 rename은 실제로 copy+delete 작업이고 DB 커밋 이후 처리해야 하므로 수정 API에 암묵적으로 포함하면 실패 지점이 커질 수 있습니다. 이미지 경로를 새 이름 기준으로 맞추려면 새 이미지를 함께 업로드하도록 운영 정책을 두거나 별도 마이그레이션/정리 작업으로 다루는 편이 안전하다고 봅니다.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 확인했습니다! 그럼 단순 이름 변경에 대해서는 해당 API를 요청하지 않도록 방어로직을 짤 필요는 없는 걸까요? 정책으로 다룬다면 해당 부분에 대해서 명확히 FE 쪽과 협의가 필요해 보이긴 합니다!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 말씀하신 것처럼 FE/운영 정책은 명확히 맞춰야 할 것 같습니다. FE에서는 이름만 변경하는 경우 이미지 파일 없이 수정 API를 호출해도 되고 이미지까지 교체하려는 경우에만 새 파일을 함께 전달하는 방향으로 협의하겠습니다. PR 특이사항에도 이 정책을 명시하겠습니다. |
||
|
|
||
| evictUnivApplyInfoDetailCaches(id); | ||
| hostUniversity.update( | ||
| request.koreanName(), | ||
| request.englishName(), | ||
| request.formatName(), | ||
| request.homepageUrl(), | ||
| request.englishCourseUrl(), | ||
| request.accommodationUrl(), | ||
| getImageUrlOrDefault(logoImage, hostUniversity.getLogoImageUrl()), | ||
| getImageUrlOrDefault(backgroundImage, hostUniversity.getBackgroundImageUrl()), | ||
| request.detailsForLocal(), | ||
| country, | ||
| region | ||
| ); | ||
| hostUniversityRepository.flush(); | ||
| evictUnivApplyInfoDetailCaches(id); | ||
| return AdminHostUniversityDetailResponse.from(hostUniversity); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이미지 교체 성공 시 기존 S3 객체는 삭제하지 않는 걸로 보입니다!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 확인했습니다. 기존 이미지 교체 성공 후 old S3 객체 삭제는 현재 대학 이미지에는 적용되어 있지 않습니다. 다만 기존 로직을 확인해보니 프로필, 뉴스, 게시글 쪽은 old image 삭제를 수행하고 있지만, 모두 DB 트랜잭션 커밋 전 삭제하고 있습니다. 이 경우 DB 업데이트가 rollback되면 DB는 기존 URL을 유지하는데 S3 객체는 이미 삭제되는 문제가 생길 수 있습니다. 따라서 이 부분은 대학 이미지에만 즉시 추가하기보다, S3 객체 삭제 정책을 공통으로 정리하는 별도 PR에서 처리하는 것이 맞다고 판단했습니다. 후속 작업에서는 기존 이미지 삭제를 AFTER_COMMIT 기준으로 수행하도록 프로필/뉴스/게시글/대학 이미지 교체 로직을 함께 정리하겠습니다. |
||
| } catch (RuntimeException e) { | ||
| deleteUploadedImages(logoImage, backgroundImage); | ||
| throw e; | ||
| } | ||
| } | ||
|
|
||
| return AdminHostUniversityDetailResponse.from(hostUniversity); | ||
| private UploadedFileUrlResponse uploadUniversityImage( | ||
| MultipartFile imageFile, | ||
| UploadPath uploadPath, | ||
| String directoryName | ||
| ) { | ||
| return s3Service.uploadFile(imageFile, uploadPath, directoryName); | ||
| } | ||
|
|
||
| private UploadedFileUrlResponse uploadUniversityImageIfExists( | ||
| MultipartFile imageFile, | ||
| UploadPath uploadPath, | ||
| String directoryName | ||
| ) { | ||
| if (imageFile == null || imageFile.isEmpty()) { | ||
| return null; | ||
| } | ||
| return uploadUniversityImage(imageFile, uploadPath, directoryName); | ||
| } | ||
|
|
||
| private String getImageUrlOrDefault(UploadedFileUrlResponse uploadedImage, String defaultImageUrl) { | ||
| if (uploadedImage == null) { | ||
| return defaultImageUrl; | ||
| } | ||
| return uploadedImage.fileUrl(); | ||
| } | ||
|
|
||
| private void deleteUploadedImages(UploadedFileUrlResponse... uploadedImages) { | ||
| for (UploadedFileUrlResponse uploadedImage : uploadedImages) { | ||
| if (uploadedImage != null) { | ||
| try { | ||
| s3Service.deleteUploadedFile(uploadedImage); | ||
| } catch (RuntimeException deleteException) { | ||
| log.warn( | ||
| "Failed to delete uploaded university image. fileUrl={}", | ||
| uploadedImage.fileUrl(), | ||
| deleteException | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void validateKoreanNameNotDuplicated(String koreanName, Long excludeId) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,12 @@ | ||
| package com.example.solidconnection.s3.dto; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonIgnore; | ||
|
|
||
| public record UploadedFileUrlResponse( | ||
| String fileUrl) { | ||
| String fileUrl, | ||
| @JsonIgnore String deletionKey) { | ||
|
|
||
| public UploadedFileUrlResponse(String fileUrl) { | ||
| this(fileUrl, fileUrl); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This create endpoint now requires
logoFileandbackgroundFilein the same multipart request, butapplication.ymlstill capsspring.servlet.multipart.max-request-sizeat 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 👍 / 👎.