From 06c3d7e4b659a64a27d0d2678481d0fbe3c0dd49 Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Fri, 18 Dec 2020 13:27:52 +0100 Subject: [PATCH] re-do `with` conflicts: last one wins, in wasm and topdown (#3010) * 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 * compiler/wasm: make last 'with' statement win, update tests Signed-off-by: Stephan Renatus * 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 --- ast/term.go | 5 +- internal/compiler/wasm/wasm.go | 43 +++------ internal/planner/planner.go | 6 +- internal/wasm/instruction/variable.go | 16 ++++ internal/wasm/sdk/opa/opa_test.go | 17 +--- internal/wasm/sdk/test/e2e/exceptions.yaml | 7 -- .../withkeyword/test-withkeyword-1019.yaml | 96 ++----------------- .../withkeyword/test-withkeyword-1035.yaml | 88 +---------------- test/wasm/assets/016_with.yaml | 5 +- topdown/eval.go | 4 +- topdown/eval_test.go | 52 ++++++++-- topdown/exported_test.go | 4 + topdown/input.go | 32 ++++--- topdown/input_test.go | 9 +- 14 files changed, 133 insertions(+), 251 deletions(-) diff --git a/ast/term.go b/ast/term.go index 67198ed3c7..370e6a7fc7 100644 --- a/ast/term.go +++ b/ast/term.go @@ -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) { diff --git a/internal/compiler/wasm/wasm.go b/internal/compiler/wasm/wasm.go index 78172e9511..7e25b47150 100644 --- a/internal/compiler/wasm/wasm.go +++ b/internal/compiler/wasm/wasm.go @@ -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 diff --git a/internal/planner/planner.go b/internal/planner/planner.go index 32ffbfba0f..d431d69140 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -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 } diff --git a/internal/wasm/instruction/variable.go b/internal/wasm/instruction/variable.go index ac57e5048f..063ffdb96d 100644 --- a/internal/wasm/instruction/variable.go +++ b/internal/wasm/instruction/variable.go @@ -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} +} diff --git a/internal/wasm/sdk/opa/opa_test.go b/internal/wasm/sdk/opa/opa_test.go index a9d84e3e98..f22187da1b 100644 --- a/internal/wasm/sdk/opa/opa_test.go +++ b/internal/wasm/sdk/opa/opa_test.go @@ -129,19 +129,6 @@ a = "c" { input > 2 }`, Evals: []Eval{{}}, WantErr: ":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: ":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"]]}}`}, }, diff --git a/internal/wasm/sdk/test/e2e/exceptions.yaml b/internal/wasm/sdk/test/e2e/exceptions.yaml index 10197031fa..7b41fca99b 100644 --- a/internal/wasm/sdk/test/e2e/exceptions.yaml +++ b/internal/wasm/sdk/test/e2e/exceptions.yaml @@ -1,12 +1,5 @@ # Exception Format is : "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" diff --git a/test/cases/testdata/withkeyword/test-withkeyword-1019.yaml b/test/cases/testdata/withkeyword/test-withkeyword-1019.yaml index 95a7fbe68d..6c01cb4b14 100644 --- a/test/cases/testdata/withkeyword/test-withkeyword-1019.yaml +++ b/test/cases/testdata/withkeyword/test-withkeyword-1019.yaml @@ -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" diff --git a/test/cases/testdata/withkeyword/test-withkeyword-1035.yaml b/test/cases/testdata/withkeyword/test-withkeyword-1035.yaml index 2da4e9c870..74bb1e0e33 100644 --- a/test/cases/testdata/withkeyword/test-withkeyword-1035.yaml +++ b/test/cases/testdata/withkeyword/test-withkeyword-1035.yaml @@ -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 diff --git a/test/wasm/assets/016_with.yaml b/test/wasm/assets/016_with.yaml index 45e9b0c301..ccb433ac27 100644 --- a/test/wasm/assets/016_with.yaml +++ b/test/wasm/assets/016_with.yaml @@ -229,7 +229,10 @@ cases: - note: with conflict query: | input = x with input.foo as 1 with input.foo.bar as 2 - want_error: ":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 diff --git a/topdown/eval.go b/topdown/eval.go index f345f66e35..b51f5a6572 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -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 diff --git a/topdown/eval_test.go b/topdown/eval_test.go index 333c2e15f6..9f1a53d9eb 100644 --- a/topdown/eval_test.go +++ b/topdown/eval_test.go @@ -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) + } + }) } } diff --git a/topdown/exported_test.go b/topdown/exported_test.go index 45491b82d6..dcb6c73b98 100644 --- a/topdown/exported_test.go +++ b/topdown/exported_test.go @@ -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) } diff --git a/topdown/input.go b/topdown/input.go index e3c68648f6..cb70aeb71e 100644 --- a/topdown/input.go +++ b/topdown/input.go @@ -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 +} diff --git a/topdown/input_test.go b/topdown/input_test.go index bc5a04db1f..237e8eaa8d 100644 --- a/topdown/input_test.go +++ b/topdown/input_test.go @@ -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",