Composite refs #386

mmussomele · 2017-07-18T23:01:30Z

Fixes #239

tsandall

Overall this looks really good. Left a few comments but I'll have to come back and do another pass.

tsandall · 2017-07-20T15:23:54Z

ast/check.go

@@ -487,6 +487,9 @@ func (rc *refChecker) checkRef(curr *TypeEnv, node *typeTreeNode, ref Ref, idx i
 			return newRefErrInvalid(ref[0].Location, ref, idx, exist, types.S, getOneOfForNode(node))
 		}

+	case Array, Object, *Set:
+		rc.env.tree.PutOne(value, rc.env.Get(value))


I think this should be a type error. At this point, the ref operand doesn't refer to a leaf, so the key must be a string? You'll need to think about the content of the ref error details to report here. Look at the other ref errors for examples.

Yea I think you're right, this case just shouldn't exist. I'll just let it hit default

tsandall · 2017-07-20T15:24:55Z

ast/check.go

@@ -487,6 +487,9 @@ func (rc *refChecker) checkRef(curr *TypeEnv, node *typeTreeNode, ref Ref, idx i
 			return newRefErrInvalid(ref[0].Location, ref, idx, exist, types.S, getOneOfForNode(node))
 		}

+	case Array, Object, *Set:
+		rc.env.tree.PutOne(value, rc.env.Get(value))


Also, I suspect this is not being exercised by any test cases.

tsandall · 2017-07-20T15:34:06Z

ast/check.go

@@ -534,6 +538,19 @@ func (rc *refChecker) checkRefLeaf(tpe types.Type, ref Ref, idx int) *Error {
 			}
 		}

+	case Array, Object, *Set:
+		tpe := rc.env.Get(ref[:idx])


Could you explain what's happening here?

checkRefLeaf is provided with the type of the value referred to by the ref up to (but not including) the index. checkRefLeaf should not have to lookup the type here?

tsandall · 2017-07-20T15:35:20Z

ast/check.go

+			return nil
+		}
+
+		unify1(rc.env, SetTerm(head), exist, false)


Did you consider calling unify1(rc.env, head, keys, false) instead here?

ah yea that would be much better

tsandall · 2017-07-20T15:38:02Z

storage/path.go

@@ -52,6 +52,8 @@ func NewPathForRef(ref ast.Ref) (path Path, err error) {
 				Code:    NotFoundErr,
 				Message: fmt.Sprintf("%v: does not exist", ref),
 			}
+		case ast.Array, ast.Object, *ast.Set:
+			return nil, fmt.Errorf("composites may not be used as ref operands for base documents: %v", ref)


Nit: I'd try to make this error message more concise.

tsandall · 2017-07-20T15:53:36Z

topdown/topdown_test.go

+		{"object", `data.test.r[{"foo": "bar"}]`, `{"foo": "bar"}`},
+		{"set", `data.test.r[{1, 2}]`, "[1, 2]"},
+
+		{"unify array", `data.test.r[[1, x]]`, "[1, 2]"},


I'm wondering if this is the best way to test refs with var operands (or non-ground composite operands). There may be more than one query result--but the test case can also assert on one of them.

tsandall · 2017-07-20T15:54:58Z

topdown/topdown_test.go

+		{"complete doc unify", `data.test.s[[x, y]]`, `[1, 2]`},
+
+		// These should compile, but be undefined.
+		{"array doc", `data.test.a[[1, 2]]`, ``},


Could you elaborate on why this should compile? I'd expect the type checker to return an error w/ invalid ref details here.

mmussomele · 2017-07-20T19:15:54Z

All the issues should be fixed now. The commit IDs have changes due to resolving a merge conflict in ast/parser.go.

tsandall · 2017-07-21T16:15:18Z

This is close, but I think more testing is needed. Specifically, anywhere that was branching based on ref operand type needs to be reviewed. E.g., what happens if you write...

p[[x,y]] { ... }

And then query p[[foo.bar.baz[_], "qux"]]?

I imagine we need to have special handling for nested refs?

What about comprehensions nested inside the composites?

mmussomele · 2017-07-24T21:01:34Z

Ready for another look, fixed up up some checking issues and corrected eval to handle refs nested within composite operands. Comprehensions also work inside the composites.

tsandall

Only two comments (which are related) on the type checking changes.

tsandall · 2017-07-25T15:28:34Z

ast/check.go

