OAK-12364: XPath queries fail to parse if they contain invalid XML characters - #3084
OAK-12364: XPath queries fail to parse if they contain invalid XML characters#3084ChlineSaurus wants to merge 2 commits into
Conversation
b23bd17 to
976189a
Compare
| } else { | ||
| if (Character.isJavaIdentifierPart(c)) { | ||
| type = CHAR_NAME; | ||
| } else if ((settings == null || settings.isXmlNameCharsInPathEnabled()) && XMLChar.isName(c)) { |
There was a problem hiding this comment.
Shouldn't we not enable this if settings is null (to keep the existing behavior in that case) ?
There was a problem hiding this comment.
I was not sure about this. I also had in mind that we decided that we want to keep the old behavior as the default behavior for Oak changes, but in the Agents.md we write that in case something is a bug fix, default should be on, and only new features should be default off.
Further, it's not a very risky change, therefore I went with default enabled, but if we want to be more defensive I can change it. 😌
There was a problem hiding this comment.
Is there a case where the new behavior would break something that previously "worked" (that is, didn't throw an exception)?
There was a problem hiding this comment.
There is one case I found, but I'm not sure how realistic it is. The query so far interpreted XML characters as whitespaces. If this is in the middle, it failed, as described in the Issue. However, if the XML character is directly next to a separator, it is ignored, meaning the new behavior can change queries behavior for such queries instead of failing (See test below for an example.) Not sure the feature toggle is too defensive, but better safe than sorry.
@Test
public void testXmlNameCharsInPathFeatureChangesBehavior() throws ParseException {
// a middle dot (\u00b7) attached to a name is dropped when the feature is disabled,
// but kept as part of the name when enabled, resulting in a different query
String xpath = "//*[@a\u00b7 = 1]";
QueryEngineSettings disabled = new QueryEngineSettings();
disabled.setXmlNameCharsInPathFeature(createFeature(false));
String withoutFeature = new XPathToSQL2Converter(disabled).convert(xpath);
QueryEngineSettings enabled = new QueryEngineSettings();
enabled.setXmlNameCharsInPathFeature(createFeature(true));
String withFeature = new XPathToSQL2Converter(enabled).convert(xpath);
assertTrue(withoutFeature.contains("[a] = 1"));
assertTrue(withFeature.contains("[a\u00b7] = 1"));
assertNotEquals(withoutFeature, withFeature);
}
|
|
||
| @Test | ||
| public void testXmlNameCharsInPathAreQueryable() throws Exception { | ||
| root.getTree("/").addChild("content").addChild("m·d").addChild("child"); |
There was a problem hiding this comment.
readabilty: use \u notation here,
There was a problem hiding this comment.
Updated 😌
Btw. if I should first merge against an Oak branch, I'm happy to do so, but I can't create branches on my own.
3 participants
XPath queries failed to parse when a path contained certain valid characters that XML allows in names but Java does not (such as the middle dot ·), because the parser treated them as spaces and split the path into two pieces, leaving a leftover token it couldn't make sense of. This fix changes the parser to accept those XML name characters, such that those paths can be queried.
The bug fix is very simple, but in a critical path of the code (XPath to SQL converter). Therefore, I decided to put the fix behind a feature toggle (but since it's a bug fix I have it on by default, as documented in Agents.md).
Issue: https://issues.apache.org/jira/browse/OAK-12364