feat: introduce ChangeDetection strategy for file copy - #397
Conversation
464fca5 to
2b00a7e
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Well-structured refactor — the ChangeDetection enum is a clean replacement for the boolean overwrite flag, and the split into copyUnconditionally / copyIfContentsChanged makes the intent clear. The gracefulBinaryHandling migration from DefaultMavenFileFilter to DefaultMavenResourcesFiltering is correct (no double-handling). A few items below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 4fcc417 — both previous findings are addressed:
- FilteringUtils.java
@param overwrite— Javadoc now accurately documents the mapping (true→ALWAYS,false→CONTENT). ✅ - MavenResourcesExecution.java dead
overwritefield — removed entirely;isOverwrite()/setOverwrite()now delegate throughChangeDetection. ✅
Full re-review of the current diff found no new issues. The switch-based dispatch in FilteringUtils.copyFile handles all five strategies correctly with a defensive default branch. The gracefulBinaryHandling migration from DefaultMavenFileFilter to DefaultMavenResourcesFiltering is clean — the caller catches MavenFilteringException wrapping MalformedInputException and falls back to unfiltered copy with the same ChangeDetection mode. API backward compat is preserved via deprecated method delegation.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 9a3e681 (per-resource changeDetection override).
Previous findings (from first review): both addressed in 4fcc417 ✅
FilteringUtils.java— inaccurate@param overwriteJavadoc → fixed, now documents thetrue→ALWAYS / false→CONTENTmapping.MavenResourcesExecution.java— deadoverwritefield → removed entirely, getters/setters delegate throughChangeDetection.
New commit 9a3e681: Clean. The per-resource changeDetection override in Resource.java is nullable (null = inherit from request-level), and DefaultMavenResourcesFiltering resolves it correctly across all three call sites (flatten check, primary copy, binary-fallback copy). Field visibility is package-private, consistent with other Resource fields. Javadoc is accurate with @since 4.0.0-beta-2.
No new issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after a25457e (accurate copy/skip debug logging).
Previous findings (from first two reviews): all addressed ✅
FilteringUtils.java— inaccurate@param overwriteJavadoc → fixed in 4fcc417.MavenResourcesExecution.java— deadoverwritefield → removed in 4fcc417.
New commit a25457e: adds copyFileWithResult() to MavenFileFilter (returns boolean), refactors DefaultMavenFileFilter.doCopyFile() to propagate the result, and updates DefaultMavenResourcesFiltering to log "Copying file X" vs "Skipping file X (up to date)" based on the actual copy outcome — fixing the pre-existing issue where the debug log always said "Copying" even when the file was skipped by change detection.
The gracefulBinaryHandling catch is correctly moved up to DefaultMavenResourcesFiltering for the copyFileWithResult path (retries with filtering=false), while the old doCopyFile path still handles it for backward-compatible callers via copyFile(MavenFileFilterRequest). No double-handling.
No new issues found.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
a25457e to
dec4590
Compare
…S-397) Replace the boolean overwrite parameter with a ChangeDetection enum that supports ALWAYS, NEVER, TIMESTAMP, CONTENT, and TIMESTAMP_AND_CONTENT modes. - Add ChangeDetection enum with five strategies - Add MavenResourcesExecution.changeDetection field (default: CONTENT) - Add per-resource Resource.changeDetection override - Adapt MavenFileFilter / DefaultMavenFileFilter APIs (boolean overwrite overloads deprecated) - FilteringUtils.copyFile: return boolean (was copied or skipped) - Fix debug log to report actual copy/skip outcome per file
dec4590 to
4baf4b4
Compare
Replace the deprecated boolean 'overwrite' parameter with a 'changeDetection' strategy enum (CONTENT / TIMESTAMP / TIMESTAMP_AND_CONTENT / ALWAYS / NEVER), forwarded from maven-filtering 4.0.0-beta-2-SNAPSHOT (PR apache/maven-filtering#397). - Add 'changeDetection' @parameter (default CONTENT, i.e. current behaviour) - Deprecate 'overwrite'; bridge: overwrite=true maps to ALWAYS, overwrite=false keeps the declared changeDetection value (backward-compat) - Add IT MRESOURCES-453: verifies that changeDetection=NEVER leaves an existing destination file untouched even when source content differs Fixes: #453 Depends on: apache/maven-filtering#397
Summary
Port of the
ChangeDetectionstrategy from themaven-filtering-3.xbranch (commit d9bc10a) to themasterbranch (Maven 4 / Path-based API).Instead of jumping between ways to perform change detection — deciding whether an existing target file needs to be overwritten — make it a configurable strategy. Before 3.4.0 it was timestamp-based, post 3.4.0 it switched to content-based via
CachingOutputStream, but in any case users were left with no control.Changes
New
ChangeDetectionenumFive strategies:
TIMESTAMPCONTENTCachingOutputStream/CachingWriter(current default)TIMESTAMP_AND_CONTENTALWAYSoverwrite=true)NEVERAPI changes
AbstractMavenFilteringRequest— newchangeDetectionfield (defaultCONTENT) with getter/setterFilteringUtils— newcopyFile(Path, Path, String, FilterWrapper[], ChangeDetection)overload returningboolean(true=written, false=skipped); oldboolean overwritesignature deprecatedMavenFileFilter— newcopyFile(…, ChangeDetection)andcopyFileWithResult(…, ChangeDetection)methods; old signature deprecatedDefaultMavenFileFilter— implements new interface methods, returnsbooleanindicating whether the destination was writtenDefaultMavenResourcesFiltering— usesgetChangeDetection()from the request instead ofisOverwrite(); per-resource override takes precedence over request-level setting; emits "Copying file X" or "Skipping file X (up to date)" based on the actual copy outcomeMavenResourcesExecution—isOverwrite()/setOverwrite()deprecated, now delegate throughChangeDetectionResource— newchangeDetectionfield so individual<resource>elements can declare their own strategy, overriding the request-level defaultImplementation details
CONTENT/TIMESTAMP_AND_CONTENTuseCachingOutputStream/CachingWriter(existing behaviour)ALWAYS/TIMESTAMP/NEVERbypass caching and write unconditionally viaFiles.newOutputStream/Files.newBufferedWriterFiles.getLastModifiedTime(path).toMillis()for Path-based comparisonchangeDetectionwins when set; falls back to the request-level value otherwisebuildContext.refresh()is always called for processed outputs so incremental build contexts (m2e / Eclipse) correctly track them regardless of whether the file was skippedFixes
changeDetectiononResource)Related
Plugin-side companion PR: apache/maven-resources-plugin#527
Origin
Ported from maven-filtering-3.x commit d9bc10a by Tamas Cservenak.