From 86eebafa8924156576f8d84bf2622b08313172d8 Mon Sep 17 00:00:00 2001
From: Torin Sandall <torinsandall@gmail.com>
Date: Mon, 28 Oct 2019 11:03:58 -0400
Subject: [PATCH] ast: Fix virtual predicate used for rule index build

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 <torinsandall@gmail.com>
---
 ast/compile.go          | 15 ++++++++++++++-
 ast/compile_test.go     |  4 ++++
 topdown/topdown_test.go | 22 ++++++++++++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/ast/compile.go b/ast/compile.go
index 4c268dd15a..c51951c383 100644
--- a/ast/compile.go
+++ b/ast/compile.go
@@ -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)
@@ -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 {
diff --git a/ast/compile_test.go b/ast/compile_test.go
index 72050d5808..98649278f8 100644
--- a/ast/compile_test.go
+++ b/ast/compile_test.go
@@ -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) {
diff --git a/topdown/topdown_test.go b/topdown/topdown_test.go
index f41a059262..011fc91245 100644
--- a/topdown/topdown_test.go
+++ b/topdown/topdown_test.go
@@ -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