Fix mvnup tool issues #7934-#7938 #9311

gnodet · 2025-07-07T08:15:04Z

Summary

This PR addresses five critical issues with the Maven upgrade tool (mvnup) reported in issues #7934, #7935, #7936, #7937, and #7938.

Issues Fixed

🔧 Issue #7934: External parent preservation

Problem: Tool removed parent groupId and artifactId from external META-POMs, causing "parent.groupId is missing" errors
Solution: Enhanced InferenceStrategy to detect external parents vs reactor parents and preserve required elements for external parents
Impact: Projects using external META-POMs now work correctly after upgrade

🔧 Issue #7935: .mvn directory creation for all model versions

Problem: .mvn directory was only created for 4.0.0 upgrades, not 4.1.0, causing root directory warnings
Solution: Modified AbstractUpgradeGoal to create .mvn directory for both 4.0.0 and 4.1.0 upgrades
Impact: Eliminates root directory warnings for all upgrade scenarios

🔧 Issue #7936: Downgrade error handling

Problem: Tool showed warnings but continued execution when attempting invalid downgrades (4.1.0 → 4.0.0)
Solution: Changed ModelUpgradeStrategy to fail with errors instead of warnings for invalid version transitions
Impact: Tool now properly fails and exits with error code for invalid downgrades

🔧 Issue #7937: Unicode icon fallback

Problem: Unicode characters (✓, ✗, ⚠, •, →) didn't display properly on Windows CMD/PowerShell
Solution: Enhanced UpgradeContext to detect Unicode support using terminal.stdoutEncoding() and fall back to ASCII characters
Impact: Better compatibility across different terminal environments

🔧 Issue #7938: Child version removal in 4.1.0

Problem: Child project versions weren't being removed when they matched parent version in 4.1.0 upgrades
Solution: Enhanced inference logic to properly handle version removal in subprojects
Impact: Cleaner POMs with proper inference optimizations in multi-module projects

Technical Details

Key Changes

InferenceStrategy.java:
- Added isParentInReactor() method to distinguish external vs reactor parents
- Modified trimParentElementFull() to preserve external parent elements
- Improved child version removal logic
AbstractUpgradeGoal.java:
- Removed conditional .mvn directory creation based on model version
- Now creates .mvn directory for all upgrade scenarios
ModelUpgradeStrategy.java:
- Changed invalid upgrade handling from warnings to errors
- Improved error reporting and exit codes
UpgradeContext.java:
- Replaced complex Unicode detection heuristics with terminal.stdoutEncoding() check
- Added ASCII fallback icons: [OK], [ERROR], [WARNING], -, >

Testing

✅ Updated existing tests to reflect correct behavior
✅ Added new tests for downgrade error handling (ModelUpgradeStrategyTest)
✅ Added new tests for Unicode detection (UpgradeContextTest)
✅ All mvnup-related tests passing
✅ Verified fixes work with real-world scenarios

Backward Compatibility

✅ No breaking changes to existing functionality
✅ All existing tests updated and passing
✅ Maintains compatibility with existing workflows

Testing Performed

Manual Testing:
- Tested external parent preservation with META-POM scenario
- Verified .mvn directory creation for both 4.0.0 and 4.1.0
- Confirmed downgrade attempts now fail with proper error codes
- Validated Unicode fallback on different terminal types
- Tested multi-module project version inference
Automated Testing:
- All existing mvnup tests updated and passing
- New test coverage for previously untested scenarios
- Integration tests verify end-to-end functionality

Files Changed

impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/UpgradeContext.java
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/AbstractUpgradeGoal.java
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/InferenceStrategy.java
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/ModelUpgradeStrategy.java
Test files updated to reflect correct behavior
New test file: UpgradeContextTest.java

Checklist

All issues addressed with appropriate solutions
Code follows existing patterns and conventions
Comprehensive test coverage added/updated
All tests passing
No breaking changes introduced
Documentation updated where necessary
Manual testing performed

Closes #7934, #7935, #7936, #7937, #7938

Pull Request opened by Augment Code with guidance from the PR author

