Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/cel/environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func DefaultEnvironment(options ...EnvOption) (*cel.Env, error) {
cel.OptionalTypes(),
ext.Encoders(),
library.Random(),
library.Maps(),
}

opts := &envOptions{}
Expand Down
103 changes: 103 additions & 0 deletions pkg/cel/library/maps.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
package library
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing license header

Copy link
Member

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?

Copy link
Member

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 generate should get that done. cc @tomasaschan

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


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 second map overwrite already available keys in the first map.
// Keys must be of type string, value types must be identical in the maps merged.
//
// map(string, T).merge(map(string, T)) -> map(string, T)
//
// 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}
Copy link
Contributor

Choose a reason for hiding this comment

The 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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ?
or
cel.lib.ext.kro.experimental.maps ?

}
Comment on lines +56 to +59
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we edit the library in name for random to match this one and similar patterns in cel-go? https://github.com/kro-run/kro/blob/main/pkg/cel/library/random.go#L53-L55 - https://github.com/google/cel-go/blob/master/ext/lists.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just matched the patterns ;). So, I think changing random would make sense from that perspective.

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we need to rename the folder from library to ext ?
Even a separate repo under kro-run ?


// CompileOptions implements the cel.Library interface method.
func (l *mapsLib) CompileOptions() []cel.EnvOption {
mapType := cel.MapType(cel.TypeParamType("K"), cel.TypeParamType("V"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of a Q: "K" and "V" are not used here. Are those more for documentation ?>
If so can we rename to Key, Value ?

// mapDynType := cel.MapType(cel.DynType, cel.DynType)
opts := []cel.EnvOption{
cel.Function("merge",
cel.MemberOverload("map_merge",
[]*cel.Type{mapType, mapType},
mapType,
cel.BinaryBinding(func(arg1, arg2 ref.Val) ref.Val {
Copy link
Member

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

self, ok := arg1.(traits.Mapper)
if !ok {
return types.ValOrErr(arg1, "no such overload: %v.merge(%v)", arg1.Type(), arg2.Type())
}
other, ok := arg2.(traits.Mapper)
if !ok {
return types.ValOrErr(arg1, "no such overload: %v.merge(%v)", arg1.Type(), arg2.Type())
}
Copy link
Member

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 :)

Copy link
Contributor Author

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.

return merge(self, other)
}),
),
),
}
return opts
}

// ProgramOptions implements the cel.Library interface method.
func (l *mapsLib) ProgramOptions() []cel.ProgramOption {
return []cel.ProgramOption{}
}

// merge merges two maps. Keys from the first map take precedence over keys in
// the second map.
func merge(self traits.Mapper, other traits.Mapper) traits.Mapper {
Copy link
Member

@jakobmoellerdev jakobmoellerdev Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this looks very complex (similar to https://github.com/kro-run/kro/pull/628/files#r2283120971 comment I looked into this a bit). why not go for something like this:

func mergeVals(lhs, rhs ref.Val) ref.Val {
	left, lok := lhs.(traits.Mapper)
	right, rok := rhs.(traits.Mapper)
	if !lok || !rok {
		return types.ValOrErr(lhs, "no such overload: %v.merge(%v)", lhs.Type(), rhs.Type())
	}
	return merge(left, right)
}

// merge returns a new map containing entries from both maps.
// Keys in 'other' overwrite keys in 'self'.
func merge(self, other traits.Mapper) traits.Mapper {
	result := mapperTraitToMutableMapper(other)
	for i := self.Iterator(); i.HasNext().(types.Bool); {
		k := i.Next()
		if !result.Contains(k).(types.Bool) {
			result.Insert(k, self.Get(k))
		}
	}
	return result.ToImmutableMap()
}

// mapperTraitToMutableMapper copies a traits.Mapper into a MutableMap.
func mapperTraitToMutableMapper(m traits.Mapper) traits.MutableMapper {
	vals := make(map[ref.Val]ref.Val, m.Size().(types.Int))
	for it := m.Iterator(); it.HasNext().(types.Bool); {
		k := it.Next()
		vals[k] = m.Get(k)
	}
	return types.NewMutableMap(types.DefaultTypeAdapter, vals)
}

Then you can call

cel.Function("merge",
			cel.MemberOverload("map_merge",
				[]*cel.Type{mapType, mapType},
				mapType,
				cel.BinaryBinding(mergeVals),
			),
		)

var result traits.MutableMapper

if mapVal, ok := other.Value().(map[ref.Val]ref.Val); ok {
result = types.NewMutableMap(types.DefaultTypeAdapter, mapVal)
} else {
result = types.NewMutableMap(types.DefaultTypeAdapter, nil)
for i := other.Iterator(); i.HasNext().(types.Bool); {
k := i.Next()
v := other.Get(k)
result.Insert(k, v)
}
}

for i := self.Iterator(); i.HasNext().(types.Bool); {
k := i.Next()
if result.Contains(k).(types.Bool) {
continue
}
v := self.Get(k)
result.Insert(k, v)
}
return result.ToImmutableMap()
Copy link
Member

@a-hilaly a-hilaly Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm thinking, wouldn't something like this just work?

    // populate with self map
    for it := self.Iterator(); it.HasNext() == types.True; {
        k := it.Next()
        result.Insert(k, self.Get(k))
    }
    
    // overwrides with other map
    for it := other.Iterator(); it.HasNext() == types.True; {
        k := it.Next()
        result.Insert(k, other.Get(k))
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works in general (flip order so that other is written first, then fill in whatever keys aren't there from self); have picked suggestions from @jakobmoellerdev

}
76 changes: 76 additions & 0 deletions pkg/cel/library/maps_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package library
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The 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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 (r := require.New(t) f.ex. and usage of ErrorAssertionFunc)

mapsTests := []struct {
expr string
err string
}{
{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: "ERROR: <input>:1:9: found no matching overload for 'merge' applied to 'map(dyn, dyn).(list(dyn))'"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we want dynamic support in here? If so, either uncomment the test, add todo for the comment if outstanding, or remove the comments

}

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
}