From 525c93d317bbd09b6c2440e6f01286870acaaa4b Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Thu, 6 Apr 2023 19:25:07 +0200 Subject: [PATCH 01/27] topdown: Adding support for multiple variables in rule refs This is the topdown part of supporting generic refs. Fixes: #5993 Signed-off-by: Johan Fylling --- ast/compile.go | 165 +++-- ast/compile_test.go | 206 +++++- ast/index.go | 8 + ast/parser.go | 19 +- ast/policy.go | 6 + internal/wasm/sdk/test/e2e/exceptions.yaml | 6 +- test/cases/cases.go | 1 + .../refheads/test-arbitrary-var-refs.yaml | 91 +++ topdown/eval.go | 153 ++++- topdown/eval_test.go | 602 ++++++++++++++++++ topdown/exported_test.go | 4 + 11 files changed, 1170 insertions(+), 91 deletions(-) create mode 100644 test/cases/testdata/refheads/test-arbitrary-var-refs.yaml diff --git a/ast/compile.go b/ast/compile.go index 1f99e09692..4d7cde3567 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -7,6 +7,7 @@ package ast import ( "fmt" "io" + "os" "sort" "strconv" "strings" @@ -81,6 +82,28 @@ type Compiler struct { // +--- b // | // +--- c (1 rule) + // + // Another example with general refs containing vars at arbitrary locations: + // + // package ex + // a.b[x].d { x := "c" } # R1 + // a.b.c[x] { x := "d" } # R2 + // a.b[x][y] { x := "c"; y := "d" } # R3 + // p := true # R4 + // + // root + // | + // +--- data (no rules) + // | + // +--- ex (no rules) + // | + // +--- a + // | | + // | +--- b (R1, R3) + // | | + // | +--- c (R2) + // | + // +--- p (R4) RuleTree *TreeNode // Graph contains dependencies between rules. An edge (u,v) is added to the @@ -123,6 +146,7 @@ type Compiler struct { keepModules bool // whether to keep the unprocessed, parse modules (below) parsedModules map[string]*Module // parsed, but otherwise unprocessed modules, kept track of when keepModules is true useTypeCheckAnnotations bool // whether to provide annotated information (schemas) to the type checker + generalRuleRefsEnabled bool } // CompilerStage defines the interface for stages in the compiler. @@ -311,6 +335,8 @@ func NewCompiler() *Compiler { {"BuildComprehensionIndices", "compile_stage_rebuild_comprehension_indices", c.buildComprehensionIndices}, } + _, c.generalRuleRefsEnabled = os.LookupEnv("OPA_ENABLE_GENERAL_RULE_REFS") + return c } @@ -790,17 +816,15 @@ func (c *Compiler) buildRuleIndices() { rules := extractRules(node.Values) hasNonGroundKey := false for _, r := range rules { - if ref := r.Head.Ref(); len(ref) > 1 { - if !ref[len(ref)-1].IsGround() { - hasNonGroundKey = true - } - } + hasNonGroundKey = !r.Head.Ref().IsGround() } if hasNonGroundKey { - // collect children: as of now, this cannot go deeper than one level, - // so we grab those, and abort the DepthFirst processing for this branch - for _, n := range node.Children { - rules = append(rules, extractRules(n.Values)...) + // collect children + for _, child := range node.Children { + child.DepthFirst(func(c *TreeNode) bool { + rules = append(rules, extractRules(c.Values)...) + return false + }) } } @@ -870,10 +894,11 @@ func (c *Compiler) checkRuleConflicts() { kinds := make(map[RuleKind]struct{}, len(node.Values)) defaultRules := 0 + completeRules := 0 + partialRules := 0 arities := make(map[int]struct{}, len(node.Values)) name := "" - var singleValueConflicts []Ref - var multiValueConflicts []Ref + var conflicts []Ref for _, rule := range node.Values { r := rule.(*Rule) @@ -885,70 +910,99 @@ func (c *Compiler) checkRuleConflicts() { defaultRules++ } - // Single-value rules may not have any other rules in their extent: these pairs are invalid: + // Single-value rules may not have any other rules in their extent. + // Rules with vars in their ref are allowed to have rules inside their extent. + // Only the ground portion (terms before the first var term) of a rule's ref is considered when determining + // whether it's inside the extent of another (c.RuleTree is organized this way already). + // These pairs are invalid: // // data.p.q.r { true } # data.p.q is { "r": true } // data.p.q.r.s { true } // - // data.p.q[r] { r := input.r } # data.p.q could be { "r": true } - // data.p.q.r.s { true } + // data.p.q.r { true } + // data.p.q.r[s].t { s = input.key } // - // data.p[r] := x { r = input.key; x = input.bar } - // data.p.q[r] := x { r = input.key; x = input.bar } - // But this is allowed: + // + // data.p.q.r { true } + // data.p.q[r].s.t { r = input.key } + // + // data.p[r] := x { r = input.key; x = input.bar } + // data.p.q[r] := x { r = input.key; x = input.bar } + // + // data.p.q[r] { r := input.r } + // data.p.q.r.s { true } + // // data.p.q[r] = 1 { r := "r" } // data.p.q.s = 2 + // + // data.p[q][r] { q := input.q; r := input.r } + // data.p.q.r { true } + // + // data.p.q[r] { r := input.r } + // data.p[q].r { q := input.q } + // + // data.p.q[r][s] { r := input.r; s := input.s } + // data.p[q].r.s { q := input.q } - if r.Head.RuleKind() == SingleValue && len(node.Children) > 0 { - if len(ref) > 1 && !ref[len(ref)-1].IsGround() { // p.q[x] and p.q.s.t => check grandchildren - for _, c := range node.Children { - grandchildrenFound := false - - if len(c.Values) > 0 { - childRules := extractRules(c.Values) - for _, childRule := range childRules { - childRef := childRule.Ref() - if childRule.Head.RuleKind() == SingleValue && !childRef[len(childRef)-1].IsGround() { - // The child is a partial object rule, so it's effectively "generating" grandchildren. - grandchildrenFound = true - break + if c.generalRuleRefsEnabled { + if r.Ref().IsGround() && len(node.Children) > 0 { + conflicts = node.flattenChildren() + } + } else { // TODO: Remove when general rule refs are enabled by default. + if r.Head.RuleKind() == SingleValue && len(node.Children) > 0 { + if len(ref) > 1 && !ref[len(ref)-1].IsGround() { // p.q[x] and p.q.s.t => check grandchildren + for _, c := range node.Children { + grandchildrenFound := false + + if len(c.Values) > 0 { + childRules := extractRules(c.Values) + for _, childRule := range childRules { + childRef := childRule.Ref() + if childRule.Head.RuleKind() == SingleValue && !childRef[len(childRef)-1].IsGround() { + // The child is a partial object rule, so it's effectively "generating" grandchildren. + grandchildrenFound = true + break + } } } - } - if len(c.Children) > 0 { - grandchildrenFound = true - } + if len(c.Children) > 0 { + grandchildrenFound = true + } - if grandchildrenFound { - singleValueConflicts = node.flattenChildren() - break + if grandchildrenFound { + conflicts = node.flattenChildren() + break + } } + } else { // p.q.s and p.q.s.t => any children are in conflict + conflicts = node.flattenChildren() } - } else { // p.q.s and p.q.s.t => any children are in conflict - singleValueConflicts = node.flattenChildren() } - } - // Multi-value rules may not have any other rules in their extent; e.g.: - // - // data.p[v] { v := ... } - // data.p.q := 42 # In direct conflict with data.p[v], which is constructing a set and cannot have values assigned to a sub-path. + // Multi-value rules may not have any other rules in their extent; e.g.: + // + // data.p[v] { v := ... } + // data.p.q := 42 # In direct conflict with data.p[v], which is constructing a set and cannot have values assigned to a sub-path. - if r.Head.RuleKind() == MultiValue && len(node.Children) > 0 { - multiValueConflicts = node.flattenChildren() + if r.Head.RuleKind() == MultiValue && len(node.Children) > 0 { + conflicts = node.flattenChildren() + } + } + + if r.Head.RuleKind() == SingleValue && r.Head.Ref().IsGround() { + completeRules++ + } else { + partialRules++ } } switch { - case singleValueConflicts != nil: - c.err(NewError(TypeErr, node.Values[0].(*Rule).Loc(), "single-value rule %v conflicts with %v", name, singleValueConflicts)) - - case multiValueConflicts != nil: - c.err(NewError(TypeErr, node.Values[0].(*Rule).Loc(), "multi-value rule %v conflicts with %v", name, multiValueConflicts)) + case conflicts != nil: + c.err(NewError(TypeErr, node.Values[0].(*Rule).Loc(), "rule %v conflicts with %v", name, conflicts)) - case len(kinds) > 1 || len(arities) > 1: + case len(kinds) > 1 || len(arities) > 1 || (completeRules >= 1 && partialRules >= 1): c.err(NewError(TypeErr, node.Values[0].(*Rule).Loc(), "conflicting rules %v found", name)) case defaultRules > 1: @@ -1697,13 +1751,12 @@ func (c *Compiler) rewriteRuleHeadRefs() { } for i := 1; i < len(ref); i++ { - // NOTE(sr): In the first iteration, non-string values in the refs are forbidden + // NOTE: Unless enabled via the OPA_ENABLE_GENERAL_RULE_REFS env var, non-string values in the refs are forbidden // except for the last position, e.g. // OK: p.q.r[s] // NOT OK: p[q].r.s - // TODO(sr): This is stricter than necessary. We could allow any non-var values there, - // but we'll also have to adjust the type tree, for example. - if i != len(ref)-1 { // last + // TODO: Remove when general rule refs are enabled by default. + if !c.generalRuleRefsEnabled && i != len(ref)-1 { // last if _, ok := ref[i].Value.(String); !ok { c.err(NewError(TypeErr, rule.Loc(), "rule head must only contain string terms (except for last): %v", ref[i])) continue diff --git a/ast/compile_test.go b/ast/compile_test.go index 06890bf7a1..dc3d056e14 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -467,6 +467,7 @@ func toRef(s string) Ref { } func TestCompilerCheckRuleHeadRefs(t *testing.T) { + t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") tests := []struct { note string @@ -480,7 +481,6 @@ func TestCompilerCheckRuleHeadRefs(t *testing.T) { `package x p.q[i].r = 1 { i := 10 }`, ), - err: "rego_type_error: rule head must only contain string terms (except for last): i", }, { note: "valid: ref is single-value rule with var key", @@ -559,7 +559,6 @@ func TestCompilerCheckRuleHeadRefs(t *testing.T) { `package x p.q[arr[0]].r { i := 10 }`, ), - err: "rego_type_error: rule head must only contain string terms (except for last): arr[0]", }, { note: "invalid: non-string in ref (not last position)", @@ -567,7 +566,6 @@ func TestCompilerCheckRuleHeadRefs(t *testing.T) { `package x p.q[10].r { true }`, ), - err: "rego_type_error: rule head must only contain string terms (except for last): 10", }, { note: "valid: multi-value with var key", @@ -609,6 +607,64 @@ func TestCompilerCheckRuleHeadRefs(t *testing.T) { } } +// TODO: Remove when general rule refs are enabled by default. +func TestCompilerCheckRuleHeadRefsWithGeneralRuleRefsDisabled(t *testing.T) { + + tests := []struct { + note string + modules []*Module + expected *Rule + err string + }{ + { + note: "ref contains var", + modules: modules( + `package x + p.q[i].r = 1 { i := 10 }`, + ), + err: "rego_type_error: rule head must only contain string terms (except for last): i", + }, + { + note: "invalid: ref in ref", + modules: modules( + `package x + p.q[arr[0]].r { i := 10 }`, + ), + err: "rego_type_error: rule head must only contain string terms (except for last): arr[0]", + }, + { + note: "invalid: non-string in ref (not last position)", + modules: modules( + `package x + p.q[10].r { true }`, + ), + err: "rego_type_error: rule head must only contain string terms (except for last): 10", + }, + } + + for _, tc := range tests { + t.Run(tc.note, func(t *testing.T) { + mods := make(map[string]*Module, len(tc.modules)) + for i, m := range tc.modules { + mods[fmt.Sprint(i)] = m + } + c := NewCompiler() + c.Modules = mods + compileStages(c, c.rewriteRuleHeadRefs) + if tc.err != "" { + assertCompilerErrorStrings(t, c, []string{tc.err}) + } else { + if len(c.Errors) > 0 { + t.Fatalf("expected no errors, got %v", c.Errors) + } + if tc.expected != nil { + assertRulesEqual(t, tc.expected, mods["0"].Rules[0]) + } + } + }) + } +} + func TestRuleTreeWithDotsInHeads(t *testing.T) { // TODO(sr): multi-val with var key in ref @@ -1813,6 +1869,9 @@ p { true }`, import future.keywords bar.baz contains "quz" if true`, + "mod8.rego": `package badrules.complete_partial +p := 1 +p[r] := 2 { r := "foo" }`, }) c.WithPathConflictsCheck(func(path []string) (bool, error) { @@ -1833,6 +1892,7 @@ bar.baz contains "quz" if true`, "rego_type_error: conflicting rules data.badrules.arity.g found", "rego_type_error: conflicting rules data.badrules.arity.p.q.h found", "rego_type_error: conflicting rules data.badrules.arity.p.q.i found", + "rego_type_error: conflicting rules data.badrules.complete_partial.p[r] found", "rego_type_error: conflicting rules data.badrules.p[x] found", "rego_type_error: conflicting rules data.badrules.q found", "rego_type_error: multiple default rules data.badrules.defkw.foo found", @@ -1845,6 +1905,7 @@ bar.baz contains "quz" if true`, } func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { + t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") tests := []struct { note string @@ -1931,7 +1992,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p.q.r { true }`, `package pkg p.q.r.s { true }`), - err: "rego_type_error: single-value rule data.pkg.p.q.r conflicts with [data.pkg.p.q.r.s]", + err: "rego_type_error: rule data.pkg.p.q.r conflicts with [data.pkg.p.q.r.s]", }, { note: "single-value with other rule overlap", @@ -1940,7 +2001,15 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p.q.r { true } p.q.r.s { true } p.q.r.t { true }`), - err: "rego_type_error: single-value rule data.pkg.p.q.r conflicts with [data.pkg.p.q.r.s data.pkg.p.q.r.t]", + err: "rego_type_error: rule data.pkg.p.q.r conflicts with [data.pkg.p.q.r.s data.pkg.p.q.r.t]", + }, + { + note: "single-value with other partial object (same ref) overlap", + modules: modules( + `package pkg + p.q := 1 + p.q[r] := 2 { r := "foo" }`), + err: "rego_type_error: conflicting rules data.pkg.p.q[r] foun", }, { note: "single-value with other rule overlap, unknown key", @@ -1949,16 +2018,22 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p.q[r] = x { r = input.key; x = input.foo } p.q.r.s = x { true } `), - err: "rego_type_error: single-value rule data.pkg.p.q[r] conflicts with [data.pkg.p.q.r.s]", }, { - note: "single-value partial object with other partial object rule overlap, unknown keys (regression test for #5855)", + note: "single-value with other rule overlap, unknown ref var and key", + modules: modules( + `package pkg + p.q[r][s] = x { r = input.key1; s = input.key2; x = input.foo } + p.q.r.s.t = x { true } + `), + }, + { + note: "single-value partial object with other partial object rule overlap, unknown keys (regression test for #5855; invalidated by multi-var refs)", modules: modules( `package pkg p[r] := x { r = input.key; x = input.bar } p.q[r] := x { r = input.key; x = input.bar } `), - err: "rego_type_error: single-value rule data.pkg.p[r] conflicts with [data.pkg.p.q[r]]", }, { note: "single-value partial object with other partial object (implicit 'true' value) rule overlap, unknown keys", @@ -1967,7 +2042,6 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p[r] := x { r = input.key; x = input.bar } p.q[r] { r = input.key } `), - err: "rego_type_error: single-value rule data.pkg.p[r] conflicts with [data.pkg.p.q[r]]", }, { note: "single-value partial object with multi-value rule (ref head) overlap, unknown key", @@ -2002,7 +2076,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p[v] { v := ["a", "b"][_] } p.q := 42 `), - err: "rego_type_error: multi-value rule data.pkg.p conflicts with [data.pkg.p.q]", + err: "rego_type_error: rule data.pkg.p conflicts with [data.pkg.p.q]", }, { note: "multi-value rule with other rule (ref) overlap", @@ -2011,7 +2085,117 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { p[v] { v := ["a", "b"][_] } p.q.r { true } `), - err: "rego_type_error: multi-value rule data.pkg.p conflicts with [data.pkg.p.q.r]", + err: "rego_type_error: rule data.pkg.p conflicts with [data.pkg.p.q.r]", + }, + { + note: "multi-value rule (dots in head) with other rule (ref) overlap", + modules: modules( + `package pkg + import future.keywords + p.q contains v { v := ["a", "b"][_] } + p.q.r { true } + `), + err: "rule data.pkg.p.q conflicts with [data.pkg.p.q.r]", + }, + { + note: "multi-value rule (dots and var in head) with other rule (ref) overlap", + modules: modules( + `package pkg + import future.keywords + p[q] contains v { v := ["a", "b"][_] } + p.q.r { true } + `), + }, + { + note: "function with other rule (ref) overlap", + modules: modules( + `package pkg + p(x) := x + p.q.r { true } + `), + err: "rego_type_error: rule data.pkg.p conflicts with [data.pkg.p.q.r]", + }, + { + note: "function with other rule (ref) overlap", + modules: modules( + `package pkg + p(x) := x + p.q.r { true } + `), + err: "rego_type_error: rule data.pkg.p conflicts with [data.pkg.p.q.r]", + }, + { + note: "function (ref) with other rule (ref) overlap", + modules: modules( + `package pkg + p.q(x) := x + p.q.r { true } + `), + err: "rego_type_error: rule data.pkg.p.q conflicts with [data.pkg.p.q.r]", + }, + } + for _, tc := range tests { + t.Run(tc.note, func(t *testing.T) { + mods := make(map[string]*Module, len(tc.modules)) + for i, m := range tc.modules { + mods[fmt.Sprint(i)] = m + } + c := NewCompiler() + c.Modules = mods + compileStages(c, c.checkRuleConflicts) + if tc.err != "" { + assertCompilerErrorStrings(t, c, []string{tc.err}) + } else { + assertCompilerErrorStrings(t, c, []string{}) + } + }) + } +} + +// TODO: Remove when general rule refs are enabled by default. +func TestGeneralRuleRefsDisabled(t *testing.T) { + // OPA_ENABLE_GENERAL_RULE_REFS env var not set + + tests := []struct { + note string + modules []*Module + err string + }{ + { + note: "single-value with other rule overlap, unknown key", + modules: modules( + `package pkg + p.q[r] = x { r = input.key; x = input.foo } + p.q.r.s = x { true } + `), + err: "rego_type_error: rule data.pkg.p.q[r] conflicts with [data.pkg.p.q.r.s]", + }, + { + note: "single-value with other rule overlap, unknown ref var and key", + modules: modules( + `package pkg + p.q[r][s] = x { r = input.key1; s = input.key2; x = input.foo } + p.q.r.s.t = x { true } + `), + err: "rego_type_error: rule head must only contain string terms (except for last): r", + }, + { + note: "single-value partial object with other partial object rule overlap, unknown keys (regression test for #5855; invalidated by multi-var refs)", + modules: modules( + `package pkg + p[r] := x { r = input.key; x = input.bar } + p.q[r] := x { r = input.key; x = input.bar } + `), + err: "rego_type_error: rule data.pkg.p[r] conflicts with [data.pkg.p.q[r]]", + }, + { + note: "single-value partial object with other partial object (implicit 'true' value) rule overlap, unknown keys", + modules: modules( + `package pkg + p[r] := x { r = input.key; x = input.bar } + p.q[r] { r = input.key } + `), + err: "rego_type_error: rule data.pkg.p[r] conflicts with [data.pkg.p.q[r]]", }, } for _, tc := range tests { diff --git a/ast/index.go b/ast/index.go index e14bb72eef..99439063cd 100644 --- a/ast/index.go +++ b/ast/index.go @@ -38,6 +38,7 @@ type IndexResult struct { Default *Rule EarlyExit bool OnlyGroundRefs bool + DynamicRef bool } // NewIndexResult returns a new IndexResult object. @@ -60,6 +61,7 @@ type baseDocEqIndex struct { defaultRule *Rule kind RuleKind onlyGroundRefs bool + dynamicRef bool } func newBaseDocEqIndex(isVirtual func(Ref) bool) *baseDocEqIndex { @@ -68,6 +70,7 @@ func newBaseDocEqIndex(isVirtual func(Ref) bool) *baseDocEqIndex { isVirtual: isVirtual, root: newTrieNodeImpl(), onlyGroundRefs: true, + dynamicRef: false, } } @@ -89,6 +92,9 @@ func (i *baseDocEqIndex) Build(rules []*Rule) bool { if i.onlyGroundRefs { i.onlyGroundRefs = rule.Head.Reference.IsGround() } + if !i.dynamicRef { + i.dynamicRef = rule.Head.HasDynamicRef() + } var skip bool for _, expr := range rule.Body { if op := expr.OperatorTerm(); op != nil && i.skipIndexing.Contains(op) { @@ -141,6 +147,7 @@ func (i *baseDocEqIndex) Lookup(resolver ValueResolver) (*IndexResult, error) { result := NewIndexResult(i.kind) result.Default = i.defaultRule result.OnlyGroundRefs = i.onlyGroundRefs + result.DynamicRef = i.dynamicRef result.Rules = make([]*Rule, 0, len(tr.ordering)) for _, pos := range tr.ordering { @@ -173,6 +180,7 @@ func (i *baseDocEqIndex) AllRules(resolver ValueResolver) (*IndexResult, error) result := NewIndexResult(i.kind) result.Default = i.defaultRule + result.DynamicRef = i.dynamicRef result.OnlyGroundRefs = i.onlyGroundRefs result.Rules = make([]*Rule, 0, len(tr.ordering)) diff --git a/ast/parser.go b/ast/parser.go index 58e9e73c8a..539c0f6d21 100644 --- a/ast/parser.go +++ b/ast/parser.go @@ -11,6 +11,7 @@ import ( "io" "math/big" "net/url" + "os" "regexp" "sort" "strconv" @@ -96,13 +97,14 @@ func (e *parsedTermCacheItem) String() string { // ParserOptions defines the options for parsing Rego statements. type ParserOptions struct { - Capabilities *Capabilities - ProcessAnnotation bool - AllFutureKeywords bool - FutureKeywords []string - SkipRules bool - JSONOptions *JSONOptions - unreleasedKeywords bool // TODO(sr): cleanup + Capabilities *Capabilities + ProcessAnnotation bool + AllFutureKeywords bool + FutureKeywords []string + SkipRules bool + JSONOptions *JSONOptions + unreleasedKeywords bool // TODO(sr): cleanup + generalRuleRefsEnabled bool } // JSONOptions defines the options for JSON operations, @@ -140,6 +142,7 @@ func NewParser() *Parser { s: &state{}, po: ParserOptions{}, } + _, p.po.generalRuleRefsEnabled = os.LookupEnv("OPA_ENABLE_GENERAL_RULE_REFS") return p } @@ -618,7 +621,7 @@ func (p *Parser) parseRules() []*Rule { return []*Rule{&rule} } - if usesContains && !rule.Head.Reference.IsGround() { + if !p.po.generalRuleRefsEnabled && usesContains && !rule.Head.Reference.IsGround() { p.error(p.s.Loc(), "multi-value rules need ground refs") return nil } diff --git a/ast/policy.go b/ast/policy.go index caf756f6aa..01790926bf 100644 --- a/ast/policy.go +++ b/ast/policy.go @@ -975,6 +975,12 @@ func (head *Head) SetLoc(loc *Location) { head.Location = loc } +func (head *Head) HasDynamicRef() bool { + pos := head.Reference.Dynamic() + // Ref is dynamic if it has one non-constant term that isn't the first or last term. + return pos > 0 && pos < len(head.Reference)-1 +} + // Copy returns a deep copy of a. func (a Args) Copy() Args { cpy := Args{} diff --git a/internal/wasm/sdk/test/e2e/exceptions.yaml b/internal/wasm/sdk/test/e2e/exceptions.yaml index 714af1ba93..6989c38020 100644 --- a/internal/wasm/sdk/test/e2e/exceptions.yaml +++ b/internal/wasm/sdk/test/e2e/exceptions.yaml @@ -3,4 +3,8 @@ "data/toplevel integer": "https://github.com/open-policy-agent/opa/issues/3711" "data/nested integer": "https://github.com/open-policy-agent/opa/issues/3711" "withkeyword/function: indirect call, arity 1, replacement is value that needs eval (array comprehension)": "https://github.com/open-policy-agent/opa/issues/5311" -"withkeyword/builtin: indirect call, arity 1, replacement is value that needs eval (array comprehension)": "https://github.com/open-policy-agent/opa/issues/5311" \ No newline at end of file +"withkeyword/builtin: indirect call, arity 1, replacement is value that needs eval (array comprehension)": "https://github.com/open-policy-agent/opa/issues/5311" +"refheads/arbitrary-var, single": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/arbitrary-var, multiple": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/arbitrary-var, overlapping rule, no conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/arbitrary-var, overlapping rule, conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" \ No newline at end of file diff --git a/test/cases/cases.go b/test/cases/cases.go index a614d75e8e..1b4a0f5a0b 100644 --- a/test/cases/cases.go +++ b/test/cases/cases.go @@ -44,6 +44,7 @@ type TestCase struct { WantError *string `json:"want_error,omitempty"` // expect query error message (overrides error code) SortBindings bool `json:"sort_bindings,omitempty"` // indicates that binding values should be treated as sets StrictError bool `json:"strict_error,omitempty"` // indicates that the error depends on strict builtin error mode + Env map[string]string `json:"env,omitempty"` // environment variables to be set during the test } // Load returns a set of built-in test cases. diff --git a/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml b/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml new file mode 100644 index 0000000000..3770c56896 --- /dev/null +++ b/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml @@ -0,0 +1,91 @@ +cases: +- note: 'refheads/arbitrary-var, single' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q].r := i { q := ["a", "b", "c"][i] } + query: data.test.p = x + want_result: + - x: + a: + r: 0 + b: + r: 1 + c: + r: 2 +- note: 'refheads/arbitrary-var, multiple' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q][r] { q := ["a", "b", "c"][r] } + query: data.test.p = x + want_result: + - x: + a: + 0: true + b: + 1: true + c: + 2: true +# FIXME: Below test case returns: rego_type_error: undefined ref: data.test.p.b +#- note: 'refheads/arbitrary-var, deep query' +# env: +# OPA_ENABLE_GENERAL_RULE_REFS: "true" +# modules: +# - | +# package test +# +# p[q][r] { q := ["a", "b", "c"][r] } +# query: data.test.p.b = x +# want_result: +# - x: +# 1: true +- note: 'refheads/arbitrary-var, overlapping rule, no conflict' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q].r := i { q := ["a", "b", "c"][i] } + p.a.r := 0 + query: data.test.p = x + want_result: + - x: + a: + r: 0 + b: + r: 1 + c: + r: 2 +- note: 'refheads/arbitrary-var, overlapping rule, conflict' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q].r := i { q := ["a", "b", "c"][i] } + p.a.r := 42 + query: data.test.p = x + want_error: eval_conflict_error +# FIXME: We can't do complex queries in these tests? +#- note: 'refheads/arbitrary-var, multiple result-set entries' +# env: +# OPA_ENABLE_GENERAL_RULE_REFS: "true" +# modules: +# - | +# package test +# +# p.q[r].s := 1 { r := "foo" } +# p.q[r].s := 2 { r := "bar" } +# query: i := ["foo", "bar"][_]; data.test.p.q[i].s = x +# want_result: +# - x: 1 +# - x: 2 \ No newline at end of file diff --git a/topdown/eval.go b/topdown/eval.go index 1072c95c06..d4080ab42b 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -2269,6 +2269,14 @@ func (e evalVirtual) eval(iter unifyIterator) error { switch ir.Kind { case ast.MultiValue: + var empty *ast.Term + if ir.OnlyGroundRefs { //e.pos == len(e.ref)-1 { + // rule ref contains no vars, so we're building a set + empty = ast.SetTerm() + } else { + // rule ref contains vars, so we're building an object + empty = ast.ObjectTerm() + } eval := evalVirtualPartial{ e: e.e, ref: e.ref, @@ -2278,7 +2286,7 @@ func (e evalVirtual) eval(iter unifyIterator) error { bindings: e.bindings, rterm: e.rterm, rbindings: e.rbindings, - empty: ast.SetTerm(), + empty: empty, } return eval.eval(iter) case ast.SingleValue: @@ -2378,6 +2386,17 @@ func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error } result := e.empty + if e.ir.DynamicRef { + for _, rule := range e.ir.Rules { + result, err = e.evalOneDynamicRefRulePreUnify(rule, result, unknown) + if err != nil { + return err + } + } + e.e.virtualCache.Put(hint.key, result) + return e.evalTerm(iter, e.pos+1, result, e.bindings) + } + for _, rule := range e.ir.Rules { if err := e.evalOneRulePreUnify(iter, rule, hint, result, unknown); err != nil { return err @@ -2419,7 +2438,7 @@ func (e evalVirtualPartial) evalAllRulesNoCache(rules []*ast.Rule) (*ast.Term, e err := child.eval(func(*eval) error { child.traceExit(rule) var err error - result, _, err = e.reduce(rule.Head, child.bindings, result) + result, _, err = e.reduce(rule, child.bindings, result) if err != nil { return err } @@ -2469,7 +2488,7 @@ func (e evalVirtualPartial) evalOneRulePreUnify(iter unifyIterator, rule *ast.Ru if !unknown { var dup bool var err error - result, dup, err = e.reduce(rule.Head, child.bindings, result) + result, dup, err = e.reduce(rule, child.bindings, result) if err != nil { return err } else if dup { @@ -2502,6 +2521,55 @@ func (e evalVirtualPartial) evalOneRulePreUnify(iter unifyIterator, rule *ast.Ru return nil } +func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(rule *ast.Rule, result *ast.Term, unknown bool) (*ast.Term, error) { + + child := e.e.child(rule.Body) + + child.traceEnter(rule) + var defined bool + + // Walk the dynamic portion of rule ref to unify vars + err := child.biunifyDynamicRef(e.pos+1, rule.Ref(), e.ref, child.bindings, e.bindings, func() error { + defined = true + return child.eval(func(child *eval) error { + child.traceExit(rule) + var dup bool + var err error + result, dup, err = e.reduce(rule, child.bindings, result) + if err != nil { + return err + } else if !unknown && dup { + child.traceDuplicate(rule) + return nil + } + + child.traceRedo(rule) + return nil + }) + }) + + if err != nil { + return nil, err + } + + // We're tracing here to exhibit similar behaviour to evalOneRulePreUnify + if !defined { + child.traceFail(rule) + } + + return result, nil +} + +func (e *eval) biunifyDynamicRef(pos int, a, b ast.Ref, b1, b2 *bindings, iter unifyIterator) error { + if pos >= len(a) || pos >= len(b) { + return iter() + } + + return e.biunify(a[pos], b[pos], b1, b2, func() error { + return e.biunifyDynamicRef(pos+1, a, b, b1, b2, iter) + }) +} + func (e evalVirtualPartial) evalOneRulePostUnify(iter unifyIterator, rule *ast.Rule) error { key := e.ref[e.pos+1] @@ -2678,26 +2746,81 @@ func (e evalVirtualPartial) evalCache(iter unifyIterator) (evalVirtualPartialCac return hint, nil } -func (e evalVirtualPartial) reduce(head *ast.Head, b *bindings, result *ast.Term) (*ast.Term, bool, error) { +func getNestedObject(ref ast.Ref, rootObj *ast.Object, b *bindings, l *ast.Location) (*ast.Object, error) { + current := rootObj + for _, term := range ref { + key := b.Plug(term) + if child := (*current).Get(key); child != nil { + if val, ok := child.Value.(ast.Object); ok { + current = &val + } else { + return nil, objectDocKeyConflictErr(l) + } + } else { + child := ast.NewObject() + (*current).Insert(key, ast.NewTerm(child)) + current = &child + } + } + + return current, nil +} + +func (e evalVirtualPartial) reduce(rule *ast.Rule, b *bindings, result *ast.Term) (*ast.Term, bool, error) { var exists bool + head := rule.Head switch v := result.Value.(type) { - case ast.Set: // MultiValue + case ast.Set: key := b.Plug(head.Key) exists = v.Contains(key) v.Add(key) - case ast.Object: // SingleValue - key := head.Reference[len(head.Reference)-1] // NOTE(sr): multiple vars in ref heads need to deal with this better - key = b.Plug(key) - value := b.Plug(head.Value) - if curr := v.Get(key); curr != nil { - if !curr.Equal(value) { - return nil, false, objectDocKeyConflictErr(head.Location) - } - exists = true + case ast.Object: + // data.p.q[r].s.t := 42 {...} + // |----|-| + // ^ ^ + // | leafKey + // objPath + fullPath := rule.Ref() + objPath := fullPath[e.pos+1 : len(fullPath)-1] // the portion of the ref that generates nested objects + leafKey := b.Plug(fullPath[len(fullPath)-1]) // the portion of the ref that is the deepest nested key for the value + + leafObj, err := getNestedObject(objPath, &v, b, head.Location) + if err != nil { + return nil, false, err + } + + if kind := head.RuleKind(); kind == ast.SingleValue { + // We're inserting into an object + val := b.Plug(head.Value) + + if curr := (*leafObj).Get(leafKey); curr != nil { + if !curr.Equal(val) { + return nil, false, objectDocKeyConflictErr(head.Location) + } + exists = true + } else { + (*leafObj).Insert(leafKey, val) + } } else { - v.Insert(key, value) + // We're inserting into a set + var set *ast.Set + if leaf := (*leafObj).Get(leafKey); leaf != nil { + if s, ok := leaf.Value.(ast.Set); ok { + set = &s + } else { + return nil, false, objectDocKeyConflictErr(head.Location) + } + } else { + s := ast.NewSet() + (*leafObj).Insert(leafKey, ast.NewTerm(s)) + set = &s + } + + key := b.Plug(head.Key) + exists = (*set).Contains(key) + (*set).Add(key) } } diff --git a/topdown/eval_test.go b/topdown/eval_test.go index ace27b65c2..6d1379610b 100644 --- a/topdown/eval_test.go +++ b/topdown/eval_test.go @@ -6,6 +6,8 @@ package topdown import ( "context" + "encoding/json" + "strings" "testing" "github.com/open-policy-agent/opa/ast" @@ -443,3 +445,603 @@ func TestTopdownVirtualCache(t *testing.T) { }) } } + +func TestPartialRule(t *testing.T) { + t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + + ctx := context.Background() + store := inmem.New() + + tests := []struct { + note string + module string + query string + exp string + expErr string + }{ + { + note: "partial set", + module: `package test + p[v] { + v := [1, 2, 3][_] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": [1, 2, 3]}}}]`, + }, + { + note: "partial object", + module: `package test + p[i] := v { + v := [1, 2, 3][i] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"0": 1, "1": 2, "2": 3}}}}]`, + }, + { + note: "partial object (const key)", + module: `package test + p["foo"] := v { + v := 42 + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": 42}}}}]`, + }, + { + note: "partial object (dots in head)", + module: `package test + p.foo := v { + v := 42 + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": 42}}}}]`, + }, + { + note: "partial object (dots and var in head)", + module: `package test + p.q.r[i] := v { + v := ["a", "b", "c"][i] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"q": {"r": {"0": "a", "1": "b", "2": "c"}}}}}}]`, + }, + { + note: "partial object (dots in head and implicit 'true' value)", + module: `package test + p.q.r[v] { + v := [1, 2, 3][_] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"q": {"r": {"1": true, "2": true, "3": true}}}}}}]`, + }, + { + note: "partial set (dots in head)", + module: `package test + import future.keywords + p.q contains v if { + v := [1, 2, 3][_] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"q": [1, 2, 3]}}}}]`, + }, + { + note: "partial set (var in head)", + module: `package test + import future.keywords + p[q] contains v if { + q := "foo" + v := [1, 2, 3][_] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": [1, 2, 3]}}}}]`, + }, + { + note: "partial set (dots and vars in head)", + module: `package test + import future.keywords + p[q].r contains v if { + q := "foo" + v := [1, 2, 3][_] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": {"r": [1, 2, 3]}}}}}]`, + }, + { + note: "partial object (multiple vars in head ref)", + module: `package test + p.q[x].r[i] := v { + some i + v := [1, 2, 3][i] + x := ["a", "b", "c"][_] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"q": {"a": {"r": {"0": 1, "1": 2, "2": 3}}, "b": {"r": {"0": 1, "1": 2, "2": 3}}, "c": {"r": {"0": 1, "1": 2, "2": 3}}}}}}}]`, + }, + { + note: "partial object (multiple vars in head ref) #2", + module: `package test + p[j].foo[i] := v { + v := [1, 2, 3][i] + j := ["a", "b", "c"][_] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"a": {"foo": {"0": 1, "1": 2, "2": 3}}, "b": {"foo": {"0": 1, "1": 2, "2": 3}}, "c": {"foo": {"0": 1, "1": 2, "2": 3}}}}}}]`, + }, + { + note: "partial object (var in head ref, set leaf)", + module: `package test + import future.keywords + p[j] contains v if { + v := [1, 2, 3][_] + j := ["a", "b", "c"][_] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"a": [1, 2, 3], "b": [1, 2, 3], "c": [1, 2, 3]}}}}]`, + }, + { + note: "partial object (multiple vars in head ref, partial set)", + module: `package test + import future.keywords + p[j][i] contains v if { + v := [1, 2, 3][_] + j := ["a", "b", "c"][_] + i := "foo" + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"a": {"foo": [1, 2, 3]}, "b": {"foo": [1, 2, 3]}, "c": {"foo": [1, 2, 3]}}}}}]`, + }, + { + note: "partial object generating conflicting keys", + module: `package test + p[k] := x { + k := "foo" + x := [1, 2][_] + }`, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object generating conflicting keys (dots in head)", + module: `package test + p.q[k] := x { + k := "foo" + x := [1, 2][_] + }`, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object generating conflicting nested keys", + module: `package test + p.q[k].s := x { + k := "foo" + x := [1, 2][_] + }`, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + // TODO: Add test case with else block + // Overlapping rules + { + note: "partial object with overlapping rule (defining key/value in object)", + module: `package test + foo.bar[i] := v { + v := ["a", "b", "c"][i] + } + foo.bar.baz := 42 + `, + query: `data = x`, + exp: `[{"x": {"test": {"foo": {"bar": {"0": "a", "1": "b", "2": "c", "baz": 42}}}}}]`, + }, + { + note: "partial object with overlapping rule (dee ref on overlap)", + module: `package test + p[k] := 1 { + k := "foo" + } + p.q.r.s.t := 42 + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": 1, "q": {"r": {"s": {"t": 42}}}}}}}]`, + }, + { + note: "partial object with overlapping rule (dee ref on overlap; conflict)", + module: `package test + p[k] := 1 { + k := "q" + } + p.q.r.s.t := 42 + `, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object with overlapping rule (key conflict)", + module: `package test + foo.bar[k] := v { + k := "a" + v := 43 + } + foo.bar["a"] := 42 + `, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object generating conflicting nested keys (different nested object depth)", + module: `package test + p.q.r { + true + } + p.q[r].s.t { + r := "foo" + }`, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"q": {"foo": {"s": {"t": true}}, "r": true}}}}}]`, + }, + { + note: "partial object generating conflicting nested keys (different nested object depth; key conflict)", + module: `package test + p.q[k].s := 1 { + k := "r" + } + p.q[k].s.t := 1 { + k := "r" + }`, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object (overlapping rules producing same values)", + module: `package test + p.foo.bar[i] := v { + v := ["a", "b", "c"][i] + } + p.foo[i][j] := v { + i := "bar" + v := ["a", "b", "c"][j] + } + p[q][i][j] := v { + q := "foo" + i := "bar" + v := ["a", "b", "c"][j] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": {"bar": {"0": "a", "1": "b", "2": "c"}}}}}}]`, + }, + { + note: "partial object (overlapping rules, same depth, producing non-conflicting keys)", + module: `package test + p.foo[i].bar := v { + v := ["a", "b", "c"][i] + } + p.foo.bar[i] := v { + v := ["a", "b", "c"][i] + } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": { + "0": {"bar": "a"}, + "1": {"bar": "b"}, + "2": {"bar": "c"}, + "bar": {"0": "a", "1": "b", "2": "c"}}}}}}]`, + }, + // Intersections with object values + { + note: "partial object NOT intersecting with object value of other rule", + module: `package test + p.foo := {"bar": {"baz": 1}} + p[k] := 2 {k := "other"} + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": {"bar": {"baz": 1}}, "other": 2}}}}]`, + }, + { + note: "partial object NOT intersecting with object value of other rule (nested object merge along rule refs)", + module: `package test + p.foo.bar := {"baz": 1} # p.foo.bar == {"baz": 1} + p[k].bar2 := v {k := "foo"; v := {"other": 2}} # p.foo.bar2 == {"other": 2} + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"foo": {"bar": {"baz": 1}, "bar2": {"other": 2}}}}}}]`, + }, + { + note: "partial object intersecting with object value of other rule (not merging otherwise conflict-free obj values)", + module: `package test + p.foo := {"bar": {"baz": 1}} # p == {"foo": {"bar": {"baz": 1}}} + p[k] := v {k := "foo"; v := {"bar": {"other": 2}}} # p == {"foo": {"bar": {"other": 2}}} + `, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", // conflict on key "bar" which is inside rule values, which may not be modified by other rule + }, + { + note: "partial object rules with overlapping known ref vars (no eval-time conflict)", + module: `package test + p[k].r1 := 1 { k := "q" } + p[k].r2 := 2 { k := "q" } + `, + query: `data = x`, + exp: `[{"x": {"test": {"p": {"q": {"r1": 1, "r2": 2}}}}}]`, + }, + { + note: "partial object rules with overlapping known ref vars (eval-time conflict)", + module: `package test + p[k].r := 1 { k := "q" } + p[k].r := 2 { k := "q" } + `, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object rules with overlapping known ref vars, non-overlapping object type values (eval-time conflict)", + module: `package test + p[k].r := {"s1": 1} { k := "q" } + p[k].r := {"s2": 2} { k := "q" } + `, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + // Deep queries + { + note: "deep query into partial object", + module: `package test + p.q[r] := 1 { r := "foo" } + `, + query: `data.test.p.q.foo = x`, + exp: `[{"x": 1}]`, + }, + { + note: "deep query to, but not into partial set", + module: `package test + import future.keywords + p.q.r contains s { {"foo", "bar", "bax"}[s] } + `, + query: `data.test.p = x`, + exp: `[{"x": {"q": {"r": ["bar", "bax", "foo"]}}}]`, + }, + { + note: "deep query to, but not into partial set", + module: `package test + import future.keywords + p.q.r contains s { {"foo", "bar", "bax"}[s] } + `, + query: `data.test.p.q = x`, + exp: `[{"x": {"r": ["bar", "bax", "foo"]}}]`, + }, + { + note: "deep query to, but not into partial set", + module: `package test + import future.keywords + p.q.r contains s { {"foo", "bar", "bax"}[s] } + `, + query: `data.test.p.q.r = x`, + exp: `[{"x": ["bar", "bax", "foo"]}]`, + }, + { + note: "deep query into partial set", + module: `package test + import future.keywords + p.q contains r { {"foo", "bar", "bax"}[r] } + `, + query: `data.test.p.q.foo = x`, + exp: `[{"x": "foo"}]`, + }, + { + note: "deep query into partial object and object value", + module: `package test + p.q[r] := x { + r := "foo" + x := {"bar": {"baz": 1}} + } + `, + query: `data.test.p.q.foo.bar = x`, + exp: `[{"x": {"baz": 1}}]`, + }, + { + note: "deep query into partial object and set value", + module: `package test + import future.keywords + p.q[r].s contains x { + r := "foo" + {"foo", "bar", "bax"}[x] + } + `, + query: `data.test.p.q.foo.s.bar = x`, + exp: `[{"x": "bar"}]`, + }, + { + note: "deep query into partial object and object value, non-tail var", + module: `package test + p.q[r].s := x { + r := "foo" + x := {"bar": {"baz": 1}} + } + `, + query: `data.test.p.q.foo.s.bar = x`, + exp: `[{"x": {"baz": 1}}]`, + }, + { + note: "deep query into partial object, on first var in ref", + module: `package test + p.q[r].s := 1 { r := "foo" } + `, + query: `data.test.p.q.foo = x`, + exp: `[{"x": {"s": 1}}]`, + }, + { + note: "deep query into partial object, beyond first var in ref", + module: `package test + p.q[r].s := 1 { r := "foo" } + `, + query: `data.test.p.q.foo.s = x`, + exp: `[{"x": 1}]`, + }, + { + note: "deep query into partial object, on first var in ref, multiple vars", + module: `package test + p.q[r][s] := 1 { r := "foo"; s := "bar" } + `, + query: `data.test.p.q.foo = x`, + exp: `[{"x": {"bar": 1}}]`, + }, + { + note: "deep query into partial object, beyond first var in ref, multiple vars", + module: `package test + p.q[r][s] := 1 { r := "foo"; s := "bar" } + `, + query: `data.test.p.q.foo.bar = x`, + exp: `[{"x": 1}]`, + }, + { + note: "deep query into partial object, beyond first var in ref, multiple vars", + module: `package test + p.q[r][s].t := 1 { r := "foo"; s := "bar" } + `, + query: `data.test.p.q.foo.bar = x`, + exp: `[{"x": {"t": 1}}]`, + }, + { + note: "deep query to partial object, overlapping rules (key override), no dynamic ref", + module: `package test + p.q[r] := 1 { r := "foo" } + p.q.r := 2 + `, + query: `data.test.p.q = x`, + exp: `[{"x": {"foo": 1, "r": 2}}]`, + }, + { + note: "deep query into partial object, overlapping rules (key override), no dynamic ref", + module: `package test + p.q[r] := 1 { r := "foo" } + p.q.r := 2 + `, + query: `data.test.p.q.r = x`, + exp: `[{"x": 2}]`, + }, + { + note: "deep query into partial object, overlapping rules, no dynamic ref", + module: `package test + p.q[r] := 1 { r := "foo" } + p.q[r] := 2 { r := "bar" } + `, + query: `data.test.p.q.foo = x`, + exp: `[{"x": 1}]`, + }, + { + note: "deep query into partial object, overlapping rules with same key/value, no dynamic ref", + module: `package test + p.q[r] := 1 { r := "foo" } + p.q[r] := 1 { r := "foo" } + `, + query: `data.test.p.q.foo = x`, + exp: `[{"x": 1}]`, + }, + { + note: "deep query into partial object, overlapping rules, dynamic ref", + module: `package test + p.q[r].s := 1 { r := "r" } + p.q.r[s] := 2 { s := "foo" } + `, + query: `data.test.p.q.r = x`, + exp: `[{"x": {"s": 1, "foo": 2}}]`, + }, + { + note: "deep query into partial object, overlapping rules with same key/value, dynamic ref", + module: `package test + p.q[r].s := 1 { r := "r" } + p.q.r[s] := 1 { s := "s" } + `, + query: `data.test.p.q.r = x`, + exp: `[{"x": {"s": 1}}]`, + }, + // Multiple results + //{ + // note: "deep query to partial object, overlapping rules with same key/value, dynamic ref", + // module: `package test + // p.q[r] := 1 { r := "foo" } + // p.q[r] := 2 { r := "bar" } + // `, + // query: `i := ["foo", "bar"][_]; data.test.p.q[i] = x`, + // exp: `[{"x": {"s": 1}}]`, + //}, + //{ + // note: "deep query into partial object, overlapping rules with same key/value, dynamic ref", + // module: `package test + // p.q[r].s := 1 { r := "foo" } + // p.q[r].s := 2 { r := "bar" } + // `, + // query: `i := ["foo", "bar"][_]; data.test.p.q[i].s = x`, + // exp: `[{"x": 1}, {"x": 2}]`, + //}, + } + + for _, tc := range tests { + t.Run(tc.note, func(t *testing.T) { + compiler := compileModules([]string{tc.module}) + txn := storage.NewTransactionOrDie(ctx, store) + defer store.Abort(ctx, txn) + + query := NewQuery(ast.MustParseBody(tc.query)). + WithCompiler(compiler). + WithStore(store). + WithTransaction(txn) + + qrs, err := query.Run(ctx) + if tc.expErr != "" { + if err == nil { + t.Fatalf("Expected error %v but got result: %v", tc.expErr, qrs) + } + if exp, act := tc.expErr, err.Error(); !strings.Contains(act, exp) { + t.Fatalf("Expected error %v but got: %v", exp, act) + } + } else { + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if exp, act := 1, len(qrs); exp != act { + t.Fatalf("expected %d query result, got %d query results: %+v", exp, act, qrs) + } + var exp []map[string]interface{} + _ = json.Unmarshal([]byte(tc.exp), &exp) + testAssertResultSet(t, exp, qrs, false) + } + }) + } +} + +// TODO: Remove when general rule refs are enabled by default. +func TestGeneralRuleRefsFeatureFlag(t *testing.T) { + module := ast.MustParseModule(`package test + p[q].r { q := "q" }`) + mods := map[string]*ast.Module{ + "": module, + } + c := ast.NewCompiler() + c.Compile(mods) + + if !strings.Contains(c.Errors.Error(), "rego_type_error: rule head must only contain string terms (except for last)") { + t.Fatal("Expected error but got:", c.Errors) + } + + t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + + c = ast.NewCompiler() + c.Compile(mods) + + if c.Errors != nil { + t.Fatal("Unexpected error:", c.Errors) + } +} diff --git a/topdown/exported_test.go b/topdown/exported_test.go index 675132d423..82b6f52576 100644 --- a/topdown/exported_test.go +++ b/topdown/exported_test.go @@ -49,6 +49,10 @@ type opt func(*Query) *Query func testRun(t *testing.T, tc cases.TestCase, opts ...opt) { + for k, v := range tc.Env { + t.Setenv(k, v) + } + ctx := context.Background() modules := map[string]string{} From d387cfd43500c8873b683dddeb59e796e711c32a Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Thu, 6 Jul 2023 16:57:49 +0200 Subject: [PATCH 02/27] Updating commented-out tests. Signed-off-by: Johan Fylling --- topdown/eval_test.go | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/topdown/eval_test.go b/topdown/eval_test.go index 6d1379610b..750de2317f 100644 --- a/topdown/eval_test.go +++ b/topdown/eval_test.go @@ -968,24 +968,24 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"s": 1}}]`, }, // Multiple results - //{ - // note: "deep query to partial object, overlapping rules with same key/value, dynamic ref", - // module: `package test - // p.q[r] := 1 { r := "foo" } - // p.q[r] := 2 { r := "bar" } - // `, - // query: `i := ["foo", "bar"][_]; data.test.p.q[i] = x`, - // exp: `[{"x": {"s": 1}}]`, - //}, - //{ - // note: "deep query into partial object, overlapping rules with same key/value, dynamic ref", - // module: `package test - // p.q[r].s := 1 { r := "foo" } - // p.q[r].s := 2 { r := "bar" } - // `, - // query: `i := ["foo", "bar"][_]; data.test.p.q[i].s = x`, - // exp: `[{"x": 1}, {"x": 2}]`, - //}, + { + note: "query to partial object, overlapping rules, dynamic ref, multiple results", + module: `package test + p.q[r].s := 1 { r := "foo" } + p.q[r].s := 2 { r := "bar" } + `, + query: `data.test.p.q[i] = x`, + exp: `[{"i": "bar", "x": {"s": 2}}, {"i": "foo", "x": {"s": 1}}]`, + }, + { + note: "deep query into partial object, overlapping rules, dynamic ref, multiple results", + module: `package test + p.q[r].s := 1 { r := "foo" } + p.q[r].s := 2 { r := "bar" } + `, + query: `data.test.p.q[i].s = x`, + exp: `[{"i": "bar", "x": 2}, {"i": "foo", "x": 1}]`, + }, } for _, tc := range tests { @@ -1011,11 +1011,12 @@ func TestPartialRule(t *testing.T) { if err != nil { t.Fatalf("Unexpected error: %v", err) } - if exp, act := 1, len(qrs); exp != act { - t.Fatalf("expected %d query result, got %d query results: %+v", exp, act, qrs) - } + var exp []map[string]interface{} _ = json.Unmarshal([]byte(tc.exp), &exp) + if exp, act := len(exp), len(qrs); exp != act { + t.Fatalf("expected %d query result, got %d query results: %+v", exp, act, qrs) + } testAssertResultSet(t, exp, qrs, false) } }) From d1dcb034b874506217648f0edbd037644efb4877 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Mon, 10 Jul 2023 10:24:17 +0200 Subject: [PATCH 03/27] Fixing yaml test expecting multiple results. Signed-off-by: Johan Fylling --- .../refheads/test-arbitrary-var-refs.yaml | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml b/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml index 3770c56896..7bae8703c8 100644 --- a/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml +++ b/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml @@ -75,17 +75,18 @@ cases: p.a.r := 42 query: data.test.p = x want_error: eval_conflict_error -# FIXME: We can't do complex queries in these tests? -#- note: 'refheads/arbitrary-var, multiple result-set entries' -# env: -# OPA_ENABLE_GENERAL_RULE_REFS: "true" -# modules: -# - | -# package test -# -# p.q[r].s := 1 { r := "foo" } -# p.q[r].s := 2 { r := "bar" } -# query: i := ["foo", "bar"][_]; data.test.p.q[i].s = x -# want_result: -# - x: 1 -# - x: 2 \ No newline at end of file +- note: 'refheads/arbitrary-var, multiple result-set entries' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p.q[r].s := 1 { r := "foo" } + p.q[r].s := 2 { r := "bar" } + query: data.test.p.q[i].s = x + want_result: + - i: foo + x: 1 + - i: bar + x: 2 \ No newline at end of file From 2073c3d32eca6c10c435e3d458bab08eaa1b3f80 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Wed, 12 Jul 2023 10:30:10 +0200 Subject: [PATCH 04/27] Not running yaml test for WASM Signed-off-by: Johan Fylling --- internal/wasm/sdk/test/e2e/exceptions.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/wasm/sdk/test/e2e/exceptions.yaml b/internal/wasm/sdk/test/e2e/exceptions.yaml index 6989c38020..fa3097e7b9 100644 --- a/internal/wasm/sdk/test/e2e/exceptions.yaml +++ b/internal/wasm/sdk/test/e2e/exceptions.yaml @@ -7,4 +7,5 @@ "refheads/arbitrary-var, single": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/arbitrary-var, multiple": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/arbitrary-var, overlapping rule, no conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" -"refheads/arbitrary-var, overlapping rule, conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" \ No newline at end of file +"refheads/arbitrary-var, overlapping rule, conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/arbitrary-var, multiple result-set entries": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" \ No newline at end of file From 659ef2abadab58bf5eb78649c758009b52bfd9d4 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 4 Jul 2023 10:59:18 +0200 Subject: [PATCH 05/27] Moving type merge from `typeChecker.checkRule()` to `typeTreeNode.Insert()` Signed-off-by: Johan Fylling --- ast/check.go | 13 ++----------- ast/env.go | 14 ++++++++++++++ types/types.go | 4 ++++ 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/ast/check.go b/ast/check.go index 217ab78856..2cfe3ddfaf 100644 --- a/ast/check.go +++ b/ast/check.go @@ -232,10 +232,7 @@ func (tc *typeChecker) checkRule(env *TypeEnv, as *AnnotationSet, rule *Rule) { f := types.NewFunction(args, cpy.Get(rule.Head.Value)) - // Union with existing. - exist := env.tree.Get(path) - tpe = types.Or(exist, f) - + tpe = f } else { switch rule.Head.RuleKind() { case SingleValue: @@ -247,22 +244,16 @@ func (tc *typeChecker) checkRule(env *TypeEnv, as *AnnotationSet, rule *Rule) { typeK := cpy.Get(last) if typeK != nil && typeV != nil { - exist := env.tree.Get(path) - typeV = types.Or(types.Values(exist), typeV) - typeK = types.Or(types.Keys(exist), typeK) tpe = types.NewObject(nil, types.NewDynamicProperty(typeK, typeV)) } } else { if typeV != nil { - exist := env.tree.Get(path) - tpe = types.Or(typeV, exist) + tpe = typeV } } case MultiValue: typeK := cpy.Get(rule.Head.Key) if typeK != nil { - exist := env.tree.Get(path) - typeK = types.Or(types.Keys(exist), typeK) tpe = types.NewSet(typeK) } } diff --git a/ast/env.go b/ast/env.go index d5a0706614..6d365378fe 100644 --- a/ast/env.go +++ b/ast/env.go @@ -321,6 +321,7 @@ func (n *typeTreeNode) Insert(path Ref, tpe types.Type) { child = c.(*typeTreeNode) if child.value != nil && i+1 < len(path) { + // If child has an object value, merge the new value into it. if o, ok := child.value.(*types.Object); ok { var err error @@ -335,6 +336,19 @@ func (n *typeTreeNode) Insert(path Ref, tpe types.Type) { curr = child } + if curr.value != nil { + if tpeObj, ok := tpe.(*types.Object); ok { + // FIXME: Merge objects differently to preserve static and dynamic parts(?) + typeK := types.Or(types.Keys(curr.value), types.Keys(tpeObj)) + typeV := types.Or(types.Values(curr.value), types.Values(tpeObj)) + tpe = types.NewObject(nil, types.NewDynamicProperty(typeK, typeV)) + } else if tpeSet, ok := tpe.(*types.Set); ok { + typeK := types.Or(types.Keys(curr.value), tpeSet.Of()) + tpe = types.NewSet(typeK) + } else { + tpe = types.Or(curr.value, tpe) + } + } curr.value = tpe if _, ok := tpe.(*types.Object); ok && curr.children.Len() > 0 { diff --git a/types/types.go b/types/types.go index 24b1d1248f..bc2a50b4c1 100644 --- a/types/types.go +++ b/types/types.go @@ -259,6 +259,10 @@ func NewSet(of Type) *Set { } } +func (t *Set) Of() Type { + return t.of +} + // MarshalJSON returns the JSON encoding of t. func (t *Set) MarshalJSON() ([]byte, error) { return json.Marshal(t.toMap()) From 692770ef0bc8c4e019d8346e15d9cdf542b8b4e5 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 4 Jul 2023 20:52:01 +0200 Subject: [PATCH 06/27] Updating type-checker to handle generic rule refs Fixes: #5994 Signed-off-by: Johan Fylling --- ast/check.go | 47 ++++++- ast/check_test.go | 121 ++++++++++++++++++ ast/env.go | 24 +++- ast/term.go | 8 ++ .../refheads/test-arbitrary-var-refs.yaml | 25 ++-- types/types.go | 27 ++++ 6 files changed, 229 insertions(+), 23 deletions(-) diff --git a/ast/check.go b/ast/check.go index 2cfe3ddfaf..2481cf8ea5 100644 --- a/ast/check.go +++ b/ast/check.go @@ -237,14 +237,16 @@ func (tc *typeChecker) checkRule(env *TypeEnv, as *AnnotationSet, rule *Rule) { switch rule.Head.RuleKind() { case SingleValue: typeV := cpy.Get(rule.Head.Value) - if last := path[len(path)-1]; !last.IsGround() { - - // e.g. store object[string: whatever] at data.p.q.r, not data.p.q.r[x] + if !path.IsGround() { + // e.g. store object[string: whatever] at data.p.q.r, not data.p.q.r[x] or data.p.q.r[x].y[z] + objPath := path.DynamicSuffix() path = path.GroundPrefix() - typeK := cpy.Get(last) - if typeK != nil && typeV != nil { - tpe = types.NewObject(nil, types.NewDynamicProperty(typeK, typeV)) + var err error + tpe, err = nestedObject(cpy, objPath, typeV) + if err != nil { + tc.err([]*Error{NewError(TypeErr, rule.Head.Location, err.Error())}) + tpe = nil } } else { if typeV != nil { @@ -264,6 +266,39 @@ func (tc *typeChecker) checkRule(env *TypeEnv, as *AnnotationSet, rule *Rule) { } } +func nestedObject(env *TypeEnv, path Ref, tpe types.Type) (types.Type, error) { + if len(path) == 0 { + return tpe, nil + } + + k := path[0] + typeV, err := nestedObject(env, path[1:], tpe) + if err != nil { + return nil, err + } + if typeV == nil { + return nil, nil + } + + var staticProperties []*types.StaticProperty + var dynamicProperty *types.DynamicProperty + if k.IsGround() { + key, err := JSON(path[0].Value) + if err != nil { + return nil, err + } + staticProperties = append(staticProperties, types.NewStaticProperty(key, typeV)) + } else { + typeK := env.Get(k) + if typeK == nil { + return nil, nil + } + dynamicProperty = types.NewDynamicProperty(typeK, typeV) + } + + return types.NewObject(staticProperties, dynamicProperty), nil +} + func (tc *typeChecker) checkExpr(env *TypeEnv, expr *Expr) *Error { if err := tc.checkExprWith(env, expr, 0); err != nil { return err diff --git a/ast/check_test.go b/ast/check_test.go index ef214d9392..7d486dd399 100644 --- a/ast/check_test.go +++ b/ast/check_test.go @@ -358,6 +358,16 @@ func TestCheckInferenceRules(t *testing.T) { {`overlap`, `p.q.a = input.a { true }`}, {`overlap`, `p.q[56] = input.a { true }`}, } + ruleset3 := [][2]string{ + {`simple`, `p.q[r][s] = 42 { x = ["a", "b"]; r = x[s] }`}, + {`mixed`, `p.q[r].s[t] = 42 { x = ["a", "b"]; r = x[t] }`}, + {`overrides`, `p.q[r] = "foo" { x = ["a", "b"]; r = x[_] }`}, + {`overrides`, `p.q.r[s] = 42 { x = ["a", "b"]; x[s] }`}, + {`overrides`, `p.q[r].s = true { x = [true, false]; r = x[_] }`}, + {`overrides_static`, `p.q[r].a = "foo" { r = "bar"; s = "baz" }`}, + {`overrides_static`, `p.q[r].b = 42 { r = "bar" }`}, + {`overrides_static`, `p.q[r].c = true { r = "bar" }`}, + } tests := []struct { note string @@ -554,6 +564,117 @@ func TestCheckInferenceRules(t *testing.T) { types.NewDynamicProperty(types.N, types.S), ), }, + { + note: "generic ref-rules, only vars in obj-path, complete obj access", + rules: ruleset3, + ref: "data.simple.p.q", + expected: types.NewObject( + []*types.StaticProperty{}, + types.NewDynamicProperty(types.S, + types.NewObject( + []*types.StaticProperty{}, + types.NewDynamicProperty(types.N, types.N), + ), + ), + ), + }, + { + note: "generic ref-rules, only vars in obj-path, intermediate obj access", + rules: ruleset3, + ref: "data.simple.p.q.b", + expected: types.NewObject( + []*types.StaticProperty{}, + types.NewDynamicProperty(types.N, types.N), + ), + }, + { + note: "generic ref-rules, only vars in obj-path, leaf access", + rules: ruleset3, + ref: "data.simple.p.q.b[1]", + expected: types.N, + }, + { + note: "generic ref-rules, vars and constants in obj-path, complete obj access", + rules: ruleset3, + ref: "data.mixed.p.q", + expected: types.NewObject( + []*types.StaticProperty{}, + types.NewDynamicProperty(types.S, + types.NewObject( + []*types.StaticProperty{types.NewStaticProperty("s", + types.NewObject( + []*types.StaticProperty{}, + types.NewDynamicProperty(types.N, types.N), + ))}, + nil, + ), + ), + ), + }, + { + note: "generic ref-rules, key overrides, complete obj access", + rules: ruleset3, + ref: "data.overrides.p.q", + expected: types.NewObject( + []*types.StaticProperty{types.NewStaticProperty("r", + types.NewObject( + nil, + types.NewDynamicProperty(types.N, types.N), + ))}, + types.NewDynamicProperty(types.Or(types.B, types.S), + types.Or( + types.S, + types.NewObject( + []*types.StaticProperty{types.NewStaticProperty("s", types.B)}, + nil, + ), + ), + ), + ), + }, + { + note: "generic ref-rules, multiple static key overrides, complete obj access", + rules: ruleset3, + ref: "data.overrides_static.p.q", + expected: types.NewObject( + []*types.StaticProperty{}, + types.NewDynamicProperty(types.S, + types.NewAny( + types.NewObject([]*types.StaticProperty{types.NewStaticProperty("a", types.S)}, nil), + types.NewObject([]*types.StaticProperty{types.NewStaticProperty("b", types.N)}, nil), + types.NewObject([]*types.StaticProperty{types.NewStaticProperty("c", types.B)}, nil), + ), + ), + ), + }, + { + note: "generic ref-rules, multiple static key overrides, intermediate obj access", + rules: ruleset3, + ref: "data.overrides_static.p.q.foo", + expected: types.NewAny( + types.NewObject([]*types.StaticProperty{types.NewStaticProperty("a", types.S)}, nil), + types.NewObject([]*types.StaticProperty{types.NewStaticProperty("b", types.N)}, nil), + types.NewObject([]*types.StaticProperty{types.NewStaticProperty("c", types.B)}, nil), + ), + }, + { + note: "generic ref-rules, multiple static key overrides, leaf access (a)", + rules: ruleset3, + ref: "data.overrides_static.p.q.foo.a", + expected: types.S, + }, + { + note: "generic ref-rules, multiple static key overrides, leaf access (b)", + rules: ruleset3, + ref: "data.overrides_static.p.q.bar.b", + expected: types.N, + }, + { + note: "generic ref-rules, multiple static key overrides, leaf access (c)", + rules: ruleset3, + ref: "data.overrides_static.p.q.baz.c", + expected: types.B, + }, } for _, tc := range tests { diff --git a/ast/env.go b/ast/env.go index 6d365378fe..384a31218d 100644 --- a/ast/env.go +++ b/ast/env.go @@ -338,10 +338,26 @@ func (n *typeTreeNode) Insert(path Ref, tpe types.Type) { if curr.value != nil { if tpeObj, ok := tpe.(*types.Object); ok { - // FIXME: Merge objects differently to preserve static and dynamic parts(?) - typeK := types.Or(types.Keys(curr.value), types.Keys(tpeObj)) - typeV := types.Or(types.Values(curr.value), types.Values(tpeObj)) - tpe = types.NewObject(nil, types.NewDynamicProperty(typeK, typeV)) + if currObj, ok := curr.value.(*types.Object); ok { + tpe = tpeObj.Merge(currObj) + } else { + var typeK types.Type + var typeV types.Type + dynProps := tpeObj.DynamicProperties() + if dynProps != nil { + typeK = types.Or(types.Keys(curr.value), dynProps.Key) + typeV = types.Or(types.Values(curr.value), dynProps.Value) + dynProps = types.NewDynamicProperty(typeK, typeV) + } else { + typeK = types.Keys(curr.value) + typeV = types.Values(curr.value) + if typeK != nil && typeV != nil { + dynProps = types.NewDynamicProperty(typeK, typeV) + } + } + + tpe = types.NewObject(tpeObj.StaticProperties(), dynProps) + } } else if tpeSet, ok := tpe.(*types.Set); ok { typeK := types.Or(types.Keys(curr.value), tpeSet.Of()) tpe = types.NewSet(typeK) diff --git a/ast/term.go b/ast/term.go index c4f560f6e2..e89366aa74 100644 --- a/ast/term.go +++ b/ast/term.go @@ -1043,6 +1043,14 @@ func (ref Ref) GroundPrefix() Ref { return prefix } +func (ref Ref) DynamicSuffix() Ref { + i := ref.Dynamic() + if i < 0 { + return nil + } + return ref[i:] +} + // IsGround returns true if all of the parts of the Ref are ground. func (ref Ref) IsGround() bool { if len(ref) == 0 { diff --git a/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml b/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml index 7bae8703c8..03863e7248 100644 --- a/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml +++ b/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml @@ -33,19 +33,18 @@ cases: 1: true c: 2: true -# FIXME: Below test case returns: rego_type_error: undefined ref: data.test.p.b -#- note: 'refheads/arbitrary-var, deep query' -# env: -# OPA_ENABLE_GENERAL_RULE_REFS: "true" -# modules: -# - | -# package test -# -# p[q][r] { q := ["a", "b", "c"][r] } -# query: data.test.p.b = x -# want_result: -# - x: -# 1: true +- note: 'refheads/arbitrary-var, deep query' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q][r] { q := ["a", "b", "c"][r] } + query: data.test.p.b = x + want_result: + - x: + 1: true - note: 'refheads/arbitrary-var, overlapping rule, no conflict' env: OPA_ENABLE_GENERAL_RULE_REFS: "true" diff --git a/types/types.go b/types/types.go index bc2a50b4c1..af8981a81a 100644 --- a/types/types.go +++ b/types/types.go @@ -430,6 +430,33 @@ func (t *Object) Select(name interface{}) Type { return nil } +// TODO: Merge sub-objects instead of putting them all in a types.Or +func (t *Object) Merge(other *Object) *Object { + typeK := Or(t.DynamicProperties().Key, other.DynamicProperties().Key) + typeV := Or(t.DynamicProperties().Value, other.DynamicProperties().Value) + staticPropsMap := make(map[interface{}]Type) + + for _, sp := range t.StaticProperties() { + staticPropsMap[sp.Key] = sp.Value + } + + for _, sp := range other.StaticProperties() { + currV := staticPropsMap[sp.Key] + if currV != nil { + staticPropsMap[sp.Key] = Or(currV, sp.Value) + } else { + staticPropsMap[sp.Key] = sp.Value + } + } + + var staticProps []*StaticProperty + for k, v := range staticPropsMap { + staticProps = append(staticProps, NewStaticProperty(k, v)) + } + + return NewObject(staticProps, NewDynamicProperty(typeK, typeV)) +} + // Any represents a dynamic type. type Any []Type From 9707e6680235394268e172e65738e3785b642150 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Wed, 5 Jul 2023 17:16:09 +0200 Subject: [PATCH 07/27] Updating tests Signed-off-by: Johan Fylling --- ast/check_test.go | 20 +- .../refheads/test-arbitrary-var-refs.yaml | 91 --------- .../testdata/refheads/test-generic-refs.yaml | 175 ++++++++++++++++++ 3 files changed, 185 insertions(+), 101 deletions(-) delete mode 100644 test/cases/testdata/refheads/test-arbitrary-var-refs.yaml create mode 100644 test/cases/testdata/refheads/test-generic-refs.yaml diff --git a/ast/check_test.go b/ast/check_test.go index 7d486dd399..86cfd2bdfd 100644 --- a/ast/check_test.go +++ b/ast/check_test.go @@ -565,7 +565,7 @@ func TestCheckInferenceRules(t *testing.T) { ), }, { - note: "generic ref-rules, only vars in obj-path, complete obj access", + note: "general ref-rules, only vars in obj-path, complete obj access", rules: ruleset3, ref: "data.simple.p.q", expected: types.NewObject( @@ -579,7 +579,7 @@ func TestCheckInferenceRules(t *testing.T) { ), }, { - note: "generic ref-rules, only vars in obj-path, intermediate obj access", + note: "general ref-rules, only vars in obj-path, intermediate obj access", rules: ruleset3, ref: "data.simple.p.q.b", expected: types.NewObject( @@ -588,13 +588,13 @@ func TestCheckInferenceRules(t *testing.T) { ), }, { - note: "generic ref-rules, only vars in obj-path, leaf access", + note: "general ref-rules, only vars in obj-path, leaf access", rules: ruleset3, ref: "data.simple.p.q.b[1]", expected: types.N, }, { - note: "generic ref-rules, vars and constants in obj-path, complete obj access", + note: "general ref-rules, vars and constants in obj-path, complete obj access", rules: ruleset3, ref: "data.mixed.p.q", expected: types.NewObject( @@ -612,7 +612,7 @@ func TestCheckInferenceRules(t *testing.T) { ), }, { - note: "generic ref-rules, key overrides, complete obj access", + note: "general ref-rules, key overrides, complete obj access", rules: ruleset3, ref: "data.overrides.p.q", expected: types.NewObject( @@ -633,7 +633,7 @@ func TestCheckInferenceRules(t *testing.T) { ), }, { - note: "generic ref-rules, multiple static key overrides, complete obj access", + note: "general ref-rules, multiple static key overrides, complete obj access", rules: ruleset3, ref: "data.overrides_static.p.q", expected: types.NewObject( @@ -648,7 +648,7 @@ func TestCheckInferenceRules(t *testing.T) { ), }, { - note: "generic ref-rules, multiple static key overrides, intermediate obj access", + note: "general ref-rules, multiple static key overrides, intermediate obj access", rules: ruleset3, ref: "data.overrides_static.p.q.foo", expected: types.NewAny( @@ -658,19 +658,19 @@ func TestCheckInferenceRules(t *testing.T) { ), }, { - note: "generic ref-rules, multiple static key overrides, leaf access (a)", + note: "general ref-rules, multiple static key overrides, leaf access (a)", rules: ruleset3, ref: "data.overrides_static.p.q.foo.a", expected: types.S, }, { - note: "generic ref-rules, multiple static key overrides, leaf access (b)", + note: "general ref-rules, multiple static key overrides, leaf access (b)", rules: ruleset3, ref: "data.overrides_static.p.q.bar.b", expected: types.N, }, { - note: "generic ref-rules, multiple static key overrides, leaf access (c)", + note: "general ref-rules, multiple static key overrides, leaf access (c)", rules: ruleset3, ref: "data.overrides_static.p.q.baz.c", expected: types.B, diff --git a/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml b/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml deleted file mode 100644 index 03863e7248..0000000000 --- a/test/cases/testdata/refheads/test-arbitrary-var-refs.yaml +++ /dev/null @@ -1,91 +0,0 @@ -cases: -- note: 'refheads/arbitrary-var, single' - env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" - modules: - - | - package test - - p[q].r := i { q := ["a", "b", "c"][i] } - query: data.test.p = x - want_result: - - x: - a: - r: 0 - b: - r: 1 - c: - r: 2 -- note: 'refheads/arbitrary-var, multiple' - env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" - modules: - - | - package test - - p[q][r] { q := ["a", "b", "c"][r] } - query: data.test.p = x - want_result: - - x: - a: - 0: true - b: - 1: true - c: - 2: true -- note: 'refheads/arbitrary-var, deep query' - env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" - modules: - - | - package test - - p[q][r] { q := ["a", "b", "c"][r] } - query: data.test.p.b = x - want_result: - - x: - 1: true -- note: 'refheads/arbitrary-var, overlapping rule, no conflict' - env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" - modules: - - | - package test - - p[q].r := i { q := ["a", "b", "c"][i] } - p.a.r := 0 - query: data.test.p = x - want_result: - - x: - a: - r: 0 - b: - r: 1 - c: - r: 2 -- note: 'refheads/arbitrary-var, overlapping rule, conflict' - env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" - modules: - - | - package test - - p[q].r := i { q := ["a", "b", "c"][i] } - p.a.r := 42 - query: data.test.p = x - want_error: eval_conflict_error -- note: 'refheads/arbitrary-var, multiple result-set entries' - env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" - modules: - - | - package test - - p.q[r].s := 1 { r := "foo" } - p.q[r].s := 2 { r := "bar" } - query: data.test.p.q[i].s = x - want_result: - - i: foo - x: 1 - - i: bar - x: 2 \ No newline at end of file diff --git a/test/cases/testdata/refheads/test-generic-refs.yaml b/test/cases/testdata/refheads/test-generic-refs.yaml new file mode 100644 index 0000000000..968e7babaf --- /dev/null +++ b/test/cases/testdata/refheads/test-generic-refs.yaml @@ -0,0 +1,175 @@ +cases: + - note: 'refheads/general, single' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q].r := i { q := ["a", "b", "c"][i] } + query: data.test.p = x + want_result: + - x: + a: + r: 0 + b: + r: 1 + c: + r: 2 + - note: 'refheads/general, multiple' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q][r] { q := ["a", "b", "c"][r] } + query: data.test.p = x + want_result: + - x: + a: + 0: true + b: + 1: true + c: + 2: true + - note: 'refheads/general, deep query' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q][r] { q := ["a", "b", "c"][r] } + query: data.test.p.b = x + want_result: + - x: + 1: true + - note: 'refheads/general, overlapping rule, no conflict' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q].r := i { q := ["a", "b", "c"][i] } + p.a.r := 0 + query: data.test.p = x + want_result: + - x: + a: + r: 0 + b: + r: 1 + c: + r: 2 + - note: 'refheads/general, overlapping rule, conflict' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[q].r := i { q := ["a", "b", "c"][i] } + p.a.r := 42 + query: data.test.p = x + want_error: eval_conflict_error + - note: 'refheads/general, set leaf' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + import future.keywords + + p[q].r contains s { + x := ["a", "b", "c"] + q := x[i] + s := x[_] + q != s + } + + p.b.r contains "foo" + query: data.test.p = x + want_result: + - x: + a: + r: [ "b", "c" ] + b: + r: [ "a", "c", "foo" ] + c: + r: [ "a", "b" ] + - note: 'refheads/general, set leaf, deep query' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + import future.keywords + + p[q].r contains s { + x := ["a", "b", "c"] + q := x[i] + s := x[_] + q != s + } + + p.b.r contains "foo" + query: data.test.p.b.r.c = x + want_result: + - x: "c" + - note: 'refheads/general, input var' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p[input.x].r := "foo" + query: data.test.p = x + input: + x: "bar" + want_result: + - x: + bar: + r: "foo" + - note: 'refheads/general, external non-ground var' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + a := [x | x := input.x[_]] + b := input.y + + p[a[b]].r[s] := i { + s := a[i] + } + query: data.test.p = x + input: + x: [ "foo", "bar", "baz" ] + "y": 1 + want_result: + - x: + bar: + r: + foo: 0 + bar: 1 + baz: 2 +# FIXME: We can't do complex queries in these tests? + - note: 'refheads/general, multiple result-set entries' + env: + OPA_ENABLE_GENERAL_RULE_REFS: "true" + modules: + - | + package test + + p.q[r].s := 1 { r := "foo" } + p.q[r].s := 2 { r := "bar" } + query: data.test.p.q[i].s = x + want_result: + - i: foo + x: 1 + - i: bar + x: 2 \ No newline at end of file From 391d4694ef613d264269dd562cb2b099973621df Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Wed, 5 Jul 2023 17:22:57 +0200 Subject: [PATCH 08/27] Generalizing `types.Object.Merge()`` Signed-off-by: Johan Fylling --- ast/env.go | 21 +------------------ types/types.go | 55 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/ast/env.go b/ast/env.go index 384a31218d..e7b1ee9f77 100644 --- a/ast/env.go +++ b/ast/env.go @@ -338,26 +338,7 @@ func (n *typeTreeNode) Insert(path Ref, tpe types.Type) { if curr.value != nil { if tpeObj, ok := tpe.(*types.Object); ok { - if currObj, ok := curr.value.(*types.Object); ok { - tpe = tpeObj.Merge(currObj) - } else { - var typeK types.Type - var typeV types.Type - dynProps := tpeObj.DynamicProperties() - if dynProps != nil { - typeK = types.Or(types.Keys(curr.value), dynProps.Key) - typeV = types.Or(types.Values(curr.value), dynProps.Value) - dynProps = types.NewDynamicProperty(typeK, typeV) - } else { - typeK = types.Keys(curr.value) - typeV = types.Values(curr.value) - if typeK != nil && typeV != nil { - dynProps = types.NewDynamicProperty(typeK, typeV) - } - } - - tpe = types.NewObject(tpeObj.StaticProperties(), dynProps) - } + tpe = tpeObj.Merge(curr.value) } else if tpeSet, ok := tpe.(*types.Set); ok { typeK := types.Or(types.Keys(curr.value), tpeSet.Of()) tpe = types.NewSet(typeK) diff --git a/types/types.go b/types/types.go index af8981a81a..83fc0e16f5 100644 --- a/types/types.go +++ b/types/types.go @@ -431,30 +431,49 @@ func (t *Object) Select(name interface{}) Type { } // TODO: Merge sub-objects instead of putting them all in a types.Or -func (t *Object) Merge(other *Object) *Object { - typeK := Or(t.DynamicProperties().Key, other.DynamicProperties().Key) - typeV := Or(t.DynamicProperties().Value, other.DynamicProperties().Value) - staticPropsMap := make(map[interface{}]Type) +func (t *Object) Merge(other Type) *Object { + if otherObj, ok := other.(*Object); ok { + typeK := Or(t.dynamic.Key, otherObj.dynamic.Key) + typeV := Or(t.dynamic.Value, otherObj.dynamic.Value) + staticPropsMap := make(map[interface{}]Type) - for _, sp := range t.StaticProperties() { - staticPropsMap[sp.Key] = sp.Value - } + for _, sp := range t.static { + staticPropsMap[sp.Key] = sp.Value + } + + for _, sp := range otherObj.static { + currV := staticPropsMap[sp.Key] + if currV != nil { + staticPropsMap[sp.Key] = Or(currV, sp.Value) + } else { + staticPropsMap[sp.Key] = sp.Value + } + } - for _, sp := range other.StaticProperties() { - currV := staticPropsMap[sp.Key] - if currV != nil { - staticPropsMap[sp.Key] = Or(currV, sp.Value) + var staticProps []*StaticProperty + for k, v := range staticPropsMap { + staticProps = append(staticProps, NewStaticProperty(k, v)) + } + + return NewObject(staticProps, NewDynamicProperty(typeK, typeV)) + } else { + var typeK Type + var typeV Type + dynProps := t.DynamicProperties() + if dynProps != nil { + typeK = Or(Keys(other), dynProps.Key) + typeV = Or(Values(other), dynProps.Value) + dynProps = NewDynamicProperty(typeK, typeV) } else { - staticPropsMap[sp.Key] = sp.Value + typeK = Keys(other) + typeV = Values(other) + if typeK != nil && typeV != nil { + dynProps = NewDynamicProperty(typeK, typeV) + } } - } - var staticProps []*StaticProperty - for k, v := range staticPropsMap { - staticProps = append(staticProps, NewStaticProperty(k, v)) + return NewObject(t.StaticProperties(), dynProps) } - - return NewObject(staticProps, NewDynamicProperty(typeK, typeV)) } // Any represents a dynamic type. From ea52a013bf4886e71bf9527c6b8d431fe4f8b331 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Wed, 5 Jul 2023 18:11:26 +0200 Subject: [PATCH 09/27] Merging sub-objects in `types.Object.Merge()` instead of `Or`:ing them Signed-off-by: Johan Fylling --- ast/check_test.go | 20 +++++++------ types/types.go | 73 +++++++++++++++++++++++++++++++---------------- 2 files changed, 61 insertions(+), 32 deletions(-) diff --git a/ast/check_test.go b/ast/check_test.go index 86cfd2bdfd..249b98aa63 100644 --- a/ast/check_test.go +++ b/ast/check_test.go @@ -639,10 +639,12 @@ func TestCheckInferenceRules(t *testing.T) { expected: types.NewObject( []*types.StaticProperty{}, types.NewDynamicProperty(types.S, - types.NewAny( - types.NewObject([]*types.StaticProperty{types.NewStaticProperty("a", types.S)}, nil), - types.NewObject([]*types.StaticProperty{types.NewStaticProperty("b", types.N)}, nil), - types.NewObject([]*types.StaticProperty{types.NewStaticProperty("c", types.B)}, nil), + types.NewObject( + []*types.StaticProperty{ + types.NewStaticProperty("a", types.S), + types.NewStaticProperty("b", types.N), + types.NewStaticProperty("c", types.B)}, + nil, ), ), ), @@ -651,10 +653,12 @@ func TestCheckInferenceRules(t *testing.T) { note: "general ref-rules, multiple static key overrides, intermediate obj access", rules: ruleset3, ref: "data.overrides_static.p.q.foo", - expected: types.NewAny( - types.NewObject([]*types.StaticProperty{types.NewStaticProperty("a", types.S)}, nil), - types.NewObject([]*types.StaticProperty{types.NewStaticProperty("b", types.N)}, nil), - types.NewObject([]*types.StaticProperty{types.NewStaticProperty("c", types.B)}, nil), + expected: types.NewObject( + []*types.StaticProperty{ + types.NewStaticProperty("a", types.S), + types.NewStaticProperty("b", types.N), + types.NewStaticProperty("c", types.B)}, + nil, ), }, { diff --git a/types/types.go b/types/types.go index 83fc0e16f5..b61a0eb2e4 100644 --- a/types/types.go +++ b/types/types.go @@ -430,32 +430,9 @@ func (t *Object) Select(name interface{}) Type { return nil } -// TODO: Merge sub-objects instead of putting them all in a types.Or func (t *Object) Merge(other Type) *Object { if otherObj, ok := other.(*Object); ok { - typeK := Or(t.dynamic.Key, otherObj.dynamic.Key) - typeV := Or(t.dynamic.Value, otherObj.dynamic.Value) - staticPropsMap := make(map[interface{}]Type) - - for _, sp := range t.static { - staticPropsMap[sp.Key] = sp.Value - } - - for _, sp := range otherObj.static { - currV := staticPropsMap[sp.Key] - if currV != nil { - staticPropsMap[sp.Key] = Or(currV, sp.Value) - } else { - staticPropsMap[sp.Key] = sp.Value - } - } - - var staticProps []*StaticProperty - for k, v := range staticPropsMap { - staticProps = append(staticProps, NewStaticProperty(k, v)) - } - - return NewObject(staticProps, NewDynamicProperty(typeK, typeV)) + return mergeObjects(t, otherObj) } else { var typeK Type var typeV Type @@ -476,6 +453,54 @@ func (t *Object) Merge(other Type) *Object { } } +func mergeObjects(a, b *Object) *Object { + var dynamicProps *DynamicProperty + if a.dynamic != nil && b.dynamic != nil { + typeK := Or(a.dynamic.Key, b.dynamic.Key) + var typeV Type + aObj, aIsObj := a.dynamic.Value.(*Object) + bObj, bIsObj := b.dynamic.Value.(*Object) + if aIsObj && bIsObj { + typeV = mergeObjects(aObj, bObj) + } else { + typeV = Or(a.dynamic.Value, b.dynamic.Value) + } + dynamicProps = NewDynamicProperty(typeK, typeV) + } else if a.dynamic != nil { + dynamicProps = a.dynamic + } else { + dynamicProps = b.dynamic + } + + staticPropsMap := make(map[interface{}]Type) + + for _, sp := range a.static { + staticPropsMap[sp.Key] = sp.Value + } + + for _, sp := range b.static { + currV := staticPropsMap[sp.Key] + if currV != nil { + currVObj, currVIsObj := currV.(*Object) + spVObj, spVIsObj := sp.Value.(*Object) + if currVIsObj && spVIsObj { + staticPropsMap[sp.Key] = mergeObjects(currVObj, spVObj) + } else { + staticPropsMap[sp.Key] = Or(currV, sp.Value) + } + } else { + staticPropsMap[sp.Key] = sp.Value + } + } + + var staticProps []*StaticProperty + for k, v := range staticPropsMap { + staticProps = append(staticProps, NewStaticProperty(k, v)) + } + + return NewObject(staticProps, dynamicProps) +} + // Any represents a dynamic type. type Any []Type From 996cdec02615976e261985637ee8ef19b218f6c5 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Mon, 10 Jul 2023 14:03:46 +0200 Subject: [PATCH 10/27] Removing comment in test Signed-off-by: Johan Fylling --- test/cases/testdata/refheads/test-generic-refs.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/test/cases/testdata/refheads/test-generic-refs.yaml b/test/cases/testdata/refheads/test-generic-refs.yaml index 968e7babaf..85fee28db2 100644 --- a/test/cases/testdata/refheads/test-generic-refs.yaml +++ b/test/cases/testdata/refheads/test-generic-refs.yaml @@ -157,7 +157,6 @@ cases: foo: 0 bar: 1 baz: 2 -# FIXME: We can't do complex queries in these tests? - note: 'refheads/general, multiple result-set entries' env: OPA_ENABLE_GENERAL_RULE_REFS: "true" From 1065755d929ec776336863e6f804fc7dba7d8626 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Wed, 5 Jul 2023 17:16:09 +0200 Subject: [PATCH 11/27] Updating tests Signed-off-by: Johan Fylling --- internal/wasm/sdk/test/e2e/exceptions.yaml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/wasm/sdk/test/e2e/exceptions.yaml b/internal/wasm/sdk/test/e2e/exceptions.yaml index baaba81c0d..115559285f 100644 --- a/internal/wasm/sdk/test/e2e/exceptions.yaml +++ b/internal/wasm/sdk/test/e2e/exceptions.yaml @@ -3,8 +3,8 @@ "data/nested integer": "https://github.com/open-policy-agent/opa/issues/3711" "withkeyword/function: indirect call, arity 1, replacement is value that needs eval (array comprehension)": "https://github.com/open-policy-agent/opa/issues/5311" "withkeyword/builtin: indirect call, arity 1, replacement is value that needs eval (array comprehension)": "https://github.com/open-policy-agent/opa/issues/5311" -"refheads/arbitrary-var, single": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" -"refheads/arbitrary-var, multiple": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" -"refheads/arbitrary-var, overlapping rule, no conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" -"refheads/arbitrary-var, overlapping rule, conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" -"refheads/arbitrary-var, multiple result-set entries": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" \ No newline at end of file +"refheads/general, single": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, multiple": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, overlapping rule, no conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, overlapping rule, conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, multiple result-set entries": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" \ No newline at end of file From 67ba943e56712760049022626d637e6b0263e04a Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 11 Jul 2023 15:54:39 +0200 Subject: [PATCH 12/27] Aligning hint-key and entry for general refs on virtual partial eval Signed-off-by: Johan Fylling --- topdown/eval.go | 13 ++++++++++++- topdown/eval_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/topdown/eval.go b/topdown/eval.go index 8ded12c15d..d079c4a130 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -2402,10 +2402,21 @@ func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error return err } } - e.e.virtualCache.Put(hint.key, result) + if hint.key != nil { + pluggedKey := e.bindings.Plug(e.ref[e.pos+1]) + if pluggedKey.IsGround() { + hintResult := result.Get(pluggedKey) + if hintResult != nil { + e.e.virtualCache.Put(hint.key, hintResult) + } + } + } return e.evalTerm(iter, e.pos+1, result, e.bindings) } + // FIXME: It should be possible to skip this loop, and always apply the dymanic ref case above. + // FIXME: Tracing breaks: TestFilterTraceDefault + // FIXME: PE fails: TestTopDownPartialEval for _, rule := range e.ir.Rules { if err := e.evalOneRulePreUnify(iter, rule, hint, result, unknown); err != nil { return err diff --git a/topdown/eval_test.go b/topdown/eval_test.go index 750de2317f..24769e9cda 100644 --- a/topdown/eval_test.go +++ b/topdown/eval_test.go @@ -247,6 +247,9 @@ func TestContainsNestedRefOrCall(t *testing.T) { } func TestTopdownVirtualCache(t *testing.T) { + // TODO: break out into separate tests + t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + ctx := context.Background() store := inmem.New() @@ -327,6 +330,35 @@ func TestTopdownVirtualCache(t *testing.T) { hit: 1, miss: 1, }, + { + note: "partial object: simple, query into value", + module: `package p + s["foo"] = { "x": 42, "y": 43 } { true } + s["bar"] = { "x": 42, "y": 43 } { true }`, + query: `data.p.s["foo"].x = x; data.p.s["foo"].y`, + hit: 1, + miss: 1, + exp: 42, + }, + { + note: "partial object: simple, general ref", + module: `package p + s.t[u].v = true { x = ["foo", "bar"]; u = x[_] }`, + query: `data.p.s.t["foo"].v = x; data.p.s.t["foo"].v`, + hit: 1, + miss: 1, + exp: true, + }, + { + note: "partial object: simple, query into value", + module: `package p + s["foo"].t = { "x": 42, "y": 43 } { true } + s["bar"].t = { "x": 42, "y": 43 } { true }`, + query: `data.p.s["foo"].t.x = x; data.p.s["foo"].t.x`, + hit: 1, + miss: 1, + exp: 42, + }, { note: "partial set: simple", module: `package p From 8ffcf1dd38b68a754679b9634c9f3bdf28203fdb Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Thu, 10 Aug 2023 17:27:38 +0200 Subject: [PATCH 13/27] Enforcing no-else parser rule on general ref heads Signed-off-by: Johan Fylling --- ast/parser.go | 2 +- ast/parser_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/ast/parser.go b/ast/parser.go index de0ca1c783..ede432ae85 100644 --- a/ast/parser.go +++ b/ast/parser.go @@ -704,7 +704,7 @@ func (p *Parser) parseRules() []*Rule { } if p.s.tok == tokens.Else { - if r := rule.Head.Ref(); len(r) > 1 && !r[len(r)-1].Value.IsGround() { + if r := rule.Head.Ref(); len(r) > 1 && !r.IsGround() { p.error(p.s.Loc(), "else keyword cannot be used on rules with variables in head") return nil } diff --git a/ast/parser_test.go b/ast/parser_test.go index 11d522fd9c..b4cb9951a9 100644 --- a/ast/parser_test.go +++ b/ast/parser_test.go @@ -2536,6 +2536,14 @@ else := 2 rule: ` a.b[x] := 1 if false else := 2 +`, + err: "else keyword cannot be used on rules with variables in head", + }, + { + note: "single-value general ref head with var", + rule: ` +a.b[x].c := 1 if false +else := 2 `, err: "else keyword cannot be used on rules with variables in head", }, From f02441d82bcfa8e220f6c894dc42c1b90dedef02 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Mon, 14 Aug 2023 17:11:20 +0200 Subject: [PATCH 14/27] PE impl Signed-off-by: Johan Fylling --- ast/policy.go | 4 +- internal/wasm/sdk/test/e2e/exceptions.yaml | 4 +- .../testdata/refheads/test-generic-refs.yaml | 8 +- topdown/eval.go | 135 +++-- topdown/eval_test.go | 468 +++++++++++++++--- topdown/topdown_partial_test.go | 158 ++++++ 6 files changed, 664 insertions(+), 113 deletions(-) diff --git a/ast/policy.go b/ast/policy.go index 01790926bf..2822b82d87 100644 --- a/ast/policy.go +++ b/ast/policy.go @@ -977,8 +977,8 @@ func (head *Head) SetLoc(loc *Location) { func (head *Head) HasDynamicRef() bool { pos := head.Reference.Dynamic() - // Ref is dynamic if it has one non-constant term that isn't the first or last term. - return pos > 0 && pos < len(head.Reference)-1 + // Ref is dynamic if it has one non-constant term that isn't the first or last term or if it's a partial set rule. + return pos > 0 && (pos < len(head.Reference)-1 || head.RuleKind() == MultiValue) } // Copy returns a deep copy of a. diff --git a/internal/wasm/sdk/test/e2e/exceptions.yaml b/internal/wasm/sdk/test/e2e/exceptions.yaml index 115559285f..51c1b219b0 100644 --- a/internal/wasm/sdk/test/e2e/exceptions.yaml +++ b/internal/wasm/sdk/test/e2e/exceptions.yaml @@ -3,8 +3,8 @@ "data/nested integer": "https://github.com/open-policy-agent/opa/issues/3711" "withkeyword/function: indirect call, arity 1, replacement is value that needs eval (array comprehension)": "https://github.com/open-policy-agent/opa/issues/5311" "withkeyword/builtin: indirect call, arity 1, replacement is value that needs eval (array comprehension)": "https://github.com/open-policy-agent/opa/issues/5311" -"refheads/general, single": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" -"refheads/general, multiple": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, single var": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, multiple vars": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/general, overlapping rule, no conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/general, overlapping rule, conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/general, multiple result-set entries": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" \ No newline at end of file diff --git a/test/cases/testdata/refheads/test-generic-refs.yaml b/test/cases/testdata/refheads/test-generic-refs.yaml index 85fee28db2..a0dae45f8c 100644 --- a/test/cases/testdata/refheads/test-generic-refs.yaml +++ b/test/cases/testdata/refheads/test-generic-refs.yaml @@ -1,5 +1,5 @@ cases: - - note: 'refheads/general, single' + - note: 'refheads/general, single var' env: OPA_ENABLE_GENERAL_RULE_REFS: "true" modules: @@ -16,7 +16,7 @@ cases: r: 1 c: r: 2 - - note: 'refheads/general, multiple' + - note: 'refheads/general, multiple vars' env: OPA_ENABLE_GENERAL_RULE_REFS: "true" modules: @@ -84,7 +84,7 @@ cases: p[q].r contains s { x := ["a", "b", "c"] - q := x[i] + q := x[_] s := x[_] q != s } @@ -109,7 +109,7 @@ cases: p[q].r contains s { x := ["a", "b", "c"] - q := x[i] + q := x[_] s := x[_] q != s } diff --git a/topdown/eval.go b/topdown/eval.go index d079c4a130..b1d33c2c65 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -23,6 +23,8 @@ type evalIterator func(*eval) error type unifyIterator func() error +type unifyRefIterator func(pos int) error + type queryIDFactory struct { curr uint64 } @@ -185,7 +187,7 @@ func (e *eval) unknown(x interface{}, b *bindings) bool { x = ast.NewTerm(v) } - return saveRequired(e.compiler, e.inliningControl, true, e.saveSet, b, x, false) + return saveRequired(e.compiler, e.inliningControl, true, e.saveSet, b, x, false) // TODO: update for general refs? } func (e *eval) traceEnter(x ast.Node) { @@ -2369,7 +2371,9 @@ func (e evalVirtualPartial) eval(iter unifyIterator) error { func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error { + // FIXME: Check if any part of ref[e.pos+1:] is unknown? if e.e.unknown(e.ref[e.pos+1], e.bindings) { + // Queried rule key is unknown for _, rule := range e.ir.Rules { if err := e.evalOneRulePostUnify(iter, rule); err != nil { return err @@ -2395,26 +2399,30 @@ func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error } result := e.empty + if e.ir.DynamicRef { for _, rule := range e.ir.Rules { - result, err = e.evalOneDynamicRefRulePreUnify(rule, result, unknown) + result, err = e.evalOneDynamicRefRulePreUnify(iter, rule, hint, result, unknown) if err != nil { return err } } if hint.key != nil { pluggedKey := e.bindings.Plug(e.ref[e.pos+1]) - if pluggedKey.IsGround() { + if pluggedKey.IsGround() { // FIXME: This should be unnecessary hintResult := result.Get(pluggedKey) if hintResult != nil { e.e.virtualCache.Put(hint.key, hintResult) } } } - return e.evalTerm(iter, e.pos+1, result, e.bindings) + if !unknown { + return e.evalTerm(iter, e.pos+1, result, e.bindings) + } + return nil } - // FIXME: It should be possible to skip this loop, and always apply the dymanic ref case above. + // FIXME: It should be possible to skip this loop, and always apply the dynamic ref case above. // FIXME: Tracing breaks: TestFilterTraceDefault // FIXME: PE fails: TestTopDownPartialEval for _, rule := range e.ir.Rules { @@ -2541,29 +2549,82 @@ func (e evalVirtualPartial) evalOneRulePreUnify(iter unifyIterator, rule *ast.Ru return nil } -func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(rule *ast.Rule, result *ast.Term, unknown bool) (*ast.Term, error) { +func wrapInObjects(val *ast.Term, ref ast.Ref) *ast.Term { + if len(ref) == 0 { + return val + } + obj := ast.NewObject() + current := obj + for i := 0; i < len(ref); i++ { + key := ref[i] + if i == len(ref)-1 { + current.Insert(key, val) + } else { + next := ast.NewObject() + current.Insert(key, ast.NewTerm(next)) + current = next + } + } + return ast.NewTerm(obj) +} + +func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(iter unifyIterator, rule *ast.Rule, hint evalVirtualPartialCacheHint, result *ast.Term, unknown bool) (*ast.Term, error) { child := e.e.child(rule.Body) child.traceEnter(rule) var defined bool - // Walk the dynamic portion of rule ref to unify vars - err := child.biunifyDynamicRef(e.pos+1, rule.Ref(), e.ref, child.bindings, e.bindings, func() error { + headKey := rule.Head.Key + if headKey == nil { + headKey = rule.Head.Reference[len(rule.Head.Reference)-1] + } + + // Walk the dynamic portion of rule ref and key to unify vars + err := child.biunifyRuleHead(e.pos+1, e.ref, rule, e.bindings, child.bindings, func(pos int) error { defined = true return child.eval(func(child *eval) error { + child.traceExit(rule) - var dup bool - var err error - result, dup, err = e.reduce(rule, child.bindings, result) - if err != nil { - return err - } else if !unknown && dup { - child.traceDuplicate(rule) - return nil + + term := rule.Head.Value + if term == nil { + term = headKey + } + + if unknown { + fullPath := rule.Ref() + // FIXME: Anchor at pos instead of e.pos+1 (?) + objPath := fullPath[e.pos+1 : len(fullPath)-1] // the portion of the ref that generates nested objects + leafKey := child.bindings.Plug(fullPath[len(fullPath)-1]) // the portion of the ref that is the deepest nested key for the value + + obj := ast.NewObject() + // FIXME: getNestedObject() will plug bindings for each key, is this necessary as evalTerm() will apply them? + leafObj, err := getNestedObject(objPath, &obj, child.bindings, rule.Head.Location) + if err != nil { + return err + } + (*leafObj).Insert(leafKey, term) + + term, termbindings := child.bindings.apply(ast.NewTerm(obj)) + err = e.evalTerm(iter, e.pos+1, term, termbindings) + if err != nil { + return err + } + } else { + var dup bool + var err error + result, dup, err = e.reduce(rule, child.bindings, result) + if err != nil { + return err + } else if !unknown && dup { + child.traceDuplicate(rule) + return nil + } } child.traceRedo(rule) + return nil }) }) @@ -2580,9 +2641,25 @@ func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(rule *ast.Rule, result return result, nil } -func (e *eval) biunifyDynamicRef(pos int, a, b ast.Ref, b1, b2 *bindings, iter unifyIterator) error { +func (e *eval) biunifyRuleHead(pos int, ref ast.Ref, rule *ast.Rule, refBindings, ruleBindings *bindings, iter unifyRefIterator) error { + return e.biunifyDynamicRef(pos, ref, rule.Ref(), refBindings, ruleBindings, func(pos int) error { + // FIXME: Is there a simpler, more robust way of figuring out that we should biunify the rule key? + if rule.Head.RuleKind() == ast.MultiValue && pos < len(ref) && len(rule.Ref()) <= len(ref) { + headKey := rule.Head.Key + if headKey == nil { + headKey = rule.Head.Reference[len(rule.Head.Reference)-1] + } + return e.biunify(ref[pos], headKey, refBindings, ruleBindings, func() error { + return iter(pos + 1) + }) + } + return iter(pos) + }) +} + +func (e *eval) biunifyDynamicRef(pos int, a, b ast.Ref, b1, b2 *bindings, iter unifyRefIterator) error { if pos >= len(a) || pos >= len(b) { - return iter() + return iter(pos) } return e.biunify(a[pos], b[pos], b1, b2, func() error { @@ -2591,12 +2668,6 @@ func (e *eval) biunifyDynamicRef(pos int, a, b ast.Ref, b1, b2 *bindings, iter u } func (e evalVirtualPartial) evalOneRulePostUnify(iter unifyIterator, rule *ast.Rule) error { - headKey := rule.Head.Key - if headKey == nil { - headKey = rule.Head.Reference[len(rule.Head.Reference)-1] - } - - key := e.ref[e.pos+1] child := e.e.child(rule.Body) child.traceEnter(rule) @@ -2604,7 +2675,7 @@ func (e evalVirtualPartial) evalOneRulePostUnify(iter unifyIterator, rule *ast.R err := child.eval(func(child *eval) error { defined = true - return e.e.biunify(headKey, key, child.bindings, e.bindings, func() error { + return e.e.biunifyRuleHead(e.pos+1, e.ref, rule, e.bindings, child.bindings, func(pos int) error { return e.evalOneRuleContinue(iter, rule, child) }) }) @@ -2629,8 +2700,15 @@ func (e evalVirtualPartial) evalOneRuleContinue(iter unifyIterator, rule *ast.Ru term = rule.Head.Key } + if rule.Head.RuleKind() == ast.MultiValue { + term = ast.SetTerm(term) + } + + objRef := rule.Ref()[e.pos+1:] + term = wrapInObjects(term, objRef) + term, termbindings := child.bindings.apply(term) - err := e.evalTerm(iter, e.pos+2, term, termbindings) + err := e.evalTerm(iter, e.pos+1, term, termbindings) if err != nil { return err } @@ -2712,10 +2790,10 @@ func (e evalVirtualPartial) partialEvalSupportRule(rule *ast.Rule, path ast.Ref) ruleRef[i] = child.bindings.plugNamespaced(ruleRef[i], e.e.caller.bindings) } head.Reference = ruleRef - if head.Name.Equal(ast.Var("")) { + if (len(ruleRef) == 1 || len(ruleRef) == 2) && head.Name.Equal(ast.Var("")) { head.Name = ruleRef[0].Value.(ast.Var) } - if len(ruleRef) > 1 && head.Key == nil { + if len(ruleRef) == 2 && head.Key == nil { head.Key = ruleRef[len(ruleRef)-1] } } @@ -3090,6 +3168,7 @@ func (e evalVirtualComplete) partialEvalSupportRule(rule *ast.Rule, path ast.Ref s := ref[len(ref)-1].Value.(ast.String) name = ast.Var(s) } + // TODO: Do we need to deal with general refs here? head := ast.NewHead(name, nil, child.bindings.PlugNamespaced(rule.Head.Value, e.e.caller.bindings)) if !e.e.inliningControl.shallow { diff --git a/topdown/eval_test.go b/topdown/eval_test.go index 24769e9cda..6ad8518f01 100644 --- a/topdown/eval_test.go +++ b/topdown/eval_test.go @@ -522,7 +522,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"test": {"p": {"foo": 42}}}}]`, }, { - note: "partial object (dots in head)", + note: "ref head", module: `package test p.foo := v { v := 42 @@ -532,7 +532,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"test": {"p": {"foo": 42}}}}]`, }, { - note: "partial object (dots and var in head)", + note: "partial object (ref head)", module: `package test p.q.r[i] := v { v := ["a", "b", "c"][i] @@ -542,7 +542,28 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"test": {"p": {"q": {"r": {"0": "a", "1": "b", "2": "c"}}}}}}]`, }, { - note: "partial object (dots in head and implicit 'true' value)", + note: "partial object (ref head), query to obj root", + module: `package test + p.q.r[i] := v { + v := ["a", "b", "c"][i] + } + `, + query: `data.test.p.q.r = x`, + exp: `[{"x": {"0": "a", "1": "b", "2": "c"}}]`, + }, + { + note: "partial object (ref head), query to obj root, enumerating keys", + module: `package test + p.q.r[i] := v { + v := ["a", "b", "c"][i] + } + `, + query: `data.test.p.q.r[x]`, + // NOTE: $_term_0_0 wildcard var is filtered from eval result output + exp: `[{"x": 0, "$_term_0_0": "a"}, {"x": 1, "$_term_0_0": "b"}, {"x": 2, "$_term_0_0": "c"}]`, + }, + { + note: "partial object (ref head), implicit 'true' value", module: `package test p.q.r[v] { v := [1, 2, 3][_] @@ -552,7 +573,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"test": {"p": {"q": {"r": {"1": true, "2": true, "3": true}}}}}}]`, }, { - note: "partial set (dots in head)", + note: "partial set (ref head)", module: `package test import future.keywords p.q contains v if { @@ -563,19 +584,19 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"test": {"p": {"q": [1, 2, 3]}}}}]`, }, { - note: "partial set (var in head)", + note: "partial set (general ref head)", module: `package test import future.keywords - p[q] contains v if { - q := "foo" + p[j] contains v if { v := [1, 2, 3][_] + j := ["a", "b", "c"][_] } `, query: `data = x`, - exp: `[{"x": {"test": {"p": {"foo": [1, 2, 3]}}}}]`, + exp: `[{"x": {"test": {"p": {"a": [1, 2, 3], "b": [1, 2, 3], "c": [1, 2, 3]}}}}]`, }, { - note: "partial set (dots and vars in head)", + note: "partial set (general ref head, static suffix)", module: `package test import future.keywords p[q].r contains v if { @@ -587,7 +608,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"test": {"p": {"foo": {"r": [1, 2, 3]}}}}}]`, }, { - note: "partial object (multiple vars in head ref)", + note: "partial object (general ref head, multiple vars)", module: `package test p.q[x].r[i] := v { some i @@ -599,7 +620,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"test": {"p": {"q": {"a": {"r": {"0": 1, "1": 2, "2": 3}}, "b": {"r": {"0": 1, "1": 2, "2": 3}}, "c": {"r": {"0": 1, "1": 2, "2": 3}}}}}}}]`, }, { - note: "partial object (multiple vars in head ref) #2", + note: "partial object (general ref head, multiple vars) #2", module: `package test p[j].foo[i] := v { v := [1, 2, 3][i] @@ -610,19 +631,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"test": {"p": {"a": {"foo": {"0": 1, "1": 2, "2": 3}}, "b": {"foo": {"0": 1, "1": 2, "2": 3}}, "c": {"foo": {"0": 1, "1": 2, "2": 3}}}}}}]`, }, { - note: "partial object (var in head ref, set leaf)", - module: `package test - import future.keywords - p[j] contains v if { - v := [1, 2, 3][_] - j := ["a", "b", "c"][_] - } - `, - query: `data = x`, - exp: `[{"x": {"test": {"p": {"a": [1, 2, 3], "b": [1, 2, 3], "c": [1, 2, 3]}}}}]`, - }, - { - note: "partial object (multiple vars in head ref, partial set)", + note: "partial set (multiple vars in general ref head)", module: `package test import future.keywords p[j][i] contains v if { @@ -634,37 +643,6 @@ func TestPartialRule(t *testing.T) { query: `data = x`, exp: `[{"x": {"test": {"p": {"a": {"foo": [1, 2, 3]}, "b": {"foo": [1, 2, 3]}, "c": {"foo": [1, 2, 3]}}}}}]`, }, - { - note: "partial object generating conflicting keys", - module: `package test - p[k] := x { - k := "foo" - x := [1, 2][_] - }`, - query: `data = x`, - expErr: "eval_conflict_error: object keys must be unique", - }, - { - note: "partial object generating conflicting keys (dots in head)", - module: `package test - p.q[k] := x { - k := "foo" - x := [1, 2][_] - }`, - query: `data = x`, - expErr: "eval_conflict_error: object keys must be unique", - }, - { - note: "partial object generating conflicting nested keys", - module: `package test - p.q[k].s := x { - k := "foo" - x := [1, 2][_] - }`, - query: `data = x`, - expErr: "eval_conflict_error: object keys must be unique", - }, - // TODO: Add test case with else block // Overlapping rules { note: "partial object with overlapping rule (defining key/value in object)", @@ -826,9 +804,124 @@ func TestPartialRule(t *testing.T) { query: `data = x`, expErr: "eval_conflict_error: object keys must be unique", }, + // NOTE: There is a bit of ambiguity in the parser about when an else-body is permitted. + // Sometimes when successfully parsed, evaluation doesn't consider the else body, though. + // else bodies + //{ + // note: "partial object with else block on key override rule (else body defined)", + // module: `package test + // p[x] := i { + // x := ["foo", "bar"][i] + // } + // p.baz := "a" { + // x == 1 + // } else := "b" { + // x == 2 + // } + // x := 2 + // `, + // query: `data = x`, + // exp: `[{"x": {"test": {"p": {"bar": 1, "baz": "a", "foo": 0}}, "x": 2}}]`, + //}, + //{ + // note: "partial object (ref head) with else block on key override rule (primary body defined)", + // module: `package test + // p.q[x] := i { + // x := ["foo", "bar"][i] + // } + // p.q.baz := "a" { + // x == 1 + // } else := "b" { + // x == 2 + // } + // x := 1 + // `, + // query: `data = x`, + // exp: `[{"x": {"test": {"p": {"q": {"bar": 1, "baz": "a", "foo": 0}}, "x": 1}}}]`, + //}, + //{ + // note: "partial object (ref head) with else block on key override rule (else body defined)", + // module: `package test + // p.q[x] := i { + // x := ["foo", "bar"][i] + // } + // p.q.baz := "a" { + // x == 1 + // } else := "b" { + // x == 2 + // } + // x := 2 + // `, + // query: `data = x`, + // exp: `[{"x": {"test": {"p": {"q": {"bar": 1, "baz": "a", "foo": 0}}, "x": 2}}}]`, + //}, + //{ + // note: "partial object (ref head) with else block on key override rule (no body defined)", + // module: `package test + // p.q[x] := i { + // x := ["foo", "bar"][i] + // } + // p.q.baz := "a" { + // x == 1 + // } else := "b" { + // x == 2 + // } + // x := 3 + // `, + // query: `data = x`, + // exp: `[{"x": {"test": {"p": {"q": {"bar": 1, "foo": 0}}, "x": 3}}}]`, + //}, + //{ + // note: "partial object (general ref head) with else block on key override rule (primary body defined)", + // module: `package test + // p.q[x].r := i { + // x := ["foo", "bar"][i] + // } + // p.q.baz.s := "a" { + // x == 1 + // } else := "b" { + // x == 2 + // } + // x := 1 + // `, + // query: `data = x`, + // exp: `[{"x": {"test": {"p": {"q": {"bar": {"r": 1}, "baz": {"s": "a"}, "foo": {"r": 0}}}, "x": 1}}}]`, + //}, + //{ + // note: "partial object (general ref head) with else block on key override rule (else body defined)", + // module: `package test + // p.q[x].r := i { + // x := ["foo", "bar"][i] + // } + // p.q.baz.s := "a" { + // x == 1 + // } else := "b" { + // x == 2 + // } + // x := 2 + // `, + // query: `data = x`, + // exp: `[{"x": {"test": {"p": {"q": {"bar": {"r": 1}, "baz": {"s": "b"}, "foo": {"r": 0}}}, "x": 1}}}]`, + //}, + //{ + // note: "partial object (general ref head) with else block on key override rule (no body defined)", + // module: `package test + // p.q[x].r := i { + // x := ["foo", "bar"][i] + // } + // p.q.baz.s := "a" { + // x == 1 + // } else := "b" { + // x == 2 + // } + // x := 3 + // `, + // query: `data = x`, + // exp: `[{"x": {"test": {"p": {"q": {"bar": {"r": 1}, "foo": {"r": 0}}}, "x": 3}}}]`, + //}, // Deep queries { - note: "deep query into partial object", + note: "deep query into partial object (ref head)", module: `package test p.q[r] := 1 { r := "foo" } `, @@ -836,7 +929,98 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": 1}]`, }, { - note: "deep query to, but not into partial set", + note: "deep query into partial object (ref head) and object value", + module: `package test + p.q[r] := x { + r := "foo" + x := {"bar": {"baz": 1}} + } + `, + query: `data.test.p.q.foo.bar = x`, + exp: `[{"x": {"baz": 1}}]`, + }, + { + note: "deep query into partial object starting-point (general ref head) up to array value", + module: `package test + p.q[r].s[t].u := x { + obj := { + "foo": { + "do": ["a", "b", "c"], + "re": ["d", "e", "f"], + }, + "bar": { + "mi": ["g", "h", "i"], + "fa": ["j", "k", "l"], + } + } + x := obj[r][t] + } + `, + query: `data.test.p.q = x`, + exp: `[{"x": {"bar": {"s": {"fa": {"u": ["j", "k", "l"]}, "mi": {"u": ["g", "h", "i"]}}}, "foo": {"s": {"do": {"u": ["a", "b", "c"]}, "re": {"u": ["d", "e", "f"]}}}}}]`, + }, + { + note: "deep query into partial object mid-point (general ref head) up to array value", + module: `package test + p.q[r].s[t].u := x { + obj := { + "foo": { + "do": ["a", "b", "c"], + "re": ["d", "e", "f"], + }, + "bar": { + "mi": ["g", "h", "i"], + "fa": ["j", "k", "l"], + } + } + x := obj[r][t] + } + `, + query: `data.test.p.q.bar.s = x`, + exp: `[{"x": {"fa": {"u": ["j", "k", "l"]}, "mi": {"u": ["g", "h", "i"]}}}]`, + }, + { + note: "deep query into partial object (general ref head) up to array value", + module: `package test + p.q[r].s[t].u := x { + obj := { + "foo": { + "do": ["a", "b", "c"], + "re": ["d", "e", "f"], + }, + "bar": { + "mi": ["g", "h", "i"], + "fa": ["j", "k", "l"], + } + } + x := obj[r][t] + } + `, + query: `data.test.p.q.bar.s.mi.u = x`, + exp: `[{"x": ["g", "h", "i"]}]`, + }, + { + note: "deep query into partial object (general ref head) and array value", + module: `package test + p.q[r].s[t].u := x { + obj := { + "foo": { + "do": ["a", "b", "c"], + "re": ["d", "e", "f"], + }, + "bar": { + "mi": ["g", "h", "i"], + "fa": ["j", "k", "l"], + } + } + x := obj[r][t] + } + `, + query: `data.test.p.q.foo.s.re.u[1] = x`, + exp: `[{"x": "e"}]`, + }, + { + note: "query up to (ref head), but not into partial set", module: `package test import future.keywords p.q.r contains s { {"foo", "bar", "bax"}[s] } @@ -845,7 +1029,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"q": {"r": ["bar", "bax", "foo"]}}}]`, }, { - note: "deep query to, but not into partial set", + note: "deep query up to (ref mid-point), but not into partial set", module: `package test import future.keywords p.q.r contains s { {"foo", "bar", "bax"}[s] } @@ -854,7 +1038,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": {"r": ["bar", "bax", "foo"]}}]`, }, { - note: "deep query to, but not into partial set", + note: "deep query up to (ref tail), but not into partial set", module: `package test import future.keywords p.q.r contains s { {"foo", "bar", "bax"}[s] } @@ -871,28 +1055,98 @@ func TestPartialRule(t *testing.T) { query: `data.test.p.q.foo = x`, exp: `[{"x": "foo"}]`, }, - { - note: "deep query into partial object and object value", + { // enumeration + note: "deep query into partial object and object value, full depth, enumeration on object value", module: `package test p.q[r] := x { - r := "foo" - x := {"bar": {"baz": 1}} + r := ["foo", "bar"][_] + x := {"s": {"do": 0, "re": 1, "mi": 2}} } `, - query: `data.test.p.q.foo.bar = x`, - exp: `[{"x": {"baz": 1}}]`, + query: `data.test.p.q.bar.s[y] = z`, + exp: `[{"y": "do", "z": 0}, {"y": "re", "z": 1}, {"y": "mi", "z": 2}]`, + }, + { // enumeration + note: "deep query into partial object and object value, full depth, enumeration on rule path and object value", + module: `package test + p.q[r] := x { + r := ["foo", "bar"][_] + x := {"s": {"do": 0, "re": 1, "mi": 2}} + } + `, + query: `data.test.p.q[x].s[y] = z`, + exp: `[{"x": "foo", "y": "do", "z": 0}, {"x": "foo", "y": "re", "z": 1}, {"x": "foo", "y": "mi", "z": 2}, {"x": "bar", "y": "do", "z": 0}, {"x": "bar", "y": "re", "z": 1}, {"x": "bar", "y": "mi", "z": 2}]`, }, { - note: "deep query into partial object and set value", + note: "deep query into partial object (ref head) and set value", module: `package test import future.keywords - p.q[r].s contains x { - r := "foo" - {"foo", "bar", "bax"}[x] + p.q contains t { + {"do", "re", "mi"}[t] } `, - query: `data.test.p.q.foo.s.bar = x`, - exp: `[{"x": "bar"}]`, + query: `data.test.p.q.re = x`, + exp: `[{"x": "re"}]`, + }, + { + note: "deep query into partial object (general ref head) and set value", + module: `package test + import future.keywords + p.q[r] contains t { + r := ["foo", "bar"][_] + {"do", "re", "mi"}[t] + } + `, + query: `data.test.p.q.foo.re = x`, + exp: `[{"x": "re"}]`, + }, + { + note: "deep query into partial object (general ref head, static tail) and set value", + module: `package test + import future.keywords + p.q[r].s contains t { + r := ["foo", "bar"][_] + {"do", "re", "mi"}[t] + } + `, + query: `data.test.p.q.foo.s.re = x`, + exp: `[{"x": "re"}]`, + }, + { + note: "deep query into general ref to set value", + module: `package test + import future.keywords + p.q[r].s contains t { + r := ["foo", "bar"][_] + t := ["do", "re", "mi"][_] + } + `, + query: `data.test.p.q.foo.s = x`, + exp: `[{"x": ["do", "mi", "re"]}]`, // FIXME: set ordering makes this test brittle + }, + { + note: "deep query into general ref to object value", + module: `package test + p.q[r].s[t] := u { + r := ["foo", "bar"][_] + t := ["do", "re", "mi"][u] + } + `, + query: `data.test.p.q.foo.s = x`, + exp: `[{"x": {"do": 0, "re": 1, "mi": 2}}]`, + }, + { + note: "deep query into general ref enumerating set values", + module: `package test + import future.keywords + p.q[r].s contains t { + r := ["foo", "bar"][_] + {"do", "re", "mi"}[t] + } + `, + query: `data.test.p.q.foo.s[x]`, + // NOTE: $_term_0_0 wildcard var is filtered from eval result output + exp: `[{"$_term_0_0": "do", "x": "do"}, {"$_term_0_0": "re", "x": "re"}, {"$_term_0_0": "mi", "x": "mi"}]`, }, { note: "deep query into partial object and object value, non-tail var", @@ -922,13 +1176,21 @@ func TestPartialRule(t *testing.T) { exp: `[{"x": 1}]`, }, { - note: "deep query into partial object, on first var in ref, multiple vars", + note: "deep query into partial object, shallow rule ref", module: `package test p.q[r][s] := 1 { r := "foo"; s := "bar" } `, query: `data.test.p.q.foo = x`, exp: `[{"x": {"bar": 1}}]`, }, + { + note: "deep query into partial object, shallow rule ref, multiple keys", + module: `package test + p.q[r][s] := t { l := ["do", "re", "mi"]; r := "foo"; s := l[t] } + `, + query: `data.test.p.q.foo = x`, + exp: `[{"x": {"do": 0, "re": 1, "mi": 2}}]`, + }, { note: "deep query into partial object, beyond first var in ref, multiple vars", module: `package test @@ -999,9 +1261,21 @@ func TestPartialRule(t *testing.T) { query: `data.test.p.q.r = x`, exp: `[{"x": {"s": 1}}]`, }, - // Multiple results + // Multiple results (enumeration) { - note: "query to partial object, overlapping rules, dynamic ref, multiple results", + note: "shallow query into general ref, key enumeration", + module: `package test + p.q[r].s[t] := u { + r := ["a", "b", "c"][_] + t := ["d", "e", "f"][u] + }`, + query: `data.test.p.q[x] = y`, + exp: `[{"x": "a", "y": {"s": {"d": 0, "e": 1, "f": 2}}}, + {"x": "b", "y": {"s": {"d": 0, "e": 1, "f": 2}}}, + {"x": "c", "y": {"s": {"d": 0, "e": 1, "f": 2}}}]`, + }, + { + note: "query to partial object, overlapping rules, dynamic ref, key enumeration", module: `package test p.q[r].s := 1 { r := "foo" } p.q[r].s := 2 { r := "bar" } @@ -1010,7 +1284,7 @@ func TestPartialRule(t *testing.T) { exp: `[{"i": "bar", "x": {"s": 2}}, {"i": "foo", "x": {"s": 1}}]`, }, { - note: "deep query into partial object, overlapping rules, dynamic ref, multiple results", + note: "deep query into partial object, overlapping rules, dynamic ref, key enumeration", module: `package test p.q[r].s := 1 { r := "foo" } p.q[r].s := 2 { r := "bar" } @@ -1018,6 +1292,46 @@ func TestPartialRule(t *testing.T) { query: `data.test.p.q[i].s = x`, exp: `[{"i": "bar", "x": 2}, {"i": "foo", "x": 1}]`, }, + // Errors + { + note: "partial object generating conflicting keys", + module: `package test + p[k] := x { + k := "foo" + x := [1, 2][_] + }`, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object (ref head) generating conflicting keys (dots in head)", + module: `package test + p.q[k] := x { + k := "foo" + x := [1, 2][_] + }`, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object (general ref head) generating conflicting nested keys", + module: `package test + p.q[k].s := x { + k := "foo" + x := [1, 2][_] + }`, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, + { + note: "partial object (general ref head) generating conflicting ref vars", + module: `package test + p.q[k].s := x { + k := ["foo", "foo"][x] + }`, + query: `data = x`, + expErr: "eval_conflict_error: object keys must be unique", + }, } for _, tc := range tests { @@ -1046,8 +1360,8 @@ func TestPartialRule(t *testing.T) { var exp []map[string]interface{} _ = json.Unmarshal([]byte(tc.exp), &exp) - if exp, act := len(exp), len(qrs); exp != act { - t.Fatalf("expected %d query result, got %d query results: %+v", exp, act, qrs) + if expLen, act := len(exp), len(qrs); expLen != act { + t.Fatalf("expected %d query result:\n\n%+v,\n\ngot %d query results:\n\n%+v", expLen, exp, act, qrs) } testAssertResultSet(t, exp, qrs, false) } diff --git a/topdown/topdown_partial_test.go b/topdown/topdown_partial_test.go index 689745f42d..8535bb1cba 100644 --- a/topdown/topdown_partial_test.go +++ b/topdown/topdown_partial_test.go @@ -18,6 +18,8 @@ import ( ) func TestTopDownPartialEval(t *testing.T) { + // TODO: break out into separate tests + t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") tests := []struct { note string @@ -203,6 +205,7 @@ func TestTopDownPartialEval(t *testing.T) { `input.x = "bar"; x = "bar"`, }, }, + // TODO: "single term, general ref: save"(?) { note: "single term: false save", query: `input = x; x = false; x`, // last expression must be preserved @@ -222,6 +225,128 @@ func TestTopDownPartialEval(t *testing.T) { `x = input.x`, }, }, + //{ // FIXME: Just for experimenting, don't keep + // note: "reference: partial object, no query match", + // query: "data.test.p[x].foo = 2", + // modules: []string{ + // `package test + // p[x] = {y: z} { x = input.x; y = "foo"; z = 1 } + // p[x] = {y: z} { x = input.y; y = "bar"; z = 2 }`, + // }, + // wantQueries: []string{ + // `x = input.x`, + // }, + //}, + { + note: "reference: partial object, general ref", + query: "data.test.p[x].q.foo = 1", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 }`, + }, + wantQueries: []string{ + `x = input.a`, + }, + }, + { + note: "reference: partial object, general ref (2)", + query: "data.test.p[x].q.foo", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 }`, + }, + wantQueries: []string{ + `x = input.a`, + }, + }, + { + note: "reference: partial object, general ref (3)", + query: "data.test.p[x].q", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 }`, + }, + wantQueries: []string{ + `x = input.a`, + `x = input.b`, + }, + }, + { + note: "reference: partial object, general ref (4)", + query: "data.test.p[x].q[y]", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 }`, + }, + wantQueries: []string{ + `x = input.a; y = "foo"`, + `x = input.b; y = "bar"`, + }, + }, + { + note: "reference: partial object, general ref (5)", + query: "data.test.p[x].q[y] = z", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 }`, + }, + wantQueries: []string{ + `x = input.a; y = "foo"; z = 1`, + `x = input.b; y = "bar"; z = 2`, + }, + }, + { + note: "reference: partial object, general ref (6)", + query: "data.test.p = z", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 }`, + }, + wantQueries: []string{ + `data.partial.test.p = z`, + }, + wantSupport: []string{ + `package partial.test + p[a2].q = {"foo": 1} { a2 = input.a } + p[a1].q = {"bar": 2} { a1 = input.b }`, + }, + }, + { + note: "reference: partial object, general ref (7)", + query: "data.test.p[x] = z", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 }`, + }, + wantQueries: []string{ + `x = input.a; z = {"q": {"foo": 1}}`, + `x = input.b; z = {"q": {"bar": 2}}`, + }, + }, + { + note: "reference: partial object, general ref (8)", + query: "data.test.p[x] = z", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 } + p.foo.r = a { a = "baz" } + p.foo.s = a { a = input.c }`, + }, + wantQueries: []string{ + `x = input.a; z = {"q": {"foo": 1}}`, + `x = input.b; z = {"q": {"bar": 2}}`, + `x = "foo"; z = {"r": "baz"}`, + `z = {"s": input.c}; x = "foo"`, + }, + }, { note: "reference: partial set", query: "data.test.p[x].foo = 1", @@ -246,6 +371,18 @@ func TestTopDownPartialEval(t *testing.T) { `input.x = 1`, }, }, + { + note: "reference: complete, ref head", + query: "data.test.p.q = 1", + modules: []string{ + `package test + + p.q = x { input.x = x }`, + }, + wantQueries: []string{ + `input.x = 1`, + }, + }, { note: "reference: complete: suffix", query: "data.test.p = true", @@ -3329,6 +3466,16 @@ func TestTopDownPartialEval(t *testing.T) { }`}, wantQueries: []string{`"bar" = input.a; "baz" = input.b`}, }, + { + note: "general ref heads: \"triple\" unification, single-value rule", + query: "data.test.foo[input.a][input.b][input.c]", + modules: []string{`package test + foo.bar[baz][bax] { + baz := "baz" + bax := "bax" + }`}, + wantQueries: []string{`"bar" = input.a; "baz" = input.b; "bax" = input.c`}, + }, { // https://github.com/open-policy-agent/opa/issues/6027 note: "ref heads: \"double\" unification, multi-value rule", query: "data.test.foo[input.a][input.b]", @@ -3339,6 +3486,17 @@ func TestTopDownPartialEval(t *testing.T) { }`}, wantQueries: []string{`"bar" = input.a; "baz" = input.b`}, }, + { + note: "general ref heads: \"triple\" unification, multi-value rule", + query: "data.test.foo[input.a][input.b][input.c]", + modules: []string{`package test + import future.keywords.contains + foo.bar[baz] contains bax { + baz := "baz" + bax := "bax" + }`}, + wantQueries: []string{`"bar" = input.a; "baz" = input.b; "bax" = input.c`}, + }, { note: "ref heads: unknown rule value", query: "data.test.p.q[x]", From 68f94361ec9bb7eb5dc5722eaefd3795824a1c1f Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Mon, 14 Aug 2023 17:36:15 +0200 Subject: [PATCH 15/27] Removing redundant legacy PE eval branch Signed-off-by: Johan Fylling --- topdown/eval.go | 59 ++++++++++++--------------------- topdown/topdown_partial_test.go | 9 ++--- 2 files changed, 27 insertions(+), 41 deletions(-) diff --git a/topdown/eval.go b/topdown/eval.go index b1d33c2c65..1510758deb 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -2400,35 +2400,25 @@ func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error result := e.empty - if e.ir.DynamicRef { - for _, rule := range e.ir.Rules { - result, err = e.evalOneDynamicRefRulePreUnify(iter, rule, hint, result, unknown) - if err != nil { - return err - } + for _, rule := range e.ir.Rules { + result, err = e.evalOneDynamicRefRulePreUnify(iter, rule, hint, result, unknown) + if err != nil { + return err } - if hint.key != nil { - pluggedKey := e.bindings.Plug(e.ref[e.pos+1]) - if pluggedKey.IsGround() { // FIXME: This should be unnecessary - hintResult := result.Get(pluggedKey) - if hintResult != nil { - e.e.virtualCache.Put(hint.key, hintResult) - } + } + + if hint.key != nil { + pluggedKey := e.bindings.Plug(e.ref[e.pos+1]) + if pluggedKey.IsGround() { // FIXME: This should be unnecessary + hintResult := result.Get(pluggedKey) + if hintResult != nil { + e.e.virtualCache.Put(hint.key, hintResult) } } - if !unknown { - return e.evalTerm(iter, e.pos+1, result, e.bindings) - } - return nil } - // FIXME: It should be possible to skip this loop, and always apply the dynamic ref case above. - // FIXME: Tracing breaks: TestFilterTraceDefault - // FIXME: PE fails: TestTopDownPartialEval - for _, rule := range e.ir.Rules { - if err := e.evalOneRulePreUnify(iter, rule, hint, result, unknown); err != nil { - return err - } + if !unknown { + return e.evalTerm(iter, e.pos+1, result, e.bindings) } return nil @@ -2483,6 +2473,7 @@ func (e evalVirtualPartial) evalAllRulesNoCache(rules []*ast.Rule) (*ast.Term, e return result, nil } +// FIXME: unused; remove func (e evalVirtualPartial) evalOneRulePreUnify(iter unifyIterator, rule *ast.Rule, hint evalVirtualPartialCacheHint, result *ast.Term, unknown bool) error { key := e.ref[e.pos+1] @@ -2593,21 +2584,15 @@ func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(iter unifyIterator, ru } if unknown { - fullPath := rule.Ref() - // FIXME: Anchor at pos instead of e.pos+1 (?) - objPath := fullPath[e.pos+1 : len(fullPath)-1] // the portion of the ref that generates nested objects - leafKey := child.bindings.Plug(fullPath[len(fullPath)-1]) // the portion of the ref that is the deepest nested key for the value - - obj := ast.NewObject() - // FIXME: getNestedObject() will plug bindings for each key, is this necessary as evalTerm() will apply them? - leafObj, err := getNestedObject(objPath, &obj, child.bindings, rule.Head.Location) - if err != nil { - return err + if rule.Head.RuleKind() == ast.MultiValue { + term = ast.SetTerm(term) } - (*leafObj).Insert(leafKey, term) - term, termbindings := child.bindings.apply(ast.NewTerm(obj)) - err = e.evalTerm(iter, e.pos+1, term, termbindings) + objRef := rule.Ref()[e.pos+1:] + term = wrapInObjects(term, objRef) + + term, termbindings := child.bindings.apply(term) + err := e.evalTerm(iter, e.pos+1, term, termbindings) if err != nil { return err } diff --git a/topdown/topdown_partial_test.go b/topdown/topdown_partial_test.go index 8535bb1cba..2e33581b35 100644 --- a/topdown/topdown_partial_test.go +++ b/topdown/topdown_partial_test.go @@ -454,11 +454,12 @@ func TestTopDownPartialEval(t *testing.T) { x.b = 2 }`, }, + // FIXME: is this a problem? wantQueries: []string{` - input[x_term_1_01] - x_term_1_01.b = 2 - x_term_1_01 - x_term_1_01.a = 1 + input[x_ref_01] + x_ref_01.b = 2 + x_ref_01 + x_ref_01.a = 1 `}, }, { From bec96d1229cd0245ce978d3cedde2c3903e76966 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Fri, 18 Aug 2023 15:41:29 +0200 Subject: [PATCH 16/27] Fixing issue where PE-produced query contained non-namespaced local vars Nested object created by general ref didn't maintain ground:ness even though containing vars. Signed-off-by: Johan Fylling --- topdown/eval.go | 30 ++-- topdown/topdown_partial_test.go | 256 ++++++++++++++++++++++++++++++-- 2 files changed, 252 insertions(+), 34 deletions(-) diff --git a/topdown/eval.go b/topdown/eval.go index 1510758deb..45b4c64220 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -2372,6 +2372,7 @@ func (e evalVirtualPartial) eval(iter unifyIterator) error { func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error { // FIXME: Check if any part of ref[e.pos+1:] is unknown? + // TODO: Make TestTopDownPartialEval test for above if e.e.unknown(e.ref[e.pos+1], e.bindings) { // Queried rule key is unknown for _, rule := range e.ir.Rules { @@ -2540,23 +2541,14 @@ func (e evalVirtualPartial) evalOneRulePreUnify(iter unifyIterator, rule *ast.Ru return nil } -func wrapInObjects(val *ast.Term, ref ast.Ref) *ast.Term { +func wrapInObjects(leaf *ast.Term, ref ast.Ref) *ast.Term { + // We build the nested objects leaf-to-root to preserve ground:ness if len(ref) == 0 { - return val - } - obj := ast.NewObject() - current := obj - for i := 0; i < len(ref); i++ { - key := ref[i] - if i == len(ref)-1 { - current.Insert(key, val) - } else { - next := ast.NewObject() - current.Insert(key, ast.NewTerm(next)) - current = next - } + return leaf } - return ast.NewTerm(obj) + key := ref[0] // Do we need to plug bindings here? + val := wrapInObjects(leaf, ref[1:]) + return ast.ObjectTerm(ast.Item(key, val)) } func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(iter unifyIterator, rule *ast.Rule, hint evalVirtualPartialCacheHint, result *ast.Term, unknown bool) (*ast.Term, error) { @@ -2584,6 +2576,8 @@ func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(iter unifyIterator, ru } if unknown { + term, termbindings := child.bindings.apply(term) + if rule.Head.RuleKind() == ast.MultiValue { term = ast.SetTerm(term) } @@ -2591,7 +2585,6 @@ func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(iter unifyIterator, ru objRef := rule.Ref()[e.pos+1:] term = wrapInObjects(term, objRef) - term, termbindings := child.bindings.apply(term) err := e.evalTerm(iter, e.pos+1, term, termbindings) if err != nil { return err @@ -2685,6 +2678,8 @@ func (e evalVirtualPartial) evalOneRuleContinue(iter unifyIterator, rule *ast.Ru term = rule.Head.Key } + term, termbindings := child.bindings.apply(term) + if rule.Head.RuleKind() == ast.MultiValue { term = ast.SetTerm(term) } @@ -2692,7 +2687,6 @@ func (e evalVirtualPartial) evalOneRuleContinue(iter unifyIterator, rule *ast.Ru objRef := rule.Ref()[e.pos+1:] term = wrapInObjects(term, objRef) - term, termbindings := child.bindings.apply(term) err := e.evalTerm(iter, e.pos+1, term, termbindings) if err != nil { return err @@ -2775,7 +2769,7 @@ func (e evalVirtualPartial) partialEvalSupportRule(rule *ast.Rule, path ast.Ref) ruleRef[i] = child.bindings.plugNamespaced(ruleRef[i], e.e.caller.bindings) } head.Reference = ruleRef - if (len(ruleRef) == 1 || len(ruleRef) == 2) && head.Name.Equal(ast.Var("")) { + if head.Name.Equal(ast.Var("")) && (len(ruleRef) == 1 || (len(ruleRef) == 2 && head.Key == nil)) { head.Name = ruleRef[0].Value.(ast.Var) } if len(ruleRef) == 2 && head.Key == nil { diff --git a/topdown/topdown_partial_test.go b/topdown/topdown_partial_test.go index 2e33581b35..cf6c93d68b 100644 --- a/topdown/topdown_partial_test.go +++ b/topdown/topdown_partial_test.go @@ -139,7 +139,7 @@ func TestTopDownPartialEval(t *testing.T) { }, wantQueries: []string{}, }, - { + { // TODO: duplicate for general refs? note: "iterate rules: partial object", query: `data.test.p[x] = input.x`, modules: []string{ @@ -154,7 +154,7 @@ func TestTopDownPartialEval(t *testing.T) { `"d" = input.x; x = "c"`, }, }, - { + { // TODO: duplicate for general refs? note: "iterate rules: partial set", query: `input.x = x; data.test.p[x]`, modules: []string{ @@ -192,7 +192,7 @@ func TestTopDownPartialEval(t *testing.T) { `x = input; x[j] = z; y = [x]; i = 0`, }, }, - { + { // TODO: duplicate for general refs? note: "single term: save", query: `input.x = x; data.test.p[x]`, modules: []string{ @@ -205,7 +205,6 @@ func TestTopDownPartialEval(t *testing.T) { `input.x = "bar"; x = "bar"`, }, }, - // TODO: "single term, general ref: save"(?) { note: "single term: false save", query: `input = x; x = false; x`, // last expression must be preserved @@ -225,18 +224,6 @@ func TestTopDownPartialEval(t *testing.T) { `x = input.x`, }, }, - //{ // FIXME: Just for experimenting, don't keep - // note: "reference: partial object, no query match", - // query: "data.test.p[x].foo = 2", - // modules: []string{ - // `package test - // p[x] = {y: z} { x = input.x; y = "foo"; z = 1 } - // p[x] = {y: z} { x = input.y; y = "bar"; z = 2 }`, - // }, - // wantQueries: []string{ - // `x = input.x`, - // }, - //}, { note: "reference: partial object, general ref", query: "data.test.p[x].q.foo = 1", @@ -332,6 +319,27 @@ func TestTopDownPartialEval(t *testing.T) { }, { note: "reference: partial object, general ref (8)", + query: "data.test.p = z", + modules: []string{ + `package test + p[a].q = {b: c} { a = input.a; b = "foo"; c = 1 } + p[a].q = {b: c} { a = input.b; b = "bar"; c = 2 } + p.foo.r = a { a = "baz" } + p.foo.s = a { a = input.c }`, + }, + wantQueries: []string{ + `data.partial.test.p = z`, + }, + wantSupport: []string{ + `package partial.test + p[a4].q = {"foo": 1} { a4 = input.a } + p[a3].q = {"bar": 2} { a3 = input.b } + p.foo.r = "baz" { true } + p.foo.s = a2 { a2 = input.c }`, + }, + }, + { + note: "reference: partial object, general ref (9)", query: "data.test.p[x] = z", modules: []string{ `package test @@ -347,6 +355,54 @@ func TestTopDownPartialEval(t *testing.T) { `z = {"s": input.c}; x = "foo"`, }, }, + { + note: "reference: partial object, general ref, multiple vars", + query: `data.test.p = x`, + modules: []string{ + `package test + p[q].r[s] := v { v := "foo"; q := 42; s := "bar" } + p[q].r[s].t := v { v := input.x; q := input.y; s := "baz" }`, + }, + wantQueries: []string{ + `data.partial.test.p = x`, + }, + wantSupport: []string{ + `package partial.test + p[42].r.bar = "foo" { true } + p[__local4__2].r.baz.t = __local3__2 { __local3__2 = input.x; __local4__2 = input.y }`, + }, + }, + { + note: "reference: partial object, general ref, multiple vars (2)", + query: `data.test.p[42] = x`, + modules: []string{ + `package test + p[q].r[s] := v { v := "foo"; q := 42; s := "bar" } + p[q].r[s].t := v { v := input.x; q := input.y; s := "baz" }`, + }, + wantQueries: []string{ + `x = {"r": {"bar": "foo"}}`, + `42 = input.y; x = {"r": {"baz": {"t": input.x}}}`, + }, + }, + { + note: "reference: partial object, general ref, multiple vars (2) (shallow)", + query: `data.test.p[42] = x`, + shallow: true, + modules: []string{ + `package test + #p[q].r[s] := v { v := "foo"; q := 42; s := "bar" } + #p[q].r[s].t := v { v := input.x; q := input.y; s := "baz" } + p[q][r][s].t := v { v := input.x; q := input.y; s := input.z; r := "known" }`, + }, + wantQueries: []string{ + `data.partial.test.p[42] = x`, + }, + wantSupport: []string{ + `package partial.test + p[__local1__1].known[__local2__1].t = __local0__1 { __local0__1 = input.x; __local1__1 = input.y; __local2__1 = input.z }`, + }, + }, { note: "reference: partial set", query: "data.test.p[x].foo = 1", @@ -359,6 +415,174 @@ func TestTopDownPartialEval(t *testing.T) { `1 = input.x; x = {"foo": 1}`, }, }, + { + note: "reference: partial set, general ref", + query: "data.test.p[x][y].foo = 1", + modules: []string{ + `package test + import future.keywords.contains + p[x] contains y { y = {a: b}; a = "foo"; b = input.x; x := 42 } + p[x] contains y { y = {a: b}; a = "bar"; b = input.x; x := input.y }`, + }, + wantQueries: []string{ + `1 = input.x; y = {"foo": 1}; x = 42`, + }, + }, + { + note: "reference: partial set, general ref (2)", + query: "data.test.p[x][y].bar = 1", + modules: []string{ + `package test + import future.keywords.contains + p[x] contains y { y = {a: b}; a = "foo"; b = input.x; x = 42 } + p[x] contains y { y = {a: b}; a = "bar"; b = input.x; x = input.y }`, + }, + wantQueries: []string{ + `1 = input.x; x = input.y; y = {"bar": 1}`, + }, + }, + { + note: "reference: partial set, general ref (3)", + query: "data.test.p[42][y].foo = 1", + modules: []string{ + `package test + import future.keywords.contains + p[x] contains y { y = {a: b}; a = "foo"; b = input.x; x := 42 } + p[x] contains y { y = {a: b}; a = "bar"; b = input.x; x := input.y }`, + }, + wantQueries: []string{ + `1 = input.x; y = {"foo": 1}`, + }, + }, + { + note: "reference: partial set, general ref (4)", + query: `data.test.p[x][y] = {"foo": 1}`, + modules: []string{ + `package test + import future.keywords.contains + p[x] contains y { y = {a: b}; a = "foo"; b = input.x; x := 42 } + p[x] contains y { y = {a: b}; a = "bar"; b = input.x; x := input.y }`, + }, + wantQueries: []string{ + `1 = input.x; y = {"foo": 1}; x = 42`, + }, + }, + { + note: "reference: partial set, general ref (5)", + query: `data.test.p[x] = {{"foo": 1}}`, + modules: []string{ + `package test + import future.keywords.contains + p[x] contains y { y = {a: b}; a = "foo"; b = input.x; x := 42 } + p[x] contains y { y = {a: b}; a = "bar"; b = input.x; x := input.y }`, + }, + wantQueries: []string{ + `{{"foo": input.x}} = {{"foo": 1}}; x = 42`, // `1 = input.x; x = 42` would be a more precise optimization (?) + }, + }, + { + note: "reference: partial set, general ref (6)", + query: `data.test.p`, + modules: []string{ + `package test + import future.keywords.contains + p[x] contains y { y = {a: b}; a = "foo"; b = input.x; x := 42 } + p[x] contains y { y = {a: b}; a = "bar"; b = input.x; x := input.y }`, + }, + wantQueries: []string{ + `data.partial.test.p`, + }, + wantSupport: []string{ + `package partial.test + import future.keywords.contains + p[42] contains {"foo": b1} { b1 = input.x } + p[__local1__2] contains {"bar": b2} { b2 = input.x; __local1__2 = input.y }`, + }, + }, + { + note: "reference: partial set, general ref (7)", + query: `data.test.p = x`, + modules: []string{ + `package test + import future.keywords.contains + p[x] contains y { y = {a: b}; a = "foo"; b = input.x; x := 42 } + p[x] contains y { y = {a: b}; a = "bar"; b = input.x; x := input.y }`, + }, + wantQueries: []string{ + `data.partial.test.p = x`, + }, + wantSupport: []string{ + `package partial.test + import future.keywords.contains + p[42] contains {"foo": b1} { b1 = input.x } + p[__local1__2] contains {"bar": b2} { b2 = input.x; __local1__2 = input.y }`, + }, + }, + { + note: "reference: partial set, general ref (8)", + query: `data.test.p = x`, + modules: []string{ + `package test + import future.keywords.contains + p[x].r contains y { y = {a: b}; a = "foo"; b = input.x; x := 42 } + p[x].r contains y { y = {a: b}; a = "bar"; b = input.x; x := input.y }`, + }, + wantQueries: []string{ + `data.partial.test.p = x`, + }, + wantSupport: []string{ + `package partial.test + import future.keywords.contains + p[42].r contains {"foo": b1} { b1 = input.x } + p[__local1__2].r contains {"bar": b2} { b2 = input.x; __local1__2 = input.y }`, + }, + }, + { + note: "reference: partial set, general ref, multiple vars", + query: `data.test.p = x`, + modules: []string{ + `package test + import future.keywords.contains + p[q].r[s] contains x { x = "foo"; q := 42; s = "bar" } + p[q].r[s].t contains x { x = input.x; q := input.y; s = "baz" }`, + }, + wantQueries: []string{ + `data.partial.test.p = x`, + }, + wantSupport: []string{ + `package partial.test + import future.keywords.contains + p[42].r.bar contains "foo" { true } + p[__local1__2].r.baz.t contains x2 { x2 = input.x; __local1__2 = input.y }`, + }, + }, + { + note: "reference: partial set, general ref, multiple vars (2)", + query: `data.test.p[42] = x`, + modules: []string{ + `package test + import future.keywords.contains + p[q].r[s] contains v { v := "foo"; q := 42; s := "bar" } + p[q].r[s].t contains v { v := input.x; q := input.y; s := "baz" }`, + }, + wantQueries: []string{ + `x = {"r": {"bar": {"foo"}}}`, + `42 = input.y; x = {"r": {"baz": {"t": {input.x}}}}`, + }, + }, + { + note: "reference: partial set, general ref, multiple vars (3)", + query: `data.test.p.foo = x`, + modules: []string{ + `package test + import future.keywords.contains + p[q].r[s] contains x { x = "foo"; q := 42; s = "bar" } + p[q].r[s].t contains x { x = input.x; q := input.y; s = "baz" }`, + }, + wantQueries: []string{ + `"foo" = input.y; x = {"r": {"baz": {"t": {input.x}}}}`, + }, + }, { note: "reference: complete", query: "data.test.p = 1", From 33ebc2f613ff2933998b7b8ed70063809b93179f Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Fri, 18 Aug 2023 18:31:35 +0200 Subject: [PATCH 17/27] Adding PE tests Signed-off-by: Johan Fylling --- topdown/eval.go | 8 ++- topdown/topdown_partial_test.go | 96 ++++++++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 6 deletions(-) diff --git a/topdown/eval.go b/topdown/eval.go index 45b4c64220..dd8f7fe9e0 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -2410,11 +2410,9 @@ func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error if hint.key != nil { pluggedKey := e.bindings.Plug(e.ref[e.pos+1]) - if pluggedKey.IsGround() { // FIXME: This should be unnecessary - hintResult := result.Get(pluggedKey) - if hintResult != nil { - e.e.virtualCache.Put(hint.key, hintResult) - } + hintResult := result.Get(pluggedKey) + if hintResult != nil { + e.e.virtualCache.Put(hint.key, hintResult) } } diff --git a/topdown/topdown_partial_test.go b/topdown/topdown_partial_test.go index cf6c93d68b..1618104226 100644 --- a/topdown/topdown_partial_test.go +++ b/topdown/topdown_partial_test.go @@ -662,6 +662,38 @@ func TestTopDownPartialEval(t *testing.T) { `y.bar = 1; z1 = input.foo[y]`, }, }, + { + note: "reference: ref head: from query", + query: "data.test.p.q[y] = 1", + modules: []string{ + `package test + + p.q[x] = 1 { + input.foo[x] = z + x.bar = 1 + } + `, + }, + wantQueries: []string{ + `y.bar = 1; z1 = input.foo[y]`, + }, + }, + { + note: "reference: general ref head: from query", + query: "data.test.p.q[y].s = 1", + modules: []string{ + `package test + + p.q[x].s = 1 { + input.foo[x] = z + x.bar = 1 + } + `, + }, + wantQueries: []string{ + `y.bar = 1; z1 = input.foo[y]`, + }, + }, { note: "reference: head: applied", query: "data.test.p = true", @@ -743,6 +775,28 @@ func TestTopDownPartialEval(t *testing.T) { `input.x = "foo"; x = "foo"; y = 2`, }, }, + { + note: "namespace: partial object, ref head", + query: "input.x = x; data.test.p.q[x] = y; y = 2", + modules: []string{ + `package test + p.q[y] = x { y = "foo"; x = 2 }`, + }, + wantQueries: []string{ + `input.x = "foo"; x = "foo"; y = 2`, + }, + }, + { + note: "namespace: partial object, general ref head", + query: "input.x = x; input.y = y; data.test.p.q[x][y] = z; z = 2", + modules: []string{ + `package test + p.q[x][y] = z { x = "foo"; y = "bar"; z = 2 }`, + }, + wantQueries: []string{ + `input.x = "foo"; input.y = "bar"; x = "foo"; y = "bar"; z = 2`, + }, + }, { note: "namespace: embedding", query: "data.test.p = x", @@ -1506,6 +1560,23 @@ func TestTopDownPartialEval(t *testing.T) { p[x2] { input.x = x2 } `}, }, + { + note: "automatic shallow inlining: full extent: partial set, general ref head", + query: "data.test.p.q = x", + modules: []string{ + `package test + import future.keywords.contains + p.q contains x { input.x = x } + p.q[r].s contains t { input.r = r; input.t = t }`, + }, + wantQueries: []string{`data.partial.test.p.q = x`}, + wantSupport: []string{` + package partial.test.p + import future.keywords.contains + q[x2] { input.x = x2 } + q[r1].s contains t1 { input.r = r1; input.t = t1 } + `}, + }, { note: "automatic shallow inlining: full extent: partial object", query: "data.test.p = x", @@ -1521,6 +1592,21 @@ func TestTopDownPartialEval(t *testing.T) { p[x2] = y2 { x2 = input.x; y2 = input.y } `}, }, + { + note: "automatic shallow inlining: full extent: partial object, general ref head", + query: "data.test.p.q = x", + modules: []string{ + `package test + p.q[x] = y { x = input.x; y = input.y } + p.q[r].s[t] = y { r = input.r; t = input.t; y = input.y }`, + }, + wantQueries: []string{`data.partial.test.p.q = x`}, + wantSupport: []string{` + package partial.test.p + q[x2] = y2 { x2 = input.x; y2 = input.y } + q[r1].s[t1] = y1 { r1 = input.r; t1 = input.t; y1 = input.y } + `}, + }, { note: "automatic shallow inlining: full extent: no solutions", query: "data.test.p = x", @@ -1536,19 +1622,27 @@ func TestTopDownPartialEval(t *testing.T) { query: "data.test[x] = y", modules: []string{ `package test + import future.keywords.contains s[x] { x = input.x } + s2[x].u contains y { x = input.x; y = input.y } p[x] = y { x = input.x; y = input.y } + p2[x].r[y] = z { x = input.x; y = input.y; z = input.z } r = x { x = input.x }`, }, wantQueries: []string{ `data.partial.test.s = y; x = "s"`, + `data.partial.test.s2 = y; x = "s2"`, `data.partial.test.p = y; x = "p"`, + `data.partial.test.p2 = y; x = "p2"`, `y = input.x; x = "r"`, }, wantSupport: []string{` package partial.test + import future.keywords.contains p[x1] = y1 { x1 = input.x; y1 = input.y } - s[x3] { x3 = input.x } + p2[x2].r[y2] = z2 { x2 = input.x; y2 = input.y; z2 = input.z } + s[x4] { x4 = input.x } + s2[x5].u contains y5 { x5 = input.x; y5 = input.y } `}, }, { From 80620a027f0080fa7fdf3f3713bcc3ee15a0cacf Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Mon, 21 Aug 2023 15:21:18 +0200 Subject: [PATCH 18/27] Invoking evalOneRulePostUnify() for any ref with unknowns overlapping with dynamic portion of a rule ref Signed-off-by: Johan Fylling --- topdown/eval.go | 27 ++++++++-- topdown/topdown_partial_test.go | 93 +++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 4 deletions(-) diff --git a/topdown/eval.go b/topdown/eval.go index dd8f7fe9e0..9d91eca9ff 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -2369,12 +2369,31 @@ func (e evalVirtualPartial) eval(iter unifyIterator) error { return e.evalEachRule(iter, unknown) } +// returns the maximum length a ref can be without being longer than the longest rule ref in rules. +func maxRefLength(rules []*ast.Rule, ceil int) int { + var l int + for _, r := range rules { + rl := len(r.Ref()) + if r.Head.RuleKind() == ast.MultiValue { + rl = rl + 1 + } + if rl >= ceil { + return ceil + } else if rl > l { + l = rl + } + } + return l +} + func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error { - // FIXME: Check if any part of ref[e.pos+1:] is unknown? - // TODO: Make TestTopDownPartialEval test for above - if e.e.unknown(e.ref[e.pos+1], e.bindings) { - // Queried rule key is unknown + if e.ir.Empty() { + return nil + } + + m := maxRefLength(e.ir.Rules, len(e.ref)) + if e.e.unknown(e.ref[e.pos+1:m], e.bindings) { for _, rule := range e.ir.Rules { if err := e.evalOneRulePostUnify(iter, rule); err != nil { return err diff --git a/topdown/topdown_partial_test.go b/topdown/topdown_partial_test.go index 1618104226..3a3472a888 100644 --- a/topdown/topdown_partial_test.go +++ b/topdown/topdown_partial_test.go @@ -583,6 +583,99 @@ func TestTopDownPartialEval(t *testing.T) { `"foo" = input.y; x = {"r": {"baz": {"t": {input.x}}}}`, }, }, + { + note: "reference: partial object, unknown in query ref", + query: "data.test.p[input.x]", + modules: []string{ + `package test + p[q].r[s] = v { q = {"foo", "bar"}[s]; v = "baz" } + p.q.r.s := 1`, + }, + wantQueries: []string{ + `"foo" = input.x`, + `"bar" = input.x`, + `"q" = input.x`, + }, + }, + { + note: "reference: partial object, unknown in query ref (2)", + query: "data.test.p.foo.r[input.x]", + modules: []string{ + `package test + p[q].r[s] = v { q = {"foo", "bar"}[s]; v = "baz" } + p.q.r.s := 1`, + }, + wantQueries: []string{ + `"foo" = input.x`, + }, + }, + { + note: "reference: partial object, unknown in query ref (3)", + query: "data.test.p[input.x].r[input.y]", + modules: []string{ + `package test + p[q].r[s] = v { q = {"foo", "bar"}[s]; v = "baz" } + p.q.r.s := 1`, + }, + wantQueries: []string{ + `"foo" = input.x; "foo" = input.y`, + `"bar" = input.x; "bar" = input.y`, + `"q" = input.x; "s" = input.y`, + }, + }, + { + note: "reference: partial object, unknown in query ref (4)", + query: "data.test.p[x].r[y][input.x]", + modules: []string{ + `package test + p[q].r[s] = {v: w} { q = {"foo", "bar"}[s]; v = "baz"; w = "bax" } + p.q.r.s := {1: 2}`, + }, + wantQueries: []string{ + `"baz" = input.x; x = "foo"; y = "foo"`, + `"baz" = input.x; x = "bar"; y = "bar"`, + `1 = input.x; x = "q"; y = "s"`, + }, + }, + { + note: "reference: partial object, unknown in query ref (5)", + query: "data.test.p[x].r[y][input.x] = input.y", + modules: []string{ + `package test + p[q].r[s] = {v: w} { q = {"foo", "bar"}[s]; v = "baz"; w = "bax" } + p.q.r.s := {1: 2}`, + }, + wantQueries: []string{ + `"baz" = input.x; "bax" = input.y; x = "foo"; y = "foo"`, + `"baz" = input.x; "bax" = input.y; x = "bar"; y = "bar"`, + `1 = input.x; 2 = input.y; x = "q"; y = "s"`, + }, + }, + { + note: "reference: partial object, unknown in query ref (6)", + query: `data.test.p[x].r[y][input.x] = "bax"`, + modules: []string{ + `package test + p[q].r[s] = {v: w} { q = {"foo", "bar"}[s]; v = "baz"; w = "bax" } + p.q.r.s := {1: 2}`, + }, + wantQueries: []string{ + `"baz" = input.x; x = "foo"; y = "foo"`, + `"baz" = input.x; x = "bar"; y = "bar"`, + }, + }, + { + note: "reference: partial object, unknown in query ref (7)", + query: `data.test.p[x].r[y][input.x] = 2`, + modules: []string{ + `package test + p[q].r[s] = {v: w} { q = {"foo", "bar"}[s]; v = "baz"; w = "bax" } + p.q.r.s := {1: 2}`, + }, + wantQueries: []string{ + `1 = input.x; x = "q"; y = "s"`, + }, + }, { note: "reference: complete", query: "data.test.p = 1", From 98178fbaeb0f81e38721fa3d1368e7c5f313c333 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Mon, 21 Aug 2023 16:26:32 +0200 Subject: [PATCH 19/27] fmt: Not using rule ref ground prefix if ref has dynamic part Signed-off-by: Johan Fylling --- format/format.go | 2 +- format/format_test.go | 2 ++ format/testfiles/test_ref_heads.rego | 7 ++++++ .../testfiles/test_ref_heads.rego.formatted | 23 +++++++++++++++++++ 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/format/format.go b/format/format.go index 22cabd9605..38304a55c4 100644 --- a/format/format.go +++ b/format/format.go @@ -453,7 +453,7 @@ func (w *writer) writeElse(rule *ast.Rule, o fmtOpts, comments []*ast.Comment) [ func (w *writer) writeHead(head *ast.Head, isDefault, isExpandedConst bool, o fmtOpts, comments []*ast.Comment) []*ast.Comment { ref := head.Ref() - if head.Key != nil && head.Value == nil { + if head.Key != nil && head.Value == nil && !head.HasDynamicRef() { ref = ref.GroundPrefix() } if o.refHeads || len(ref) == 1 { diff --git a/format/format_test.go b/format/format_test.go index 8d363210ff..d562b53a14 100644 --- a/format/format_test.go +++ b/format/format_test.go @@ -78,6 +78,8 @@ func TestFormatSourceError(t *testing.T) { } func TestFormatSource(t *testing.T) { + t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + regoFiles, err := filepath.Glob("testfiles/*.rego") if err != nil { panic(err) diff --git a/format/testfiles/test_ref_heads.rego b/format/testfiles/test_ref_heads.rego index a55386ab59..f67663a4d1 100644 --- a/format/testfiles/test_ref_heads.rego +++ b/format/testfiles/test_ref_heads.rego @@ -11,3 +11,10 @@ q[1] = y if true r[x] if x := 10 p.q.r[x] if x := 10 p.q.r[2] if true + +g[h].i[j].k { true } +g[h].i[j].k { h := 1; j = 2 } +g[3].i[j].k = x { j := 3; x = 4 } +g[h].i[j].k[l] if { true } +g[h].i[j].k[l] contains x { x = "foo" } +g[h].i[j].k[l] contains x { h := 5; j := 6; l = 7; x = "foo" } diff --git a/format/testfiles/test_ref_heads.rego.formatted b/format/testfiles/test_ref_heads.rego.formatted index 5f23e3681b..c5fad5f043 100644 --- a/format/testfiles/test_ref_heads.rego.formatted +++ b/format/testfiles/test_ref_heads.rego.formatted @@ -17,3 +17,26 @@ r[x] if x := 10 p.q.r[x] if x := 10 p.q.r[2] = true + +g[h].i[j].k = true + +g[h].i[j].k if { + h := 1 + j = 2 +} + +g[3].i[j].k = x if { + j := 3 + x = 4 +} + +g[h].i[j].k[l] = true + +g[h].i[j].k[l] contains x if x = "foo" + +g[h].i[j].k[l] contains x if { + h := 5 + j := 6 + l = 7 + x = "foo" +} From 372def85671f1ea667e21dfa8f5e91a1626f4f3c Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Mon, 21 Aug 2023 16:42:55 +0200 Subject: [PATCH 20/27] Fixing linter warnings Signed-off-by: Johan Fylling --- topdown/eval.go | 67 ------------------------------------------------- types/types.go | 30 +++++++++++----------- 2 files changed, 15 insertions(+), 82 deletions(-) diff --git a/topdown/eval.go b/topdown/eval.go index 9d91eca9ff..c5ec5f22c6 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -2491,73 +2491,6 @@ func (e evalVirtualPartial) evalAllRulesNoCache(rules []*ast.Rule) (*ast.Term, e return result, nil } -// FIXME: unused; remove -func (e evalVirtualPartial) evalOneRulePreUnify(iter unifyIterator, rule *ast.Rule, hint evalVirtualPartialCacheHint, result *ast.Term, unknown bool) error { - - key := e.ref[e.pos+1] - child := e.e.child(rule.Body) - - child.traceEnter(rule) - var defined bool - - headKey := rule.Head.Key - if headKey == nil { - headKey = rule.Head.Reference[len(rule.Head.Reference)-1] - } - err := child.biunify(headKey, key, child.bindings, e.bindings, func() error { - defined = true - return child.eval(func(child *eval) error { - - term := rule.Head.Value - if term == nil { - term = headKey - } - - if hint.key != nil { - result := child.bindings.Plug(term) - e.e.virtualCache.Put(hint.key, result) - } - - // NOTE(tsandall): if the rule set depends on any unknowns then do - // not perform the duplicate check because evaluation of the ruleset - // may not produce a definitive result. This is a bit strict--we - // could improve by skipping only when saves occur. - if !unknown { - var dup bool - var err error - result, dup, err = e.reduce(rule, child.bindings, result) - if err != nil { - return err - } else if dup { - child.traceDuplicate(rule) - return nil - } - } - - child.traceExit(rule) - term, termbindings := child.bindings.apply(term) - err := e.evalTerm(iter, e.pos+2, term, termbindings) - if err != nil { - return err - } - - child.traceRedo(rule) - return nil - }) - }) - - if err != nil { - return err - } - - // TODO(tsandall): why are we tracing here? this looks wrong. - if !defined { - child.traceFail(rule) - } - - return nil -} - func wrapInObjects(leaf *ast.Term, ref ast.Ref) *ast.Term { // We build the nested objects leaf-to-root to preserve ground:ness if len(ref) == 0 { diff --git a/types/types.go b/types/types.go index b61a0eb2e4..96d5140d7c 100644 --- a/types/types.go +++ b/types/types.go @@ -433,24 +433,24 @@ func (t *Object) Select(name interface{}) Type { func (t *Object) Merge(other Type) *Object { if otherObj, ok := other.(*Object); ok { return mergeObjects(t, otherObj) + } + + var typeK Type + var typeV Type + dynProps := t.DynamicProperties() + if dynProps != nil { + typeK = Or(Keys(other), dynProps.Key) + typeV = Or(Values(other), dynProps.Value) + dynProps = NewDynamicProperty(typeK, typeV) } else { - var typeK Type - var typeV Type - dynProps := t.DynamicProperties() - if dynProps != nil { - typeK = Or(Keys(other), dynProps.Key) - typeV = Or(Values(other), dynProps.Value) + typeK = Keys(other) + typeV = Values(other) + if typeK != nil && typeV != nil { dynProps = NewDynamicProperty(typeK, typeV) - } else { - typeK = Keys(other) - typeV = Values(other) - if typeK != nil && typeV != nil { - dynProps = NewDynamicProperty(typeK, typeV) - } } - - return NewObject(t.StaticProperties(), dynProps) } + + return NewObject(t.StaticProperties(), dynProps) } func mergeObjects(a, b *Object) *Object { @@ -493,7 +493,7 @@ func mergeObjects(a, b *Object) *Object { } } - var staticProps []*StaticProperty + staticProps := make([]*StaticProperty, 0, len(staticPropsMap)) for k, v := range staticPropsMap { staticProps = append(staticProps, NewStaticProperty(k, v)) } From 9e544c6f5387ee6cb31d79172d65b49708fbfe62 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 22 Aug 2023 11:01:34 +0200 Subject: [PATCH 21/27] Ignoring additional yaml tests not supported by Wasm Signed-off-by: Johan Fylling --- internal/wasm/sdk/test/e2e/exceptions.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/wasm/sdk/test/e2e/exceptions.yaml b/internal/wasm/sdk/test/e2e/exceptions.yaml index 51c1b219b0..951a438bd4 100644 --- a/internal/wasm/sdk/test/e2e/exceptions.yaml +++ b/internal/wasm/sdk/test/e2e/exceptions.yaml @@ -5,6 +5,11 @@ "withkeyword/builtin: indirect call, arity 1, replacement is value that needs eval (array comprehension)": "https://github.com/open-policy-agent/opa/issues/5311" "refheads/general, single var": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/general, multiple vars": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, deep query": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/general, overlapping rule, no conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/general, overlapping rule, conflict": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, set leaf": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, set leaf, deep query": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, input var": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" +"refheads/general, external non-ground var": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" "refheads/general, multiple result-set entries": "Tests with arbitrary vars in rule refs (general refs) are not supported by the planner yet" \ No newline at end of file From 52bb04d39ee689b22036026d132e4b926737d0e6 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 22 Aug 2023 12:20:32 +0200 Subject: [PATCH 22/27] Cleanup * Adding comments * Removing unused stuff * Rename evalOneDynamicRefRulePreUnify() -> evalOneRulePreUnify() Signed-off-by: Johan Fylling --- ast/compile.go | 16 +++++++++++----- ast/index.go | 8 -------- topdown/eval.go | 15 ++++++--------- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/ast/compile.go b/ast/compile.go index 5be7ed7e5a..c9f8c8cd54 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -814,12 +814,18 @@ func (c *Compiler) buildRuleIndices() { return false } rules := extractRules(node.Values) - hasNonGroundKey := false + hasNonGroundRef := false for _, r := range rules { - hasNonGroundKey = !r.Head.Ref().IsGround() + hasNonGroundRef = !r.Head.Ref().IsGround() } - if hasNonGroundKey { - // collect children + if hasNonGroundRef { + // Collect children to ensure that all rules within the extent of a rule with a general ref + // are found on the same index. E.g. the following rules should be indexed under data.a.b.c: + // + // package a + // b.c[x].e := 1 { x := input.x } + // b.c.d := 2 + // b.c.d2.e[x] := 3 { x := input.x } for _, child := range node.Children { child.DepthFirst(func(c *TreeNode) bool { rules = append(rules, extractRules(c.Values)...) @@ -834,7 +840,7 @@ func (c *Compiler) buildRuleIndices() { if index.Build(rules) { c.ruleIndices.Put(rules[0].Ref().GroundPrefix(), index) } - return hasNonGroundKey // currently, we don't allow those branches to go deeper + return hasNonGroundRef // currently, we don't allow those branches to go deeper }) } diff --git a/ast/index.go b/ast/index.go index 99439063cd..e14bb72eef 100644 --- a/ast/index.go +++ b/ast/index.go @@ -38,7 +38,6 @@ type IndexResult struct { Default *Rule EarlyExit bool OnlyGroundRefs bool - DynamicRef bool } // NewIndexResult returns a new IndexResult object. @@ -61,7 +60,6 @@ type baseDocEqIndex struct { defaultRule *Rule kind RuleKind onlyGroundRefs bool - dynamicRef bool } func newBaseDocEqIndex(isVirtual func(Ref) bool) *baseDocEqIndex { @@ -70,7 +68,6 @@ func newBaseDocEqIndex(isVirtual func(Ref) bool) *baseDocEqIndex { isVirtual: isVirtual, root: newTrieNodeImpl(), onlyGroundRefs: true, - dynamicRef: false, } } @@ -92,9 +89,6 @@ func (i *baseDocEqIndex) Build(rules []*Rule) bool { if i.onlyGroundRefs { i.onlyGroundRefs = rule.Head.Reference.IsGround() } - if !i.dynamicRef { - i.dynamicRef = rule.Head.HasDynamicRef() - } var skip bool for _, expr := range rule.Body { if op := expr.OperatorTerm(); op != nil && i.skipIndexing.Contains(op) { @@ -147,7 +141,6 @@ func (i *baseDocEqIndex) Lookup(resolver ValueResolver) (*IndexResult, error) { result := NewIndexResult(i.kind) result.Default = i.defaultRule result.OnlyGroundRefs = i.onlyGroundRefs - result.DynamicRef = i.dynamicRef result.Rules = make([]*Rule, 0, len(tr.ordering)) for _, pos := range tr.ordering { @@ -180,7 +173,6 @@ func (i *baseDocEqIndex) AllRules(resolver ValueResolver) (*IndexResult, error) result := NewIndexResult(i.kind) result.Default = i.defaultRule - result.DynamicRef = i.dynamicRef result.OnlyGroundRefs = i.onlyGroundRefs result.Rules = make([]*Rule, 0, len(tr.ordering)) diff --git a/topdown/eval.go b/topdown/eval.go index c5ec5f22c6..cbf55299cb 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -187,7 +187,7 @@ func (e *eval) unknown(x interface{}, b *bindings) bool { x = ast.NewTerm(v) } - return saveRequired(e.compiler, e.inliningControl, true, e.saveSet, b, x, false) // TODO: update for general refs? + return saveRequired(e.compiler, e.inliningControl, true, e.saveSet, b, x, false) } func (e *eval) traceEnter(x ast.Node) { @@ -2281,11 +2281,11 @@ func (e evalVirtual) eval(iter unifyIterator) error { switch ir.Kind { case ast.MultiValue: var empty *ast.Term - if ir.OnlyGroundRefs { //e.pos == len(e.ref)-1 { + if ir.OnlyGroundRefs { // rule ref contains no vars, so we're building a set empty = ast.SetTerm() } else { - // rule ref contains vars, so we're building an object + // rule ref contains vars, so we're building an object containing a set leaf empty = ast.ObjectTerm() } eval := evalVirtualPartial{ @@ -2301,8 +2301,6 @@ func (e evalVirtual) eval(iter unifyIterator) error { } return eval.eval(iter) case ast.SingleValue: - // NOTE(sr): If we allow vars in others than the last position of a ref, we need - // to start reworking things here if ir.OnlyGroundRefs { eval := evalVirtualComplete{ e: e.e, @@ -2421,7 +2419,7 @@ func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error result := e.empty for _, rule := range e.ir.Rules { - result, err = e.evalOneDynamicRefRulePreUnify(iter, rule, hint, result, unknown) + result, err = e.evalOneRulePreUnify(iter, rule, hint, result, unknown) if err != nil { return err } @@ -2496,12 +2494,12 @@ func wrapInObjects(leaf *ast.Term, ref ast.Ref) *ast.Term { if len(ref) == 0 { return leaf } - key := ref[0] // Do we need to plug bindings here? + key := ref[0] val := wrapInObjects(leaf, ref[1:]) return ast.ObjectTerm(ast.Item(key, val)) } -func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(iter unifyIterator, rule *ast.Rule, hint evalVirtualPartialCacheHint, result *ast.Term, unknown bool) (*ast.Term, error) { +func (e evalVirtualPartial) evalOneRulePreUnify(iter unifyIterator, rule *ast.Rule, hint evalVirtualPartialCacheHint, result *ast.Term, unknown bool) (*ast.Term, error) { child := e.e.child(rule.Body) @@ -2561,7 +2559,6 @@ func (e evalVirtualPartial) evalOneDynamicRefRulePreUnify(iter unifyIterator, ru return nil, err } - // We're tracing here to exhibit similar behaviour to evalOneRulePreUnify if !defined { child.traceFail(rule) } From 18702a390849765563682f32c5adee66bff1997c Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 22 Aug 2023 18:40:45 +0200 Subject: [PATCH 23/27] Producing cache hint keys containing entire applicable ref for general ref heads Signed-off-by: Johan Fylling --- topdown/eval.go | 31 ++++-- topdown/eval_test.go | 242 ++++++++++++++++++++++--------------------- 2 files changed, 147 insertions(+), 126 deletions(-) diff --git a/topdown/eval.go b/topdown/eval.go index cbf55299cb..292c8fd8ca 100644 --- a/topdown/eval.go +++ b/topdown/eval.go @@ -2426,10 +2426,8 @@ func (e evalVirtualPartial) evalEachRule(iter unifyIterator, unknown bool) error } if hint.key != nil { - pluggedKey := e.bindings.Plug(e.ref[e.pos+1]) - hintResult := result.Get(pluggedKey) - if hintResult != nil { - e.e.virtualCache.Put(hint.key, hintResult) + if v, err := result.Value.Find(hint.key[e.pos+1:]); err == nil && v != nil { + e.e.virtualCache.Put(hint.key, ast.NewTerm(v)) } } @@ -2764,6 +2762,7 @@ func (e evalVirtualPartial) evalCache(iter unifyIterator) (evalVirtualPartialCac var hint evalVirtualPartialCacheHint if e.e.unknown(e.ref[:e.pos+1], e.bindings) { + // FIXME: Return empty hint if unknowns in any e.ref elem overlapping with applicable rule refs? return hint, nil } @@ -2775,17 +2774,29 @@ func (e evalVirtualPartial) evalCache(iter unifyIterator) (evalVirtualPartialCac plugged := e.bindings.Plug(e.ref[e.pos+1]) - if plugged.IsGround() { - hint.key = append(e.plugged[:e.pos+1], plugged) + if _, ok := plugged.Value.(ast.Var); ok { + hint.full = true + hint.key = e.plugged[:e.pos+1] + e.e.instr.counterIncr(evalOpVirtualCacheMiss) + return hint, nil + } + + m := maxRefLength(e.ir.Rules, len(e.ref)) + + for i := e.pos + 1; i < m; i++ { + plugged = e.bindings.Plug(e.ref[i]) + + if !plugged.IsGround() { + break + } + + hint.key = append(e.plugged[:i], plugged) if cached, _ := e.e.virtualCache.Get(hint.key); cached != nil { e.e.instr.counterIncr(evalOpVirtualCacheHit) hint.hit = true - return hint, e.evalTerm(iter, e.pos+2, cached, e.bindings) + return hint, e.evalTerm(iter, i+1, cached, e.bindings) } - } else if _, ok := plugged.Value.(ast.Var); ok { - hint.full = true - hint.key = e.plugged[:e.pos+1] } e.e.instr.counterIncr(evalOpVirtualCacheMiss) diff --git a/topdown/eval_test.go b/topdown/eval_test.go index 6ad8518f01..36531c018e 100644 --- a/topdown/eval_test.go +++ b/topdown/eval_test.go @@ -331,7 +331,7 @@ func TestTopdownVirtualCache(t *testing.T) { miss: 1, }, { - note: "partial object: simple, query into value", + note: "partial object: query into object value", module: `package p s["foo"] = { "x": 42, "y": 43 } { true } s["bar"] = { "x": 42, "y": 43 } { true }`, @@ -349,6 +349,131 @@ func TestTopdownVirtualCache(t *testing.T) { miss: 1, exp: true, }, + { + note: "partial object: simple, general ref, multiple vars", + module: `package p + s.t[u].v[w] = true { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[_] }`, + query: `data.p.s.t = x; data.p.s.t`, + hit: 1, + miss: 1, + exp: map[string]interface{}{ + "foo": map[string]interface{}{ + "v": map[string]interface{}{ + "do": true, + "re": true, + }, + }, + "bar": map[string]interface{}{ + "v": map[string]interface{}{ + "do": true, + "re": true, + }, + }, + }, + }, + { + note: "partial object: simple, general ref, multiple vars (2)", + module: `package p + s.t[u].v[w] = true { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[_] }`, + query: `data.p.s.t.foo = x; data.p.s.t["foo"]`, + hit: 1, + miss: 1, + exp: map[string]interface{}{ + "v": map[string]interface{}{ + "do": true, + "re": true, + }, + }, + }, + { + note: "partial object: simple, general ref, multiple vars (3)", + module: `package p + s.t[u].v[w] = true { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[_] }`, + query: `data.p.s.t.foo.v = x; data.p.s.t["foo"].v`, + hit: 1, + miss: 1, + exp: map[string]interface{}{ + "do": true, + "re": true, + }, + }, + { + note: "partial object: simple, general ref, multiple vars (4)", + module: `package p + s.t[u].v[w] = true { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[_] }`, + query: `data.p.s.t.foo.v.re = x; data.p.s.t["foo"].v["re"]`, + hit: 1, + miss: 1, + exp: true, + }, + { + note: "partial object: simple, general ref, miss", + module: `package p + s.t[u].v[w] = true { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[_] }`, + query: `data.p.s.t.foo.v.re = x; data.p.s.t.foo.v.do`, + hit: 0, + miss: 2, + exp: true, + }, + { + note: "partial object: simple, general ref, miss (2)", + module: `package p + s.t[u].v[w] = i { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[i] }`, + query: `data.p.s.t.foo.v.re = x; data.p.s.t.foo.v.do; data.p.s.t.foo.v.re`, + hit: 1, + miss: 2, + exp: 1, + }, + { + note: "partial object: simple, general ref, miss (3)", + module: `package p + s.t[u].v[w] = i { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[i] }`, + query: `data.p.s.t.foo.v.re = x; data.p.s.t.foo.v.do; data.p.s.t.bar.v.re`, + hit: 0, + miss: 3, + exp: 1, + }, + { + note: "partial object: simple, general ref, miss (3)", + module: `package p + s.t[u].v[w] = i { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[i] }`, + query: `data.p.s.t.foo.v.re = x; data.p.s.t.foo.v.do; data.p.s.t.bar.v.re; data.p.s.t.foo.v.do`, + hit: 1, + miss: 3, + exp: 1, + }, + { + note: "partial object: simple, general ref, miss (4)", + module: `package p + s.t[u].v[w] = i { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[i] }`, + query: `data.p.s.t.foo = x; data.p.s.t.foo.v.do`, + hit: 1, + miss: 1, + exp: map[string]interface{}{ + "v": map[string]interface{}{ + "do": 0, + "re": 1, + }, + }, + }, + { + note: "partial object: simple, general ref, miss (5)", + module: `package p + s.t[u].v[w] = i { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[i] }`, + query: `data.p.s.t.foo; data.p.s.t.foo.v.do = x`, + hit: 1, + miss: 1, + exp: 0, + }, + { + note: "partial object: simple, general ref, miss (6)", + module: `package p + s.t[u].v[w] = i { x = ["foo", "bar"]; u = x[_]; y = ["do", "re"]; w = y[i] }`, + query: `data.p.s.t.foo.v.do = x; data.p.s.t.foo`, + hit: 0, // Note: Could we be smart in query term eval order to gain an extra hit here? + miss: 2, + exp: 0, + }, { note: "partial object: simple, query into value", module: `package p @@ -804,121 +929,6 @@ func TestPartialRule(t *testing.T) { query: `data = x`, expErr: "eval_conflict_error: object keys must be unique", }, - // NOTE: There is a bit of ambiguity in the parser about when an else-body is permitted. - // Sometimes when successfully parsed, evaluation doesn't consider the else body, though. - // else bodies - //{ - // note: "partial object with else block on key override rule (else body defined)", - // module: `package test - // p[x] := i { - // x := ["foo", "bar"][i] - // } - // p.baz := "a" { - // x == 1 - // } else := "b" { - // x == 2 - // } - // x := 2 - // `, - // query: `data = x`, - // exp: `[{"x": {"test": {"p": {"bar": 1, "baz": "a", "foo": 0}}, "x": 2}}]`, - //}, - //{ - // note: "partial object (ref head) with else block on key override rule (primary body defined)", - // module: `package test - // p.q[x] := i { - // x := ["foo", "bar"][i] - // } - // p.q.baz := "a" { - // x == 1 - // } else := "b" { - // x == 2 - // } - // x := 1 - // `, - // query: `data = x`, - // exp: `[{"x": {"test": {"p": {"q": {"bar": 1, "baz": "a", "foo": 0}}, "x": 1}}}]`, - //}, - //{ - // note: "partial object (ref head) with else block on key override rule (else body defined)", - // module: `package test - // p.q[x] := i { - // x := ["foo", "bar"][i] - // } - // p.q.baz := "a" { - // x == 1 - // } else := "b" { - // x == 2 - // } - // x := 2 - // `, - // query: `data = x`, - // exp: `[{"x": {"test": {"p": {"q": {"bar": 1, "baz": "a", "foo": 0}}, "x": 2}}}]`, - //}, - //{ - // note: "partial object (ref head) with else block on key override rule (no body defined)", - // module: `package test - // p.q[x] := i { - // x := ["foo", "bar"][i] - // } - // p.q.baz := "a" { - // x == 1 - // } else := "b" { - // x == 2 - // } - // x := 3 - // `, - // query: `data = x`, - // exp: `[{"x": {"test": {"p": {"q": {"bar": 1, "foo": 0}}, "x": 3}}}]`, - //}, - //{ - // note: "partial object (general ref head) with else block on key override rule (primary body defined)", - // module: `package test - // p.q[x].r := i { - // x := ["foo", "bar"][i] - // } - // p.q.baz.s := "a" { - // x == 1 - // } else := "b" { - // x == 2 - // } - // x := 1 - // `, - // query: `data = x`, - // exp: `[{"x": {"test": {"p": {"q": {"bar": {"r": 1}, "baz": {"s": "a"}, "foo": {"r": 0}}}, "x": 1}}}]`, - //}, - //{ - // note: "partial object (general ref head) with else block on key override rule (else body defined)", - // module: `package test - // p.q[x].r := i { - // x := ["foo", "bar"][i] - // } - // p.q.baz.s := "a" { - // x == 1 - // } else := "b" { - // x == 2 - // } - // x := 2 - // `, - // query: `data = x`, - // exp: `[{"x": {"test": {"p": {"q": {"bar": {"r": 1}, "baz": {"s": "b"}, "foo": {"r": 0}}}, "x": 1}}}]`, - //}, - //{ - // note: "partial object (general ref head) with else block on key override rule (no body defined)", - // module: `package test - // p.q[x].r := i { - // x := ["foo", "bar"][i] - // } - // p.q.baz.s := "a" { - // x == 1 - // } else := "b" { - // x == 2 - // } - // x := 3 - // `, - // query: `data = x`, - // exp: `[{"x": {"test": {"p": {"q": {"bar": {"r": 1}, "foo": {"r": 0}}}, "x": 3}}}]`, - //}, // Deep queries { note: "deep query into partial object (ref head)", From 9e46555ee6379296e23f68e16c50216a00628876 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 29 Aug 2023 15:54:44 +0200 Subject: [PATCH 24/27] Updating type-tree construction Signed-off-by: Johan Fylling --- ast/check.go | 21 ++++++------------ ast/check_test.go | 54 +++++++++++++++++------------------------------ ast/env.go | 8 ++++--- 3 files changed, 31 insertions(+), 52 deletions(-) diff --git a/ast/check.go b/ast/check.go index 6c8a4d976b..73eacfacd2 100644 --- a/ast/check.go +++ b/ast/check.go @@ -265,6 +265,8 @@ func (tc *typeChecker) checkRule(env *TypeEnv, as *AnnotationSet, rule *Rule) { } } +// nestedObject creates a nested structure of object types, where each term on path corresponds to a level in the +// nesting. Each term in the path only contributes to the dynamic portion of its corresponding object. func nestedObject(env *TypeEnv, path Ref, tpe types.Type) (types.Type, error) { if len(path) == 0 { return tpe, nil @@ -279,23 +281,14 @@ func nestedObject(env *TypeEnv, path Ref, tpe types.Type) (types.Type, error) { return nil, nil } - var staticProperties []*types.StaticProperty var dynamicProperty *types.DynamicProperty - if k.IsGround() { - key, err := JSON(path[0].Value) - if err != nil { - return nil, err - } - staticProperties = append(staticProperties, types.NewStaticProperty(key, typeV)) - } else { - typeK := env.Get(k) - if typeK == nil { - return nil, nil - } - dynamicProperty = types.NewDynamicProperty(typeK, typeV) + typeK := env.Get(k) + if typeK == nil { + return nil, nil } + dynamicProperty = types.NewDynamicProperty(typeK, typeV) - return types.NewObject(staticProperties, dynamicProperty), nil + return types.NewObject(nil, dynamicProperty), nil } func (tc *typeChecker) checkExpr(env *TypeEnv, expr *Expr) *Error { diff --git a/ast/check_test.go b/ast/check_test.go index 7f120ad023..57e76bb48e 100644 --- a/ast/check_test.go +++ b/ast/check_test.go @@ -595,13 +595,9 @@ func TestCheckInferenceRules(t *testing.T) { expected: types.NewObject( []*types.StaticProperty{}, types.NewDynamicProperty(types.S, - types.NewObject( - []*types.StaticProperty{types.NewStaticProperty("s", - types.NewObject( - []*types.StaticProperty{}, - types.NewDynamicProperty(types.N, types.N), - ))}, - nil, + types.NewObject(nil, + types.NewDynamicProperty(types.S, types.NewObject(nil, + types.NewDynamicProperty(types.N, types.N))), ), ), ), @@ -610,21 +606,15 @@ func TestCheckInferenceRules(t *testing.T) { note: "general ref-rules, key overrides, complete obj access", rules: ruleset3, ref: "data.overrides.p.q", - expected: types.NewObject( - []*types.StaticProperty{types.NewStaticProperty("r", - types.NewObject( - nil, - types.NewDynamicProperty(types.N, types.N), - ))}, - types.NewDynamicProperty(types.Or(types.B, types.S), - types.Or( - types.S, - types.NewObject( - []*types.StaticProperty{types.NewStaticProperty("s", types.B)}, - nil, - ), - ), - ), + expected: types.NewObject(nil, types.NewDynamicProperty( + types.Or(types.B, types.S), + types.Any{ + types.S, + types.NewObject(nil, types.NewDynamicProperty( + types.Any{types.N, types.S}, + types.Any{types.B, types.N})), + }, + ), ), }, { @@ -635,11 +625,8 @@ func TestCheckInferenceRules(t *testing.T) { []*types.StaticProperty{}, types.NewDynamicProperty(types.S, types.NewObject( - []*types.StaticProperty{ - types.NewStaticProperty("a", types.S), - types.NewStaticProperty("b", types.N), - types.NewStaticProperty("c", types.B)}, nil, + types.NewDynamicProperty(types.S, types.Any{types.B, types.N, types.S}), ), ), ), @@ -648,31 +635,27 @@ func TestCheckInferenceRules(t *testing.T) { note: "general ref-rules, multiple static key overrides, intermediate obj access", rules: ruleset3, ref: "data.overrides_static.p.q.foo", - expected: types.NewObject( - []*types.StaticProperty{ - types.NewStaticProperty("a", types.S), - types.NewStaticProperty("b", types.N), - types.NewStaticProperty("c", types.B)}, - nil, + expected: types.NewObject(nil, + types.NewDynamicProperty(types.S, types.Any{types.B, types.N, types.S}), ), }, { note: "general ref-rules, multiple static key overrides, leaf access (a)", rules: ruleset3, ref: "data.overrides_static.p.q.foo.a", - expected: types.S, + expected: types.Any{types.B, types.N, types.S}, // Dynamically build object types don't have static properties, so even though we "know" the 'a' key has a string value, we've lost this information. }, { note: "general ref-rules, multiple static key overrides, leaf access (b)", rules: ruleset3, ref: "data.overrides_static.p.q.bar.b", - expected: types.N, + expected: types.Any{types.B, types.N, types.S}, }, { note: "general ref-rules, multiple static key overrides, leaf access (c)", rules: ruleset3, ref: "data.overrides_static.p.q.baz.c", - expected: types.B, + expected: types.Any{types.B, types.N, types.S}, }, } @@ -704,6 +687,7 @@ func TestCheckInferenceRules(t *testing.T) { t.Fatalf("Unexpected error %v:", err) } + fmt.Println(env.tree.String()) result := env.Get(ref) if tc.expected == nil { if result != nil { diff --git a/ast/env.go b/ast/env.go index 21c56392b1..784a34c047 100644 --- a/ast/env.go +++ b/ast/env.go @@ -357,7 +357,8 @@ func (n *typeTreeNode) Insert(path Ref, tpe types.Type, env *TypeEnv) { } // mergeTypes merges the types of 'a' and 'b'. If both are sets, their 'of' types are joined with an types.Or. -// If both are objects, the key and value types of their dynamic properties are joined with types.Or:s. +// If both are objects, the key types of their dynamic properties are joined with types.Or:s, and their value types +// are recursively merged (using mergeTypes). // If 'a' and 'b' are both objects, and at least one of them have static properties, they are joined // with an types.Or, instead of being merged. // If 'a' is an Any containing an Object, and 'b' is an Object (or vice versa); AND both objects have no @@ -381,9 +382,10 @@ func mergeTypes(a, b types.Type) types.Type { aDynProps := a.DynamicProperties() bDynProps := bObj.DynamicProperties() - return types.NewObject(nil, types.NewDynamicProperty( + dynProps := types.NewDynamicProperty( types.Or(aDynProps.Key, bDynProps.Key), - types.Or(aDynProps.Value, bDynProps.Value))) + mergeTypes(aDynProps.Value, bDynProps.Value)) + return types.NewObject(nil, dynProps) } else if bAny, ok := b.(types.Any); ok && len(a.StaticProperties()) == 0 { // If a is an object type with no static components ... for _, t := range bAny { From 2e7121ba4a236c75012f76b5e350ec8292570dcb Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Tue, 29 Aug 2023 19:59:17 +0200 Subject: [PATCH 25/27] Removing debug print Signed-off-by: Johan Fylling --- ast/check_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/ast/check_test.go b/ast/check_test.go index 57e76bb48e..8127dd1680 100644 --- a/ast/check_test.go +++ b/ast/check_test.go @@ -687,7 +687,6 @@ func TestCheckInferenceRules(t *testing.T) { t.Fatalf("Unexpected error %v:", err) } - fmt.Println(env.tree.String()) result := env.Get(ref) if tc.expected == nil { if result != nil { From 9dcaac83ebd7e916a902406208e1779a88167ef4 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Wed, 30 Aug 2023 19:15:03 +0200 Subject: [PATCH 26/27] Adding docs Signed-off-by: Johan Fylling --- docs/content/policy-language.md | 114 ++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/docs/content/policy-language.md b/docs/content/policy-language.md index 09f0a608f3..2321bf3110 100644 --- a/docs/content/policy-language.md +++ b/docs/content/policy-language.md @@ -992,6 +992,120 @@ data.example ```live:eg/ref_heads:output ``` +#### General References + +Any term, except the very first, in a rule head's reference can be a variable. These variables can be assigned within the rule, just as for any other partial rule, to dynamically construct a nested collection of objects. + +{{< danger >}} +General refs in rule heads is an experimental feature, and can be enabled by setting the `OPA_ENABLE_GENERAL_RULE_REFS` environment variable. + +This feature is currently not supported for Wasm and IR. +{{< /danger >}} + +Data: + +```json +{ + "users": [ + { + "id": "alice", + "role": "employee", + "country": "USA" + }, + { + "id": "bob", + "role": "customer", + "country": "USA" + }, + { + "id": "dora", + "role": "admin", + "country": "Sweden" + } + ], + "admins": [ + { + "id": "charlie" + } + ] +} +``` + +Module: + +```rego +package example + +import future.keywords + +# A partial object rule that converts a list of users to a mapping by "role" and then "id". +users_by_role[role][id] := user { + some user in data.users + id := user.id + role := user.role +} + +# Partial rule with an explicit "admin" key override +users_by_role.admin[id] := user { + some user in data.admins + id := user.id +} + +# Leaf entries can be partial sets +users_by_country[country] contains user.id { + some user in data.users + country := user.country +} +``` + +Query: + +``` +data.example +``` + +Output: + +```json +{ + "users_by_country": { + "Sweden": [ + "dora" + ], + "USA": [ + "alice", + "bob" + ] + }, + "users_by_role": { + "admin": { + "charlie": { + "id": "charlie" + }, + "dora": { + "country": "Sweden", + "id": "dora", + "role": "admin" + } + }, + "customer": { + "bob": { + "country": "USA", + "id": "bob", + "role": "customer" + } + }, + "employee": { + "alice": { + "country": "USA", + "id": "alice", + "role": "employee" + } + } + } +} +``` + ### Functions Rego supports user-defined functions that can be called with the same semantics as [Built-in Functions](#built-in-functions). They have access to both the [the data Document](../philosophy/#the-opa-document-model) and [the input Document](../philosophy/#the-opa-document-model). From ebf15034d1c9ee81f0d869cca6d94ea3933bc2ad Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Thu, 31 Aug 2023 12:11:23 +0200 Subject: [PATCH 27/27] Making changes suggested by @srenatus Signed-off-by: Johan Fylling --- ast/compile.go | 4 ++-- ast/compile_test.go | 6 +++--- ast/parser.go | 2 +- docs/content/policy-language.md | 8 ++++---- format/format_test.go | 2 +- .../testdata/refheads/test-generic-refs.yaml | 20 +++++++++---------- topdown/eval_test.go | 6 +++--- topdown/topdown_partial_test.go | 2 +- 8 files changed, 25 insertions(+), 25 deletions(-) diff --git a/ast/compile.go b/ast/compile.go index 90a4ed8041..112571e7c4 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -335,7 +335,7 @@ func NewCompiler() *Compiler { {"BuildComprehensionIndices", "compile_stage_rebuild_comprehension_indices", c.buildComprehensionIndices}, } - _, c.generalRuleRefsEnabled = os.LookupEnv("OPA_ENABLE_GENERAL_RULE_REFS") + _, c.generalRuleRefsEnabled = os.LookupEnv("EXPERIMENTAL_GENERAL_RULE_REFS") return c } @@ -1757,7 +1757,7 @@ func (c *Compiler) rewriteRuleHeadRefs() { } for i := 1; i < len(ref); i++ { - // NOTE: Unless enabled via the OPA_ENABLE_GENERAL_RULE_REFS env var, non-string values in the refs are forbidden + // NOTE: Unless enabled via the EXPERIMENTAL_GENERAL_RULE_REFS env var, non-string values in the refs are forbidden // except for the last position, e.g. // OK: p.q.r[s] // NOT OK: p[q].r.s diff --git a/ast/compile_test.go b/ast/compile_test.go index 0dd6db7d76..bb8ca866a4 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -467,7 +467,7 @@ func toRef(s string) Ref { } func TestCompilerCheckRuleHeadRefs(t *testing.T) { - t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") tests := []struct { note string @@ -1940,7 +1940,7 @@ func TestCompilerCheckRuleConflictsDefaultFunction(t *testing.T) { } func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { - t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") tests := []struct { note string @@ -2189,7 +2189,7 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) { // TODO: Remove when general rule refs are enabled by default. func TestGeneralRuleRefsDisabled(t *testing.T) { - // OPA_ENABLE_GENERAL_RULE_REFS env var not set + // EXPERIMENTAL_GENERAL_RULE_REFS env var not set tests := []struct { note string diff --git a/ast/parser.go b/ast/parser.go index ede432ae85..540d7b80c1 100644 --- a/ast/parser.go +++ b/ast/parser.go @@ -142,7 +142,7 @@ func NewParser() *Parser { s: &state{}, po: ParserOptions{}, } - _, p.po.generalRuleRefsEnabled = os.LookupEnv("OPA_ENABLE_GENERAL_RULE_REFS") + _, p.po.generalRuleRefsEnabled = os.LookupEnv("EXPERIMENTAL_GENERAL_RULE_REFS") return p } diff --git a/docs/content/policy-language.md b/docs/content/policy-language.md index 2321bf3110..043115b110 100644 --- a/docs/content/policy-language.md +++ b/docs/content/policy-language.md @@ -997,7 +997,7 @@ data.example Any term, except the very first, in a rule head's reference can be a variable. These variables can be assigned within the rule, just as for any other partial rule, to dynamically construct a nested collection of objects. {{< danger >}} -General refs in rule heads is an experimental feature, and can be enabled by setting the `OPA_ENABLE_GENERAL_RULE_REFS` environment variable. +General refs in rule heads is an experimental feature, and can be enabled by setting the `EXPERIMENTAL_GENERAL_RULE_REFS` environment variable. This feature is currently not supported for Wasm and IR. {{< /danger >}} @@ -1039,20 +1039,20 @@ package example import future.keywords # A partial object rule that converts a list of users to a mapping by "role" and then "id". -users_by_role[role][id] := user { +users_by_role[role][id] := user if { some user in data.users id := user.id role := user.role } # Partial rule with an explicit "admin" key override -users_by_role.admin[id] := user { +users_by_role.admin[id] := user if { some user in data.admins id := user.id } # Leaf entries can be partial sets -users_by_country[country] contains user.id { +users_by_country[country] contains user.id if { some user in data.users country := user.country } diff --git a/format/format_test.go b/format/format_test.go index d562b53a14..0c545fb557 100644 --- a/format/format_test.go +++ b/format/format_test.go @@ -78,7 +78,7 @@ func TestFormatSourceError(t *testing.T) { } func TestFormatSource(t *testing.T) { - t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") regoFiles, err := filepath.Glob("testfiles/*.rego") if err != nil { diff --git a/test/cases/testdata/refheads/test-generic-refs.yaml b/test/cases/testdata/refheads/test-generic-refs.yaml index a0dae45f8c..74fb221f6e 100644 --- a/test/cases/testdata/refheads/test-generic-refs.yaml +++ b/test/cases/testdata/refheads/test-generic-refs.yaml @@ -1,7 +1,7 @@ cases: - note: 'refheads/general, single var' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -18,7 +18,7 @@ cases: r: 2 - note: 'refheads/general, multiple vars' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -35,7 +35,7 @@ cases: 2: true - note: 'refheads/general, deep query' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -47,7 +47,7 @@ cases: 1: true - note: 'refheads/general, overlapping rule, no conflict' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -65,7 +65,7 @@ cases: r: 2 - note: 'refheads/general, overlapping rule, conflict' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -76,7 +76,7 @@ cases: want_error: eval_conflict_error - note: 'refheads/general, set leaf' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -101,7 +101,7 @@ cases: r: [ "a", "b" ] - note: 'refheads/general, set leaf, deep query' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -120,7 +120,7 @@ cases: - x: "c" - note: 'refheads/general, input var' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -135,7 +135,7 @@ cases: r: "foo" - note: 'refheads/general, external non-ground var' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test @@ -159,7 +159,7 @@ cases: baz: 2 - note: 'refheads/general, multiple result-set entries' env: - OPA_ENABLE_GENERAL_RULE_REFS: "true" + EXPERIMENTAL_GENERAL_RULE_REFS: "true" modules: - | package test diff --git a/topdown/eval_test.go b/topdown/eval_test.go index 36531c018e..383f3a5ce5 100644 --- a/topdown/eval_test.go +++ b/topdown/eval_test.go @@ -248,7 +248,7 @@ func TestContainsNestedRefOrCall(t *testing.T) { func TestTopdownVirtualCache(t *testing.T) { // TODO: break out into separate tests - t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") ctx := context.Background() store := inmem.New() @@ -604,7 +604,7 @@ func TestTopdownVirtualCache(t *testing.T) { } func TestPartialRule(t *testing.T) { - t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") ctx := context.Background() store := inmem.New() @@ -1393,7 +1393,7 @@ func TestGeneralRuleRefsFeatureFlag(t *testing.T) { t.Fatal("Expected error but got:", c.Errors) } - t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") c = ast.NewCompiler() c.Compile(mods) diff --git a/topdown/topdown_partial_test.go b/topdown/topdown_partial_test.go index 3a3472a888..126c7273a4 100644 --- a/topdown/topdown_partial_test.go +++ b/topdown/topdown_partial_test.go @@ -19,7 +19,7 @@ import ( func TestTopDownPartialEval(t *testing.T) { // TODO: break out into separate tests - t.Setenv("OPA_ENABLE_GENERAL_RULE_REFS", "true") + t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true") tests := []struct { note string