Conversation
|
Caution Review failedThe pull request is closed. 워크스루이미지 업로드 서비스의 디렉토리 기반 키 생성을 제거하고, 새로운 WebP 업로드 메서드를 추가했습니다. 단일 및 다중 크기의 WebP 이미지를 저장하는 기능이 추가되었으며, 디렉토리 유효성 검사 규칙이 완화되었습니다. 변경 사항
예상 코드 리뷰 노력🎯 2 (Simple) | ⏱️ ~10 분
관련 PR
시
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements functionality to store images in the root directory without requiring a subdirectory path. The implementation attempts to preserve backward compatibility through method overloading, adding new methods that don't require the dir parameter while keeping existing methods intact.
- Added overloaded methods
uploadAsWebpWithSizeanduploadAsWebpWithSizeswithoutdirparameter for root directory uploads - Added helper methods
buildKey(String, int)andbuildBaseName()for generating keys without directory paths - Modified HTTP test file to update multipart form boundaries
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/test/http/group/create.http |
Updated multipart form-data boundary separators in HTTP test file (contains syntax errors) |
src/main/java/team/wego/wegobackend/image/presentation/ImageController.java |
Removed unused ArrayList import |
src/main/java/team/wego/wegobackend/image/application/service/ImageUploadService.java |
Added overloaded upload methods for root directory storage, modified existing uploadOriginal method, and partially disabled directory validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Content-Type: multipart/form-data; boundary=boundary | ||
|
|
||
| --boundary | ||
| --boundary-- |
There was a problem hiding this comment.
The boundary separator syntax is incorrect. In multipart/form-data, boundary separators should use --boundary (two hyphens at the start), not --boundary-- (which is only used for the final closing boundary). This will cause the HTTP request to fail or be malformed.
Each part should start with --boundary and only the very last line after all parts should be --boundary--.
|
|
||
| < ../image/resources/test-webp1.webp | ||
| --boundary | ||
| --boundary-- |
There was a problem hiding this comment.
The boundary separator syntax is incorrect. In multipart/form-data, boundary separators should use --boundary (two hyphens at the start), not --boundary-- (which is only used for the final closing boundary). This will cause the HTTP request to fail or be malformed.
Each part should start with --boundary and only the very last line after all parts should be --boundary--.
|
|
||
| < ../image/resources/test-webp1.webp | ||
| --boundary | ||
| --boundary-- |
There was a problem hiding this comment.
The boundary separator syntax is incorrect. In multipart/form-data, boundary separators should use --boundary (two hyphens at the start), not --boundary-- (which is only used for the final closing boundary). This will cause the HTTP request to fail or be malformed.
Each part should start with --boundary and only the very last line after all parts should be --boundary--.
|
|
||
| < ../image/resources/img1.png | ||
| --boundary | ||
| --boundary-- |
There was a problem hiding this comment.
The boundary separator syntax is incorrect. In multipart/form-data, boundary separators should use --boundary (two hyphens at the start), not --boundary-- (which is only used for the final closing boundary). This will cause the HTTP request to fail or be malformed.
Each part should start with --boundary and only the very last line after all parts should be --boundary--.
| // String key = buildKey(dir, originalFilename, index); | ||
| String key = buildKey(originalFilename, index); |
There was a problem hiding this comment.
The uploadOriginal method still validates the dir parameter (line 49) and accepts it as an argument, but then ignores it when building the key (line 56), using the overloaded buildKey(originalFilename, index) method instead. This creates inconsistent behavior where:
- The method still enforces directory validation rules
- But the validated directory is never used
This breaks the existing API contract. Since GroupImageService at line 68-74 still calls uploadAsWebpWithSizes(dir, file, i, widths, heights) with a directory path, and the PR description states method overloading should "preserve existing functionality", this implementation is incorrect.
Either:
- Create a new overloaded method
uploadOriginal(MultipartFile file, int index)without thedirparameter for the new behavior - Or update all callers to use the new API
| // String key = buildKey(dir, originalFilename, index); | |
| String key = buildKey(originalFilename, index); | |
| String key = buildKey(dir, originalFilename, index); |
|
|
||
| String originalFilename = file.getOriginalFilename(); | ||
| String key = buildKey(dir, originalFilename, index); | ||
| // String key = buildKey(dir, originalFilename, index); |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. If this code is needed for reference, it should be documented in a TODO comment or commit message instead. The PR description mentions implementing the feature with method overloading to preserve previous functionality, so the old implementation can be recovered from git history if needed.
| // String key = buildKey(dir, originalFilename, index); |
| // TODO: 모임 경로: FE 요청으로 루트 디렉토리로 개선했습니다. | ||
| // if (dir == null || dir.isBlank()) { | ||
| // throw new ImageException(ImageExceptionCode.DIR_REQUIRED); | ||
| // } | ||
|
|
||
| // if (!dir.matches("[a-zA-Z0-9_\\-/]+")) { | ||
| // throw new ImageException(ImageExceptionCode.DIR_INVALID_PATTERN); | ||
| // } |
There was a problem hiding this comment.
The validateDir method has most of its validation logic commented out (lines 258-264), but still validates for path traversal (line 266-268) and trailing slashes (line 270-272). This creates an inconsistent state where:
- The method is still called by existing methods like
uploadAsWebpWithSizes(String dir, ...)at line 132 - But it only performs partial validation
This makes the code confusing and error-prone. Consider either:
- Removing the commented code if directory validation is no longer needed
- Or creating a separate validation method for the new root-only functionality
The commented-out validation checks are still being called by the existing dir-based methods, so they may cause unexpected behavior.
📝 Pull Request
📌 PR 종류
해당하는 항목에 체크해주세요.
✨ 변경 내용
이미지 경로를 받지 않는 기능을 구현했습니다.
메서드 오버로딩으로 이전 기능을 살리면서, 모든 이미지를 루트에 저장하도록 구현했습니다.
🔍 관련 이슈
🧪 테스트
변경된 기능에 대한 테스트 범위 또는 테스트 결과를 작성해주세요.
🚨 확인해야 할 사항 (Checklist)
PR을 제출하기 전에 아래 항목들을 확인해주세요.
🙋 기타 참고 사항
리뷰어가 참고하면 좋을 만한 추가 설명이 있다면 적어주세요.
Summary by CodeRabbit
릴리스 노트
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.