[#10939] Enable addLocationInformation in DefaultModelXmlFactory base on InputLocationFormatter presence by Giovds · Pull Request #10940 · apache/maven

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

        }
        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

        }
        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

-    }
+    public MavenXpp3Writer() {}

-    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


    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

    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


    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


    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

gnodet mentioned this pull request Sep 4, 2025

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

Merged

2 tasks

gnodet closed this Sep 4, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Giovds commented Jul 18, 2025 •

edited

Loading