Bug fixed: XPath queries that has only contained characters that are valid in XML element names and has also contained :: (which is valid in names in namespace-unware documents), like e['following-sibling::foo'], were interpreted as literal element names (giving 0 hits) rather than as XPath expressions. Note that there were no such problem with e['following-sibling::*'] for example, as it's not a valid XML element name according the XML specification. This fix can actually break applications that has processed namespace unaware XML that use :: as part of element or attribute names, but such an application is highly unlikely, unlike running into the fixed problem. (Unfortunately, using incompatible_improvements wasn't technically possible here.)
Project: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/commit/33ba278f Tree: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/tree/33ba278f Diff: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/diff/33ba278f Branch: refs/heads/master Commit: 33ba278fdd7a26653d2f96f945c3e5e701a9acf5 Parents: 55cb12e Author: ddekany <[email protected]> Authored: Mon Dec 28 20:17:40 2015 +0100 Committer: ddekany <[email protected]> Committed: Mon Dec 28 20:17:40 2015 +0100 ---------------------------------------------------------------------- .../java/freemarker/ext/dom/NodeListModel.java | 5 ++ src/main/java/freemarker/ext/dom/NodeModel.java | 6 ++- .../freemarker/template/utility/StringUtil.java | 27 +++++++--- src/manual/en_US/book.xml | 40 +++++++++++++- src/test/java/freemarker/ext/dom/DOMTest.java | 55 ++++++++++++++++++++ 5 files changed, 124 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/33ba278f/src/main/java/freemarker/ext/dom/NodeListModel.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/ext/dom/NodeListModel.java b/src/main/java/freemarker/ext/dom/NodeListModel.java index c0e85b9..22e555a 100644 --- a/src/main/java/freemarker/ext/dom/NodeListModel.java +++ b/src/main/java/freemarker/ext/dom/NodeListModel.java @@ -42,6 +42,11 @@ import freemarker.template.TemplateScalarModel; import freemarker.template.TemplateSequenceModel; import freemarker.template.utility.StringUtil; +/** + * Used when the result set contains 0 or multiple nodes; shouldn't be used when you have exactly 1 node. For exactly 1 + * node, use {@link NodeModel#wrap(Node)}, because {@link NodeModel} subclasses can have extra features building on that + * restriction, like single elements with text content can be used as FTL string-s. + */ class NodeListModel extends SimpleSequence implements TemplateHashModel, _UnexpectedTypeErrorExplainerTemplateModel { NodeModel contextNode; http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/33ba278f/src/main/java/freemarker/ext/dom/NodeModel.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/ext/dom/NodeModel.java b/src/main/java/freemarker/ext/dom/NodeModel.java index c0ade05..c338664 100644 --- a/src/main/java/freemarker/ext/dom/NodeModel.java +++ b/src/main/java/freemarker/ext/dom/NodeModel.java @@ -63,12 +63,16 @@ import freemarker.template.TemplateNumberModel; import freemarker.template.TemplateSequenceModel; /** - * A base class for wrapping a W3C DOM Node as a FreeMarker template model. + * A base class for wrapping a single W3C DOM Node as a FreeMarker template model. * * <p> * Note that {@link DefaultObjectWrapper} automatically wraps W3C DOM {@link Node}-s into this, so you may not need to * do that with this class manually. Though, before dropping the {@link Node}-s into the data-model, you may want to * apply {@link NodeModel#simplify(Node)} on them. + * + * <p> + * Note that this class shouldn't be used to represent a result set of 0 or multiple nodes (we use {@link NodeListModel} + * then), but should be used to represent a node set of exactly 1 node. */ abstract public class NodeModel implements TemplateNodeModel, TemplateHashModel, TemplateSequenceModel, http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/33ba278f/src/main/java/freemarker/template/utility/StringUtil.java ---------------------------------------------------------------------- diff --git a/src/main/java/freemarker/template/utility/StringUtil.java b/src/main/java/freemarker/template/utility/StringUtil.java index 881bb3b..973c1f4 100644 --- a/src/main/java/freemarker/template/utility/StringUtil.java +++ b/src/main/java/freemarker/template/utility/StringUtil.java @@ -1653,18 +1653,31 @@ public class StringUtil { } /** - * @return whether the name is a valid XML tagname. - * (This routine might only be 99% accurate. Should maybe REVISIT) + * Used internally by the XML DOM wrapper to check if the subvariable name is just an element name, or a more + * complex XPath expression. + * + * @return whether the name is a valid XML tagname. (This routine might only be 99% accurate. Should maybe REVISIT) + * + * @deprecated Don't use this outside FreeMarker; it's name if misleading, and it doesn't follow the XML specs. */ + @Deprecated static public boolean isXMLID(String name) { - for (int i = 0; i < name.length(); i++) { + int ln = name.length(); + for (int i = 0; i < ln; i++) { char c = name.charAt(i); - if (i == 0) { - if (c == '-' || c == '.' || Character.isDigit(c)) + if (i == 0 && (c == '-' || c == '.' || Character.isDigit(c))) { return false; } - if (!Character.isLetterOrDigit(c) && c != ':' && c != '_' && c != '-' && c != '.') { - return false; + if (!Character.isLetterOrDigit(c) && c != '_' && c != '-' && c != '.') { + if (c == ':') { + if (i + 1 < ln && name.charAt(i + 1) == ':') { + // "::" is used in XPath + return false; + } + // We don't return here, as a lonely ":" is allowed. + } else { + return false; + } } } return true; http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/33ba278f/src/manual/en_US/book.xml ---------------------------------------------------------------------- diff --git a/src/manual/en_US/book.xml b/src/manual/en_US/book.xml index a08d339..ba8c1b1 100644 --- a/src/manual/en_US/book.xml +++ b/src/manual/en_US/book.xml @@ -7250,7 +7250,7 @@ public class RepeatDirective implements TemplateDirectiveModel { nested calls of the same directive, or directive objects used as shared variables accessed by multiple threads concurrently.</para> - <para>Unfortunatelly, <literal>TemplateDirectiveModel</literal>-s + <para>Unfortunately, <literal>TemplateDirectiveModel</literal>-s don't support passing parameters by position (rather than by name). This is fixed starting from FreeMarker 2.4.</para> </section> @@ -26928,6 +26928,25 @@ TemplateModel x = env.getVariable("x"); // get variable x</programlisting> </listitem> <listitem> + <para>Bug fixed: XPath queries that has only contained + characters that are valid in XML element names and has also + contained <literal>::</literal> (which is valid in names in + namespace-unware documents), like + <literal>e['following-sibling::foo']</literal>, were interpreted + as literal element names (giving 0 hits) rather than as XPath + expressions. Note that there were no such problem with + <literal>e['following-sibling::*']</literal> for example, as + it's not a valid XML element name according the XML + specification. This fix can actually break applications that has + processed namespace unaware XML that use <literal>::</literal> + as part of element or attribute names, but such an application + is highly unlikely, unlike running into the fixed problem. + (Unfortunately, using + <literal>incompatible_improvements</literal> wasn't technically + possible here.)</para> + </listitem> + + <listitem> <para>Internal code cleanup: Mostly source code formatting, also many parser construction/setup cleanup</para> </listitem> @@ -27264,6 +27283,25 @@ TemplateModel x = env.getVariable("x"); // get variable x</programlisting> the imported template was already imported earlier in another namespace.</para> </listitem> + + <listitem> + <para>Bug fixed: XPath queries that has only contained + characters that are valid in XML element names and has also + contained <literal>::</literal> (which is valid in names in + namespace-unware documents), like + <literal>e['following-sibling::foo']</literal>, were interpreted + as literal element names (giving 0 hits) rather than as XPath + expressions. Note that there were no such problem with + <literal>e['following-sibling::*']</literal> for example, as + it's not a valid XML element name according the XML + specification. This fix can actually break applications that has + processed namespace unaware XML that use <literal>::</literal> + as part of element or attribute names, but such an application + is highly unlikely, unlike running into the fixed problem. + (Unfortunately, using + <literal>incompatible_improvements</literal> wasn't technically + possible here.)</para> + </listitem> </itemizedlist> </section> </section> http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/33ba278f/src/test/java/freemarker/ext/dom/DOMTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/freemarker/ext/dom/DOMTest.java b/src/test/java/freemarker/ext/dom/DOMTest.java new file mode 100644 index 0000000..182ac31 --- /dev/null +++ b/src/test/java/freemarker/ext/dom/DOMTest.java @@ -0,0 +1,55 @@ +package freemarker.ext.dom; + +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.*; + +import java.io.IOException; +import java.io.StringReader; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; + +import org.junit.Test; +import org.w3c.dom.Document; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +import freemarker.template.TemplateException; +import freemarker.test.TemplateTest; + +public class DOMTest extends TemplateTest { + + @Test + public void xpathDetectionBugfix() throws Exception { + addDocToDataModel("<root><a>A</a><b>B</b><c>C</c></root>"); + assertOutput("${doc.root.b['following-sibling::c']}", "C"); + assertOutput("${doc.root.b['following-sibling::*']}", "C"); + } + + @Test + public void namespaceUnaware() throws Exception { + addNSUnawareDocToDataModel("<root><x:a>A</x:a><:>B</:><xyz::c>C</xyz::c></root>"); + assertOutput("${doc.root['x:a']}", "A"); + assertOutput("${doc.root[':']}", "B"); + try { + assertOutput("${doc.root['xyz::c']}", "C"); + fail(); + } catch (TemplateException e) { + assertThat(e.getMessage(), containsString("xyz")); + } + } + + private void addDocToDataModel(String xml) throws SAXException, IOException, ParserConfigurationException { + addToDataModel("doc", NodeModel.parse(new InputSource(new StringReader(xml)))); + } + + private void addNSUnawareDocToDataModel(String xml) throws ParserConfigurationException, SAXException, IOException { + DocumentBuilderFactory newFactory = DocumentBuilderFactory.newInstance(); + newFactory.setNamespaceAware(false); + DocumentBuilder builder = newFactory.newDocumentBuilder(); + Document doc = builder.parse(new InputSource(new StringReader(xml))); + addToDataModel("doc", doc); + } + +}
