[Feat/s3-presigned-url]#51
Conversation
WalkthroughThis pull request integrates AWS S3 support into the project. New dependencies were added to the build configuration for AWS SDK and Spring Cloud AWS. A configuration class was introduced to provide an Amazon S3 client bean with AWS credentials and region setup. Additional components include an enum for response codes, a REST controller for handling image upload requests by generating presigned URLs, a service for interacting with AWS S3, and a request record for image uploads. The application configuration specifies credentials, region, and S3 bucket details. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant CTRL as ImageUploadController
participant SVC as S3Service
participant AWS as AmazonS3
C->>CTRL: POST /api/v1/uploads (with fileName)
CTRL->>SVC: generatePresignedUrl(fileName)
SVC->>AWS: GeneratePresignedUrlRequest (bucket, object key, expiration)
AWS-->>SVC: presigned URL
SVC-->>CTRL: URL
CTRL-->>C: HTTP 201 Created (SuccessResponse with presigned URL)
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
src/main/java/org/runimo/runimo/external/ImageUploadRequest.java (1)
7-10: LGTM with a suggestion for enhanced validationThe record is well-designed and uses appropriate annotations for documentation and basic validation. Consider adding more robust validation to prevent potential security issues like path traversal attacks.
Consider adding an additional validation to ensure the fileName doesn't contain path traversal characters:
@Schema(description = "이미지 업로드 url발급 요청") public record ImageUploadRequest( @Schema(description = "파일명") - @NotEmpty String fileName + @NotEmpty + @Pattern(regexp = "^[^/\\\\:*?\"<>|]+$", message = "Invalid filename") String fileName ) { }src/main/java/org/runimo/runimo/config/S3Config.java (1)
23-31: Consider leveraging Spring Cloud AWS auto-configurationSpring Cloud AWS starter typically provides auto-configuration for AWS clients, making this manual configuration potentially redundant.
If you're using
spring-cloud-aws-starter:3.1.1, verify if this manual configuration is necessary or if it can be replaced by using the auto-configured beans.Check if you're duplicating the auto-configuration provided by Spring Cloud AWS. If manual configuration is needed, consider adding validation and error handling for the injected values:
@Bean public AmazonS3 amazonS3Client() { + if (accessKey == null || accessKey.isEmpty() || + secretKey == null || secretKey.isEmpty() || + region == null || region.isEmpty()) { + throw new IllegalStateException("AWS credentials or region not properly configured"); + } return AmazonS3Client.builder() .withRegion(region) .withCredentials(new AWSStaticCredentialsProvider( new BasicAWSCredentials(accessKey, secretKey) )) .build(); }src/main/java/org/runimo/runimo/external/S3Service.java (2)
18-19: Consider making the S3 path prefix configurable.The "uploads/" prefix is hardcoded in the method. For better flexibility, consider making this configurable via properties.
@Value("${spring.cloud.aws.s3.bucket}") private String bucketName; +@Value("${spring.cloud.aws.s3.upload-prefix:uploads/}") +private String uploadPrefix; public URL generatePresignedUrl(String fileName) { - String objectKey = "uploads/" + UUID.randomUUID() + "_" + fileName; + String objectKey = uploadPrefix + UUID.randomUUID() + "_" + fileName;
25-25: Translate comment to English for consistency.The comment "15분" is in Korean. For consistent code readability, consider using English comments.
- expTimeMillis += 1000 * 60 * 15; // 15분 + expTimeMillis += 1000 * 60 * 15; // 15 minutessrc/main/java/org/runimo/runimo/external/ExternalResponseCode.java (1)
7-7: Consider internationalization for messages.The success messages are currently hardcoded in Korean. For international applications, consider implementing a message source for internationalization.
You could use Spring's
MessageSourceto load messages based on the user's locale:- PRESIGNED_URL_FETCHED(HttpStatus.CREATED, "Presigned URL 발급 성공", "Presigned URL 발급 성공"); + PRESIGNED_URL_FETCHED(HttpStatus.CREATED, "presigned.url.success", "presigned.url.success.log"); @Override public String getClientMessage() { - return this.clientMessage; + return messageSource.getMessage(this.clientMessage, null, LocaleContextHolder.getLocale()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build.gradle(1 hunks)src/main/java/org/runimo/runimo/config/S3Config.java(1 hunks)src/main/java/org/runimo/runimo/external/ExternalResponseCode.java(1 hunks)src/main/java/org/runimo/runimo/external/ImageUploadController.java(1 hunks)src/main/java/org/runimo/runimo/external/ImageUploadRequest.java(1 hunks)src/main/java/org/runimo/runimo/external/S3Service.java(1 hunks)src/main/resources/application.yml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/runimo/runimo/external/ImageUploadController.java (1)
src/main/java/org/runimo/runimo/user/controller/MyPageController.java (1)
RestController(13-28)
🔇 Additional comments (2)
src/main/java/org/runimo/runimo/external/ExternalResponseCode.java (1)
6-38: LGTM! Well-structured implementation of custom response codes.The enum implementation of
CustomResponseCodeis clean and follows good practices. The use of an enum for response codes makes the code more maintainable and less error-prone.src/main/java/org/runimo/runimo/external/ImageUploadController.java (1)
16-20: Well-structured controller with clear API documentation.The controller is well-organized with appropriate Swagger annotations for API documentation. The endpoint follows RESTful practices with proper HTTP status codes.
|
AWS SDK 1.xx 버전이 2025년 12월까지만 지원하는 이유로 업데이트를 진행했습니다. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/main/java/org/runimo/runimo/config/S3Config.java (1)
13-20:⚠️ Potential issueFix property path mismatch in @value annotations.
There's a mismatch between the property paths in the
@Valueannotations and how they're defined in application.yml. This will prevent the values from being injected correctly.Correct the property paths in the
@Valueannotations:-@Value("${spring.cloud.aws.credentials.access-key}") +@Value("${cloud.aws.credentials.access-key}") private String accessKey; -@Value("${spring.cloud.aws.credentials.secret-key}") +@Value("${cloud.aws.credentials.secret-key}") private String secretKey; -@Value("${spring.cloud.aws.region.static}") +@Value("${cloud.aws.region.static}") private String region;src/main/java/org/runimo/runimo/external/ImageUploadController.java (2)
31-41:⚠️ Potential issueAdd request validation, authentication, and error handling.
The current implementation is missing several important aspects for a secure and robust API endpoint: user authentication, input validation, and error handling.
@PostMapping public ResponseEntity<SuccessResponse<String>> upload( + @UserId Long userId, // Get authenticated user ID - @RequestBody ImageUploadRequest request + @Valid @RequestBody ImageUploadRequest request ) { + try { URL presignedUrl = s3Service.generatePresignedUrl(request.fileName()); return ResponseEntity.status(201) .body(SuccessResponse.of( ExternalResponseCode.PRESIGNED_URL_FETCHED, presignedUrl.toExternalForm() )); + } catch (IllegalArgumentException e) { + // Log the error + return ResponseEntity.badRequest() + .body(/* appropriate error response */); + } catch (Exception e) { + // Log the error + return ResponseEntity.status(500) + .body(/* appropriate error response */); + } }Also, ensure your
ImageUploadRequestrecord includes proper validation:public record ImageUploadRequest( @NotBlank(message = "File name is required") @Pattern(regexp = ".*\\.(jpg|jpeg|png|gif)$", message = "Only image files (jpg, jpeg, png, gif) are allowed") String fileName, @NotBlank(message = "Content type is required") @Pattern(regexp = "image/(jpeg|png|gif|svg\\+xml)", message = "Invalid image content type") String contentType, @NotNull(message = "File size is required") @Max(value = 5242880, message = "File size must not exceed 5MB") Long fileSize ) {}
16-20: 🛠️ Refactor suggestionController setup is good, but missing security annotations.
The controller setup with API documentation looks good, but based on other controllers in your codebase, authentication mechanisms should be in place.
Consider adding security annotations to restrict access:
@Tag(description = "파일 업로드 관련 API", name = "FILE UPLOAD") @RestController @RequestMapping("/api/v1/uploads") @RequiredArgsConstructor +@SecurityRequirement(name = "bearerAuth") // If you're using OpenAPI with security public class ImageUploadController {
🧹 Nitpick comments (3)
build.gradle (1)
47-51: AWS SDK integration looks good, but consider updating to dedicated S3 starter.The AWS SDK 2.x dependencies are correctly added with a specified BOM version (2.20.56), which is a good practice. The Spring Cloud AWS starter is also included.
For better dependency management, consider using the S3-specific starter instead of the generic one:
// infra implementation(platform("software.amazon.awssdk:bom:2.20.56")) implementation("software.amazon.awssdk:s3") -implementation 'io.awspring.cloud:spring-cloud-aws-starter:3.1.1' +implementation 'io.awspring.cloud:spring-cloud-aws-starter-s3:3.1.1'src/main/java/org/runimo/runimo/config/S3Config.java (1)
22-30: S3Presigner implementation looks good but lacks error handling.The S3Presigner bean is correctly implemented using the builder pattern with region and credentials. However, consider adding error handling for cases where the credentials or region might be invalid.
Add try-catch block to handle credential initialization errors:
@Bean public S3Presigner amazonS3Client() { + try { return S3Presigner.builder() .region(Region.of(region)) .credentialsProvider( () -> software.amazon.awssdk.auth.credentials.AwsBasicCredentials.create(accessKey, secretKey) ) .build(); + } catch (IllegalArgumentException e) { + throw new IllegalStateException("Failed to initialize S3Presigner: " + e.getMessage(), e); + } }src/main/java/org/runimo/runimo/external/S3Service.java (1)
39-44: Helper method for GET requests won't be needed after switching to PUT.If you switch to using PUT requests for uploads as suggested above, you won't need this helper method since we'll directly create the PutObjectRequest. If you keep it, consider adding null validation for parameters.
If keeping this method:
private GetObjectRequest createGetObjectRequest(String bucketName, String objectKey) { + if (bucketName == null || objectKey == null) { + throw new IllegalArgumentException("Bucket name and object key cannot be null"); + } return GetObjectRequest.builder() .bucket(bucketName) .key(objectKey) .build(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build.gradle(1 hunks)src/main/java/org/runimo/runimo/config/S3Config.java(1 hunks)src/main/java/org/runimo/runimo/external/ImageUploadController.java(1 hunks)src/main/java/org/runimo/runimo/external/S3Service.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/org/runimo/runimo/external/ImageUploadController.java (1)
src/main/java/org/runimo/runimo/user/controller/MyPageController.java (1)
RestController(13-28)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/resources/application.yml (1)
61-70: AWS S3 Configuration IntegrationThe new
cloud.awssection is well structured and clearly defines the necessary parameters for AWS S3 integration. The use of environment variable placeholders (${AWS_ACCESS_KEY},${AWS_SECRET_KEY},${AWS_REGION},${AWS_S3_BUCKET_NAME}) ensures that sensitive credentials are not hard-coded, which aligns with best practices. Please verify that these property names match those expected in yourS3Configclass and that the necessary environment variables are set in your test and deployment environments. Additionally, you might consider adding inline comments to document that these values are dynamically injected from the environment, which can aid future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/runimo/runimo/config/S3Config.java(1 hunks)src/main/java/org/runimo/runimo/external/S3Service.java(1 hunks)src/main/resources/application.yml(1 hunks)src/test/resources/application.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/resources/application.yml
- src/main/java/org/runimo/runimo/config/S3Config.java
- src/main/java/org/runimo/runimo/external/S3Service.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/runimo/runimo/external/S3Service.java (1)
24-36:⚠️ Potential issueAdd input validation and error handling.
The method should validate the input parameter and handle potential AWS exceptions.
public URL generatePresignedUrl(String fileName) { + if (fileName == null || fileName.trim().isEmpty()) { + throw new IllegalArgumentException("File name cannot be null or empty"); + } + + try { String objectKey = "uploads/" + UUID.randomUUID() + "_" + fileName; PutObjectPresignRequest presignRequest = PutObjectPresignRequest.builder() .signatureDuration(Duration.ofMinutes(10)) // The URL expires in 10 minutes. .putObjectRequest(createPutObjectRequest(bucketName, objectKey)) .build(); PresignedPutObjectRequest presignedRequest = s3Presigner.presignPutObject(presignRequest); log.info("Presigned URL: [{}]", presignedRequest.url().toString()); log.debug("HTTP method: [{}]", presignedRequest.httpRequest().method()); return presignedRequest.url(); + } catch (Exception e) { + log.error("Error generating presigned URL: {}", e.getMessage(), e); + throw new RuntimeException("Failed to generate presigned URL", e); + } }
🧹 Nitpick comments (2)
src/main/java/org/runimo/runimo/external/S3Service.java (2)
24-36: Consider returning additional metadata with the presigned URL.The current implementation only returns the URL. It might be helpful to return additional information such as the generated object key or expiration time to the client.
-public URL generatePresignedUrl(String fileName) { +public PresignedUrlResponse generatePresignedUrl(String fileName) { String objectKey = "uploads/" + UUID.randomUUID() + "_" + fileName; PutObjectPresignRequest presignRequest = PutObjectPresignRequest.builder() .signatureDuration(Duration.ofMinutes(10)) // The URL expires in 10 minutes. .putObjectRequest(createPutObjectRequest(bucketName, objectKey)) .build(); PresignedPutObjectRequest presignedRequest = s3Presigner.presignPutObject(presignRequest); log.info("Presigned URL: [{}]", presignedRequest.url().toString()); log.debug("HTTP method: [{}]", presignedRequest.httpRequest().method()); - return presignedRequest.url(); + return new PresignedUrlResponse( + presignedRequest.url(), + objectKey, + presignedRequest.expiration() + ); } // Create this class in an appropriate location +public record PresignedUrlResponse(URL url, String objectKey, Instant expiration) {}
38-43: Consider adding content-type and other metadata options.The current implementation doesn't specify content-type or other metadata for the uploaded files, which might be important for proper handling by browsers or downstream systems.
private PutObjectRequest createPutObjectRequest(String bucketName, String objectKey) { return PutObjectRequest.builder() .bucket(bucketName) .key(objectKey) + .contentType(determineContentType(objectKey)) .build(); } +/** + * Determine the content type based on file extension. + * This is important for browsers to properly handle the file. + */ +private String determineContentType(String objectKey) { + String extension = objectKey.substring(objectKey.lastIndexOf('.') + 1).toLowerCase(); + return switch (extension) { + case "jpg", "jpeg" -> "image/jpeg"; + case "png" -> "image/png"; + case "gif" -> "image/gif"; + case "pdf" -> "application/pdf"; + default -> "application/octet-stream"; + }; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/runimo/runimo/external/ExternalResponseCode.java(1 hunks)src/main/java/org/runimo/runimo/external/S3Service.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/runimo/runimo/external/ExternalResponseCode.java
🔇 Additional comments (1)
src/main/java/org/runimo/runimo/external/S3Service.java (1)
21-22:Details
❓ Verification inconclusive
Make sure the property path matches your application configuration.
The
@Valueannotation refers tocloud.aws.s3.bucket, so ensure this property is correctly defined in your application.yml or application.properties file.
🏁 Script executed:
#!/bin/bash # Check if the property is defined in application configuration files echo "Looking for S3 bucket configuration in application files..." fd -t f "application.*yml" -x cat {} | grep -n "cloud.aws.s3.bucket" fd -t f "application.*properties" -x cat {} | grep -n "cloud.aws.s3.bucket"Length of output: 277
Re-check the S3 Bucket Configuration in Your App Files
The code in
src/main/java/org/runimo/runimo/external/S3Service.javauses the propertycloud.aws.s3.bucketvia the@Valueannotation. However, our initial automated search did not return any occurrences of this property in your application configuration files (e.g.,application.ymlorapplication.properties). Please manually verify that the property is correctly defined in your configuration—if it’s missing, add it to ensure proper functionality.
- File:
src/main/java/org/runimo/runimo/external/S3Service.java(Lines 21-22)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/org/runimo/runimo/external/ExternalResponseCode.java (1)
6-41: Consider consolidating response code conventions across the project.
As noted in previous reviews, there are multiple variations of response code fields. Adopting a unified naming convention or class structure for these enums or constants can simplify maintenance and improve clarity.Would you like me to outline a refactoring approach for standardizing these code fields?
src/main/java/org/runimo/runimo/external/S3Service.java (1)
50-54: Filename sanitization approach is reasonable.
Replacing unsafe characters with underscores helps mitigate potential security issues like path traversal.Consider logging or tracking the original filename for debugging purposes (e.g., user naming habits, potential repeated conflicts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java(2 hunks)src/main/java/org/runimo/runimo/external/ExternalResponseCode.java(1 hunks)src/main/java/org/runimo/runimo/external/ExternalServiceException.java(1 hunks)src/main/java/org/runimo/runimo/external/S3Service.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java (2)
src/main/java/org/runimo/runimo/external/ExternalServiceException.java (1)
ExternalServiceException(6-15)src/main/java/org/runimo/runimo/common/response/ErrorResponse.java (1)
ErrorResponse(5-22)
src/main/java/org/runimo/runimo/external/S3Service.java (1)
src/main/java/org/runimo/runimo/external/ExternalServiceException.java (1)
ExternalServiceException(6-15)
🔇 Additional comments (7)
src/main/java/org/runimo/runimo/exceptions/GlobalExceptionHandler.java (2)
10-10: No issues with the new import.
133-139: Looks good!
Having a dedicated handler forExternalServiceExceptionincreases clarity and maintainability. The mapping of the exception to its corresponding HTTP status and error response is properly implemented.src/main/java/org/runimo/runimo/external/ExternalServiceException.java (1)
6-14: Nicely structured custom exception.
The protected constructor and static factory method simplify exception creation, and the approach ensures consistent usage ofCustomResponseCode.src/main/java/org/runimo/runimo/external/S3Service.java (4)
24-39: Robust logic for generating presigned PUT URLs.
- The try-catch block is appropriate for error handling, throwing a meaningful external service exception.
- The default 10-minute expiration is reasonable.
- The unique object key naming pattern is good, but ensure it aligns with your desired upload path.
41-48: Good filename validation.
Checks for null or empty strings and enforces a maximum length. This step helps prevent edge-case errors downstream.
56-61: Concise helper method for building a PutObjectRequest.
This improves readability by keeping the main method uncluttered. No issues noted.
21-22:Details
❓ Verification inconclusive
Confirm bucket property path.
Make sure"${cloud.aws.s3.bucket}"aligns with yourapplication.ymlproperty. If your config referencesspring.cloud.aws.s3.bucketinstead, adjust accordingly.
🏁 Script executed:
#!/bin/bash # Searching for the s3 bucket property reference in yml files. fd -e yml --exec cat {} | rg 's3'Length of output: 51
Action: Confirm Correct S3 Bucket Property Path
The service currently injects the value using
"${cloud.aws.s3.bucket}", but your YAML files only reveal ans3:section without clear evidence of whether the full key is undercloud.aws.s3.bucketorspring.cloud.aws.s3.bucket. Please verify manually in your YAML configuration that the hierarchical key matches the property injected inS3Service.java(lines 21–22). If the YAML defines the bucket underspring.cloud.aws.s3.bucket, update the annotation accordingly.
2 participants
작업내역
#37
AWS 계정 만들기
AWS S3 버킷 생성
AWS S3 IAM, key 생성
AWS starter 의존성을 추가했습니다.
Pre-signed url을 발급받는 로직을 추가했습니다.
POST /api/v1/uploads다음 작업
Summary by CodeRabbit