From c21784843c57647a36613476a02b98805bab5d1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 26 Oct 2020 17:47:48 +0100 Subject: [PATCH 1/6] Check XPath expression result for correct type Fixes #1939 --- .../lang/rule/xpath/SaxonXPathRuleQuery.java | 16 ++++++---------- .../rule/xpath/SaxonXPathRuleQueryTest.java | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java index 4bcb8f7423..b1456b5754 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java @@ -120,25 +120,21 @@ public class SaxonXPathRuleQuery extends AbstractXPathRuleQuery { assert rootElementNode != null : "Cannot find " + node; final XPathDynamicContext xpathDynamicContext = createDynamicContext(rootElementNode); - final List nodes = new LinkedList<>(); + final List results = new LinkedList<>(); List expressions = getXPathExpressionForNodeOrDefault(node.getXPathNodeName()); for (Expression expression : expressions) { SequenceIterator iterator = expression.iterate(xpathDynamicContext.getXPathContextObject()); Item current = iterator.next(); while (current != null) { - nodes.add((ElementNode) current); + if (current instanceof ElementNode) { + results.add((Node) ((ElementNode) current).getUnderlyingNode()); + } else { + throw new RuntimeException("XPath rule expression returned a non-node (" + current.getClass() + "): " + current); + } current = iterator.next(); } } - /* - Map List of Saxon Nodes -> List of AST Nodes, which were detected to match the XPath expression - (i.e. violation found) - */ - final List results = new ArrayList<>(nodes.size()); - for (final ElementNode elementNode : nodes) { - results.add((Node) elementNode.getUnderlyingNode()); - } Collections.sort(results, RuleChainAnalyzer.documentOrderComparator()); return results; } catch (final XPathException e) { diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java index 0920d7c4e4..457ccdd6a0 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java @@ -9,8 +9,11 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import org.hamcrest.CoreMatchers; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.lang.ast.DummyNodeWithListAndEnum; @@ -22,6 +25,9 @@ import net.sf.saxon.expr.Expression; public class SaxonXPathRuleQueryTest { + @Rule + public final ExpectedException expected = ExpectedException.none(); + @Test public void testListAttribute() { DummyNodeWithListAndEnum dummy = new DummyNodeWithListAndEnum(1); @@ -37,6 +43,18 @@ public class SaxonXPathRuleQueryTest { assertQuery(0, "//dummyNode[@EmptyList = (\"A\")]", dummy); } + @Test + public void testInvalidReturn() { + DummyNodeWithListAndEnum dummy = new DummyNodeWithListAndEnum(1); + + + expected.expect(RuntimeException.class); + expected.expectMessage(CoreMatchers.containsString("XPath rule expression returned a non-node")); + expected.expectMessage(CoreMatchers.containsString("Int64Value")); + + createQuery("1+2").evaluate(dummy, new RuleContext()); + } + @Test public void ruleChainVisits() { SaxonXPathRuleQuery query = createQuery("//dummyNode[@Image='baz']/foo | //bar[@Public = 'true'] | //dummyNode[@Public = false()] | //dummyNode"); From 7a8ca28c3f7cf3e895fc106f9496eeb7567e6c15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 26 Oct 2020 17:54:33 +0100 Subject: [PATCH 2/6] Check result of / expression Refs #1938 --- .../pmd/lang/ast/xpath/internal/AstNodeOwner.java | 15 +++++++++++++++ .../pmd/lang/ast/xpath/saxon/DocumentNode.java | 10 +++++++++- .../pmd/lang/ast/xpath/saxon/ElementNode.java | 5 +++-- .../pmd/lang/rule/xpath/SaxonXPathRuleQuery.java | 5 +++-- .../lang/rule/xpath/SaxonXPathRuleQueryTest.java | 12 +++++++++++- 5 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/internal/AstNodeOwner.java diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/internal/AstNodeOwner.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/internal/AstNodeOwner.java new file mode 100644 index 0000000000..3d7f3a7f7e --- /dev/null +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/internal/AstNodeOwner.java @@ -0,0 +1,15 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.ast.xpath.internal; + +import net.sourceforge.pmd.lang.ast.Node; + +/** + * Marker interface. + */ +public interface AstNodeOwner { + + Node getUnderlyingNode(); +} diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java index d91f1828a6..23d99fb778 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java @@ -10,6 +10,7 @@ import java.util.Map; import net.sourceforge.pmd.annotation.InternalApi; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.ast.xpath.internal.AstNodeOwner; import net.sourceforge.pmd.lang.ast.xpath.internal.DeprecatedAttrLogger; import net.sourceforge.pmd.lang.rule.xpath.SaxonXPathRuleQuery; @@ -27,7 +28,7 @@ import net.sf.saxon.type.Type; */ @Deprecated @InternalApi -public class DocumentNode extends BaseNodeInfo implements DocumentInfo { +public class DocumentNode extends BaseNodeInfo implements DocumentInfo, AstNodeOwner { /** * The root ElementNode of the DocumentNode. @@ -99,6 +100,13 @@ public class DocumentNode extends BaseNodeInfo implements DocumentInfo { } } + @Override + public Node getUnderlyingNode() { + // this is a concession to the model, so that the expression "/" + // may be interpreted as the root node + return rootNode.getUnderlyingNode(); + } + public DeprecatedAttrLogger getAttrCtx() { return attrCtx == null ? DeprecatedAttrLogger.noop() : attrCtx; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/ElementNode.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/ElementNode.java index 6db157eac4..e9341542ea 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/ElementNode.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/ElementNode.java @@ -11,6 +11,7 @@ import java.util.Map; import net.sourceforge.pmd.annotation.InternalApi; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.ast.xpath.Attribute; +import net.sourceforge.pmd.lang.ast.xpath.internal.AstNodeOwner; import net.sourceforge.pmd.lang.rule.xpath.SaxonXPathRuleQuery; import net.sf.saxon.om.Axis; @@ -38,7 +39,7 @@ import net.sf.saxon.value.Value; */ @Deprecated @InternalApi -public class ElementNode extends BaseNodeInfo { +public class ElementNode extends BaseNodeInfo implements AstNodeOwner { protected final DocumentNode document; protected final ElementNode parent; @@ -94,7 +95,7 @@ public class ElementNode extends BaseNodeInfo { } @Override - public Object getUnderlyingNode() { + public Node getUnderlyingNode() { return node; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java index b1456b5754..2a8815798c 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQuery.java @@ -17,6 +17,7 @@ import java.util.regex.Pattern; import net.sourceforge.pmd.RuleContext; import net.sourceforge.pmd.annotation.InternalApi; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.ast.xpath.internal.AstNodeOwner; import net.sourceforge.pmd.lang.ast.xpath.internal.DeprecatedAttrLogger; import net.sourceforge.pmd.lang.ast.xpath.saxon.DocumentNode; import net.sourceforge.pmd.lang.ast.xpath.saxon.ElementNode; @@ -126,8 +127,8 @@ public class SaxonXPathRuleQuery extends AbstractXPathRuleQuery { SequenceIterator iterator = expression.iterate(xpathDynamicContext.getXPathContextObject()); Item current = iterator.next(); while (current != null) { - if (current instanceof ElementNode) { - results.add((Node) ((ElementNode) current).getUnderlyingNode()); + if (current instanceof AstNodeOwner) { + results.add(((AstNodeOwner) current).getUnderlyingNode()); } else { throw new RuntimeException("XPath rule expression returned a non-node (" + current.getClass() + "): " + current); } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java index 457ccdd6a0..e2be132092 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java @@ -16,6 +16,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; import net.sourceforge.pmd.RuleContext; +import net.sourceforge.pmd.lang.ast.DummyNode; import net.sourceforge.pmd.lang.ast.DummyNodeWithListAndEnum; import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.properties.PropertyDescriptor; @@ -55,6 +56,14 @@ public class SaxonXPathRuleQueryTest { createQuery("1+2").evaluate(dummy, new RuleContext()); } + @Test + public void testRootExpression() { + DummyNode dummy = new DummyNode(); // todo in pmd 7 this should be a RootNode + + List result = assertQuery(1, "/", dummy); + Assert.assertEquals(dummy, result.get(0)); + } + @Test public void ruleChainVisits() { SaxonXPathRuleQuery query = createQuery("//dummyNode[@Image='baz']/foo | //bar[@Public = 'true'] | //dummyNode[@Public = false()] | //dummyNode"); @@ -193,10 +202,11 @@ public class SaxonXPathRuleQueryTest { assertExpression("((DocumentSorter(((((/)/descendant::element(dummyNode, xs:anyType))[QuantifiedExpression(Atomizer(attribute::attribute(Image, xs:anyAtomicType)), ($qq:qq692331943 singleton eq \"baz\"))])/child::element(foo, xs:anyType))) | (((/)/descendant::element(bar, xs:anyType))[QuantifiedExpression(Atomizer(attribute::attribute(Public, xs:anyAtomicType)), ($qq:qq2127036371 singleton eq \"true\"))])) | (((/)/descendant::element(dummyNode, xs:anyType))[QuantifiedExpression(Atomizer(attribute::attribute(Public, xs:anyAtomicType)), ($qq:qq1529060733 singleton eq \"false\"))]))", query.nodeNameToXPaths.get(SaxonXPathRuleQuery.AST_ROOT).get(0)); } - private static void assertQuery(int resultSize, String xpath, Node node) { + private static List assertQuery(int resultSize, String xpath, Node node) { SaxonXPathRuleQuery query = createQuery(xpath); List result = query.evaluate(node, new RuleContext()); Assert.assertEquals(resultSize, result.size()); + return result; } private static SaxonXPathRuleQuery createQuery(String xpath, PropertyDescriptor ...descriptors) { From 0a4dd810c9901cb4a80219b7e7e1938805c573ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 26 Oct 2020 17:57:42 +0100 Subject: [PATCH 3/6] Test remaining cases of #1938 --- .../pmd/lang/ast/xpath/saxon/DocumentNode.java | 2 ++ .../rule/xpath/SaxonXPathRuleQueryTest.java | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java index 23d99fb778..65c5ecec8b 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/lang/ast/xpath/saxon/DocumentNode.java @@ -95,6 +95,8 @@ public class DocumentNode extends BaseNodeInfo implements DocumentInfo, AstNodeO return new Navigator.DescendantEnumeration(this, true, true); case Axis.CHILD: return SingleNodeIterator.makeIterator(rootNode); + case Axis.SELF: + return SingleNodeIterator.makeIterator(this); default: return super.iterateAxis(axisNumber); } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java index e2be132092..bfe925bac3 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/lang/rule/xpath/SaxonXPathRuleQueryTest.java @@ -64,6 +64,24 @@ public class SaxonXPathRuleQueryTest { Assert.assertEquals(dummy, result.get(0)); } + @Test + public void testRootExpressionIsADocumentNode() { + DummyNode dummy = new DummyNode(); // todo in pmd 7 this should be a RootNode + + List result = assertQuery(1, "(/)[self::document-node()]", dummy); + Assert.assertEquals(dummy, result.get(0)); + } + + @Test + public void testRootExpressionWithName() { + DummyNode dummy = new DummyNode(0, false, "DummyNodeA"); // todo in pmd 7 this should be a RootNode + + List result = assertQuery(1, "(/)[self::document-node(element(DummyNodeA))]", dummy); + Assert.assertEquals(dummy, result.get(0)); + + assertQuery(0, "(/)[self::document-node(element(DummyNodeX))]", dummy); + } + @Test public void ruleChainVisits() { SaxonXPathRuleQuery query = createQuery("//dummyNode[@Image='baz']/foo | //bar[@Public = 'true'] | //dummyNode[@Public = false()] | //dummyNode"); From 27441ae3dcc5c3e05dc93fa2726f086e4c467a6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 26 Oct 2020 18:30:21 +0100 Subject: [PATCH 4/6] Document #1938 in migration guide --- .../pmd/userdocs/extending/writing_xpath_rules.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/pages/pmd/userdocs/extending/writing_xpath_rules.md b/docs/pages/pmd/userdocs/extending/writing_xpath_rules.md index 5b8f886623..a24a512cf7 100644 --- a/docs/pages/pmd/userdocs/extending/writing_xpath_rules.md +++ b/docs/pages/pmd/userdocs/extending/writing_xpath_rules.md @@ -127,6 +127,18 @@ represented by our 1.0 implementation as strings, meaning that `@BeginLine > "1" worked ---that's not the case in 2.0 mode. * @ArgumentCount > '1' → `@ArgumentCount > 1` +* In XPath 1.0, the expression `/Foo` matches the *children* of the root named `Foo`. +In XPath 2.0, that expression matches the root, if it is named `Foo`. Consider the following tree: +```java +Foo +└─ Foo +└─ Foo +``` +Then `/Foo` will match the root in XPath 2, and the other nodes (but not the root) in XPath 1. +See eg [an issue caused by this](https://github.com/pmd/pmd/issues/1919#issuecomment-512865434) in Apex, +with nested classes. + + ## Rule properties **See [Defining rule properties](pmd_userdocs_extending_defining_properties.html#for-xpath-rules)** From ddcf3536b438c4e7ec8e1ce85a07e2f9bc8c3dcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Mon, 26 Oct 2020 18:32:53 +0100 Subject: [PATCH 5/6] Update release notes, refs #1939 --- docs/pages/release_notes.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index b8f8783555..973e2dd715 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -16,6 +16,10 @@ This is a {{ site.pmd.release_type }} release. ### Fixed Issues +- pmd-core + - [#1939](https://github.com/pmd/pmd/issues/1939) \[core] XPath expressions return handling + + ### API Changes ### External Contributions From 3e23e6645e92d022bca4378c282712f94dcc1952 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fournier?= Date: Wed, 28 Oct 2020 22:48:03 +0100 Subject: [PATCH 6/6] Fix release notes format Co-authored-by: Andreas Dangel --- docs/pages/release_notes.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 973e2dd715..1f2ee87b35 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -16,8 +16,8 @@ This is a {{ site.pmd.release_type }} release. ### Fixed Issues -- pmd-core - - [#1939](https://github.com/pmd/pmd/issues/1939) \[core] XPath expressions return handling +* pmd-core + * [#1939](https://github.com/pmd/pmd/issues/1939): \[core] XPath expressions return handling ### API Changes @@ -25,4 +25,3 @@ This is a {{ site.pmd.release_type }} release. ### External Contributions {% endtocmaker %} -