Fix #12430: mvnup upgrade strategies and compatibility improvements#12454
Fix #12430: mvnup upgrade strategies and compatibility improvements#12454gnodet wants to merge 17 commits into
Conversation
…o mvnup Since Maven 3.8.1+, HTTP repositories are blocked by the maven-default-http-blocker mirror. This adds a RepositoryHttpsUpgradeStrategy that automatically converts http:// repository URLs to https:// in POM files. Well-known repositories (Apache snapshots/releases, Maven Central variants) are mapped to their canonical HTTPS equivalents. All other http:// URLs get a generic scheme upgrade. Handles repositories, pluginRepositories, distributionManagement (both repository and snapshotRepository), and profile-scoped repositories. Includes 27 tests covering well-known URL normalization, generic http->https conversion, all POM sections, profile-scoped repos, and edge cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… dedup last-wins Add EnforcerVersionRangeStrategy that widens requireMavenVersion ranges excluding Maven 4 (e.g. [3.8.8,4) → [3.8.8,5)) in the maven-enforcer-plugin. Handles top-level and per-execution configurations, pluginManagement, and profile-scoped declarations. Fix CompatibilityFixStrategy dependency/plugin deduplication to use last-wins semantics (matching Maven 3 runtime behavior) instead of first-wins. Update log messages to include the kept version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
changed the title
Add PluginUpgrade entries for quarkus-maven-plugin (both io.quarkus and io.quarkus.platform groupIds) with minimum version 3.26.0. When the plugin version references the same property as a Quarkus BOM, the strategy decouples it by introducing a separate quarkus-plugin.version property. After upgrading, emits a warning if the platform BOM version is significantly older than the plugin version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…maven.config mvnup inherits the Maven launcher's argument handling, which appends options from .mvn/maven.config. That file often contains build flags like -ntp, -U, or -T that mvnup does not recognize, causing a ParseException before any upgrades are applied. Override CLIManager.parse() to catch UnrecognizedOptionException and retry after removing the offending option, so mvnup works in projects with a .mvn/maven.config file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gmavenplus-plugin (all versions up to 4.1.1) calls mutating methods on immutable lists returned by the Maven 4 API, causing UnsupportedOperationException on goals like removeStubs. Add a warning-only check to CompatibilityFixStrategy that detects known incompatible plugins and emits a warning with a link to the upstream issue tracker. The check does not modify the POM. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Maven 4's prefix-based artifact filtering may block resolution from repositories that do not publish a prefixes.txt file. When Maven 4 auto-generates an incomplete prefix list, legitimate artifacts are rejected with ArtifactFilteredOutException. Add a warning for non-Maven-Central repositories detected in POMs, advising users to check prefix configuration if builds fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Maven 4 validates module paths during POM parsing, before property
interpolation occurs. Paths like <module>spark-${spark.version}</module>
are rejected because the literal string (with the unresolved
expression) does not correspond to an existing directory.
Add a warning for module/subproject elements containing ${...}
expressions, both at root level and inside profiles.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cy versions
In CI-friendly projects using ${revision}, ${sha1}, or ${changelist}
as the parent version, child modules with versionless dependencies
may fail with 'dependencies.dependency.version is missing' because
Maven 4 validates dependency completeness before fully resolving the
parent's dependencyManagement chain.
Add a warning that detects this pattern: a parent with a CI-friendly
version expression and dependencies without explicit <version> elements.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Maven 4's prefix filtering defaults to permissive behavior when a repository has no prefixes.txt — artifacts are allowed through, not blocked. The warning was based on a misunderstanding of the default noInputOutcome=true setting. The actual issue is narrower: only auto-discovered prefix files with incomplete content can cause false negatives, and Maven already provides per-repository disable options and helpful hints in ArtifactFilteredOutException messages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rty guard - CLI option stripping: also remove trailing argument values for options like "-T 4" (two tokens) to prevent the value from being treated as a spurious goal. - BOM property decoupling: skip when the shared property is not declared in the current POM (inherited from parent), since we cannot resolve its actual value and introducing a new property could downgrade an already-sufficient inherited version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n version gap warning - Move gmavenplus-plugin from KNOWN_INCOMPATIBLE_PLUGINS (permanent warning) to PluginUpgrade with minimum version 4.2.0. The Maven 4 incompatibility was fixed in groovy/GMavenPlus#328 (released in 4.2.0), and the Maven-side fix landed in MNG-8662. - Soften the Quarkus version gap warning: remove the unsubstantiated claim about binary incompatibility at 3.32.0 and replace with general version alignment advice. - Fix wrong Quarkus upstream reference in PR description (#46889 → #37627). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When mvnup inserts "Override version inherited from parent" comments before plugin elements, the comment was placed on the same line as the preceding </plugin> closing tag, causing POM formatting violations in projects that enforce XML formatting (e.g. via spotless). The fix sets precedingWhitespace on the Comment node to match the plugin element's indentation, ensuring the comment appears on its own line. Fixes gnodet/maven4-testing#23134 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the manual precedingWhitespace() workaround with the new Editor.insertCommentBefore() method added in DomTrip 1.6.0 (maveniverse/domtrip#254), which handles whitespace automatically. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rojects Detect the Quarkus platform version before upgrading quarkus-maven-plugin. Projects using Quarkus 2.x (or 1.x) get their plugin upgrade skipped to prevent NoSuchMethodError and build failures from the 2.x→3.x jump. Detection checks dependencyManagement for quarkus-bom (resolving property references), then falls back to quarkus.platform.version / quarkus.version / quarkus-plugin.version properties. When the version cannot be determined, a warning is emitted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
changed the title
…nputException on binary files Maven 4 strictly decodes filtered resources as UTF-8, causing MalformedInputException when binary files (.xlsx, .docx, etc.) are present in resource directories with <filtering>true</filtering>. This strategy scans all <resource> and <testResource> blocks. When filtering is enabled and the maven-resources-plugin does not already declare <nonFilteredFileExtensions>, adds a comprehensive list of 23 binary file extensions (office docs, archives, fonts, executables, keystores) to pluginManagement. If a partial config exists, merges the missing extensions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Maven 4 rejects duplicate dependency/plugin declarations that Maven 3 silently accepted. Extract the deduplication logic into a standalone strategy for clarity and independent control. The new strategy scans <dependencies>, <dependencyManagement>, and <plugins>/<pluginManagement> sections (including profiles) and removes duplicates using last-wins semantics matching Maven 3's runtime behavior. The key is groupId:artifactId:type:classifier for dependencies and groupId:artifactId for plugins. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thorough review of this large PR (6900+ lines, 17 files) covering mvnup upgrade strategies and compatibility improvements.
Verified findings (4 confirmed, 2 false positives dropped):
-
DeduplicateDependenciesStrategy — "last-wins" semantics documentation (medium): The Javadoc on line 59 states "keeping the last declaration (matching Maven 3's last-wins behavior at runtime)", but Maven 3's dependency resolver actually uses first-wins for same-depth conflicts. For plugins, Maven 3 does merge later declarations over earlier ones (effectively last-wins for configuration), so last-wins is correct for plugins but not for dependencies. The old code in CompatibilityFixStrategy used first-wins for dependencies. Consider either updating the Javadoc to document this is an intentional behavior change, or switching to first-wins for dependencies.
-
CommonsCliUpgradeOptions parse() retry loop (low): When
CleanArgument.cleanArgs()strips surrounding quotes,UnrecognizedOptionException.getOption()returns the post-cleaned option name whilecurrentArgshas pre-cleaned strings. For a quoted arg like"-ntp",indexOf("-ntp")would fail to match. Theidx < 0guard correctly throws, so behavior degrades gracefully to an error rather than a silent bug. Very unlikely in practice. -
EnforcerVersionRangeStrategy regex scope (low): The regex
4(?:\.\d+)*matches any 4.x upper bound (4.1, 4.2, etc.), not just 4/4.0/4.0.0. The Javadoc examples only show the narrower case. Consider tightening to4(?:\.0+)*or updating the documentation. -
ResourceFilteringStrategy BINARY_EXTENSIONS (low): The list of 23 entries is labeled "comprehensive" but omits common binary types (png, jpg/jpeg, gif, mp3, mp4, sqlite, p12/pfx). Consider expanding the list or softening the "comprehensive" claim.
-
RepositoryHttpsUpgradeStrategy LinkedHashMap comment (low): Comment claims ordering ensures "longer prefixes are checked before shorter ones", but entries are not actually ordered by length. Ordering doesn't matter for correctness since no key is a prefix of another, but the comment is misleading.
False positives dropped by verifier:
- Multi-segment version ranges: the regex DOES match union ranges correctly (lazy quantifier handles them)
- Missing test edge cases: existing test coverage is adequate
Overall: Well-structured PR with clean separation of strategies. No blocking issues — findings are informational comments and minor documentation improvements.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
- DeduplicateDependenciesStrategy: clarify last-wins is intentional, not matching Maven 3's first-wins dependency resolution - EnforcerVersionRangeStrategy: document regex matches any 4.x upper bound, not just 4/4.0/4.0.0 - ResourceFilteringStrategy: add 10 missing binary extensions (png, jpg, jpeg, gif, bmp, tiff, mp3, mp4, p12, pfx) and soften "comprehensive" wording to "common" - RepositoryHttpsUpgradeStrategy: fix misleading LinkedHashMap comment about prefix length ordering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the review feedback in cbdee27:
|
PR apache#12454 received a new commit at 23:24Z after our review at 23:02Z on the same calendar day. The precondition's date-level granularity missed this because updatedAt was still within the same day as the review date. Backdated the review entry so it gets picked up. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-review after commit cbdee27
The new commit addresses four of the five findings from the prior review:
- ✅ Class-level Javadoc corrected to clarify last-wins is intentional (not matching Maven 3 first-wins)
- ✅ EnforcerVersionRangeStrategy docs updated to document regex matching
- ✅ BINARY_EXTENSIONS expanded from 23 to 33 entries with softened wording
- ✅ LinkedHashMap comment fixed in RepositoryHttpsUpgradeStrategy
Remaining issue
Low — Javadoc inconsistency in DeduplicateDependenciesStrategy
The class-level Javadoc was correctly updated to say:
"Maven 3's dependency resolver uses first-wins for same-depth conflicts; last-wins is chosen here intentionally"
But the method-level Javadoc for fixDuplicateDependenciesInSection (~line 183) still says:
"matching Maven 3's runtime behavior"
These contradict each other. Suggest changing the method Javadoc to just say "last-wins" without referencing Maven 3 behavior.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Reviewed 3 PRs: apache#12454 (re-review), apache#11502 (new), apache#11405 (new). Added 2 new dependabot PRs to skip list. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Consolidates #12430 and #12443 into a single PR, plus new features:
New strategies
RepositoryHttpsUpgradeStrategy— converts HTTP repository URLs to HTTPS (blocked since Maven 3.8.1+), with well-known canonical URL mappings for Maven Central and Apache repositoriesEnforcerVersionRangeStrategy— widensrequireMavenVersionranges that exclude Maven 4 (e.g.,[3.8.8,4)→[3.8.8,5))Quarkus plugin improvements
dependencyManagement(quarkus-bom) or well-known properties (quarkus.platform.version,quarkus.version) and skips thequarkus-maven-pluginupgrade when the project uses Quarkus 2.x, preventingNoSuchMethodErrorbuild failuresBug fixes
CompatibilityFixStrategydependency/plugin deduplication to keep the last occurrence (matching Maven 3 runtime behavior) instead of first-winsOther improvements (from #12430)
.mvn/maven.config${revision}missing dependency versionsgmavenplus-pluginto Maven 4-compatible versionSupersedes #12430 and #12443 (commits cherry-picked).
Test plan
RepositoryHttpsUpgradeStrategyEnforcerVersionRangeStrategy🤖 Generated with Claude Code