Fix #10939: DefaultModelXmlFactory: make location tracking opt-in—disabled by def… #11092

arturobernalg · 2025-09-03T11:35:01Z

…ault, enabled when an InputLocationFormatter is provided.

Adapt formatter to Function<InputLocation,String> and write to the actually opened stream for path output. Fixes #10939.

Following this checklist to help us incorporate your
contribution quickly and easily:

[ X] Your pull request should address just one issue, without pulling in other changes.
[ X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
[ X] Each commit in the pull request should have a meaningful subject line and body.
Note that commits might be squashed by a maintainer on merge.
[ X] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
This may not always be possible but is a best-practice.
[ X] Run mvn verify to make sure basic checks pass.
A more thorough check will be performed on your pull request automatically.
You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

[ X] I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

====

Supersedes #10940

elharo

There seem to be a lot of changes here that have nothing to do with teh purpose of thiks PR. Try to keep the changes more focused

elharo · 2025-09-03T11:37:03Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

-            if (inputLocationFormatter != null) {
-                w.setStringFormatter((Function) inputLocationFormatter);
+            // OFF by default
+            final MavenStaxWriter xml = new MavenStaxWriter();


xml --> xmlWriter

elharo · 2025-09-03T11:37:46Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

            }
        } catch (Exception e) {
-            throw new XmlWriterException("Unable to write model: " + getMessage(e), getLocation(e), e);
+            throw new XmlWriterException("Unable to write model " + getMessage(e), getLocation(e), e);


This line doesn't need to be changed in this PR

elharo · 2025-09-03T11:39:01Z

impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultModelXmlFactoryTest.java

+
+        String result = out.toString();
+        // Heuristic: when tracking is off there should be no location output (no comments)
+        assertTrue(!result.contains("<!--"), "Expected no XML comments when location tracking is disabled");


assretFalse

elharo · 2025-09-03T11:39:31Z

impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultModelXmlFactoryTest.java

+        // Heuristic: when tracking is off there should be no location output (no comments)
+        assertTrue(!result.contains("<!--"), "Expected no XML comments when location tracking is disabled");
+        // And certainly not our marker
+        assertTrue(!result.contains("LOC_MARK"), "Unexpected marker found in output");


This isn't needed; or maybe keep this and get rid of the previous assert; if so, assertFalse

arturobernalg · 2025-09-03T12:01:46Z

please do another pass

elharo · 2025-09-03T12:16:20Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

-        Function<Object, String> inputLocationFormatter = request.getInputLocationFormatter();
+    public void write(final XmlWriterRequest<Model> request) throws XmlWriterException {
+        Objects.requireNonNull(request, "request can not be null");
+        final Model content = Objects.requireNonNull(request.getContent(), "content can not be null");


Is there a reason to make all these variables final in this PR?

elharo · 2025-09-03T12:16:40Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

-            MavenStaxWriter w = new MavenStaxWriter();
-            if (inputLocationFormatter != null) {
-                w.setStringFormatter((Function) inputLocationFormatter);
+            // OFF by default


what is off by default?

elharo · 2025-09-03T12:17:24Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

+            final MavenStaxWriter xmlWriter = new MavenStaxWriter();
+            xmlWriter.setAddLocationInformation(false);
+
+            // Opt-in when a formatter is provided; adapt to the required type


"Optt-in" feels like the wrong language here. No human is making a choice

elharo · 2025-09-03T12:17:38Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

+            xmlWriter.setAddLocationInformation(false);
+
+            // Opt-in when a formatter is provided; adapt to the required type
+            final Function<Object, String> fmt = request.getInputLocationFormatter();


fmt --> formatter

elharo · 2025-09-03T12:18:37Z

impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultModelXmlFactoryTest.java

+                .build());
+
+        String result = out.toString();
+        // And certainly not our marker


rewrite comment

elharo · 2025-09-03T15:08:27Z

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

+            final MavenStaxWriter xmlWriter = new MavenStaxWriter();
+            xmlWriter.setAddLocationInformation(false);
+
+            final Function<Object, String> fmtformatter = request.getInputLocationFormatter();


what is an fmtformatter? just formatter should be OK, or maybe inputLocationFormatter

what is an fmtformatter? just formatter should be OK, or maybe inputLocationFormatter

Sorry. type. Fixed

…ault, enabled when an InputLocationFormatter is provided. Adapt formatter to Function<InputLocation,String> and write to the actually opened stream for path output. Fixes apache#10939.

gnodet

No need for final variables here.

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultModelXmlFactory.java

Remove final keywords

…in—disabled by def… (apache#11092) DefaultModelXmlFactory: make location tracking opt-in—disabled by default, enabled when an InputLocationFormatter is provided. Adapt formatter to Function<InputLocation,String> and write to the actually opened stream for path output. Fixes apache#10939. (cherry picked from commit 3338476)

gnodet · 2025-10-03T07:57:50Z

💚 All backports created successfully

Status	Branch	Result
✅	master

Questions ?

Please refer to the Backport tool documentation

…abled by def… (#11092) (#11198) DefaultModelXmlFactory: make location tracking opt-in—disabled by default, enabled when an InputLocationFormatter is provided. Adapt formatter to Function<InputLocation,String> and write to the actually opened stream for path output. Fixes #10939. (cherry picked from commit 3338476) Co-authored-by: Arturo Bernal <[email protected]>

elharo requested changes Sep 3, 2025

View reviewed changes

arturobernalg force-pushed the 10939 branch from e642cab to 4c32684 Compare September 3, 2025 12:01

arturobernalg requested a review from elharo September 3, 2025 12:01

elharo requested changes Sep 3, 2025

View reviewed changes

arturobernalg force-pushed the 10939 branch 2 times, most recently from 87ca49b to 28e2f84 Compare September 3, 2025 12:30

arturobernalg requested a review from elharo September 3, 2025 12:36

elharo reviewed Sep 3, 2025

View reviewed changes

DefaultModelXmlFactory: make location tracking opt-in—disabled by def…

e94323f

…ault, enabled when an InputLocationFormatter is provided. Adapt formatter to Function<InputLocation,String> and write to the actually opened stream for path output. Fixes apache#10939.

arturobernalg force-pushed the 10939 branch from 6952f3c to e94323f Compare September 3, 2025 16:01

arturobernalg requested a review from elharo September 3, 2025 16:01

elharo approved these changes Sep 3, 2025

View reviewed changes

gnodet approved these changes Sep 4, 2025

View reviewed changes

gnodet requested changes Sep 4, 2025

View reviewed changes

Apply suggestions from code review

9274199

Remove final keywords

gnodet approved these changes Sep 4, 2025

View reviewed changes

gnodet added the bug Something isn't working label Sep 4, 2025

gnodet added the backport-to-4.0.x label Oct 3, 2025

gnodet merged commit 3338476 into apache:maven-4.0.x Oct 3, 2025
37 of 38 checks passed

github-actions bot assigned gnodet Oct 3, 2025

github-actions bot added this to the 4.0.0 milestone Oct 3, 2025

gnodet mentioned this pull request Oct 3, 2025

[master] Fix #10939: DefaultModelXmlFactory: make location tracking opt-in—disabled by def… (#11092) #11198

Merged

gnodet removed the backport-to-4.0.x label Oct 3, 2025

Fix #10939: DefaultModelXmlFactory: make location tracking opt-in—disabled by def… #11092

Fix #10939: DefaultModelXmlFactory: make location tracking opt-in—disabled by def… #11092

Uh oh!

Conversation

arturobernalg commented Sep 3, 2025 • edited by gnodet Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturobernalg commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gnodet commented Oct 3, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arturobernalg commented Sep 3, 2025 •

edited by gnodet

Loading