From 2a25a91d35dc421e6d20773269d83c368e2fc910 Mon Sep 17 00:00:00 2001 From: Alex Shtin Date: Fri, 31 Jan 2025 11:52:46 -0800 Subject: [PATCH] Use pointers, Luke --- chasm/export_test.go | 41 +++++++++++++ chasm/library.go | 8 +-- chasm/library_mock.go | 8 +-- chasm/registrable_component.go | 10 +--- chasm/registrable_task.go | 10 +--- chasm/registry.go | 105 +++++++++++++++------------------ chasm/registry_test.go | 44 +++++++++----- 7 files changed, 130 insertions(+), 96 deletions(-) create mode 100644 chasm/export_test.go diff --git a/chasm/export_test.go b/chasm/export_test.go new file mode 100644 index 00000000000..83a8b601a22 --- /dev/null +++ b/chasm/export_test.go @@ -0,0 +1,41 @@ +// The MIT License +// +// Copyright (c) 2020 Temporal Technologies Inc. All rights reserved. +// +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package chasm + +func (r *Registry) Component(fqn string) (*RegistrableComponent, bool) { + return r.component(fqn) +} + +func (r *Registry) Task(fqn string) (*RegistrableTask, bool) { + return r.task(fqn) +} + +func (r *Registry) ComponentFor(componentInstance any) (*RegistrableComponent, bool) { + return r.componentFor(componentInstance) +} + +func (r *Registry) TaskFor(taskInstance any) (*RegistrableTask, bool) { + return r.taskFor(taskInstance) +} diff --git a/chasm/library.go b/chasm/library.go index 31a14896daf..86cde8137b0 100644 --- a/chasm/library.go +++ b/chasm/library.go @@ -29,8 +29,8 @@ package chasm type ( Library interface { Name() string - Components() []RegistrableComponent - Tasks() []RegistrableTask + Components() []*RegistrableComponent + Tasks() []*RegistrableTask // Service() mustEmbedUnimplementedLibrary() @@ -39,11 +39,11 @@ type ( UnimplementedLibrary struct{} ) -func (UnimplementedLibrary) Components() []RegistrableComponent { +func (UnimplementedLibrary) Components() []*RegistrableComponent { return nil } -func (UnimplementedLibrary) Tasks() []RegistrableTask { +func (UnimplementedLibrary) Tasks() []*RegistrableTask { return nil } diff --git a/chasm/library_mock.go b/chasm/library_mock.go index 7af8e63088b..281f162c831 100644 --- a/chasm/library_mock.go +++ b/chasm/library_mock.go @@ -63,10 +63,10 @@ func (m *MockLibrary) EXPECT() *MockLibraryMockRecorder { } // Components mocks base method. -func (m *MockLibrary) Components() []RegistrableComponent { +func (m *MockLibrary) Components() []*RegistrableComponent { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Components") - ret0, _ := ret[0].([]RegistrableComponent) + ret0, _ := ret[0].([]*RegistrableComponent) return ret0 } @@ -91,10 +91,10 @@ func (mr *MockLibraryMockRecorder) Name() *gomock.Call { } // Tasks mocks base method. -func (m *MockLibrary) Tasks() []RegistrableTask { +func (m *MockLibrary) Tasks() []*RegistrableTask { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Tasks") - ret0, _ := ret[0].([]RegistrableTask) + ret0, _ := ret[0].([]*RegistrableTask) return ret0 } diff --git a/chasm/registrable_component.go b/chasm/registrable_component.go index 4be99a351dd..10993e6dacd 100644 --- a/chasm/registrable_component.go +++ b/chasm/registrable_component.go @@ -28,10 +28,6 @@ import ( "reflect" ) -var ( - EmptyRegistrableComponent = RegistrableComponent{} -) - type ( RegistrableComponent struct { name string @@ -48,14 +44,14 @@ type ( func NewRegistrableComponent[C Component]( name string, opts ...RegistrableComponentOption, -) RegistrableComponent { - rc := RegistrableComponent{ +) *RegistrableComponent { + rc := &RegistrableComponent{ name: name, goType: reflect.TypeFor[C](), shardingFn: func(_ EntityKey) string { return "" }, } for _, opt := range opts { - opt(&rc) + opt(rc) } return rc } diff --git a/chasm/registrable_task.go b/chasm/registrable_task.go index f5e8269d7d0..405e0aef4cd 100644 --- a/chasm/registrable_task.go +++ b/chasm/registrable_task.go @@ -28,10 +28,6 @@ import ( "reflect" ) -var ( - EmptyRegistrableTask = RegistrableTask{} -) - type ( RegistrableTask struct { name string @@ -48,15 +44,15 @@ func NewRegistrableTask[C any, T any]( name string, handler TaskHandler[C, T], opts ...RegistrableTaskOption, -) RegistrableTask { - rt := RegistrableTask{ +) *RegistrableTask { + rt := &RegistrableTask{ name: name, goType: reflect.TypeFor[T](), componentGoType: reflect.TypeFor[C](), handler: handler, } for _, opt := range opts { - opt(&rt) + opt(rt) } return rt } diff --git a/chasm/registry.go b/chasm/registry.go index 5d3d687f0cc..ff9e3d22ff1 100644 --- a/chasm/registry.go +++ b/chasm/registry.go @@ -38,20 +38,20 @@ var ( type ( Registry struct { - components map[string]RegistrableComponent // fully qualified name -> component - componentNames map[reflect.Type]string // component type -> fully qualified name + componentByName map[string]*RegistrableComponent // fully qualified name -> component + componentByType map[reflect.Type]*RegistrableComponent // component type -> component - tasks map[string]RegistrableTask // fully qualified name -> task - taskNames map[reflect.Type]string // task type -> fully qualified name + taskByName map[string]*RegistrableTask // fully qualified name -> task + taskByType map[reflect.Type]*RegistrableTask // task type -> task } ) func NewRegistry() *Registry { return &Registry{ - components: make(map[string]RegistrableComponent), - componentNames: make(map[reflect.Type]string), - tasks: make(map[string]RegistrableTask), - taskNames: make(map[reflect.Type]string), + componentByName: make(map[string]*RegistrableComponent), + componentByType: make(map[reflect.Type]*RegistrableComponent), + taskByName: make(map[string]*RegistrableTask), + taskByType: make(map[reflect.Type]*RegistrableTask), } } @@ -72,36 +72,24 @@ func (r *Registry) Register(lib Library) error { return nil } -func (r *Registry) Component(fqn string) (RegistrableComponent, bool) { - rc, ok := r.components[fqn] - if !ok { - return EmptyRegistrableComponent, false - } +func (r *Registry) component(fqn string) (*RegistrableComponent, bool) { + rc, ok := r.componentByName[fqn] return rc, ok } -func (r *Registry) Task(fqn string) (RegistrableTask, bool) { - rt, ok := r.tasks[fqn] - if !ok { - return EmptyRegistrableTask, false - } +func (r *Registry) task(fqn string) (*RegistrableTask, bool) { + rt, ok := r.taskByName[fqn] return rt, ok } -func (r *Registry) ComponentFor(componentInstance any) (RegistrableComponent, bool) { - fqn, ok := r.componentNames[reflect.TypeOf(componentInstance)] - if !ok { - return EmptyRegistrableComponent, false - } - return r.Component(fqn) +func (r *Registry) componentFor(componentInstance any) (*RegistrableComponent, bool) { + rt, ok := r.componentByType[reflect.TypeOf(componentInstance)] + return rt, ok } -func (r *Registry) TaskFor(taskInstance any) (RegistrableTask, bool) { - fqn, ok := r.taskNames[reflect.TypeOf(taskInstance)] - if !ok { - return EmptyRegistrableTask, false - } - return r.Task(fqn) +func (r *Registry) taskFor(taskInstance any) (*RegistrableTask, bool) { + rt, ok := r.taskByType[reflect.TypeOf(taskInstance)] + return rt, ok } func (r *Registry) fqn(libName, name string) string { @@ -110,54 +98,55 @@ func (r *Registry) fqn(libName, name string) string { func (r *Registry) registerComponent( libName string, - c RegistrableComponent, + rc *RegistrableComponent, ) error { - if err := r.validateName(c.name); err != nil { + if err := r.validateName(rc.name); err != nil { return err } - fqn := r.fqn(libName, c.name) - if _, ok := r.components[fqn]; ok { + fqn := r.fqn(libName, rc.name) + if _, ok := r.componentByName[fqn]; ok { return fmt.Errorf("component %s is already registered", fqn) } - // TODO: this step is redundant. c.goType implements Component interface, therefore it must be a struct. - if !(c.goType.Kind() == reflect.Struct || - (c.goType.Kind() == reflect.Ptr && c.goType.Elem().Kind() == reflect.Struct)) { - return fmt.Errorf("component type %s must be struct or pointer to struct", c.goType.String()) + // rc.goType implements Component interface; therefore, it must be a struct. + // This check to protect against the interface itself being registered. + if !(rc.goType.Kind() == reflect.Struct || + (rc.goType.Kind() == reflect.Ptr && rc.goType.Elem().Kind() == reflect.Struct)) { + return fmt.Errorf("component type %s must be struct or pointer to struct", rc.goType.String()) } - if _, ok := r.componentNames[c.goType]; ok { - return fmt.Errorf("component type %s is already registered", c.goType.String()) + if _, ok := r.componentByType[rc.goType]; ok { + return fmt.Errorf("component type %s is already registered", rc.goType.String()) } - r.components[fqn] = c - r.componentNames[c.goType] = fqn + r.componentByName[fqn] = rc + r.componentByType[rc.goType] = rc return nil } func (r *Registry) registerTask( libName string, - t RegistrableTask, + rt *RegistrableTask, ) error { - if err := r.validateName(t.name); err != nil { + if err := r.validateName(rt.name); err != nil { return err } - fqn := r.fqn(libName, t.name) - if _, ok := r.tasks[fqn]; ok { + fqn := r.fqn(libName, rt.name) + if _, ok := r.taskByName[fqn]; ok { return fmt.Errorf("task %s is already registered", fqn) } - if !(t.goType.Kind() == reflect.Struct || - (t.goType.Kind() == reflect.Ptr && t.goType.Elem().Kind() == reflect.Struct)) { - return fmt.Errorf("task type %s must be struct or pointer to struct", t.goType.String()) + if !(rt.goType.Kind() == reflect.Struct || + (rt.goType.Kind() == reflect.Ptr && rt.goType.Elem().Kind() == reflect.Struct)) { + return fmt.Errorf("task type %s must be struct or pointer to struct", rt.goType.String()) } - if _, ok := r.taskNames[t.goType]; ok { - return fmt.Errorf("task type %s is already registered", t.goType.String()) + if _, ok := r.taskByType[rt.goType]; ok { + return fmt.Errorf("task type %s is already registered", rt.goType.String()) } - if !(t.componentGoType.Kind() == reflect.Interface || - (t.componentGoType.Kind() == reflect.Struct || - (t.componentGoType.Kind() == reflect.Ptr && t.componentGoType.Elem().Kind() == reflect.Struct)) && - t.componentGoType.AssignableTo(reflect.TypeOf((*Component)(nil)).Elem())) { - return fmt.Errorf("component type %s must be and interface or struct that implements Component interface", t.componentGoType.String()) + if !(rt.componentGoType.Kind() == reflect.Interface || + (rt.componentGoType.Kind() == reflect.Struct || + (rt.componentGoType.Kind() == reflect.Ptr && rt.componentGoType.Elem().Kind() == reflect.Struct)) && + rt.componentGoType.AssignableTo(reflect.TypeOf((*Component)(nil)).Elem())) { + return fmt.Errorf("component type %s must be and interface or struct that implements Component interface", rt.componentGoType.String()) } - r.tasks[fqn] = t - r.taskNames[t.goType] = fqn + r.taskByName[fqn] = rt + r.taskByType[rt.goType] = rt return nil } diff --git a/chasm/registry_test.go b/chasm/registry_test.go index f45b6c3ee35..d4c2ce2401e 100644 --- a/chasm/registry_test.go +++ b/chasm/registry_test.go @@ -45,7 +45,7 @@ func TestRegistry_RegisterComponents_Success(t *testing.T) { ctrl := gomock.NewController(t) lib := chasm.NewMockLibrary(ctrl) lib.EXPECT().Name().Return("TestLibrary").AnyTimes() - lib.EXPECT().Components().Return([]chasm.RegistrableComponent{ + lib.EXPECT().Components().Return([]*chasm.RegistrableComponent{ chasm.NewRegistrableComponent[*chasm.MockComponent]("Component1"), }) @@ -60,7 +60,7 @@ func TestRegistry_RegisterComponents_Success(t *testing.T) { missingRC, ok := r.Component("TestLibrary.Component2") require.False(t, ok) - require.Equal(t, chasm.EmptyRegistrableComponent, missingRC) + require.Nil(t, missingRC) cInstance1 := chasm.NewMockComponent(ctrl) rc2, ok := r.ComponentFor(cInstance1) @@ -70,7 +70,7 @@ func TestRegistry_RegisterComponents_Success(t *testing.T) { cInstance2 := "invalid component instance" rc3, ok := r.ComponentFor(cInstance2) require.False(t, ok) - require.Equal(t, chasm.EmptyRegistrableComponent, rc3) + require.Nil(t, rc3) } func TestRegistry_RegisterTasks_Success(t *testing.T) { @@ -80,7 +80,7 @@ func TestRegistry_RegisterTasks_Success(t *testing.T) { lib.EXPECT().Name().Return("TestLibrary").AnyTimes() lib.EXPECT().Components().Return(nil) - lib.EXPECT().Tasks().Return([]chasm.RegistrableTask{ + lib.EXPECT().Tasks().Return([]*chasm.RegistrableTask{ chasm.NewRegistrableTask[*chasm.MockComponent, testTask1]("Task1", chasm.NewMockTaskHandler[*chasm.MockComponent, testTask1](ctrl)), chasm.NewRegistrableTask[testTaskComponentInterface, testTask2]("Task2", chasm.NewMockTaskHandler[testTaskComponentInterface, testTask2](ctrl)), }) @@ -94,7 +94,7 @@ func TestRegistry_RegisterTasks_Success(t *testing.T) { missingRT, ok := r.Task("TestLibrary.TaskMissing") require.False(t, ok) - require.Equal(t, chasm.EmptyRegistrableTask, missingRT) + require.Nil(t, missingRT) tInstance1 := testTask2{} rt2, ok := r.TaskFor(tInstance1) @@ -104,7 +104,7 @@ func TestRegistry_RegisterTasks_Success(t *testing.T) { tInstance2 := "invalid task instance" rt3, ok := r.TaskFor(tInstance2) require.False(t, ok) - require.Equal(t, chasm.EmptyRegistrableTask, rt3) + require.Nil(t, rt3) } func TestRegistry_Register_LibraryError(t *testing.T) { @@ -134,7 +134,7 @@ func TestRegistry_RegisterComponents_Error(t *testing.T) { lib.EXPECT().Name().Return("TestLibrary").AnyTimes() t.Run("component name must not be empty", func(t *testing.T) { - lib.EXPECT().Components().Return([]chasm.RegistrableComponent{ + lib.EXPECT().Components().Return([]*chasm.RegistrableComponent{ chasm.NewRegistrableComponent[*chasm.MockComponent](""), }) r := chasm.NewRegistry() @@ -144,7 +144,7 @@ func TestRegistry_RegisterComponents_Error(t *testing.T) { }) t.Run("component name must follow rules", func(t *testing.T) { - lib.EXPECT().Components().Return([]chasm.RegistrableComponent{ + lib.EXPECT().Components().Return([]*chasm.RegistrableComponent{ chasm.NewRegistrableComponent[*chasm.MockComponent]("bad.component.name"), }) r := chasm.NewRegistry() @@ -154,7 +154,7 @@ func TestRegistry_RegisterComponents_Error(t *testing.T) { }) t.Run("component is already registered by name", func(t *testing.T) { - lib.EXPECT().Components().Return([]chasm.RegistrableComponent{ + lib.EXPECT().Components().Return([]*chasm.RegistrableComponent{ chasm.NewRegistrableComponent[*chasm.MockComponent]("Component1"), chasm.NewRegistrableComponent[*chasm.MockComponent]("Component1"), }) @@ -165,7 +165,7 @@ func TestRegistry_RegisterComponents_Error(t *testing.T) { }) t.Run("component is already registered by type", func(t *testing.T) { - lib.EXPECT().Components().Return([]chasm.RegistrableComponent{ + lib.EXPECT().Components().Return([]*chasm.RegistrableComponent{ chasm.NewRegistrableComponent[*chasm.MockComponent]("Component1"), chasm.NewRegistrableComponent[*chasm.MockComponent]("Component2"), }) @@ -175,6 +175,18 @@ func TestRegistry_RegisterComponents_Error(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "is already registered") }) + + t.Run("component must be a struct", func(t *testing.T) { + lib.EXPECT().Components().Return([]*chasm.RegistrableComponent{ + chasm.NewRegistrableComponent[chasm.Component]("Component1"), + }) + r := chasm.NewRegistry() + + err := r.Register(lib) + require.Error(t, err) + require.Contains(t, err.Error(), "must be struct or pointer to struct") + }) + } func TestRegistry_RegisterTasks_Error(t *testing.T) { @@ -185,7 +197,7 @@ func TestRegistry_RegisterTasks_Error(t *testing.T) { t.Run("task name must not be empty", func(t *testing.T) { r := chasm.NewRegistry() - lib.EXPECT().Tasks().Return([]chasm.RegistrableTask{ + lib.EXPECT().Tasks().Return([]*chasm.RegistrableTask{ chasm.NewRegistrableTask[*chasm.MockComponent, testTask1]("", chasm.NewMockTaskHandler[*chasm.MockComponent, testTask1](ctrl)), }) err := r.Register(lib) @@ -194,7 +206,7 @@ func TestRegistry_RegisterTasks_Error(t *testing.T) { }) t.Run("task name must follow rules", func(t *testing.T) { - lib.EXPECT().Tasks().Return([]chasm.RegistrableTask{ + lib.EXPECT().Tasks().Return([]*chasm.RegistrableTask{ chasm.NewRegistrableTask[*chasm.MockComponent, testTask1]("bad.task.name", chasm.NewMockTaskHandler[*chasm.MockComponent, testTask1](ctrl)), }) r := chasm.NewRegistry() @@ -204,7 +216,7 @@ func TestRegistry_RegisterTasks_Error(t *testing.T) { }) t.Run("task is already registered by name", func(t *testing.T) { - lib.EXPECT().Tasks().Return([]chasm.RegistrableTask{ + lib.EXPECT().Tasks().Return([]*chasm.RegistrableTask{ chasm.NewRegistrableTask[*chasm.MockComponent, testTask1]("Task1", chasm.NewMockTaskHandler[*chasm.MockComponent, testTask1](ctrl)), chasm.NewRegistrableTask[*chasm.MockComponent, testTask1]("Task1", chasm.NewMockTaskHandler[*chasm.MockComponent, testTask1](ctrl)), }) @@ -215,7 +227,7 @@ func TestRegistry_RegisterTasks_Error(t *testing.T) { }) t.Run("task is already registered by type", func(t *testing.T) { - lib.EXPECT().Tasks().Return([]chasm.RegistrableTask{ + lib.EXPECT().Tasks().Return([]*chasm.RegistrableTask{ chasm.NewRegistrableTask[*chasm.MockComponent, testTask1]("Task1", chasm.NewMockTaskHandler[*chasm.MockComponent, testTask1](ctrl)), chasm.NewRegistrableTask[*chasm.MockComponent, testTask1]("Task2", chasm.NewMockTaskHandler[*chasm.MockComponent, testTask1](ctrl)), }) @@ -226,7 +238,7 @@ func TestRegistry_RegisterTasks_Error(t *testing.T) { }) t.Run("task component struct must implement Component", func(t *testing.T) { - lib.EXPECT().Tasks().Return([]chasm.RegistrableTask{ + lib.EXPECT().Tasks().Return([]*chasm.RegistrableTask{ // MockComponent has only pointer receivers and therefore does not implement Component interface. chasm.NewRegistrableTask[chasm.MockComponent, testTask1]("Task1", chasm.NewMockTaskHandler[chasm.MockComponent, testTask1](ctrl)), }) @@ -237,7 +249,7 @@ func TestRegistry_RegisterTasks_Error(t *testing.T) { }) t.Run("task must be struct", func(t *testing.T) { - lib.EXPECT().Tasks().Return([]chasm.RegistrableTask{ + lib.EXPECT().Tasks().Return([]*chasm.RegistrableTask{ chasm.NewRegistrableTask[*chasm.MockComponent, string]("Task1", chasm.NewMockTaskHandler[*chasm.MockComponent, string](ctrl)), }) r := chasm.NewRegistry()