Skip to content

Commit

Permalink
Remove incorrect early exit from ref resolver
Browse files Browse the repository at this point in the history
In 986d82f we added support for applying the with keyword to the data
document. In the implementation, the ref resolver had to be updated to
check an additional cache. Those changes contained a bug that caused the
ref resolver to exit early and indicate that the ref was undefined if
the cache did not contain a value for the ref (partial or complete).

If the with cache does not contain a value for the ref, the value from
storage ought to be used. These changes just remove the early
exit--which is safe since the rest of the code checks if the value
returned by the with cache is non-nil (and handles the nil case
accordingly).

Fixes #1110

Signed-off-by: Torin Sandall <torinsandall@gmail.com>
  • Loading branch information
tsandall committed Dec 18, 2018
1 parent 4c77f5f commit fa19e5e
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 11 deletions.
8 changes: 0 additions & 8 deletions topdown/eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -979,22 +979,14 @@ func (e *eval) Resolve(ref ast.Ref) (ast.Value, error) {

if ref[0].Equal(ast.DefaultRootDocument) {

// Check cache with replacement data
repValue, ok := e.withCache.Get(ref)
if ok {
return repValue, nil
}

expr := e.query[e.index]
if len(expr.With) > 0 && repValue == nil {
return nil, nil
}

var merged ast.Value
var err error

// Check cache with real data

// Converting large JSON values into AST values can be fairly expensive. For
// example, a 2MB JSON value can take upwards of 30 millisceonds to convert.
// We cache the result of conversion here in case the same base document is
Expand Down
6 changes: 3 additions & 3 deletions topdown/topdown_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"sort"
"strings"
"testing"

"time"

"github.com/open-policy-agent/opa/ast"
Expand Down Expand Up @@ -2588,6 +2587,7 @@ loopback = input { true }
composite[x] { input.foo[_] = x; x > 2 }
vars = {"foo": input.foo, "bar": input.bar} { true }
input_eq { input.x = 1 }
data_eq { data.a = x }
allow_basic = true {data.a = "testdata"}
allow_merge_1 = true {data.b = {"v1": "hello", "v2": "world"}}
allow_merge_2 = true {data.b = {"v1": "hello", "v2": "world", "v3": "again"}}
Expand Down Expand Up @@ -2645,7 +2645,7 @@ test_mock_var = y {y = ex.mock_var with ex.mock_var as {"c": 1, "d": 2}}
test_mock_rule {ex.mock_rule with ex.mock_rule as true}
test_rule_chain {ex.allow with data.label.b.c as [1,2,3]}
only_with_data_no_with_input { ex.input_eq with data.foo as 1 }
`,
only_with_input_no_with_data { ex.data_eq with input as {} }`,
})

store := inmem.NewFromObject(loadSmallTestData())
Expand All @@ -2672,7 +2672,7 @@ only_with_data_no_with_input { ex.input_eq with data.foo as 1 }
assertTopDownWithPath(t, compiler, store, "with mock rule", []string{"test", "test_mock_rule"}, "", "true")
assertTopDownWithPath(t, compiler, store, "with rule chain", []string{"test", "test_rule_chain"}, "", "true")
assertTopDownWithPath(t, compiler, store, "bug 1083", []string{"test", "only_with_data_no_with_input"}, "", "")

assertTopDownWithPath(t, compiler, store, "bug 1100", []string{"test", "only_with_input_no_with_data"}, "", "true")
}

func TestTopDownElseKeyword(t *testing.T) {
Expand Down

0 comments on commit fa19e5e

Please sign in to comment.