Skip to content

[MRESOURCES-453] Leverage ChangeDetection from maven-filtering #397 - #527

Open
gnodet wants to merge 3 commits into
masterfrom
fix/change-detection
Open

gnodet wants to merge 3 commits into
masterfrom
fix/change-detection

Conversation

@gnodet

@gnodet gnodet commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Port of the ChangeDetection strategy from apache/maven-filtering#397 into the plugin.

Instead of a binary overwrite boolean, expose the new changeDetection enum parameter so users have explicit control over the copy semantics:

Value Behaviour
CONTENT (default) Only overwrite when content differs — i.e. current post-3.4.0 behaviour via CachingOutputStream
TIMESTAMP Only overwrite when source is newer than destination — i.e. pre-3.4.0 behaviour
TIMESTAMP_AND_CONTENT Combine both checks
ALWAYS Always overwrite (replaces <overwrite>true</overwrite>)
NEVER Never overwrite an existing destination file

Changes

  • pom.xml: bump mavenFilteringVersion to 4.0.0-beta-2-SNAPSHOT (requires feat: introduce ChangeDetection strategy for file copy maven-filtering#397 to be merged and released first)
  • ResourcesMojo: add changeDetection @Parameter (default CONTENT); deprecate overwrite with backward-compat bridge (overwrite=true → ALWAYS; overwrite=false defers to the declared changeDetection)
  • New IT MRESOURCES-453: verifies changeDetection=NEVER leaves an existing destination file untouched even when source content differs

Fixes

The root cause is in maven-filtering (the overwrite parameter was silently dropped when the NIO/CachingOutputStream migration landed). This PR is the plugin-side companion to apache/maven-filtering#397.

Related

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

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clean and well-structured change. The backward-compat bridge (overwrite=true → ALWAYS) is correct, the Javadoc is thorough, and the IT covers the NEVER edge case properly with setup/verify scripts.

One minor documentation nit below.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

* <li>{@code NEVER} — never overwrite an existing destination file.</li>
* </ul>
*
* @since 3.4.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 @since 3.4.0 is wrong — this parameter is introduced in this PR, and the project version is 4.0.0-beta-2-SNAPSHOT. The @since should reflect the actual release version.

Suggested change
* @since 3.4.0
* @since 4.0.0-beta-2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 83ca419.

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review after new commit 476787e (CI: enable Apache Snapshots repo for SNAPSHOT dependency). The CI change is correct — needed for the maven-filtering 4.0.0-beta-2-SNAPSHOT dependency.

However, the @since finding from the previous review is still unaddressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

* <li>{@code NEVER} — never overwrite an existing destination file.</li>
* </ul>
*
* @since 3.4.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Still unresolved from previous review: @since 3.4.0 is incorrect — this parameter is introduced in this PR. The project version is 4.0.0-beta-2-SNAPSHOT, so this should be @since 4.0.0-beta-2.

Suggested change
* @since 3.4.0
* @since 4.0.0-beta-2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 83ca419.

@gnodet

gnodet commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

CI failure: ChangeDetection class not found in maven-filtering 4.0.0-beta-2-SNAPSHOT.

The prerequisite PR apache/maven-filtering#397 (which introduces the ChangeDetection enum) has not been merged yet. The current SNAPSHOT deployed to Apache Snapshots (build -100, Sep 24) does not contain this class.

This PR cannot compile until maven-filtering#397 is merged and a new SNAPSHOT is deployed.

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review after commit 476787e (CI: add -Dapache.snapshots for SNAPSHOT dependency). CI change is correct and necessary.

The @since finding from both previous reviews remains unaddressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

* <li>{@code NEVER} — never overwrite an existing destination file.</li>
* </ul>
*
* @since 3.4.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Still unresolved (3rd review): @since 3.4.0 is incorrect — the changeDetection parameter is introduced in this PR. The milestone is 4.0.0-beta-2.

Suggested change
* @since 3.4.0
* @since 4.0.0-beta-2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 83ca419.

@gnodet

gnodet commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

CI failure on 4.0.0-rc-7: ChangeDetection class not found in org.apache.maven.shared.filtering.

This is expected — this PR depends on apache/maven-filtering#397 being merged and deployed as a SNAPSHOT first. That PR is open with CI green, awaiting review/merge.

Once maven-filtering#397 is merged and the SNAPSHOT is published, CI here will pass.

@gnodet-bot gnodet-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review after commit 83ca419 — the @since tag is now correctly set to 4.0.0-beta-2. Previous finding addressed.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

@gnodet

gnodet commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

⚠️ CI blocked on upstream dependency

The compilation fails because ChangeDetection class does not exist in the current maven-filtering 4.0.0-beta-2-SNAPSHOT. This class is introduced by apache/maven-filtering#397, which is still open (CI is green, awaiting merge).

Once maven-filtering#397 is merged and the snapshot is deployed, this PR's CI should pass.

— Hermès 🤖

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.

2 participants