feat(#4846): implement a simple caching mechanism in the Maven plugin#4847
Conversation
📝 WalkthroughWalkthroughAdds a package-private Changes
Sequence Diagram(s)sequenceDiagram
participant Source as Source File
participant Cache as Cache Instance
participant Storage as Hash & Cache Storage
participant Comp as Compilation Function
participant Target as Target File
Source->>Cache: apply(source, target, tail)
Cache->>Source: compute SHA-256 (Base64)
Cache->>Storage: read stored hash & cached content
alt hash exists and matches
Storage-->>Cache: return cached content
Cache->>Target: write cached content
else missing or mismatch
Storage-->>Cache: missing or differs
Cache->>Comp: invoke compilation(source)
Comp-->>Cache: return compiled content
Cache->>Storage: write new hash and cached content
Cache->>Target: write compiled content
end
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)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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: 4
🤖 Fix all issues with AI agents
In `@eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java`:
- Around line 58-59: The cache branch in Cache currently assumes cfile exists
and calls Files.readString(cfile), causing a failure if the cached file was
deleted; update the cache-hit condition (in class Cache where cfile and target
are handled) to treat Files.notExists(cfile) as a cache miss (i.e., include
Files.exists(cfile) in the hit check or add Files.notExists(cfile) to the miss
condition) so you only call Files.readString(cfile) when the file actually
exists and otherwise recompile/write the fresh content to target.
- Around line 55-56: The write calls Files.writeString(this.hash(tail), hash)
and Files.writeString(cfile, content) can fail if target parent directories
don't exist; in the Cache class ensure you create parent directories before
writing by invoking Files.createDirectories(...) on the parent path(s) of
this.hash(tail) and of cfile (or their resolved Paths) prior to the
Files.writeString calls so the directory tree exists.
- Around line 76-78: In Cache.hash, avoid path-escape by resolving then
normalizing the tail and enforcing it remains under base: compute full =
this.base.resolve(tail).normalize(), then verify full.startsWith(this.base) and
throw an IllegalArgumentException (or similar) if it does not; finally return
full.getParent().resolve(String.format("%s.sha256",
full.getFileName().toString())). This ensures absolute tails or '/../' segments
cannot escape the cache base.
- Line 54: In Cache.fix the compilation call: the
UncheckedFunc<>(this.compilation).apply(...) is currently passed target but
should be passed source so the compilation function reads the original source
file (variable name source) rather than the destination target; update the call
that creates content to use source and ensure it still assigns to final String
content so downstream logic (hashing/target writing) operates on compiled output
from this.compilation.
🧹 Nitpick comments (1)
eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java (1)
88-90: Stream hashing to avoid loading large files into memory.
Files.readAllBytesloads the full source into RAM. For large sources, a streaming digest is more memory‑friendly.♻️ Suggested refactor
- final byte[] hash = digest.digest(Files.readAllBytes(file)); - return Base64.getEncoder().encodeToString(hash); + try (var input = Files.newInputStream(file); + var dis = new java.security.DigestInputStream(input, digest)) { + final byte[] buffer = new byte[8192]; + while (dis.read(buffer) != -1) { + // consume stream + } + } + return Base64.getEncoder().encodeToString(digest.digest());
| Files.writeString(this.hash(tail), hash); | ||
| Files.writeString(cfile, content); |
There was a problem hiding this comment.
Create parent directories before writing cache files.
Lines 55–56 will throw NoSuchFileException when tail includes subdirectories. Ensure the cache parent directory exists before writing hash/content.
✅ Proposed fix
final Path hfile = this.hash(tail);
final Path cfile = this.base.resolve(tail);
+ final Path parent = cfile.getParent();
+ if (parent != null) {
+ Files.createDirectories(parent);
+ }
if (Files.notExists(hfile) || !Files.readString(hfile).equals(hash)) {🤖 Prompt for AI Agents
In `@eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java` around lines 55 -
56, The write calls Files.writeString(this.hash(tail), hash) and
Files.writeString(cfile, content) can fail if target parent directories don't
exist; in the Cache class ensure you create parent directories before writing by
invoking Files.createDirectories(...) on the parent path(s) of this.hash(tail)
and of cfile (or their resolved Paths) prior to the Files.writeString calls so
the directory tree exists.
There was a problem hiding this comment.
@volodya-lombrozo this comment sounds reasonable to me
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| private Path hash(final Path tail) { | ||
| final Path full = this.base.resolve(tail); | ||
| return full.getParent().resolve(String.format("%s.sha256", full.getFileName().toString())); |
There was a problem hiding this comment.
Prevent cache path escape via absolute or .. tail.
Line 77 resolves tail directly; if tail is absolute or contains .., the cache can write outside base. Normalize and enforce that the resolved path stays within base.
🔒 Proposed fix
- final Path full = this.base.resolve(tail);
- return full.getParent().resolve(String.format("%s.sha256", full.getFileName().toString()));
+ final Path base = this.base.normalize();
+ final Path full = base.resolve(tail).normalize();
+ if (!full.startsWith(base)) {
+ throw new IllegalArgumentException("Cache tail must be within base directory");
+ }
+ return full.getParent().resolve(String.format("%s.sha256", full.getFileName().toString()));🤖 Prompt for AI Agents
In `@eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java` around lines 76 -
78, In Cache.hash, avoid path-escape by resolving then normalizing the tail and
enforcing it remains under base: compute full =
this.base.resolve(tail).normalize(), then verify full.startsWith(this.base) and
throw an IllegalArgumentException (or similar) if it does not; finally return
full.getParent().resolve(String.format("%s.sha256",
full.getFileName().toString())). This ensures absolute tails or '/../' segments
cannot escape the cache base.
There was a problem hiding this comment.
@volodya-lombrozo this comment sounds reasonable to me
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@yegor256 could you have a look, please? This is a new caching mechanism proposal. It uses hashing instead of relying on timestamps. Here is the explanation why it's a bad idea to rely on time: |
volodya-lombrozo
force-pushed
the
4846-new-cache
branch
from
7837369 to
1a1eb05
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Cache class intended to provide hash-based caching for the EO Maven plugin. The PR is related to issue #4846 about inadequate caching implementation.
Changes:
- Added a new
Cacheclass with SHA-256 hash-based cache validation mechanism - Added comprehensive unit tests for the Cache class in
CacheTest
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java | New cache implementation using SHA-256 hashes to validate cache freshness and avoid timestamp-based validation |
| eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java | Test suite for the new Cache class covering compilation, cache hits, and cache invalidation scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void apply(final Path source, final Path target, final Path tail) { | ||
| try { | ||
| final String hash = Cache.sha(source); | ||
| final Path hfile = this.hash(tail); | ||
| final Path cfile = this.base.resolve(tail); | ||
| if (Files.notExists(hfile) || !Files.readString(hfile).equals(hash)) { | ||
| final String content = new UncheckedFunc<>(this.compilation).apply(source); | ||
| Files.writeString(this.hash(tail), hash); | ||
| Files.writeString(cfile, content); | ||
| Files.writeString(target, content); | ||
| } else { | ||
| Files.writeString(target, Files.readString(cfile)); | ||
| } |
There was a problem hiding this comment.
The Cache class lacks thread-safety mechanisms. If multiple threads call the apply method concurrently with the same source file, there could be race conditions where both threads determine the cache is stale and attempt to write simultaneously, potentially leading to corrupted files or inconsistent state. Consider adding synchronization or using atomic file operations. See ChCachedTest.java:63-86 for an example of how concurrency is tested in similar caching classes in this codebase.
There was a problem hiding this comment.
@volodya-lombrozo this comment sounds reasonable to me
| */ | ||
| private static String sha(final Path file) throws NoSuchAlgorithmException, IOException { | ||
| final MessageDigest digest = MessageDigest.getInstance("SHA-256"); | ||
| final byte[] hash = digest.digest(Files.readAllBytes(file)); |
There was a problem hiding this comment.
The sha method reads the entire file into memory using Files.readAllBytes(file) which could cause OutOfMemoryError for large files. Consider using a streaming approach with MessageDigest.update() in a loop to hash the file in chunks, similar to how it's typically done for large file hashing operations.
| final byte[] hash = digest.digest(Files.readAllBytes(file)); | |
| try (var stream = Files.newInputStream(file)) { | |
| final byte[] buf = new byte[8192]; | |
| int read = stream.read(buf); | |
| while (read != -1) { | |
| digest.update(buf, 0, read); | |
| read = stream.read(buf); | |
| } | |
| } | |
| final byte[] hash = digest.digest(); |
There was a problem hiding this comment.
@volodya-lombrozo this comment sounds reasonable to me
| */ | ||
| private Path hash(final Path tail) { | ||
| final Path full = this.base.resolve(tail); | ||
| return full.getParent().resolve(String.format("%s.sha256", full.getFileName().toString())); |
There was a problem hiding this comment.
If the resolved path has no parent directory (e.g., when tail is just a filename without any parent), full.getParent() will return null, causing a NullPointerException when calling resolve on it. Consider handling this case or ensuring that tail always has a parent directory.
| return full.getParent().resolve(String.format("%s.sha256", full.getFileName().toString())); | |
| final Path parent = full.getParent() == null ? this.base : full.getParent(); | |
| return parent.resolve(String.format("%s.sha256", full.getFileName().toString())); |
| final Path hfile = this.hash(tail); | ||
| final Path cfile = this.base.resolve(tail); |
There was a problem hiding this comment.
According to the custom coding guideline, compound variable names should be avoided in favor of single-word nouns. The variables 'hfile' and 'cfile' are compound abbreviations. Consider renaming them to single-word nouns like 'signature' and 'cached' respectively.
There was a problem hiding this comment.
@volodya-lombrozo this comment sounds reasonable to me
| Files.writeString(this.hash(tail), hash); | ||
| Files.writeString(cfile, content); | ||
| Files.writeString(target, content); |
There was a problem hiding this comment.
The parent directories for the hash file, cache file, and target file may not exist when attempting to write. This will cause an IOException. Consider creating parent directories before writing, similar to how the Saved class does it with this.target.toFile().getParentFile().mkdirs() at line 77 in Saved.java.
There was a problem hiding this comment.
@volodya-lombrozo this comment sounds reasonable to me
🚀 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
|
volodya-lombrozo
force-pushed
the
4846-new-cache
branch
from
84dd9fd to
fb0e998
Compare
|
@yegor256 could you have a look one more time, please? |
|
@volodya-lombrozo Thank you for your contribution! You've earned +12 points for this submission: +16 as the base reward, +9.65 for 193 hits-of-code (0.05 × 193), and -4 deducted for exceeding 100 hits-of-code, then capped at +12 per policy. Please keep them coming! Your running score is +284; don't forget to check your Zerocracy account too. |
|
@yegor256 Thanks for the excellent review! 🎉 You've earned +7 points (+12 base, -5 for minimal comments). Your total score is now +142 - keep up the great work! Don't forget to check your Zerocracy account for updates. |
This PR introduces a new
Cacheclass for improved caching functionality in theeo-maven-plugin.Related to #4846
Summary by CodeRabbit
New Features
Tests