[#10939] Enable addLocationInformation in DefaultModelXmlFactory base on InputLocationFormatter presence #10940

Giovds · 2025-07-18T11:48:05Z

Show example for [#10939]

Set addLocationInformation to false by default in DefaultModelXmlFactory.
If the XmlWriterRequest specifies an InputLocationFormatter, set addLocationInformation to true and apply the formatter.

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

Your pull request should address just one issue, without pulling in other changes.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
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.
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.
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.

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.

…y based on InputLocationFormatter presence

Giovds · 2025-07-18T11:52:37Z

Current behaviour without a formatter will result in:

<?xml version="1.0" encoding="UTF-8"?>
  <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion><!-- 2 -->
    <groupId>org.example</groupId><!-- 3 -->
    <artifactId>example-artifact</artifactId><!-- 4 -->
    <version>1.0.0</version><!-- 5 -->
    <name>Example Project</name><!-- 6 -->
    <properties>
      <example.property>value</example.property><!-- 8 -->
    </properties>
  </project>

elharo · 2025-07-18T12:01:45Z

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

@@ -127,7 +127,9 @@ public void write(XmlWriterRequest<Model> request) throws XmlWriterException {
        }
        try {
            MavenStaxWriter w = new MavenStaxWriter();
+            w.setAddLocationInformation(false);


maybe use an else block for this; what's the default?

The MavenStaxWriter is initialized with true therefore I explicitly set it to false. I personally prefer to keep nesting brackets to a minimum, as this makes it less verbose and easier to understand what's happening. The result would be the same?

In that case, no need to set it to true below. Use an else block to set it to false

That would make it depend on the implementation of the MavenStaxWriter. If it changes the default this will affect our class.

I was thinking the same as @gnodet:

I wonder if this should be moved further down in the MavenStaxWriter and remove the addLocationInformation completely in that case.

If we use the proposed solution of moving this behaviour to the MavenStaxWriter this might solve both the need to set the value, and other XML factory implementations.

This is a generic generated class (throughwriter-stax.vm), so all ZZZStaxWriter classes (which are about 8 classes) will use this behaviour from then on. I'm not entirely sure about that change, but probably will be the better option as it will solve the issues for other DefaultZZZXmlFactory classes as well that use one of the ZZZStaxWriter's.

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

gnodet · 2025-07-18T15:24:44Z

I wonder if this should be moved further down in the MavenStaxWriter and remove the addLocationInformation completely in that case.

elharo · 2025-07-18T22:17:35Z

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

@@ -127,7 +127,9 @@ public void write(XmlWriterRequest<Model> request) throws XmlWriterException {
        }
        try {
            MavenStaxWriter w = new MavenStaxWriter();
+            w.setAddLocationInformation(false);


In that case, no need to set it to true below. Use an else block to set it to false

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

…lFactoryTest

…rmatter presence

…viour

gnodet

We cannot break compatibility with Maven 3 API here.
Undo the changes in compat/* modules.

gnodet · 2025-07-24T09:56:38Z

compat/maven-model/src/main/java/org/apache/maven/model/io/xpp3/MavenXpp3Writer.java


-    protected MavenXpp3Writer(boolean addLocationInformation) {


No API breakage in Maven 3 compatibility layer.

Check. Won’t be able to work on this due to holiday for the next couple weeks. Feel free to adjust it, otherwise I’ll look into this when I’m back.

gnodet · 2025-07-24T09:57:10Z

compat/maven-model/src/main/java/org/apache/maven/model/io/xpp3/MavenXpp3WriterEx.java

@@ -36,7 +37,7 @@ public class MavenXpp3WriterEx extends MavenXpp3Writer {
    // -----------/

    public MavenXpp3WriterEx() {
-        super(true);
+        super((inputLocation) -> inputLocation.toString());


gnodet · 2025-07-24T09:57:18Z

compat/maven-model/src/test/java/org/apache/maven/model/v4/ModelXmlTest.java

@@ -89,7 +89,6 @@ void testNamespaceInXmlNode() throws XMLStreamException {
    String toXml(Model model) throws IOException, XMLStreamException {
        StringWriter sw = new StringWriter();
        MavenStaxWriter writer = new MavenStaxWriter();
-        writer.setAddLocationInformation(false);


gnodet · 2025-07-24T09:57:33Z

compat/maven-settings/src/main/java/org/apache/maven/settings/io/xpp3/SettingsXpp3Writer.java

@@ -38,7 +38,6 @@ public class SettingsXpp3Writer {

    public SettingsXpp3Writer() {
        delegate = new SettingsStaxWriter();
-        delegate.setAddLocationInformation(false);


gnodet · 2025-07-24T09:57:39Z

...-model/src/main/java/org/apache/maven/toolchain/model/io/xpp3/MavenToolchainsXpp3Writer.java

@@ -41,7 +41,6 @@ public class MavenToolchainsXpp3Writer {

    public MavenToolchainsXpp3Writer() {
        delegate = new MavenToolchainsStaxWriter();
-        delegate.setAddLocationInformation(false);


[apache#10939] Enable addLocationInformation in DefaultModelXmlFactor…

acbc713

…y based on InputLocationFormatter presence

elharo requested changes Jul 18, 2025

View reviewed changes

Giovds added 4 commits July 20, 2025 21:09

[apache#10939] Replace assertThat with assertEquals in DefaultModelXm…

4773734

…lFactoryTest

[apache#10939] Refactor location tracking logic to depend on stringFo…

d9a3399

…rmatter presence

[apache#10939] Add non-formatter constructor to reflect original beha…

d521474

…viour

[apache#10939] Apply Spotless

db06137

gnodet requested changes Jul 24, 2025

View reviewed changes

[#10939] Enable addLocationInformation in DefaultModelXmlFactory base on InputLocationFormatter presence #10940

Are you sure you want to change the base?

[#10939] Enable addLocationInformation in DefaultModelXmlFactory base on InputLocationFormatter presence #10940

Uh oh!

Conversation

Giovds commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Giovds commented Jul 18, 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!

Uh oh!

Uh oh!

Uh oh!

gnodet commented Jul 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gnodet 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Giovds commented Jul 18, 2025 •

edited

Loading