Conversation
AWS S3 버킷 세부 정보 및 이미지 크기 제약 조건을 관리하기 위한 구성 클래스를 소개합니다. 이러한 속성은 외부 구성을 통해 관리되므로 코드를 수정하지 않고도 유연성과 조정 용이성을 제공합니다.
이미지 구성 및 S3 속성을 별도의 클래스로 이동하여 관리 및 테스트 편의성을 향상합니다. 동적 크기 조절을 위한 이미지 크기 구성을 도입합니다. 단일 WebP 업로드를 더욱 유연한 크기 기반 방식으로 대체하여 여러 크기를 한 번에 생성할 수 있습니다.
단일 및 다중 WebP 변환에 대한 이미지 업로드 엔드포인트를 간소화하여 이미지 크기를 지정할 수 있습니다. 이미지 삭제 엔드포인트를 개선하여 삭제 성공 시 콘텐츠를 반환하지 않습니다. API 엔드포인트 디자인을 개선하여 명확성과 일관성을 높였습니다.
|
Caution Review failedThe pull request is closed. Walkthrough이미지 업로드 서비스 리팩토링으로, 설정 주입 방식을 개선하고 WebP 이미지 크기 관련 메서드 시그니처를 확장했습니다. 새로운 설정 클래스(AwsS3Properties, ImageProperties)를 도입하고 컨트롤러 응답 처리를 정규화하며 이미지 크기를 매개변수로 전달하는 구조로 변경했습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
Comment |
github-project-automation
bot
moved this from In progress
to Done
in WeGo-Together Backend
There was a problem hiding this comment.
Pull request overview
This PR refactors the image upload functionality by simplifying endpoints and introducing dynamic image sizing for WebP conversions. The main goal is to improve API design clarity, maintainability, and flexibility in handling various image dimensions.
Key Changes:
- Replaced fixed-size WebP conversion with configurable size-based endpoints that accept width and height parameters
- Introduced configuration classes (AwsS3Properties, ImageProperties) to externalize S3 and image constraints instead of using @value annotations
- Simplified API responses by removing redundant success messages and using 204 No Content for deletions
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ImageController.java | Refactored endpoints to support dynamic sizing; replaced message-based responses with cleaner API design; removed thumbnail endpoint |
| ImageUploadService.java | Replaced @value injections with Properties classes; consolidated WebP conversion methods to support multiple sizes; removed deprecated thumbnail upload |
| ImageSize.java | New record to encapsulate width and height for image dimensions |
| ImageProperties.java | Configuration class for image constraints (max size, dimensions) |
| AwsS3Properties.java | Configuration class for AWS S3 bucket details |
| ApiResponse.java | Changed Boolean.TRUE to primitive true for consistency |
| image-api.http | Updated test cases to reflect new endpoint signatures and parameter requirements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (widths.size() != heights.size()) { | ||
| // 여기서 어떻게 에러를 내려줄지는 프로젝트의 공통 에러 응답 정책에 맞춰서 조정해도 됨 | ||
| throw new IllegalArgumentException("widths와 heights의 길이가 일치해야 합니다."); | ||
| } |
There was a problem hiding this comment.
The error message is in Korean while other parts of the codebase may be in English. Consider internationalizing error messages or maintaining consistency with the project's language standard. Also, IllegalArgumentException should be replaced with a custom ImageException to maintain consistency with the existing error handling pattern (e.g., ImageExceptionCode).
| import lombok.Data; | ||
| import org.springframework.boot.context.properties.ConfigurationProperties; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Data |
There was a problem hiding this comment.
Using Lombok's @DaTa on a configuration properties class can expose setters unnecessarily. Consider using @Getter instead, or the more specific @ConfigurationProperties annotations pattern. Configuration properties should typically be immutable after binding.
| import lombok.Data; | |
| import org.springframework.boot.context.properties.ConfigurationProperties; | |
| import org.springframework.stereotype.Component; | |
| @Data | |
| import lombok.Getter; | |
| import org.springframework.boot.context.properties.ConfigurationProperties; | |
| import org.springframework.stereotype.Component; | |
| @Getter |
| import lombok.Data; | ||
| import org.springframework.boot.context.properties.ConfigurationProperties; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Data |
There was a problem hiding this comment.
Using Lombok's @DaTa on a configuration properties class can expose setters unnecessarily. Consider using @Getter instead to make the configuration immutable after initial binding.
| import lombok.Data; | |
| import org.springframework.boot.context.properties.ConfigurationProperties; | |
| import org.springframework.stereotype.Component; | |
| @Data | |
| import lombok.Getter; | |
| import org.springframework.boot.context.properties.ConfigurationProperties; | |
| import org.springframework.stereotype.Component; | |
| @Getter |
| private byte[] convertToWebpWithSize(MultipartFile file, ImageSize size) { | ||
| try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream()) { | ||
| Thumbnails.of(file.getInputStream()) | ||
| .size(size.width(), size.height()) | ||
| .outputFormat("webp") | ||
| .toOutputStream(byteArrayOutputStream); |
There was a problem hiding this comment.
The convertToWebpWithSize method lacks documentation explaining its behavior. Specifically, it should document that Thumbnails.size() maintains aspect ratio by default, which may not produce exact dimensions if the source image has a different aspect ratio. This could lead to unexpected results when precise dimensions are required.
| @@ -0,0 +1,4 @@ | |||
| package team.wego.wegobackend.image.domain; | |||
|
|
|||
| public record ImageSize(int width, int height) { | |||
There was a problem hiding this comment.
The ImageSize record lacks validation for width and height parameters. Consider adding a compact constructor to validate that width and height are positive values to prevent invalid image dimensions.
| public record ImageSize(int width, int height) { | |
| public record ImageSize(int width, int height) { | |
| public ImageSize { | |
| if (width <= 0) { | |
| throw new IllegalArgumentException("Width must be positive, got: " + width); | |
| } | |
| if (height <= 0) { | |
| throw new IllegalArgumentException("Height must be positive, got: " + height); | |
| } | |
| } |
1 participant
📝 Pull Request
📌 PR 종류
해당하는 항목에 체크해주세요.
✨ 변경 내용
단일 및 다중 WebP 변환에 대한 이미지 업로드 엔드포인트를 간소화하여 이미지 크기를 지정할 수 있습니다. 이미지 삭제 엔드포인트를 개선하여 삭제 성공 시 콘텐츠를 반환하지 않습니다. API 엔드포인트 디자인을 개선하여 명확성과 일관성을 높였습니다.
이미지 구성 및 S3 속성을 별도의 클래스로 이동하여 관리 및 테스트 편의성을 향상합니다. 동적 크기 조절을 위한 이미지 크기 구성을 도입합니다. 단일 WebP 업로드를 더욱 유연한 크기 기반 방식으로 대체하여 여러 크기를 한 번에 생성할 수 있습니다.
AWS S3 버킷 세부 정보 및 이미지 크기 제약 조건을 관리하기 위한 구성 클래스를 소개합니다. 이러한 속성은 외부 구성을 통해 관리되므로 코드를 수정하지 않고도 유연성과 조정 용이성을 제공합니다.
🔍 관련 이슈
🧪 테스트
변경된 기능에 대한 테스트 범위 또는 테스트 결과를 작성해주세요.
🚨 확인해야 할 사항 (Checklist)
PR을 제출하기 전에 아래 항목들을 확인해주세요.
🙋 기타 참고 사항
리뷰어가 참고하면 좋을 만한 추가 설명이 있다면 적어주세요.
Summary by CodeRabbit
변경 사항
새로운 기능
API 개선
✏️ Tip: You can customize this high-level summary in your review settings.