Skip to content

Commit

Permalink
fix(filtering): set expr.ID on nested AND expressions
Browse files Browse the repository at this point in the history
Previously nested AND expressions (ex: "a AND b AND c") would not set an
ID on the innermost AND expression. This commit changes the "folding" of
a sequence of AND expressions to be similar to OR expressions.

Also adds a test to ensure that all tested expressions produce
subexpressions with unique IDs.

This fixes an issue where type checking fails because of duplicate IDs.
  • Loading branch information
ericwenn committed Mar 23, 2021
1 parent 30b758f commit 4b73996
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 3 deletions.
7 changes: 4 additions & 3 deletions filtering/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,11 @@ func (p *Parser) ParseExpression() (_ *expr.Expr, err error) {
break
}
}
if len(sequences) == 1 {
return sequences[0], nil
exp := sequences[0]
for _, seq := range sequences[1:] {
exp = parsedExpression(p.nextID(start), exp, seq)
}
return parsedExpression(p.nextID(start), sequences...), nil
return exp, nil
}

// ParseSequence parses a Sequence.
Expand Down
21 changes: 21 additions & 0 deletions filtering/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ func TestParser(t *testing.T) {
GreaterEquals(Text("a"), Int(100)),
),
},
{
filter: "a OR b OR c",
expected: Or(
Text("a"),
Text("b"),
Text("c"),
),
},

{
filter: "NOT (a OR b)",
Expand Down Expand Up @@ -251,7 +259,20 @@ func TestParser(t *testing.T) {
protocmp.Transform(),
protocmp.IgnoreFields(&expr.Expr{}, "id"),
)
assertUniqueExprIDs(t, actual.Expr)
}
})
}
}

func assertUniqueExprIDs(t *testing.T, exp *expr.Expr) {
t.Helper()
seenIDs := make(map[int64]struct{})
Walk(func(currExpr, parentExpr *expr.Expr) bool {
if _, ok := seenIDs[currExpr.Id]; ok {
t.Fatalf("duplicate expression ID '%d' for expr %v", currExpr.Id, currExpr)
}
seenIDs[currExpr.Id] = struct{}{}
return true
}, exp)
}

0 comments on commit 4b73996

Please sign in to comment.