[OT-309] [FEAT]: 멀티 파트 업로드 구현#165
Merged
arlen02-01 merged 10 commits intodevelopfrom Mar 14, 2026
Merged
Conversation
Walkthrough멀티파트 업로드 기능 추가: 파트 presigned URL 조회 및 업로드 완료 엔드포인트·DTO 도입, 업로드 응답을 단일 URL에서 multipart 메타데이터( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant UploadHelper
participant S3PresignService
rect rgba(100,150,255,0.5)
Note over Client,Controller: 파트 URL 조회
Client->>Controller: GET /{id}/upload/parts?objectKey=&uploadId=&page=&size=
Controller->>Service: get*OriginUploadPartUrls(id, objectKey, uploadId, page, size[, auth])
Service->>UploadHelper: validateOriginObjectKey(objectKey, expectedOriginUrl, ...)
Service->>UploadHelper: getMultipartPartUrls(objectKey, uploadId, totalPartCount, page, size)
UploadHelper->>S3PresignService: createUploadPartPresignedUrl(objectKey, uploadId, partNumber)
S3PresignService-->>UploadHelper: presigned URL
UploadHelper-->>Service: PageResponse<MultipartUploadPartUrl>
Service-->>Controller: SuccessResponse<PageResponse>
Controller-->>Client: 200 OK
end
rect rgba(150,200,100,0.5)
Note over Client,Controller: 멀티파트 업로드 완료
Client->>Controller: POST /{id}/upload/complete {objectKey, uploadId, parts}
Controller->>Service: complete*OriginUpload(id, objectKey, uploadId, parts[, auth])
Service->>UploadHelper: validateOriginObjectKey(...)
Service->>UploadHelper: completeMultipartUpload(objectKey, uploadId, partETags)
UploadHelper->>S3PresignService: completeMultipartUpload(objectKey, uploadId, partETags)
S3PresignService-->>UploadHelper: void
UploadHelper-->>Service: void
Service-->>Controller: SuccessResponse<Void>
Controller-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 unit tests (beta)
📝 Coding Plan
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 |
Contributor
Author
|
@CodeRabbit 이 pr에 대해 컨밴션 지켰는지와 코드리뷰 수행 했는지 여부 알려줘 안했으면 코드리뷰 바로 해줘 리뷰는 각각을 따로 코맨트로 남겨 한번에 남기지 말고 |
🧩 Analysis chain🏁 Script executed: Length of output: 60836
컨벤션 준수 여부:
코드 리뷰 수행 여부:
지금 바로 코드 리뷰를 각각 별도 코멘트로 진행합니다. 🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java (1)
261-288: 컬렉션 변수명List접미사 권장 및 로직 확인코딩 가이드라인에 따라
partETags→partETagList,normalizedParts→normalizedPartList로 변경을 권장합니다.정렬 및 중복 검증 로직은 올바르게 구현되어 있습니다. S3는 파트가 정렬된 순서로 전달되어야 하므로
sorted()호출이 필수적입니다.♻️ 변수명 개선 제안
public void completeMultipartUpload( String objectKey, String uploadId, - List<MultipartPartETag> partETags + List<MultipartPartETag> partETagList ) { - if (!StringUtils.hasText(objectKey) || !StringUtils.hasText(uploadId) || partETags == null || partETags.isEmpty()) { + if (!StringUtils.hasText(objectKey) || !StringUtils.hasText(uploadId) || partETagList == null || partETagList.isEmpty()) { throw new BusinessException(ErrorCode.ETAG_LIST_INVALID); } - List<MultipartPartETag> normalizedParts = partETags.stream() + List<MultipartPartETag> normalizedPartList = partETagList.stream() .sorted(Comparator.comparingInt(MultipartPartETag::partNumber)) .toList();As per coding guidelines: "Collection variable names with
Listsuffix".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java` around lines 261 - 288, Rename the collection variables to follow the "List" suffix guideline: change the method parameter partETags to partETagList and the local variable normalizedParts to normalizedPartList; update all usages inside completeMultipartUpload including the validation block, the sorting/stream pipeline, the seenPartNumbers checks, and the final s3PresignService.completeMultipartUpload(...) mapping (including MultipartPartETag references) so identifiers remain consistent and compilation succeeds.apps/api-admin/src/main/java/com/ott/api_admin/upload/dto/request/MultipartUploadCompleteRequest.java (1)
22-24: 컬렉션 변수명에List접미사 권장코딩 가이드라인에 따르면 컬렉션 변수명에는
List접미사를 사용해야 합니다.parts→partList로 변경을 권장합니다.♻️ 제안된 변경
`@Schema`(description = "업로드된 파트 eTag 목록") `@NotEmpty` - List<@Valid `@NotNull` PartETagRequest> parts + List<@Valid `@NotNull` PartETagRequest> partList ) {As per coding guidelines: "Collection variable names with
Listsuffix".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/upload/dto/request/MultipartUploadCompleteRequest.java` around lines 22 - 24, Rename the collection field parts in MultipartUploadCompleteRequest to partList to follow the "List" suffix guideline: update the field declaration List<@Valid `@NotNull` PartETagRequest> parts → partList, and then rename all related symbols/usages (getters/setters, constructors, builders, equals/hashCode, Jackson/Swagger annotations if present, and any references across the codebase) to use partList so serialization/deserialization and compilation remain correct.apps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsController.java (1)
77-104: 새 엔드포인트에@Override어노테이션이 누락되었습니다.
BackOfficeContentsApi인터페이스에completeContentsUpload와getContentsUploadPartUrls메서드가 선언되어 있으므로, 기존 메서드들과의 일관성을 위해@Override어노테이션을 추가하세요.💡 수정 제안
+ `@Override` `@PostMapping`("/{contentsId}/upload/complete") public ResponseEntity<SuccessResponse<Void>> completeContentsUpload( ... + `@Override` `@GetMapping`("/{contentsId}/upload/parts") public ResponseEntity<SuccessResponse<PageResponse<MultipartUploadPartUrlResponse>>> getContentsUploadPartUrls(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsController.java` around lines 77 - 104, Add the missing `@Override` annotations to the controller methods that implement the BackOfficeContentsApi: annotate completeContentsUpload(...) and getContentsUploadPartUrls(...) in BackOfficeContentsController with `@Override` to keep consistency with other implemented interface methods and ensure the compiler/linker checks the implementations against BackOfficeContentsApi.apps/api-admin/src/main/java/com/ott/api_admin/shortform/dto/response/ShortFormUploadResponse.java (1)
34-35: Schema example 값에 불필요한 텍스트가 포함되어 있습니다.
example값에"16777216 <-(16mb)"처럼 설명 텍스트가 포함되어 있어, Swagger UI에서 잘못된 예시로 표시됩니다. 숫자 값만 남기거나description에 설명을 추가하세요.💡 수정 제안
- `@Schema`(type = "Long", description = "파트별 크기", example = "16777216 <-(16mb)") + `@Schema`(type = "Long", description = "파트별 크기 (16MB = 16777216 bytes)", example = "16777216") long originPartSizeBytes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/dto/response/ShortFormUploadResponse.java` around lines 34 - 35, The `@Schema` annotation on the field originPartSizeBytes in ShortFormUploadResponse includes explanatory text inside the example value; change the annotation so example contains only the numeric string (e.g. "16777216") and, if you need the "(16mb)" explanation, move that text into the description property (e.g. append " (16 MB)" to the description) so Swagger shows a valid numeric example and keeps the explanation in description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/controller/BackOfficeShortFormController.java`:
- Around line 94-110: Add the two new endpoints to the BackOfficeShortFormApi
interface so the controller correctly implements the contract: declare methods
matching the controller signatures for completeShortFormUpload (taking Long
shortformId, MultipartUploadCompleteRequest request, Authentication
authentication and returning ResponseEntity<SuccessResponse<Void>>) and
getShortFormUploadPartUrls (signature matching the controller's existing
method), then update BackOfficeShortFormController to mark both methods with
`@Override` (specifically annotate completeShortFormUpload and
getShortFormUploadPartUrls). Ensure method signatures in the interface exactly
match the controller (including parameter types like
MultipartUploadCompleteRequest, Authentication and return types) so compilation
succeeds.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java`:
- Line 297: In BackOfficeShortFormService, guard against ShortForm.videoSize
being null before calling uploadHelper.calculateMultipartPartCount; check
shortForm.getVideoSize() and if null throw a clear, specific exception (e.g.,
IllegalArgumentException or your domain ValidationException) with a message like
"videoSize is required for multipart calculation" or handle a sensible default,
then pass the non-null value to uploadHelper.calculateMultipartPartCount so
callers get an explicit, actionable error instead of the vague INVALID_INPUT
downstream.
In
`@modules/infra-s3/src/main/java/com/ott/infra/s3/config/S3PresignerConfig.java`:
- Around line 22-28: There is a duplicate s3Client `@Bean` defined in both
S3ClientConfig and S3PresignerConfig causing BeanDefinitionOverrideException;
remove the s3Client bean from S3PresignerConfig and let the single S3Client bean
live in S3ClientConfig, then update S3PresignerConfig to accept S3Client (the
shared S3Client bean) via constructor or method injection for any presigner
setup (e.g., use the S3Client parameter in the presigner `@Bean` method).
---
Nitpick comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsController.java`:
- Around line 77-104: Add the missing `@Override` annotations to the controller
methods that implement the BackOfficeContentsApi: annotate
completeContentsUpload(...) and getContentsUploadPartUrls(...) in
BackOfficeContentsController with `@Override` to keep consistency with other
implemented interface methods and ensure the compiler/linker checks the
implementations against BackOfficeContentsApi.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/dto/response/ShortFormUploadResponse.java`:
- Around line 34-35: The `@Schema` annotation on the field originPartSizeBytes in
ShortFormUploadResponse includes explanatory text inside the example value;
change the annotation so example contains only the numeric string (e.g.
"16777216") and, if you need the "(16mb)" explanation, move that text into the
description property (e.g. append " (16 MB)" to the description) so Swagger
shows a valid numeric example and keeps the explanation in description.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/upload/dto/request/MultipartUploadCompleteRequest.java`:
- Around line 22-24: Rename the collection field parts in
MultipartUploadCompleteRequest to partList to follow the "List" suffix
guideline: update the field declaration List<@Valid `@NotNull` PartETagRequest>
parts → partList, and then rename all related symbols/usages (getters/setters,
constructors, builders, equals/hashCode, Jackson/Swagger annotations if present,
and any references across the codebase) to use partList so
serialization/deserialization and compilation remain correct.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java`:
- Around line 261-288: Rename the collection variables to follow the "List"
suffix guideline: change the method parameter partETags to partETagList and the
local variable normalizedParts to normalizedPartList; update all usages inside
completeMultipartUpload including the validation block, the sorting/stream
pipeline, the seenPartNumbers checks, and the final
s3PresignService.completeMultipartUpload(...) mapping (including
MultipartPartETag references) so identifiers remain consistent and compilation
succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b13aadd-1b96-4423-be7e-291165423196
📒 Files selected for processing (15)
apps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsApi.javaapps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsController.javaapps/api-admin/src/main/java/com/ott/api_admin/content/dto/response/ContentsUploadResponse.javaapps/api-admin/src/main/java/com/ott/api_admin/content/mapper/BackOfficeContentsMapper.javaapps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/controller/BackOfficeShortFormController.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/dto/response/ShortFormUploadResponse.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/mapper/BackOfficeShortFormMapper.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.javaapps/api-admin/src/main/java/com/ott/api_admin/upload/dto/request/MultipartUploadCompleteRequest.javaapps/api-admin/src/main/java/com/ott/api_admin/upload/dto/response/MultipartUploadPartUrlResponse.javaapps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.javamodules/common-web/src/main/java/com/ott/common/web/exception/ErrorCode.javamodules/infra-s3/src/main/java/com/ott/infra/s3/config/S3PresignerConfig.javamodules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
modules/infra-s3/src/main/java/com/ott/infra/s3/config/S3PresignerConfig.java (1)
22-28:⚠️ Potential issue | 🔴 Critical
S3Client빈이 중복 정의돼서 부팅 시점에 깨질 수 있습니다.이전 리뷰와 동일한 이슈인데 아직 남아 있습니다.
modules/infra-s3/src/main/java/com/ott/infra/s3/config/S3ClientConfig.java:10-19에 이미s3Client빈이 있어서, 여기 정의까지 유지하면 기본 설정에서는 중복 bean 정의로 부팅이 실패하고, override가 켜져 있어도 어떤 설정이 실제로 살아남는지 불명확해집니다.S3Client는 기존 config 한 곳에서만 등록하고, 이 클래스는S3Presigner만 맡기는 편이 안전합니다.제안 수정
-import software.amazon.awssdk.services.s3.S3Client; import software.amazon.awssdk.services.s3.presigner.S3Presigner; @@ - - `@Bean` - public S3Client s3Client(`@Value`("${aws.region:ap-northeast-2}") String region) { - return S3Client.builder() - .region(Region.of(region)) - .credentialsProvider(DefaultCredentialsProvider.builder().build()) - .build(); - } }아래 스크립트는 config 디렉터리의
S3Clientbean 정의 수와 bean override 설정 사용 여부를 확인합니다. 기대 결과는public S3Client s3Client(...)정의가 1건이고, override 설정에 의존하지 않는 상태입니다.#!/bin/bash set -euo pipefail echo "=== S3Client bean definitions ===" rg -n --type=java -C2 'public\s+S3Client\s+s3Client\s*\(' modules/infra-s3/src/main/java/com/ott/infra/s3/config echo echo "=== Bean overriding config ===" rg -n 'allow-bean-definition-overriding' .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/infra-s3/src/main/java/com/ott/infra/s3/config/S3PresignerConfig.java` around lines 22 - 28, The S3Client bean is duplicated in S3PresignerConfig: remove the public S3Client s3Client(...) method from S3PresignerConfig and leave S3Client registration only in the existing S3ClientConfig; instead, in S3PresignerConfig expose only the S3Presigner bean (e.g., a method like public S3Presigner s3Presigner(S3Client s3Client, ...)) that accepts the S3Client as a parameter so it uses the single shared S3Client bean from S3ClientConfig; update imports and constructor/parameters in S3PresignerConfig accordingly and do not re-declare DefaultCredentialsProvider/Region there.
🧹 Nitpick comments (3)
modules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java (2)
109-119: 컬렉션 변수명은Listsuffix를 맞춰 주세요.Line 109의
partETags와 Line 111의completedParts는 둘 다List타입이라 가이드와 어긋납니다.partETagList,completedPartList처럼 맞추면 네이밍이 일관됩니다.As per coding guidelines, `**/*.java`: Enforce: - Collection variable names with `List` suffix.제안 수정
- public void completeMultipartUpload(String objectKey, String uploadId, List< MultipartPartETag> partETags) { + public void completeMultipartUpload(String objectKey, String uploadId, List<MultipartPartETag> partETagList) { try { - List<CompletedPart> completedParts = partETags.stream() + List<CompletedPart> completedPartList = partETagList.stream() .map(part -> CompletedPart.builder() .partNumber(part.partNumber()) .eTag(part.eTag()) .build()) .toList(); @@ CompletedMultipartUpload completedMultipartUpload = CompletedMultipartUpload.builder() - .parts(completedParts) + .parts(completedPartList) .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java` around lines 109 - 119, Rename the collection-typed variables in S3PresignService.completeMultipartUpload to follow the List suffix guideline: change the method parameter partETags to partETagList and the local variable completedParts to completedPartList, then update all usages within completeMultipartUpload (including where CompletedPart.builder() maps part.partNumber()/part.eTag() and where CompletedMultipartUpload.builder().parts(...) is called) to use the new names so the API and local variables consistently end with "List".
81-82: 업로드 단계별 예외 타입을 나눠 두는 편이 좋겠습니다.현재
apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java:261-371에서는 이 예외들을 별도로 재매핑하지 않아서, 여기서 모두IllegalStateException으로 감싸면 멀티파트 초기화, 파트 URL 발급, 완료 실패가 동일한INTERNAL_ERROR로 합쳐집니다. 단계별 전용 예외로 나눠 두면 API 매핑과 장애 추적이 훨씬 쉬워집니다.Also applies to: 104-105, 130-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java` around lines 81 - 82, The catch block in S3PresignService (currently catching SdkException and rethrowing IllegalStateException) should be split into stage-specific custom exceptions so callers like UploadHelper can remap them individually: define and throw distinct exceptions such as MultipartInitException, PartPresignException, and MultipartCompleteException from the S3PresignService methods (the methods that initialize multipart upload, generate part presign URLs, and complete multipart upload), preserving the original SdkException as the cause; update the three catch sites (the current catch at the init, presign and complete locations) to throw the corresponding custom exception instead of IllegalStateException so API mapping and error tracking can differentiate failures by stage.apps/api-admin/src/main/java/com/ott/api_admin/upload/dto/response/MultipartUploadPartUrlResponse.java (1)
7-10: @Schema의 type 값을 OpenAPI 표준에 맞게 소문자로 수정하세요.현재 코드의
@Schema(type = "Integer"),@Schema(type = "String")는 OpenAPI Specification을 따르지 않습니다. OpenAPI는 primitive type을integer,string등 소문자로 정의하고 있으며, swagger-core 문서에서도type값은 "OpenAPI Specification의 유효한 타입이어야 한다"고 명시합니다. 대소문자를 잘못 사용하면 생성기에 따라 무시되거나 잘못 반영될 수 있으므로,type필드를 생략하거나integer,string으로 수정하세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/upload/dto/response/MultipartUploadPartUrlResponse.java` around lines 7 - 10, Summary: The `@Schema` type values use incorrect capitalization; change them to OpenAPI primitive names. Update the `@Schema` on the partNumber field in MultipartUploadPartUrlResponse from type = "Integer" to type = "integer" (or remove the type attribute) and update the `@Schema` for the presigned URL field from type = "String" to type = "string" (or remove the type attribute) so they follow the OpenAPI specification; ensure you modify the annotations on the fields referenced as partNumber and the presigned-URL response field in the MultipartUploadPartUrlResponse class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java`:
- Around line 137-144: prepareMediaCreate(...) starts the S3 multipart upload
inside the DB transaction (via UploadHelper.MediaCreateUploadResult) which can
leave an orphaned upload if subsequent DB operations like linkTags(...) or
commit fail; move the multipart-init call (prepareMediaCreate) to execute before
the transaction begins or add a compensation path that aborts the multipart
upload on transaction rollback/exception by capturing the returned uploadId from
UploadHelper.MediaCreateUploadResult and calling the helper's abort/cleanup
method in the catch/finally (or a transaction rollback listener) within
BackOfficeContentsService so S3 uploads are always aborted when the DB does not
commit.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java`:
- Around line 211-217: 현재 트랜잭션 내부에서 uploadHelper.prepareMediaCreate(...)가 S3
multipart uploadId를 생성하므로 DB 커밋 실패 시 multipart가 고아가 됩니다; 해결하려면
prepareMediaCreate(...) 호출을 트랜잭션 밖으로 옮기거나 트랜잭션 실패 시 보상으로 multipart를 abort하도록
추가하세요 — 구체적으로 BackOfficeShortFormService에서
UploadHelper.MediaCreateUploadResult(내부의 uploadId)를 얻는 로직을 트랜잭션 시작 이전으로 이동하거나,
inheritOriginMediaTags(...)나 DB 작업이 실패할 경우 catch/transaction synchronization
블록에서 uploadHelper.abortMultipart(uploadId) 또는 해당 abort 메서드를 호출해 S3 multipart를
명시적으로 종료하도록 처리하세요.
---
Duplicate comments:
In
`@modules/infra-s3/src/main/java/com/ott/infra/s3/config/S3PresignerConfig.java`:
- Around line 22-28: The S3Client bean is duplicated in S3PresignerConfig:
remove the public S3Client s3Client(...) method from S3PresignerConfig and leave
S3Client registration only in the existing S3ClientConfig; instead, in
S3PresignerConfig expose only the S3Presigner bean (e.g., a method like public
S3Presigner s3Presigner(S3Client s3Client, ...)) that accepts the S3Client as a
parameter so it uses the single shared S3Client bean from S3ClientConfig; update
imports and constructor/parameters in S3PresignerConfig accordingly and do not
re-declare DefaultCredentialsProvider/Region there.
---
Nitpick comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/upload/dto/response/MultipartUploadPartUrlResponse.java`:
- Around line 7-10: Summary: The `@Schema` type values use incorrect
capitalization; change them to OpenAPI primitive names. Update the `@Schema` on
the partNumber field in MultipartUploadPartUrlResponse from type = "Integer" to
type = "integer" (or remove the type attribute) and update the `@Schema` for the
presigned URL field from type = "String" to type = "string" (or remove the type
attribute) so they follow the OpenAPI specification; ensure you modify the
annotations on the fields referenced as partNumber and the presigned-URL
response field in the MultipartUploadPartUrlResponse class.
In
`@modules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java`:
- Around line 109-119: Rename the collection-typed variables in
S3PresignService.completeMultipartUpload to follow the List suffix guideline:
change the method parameter partETags to partETagList and the local variable
completedParts to completedPartList, then update all usages within
completeMultipartUpload (including where CompletedPart.builder() maps
part.partNumber()/part.eTag() and where
CompletedMultipartUpload.builder().parts(...) is called) to use the new names so
the API and local variables consistently end with "List".
- Around line 81-82: The catch block in S3PresignService (currently catching
SdkException and rethrowing IllegalStateException) should be split into
stage-specific custom exceptions so callers like UploadHelper can remap them
individually: define and throw distinct exceptions such as
MultipartInitException, PartPresignException, and MultipartCompleteException
from the S3PresignService methods (the methods that initialize multipart upload,
generate part presign URLs, and complete multipart upload), preserving the
original SdkException as the cause; update the three catch sites (the current
catch at the init, presign and complete locations) to throw the corresponding
custom exception instead of IllegalStateException so API mapping and error
tracking can differentiate failures by stage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2e446f1-1638-4629-897a-612d10382d9a
📒 Files selected for processing (15)
apps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsApi.javaapps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsController.javaapps/api-admin/src/main/java/com/ott/api_admin/content/dto/response/ContentsUploadResponse.javaapps/api-admin/src/main/java/com/ott/api_admin/content/mapper/BackOfficeContentsMapper.javaapps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/controller/BackOfficeShortFormController.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/dto/response/ShortFormUploadResponse.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/mapper/BackOfficeShortFormMapper.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.javaapps/api-admin/src/main/java/com/ott/api_admin/upload/dto/request/MultipartUploadCompleteRequest.javaapps/api-admin/src/main/java/com/ott/api_admin/upload/dto/response/MultipartUploadPartUrlResponse.javaapps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.javamodules/common-web/src/main/java/com/ott/common/web/exception/ErrorCode.javamodules/infra-s3/src/main/java/com/ott/infra/s3/config/S3PresignerConfig.javamodules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java (1)
257-261: 컬렉션 변수명에List접미사 사용을 권장합니다.코딩 가이드라인에 따라 컬렉션 변수명에는
List접미사를 사용하는 것이 좋습니다.public void completeMultipartUpload( String objectKey, String uploadId, - List<MultipartPartETag> partETags + List<MultipartPartETag> partETagList ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java` around lines 257 - 261, The parameter name partETags in the method completeMultipartUpload is a collection and should use the List suffix per our naming guideline; rename the parameter to partETagsList (and update every usage inside completeMultipartUpload) and then update all callers/signatures that reference completeMultipartUpload to use the new parameter name so compilation stays consistent.apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java (1)
262-288:@Transactional(readOnly = true)가 의미적으로 혼란을 줄 수 있습니다.
completeShortFormOriginUpload는 S3 멀티파트 업로드를 "완료"하는 작업인데,readOnly = true로 표시되어 있습니다. DB 변경이 없으므로 기술적으로는 맞지만, 외부 시스템(S3)에 상태 변경을 일으키므로 의미적 혼란이 있을 수 있습니다. 트랜잭션이 필요 없다면@Transactional자체를 제거하는 것도 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java` around lines 262 - 288, The `@Transactional`(readOnly = true) on BackOfficeShortFormService.completeShortFormOriginUpload is misleading because the method performs an external S3 state change rather than read-only DB work; remove the `@Transactional` annotation from the completeShortFormOriginUpload method (or replace it with `@Transactional` without readOnly if you intend to allow DB updates later) and clean up any now-unused import for Transactional so the method clearly reflects that it does not participate in a read-only DB transaction.modules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java (1)
110-120: 컬렉션 변수명은List접미사로 맞춰 주세요.Line 110의
partETags, Line 112의completedParts는 둘 다 컬렉션인데 이 저장소 컨벤션의List접미사를 따르지 않습니다.partETagList,completedPartList처럼 맞추면 다른 Java 코드와 이름 규칙이 일관됩니다.♻️ Proposed rename
- public void completeMultipartUpload(String objectKey, String uploadId, List< MultipartPartETag> partETags) { + public void completeMultipartUpload(String objectKey, String uploadId, List<MultipartPartETag> partETagList) { try { - List<CompletedPart> completedParts = partETags.stream() + List<CompletedPart> completedPartList = partETagList.stream() .map(part -> CompletedPart.builder() .partNumber(part.partNumber()) .eTag(part.eTag()) .build()) .toList(); CompletedMultipartUpload completedMultipartUpload = CompletedMultipartUpload.builder() - .parts(completedParts) + .parts(completedPartList) .build();As per coding guidelines, 'Collection variable names with
Listsuffix'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java` around lines 110 - 120, Rename collection variables in completeMultipartUpload to follow the repository convention of using a List suffix: change the method parameter partETags to partETagList and the local variable completedParts to completedPartList, then update all downstream usages (stream, mapping, CompletedMultipartUpload.parts(...) and any callers of completeMultipartUpload) to use the new names so code and API remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsController.java`:
- Around line 76-105: The two controller endpoints completeContentsUpload and
getContentsUploadPartUrls are missing authentication/role checks; add an
Authentication parameter to both controller methods and enforce ADMIN or EDITOR
role before calling BackOfficeContentsService (or delegate to service methods
completeContentsOriginUpload and getContentsOriginUploadPartUrls to perform the
check), mirroring the pattern used in BackOfficeShortFormService; ensure the
service method(s) validate the caller's roles (ADMIN/EDITOR) and throw an
appropriate access exception if unauthorized so back-office routes are protected
per guidelines.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java`:
- Around line 206-237: Add the missing EDITOR permission check at the start of
getContentsOriginUploadPartUrls (same pattern used in
BackOfficeShortFormService.getShortFormOriginUploadPartUrls and as done in
completeContentsOriginUpload) so requests without EDITOR role are rejected
before proceeding; also validate contents.getVideoSize() for null before calling
uploadHelper.getMultipartPartCount(contents.getVideoSize()) and throw a clear
BusinessException (use the appropriate CONTENTS_* error code used elsewhere,
e.g., CONTENTS_VIDEO_SIZE_MISSING) if it is null to avoid ambiguous NPEs.
- Around line 192-204: completeContentsOriginUpload is missing the EDITOR
ownership check present in
BackOfficeShortFormService.completeShortFormOriginUpload; add a permission check
after loading Contents (from contentsRepository.findById) that verifies if the
current principal has role EDITOR and that contents.getCreatedBy (or the
equivalent owner field on Contents) matches the current user's id, otherwise
throw the same BusinessException/ErrorCode used for forbidden actions; reuse the
same authorization utility or method used by
BackOfficeShortFormService.completeShortFormOriginUpload to perform the
role+ownership validation before calling uploadHelper.validateOriginObjectKey
and uploadHelper.completeMultipartUpload.
In
`@modules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java`:
- Around line 110-117: The completeMultipartUpload method currently maps
partETags to CompletedPart preserving client order, which can violate S3's
requirement for parts to be sorted by partNumber; modify completeMultipartUpload
to first sort the incoming List<MultipartPartETag> partETags by
part.partNumber() (ascending) before mapping to
CompletedPart.builder().partNumber(...).eTag(...).build() so the list passed to
the CompleteMultipartUpload call is guaranteed to be in partNumber order
(preventing InvalidPartOrder errors).
---
Nitpick comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java`:
- Around line 262-288: The `@Transactional`(readOnly = true) on
BackOfficeShortFormService.completeShortFormOriginUpload is misleading because
the method performs an external S3 state change rather than read-only DB work;
remove the `@Transactional` annotation from the completeShortFormOriginUpload
method (or replace it with `@Transactional` without readOnly if you intend to
allow DB updates later) and clean up any now-unused import for Transactional so
the method clearly reflects that it does not participate in a read-only DB
transaction.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java`:
- Around line 257-261: The parameter name partETags in the method
completeMultipartUpload is a collection and should use the List suffix per our
naming guideline; rename the parameter to partETagsList (and update every usage
inside completeMultipartUpload) and then update all callers/signatures that
reference completeMultipartUpload to use the new parameter name so compilation
stays consistent.
In
`@modules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java`:
- Around line 110-120: Rename collection variables in completeMultipartUpload to
follow the repository convention of using a List suffix: change the method
parameter partETags to partETagList and the local variable completedParts to
completedPartList, then update all downstream usages (stream, mapping,
CompletedMultipartUpload.parts(...) and any callers of completeMultipartUpload)
to use the new names so code and API remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 13dbe75e-b6a2-4a52-a3a9-eb00f7106f97
📒 Files selected for processing (10)
apps/api-admin/src/main/java/com/ott/api_admin/content/controller/BackOfficeContentsController.javaapps/api-admin/src/main/java/com/ott/api_admin/content/dto/request/ContentsUploadRequest.javaapps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/controller/BackOfficeShortFormApi.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/controller/BackOfficeShortFormController.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/dto/request/ShortFormUploadRequest.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.javaapps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.javamodules/infra-s3/src/main/java/com/ott/infra/s3/config/S3PresignerConfig.javamodules/infra-s3/src/main/java/com/ott/infra/s3/service/S3PresignService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/infra-s3/src/main/java/com/ott/infra/s3/config/S3PresignerConfig.java
- apps/api-admin/src/main/java/com/ott/api_admin/shortform/controller/BackOfficeShortFormController.java
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java (3)
229-229:⚠️ Potential issue | 🟠 Major
videoSizenull 가드를 추가해 명확한 오류를 반환해 주세요.Line 229에서
contents.getVideoSize()를 바로 사용하고 있어 null 데이터에서 런타임 예외(또는 모호한 실패)가 발생할 수 있습니다. null인 경우 전용BusinessException(ErrorCode ...)으로 명확히 차단하는 것이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java` at line 229, Add a null guard for contents.getVideoSize() in BackOfficeContentsService before calling uploadHelper.getMultipartPartCount: check if contents.getVideoSize() == null and if so throw a BusinessException with the appropriate ErrorCode (use the existing ErrorCode enum entry used for invalid/empty input) so that totalPartCount = uploadHelper.getMultipartPartCount(contents.getVideoSize()) never receives null and returns a clear business-level error instead of a runtime NPE.
198-210:⚠️ Potential issue | 🔴 Critical[P0] 업로드 완료 API에 EDITOR 소유권 검증이 없습니다.
현재
completeContentsOriginUpload(...)는 콘텐츠 존재/키 일치만 확인하고 완료를 수행합니다. EDITOR는 본인 소유 콘텐츠에만 접근 가능하도록 소유권 검증이 필요합니다.수정 예시
-@Transactional(readOnly = true) -public void completeContentsOriginUpload(Long contentsId, String objectKey, String uploadId, List<UploadHelper.MultipartPartETag> parts) { - Contents contents = contentsRepository.findById(contentsId) +@Transactional(readOnly = true) +public void completeContentsOriginUpload( + Long contentsId, + String objectKey, + String uploadId, + List<UploadHelper.MultipartPartETag> parts, + Authentication authentication +) { + Contents contents = contentsRepository.findWithMediaAndUploaderById(contentsId) .orElseThrow(() -> new BusinessException(ErrorCode.CONTENTS_NOT_FOUND)); + + Media media = contents.getMedia(); + Long memberId = (Long) authentication.getPrincipal(); + boolean isEditor = authentication.getAuthorities().stream() + .anyMatch(authority -> Role.EDITOR.getKey().equals(authority.getAuthority())); + if (isEditor && !media.getUploader().getId().equals(memberId)) { + throw new BusinessException(ErrorCode.FORBIDDEN); + }As per coding guidelines:
apps/api-admin/**에서는 백오피스 권한/역할 제약을 점검하고, EDITOR 우회 가능성은 P0로 처리해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java` around lines 198 - 210, Add an ownership/role check to completeContentsOriginUpload: after loading Contents (contents variable) and before calling uploadHelper.completeMultipartUpload, resolve the current authenticated user (via your security context/util used elsewhere in the codebase) and verify that either the caller has a backoffice/admin role or, if the caller is an EDITOR, that contents.getOwnerId() (or equivalent ownership field on Contents) equals the current user's id; if the check fails throw a BusinessException with an appropriate error code (e.g., ErrorCode.FORBIDDEN or a CONTENTS_OWNERSHIP_MISMATCH code). Keep the existing uploadHelper.validateOriginObjectKey call, but perform the ownership/role guard immediately after it and before uploadHelper.completeMultipartUpload.
212-243:⚠️ Potential issue | 🔴 Critical[P0] 파트 URL 조회 API에도 EDITOR 소유권 검증이 빠져 있습니다.
getContentsOriginUploadPartUrls(...)역시 소유권 검증 없이 파트 업로드 URL을 발급합니다. 완료 API와 동일한 EDITOR 권한/소유자 검증을 선행해야 합니다.As per coding guidelines:
apps/api-admin/**에서는 백오피스 권한/역할 제약을 점검하고, EDITOR 우회 가능성은 P0로 처리해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java` around lines 212 - 243, getContentsOriginUploadPartUrls currently issues multipart part URLs without verifying EDITOR ownership; after loading Contents (contentsRepository.findById) add the same back-office EDITOR ownership/role validation used in the contents-completion flow (reuse the existing helper/service method rather than duplicating logic — e.g., call the back-office authorization helper used elsewhere such as validateEditorOwnership(contents) or BackOfficeAuthorizationService.checkEditorOwnership(contents) or the exact method used in the completion API), and if validation fails throw the existing BusinessException with the proper ErrorCode (matching other APIs) before calling uploadHelper.getMultipartPartUrls; this ensures the EDITOR owner check runs for getContentsOriginUploadPartUrls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java`:
- Line 229: Add a null guard for contents.getVideoSize() in
BackOfficeContentsService before calling uploadHelper.getMultipartPartCount:
check if contents.getVideoSize() == null and if so throw a BusinessException
with the appropriate ErrorCode (use the existing ErrorCode enum entry used for
invalid/empty input) so that totalPartCount =
uploadHelper.getMultipartPartCount(contents.getVideoSize()) never receives null
and returns a clear business-level error instead of a runtime NPE.
- Around line 198-210: Add an ownership/role check to
completeContentsOriginUpload: after loading Contents (contents variable) and
before calling uploadHelper.completeMultipartUpload, resolve the current
authenticated user (via your security context/util used elsewhere in the
codebase) and verify that either the caller has a backoffice/admin role or, if
the caller is an EDITOR, that contents.getOwnerId() (or equivalent ownership
field on Contents) equals the current user's id; if the check fails throw a
BusinessException with an appropriate error code (e.g., ErrorCode.FORBIDDEN or a
CONTENTS_OWNERSHIP_MISMATCH code). Keep the existing
uploadHelper.validateOriginObjectKey call, but perform the ownership/role guard
immediately after it and before uploadHelper.completeMultipartUpload.
- Around line 212-243: getContentsOriginUploadPartUrls currently issues
multipart part URLs without verifying EDITOR ownership; after loading Contents
(contentsRepository.findById) add the same back-office EDITOR ownership/role
validation used in the contents-completion flow (reuse the existing
helper/service method rather than duplicating logic — e.g., call the back-office
authorization helper used elsewhere such as validateEditorOwnership(contents) or
BackOfficeAuthorizationService.checkEditorOwnership(contents) or the exact
method used in the completion API), and if validation fails throw the existing
BusinessException with the proper ErrorCode (matching other APIs) before calling
uploadHelper.getMultipartPartUrls; this ensures the EDITOR owner check runs for
getContentsOriginUploadPartUrls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67e9f1e5-8a6a-4e33-86a7-f569974875d7
📒 Files selected for processing (1)
apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java
yubin012
reviewed
Mar 14, 2026
Contributor
yubin012
left a comment
yubin012
left a comment
There was a problem hiding this comment.
수고하셨습니다!
필수 수정 사항은 아니지만
개인적으로 업로드 생성, part 계산, presigned URL 생성, 검증 로직 등이 BackOfficeContentsService와 UploadHelper에 함께 모여 있어 역할 구분이 조금 어렵게 느껴졌습니다. 현재 헬퍼 코드만 봐도 도메인 서비스 같은 느낌이 있어서
추후 리팩토링 시 책임을 조금 더 분리하면 가독성이 더 좋아질 것 같습니다!
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java (2)
198-211:⚠️ Potential issue | 🔴 Critical[P0]
completeContentsOriginUpload에 EDITOR 권한 체크가 누락되어 있습니다.
BackOfficeShortFormService.completeShortFormOriginUpload와 달리 이 메서드에는Authentication파라미터와 EDITOR 권한 체크가 없습니다. EDITOR는 자신이 업로드한 콘텐츠만 완료할 수 있어야 합니다.🔒 권한 체크 추가 제안
- `@Transactional`(readOnly = true) - public void completeContentsOriginUpload(Long contentsId, String objectKey, String uploadId, List<UploadHelper.MultipartPartETag> parts) { - Contents contents = contentsRepository.findById(contentsId) + public void completeContentsOriginUpload( + Long contentsId, + String objectKey, + String uploadId, + List<UploadHelper.MultipartPartETag> partList, + Authentication authentication + ) { + Contents contents = contentsRepository.findWithMediaAndUploaderById(contentsId) .orElseThrow(() -> new BusinessException(ErrorCode.CONTENTS_NOT_FOUND)); + + Media media = contents.getMedia(); + Long memberId = (Long) authentication.getPrincipal(); + boolean isEditor = authentication.getAuthorities().stream() + .anyMatch(authority -> Role.EDITOR.getKey().equals(authority.getAuthority())); + if (isEditor && !media.getUploader().getId().equals(memberId)) { + throw new BusinessException(ErrorCode.FORBIDDEN); + } uploadHelper.validateOriginObjectKey(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java` around lines 198 - 211, completeContentsOriginUpload is missing the EDITOR permission and ownership check; modify its signature to accept Authentication (same pattern as BackOfficeShortFormService.completeShortFormOriginUpload), verify the caller has the EDITOR authority/role, then confirm the authenticated user's id matches the content owner (e.g., compare Authentication principal id to contents.getUserId()/contents.getCreatedBy or equivalent on Contents) and throw the appropriate BusinessException (use existing ErrorCode like FORBIDDEN or a new CONTENTS_FORBIDDEN) if checks fail before calling uploadHelper.validateOriginObjectKey and completeMultipartUpload.
213-244:⚠️ Potential issue | 🔴 Critical[P0]
getContentsOriginUploadPartUrls에도 EDITOR 권한 체크가 필요합니다.
BackOfficeShortFormService.getShortFormOriginUploadPartUrls와 동일하게Authentication파라미터와 EDITOR 권한 체크를 추가해야 합니다.추가로, Line 230의
contents.getVideoSize()가 null일 경우getMultipartPartCount에서 예상치 못한 에러가 발생할 수 있습니다. 명시적인 null 체크를 권장합니다.🔒 권한 체크 및 null 검증 추가 제안
`@Transactional`(readOnly = true) public PageResponse<MultipartUploadPartUrlResponse> getContentsOriginUploadPartUrls( Long contentsId, String objectKey, String uploadId, Integer page, - Integer size + Integer size, + Authentication authentication ) { - Contents contents = contentsRepository.findById(contentsId) + Contents contents = contentsRepository.findWithMediaAndUploaderById(contentsId) .orElseThrow(() -> new BusinessException(ErrorCode.CONTENTS_NOT_FOUND)); + Media media = contents.getMedia(); + Long memberId = (Long) authentication.getPrincipal(); + boolean isEditor = authentication.getAuthorities().stream() + .anyMatch(authority -> Role.EDITOR.getKey().equals(authority.getAuthority())); + if (isEditor && !media.getUploader().getId().equals(memberId)) { + throw new BusinessException(ErrorCode.FORBIDDEN); + } + uploadHelper.validateOriginObjectKey( objectKey, contents.getOriginUrl(), ErrorCode.CONTENTS_ORIGIN_OBJECT_KEY_MISMATCH ); + if (contents.getVideoSize() == null) { + throw new BusinessException(ErrorCode.INVALID_INPUT, "영상 크기 정보가 없습니다."); + } int totalPartCount = uploadHelper.getMultipartPartCount(contents.getVideoSize());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java` around lines 213 - 244, Add an Authentication parameter to getContentsOriginUploadPartUrls and enforce the same EDITOR permission check used in BackOfficeShortFormService.getShortFormOriginUploadPartUrls (call the existing permission validator/auth check with the Authentication and contents or user context before proceeding); also add an explicit null check for contents.getVideoSize() before calling uploadHelper.getMultipartPartCount and throw a BusinessException with an appropriate error code if videoSize is null to avoid passing null into uploadHelper.getMultipartPartCount.
🧹 Nitpick comments (3)
apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java (1)
257-299: 컬렉션 변수명에List접미사 추가를 권장합니다.코딩 가이드라인에 따르면 컬렉션 변수명에는
List접미사를 사용해야 합니다.partETags→partETagList로 변경하는 것을 고려해 주세요.검증 로직 자체는 ETag 개수, 파트 번호 범위, 중복 여부를 철저히 검사하고 있어 좋습니다.
♻️ 변수명 수정 제안
public void completeMultipartUpload( String objectKey, String uploadId, int totalPartCount, - List<MultipartPartETag> partETags + List<MultipartPartETag> partETagList ) { // 1) 입력값 기본 유효성 검증 if (!StringUtils.hasText(objectKey) || !StringUtils.hasText(uploadId) || totalPartCount <= 0 - || partETags == null - || partETags.isEmpty()) { + || partETagList == null + || partETagList.isEmpty()) { throw new BusinessException(ErrorCode.ETAG_LIST_INVALID); } - List<MultipartPartETag> normalizedParts = partETags.stream() + List<MultipartPartETag> normalizedPartList = partETagList.stream() .sorted(Comparator.comparingInt(MultipartPartETag::partNumber)) .toList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java` around lines 257 - 299, Rename the collection parameter partETags to partETagList throughout the completeMultipartUpload method (method signature and every usage) to follow the List-suffix naming guideline: update the parameter declaration in UploadHelper.completeMultipartUpload, replace references in the initial null/empty check, the stream sort/normalizedParts assignment, the size comparison, the forEach validation, and the final s3PresignService.completeMultipartUpload stream mapping; also update any callers of UploadHelper.completeMultipartUpload to pass the renamed parameter name where applicable.apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java (2)
287-288:videoSizenull 검증을 명시적으로 추가하는 것을 권장합니다.
shortForm.getVideoSize()가 null일 경우getMultipartPartCount에서INVALID_INPUT에러가 발생하지만, 사용자 입장에서 원인을 파악하기 어렵습니다.♻️ 명시적 검증 추가 제안
+ if (shortForm.getVideoSize() == null) { + throw new BusinessException(ErrorCode.INVALID_INPUT, "영상 크기 정보가 없습니다."); + } int totalPartCount = uploadHelper.getMultipartPartCount(shortForm.getVideoSize());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java` around lines 287 - 288, The call sequence in BackOfficeShortFormService currently passes shortForm.getVideoSize() directly to uploadHelper.getMultipartPartCount which can trigger a generic INVALID_INPUT error deep inside uploadHelper; add an explicit null check for shortForm.getVideoSize() before calling uploadHelper.getMultipartPartCount (in the method around where totalPartCount is computed), and if null throw a clear, domain-appropriate exception (e.g., IllegalArgumentException or the service's BadRequest/InvalidInput exception) with a message like "videoSize is null for shortForm [id]" (and optionally log it) so callers get an actionable error instead of the upstream INVALID_INPUT from getMultipartPartCount; do not call uploadHelper.getMultipartPartCount or uploadHelper.completeMultipartUpload when videoSize is null.
317-318: 동일하게videoSizenull 검증이 필요합니다.Line 287과 동일한 문제입니다.
shortForm.getVideoSize()null 체크를 추가해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java` around lines 317 - 318, The call site in BackOfficeShortFormService uses shortForm.getVideoSize() when calling uploadHelper.getMultipartPartCount and uploadHelper.getMultipartPartUrls without null checks; add a guard identical to the one used earlier (for example the check you added at the previous shortForm.getVideoSize() usage) to verify shortForm.getVideoSize() is not null before calling uploadHelper.getMultipartPartCount(...) and uploadHelper.getMultipartPartUrls(...), and handle the null case consistently (throw or return an error/empty response) so neither uploadHelper.getMultipartPartCount nor getMultipartPartUrls receive a null videoSize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.java`:
- Around line 198-211: completeContentsOriginUpload is missing the EDITOR
permission and ownership check; modify its signature to accept Authentication
(same pattern as BackOfficeShortFormService.completeShortFormOriginUpload),
verify the caller has the EDITOR authority/role, then confirm the authenticated
user's id matches the content owner (e.g., compare Authentication principal id
to contents.getUserId()/contents.getCreatedBy or equivalent on Contents) and
throw the appropriate BusinessException (use existing ErrorCode like FORBIDDEN
or a new CONTENTS_FORBIDDEN) if checks fail before calling
uploadHelper.validateOriginObjectKey and completeMultipartUpload.
- Around line 213-244: Add an Authentication parameter to
getContentsOriginUploadPartUrls and enforce the same EDITOR permission check
used in BackOfficeShortFormService.getShortFormOriginUploadPartUrls (call the
existing permission validator/auth check with the Authentication and contents or
user context before proceeding); also add an explicit null check for
contents.getVideoSize() before calling uploadHelper.getMultipartPartCount and
throw a BusinessException with an appropriate error code if videoSize is null to
avoid passing null into uploadHelper.getMultipartPartCount.
---
Nitpick comments:
In
`@apps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.java`:
- Around line 287-288: The call sequence in BackOfficeShortFormService currently
passes shortForm.getVideoSize() directly to uploadHelper.getMultipartPartCount
which can trigger a generic INVALID_INPUT error deep inside uploadHelper; add an
explicit null check for shortForm.getVideoSize() before calling
uploadHelper.getMultipartPartCount (in the method around where totalPartCount is
computed), and if null throw a clear, domain-appropriate exception (e.g.,
IllegalArgumentException or the service's BadRequest/InvalidInput exception)
with a message like "videoSize is null for shortForm [id]" (and optionally log
it) so callers get an actionable error instead of the upstream INVALID_INPUT
from getMultipartPartCount; do not call uploadHelper.getMultipartPartCount or
uploadHelper.completeMultipartUpload when videoSize is null.
- Around line 317-318: The call site in BackOfficeShortFormService uses
shortForm.getVideoSize() when calling uploadHelper.getMultipartPartCount and
uploadHelper.getMultipartPartUrls without null checks; add a guard identical to
the one used earlier (for example the check you added at the previous
shortForm.getVideoSize() usage) to verify shortForm.getVideoSize() is not null
before calling uploadHelper.getMultipartPartCount(...) and
uploadHelper.getMultipartPartUrls(...), and handle the null case consistently
(throw or return an error/empty response) so neither
uploadHelper.getMultipartPartCount nor getMultipartPartUrls receive a null
videoSize.
In
`@apps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java`:
- Around line 257-299: Rename the collection parameter partETags to partETagList
throughout the completeMultipartUpload method (method signature and every usage)
to follow the List-suffix naming guideline: update the parameter declaration in
UploadHelper.completeMultipartUpload, replace references in the initial
null/empty check, the stream sort/normalizedParts assignment, the size
comparison, the forEach validation, and the final
s3PresignService.completeMultipartUpload stream mapping; also update any callers
of UploadHelper.completeMultipartUpload to pass the renamed parameter name where
applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef1615cc-853c-4a5a-9caf-54498e66da2a
📒 Files selected for processing (3)
apps/api-admin/src/main/java/com/ott/api_admin/content/service/BackOfficeContentsService.javaapps/api-admin/src/main/java/com/ott/api_admin/shortform/service/BackOfficeShortFormService.javaapps/api-admin/src/main/java/com/ott/api_admin/upload/support/UploadHelper.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 작업 내용
📷 스크린샷
[메타 업로드]
[멀티파트 url 생성]
[파트 업로드]
[파트 업로드 완료 요청]
☑️ 체크 리스트
#️⃣ 연관된 이슈
💬 리뷰 요구사항
3개중에 가장 큰 용량을 선택하는 로직이 있습니다. 코드에 최대한 주석 붙여 놓았습니다.
Summary by CodeRabbit
새로운 기능
개선사항
버그 수정