Arbitrary vars in rule refs #5913

johanfylling · 2023-05-10T18:10:50Z

This PR adds the possibility to declare rules with variables at arbitrary locations int the ref; e.g.

package example

p[q].r[s] := 42 { q := "foo"; s := "bar" }

By default, the compiler won't allow rule refs to have vars in any other position than the last term; to enable general rule refs, set the OPA_ENABLE_GENERAL_RULE_REFS env var (any value). (is this the best way to do this, or do we have precedence of other ways of setting feature flags?)

Not included in this PR:

~~Feature flag to enable/disable~~
~~Partial eval~~
Planner implementation
~~Type-checker update~~
Language definition update in docs
~~fmt~~

netlify · 2023-05-29T15:11:33Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`ebf1503`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/64f067600cca9e00073f7213
😎 Deploy Preview	https://deploy-preview-5913--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

srenatus · 2023-05-31T11:15:15Z

ast/compile.go

@@ -311,6 +335,8 @@ func NewCompiler() *Compiler {
 		{"BuildComprehensionIndices", "compile_stage_rebuild_comprehension_indices", c.buildComprehensionIndices},
 	}

+	_, c.generalRuleRefsEnabled = os.LookupEnv("OPA_ENABLE_GENERAL_RULE_REFS")


Huh interesting 💡 yeah, why not. We might throw in a _WIP_

srenatus

Started looking at it. Still much ground to cover on my side 🔍

ast/index.go

ast/parser.go

test/cases/cases.go

topdown/exported_test.go

This is the topdown part of supporting generic refs. Fixes: open-policy-agent#5993 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

srenatus

Sorry it took me so long to get back to this. Some comments and questions inside

srenatus · 2023-07-06T09:56:53Z

ast/parser.go

@@ -140,6 +142,7 @@ func NewParser() *Parser {
 		s:  &state{},
 		po: ParserOptions{},
 	}
+	_, p.po.generalRuleRefsEnabled = os.LookupEnv("OPA_ENABLE_GENERAL_RULE_REFS")


💭 treating the environment as a global variable is better avoided, but if we can keep this as a short-term stop-gap, it's probably OK.

This is temporary, and will be removed once general refs are feature complete. Hopefully soon 🤞. I'm open to improve this if it's too much of an eyesore, though 😅 .

srenatus · 2023-07-06T09:57:43Z

test/cases/testdata/refheads/test-arbitrary-var-refs.yaml

+        1: true
+      c:
+        2: true
+# FIXME: Below test case returns: rego_type_error: undefined ref: data.test.p.b


This has been solved in the type-checker update I have lined up: johanfylling/opa@jf/5685/multi-var-rule-refs...generic_refs/type-check

srenatus · 2023-07-06T09:59:18Z

test/cases/testdata/refheads/test-arbitrary-var-refs.yaml

+#
+#      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


Maybe quotes help?

Suggested change

# query: i := ["foo", "bar"][_]; data.test.p.q[i].s = x

# query: 'i := ["foo", "bar"][_]; data.test.p.q[i].s = x'

Alternatively, you could query data.test.q and add

q = x { i := ["foo", "bar"][_] p.q[i].s = x }

Hm, looking at this again, I actually get a result with the expected parameters, but with some added "stuff":

{{"$0": 0, "__localq0__": ["foo", "bar"], "i": "foo", "x": 1}, {"$0": 1, "__localq0__": ["foo", "bar"], "i": "bar", "x": 2}}

If I change the query to data.test.p.q[i].s = x, I get a good result:

{{"i": "bar", "x": 2}, {"i": "foo", "x": 1}}

Is this expected, or perhaps an unwanted side effect of my changes 🤔 ? Will try a similar thing on main.

main exhibit the same behavior. Running opa eval does not output bindings for $0 and __localq0__; so maybe this is a side effect of how the test works?

Those should never make it out, I think; but opa eval might have some extra code to filter them. Or maybe that was the rego package, https://github.com/open-policy-agent/opa/blob/main/rego/rego.go#L2166-L2168

srenatus · 2023-07-06T10:02:24Z

topdown/eval.go

+
+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()


So if they don't have the same length, but overlap, we'd accept that?

data.foo.bar = data.foo[x].z

we'd add x = "bar" to the bindings and go on?

But I might be missing something here, I haven't fully understood the topdown change yet.

We're simply binding any possibly known vars in a rule's ref before evaluating its body.

If data.foo.bar == data.foo.bar.z then I'd expect your snipped should evaluate. (is that even possible, though? 😵‍💫)

This is an interesting case to test for, though; hadn't thought about it from this angle before 🤔.

Perhaps a bit hacky test, but this should trigger this scenario:

package unification p := i { foo.bar.x.y = foo[i].z } foo[a][b] := c { a := "bar" c := { "x": { "y": 42 }, "z": 42 }[b] }

$ OPA_ENABLE_GENERAL_RULE_REFS=true opa eval -d . 'data.unification.p'

Or am I completely misunderstanding you?

srenatus · 2023-07-06T10:03:30Z

topdown/eval_test.go

