diff --git a/cel/cel_test.go b/cel/cel_test.go index 713a129f..7cb32afb 100644 --- a/cel/cel_test.go +++ b/cel/cel_test.go @@ -95,6 +95,90 @@ func Test_ExampleWithBuiltins(t *testing.T) { } } +func TestExtendCheckerParity(t *testing.T) { + // Base environment carrying standard library functions + baseEnv, err := NewEnv( + Variable("baseVar", StringType), + ) + if err != nil { + t.Fatalf("NewEnv() failed: %v", err) + } + + // Extended environment adding child variables (K8s CRD pattern) + extEnv, err := baseEnv.Extend( + Variable("value", StringType), + Variable("oldValue", StringType), + ) + if err != nil { + t.Fatalf("baseEnv.Extend() failed: %v", err) + } + + // Equivalent flat environment created from scratch + flatEnv, err := NewEnv( + Variable("baseVar", StringType), + Variable("value", StringType), + Variable("oldValue", StringType), + ) + if err != nil { + t.Fatalf("flat NewEnv() failed: %v", err) + } + + testCases := []struct { + expr string + vars map[string]any + want ref.Val + }{ + { + expr: `value + " " + oldValue + " " + baseVar`, + vars: map[string]any{"value": "new", "oldValue": "old", "baseVar": "base"}, + want: types.String("new old base"), + }, + { + expr: `size(value) > 0 && [1, 2, 3].exists(x, x > 2)`, + vars: map[string]any{"value": "test"}, + want: types.True, + }, + } + + for _, tc := range testCases { + extAst, extIss := extEnv.Compile(tc.expr) + if extIss.Err() != nil { + t.Fatalf("extEnv.Compile(%q) failed: %v", tc.expr, extIss.Err()) + } + flatAst, flatIss := flatEnv.Compile(tc.expr) + if flatIss.Err() != nil { + t.Fatalf("flatEnv.Compile(%q) failed: %v", tc.expr, flatIss.Err()) + } + + if extAst.OutputType().TypeName() != flatAst.OutputType().TypeName() { + t.Errorf("OutputType mismatch for %q: ext %v, flat %v", tc.expr, extAst.OutputType(), flatAst.OutputType()) + } + + extPrg, err := extEnv.Program(extAst) + if err != nil { + t.Fatalf("extEnv.Program() failed: %v", err) + } + flatPrg, err := flatEnv.Program(flatAst) + if err != nil { + t.Fatalf("flatEnv.Program() failed: %v", err) + } + + extOut, _, err := extPrg.Eval(tc.vars) + if err != nil { + t.Fatalf("extPrg.Eval() failed: %v", err) + } + flatOut, _, err := flatPrg.Eval(tc.vars) + if err != nil { + t.Fatalf("flatPrg.Eval() failed: %v", err) + } + + if extOut.Equal(tc.want) != types.True || flatOut.Equal(tc.want) != types.True { + t.Errorf("Eval result mismatch for %q: ext %v, flat %v, want %v", tc.expr, extOut, flatOut, tc.want) + } + } +} + + func TestCompile(t *testing.T) { prg, err := Compile(`"hello " + name`, Variable("name", StringType)) if err != nil { @@ -4003,12 +4087,45 @@ func BenchmarkDynamicDispatch(b *testing.B) { } func BenchmarkProgramPlan(b *testing.B) { - env, err := NewEnv( + b.Run("NewEnv", func(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, err := NewEnv( + Variable("ai", IntType), + Variable("ar", MapType(StringType, StringType)), + ) + if err != nil { + b.Fatalf("NewEnv() failed: %v", err) + } + } + }) + + baseEnv, err := NewEnv() + if err != nil { + b.Fatalf("NewEnv() failed: %v", err) + } + + b.Run("ExtendEnv", func(b *testing.B) { + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _, err := baseEnv.Extend( + Variable("ai", IntType), + Variable("ar", MapType(StringType, StringType)), + ) + if err != nil { + b.Fatalf("baseEnv.Extend() failed: %v", err) + } + } + }) + + env, err := baseEnv.Extend( Variable("ai", IntType), Variable("ar", MapType(StringType, StringType)), ) if err != nil { - b.Fatalf("NewEnv() failed: %v", err) + b.Fatalf("Extend() failed: %v", err) } astSimple, iss := env.Compile("ai == 20 || ar['foo'] == 'bar'") if iss.Err() != nil { diff --git a/cel/decls.go b/cel/decls.go index 55328805..53941146 100644 --- a/cel/decls.go +++ b/cel/decls.go @@ -223,8 +223,8 @@ func FunctionDecls(funcs ...*decls.FunctionDecl) EnvOption { if len(funcs) == 0 { return e, nil } - e.ensureMutableFunctions() var err error + e.ensureMutableFunctions() for _, fn := range funcs { if existing, found := e.functions[fn.Name()]; found { fn, err = existing.Merge(fn) diff --git a/cel/env.go b/cel/env.go index 3d0f8d10..794c62f7 100644 --- a/cel/env.go +++ b/cel/env.go @@ -532,34 +532,17 @@ func (e *Env) CompileSource(src Source) (*Ast, *Issues) { // TypeProvider are immutable, or that their underlying implementations are based on the // ref.TypeRegistry which provides a Copy method which will be invoked by this method. func (e *Env) Extend(opts ...EnvOption) (*Env, error) { - chk, chkErr := e.getCheckerOrError() - if chkErr != nil { + if _, chkErr := e.getCheckerOrError(); chkErr != nil { return nil, chkErr } - prsrOptsCopy := make([]parser.Option, len(e.prsrOpts)) - copy(prsrOptsCopy, e.prsrOpts) - - // The type-checker is configured with Declarations. The declarations may either be provided - // as options which have not yet been validated, or may come from a previous checker instance - // whose types have already been validated. - chkOptsCopy := make([]checker.Option, len(e.chkOpts)) - copy(chkOptsCopy, e.chkOpts) - - // Copy the declarations if needed. - if chk != nil { - // If the type-checker has already been instantiated, then the e.declarations have been - // validated within the chk instance. - chkOptsCopy = append(chkOptsCopy, checker.ValidatedDeclarations(chk)) - } - varsCopy := make([]*decls.VariableDecl, len(e.variables)) - copy(varsCopy, e.variables) - - // Copy macros and program options - macsCopy := make([]parser.Macro, len(e.macros)) - progOptsCopy := make([]ProgramOption, len(e.progOpts)) - copy(macsCopy, e.macros) - copy(progOptsCopy, e.progOpts) + prsrOptsCopy := slices.Clone(e.prsrOpts) + chkOptsCopy := slices.Clone(e.chkOpts) + varsCopy := slices.Clone(e.variables) + macsCopy := slices.Clone(e.macros) + progOptsCopy := slices.Clone(e.progOpts) + validatorsCopy := slices.Clone(e.validators) + costOptsCopy := slices.Clone(e.costOptions) // Copy the adapter / provider if they appear to be mutable. adapter := e.adapter @@ -588,12 +571,6 @@ func (e *Env) Extend(opts ...EnvOption) (*Env, error) { adapter = adapterReg.Copy() } - validatorsCopy := make([]ASTValidator, len(e.validators)) - copy(validatorsCopy, e.validators) - - costOptsCopy := make([]checker.CostOption, len(e.costOptions)) - copy(costOptsCopy, e.costOptions) - ext := &Env{ parent: e, Container: e.Container, @@ -687,25 +664,17 @@ func (e *Env) HasFunction(functionName string) bool { // Functions returns a shallow copy of the Functions, keyed by function name, that have been configured in the environment. func (e *Env) Functions() map[string]*decls.FunctionDecl { - shallowCopy := make(map[string]*decls.FunctionDecl, len(e.functions)) - for nm, fn := range e.functions { - shallowCopy[nm] = fn - } - return shallowCopy + return maps.Clone(e.functions) } // Variables returns a shallow copy of the variables associated with the environment. func (e *Env) Variables() []*decls.VariableDecl { - shallowCopy := make([]*decls.VariableDecl, len(e.variables)) - copy(shallowCopy, e.variables) - return shallowCopy + return slices.Clone(e.variables) } // Macros returns a shallow copy of macros associated with the environment. func (e *Env) Macros() []Macro { - shallowCopy := make([]Macro, len(e.macros)) - copy(shallowCopy, e.macros) - return shallowCopy + return slices.Clone(e.macros) } // HasValidator returns whether a specific ASTValidator has been configured in the environment. @@ -718,9 +687,9 @@ func (e *Env) HasValidator(name string) bool { return false } -// Validators returns the set of ASTValidators configured on the environment. +// Validators returns a shallow copy of the set of ASTValidators configured on the environment. func (e *Env) Validators() []ASTValidator { - return e.validators[:] + return slices.Clone(e.validators) } // Parse parses the input expression value `txt` to a Ast and/or a set of Issues. @@ -1007,6 +976,15 @@ func (e *Env) initChecker() (*checker.Env, error) { chkOpts = append(chkOpts, checker.JSONFieldNames(e.HasFeature(featureJSONFieldNames))) + if e.parent != nil && e.funcsShared { + parentChk, err := e.parent.initChecker() + if err != nil { + e.setCheckerOrError(nil, err) + return + } + chkOpts = append(chkOpts, checker.ValidatedDeclarations(parentChk)) + } + ce, err := checker.NewEnv(e.Container, e.provider, chkOpts...) if err != nil { e.setCheckerOrError(nil, err) @@ -1019,14 +997,16 @@ func (e *Env) initChecker() (*checker.Env, error) { return } // Add the function declarations which are derived from the FunctionDecl instances. - for _, fn := range e.functions { - if fn.IsDeclarationDisabled() { - continue - } - err = ce.AddFunctions(fn) - if err != nil { - e.setCheckerOrError(nil, err) - return + if e.parent == nil || !e.funcsShared { + for _, fn := range e.functions { + if fn.IsDeclarationDisabled() { + continue + } + err = ce.AddFunctions(fn) + if err != nil { + e.setCheckerOrError(nil, err) + return + } } } // Add function declarations here separately. diff --git a/cel/env_test.go b/cel/env_test.go index 4957011e..004ab9f0 100644 --- a/cel/env_test.go +++ b/cel/env_test.go @@ -164,6 +164,37 @@ func TestFormatCELTypeEquivalence(t *testing.T) { } } +func TestEnvExtendDisableDeclaration(t *testing.T) { + baseEnv, err := NewCustomEnv( + Function("foo", + Overload("foo_bool", []*Type{BoolType}, BoolType), + ), + ) + if err != nil { + t.Fatalf("NewCustomEnv() failed: %v", err) + } + _, iss := baseEnv.Compile("foo(true)") + if iss.Err() != nil { + t.Fatalf("baseEnv.Compile(foo(true)) failed: %v", iss.Err()) + } + + childEnv, err := baseEnv.Extend( + Function("foo", + DisableDeclaration(true), + Overload("foo_bool", []*Type{BoolType}, BoolType), + ), + ) + if err != nil { + t.Fatalf("baseEnv.Extend() failed: %v", err) + } + + _, iss = childEnv.Compile("foo(true)") + if iss.Err() == nil { + t.Errorf("childEnv.Compile(foo(true)) succeeded, wanted error") + } +} + + func TestEnvCheckExtendRace(t *testing.T) { t.Parallel() for i := 0; i < 500; i++ { @@ -189,6 +220,116 @@ func TestEnvCheckExtendRace(t *testing.T) { } } +func TestEnvConcurrentExtend(t *testing.T) { + t.Parallel() + baseEnv, err := NewCustomEnv(StdLib()) + if err != nil { + t.Fatalf("NewCustomEnv() failed: %v", err) + } + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + _, err := baseEnv.Extend(Variable(fmt.Sprintf("v%d", id), StringType)) + if err != nil { + t.Errorf("Extend() failed: %v", err) + } + }(i) + } + wg.Wait() +} + +func TestEnvConcurrentExtendAndCompile(t *testing.T) { + t.Parallel() + baseEnv, err := NewCustomEnv(StdLib()) + if err != nil { + t.Fatalf("NewCustomEnv() failed: %v", err) + } + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + varName := fmt.Sprintf("v%d", id) + extEnv, err := baseEnv.Extend(Variable(varName, IntType)) + if err != nil { + t.Errorf("Extend() failed: %v", err) + return + } + ast, iss := extEnv.Compile(fmt.Sprintf("%s > 0", varName)) + if iss.Err() != nil { + t.Errorf("Compile() failed: %v", iss.Err()) + return + } + prg, err := extEnv.Program(ast) + if err != nil { + t.Errorf("Program() failed: %v", err) + return + } + out, _, err := prg.Eval(map[string]any{varName: 10}) + if err != nil { + t.Errorf("Eval() failed: %v", err) + return + } + if out.Value() != true { + t.Errorf("got %v, wanted true", out.Value()) + } + }(i) + } + wg.Wait() +} + +func TestEnvConcurrentExtendWithMutation(t *testing.T) { + t.Parallel() + baseEnv, err := NewCustomEnv(StdLib()) + if err != nil { + t.Fatalf("NewCustomEnv() failed: %v", err) + } + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + fnName := fmt.Sprintf("custom_func_%d", id) + extEnv, err := baseEnv.Extend( + Function(fnName, + Overload(fnName+"_int", []*Type{IntType}, IntType, + UnaryBinding(func(val ref.Val) ref.Val { + return val + }), + ), + ), + ) + if err != nil { + t.Errorf("Extend() failed: %v", err) + return + } + ast, iss := extEnv.Compile(fmt.Sprintf("%s(42) == 42", fnName)) + if iss.Err() != nil { + t.Errorf("Compile() failed: %v", iss.Err()) + return + } + prg, err := extEnv.Program(ast) + if err != nil { + t.Errorf("Program() failed: %v", err) + return + } + out, _, err := prg.Eval(NoVars()) + if err != nil { + t.Errorf("Eval() failed: %v", err) + return + } + if out.Value() != true { + t.Errorf("got %v, wanted true", out.Value()) + } + }(i) + } + wg.Wait() +} + + + func TestEnvPartialVarsError(t *testing.T) { env := testEnv(t) _, err := env.PartialVars(10) diff --git a/cel/library.go b/cel/library.go index 34ca9355..43912eaa 100644 --- a/cel/library.go +++ b/cel/library.go @@ -184,6 +184,9 @@ func (lib *stdLibrary) CompileOptions() []EnvOption { if err = lib.subset.Validate(); err != nil { return nil, err } + if len(funcs) > 0 { + e.ensureMutableFunctions() + } for _, fn := range funcs { existing, found := e.functions[fn.Name()] if found { diff --git a/checker/checker_test.go b/checker/checker_test.go index be84032f..b61a226c 100644 --- a/checker/checker_test.go +++ b/checker/checker_test.go @@ -2870,3 +2870,50 @@ func testFunction(t testing.TB, name string, opts ...decls.FunctionOpt) *decls.F } return fn } + +func TestVarsInheritance(t *testing.T) { + // Parent environment containing inherited variables 'y' and 'x' + parentEnv, err := NewEnv(containers.DefaultContainer, newTestRegistry(t)) + if err != nil { + t.Fatalf("NewEnv() failed: %v", err) + } + err = parentEnv.AddFunctions(stdlib.Functions()...) + if err != nil { + t.Fatalf("parentEnv.AddFunctions() failed: %v", err) + } + err = parentEnv.AddIdents(decls.NewVariable("z", types.IntType)) + if err != nil { + t.Fatalf("parentEnv.AddIdents() failed: %v", err) + } + + // Child environment inheriting declarations from parentEnv + childEnv, err := NewEnv(containers.DefaultContainer, newTestRegistry(t), ValidatedDeclarations(parentEnv)) + if err != nil { + t.Fatalf("NewEnv(ValidatedDeclarations) failed: %v", err) + } + err = childEnv.AddIdents(decls.NewVariable("y", types.NewListType(types.IntType))) + if err != nil { + t.Fatalf("childEnv.AddIdents() failed: %v", err) + } + + src := common.NewTextSource(`y + [1, 2, 3].filter(x, .z > x)`) + p, err := parser.NewParser(parser.Macros(parser.AllMacros...)) + if err != nil { + t.Fatalf("parser.NewParser() failed: %v", err) + } + parsedAst, iss := p.Parse(src) + if len(iss.GetErrors()) > 0 { + t.Fatalf("parser.Parse() failed: %v", iss.ToDisplayString()) + } + + checkedAst, iss := Check(parsedAst, src, childEnv) + if len(iss.GetErrors()) > 0 { + t.Fatalf("Check() failed: %v", iss.ToDisplayString()) + } + + wantType := types.NewListType(types.IntType) + gotType := checkedAst.GetType(checkedAst.Expr().ID()) + if !gotType.IsExactType(wantType) { + t.Errorf("got result type %v, wanted %v", gotType, wantType) + } +} diff --git a/checker/env.go b/checker/env.go index 477918c4..9c3de951 100644 --- a/checker/env.go +++ b/checker/env.go @@ -97,7 +97,7 @@ func NewEnv(container *containers.Container, provider types.Provider, opts ...Op filteredOverloadIDs = make(map[string]struct{}) } if envOptions.validatedDeclarations != nil { - declarations = envOptions.validatedDeclarations.Copy() + declarations = envOptions.validatedDeclarations.PushInherited() } return &Env{ container: container, diff --git a/checker/env_test.go b/checker/env_test.go index 2ec7f13f..c3a6aa35 100644 --- a/checker/env_test.go +++ b/checker/env_test.go @@ -77,20 +77,6 @@ func BenchmarkNewStdEnv(b *testing.B) { } } -func BenchmarkCopyDeclarations(b *testing.B) { - env, err := NewEnv(containers.DefaultContainer, newTestRegistry(b)) - if err != nil { - b.Fatalf("NewEnv() failed: %v", err) - } - err = env.AddFunctions(stdlib.Functions()...) - if err != nil { - b.Fatalf("env.AddFunctions(stdlib.Functions()...) failed: %v", err) - } - for i := 0; i < b.N; i++ { - env.validatedDeclarations().Copy() - } -} - func newStdEnv(t *testing.T) *Env { t.Helper() env, err := NewEnv(containers.DefaultContainer, newTestRegistry(t)) diff --git a/checker/options.go b/checker/options.go index af714323..10d3bcbc 100644 --- a/checker/options.go +++ b/checker/options.go @@ -33,8 +33,8 @@ func CrossTypeNumericComparisons(enabled bool) Option { } } -// ValidatedDeclarations provides a references to validated declarations which will be copied -// into new checker instances. +// ValidatedDeclarations provides a reference to validated declarations which will be inherited +// as a parent scope without copying. func ValidatedDeclarations(env *Env) Option { return func(opts *options) error { opts.validatedDeclarations = env.validatedDeclarations() @@ -49,3 +49,4 @@ func JSONFieldNames(enabled bool) Option { return nil } } + diff --git a/checker/scopes.go b/checker/scopes.go index 9ae9832e..2fcfdf29 100644 --- a/checker/scopes.go +++ b/checker/scopes.go @@ -25,8 +25,9 @@ import ( // Each Groups value is a mapping of names to Decls in the ident and function namespaces. // Lookups are performed such that bindings in inner scopes shadow those in outer scopes. type Scopes struct { - parent *Scopes - scopes *Group + parent *Scopes + inherited *Scopes + scopes *Group } // newScopes creates a new, empty Scopes. @@ -37,19 +38,6 @@ func newScopes() *Scopes { } } -// Copy creates a copy of the current Scopes values, including a copy of its parent if non-nil. -func (s *Scopes) Copy() *Scopes { - cpy := newScopes() - if s == nil { - return cpy - } - if s.parent != nil { - cpy.parent = s.parent.Copy() - } - cpy.scopes = s.scopes.copy() - return cpy -} - // Push creates a new Scopes value which references the current Scope as its parent. func (s *Scopes) Push() *Scopes { return &Scopes{ @@ -58,6 +46,14 @@ func (s *Scopes) Push() *Scopes { } } +// PushInherited creates a new Scopes value which references the current Scope as its inherited parent. +func (s *Scopes) PushInherited() *Scopes { + return &Scopes{ + inherited: s, + scopes: newGroup(), + } +} + // Pop returns the parent Scopes value for the current scope, or the current scope if the parent // is nil. func (s *Scopes) Pop() *Scopes { @@ -74,20 +70,6 @@ func (s *Scopes) AddIdent(decl *decls.VariableDecl) { s.scopes.idents[decl.Name()] = decl } -// FindIdent finds the first ident Decl with a matching name in Scopes, or nil if one cannot be -// found. -// Note: The search is performed from innermost to outermost. -func (s *Scopes) FindIdent(name string) *decls.VariableDecl { - name = strings.TrimPrefix(name, ".") - if ident, found := s.scopes.idents[name]; found { - return ident - } - if s.parent != nil { - return s.parent.FindIdent(name) - } - return nil -} - // FindIdentInScope finds the first ident Decl with a matching name in the current Scopes value, or // nil if one does not exist. // Note: The search is only performed on the current scope and does not search outer scopes. @@ -116,7 +98,13 @@ func (s *Scopes) FindGlobalIdent(name string) *decls.VariableDecl { for scope.parent != nil { scope = scope.parent } - return scope.FindIdentInScope(name) + if ident := scope.FindIdentInScope(name); ident != nil { + return ident + } + if scope.inherited != nil { + return scope.inherited.FindGlobalIdent(name) + } + return nil } // SetFunction adds the function Decl to the current scope. @@ -134,7 +122,14 @@ func (s *Scopes) FindFunction(name string) *decls.FunctionDecl { return fn } if s.parent != nil { - return s.parent.FindFunction(name) + if fn := s.parent.FindFunction(name); fn != nil { + return fn + } + } + if s.inherited != nil { + if fn := s.inherited.FindFunction(name); fn != nil { + return fn + } } return nil } @@ -147,22 +142,6 @@ type Group struct { functions map[string]*decls.FunctionDecl } -// copy creates a new Group instance with a shallow copy of the variables and functions. -// If callers need to mutate the exprpb.Decl definitions for a Function, they should copy-on-write. -func (g *Group) copy() *Group { - cpy := &Group{ - idents: make(map[string]*decls.VariableDecl, len(g.idents)), - functions: make(map[string]*decls.FunctionDecl, len(g.functions)), - } - for n, id := range g.idents { - cpy.idents[n] = id - } - for n, fn := range g.functions { - cpy.functions[n] = fn - } - return cpy -} - // newGroup creates a new Group with empty maps for identifiers and functions. func newGroup() *Group { return &Group{ diff --git a/common/types/provider.go b/common/types/provider.go index 76285143..2321828d 100644 --- a/common/types/provider.go +++ b/common/types/provider.go @@ -18,6 +18,7 @@ import ( "fmt" "maps" "reflect" + "sync/atomic" "time" "google.golang.org/protobuf/proto" @@ -92,6 +93,7 @@ type Registry struct { revTypeMap map[string]*Type structTypes map[string]StructTypeDescriptor reflectTypes map[reflect.Type]StructTypeDescriptor + shared atomic.Bool pbdb *pb.Db provider Provider adapter Adapter @@ -218,15 +220,31 @@ func ComposeTypes(provider Provider, adapter Adapter, types ...any) (Provider, A // Copy copies the current state of the registry into its own memory space. func (p *Registry) Copy() *Registry { - copy := NewEmptyRegistry() - copy.pbdb = p.pbdb.Copy() - copy.provider = p.provider - copy.adapter = p.adapter - copy.nativeOptions = p.nativeOptions - maps.Copy(copy.revTypeMap, p.revTypeMap) - maps.Copy(copy.structTypes, p.structTypes) - maps.Copy(copy.reflectTypes, p.reflectTypes) - return copy + if p == nil { + return nil + } + p.shared.Store(true) + cpy := &Registry{ + revTypeMap: p.revTypeMap, + structTypes: p.structTypes, + reflectTypes: p.reflectTypes, + nativeOptions: p.nativeOptions, + pbdb: p.pbdb, + provider: p.provider, + adapter: p.adapter, + } + cpy.shared.Store(true) + return cpy +} + +func (p *Registry) ensureMutable() { + if p.shared.Load() { + p.revTypeMap = maps.Clone(p.revTypeMap) + p.structTypes = maps.Clone(p.structTypes) + p.reflectTypes = maps.Clone(p.reflectTypes) + p.pbdb = p.pbdb.Copy() + p.shared.Store(false) + } } // JSONFieldNames returns whether json field names are enabled in this registry. @@ -239,6 +257,7 @@ func (p *Registry) WithJSONFieldNames(enabled bool) error { if enabled == p.pbdb.JSONFieldNames() { return nil } + p.ensureMutable() newDB := pb.NewDb(pb.JSONFieldNames(enabled)) files := p.pbdb.FileDescriptions() for _, fd := range files { @@ -447,6 +466,7 @@ func (p *Registry) NewValue(structType string, fields map[string]ref.Val) ref.Va // RegisterDescriptor registers the contents of a protocol buffer `FileDescriptor`. func (p *Registry) RegisterDescriptor(fileDesc protoreflect.FileDescriptor) error { + p.ensureMutable() fd, err := p.pbdb.RegisterDescriptor(fileDesc) if err != nil { return err @@ -456,6 +476,7 @@ func (p *Registry) RegisterDescriptor(fileDesc protoreflect.FileDescriptor) erro // RegisterMessage registers a protocol buffer message and its dependencies. func (p *Registry) RegisterMessage(message proto.Message) error { + p.ensureMutable() fd, err := p.pbdb.RegisterMessage(message) if err != nil { return err @@ -488,6 +509,7 @@ func (p *Registry) RegisterType(types ...ref.Type) error { continue } + p.ensureMutable() typeName := t.TypeName() p.revTypeMap[typeName] = celType if st, ok := t.(StructTypeDescriptor); ok { diff --git a/common/types/provider_test.go b/common/types/provider_test.go index 2197d38a..65f695b1 100644 --- a/common/types/provider_test.go +++ b/common/types/provider_test.go @@ -20,6 +20,7 @@ import ( "reflect" "sort" "strings" + "sync" "testing" "time" @@ -60,8 +61,219 @@ func TestRegistryCopy(t *testing.T) { } }) } + + t.Run("nil registry", func(t *testing.T) { + var reg *Registry + if reg.Copy() != nil { + t.Error("expected nil registry copy to return nil") + } + }) +} + +func assertShared(t *testing.T, reg *Registry) { + t.Helper() + if !reg.shared.Load() { + t.Errorf("registry.shared = false, want true") + } +} + +func assertUnshared(t *testing.T, reg *Registry) { + t.Helper() + if reg.shared.Load() { + t.Errorf("registry.shared = true, want false") + } +} + +func newSharedRegistryPair(t *testing.T, opts ...RegistryOption) (*Registry, *Registry) { + t.Helper() + reg := newTestRegistry(t, opts...) + copied := reg.Copy() + assertShared(t, reg) + assertShared(t, copied) + return reg, copied +} + +func TestRegistrySharedOnCopy(t *testing.T) { + reg := NewEmptyRegistry() + assertUnshared(t, reg) + + copied := reg.Copy() + assertShared(t, reg) + assertShared(t, copied) + + if !reflect.DeepEqual(reg, copied) { + t.Errorf("reg.Copy() expected equivalent registries") + } +} + +func TestRegistryUnshared_RegisterTypeOnCopy(t *testing.T) { + reg, copied := newSharedRegistryPair(t) + + customType := NewObjectType("custom.TypeA") + if err := copied.RegisterType(customType); err != nil { + t.Fatalf("RegisterType() failed: %v", err) + } + + assertUnshared(t, copied) + assertShared(t, reg) + + if _, found := copied.FindIdent("custom.TypeA"); !found { + t.Errorf("copied.FindIdent('custom.TypeA') expected found == true") + } + if _, found := reg.FindIdent("custom.TypeA"); found { + t.Errorf("reg.FindIdent('custom.TypeA') expected found == false after mutating copy") + } + + // Subsequent mutation on already unshared copy stays unshared + customTypeB := NewObjectType("custom.TypeB") + if err := copied.RegisterType(customTypeB); err != nil { + t.Fatalf("RegisterType() failed: %v", err) + } + assertUnshared(t, copied) + if _, found := copied.FindIdent("custom.TypeB"); !found { + t.Errorf("copied.FindIdent('custom.TypeB') expected found == true") + } + if _, found := reg.FindIdent("custom.TypeB"); found { + t.Errorf("reg.FindIdent('custom.TypeB') expected found == false") + } +} + +func TestRegistryUnshared_RegisterTypeOnOriginal(t *testing.T) { + reg, copied := newSharedRegistryPair(t) + + customType := NewObjectType("custom.TypeOrig") + if err := reg.RegisterType(customType); err != nil { + t.Fatalf("RegisterType() failed: %v", err) + } + + assertUnshared(t, reg) + assertShared(t, copied) + + if _, found := reg.FindIdent("custom.TypeOrig"); !found { + t.Errorf("reg.FindIdent('custom.TypeOrig') expected found == true") + } + if _, found := copied.FindIdent("custom.TypeOrig"); found { + t.Errorf("copied.FindIdent('custom.TypeOrig') expected found == false after mutating original") + } +} + +func TestRegistryUnshared_RegisterMessage(t *testing.T) { + reg, copied := newSharedRegistryPair(t) + + if err := copied.RegisterMessage(&proto3pb.TestAllTypes{}); err != nil { + t.Fatalf("RegisterMessage() failed: %v", err) + } + + assertUnshared(t, copied) + assertShared(t, reg) + + if _, found := copied.FindStructType("google.expr.proto3.test.TestAllTypes"); !found { + t.Errorf("copied.FindStructType() expected found == true") + } + if _, found := reg.FindStructType("google.expr.proto3.test.TestAllTypes"); found { + t.Errorf("reg.FindStructType() expected found == false") + } +} + +func TestRegistryUnshared_RegisterDescriptor(t *testing.T) { + reg, copied := newSharedRegistryPair(t) + + err := copied.RegisterDescriptor(proto3pb.GlobalEnum_GOO.Descriptor().ParentFile()) + if err != nil { + t.Fatalf("RegisterDescriptor() failed: %v", err) + } + + assertUnshared(t, copied) + assertShared(t, reg) + + enumVal := copied.EnumValue("google.expr.proto3.test.GlobalEnum.GOO") + if IsError(enumVal) || enumVal.(Int) != Int(proto3pb.GlobalEnum_GOO.Number()) { + t.Errorf("copied.EnumValue() got %v, wanted %v", enumVal, proto3pb.GlobalEnum_GOO.Number()) + } + origEnumVal := reg.EnumValue("google.expr.proto3.test.GlobalEnum.GOO") + if !IsError(origEnumVal) { + t.Errorf("reg.EnumValue() expected error, got %v", origEnumVal) + } +} + +func TestRegistryUnshared_WithJSONFieldNames(t *testing.T) { + reg, copied := newSharedRegistryPair(t, ProtoTypeDefs(&proto3pb.TestAllTypes{})) + + if err := copied.WithJSONFieldNames(true); err != nil { + t.Fatalf("WithJSONFieldNames() failed: %v", err) + } + + assertUnshared(t, copied) + assertShared(t, reg) + + if !copied.JSONFieldNames() { + t.Errorf("copied.JSONFieldNames() expected true, got false") + } + if reg.JSONFieldNames() { + t.Errorf("reg.JSONFieldNames() expected false, got true") + } } +func TestRegistryUnshared_ChainedCopies(t *testing.T) { + r1 := NewEmptyRegistry() + r2 := r1.Copy() + r3 := r2.Copy() + + assertShared(t, r1) + assertShared(t, r2) + assertShared(t, r3) + + typeInR2 := NewObjectType("custom.InR2") + if err := r2.RegisterType(typeInR2); err != nil { + t.Fatalf("RegisterType() failed: %v", err) + } + + assertUnshared(t, r2) + assertShared(t, r1) + assertShared(t, r3) + + if _, found := r2.FindIdent("custom.InR2"); !found { + t.Errorf("r2.FindIdent('custom.InR2') expected found == true") + } + if _, found := r1.FindIdent("custom.InR2"); found { + t.Errorf("r1.FindIdent('custom.InR2') expected found == false") + } + if _, found := r3.FindIdent("custom.InR2"); found { + t.Errorf("r3.FindIdent('custom.InR2') expected found == false") + } + + typeInR3 := NewObjectType("custom.InR3") + if err := r3.RegisterType(typeInR3); err != nil { + t.Fatalf("RegisterType() failed: %v", err) + } + + assertUnshared(t, r3) + if _, found := r3.FindIdent("custom.InR3"); !found { + t.Errorf("r3.FindIdent('custom.InR3') expected found == true") + } + if _, found := r1.FindIdent("custom.InR3"); found { + t.Errorf("r1.FindIdent('custom.InR3') expected found == false") + } + if _, found := r2.FindIdent("custom.InR3"); found { + t.Errorf("r2.FindIdent('custom.InR3') expected found == false") + } +} + +func TestRegistryConcurrentCopy(t *testing.T) { + reg := NewEmptyRegistry() + var wg sync.WaitGroup + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _ = reg.Copy() + }() + } + wg.Wait() +} + + + func TestRegistryRegisterType(t *testing.T) { tests := []struct { name string