Cloudinary File Upload Integration - #7
Conversation
…apper accordingly
…and delete functionalities
…iable in deleteFile method
📝 WalkthroughWalkthroughReplaces local file storage with Cloudinary: adds CloudinaryService and config, removes FileStorageService, changes BookResponse.cover from byte[] to String, updates BookService to use Cloudinary for upload/delete, updates tests, and bumps UI and backend versions and dependencies. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant BookService as BookService
participant CloudinaryService as CloudinaryService
participant CloudinaryAPI as Cloudinary API
participant Database as Database
Client->>BookService: uploadCover(file, bookId)
BookService->>Database: find book by id
Database-->>BookService: book
BookService->>CloudinaryService: uploadUserFile(file, userId)
CloudinaryService->>CloudinaryAPI: upload(file, transformations)
CloudinaryAPI-->>CloudinaryService: secureUrl
CloudinaryService-->>BookService: coverUrl
BookService->>CloudinaryService: deleteFile(oldCoverUrl)
CloudinaryService->>CloudinaryAPI: destroy(publicId)
CloudinaryAPI-->>CloudinaryService: result
BookService->>Database: save book with coverUrl
Database-->>BookService: saved
BookService-->>Client: success response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
book-network/src/main/java/com/ichaabane/book_network/application/service/BookService.java (1)
273-291:⚠️ Potential issue | 🔴 CriticalMissing authorization check before cover upload.
The
uploadCovermethod retrieves theconnectedUserbut never verifies that the user owns the book before uploading a cover. Other methods in this class (e.g.,updateShareableStatus,updateArchivedStatus) properly check ownership:if (!Objects.equals(book.getOwner().getId(), user.getId())) { throw new OperationNotPermittedException("..."); }Without this check, any authenticated user could upload/replace the cover of any book.
Proposed fix
Book book = bookRepository.findById(bookId) .orElseThrow(() -> new EntityNotFoundException(NO_BOOK_FOUND_PREFIX + bookId)); User user = ((User) connectedUser.getPrincipal()); + if (!Objects.equals(book.getOwner().getId(), user.getId())) { + throw new OperationNotPermittedException("You cannot update others book cover"); + } + // Delete old cover if exists if (book.getBookCover() != null) { cloudinaryService.deleteFile(book.getBookCover()); }
🤖 Fix all issues with AI agents
In `@book-network/pom.xml`:
- Around line 77-82: Update the Cloudinary dependency from
com.cloudinary:cloudinary-http44 version 1.38.0 to 1.39.0 and add explicit
dependency overrides for the transitive vulnerable libraries by declaring
org.apache.commons:commons-lang3 at least 3.18.0 and
org.apache.httpcomponents:httpclient at least 4.5.13 so the newer safe versions
are used instead of the old transitive ones; locate the existing
cloudinary-http44 dependency declaration and add the two explicit dependency
entries for commons-lang3 and httpclient to the same pom to force the upgraded
transitive versions.
In
`@book-network/src/main/java/com/ichaabane/book_network/application/service/BookService.java`:
- Around line 280-290: The code currently deletes the old cover via
book.getBookCover() before calling cloudinaryService.uploadUserFile, which risks
losing the cover if uploadUserFile fails; change the flow in BookService so you
first call cloudinaryService.uploadUserFile(file, user.getId()), verify the
returned coverUrl is non-null/valid (or throw an exception), then only call
cloudinaryService.deleteFile(book.getBookCover()) for the old cover, set
book.setBookCover(coverUrl), and persist with bookRepository.save(book); keep
the null-check and error handling around uploadUserFile to avoid overwriting the
existing cover with null.
In
`@book-network/src/main/java/com/ichaabane/book_network/application/service/CloudinaryService.java`:
- Around line 98-104: The generatePublicId method currently appends the file
extension from originalFilename which causes double extensions on Cloudinary
when using "resource_type":"auto"; update generatePublicId(String
originalFilename) to stop extracting/appending the extension and instead return
a unique id (e.g., UUID.randomUUID().toString() + "-" +
System.currentTimeMillis()) so public_id contains no file extension; keep the
method signature and uniqueness logic but remove any use of
originalFilename.substring(...) and extension concatenation (if raw asset
handling is required, add a separate code path outside generatePublicId to
preserve extensions).
🧹 Nitpick comments (6)
book-network/src/test/java/com/ichaabane/book_network/application/mapper/BookMapperTest.java (3)
11-24: Remove unused imports.
FileUtils(line 11) andMockedStatic(line 18) are imported but no longer used after the migration to Cloudinary URL-based covers. These should be cleaned up.-import com.ichaabane.book_network.infrastructure.file.FileUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; -import org.mockito.MockedStatic; import org.mockito.junit.jupiter.MockitoExtension;
201-212: Redundant.toString()call on String type.At line 211,
result.getCover()already returns aString, so calling.toString()is unnecessary. The assertion at line 210 already validates it's aStringinstance.Suggested fix
assertThat(result.getCover()).isNotNull(); assertThat(result.getCover()).isInstanceOf(String.class); - assertThat(result.getCover().toString()).contains("cloudinary.com"); + assertThat(result.getCover()).contains("cloudinary.com");
435-446: Duplicate test case.This test (
shouldHandleBookWithoutCoverinEdgeCaseTests) is identical to the one at lines 227-238 inToBookResponseTests. Consider removing the duplicate.book-network/src/main/java/com/ichaabane/book_network/infrastructure/config/CloudinaryConfig.java (1)
9-29: Consider adding startup validation for required Cloudinary credentials.If the Cloudinary environment variables are missing or empty, the application will start but fail at runtime when the first upload/delete is attempted. Adding validation during bean creation provides fail-fast behavior.
Suggested enhancement
+import org.springframework.util.Assert; + `@Configuration` public class CloudinaryConfig { `@Value`("${cloudinary.cloud-name}") private String cloudName; `@Value`("${cloudinary.api-key}") private String apiKey; `@Value`("${cloudinary.api-secret}") private String apiSecret; `@Bean` public Cloudinary cloudinary() { + Assert.hasText(cloudName, "Cloudinary cloud-name must be configured"); + Assert.hasText(apiKey, "Cloudinary api-key must be configured"); + Assert.hasText(apiSecret, "Cloudinary api-secret must be configured"); + return new Cloudinary(ObjectUtils.asMap(book-network/src/test/java/com/ichaabane/book_network/application/service/CloudinaryServiceTest.java (1)
348-366: Test doesn't actually verify the folder structure.The comment states the folder should be "book-network/users/42" but the test only verifies that
uploadwas called. UseArgumentCaptorto assert the actual folder parameter.Suggested improvement
+import org.mockito.ArgumentCaptor; + `@Test` `@DisplayName`("Should verify correct folder structure for user files") void shouldVerifyCorrectFolderStructure() throws IOException { // Given Integer userId = 42; Map<String, Object> uploadResult = new HashMap<>(); uploadResult.put("secure_url", "https://res.cloudinary.com/demo/image/upload/test.jpg"); given(mockFile.getBytes()).willReturn("test content".getBytes()); given(mockFile.getOriginalFilename()).willReturn("test.jpg"); - given(uploader.upload(any(byte[].class), any())).willReturn(uploadResult); + ArgumentCaptor<Map<String, Object>> optionsCaptor = ArgumentCaptor.forClass(Map.class); + given(uploader.upload(any(byte[].class), optionsCaptor.capture())).willReturn(uploadResult); // When cloudinaryService.uploadUserFile(mockFile, userId); // Then - verify(uploader).upload(any(byte[].class), any()); - // The folder should be "book-network/users/42" - verified in the service implementation + Map<String, Object> capturedOptions = optionsCaptor.getValue(); + assertThat(capturedOptions.get("folder")).isEqualTo("book-network/users/42"); }book-network/src/main/java/com/ichaabane/book_network/application/service/CloudinaryService.java (1)
122-125: Version removal logic may fail for URLs without version.The version removal assumes a
/exists after the version segment. If the URL isupload/vABCwithout a trailing/,indexOf("/")returns-1andsubstring(0)returns the original string including the malformed version prefix.Consider a more robust check:
Suggested fix
// Remove version (v1234567890) if (afterUpload.startsWith("v")) { - afterUpload = afterUpload.substring(afterUpload.indexOf("/") + 1); + int slashIndex = afterUpload.indexOf("/"); + if (slashIndex > 0) { + afterUpload = afterUpload.substring(slashIndex + 1); + } else { + // No folder after version, likely invalid URL + return null; + } }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
book-network/src/main/java/com/ichaabane/book_network/application/service/BookService.java (1)
273-297:⚠️ Potential issue | 🟠 MajorThe data loss fix is properly implemented, but missing ownership validation.
The upload-then-delete flow correctly addresses the previous concern about data loss. However, this method lacks the ownership check that exists in similar methods like
updateShareableStatusandupdateArchivedStatus. Any authenticated user could potentially upload a cover to any book.🔒 Proposed fix to add ownership validation
public void uploadCover(MultipartFile file, Authentication connectedUser, Integer bookId) { if (file == null || file.isEmpty()) return; Book book = bookRepository.findById(bookId) .orElseThrow(() -> new EntityNotFoundException(NO_BOOK_FOUND_PREFIX + bookId)); User user = ((User) connectedUser.getPrincipal()); + if (!Objects.equals(book.getOwner().getId(), user.getId())) { + throw new OperationNotPermittedException("You cannot upload a cover for a book you do not own"); + } + // Upload new cover to Cloudinary first String coverUrl = cloudinaryService.uploadUserFile(file, user.getId());
🤖 Fix all issues with AI agents
In `@book-network/pom.xml`:
- Around line 77-92: The PR pins commons-lang3:3.18.0 and httpclient:4.5.13 to
mitigate CVEs but diverges from the Spring Boot 3.5.3 BOM (commons-lang3 3.17.0
and httpclient5 5.x); run a full build and cross-module integration test matrix
to confirm the app works with cloudinary-http44 (which pulls httpclient 4.x)
alongside Spring Boot 3.5.3, and if any incompatibility appears either align
versions via dependencyManagement (override BOM) or add exclusions and explicit
replacements (use httpclient5-compatible Cloudinary or force commons-lang3 to
BOM if necessary); focus checks on classes that use httpclient and commons-lang3
and update the pom dependencyManagement or exclusions for artifactIds
cloudinary-http44, commons-lang3, and httpclient accordingly.
| <!-- Cloudinary for cloud storage --> | ||
| <dependency> | ||
| <groupId>com.cloudinary</groupId> | ||
| <artifactId>cloudinary-http44</artifactId> | ||
| <version>1.39.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-lang3</artifactId> | ||
| <version>3.18.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.httpcomponents</groupId> | ||
| <artifactId>httpclient</artifactId> | ||
| <version>4.5.13</version> | ||
| </dependency> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Spring Boot 3.5.3 BOM dependency management cloudinary commons-lang3 httpclient versions
💡 Result:
For Spring Boot 3.5.3 (org.springframework.boot:spring-boot-dependencies:3.5.3) the BOM manages:
- Apache Commons Lang (
org.apache.commons:commons-lang3):3.17.0[1] - Apache HttpClient 5 (
org.apache.httpcomponents.client5:httpclient5):5.5[1]
Cloudinary: there is no Cloudinary (com.cloudinary:*) entry/version managed by the Spring Boot 3.5.3 BOM (no cloudinary property and no managed dependency coordinates for it), so you must specify Cloudinary’s version yourself (or use Cloudinary’s own BOM, if they provide one). [1]
Sources
[1] Maven Central (Sonatype) – spring-boot-dependencies:3.5.3 POM/properties and managed dependencies list.
🌐 Web query:
CVE cloudinary-http44 1.39.0 security vulnerabilities
💡 Result:
For com.cloudinary:cloudinary-http44:1.39.0, there don’t appear to be direct CVEs filed against the Cloudinary artifact itself, but it pulls in vulnerable transitive dependencies. [5][1]
CVEs impacting cloudinary-http44 1.39.0 via dependencies
- CVE-2020-13956 (Apache HttpClient):
cloudinary-http44:1.39.0depends onorg.apache.httpcomponents:httpclient:4.4, and HttpClient versions < 4.5.13 are affected (wrong target host selection via malformed URI authority). Mitigation: upgrade HttpClient to ≥ 4.5.13. [1][4] - CVE-2025-48924 (Apache Commons Lang):
cloudinary-http44:1.39.0depends onorg.apache.commons:commons-lang3:3.1, and Commons Lang 3 versions < 3.18.0 are affected (uncontrolled recursion/StackOverflowError DoS). Mitigation: upgrade commons-lang3 to ≥ 3.18.0. [1][3]
What to do
- If you must stay on
cloudinary-http44, use Maven/Gradle dependency overrides to pin:org.apache.httpcomponents:httpclientto 4.5.13+org.apache.commons:commons-lang3to 3.18.0+ (or newer)
then run tests to ensure compatibility. [1][3][4]
🌐 Web query:
CVE commons-lang3 3.18.0 Apache Commons Lang security issues
💡 Result:
For Apache Commons Lang (org.apache.commons:commons-lang3), the main published security issue relevant to 3.18.0 is:
- CVE-2025-48924 (Uncontrolled Recursion / DoS): In
ClassUtils.getClass(...), very long crafted inputs can trigger aStackOverflowError, potentially causing an application crash/denial of service. Affected versions:commons-lang33.0 up to (but not including) 3.18.0. Fixed in: 3.18.0. [1][2]
So, Commons Lang 3.18.0 is the patched (non-vulnerable) version for CVE-2025-48924, and upgrading to 3.18.0 or later is the recommended remediation. [1][3]
Sources:
[1] NVD CVE record (CVE-2025-48924)
[2] GitHub Advisory (GHSA-j288-q9x7-2f5v / CVE-2025-48924)
[3] SUSE security update noting update to 3.18.0 to fix CVE-2025-48924
🌐 Web query:
CVE httpclient 4.5.13 Apache HttpComponents vulnerabilities 2024 2025
💡 Result:
For Apache HttpComponents HttpClient 4.5.13 (the 4.x line), I could not find any new CVEs from 2024 or 2025 that list 4.5.13 as an affected version.
What you will commonly see in 2024/2025 scanners is older items:
- CVE-2020-13956 (malformed URI authority can cause “wrong target host” selection) affects HttpClient < 4.5.13 and is fixed in 4.5.13. So 4.5.13 is not vulnerable to this CVE. [1][2]
A 2025 CVE people sometimes confuse with 4.x is:
- CVE-2025-27820 (PSL validation bug disabling domain checks impacting cookie/hostname verification) affects Apache HttpClient 5.4.0–5.4.2 and is fixed in 5.4.3—this is HttpClient 5.x, not 4.5.13. [3][4]
If you want the safest 4.x baseline anyway, the latest 4.5 line commonly recommended by vulnerability databases is 4.5.14. [5]
Sources: [1] [2] [3] [4] [5]
Version-convergence conflicts confirmed; explicit pins are necessary for CVE mitigation but require integration testing.
The pinned versions mitigate real CVEs in Cloudinary's transitive dependencies:
commons-lang3 3.18.0patches CVE-2025-48924 (DoS via uncontrolled recursion)httpclient 4.5.13patches CVE-2020-13956 (malformed URI handling)
However, these pins diverge from Spring Boot 3.5.3 BOM expectations:
- BOM manages
commons-lang3 3.17.0(PR pins 3.18.0—newer, acceptable but needs testing) - BOM manages
httpclient5 5.5(PR pinshttpclient 4.x—different artifact line due to Cloudinary's dependency)
Confirm your build and integration tests validate cross-module compatibility with the 4.x HttpClient line alongside Spring Boot 3.5.3's 5.x expectations.
🤖 Prompt for AI Agents
In `@book-network/pom.xml` around lines 77 - 92, The PR pins commons-lang3:3.18.0
and httpclient:4.5.13 to mitigate CVEs but diverges from the Spring Boot 3.5.3
BOM (commons-lang3 3.17.0 and httpclient5 5.x); run a full build and
cross-module integration test matrix to confirm the app works with
cloudinary-http44 (which pulls httpclient 4.x) alongside Spring Boot 3.5.3, and
if any incompatibility appears either align versions via dependencyManagement
(override BOM) or add exclusions and explicit replacements (use
httpclient5-compatible Cloudinary or force commons-lang3 to BOM if necessary);
focus checks on classes that use httpclient and commons-lang3 and update the pom
dependencyManagement or exclusions for artifactIds cloudinary-http44,
commons-lang3, and httpclient accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@book-network/src/main/java/com/ichaabane/book_network/application/service/CloudinaryService.java`:
- Around line 108-128: The extractPublicId method is incorrectly assuming the
version segment follows "/upload/" immediately; update extractPublicId to locate
the version token (pattern "v" followed by digits) within the afterUpload string
and extract the public_id as the substring after that version segment, stripping
any leading "/" and the file extension; ensure it still returns null if no
"/upload/" exists or if a version token cannot be found, and keep using that
public_id when calling destroy() so deletions succeed for transformed URLs.
In
`@book-network/src/test/java/com/ichaabane/book_network/application/service/CloudinaryServiceTest.java`:
- Around line 341-407: The tests currently only verify uploader.upload() was
called but don't assert the actual upload options; update the three test methods
(shouldVerifyCorrectFolderStructure, shouldVerifyTransformationParameters,
shouldGenerateUniquePublicIds) to capture the second argument passed to
uploader.upload using an ArgumentCaptor<Map<String,Object>> and assert the
expected values: in shouldVerifyCorrectFolderStructure assert the "folder"
equals "book-network/users/42" when calling
cloudinaryService.uploadUserFile(mockFile, userId); in
shouldVerifyTransformationParameters assert the captured "transformation" map
contains width=500, height=700 and quality="auto:good" when calling
cloudinaryService.uploadFile(mockFile, folder); in shouldGenerateUniquePublicIds
capture the "public_id" for two successive cloudinaryService.uploadFile calls
and assert the two public_id strings are not equal (and that uploader.upload was
called twice).
|
1 participant
📋 Overview
This PR migrates the book cover image storage from local file system to Cloudinary cloud storage service, improving scalability, reliability, and deployment flexibility.
🎯 Version
1.0.0→1.1.01.0.0→1.1.0✨ Key Features
1. Cloud Storage Integration
2. Service Layer Refactoring
CloudinaryServicewith upload and delete operationsFileStorageServiceBookServiceto use Cloudinary for cover imagesBookResponse.coverfield type from byte array to String (URL)3. Enhanced Error Handling
GlobalExceptionHandler📦 Dependencies Added
🔧 Configuration Changes
Application Configuration Files
application-dev.yml: Added Cloudinary configuration for developmentapplication-prod.yml: Added Cloudinary configuration for productionapplication-local.yml: Created local environment configurationRequired Environment Variables
📁 Files Changed
Added Files
src/main/java/com/ichaabane/book_network/infrastructure/cloudinary/CloudinaryConfig.javasrc/main/java/com/ichaabane/book_network/infrastructure/cloudinary/CloudinaryService.javasrc/test/java/com/ichaabane/book_network/infrastructure/cloudinary/CloudinaryServiceTest.javasrc/main/resources/application-local.ymlCLOUDINARY_INTEGRATION_GUIDE.mdCLOUD_DEPLOYMENT_GUIDE.mdCLOUD_DEPLOYMENT_COMPLETE_GUIDE.mdDOCKER_DEPLOY_QUICK.mdModified Files
src/main/java/com/ichaabane/book_network/application/service/BookService.javasrc/main/java/com/ichaabane/book_network/application/mapper/BookMapper.javasrc/main/java/com/ichaabane/book_network/presentation/dto/BookResponse.javasrc/main/java/com/ichaabane/book_network/presentation/handler/GlobalExceptionHandler.javasrc/test/java/com/ichaabane/book_network/application/service/BookServiceTest.javasrc/main/resources/application-dev.ymlsrc/main/resources/application-prod.ymlpom.xmlDeleted Files
src/main/java/com/ichaabane/book_network/infrastructure/file/FileStorageService.javasrc/test/java/com/ichaabane/book_network/infrastructure/file/FileStorageServiceTest.java🧪 Testing
New Tests Added
CloudinaryServiceTest: Comprehensive unit tests for Cloudinary operationsUpdated Tests
BookServiceTest: Refactored to useCloudinaryServiceinstead ofFileStorageService🔄 Migration Impact
Breaking Changes
Database Changes
validatein production🚀 Deployment Considerations
Prerequisites
Environment Setup
Docker Deployment
docker-compose.ymlwith Cloudinary environment variables✅ Checklist
🔗 Related Issues
Closes: N/A (Feature implementation)
🎓 Benefits
🔍 Review Focus Areas
📝 Notes for Reviewers
Branch:
cloudinary-file-uploadTarget:
mainType: Feature
Breaking Changes: Yes
Summary by CodeRabbit
New Features
Chores
Refactor
Tests