Push app sorting logic to repo layer

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")

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)

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,

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,
 	}
 }
 

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",
 			}))
 		})

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,19 +48,65 @@ 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(
 	namespaceRetriever NamespaceRetriever,
 	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) {

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),
+	),
+)