[core] Make Saxon support multi valued XPath properties by oowekyala · Pull Request #736 · pmd/pmd

oowekyala · 2017-11-17T18:21:58Z

As exposed in #735, XPath rules couldn't declare multi valued properties due to a trivial error. That is now corrected.

Additionally, XPath rules couldn't actually use such properties, nor did they support all properties they could (see the code which translates PMD properties to XPath variables for Saxon). I corrected that for Saxon, making use of the sequence datatype provided by XPath 2.0. Jaxen treats any value as a string anyway

Hopefully this can make XPath rules more expressive, and make better use of saxon's capabilities.

Some possible improvements

There are factories for TypeProperty, MethodProperty, their multivalue counterparts, and FileProperty in PropertyDescriptorUtil, so users can technically declare such properties on an XPath rule. However, their use only throws an exception (with Saxon), because we don't translate the value to a Saxon value. In fact, there's no corresponding data type in the XQuery Data Model, which means that, at best, Saxon could treat the values of these properties as "untyped atomic" values. In effect they'd be pretty much used like strings, but without type safety. So either we do that, or...
I'd say that we should remove the factories for Method and File properties. They're unlikely to be useful anyway, and people can use StringProperty instead. TypeProperty could still be useful, especially with type resolution. Wdyt?
I'm working on a documentation PR for properties, and will include info about declaring properties from the XPath (previously undocumented). I think that, documenting it means it should work, which means we must write more tests (yay).

Should I create tickets to track these, too?

oowekyala · 2017-11-18T14:40:25Z

Sidenote: Maybe we should throw an error when trying to use a multivalued property with Jaxen. The problem is that it treats the value as a string, which is the toString of the list (eg. "[1, 2]"), which isn't usable.

adangel · 2017-11-19T09:43:45Z

        Map<String, PropertyDescriptorExternalBuilder<?>> temp = new HashMap<>(18);
        temp.put("Boolean", BooleanProperty.extractor());
-        temp.put("List<Boolean>", BooleanMultiProperty.extractor());
+        temp.put("List[Boolean]", BooleanMultiProperty.extractor());


Wouldn't that break compatibility with existing (Java-based) rules, that use already multi-valued properties? Maybe we can have both variants in the map?

Java-based rules don't use these factories, only XPath rules.

It breaks compatibility, but no more than the List<> form. As of release 5.8.1, the name of those properties had the form Boolean[], like an array (the List<> form was introduced during #504). Whatever we choose breaks compatibility.

But I have to stress that none of that is documented behaviour for now. It's very unlikely anyone uses it, and if they do, documentation and release notes may be enough.

@oowekyala I agree - the old code for XPath 1.0 (Jaxen) just does a toString (which was for multi-valued properties just an array) and with XPath 1.0 compatible/2.0 (Saxon) we rejected such properties.

So, yes, we can choose freely here. Avoiding the < helps for usage inside XML, so I'm good with your suggestion List[Boolean].

adangel · 2017-11-19T09:47:40Z

@oowekyala Could you check, whether this todo is the same problem? Not possible to declare a multi-valued string property for a xpath rule? Thanks!

oowekyala · 2017-11-19T12:36:12Z

@adangel The todo is now fixed. The type attribute was missing, so the property was not declared. Besides that, I think the person who wrote this originally intended using strings, not sequences, which could have been solved earlier if type="String" was provided. Though using sequences is more elegant I think

The todo also mentioned using the value of the property in the message, which isn't currently supported. There may be a use case for it though.

oowekyala · 2017-11-19T13:01:46Z

Err on jdk8 the tests of InvalidDependencyType work, but on jdk9 there's an IllegalAccessException thrown from there. Debugger shows that it's a call to getImage. Any ideas?

java.lang.IllegalAccessException: class net.sourceforge.pmd.lang.ast.xpath.Attribute cannot access class com.sun.proxy.jdk.proxy2.$Proxy10 (in module jdk.proxy2) because module jdk.proxy2 does not export com.sun.proxy.jdk.proxy2 to unnamed module @59e84876
	at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:361)
	at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:589)
	at java.base/java.lang.reflect.Method.invoke(Method.java:556)
	at net.sourceforge.pmd.lang.ast.xpath.Attribute.getValue(Attribute.java:43)
	at net.sourceforge.pmd.lang.ast.xpath.saxon.AttributeNode.atomize(AttributeNode.java:50)
	at net.sf.saxon.om.AxisIteratorImpl.atomize(AxisIteratorImpl.java:74)
	at net.sf.saxon.expr.AxisAtomizingIterator.next(AxisAtomizingIterator.java:43)
	at net.sf.saxon.value.SequenceExtent.<init>(SequenceExtent.java:98)
	at net.sf.saxon.value.SequenceExtent.makeSequenceExtent(SequenceExtent.java:134)
	at net.sf.saxon.expr.GeneralComparison.effectiveBooleanValue(GeneralComparison.java:521)
	at net.sf.saxon.functions.BooleanFn.effectiveBooleanValue(BooleanFn.java:179)
	at net.sf.saxon.expr.FilterIterator$NonNumeric.matches(FilterIterator.java:182)
	at net.sf.saxon.expr.FilterIterator.getNextMatchingItem(FilterIterator.java:65)
	at net.sf.saxon.expr.FilterIterator.next(FilterIterator.java:44)
	at net.sf.saxon.value.SequenceExtent.<init>(SequenceExtent.java:98)
	at net.sf.saxon.sort.DocumentOrderIterator.<init>(DocumentOrderIterator.java:30)
	at net.sf.saxon.sort.DocumentSorter.iterate(DocumentSorter.java:85)
	at net.sf.saxon.sxpath.XPathExpression.evaluate(XPathExpression.java:98)
	at net.sourceforge.pmd.lang.rule.xpath.SaxonXPathRuleQuery.evaluate(SaxonXPathRuleQuery.java:96)
	at net.sourceforge.pmd.lang.rule.XPathRule.evaluate(XPathRule.java:93)
	at net.sourceforge.pmd.lang.rule.XPathRule.apply(XPathRule.java:78)
	at net.sourceforge.pmd.RuleSet.apply(RuleSet.java:473)
	at net.sourceforge.pmd.RuleSets.apply(RuleSets.java:143)
	at net.sourceforge.pmd.SourceCodeProcessor.processSource(SourceCodeProcessor.java:181)
	at net.sourceforge.pmd.SourceCodeProcessor.processSourceCode(SourceCodeProcessor.java:95)
	at net.sourceforge.pmd.testframework.RuleTst.runTestFromString(RuleTst.java:273)
	at net.sourceforge.pmd.testframework.RuleTst.runTestFromString(RuleTst.java:280)
	at net.sourceforge.pmd.testframework.RuleTst.processUsingStringReader(RuleTst.java:247)
	at net.sourceforge.pmd.testframework.RuleTst.runTest(RuleTst.java:139)
	at net.sourceforge.pmd.testframework.RuleTestRunner$2.evaluate(RuleTestRunner.java:116)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at net.sourceforge.pmd.testframework.RuleTestRunner.runChild(RuleTestRunner.java:101)
	at net.sourceforge.pmd.testframework.RuleTestRunner.runChild(RuleTestRunner.java:36)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at net.sourceforge.pmd.testframework.PMDTestRunner.run(PMDTestRunner.java:95)

