diff --git a/api/handlers/app.go b/api/handlers/app.go index bc37f1854..5c6c30dea 100644 --- a/api/handlers/app.go +++ b/api/handlers/app.go @@ -6,7 +6,6 @@ import ( "fmt" "net/http" "net/url" - "sort" "strconv" "time" @@ -160,19 +159,17 @@ func (h *App) list(r *http.Request) (*routing.Response, error) { //nolint:dupl authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.app.list") - appListFilter := new(payloads.AppList) - err := h.requestValidator.DecodeAndValidateURLValues(r, appListFilter) + payload := new(payloads.AppList) + err := h.requestValidator.DecodeAndValidateURLValues(r, payload) if err != nil { return nil, apierrors.LogAndReturn(logger, err, "Unable to decode request query parameters") } - appList, err := h.appRepo.ListApps(r.Context(), authInfo, appListFilter.ToMessage()) + appList, err := h.appRepo.ListApps(r.Context(), authInfo, payload.ToMessage()) if err != nil { return nil, apierrors.LogAndReturn(logger, err, "Failed to fetch app(s) from Kubernetes") } - h.sortList(appList, appListFilter.OrderBy) - return routing.NewResponse(http.StatusOK).WithBody(presenter.ForList(presenter.ForApp, appList, h.serverURL, *r.URL)), nil } @@ -184,28 +181,6 @@ func timePtrAfter(t1, t2 *time.Time) bool { return (*t1).After(*t2) } -func (h *App) sortList(appList []repositories.AppRecord, order string) { - switch order { - case "": - case "created_at": - sort.Slice(appList, func(i, j int) bool { return timePtrAfter(&appList[j].CreatedAt, &appList[i].CreatedAt) }) - case "-created_at": - sort.Slice(appList, func(i, j int) bool { return timePtrAfter(&appList[i].CreatedAt, &appList[j].CreatedAt) }) - case "updated_at": - sort.Slice(appList, func(i, j int) bool { return timePtrAfter(appList[j].UpdatedAt, appList[i].UpdatedAt) }) - case "-updated_at": - sort.Slice(appList, func(i, j int) bool { return timePtrAfter(appList[i].UpdatedAt, appList[j].UpdatedAt) }) - case "name": - sort.Slice(appList, func(i, j int) bool { return appList[i].Name < appList[j].Name }) - case "-name": - sort.Slice(appList, func(i, j int) bool { return appList[i].Name > appList[j].Name }) - case "state": - sort.Slice(appList, func(i, j int) bool { return appList[i].State < appList[j].State }) - case "-state": - sort.Slice(appList, func(i, j int) bool { return appList[i].State > appList[j].State }) - } -} - func (h *App) setCurrentDroplet(r *http.Request) (*routing.Response, error) { authInfo, _ := authorization.InfoFromContext(r.Context()) logger := logr.FromContextOrDiscard(r.Context()).WithName("handlers.app.set-current-droplet") diff --git a/api/handlers/app_test.go b/api/handlers/app_test.go index ee245dab3..40915d193 100644 --- a/api/handlers/app_test.go +++ b/api/handlers/app_test.go @@ -4,7 +4,6 @@ import ( "errors" "io" "net/http" - "net/http/httptest" "strings" "time" @@ -385,60 +384,6 @@ var _ = Describe("App", func() { }) }) - Describe("Order results", func() { - BeforeEach(func() { - appRepo.ListAppsReturns([]repositories.AppRecord{ - { - GUID: "1", - Name: "first-test-app", - State: "STOPPED", - CreatedAt: time.UnixMilli(4000), - UpdatedAt: tools.PtrTo(time.UnixMilli(5000)), - }, - { - GUID: "2", - Name: "second-test-app", - State: "BROKEN", - CreatedAt: time.UnixMilli(3000), - UpdatedAt: tools.PtrTo(time.UnixMilli(6000)), - }, - { - GUID: "3", - Name: "third-test-app", - State: "STARTED", - CreatedAt: time.UnixMilli(1000), - UpdatedAt: tools.PtrTo(time.UnixMilli(8000)), - }, - { - GUID: "4", - Name: "fourth-test-app", - State: "FIXED", - CreatedAt: time.UnixMilli(2000), - UpdatedAt: tools.PtrTo(time.UnixMilli(7000)), - }, - }, nil) - }) - - DescribeTable("ordering results", func(orderBy string, expectedOrder ...any) { - requestValidator.DecodeAndValidateURLValuesStub = decodeAndValidateURLValuesStub(&payloads.AppList{ - OrderBy: orderBy, - }) - req = createHttpRequest("GET", "/v3/apps?order_by=whatever", nil) - rr = httptest.NewRecorder() - routerBuilder.Build().ServeHTTP(rr, req) - Expect(rr).To(HaveHTTPBody(MatchJSONPath("$.resources[*].guid", expectedOrder))) - }, - Entry("created_at ASC", "created_at", "3", "4", "2", "1"), - Entry("created_at DESC", "-created_at", "1", "2", "4", "3"), - Entry("updated_at ASC", "updated_at", "1", "2", "4", "3"), - Entry("updated_at DESC", "-updated_at", "3", "4", "2", "1"), - Entry("name ASC", "name", "1", "4", "2", "3"), - Entry("name DESC", "-name", "3", "2", "4", "1"), - Entry("state ASC", "state", "2", "4", "3", "1"), - Entry("state DESC", "-state", "1", "3", "4", "2"), - ) - }) - When("no apps can be found", func() { BeforeEach(func() { appRepo.ListAppsReturns([]repositories.AppRecord{}, nil) diff --git a/api/main.go b/api/main.go index 4e6c1fbdd..9dba65120 100644 --- a/api/main.go +++ b/api/main.go @@ -150,6 +150,7 @@ func main() { userClientFactory, nsPermissions, conditions.NewConditionAwaiter[*korifiv1alpha1.CFApp, korifiv1alpha1.CFApp, korifiv1alpha1.CFAppList](conditionTimeout), + repositories.NewAppSorter(), ) dropletRepo := repositories.NewDropletRepo( userClientFactory, diff --git a/api/payloads/app.go b/api/payloads/app.go index c9068a7f3..5eaaeb9fa 100644 --- a/api/payloads/app.go +++ b/api/payloads/app.go @@ -101,6 +101,7 @@ func (a *AppList) ToMessage() repositories.ListAppsMessage { Guids: parse.ArrayParam(a.GUIDs), SpaceGUIDs: parse.ArrayParam(a.SpaceGUIDs), LabelSelector: a.LabelSelector, + OrderBy: a.OrderBy, } } diff --git a/api/payloads/app_test.go b/api/payloads/app_test.go index ab937c9e7..2d86d20e5 100644 --- a/api/payloads/app_test.go +++ b/api/payloads/app_test.go @@ -55,6 +55,7 @@ var _ = Describe("AppList", func() { Names: []string{"n1", "n2"}, Guids: []string{"g1", "g2"}, SpaceGUIDs: []string{"s1", "s2"}, + OrderBy: "created_at", LabelSelector: "foo=bar", })) }) diff --git a/api/repositories/app_repository.go b/api/repositories/app_repository.go index ddbd125d3..7c2c75268 100644 --- a/api/repositories/app_repository.go +++ b/api/repositories/app_repository.go @@ -11,6 +11,7 @@ import ( "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/controllers/controllers/workloads/env" "code.cloudfoundry.org/korifi/controllers/webhooks/validation" @@ -47,6 +48,50 @@ type AppRepo struct { userClientFactory authorization.UserK8sClientFactory namespacePermissions *authorization.NamespacePermissions appAwaiter Awaiter[*korifiv1alpha1.CFApp] + sorter AppSorter +} + +//counterfeiter:generate -o fake -fake-name AppSorter . AppSorter +type AppSorter interface { + Sort(records []AppRecord, order string) []AppRecord +} + +type appSorter struct { + sorter *compare.Sorter[AppRecord] +} + +func NewAppSorter() *appSorter { + return &appSorter{ + sorter: compare.NewSorter(AppComparator), + } +} + +func (s *appSorter) Sort(records []AppRecord, order string) []AppRecord { + return s.sorter.Sort(records, order) +} + +func AppComparator(fieldName string) func(AppRecord, AppRecord) int { + return func(a1, a2 AppRecord) int { + switch fieldName { + case "", "name": + return strings.Compare(a1.Name, a2.Name) + case "-name": + return strings.Compare(a2.Name, a1.Name) + case "created_at": + return tools.CompareTimePtr(&a1.CreatedAt, &a2.CreatedAt) + case "-created_at": + return tools.CompareTimePtr(&a2.CreatedAt, &a1.CreatedAt) + case "updated_at": + return tools.CompareTimePtr(a1.UpdatedAt, a2.UpdatedAt) + case "-updated_at": + return tools.CompareTimePtr(a2.UpdatedAt, a1.UpdatedAt) + case "state": + return strings.Compare(string(a1.State), string(a2.State)) + case "-state": + return strings.Compare(string(a2.State), string(a1.State)) + } + return 0 + } } func NewAppRepo( @@ -54,12 +99,14 @@ func NewAppRepo( userClientFactory authorization.UserK8sClientFactory, authPerms *authorization.NamespacePermissions, appAwaiter Awaiter[*korifiv1alpha1.CFApp], + sorter AppSorter, ) *AppRepo { return &AppRepo{ namespaceRetriever: namespaceRetriever, userClientFactory: userClientFactory, namespacePermissions: authPerms, appAwaiter: appAwaiter, + sorter: sorter, } } @@ -188,6 +235,7 @@ type ListAppsMessage struct { Guids []string SpaceGUIDs []string LabelSelector string + OrderBy string } func (m *ListAppsMessage) matchesNamespace(ns string) bool { @@ -334,15 +382,9 @@ func (f *AppRepo) ListApps(ctx context.Context, authInfo authorization.Info, mes apps = append(apps, appList.Items...) } - // By default sort it by App.DisplayName - appRecords := slices.SortedFunc( - it.Map(itx.FromSlice(apps).Filter(message.matches), cfAppToAppRecord), - func(a, b AppRecord) int { - return strings.Compare(a.Name, b.Name) - }, - ) + appRecords := it.Map(itx.FromSlice(apps).Filter(message.matches), cfAppToAppRecord) - return appRecords, nil + return f.sorter.Sort(slices.Collect(appRecords), message.OrderBy), nil } func (f *AppRepo) PatchAppEnvVars(ctx context.Context, authInfo authorization.Info, message PatchAppEnvVarsMessage) (AppEnvVarsRecord, error) { diff --git a/api/repositories/app_repository_test.go b/api/repositories/app_repository_test.go index cc33b9ac2..aff8e0ca2 100644 --- a/api/repositories/app_repository_test.go +++ b/api/repositories/app_repository_test.go @@ -4,12 +4,11 @@ import ( "context" "encoding/json" "errors" - "fmt" - "sort" "time" 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/controllers/controllers/workloads/env" @@ -22,6 +21,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" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -42,6 +42,7 @@ var _ = Describe("AppRepository", func() { korifiv1alpha1.CFAppList, *korifiv1alpha1.CFAppList, ] + sorter *fake.AppSorter appRepo *repositories.AppRepo cfOrg *korifiv1alpha1.CFOrg cfSpace *korifiv1alpha1.CFSpace @@ -55,7 +56,11 @@ var _ = Describe("AppRepository", func() { korifiv1alpha1.CFAppList, *korifiv1alpha1.CFAppList, ]{} - appRepo = repositories.NewAppRepo(namespaceRetriever, userClientFactory, nsPerms, appAwaiter) + sorter = new(fake.AppSorter) + sorter.SortStub = func(records []repositories.AppRecord, _ string) []repositories.AppRecord { + return records + } + appRepo = repositories.NewAppRepo(namespaceRetriever, userClientFactory, nsPerms, appAwaiter, sorter) cfOrg = createOrgWithCleanup(ctx, prefixedGUID("org")) cfSpace = createSpaceWithCleanup(ctx, cfOrg.Name, prefixedGUID("space1")) @@ -194,7 +199,7 @@ var _ = Describe("AppRepository", func() { ) BeforeEach(func() { - message = repositories.ListAppsMessage{} + message = repositories.ListAppsMessage{OrderBy: "foo"} space2 := createSpaceWithCleanup(ctx, cfOrg.Name, prefixedGUID("space2")) space3 := createSpaceWithCleanup(ctx, cfOrg.Name, prefixedGUID("space3")) @@ -215,12 +220,17 @@ var _ = Describe("AppRepository", func() { MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp.Name)}), MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp2.Name)}), )) + }) - sortedByName := sort.SliceIsSorted(appList, func(i, j int) bool { - return appList[i].Name < appList[j].Name - }) + It("sorts the apps", func() { + Expect(sorter.SortCallCount()).To(Equal(1)) + sortedApps, field := sorter.SortArgsForCall(0) + Expect(field).To(Equal("foo")) - Expect(sortedByName).To(BeTrue(), fmt.Sprintf("AppList was not sorted by Name : App1 : %s , App2: %s", appList[0].Name, appList[1].Name)) + Expect(sortedApps).To(ConsistOf( + MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp.Name)}), + MatchFields(IgnoreExtras, Fields{"GUID": Equal(cfApp2.Name)}), + )) }) When("there are apps in non-cf namespaces", func() { @@ -1386,3 +1396,63 @@ func asMapOfStrings(data map[string][]byte) map[string]string { return result } + +var _ = DescribeTable("AppSorter", + func(a1, a2 repositories.AppRecord, field string, match gomega_types.GomegaMatcher) { + Expect(repositories.AppComparator(field)(a1, a2)).To(match) + }, + Entry("default sorting", + repositories.AppRecord{Name: "first-app"}, + repositories.AppRecord{Name: "second-app"}, + "", + BeNumerically("<", 0), + ), + Entry("name", + repositories.AppRecord{Name: "first-app"}, + repositories.AppRecord{Name: "second-app"}, + "name", + BeNumerically("<", 0), + ), + Entry("-name", + repositories.AppRecord{Name: "first-app"}, + repositories.AppRecord{Name: "second-app"}, + "-name", + BeNumerically(">", 0), + ), + Entry("created_at", + repositories.AppRecord{CreatedAt: time.UnixMilli(1)}, + repositories.AppRecord{CreatedAt: time.UnixMilli(2)}, + "created_at", + BeNumerically("<", 0), + ), + Entry("-created_at", + repositories.AppRecord{CreatedAt: time.UnixMilli(1)}, + repositories.AppRecord{CreatedAt: time.UnixMilli(2)}, + "-created_at", + BeNumerically(">", 0), + ), + Entry("updated_at", + repositories.AppRecord{UpdatedAt: tools.PtrTo(time.UnixMilli(1))}, + repositories.AppRecord{UpdatedAt: tools.PtrTo(time.UnixMilli(2))}, + "updated_at", + BeNumerically("<", 0), + ), + Entry("-updated_at", + repositories.AppRecord{UpdatedAt: tools.PtrTo(time.UnixMilli(1))}, + repositories.AppRecord{UpdatedAt: tools.PtrTo(time.UnixMilli(2))}, + "-updated_at", + BeNumerically(">", 0), + ), + Entry("state", + repositories.AppRecord{State: repositories.StartedState}, + repositories.AppRecord{State: repositories.StoppedState}, + "state", + BeNumerically("<", 0), + ), + Entry("-state", + repositories.AppRecord{State: repositories.StartedState}, + repositories.AppRecord{State: repositories.StoppedState}, + "-state", + BeNumerically(">", 0), + ), +) diff --git a/api/repositories/fake/app_sorter.go b/api/repositories/fake/app_sorter.go new file mode 100644 index 000000000..c71373f92 --- /dev/null +++ b/api/repositories/fake/app_sorter.go @@ -0,0 +1,118 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package fake + +import ( + "sync" + + "code.cloudfoundry.org/korifi/api/repositories" +) + +type AppSorter struct { + SortStub func([]repositories.AppRecord, string) []repositories.AppRecord + sortMutex sync.RWMutex + sortArgsForCall []struct { + arg1 []repositories.AppRecord + arg2 string + } + sortReturns struct { + result1 []repositories.AppRecord + } + sortReturnsOnCall map[int]struct { + result1 []repositories.AppRecord + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *AppSorter) Sort(arg1 []repositories.AppRecord, arg2 string) []repositories.AppRecord { + var arg1Copy []repositories.AppRecord + if arg1 != nil { + arg1Copy = make([]repositories.AppRecord, len(arg1)) + copy(arg1Copy, arg1) + } + fake.sortMutex.Lock() + ret, specificReturn := fake.sortReturnsOnCall[len(fake.sortArgsForCall)] + fake.sortArgsForCall = append(fake.sortArgsForCall, struct { + arg1 []repositories.AppRecord + 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 *AppSorter) SortCallCount() int { + fake.sortMutex.RLock() + defer fake.sortMutex.RUnlock() + return len(fake.sortArgsForCall) +} + +func (fake *AppSorter) SortCalls(stub func([]repositories.AppRecord, string) []repositories.AppRecord) { + fake.sortMutex.Lock() + defer fake.sortMutex.Unlock() + fake.SortStub = stub +} + +func (fake *AppSorter) SortArgsForCall(i int) ([]repositories.AppRecord, string) { + fake.sortMutex.RLock() + defer fake.sortMutex.RUnlock() + argsForCall := fake.sortArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *AppSorter) SortReturns(result1 []repositories.AppRecord) { + fake.sortMutex.Lock() + defer fake.sortMutex.Unlock() + fake.SortStub = nil + fake.sortReturns = struct { + result1 []repositories.AppRecord + }{result1} +} + +func (fake *AppSorter) SortReturnsOnCall(i int, result1 []repositories.AppRecord) { + fake.sortMutex.Lock() + defer fake.sortMutex.Unlock() + fake.SortStub = nil + if fake.sortReturnsOnCall == nil { + fake.sortReturnsOnCall = make(map[int]struct { + result1 []repositories.AppRecord + }) + } + fake.sortReturnsOnCall[i] = struct { + result1 []repositories.AppRecord + }{result1} +} + +func (fake *AppSorter) 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 *AppSorter) 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.AppSorter = new(AppSorter)