From 5975776d1700ffa03ba383df9bb0281ebdc4e377 Mon Sep 17 00:00:00 2001 From: Juan Hernandez Date: Wed, 24 Jun 2026 18:37:22 +0200 Subject: [PATCH 1/2] NO-ISSUE: Extract interfaces for reflection `Helper` and `ObjectHelper` Replace the concrete `Helper` and `ObjectHelper` structs with interfaces, renaming the implementations to unexported `helper` and `objectHelper` types. Add `go:generate` directives for `mockgen` so that mocks are produced automatically. This is a preparation step for adding unit tests to the CLI rendering tables, which need to mock the reflection layer without requiring a live gRPC connection. Assisted-by: Cursor Signed-off-by: Juan Hernandez --- internal/cmd/cli/annotate/annotate_cmd.go | 2 +- internal/cmd/cli/delete/delete_cmd.go | 2 +- internal/cmd/cli/edit/edit_cmd.go | 2 +- internal/cmd/cli/edit/edit_cmd_test.go | 2 +- internal/cmd/cli/get/get_cmd.go | 4 +- .../cmd/cli/get/get_cmd_watch_e2e_test.go | 2 +- internal/cmd/cli/label/label_cmd.go | 2 +- internal/reflection/find_object.go | 2 +- internal/reflection/reflection_helper.go | 172 +++++++----- internal/reflection/reflection_helper_mock.go | 96 +++++++ internal/reflection/reflection_helper_test.go | 2 +- .../reflection_object_helper_mock.go | 258 ++++++++++++++++++ internal/rendering/table_renderer.go | 10 +- internal/rendering/table_renderer_test.go | 2 +- internal/terminal/terminal_console.go | 8 +- 15 files changed, 476 insertions(+), 90 deletions(-) create mode 100644 internal/reflection/reflection_helper_mock.go create mode 100644 internal/reflection/reflection_object_helper_mock.go diff --git a/internal/cmd/cli/annotate/annotate_cmd.go b/internal/cmd/cli/annotate/annotate_cmd.go index a8d5ee2c4..800a5cb04 100644 --- a/internal/cmd/cli/annotate/annotate_cmd.go +++ b/internal/cmd/cli/annotate/annotate_cmd.go @@ -48,7 +48,7 @@ type runnerContext struct { logger *slog.Logger console *terminal.Console conn *grpc.ClientConn - helper *reflection.ObjectHelper + helper reflection.ObjectHelper } func (c *runnerContext) run(cmd *cobra.Command, args []string) error { diff --git a/internal/cmd/cli/delete/delete_cmd.go b/internal/cmd/cli/delete/delete_cmd.go index bfa218a15..c678bd3aa 100644 --- a/internal/cmd/cli/delete/delete_cmd.go +++ b/internal/cmd/cli/delete/delete_cmd.go @@ -53,7 +53,7 @@ type runnerContext struct { logger *slog.Logger console *terminal.Console conn *grpc.ClientConn - helper *reflection.ObjectHelper + helper reflection.ObjectHelper } func (c *runnerContext) run(cmd *cobra.Command, args []string) error { diff --git a/internal/cmd/cli/edit/edit_cmd.go b/internal/cmd/cli/edit/edit_cmd.go index 5c7270478..2ef9f00bd 100644 --- a/internal/cmd/cli/edit/edit_cmd.go +++ b/internal/cmd/cli/edit/edit_cmd.go @@ -75,7 +75,7 @@ type runnerContext struct { format string conn *grpc.ClientConn marshalOptions protojson.MarshalOptions - helper *reflection.ObjectHelper + helper reflection.ObjectHelper } func (c *runnerContext) run(cmd *cobra.Command, args []string) error { diff --git a/internal/cmd/cli/edit/edit_cmd_test.go b/internal/cmd/cli/edit/edit_cmd_test.go index 5949f6f0d..1cad77172 100644 --- a/internal/cmd/cli/edit/edit_cmd_test.go +++ b/internal/cmd/cli/edit/edit_cmd_test.go @@ -37,7 +37,7 @@ var _ = Describe("Edit command", func() { conn *grpc.ClientConn console *terminal.Console output *bytes.Buffer - helper *reflection.ObjectHelper + helper reflection.ObjectHelper ) BeforeEach(func() { diff --git a/internal/cmd/cli/get/get_cmd.go b/internal/cmd/cli/get/get_cmd.go index 19e65e49d..32872f165 100644 --- a/internal/cmd/cli/get/get_cmd.go +++ b/internal/cmd/cli/get/get_cmd.go @@ -101,8 +101,8 @@ type runnerContext struct { console *terminal.Console conn *grpc.ClientConn marshalOptions protojson.MarshalOptions - globalHelper *reflection.Helper - objectHelper *reflection.ObjectHelper + globalHelper reflection.Helper + objectHelper reflection.ObjectHelper } func (c *runnerContext) run(cmd *cobra.Command, args []string) error { diff --git a/internal/cmd/cli/get/get_cmd_watch_e2e_test.go b/internal/cmd/cli/get/get_cmd_watch_e2e_test.go index 559c84067..ef0ec6881 100644 --- a/internal/cmd/cli/get/get_cmd_watch_e2e_test.go +++ b/internal/cmd/cli/get/get_cmd_watch_e2e_test.go @@ -36,7 +36,7 @@ var _ = Describe("Watch e2e", func() { server *testing.Server conn *grpc.ClientConn eventsServer *testing.EventsServerFuncs - helper *reflection.ObjectHelper + helper reflection.ObjectHelper console *terminal.Console ) diff --git a/internal/cmd/cli/label/label_cmd.go b/internal/cmd/cli/label/label_cmd.go index 090608920..4ce782e8c 100644 --- a/internal/cmd/cli/label/label_cmd.go +++ b/internal/cmd/cli/label/label_cmd.go @@ -48,7 +48,7 @@ type runnerContext struct { logger *slog.Logger console *terminal.Console conn *grpc.ClientConn - helper *reflection.ObjectHelper + helper reflection.ObjectHelper } func (c *runnerContext) run(cmd *cobra.Command, args []string) error { diff --git a/internal/reflection/find_object.go b/internal/reflection/find_object.go index b71824c79..2fda22f61 100644 --- a/internal/reflection/find_object.go +++ b/internal/reflection/find_object.go @@ -35,7 +35,7 @@ type Renderer interface { // Expected templates (looked up in the console's registered template set): // - "no_matches.txt" vars: Object (string), Ref (string) // - "multiple_matches.txt" vars: Matches ([]proto.Message), Object (string), Ref (string), Total (int32) -func (h *ObjectHelper) FindObject(ctx context.Context, ref string, console Renderer) (result proto.Message, err error) { +func (h *objectHelper) FindObject(ctx context.Context, ref string, console Renderer) (result proto.Message, err error) { filter := fmt.Sprintf(`this.id == %[1]q || this.metadata.name == %[1]q`, ref) response, err := h.List(ctx, ListOptions{ Filter: filter, diff --git a/internal/reflection/reflection_helper.go b/internal/reflection/reflection_helper.go index b71b8c5be..d67768292 100644 --- a/internal/reflection/reflection_helper.go +++ b/internal/reflection/reflection_helper.go @@ -35,24 +35,46 @@ import ( _ "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" ) -// Frequently used names: -const ( - // Methods: - createMethodName = protoreflect.Name("Create") - deleteMethodName = protoreflect.Name("Delete") - getMethodName = protoreflect.Name("Get") - listMethodName = protoreflect.Name("List") - updateMethodName = protoreflect.Name("Update") +// Helper simplifies use of the protocol buffers reflection facility. It knows how to extract from the descriptors the +// list of message types that satisfy the conditions to be considered objects, as well as the services that support them +// and the methods to get, list, update and delete instances. +// +//go:generate mockgen -destination=reflection_helper_mock.go -package=reflection . Helper +type Helper interface { + // Names returns the full names of the object types. The results are sorted by the order of the packages, and + // alphabetically within each package. + Names() []string - // Fields: - filterFieldName = protoreflect.Name("filter") - idFieldName = protoreflect.Name("id") - itemsFieldName = protoreflect.Name("items") - limitFieldName = protoreflect.Name("limit") - metadataFieldName = protoreflect.Name("metadata") - objectFieldName = protoreflect.Name("object") - totalFieldName = protoreflect.Name("total") -) + // Singulars returns the object types in singular. The results are in lower case and sorted alphabetically. + Singulars() []string + + // Plurals returns the object types in plural. The results are in lower case and sorted alphabetically. + Plurals() []string + + // Lookup returns the helper for the given object type. Returns nil if there is no such object. + Lookup(objectType string) ObjectHelper +} + +// ObjectHelper contains information about a message type that satisfies the conditions to be considered an object. +// +//go:generate mockgen -destination=reflection_object_helper_mock.go -package=reflection . ObjectHelper +type ObjectHelper interface { + Descriptor() protoreflect.MessageDescriptor + Instance() proto.Message + FullName() protoreflect.FullName + String() string + Singular() string + Plural() string + List(ctx context.Context, options ListOptions) (ListResult, error) + Get(ctx context.Context, id string) (proto.Message, error) + GetId(object proto.Message) string + GetName(object proto.Message) string + GetMetadata(object proto.Message) Metadata + Create(ctx context.Context, object proto.Message) (proto.Message, error) + Update(ctx context.Context, object proto.Message) (proto.Message, error) + Delete(ctx context.Context, id string) error + FindObject(ctx context.Context, ref string, console Renderer) (proto.Message, error) +} // HelperBuilder contains the data and logic needed to create a reflection helper. // @@ -63,17 +85,13 @@ type HelperBuilder struct { packages map[string]int } -// Helper simplifies use of the protocol buffers reflection facility. It knows how to extract from the descriptors the -// list of message types that satisfy the conditions to be considered objects, as well as the services that support them -// and the methods to get, list, update and delete instances. -// -// Don't create instances of this type directly, use the NewHelper function instead. -type Helper struct { +// helper is the default implementation of the Helper interface. +type helper struct { logger *slog.Logger connection *grpc.ClientConn packages map[protoreflect.FullName]int scanOnce *sync.Once - helpers []ObjectHelper + helpers []objectHelper } // NewHelper creates a builder that can then be used to configure a reflection helper. @@ -116,7 +134,7 @@ func (b *HelperBuilder) AddPackages(values map[string]int) *HelperBuilder { } // Build uses the data stored in the builder to create a new reflection helper. -func (b *HelperBuilder) Build() (result *Helper, err error) { +func (b *HelperBuilder) Build() (result Helper, err error) { // Check the parameters: if b.logger == nil { err = errors.New("logger is mandatory") @@ -138,23 +156,23 @@ func (b *HelperBuilder) Build() (result *Helper, err error) { } // Create and populate the object: - result = &Helper{ + result = &helper{ logger: b.logger, packages: packages, connection: b.connection, scanOnce: &sync.Once{}, - helpers: []ObjectHelper{}, + helpers: []objectHelper{}, } return } -func (h *Helper) scanIfNeeded() { +func (h *helper) scanIfNeeded() { h.scanOnce.Do(func() { h.scan() }) } -func (h *Helper) scan() { +func (h *helper) scan() { protoregistry.GlobalFiles.RangeFiles(h.scanFile) sort.Slice( h.helpers, @@ -171,7 +189,7 @@ func (h *Helper) scan() { ) } -func (h *Helper) scanFile(fileDesc protoreflect.FileDescriptor) bool { +func (h *helper) scanFile(fileDesc protoreflect.FileDescriptor) bool { _, ok := h.packages[fileDesc.Package()] if !ok { h.logger.Debug( @@ -192,7 +210,7 @@ func (h *Helper) scanFile(fileDesc protoreflect.FileDescriptor) bool { return true } -func (h *Helper) scanService(serviceDesc protoreflect.ServiceDescriptor) { +func (h *helper) scanService(serviceDesc protoreflect.ServiceDescriptor) { // The service must have the get, list, update and delete method: h.logger.Debug( "Scanning service", @@ -310,7 +328,7 @@ func (h *Helper) scanService(serviceDesc protoreflect.ServiceDescriptor) { metadataFieldDesc := objectFields.ByName(metadataFieldName) // This is a supported object type: - helper := ObjectHelper{ + helper := objectHelper{ parent: h, descriptor: objectDesc, idField: idFieldDesc, @@ -368,7 +386,7 @@ func (h *Helper) scanService(serviceDesc protoreflect.ServiceDescriptor) { h.helpers = append(h.helpers, helper) } -func (h *Helper) getIdField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { +func (h *helper) getIdField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { fieldDesc := messageDesc.Fields().ByName(idFieldName) if fieldDesc == nil { return nil @@ -382,7 +400,7 @@ func (h *Helper) getIdField(messageDesc protoreflect.MessageDescriptor) protoref return fieldDesc } -func (h *Helper) getObjectField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { +func (h *helper) getObjectField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { fieldDesc := messageDesc.Fields().ByName(objectFieldName) if fieldDesc == nil { return nil @@ -396,7 +414,7 @@ func (h *Helper) getObjectField(messageDesc protoreflect.MessageDescriptor) prot return fieldDesc } -func (h *Helper) getFilterField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { +func (h *helper) getFilterField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { fieldDesc := messageDesc.Fields().ByName(filterFieldName) if fieldDesc == nil { return nil @@ -410,7 +428,7 @@ func (h *Helper) getFilterField(messageDesc protoreflect.MessageDescriptor) prot return fieldDesc } -func (h *Helper) getLimitField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { +func (h *helper) getLimitField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { fieldDesc := messageDesc.Fields().ByName(limitFieldName) if fieldDesc == nil { return nil @@ -424,7 +442,7 @@ func (h *Helper) getLimitField(messageDesc protoreflect.MessageDescriptor) proto return fieldDesc } -func (h *Helper) getItemsField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { +func (h *helper) getItemsField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { fieldDesc := messageDesc.Fields().ByName(itemsFieldName) if fieldDesc == nil { return nil @@ -438,7 +456,7 @@ func (h *Helper) getItemsField(messageDesc protoreflect.MessageDescriptor) proto return fieldDesc } -func (h *Helper) getTotalField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { +func (h *helper) getTotalField(messageDesc protoreflect.MessageDescriptor) protoreflect.FieldDescriptor { fieldDesc := messageDesc.Fields().ByName(totalFieldName) if fieldDesc == nil { return nil @@ -452,9 +470,7 @@ func (h *Helper) getTotalField(messageDesc protoreflect.MessageDescriptor) proto return fieldDesc } -// Names returns the full names of the object types. The results are sorted by the order of the packages, and -// alphabetically within each package. -func (h *Helper) Names() []string { +func (h *helper) Names() []string { h.scanIfNeeded() results := make([]string, len(h.helpers)) for i, objectInfo := range h.helpers { @@ -463,8 +479,7 @@ func (h *Helper) Names() []string { return results } -// Singulars returns the object types in singular. The results are in lower case and sorted alphabetically. -func (h *Helper) Singulars() []string { +func (h *helper) Singulars() []string { h.scanIfNeeded() set := make(map[string]bool, len(h.helpers)) for _, objectInfo := range h.helpers { @@ -475,8 +490,7 @@ func (h *Helper) Singulars() []string { return results } -// Plurals the object types in plural. The results are in lower case and sorted alphabetically.. -func (h *Helper) Plurals() []string { +func (h *helper) Plurals() []string { h.scanIfNeeded() set := make(map[string]bool, len(h.helpers)) for _, objectInfo := range h.helpers { @@ -487,8 +501,7 @@ func (h *Helper) Plurals() []string { return results } -// Lookup returns the helper for the given object type. Returns nil if there is no such object. -func (h *Helper) Lookup(objectType string) *ObjectHelper { +func (h *helper) Lookup(objectType string) ObjectHelper { h.scanIfNeeded() for i, objectInfo := range h.helpers { if objectType == string(objectInfo.descriptor.FullName()) { @@ -504,18 +517,18 @@ func (h *Helper) Lookup(objectType string) *ObjectHelper { return nil } -func (h *Helper) makeMethodPath(methodDesc protoreflect.MethodDescriptor) string { +func (h *helper) makeMethodPath(methodDesc protoreflect.MethodDescriptor) string { return fmt.Sprintf("/%s/%s", methodDesc.FullName().Parent(), methodDesc.Name()) } -func (h *Helper) makeMethodTemplates(methodDesc protoreflect.MethodDescriptor) (requestTemplate, +func (h *helper) makeMethodTemplates(methodDesc protoreflect.MethodDescriptor) (requestTemplate, responseTemplate proto.Message) { requestTemplate = h.makeTemplate(methodDesc.Input()) responseTemplate = h.makeTemplate(methodDesc.Output()) return } -func (h *Helper) makeTemplate(messageDesc protoreflect.MessageDescriptor) proto.Message { +func (h *helper) makeTemplate(messageDesc protoreflect.MessageDescriptor) proto.Message { messageType, err := protoregistry.GlobalTypes.FindMessageByName(messageDesc.FullName()) if err != nil { panic(err) @@ -523,9 +536,9 @@ func (h *Helper) makeTemplate(messageDesc protoreflect.MessageDescriptor) proto. return messageType.New().Interface() } -// ObjectHelper contains information about a message type that satisfies the conditions to be considered an object. -type ObjectHelper struct { - parent *Helper +// objectHelper is the default implementation of the ObjectHelper interface. +type objectHelper struct { + parent *helper descriptor protoreflect.MessageDescriptor singular string plural string @@ -576,27 +589,27 @@ type deleteInfo struct { id protoreflect.FieldDescriptor } -func (h *ObjectHelper) Descriptor() protoreflect.MessageDescriptor { +func (h *objectHelper) Descriptor() protoreflect.MessageDescriptor { return h.descriptor } -func (h *ObjectHelper) Instance() proto.Message { +func (h *objectHelper) Instance() proto.Message { return proto.Clone(h.template) } -func (h *ObjectHelper) FullName() protoreflect.FullName { +func (h *objectHelper) FullName() protoreflect.FullName { return h.descriptor.FullName() } -func (h *ObjectHelper) String() string { +func (h *objectHelper) String() string { return string(h.descriptor.FullName()) } -func (h *ObjectHelper) Singular() string { +func (h *objectHelper) Singular() string { return h.singular } -func (h *ObjectHelper) Plural() string { +func (h *objectHelper) Plural() string { return h.plural } @@ -610,7 +623,7 @@ type ListResult struct { Total int32 } -func (h *ObjectHelper) List(ctx context.Context, options ListOptions) (result ListResult, err error) { +func (h *objectHelper) List(ctx context.Context, options ListOptions) (result ListResult, err error) { filter := options.Filter request := proto.Clone(h.list.request) if filter != "" { @@ -637,7 +650,7 @@ func (h *ObjectHelper) List(ctx context.Context, options ListOptions) (result Li return } -func (h *ObjectHelper) Get(ctx context.Context, id string) (result proto.Message, err error) { +func (h *objectHelper) Get(ctx context.Context, id string) (result proto.Message, err error) { request := proto.Clone(h.get.request) h.setId(request, h.get.id, id) response := proto.Clone(h.get.response) @@ -649,19 +662,19 @@ func (h *ObjectHelper) Get(ctx context.Context, id string) (result proto.Message return } -func (h *ObjectHelper) GetId(object proto.Message) string { +func (h *objectHelper) GetId(object proto.Message) string { return object.ProtoReflect().Get(h.idField).String() } -func (h *ObjectHelper) GetName(object proto.Message) string { +func (h *objectHelper) GetName(object proto.Message) string { return h.GetMetadata(object).GetName() } -func (h *ObjectHelper) GetMetadata(object proto.Message) Metadata { +func (h *objectHelper) GetMetadata(object proto.Message) Metadata { return object.ProtoReflect().Get(h.metadataField).Message().Interface().(Metadata) } -func (h *ObjectHelper) Create(ctx context.Context, object proto.Message) (result proto.Message, err error) { +func (h *objectHelper) Create(ctx context.Context, object proto.Message) (result proto.Message, err error) { request := proto.Clone(h.create.request) h.setObject(request, h.create.in, object) response := proto.Clone(h.create.response) @@ -673,7 +686,7 @@ func (h *ObjectHelper) Create(ctx context.Context, object proto.Message) (result return } -func (h *ObjectHelper) Update(ctx context.Context, object proto.Message) (result proto.Message, err error) { +func (h *objectHelper) Update(ctx context.Context, object proto.Message) (result proto.Message, err error) { request := proto.Clone(h.update.request) h.setObject(request, h.update.in, object) response := proto.Clone(h.update.response) @@ -685,21 +698,40 @@ func (h *ObjectHelper) Update(ctx context.Context, object proto.Message) (result return } -func (h *ObjectHelper) Delete(ctx context.Context, id string) error { +func (h *objectHelper) Delete(ctx context.Context, id string) error { request := proto.Clone(h.delete.request) h.setId(request, h.delete.id, id) response := proto.Clone(h.delete.response) return h.parent.connection.Invoke(ctx, h.delete.path, request, response) } -func (h *ObjectHelper) setId(message proto.Message, field protoreflect.FieldDescriptor, value string) { +func (h *objectHelper) setId(message proto.Message, field protoreflect.FieldDescriptor, value string) { message.ProtoReflect().Set(field, protoreflect.ValueOfString(value)) } -func (h *ObjectHelper) setObject(message proto.Message, field protoreflect.FieldDescriptor, value proto.Message) { +func (h *objectHelper) setObject(message proto.Message, field protoreflect.FieldDescriptor, value proto.Message) { message.ProtoReflect().Set(field, protoreflect.ValueOfMessage(value.ProtoReflect())) } -func (h *ObjectHelper) getObject(message proto.Message, field protoreflect.FieldDescriptor) proto.Message { +func (h *objectHelper) getObject(message proto.Message, field protoreflect.FieldDescriptor) proto.Message { return message.ProtoReflect().Get(field).Message().Interface() } + +// Frequently used names: +const ( + // Methods: + createMethodName = protoreflect.Name("Create") + deleteMethodName = protoreflect.Name("Delete") + getMethodName = protoreflect.Name("Get") + listMethodName = protoreflect.Name("List") + updateMethodName = protoreflect.Name("Update") + + // Fields: + filterFieldName = protoreflect.Name("filter") + idFieldName = protoreflect.Name("id") + itemsFieldName = protoreflect.Name("items") + limitFieldName = protoreflect.Name("limit") + metadataFieldName = protoreflect.Name("metadata") + objectFieldName = protoreflect.Name("object") + totalFieldName = protoreflect.Name("total") +) diff --git a/internal/reflection/reflection_helper_mock.go b/internal/reflection/reflection_helper_mock.go new file mode 100644 index 000000000..0e1a80c4b --- /dev/null +++ b/internal/reflection/reflection_helper_mock.go @@ -0,0 +1,96 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/osac-project/fulfillment-service/internal/reflection (interfaces: Helper) +// +// Generated by this command: +// +// mockgen -destination=reflection_helper_mock.go -package=reflection . Helper +// + +// Package reflection is a generated GoMock package. +package reflection + +import ( + reflect "reflect" + + gomock "go.uber.org/mock/gomock" +) + +// MockHelper is a mock of Helper interface. +type MockHelper struct { + ctrl *gomock.Controller + recorder *MockHelperMockRecorder + isgomock struct{} +} + +// MockHelperMockRecorder is the mock recorder for MockHelper. +type MockHelperMockRecorder struct { + mock *MockHelper +} + +// NewMockHelper creates a new mock instance. +func NewMockHelper(ctrl *gomock.Controller) *MockHelper { + mock := &MockHelper{ctrl: ctrl} + mock.recorder = &MockHelperMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockHelper) EXPECT() *MockHelperMockRecorder { + return m.recorder +} + +// Lookup mocks base method. +func (m *MockHelper) Lookup(objectType string) ObjectHelper { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Lookup", objectType) + ret0, _ := ret[0].(ObjectHelper) + return ret0 +} + +// Lookup indicates an expected call of Lookup. +func (mr *MockHelperMockRecorder) Lookup(objectType any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Lookup", reflect.TypeOf((*MockHelper)(nil).Lookup), objectType) +} + +// Names mocks base method. +func (m *MockHelper) Names() []string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Names") + ret0, _ := ret[0].([]string) + return ret0 +} + +// Names indicates an expected call of Names. +func (mr *MockHelperMockRecorder) Names() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Names", reflect.TypeOf((*MockHelper)(nil).Names)) +} + +// Plurals mocks base method. +func (m *MockHelper) Plurals() []string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Plurals") + ret0, _ := ret[0].([]string) + return ret0 +} + +// Plurals indicates an expected call of Plurals. +func (mr *MockHelperMockRecorder) Plurals() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Plurals", reflect.TypeOf((*MockHelper)(nil).Plurals)) +} + +// Singulars mocks base method. +func (m *MockHelper) Singulars() []string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Singulars") + ret0, _ := ret[0].([]string) + return ret0 +} + +// Singulars indicates an expected call of Singulars. +func (mr *MockHelperMockRecorder) Singulars() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Singulars", reflect.TypeOf((*MockHelper)(nil).Singulars)) +} diff --git a/internal/reflection/reflection_helper_test.go b/internal/reflection/reflection_helper_test.go index 5c487991a..6c5c765f1 100644 --- a/internal/reflection/reflection_helper_test.go +++ b/internal/reflection/reflection_helper_test.go @@ -119,7 +119,7 @@ var _ = Describe("Reflection helper", func() { }) Describe("Behaviour", func() { - var helper *Helper + var helper Helper BeforeEach(func() { var err error diff --git a/internal/reflection/reflection_object_helper_mock.go b/internal/reflection/reflection_object_helper_mock.go new file mode 100644 index 000000000..cebdf2ac1 --- /dev/null +++ b/internal/reflection/reflection_object_helper_mock.go @@ -0,0 +1,258 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/osac-project/fulfillment-service/internal/reflection (interfaces: ObjectHelper) +// +// Generated by this command: +// +// mockgen -destination=reflection_object_helper_mock.go -package=reflection . ObjectHelper +// + +// Package reflection is a generated GoMock package. +package reflection + +import ( + context "context" + reflect "reflect" + + gomock "go.uber.org/mock/gomock" + proto "google.golang.org/protobuf/proto" + protoreflect "google.golang.org/protobuf/reflect/protoreflect" +) + +// MockObjectHelper is a mock of ObjectHelper interface. +type MockObjectHelper struct { + ctrl *gomock.Controller + recorder *MockObjectHelperMockRecorder + isgomock struct{} +} + +// MockObjectHelperMockRecorder is the mock recorder for MockObjectHelper. +type MockObjectHelperMockRecorder struct { + mock *MockObjectHelper +} + +// NewMockObjectHelper creates a new mock instance. +func NewMockObjectHelper(ctrl *gomock.Controller) *MockObjectHelper { + mock := &MockObjectHelper{ctrl: ctrl} + mock.recorder = &MockObjectHelperMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockObjectHelper) EXPECT() *MockObjectHelperMockRecorder { + return m.recorder +} + +// Create mocks base method. +func (m *MockObjectHelper) Create(ctx context.Context, object proto.Message) (proto.Message, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Create", ctx, object) + ret0, _ := ret[0].(proto.Message) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Create indicates an expected call of Create. +func (mr *MockObjectHelperMockRecorder) Create(ctx, object any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Create", reflect.TypeOf((*MockObjectHelper)(nil).Create), ctx, object) +} + +// Delete mocks base method. +func (m *MockObjectHelper) Delete(ctx context.Context, id string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Delete", ctx, id) + ret0, _ := ret[0].(error) + return ret0 +} + +// Delete indicates an expected call of Delete. +func (mr *MockObjectHelperMockRecorder) Delete(ctx, id any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Delete", reflect.TypeOf((*MockObjectHelper)(nil).Delete), ctx, id) +} + +// Descriptor mocks base method. +func (m *MockObjectHelper) Descriptor() protoreflect.MessageDescriptor { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Descriptor") + ret0, _ := ret[0].(protoreflect.MessageDescriptor) + return ret0 +} + +// Descriptor indicates an expected call of Descriptor. +func (mr *MockObjectHelperMockRecorder) Descriptor() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Descriptor", reflect.TypeOf((*MockObjectHelper)(nil).Descriptor)) +} + +// FindObject mocks base method. +func (m *MockObjectHelper) FindObject(ctx context.Context, ref string, console Renderer) (proto.Message, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FindObject", ctx, ref, console) + ret0, _ := ret[0].(proto.Message) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FindObject indicates an expected call of FindObject. +func (mr *MockObjectHelperMockRecorder) FindObject(ctx, ref, console any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindObject", reflect.TypeOf((*MockObjectHelper)(nil).FindObject), ctx, ref, console) +} + +// FullName mocks base method. +func (m *MockObjectHelper) FullName() protoreflect.FullName { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FullName") + ret0, _ := ret[0].(protoreflect.FullName) + return ret0 +} + +// FullName indicates an expected call of FullName. +func (mr *MockObjectHelperMockRecorder) FullName() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FullName", reflect.TypeOf((*MockObjectHelper)(nil).FullName)) +} + +// Get mocks base method. +func (m *MockObjectHelper) Get(ctx context.Context, id string) (proto.Message, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Get", ctx, id) + ret0, _ := ret[0].(proto.Message) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Get indicates an expected call of Get. +func (mr *MockObjectHelperMockRecorder) Get(ctx, id any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockObjectHelper)(nil).Get), ctx, id) +} + +// GetId mocks base method. +func (m *MockObjectHelper) GetId(object proto.Message) string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetId", object) + ret0, _ := ret[0].(string) + return ret0 +} + +// GetId indicates an expected call of GetId. +func (mr *MockObjectHelperMockRecorder) GetId(object any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetId", reflect.TypeOf((*MockObjectHelper)(nil).GetId), object) +} + +// GetMetadata mocks base method. +func (m *MockObjectHelper) GetMetadata(object proto.Message) Metadata { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetMetadata", object) + ret0, _ := ret[0].(Metadata) + return ret0 +} + +// GetMetadata indicates an expected call of GetMetadata. +func (mr *MockObjectHelperMockRecorder) GetMetadata(object any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMetadata", reflect.TypeOf((*MockObjectHelper)(nil).GetMetadata), object) +} + +// GetName mocks base method. +func (m *MockObjectHelper) GetName(object proto.Message) string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetName", object) + ret0, _ := ret[0].(string) + return ret0 +} + +// GetName indicates an expected call of GetName. +func (mr *MockObjectHelperMockRecorder) GetName(object any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetName", reflect.TypeOf((*MockObjectHelper)(nil).GetName), object) +} + +// Instance mocks base method. +func (m *MockObjectHelper) Instance() proto.Message { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Instance") + ret0, _ := ret[0].(proto.Message) + return ret0 +} + +// Instance indicates an expected call of Instance. +func (mr *MockObjectHelperMockRecorder) Instance() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Instance", reflect.TypeOf((*MockObjectHelper)(nil).Instance)) +} + +// List mocks base method. +func (m *MockObjectHelper) List(ctx context.Context, options ListOptions) (ListResult, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "List", ctx, options) + ret0, _ := ret[0].(ListResult) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// List indicates an expected call of List. +func (mr *MockObjectHelperMockRecorder) List(ctx, options any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockObjectHelper)(nil).List), ctx, options) +} + +// Plural mocks base method. +func (m *MockObjectHelper) Plural() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Plural") + ret0, _ := ret[0].(string) + return ret0 +} + +// Plural indicates an expected call of Plural. +func (mr *MockObjectHelperMockRecorder) Plural() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Plural", reflect.TypeOf((*MockObjectHelper)(nil).Plural)) +} + +// Singular mocks base method. +func (m *MockObjectHelper) Singular() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Singular") + ret0, _ := ret[0].(string) + return ret0 +} + +// Singular indicates an expected call of Singular. +func (mr *MockObjectHelperMockRecorder) Singular() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Singular", reflect.TypeOf((*MockObjectHelper)(nil).Singular)) +} + +// String mocks base method. +func (m *MockObjectHelper) String() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "String") + ret0, _ := ret[0].(string) + return ret0 +} + +// String indicates an expected call of String. +func (mr *MockObjectHelperMockRecorder) String() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "String", reflect.TypeOf((*MockObjectHelper)(nil).String)) +} + +// Update mocks base method. +func (m *MockObjectHelper) Update(ctx context.Context, object proto.Message) (proto.Message, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Update", ctx, object) + ret0, _ := ret[0].(proto.Message) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Update indicates an expected call of Update. +func (mr *MockObjectHelperMockRecorder) Update(ctx, object any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Update", reflect.TypeOf((*MockObjectHelper)(nil).Update), ctx, object) +} diff --git a/internal/rendering/table_renderer.go b/internal/rendering/table_renderer.go index 6c504c736..66f47ee5c 100644 --- a/internal/rendering/table_renderer.go +++ b/internal/rendering/table_renderer.go @@ -80,7 +80,7 @@ type columnLayout struct { // NewTableRenderer function instead. type TableRendererBuilder struct { logger *slog.Logger - helper *reflection.Helper + helper reflection.Helper writer io.Writer } @@ -88,7 +88,7 @@ type TableRendererBuilder struct { // directly, use the NewTableRenderer function instead. type TableRenderer struct { logger *slog.Logger - helper *reflection.Helper + helper reflection.Helper writer *tabwriter.Writer cache map[protoreflect.FullName]map[string]string } @@ -105,7 +105,7 @@ func (b *TableRendererBuilder) SetLogger(value *slog.Logger) *TableRendererBuild } // SetHelper sets the reflection helper that will be used to introspect objects. This is mandatory. -func (b *TableRendererBuilder) SetHelper(value *reflection.Helper) *TableRendererBuilder { +func (b *TableRendererBuilder) SetHelper(value reflection.Helper) *TableRendererBuilder { b.helper = value return b } @@ -244,7 +244,7 @@ func (r *TableRenderer) Render(ctx context.Context, objects any) error { } // loadTable loads the table definition for the given object type from the embedded filesystem. -func (r *TableRenderer) loadTable(helper *reflection.ObjectHelper) (result *tableLayout, err error) { +func (r *TableRenderer) loadTable(helper reflection.ObjectHelper) (result *tableLayout, err error) { // Try to read the table definition file: file := fmt.Sprintf("%s.yaml", helper.FullName()) data, err := fs.ReadFile(tablesFS, path.Join("tables", file)) @@ -297,7 +297,7 @@ func (r *TableRenderer) renderHeader(cols []*columnLayout) error { // renderRow renders a single row of the table. func (r *TableRenderer) renderRow(ctx context.Context, cols []*columnLayout, prgs []cel.Program, object proto.Message, - helper *reflection.ObjectHelper) error { + helper reflection.ObjectHelper) error { // Wrap the object in a top-level "this" field to avoid conflicts with reserved words: in := map[string]any{ "this": object, diff --git a/internal/rendering/table_renderer_test.go b/internal/rendering/table_renderer_test.go index 5f065264d..378ac7eba 100644 --- a/internal/rendering/table_renderer_test.go +++ b/internal/rendering/table_renderer_test.go @@ -35,7 +35,7 @@ var _ = Describe("Table renderer", func() { ctx context.Context server *internaltesting.Server connection *grpc.ClientConn - helper *reflection.Helper + helper reflection.Helper ) BeforeEach(func() { diff --git a/internal/terminal/terminal_console.go b/internal/terminal/terminal_console.go index 4f6f862f7..fd545f090 100644 --- a/internal/terminal/terminal_console.go +++ b/internal/terminal/terminal_console.go @@ -43,7 +43,7 @@ type ConsoleBuilder struct { logger *slog.Logger stdout io.Writer stderr io.Writer - helper *reflection.Helper + helper reflection.Helper } // Console is helps writing messages to the console. Don't create objects of this type directly, use the NewConsole @@ -53,7 +53,7 @@ type Console struct { stdout io.Writer stderr io.Writer engine *templating.Engine - helper *reflection.Helper + helper reflection.Helper } // NewConsole creates a builder that can the be used to create a template engine. @@ -83,7 +83,7 @@ func (b *ConsoleBuilder) SetStderr(value io.Writer) *ConsoleBuilder { // SetHelper sets the reflection helper that will be used to introspect objects. This is optional. If not set then // functions like 'table' that need reflection will not be available. -func (b *ConsoleBuilder) SetHelper(value *reflection.Helper) *ConsoleBuilder { +func (b *ConsoleBuilder) SetHelper(value reflection.Helper) *ConsoleBuilder { b.helper = value return b } @@ -142,7 +142,7 @@ func (c *Console) AddTemplates(fs iofs.FS, dir string) error { // SetHelper sets the reflection helper that will be used to introspect objects. This is optional. If not set then // functions like 'table' that need reflection will not be available. -func (c *Console) SetHelper(value *reflection.Helper) { +func (c *Console) SetHelper(value reflection.Helper) { c.helper = value } From 1dc5eed36442aba9d64cd70db2bc074a189b2c44 Mon Sep 17 00:00:00 2001 From: Juan Hernandez Date: Wed, 24 Jun 2026 20:38:32 +0200 Subject: [PATCH 2/2] NO-ISSUE: Add unit test to validate CEL expressions in table definitions Rewrite the table renderer tests to use mock reflection helpers instead of a real gRPC server, making them faster and independent of network infrastructure. Add a new test that scans all YAML table definition files, resolves the corresponding protobuf type from the registry, and renders an empty instance through the table renderer. This forces compilation and evaluation of every CEL expression, catching syntax errors and references to non-existent fields at test time. Remove the stale `HostClass` table definitions which referenced a type that no longer exists in the proto registry. Assisted-by: Cursor Signed-off-by: Juan Hernandez --- internal/rendering/table_renderer.go | 2 +- .../table_renderer_compute_instance_test.go | 146 +++++++++ internal/rendering/table_renderer_test.go | 286 ++++++++++-------- .../tables/osac.private.v1.HostClass.yaml | 23 -- .../tables/osac.private.v1.RoleBinding.yaml | 4 +- .../tables/osac.public.v1.HostClass.yaml | 23 -- .../tables/osac.public.v1.RoleBinding.yaml | 4 +- 7 files changed, 313 insertions(+), 175 deletions(-) create mode 100644 internal/rendering/table_renderer_compute_instance_test.go delete mode 100644 internal/rendering/tables/osac.private.v1.HostClass.yaml delete mode 100644 internal/rendering/tables/osac.public.v1.HostClass.yaml diff --git a/internal/rendering/table_renderer.go b/internal/rendering/table_renderer.go index 66f47ee5c..737d67922 100644 --- a/internal/rendering/table_renderer.go +++ b/internal/rendering/table_renderer.go @@ -106,7 +106,7 @@ func (b *TableRendererBuilder) SetLogger(value *slog.Logger) *TableRendererBuild // SetHelper sets the reflection helper that will be used to introspect objects. This is mandatory. func (b *TableRendererBuilder) SetHelper(value reflection.Helper) *TableRendererBuilder { - b.helper = value + b.helper = reflection.NormalizeNil(value) return b } diff --git a/internal/rendering/table_renderer_compute_instance_test.go b/internal/rendering/table_renderer_compute_instance_test.go new file mode 100644 index 000000000..4d8e02de6 --- /dev/null +++ b/internal/rendering/table_renderer_compute_instance_test.go @@ -0,0 +1,146 @@ +/* +Copyright (c) 2025 Red Hat Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the +License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific +language governing permissions and limitations under the License. +*/ + +package rendering + +import ( + "bytes" + "context" + + . "github.com/onsi/ginkgo/v2/dsl/core" + . "github.com/onsi/ginkgo/v2/dsl/table" + . "github.com/onsi/gomega" + "go.uber.org/mock/gomock" + "google.golang.org/protobuf/proto" + + publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" + "github.com/osac-project/fulfillment-service/internal/reflection" + "github.com/osac-project/fulfillment-service/internal/uuid" +) + +var _ = Describe("Compute instance table rendering", func() { + var ctrl *gomock.Controller + + BeforeEach(func() { + ctrl = gomock.NewController(GinkgoT()) + DeferCleanup(ctrl.Finish) + }) + + DescribeTable( + "Resolves the TEMPLATE column", + func(ctx context.Context, templateName string, expectedSubstring string) { + // Create the template object: + templateMetadata := &publicv1.Metadata{} + if templateName != "" { + templateMetadata.SetName(templateName) + } + templateObj := &publicv1.ComputeInstanceTemplate{} + templateObj.SetId("my-template") + templateObj.SetMetadata(templateMetadata) + + // Create a helper that returns the template object: + templateDescriptor := templateObj.ProtoReflect().Descriptor() + templateFullName := templateDescriptor.FullName() + templateHelper := reflection.NewMockObjectHelper(ctrl) + templateHelper.EXPECT().FullName(). + Return(templateFullName). + AnyTimes() + templateHelper.EXPECT().Descriptor(). + Return(templateDescriptor). + AnyTimes() + templateHelper.EXPECT().String(). + Return(string(templateFullName)). + AnyTimes() + templateHelper.EXPECT(). + List(gomock.Any(), gomock.Any()). + Return( + reflection.ListResult{ + Items: []proto.Message{ + templateObj, + }, + Total: 1, + }, + nil). + AnyTimes() + templateHelper.EXPECT(). + GetMetadata(gomock.Any()). + Return(templateMetadata). + AnyTimes() + + // Create the compute instance object: + computeObj := publicv1.ComputeInstance_builder{ + Id: uuid.New(), + Metadata: publicv1.Metadata_builder{ + Name: "my-instance", + }.Build(), + Spec: publicv1.ComputeInstanceSpec_builder{ + Template: "my-template", + }.Build(), + }.Build() + + // Mock helper for the compute instance object: + computeDescriptor := computeObj.ProtoReflect().Descriptor() + computeHelper := reflection.NewMockObjectHelper(ctrl) + computeHelper.EXPECT().FullName(). + Return(computeDescriptor.FullName()). + AnyTimes() + computeHelper.EXPECT().Descriptor(). + Return(computeDescriptor). + AnyTimes() + computeHelper.EXPECT().String(). + Return(string(computeDescriptor.FullName())). + AnyTimes() + + // Create a helper that returns compute instances or templates based on the object type: + helper := reflection.NewMockHelper(ctrl) + helper.EXPECT(). + Lookup(gomock.Any()). + DoAndReturn(func(objectType string) reflection.ObjectHelper { + switch objectType { + case string(computeDescriptor.FullName()): + return computeHelper + case string(templateDescriptor.FullName()): + return templateHelper + default: + return nil + } + }). + AnyTimes() + + // Create the renderer: + buffer := &bytes.Buffer{} + renderer, err := NewTableRenderer(). + SetLogger(logger). + SetHelper(helper). + SetWriter(buffer). + Build() + Expect(err).ToNot(HaveOccurred()) + + // Render the compute instance object: + err = renderer.Render(ctx, []proto.Message{computeObj}) + Expect(err).ToNot(HaveOccurred()) + Expect(buffer.String()).To(ContainSubstring(expectedSubstring)) + }, + Entry( + // Regression for MGMT-23970: TEMPLATE column was blank when 'metadata.name' was empty. + "Falls back to the key when the looked-up object has no name", + "", + "my-template", + ), + Entry( + "Shows the template name when the looked-up object has a name", + "my-name", + "my-name", + ), + ) +}) diff --git a/internal/rendering/table_renderer_test.go b/internal/rendering/table_renderer_test.go index 378ac7eba..a9ed8d436 100644 --- a/internal/rendering/table_renderer_test.go +++ b/internal/rendering/table_renderer_test.go @@ -16,166 +16,204 @@ package rendering import ( "bytes" "context" + "io" + "path/filepath" + "strings" . "github.com/onsi/ginkgo/v2/dsl/core" - . "github.com/onsi/ginkgo/v2/dsl/table" . "github.com/onsi/gomega" - "google.golang.org/grpc" - "google.golang.org/grpc/credentials/insecure" + "go.uber.org/mock/gomock" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" "google.golang.org/protobuf/types/known/timestamppb" publicv1 "github.com/osac-project/fulfillment-service/internal/api/osac/public/v1" - "github.com/osac-project/fulfillment-service/internal/packages" "github.com/osac-project/fulfillment-service/internal/reflection" - internaltesting "github.com/osac-project/fulfillment-service/internal/testing" ) var _ = Describe("Table renderer", func() { - var ( - ctx context.Context - server *internaltesting.Server - connection *grpc.ClientConn - helper reflection.Helper - ) + var ctrl *gomock.Controller BeforeEach(func() { - var err error - ctx = context.Background() - - server = internaltesting.NewServer() - DeferCleanup(server.Stop) - - connection, err = grpc.NewClient( - server.Address(), - grpc.WithTransportCredentials(insecure.NewCredentials()), - ) - Expect(err).ToNot(HaveOccurred()) - DeferCleanup(connection.Close) - - helper, err = reflection.NewHelper(). - SetLogger(logger). - SetConnection(connection). - AddPackage(packages.PublicV1, 1). - Build() - Expect(err).ToNot(HaveOccurred()) + ctrl = gomock.NewController(GinkgoT()) + DeferCleanup(ctrl.Finish) }) - // registerTemplateAndRender registers a ComputeInstanceTemplates server that returns a single - // template with the given name (empty string means no name set), starts the server, renders one - // ComputeInstance via the table renderer, and returns the output. - registerTemplateAndRender := func(templateName string) string { - tmplBuilder := publicv1.ComputeInstanceTemplate_builder{ - Id: "osac.templates.ocp_virt_vm", - } - if templateName != "" { - tmplBuilder.Metadata = publicv1.Metadata_builder{Name: templateName}.Build() - } - publicv1.RegisterComputeInstanceTemplatesServer( - server.Registrar(), - &internaltesting.ComputeInstanceTemplatesServerFuncs{ - ListFunc: func( - _ context.Context, - _ *publicv1.ComputeInstanceTemplatesListRequest, - ) (*publicv1.ComputeInstanceTemplatesListResponse, error) { - return publicv1.ComputeInstanceTemplatesListResponse_builder{ - Size: 1, - Total: 1, - Items: []*publicv1.ComputeInstanceTemplate{tmplBuilder.Build()}, - }.Build(), nil - }, - }, - ) - server.Start() - - var buf bytes.Buffer - renderer, err := NewTableRenderer(). - SetLogger(logger). - SetHelper(helper). - SetWriter(&buf). - Build() - Expect(err).ToNot(HaveOccurred()) - - instance := publicv1.ComputeInstance_builder{ - Id: "019d53bd-42b4-7e23-b98e-6368490d3d83", - Metadata: publicv1.Metadata_builder{Name: "test"}.Build(), - Spec: publicv1.ComputeInstanceSpec_builder{Template: "osac.templates.ocp_virt_vm"}.Build(), - }.Build() - - err = renderer.Render(ctx, []*publicv1.ComputeInstance{instance}) - Expect(err).ToNot(HaveOccurred()) - return buf.String() + // makeObjectHelper creates a object helper that returns the descriptor for the given type. + makeObjectHelper := func(object proto.Message) *reflection.MockObjectHelper { + descriptor := object.ProtoReflect().Descriptor() + fullName := descriptor.FullName() + helper := reflection.NewMockObjectHelper(ctrl) + helper.EXPECT().FullName(). + Return(fullName). + AnyTimes() + helper.EXPECT().Descriptor(). + Return(descriptor). + AnyTimes() + helper.EXPECT().String(). + Return(string(fullName)). + AnyTimes() + return helper } - Describe("Lookup columns", func() { - DescribeTable( - "Resolves the TEMPLATE column", - func(templateName, expectedSubstring string) { - Expect(registerTemplateAndRender(templateName)).To(ContainSubstring(expectedSubstring)) - }, - Entry( - // Regression for MGMT-23970: TEMPLATE column was blank when metadata.name was empty. - "Falls back to the key when the looked-up object has no name", - "", - "osac.templates.ocp_virt_vm", - ), - Entry( - "Shows the template name when the looked-up object has a name", - "OpenShift Virt VM", - "OpenShift Virt VM", - ), - ) - }) + // makeLookupHelper creates a lookup helper that returns an empty list for any lookup column. + makeLookupHelper := func() *reflection.MockObjectHelper { + helper := reflection.NewMockObjectHelper(ctrl) + helper.EXPECT(). + List(gomock.Any(), gomock.Any()). + Return(reflection.ListResult{}, nil). + AnyTimes() + return helper + } Describe("DELETING column", func() { - renderSubnets := func(items []*publicv1.Subnet) string { - server.Start() - - var buf bytes.Buffer + renderSubnets := func(ctx context.Context, items []*publicv1.Subnet) string { + // Create the object helpers for the type of the table and for lookups: + objectHelper := makeObjectHelper(&publicv1.Subnet{}) + lookupHelper := makeLookupHelper() + + // Create the helper: + helper := reflection.NewMockHelper(ctrl) + helper.EXPECT(). + Lookup(objectHelper.String()). + Return(objectHelper). + AnyTimes() + helper.EXPECT(). + Lookup(gomock.Any()). + Return(lookupHelper). + AnyTimes() + + // Try to render the table: + buffer := &bytes.Buffer{} renderer, err := NewTableRenderer(). SetLogger(logger). SetHelper(helper). - SetWriter(&buf). + SetWriter(buffer). Build() Expect(err).ToNot(HaveOccurred()) - err = renderer.Render(ctx, items) Expect(err).ToNot(HaveOccurred()) - return buf.String() + return buffer.String() } - It("always includes the DELETING header", func() { - output := renderSubnets([]*publicv1.Subnet{ - publicv1.Subnet_builder{ - Id: "subnet-1", - Metadata: publicv1.Metadata_builder{Name: "active-subnet"}.Build(), - }.Build(), - }) + It("Always includes the DELETING header", func(ctx context.Context) { + output := renderSubnets( + ctx, + []*publicv1.Subnet{ + publicv1.Subnet_builder{ + Id: "subnet-1", + Metadata: publicv1.Metadata_builder{ + Name: "active-subnet", + }.Build(), + }.Build(), + }, + ) Expect(output).To(ContainSubstring("DELETING")) }) - It("shows dash for non-deleting objects", func() { - output := renderSubnets([]*publicv1.Subnet{ - publicv1.Subnet_builder{ - Id: "subnet-1", - Metadata: publicv1.Metadata_builder{Name: "active-subnet"}.Build(), - }.Build(), - }) + It("Shows dash for non-deleting objects", func(ctx context.Context) { + output := renderSubnets( + ctx, + []*publicv1.Subnet{ + publicv1.Subnet_builder{ + Id: "subnet-1", + Metadata: publicv1.Metadata_builder{ + Name: "active-subnet", + }.Build(), + }.Build(), + }, + ) Expect(output).To(MatchRegexp(`DELETING.*\n.*-`)) }) - It("shows Yes for deleting objects", func() { - ts := timestamppb.Now() - output := renderSubnets([]*publicv1.Subnet{ - publicv1.Subnet_builder{ - Id: "subnet-2", - Metadata: publicv1.Metadata_builder{ - Name: "deleting-subnet", - DeletionTimestamp: ts, + It("Shows 'Yes' for deleting objects", func(ctx context.Context) { + output := renderSubnets( + ctx, + []*publicv1.Subnet{ + publicv1.Subnet_builder{ + Id: "subnet-2", + Metadata: publicv1.Metadata_builder{ + Name: "deleting-subnet", + DeletionTimestamp: timestamppb.Now(), + }.Build(), }.Build(), - }.Build(), - }) + }) Expect(output).To(ContainSubstring("DELETING")) Expect(output).To(MatchRegexp(`subnet-2\s+Yes\s`)) }) }) + + It("Compiles CEL expressions successfully for all table definitions", func(ctx context.Context) { + // Collect all table definition files: + tableFiles, err := filepath.Glob("tables/*.yaml") + Expect(err).ToNot(HaveOccurred()) + for i, tableFile := range tableFiles { + tableFiles[i] = filepath.Base(tableFile) + } + Expect(tableFiles).ToNot( + BeEmpty(), + "Expected at least one '.yaml' file in the 'tables' directory, but found none", + ) + + // Iterate over all table definition files and compile the CEL expressions for each table. + for _, tableFile := range tableFiles { + // Find the object type: + objectName := strings.TrimSuffix(tableFile, ".yaml") + objectFullName := protoreflect.FullName(objectName) + objectType, err := protoregistry.GlobalTypes.FindMessageByName(objectFullName) + Expect(err).ToNot( + HaveOccurred(), + "Type '%s' for table file '%s' not found", + objectFullName, tableFile, + ) + + // Create the object helper for the type of the table. This needs to return the real full name + // and descriptor. + objectHelper := makeObjectHelper(objectType.New().Interface()) + + // The table will probably use lookup columns, and that requires an object helper for the looked + // up type. But we don't know that in advance, and we don't want to poke into the internals of + // the format of the table. Instead of that we create a helper that always responds to the + // 'List' method with an empty list of objects. It doesn't need to respond to any other methods. + lookupHelper := makeLookupHelper() + + // Configure the helper to return the object helper for the type of the table, and the lookup + // helper for any other type. + helper := reflection.NewMockHelper(ctrl) + helper.EXPECT(). + Lookup(objectName). + Return(objectHelper). + AnyTimes() + helper.EXPECT(). + Lookup(gomock.Any()). + Return(lookupHelper). + AnyTimes() + + // Build the renderer: + renderer, err := NewTableRenderer(). + SetLogger(logger). + SetHelper(helper). + SetWriter(io.Discard). + Build() + Expect(err).ToNot( + HaveOccurred(), + "Failed to build renderer for table file '%s'", + tableFile, + ) + + // Render a slice with a single empty instance, to force compilation and evaluation of all CEL + // program expressions in the table definition. + object := objectType.New().Interface() + objects := []proto.Message{ + object, + } + err = renderer.Render(ctx, objects) + Expect(err).ToNot( + HaveOccurred(), + "Failed to render table file '%s'", + tableFile, + ) + } + }) }) diff --git a/internal/rendering/tables/osac.private.v1.HostClass.yaml b/internal/rendering/tables/osac.private.v1.HostClass.yaml deleted file mode 100644 index 6e78c9a63..000000000 --- a/internal/rendering/tables/osac.private.v1.HostClass.yaml +++ /dev/null @@ -1,23 +0,0 @@ -# -# Copyright (c) 2025 Red Hat Inc. -# -# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with -# the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the -# specific language governing permissions and limitations under the License. -# - -columns: - -- header: ID - value: this.id - -- header: NAME - value: "has(this.metadata.name)? this.metadata.name: '-'" - -- header: TITLE - value: this.title diff --git a/internal/rendering/tables/osac.private.v1.RoleBinding.yaml b/internal/rendering/tables/osac.private.v1.RoleBinding.yaml index 0cd856f44..57ae40407 100644 --- a/internal/rendering/tables/osac.private.v1.RoleBinding.yaml +++ b/internal/rendering/tables/osac.private.v1.RoleBinding.yaml @@ -28,8 +28,8 @@ columns: type: osac.private.v1.Role lookup: true -- header: GROUPS - value: "size(this.spec.groups) > 0? this.spec.groups.join(', '): '-'" +- header: USERS + value: "size(this.spec.users) > 0? this.spec.users.join(', '): '-'" - header: TENANT value: "this.metadata.tenant != ''? this.metadata.tenant: '-'" diff --git a/internal/rendering/tables/osac.public.v1.HostClass.yaml b/internal/rendering/tables/osac.public.v1.HostClass.yaml deleted file mode 100644 index 6e78c9a63..000000000 --- a/internal/rendering/tables/osac.public.v1.HostClass.yaml +++ /dev/null @@ -1,23 +0,0 @@ -# -# Copyright (c) 2025 Red Hat Inc. -# -# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with -# the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the -# specific language governing permissions and limitations under the License. -# - -columns: - -- header: ID - value: this.id - -- header: NAME - value: "has(this.metadata.name)? this.metadata.name: '-'" - -- header: TITLE - value: this.title diff --git a/internal/rendering/tables/osac.public.v1.RoleBinding.yaml b/internal/rendering/tables/osac.public.v1.RoleBinding.yaml index 5e61179e1..41269dba2 100644 --- a/internal/rendering/tables/osac.public.v1.RoleBinding.yaml +++ b/internal/rendering/tables/osac.public.v1.RoleBinding.yaml @@ -28,5 +28,5 @@ columns: type: osac.public.v1.Role lookup: true -- header: GROUPS - value: "size(this.spec.groups) > 0? this.spec.groups.join(', '): '-'" +- header: USERS + value: "size(this.spec.users) > 0? this.spec.users.join(', '): '-'"