Conversation
A predicate chained onto a parenthesized node test is ignored, so the
expression returns whatever the first predicate selected as though the second
predicate had not been written.
(//employee)[1][false()] 1 node, want 0
(//employee)[2][false()] 1 node, want 0
(//employee)[1][@id = "2"] employee 1, want 0
The third case is the consequential one: a genuine filter is discarded, so the
expression returns the wrong node rather than no node. A caller that chains a
predicate to confirm it selected the node it intended gets no confirmation and
no error.
The un-parenthesized spelling is already covered by TestPredicates
(/empinfo/employee[1][@id=1]) and passes, and a parenthesized node test with a
single predicate is covered by TestPositions and also passes, so only the
combination of the two was untested.
Three cases whose predicate the selected node does satisfy are included so the
test cannot be satisfied by dropping the node unconditionally.
FilterExpr is left-recursive, so a filter expression can carry any number of
predicates. parseFilterExpr consumed at most one, silently discarding the rest:
(//employee)[1][false()] parsed to the same AST as (//employee)[1], and the
second predicate never reached the query builder.
The visible effect is that a chained predicate on a parenthesized node test is
ignored. For an impossible predicate the expression returns a node where it
should return none, and for a real filter it returns the wrong node, which is
worse because there is no error either way.
parseStep already loops for exactly this reason. Matching it here is the whole
fix; nothing in build.go needed to change, since the builder handled the nested
filterNode correctly once the parser produced it.
Verified against xmllint, which agrees on all of:
(//table)[1][false()] 0
(//table)[1][@Class="u"] 0
(//table)[1][@Class="t"] 1
(//table)[2][@Class="u"] 1
(//table)[1][true()][@Class="u"] 0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
parseFilterExprusesifwhere the left-recursive grammar in its own commentneeds a loop, so every predicate after the first is silently discarded.
(//a)[1][b]parses to the same AST as(//a)[1].with
<root><table class="t"/><table class="u"/></root>:(//table)[1][false()](//table)[1][@class="u"]class="t"nodeThe second row is the one that bites: a real filter is dropped, so the result is
the wrong node rather than no node, with no error.
parseStepalready loops;the un-parenthesized
//table[1][@class="u"]goes through it and is correct today.Nothing in
build.gochanges. The builder already handled the nestedfilterNode; it never received one.TestChainedPredicateOnGroupedExpressionasserts three cases where the predicatemust eliminate the node and three where it must keep one, so it cannot be
satisfied by dropping nodes unconditionally. The two halves that already work were
each covered before (
TestPredicateshas/empinfo/employee[1][@id=1],TestPositionshas(//book[@category = "web"])[2]) only the combination wasuntested.
This changes results for any
(...)[a][b]expression that previously ignoredeverything after
[a]. The old result was incorrect, so this is a fix rather thana regression; the existing suite passes unchanged.