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

wasm/planner: avoid ir.ScanStmt in ir.NotStmt (#3287)

Browse Source 
* planner: optimize planRefData for all known vars, no rules

This was prompted by #3279: When planning an ir.ScanStmt inside of an
ir.NotStmt, the condition variable trick doesn't work: the scan may
never conclude successfully, but the code still runs into the condition
trap.

Now, not stmts should only ever have safe variables, so we should be
bypassing this by safely avoiding the scan.

* internal/ir: add fmt.Stringer to localOrConst interface

This way, the pretty-printed policy IR becomes a lot more transparent.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
This commit is contained in:
Stephan Renatus
2021-03-25 11:01:16 +01:00
committed by GitHub
parent 710b8574f5
commit 6a8fa0dd96
8 changed files with 114 additions and 369 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/ir/ir.go
+13 -1
View File
@@ -185,20 +185,32 @@ type CallDynamicStmt struct {
// LocalOrConst is a tagged union of the two types, Local and StringIndex.
// It's used with CallDynamicStmt.
type LocalOrConst interface{ localOrConst() }
type LocalOrConst interface {
fmt.Stringer
localOrConst()
}
func (Local) localOrConst() {}
func (l Local) String() string {
return fmt.Sprintf("Local<%d>", int(l))
}
// StringIndex represents the index into the plan's list of constant strings
// of a constant string.
type StringIndex int
func (StringIndex) localOrConst() {}
func (s StringIndex) String() string {
return fmt.Sprintf("String<%d>", int(s))
}
// Bool represents a constant boolean.
type Bool bool
func (Bool) localOrConst() {}
func (b Bool) String() string {
return fmt.Sprintf("Bool<%v>", bool(b))
}
// BlockStmt represents a nested block. Nested blocks and break statements can
// be used to short-circuit execution.
internal/planner/planner.go
+41 -46
View File
@@ -536,7 +536,6 @@ func (p *Planner) planExpr(e *ast.Expr, iter planiter) error {
}
func (p *Planner) planNot(e *ast.Expr, iter planiter) error {
not := &ir.NotStmt{
Block: &ir.Block{},
}
@@ -544,9 +543,7 @@ func (p *Planner) planNot(e *ast.Expr, iter planiter) error {
prev := p.curr
p.curr = not.Block
if err := p.planExpr(e.Complement(), func() error {
return nil
}); err != nil {
if err := p.planExpr(e.Complement(), func() error { return nil }); err != nil {
return err
}
@@ -1414,19 +1411,7 @@ func (p *Planner) planRefRec(ref ast.Ref, index int, iter planiter) error {
return iter()
}
scan := false
ast.WalkVars(ref[index], func(v ast.Var) bool {
if !scan {
_, exists := p.vars.Get(v)
if !exists {
scan = true
}
}
return scan
})
if !scan {
if !p.unseenVars(ref[index]) {
return p.planDot(ref[index], func() error {
return p.planRefRec(ref, index+1, iter)
})
@@ -1580,32 +1565,36 @@ func (p *Planner) planRefData(virtual *ruletrie, base *baseptr, ref ast.Ref, ind
}); err != nil {
return err
}
// Perform a scan of the base documents starting from the location referred
// to by the 'path' data pointer. Use the `lexclude` set to avoid revisiting
// sub trees.
p.ltarget = base.local
return p.planRefRec(base.path, 0, func() error {
return p.planScan(ref[index], func(lkey ir.Local) error {
if lexclude != nil {
lignore := p.newLocal()
p.appendStmt(&ir.NotStmt{
Block: p.blockWithStmt(&ir.DotStmt{
Source: *lexclude,
Key: lkey,
Target: lignore,
})})
}
// Assume that virtual sub trees have been visited already so
// recurse without the virtual node.
return p.planRefData(nil, &baseptr{local: p.ltarget.(ir.Local)}, ref, index+1, iter)
})
})
}
// There is nothing to exclude, so we do the same thing done above, but
// use planRefRec to avoid the scan if ref[index] is ground or seen.
p.ltarget = base.local
return p.planRefDataBaseScan(base.path, ref, index, lexclude, iter)
}
// Perform a scan of the base documents starting from the location referred
// to by the 'path' data pointer. Use the set (planned into 'lexclude') to
// avoid revisiting sub trees.
func (p *Planner) planRefDataBaseScan(path ast.Ref, ref ast.Ref, index int, lexclude *ir.Local, iter planiter) error {
return p.planRefRec(path, 0, func() error {
return p.planScan(ref[index], func(lkey ir.Local) error {
if lexclude != nil {
lignore := p.newLocal()
p.appendStmt(&ir.NotStmt{
Block: p.blockWithStmt(&ir.DotStmt{
Source: *lexclude,
Key: lkey,
Target: lignore,
})})
}
// Assume that virtual sub trees have been visited already so
// recurse without the virtual node.
return p.planRefData(nil, &baseptr{local: p.ltarget.(ir.Local)}, ref, index+1, iter)
})
base.path = append(base.path, ref[index])
return p.planRefRec(base.path, 0, func() error {
return p.planRefData(nil, &baseptr{local: p.ltarget.(ir.Local)}, ref, index+1, iter)
})
}
@@ -2058,10 +2047,16 @@ func (p *Planner) optimizeLookup(t *ruletrie, ref ast.Ref) ([][]*ast.Rule, []ir.
return res, path, index, true
}
func (p *Planner) seenVar(ref *ast.Term) (bool, bool) {
if v, ok := ref.Value.(ast.Var); ok {
_, ok := p.vars.Get(v)
return true, ok
}
return false, false
func (p *Planner) unseenVars(t *ast.Term) bool {
unseen := false // any var unseen?
ast.WalkVars(t, func(v ast.Var) bool {
if !unseen {
_, exists := p.vars.Get(v)
if !exists {
unseen = true
}
}
return unseen
})
return unseen
}
internal/planner/planner_test.go
+4
View File
@@ -64,6 +64,10 @@ func TestPlannerHelloWorld(t *testing.T) {
note: "negation",
queries: []string{"not input.x.y = 1"},
},
{
note: "not and known vars", // https://github.com/open-policy-agent/opa/issues/3279
queries: []string{`x = "foo"; not data.tenants[x]`},
},
{
note: "array ref pattern match",
queries: []string{"input.x = [1, [y]]"},
test/cases/testdata/negation/test-negation-0777.yaml
Vendored
-82
View File
@@ -1,87 +1,5 @@
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
test/cases/testdata/negation/test-negation-0778.yaml
Vendored
-82
View File
@@ -1,87 +1,5 @@
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
test/cases/testdata/negation/test-negation-0779.yaml
Vendored
-79
View File
@@ -1,87 +1,8 @@
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
test/cases/testdata/negation/test-negation-0780.yaml
Vendored
-79
View File
@@ -1,87 +1,8 @@
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
test/cases/testdata/negation/test-negation-data-ref-with-var.yaml
Vendored
+56
View File
@@ -0,0 +1,56 @@
cases:
- data:
b:
v1: hello
v2: goodbye
modules:
- |
package test
p = y {
y := "v0"
not data.test.q[y]
}
q[x] {
data.b[x] = v
}
note: 'negation/pos: ref with variable'
query: data.test.p = x
want_result:
- x: v0
- data:
b:
v1: hello
v2: goodbye
modules:
- |
package test
p = y {
y := "v1"
not data.test.q[y]
}
q[x] {
data.b[x] = v
}
note: 'negation/neg: ref with variable'
query: data.test.p = x
want_result: []
- data:
bar:
q: 8
modules:
- |
package foo
p {
k := "p"
not data.bar[k]
}
- |
package bar
p = 7
note: 'negation/neg: ref with variable and virtual doc'
query: data.foo.p = x
want_result: []