-
Notifications
You must be signed in to change notification settings - Fork 265
Implement kro CEL library for maps #628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
3bb33f5
1aaecef
efa762e
e28f93e
44ece4d
89467b4
904e210
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,106 @@ | ||||||||
| package library | ||||||||
|
|
||||||||
| import ( | ||||||||
| "math" | ||||||||
|
|
||||||||
| "github.com/google/cel-go/cel" | ||||||||
| "github.com/google/cel-go/common/types" | ||||||||
| "github.com/google/cel-go/common/types/ref" | ||||||||
| "github.com/google/cel-go/common/types/traits" | ||||||||
| ) | ||||||||
|
|
||||||||
| // Maps returns a cel.EnvOption to configure extended functions for map manipulation. | ||||||||
| // | ||||||||
| // # Merge | ||||||||
| // | ||||||||
| // Merges two maps. Keys from the first map take precedence over keys in the second map. | ||||||||
|
||||||||
| // | ||||||||
| // map(string, int).merge(map(string, int)) -> map(string, int) | ||||||||
|
||||||||
| // | ||||||||
| // Examples: | ||||||||
| // | ||||||||
| // {}.merge({}) == {} | ||||||||
| // {}.merge({'a': 1}) == {'a': 1} | ||||||||
| // {'a': 1}.merge({}) == {'a': 1} | ||||||||
| // {'a': 1}.merge({'b': 2}) == {'a': 1, 'b': 2} | ||||||||
| // {'a': 1}.merge({'a': 2, 'b': 2}) == {'a': 1, 'b': 2} | ||||||||
| func Maps(options ...MapsOption) cel.EnvOption { | ||||||||
| l := &mapsLib{version: math.MaxUint32} | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this common to version it to MAX_INT ? |
||||||||
| for _, opt := range options { | ||||||||
| opt(l) | ||||||||
| } | ||||||||
| return cel.Lib(l) | ||||||||
| } | ||||||||
|
|
||||||||
| type mapsLib struct { | ||||||||
| version uint32 | ||||||||
| } | ||||||||
|
|
||||||||
| type MapsOption func(*mapsLib) *mapsLib | ||||||||
|
|
||||||||
| // LibraryName implements the cel.SingletonLibrary interface method. | ||||||||
| func (l *mapsLib) LibraryName() string { | ||||||||
| return "cel.lib.ext.kro.maps" | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a way to indicate this is still experimental/alpha. cel.lib.ext.experimental.maps ? |
||||||||
| } | ||||||||
|
Comment on lines
+56
to
+59
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shall we edit the library in name for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've just matched the patterns ;). So, I think changing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for both of these. Let's change up the random lib name in a separate mini PR. Shouldnt block anything here though since its mostly for conflict resolution
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering if we need to rename the folder from library to ext ? |
||||||||
|
|
||||||||
| // CompileOptions implements the cel.Library interface method. | ||||||||
| func (l *mapsLib) CompileOptions() []cel.EnvOption { | ||||||||
| mapType := cel.MapType(cel.TypeParamType("K"), cel.DynType) | ||||||||
| // mapDynType := cel.MapType(cel.DynType, cel.DynType) | ||||||||
|
||||||||
| mapType := cel.MapType(cel.TypeParamType("K"), cel.DynType) | |
| // mapDynType := cel.MapType(cel.DynType, cel.DynType) | |
| mapType := cel.MapType(cel.TypeParamType("K"), cel.TypeParamType("V") |
I'm wondering if we should restrict this function to only act on maps of similar types? Like map[string]int only merging with map[string]int. Users could leverage macros to transform map types if needed. My highlevel toughts here is that i'd rather restrict this now and relax it later, than try to make a generic function and hit tricky type situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets worse. It allows memory to grow, which undermines some of the core CEL assumptions, if I understand that correctly.
Despite all of this, my motivation is to get from a list of strings (say a list of availability zones to a list of subnet configurations that I can use for setting up a VPC. This would require this feature, but also the ability to merge maps of arbitrary types (not sure the suggested cel.TypeParamType("V") would allow for that).
Upon reflecting on that for some more, I agree. Let's start simple and extend if necessary. We know it is possible, but not need to go there immediately. I'll try to build it with pinned types.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: expose this function separately instead of inlining, and then unit test this function as such instead of having to go through the environment
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if the !ok checks are redundant here - doesn't CEL already validate argument types?
PS: I see what we're doing in https://github.com/kro-run/kro/blob/main/pkg/cel/library/random.go#L53-L55 and i wonder if we should fix that too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, at least in the test added, the error is found prior to any execution in the env.Check call.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
golangcilint is complaining here because merge never returns a non-nil error (which makes it a redundant parameter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, my bad. Removed the unused error return value.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| package library | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/google/cel-go/cel" | ||
| ) | ||
|
|
||
| func TestMaps(t *testing.T) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: using testify can make your life a lot easier here probably (we already use it in other tests in this package): package library
import (
"fmt"
"testing"
"github.com/google/cel-go/cel"
"github.com/stretchr/testify/require"
)
func TestMaps(t *testing.T) {
tests := []struct {
expr string
err require.ErrorAssertionFunc
}{
{expr: `{}.merge({}) == {}`},
{expr: `{}.merge({'a': 1}) == {'a': 1}`},
{expr: `{}.merge({'a': 2.1}) == {'a': 2.1}`},
{expr: `{}.merge({'a': 'foo'}) == {'a': 'foo'}`},
{expr: `{'a': 1}.merge({}) == {'a': 1}`},
{expr: `{'a': 1}.merge({'b': 2}) == {'a': 1, 'b': 2}`},
{expr: `{'a': 1}.merge({'a': 2, 'b': 2}) == {'a': 2, 'b': 2}`},
{expr: `{}.merge([])`, err: func(t require.TestingT, err error, i ...interface{}) {
require.ErrorContains(t, err, "ERROR: <input>:1:9: found no matching overload for 'merge' applied to 'map(dyn, dyn).(list(dyn))'")
}},
}
env := testMapsEnv(t)
for i, tc := range tests {
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
r := require.New(t)
ast, iss := env.Compile(tc.expr)
if tc.err != nil {
tc.err(t, iss.Err())
return
}
r.NoError(iss.Err(), "compile failed for expr: %s", tc.expr)
prg, err := env.Program(ast)
r.NoError(err, "program failed for expr: %s", tc.expr)
out, _, err := prg.Eval(cel.NoVars())
if tc.err != nil {
r.Error(err, "expected error for expr: %s", tc.expr)
r.Contains(err.Error(), tc.err)
} else {
r.NoError(err, "unexpected error for expr: %s", tc.expr)
r.IsType(cel.BoolType, out.Type(), "unexpected type for expr: %s", tc.expr)
r.True(out.Value().(bool), "unexpected result for expr: %s", tc.expr)
}
})
}
}
func testMapsEnv(t *testing.T, opts ...cel.EnvOption) *cel.Env {
t.Helper()
env, err := cel.NewEnv(append([]cel.EnvOption{Maps()}, opts...)...)
require.NoError(t, err, "cel.NewEnv(Maps()) failed")
return env
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, I had started converting the tests to using testify (back when I originally wrote this code, I didn't see that testify was available). Thanks, learnt a few tricks from this ( |
||
| mapsTests := []struct { | ||
| expr string | ||
| err string | ||
| }{ | ||
| {expr: `{}.merge({}) == {}`}, | ||
| {expr: `{}.merge({'a': 1}) == {'a': 1}`}, | ||
| {expr: `{'a': 1}.merge({}) == {'a': 1}`}, | ||
| {expr: `{'a': 1}.merge({'b': 2}) == {'a': 1, 'b': 2}`}, | ||
| {expr: `{'a': 1}.merge({'a': 2, 'b': 2}) == {'a': 1, 'b': 2}`}, | ||
| {expr: `{'a': 1}.merge({'b': 2.1}) == {'a': 1, 'b': 2.1}`}, | ||
| {expr: `{'a': 1}.merge({'b': 'foo'}) == {'a': 1, 'b': 'foo'}`}, | ||
| {expr: `{'a': 1}.merge({'b': {'c': 2}}) == {'a': 1, 'b': {'c': 2}}`}, | ||
|
|
||
| // {expr: `{}.merge([])`, err: "ERROR: <input>:1:9: found no matching overload for 'merge' applied to 'map(dyn, dyn).(list(dyn))'"}, | ||
|
||
| } | ||
|
|
||
| env := testMapsEnv(t) | ||
| for i, tc := range mapsTests { | ||
| t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { | ||
| var asts []*cel.Ast | ||
| pAst, iss := env.Parse(tc.expr) | ||
| if iss.Err() != nil { | ||
| t.Fatalf("env.Parse(%v) failed: %v", tc.expr, iss.Err()) | ||
| } | ||
| asts = append(asts, pAst) | ||
| cAst, iss := env.Check(pAst) | ||
| if iss.Err() != nil { | ||
| t.Fatalf("env.Check(%v) failed: %v", tc.expr, iss.Err()) | ||
| } | ||
| asts = append(asts, cAst) | ||
|
|
||
| for _, ast := range asts { | ||
| prg, err := env.Program(ast) | ||
| if err != nil { | ||
| t.Fatalf("env.Program() failed: %v", err) | ||
| } | ||
| out, _, err := prg.Eval(cel.NoVars()) | ||
| if tc.err != "" { | ||
| if err == nil { | ||
| t.Fatalf("got value %v, wanted error %s for expr: %s", | ||
| out.Value(), tc.err, tc.expr) | ||
| } | ||
| if !strings.Contains(err.Error(), tc.err) { | ||
| t.Errorf("got error %v, wanted error %s for expr: %s", err, tc.err, tc.expr) | ||
| } | ||
| } else if err != nil { | ||
| t.Fatal(err) | ||
| } else if out.Value() != true { | ||
| t.Errorf("got %v, wanted true for expr: %s", out.Value(), tc.expr) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func testMapsEnv(t *testing.T, opts ...cel.EnvOption) *cel.Env { | ||
| t.Helper() | ||
| baseOpts := []cel.EnvOption{ | ||
| Maps(), | ||
| } | ||
| env, err := cel.NewEnv(append(baseOpts, opts...)...) | ||
| if err != nil { | ||
| t.Fatalf("cel.NewEnv(Maps()) failed: %v", err) | ||
| } | ||
| return env | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing license header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@a-hilaly we should maybe write a linter for new files to avoid this. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we had one! :)
go generateshould get that done. cc @tomasaschanThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#640