-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(S3PreSignedUrl): S3 파일 업로드 방식을 PreSigned URL로 구현 #159
base: develop
Are you sure you want to change the base?
Changes from 4 commits
f9b4ddf
24dc491
b373c47
8a01a16
1de4c10
4315239
6b79342
c1d39da
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 |
---|---|---|
@@ -0,0 +1,61 @@ | ||
package ussum.homepage.infra.jpa.post.entity; | ||
|
||
import static com.querydsl.core.types.PathMetadataFactory.*; | ||
|
||
import com.querydsl.core.types.dsl.*; | ||
|
||
import com.querydsl.core.types.PathMetadata; | ||
import javax.annotation.processing.Generated; | ||
import com.querydsl.core.types.Path; | ||
import com.querydsl.core.types.dsl.PathInits; | ||
|
||
|
||
/** | ||
* QRightsDetailEntity is a Querydsl query type for RightsDetailEntity | ||
*/ | ||
@Generated("com.querydsl.codegen.DefaultEntitySerializer") | ||
public class QRightsDetailEntity extends EntityPathBase<RightsDetailEntity> { | ||
|
||
private static final long serialVersionUID = 30014936L; | ||
|
||
private static final PathInits INITS = PathInits.DIRECT2; | ||
|
||
public static final QRightsDetailEntity rightsDetailEntity = new QRightsDetailEntity("rightsDetailEntity"); | ||
|
||
public final NumberPath<Long> id = createNumber("id", Long.class); | ||
|
||
public final StringPath major = createString("major"); | ||
|
||
public final StringPath name = createString("name"); | ||
|
||
public final EnumPath<RightsDetailEntity.PersonType> personType = createEnum("personType", RightsDetailEntity.PersonType.class); | ||
|
||
public final StringPath phoneNumber = createString("phoneNumber"); | ||
|
||
public final QPostEntity postEntity; | ||
|
||
public final StringPath studentId = createString("studentId"); | ||
|
||
public QRightsDetailEntity(String variable) { | ||
this(RightsDetailEntity.class, forVariable(variable), INITS); | ||
} | ||
|
||
public QRightsDetailEntity(Path<? extends RightsDetailEntity> path) { | ||
this(path.getType(), path.getMetadata(), PathInits.getFor(path.getMetadata(), INITS)); | ||
} | ||
|
||
public QRightsDetailEntity(PathMetadata metadata) { | ||
this(metadata, PathInits.getFor(metadata, INITS)); | ||
} | ||
|
||
public QRightsDetailEntity(PathMetadata metadata, PathInits inits) { | ||
this(RightsDetailEntity.class, metadata, inits); | ||
} | ||
|
||
public QRightsDetailEntity(Class<? extends RightsDetailEntity> type, PathMetadata metadata, PathInits inits) { | ||
super(type, metadata, inits); | ||
this.postEntity = inits.isInitialized("postEntity") ? new QPostEntity(forProperty("postEntity"), inits.get("postEntity")) : null; | ||
} | ||
|
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package ussum.homepage.application.image.controller; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.http.ResponseEntity; | ||
import org.springframework.web.bind.annotation.*; | ||
import ussum.homepage.application.image.controller.dto.request.FileUploadConfirmRequest; | ||
import ussum.homepage.application.image.controller.dto.request.FileUploadRequest; | ||
import ussum.homepage.application.image.controller.dto.response.PreSignedUrlResponse; | ||
import ussum.homepage.application.image.service.ImageService; | ||
import ussum.homepage.application.post.service.dto.response.postSave.PostFileListResponse; | ||
import ussum.homepage.global.ApiResponse; | ||
|
||
import java.util.List; | ||
|
||
@RestController | ||
@RequiredArgsConstructor | ||
@RequestMapping("/image") | ||
public class ImageController { | ||
|
||
private final ImageService imageService; | ||
|
||
@PostMapping("/presigned-url") | ||
public ResponseEntity<ApiResponse<?>> getPreSignedUrls( | ||
@RequestParam(value = "userId") Long userId, | ||
@RequestParam(value = "boardCode") String boardCode, | ||
@RequestBody FileUploadRequest request | ||
) { | ||
return ApiResponse.success( | ||
imageService.createPreSignedUrls( | ||
userId, | ||
boardCode, | ||
request.files(), | ||
request.images() | ||
) | ||
); | ||
} | ||
|
||
@PostMapping("/confirm") | ||
public ResponseEntity<ApiResponse<?>> confirmUpload( | ||
@RequestParam(value = "userId") Long userId, | ||
@RequestBody List<FileUploadConfirmRequest> confirmRequests | ||
) { | ||
return ApiResponse.success( | ||
imageService.confirmUpload(userId, confirmRequests) | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package ussum.homepage.application.image.controller.dto.request; | ||
|
||
public record FileRequest( | ||
String fileName, | ||
String contentType | ||
) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package ussum.homepage.application.image.controller.dto.request; | ||
|
||
public record FileUploadConfirmRequest( | ||
String fileUrl, | ||
String fileType, | ||
String originalFileName | ||
) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
package ussum.homepage.application.image.controller.dto.request; | ||
|
||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
public record FileUploadRequest( | ||
List<FileRequest> files, | ||
List<FileRequest> images | ||
) { | ||
public static FileUploadRequest of(List<FileRequest> files, List<FileRequest> images) { | ||
return new FileUploadRequest( | ||
files != null ? files : Collections.emptyList(), | ||
images != null ? images : Collections.emptyList() | ||
); | ||
} | ||
|
||
// null check를 위한 기본 생성자 | ||
public FileUploadRequest { | ||
files = files != null ? files : Collections.emptyList(); | ||
images = images != null ? images : Collections.emptyList(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package ussum.homepage.application.image.controller.dto.response; | ||
|
||
import java.util.List; | ||
import java.util.Map; | ||
|
||
public record PreSignedUrlResponse( | ||
List<Map<String, String>> preSignedUrls, | ||
List<String> originalFileNames | ||
) { | ||
public static PreSignedUrlResponse of(List<Map<String, String>> preSignedUrls, List<String> originalFileNames) { | ||
return new PreSignedUrlResponse(preSignedUrls, originalFileNames); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
package ussum.homepage.application.image.service; | ||
|
||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
import ussum.homepage.application.image.controller.dto.request.FileRequest; | ||
import ussum.homepage.application.image.controller.dto.request.FileUploadConfirmRequest; | ||
import ussum.homepage.application.image.controller.dto.response.PreSignedUrlResponse; | ||
import ussum.homepage.application.post.service.dto.response.postSave.PostFileListResponse; | ||
import ussum.homepage.application.post.service.dto.response.postSave.PostFileResponse; | ||
import ussum.homepage.domain.post.PostFile; | ||
import ussum.homepage.domain.post.service.PostFileAppender; | ||
import ussum.homepage.infra.utils.S3PreSignedUrlUtils; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Comparator; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
import java.util.stream.Collectors; | ||
|
||
@Service | ||
@RequiredArgsConstructor | ||
@Transactional(readOnly = true) | ||
public class ImageService { | ||
private final S3PreSignedUrlUtils s3PreSignedUrlUtils; | ||
private final PostFileAppender postFileAppender; | ||
|
||
public PreSignedUrlResponse createPreSignedUrls( | ||
Long userId, | ||
String boardCode, | ||
List<FileRequest> files, | ||
List<FileRequest> images | ||
) { | ||
List<Map<String, String>> preSignedUrls = new ArrayList<>(); | ||
List<String> originalFileNames = new ArrayList<>(); | ||
|
||
// 이미지 파일 처리 | ||
if (images != null && !images.isEmpty()) { | ||
List<Map<String, String>> imageUrls = s3PreSignedUrlUtils.generatePreSignedUrlWithPath( | ||
userId, | ||
boardCode, | ||
images.stream().map(FileRequest::fileName).collect(Collectors.toList()), | ||
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. 여기 어차피 우리 자바17이어서 바로 toList() 어때여 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. 넵 stream.toList()로 하면 수정 불가능한 리스트를 반환하고 수정이 필요 없다면 toList()를 사용하는게 좋을 것 같습니다! |
||
images.stream().map(FileRequest::contentType).collect(Collectors.toList()), | ||
"images" | ||
); | ||
preSignedUrls.addAll(imageUrls); | ||
originalFileNames.addAll(images.stream().map(FileRequest::fileName).collect(Collectors.toList())); | ||
} | ||
|
||
// 일반 파일 처리 | ||
if (files != null && !files.isEmpty()) { | ||
List<Map<String, String>> fileUrls = s3PreSignedUrlUtils.generatePreSignedUrlWithPath( | ||
userId, | ||
boardCode, | ||
files.stream().map(FileRequest::fileName).collect(Collectors.toList()), | ||
files.stream().map(FileRequest::contentType).collect(Collectors.toList()), | ||
"files" | ||
); | ||
preSignedUrls.addAll(fileUrls); | ||
originalFileNames.addAll(files.stream().map(FileRequest::fileName).collect(Collectors.toList())); | ||
} | ||
|
||
return PreSignedUrlResponse.of(preSignedUrls, originalFileNames); | ||
} | ||
|
||
@Transactional | ||
public PostFileListResponse confirmUpload(Long userId, List<FileUploadConfirmRequest> confirmRequests) { | ||
List<PostFile> postFiles = confirmRequests.stream() | ||
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. S3PreSignedUrlUtils.java 클래스에
이 메소드를 추가해서 ImageService의 confirmUpload 메소드에서 builder 들어가기전에, 한번 S3 객체 존재 여부를 확인하는건 어떻게 생각하시나여?
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.
이 방식에서 더 나아가서 존재 여부 확인 메서드 같은 경우에는 네크워크 요청이기에 다수의 파일일때 블로킹으로 인한 자원 낭비가 있기 때문에, 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. 그렇네요! doesObjectExist 메서드는 S3 파일 존재 여부를 확인하는 간단한 방식으로 구현되어 있지만, 코멘트 주신 대로 다수의 파일을 처리할 때는 네트워크 요청이 블로킹 방식으로 동작하여 비효율적일 수 있을 것 같네여!!;; 말씀하신 시스템이 작더라도 확장성과 효율성을 고려해 비동기 방식(CompletableFuture)으로 리팩토링하여 네트워크 요청의 병렬 처리를 지원하는 것은 좋은 방식인 것 같습니다!! |
||
.map(request -> PostFile.builder() | ||
.url(request.fileUrl()) | ||
.typeName(request.fileType()) | ||
.build()) | ||
.collect(Collectors.toList()); | ||
|
||
List<PostFile> afterSaveList = postFileAppender.saveAllPostFile(postFiles); | ||
|
||
String thumbnailUrl = afterSaveList.stream() | ||
.filter(postFile -> postFile.getTypeName().equals("images")) | ||
.min(Comparator.comparing(PostFile::getId)) | ||
.map(PostFile::getUrl) | ||
.orElse(null); | ||
|
||
AtomicInteger index = new AtomicInteger(0); | ||
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. thread safe!!👍 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. AtomicInteger는 멀티스레드 환경을 위한 것인데, 여기서는 불필요한 오버헤드일 수 있다는 생각이 들어서, "0부터 리스트 크기만큼 순회하면서 매핑한다"는 의도로 작성했습니다. 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. AtomicInteger가 멀티스레드 환경에서 유용하다는 점에는 동의합니다. 하지만, 현재 코드에서는 단일 스레드 환경에서만 동작하므로 오버헤드가 발생할 수 있는 점을 고려해, 말씀하신 IntStream 방식으로 처리하는 것이 저도 더 적합하다고 생각이 드네요.. |
||
List<PostFileResponse> postFileResponses = afterSaveList.stream() | ||
.map(postFile -> { | ||
int currentIndex = index.getAndIncrement(); | ||
return PostFileResponse.of( | ||
postFile.getId(), | ||
postFile.getUrl(), | ||
confirmRequests.get(currentIndex).originalFileName() | ||
); | ||
}) | ||
.collect(Collectors.toList()); | ||
|
||
return PostFileListResponse.of(thumbnailUrl, postFileResponses); | ||
} | ||
} |
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.
이 Map<String,String> 대신 명확한 의미를 가진 record 사용하면 타입 안정성과 가독성을 향상시킬 수 있을 것 같은데 어떻게 생각해???
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.
명시적 도메인 객체 생성해서 관리하도록 수정하는게 좋을 것 같네요!!