ewilliams28/releases
1
0
Fork 0
mirror of https://github.com/open-policy-agent/opa.git synced 2026-08-12 19:32:48 -06:00
Code Issues Packages Projects Releases Wiki Activity

re-do with conflicts: last one wins, in wasm and topdown (#3010)

Browse Source 
* compiler/wasm: use tee_local

It's the same as set_local followed by get_local: the target variable is set
to the value from the top of the stack, but that value is retained there.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

* compiler/wasm: make last 'with' statement win, update tests

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

* topdown/input: allow `with` overwrites that previously a conflict

This makes topdown behave more like WASM. Last `with` wins when in conflict:

    input with input.foo as "foo" with input.foo.bar as "baz"

will resolve to

    {"foo": {"bar": "baz"}}

now. Same with data references.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This commit is contained in:
Stephan Renatus
2020-12-18 13:27:52 +01:00
committed by GitHub
parent 499f018b1a
commit 06c3d7e4b6
14 changed files with 133 additions and 251 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ast/term.go
+3 -2
View File
@@ -1814,8 +1814,9 @@ func (obj *object) Iter(f func(*Term, *Term) error) error {
return nil
}
// Until calls f for each key-value pair in the object. If f returns true,
// iteration stops.
// Until calls f for each key-value pair in the object. If f returns
// true, iteration stops and Until returns true. Otherwise, return
// false.
func (obj *object) Until(f func(*Term, *Term) bool) bool {
err := obj.Iter(func(k, v *Term) error {
if f(k, v) {
internal/compiler/wasm/wasm.go
+14 -29
View File
@@ -179,7 +179,6 @@ const (
errVarAssignConflict int = iota
errObjectInsertConflict
errObjectMergeConflict
errWithConflict
errIllegalEntrypoint
)
@@ -190,7 +189,6 @@ var errorMessages = [...]struct {
{errVarAssignConflict, "var assignment conflict"},
{errObjectInsertConflict, "object insert conflict"},
{errObjectMergeConflict, "object merge conflict"},
{errWithConflict, "with target conflict"},
{errIllegalEntrypoint, "internal: illegal entrypoint id"},
}
@@ -672,8 +670,7 @@ func (c *Compiler) compileBlock(block *ir.Block) ([]instruction.Instruction, err
instrs = append(instrs, instruction.GetLocal{Index: c.local(stmt.Source)})
instrs = append(instrs, instruction.GetLocal{Index: c.local(stmt.Key)})
instrs = append(instrs, instruction.Call{Index: c.function(opaValueGet)})
instrs = append(instrs, instruction.SetLocal{Index: c.local(stmt.Target)})
instrs = append(instrs, instruction.GetLocal{Index: c.local(stmt.Target)})
instrs = append(instrs, instruction.TeeLocal{Index: c.local(stmt.Target)})
instrs = append(instrs, instruction.I32Eqz{})
instrs = append(instrs, instruction.BrIf{Index: 0})
case *ir.LenStmt:
@@ -799,8 +796,7 @@ func (c *Compiler) compileBlock(block *ir.Block) ([]instruction.Instruction, err
instruction.GetLocal{Index: c.local(stmt.Object)},
instruction.GetLocal{Index: c.local(stmt.Key)},
instruction.Call{Index: c.function(opaValueGet)},
instruction.SetLocal{Index: tmp},
instruction.GetLocal{Index: tmp},
instruction.TeeLocal{Index: tmp},
instruction.I32Eqz{},
instruction.BrIf{Index: 0},
instruction.GetLocal{Index: tmp},
@@ -867,8 +863,7 @@ func (c *Compiler) compileScanBlock(scan *ir.ScanStmt) ([]instruction.Instructio
instrs = append(instrs, instruction.Call{Index: c.function(opaValueIter)})
// Check for emptiness.
instrs = append(instrs, instruction.SetLocal{Index: c.local(scan.Key)})
instrs = append(instrs, instruction.GetLocal{Index: c.local(scan.Key)})
instrs = append(instrs, instruction.TeeLocal{Index: c.local(scan.Key)})
instrs = append(instrs, instruction.I32Eqz{})
instrs = append(instrs, instruction.BrIf{Index: 1})
@@ -971,14 +966,12 @@ func (c *Compiler) compileUpsert(local ir.Local, path []int, value ir.Local, loc
instruction.BrIf{Index: 0},
instruction.GetLocal{Index: lcopy},
instruction.Call{Index: c.function(opaValueShallowCopy)},
instruction.SetLocal{Index: lcopy},
instruction.GetLocal{Index: lcopy},
instruction.TeeLocal{Index: lcopy},
instruction.SetLocal{Index: c.local(local)},
instruction.Br{Index: 1},
}},
instruction.Call{Index: c.function(opaObject)},
instruction.SetLocal{Index: lcopy},
instruction.GetLocal{Index: lcopy},
instruction.TeeLocal{Index: lcopy},
instruction.SetLocal{Index: c.local(local)},
},
})
@@ -1012,17 +1005,12 @@ func (c *Compiler) compileUpsert(local ir.Local, path []int, value ir.Local, loc
inner = append(inner, instruction.I32Eqz{})
inner = append(inner, instruction.BrIf{Index: uint32(i)})
// If the next node is not an object, generate a conflict error.
inner = append(inner, instruction.Block{
Instrs: append([]instruction.Instruction{
instruction.GetLocal{Index: ltemp},
instruction.Call{Index: c.function(opaValueType)},
instruction.I32Const{Value: opaTypeObject},
instruction.I32Eq{},
instruction.BrIf{Index: 0},
},
c.runtimeErrorAbort(loc, errWithConflict)...),
})
// If the next node is not an object, break.
inner = append(inner, instruction.GetLocal{Index: ltemp})
inner = append(inner, instruction.Call{Index: c.function(opaValueType)})
inner = append(inner, instruction.I32Const{Value: opaTypeObject})
inner = append(inner, instruction.I32Ne{})
inner = append(inner, instruction.BrIf{Index: uint32(i)})
// Otherwise, shallow copy the next node node and insert into the copy
// before continuing.
@@ -1103,8 +1091,7 @@ func (c *Compiler) compileInternalCall(stmt *ir.CallStmt, index uint32, result *
block.Instrs = append(block.Instrs,
instruction.I32Const{Value: int32(index)},
instruction.Call{Index: c.function(opaMemoizeGet)},
instruction.SetLocal{Index: c.local(stmt.Result)},
instruction.GetLocal{Index: c.local(stmt.Result)},
instruction.TeeLocal{Index: c.local(stmt.Result)},
instruction.BrIf{Index: 0})
}
@@ -1115,8 +1102,7 @@ func (c *Compiler) compileInternalCall(stmt *ir.CallStmt, index uint32, result *
block.Instrs = append(block.Instrs,
instruction.Call{Index: index},
instruction.SetLocal{Index: c.local(stmt.Result)},
instruction.GetLocal{Index: c.local(stmt.Result)},
instruction.TeeLocal{Index: c.local(stmt.Result)},
instruction.I32Eqz{},
instruction.BrIf{Index: 1})
@@ -1149,8 +1135,7 @@ func (c *Compiler) compileExternalCall(stmt *ir.CallStmt, id int32, result *[]in
}
instrs = append(instrs, instruction.Call{Index: c.funcs[builtinDispatchers[len(stmt.Args)]]})
instrs = append(instrs, instruction.SetLocal{Index: c.local(stmt.Result)})
instrs = append(instrs, instruction.GetLocal{Index: c.local(stmt.Result)})
instrs = append(instrs, instruction.TeeLocal{Index: c.local(stmt.Result)})
instrs = append(instrs, instruction.I32Eqz{})
instrs = append(instrs, instruction.BrIf{Index: 0})
*result = instrs
internal/planner/planner.go
+2 -4
View File
@@ -620,14 +620,12 @@ func (p *Planner) planWithRec(e *ast.Expr, targets [][]int, values []ir.Local, i
target := e.With[index].Target.Value.(ast.Ref)
head := target[0].Value.(ast.Var)
stmt := &ir.WithStmt{
p.appendStmt(&ir.WithStmt{
Local: p.vars.GetOrEmpty(head),
Path: targets[index],
Value: values[index],
Block: block,
}
p.appendStmt(stmt)
})
return nil
}
internal/wasm/instruction/variable.go
+16
View File
@@ -36,3 +36,19 @@ func (SetLocal) Op() opcode.Opcode {
func (i SetLocal) ImmediateArgs() []interface{} {
return []interface{}{i.Index}
}
// TeeLocal represents the WASM tee_local instruction.
type TeeLocal struct {
Index uint32
}
// Op returns the opcode of the instruction.
func (TeeLocal) Op() opcode.Opcode {
return opcode.TeeLocal
}
// ImmediateArgs returns the index of the local variable to "tee" with the top of
// the stack (like set, but retaining the top of the stack).
func (i TeeLocal) ImmediateArgs() []interface{} {
return []interface{}{i.Index}
}
internal/wasm/sdk/opa/opa_test.go
+2 -15
View File
@@ -129,19 +129,6 @@ a = "c" { input > 2 }`,
Evals: []Eval{{}},
WantErr: "<query>:1:1: object merge conflict: internal error",
},
{
Description: "Runtime error/with target conflict in policy",
Policy: `a = x { input = x with input.foo as 1 with input.foo.bar as 2 }`,
Query: "data.p = x",
Evals: []Eval{{}},
WantErr: "module.rego:2:9: with target conflict: internal error",
},
{
Description: "Runtime error/with target conflict in query",
Query: "input = x with input.foo as 1 with input.foo.bar as 2",
Evals: []Eval{{}},
WantErr: "<query>:1:1: with target conflict: internal error",
},
// NOTE(sr): The next two test cases were used to replicate issue
// https://github.com/open-policy-agent/opa/issues/2962 -- their raison d'être
// is thus questionable, but it might be good to keep them around a bit.
@@ -173,14 +160,14 @@ a = "c" { input > 2 }`,
},
{
Description: "regex.match with pattern from input",
Query: `x = regex.match(input.re, "foo")`,
Query: `x = regex.match(input.re, "foo")`,
Evals: []Eval{
Eval{Input: `{"re": "^foo$"}`, Result: `{{"x": true}}`},
},
},
{
Description: "regex.find_all_string_submatch_n with pattern from input",
Query: `x = regex.find_all_string_submatch_n(input.re, "-axxxbyc-", -1)`,
Query: `x = regex.find_all_string_submatch_n(input.re, "-axxxbyc-", -1)`,
Evals: []Eval{
Eval{Input: `{"re": "a(x*)b(y|z)c"}`, Result: `{{"x":[["axxxbyc","xxx","y"]]}}`},
},
internal/wasm/sdk/test/e2e/exceptions.yaml
-7
View File
@@ -1,12 +1,5 @@
# Exception Format is <test name>: <reason>
"baseandvirtualdocs/base/virtual: conflicts": "document merge conflict - https://github.com/open-policy-agent/opa/issues/2926"
"withkeyword/with virtual doc specific index": "with target conflict issue - https://github.com/open-policy-agent/opa/issues/2922"
"withkeyword/with virtual doc not specific index": "with target conflict issue - https://github.com/open-policy-agent/opa/issues/2922"
"withkeyword/with virtual doc exact value": "with target conflict issue - https://github.com/open-policy-agent/opa/issues/2922"
"withkeyword/with virtual doc any index": "with target conflict issue - https://github.com/open-policy-agent/opa/issues/2922"
"withkeyword/with base doc exact value": "with target conflict issue - https://github.com/open-policy-agent/opa/issues/2922"
"withkeyword/with base doc any index": "with target conflict issue - https://github.com/open-policy-agent/opa/issues/2922"
"withkeyword/undefined_1": "with target conflict issue - https://github.com/open-policy-agent/opa/issues/2922"
"jsonpatch/set": "unexpected panic or evaluation error - https://github.com/open-policy-agent/opa/issues/2949"
"jsonpatch/json_patch_tests": "unexpected panic or evaluation error - https://github.com/open-policy-agent/opa/issues/2949"
"arithmetic/remainder+error+floating": "expected error missing - https://github.com/open-policy-agent/opa/issues/2954"
test/cases/testdata/withkeyword/test-withkeyword-1019.yaml
Vendored
+8 -88
View File
@@ -1,102 +1,22 @@
cases:
- data:
a:
- 1
- 2
- 3
- 4
b:
v1: hello
v2: goodbye
c:
- x:
- true
- false
- foo
"y":
- null
- 3.14159
z:
p: true
q: false
d:
e:
- bar
- baz
f:
- xs:
- 1
ys:
- 2
- xs:
- 2
ys:
- 3
g:
a:
- 1
- 0
- 0
- 0
b:
- 0
- 2
- 0
- 0
c:
- 0
- 0
- 0
- 4
h:
- - 1
- 2
- 3
- - 2
- 3
- 4
l:
- a: bob
b: -1
c:
- 1
- 2
- 3
- 4
- a: alice
b: 1
c:
- 2
- 3
- 4
- 5
d: null
m: []
numbers:
- "1"
- "2"
- "3"
- "4"
strings:
bar: 2
baz: 3
foo: 1
three: 3
modules:
- |
package generated
p {
data.ex.loopback with input.foo as "x" with input.foo.bar as "y"
p = x {
x := data.ex.loopback with input.foo as "x" with input.foo.bar as "y"
}
- |
package ex
loopback = __local0__ {
loopback = y {
true
__local0__ = input
y = input
}
note: withkeyword/with conflict
query: data.generated.p = x
want_error: conflicting documents
want_error_code: eval_conflict_error
want_result:
- x:
foo:
bar: "y"
test/cases/testdata/withkeyword/test-withkeyword-1035.yaml
Vendored
+4 -84
View File
@@ -1,91 +1,11 @@
cases:
- data:
a:
- 1
- 2
- 3
- 4
b:
v1: hello
v2: goodbye
c:
- x:
- true
- false
- foo
"y":
- null
- 3.14159
z:
p: true
q: false
d:
e:
- bar
- baz
f:
- xs:
- 1
ys:
- 2
- xs:
- 2
ys:
- 3
g:
a:
- 1
- 0
- 0
- 0
b:
- 0
- 2
- 0
- 0
c:
- 0
- 0
- 0
- 4
h:
- - 1
- 2
- 3
- - 2
- 3
- 4
l:
- a: bob
b: -1
c:
- 1
- 2
- 3
- 4
- a: alice
b: 1
c:
- 2
- 3
- 4
- 5
d: null
m: []
numbers:
- "1"
- "2"
- "3"
- "4"
strings:
bar: 2
baz: 3
foo: 1
three: 3
a: testdata
modules:
- |
package generated
default p = false
p {
data.ex.allow_basic = true with data.a.b as 5
}
@@ -97,5 +17,5 @@ cases:
}
note: withkeyword/with data conflict
query: data.generated.p = x
want_error: real and replacement data could not be merged
want_error_code: eval_with_merge_error
want_result:
- x: false
test/wasm/assets/016_with.yaml
+4 -1
View File
@@ -229,7 +229,10 @@ cases:
- note: with conflict
query: |
input = x with input.foo as 1 with input.foo.bar as 2
want_error: "<query>:1:1: with target conflict"
want_result:
- x:
foo:
bar: 2
- note: with virtual doc iteration
query: |
x := data[i][j] with data.bar.p as 3 with data.bar.q as 4; y = data.bar.p; z = data.bar.q
topdown/eval.go
+3 -1
View File
@@ -2781,7 +2781,9 @@ func merge(a, b ast.Value) (ast.Value, bool) {
if ok1 && ok2 {
return mergeObjects(aObj, bObj)
}
return nil, false
// nothing to merge, a wins
return a, true
}
// mergeObjects returns a new Object containing the non-overlapping keys of
topdown/eval_test.go
+46 -6
View File
@@ -64,13 +64,53 @@ func TestMergeOverlappingKeys(t *testing.T) {
}
func TestMergeError(t *testing.T) {
realData := ast.MustParseTerm(`{"foo": "bar"}`).Value.(ast.Object)
mockData := ast.StringTerm("baz").Value
func TestMergeWhenHittingNonObject(t *testing.T) {
cases := []struct {
note string
real, mock, exp *ast.Term
}{
{
note: "real object, mock string",
real: ast.MustParseTerm(`{"foo": "bar"}`),
mock: ast.StringTerm("foo"),
exp: ast.StringTerm("foo"),
},
{
note: "real string, mock object",
real: ast.StringTerm("foo"),
mock: ast.MustParseTerm(`{"foo": "bar"}`),
exp: ast.MustParseTerm(`{"foo": "bar"}`),
},
{
note: "real object with string value, where mock has object-value",
real: ast.MustParseTerm(`{"foo": ["bar"], "quz": false}`),
mock: ast.MustParseTerm(`{"foo": {"bar": 123}}`),
exp: ast.MustParseTerm(`{"foo": {"bar": 123}, "quz": false}`),
},
{
note: "real object with deeply-nested object value, where mock has number-value",
real: ast.MustParseTerm(`{"foo": {"bar": {"baz": "quz"}, "quz": true}}`),
mock: ast.MustParseTerm(`{"foo": {"bar": 10}}`),
exp: ast.MustParseTerm(`{"foo": {"bar": 10, "quz": true}}`),
},
{
note: "real object with deeply-nested string value, where mock has object-value",
real: ast.MustParseTerm(`{"foo": {"bar": {"baz": "quz"}, "quz": true}}`),
mock: ast.MustParseTerm(`{"foo": {"bar": {"baz": {"foo": "bar"}}}}`),
exp: ast.MustParseTerm(`{"foo": {"bar": {"baz": {"foo": "bar"}}, "quz": true}}`),
},
}
_, ok := merge(mockData, realData)
if ok {
t.Fatal("Expected error")
for _, tc := range cases {
t.Run(tc.note, func(t *testing.T) {
merged, ok := merge(tc.mock.Value, tc.real.Value)
if !ok {
t.Fatal("expected no error")
}
if tc.exp.Value.Compare(merged) != 0 {
t.Errorf("Expected %v but got %v", tc.exp, merged)
}
})
}
}
topdown/exported_test.go
+4
View File
@@ -83,6 +83,10 @@ func testRun(t *testing.T, tc cases.TestCase) {
testAssertErrorCode(t, *tc.WantErrorCode, err)
}
if err != nil && tc.WantErrorCode == nil && tc.WantError == nil {
t.Fatalf("unexpected error: %v", err)
}
if tc.WantResult != nil {
testAssertResultSet(t, *tc.WantResult, rs, tc.SortBindings)
}
topdown/input.go
+20 -12
View File
@@ -10,7 +10,6 @@ import (
"github.com/open-policy-agent/opa/ast"
)
var errConflictingDoc = fmt.Errorf("conflicting documents")
var errBadPath = fmt.Errorf("bad document path")
func mergeTermWithValues(exist *ast.Term, pairs [][2]*ast.Term) (*ast.Term, error) {
@@ -43,30 +42,31 @@ func mergeTermWithValues(exist *ast.Term, pairs [][2]*ast.Term) (*ast.Term, erro
result = exist.Copy()
init = true
}
if result == nil {
result = ast.NewTerm(makeTree(target[1:], pair[1]))
} else {
node := result
done := false
for i := 1; i < len(target)-1 && !done; i++ {
if child := node.Get(target[i]); child == nil {
obj, ok := node.Value.(ast.Object)
if !ok {
return nil, errConflictingDoc
}
obj, ok := node.Value.(ast.Object)
if !ok {
result = ast.NewTerm(makeTree(target[i:], pair[1]))
done = true
continue
}
if child := obj.Get(target[i]); !isObject(child) {
obj.Insert(target[i], ast.NewTerm(makeTree(target[i+1:], pair[1])))
done = true
} else {
} else { // child is object
node = child
}
}
if !done {
obj, ok := node.Value.(ast.Object)
if !ok {
return nil, errConflictingDoc
if obj, ok := node.Value.(ast.Object); ok {
obj.Insert(target[len(target)-1], pair[1])
} else {
result = ast.NewTerm(makeTree(target[len(target)-1:], pair[1]))
}
obj.Insert(target[len(target)-1], pair[1])
}
}
}
@@ -90,3 +90,11 @@ func makeTree(k ast.Ref, v *ast.Term) ast.Object {
obj = ast.NewObject(ast.Item(k[0], v))
return obj
}
func isObject(x *ast.Term) bool {
if x == nil {
return false
}
_, ok := x.Value.(ast.Object)
return ok
}
topdown/input_test.go
+7 -2
View File
@@ -45,13 +45,18 @@ func TestMergeTermWithValues(t *testing.T) {
},
{
note: "conflicting value",
input: [][2]string{{"input", "[1,2,3]"}, {"input.a", "true"}},
expected: `{"a": true}`,
},
{
note: "conflicting value, nested trailing terms",
input: [][2]string{{"input", "[1,2,3]"}, {"input.a.b", "true"}},
expected: errConflictingDoc,
expected: `{"a": {"b": true}}`,
},
{
note: "conflicting merge",
input: [][2]string{{`input.a.b`, `"c"`}, {`input.a.b.d`, `"d"`}},
expected: errConflictingDoc,
expected: `{"a": {"b": {"d": "d"}}}`,
},
{
note: "ordered roots",