This commit addresses five critical issues with the Maven upgrade tool (mvnup): **Issue apache#7934: External parent preservation** - Fixed InferenceStrategy to detect external parents vs reactor parents - External parents now preserve required groupId, artifactId, and version elements - Prevents 'parent.groupId is missing' errors when using META-POMs **Issue apache#7935: .mvn directory creation for all model versions** - Modified AbstractUpgradeGoal to create .mvn directory for both 4.0.0 and 4.1.0 - Helps Maven find project root and avoids root directory warnings **Issue apache#7936: Downgrade error handling** - Changed ModelUpgradeStrategy to fail with errors instead of warnings for invalid downgrades - Tool now properly exits with error code when attempting 4.1.0 → 4.0.0 downgrade **Issue apache#7937: Unicode icon fallback** - Enhanced UpgradeContext to detect Unicode support using terminal.stdoutEncoding() - Falls back to ASCII characters ([OK], [ERROR], etc.) when Unicode not supported - Improves compatibility with Windows CMD/PowerShell and other terminals **Issue apache#7938: Child version removal in 4.1.0** - Enhanced inference logic to properly remove matching child versions in subprojects - Ensures child project versions are removed when they match parent version **Testing:** - Updated existing tests to reflect correct behavior - Added new tests for downgrade handling and Unicode detection - All mvnup tests passing with comprehensive coverage Fixes apache#7934, apache#7935, apache#7936, apache#7937, apache#7938

- Replace heuristic-based approach with direct GAV matching against pomMap - More accurate detection of external vs reactor parents - Added comprehensive test coverage for both scenarios - Suggested by code review feedback

- Replace manual GAV extraction with existing GAVUtils method - More robust handling of parent resolution and inheritance - Leverages existing tested utility for GAV extraction - Cleaner and more maintainable code

impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/UpgradeContext.java

@elharo

…ction This commit addresses all review feedback from @elharo and @Bukama: **Icon Management Improvements:** - Created ConsoleIcon enum with Unicode/ASCII fallback pairs - Moved all icon logic into the enum for consistency and reusability - Implemented ConsoleIcon.getIcon(Terminal) for clean integration **Unicode Detection Enhancements:** - Use Charset.newEncoder().canEncode(char) for accurate character testing - Use terminal.encoding() instead of deprecated stdoutEncoding() - Simplified fallback to Charset.defaultCharset() - Removed complex heuristics and exception handling **Code Quality Improvements:** - UpgradeContext methods are now clean one-liners - Better separation of concerns between icon rendering and context management - Comprehensive test coverage for all scenarios - Future-proof design for adding new icons **Review Comments Addressed:** - @elharo: 'maybe this should be a field that can be shared?' → Icons now shared via enum - @Bukama: 'maybe an enum with icon and fallback text?' → Implemented with Terminal integration - @elharo: 'There are others that also support Unicode, e.g. UCS-2., UCS-4' → Now tests actual encoding capability - @elharo: 'Please be specific about exceptions' → No longer needed with simplified approach - @elharo: 'toLowerCase(Locale.ENGLISH)' → No longer needed with canEncode() approach **Technical Benefits:** - More accurate Unicode detection per character - Works with any charset automatically - Cleaner and more maintainable code - Better test coverage and reliability

Bukama

Thanks for update :)

gnodet · 2025-07-15T22:13:07Z

💚 All backports created successfully

Status	Branch	Result
✅	maven-4.0.x

Questions ?

Please refer to the Backport tool documentation

@elharo

