Preserve POM format when extensions are added/removed

[gastaldi]

It also bumps maven-model-helper.

• Fixes Solve POM formatting issues when creating project/adding extension/removing extension #39088

[gastaldi]

@gsmet please give it a try. I tested it locally and it worked as advertised, but I appreciate your feedback ;)

[gsmet]

Awesome! I will have a look and make sure it ends up in 3.9 final.

[gsmet]

@gastaldi I tested it by trying to add an extension to https://github.com/quarkiverse/quarkus-github-app/blob/main/runtime/pom.xml

qss extension add resteasy-reactive-jackson

• Comments are kept, that's good
• Indentation is kept as is, that's good
• New lines are lost (they should be kept)
• a </deployment> node was moved. While it's arguably better in this very case, I'm not sure if it's a good idea to move things around

What decentxml is doing is that it actually record all the blank stuff as nodes to make sure it's not lost.

I'm not sure it can be fixed with your approach but I haven't looked at the code.

[gsmet]

Also, it doesn't fix the POM creation that is also part of the original issue. Either you fix it there or we avoid closing the issue.

In SmartPomMergeCodestartFileStrategyHandler, we should be able to force the indentation to 4 spaces.
And ideally having a method to get a string output would be awesome. I could use a StringWriter but having a shortcut would be nice.

Also we probably need to check what CreateProjectMojo is doing as I see some code there that could update a parent POM (and we would need to be as careful as for adding an extension).

[gsmet]

Tested on another project and I also have this in the diff:

@@ -1,8 +1,5 @@
-<?xml version="1.0"?>
-<project
-    xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd"
-    xmlns="http://maven.apache.org/POM/4.0.0"
-    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+<?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">

While it's not a biggie, it's still unnecessary changes (and could go against your conventions if you are enforcing a line length limit).

[gastaldi]

Tested on another project and I also have this in the diff:

@@ -1,8 +1,5 @@
-<?xml version="1.0"?>
-<project
-    xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd"
-    xmlns="http://maven.apache.org/POM/4.0.0"
-    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+<?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">

While it's not a biggie, it's still unnecessary changes (and could go against your conventions if you are enforcing a line length limit).

Yeah, that's a known issue, JDOM does not keep track of line breaks inside attributes. I'm working on the other issues you reported now

[gastaldi]

• a </deployment> node was moved. While it's arguably better in this very case, I'm not sure if it's a good idea to move things around

That's caused by MavenXpp3Reader, which trims the content when transforming to a Model object. I'll investigate if I can bypass that somehow

[gastaldi]

• New lines are lost (they should be kept)

I've managed to fix that in the codebase but I broke some tests, working on them now

[yrodiere]

@yrodiere WDYT?

I can confirm this solves the problem reported in #39048 : XML comments are preserved. With the reproducer from that issue, I get:

diff --git a/pom.xml b/pom.xml
index 2e7443b..1cc4c8c 100644
--- a/pom.xml
+++ b/pom.xml
@@ -1,6 +1,5 @@
-<?xml version="1.0"?>
-<project xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd" xmlns="http://maven.apache.org/POM/4.0.0"
-    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+<?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>
   <groupId>io.quarkus.search</groupId>
   <artifactId>search-quarkus-io</artifactId>
@@ -168,6 +167,10 @@
       <artifactId>quarkus-helm</artifactId>
       <version>1.2.3</version>
     </dependency>
+    <dependency>
+      <groupId>io.quarkus</groupId>
+      <artifactId>quarkus-hibernate-search-orm-outbox-polling</artifactId>
+    </dependency>
     <dependency>
       <groupId>io.quarkus</groupId>
       <artifactId>quarkus-junit5</artifactId>

Another improvement is the handling of spaces vs. tabs: where the previous behavior would absolutely wreck formatting in an XML file containing tabs, with this PR it seems to handle it just fine.

I think even if formatting within XML tags isn't preserved (anymore?), this is still a huge improvements over the current behavior, so +1 to merge from me.

Once CI agrees, of course. Currently it's quite angry 😁

Thanks @gastaldi!

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 0a3b45c.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 core/deployment

✖ io.quarkus.deployment.dev.FileSystemWatcherTestCase.testFileSystemWatcher - History

• expected: not <null> - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: expected: not <null>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertNotNull.failNull(AssertNotNull.java:49)
	at org.junit.jupiter.api.AssertNotNull.assertNotNull(AssertNotNull.java:35)
	at org.junit.jupiter.api.AssertNotNull.assertNotNull(AssertNotNull.java:30)
	at org.junit.jupiter.api.Assertions.assertNotNull(Assertions.java:304)
	at io.quarkus.deployment.dev.FileSystemWatcherTestCase.consumeEvents(FileSystemWatcherTestCase.java:172)

---

⚙️ JVM Tests - JDK 17 Windows

📦 extensions/websockets-next/server/deployment

✖ io.quarkus.websockets.next.test.broadcast.BroadcastOnOpenTest.testLoMultiBidi - History

• Messages: [c2] ==> expected: <true> but was: <false> - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: Messages: [c2] ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
	at io.quarkus.websockets.next.test.broadcast.BroadcastOnOpenTest.assertBroadcast(BroadcastOnOpenTest.java:98)
	at io.quarkus.websockets.next.test.broadcast.BroadcastOnOpenTest.testLoMultiBidi(BroadcastOnOpenTest.java:50)

---

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

• Assertion condition Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)