+		if !unifies(tpe, types.NewSet(types.A)) {
+			return newRefErrInvalid(ref[0].Location, ref, idx, tpe, types.NewSet(types.A), nil)
+		}
+		unify1(rc.env, head, keys, false)


This should check if the head unified w/ the keys.

if !unify1(...) { // type error }

tsandall · 2017-07-25T15:38:09Z

ast/check_test.go

@@ -738,6 +743,14 @@ func TestCheckRefErrInvalid(t *testing.T) {
 			want:  types.S,
 			oneOf: []Value{String("p"), String("q")},
 		},
+		{
+			note:  "composite ref into non-set",


What about type errors with terms embedded within composite operands? This is related to the comment from above.

For example...

p = {[1],[2],[3]}

And then...p[["foo"]] or p[{"a": b"}].

mmussomele · 2017-07-25T17:56:00Z

Comments addressed.

tsandall · 2017-07-26T16:41:35Z

topdown/topdown.go

 			if err := evalRefRuleResultRec(t, e.Value, tail, path, iter); err != nil {
 				return err
 			}
 			path = path[:len(path)-1]
 			t.Unbind(undo)
 		}
 		return nil
+	case ast.Array, ast.Object, *ast.Set:


This entire function can be simplified to unify the set elements with the next element in the ref. Something like...

head, tail := ref[0], ref[1:] k := head.Value for _, e := range *set { undo, err := evalEqUnify(t, e.Value, k, nil, func(t *Topdown) error { return evalRefRuleResultRec(t, e.Value, tail, path.Append(e), iter) }) if err != nil { return err } t.Unbind(undo) } return nil

0.o it's like magic

tsandall

LGTM. Please squash your commits and then we can merge this.

mmussomele · 2017-07-27T17:43:08Z

Done

mmussomele force-pushed the composite-refs branch 5 times, most recently from 577290c to dd04aca Compare July 20, 2017 00:39

tsandall reviewed Jul 20, 2017

View reviewed changes

mmussomele force-pushed the composite-refs branch from fad742b to 86d5119 Compare July 20, 2017 19:26

tsandall reviewed Jul 25, 2017

View reviewed changes

tsandall reviewed Jul 26, 2017

View reviewed changes

mmussomele force-pushed the composite-refs branch from 702e45b to 69501e1 Compare July 26, 2017 16:57

tsandall approved these changes Jul 26, 2017

View reviewed changes

mmussomele added 2 commits July 27, 2017 10:42

Support composite terms as reference operands

398539c

Update docs with composite reference operands

539afdf

mmussomele force-pushed the composite-refs branch from 69501e1 to 539afdf Compare July 27, 2017 17:42

tsandall merged commit 140599f into open-policy-agent:master Jul 31, 2017

mmussomele deleted the composite-refs branch July 31, 2017 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composite refs #386

Composite refs #386

mmussomele commented Jul 18, 2017 •

edited

Loading

tsandall left a comment

tsandall Jul 20, 2017

mmussomele Jul 20, 2017

tsandall Jul 20, 2017

tsandall Jul 20, 2017

tsandall Jul 20, 2017

mmussomele Jul 20, 2017

tsandall Jul 20, 2017

tsandall Jul 20, 2017

tsandall Jul 20, 2017

tsandall Jul 20, 2017

mmussomele commented Jul 20, 2017 •

edited

Loading

tsandall commented Jul 21, 2017

mmussomele commented Jul 24, 2017

tsandall left a comment

tsandall Jul 25, 2017

tsandall Jul 25, 2017

mmussomele commented Jul 25, 2017

tsandall Jul 26, 2017 •

edited

Loading

mmussomele Jul 26, 2017

mmussomele Jul 26, 2017

tsandall left a comment

mmussomele commented Jul 27, 2017

Composite refs #386

Composite refs #386

Conversation

mmussomele commented Jul 18, 2017 • edited Loading

tsandall 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

mmussomele commented Jul 20, 2017 • edited Loading

tsandall commented Jul 21, 2017

mmussomele commented Jul 24, 2017

tsandall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmussomele commented Jul 25, 2017

tsandall Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsandall left a comment

Choose a reason for hiding this comment

mmussomele commented Jul 27, 2017

mmussomele commented Jul 18, 2017 •

edited

Loading

mmussomele commented Jul 20, 2017 •

edited

Loading

tsandall Jul 26, 2017 •

edited

Loading