From 5980cc1642ac71a7dd191e0ceb31ff0cfbf604fa Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Wed, 8 Nov 2023 01:32:17 +0900 Subject: [PATCH 1/5] 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 `3`. 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 --- .../java/org/apache/ibatis/parsing/XNode.java | 52 +++++----- .../org/apache/ibatis/parsing/XNodeTest.java | 95 +++++++++++++++++++ .../ibatis/parsing/XPathParserTest.java | 74 ++++++++++----- 3 files changed, 172 insertions(+), 49 deletions(-) create mode 100644 src/test/java/org/apache/ibatis/parsing/XNodeTest.java diff --git a/src/main/java/org/apache/ibatis/parsing/XNode.java b/src/main/java/org/apache/ibatis/parsing/XNode.java index a7c9519454e..b5b7ac82c60 100644 --- a/src/main/java/org/apache/ibatis/parsing/XNode.java +++ b/src/main/java/org/apache/ibatis/parsing/XNode.java @@ -280,14 +280,11 @@ 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 entry : attributes.entrySet()) { builder.append(" "); builder.append(entry.getKey()); @@ -295,34 +292,35 @@ private void toString(StringBuilder builder, int level) { builder.append(entry.getValue()); builder.append("\""); } - List 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(""); - } else if (body != null) { - builder.append(">"); - builder.append(body); - builder.append(""); - } else { - builder.append("/>"); - indent(builder, level); + indent(builder, indent).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) { diff --git a/src/test/java/org/apache/ibatis/parsing/XNodeTest.java b/src/test/java/org/apache/ibatis/parsing/XNodeTest.java new file mode 100644 index 00000000000..c43dc72bdb2 --- /dev/null +++ b/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 = "\n" + + " \n" + + ""; + + String expected = "\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 = "y = ${y}x = ${x}"; + String expected = "\n y = bar\n \n x = foo\n \n\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()); + } + +} diff --git a/src/test/java/org/apache/ibatis/parsing/XPathParserTest.java b/src/test/java/org/apache/ibatis/parsing/XPathParserTest.java index aee70307a2e..d255eb2caf4 100644 --- a/src/test/java/org/apache/ibatis/parsing/XPathParserTest.java +++ b/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_var}", parser.evalNode("/employee/@id").toString().trim()); + assertEquals("\n ${id_var}\n", 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 = "\n" - + " \n" - + " 100\n" - + " Tom\n" - + " 30\n" - + " \n" - + " BMW\n" - + " Audi\n" - + " Benz\n" - + " \n" - + " \n" + + " \n" + + " \n" + + " 100\n" + + " \n" + + " \n" + + " Tom\n" + + " \n" + + " \n" + + " 30\n" + + " \n" + + " \n" + + " \n" + + " BMW\n" + + " \n" + + " \n" + + " Audi\n" + + " \n" + + " \n" + + " Benz\n" + + " \n" + + " \n" + + " \n" + "\n"; // @formatter:on // @formatter:off String userNodeToStringExpect = "\n" - + " 100\n" - + " Tom\n" - + " 30\n" - + " \n" - + " BMW\n" - + " Audi\n" - + " Benz\n" - + " \n" + + " \n" + + " 100\n" + + " \n" + + " \n" + + " Tom\n" + + " \n" + + " \n" + + " 30\n" + + " \n" + + " \n" + + " \n" + + " BMW\n" + + " \n" + + " \n" + + " Audi\n" + + " \n" + + " \n" + + " Benz\n" + + " \n" + + " \n" + "\n"; // @formatter:on // @formatter:off String carsNodeToStringExpect = "\n" - + " BMW\n" - + " Audi\n" - + " Benz\n" + + " \n" + + " BMW\n" + + " \n" + + " \n" + + " Audi\n" + + " \n" + + " \n" + + " Benz\n" + + " \n" + "\n"; // @formatter:on From bd58d4b1e338c51ea621bba0a00c8b3d1816bc54 Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Wed, 8 Nov 2023 01:33:46 +0900 Subject: [PATCH 2/5] Moved an old test --- .../org/apache/ibatis/parsing/XNodeTest.java | 82 +++++++++++++++++++ .../ibatis/parsing/XPathParserTest.java | 82 ------------------- 2 files changed, 82 insertions(+), 82 deletions(-) diff --git a/src/test/java/org/apache/ibatis/parsing/XNodeTest.java b/src/test/java/org/apache/ibatis/parsing/XNodeTest.java index c43dc72bdb2..ab0cc9d86fe 100644 --- a/src/test/java/org/apache/ibatis/parsing/XNodeTest.java +++ b/src/test/java/org/apache/ibatis/parsing/XNodeTest.java @@ -24,6 +24,88 @@ class XNodeTest { + @Test + void formatXNodeToString() { + XPathParser parser = new XPathParser( + "100Tom30BMWAudiBenz"); + String usersNodeToString = parser.evalNode("/users").toString(); + String userNodeToString = parser.evalNode("/users/user").toString(); + String carsNodeToString = parser.evalNode("/users/user/cars").toString(); + + // @formatter:off + String usersNodeToStringExpect = + "\n" + + " \n" + + " \n" + + " 100\n" + + " \n" + + " \n" + + " Tom\n" + + " \n" + + " \n" + + " 30\n" + + " \n" + + " \n" + + " \n" + + " BMW\n" + + " \n" + + " \n" + + " Audi\n" + + " \n" + + " \n" + + " Benz\n" + + " \n" + + " \n" + + " \n" + + "\n"; + // @formatter:on + + // @formatter:off + String userNodeToStringExpect = + "\n" + + " \n" + + " 100\n" + + " \n" + + " \n" + + " Tom\n" + + " \n" + + " \n" + + " 30\n" + + " \n" + + " \n" + + " \n" + + " BMW\n" + + " \n" + + " \n" + + " Audi\n" + + " \n" + + " \n" + + " Benz\n" + + " \n" + + " \n" + + "\n"; + // @formatter:on + + // @formatter:off + String carsNodeToStringExpect = + "\n" + + " \n" + + " BMW\n" + + " \n" + + " \n" + + " Audi\n" + + " \n" + + " \n" + + " Benz\n" + + " \n" + + "\n"; + // @formatter:on + + assertEquals(usersNodeToStringExpect, usersNodeToString); + assertEquals(userNodeToStringExpect, userNodeToString); + assertEquals(carsNodeToStringExpect, carsNodeToString); + } + @Test void xNodeToString() { // @formatter:off diff --git a/src/test/java/org/apache/ibatis/parsing/XPathParserTest.java b/src/test/java/org/apache/ibatis/parsing/XPathParserTest.java index d255eb2caf4..f82c912432f 100644 --- a/src/test/java/org/apache/ibatis/parsing/XPathParserTest.java +++ b/src/test/java/org/apache/ibatis/parsing/XPathParserTest.java @@ -211,86 +211,4 @@ private void testEvalMethod(XPathParser parser) { assertEquals("employee[${id_var}]_height", node.getValueBasedIdentifier()); } - @Test - void formatXNodeToString() { - XPathParser parser = new XPathParser( - "100Tom30BMWAudiBenz"); - String usersNodeToString = parser.evalNode("/users").toString(); - String userNodeToString = parser.evalNode("/users/user").toString(); - String carsNodeToString = parser.evalNode("/users/user/cars").toString(); - - // @formatter:off - String usersNodeToStringExpect = - "\n" - + " \n" - + " \n" - + " 100\n" - + " \n" - + " \n" - + " Tom\n" - + " \n" - + " \n" - + " 30\n" - + " \n" - + " \n" - + " \n" - + " BMW\n" - + " \n" - + " \n" - + " Audi\n" - + " \n" - + " \n" - + " Benz\n" - + " \n" - + " \n" - + " \n" - + "\n"; - // @formatter:on - - // @formatter:off - String userNodeToStringExpect = - "\n" - + " \n" - + " 100\n" - + " \n" - + " \n" - + " Tom\n" - + " \n" - + " \n" - + " 30\n" - + " \n" - + " \n" - + " \n" - + " BMW\n" - + " \n" - + " \n" - + " Audi\n" - + " \n" - + " \n" - + " Benz\n" - + " \n" - + " \n" - + "\n"; - // @formatter:on - - // @formatter:off - String carsNodeToStringExpect = - "\n" - + " \n" - + " BMW\n" - + " \n" - + " \n" - + " Audi\n" - + " \n" - + " \n" - + " Benz\n" - + " \n" - + "\n"; - // @formatter:on - - assertEquals(usersNodeToStringExpect, usersNodeToString); - assertEquals(userNodeToStringExpect, userNodeToString); - assertEquals(carsNodeToStringExpect, carsNodeToString); - } - } From ca74a2ceb454e1af2e6564f41f52407c414499d1 Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Wed, 8 Nov 2023 09:42:18 +0900 Subject: [PATCH 3/5] Avoid recalculating node length --- src/main/java/org/apache/ibatis/parsing/XNode.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/ibatis/parsing/XNode.java b/src/main/java/org/apache/ibatis/parsing/XNode.java index b5b7ac82c60..53f1d870abd 100644 --- a/src/main/java/org/apache/ibatis/parsing/XNode.java +++ b/src/main/java/org/apache/ibatis/parsing/XNode.java @@ -294,11 +294,12 @@ private StringBuilder buildToString(StringBuilder builder, int indent) { } NodeList nodeList = node.getChildNodes(); - if (nodeList == null || nodeList.getLength() == 0) { + int nodeLen = nodeList.getLength(); + if (nodeList == null || nodeLen == 0) { builder.append(" />\n"); } else { builder.append(">\n"); - for (int i = 0, n = nodeList.getLength(); i < n; i++) { + for (int i = 0, n = nodeLen; i < n; i++) { Node node = nodeList.item(i); short nodeType = node.getNodeType(); if (nodeType == Node.ELEMENT_NODE) { From d9eb9e5b7e2d950a1038e91651b5543d02037853 Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Wed, 8 Nov 2023 11:18:43 +0900 Subject: [PATCH 4/5] Revert "Avoid recalculating node length" ca74a2ceb454e1af2e6564f41f52407c414499d1 --- src/main/java/org/apache/ibatis/parsing/XNode.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/apache/ibatis/parsing/XNode.java b/src/main/java/org/apache/ibatis/parsing/XNode.java index 53f1d870abd..b5b7ac82c60 100644 --- a/src/main/java/org/apache/ibatis/parsing/XNode.java +++ b/src/main/java/org/apache/ibatis/parsing/XNode.java @@ -294,12 +294,11 @@ private StringBuilder buildToString(StringBuilder builder, int indent) { } NodeList nodeList = node.getChildNodes(); - int nodeLen = nodeList.getLength(); - if (nodeList == null || nodeLen == 0) { + if (nodeList == null || nodeList.getLength() == 0) { builder.append(" />\n"); } else { builder.append(">\n"); - for (int i = 0, n = nodeLen; i < n; i++) { + for (int i = 0, n = nodeList.getLength(); i < n; i++) { Node node = nodeList.item(i); short nodeType = node.getNodeType(); if (nodeType == Node.ELEMENT_NODE) { From b880ac03c6cb77bf1cb4e82b5d365826fbf61cfa Mon Sep 17 00:00:00 2001 From: Iwao AVE! Date: Wed, 8 Nov 2023 11:32:20 +0900 Subject: [PATCH 5/5] Rename argument --- src/main/java/org/apache/ibatis/parsing/XNode.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/apache/ibatis/parsing/XNode.java b/src/main/java/org/apache/ibatis/parsing/XNode.java index b5b7ac82c60..6d9a3f8aa5d 100644 --- a/src/main/java/org/apache/ibatis/parsing/XNode.java +++ b/src/main/java/org/apache/ibatis/parsing/XNode.java @@ -283,8 +283,8 @@ public String toString() { return buildToString(new StringBuilder(), 0).toString(); } - private StringBuilder buildToString(StringBuilder builder, int indent) { - indent(builder, indent).append("<").append(name); + private StringBuilder buildToString(StringBuilder builder, int indentLevel) { + indent(builder, indentLevel).append("<").append(name); for (Map.Entry entry : attributes.entrySet()) { builder.append(" "); builder.append(entry.getKey()); @@ -302,15 +302,15 @@ private StringBuilder buildToString(StringBuilder builder, int indent) { Node node = nodeList.item(i); short nodeType = node.getNodeType(); if (nodeType == Node.ELEMENT_NODE) { - new XNode(xpathParser, node, variables).buildToString(builder, indent + 1); + new XNode(xpathParser, node, variables).buildToString(builder, indentLevel + 1); } else { String text = getBodyData(node).trim(); if (text.length() > 0) { - indent(builder, indent + 1).append(text).append("\n"); + indent(builder, indentLevel + 1).append(text).append("\n"); } } } - indent(builder, indent).append("\n"); + indent(builder, indentLevel).append("\n"); } return builder;