Skip to content

Commit

Permalink
ast: Fix virtual predicate used for rule index build
Browse files Browse the repository at this point in the history
This commit fixes a bug in the predicate that checks if a ref refers
to a virtual doc during the rule index build.

Empty packages or packages containing only undefined rules generate an
empty object. The bug was causing refs to empty packages to not be
considered _virtual_ and therefore they could be indexed.

In the issue, the case that passes did so because the ref was
non-ground so the indexer excluded it. The case the failed was ground
and the ref referred to an empty package so the indexer included
it. This could be seen by enabling tracing (the index event showed
zero matching rules so the expression failed.)

The fix updates the virtual predicate to just walk down the rule
tree instead of accumulating rules. If at any point there are no rules
AND there are no children, the ref is not virtual. Otherwise, the ref
is virtual.

Fixes #1863

Signed-off-by: Torin Sandall <[email protected]>
  • Loading branch information
tsandall committed Oct 28, 2019
1 parent 25cd90d commit 86eebaf
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 1 deletion.
15 changes: 14 additions & 1 deletion ast/compile.go
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ func (c *Compiler) buildRuleIndices() {
return false
}
index := newBaseDocEqIndex(func(ref Ref) bool {
return len(c.GetRules(ref.GroundPrefix())) > 0
return isVirtual(c.RuleTree, ref.GroundPrefix())
})
if rules := extractRules(node.Values); index.Build(rules) {
c.ruleIndices.Put(rules[0].Path(), index)
Expand Down Expand Up @@ -3229,6 +3229,19 @@ func isDataRef(term *Term) bool {
return false
}

func isVirtual(node *TreeNode, ref Ref) bool {
for i := 0; i < len(ref); i++ {
child := node.Child(ref[i].Value)
if child == nil {
return false
} else if len(child.Values) > 0 {
return true
}
node = child
}
return true
}

func safetyErrorSlice(unsafe unsafeVars) (result Errors) {

if len(unsafe) == 0 {
Expand Down
4 changes: 4 additions & 0 deletions ast/compile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ s[2] { true }`,
if user.Hide {
t.Fatalf("Expected user.system node to be visible")
}

if !isVirtual(tree, MustParseRef("data.a.b.empty")) {
t.Fatal("Expected data.a.b.empty to be virtual")
}
}

func TestCompilerEmpty(t *testing.T) {
Expand Down
22 changes: 22 additions & 0 deletions topdown/topdown_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,28 @@ p[x] = y { data.enum_errors.a[x] = y }`,
assertTopDownWithPath(t, compiler, store, "enumerate virtual errors", []string{"enum_errors", "caller", "p"}, `{}`, fmt.Errorf("divide by zero"))
}

func TestTopDownFix1863(t *testing.T) {

compiler := ast.MustCompileModules(map[string]string{
"test1.rego": `
package a.b
# this module is empty
`,
"test2.rego": `
package x
p = data.a.b # p should be defined (an empty object)
`,
})

store := inmem.New()

assertTopDownWithPath(t, compiler, store, "is defined", []string{}, ``, `{"a": {"b": {}}, "x": {"p": {}}}`)
assertTopDownWithPath(t, compiler, store, "is defined", []string{"x"}, ``, `{"p": {}}`)
assertTopDownWithPath(t, compiler, store, "is defined", []string{"x", "p"}, ``, `{}`)
}

func TestTopDownNestedReferences(t *testing.T) {
tests := []struct {
note string
Expand Down

0 comments on commit 86eebaf

Please sign in to comment.