* Fix mvnup tool issues apache#7934-apache#7938 This commit addresses five critical issues with the Maven upgrade tool (mvnup): **Issue apache#7934: External parent preservation** - Fixed InferenceStrategy to detect external parents vs reactor parents - External parents now preserve required groupId, artifactId, and version elements - Prevents 'parent.groupId is missing' errors when using META-POMs **Issue apache#7935: .mvn directory creation for all model versions** - Modified AbstractUpgradeGoal to create .mvn directory for both 4.0.0 and 4.1.0 - Helps Maven find project root and avoids root directory warnings **Issue apache#7936: Downgrade error handling** - Changed ModelUpgradeStrategy to fail with errors instead of warnings for invalid downgrades - Tool now properly exits with error code when attempting 4.1.0 → 4.0.0 downgrade **Issue apache#7937: Unicode icon fallback** - Enhanced UpgradeContext to detect Unicode support using terminal.stdoutEncoding() - Falls back to ASCII characters ([OK], [ERROR], etc.) when Unicode not supported - Improves compatibility with Windows CMD/PowerShell and other terminals **Issue apache#7938: Child version removal in 4.1.0** - Enhanced inference logic to properly remove matching child versions in subprojects - Ensures child project versions are removed when they match parent version **Testing:** - Updated existing tests to reflect correct behavior - Added new tests for downgrade handling and Unicode detection - All mvnup tests passing with comprehensive coverage Fixes apache#7934, apache#7935, apache#7936, apache#7937, apache#7938 * Improve parent reactor detection logic - Replace heuristic-based approach with direct GAV matching against pomMap - More accurate detection of external vs reactor parents - Added comprehensive test coverage for both scenarios - Suggested by code review feedback * Refactor to use GAVUtils.extractGAVWithParentResolution - Replace manual GAV extraction with existing GAVUtils method - More robust handling of parent resolution and inheritance - Leverages existing tested utility for GAV extraction - Cleaner and more maintainable code * Address review feedback: Implement ConsoleIcon enum with charset detection This commit addresses all review feedback from @elharo and @Bukama: **Icon Management Improvements:** - Created ConsoleIcon enum with Unicode/ASCII fallback pairs - Moved all icon logic into the enum for consistency and reusability - Implemented ConsoleIcon.getIcon(Terminal) for clean integration **Unicode Detection Enhancements:** - Use Charset.newEncoder().canEncode(char) for accurate character testing - Use terminal.encoding() instead of deprecated stdoutEncoding() - Simplified fallback to Charset.defaultCharset() - Removed complex heuristics and exception handling **Code Quality Improvements:** - UpgradeContext methods are now clean one-liners - Better separation of concerns between icon rendering and context management - Comprehensive test coverage for all scenarios - Future-proof design for adding new icons **Review Comments Addressed:** - @elharo: 'maybe this should be a field that can be shared?' → Icons now shared via enum - @Bukama: 'maybe an enum with icon and fallback text?' → Implemented with Terminal integration - @elharo: 'There are others that also support Unicode, e.g. UCS-2., UCS-4' → Now tests actual encoding capability - @elharo: 'Please be specific about exceptions' → No longer needed with simplified approach - @elharo: 'toLowerCase(Locale.ENGLISH)' → No longer needed with canEncode() approach **Technical Benefits:** - More accurate Unicode detection per character - Works with any charset automatically - Cleaner and more maintainable code - Better test coverage and reliability (cherry picked from commit adf2c49)

@elharo

