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

Removing EXPERIMENTAL_GENERAL_RULE_REFS feature flag (#6252)

Browse Source 
Fixes: #6245

Signed-off-by: Johan Fylling <johan.dev@fylling.se>
This commit is contained in:
Johan Fylling
2023-09-27 14:41:39 +02:00
committed by GitHub
parent c9d1a8db1f
commit c5314e357d
9 changed files with 9 additions and 273 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ast/compile.go
+2 -60
View File
@@ -7,7 +7,6 @@ package ast
import (
"fmt"
"io"
"os"
"sort"
"strconv"
"strings"
@@ -146,7 +145,6 @@ 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.
@@ -335,8 +333,6 @@ func NewCompiler() *Compiler {
{"BuildComprehensionIndices", "compile_stage_rebuild_comprehension_indices", c.buildComprehensionIndices},
}
_, c.generalRuleRefsEnabled = os.LookupEnv("EXPERIMENTAL_GENERAL_RULE_REFS")
return c
}
@@ -1005,50 +1001,8 @@ func (c *Compiler) checkRuleConflicts() {
// data.p.q[r][s] { r := input.r; s := input.s }
// data.p[q].r.s { q := input.q }
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 grandchildrenFound {
conflicts = node.flattenChildren()
break
}
}
} else { // p.q.s and p.q.s.t => any children are in conflict
conflicts = 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.
if r.Head.RuleKind() == MultiValue && len(node.Children) > 0 {
conflicts = node.flattenChildren()
}
if r.Ref().IsGround() && len(node.Children) > 0 {
conflicts = node.flattenChildren()
}
if r.Head.RuleKind() == SingleValue && r.Head.Ref().IsGround() {
@@ -1811,18 +1765,6 @@ func (c *Compiler) rewriteRuleHeadRefs() {
}
for i := 1; i < len(ref); i++ {
// 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
// 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
}
}
// Rewrite so that any non-scalar elements that in the last position of
// the rule are vars:
// p.q.r[y.z] { ... } => p.q.r[__local0__] { __local0__ = y.z }
ast/compile_test.go
-128
View File
@@ -467,8 +467,6 @@ func toRef(s string) Ref {
}
func TestCompilerCheckRuleHeadRefs(t *testing.T) {
t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
tests := []struct {
note string
modules []*Module
@@ -607,64 +605,6 @@ 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
@@ -1940,8 +1880,6 @@ func TestCompilerCheckRuleConflictsDefaultFunction(t *testing.T) {
}
func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) {
t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
tests := []struct {
note string
modules []*Module
@@ -2187,70 +2125,6 @@ func TestCompilerCheckRuleConflictsDotsInRuleHeads(t *testing.T) {
}
}
// TODO: Remove when general rule refs are enabled by default.
func TestGeneralRuleRefsDisabled(t *testing.T) {
// EXPERIMENTAL_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 {
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{})
}
})
}
}
func TestCompilerCheckUndefinedFuncs(t *testing.T) {
module := `
@@ -6861,8 +6735,6 @@ func TestCompilerMockVirtualDocumentPartially(t *testing.T) {
}
func TestCompilerCheckUnusedAssignedVar(t *testing.T) {
t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
type testCase struct {
note string
module string
ast/parser.go
+7 -15
View File
@@ -11,7 +11,6 @@ import (
"io"
"math/big"
"net/url"
"os"
"regexp"
"sort"
"strconv"
@@ -98,14 +97,13 @@ 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 *astJSON.Options
unreleasedKeywords bool // TODO(sr): cleanup
generalRuleRefsEnabled bool
Capabilities *Capabilities
ProcessAnnotation bool
AllFutureKeywords bool
FutureKeywords []string
SkipRules bool
JSONOptions *astJSON.Options
unreleasedKeywords bool // TODO(sr): cleanup
}
// NewParser creates and initializes a Parser.
@@ -114,7 +112,6 @@ func NewParser() *Parser {
s: &state{},
po: ParserOptions{},
}
_, p.po.generalRuleRefsEnabled = os.LookupEnv("EXPERIMENTAL_GENERAL_RULE_REFS")
return p
}
@@ -599,11 +596,6 @@ func (p *Parser) parseRules() []*Rule {
return []*Rule{&rule}
}
if !p.po.generalRuleRefsEnabled && usesContains && !rule.Head.Reference.IsGround() {
p.error(p.s.Loc(), "multi-value rules need ground refs")
return nil
}
// back-compat with `p[x] { ... }``
hasIf := p.s.tok == tokens.If
format/format_test.go
-2
View File
@@ -78,8 +78,6 @@ func TestFormatSourceError(t *testing.T) {
}
func TestFormatSource(t *testing.T) {
t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
regoFiles, err := filepath.Glob("testfiles/*.rego")
if err != nil {
panic(err)
internal/planner/planner_test.go
-2
View File
@@ -16,8 +16,6 @@ import (
)
func TestPlannerHelloWorld(t *testing.T) {
t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
// NOTE(tsandall): These tests are not meant to give comprehensive coverage
// of the planner. Currently we have a suite of end-to-end tests in the
// test/wasm/ directory that are specified in YAML, compiled into Wasm, and
test/cases/testdata/refheads/test-generic-refs.yaml
Vendored
-32
View File
@@ -1,7 +1,5 @@
cases:
- note: 'refheads/general, single var'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -17,8 +15,6 @@ cases:
c:
r: 2
- note: 'refheads/general, multiple vars'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -34,8 +30,6 @@ cases:
c:
2: true
- note: 'refheads/general, deep query'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -46,8 +40,6 @@ cases:
- x:
1: true
- note: 'refheads/general, overlapping rule, no conflict'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -64,8 +56,6 @@ cases:
c:
r: 2
- note: 'refheads/general, overlapping rule, different dynamic depths, no conflict'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -89,8 +79,6 @@ cases:
r: 3
e: 4
- note: 'refheads/general, self-conflict'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -99,8 +87,6 @@ cases:
query: data.test.p = x
want_error_code: eval_conflict_error
- note: 'refheads/general, overlapping rule, conflict'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -110,8 +96,6 @@ cases:
query: data.test.p = x
want_error_code: eval_conflict_error
- note: 'refheads/general, overlapping rule, deep override inside other rule object value, conflict'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -129,8 +113,6 @@ cases:
query: data.test.p = x
want_error_code: eval_conflict_error
- note: 'refheads/general, overlapping rule, deep injection into other rule object value, conflict'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -148,8 +130,6 @@ cases:
query: data.test.p = x
want_error_code: eval_conflict_error
- note: 'refheads/general, set leaf (shallow ref)'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -170,8 +150,6 @@ cases:
b: [ "a", "c", "foo" ]
c: [ "a", "b" ]
- note: 'refheads/general, set leaf (other rule defines dynamic ref portion)'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -188,8 +166,6 @@ cases:
q: [ "a", "b", "c" ]
foo: bar
- note: 'refheads/general, set leaf'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -213,8 +189,6 @@ cases:
c:
r: [ "a", "b" ]
- note: 'refheads/general, set leaf, deep query'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -232,8 +206,6 @@ cases:
want_result:
- x: "c"
- note: 'refheads/general, input var'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -247,8 +219,6 @@ cases:
bar:
r: "foo"
- note: 'refheads/general, external non-ground var'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
@@ -271,8 +241,6 @@ cases:
bar: 1
baz: 2
- note: 'refheads/general, multiple result-set entries'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
test/cases/testdata/refheads/test-refs-as-rule-heads.yaml
Vendored
-2
View File
@@ -26,8 +26,6 @@ cases:
r: 1
s: 2
- note: 'refheads/single-value, with var, conflict'
env:
EXPERIMENTAL_GENERAL_RULE_REFS: "true"
modules:
- |
package test
topdown/eval_test.go
-29
View File
@@ -247,9 +247,6 @@ func TestContainsNestedRefOrCall(t *testing.T) {
}
func TestTopdownVirtualCache(t *testing.T) {
// TODO: break out into separate tests
t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
ctx := context.Background()
store := inmem.New()
@@ -604,8 +601,6 @@ func TestTopdownVirtualCache(t *testing.T) {
}
func TestPartialRule(t *testing.T) {
t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
ctx := context.Background()
store := inmem.New()
@@ -1451,27 +1446,3 @@ func TestPartialRule(t *testing.T) {
})
}
}
// 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("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
c = ast.NewCompiler()
c.Compile(mods)
if c.Errors != nil {
t.Fatal("Unexpected error:", c.Errors)
}
}
topdown/topdown_partial_test.go
-3
View File
@@ -18,9 +18,6 @@ import (
)
func TestTopDownPartialEval(t *testing.T) {
// TODO: break out into separate tests
t.Setenv("EXPERIMENTAL_GENERAL_RULE_REFS", "true")
tests := []struct {
note string
unknowns []string