fix: restore first resource entry wins for overlapping declarations - #398

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.

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.

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.