[gnodet]

Summary

Restore "first <resource> entry wins" semantics for overlapping resource declarations.

Port of maven-filtering-3.x commits 306db11 and 6d84663 by Richard Zowalla.

Problem

When two <resource> entries cover the same source directory — one with filtering=true and a narrow <include>, the other with filtering=false and <include>**</include> — the placeholder in the file selected by the first entry is no longer replaced. This regression was introduced in maven-filtering 3.3.2 when MSHARED-1216 switched to CachingOutputStream/CachingWriter: every write now goes through unconditionally if the bytes differ, so the unfiltered second entry silently clobbers the filtered first entry.

<!-- This configuration stopped working correctly -->
<resource>
  <filtering>true</filtering>
  <directory>src/main/resources</directory>
  <includes><include>config/filtered.xml</include></includes>
</resource>
<resource>
  <filtering>false</filtering>
  <directory>src/main/resources</directory>
  <includes><include>**</include></includes>
</resource>

Fix

DefaultMavenResourcesFiltering.filterResources() now tracks destination paths already written within a single call (Set<Path> alreadyCopied) and skips subsequent writes that would land on the same path.

Explicit opt-in via MavenResourcesExecution.setOverwrite(true) preserves the previous "second entry clobbers" behaviour for callers that relied on it. The existing flatten-collision policy (warn-or-throw) is left untouched since that is a conceptually distinct case.

Tests

• OverlappingResourcesFilteringTest — new test reproducing the MRP-471 regression: asserts that config/filtered.xml has its placeholder replaced while static/keep-as-is.txt retains its literal placeholder
• DefaultMavenResourcesFilteringTest.overlappingEntriesWithExplicitOverwriteStillReplace() — verifies that setOverwrite(true) still allows the second entry to clobber the first

Fixes

• Filtering changed behaviour from 3.3.1 maven-resources-plugin#471

[gnodet-bot]

Review of fix: restore first resource entry wins for overlapping declarations

Clean, well-scoped fix. The approach — tracking already-written destinations in a Set<Path> and skipping subsequent writes — is the right level of abstraction. Good test coverage with both the regression test and the overwrite-bypass guard.

Correctness notes:

1. toAbsolutePath() vs toRealPath() — the code uses destinationFile.toAbsolutePath() for tracking. This is fine in practice: the output directory is computed deterministically from getDestinationFile(), so two resource entries pointing to the same logical destination will produce identical absolute paths. toRealPath() would also resolve symlinks, but it requires the file to exist and would throw before the first write. No issue here.

2. Flatten bypass — correctly excluded from the skip logic. The existing flatten collision policy (warn-or-throw) is a distinct concern and is reached at L280+ as before. Good.

3. Test validity — overlappingResourceEntriesShouldStillFilter() would genuinely fail without the fix: the second (unfiltered) entry would overwrite config/filtered.xml with the raw ${repro.value}, and the assertTrue(filteredContent.contains("REPLACED_BY_FILTERING")) assertion would trip. The complementary static/keep-as-is.txt assertion guards against an overly broad fix. Solid regression test design.

One question on the overwrite=true semantics:

The overlappingEntriesWithExplicitOverwriteStillReplace test asserts that with overwrite=true, the unfiltered second entry wins over the filtered first entry — the output contains the raw ${repro.value}. The comment references "ChangeDetection.ALWAYS", but that enum lives in PR #397 which is still open and not yet on master. On current master, isOverwrite() is the only signal.

Is this actually the desired user-facing behavior? A user setting overwrite=true on MavenResourcesExecution likely wants "always copy, even if timestamps match" — not necessarily "let the last <resource> entry clobber the first". In the overlapping-resources scenario, the user's intent is clearly that config/filtered.xml should be filtered (that's why they declared the narrow filtered entry first). Having overwrite=true silently undo that filtering feels like a footgun.

That said, this preserves 3.3.1 behavior and the overwrite flag is rarely used directly by end users (it's more of a plugin-level control). If the intent is "don't break any existing caller that relied on overwrite semantics", the current logic is defensible. Just flagging this for awareness.

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

[gnodet-bot]

Re-reviewed after be9bc56. Both nits from the previous review are addressed:

1. DefaultMavenResourcesFilteringTest comment — forward reference to ChangeDetection.ALWAYS replaced with overwrite=true semantics. ✅
2. DefaultMavenResourcesFiltering inline comment — was already clean (only mentioned overwrite), no change needed. ✅

Core logic unchanged and solid. The Set<Path> alreadyCopied approach correctly preserves "first entry wins" semantics while respecting both the flatten collision policy and the explicit isOverwrite() escape hatch. Test coverage is thorough — regression test + overwrite-bypass guard.

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

[gnodet]

🟢 Babysit status: CI green (16/16), review approved, all findings addressed. Ready to merge.

[gnodet-bot]

Re-reviewed after rebase onto current master (c590600).

Rebase integrated cleanly with the newly merged features from #395 (nonFilteredFiles glob exclusions and gracefulBinaryHandling). The alreadyCopied skip logic at L275-280 is orthogonal to both — it fires before the filteredExt/filteredGlob/gracefulBinaryHandling path, so the two features compose correctly without interference.

Previous findings (forward references to ChangeDetection.ALWAYS) remain addressed — no such references in the rebased diff. Core logic, test coverage, and test fixtures are unchanged. Solid.

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