Wire ModelBuilderRequest.isLocationTracking() to XML parser - #12655
Wire ModelBuilderRequest.isLocationTracking() to XML parser#12655gnodet wants to merge 3 commits into
Conversation
Remove addLocationInformation from XmlReaderRequest and DefaultModelXmlFactory — these changes belong in the wire-location-tracking-to-parser branch (PR #12655), not in the model-building-pipeline optimization PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove addLocationInformation from XmlReaderRequest and DefaultModelXmlFactory — these changes belong in the wire-location-tracking-to-parser branch (PR #12655), not in the model-building-pipeline optimization PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add isAddLocationInformation() to XmlReaderRequest and wire it to MavenStaxReader so callers can skip location tracking when not needed. DefaultModelBuilder propagates the flag from ModelBuilderRequest to the XML parser, and forwards the parent's setting to BOM import sub-requests instead of hardcoding it off. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2190df7 to
43f6a73
Compare
Do not pass ModelBuilderRequest.isLocationTracking() as the
addLocationInformation flag for XmlReaderRequest. These are
different concerns:
- isLocationTracking() controls whether the model building pipeline
retains location data (used by DefaultDependencyManagementImporter
and DefaultProjectBuilder)
- isAddLocationInformation() controls whether the XML parser tracks
line/column positions during parsing
Wiring one to the other broke hasSubprojectsDefined(), which relies on
model.getLocation("modules") to detect <modules> presence. When
DefaultConsumerPomBuilder sets locationTracking(false) for consumer POM
generation, the XML parser skipped location tracking, causing spurious
auto-discovery of subprojects alongside explicit <modules> entries.
The XmlReaderRequest API + DefaultModelXmlFactory wiring remain as
correct infrastructure for future selective optimization by callers
who know they don't need XML-level locations.
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.
The new XmlReaderRequest.isAddLocationInformation() API infrastructure is correctly wired through DefaultModelXmlFactory to MavenStaxReader and will work for any future caller passing addLocationInformation(false).
Two observations:
-
locationTrackingdefault change: The default changed fromfalsetotruein theModelBuilderRequestbuilder. This affects callers not explicitly setting the flag — notablyDefaultArtifactDescriptorReaderandDefaultModelBuilder.doLoadDependencyManagement()— which will now runupdateWithImportedFrom()for every BOM dependency. The practical impact is small (proportional to unique imported dependencies), but this behavioral change merits explicit mention in the PR description. -
@sincetag: The@since 4.0.0onisAddLocationInformation()should be@since 4.1.0since the method is new and the project is at4.1.0-SNAPSHOT.
Note: Cannot submit as APPROVE because the PR author matches the review account.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
…sed opt-out
Replace the fragile location-tracking-based detection of empty <modules/>
elements with a simple non-empty list check, and add a new user property
(maven.project.discoverSubprojects) as the explicit opt-out mechanism for
subproject auto-discovery.
The previous approach relied on model.getLocation("modules") != null to
distinguish "no <modules> element" from "empty <modules/> element", which
broke whenever location tracking was disabled (e.g. by
DefaultConsumerPomBuilder). The new approach:
- hasSubprojectsDefined() now checks !getSubprojects().isEmpty() ||
!getModules().isEmpty()
- Users who want to suppress auto-discovery without listing subprojects
can set -Dmaven.project.discoverSubprojects=false
- ModelBuilderRequest.locationTracking default reverted to false since
subproject detection no longer depends on it
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.
LGTM ✅ — Re-reviewed after new commits. Both previous findings are resolved.
Previous findings status:
- ✅ locationTracking default change — ADDRESSED. The third commit reverts the default to
false(matching master) because subproject detection no longer depends on location tracking. - ✅ @SInCE tag — Acceptable.
@since 4.0.0is consistent with the convention for default methods on existing@since 4.0.0interfaces in this codebase.
New changes look good:
- The third commit replaces the fragile location-tracking-based detection of empty
<modules/>/<subprojects/>elements with a property-based opt-out (maven.project.discoverSubprojects). The rationale is sound — the location-based detection broke whenever location tracking was disabled (e.g., byDefaultConsumerPomBuilder). - The new property-based opt-out is more explicit and robust.
- Tests are updated to reflect the new behavior and a new test covers the property-based opt-out.
- The
isAddLocationInformation()API onXmlReaderRequestis correctly wired but intentionally kept separate fromModelBuilderRequest.isLocationTracking()— these are separate concerns.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
1 participant
Summary
addLocationInformationfield toXmlReaderRequestinterface and builder, defaulting totruefor backwards compatibilityDefaultModelXmlFactory.doRead()toMavenStaxReader.setAddLocationInformation()request.isLocationTracking()inDefaultModelBuilder.doReadFileModel()so the generatedMavenStaxReaderskips all 97InputLocation.of()allocations per POM when location tracking is disabledlocationTrackingfrom the parent request to BOM import requests indoLoadDependencyManagement()so that project builds (which need location tracking) still get full location info from their BOMsPreviously,
ModelBuilderRequest.isLocationTracking()was only checked in one place (DefaultDependencyManagementImporter), while the XML parser always createdInputLocationobjects regardless of the flag. This connects the existing flag to the parser, making it actually effective.Test plan
mvn verify -pl impl/maven-impl— all tests passmvn verify -pl impl/maven-core— all tests pass, includingtestLocationTrackingResolutionwhich verifies BOM dependency location tracking🤖 Generated with Claude Code