From bb45c937b6967281a21901d768cfc77d8c173db0 Mon Sep 17 00:00:00 2001 From: TristonianJones Date: Tue, 23 May 2023 15:43:33 -0700 Subject: [PATCH 1/2] Fix for macro call tracking with optional list elements --- interpreter/attributes_test.go | 42 ++++++++++++++-------------------- interpreter/interpretable.go | 7 ++---- interpreter/prune_test.go | 40 +++++++++++++++++++++++++++++++- parser/helper.go | 3 ++- parser/unparser_test.go | 20 ++++++++++++++++ 5 files changed, 80 insertions(+), 32 deletions(-) diff --git a/interpreter/attributes_test.go b/interpreter/attributes_test.go index a5c99350..cacf41d7 100644 --- a/interpreter/attributes_test.go +++ b/interpreter/attributes_test.go @@ -24,7 +24,6 @@ import ( "github.com/google/cel-go/checker/decls" "github.com/google/cel-go/common" "github.com/google/cel-go/common/containers" - "github.com/google/cel-go/common/operators" "github.com/google/cel-go/common/types" "github.com/google/cel-go/common/types/ref" "github.com/google/cel-go/parser" @@ -383,7 +382,6 @@ func TestAttributesOptional(t *testing.T) { t.Fatalf("") } attrs := NewAttributeFactory(cont, reg, reg) - tests := []struct { varName string quals []any @@ -860,7 +858,7 @@ func TestAttributeStateTracking(t *testing.T) { var tests = []struct { expr string env []*exprpb.Decl - in map[string]any + in any out ref.Val attrs []*AttributePattern state map[int64]any @@ -1083,14 +1081,14 @@ func TestAttributeStateTracking(t *testing.T) { decls.NewVar("a", decls.NewMapType(decls.String, decls.Dyn)), decls.NewVar("m", decls.NewMapType(decls.Bool, decls.String)), }, - in: map[string]any{ - "a": map[string]any{}, - "m": map[bool]string{true: "world"}, - }, - out: types.Unknown{5}, - attrs: []*AttributePattern{ + in: partialActivation( + map[string]any{ + "a": map[string]any{}, + "m": map[bool]string{true: "world"}, + }, NewAttributePattern("a").QualString("b"), - }, + ), + out: types.Unknown{5}, }, } for _, test := range tests { @@ -1123,9 +1121,16 @@ func TestAttributeStateTracking(t *testing.T) { if len(errors.GetErrors()) != 0 { t.Fatalf(errors.ToDisplayString()) } - attrs := NewAttributeFactory(cont, reg, reg) - if tc.attrs != nil { + in, err := NewActivation(tc.in) + if err != nil { + t.Fatalf("NewActivation(%v) failed: %v", tc.in, err) + } + var attrs AttributeFactory + _, isPartial := in.(PartialActivation) + if isPartial { attrs = NewPartialAttributeFactory(cont, reg, reg) + } else { + attrs = NewAttributeFactory(cont, reg, reg) } interp := NewStandardInterpreter(cont, reg, reg, attrs) // Show that program planning will now produce an error. @@ -1137,7 +1142,6 @@ func TestAttributeStateTracking(t *testing.T) { if err != nil { t.Fatal(err) } - in, _ := NewPartialActivation(tc.in, tc.attrs...) out := i.Eval(in) if types.IsUnknown(tc.out) && types.IsUnknown(out) { if !reflect.DeepEqual(tc.out, out) { @@ -1261,15 +1265,3 @@ func findField(t testing.TB, reg ref.TypeRegistry, typeName, field string) *ref. } return ft } - -func optionalSignatures() []*exprpb.Decl { - return []*exprpb.Decl{ - decls.NewFunction(operators.OptIndex, - decls.NewParameterizedOverload("map_optindex_optional_value", []*exprpb.Type{ - decls.NewMapType(decls.NewTypeParamType("K"), decls.NewTypeParamType("V")), - decls.NewTypeParamType("K")}, - decls.NewOptionalType(decls.NewTypeParamType("V")), - []string{"K", "V"}, - )), - } -} diff --git a/interpreter/interpretable.go b/interpreter/interpretable.go index 7d67a92b..32e2bcb7 100644 --- a/interpreter/interpretable.go +++ b/interpreter/interpretable.go @@ -162,9 +162,6 @@ func (q *testOnlyQualifier) Qualify(vars Activation, obj any) (any, error) { } // QualifyIfPresent returns whether the target field in the test-only expression is present. -// -// This method should never be called as the has() macro and optional syntax are incompatible -// when used on the same field. func (q *testOnlyQualifier) QualifyIfPresent(vars Activation, obj any, presenceOnly bool) (any, bool, error) { // Only ever test for presence. return q.ConstantQualifier.QualifyIfPresent(vars, obj, true) @@ -922,7 +919,7 @@ func (e *evalWatchConstQual) QualifyIfPresent(vars Activation, obj any, presence val = types.WrapErr(err) } else if out != nil { val = e.adapter.NativeToValue(out) - } else if out == nil && presenceOnly { + } else if presenceOnly { val = types.Bool(present) } if present || presenceOnly { @@ -965,7 +962,7 @@ func (e *evalWatchQual) QualifyIfPresent(vars Activation, obj any, presenceOnly val = types.WrapErr(err) } else if out != nil { val = e.adapter.NativeToValue(out) - } else if out == nil && presenceOnly { + } else if presenceOnly { val = types.Bool(present) } if present || presenceOnly { diff --git a/interpreter/prune_test.go b/interpreter/prune_test.go index f8f5f176..edbd8a4b 100644 --- a/interpreter/prune_test.go +++ b/interpreter/prune_test.go @@ -17,14 +17,19 @@ package interpreter import ( "testing" + "github.com/google/cel-go/checker/decls" "github.com/google/cel-go/common" "github.com/google/cel-go/common/containers" + "github.com/google/cel-go/common/operators" "github.com/google/cel-go/common/types" "github.com/google/cel-go/common/types/ref" "github.com/google/cel-go/interpreter/functions" "github.com/google/cel-go/parser" "github.com/google/cel-go/test" - "github.com/google/cel-go/test/proto3pb" + + proto3pb "github.com/google/cel-go/test/proto3pb" + + exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1" ) type testInfo struct { @@ -387,6 +392,27 @@ var testCases = []testInfo{ expr: `[has(a.b), has(c.d)].exists(x, x == true)`, out: `[false, has(c.d)].exists(x, x == true)`, }, + { + in: partialActivation(map[string]any{ + "a": map[string]any{}, + }, "c"), + expr: `[has(a.b), has(c.d)].exists(x, x == true)`, + out: `[false, has(c.d)].exists(x, x == true)`, + }, + { + in: partialActivation(map[string]any{ + "a": map[string]string{}, + }), + expr: `[?a[?0], a.b]`, + out: `[a.b]`, + }, + { + in: partialActivation(map[string]any{ + "a": map[string]string{}, + }, "a"), + expr: `[?a[?0], a.b].exists(x, x == true)`, + out: `[?a[?0], a.b].exists(x, x == true)`, + }, } func TestPrune(t *testing.T) { @@ -483,3 +509,15 @@ func optionalFunctions() []*functions.Overload { }, } } + +func optionalSignatures() []*exprpb.Decl { + return []*exprpb.Decl{ + decls.NewFunction(operators.OptIndex, + decls.NewParameterizedOverload("map_optindex_optional_value", []*exprpb.Type{ + decls.NewMapType(decls.NewTypeParamType("K"), decls.NewTypeParamType("V")), + decls.NewTypeParamType("K")}, + decls.NewOptionalType(decls.NewTypeParamType("V")), + []string{"K", "V"}, + )), + } +} diff --git a/parser/helper.go b/parser/helper.go index 52d7fd8b..8f8f478e 100644 --- a/parser/helper.go +++ b/parser/helper.go @@ -259,7 +259,8 @@ func (p *parserHelper) buildMacroCallArg(expr *exprpb.Expr) *exprpb.Expr { Id: expr.GetId(), ExprKind: &exprpb.Expr_ListExpr{ ListExpr: &exprpb.Expr_CreateList{ - Elements: macroListArgs, + Elements: macroListArgs, + OptionalIndices: listExpr.GetOptionalIndices(), }, }, } diff --git a/parser/unparser_test.go b/parser/unparser_test.go index aea5bdec..c464c101 100644 --- a/parser/unparser_test.go +++ b/parser/unparser_test.go @@ -151,6 +151,26 @@ func TestUnparse(t *testing.T) { in: `[1, 2, 3].map(x, x >= 2, x * 4).filter(x, x <= 10)`, requiresMacroCalls: true, }, + { + name: "comp_chained_opt", + in: `[?a, b[?0], c].map(x, x >= 2, x * 4).filter(x, x <= 10)`, + requiresMacroCalls: true, + }, + { + name: "comp_map_opt", + in: `{?a: b[?0]}.map(k, x >= 2, x * 4)`, + requiresMacroCalls: true, + }, + { + name: "comp_map_opt", + in: `{a: has(b.c)}.exists(k, k != "")`, + requiresMacroCalls: true, + }, + { + name: "comp_nested", + in: `{a: [1, 2].all(i > 0)}.exists(k, k != "")`, + requiresMacroCalls: true, + }, // These expressions will not be wrapped because they haven't met the // conditions required by the provided unparser options From b84a9e46d7134144b8042cfcedf1720f4864aa6f Mon Sep 17 00:00:00 2001 From: TristonianJones Date: Tue, 23 May 2023 15:46:45 -0700 Subject: [PATCH 2/2] Additional coverage tests --- interpreter/prune_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/interpreter/prune_test.go b/interpreter/prune_test.go index edbd8a4b..ac22a421 100644 --- a/interpreter/prune_test.go +++ b/interpreter/prune_test.go @@ -413,6 +413,20 @@ var testCases = []testInfo{ expr: `[?a[?0], a.b].exists(x, x == true)`, out: `[?a[?0], a.b].exists(x, x == true)`, }, + { + in: partialActivation(map[string]any{ + "a": map[string]string{}, + }), + expr: `[?a[?0], a.b].exists(x, x == true)`, + out: `[a.b].exists(x, x == true)`, + }, + { + in: partialActivation(map[string]any{ + "a": map[string]string{}, + }), + expr: `[a[0], a.b].exists(x, x == true)`, + out: `[a[0], a.b].exists(x, x == true)`, + }, } func TestPrune(t *testing.T) {