From c646eba38619f1549e041b0ed5651d4e98e43f4e Mon Sep 17 00:00:00 2001 From: Robert Gogolok Date: Sat, 26 Oct 2024 03:07:04 +0200 Subject: [PATCH] Push service instance sorting logic to repo layer Fixes #3539 --- api/handlers/service_instance.go | 22 ---- api/handlers/service_instance_test.go | 42 ------- api/main.go | 1 + api/payloads/service_instance.go | 1 + api/payloads/service_instance_test.go | 1 + .../fake/service_instance_sorter.go | 118 ++++++++++++++++++ .../service_instance_repository.go | 47 ++++++- .../service_instance_repository_test.go | 65 +++++++++- 8 files changed, 230 insertions(+), 67 deletions(-) create mode 100644 api/repositories/fake/service_instance_sorter.go diff --git a/api/handlers/service_instance.go b/api/handlers/service_instance.go index a34c3f90a..e16c4009b 100644 --- a/api/handlers/service_instance.go +++ b/api/handlers/service_instance.go @@ -4,7 +4,6 @@ import ( "context" "net/http" "net/url" - "sort" apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/handlers/include" @@ -160,8 +159,6 @@ func (h *ServiceInstance) list(r *http.Request) (*routing.Response, error) { return nil, apierrors.LogAndReturn(logger, err, "Failed to list service instance") } - h.sortList(serviceInstances, payload.OrderBy) - includedResources, err := h.includeResolver.ResolveIncludes(r.Context(), authInfo, serviceInstances, payload.IncludeResourceRules) if err != nil { return nil, apierrors.LogAndReturn(logger, err, "failed to build included resources") @@ -170,25 +167,6 @@ func (h *ServiceInstance) list(r *http.Request) (*routing.Response, error) { return routing.NewResponse(http.StatusOK).WithBody(presenter.ForList(presenter.ForServiceInstance, serviceInstances, h.serverURL, *r.URL, includedResources...)), nil } -// nolint:dupl -func (h *ServiceInstance) sortList(siList []repositories.ServiceInstanceRecord, order string) { - switch order { - case "": - case "created_at": - sort.Slice(siList, func(i, j int) bool { return timePtrAfter(&siList[j].CreatedAt, &siList[i].CreatedAt) }) - case "-created_at": - sort.Slice(siList, func(i, j int) bool { return timePtrAfter(&siList[i].CreatedAt, &siList[j].CreatedAt) }) - case "updated_at": - sort.Slice(siList, func(i, j int) bool { return timePtrAfter(siList[j].UpdatedAt, siList[i].UpdatedAt) }) - case "-updated_at": - sort.Slice(siList, func(i, j int) bool { return timePtrAfter(siList[i].UpdatedAt, siList[j].UpdatedAt) }) - case "name": - sort.Slice(siList, func(i, j int) bool { return siList[i].Name < siList[j].Name }) - case "-name": - sort.Slice(siList, func(i, j int) bool { return siList[i].Name > siList[j].Name }) - } -} - func (h *ServiceInstance) delete(r *http.Request) (*routing.Response, error) { authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.service-instance.delete") diff --git a/api/handlers/service_instance_test.go b/api/handlers/service_instance_test.go index 41abad7df..016fd730d 100644 --- a/api/handlers/service_instance_test.go +++ b/api/handlers/service_instance_test.go @@ -3,9 +3,7 @@ package handlers_test import ( "errors" "net/http" - "net/http/httptest" "strings" - "time" apierrors "code.cloudfoundry.org/korifi/api/errors" . "code.cloudfoundry.org/korifi/api/handlers" @@ -388,46 +386,6 @@ var _ = Describe("ServiceInstance", func() { }) }) - Describe("Order results", func() { - BeforeEach(func() { - serviceInstanceRepo.ListServiceInstancesReturns([]repositories.ServiceInstanceRecord{ - { - GUID: "1", - Name: "first-test-si", - CreatedAt: time.UnixMilli(3000), - UpdatedAt: tools.PtrTo(time.UnixMilli(4000)), - }, - { - GUID: "2", - Name: "second-test-si", - CreatedAt: time.UnixMilli(2000), - UpdatedAt: tools.PtrTo(time.UnixMilli(5000)), - }, - { - GUID: "3", - Name: "third-test-si", - CreatedAt: time.UnixMilli(1000), - UpdatedAt: tools.PtrTo(time.UnixMilli(6000)), - }, - }, nil) - }) - - DescribeTable("ordering results", func(orderBy string, expectedOrder ...any) { - requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.ServiceInstanceList{OrderBy: orderBy}) - req := createHttpRequest("GET", "/v3/service_instances?order_by=not-used", nil) - rr = httptest.NewRecorder() - routerBuilder.Build().ServeHTTP(rr, req) - Expect(rr).To(HaveHTTPBody(MatchJSONPath("$.resources[*].guid", expectedOrder))) - }, - Entry("created_at ASC", "created_at", "3", "2", "1"), - Entry("created_at DESC", "-created_at", "1", "2", "3"), - Entry("updated_at ASC", "updated_at", "1", "2", "3"), - Entry("updated_at DESC", "-updated_at", "3", "2", "1"), - Entry("name ASC", "name", "1", "2", "3"), - Entry("name DESC", "-name", "3", "2", "1"), - ) - }) - When("there is an error fetching service instances", func() { BeforeEach(func() { serviceInstanceRepo.ListServiceInstancesReturns([]repositories.ServiceInstanceRecord{}, errors.New("unknown!")) diff --git a/api/main.go b/api/main.go index 9dba65120..535626b18 100644 --- a/api/main.go +++ b/api/main.go @@ -200,6 +200,7 @@ func main() { userClientFactory, nsPermissions, conditions.NewConditionAwaiter[*korifiv1alpha1.CFServiceInstance, korifiv1alpha1.CFServiceInstance, korifiv1alpha1.CFServiceInstanceList](conditionTimeout), + repositories.NewServiceInstanceSorter(), ) serviceBindingRepo := repositories.NewServiceBindingRepo( namespaceRetriever, diff --git a/api/payloads/service_instance.go b/api/payloads/service_instance.go index f43cac410..7ade526af 100644 --- a/api/payloads/service_instance.go +++ b/api/payloads/service_instance.go @@ -210,6 +210,7 @@ func (l *ServiceInstanceList) ToMessage() repositories.ListServiceInstanceMessag Names: parse.ArrayParam(l.Names), SpaceGUIDs: parse.ArrayParam(l.SpaceGUIDs), GUIDs: parse.ArrayParam(l.GUIDs), + OrderBy: l.OrderBy, LabelSelector: l.LabelSelector, } } diff --git a/api/payloads/service_instance_test.go b/api/payloads/service_instance_test.go index 9e5aa2a4b..fa6442c48 100644 --- a/api/payloads/service_instance_test.go +++ b/api/payloads/service_instance_test.go @@ -89,6 +89,7 @@ var _ = Describe("ServiceInstanceList", func() { Names: []string{"n1", "n2"}, SpaceGUIDs: []string{"sg1", "sg2"}, GUIDs: []string{"g1", "g2"}, + OrderBy: "order", LabelSelector: "foo=bar", })) }) diff --git a/api/repositories/fake/service_instance_sorter.go b/api/repositories/fake/service_instance_sorter.go new file mode 100644 index 000000000..f74b423ba --- /dev/null +++ b/api/repositories/fake/service_instance_sorter.go @@ -0,0 +1,118 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package fake + +import ( + "sync" + + "code.cloudfoundry.org/korifi/api/repositories" +) + +type ServiceInstanceSorter struct { + SortStub func([]repositories.ServiceInstanceRecord, string) []repositories.ServiceInstanceRecord + sortMutex sync.RWMutex + sortArgsForCall []struct { + arg1 []repositories.ServiceInstanceRecord + arg2 string + } + sortReturns struct { + result1 []repositories.ServiceInstanceRecord + } + sortReturnsOnCall map[int]struct { + result1 []repositories.ServiceInstanceRecord + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *ServiceInstanceSorter) Sort(arg1 []repositories.ServiceInstanceRecord, arg2 string) []repositories.ServiceInstanceRecord { + var arg1Copy []repositories.ServiceInstanceRecord + if arg1 != nil { + arg1Copy = make([]repositories.ServiceInstanceRecord, len(arg1)) + copy(arg1Copy, arg1) + } + fake.sortMutex.Lock() + ret, specificReturn := fake.sortReturnsOnCall[len(fake.sortArgsForCall)] + fake.sortArgsForCall = append(fake.sortArgsForCall, struct { + arg1 []repositories.ServiceInstanceRecord + arg2 string + }{arg1Copy, arg2}) + stub := fake.SortStub + fakeReturns := fake.sortReturns + fake.recordInvocation("Sort", []interface{}{arg1Copy, arg2}) + fake.sortMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *ServiceInstanceSorter) SortCallCount() int { + fake.sortMutex.RLock() + defer fake.sortMutex.RUnlock() + return len(fake.sortArgsForCall) +} + +func (fake *ServiceInstanceSorter) SortCalls(stub func([]repositories.ServiceInstanceRecord, string) []repositories.ServiceInstanceRecord) { + fake.sortMutex.Lock() + defer fake.sortMutex.Unlock() + fake.SortStub = stub +} + +func (fake *ServiceInstanceSorter) SortArgsForCall(i int) ([]repositories.ServiceInstanceRecord, string) { + fake.sortMutex.RLock() + defer fake.sortMutex.RUnlock() + argsForCall := fake.sortArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *ServiceInstanceSorter) SortReturns(result1 []repositories.ServiceInstanceRecord) { + fake.sortMutex.Lock() + defer fake.sortMutex.Unlock() + fake.SortStub = nil + fake.sortReturns = struct { + result1 []repositories.ServiceInstanceRecord + }{result1} +} + +func (fake *ServiceInstanceSorter) SortReturnsOnCall(i int, result1 []repositories.ServiceInstanceRecord) { + fake.sortMutex.Lock() + defer fake.sortMutex.Unlock() + fake.SortStub = nil + if fake.sortReturnsOnCall == nil { + fake.sortReturnsOnCall = make(map[int]struct { + result1 []repositories.ServiceInstanceRecord + }) + } + fake.sortReturnsOnCall[i] = struct { + result1 []repositories.ServiceInstanceRecord + }{result1} +} + +func (fake *ServiceInstanceSorter) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.sortMutex.RLock() + defer fake.sortMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *ServiceInstanceSorter) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ repositories.ServiceInstanceSorter = new(ServiceInstanceSorter) diff --git a/api/repositories/service_instance_repository.go b/api/repositories/service_instance_repository.go index c348edf20..137df1141 100644 --- a/api/repositories/service_instance_repository.go +++ b/api/repositories/service_instance_repository.go @@ -6,10 +6,12 @@ import ( "errors" "fmt" "slices" + "strings" "time" "code.cloudfoundry.org/korifi/api/authorization" apierrors "code.cloudfoundry.org/korifi/api/errors" + "code.cloudfoundry.org/korifi/api/repositories/compare" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/model" "code.cloudfoundry.org/korifi/tools" @@ -43,6 +45,46 @@ type ServiceInstanceRepo struct { userClientFactory authorization.UserK8sClientFactory namespacePermissions *authorization.NamespacePermissions awaiter Awaiter[*korifiv1alpha1.CFServiceInstance] + sorter ServiceInstanceSorter +} + +//counterfeiter:generate -o fake -fake-name ServiceInstanceSorter . ServiceInstanceSorter +type ServiceInstanceSorter interface { + Sort(records []ServiceInstanceRecord, order string) []ServiceInstanceRecord +} + +type serviceInstanceSorter struct { + sorter *compare.Sorter[ServiceInstanceRecord] +} + +func NewServiceInstanceSorter() *serviceInstanceSorter { + return &serviceInstanceSorter{ + sorter: compare.NewSorter(ServiceInstanceComparator), + } +} + +func (s *serviceInstanceSorter) Sort(records []ServiceInstanceRecord, order string) []ServiceInstanceRecord { + return s.sorter.Sort(records, order) +} + +func ServiceInstanceComparator(fieldName string) func(ServiceInstanceRecord, ServiceInstanceRecord) int { + return func(s1, s2 ServiceInstanceRecord) int { + switch fieldName { + case "created_at": + return tools.CompareTimePtr(&s1.CreatedAt, &s2.CreatedAt) + case "-created_at": + return tools.CompareTimePtr(&s2.CreatedAt, &s1.CreatedAt) + case "updated_at": + return tools.CompareTimePtr(s1.UpdatedAt, s2.UpdatedAt) + case "-updated_at": + return tools.CompareTimePtr(s2.UpdatedAt, s1.UpdatedAt) + case "name": + return strings.Compare(s1.Name, s2.Name) + case "-name": + return strings.Compare(s2.Name, s1.Name) + } + return 0 + } } func NewServiceInstanceRepo( @@ -50,12 +92,14 @@ func NewServiceInstanceRepo( userClientFactory authorization.UserK8sClientFactory, namespacePermissions *authorization.NamespacePermissions, awaiter Awaiter[*korifiv1alpha1.CFServiceInstance], + sorter ServiceInstanceSorter, ) *ServiceInstanceRepo { return &ServiceInstanceRepo{ namespaceRetriever: namespaceRetriever, userClientFactory: userClientFactory, namespacePermissions: namespacePermissions, awaiter: awaiter, + sorter: sorter, } } @@ -102,6 +146,7 @@ type ListServiceInstanceMessage struct { SpaceGUIDs []string GUIDs []string LabelSelector string + OrderBy string } func (m *ListServiceInstanceMessage) matches(serviceInstance korifiv1alpha1.CFServiceInstance) bool { @@ -347,7 +392,7 @@ func (r *ServiceInstanceRepo) ListServiceInstances(ctx context.Context, authInfo } filteredServiceInstances := itx.FromSlice(serviceInstances).Filter(message.matches) - return slices.Collect(it.Map(filteredServiceInstances, cfServiceInstanceToRecord)), nil + return r.sorter.Sort(slices.Collect(it.Map(filteredServiceInstances, cfServiceInstanceToRecord)), message.OrderBy), nil } func (r *ServiceInstanceRepo) GetServiceInstance(ctx context.Context, authInfo authorization.Info, guid string) (ServiceInstanceRecord, error) { diff --git a/api/repositories/service_instance_repository_test.go b/api/repositories/service_instance_repository_test.go index 88a18a82e..82ef2850d 100644 --- a/api/repositories/service_instance_repository_test.go +++ b/api/repositories/service_instance_repository_test.go @@ -9,6 +9,7 @@ import ( apierrors "code.cloudfoundry.org/korifi/api/errors" "code.cloudfoundry.org/korifi/api/repositories" + "code.cloudfoundry.org/korifi/api/repositories/fake" "code.cloudfoundry.org/korifi/api/repositories/fakeawaiter" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" "code.cloudfoundry.org/korifi/model" @@ -21,6 +22,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" . "github.com/onsi/gomega/gstruct" + gomega_types "github.com/onsi/gomega/types" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -37,6 +39,7 @@ var _ = Describe("ServiceInstanceRepository", func() { korifiv1alpha1.CFServiceInstanceList, *korifiv1alpha1.CFServiceInstanceList, ] + sorter *fake.ServiceInstanceSorter org *korifiv1alpha1.CFOrg space *korifiv1alpha1.CFSpace @@ -50,7 +53,12 @@ var _ = Describe("ServiceInstanceRepository", func() { korifiv1alpha1.CFServiceInstanceList, *korifiv1alpha1.CFServiceInstanceList, ]{} - serviceInstanceRepo = repositories.NewServiceInstanceRepo(namespaceRetriever, userClientFactory, nsPerms, conditionAwaiter) + sorter = new(fake.ServiceInstanceSorter) + sorter.SortStub = func(records []repositories.ServiceInstanceRecord, _ string) []repositories.ServiceInstanceRecord { + return records + } + + serviceInstanceRepo = repositories.NewServiceInstanceRepo(namespaceRetriever, userClientFactory, nsPerms, conditionAwaiter, sorter) org = createOrgWithCleanup(ctx, uuid.NewString()) space = createSpaceWithCleanup(ctx, org.Name, uuid.NewString()) @@ -627,7 +635,7 @@ var _ = Describe("ServiceInstanceRepository", func() { }, })).To(Succeed()) - filters = repositories.ListServiceInstanceMessage{} + filters = repositories.ListServiceInstanceMessage{OrderBy: "foo"} }) JustBeforeEach(func() { @@ -654,6 +662,17 @@ var _ = Describe("ServiceInstanceRepository", func() { )) }) + It("sort the service instances", func() { + Expect(sorter.SortCallCount()).To(Equal(1)) + sortedServiceInstances, field := sorter.SortArgsForCall(0) + Expect(field).To(Equal("foo")) + Expect(sortedServiceInstances).To(ConsistOf( + MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance1.Name)}), + MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance2.Name)}), + MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfServiceInstance3.Name)}), + )) + }) + When("the name filter is set", func() { BeforeEach(func() { filters = repositories.ListServiceInstanceMessage{ @@ -914,3 +933,45 @@ var _ = Describe("ServiceInstanceRepository", func() { }) }) }) + +var _ = DescribeTable("ServiceInstanceSorter", + func(s1, s2 repositories.ServiceInstanceRecord, field string, match gomega_types.GomegaMatcher) { + Expect(repositories.ServiceInstanceComparator(field)(s1, s2)).To(match) + }, + Entry("created_at", + repositories.ServiceInstanceRecord{CreatedAt: time.UnixMilli(1)}, + repositories.ServiceInstanceRecord{CreatedAt: time.UnixMilli(2)}, + "created_at", + BeNumerically("<", 0), + ), + Entry("-created_at", + repositories.ServiceInstanceRecord{CreatedAt: time.UnixMilli(1)}, + repositories.ServiceInstanceRecord{CreatedAt: time.UnixMilli(2)}, + "-created_at", + BeNumerically(">", 0), + ), + Entry("updated_at", + repositories.ServiceInstanceRecord{UpdatedAt: tools.PtrTo(time.UnixMilli(1))}, + repositories.ServiceInstanceRecord{UpdatedAt: tools.PtrTo(time.UnixMilli(2))}, + "updated_at", + BeNumerically("<", 0), + ), + Entry("-updated_at", + repositories.ServiceInstanceRecord{UpdatedAt: tools.PtrTo(time.UnixMilli(1))}, + repositories.ServiceInstanceRecord{UpdatedAt: tools.PtrTo(time.UnixMilli(2))}, + "-updated_at", + BeNumerically(">", 0), + ), + Entry("name", + repositories.ServiceInstanceRecord{Name: "first-instance"}, + repositories.ServiceInstanceRecord{Name: "second-instance"}, + "name", + BeNumerically("<", 0), + ), + Entry("-name", + repositories.ServiceInstanceRecord{Name: "first-instance"}, + repositories.ServiceInstanceRecord{Name: "second-instance"}, + "-name", + BeNumerically(">", 0), + ), +)