* Fix mvnup tool issues #7934-#7938 This commit addresses five critical issues with the Maven upgrade tool (mvnup): **Issue #7934: External parent preservation** - Fixed InferenceStrategy to detect external parents vs reactor parents - External parents now preserve required groupId, artifactId, and version elements - Prevents 'parent.groupId is missing' errors when using META-POMs **Issue #7935: .mvn directory creation for all model versions** - Modified AbstractUpgradeGoal to create .mvn directory for both 4.0.0 and 4.1.0 - Helps Maven find project root and avoids root directory warnings **Issue #7936: Downgrade error handling** - Changed ModelUpgradeStrategy to fail with errors instead of warnings for invalid downgrades - Tool now properly exits with error code when attempting 4.1.0 → 4.0.0 downgrade **Issue #7937: Unicode icon fallback** - Enhanced UpgradeContext to detect Unicode support using terminal.stdoutEncoding() - Falls back to ASCII characters ([OK], [ERROR], etc.) when Unicode not supported - Improves compatibility with Windows CMD/PowerShell and other terminals **Issue #7938: Child version removal in 4.1.0** - Enhanced inference logic to properly remove matching child versions in subprojects - Ensures child project versions are removed when they match parent version **Testing:** - Updated existing tests to reflect correct behavior - Added new tests for downgrade handling and Unicode detection - All mvnup tests passing with comprehensive coverage Fixes #7934, #7935, #7936, #7937, #7938 * Improve parent reactor detection logic - Replace heuristic-based approach with direct GAV matching against pomMap - More accurate detection of external vs reactor parents - Added comprehensive test coverage for both scenarios - Suggested by code review feedback * Refactor to use GAVUtils.extractGAVWithParentResolution - Replace manual GAV extraction with existing GAVUtils method - More robust handling of parent resolution and inheritance - Leverages existing tested utility for GAV extraction - Cleaner and more maintainable code * Address review feedback: Implement ConsoleIcon enum with charset detection This commit addresses all review feedback from @elharo and @Bukama: **Icon Management Improvements:** - Created ConsoleIcon enum with Unicode/ASCII fallback pairs - Moved all icon logic into the enum for consistency and reusability - Implemented ConsoleIcon.getIcon(Terminal) for clean integration **Unicode Detection Enhancements:** - Use Charset.newEncoder().canEncode(char) for accurate character testing - Use terminal.encoding() instead of deprecated stdoutEncoding() - Simplified fallback to Charset.defaultCharset() - Removed complex heuristics and exception handling **Code Quality Improvements:** - UpgradeContext methods are now clean one-liners - Better separation of concerns between icon rendering and context management - Comprehensive test coverage for all scenarios - Future-proof design for adding new icons **Review Comments Addressed:** - @elharo: 'maybe this should be a field that can be shared?' → Icons now shared via enum - @Bukama: 'maybe an enum with icon and fallback text?' → Implemented with Terminal integration - @elharo: 'There are others that also support Unicode, e.g. UCS-2., UCS-4' → Now tests actual encoding capability - @elharo: 'Please be specific about exceptions' → No longer needed with simplified approach - @elharo: 'toLowerCase(Locale.ENGLISH)' → No longer needed with canEncode() approach **Technical Benefits:** - More accurate Unicode detection per character - Works with any charset automatically - Cleaner and more maintainable code - Better test coverage and reliability (cherry picked from commit adf2c49)

gnodet added bug Something isn't working backport-to-4.0.x labels Jul 7, 2025

gnodet added 2 commits July 7, 2025 09:02

Improve parent reactor detection logic

bdaa68a

- Replace heuristic-based approach with direct GAV matching against pomMap - More accurate detection of external vs reactor parents - Added comprehensive test coverage for both scenarios - Suggested by code review feedback

Refactor to use GAVUtils.extractGAVWithParentResolution

06ebcb5

- Replace manual GAV extraction with existing GAVUtils method - More robust handling of parent resolution and inheritance - Leverages existing tested utility for GAV extraction - Cleaner and more maintainable code

gnodet marked this pull request as ready for review July 8, 2025 05:40

elharo reviewed Jul 9, 2025

View reviewed changes

Bukama requested changes Jul 10, 2025

View reviewed changes

impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/UpgradeContext.java Outdated Show resolved Hide resolved

Bukama approved these changes Jul 10, 2025

View reviewed changes

rmannibucau approved these changes Jul 15, 2025

View reviewed changes

gnodet merged commit adf2c49 into apache:master Jul 15, 2025
19 checks passed

github-actions bot added this to the 4.1.0 milestone Jul 15, 2025

github-actions bot assigned gnodet Jul 15, 2025

gnodet mentioned this pull request Jul 15, 2025

[maven-4.0.x] Fix mvnup tool issues #7934-#7938 (#9311) #10915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mvnup tool issues #7934-#7938 #9311

Fix mvnup tool issues #7934-#7938 #9311

Uh oh!

gnodet commented Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bukama left a comment

Uh oh!

Uh oh!

gnodet commented Jul 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix mvnup tool issues #7934-#7938 #9311

Fix mvnup tool issues #7934-#7938 #9311

Uh oh!

Conversation

gnodet commented Jul 7, 2025

Summary

Issues Fixed

🔧 Issue #7934: External parent preservation

🔧 Issue #7935: .mvn directory creation for all model versions

🔧 Issue #7936: Downgrade error handling

🔧 Issue #7937: Unicode icon fallback

🔧 Issue #7938: Child version removal in 4.1.0

Technical Details

Key Changes

Testing

Backward Compatibility

Testing Performed

Files Changed

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bukama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gnodet commented Jul 15, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants