feat(#4983): implement SHA-256 hashing for directories in Cache#4984
Conversation
📝 WalkthroughWalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Client as "Client"
participant Cache as "Cache"
participant FS as "FileSystem"
participant MD as "MessageDigest"
Client->>Cache: apply(path)
Cache->>FS: isDirectory(path)?
alt directory
Cache->>FS: Files.walk(path) -> list regular files
FS-->>Cache: files []
Cache->>Cache: sort files by path
loop each file
Cache->>FS: readAllBytes(file)
FS-->>Cache: bytes
Cache->>MD: update(bytes)
end
else file
Cache->>FS: readAllBytes(path)
FS-->>Cache: bytes
Cache->>MD: update(bytes)
end
Cache->>MD: digest()
MD-->>Cache: hash bytes
Cache->>Cache: Base64.encode(hash)
Cache-->>Client: Base64-encoded SHA-256
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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)
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 |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
There was a problem hiding this comment.
Pull request overview
This PR extends Cache to support SHA-256 hashing for directories (in addition to files) so cache invalidation can work when the cache source is a folder (needed for upcoming WPA analysis).
Changes:
- Add directory hashing (
dirSha) and route hashing based on whether the source path is a file or directory. - Refactor file hashing to wrap checked exceptions as
IllegalStateException. - Add a unit test covering folder hashing behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java |
Adds directory hashing and refactors file hashing/exception handling. |
eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java |
Adds a new test for hashing a folder with multiple files (plus a small refactor to reuse a helper). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new Cache(cache, p -> result).apply( | ||
| source, | ||
| temp.resolve("out.txt"), | ||
| source.resolve("folder-cache") |
There was a problem hiding this comment.
In this test, the tail argument is passed as source.resolve("folder-cache"), which is an absolute path. Cache.apply() uses base.resolve(tail), and when tail is absolute it will ignore the cache base and write outside the cache directory, so this assertion will be checking the wrong location (and the cache implementation can be misused to escape the cache root). Use a relative tail (e.g., Paths.get("folder-cache") or source.getFileName() depending on intended cache key).
| source.resolve("folder-cache") | |
| Path.of("folder-cache") |
| } | ||
|
|
||
| @Test | ||
| void generateCorrectHashForEntireFolderWithSeveralFiles( |
There was a problem hiding this comment.
Test method name generateCorrectHashForEntireFolderWithSeveralFiles is inconsistent with the existing naming pattern in this class (e.g., generatesCorrectHashForTinyFile, generatesCorrectHashForLargeFile). Consider renaming to keep a consistent verb tense (likely generates...).
| void generateCorrectHashForEntireFolderWithSeveralFiles( | |
| void generatesCorrectHashForEntireFolderWithSeveralFiles( |
| return full.getParent().resolve(String.format("%s.sha256", full.getFileName().toString())); | ||
| } |
There was a problem hiding this comment.
hash(tail) uses this.base.resolve(tail.normalize()), and Path.resolve(...) will ignore base when tail is absolute. This allows callers to make the cache read/write outside the cache root (and similarly, .. segments can escape after normalization). Consider validating that tail is relative and that the resolved path stays within base before using it to construct cache/hash paths.
| private static String sha(final Path any) { | ||
| final String result; | ||
| if (Files.isDirectory(any)) { | ||
| result = Cache.dirSha(any); | ||
| } else if (Files.isRegularFile(any)) { | ||
| result = Cache.fileSha(any); | ||
| } else { | ||
| throw new IllegalArgumentException( | ||
| String.format("Path '%s' is neither a regular file nor a directory", any) | ||
| ); | ||
| } |
There was a problem hiding this comment.
sha(Path) now throws IllegalArgumentException when the path doesn't exist (neither regular file nor directory). This is a behavior change compared to the rest of Cache.apply()/fileSha() which wrap filesystem problems into IllegalStateException, and it also bypasses the apply() catch that only wraps IOException. Consider treating non-existent/unreadable paths as an I/O failure (wrap into IllegalStateException with a consistent message) instead of IllegalArgumentException.
| .map(Cache::fileSha) | ||
| .map(s -> s.getBytes(StandardCharsets.UTF_8)) | ||
| .forEach(digest::update); |
There was a problem hiding this comment.
dirSha() currently hashes only the contents of regular files (via fileSha) and ignores their relative paths. This means renaming a file (without changing its bytes) will not change the directory hash (e.g., a single-file directory), which can leave stale cache entries when directory structure changes. Consider incorporating each file’s relative path (and a separator) into the digest update along with its content hash so renames/moves inside the directory also invalidate the cache.
| .map(Cache::fileSha) | |
| .map(s -> s.getBytes(StandardCharsets.UTF_8)) | |
| .forEach(digest::update); | |
| .forEach( | |
| path -> { | |
| digest.update( | |
| dir.relativize(path).toString().getBytes(StandardCharsets.UTF_8) | |
| ); | |
| digest.update((byte) 0); | |
| digest.update(fileSha(path).getBytes(StandardCharsets.UTF_8)); | |
| digest.update((byte) 0); | |
| } | |
| ); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java`:
- Around line 210-226: The test uses an absolute Path for the tail argument
causing Cache to treat it as absolute and write the .sha256 next to the source
instead of under the cache; change the third argument passed to Cache.apply from
source.resolve("folder-cache") to a relative Path (e.g.,
Paths.get("folder-cache")) so Cache.base.resolve(tail) places the hash under the
cache directory, and also rename the test method from
generateCorrectHashForEntireFolderWithSeveralFiles to
generatesCorrectHashForEntireFolderWithSeveralFiles to match naming conventions;
locate the call site in the test method and update the tail Path and method name
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bc269ff-50b8-41d1-a8c6-0cb631a6d1c6
📒 Files selected for processing (2)
eo-maven-plugin/src/main/java/org/eolang/maven/Cache.javaeo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java (1)
217-229: Strengthen the folder-hash test to truly validate sort determinism.Right now filename order and write order both increase (
file1, thenfile2), so the test can still pass if explicit sorting is accidentally removed and traversal returns insertion order.♻️ Suggested test hardening
- final String first = "file1 content"; - final String second = "file2 content"; + final String first = "file-a content"; + final String second = "file-b content"; final String result = String.format("%s%s", first, second); - Files.writeString(source.resolve("file1.txt"), first, StandardCharsets.UTF_8); - Files.writeString(source.resolve("file2.txt"), second, StandardCharsets.UTF_8); + Files.writeString(source.resolve("b.txt"), second, StandardCharsets.UTF_8); + Files.writeString(source.resolve("a.txt"), first, StandardCharsets.UTF_8); new Cache(cache, p -> result).apply( source, temp.resolve("out.txt"), source.getFileName() ); final MessageDigest instance = MessageDigest.getInstance("SHA-256"); instance.update(CacheTest.hash(first).getBytes(StandardCharsets.UTF_8)); instance.update(CacheTest.hash(second).getBytes(StandardCharsets.UTF_8));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java` around lines 217 - 229, The test currently writes files in the same order as their sorted names, so change it to write them in a non-sorted order (e.g., write "file2.txt" then "file1.txt" or use names like "b.txt" and "a.txt") and then compute the expected folder hash in a deterministic, sorted-filenames way before comparing; specifically, when building the expected MessageDigest use CacheTest.hash(...) but apply the updates in sorted filename order rather than creation order so the assertion verifies that Cache.apply (the Cache class) is deterministic regardless of traversal order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java`:
- Around line 217-229: The test currently writes files in the same order as
their sorted names, so change it to write them in a non-sorted order (e.g.,
write "file2.txt" then "file1.txt" or use names like "b.txt" and "a.txt") and
then compute the expected folder hash in a deterministic, sorted-filenames way
before comparing; specifically, when building the expected MessageDigest use
CacheTest.hash(...) but apply the updates in sorted filename order rather than
creation order so the assertion verifies that Cache.apply (the Cache class) is
deterministic regardless of traversal order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 770156e8-d5c6-4826-ad22-d8a60a815116
📒 Files selected for processing (1)
eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java
|
@yegor256 could you have a look, please? |
|
@volodya-lombrozo Hey! Nice work on that contribution! 🎉 You snagged +12 points this time: started with the base +16, but had to dock -4 since you hit 129 hits-of-code (anything over 100 gets a penalty). For next time, try to keep those contributions a bit more focused - you'll max out the HoC bonus at 16 points when you stay under 100 lines. Your running score is looking solid at +254, so keep the momentum going and don't forget to check your Zerocracy account! |
This PR allows to compute SHA-256 from entire folders. It will be used later for WPA analysis.
Related to #4983
Summary by CodeRabbit
New Features
Tests