Fix indexed evaluation bug

Term evaluation was not plugging the expression before checking if the indexer
could be used. As a result, the index was being built on the unplugged version
of the expression which resulted in false positives being returned by the
indexer. Because the evaluation expects the indexer to provide exact matches,
the query evaluation was yielding false positives.

Also, made a slight tweak to the docs to make the example slightly clearer.

docs/CONCEPTS.md

@@ -216,7 +216,7 @@ Content-Type: application/json
 
 {
   "servers": [
-    {"id": "s1", "name": "app", "protocols": ["http", "https", "ssh"], "ports": ["p1", "p2", "p3"]},
+    {"id": "s1", "name": "app", "protocols": ["https", "ssh"], "ports": ["p1", "p2", "p3"]},
     {"id": "s2", "name": "db", "protocols": ["mysql"], "ports": ["p3"]},
     {"id": "s3", "name": "cache", "protocols": ["memcache", "http"], "ports": ["p3"]},
     {"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p1", "p2"]}
@@ -237,7 +237,7 @@ Content-Type: application/json
 We can write a rule which enumerates servers that expose HTTP (but not HTTPS) and are connected to public networks. These represent violations of policy.
 
 ```rego
-package opa.examples                            # this policy belongs the opa.examples package
+package opa.example                            # this policy belongs the opa.example package
 
 import data.servers                             # import the data.servers document to refer to it as "servers" instead of "data.servers"
 import data.networks                            # same but for data.networks
@@ -261,9 +261,9 @@ The key aspects of Rego are illustrated by this example:
 
 - Rules consist of assertions against data stored in OPA. In this case the assertions are expressions which test for equality and membership of `servers`, `networks`, and `ports` documents.
 
-- Expressions can referenced nested documents, e.g., `ports[i].networks[]` references the network IDs stored in an array on each port document.
+- Expressions can referenced nested documents, e.g., `ports[i].networks[_]` references the network IDs stored in an array on each port document.
 
-- Expressions can reference elements in a collection using the `[]` and `[<variable>]` syntax. When this is done, OPA knows to iterate over elements of the collection when processing queries.
+- Expressions can reference elements in a collection using the `[_]` and `[<variable>]` syntax. When this is done, OPA knows to iterate over elements of the collection when processing queries.
 
 If we query for the document produced by this rule, we receive an array of servers that expose an HTTP server and are connected to a public network:
 
@@ -278,7 +278,6 @@ HTTP/1.1 200 OK
 Content-Type: application/json
 
 [
-  {"id": "s1", "name": "app-server", "protocols": ["http", "https", "ssh"], "ports": ["p1", "p2", "p3"]},
   {"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p2"]}
 ]
 ```

eval/topdown.go

@@ -956,23 +956,18 @@ func evalRefRuleResult(ctx *TopDownContext, ref ast.Ref, suffix ast.Ref, result
 // variables used in references involves iterating collections in storage or
 // evaluating rules identified by the references. In either case, this function
 // will invoke the iterator with each set of bindings that should be evaluated.
-//
-// TODO(tsandall): extend to support indexing.
 func evalTerms(ctx *TopDownContext, iter TopDownIterator) error {
 
 	expr := ctx.Current()
-	var ts []*ast.Term
-	switch t := expr.Terms.(type) {
-	case []*ast.Term:
-		ts = t
-	case *ast.Term:
-		ts = append(ts, t)
-	default:
-		panic(fmt.Sprintf("illegal argument: %v", t))
-	}
 
-	if indexAvailable(ctx, ts) {
+	// Check if indexing is available for this expression. Need to
+	// perform check on the plugged version of the expression, otherwise
+	// the index will return false positives.
+	plugged := plugExpr(expr, ctx.Bindings)
 
+	if indexAvailable(ctx, plugged) {
+
+		ts := plugged.Terms.([]*ast.Term)
 		ref, isRef := ts[1].Value.(ast.Ref)
 
 		if isRef {
@@ -998,6 +993,16 @@ func evalTerms(ctx *TopDownContext, iter TopDownIterator) error {
 		}
 	}
 
+	var ts []*ast.Term
+	switch t := expr.Terms.(type) {
+	case []*ast.Term:
+		ts = t
+	case *ast.Term:
+		ts = append(ts, t)
+	default:
+		panic(fmt.Sprintf("illegal argument: %v", t))
+	}
+
 	return evalTermsRec(ctx, iter, ts)
 }
 
@@ -1127,25 +1132,30 @@ func evalTermsRecObject(ctx *TopDownContext, obj ast.Object, idx int, iter TopDo
 // Indexing is used on equality expressions where both sides are non-ground refs (to base docs) or one
 // side is a non-ground ref (to a base doc) and the other side is any ground term. In the future, indexing
 // may be used on references embedded inside array/object values.
-func indexAvailable(ctx *TopDownContext, terms []*ast.Term) bool {
+func indexAvailable(ctx *TopDownContext, expr *ast.Expr) bool {
 
-	// Indexing can only be used when evaluating equality expressions.
-	if !terms[0].Value.Equal(equalityBuiltin) {
+	ts, ok := expr.Terms.([]*ast.Term)
+	if !ok {
 		return false
 	}
 
-	pluggedA := plugTerm(terms[1], ctx.Bindings)
-	pluggedB := plugTerm(terms[2], ctx.Bindings)
-
-	_, isRefA := pluggedA.Value.(ast.Ref)
-	_, isRefB := pluggedB.Value.(ast.Ref)
-
-	if isRefA && !pluggedA.IsGround() {
-		return pluggedB.IsGround() || isRefB
+	// Indexing can only be used when evaluating equality expressions.
+	if !ts[0].Value.Equal(equalityBuiltin) {
+		return false
 	}
 
-	if isRefB && !pluggedB.IsGround() {
-		return pluggedA.IsGround() || isRefA
+	a := ts[1].Value
+	b := ts[2].Value
+
+	_, isRefA := a.(ast.Ref)
+	_, isRefB := b.(ast.Ref)
+
+	if isRefA && !a.IsGround() {
+		return b.IsGround() || isRefB
+	}
+
+	if isRefB && !b.IsGround() {
+		return a.IsGround() || isRefA
 	}
 
 	return false

eval/topdown_test.go

@@ -533,6 +533,75 @@ func TestTopDownEmbeddedVirtualDoc(t *testing.T) {
 	assertTopDown(t, store, 0, "deep embedded vdoc", []string{"b", "c", "d", "p"}, "[1, 2, 4]")
 }
 
+func TestExample(t *testing.T) {
+
+	bd := `
+		{
+			"servers": [
+				{"id": "s1", "name": "app", "protocols": ["https", "ssh"], "ports": ["p1", "p2", "p3"]},
+				{"id": "s2", "name": "db", "protocols": ["mysql"], "ports": ["p3"]},
+				{"id": "s3", "name": "cache", "protocols": ["memcache", "http"], "ports": ["p3"]},
+				{"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p1", "p2"]}
+			],
+			"networks": [
+				{"id": "n1", "public": false},
+				{"id": "n2", "public": false},
+				{"id": "n3", "public": true}
+			],
+			"ports": [
+				{"id": "p1", "networks": ["n1"]},
+				{"id": "p2", "networks": ["n3"]},
+				{"id": "p3", "networks": ["n2"]}
+			]
+		}
+	`
+
+	vd := `
+		package opa.example
+
+		import data.servers
+		import data.networks
+		import data.ports
+
+		public_servers[server] :-
+			server = servers[i],
+			server.ports[j] = ports[k].id,
+			ports[k].networks[l] = networks[m].id,
+			networks[m].public = true
+
+		violations[server] :-
+			server = servers[i],
+			server.protocols[j] = "http",
+			public_servers[server]
+	`
+
+	var doc map[string]interface{}
+
+	if err := json.Unmarshal([]byte(bd), &doc); err != nil {
+		panic(err)
+	}
+
+	mods := compileModules([]string{vd})
+
+	store, err := NewStorage([]map[string]interface{}{doc}, mods)
+	if err != nil {
+		panic(err)
+	}
+
+	assertTopDown(t, store, 0, "public servers", []string{"opa", "example", "public_servers"}, `
+		[
+			{"id": "s1", "name": "app", "protocols": ["https", "ssh"], "ports": ["p1", "p2", "p3"]},
+			{"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p1", "p2"]}
+		]
+	`)
+
+	assertTopDown(t, store, 0, "violations", []string{"opa", "example", "violations"}, `
+		[
+			{"id": "s4", "name": "dev", "protocols": ["http"], "ports": ["p1", "p2"]}
+		]
+	`)
+}
+
 func compileModules(input []string) []*ast.Module {
 
 	mods := []*ast.Module{}