Fix : XNode#toString() may not output all child nodes

- Keeping indentation, but it won't be perfect in multi-line text nodes.
- Add a new line after each open/close tag even for a short line like `<id>3</id>`. There aren't many short lines like that in MyBatis mappers.
- Use 2 spaces for indentation instead of 4 to save earth.

Should fix #2080
Should close #2084 #2552 #2998

Author: harawata

src/main/java/org/apache/ibatis/parsing/XNode.java

@@ -280,49 +280,47 @@ public Properties getChildrenAsProperties() {
 
   @Override
   public String toString() {
-    StringBuilder builder = new StringBuilder();
-    toString(builder, 0);
-    return builder.toString();
+    return buildToString(new StringBuilder(), 0).toString();
   }
 
-  private void toString(StringBuilder builder, int level) {
-    builder.append("<");
-    builder.append(name);
+  private StringBuilder buildToString(StringBuilder builder, int indent) {
+    indent(builder, indent).append("<").append(name);
     for (Map.Entry<Object, Object> entry : attributes.entrySet()) {
       builder.append(" ");
       builder.append(entry.getKey());
       builder.append("=\"");
       builder.append(entry.getValue());
       builder.append("\"");
     }
-    List<XNode> children = getChildren();
-    if (!children.isEmpty()) {
+
+    NodeList nodeList = node.getChildNodes();
+    if (nodeList == null || nodeList.getLength() == 0) {
+      builder.append(" />\n");
+    } else {
       builder.append(">\n");
-      for (XNode child : children) {
-        indent(builder, level + 1);
-        child.toString(builder, level + 1);
+      for (int i = 0, n = nodeList.getLength(); i < n; i++) {
+        Node node = nodeList.item(i);
+        short nodeType = node.getNodeType();
+        if (nodeType == Node.ELEMENT_NODE) {
+          new XNode(xpathParser, node, variables).buildToString(builder, indent + 1);
+        } else {
+          String text = getBodyData(node).trim();
+          if (text.length() > 0) {
+            indent(builder, indent + 1).append(text).append("\n");
+          }
+        }
       }
-      indent(builder, level);
-      builder.append("</");
-      builder.append(name);
-      builder.append(">");
-    } else if (body != null) {
-      builder.append(">");
-      builder.append(body);
-      builder.append("</");
-      builder.append(name);
-      builder.append(">");
-    } else {
-      builder.append("/>");
-      indent(builder, level);
+      indent(builder, indent).append("</").append(name).append(">\n");
     }
-    builder.append("\n");
+
+    return builder;
   }
 
-  private void indent(StringBuilder builder, int level) {
+  private StringBuilder indent(StringBuilder builder, int level) {
     for (int i = 0; i < level; i++) {
-      builder.append("    ");
+      builder.append("  ");
     }
+    return builder;
   }
 
   private Properties parseAttributes(Node n) {

src/test/java/org/apache/ibatis/parsing/XNodeTest.java

@@ -0,0 +1,95 @@
+/*
+ *    Copyright 2009-2023 the original author or authors.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *       https://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+package org.apache.ibatis.parsing;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.Properties;
+
+import org.junit.jupiter.api.Test;
+
+class XNodeTest {
+
+  @Test
+  void xNodeToString() {
+    // @formatter:off
+    String xml = "<mapper>\n" +
+        "  <select id='select' resultType='map'>\n" +
+        "    select\n" +
+        "    <var set='foo' value='bar' />\n" +
+        "      ID,\n" +
+        "      NAME\n" +
+        "    from STUDENT\n" +
+        "    <where>\n" +
+        "      <if test=\"name != null\">\n" +
+        "        NAME = #{name}\n" +
+        "      </if>\n" +
+        "      and DISABLED = false\n" +
+        "    </where>\n" +
+        "    order by ID\n" +
+        "    <choose>\n" +
+        "      <when test='limit10'>\n" +
+        "        limit 10\n" +
+        "      </when>\n" +
+        "      <otherwise>limit 20</otherwise>\n" +
+        "    </choose>\n" +
+        "  </select>\n" +
+        "</mapper>";
+
+    String expected = "<select id=\"select\" resultType=\"map\">\n" +
+        "  select\n" +
+        "  <var set=\"foo\" value=\"bar\" />\n" +
+        "  ID,\n" +
+        // a little bit ugly here, but not a blocker
+        "      NAME\n" +
+        "    from STUDENT\n" +
+        "  <where>\n" +
+        "    <if test=\"name != null\">\n" +
+        "      NAME = #{name}\n" +
+        "    </if>\n" +
+        "    and DISABLED = false\n" +
+        "  </where>\n" +
+        "  order by ID\n" +
+        "  <choose>\n" +
+        "    <when test=\"limit10\">\n" +
+        "      limit 10\n" +
+        "    </when>\n" +
+        "    <otherwise>\n" +
+        "      limit 20\n" +
+        "    </otherwise>\n" +
+        "  </choose>\n" +
+        "</select>\n";
+    // @formatter:on
+
+    XPathParser parser = new XPathParser(xml);
+    XNode selectNode = parser.evalNode("/mapper/select");
+    assertEquals(expected, selectNode.toString());
+  }
+
+  @Test
+  void testXnodeToStringVariables() throws Exception {
+    String src = "<root attr='${x}'>y = ${y}<sub attr='${y}'>x = ${x}</sub></root>";
+    String expected = "<root attr=\"foo\">\n  y = bar\n  <sub attr=\"bar\">\n    x = foo\n  </sub>\n</root>\n";
+    Properties vars = new Properties();
+    vars.put("x", "foo");
+    vars.put("y", "bar");
+    XPathParser parser = new XPathParser(src, false, vars);
+    XNode selectNode = parser.evalNode("/root");
+    assertEquals(expected, selectNode.toString());
+  }
+
+}

src/test/java/org/apache/ibatis/parsing/XPathParserTest.java

@@ -204,7 +204,7 @@ private void testEvalMethod(XPathParser parser) {
     assertEquals((Float) 3.2f, parser.evalNode("/employee/active").getFloatAttribute("score"));
     assertEquals((Double) 3.2d, parser.evalNode("/employee/active").getDoubleAttribute("score"));
 
-    assertEquals("<id>${id_var}</id>", parser.evalNode("/employee/@id").toString().trim());
+    assertEquals("<id>\n  ${id_var}\n</id>", parser.evalNode("/employee/@id").toString().trim());
     assertEquals(7, parser.evalNodes("/employee/*").size());
     XNode node = parser.evalNode("/employee/height");
     assertEquals("employee/height", node.getPath());
@@ -222,39 +222,69 @@ void formatXNodeToString() {
     // @formatter:off
     String usersNodeToStringExpect =
       "<users>\n"
-      + "    <user>\n"
-      + "        <id>100</id>\n"
-      + "        <name>Tom</name>\n"
-      + "        <age>30</age>\n"
-      + "        <cars>\n"
-      + "            <car index=\"1\">BMW</car>\n"
-      + "            <car index=\"2\">Audi</car>\n"
-      + "            <car index=\"3\">Benz</car>\n"
-      + "        </cars>\n"
-      + "    </user>\n"
+      + "  <user>\n"
+      + "    <id>\n"
+      + "      100\n"
+      + "    </id>\n"
+      + "    <name>\n"
+      + "      Tom\n"
+      + "    </name>\n"
+      + "    <age>\n"
+      + "      30\n"
+      + "    </age>\n"
+      + "    <cars>\n"
+      + "      <car index=\"1\">\n"
+      + "        BMW\n"
+      + "      </car>\n"
+      + "      <car index=\"2\">\n"
+      + "        Audi\n"
+      + "      </car>\n"
+      + "      <car index=\"3\">\n"
+      + "        Benz\n"
+      + "      </car>\n"
+      + "    </cars>\n"
+      + "  </user>\n"
       + "</users>\n";
     // @formatter:on
 
     // @formatter:off
     String userNodeToStringExpect =
       "<user>\n"
-      + "    <id>100</id>\n"
-      + "    <name>Tom</name>\n"
-      + "    <age>30</age>\n"
-      + "    <cars>\n"
-      + "        <car index=\"1\">BMW</car>\n"
-      + "        <car index=\"2\">Audi</car>\n"
-      + "        <car index=\"3\">Benz</car>\n"
-      + "    </cars>\n"
+      + "  <id>\n"
+      + "    100\n"
+      + "  </id>\n"
+      + "  <name>\n"
+      + "    Tom\n"
+      + "  </name>\n"
+      + "  <age>\n"
+      + "    30\n"
+      + "  </age>\n"
+      + "  <cars>\n"
+      + "    <car index=\"1\">\n"
+      + "      BMW\n"
+      + "    </car>\n"
+      + "    <car index=\"2\">\n"
+      + "      Audi\n"
+      + "    </car>\n"
+      + "    <car index=\"3\">\n"
+      + "      Benz\n"
+      + "    </car>\n"
+      + "  </cars>\n"
       + "</user>\n";
     // @formatter:on
 
     // @formatter:off
     String carsNodeToStringExpect =
       "<cars>\n"
-      + "    <car index=\"1\">BMW</car>\n"
-      + "    <car index=\"2\">Audi</car>\n"
-      + "    <car index=\"3\">Benz</car>\n"
+      + "  <car index=\"1\">\n"
+      + "    BMW\n"
+      + "  </car>\n"
+      + "  <car index=\"2\">\n"
+      + "    Audi\n"
+      + "  </car>\n"
+      + "  <car index=\"3\">\n"
+      + "    Benz\n"
+      + "  </car>\n"
       + "</cars>\n";
     // @formatter:on