+		//	`,
+		//	query: `i := ["foo", "bar"][_]; data.test.p.q[i].s = x`,
+		//	exp:   `[{"x": 1}, {"x": 2}]`,
+		//},


What's up with these? Do they not work yet; or have they been subsumed by the tests above...? 🤔

Ah, I missed un-commenting those. Fixed 👍 .

srenatus · 2023-07-06T10:07:49Z

topdown/eval.go

+		e.e.virtualCache.Put(hint.key, result)
+		return e.evalTerm(iter, e.pos+1, result, e.bindings)
+	}
+
 	for _, rule := range e.ir.Rules {


Given that generic ref head rules are the more general case, you might expect that the base case (below) is also covered by what's added above -- is that so? Or what am I missing?

I think you're right. These two branches should be possible to merge into one; given that there is no performance benefit to keeping them separate.

I think it's around here PE is breaking down too; so I'll dig around here a bit to see what improvements are necessary + possible.

It's even a bit broken as-is, given that this should be in an elseblock 😅 . I think this rule unification just won't contribute any data in applicable cases, so the result ends up being correct; but we're doing unnecessary work 🤔 .
Scratch that; I'm not even reading my own code right 😄 .

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

…ert()` Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Fixes: open-policy-agent#5994 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

instead of `Or`:ing them Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Nested object created by general ref didn't maintain ground:ness even though containing vars. Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

… with dynamic portion of a rule ref Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Adding comments * Removing unused stuff * Rename evalOneDynamicRefRulePreUnify() -> evalOneRulePreUnify() Signed-off-by: Johan Fylling <johan.dev@fylling.se>

…l ref heads Signed-off-by: Johan Fylling <johan.dev@fylling.se>

srenatus

Checked the commits since last week, only had a few questions. It's great to see this progress 🚀

srenatus · 2023-08-24T09:41:34Z

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


This looks so wrong but h and j could be rules defined somewhere in the package, right? 😅 (Not that it matters for formatting)

True, this isn't internally consistent; but it's valid Rego, and for this rule we're more interested to see what the formatter does with the redundant { true } body.

topdown/eval_test.go

Conflicts: ast/env.go

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

ashutosh-narkar · 2023-08-29T17:53:19Z

ast/check_test.go

@@ -579,6 +687,7 @@ func TestCheckInferenceRules(t *testing.T) {
 				t.Fatalf("Unexpected error %v:", err)
 			}

+			fmt.Println(env.tree.String())


Left from testing probably.

Oops, yea 🤦‍♂️ 😄

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

srenatus

Nitpicks only. Yay, can't wait to get this in. Thanks for the hard work; it's like a summer project 😎

srenatus · 2023-08-31T07:22:19Z

ast/check_test.go

-					types.NewStaticProperty("c", types.B)},
-				nil,
+			expected: types.NewObject(nil,
+				types.NewDynamicProperty(types.S, types.Any{types.B, types.N, types.S}),


📝 took me a moment to understand this, but I had overlooked that it's p.q[r].b = 42 and not p.q[r] = 42. So, the extra S keys would (could) be "a", "b", "c". ✔️

srenatus · 2023-08-31T07:24:30Z

docs/content/policy-language.md

+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.


I was wondering about the name for a moment, but I think it's OK. We've got another example here, FWIW.

Ah, I'll change this to EXPERIMENTAL_GENERAL_RULE_REFS, to align 👍 .

srenatus · 2023-08-31T07:24:53Z

docs/content/policy-language.md

+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 {


Let's throw in an if here and below

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling force-pushed the jf/5685/multi-var-rule-refs branch 2 times, most recently from 8ef1c96 to 9ae279d Compare May 30, 2023 08:42

johanfylling marked this pull request as ready for review May 30, 2023 11:23

johanfylling changed the title ~~WIP: Arbitrary vars in rule refs~~ Arbitrary vars in rule refs May 30, 2023

ashutosh-narkar requested a review from srenatus May 30, 2023 22:20

srenatus reviewed May 31, 2023

View reviewed changes

ast/index.go Outdated Show resolved Hide resolved

ast/parser.go Outdated Show resolved Hide resolved

test/cases/cases.go Show resolved Hide resolved

topdown/exported_test.go Show resolved Hide resolved

topdown: Adding support for multiple variables in rule refs

525c93d

This is the topdown part of supporting generic refs. Fixes: open-policy-agent#5993 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling force-pushed the jf/5685/multi-var-rule-refs branch from e2dbd38 to 525c93d Compare June 9, 2023 16:49

johanfylling linked an issue Jun 9, 2023 that may be closed by this pull request

general ref heads: topdown #5993

Closed

Merge branch 'main' into jf/5685/multi-var-rule-refs

5f70376

ashutosh-narkar requested a review from srenatus July 5, 2023 16:26

srenatus reviewed Jul 6, 2023

View reviewed changes

johanfylling added 16 commits July 6, 2023 16:57

Updating commented-out tests.

d387cfd

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Fixing yaml test expecting multiple results.

d1dcb03

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Not running yaml test for WASM

2073c3d

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Merge branch 'main' into jf/5685/multi-var-rule-refs

29c2a2e

Moving type merge from typeChecker.checkRule() to `typeTreeNode.Ins…

659ef2a

…ert()` Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Updating type-checker to handle generic rule refs

692770e

Fixes: open-policy-agent#5994 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Updating tests

9707e66

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Generalizing types.Object.Merge()`

391d469

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Merging sub-objects in types.Object.Merge()

ea52a01

instead of `Or`:ing them Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Removing comment in test

996cdec

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Updating tests

1065755

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Aligning hint-key and entry for general refs on virtual partial eval

67ba943

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Enforcing no-else parser rule on general ref heads

8ffcf1d

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

PE impl

f02441d

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Removing redundant legacy PE eval branch

68f9436

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Fixing issue where PE-produced query contained non-namespaced local vars

bec96d1

Nested object created by general ref didn't maintain ground:ness even though containing vars. Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling added 3 commits August 18, 2023 18:31

Adding PE tests

33ebc2f

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Invoking evalOneRulePostUnify() for any ref with unknowns overlapping…

80620a0

… with dynamic portion of a rule ref Signed-off-by: Johan Fylling <johan.dev@fylling.se>

fmt: Not using rule ref ground prefix if ref has dynamic part

98178fb

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling force-pushed the jf/5685/multi-var-rule-refs branch from 213ac32 to b9ea4d5 Compare August 21, 2023 16:20

Fixing linter warnings

372def8

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling force-pushed the jf/5685/multi-var-rule-refs branch from b9ea4d5 to 372def8 Compare August 21, 2023 16:53

johanfylling added 4 commits August 22, 2023 11:01

Ignoring additional yaml tests not supported by Wasm

9e544c6

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Merge branch 'main' into jf/5685/multi-var-rule-refs

0fa5652

Cleanup

52bb04d

* Adding comments * Removing unused stuff * Rename evalOneDynamicRefRulePreUnify() -> evalOneRulePreUnify() Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Producing cache hint keys containing entire applicable ref for genera…

18702a3

…l ref heads Signed-off-by: Johan Fylling <johan.dev@fylling.se>

srenatus reviewed Aug 24, 2023

View reviewed changes

johanfylling added 3 commits August 29, 2023 14:02

Merge branch 'main' into jf/5685/multi-var-rule-refs

d4ac3b0

Conflicts: ast/env.go

Updating type-tree construction

9e46555

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Merge branch 'main' into jf/5685/multi-var-rule-refs

e1c0a23

ashutosh-narkar reviewed Aug 29, 2023

View reviewed changes

johanfylling added 3 commits August 29, 2023 19:59

Removing debug print

2e7121b

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Merge branch 'main' into jf/5685/multi-var-rule-refs

a98a6ee

Adding docs

9dcaac8

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

srenatus previously approved these changes Aug 31, 2023

View reviewed changes

johanfylling added 2 commits August 31, 2023 11:31

Merge branch 'main' into jf/5685/multi-var-rule-refs

c0d38ea

Making changes suggested by @srenatus

ebf1503

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling dismissed srenatus’s stale review via ebf1503 August 31, 2023 10:11

srenatus approved these changes Aug 31, 2023

View reviewed changes

johanfylling merged commit 0431567 into open-policy-agent:main Aug 31, 2023
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arbitrary vars in rule refs #5913

Arbitrary vars in rule refs #5913

johanfylling commented May 10, 2023 •

edited

netlify bot commented May 29, 2023 •

edited

srenatus May 31, 2023

srenatus left a comment

srenatus left a comment

srenatus Jul 6, 2023

johanfylling Jul 6, 2023

srenatus Jul 6, 2023

johanfylling Jul 6, 2023

srenatus Jul 6, 2023

johanfylling Jul 10, 2023

johanfylling Jul 10, 2023

srenatus Jul 10, 2023

srenatus Jul 6, 2023

johanfylling Jul 6, 2023

johanfylling Jul 6, 2023

srenatus Jul 6, 2023

johanfylling Jul 6, 2023

srenatus Jul 6, 2023

johanfylling Jul 10, 2023

johanfylling Jul 10, 2023 •

edited

srenatus left a comment

srenatus Aug 24, 2023

johanfylling Aug 24, 2023

ashutosh-narkar Aug 29, 2023

johanfylling Aug 29, 2023

srenatus left a comment

srenatus Aug 31, 2023

srenatus Aug 31, 2023

johanfylling Aug 31, 2023

srenatus Aug 31, 2023

	# query: i := ["foo", "bar"][_]; data.test.p.q[i].s = x
	# query: 'i := ["foo", "bar"][_]; data.test.p.q[i].s = x'

Arbitrary vars in rule refs #5913

Arbitrary vars in rule refs #5913

Conversation

johanfylling commented May 10, 2023 • edited

netlify bot commented May 29, 2023 • edited

✅ Deploy Preview for openpolicyagent ready!

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanfylling Jul 10, 2023 • edited

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanfylling commented May 10, 2023 •

edited

netlify bot commented May 29, 2023 •

edited

johanfylling Jul 10, 2023 •

edited