Skip to content

Conversation

@kaledOu
Copy link

@kaledOu kaledOu commented Dec 25, 2025

Summary by CodeRabbit

  • Tests

    • Added extensive new test suites for POSIX, S3, and Swift storage adapters and many utility/encryption unit tests.
    • Replaced an older POSIX test suite with a new, more comprehensive POSIX adapter test collection; added coverage for tagging, metadata, packaging, encryption hooks, and error scenarios.
  • Chores

    • Updated code coverage configuration to exclude selected package patterns from reporting.

✏️ Tip: You can customize this high-level summary in your review settings.

kaledOu and others added 2 commits December 25, 2025 11:50
Signed-off-by: kaledOu <kaled.ouali@gmail.com>
Signed-off-by: kouali <kouali@talys.digital>
@coderabbitai
Copy link

coderabbitai bot commented Dec 25, 2025

Warning

Rate limit exceeded

@kaledOu has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 4f7efcb and 0d3105b.

📒 Files selected for processing (1)
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java

Walkthrough

Adds JaCoCo exclusion rules to the object-store POM, deletes a legacy PowerMock PosixAdapter test, and adds multiple new JUnit5 test suites for Posix/S3/Swift adapters plus several encryption and utility tests.

Changes

Cohort / File(s) Summary
Build config
kernel/object-store/pom.xml
Added JaCoCo plugin configuration to exclude packages/patterns from coverage: **/entity/**, **/dto/**, **/exception/**, **/config/**, **/constant/**, and any *Application.class.
Adapter tests (new)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java, kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/S3AdapterTest.java, kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/SwiftAdapterTest.java
Added comprehensive JUnit5 test suites exercising storage operations, tagging, metadata, encryption hooks, container/bucket behavior, error scenarios, and reflective injection of internal state; heavy Mockito usage and varied test cases.
Utility & crypto tests (new)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionHelperTest.java, .../EncryptionUtilTest.java, .../ObjectStoreUtilTest.java, .../OfflineEncryptionUtilTest.java, .../OnlineCryptoUtilTest.java
Added unit tests for encryption selection, merge/packet formatting, online/offline crypto integrations, RestTemplate retrieval, and object-name utilities covering success, error, null, and boundary cases.
Removed legacy tests
kernel/object-store/src/test/java/io/mosip/commons/khazana/test/adapter/PosixAdapterTest.java
Deleted old PowerMock-based PosixAdapterTest (file/ZIP/IO mocks); replaced by updated tests in the impl package.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through tests both old and new,
PowerMocks gone, JUnit5 in view,
Mocks and reflection stitched each seam,
Coverage trimmed to tidy the team,
A crunchy carrot for CI's green hue 🥕

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title 'MOSIP-41175' is a ticket/issue reference only and does not describe the actual changes made in the pull request, which involve adding comprehensive test coverage and updating Jacoco configuration for the object-store module. Replace the ticket reference with a descriptive title that summarizes the main changes, such as 'Add comprehensive unit tests for object storage adapters and utilities' or 'Refactor object-store tests and add Jacoco coverage exclusions'.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (15)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionHelperTest.java (1)

26-40: Consider reducing reliance on reflection for test setup.

Using reflection to inject private fields makes tests brittle and tightly coupled to implementation details. If field names change, all tests break. Consider requesting constructor injection or package-private setters in the production code to improve testability.

kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OfflineEncryptionUtilTest.java (1)

36-41: Consider reducing reliance on reflection for test setup.

Similar to other test files in this PR, using reflection to inject private fields couples tests to implementation details. Package-private setters or constructor injection would improve maintainability.

kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/SwiftAdapterTest.java (2)

38-47: Consider reducing reliance on reflection for test setup.

Injecting a pre-populated accounts map via reflection makes tests brittle. Consider exposing a test-friendly constructor or using dependency injection to improve testability without reflection.


171-186: Accepting any exception weakens the test.

Lines 182-186 catch any exception and treat it as test success with assertNotNull(e). This is too permissive - the comment mentions CommandException, but the code accepts any exception type. Consider asserting the specific exception type or documenting why any exception is acceptable.

kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OnlineCryptoUtilTest.java (1)

44-69: Excessive reflection in test setup reduces maintainability.

The setup method uses reflection to set 5 different private fields. This creates high coupling to implementation details. Consider redesigning OnlineCryptoUtil to accept dependencies through constructor injection or providing test-specific factory methods.

kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/S3AdapterTest.java (2)

46-61: Reflection-heavy setup reduces test maintainability.

The setup injects mocked connection and configures multiple flags via reflection. Consider providing a test-friendly constructor or builder pattern to reduce coupling to private implementation details.


507-538: Overly complex error handling test.

Lines 507-538 set up invalid credentials and a maxRetry of 0 to force an exception, then catch InvocationTargetException to verify ObjectStoreAdapterException is the cause. Consider testing connection failure through a simpler mechanism (e.g., mocking AmazonS3ClientBuilder to throw) rather than relying on actual connection attempts with invalid credentials.

kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/S3AdapterExtendedTest.java (3)

54-57: Suppress or handle unchecked cast warning.

The cast (List<String>) existing.get(adapter) is unchecked and will generate a compiler warning. Consider adding @SuppressWarnings("unchecked") to the method or using a safer pattern.

🔎 Proposed fix
     @BeforeEach
+    @SuppressWarnings("unchecked")
     void setup() throws Exception {

516-518: Remove unused variable finalKey.

The variable finalKey is declared but never used in this test method.

🔎 Proposed fix
         String account = "acct";
         String container = "cont";
-        String finalKey = "cont/s/p/o";

         byte[] data = "payload2".getBytes(StandardCharsets.UTF_8);

503-507: Consider using try-with-resources for InputStream.

The InputStream returned by adapter.getObject() should be closed in a try-with-resources block to ensure proper resource cleanup, even in test code.

🔎 Proposed fix
-        InputStream is = adapter.getObject(account, container, source, process, objectName);
-        assertNotNull(is);
-        byte[] got = is.readAllBytes();
-        assertArrayEquals(data, got);
+        try (InputStream is = adapter.getObject(account, container, source, process, objectName)) {
+            assertNotNull(is);
+            byte[] got = is.readAllBytes();
+            assertArrayEquals(data, got);
+        }
         verify(s3Object).close();
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (5)

45-62: Add @AfterEach cleanup for tempDir.

The tempDir created in @BeforeEach is not cleaned up, which can lead to accumulation of temp directories across test runs. Consider adding cleanup:

🔎 Proposed fix
+    @org.junit.jupiter.api.AfterEach
+    void cleanup() throws Exception {
+        if (tempDir != null && Files.exists(tempDir)) {
+            Files.walk(tempDir)
+                .sorted(java.util.Comparator.reverseOrder())
+                .forEach(p -> {
+                    try { Files.deleteIfExists(p); } catch (IOException ignored) {}
+                });
+        }
+    }
+
     @Test
     void putAndGetObjectShouldStoreAndRetrieveContent() throws IOException {

79-82: Close InputStream with try-with-resources.

The InputStream returned by adapter.getObject() is not closed, which can lead to resource leaks.

🔎 Proposed fix
-        InputStream is = adapter.getObject(account, container, source, process, objectName);
-        assertNotNull(is);
-        byte[] read = is.readAllBytes();
-        assertArrayEquals(payload, read);
+        try (InputStream is = adapter.getObject(account, container, source, process, objectName)) {
+            assertNotNull(is);
+            byte[] read = is.readAllBytes();
+            assertArrayEquals(payload, read);
+        }

220-227: Close InputStreams with try-with-resources.

Both i1 and i2 should be closed after use.

🔎 Proposed fix
-        InputStream i1 = adapter.getObject(account, container, source, process, obj1);
-        InputStream i2 = adapter.getObject(account, container, source, process, obj2);
-        assertNotNull(i1);
-        assertNotNull(i2);
-        byte[] read1 = i1.readAllBytes();
-        byte[] read2 = i2.readAllBytes();
-        assertArrayEquals(payload1, read1);
-        assertArrayEquals(payload2, read2);
+        try (InputStream i1 = adapter.getObject(account, container, source, process, obj1);
+             InputStream i2 = adapter.getObject(account, container, source, process, obj2)) {
+            assertNotNull(i1);
+            assertNotNull(i2);
+            byte[] read1 = i1.readAllBytes();
+            byte[] read2 = i2.readAllBytes();
+            assertArrayEquals(payload1, read1);
+            assertArrayEquals(payload2, read2);
+        }

309-322: Reuse class-level adapter instead of creating new instance.

A new PosixAdapter is instantiated here, but @BeforeEach already sets up the class-level adapter field with proper configuration. Consider using the existing adapter to reduce boilerplate and improve consistency. This applies to multiple tests in lines 309-422.


389-392: delete() on non-empty directory silently fails.

accountDir.delete() will silently fail if the directory contains files. Since this is a precondition check, consider using Files.deleteIfExists() after ensuring the directory is empty, or use a recursive delete utility.

🔎 Proposed fix
         // ensure account dir does not exist
         File accountDir = new File(tmp, account);
-        if (accountDir.exists())
-            accountDir.delete();
+        if (accountDir.exists()) {
+            Files.walk(accountDir.toPath())
+                .sorted((a, b) -> b.compareTo(a))
+                .forEach(p -> { try { Files.deleteIfExists(p); } catch (IOException ignored) {} });
+        }
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2998d0 and 75e51c5.

📒 Files selected for processing (10)
  • kernel/object-store/pom.xml
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/S3AdapterExtendedTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/S3AdapterTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/SwiftAdapterTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionHelperTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionUtilTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/util/ObjectStoreUtilTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OfflineEncryptionUtilTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OnlineCryptoUtilTest.java
🧰 Additional context used
🧬 Code graph analysis (6)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionUtilTest.java (2)
kernel/object-store/src/main/java/io/mosip/commons/khazana/constant/KhazanaConstant.java (1)
  • KhazanaConstant (3-12)
kernel/object-store/src/main/java/io/mosip/commons/khazana/util/EncryptionUtil.java (1)
  • EncryptionUtil (5-14)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (1)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionHelperTest.java (1)
  • ExtendWith (14-68)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/ObjectStoreUtilTest.java (1)
kernel/object-store/src/main/java/io/mosip/commons/khazana/util/ObjectStoreUtil.java (1)
  • ObjectStoreUtil (5-42)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OfflineEncryptionUtilTest.java (1)
kernel/object-store/src/main/java/io/mosip/commons/khazana/constant/KhazanaConstant.java (1)
  • KhazanaConstant (3-12)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/S3AdapterTest.java (1)
kernel/object-store/src/main/java/io/mosip/commons/khazana/exception/ObjectStoreAdapterException.java (1)
  • ObjectStoreAdapterException (5-14)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionHelperTest.java (2)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OfflineEncryptionUtilTest.java (1)
  • ExtendWith (24-76)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OnlineCryptoUtilTest.java (1)
  • ExtendWith (30-151)
🔇 Additional comments (16)
kernel/object-store/pom.xml (1)

142-151: LGTM! Standard coverage exclusions applied.

The Jacoco exclusions for entity, DTO, exception, config, constant packages and Application classes align with best practices for code coverage reporting. These exclusions appropriately target infrastructure and boilerplate code that typically doesn't require unit test coverage.

kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionHelperTest.java (1)

42-67: LGTM! Clear test coverage of crypto path selection.

Both offline and online encryption paths are tested with clear assertions. The test names are descriptive and the logic is easy to follow.

kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OfflineEncryptionUtilTest.java (1)

43-67: LGTM! Thorough validation of encryption output structure.

The test correctly validates both the total length and the position of encrypted data within the merged byte array. The use of KhazanaConstant for sizing is appropriate.

kernel/object-store/src/test/java/io/mosip/commons/khazana/util/ObjectStoreUtilTest.java (2)

51-54: Document trailing slash behavior in edge case.

The test documents that getName("obj", "") returns "obj/" with a trailing slash. This behavior may be unexpected - consider verifying this is intentional rather than a quirk.


10-59: LGTM! Comprehensive coverage of path composition utility.

The tests cover all getName overloads and include edge cases for empty parameters. The test structure is clean and doesn't rely on reflection.

kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionUtilTest.java (1)

16-101: LGTM! Excellent test coverage for encryption data merging.

The test suite comprehensively validates:

  • Correct concatenation order (nonce, aad, encrypted data)
  • Handling of empty encrypted data
  • Null input validation
  • Boundary conditions for oversized inputs

The assertions clearly verify both length and content positioning.

kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/SwiftAdapterTest.java (1)

49-264: LGTM! Comprehensive adapter test coverage.

The test suite thoroughly validates Swift adapter behavior including object operations, metadata management, tag handling, and edge cases. Mock verification ensures correct side effects.

kernel/object-store/src/test/java/io/mosip/commons/khazana/util/OnlineCryptoUtilTest.java (1)

71-150: LGTM! Thorough coverage of online crypto operations.

The tests validate:

  • Successful encryption with merged byte array output
  • Error handling when service returns errors
  • Decryption flow with proper byte extraction
  • RestTemplate lazy initialization fallback

The test coverage is comprehensive and assertions are clear.

kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/S3AdapterTest.java (2)

145-165: LGTM! Good coverage of backward compatibility error handling.

The test validates the retry logic when the backward-compatibility error is thrown, verifying that the adapter calls deleteObject before retrying putObject. This defensive behavior is well-tested.


63-831: LGTM! Exceptionally comprehensive S3 adapter test coverage.

The test suite thoroughly validates S3Adapter behavior across numerous scenarios:

  • Object CRUD operations
  • Tag and metadata management
  • Account-based bucket naming
  • Bucket creation on demand
  • Error handling and edge cases
  • Path manipulation utilities

The coverage demonstrates strong attention to detail and robustness.

kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/S3AdapterExtendedTest.java (3)

60-76: Good coverage of tag upload behavior.

The test correctly verifies that multiple tags are uploaded and the bucket existence check is performed.


94-115: Thorough backward compatibility error handling test.

This test properly validates the retry logic when an S3 backward compatibility error occurs, including verification of delete and retry operations.


456-488: Good coverage of connection failure scenarios.

The test thoroughly validates error handling when connection building fails, including setting up the preconditions correctly via reflection.

kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (3)

64-83: Good integration-style test for put and get operations.

This test effectively validates the round-trip behavior of storing and retrieving content through the PosixAdapter.


119-136: Good coverage of encryption integration.

The test correctly mocks the encryption helper and validates the pack operation behavior.


139-157: Comprehensive tag lifecycle test.

Good coverage of the add/get/delete tag flow with appropriate assertions.

Signed-off-by: kouali <kouali@talys.digital>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (8)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (8)

79-82: Close InputStream with try-with-resources.

The InputStream should be explicitly closed to ensure proper resource cleanup, even though readAllBytes() typically consumes the stream.

🔎 Proposed fix
-        InputStream is = adapter.getObject(account, container, source, process, objectName);
-        assertNotNull(is);
-        byte[] read = is.readAllBytes();
-        assertArrayEquals(payload, read);
+        try (InputStream is = adapter.getObject(account, container, source, process, objectName)) {
+            assertNotNull(is);
+            byte[] read = is.readAllBytes();
+            assertArrayEquals(payload, read);
+        }

85-111: Simplify test assertion and remove fallback logic.

The fallback cleanup logic and weak assertion assertTrue(removed || !Files.exists(zip)) obscure the test's intent. A test should verify the expected behavior directly. If removeContainer is expected to succeed, assert assertTrue(removed) directly.

🔎 Proposed fix
         // now remove
         boolean removed = adapter.removeContainer(account, container, source, process);
-        java.nio.file.Path zip = tempDir.resolve(account).resolve(container + ".zip");
-        if (!removed) {
-            // attempt to clean up the file if adapter failed to report success
-            try {
-                java.nio.file.Files.deleteIfExists(zip);
-            } catch (Exception ex) {
-                // ignore, will assert below
-            }
-        }
-        // After either adapter removal or manual cleanup, the zip should not exist
-        assertTrue(removed || !java.nio.file.Files.exists(zip));
+        assertTrue(removed, "Container removal should succeed");
+        
+        // Verify the zip no longer exists
+        java.nio.file.Path zip = tempDir.resolve(account).resolve(container + ".zip");
+        assertFalse(Files.exists(zip), "Zip file should be deleted");

219-227: Close InputStreams with try-with-resources.

Both InputStream instances should be explicitly closed to ensure proper resource cleanup.

🔎 Proposed fix
         // both objects should be retrievable
-        InputStream i1 = adapter.getObject(account, container, source, process, obj1);
-        InputStream i2 = adapter.getObject(account, container, source, process, obj2);
-        assertNotNull(i1);
-        assertNotNull(i2);
-        byte[] read1 = i1.readAllBytes();
-        byte[] read2 = i2.readAllBytes();
-        assertArrayEquals(payload1, read1);
-        assertArrayEquals(payload2, read2);
+        try (InputStream i1 = adapter.getObject(account, container, source, process, obj1)) {
+            assertNotNull(i1);
+            byte[] read1 = i1.readAllBytes();
+            assertArrayEquals(payload1, read1);
+        }
+        
+        try (InputStream i2 = adapter.getObject(account, container, source, process, obj2)) {
+            assertNotNull(i2);
+            byte[] read2 = i2.readAllBytes();
+            assertArrayEquals(payload2, read2);
+        }

309-349: Consider using shared adapter for consistency.

This test creates its own PosixAdapter instance and temp directory, while most other tests use the shared adapter from @BeforeEach. This inconsistency makes the test suite harder to maintain. Consider refactoring to use the shared setup unless there's a specific reason to isolate this test.


351-376: Consider using shared adapter for consistency.

This test also creates its own PosixAdapter instance instead of using the shared one from @BeforeEach, contributing to setup inconsistency across the test suite.


409-409: Use consistent cleanup pattern.

For consistency with other tests (lines 346-348, 373-375), use Files.deleteIfExists(p) instead of p.toFile().delete() in the cleanup forEach.

🔎 Proposed fix
         // cleanup
-        Files.walk(tmp.toPath()).sorted((a,b)->b.compareTo(a)).forEach(p->p.toFile().delete());
+        Files.walk(tmp.toPath())
+            .sorted((a, b) -> b.compareTo(a))
+            .forEach(p -> {
+                try {
+                    Files.deleteIfExists(p);
+                } catch (IOException ignored) {}
+            });

425-425: Use consistent cleanup pattern.

For consistency with other tests, use Files.deleteIfExists(p) with proper exception handling instead of p.toFile().delete().

🔎 Proposed fix
-        Files.walk(tmp.toPath()).sorted((a,b)->b.compareTo(a)).forEach(p->p.toFile().delete());
+        Files.walk(tmp.toPath())
+            .sorted((a, b) -> b.compareTo(a))
+            .forEach(p -> {
+                try {
+                    Files.deleteIfExists(p);
+                } catch (IOException ignored) {}
+            });

13-13: Replace wildcard import with explicit imports.

Wildcard imports reduce code clarity and can cause naming conflicts. Explicitly import only the classes needed: ByteArrayInputStream, File, FileOutputStream, InputStream, IOException, OutputStream.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75e51c5 and 2314c35.

📒 Files selected for processing (2)
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/test/adapter/PosixAdapterTest.java
💤 Files with no reviewable changes (1)
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/test/adapter/PosixAdapterTest.java
🔇 Additional comments (2)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (2)

346-348: Cleanup pattern looks good.

The directory cleanup now correctly uses reverse-sorted order to delete children before parents, addressing the previous review feedback.


373-375: Cleanup pattern looks good.

The directory cleanup correctly uses reverse-sorted order, addressing the previous review feedback.

Signed-off-by: kouali <kouali@talys.digital>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (2)

322-439: Consider using shared test setup for consistency.

Four tests (lines 322-362, 364-389, 391-423, 425-439) create their own PosixAdapter instances and temporary directories instead of using the shared @BeforeEach setup. This creates inconsistency and duplication.

Unless there's a specific reason these tests need isolated adapter instances, consider refactoring them to use the shared setup from lines 46-63. This would improve maintainability and consistency across the test suite.


422-422: Inconsistent cleanup pattern - use Files.deleteIfExists with exception handling.

Line 422 (and line 438) use .forEach(p->p.toFile().delete()) which can fail silently. Other cleanup blocks (lines 359-361, 386-388) properly use Files.deleteIfExists(p) with try-catch.

🔎 Proposed fix
         // cleanup
-        Files.walk(tmp.toPath()).sorted((a,b)->b.compareTo(a)).forEach(p->p.toFile().delete());
+        Files.walk(tmp.toPath())
+            .sorted((a, b) -> b.compareTo(a))
+            .forEach(p -> { try { Files.deleteIfExists(p); } catch (IOException ignored) {} });

Apply the same change to line 438.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2314c35 and ff9c4db.

📒 Files selected for processing (1)
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java
🔇 Additional comments (1)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (1)

46-76: LGTM! Setup and teardown are properly implemented.

The @beforeeach and @AfterEach methods correctly handle test fixture initialization and cleanup. The reverse-sorted deletion in tearDown ensures children are deleted before parents.

Signed-off-by: kouali <kouali@talys.digital>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (1)

189-198: Fix type mismatch in getMetaData call.

Line 195 incorrectly casts the result to scala.xml.MetaData, but the getMetaData method returns Map<String, Object> (as seen in line 178). This will cause a compilation error or ClassCastException.

🔎 Proposed fix
-        MetaData metaData = (MetaData) adapter.getMetaData(account, container, "s", "p", objectName);
+        Map<String, Object> metaData = adapter.getMetaData(account, container, "s", "p", objectName);
 
         assertNull(metaData, "getMetaData should return null when container is missing");
🧹 Nitpick comments (2)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (2)

307-420: Consider consolidating test fixture setup.

Several tests (lines 307-347, 349-374, 376-404, 406-420) create their own PosixAdapter instances and temporary directories instead of using the shared fixture. While some tests require specific pre-existing files, consolidating common setup would improve maintainability and reduce code duplication.

Consider whether the shared fixture could be extended to support these scenarios, or extract the repeated setup code into helper methods.


391-392: Minor formatting: add blank line before method call.

For consistency with the rest of the file, consider adding a blank line between the comment and the method declaration.

🔎 Suggested formatting
         // ensure account dir does not exist
-       Method m = PosixAdapter.class.getDeclaredMethod("createContainerWithTagging", String.class, String.class, InputStream.class);
+
+        Method m = PosixAdapter.class.getDeclaredMethod("createContainerWithTagging", String.class, String.class, InputStream.class);
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ff9c4db and 4f7efcb.

📒 Files selected for processing (1)
  • kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (1)
kernel/object-store/src/test/java/io/mosip/commons/khazana/util/EncryptionHelperTest.java (1)
  • ExtendWith (14-68)
🔇 Additional comments (1)
kernel/object-store/src/test/java/io/mosip/commons/khazana/impl/PosixAdapterTest.java (1)

42-72: LGTM - Proper setup and teardown.

The setup correctly uses reflection to inject dependencies, and the teardown properly cleans up temporary directories using reverse-sorted traversal to ensure children are deleted before parents.

kouali added 2 commits December 25, 2025 18:11
Signed-off-by: kouali <kouali@talys.digital>
Signed-off-by: kouali <kouali@talys.digital>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant