-
Notifications
You must be signed in to change notification settings - Fork 0
[#22] 작품 회차 등록 #29
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
base: feature/21
Are you sure you want to change the base?
[#22] 작품 회차 등록 #29
Conversation
|
f-lab-saponin
left a comment
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.
코드작성하시느라 고생하셨습니다!
전반적으로 파일수가 많은데, 가능하면 줄여보면 좋을 것 같습니다.
사용하지 않는 부분은 과감히 제거하구요.
추가로 *Service 에 있는 공개메소드마다 테스트를 만들어주시면 더 안전한 코드를 만들 수 있어보여요.
댓글한번 확인해주세요 감사합니다 👍👍
| @PostMapping("/create") | ||
| public ResponseEntity<EpisodeResponse> publishEpisode(@Valid @RequestBody EpisodeRequest req) { |
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.
사용자, 로그인 구현되어있으니 해당 User 정보를 조회해서 인증을하는 걸 만들면 좋을 것 같습니다 🙂
| public class FileUploadController { | ||
| private final S3PresignedUploadService uploadService; | ||
| private final FileObjectService fileObjectService; | ||
|
|
||
| /** | ||
| * 작품 썸네일 presigned url 요청 | ||
| */ | ||
| @PostMapping("/creation-thumbnails/presigned") | ||
| public ThumbnailPresignedUrlResponse createCreationThumbnailPresignedUrl(@RequestBody CreationThumbnailPresignedRequest req) { | ||
| log.info("작품 썸네일 Presigned PUT 요청 - contentType={}, thumbnailType={}, originalFilename={}", | ||
| req.contentType(), req.thumbnailType(), req.originalFilename()); | ||
|
|
||
| return uploadService.generatePresignedPutUrl(req); | ||
| } |
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.
특히 파일을 업로드하고 조회하는 API 에는 인증이 더 잘 되어있으면 안전할 것 같아요.
| AUTHENTICATION_FAILED(HttpStatus.UNAUTHORIZED, "A006", "토큰 인증에 실패했습니다."), | ||
|
|
||
| // FileObject 관련 에러 | ||
| FILE_NOT_FOUND(HttpStatus.NOT_FOUND, "F001", "존재하는 파일입니다."), |
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.
'존재하지 않는' 으로 변경해주세요 🙂
| @NotEmpty(message = "원고 파일 목록이 비어있습니다.") | ||
| @Size(max = 50, message = "원고는 50장 이하여야 합니다.") | ||
| List<@Valid ManuscriptFileRequest> files | ||
|
|
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.
여기는 50장인데, EpisodeRequest 에서는 200장이네요. 검증이 차이가 있나요?
| @Transactional | ||
| public EpisodeResponse publishEpisode(EpisodeRequest req) { |
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.
여기서는 Episode + ManuscriptImages + EpisodeThumbnails 가 함께 되면 좋아보이는데, 트랜잭션 설정하는건 어떨까요?
| private short safeToShort(Integer value, String fieldName) { | ||
| if (value == null) throw new IllegalArgumentException(fieldName + "가 null입니다."); | ||
| if (value < Short.MIN_VALUE || value > Short.MAX_VALUE) { | ||
| throw new IllegalArgumentException(fieldName + " 범위가 short를 초과합니다: " + value); | ||
| } | ||
| return value.shortValue(); | ||
| } |
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.
short 범위가 작아서, 여기서 파싱을 못한다고 예외를 내기보다 Integer 로 하는게 어떨까요?
| @Transactional // 더티체킹 | ||
| public List<FileObjectResponse> checkAndGetStatus(Long fileObjectId) { | ||
| FileObject original = fileObjectRepository.findById(fileObjectId) |
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.
여긴 조회만 하는 부분인것 같아요. 그래서 필요없을 것 같습니다
| List<FileObject> toInsert = new ArrayList<>(); | ||
|
|
||
| // 3. 6개 모두 없으면 insert 있으면 update | ||
| for (String key : derivedKeys) { | ||
| long sizeBytes = sizeByKey.getOrDefault(key, 0L); | ||
|
|
||
| boolean isMissing = missingSet.contains(key); | ||
| FileObjectStatus status = isMissing ? FileObjectStatus.FAILED : FileObjectStatus.READY; | ||
|
|
||
| FileObject fo = existingMap.get(key); | ||
|
|
||
| if (fo == null) { | ||
| // 없으면 insert | ||
| toInsert.add( | ||
| FileObject.create( | ||
| key, | ||
| original.getOriginalFilename(), | ||
| status, | ||
| original.getContentType(), | ||
| sizeBytes | ||
| ) | ||
| ); | ||
| } else { | ||
| // 있으면 update | ||
| if (status == FileObjectStatus.READY) fo.markReady(); | ||
| else fo.markFailed(); | ||
|
|
||
| fo.markSize(sizeBytes); | ||
| } | ||
| } |
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.
java stream 함수형처릴 한번 해보면 더 보기 깔끔할 것 같습니다
| List<FileObject> findByStorageKeyIn(Collection<String> storageKeys); | ||
| List<FileObject> findByStorageKeyStartingWith(String baseKey); |
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.
쿼리 작성시, 적절한 인덱스를 고려해주세요. 인덱스 역시 히스토리로 어떤 파일에 기록하면 좋아요.
| List<ManuscriptImage> manuscriptImages = req.manuscripts().stream() | ||
| .map(m -> { | ||
| FileObject fo = fileObjectMap.get(m.fileObjectId()); | ||
| short order = safeToShort(m.displayOrder(), "displayOrder"); | ||
| return ManuscriptImage.create(fo, savedEpisode, order); | ||
| }) | ||
| .toList(); | ||
|
|
||
| List<ManuscriptImage> savedManuscripts = manuscriptImageRepository.saveAll(manuscriptImages); |
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.
Episode - ManuscriptImage - FileObject 가 하나에 다 들어있어서 코드가 길어지고, 관리가 어려울 수 있는데 ManuscriptSerivce 도 관련 부분을 분리해놓고 응집시키면 어떨지 의견드립니다.



작업 내용
테스트 내용
추가 필요