clear core cache

[andiwand]

No description provided.

[claude]

Claude finished @andiwand's task —— View job

---

Pull Request Review: Clear Core Cache

Summary

This PR introduces a separate cache directory for the ODR core library's temporary files. This is a good architectural improvement that better isolates cache data from output data.

Code Quality and Best Practices ✅

Positive aspects:

• Clean separation of concerns between output and cache directories
• Consistent naming convention (core_output and core_cache)
• Proper cache cleanup with filesystem operations
• Addition of std::error_code for error handling

Suggestions:

1. Error handling in C++: While you've added std::error_code ec in CoreWrapper.cpp, it's only used with std::filesystem::remove_all() but the error code is never checked. Consider logging failures:

   std::filesystem::remove_all(cachePathCpp, ec);
   if (ec) {
       __android_log_print(ANDROID_LOG_WARN, "smn", "Failed to remove cache directory: %s", ec.message().c_str());
   }

Potential Bugs or Issues ⚠️

1. Directory creation without error checking: In CoreWrapper.cpp, std::filesystem::create_directories(cachePathCpp) is called without error handling. If directory creation fails, the subsequent operations may fail unexpectedly.

2. Missing null/empty path validation: The cachePath parameter is extracted from Java but never validated before use. Consider adding validation:

   if (cachePathCpp.empty()) {
       env->SetIntField(result, errorField, -3);
       return result;
   }

Performance Considerations 🚀

1. Cache directory lifecycle: The current implementation removes and recreates the cache directory for each operation. This is safe but may have performance implications for frequent operations. Consider if the cache could be reused across operations when appropriate.

2. Filesystem operations: Multiple filesystem operations (remove_all, create_directories) are performed synchronously. For large cache directories, this could block the calling thread.

Security Concerns 🔒

1. Path traversal: While the cache path comes from controlled sources in this code, ensure that user-controlled paths cannot be injected elsewhere in the application that might lead to path traversal vulnerabilities.

2. Temporary file permissions: The code creates temporary directories but doesn't explicitly set permissions. On Android this is generally handled by the app sandbox, but it's worth noting.

Test Coverage 📋

Current coverage:

• ✅ Basic test updated to use the new cache path
• ✅ Test verifies successful parsing with cache directory

Missing coverage:

• ❌ No test for cache directory creation failure
• ❌ No test for concurrent access to cache directory
• ❌ No verification that cache is actually cleaned up after operations

Suggested test additions:

@Test
public void testCacheDirectoryCleanup() {
    // Test that cache directory is removed after operation
    File cacheDir = new File(context.getCacheDir(), "core_cache");
    assertFalse(cacheDir.exists() || cacheDir.list().length == 0);
}

@Test
public void testInvalidCachePath() {
    // Test handling of invalid cache paths
}

Additional Notes

• The removal of lastCoreOptions from RawLoader.java is a good cleanup since it wasn't being used
• The changes are backward compatible as they only add a new field to CoreOptions
• Consider documenting the purpose and lifecycle of the cache directory in code comments

Recommendation

Approve with suggestions - The core functionality is sound and improves the architecture. The suggested improvements around error handling and test coverage should be addressed in a follow-up PR.