feat: optimize image #504
feat: optimize image #504
Conversation
📝 WalkthroughWalkthroughAdds configurable image processing and multi-variant storage: images are resized (thumbnail, medium, large) and encoded as WebP and JPEG, uploaded to S3 as six variants, and persisted as a JSON Changes
Sequence DiagramsequenceDiagram
actor Client
participant Controller
participant StoreImageService
participant ImageProcessingService
participant S3
participant Database
Client->>Controller: POST /upload (file)
Controller->>StoreImageService: uploadFile(file)
rect rgb(230,245,255)
note right of StoreImageService: Orchestrate processing
StoreImageService->>ImageProcessingService: processImage(file)
ImageProcessingService->>ImageProcessingService: validate, resize to 3 sizes
ImageProcessingService->>ImageProcessingService: convert each size to WebP & JPEG
ImageProcessingService-->>StoreImageService: ProcessedImageSet (6 byte arrays)
end
rect rgb(245,230,255)
note right of StoreImageService: Upload variants to S3
loop for each variant (6)
StoreImageService->>S3: putObject(key, bytes, contentType)
S3-->>StoreImageService: public URL
end
StoreImageService->>StoreImageService: build ImageUrlsResponse
end
rect rgb(230,255,245)
note right of StoreImageService: Persist metadata
StoreImageService->>Database: save StoreImage (imageUrls JSON)
Database-->>StoreImageService: confirmation
end
StoreImageService-->>Controller: return medium JPEG URL (compat)
Controller-->>Client: 200 OK (response includes imageUrls)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![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 (3)
src/main/kotlin/upbrella/be/store/entity/StoreImage.kt (2)
24-24: Consider extracting ObjectMapper to avoid static state in entity.While functional, having a static ObjectMapper in the entity couples JSON serialization logic to the domain model. Consider injecting ObjectMapper via a utility class or service.
Alternative approach
Create a utility class for JSON operations:
@Component class JsonUtil(private val objectMapper: ObjectMapper) { fun <T> writeValueAsString(value: T): String = objectMapper.writeValueAsString(value) fun <T> readValue(content: String, valueType: Class<T>): T = objectMapper.readValue(content, valueType) }Then use it in a service layer rather than directly in the entity.
16-17: Consider database-appropriate column type.The
TEXTcolumn type may have different behaviors across databases. For MySQL, consider usingLONGTEXTif URLs can exceed 65,535 bytes, or specify aVARCHARwith an explicit length if the JSON structure size is predictable.src/main/kotlin/upbrella/be/store/service/ImageProcessingService.kt (1)
16-31: Unused code:ImageSize.from()is never invoked and may drift out of sync.The
from(configValue: Int)companion function hardcodes the same size values defined in the enum. Since this function isn't used anywhere and the config values are external, consider removing it to avoid maintenance burden.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
build.gradlesrc/main/kotlin/upbrella/be/config/ImageProcessingConfig.ktsrc/main/kotlin/upbrella/be/store/dto/response/ImageUrlsResponse.ktsrc/main/kotlin/upbrella/be/store/dto/response/SingleImageUrlResponse.ktsrc/main/kotlin/upbrella/be/store/entity/StoreImage.ktsrc/main/kotlin/upbrella/be/store/service/ImageProcessingService.ktsrc/main/kotlin/upbrella/be/store/service/StoreImageService.ktsrc/main/resources/application.ymlsrc/test/kotlin/upbrella/be/store/controller/StoreControllerTest.ktsrc/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.kt
🧰 Additional context used
🪛 detekt (1.23.8)
src/main/kotlin/upbrella/be/store/service/StoreImageService.kt
[warning] 130-130: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (16)
src/main/resources/application.yml (1)
5-13: LGTM!The image processing configuration is well-structured with reasonable defaults for sizes and quality settings. The values align with the configuration binding in ImageProcessingConfig.
src/main/kotlin/upbrella/be/store/dto/response/ImageUrlsResponse.kt (1)
1-13: LGTM!The data classes are well-structured for representing multi-format, multi-size image URLs. The nullable
webpfield appropriately supports backward compatibility with existing images that may only have JPEG variants.src/test/kotlin/upbrella/be/store/controller/StoreControllerTest.kt (2)
988-1002: LGTM!Test data properly reflects the new multi-format, multi-size image URL structure with both WebP and JPEG variants across all three sizes.
1033-1054: LGTM!REST documentation is comprehensive and properly documents the new nested
imageUrlsstructure with all format and size variants. The optional markers for WebP fields correctly reflect backward compatibility.src/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.kt (3)
52-53: LGTM!The ImageProcessingService mock is properly integrated into the test setup to support the new multi-variant image processing flow.
68-94: LGTM!The test properly mocks the image processing service and verifies that all six image variants (3 sizes × 2 formats) are uploaded to S3. The assertion on the expected URL correctly validates backward compatibility by returning the medium JPEG URL.
146-190: LGTM!Test data properly includes the complete imageUrls structure with both WebP and JPEG variants, and the test correctly validates that WebP thumbnail is preferred when available.
src/main/kotlin/upbrella/be/store/dto/response/SingleImageUrlResponse.kt (2)
5-9: LGTM!The response structure properly extends the existing API with the new
imageUrlsfield while maintaining backward compatibility through the preservedimageUrlfield. The Korean comments clearly document the intent.
11-18: LGTM!The factory method cleanly delegates to
StoreImage.getImageUrlsResponse()to construct the complete response with both legacy and new image URL structures.src/main/kotlin/upbrella/be/store/service/ImageProcessingService.kt (2)
36-46: LGTM!The image processing flow is well-structured, clearly processing three sizes and returning a cohesive result set.
143-164: Good practice: Properequals/hashCodeimplementation forByteArrayfields.Kotlin data classes don't handle array equality correctly by default. This override using
contentEqualsandcontentHashCodeis the correct approach.src/main/kotlin/upbrella/be/store/service/StoreImageService.kt (5)
32-48: LGTM!The upload flow is well-structured: process → upload → persist → return compatible URL. Maintaining backward compatibility with the medium JPEG return value is a good approach.
50-71: LGTM!The null-safe handling for optional WebP variants and the order of operations (S3 deletions before DB) are appropriate.
97-124: Good rollback pattern for partial upload failures.Tracking uploaded keys and cleaning them up on failure is a solid resilience pattern for multi-file uploads.
73-78: LGTM!WebP-first with JPEG fallback is the right approach for modern browser compatibility.
170-173: TheStoreImage.createStoreImageWithUrlsfactory method exists and correctly handlesImageUrlsResponseby extracting the medium JPEG URL for backward compatibility while serializing the complete response object to JSON for storage.
|
|
||
| // Image Processing | ||
| implementation 'net.coobird:thumbnailator:0.4.20' | ||
| implementation 'org.sejda.imageio:webp-imageio:0.1.6' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking Thumbnailator Latest Version ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:net.coobird+AND+a:thumbnailator&rows=1&wt=json" | jq -r '.response.docs[0] | "\(.latestVersion) - Released: \(.timestamp)"'
echo -e "\n=== Checking webp-imageio Latest Version ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:org.sejda.imageio+AND+a:webp-imageio&rows=1&wt=json" | jq -r '.response.docs[0] | "\(.latestVersion) - Released: \(.timestamp)"'
echo -e "\n=== Checking Thumbnailator Version History ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:net.coobird+AND+a:thumbnailator&rows=20&wt=json" | jq -r '.response.docs[] | "v\(.v) - \(.timestamp)"' | head -10
echo -e "\n=== Checking webp-imageio Version History ==="
curl -s "https://search.maven.org/solrsearch/select?q=g:org.sejda.imageio+AND+a:webp-imageio&rows=20&wt=json" | jq -r '.response.docs[] | "v\(.v) - \(.timestamp)"' | head -10Repository: UPbrella/UPbrella_back
Length of output: 361
🌐 Web query:
Thumbnailator security vulnerabilities CVE 2024 2025
💡 Result:
Short answer: I found CVEs affecting "thumbnail" software in 2024–2025 (notably Matrix Media Repo and several WordPress thumbnail plugins), but I found no CVE entries for the Java Thumbnailator library (coobird/thumbnailator).
Notable CVEs (2024–2025)
- CVE-2024-56515 — Matrix Media Repo (thumbnailers could invoke ImageMagick/ffmpeg decoders; fixed in MMR v1.3.8). [1]
- CVE-2025-53282 — Stored XSS in the "Thumbnail Editor" WordPress plugin. [2]
- CVE-2025-12154 — Arbitrary file upload in the "Auto Thumbnailer" WordPress plugin (authenticated). [3]
No CVE for Java Thumbnailator
- The official Thumbnailator GitHub repository (coobird/thumbnailator) and its security/advisory records show no published CVE/security advisory for the library as of now. [4]
Sources
- CVE-2024-56515 details (Recorded Future / NVD references). [1]
- CVE-2025-53282 advisory summary (Wiz / Patchstack reporting). [2]
- CVE-2025-12154 (NVD / CVE Details summary). [3]
- Thumbnailator (coobird/thumbnailator) GitHub repository — no CVE listed in repo advisories. [4]
If you want, I can:
- search NVD/GHSA/NPM/Maven Central again and return exact NVD/GHSA links and publish dates, or
- monitor for new CVEs for Thumbnailator and notify you. Which would you prefer?
🌐 Web query:
webp-imageio org.sejda.imageio security advisories vulnerabilities
💡 Result:
Summary — webp-imageio (org.sejda.imageio)
- I found no public CVE or vendor security advisory that names org.sejda.imageio/webp-imageio specifically. [1][2]
- The project’s GitHub issues show a few crash/segfault and libwebp-related issues (including an open issue mentioning updating libwebp for CVE-2023-1999), so some security-relevant dependency concerns have been raised upstream. Check the repo issues for details. [3]
- Maven Central metadata lists webp-imageio 0.1.6 (May 2020) and shows dependency-vulnerability metadata (e.g., CVE-2020-15250 appears in the package page), indicating you should also audit its dependencies. [2]
Relevant sources
- Maven / package page for org.sejda.imageio:webp-imageio (artifact, version + vulnerability metadata). [2]
- N/A for org.sejda.imageio-specific CVE — but ImageIO-related CVEs (Java ImageIO component) that are relevant to image-processing libraries: CVE-2020-14562, CVE-2022-21277, CVE-2021-35586 (examples from NVD/Rapid7/Wiz). These are general ImageIO CVEs to be aware of. [4][5][6]
- sejda-pdf/webp-imageio GitHub issues (open issues include crashes and “Update libwebp CVE-2023-1999”). Monitor this repo for fixes. [3]
If you want, I can:
- Run a live dependency scan (OSS advisories/CVE lookup) against a specific webp-imageio version (e.g., 0.1.6) and list any dependency CVEs.
- Watch the GitHub repo and notify you of new security issues.
Address unmaintained webp-imageio dependency.
Thumbnailator 0.4.20 is the latest version (released June 2023) with no known security advisories. However, webp-imageio 0.1.6 has not been updated since May 2020 and appears unmaintained. Its underlying libwebp dependency is outdated and has known vulnerabilities (e.g., CVE-2023-1999). Consider evaluating alternative WebP processing libraries or submitting upstream patches to the sejda project to address the stale dependencies.
🤖 Prompt for AI Agents
In build.gradle around lines 96–99 the project pulls in the unmaintained
org.sejda.imageio:webp-imageio:0.1.6 which relies on an outdated libwebp with
known vulnerabilities; remove this dependency and replace it with a maintained
WebP solution (either a maintained ImageIO WebP plugin or a native library
wrapper) by: (1) researching and selecting a maintained WebP processing library
or using a native toolchain (e.g., a maintained imageio-webp fork or a JNI
binding to libwebp), (2) updating build.gradle to remove the sejda:webp-imageio
entry and add the chosen dependency, (3) adapting image loading/encoding call
sites to the new API and adding tests, and if you must keep sejda temporarily
open an issue/PR against their repo and add a dependency exclusion/pinning and a
comment documenting the risk until a fix is merged.
| @Configuration | ||
| @ConfigurationProperties(prefix = "image.processing") | ||
| @ConstructorBinding | ||
| data class ImageProcessingConfig( | ||
| val sizes: ImageSizes = ImageSizes(), | ||
| val quality: ImageQuality = ImageQuality() | ||
| ) |
There was a problem hiding this comment.
Incorrect usage of @ConstructorBinding with @configuration.
In Spring Boot 2.7+, @ConstructorBinding should not be combined with @Configuration. This annotation combination can prevent proper configuration binding.
🔎 Recommended fix
Option 1 (Preferred): Remove @configuration and enable via @EnableConfigurationProperties
Remove @Configuration from the data class and enable it elsewhere (e.g., in a main configuration class or application class):
-@Configuration
@ConfigurationProperties(prefix = "image.processing")
@ConstructorBinding
data class ImageProcessingConfig(
val sizes: ImageSizes = ImageSizes(),
val quality: ImageQuality = ImageQuality()
)Then add @EnableConfigurationProperties(ImageProcessingConfig::class) to your main application class or another configuration class.
Option 2: Remove @ConstructorBinding and keep @configuration
@Configuration
@ConfigurationProperties(prefix = "image.processing")
-@ConstructorBinding
data class ImageProcessingConfig(
val sizes: ImageSizes = ImageSizes(),
val quality: ImageQuality = ImageQuality()
)This allows Spring to use setter-based binding instead of constructor binding.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Configuration | |
| @ConfigurationProperties(prefix = "image.processing") | |
| @ConstructorBinding | |
| data class ImageProcessingConfig( | |
| val sizes: ImageSizes = ImageSizes(), | |
| val quality: ImageQuality = ImageQuality() | |
| ) | |
| @Configuration | |
| @ConfigurationProperties(prefix = "image.processing") | |
| data class ImageProcessingConfig( | |
| val sizes: ImageSizes = ImageSizes(), | |
| val quality: ImageQuality = ImageQuality() | |
| ) |
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/config/ImageProcessingConfig.kt around lines 7 to
13, the data class is annotated with @ConstructorBinding and @Configuration
which is incorrect for Spring Boot 2.7+; remove the @Configuration annotation
from this data class and register the properties class via
@EnableConfigurationProperties(ImageProcessingConfig::class) on your main
application or a configuration class (alternatively, if you prefer to keep
@Configuration, remove @ConstructorBinding to use setter-based binding),
ensuring ImageProcessingConfig remains a pure @ConfigurationProperties data
class.
| fun getImageUrlsResponse(): ImageUrlsResponse { | ||
| return if (imageUrls != null) { | ||
| // 새 포맷: JSON 파싱 | ||
| objectMapper.readValue(imageUrls, ImageUrlsResponse::class.java) | ||
| } else { | ||
| // 기존 이미지: 단일 URL을 모든 크기에 사용 | ||
| ImageUrlsResponse( | ||
| id = id, | ||
| webp = null, | ||
| jpeg = ImageSizeUrls( | ||
| thumb = imageUrl ?: "", | ||
| medium = imageUrl ?: "", | ||
| large = imageUrl ?: "" | ||
| ) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON parsing.
The getImageUrlsResponse() method performs JSON parsing without error handling. If the stored JSON is malformed or incompatible, this will throw unchecked exceptions during entity access, potentially breaking application flows.
🔎 Recommended fix
fun getImageUrlsResponse(): ImageUrlsResponse {
return if (imageUrls != null) {
- // 새 포맷: JSON 파싱
- objectMapper.readValue(imageUrls, ImageUrlsResponse::class.java)
+ try {
+ // 새 포맷: JSON 파싱
+ objectMapper.readValue(imageUrls, ImageUrlsResponse::class.java)
+ } catch (e: Exception) {
+ // JSON 파싱 실패 시 fallback
+ ImageUrlsResponse(
+ id = id,
+ webp = null,
+ jpeg = ImageSizeUrls(
+ thumb = imageUrl ?: "",
+ medium = imageUrl ?: "",
+ large = imageUrl ?: ""
+ )
+ )
+ }
} else {
// 기존 이미지: 단일 URL을 모든 크기에 사용
ImageUrlsResponse(
id = id,
webp = null,
jpeg = ImageSizeUrls(
thumb = imageUrl ?: "",
medium = imageUrl ?: "",
large = imageUrl ?: ""
)
)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun getImageUrlsResponse(): ImageUrlsResponse { | |
| return if (imageUrls != null) { | |
| // 새 포맷: JSON 파싱 | |
| objectMapper.readValue(imageUrls, ImageUrlsResponse::class.java) | |
| } else { | |
| // 기존 이미지: 단일 URL을 모든 크기에 사용 | |
| ImageUrlsResponse( | |
| id = id, | |
| webp = null, | |
| jpeg = ImageSizeUrls( | |
| thumb = imageUrl ?: "", | |
| medium = imageUrl ?: "", | |
| large = imageUrl ?: "" | |
| ) | |
| ) | |
| } | |
| } | |
| fun getImageUrlsResponse(): ImageUrlsResponse { | |
| return if (imageUrls != null) { | |
| try { | |
| // 새 포맷: JSON 파싱 | |
| objectMapper.readValue(imageUrls, ImageUrlsResponse::class.java) | |
| } catch (e: Exception) { | |
| // JSON 파싱 실패 시 fallback | |
| ImageUrlsResponse( | |
| id = id, | |
| webp = null, | |
| jpeg = ImageSizeUrls( | |
| thumb = imageUrl ?: "", | |
| medium = imageUrl ?: "", | |
| large = imageUrl ?: "" | |
| ) | |
| ) | |
| } | |
| } else { | |
| // 기존 이미지: 단일 URL을 모든 크기에 사용 | |
| ImageUrlsResponse( | |
| id = id, | |
| webp = null, | |
| jpeg = ImageSizeUrls( | |
| thumb = imageUrl ?: "", | |
| medium = imageUrl ?: "", | |
| large = imageUrl ?: "" | |
| ) | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/store/entity/StoreImage.kt around lines 50 to 66,
the JSON parsing of imageUrls is performed without error handling; wrap the
objectMapper.readValue call in a try-catch (catch
JsonProcessingException/IOException or Exception), log the parse error with
context (id and the raw imageUrls) using the class logger, and on error return
the same legacy fallback ImageUrlsResponse (using imageUrl for all sizes) so
malformed JSON does not throw and entity access continues.
src/main/kotlin/upbrella/be/store/service/ImageProcessingService.kt
Outdated
Show resolved
Hide resolved
| } catch (e: Exception) { | ||
| // 업로드 실패 시 이미 업로드된 파일들 삭제 (rollback) | ||
| uploadedKeys.forEach { url -> | ||
| try { | ||
| deleteFileInS3(url) | ||
| } catch (deleteError: Exception) { | ||
| // 로그만 남기고 계속 진행 | ||
| } | ||
| } | ||
| throw e | ||
| } |
There was a problem hiding this comment.
Swallowed exception during rollback cleanup.
The comment states "로그만 남기고 계속 진행" (only log and continue), but no logging is implemented. Silent failures make debugging difficult when partial cleanups fail.
🔎 Proposed fix
} catch (deleteError: Exception) {
- // 로그만 남기고 계속 진행
+ // 로그만 남기고 계속 진행
+ logger.warn("Failed to rollback uploaded file: $url", deleteError)
}Add a logger to the class:
companion object {
private val logger = LoggerFactory.getLogger(StoreImageService::class.java)
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 130-130: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/store/service/StoreImageService.kt around lines
125 to 135, the rollback catch for deleteFileInS3 swallows exceptions without
logging; add a logger to the class (companion object with
LoggerFactory.getLogger(StoreImageService::class.java>) and update the inner
catch to call logger.error(...) including the deleteError and a clear message
about which uploadedKeys/url failed to delete, then continue as before.
| private fun parseKey(url: String): String { | ||
| val splitUrl = url.split("/") | ||
| return splitUrl[splitUrl.size - 2] + "/" + splitUrl[splitUrl.size - 1] | ||
| return if (splitUrl.size >= 2) { | ||
| splitUrl[splitUrl.size - 3] + "/" + splitUrl[splitUrl.size - 2] + "/" + splitUrl[splitUrl.size - 1] | ||
| } else { | ||
| splitUrl[splitUrl.size - 2] + "/" + splitUrl[splitUrl.size - 1] | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Incorrect bounds check causes ArrayIndexOutOfBoundsException.
When splitUrl.size is 2 or 3, accessing splitUrl[splitUrl.size - 3] throws an exception. The condition should require at least 4 elements to safely access the last 3 segments. The else branch also fails when size < 2.
🔎 Proposed fix
private fun parseKey(url: String): String {
val splitUrl = url.split("/")
- return if (splitUrl.size >= 2) {
+ return if (splitUrl.size >= 4) {
splitUrl[splitUrl.size - 3] + "/" + splitUrl[splitUrl.size - 2] + "/" + splitUrl[splitUrl.size - 1]
- } else {
+ } else if (splitUrl.size >= 2) {
splitUrl[splitUrl.size - 2] + "/" + splitUrl[splitUrl.size - 1]
+ } else {
+ splitUrl.lastOrNull() ?: throw IllegalArgumentException("Invalid URL: $url")
}
}🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/store/service/StoreImageService.kt around lines
175-182, the current bounds check is incorrect and can index out of range;
change the logic to: if splitUrl.size >= 3, return the last three segments
joined with "/", else if splitUrl.size >= 2, return the last two segments joined
with "/", otherwise return the original url (or splitUrl[0]) to avoid any
ArrayIndexOutOfBoundsException. Ensure you use size-1, size-2, size-3 only when
those indices exist and handle size < 2 safely.
| { assertEquals(testUrl, testImage.imageUrl) }, | ||
| { verify(storeImageWriter, times(1)).deleteById(testImage.id!!) }, | ||
| { verify(s3Client, times(1)).deleteObject(any(DeleteObjectRequest::class.java)) }, | ||
| { verify(s3Client, times(3)).deleteObject(any(DeleteObjectRequest::class.java)) }, // 3 sizes x 1 format (JPEG only, no WebP for old images) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the StoreImageService file
echo "=== Finding StoreImageService file ==="
fd -t f 'StoreImageService\.kt' | grep -v Test
# Then check the deletion logic
echo -e "\n=== Checking deleteFile method implementation ==="
rg -n -A 20 'fun deleteFile' --type=kt -g '!Test'
# Look for deleteObject calls to understand deletion patterns
echo -e "\n=== Looking for S3 deleteObject calls in implementation ==="
rg -n -B 3 -A 3 'deleteObject' --type=kt -g '!Test' -g 'StoreImageService'
# Also check the test file for both old and new image scenarios
echo -e "\n=== Checking test file for image format scenarios ==="
rg -n -B 5 -A 5 'verify.*deleteObject.*times' --type=kt -g 'StoreImageServiceTest'Repository: UPbrella/UPbrella_back
Length of output: 432
🏁 Script executed:
#!/bin/bash
# Let's also check the entire test file structure to understand what scenarios are tested
echo "=== Examining StoreImageServiceTest structure ==="
cat -n src/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.kt | head -150
# Look for test functions related to deletion
echo -e "\n=== Finding deletion-related test functions ==="
rg -n 'fun.*delete|fun.*remove' --type=kt -g 'StoreImageServiceTest'Repository: UPbrella/UPbrella_back
Length of output: 6749
🏁 Script executed:
#!/bin/bash
# Read the StoreImageService implementation
echo "=== Reading StoreImageService.kt ==="
cat -n src/main/kotlin/upbrella/be/store/service/StoreImageService.kt
# Search for deleteFile in the service
echo -e "\n=== Searching for deleteFile method ==="
grep -n 'deleteFile\|deleteObject' src/main/kotlin/upbrella/be/store/service/StoreImageService.ktRepository: UPbrella/UPbrella_back
Length of output: 8845
🏁 Script executed:
#!/bin/bash
# Check the entire test file for all test methods
echo "=== All test methods in StoreImageServiceTest ==="
grep -n '@Test\|fun.*Test\|fun.*test' src/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.kt
# Look for any WebP or format-related tests
echo -e "\n=== Looking for WebP/format-related content in tests ==="
grep -n -i 'webp\|new.*image\|old.*image\|format' src/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.ktRepository: UPbrella/UPbrella_back
Length of output: 1542
Add test case for new images with WebP deletion.
The implementation correctly handles both old and new image formats: old images (JPEG only) delete 3 files, while new images (JPEG + WebP) delete 6 files. However, the current test deleteFileTest() only covers the old image scenario. Add a test case that verifies 6 deleteObject calls when deleting new images with both JPEG and WebP variants.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.kt (1)
126-131: Add test case for new images with WebP deletion.The current
deleteFileTest()only verifies 3deleteObjectcalls for old images (JPEG only). Add a test case that verifies 6deleteObjectcalls when deleting new images that have both JPEG and WebP variants stored inimageUrls.src/main/kotlin/upbrella/be/store/service/StoreImageService.kt (2)
128-136: Swallowed exception during rollback cleanup.The comment states "로그만 남기고 계속 진행" but no logging is implemented. Silent failures make debugging partial cleanup failures difficult.
178-185: Bug: Incorrect bounds check causesArrayIndexOutOfBoundsException.When
splitUrl.sizeis 2 or 3, accessingsplitUrl[splitUrl.size - 3]throws an exception. The condition should require at least 4 elements to safely access the last 3 segments.
🧹 Nitpick comments (3)
src/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.kt (1)
153-201: Add test case for JPEG fallback when WebP is null.The
createThumbnailTestonly covers the happy path where WebP URLs exist. Consider adding a test case wherewebpisnullto verify the fallback tojpeg.thumbworks correctly.🔎 Proposed additional test case
@Test @DisplayName("WebP가 없는 경우 JPEG 썸네일을 반환한다.") fun createThumbnailFallbackToJpegTest() { // given val imageWithoutWebp = SingleImageUrlResponse( id = 1L, imageUrl = "https://null.s3.ap-northeast-2.amazonaws.com/store-image/filename.jpg", imageUrls = ImageUrlsResponse( id = 1L, webp = null, jpeg = ImageSizeUrls( thumb = "https://null.s3.ap-northeast-2.amazonaws.com/store-image/filename-thumb.jpg", medium = "https://null.s3.ap-northeast-2.amazonaws.com/store-image/filename-medium.jpg", large = "https://null.s3.ap-northeast-2.amazonaws.com/store-image/filename-large.jpg" ) ) ) val imageUrls = listOf(imageWithoutWebp) // when val thumbnail = storeImageService.createThumbnail(imageUrls) // then assertThat(thumbnail).isEqualTo(imageUrls[0].imageUrls.jpeg.thumb) }src/main/kotlin/upbrella/be/store/service/StoreImageService.kt (2)
144-155: Consider making the CDN domain configurable.The domain
https://file.upbrella.co.kris hardcoded on line 154. If you need different domains for different environments (staging, production), extract this to a configuration property.🔎 Proposed fix
class StoreImageService( private val s3Client: S3Client, private val storeImageReader: StoreImageReader, private val storeImageWriter: StoreImageWriter, private val storeDetailReader: StoreDetailReader, private val imageProcessingService: ImageProcessingService, @Value("\${AWS_S3_BUCKET}") private val bucketName: String, + @Value("\${cdn.base-url:https://file.upbrella.co.kr}") + private val cdnBaseUrl: String, )s3Client.putObject(putObjectRequest, RequestBody.fromBytes(data)) - return "https://file.upbrella.co.kr/$key" + return "$cdnBaseUrl/$key"
168-171: Remove unused methodsaveStoreImage.This private method is never called anywhere in the codebase and has been replaced by
saveStoreImageWithUrls.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/kotlin/upbrella/be/store/service/StoreImageService.ktsrc/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
src/test/kotlin/upbrella/be/store/service/StoreImageServiceTest.kt (2)
52-67: LGTM on manual service construction.The manual
@BeforeEachsetup is appropriate here since it allows injecting thebucketNameconfiguration value that would otherwise require@InjectMocksworkarounds.
69-106: LGTM on upload test updates.The test correctly mocks
ProcessedImageSetwith all variants and verifies 6putObjectcalls for the multi-variant upload flow. The expected URL construction aligns with the service implementation.src/main/kotlin/upbrella/be/store/service/StoreImageService.kt (3)
29-31: LGTM on externalized bucket name.The bucket name is now properly injected via
@Value, addressing the hardcoded bucket name issue. Note: EnsureAWS_S3_BUCKETis set in all deployment environments.
34-51: LGTM on the refactored upload flow.The multi-variant upload pipeline is well-structured: process → upload all variants → persist → return medium JPEG URL for backward compatibility.
57-73: LGTM on multi-variant deletion logic.The deletion correctly handles both old images (JPEG only, when
webpis null) and new images (both WebP and JPEG variants). The null-safe?.lethandles backward compatibility gracefully.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/kotlin/upbrella/be/store/service/ImageProcessingService.kt (2)
16-31: Consider removing ImageSize enum or relaxing the from() mapping.The
from(configValue: Int)function uses exact value matching, which creates brittle coupling between configuration and code. If configuration values change slightly (e.g., 305 instead of 300), the application will fail at runtime.Additionally, since
config.sizesalready provides the target sizes and the enum'smaxSizeduplicates those values, the enum appears unnecessary.🔎 Option 1: Remove the enum entirely (recommended)
The enum doesn't add value since configuration already provides the sizes directly.
- enum class ImageSize(val maxSize: Int) { - THUMBNAIL(300), - MEDIUM(800), - LARGE(1200); - - companion object { - fun from(configValue: Int): ImageSize { - return when (configValue) { - 300 -> THUMBNAIL - 800 -> MEDIUM - 1200 -> LARGE - else -> throw IllegalArgumentException("Unknown image size: $configValue") - } - } - } - }🔎 Option 2: Make from() use range-based matching
If the enum must be kept, use ranges instead of exact values:
companion object { fun from(configValue: Int): ImageSize { return when { - configValue == 300 -> THUMBNAIL - configValue == 800 -> MEDIUM - configValue == 1200 -> LARGE + configValue <= 400 -> THUMBNAIL + configValue <= 900 -> MEDIUM + configValue <= 1300 -> LARGE else -> throw IllegalArgumentException("Unknown image size: $configValue") } } }
46-54: Consider making the downsampling threshold configurable.The hardcoded 2000px threshold may not suit all deployment environments. Environments with limited heap might need a lower threshold, while those with more memory could handle larger images.
🔎 Proposed enhancement
Add a configuration property for the maximum working size:
In
ImageProcessingConfig:val maxWorkingSize: Int = 2000Then use it here:
- val workingImage = if (originalImage.width > 2000 || originalImage.height > 2000) { + val workingImage = if (originalImage.width > config.maxWorkingSize || + originalImage.height > config.maxWorkingSize) { Thumbnails.of(originalImage) - .size(2000, 2000) + .size(config.maxWorkingSize, config.maxWorkingSize) .asBufferedImage() .also { originalImage.flush() } } else { originalImage }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/kotlin/upbrella/be/store/service/ImageProcessingService.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (5)
src/main/kotlin/upbrella/be/store/service/ImageProcessingService.kt (5)
78-94: LGTM!The function correctly handles image processing for each size with proper memory management. The try-finally ensures the resized image is flushed after format conversions.
99-103: LGTM!Clean and correct implementation. Thumbnailator automatically maintains aspect ratio with the
.size()method.
108-132: LGTM! Resource management concerns resolved.The resource leaks flagged in the previous review have been properly addressed:
ByteArrayOutputStreamandImageOutputStreamuse.use {}for automatic closingImageWriteris disposed in a finally block- Compression type setting safely handles null with
?.firstOrNull() ?: "Lossy"
137-179: LGTM! Resource management concerns resolved.The resource leaks flagged in the previous review have been properly addressed:
ByteArrayOutputStreamandImageOutputStreamuse.use {}for automatic closingImageWriteris disposed in a finally block- RGB conversion properly disposes Graphics
- Converted RGB image is flushed when needed
The nested try-finally structure correctly ensures all resources are cleaned up.
186-216: LGTM!The data classes are well-structured.
ImageVariantDatacorrectly overridesequals()andhashCode()usingcontentEquals()andcontentHashCode()for proper ByteArray comparison.
| } catch (e: OutOfMemoryError) { | ||
| throw IllegalStateException("Image is too large to process. Please use a smaller image (max 10MB).", e) | ||
| } | ||
| } catch (e: OutOfMemoryError) { | ||
| throw IllegalStateException("Image is too large to process. Please use a smaller image (max 10MB).", e) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate OutOfMemoryError handling.
Both catch blocks (lines 67-69 and 70-72) perform identical error handling. Keep only the outer catch block to avoid duplication.
🔎 Proposed fix
try {
// 이미지가 너무 크면 먼저 다운샘플링 (메모리 절약)
val workingImage = if (originalImage.width > 2000 || originalImage.height > 2000) {
// Thumbnailator는 자동으로 EXIF orientation 적용
Thumbnails.of(originalImage)
.size(2000, 2000)
.asBufferedImage()
.also { originalImage.flush() }
} else {
originalImage
}
- try {
- // 3가지 크기로 리사이징 (순차 처리)
- val thumbnail = processSize(workingImage, config.sizes.thumbnail)
- val medium = processSize(workingImage, config.sizes.medium)
- val large = processSize(workingImage, config.sizes.large)
-
- return ProcessedImageSet(thumbnail, medium, large)
- } finally {
- // 작업 이미지 메모리 해제
- workingImage.flush()
- }
- } catch (e: OutOfMemoryError) {
- throw IllegalStateException("Image is too large to process. Please use a smaller image (max 10MB).", e)
- }
+ // 3가지 크기로 리사이징 (순차 처리)
+ val thumbnail = processSize(workingImage, config.sizes.thumbnail)
+ val medium = processSize(workingImage, config.sizes.medium)
+ val large = processSize(workingImage, config.sizes.large)
+
+ return ProcessedImageSet(thumbnail, medium, large)
+ } finally {
+ // 작업 이미지 메모리 해제
+ workingImage.flush()
+ }
} catch (e: OutOfMemoryError) {
throw IllegalStateException("Image is too large to process. Please use a smaller image (max 10MB).", e)
}🤖 Prompt for AI Agents
In src/main/kotlin/upbrella/be/store/service/ImageProcessingService.kt around
lines 67 to 72, there are two identical catch blocks for OutOfMemoryError;
remove the inner (duplicate) catch so only the outer catch remains, keeping the
single IllegalStateException wrapping with the same message and cause, and
ensure surrounding braces/indentation remain correct after deletion.
🟢 구현내용
🧩 고민과 해결과정
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.