oowekyala · 2017-11-19T16:53:24Z

After some digging I found this relevant resource. I think we may be in case 2.a : the proxy we use is in a dynamic module, (here named jdk.proxy2) which makes it inaccessible to the unnamed package (in which the code of pmd currently lives I guess). No idea how to correct that though

Edit: This issue is not directly related to this PR. I'm reverting the commit, we can keep track of the issue in #739

This reverts commit 9e6be2d.

adangel · 2017-11-24T13:38:16Z

Note when merging this: Update the doc at Working with properties

Refs #736

adangel · 2017-11-27T20:05:23Z

@oowekyala FYI: I've decided to merge this into 6.0.0. Since it's ready and it's actually a new feature: xpath multi valued properties can be used now!

oowekyala · 2017-11-27T21:37:31Z

@adangel Great :)

oowekyala added 2 commits November 17, 2017 14:09

Change List<> syntax to List[] for xpath prop descriptors

09da425

Make Saxon support multival properties as XPath sequences

652b800

adangel added this to the 6.1.0 milestone Nov 19, 2017

adangel reviewed Nov 19, 2017

View reviewed changes

Fix xml/InvalidDependencyTypes property

9e6be2d

Make attributes support full value range

d4b3972

Revert "Fix xml/InvalidDependencyTypes property", which triggers pmd#739

7fa795b

This reverts commit 9e6be2d.

oowekyala mentioned this pull request Nov 21, 2017

[doc] Update the documentation for properties #749

Merged

adangel self-assigned this Nov 27, 2017

adangel modified the milestones: 6.1.0, 6.0.0 Nov 27, 2017

adangel added the an:enhancement An improvement on existing features / rules label Nov 27, 2017

adangel merged commit 7fa795b into pmd:master Nov 27, 2017

adangel added a commit that referenced this pull request Nov 27, 2017

Update release notes, refs #736

acef94f

adangel added a commit that referenced this pull request Nov 27, 2017

[doc] Update working with properties

7976170

Refs #736

oowekyala deleted the fix-xpath-properties branch November 27, 2017 21:37

oowekyala mentioned this pull request Nov 27, 2017

[core] Turn property descriptor util into an enum and enrich its interface #763

Closed

oowekyala added the in:xpath Relating to xpath support at large, eg Jaxen / Saxon, custom functions, attribute resolution label May 18, 2018

adangel mentioned this pull request Apr 27, 2020

[core] Deprecate support for List attributes with XPath 2.0 #2451

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[core] Make Saxon support multi valued XPath properties#736

[core] Make Saxon support multi valued XPath properties#736
adangel merged 5 commits into
pmd:masterfrom
oowekyala:fix-xpath-properties

oowekyala commented Nov 17, 2017

Uh oh!

oowekyala commented Nov 18, 2017

Uh oh!

adangel Nov 19, 2017

Uh oh!

oowekyala Nov 19, 2017

Uh oh!

adangel Nov 24, 2017

Uh oh!

adangel commented Nov 19, 2017

Uh oh!

oowekyala commented Nov 19, 2017

Uh oh!

oowekyala commented Nov 19, 2017

Uh oh!

oowekyala commented Nov 19, 2017 •

edited

Loading

Uh oh!

adangel commented Nov 24, 2017

Uh oh!

adangel commented Nov 27, 2017

Uh oh!

oowekyala commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

oowekyala commented Nov 17, 2017

Some possible improvements

Uh oh!

oowekyala commented Nov 18, 2017

Uh oh!

adangel Nov 19, 2017

Choose a reason for hiding this comment

Uh oh!

oowekyala Nov 19, 2017

Choose a reason for hiding this comment

Uh oh!

adangel Nov 24, 2017

Choose a reason for hiding this comment

Uh oh!

adangel commented Nov 19, 2017

Uh oh!

oowekyala commented Nov 19, 2017

Uh oh!

oowekyala commented Nov 19, 2017

Uh oh!

oowekyala commented Nov 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adangel commented Nov 24, 2017

Uh oh!

adangel commented Nov 27, 2017

Uh oh!

oowekyala commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oowekyala commented Nov 19, 2017 •

edited

Loading