Wire ModelBuilderRequest.isLocationTracking() to XML parser - #12655
Wire ModelBuilderRequest.isLocationTracking() to XML parser#12655gnodet wants to merge 2 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
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