fix: sort operations before pagination (#660)

Co-authored-by: Tiago Suzukayama <tiago.suzukayama@wildlifestudios.com>

Author: tsuzukayama

.github/workflows/lint.yaml

@@ -12,7 +12,7 @@ jobs:
       - name: Checkout
         uses: actions/checkout@v2
       - name: Restore cache
-        uses: actions/cache@v2
+        uses: actions/cache@v4
         with:
           path: ~/go/pkg/mod
           key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}

.github/workflows/test.yaml

@@ -12,7 +12,7 @@ jobs:
     - name: Checkout
       uses: actions/checkout@v2
     - name: Restore cache
-      uses: actions/cache@v2
+      uses: actions/cache@v4
       with:
         path: ~/go/pkg/mod
         key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
@@ -32,7 +32,7 @@ jobs:
     - name: Checkout
       uses: actions/checkout@v2
     - name: Restore cache
-      uses: actions/cache@v2
+      uses: actions/cache@v4
       with:
         path: ~/go/pkg/mod
         key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
@@ -59,7 +59,7 @@ jobs:
     - name: Checkout
       uses: actions/checkout@v2
     - name: Restore cache
-      uses: actions/cache@v2
+      uses: actions/cache@v4
       with:
         path: ~/go/pkg/mod
         key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}

internal/adapters/storage/redis/operation/operation_storage_redis.go

@@ -224,7 +224,7 @@ func (r *redisOperationStorage) ListSchedulerActiveOperations(ctx context.Contex
 	return operations, nil
 }
 
-func (r *redisOperationStorage) ListSchedulerFinishedOperations(ctx context.Context, schedulerName string, page, pageSize int64) (operations []*operation.Operation, total int64, err error) {
+func (r *redisOperationStorage) ListSchedulerFinishedOperations(ctx context.Context, schedulerName string, page, pageSize int64, order string) (operations []*operation.Operation, total int64, err error) {
 	if err := r.CleanExpiredOperations(ctx, schedulerName); err != nil {
 		return nil, -1, fmt.Errorf("failed to clean scheduler expired operations: %w", err)
 	}
@@ -234,7 +234,7 @@ func (r *redisOperationStorage) ListSchedulerFinishedOperations(ctx context.Cont
 		return nil, -1, fmt.Errorf("failed to count finished operations: %w", err)
 	}
 
-	operationsIDs, err := r.getFinishedOperationsFromHistory(ctx, schedulerName, page, pageSize)
+	operationsIDs, err := r.getFinishedOperationsFromHistory(ctx, schedulerName, page, pageSize, order)
 	operations = make([]*operation.Operation, len(operationsIDs))
 	executions := make([]redis.Cmder, len(operationsIDs))
 
@@ -275,7 +275,7 @@ func (r *redisOperationStorage) ListSchedulerFinishedOperations(ctx context.Cont
 }
 
 func (r *redisOperationStorage) CleanOperationsHistory(ctx context.Context, schedulerName string) error {
-	operationsIDs, err := r.getFinishedOperationsFromHistory(ctx, schedulerName, 0, -1)
+	operationsIDs, err := r.getFinishedOperationsFromHistory(ctx, schedulerName, 0, -1, "")
 	if err != nil {
 		return err
 	}
@@ -381,13 +381,15 @@ func (r *redisOperationStorage) getAllFinishedOperationIDs(ctx context.Context,
 	return append(operationsIDs, noActionOpIDs...), nil
 }
 
-func (r *redisOperationStorage) getFinishedOperationsFromHistory(ctx context.Context, schedulerName string, page, pageSize int64) (operationsIDs []string, err error) {
+func (r *redisOperationStorage) getFinishedOperationsFromHistory(ctx context.Context, schedulerName string, page, pageSize int64, order string) (operationsIDs []string, err error) {
 	metrics.RunWithMetrics(operationStorageMetricLabel, func() error {
-		operationsIDs, err = r.client.ZRevRangeByScore(ctx, r.buildSchedulerHistoryOperationsKey(schedulerName), &redis.ZRangeBy{
-			Min:    "-inf",
-			Max:    "+inf",
+		operationsIDs, err = r.client.Sort(ctx, r.buildSchedulerHistoryOperationsKey(schedulerName), &redis.Sort{
+			By:     createdAtRedisKey,
 			Offset: page * pageSize,
 			Count:  pageSize,
+			Get:    nil,
+			Order:  order,
+			Alpha:  false,
 		}).Result()
 		return err
 	})

internal/adapters/storage/redis/operation/operation_storage_redis_test.go

@@ -384,7 +384,7 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, page, pageSize)
+			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, page, pageSize, "desc")
 			assert.NoError(t, err)
 			assert.NotEmptyf(t, operationsReturned, "expected at least one operation")
 			assert.Equal(t, expectedOperations, operationsReturned)
@@ -424,7 +424,7 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, page, pageSize)
+			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, page, pageSize, "desc")
 			assert.NoError(t, err)
 			assert.NotEmptyf(t, operationsReturned, "expected at least one operation")
 			assert.Equal(t, expectedOperations, operationsReturned)
@@ -463,7 +463,7 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, page, pageSize)
+			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, page, pageSize, "")
 			assert.NoError(t, err)
 			assert.Empty(t, operationsReturned, "expected result to be empty")
 			assert.Equal(t, int64(4), total)
@@ -476,7 +476,7 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 			definitionProvider, _ := createOperationDefinitionProvider(t)
 			storage := NewRedisOperationStorage(client, clock, operationsTTLMap, definitionProvider)
 
-			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10)
+			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10, "")
 			assert.NoError(t, err)
 			assert.Empty(t, operationsReturned, "expected result to be empty")
 			assert.Equal(t, int64(0), total)
@@ -497,7 +497,7 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			operationsReturned, _, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10)
+			operationsReturned, _, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10, "")
 			assert.NoError(t, err)
 			assert.Empty(t, operationsReturned)
 			ids, _ := client.ZRange(context.Background(), storage.buildSchedulerHistoryOperationsKey(schedulerName), 0, -1).Result()
@@ -557,7 +557,7 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			_, _, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10)
+			_, _, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10, "")
 			assert.ErrorContains(t, err, "failed to build operation from the hash: failed to parse operation status: strconv.Atoi: parsing \"\": invalid syntax")
 		})
 
@@ -568,7 +568,7 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 			definitionProvider, _ := createOperationDefinitionProvider(t)
 			storage := NewRedisOperationStorage(client, clock, operationsTTLMap, definitionProvider)
 			client.Close()
-			_, _, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10)
+			_, _, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10, "")
 			assert.ErrorContains(t, err, "failed to clean scheduler expired operations: failed to list operations for \"test-scheduler\" when trying to clean expired operations")
 		})
 	})
@@ -1113,7 +1113,7 @@ func TestCleanOperationsHistory(t *testing.T) {
 			definitionProvider, _ := createOperationDefinitionProvider(t)
 			storage := NewRedisOperationStorage(client, clock, operationsTTLMap, definitionProvider)
 
-			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10)
+			operationsReturned, total, err := storage.ListSchedulerFinishedOperations(context.Background(), schedulerName, 0, 10, "")
 			assert.NoError(t, err)
 			assert.Empty(t, operationsReturned)
 			assert.Equal(t, total, int64(0))

internal/api/handlers/operations_handler.go

@@ -176,7 +176,7 @@ func (h *OperationsHandler) queryActiveOperations(ctx context.Context, scheduler
 }
 
 func (h *OperationsHandler) queryFinishedOperations(ctx context.Context, schedulerName, sortingOrder string, page, pageSize int64) ([]*operation.Operation, uint32, error) {
-	finishedOperationEntities, total, err := h.operationManager.ListSchedulerFinishedOperations(ctx, schedulerName, page-1, pageSize)
+	finishedOperationEntities, total, err := h.operationManager.ListSchedulerFinishedOperations(ctx, schedulerName, page-1, pageSize, sortingOrder)
 	if err != nil {
 		h.logger.Error("error listing finished operations", zap.Error(err))
 		return nil, uint32(0), status.Error(codes.Unknown, err.Error())

internal/api/handlers/operations_handler_test.go

@@ -196,7 +196,7 @@ func TestListOperations(t *testing.T) {
 		mockCtrl := gomock.NewController(t)
 		operationManager := mock.NewMockOperationManager(mockCtrl)
 
-		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15)).Return(finishedOperations, int64(3), nil)
+		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15), "desc").Return(finishedOperations, int64(3), nil)
 
 		mux := runtime.NewServeMux()
 		err := api.RegisterOperationsServiceHandlerServer(context.Background(), mux, ProvideOperationsHandler(operationManager))
@@ -218,7 +218,7 @@ func TestListOperations(t *testing.T) {
 		mockCtrl := gomock.NewController(t)
 		operationManager := mock.NewMockOperationManager(mockCtrl)
 
-		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15)).Return(finishedOperations, int64(3), nil)
+		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15), "asc").Return(finishedOperations, int64(3), nil)
 
 		mux := runtime.NewServeMux()
 		err := api.RegisterOperationsServiceHandlerServer(context.Background(), mux, ProvideOperationsHandler(operationManager))
@@ -241,7 +241,7 @@ func TestListOperations(t *testing.T) {
 		mockCtrl := gomock.NewController(t)
 		operationManager := mock.NewMockOperationManager(mockCtrl)
 
-		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15)).Return(finishedOperations, int64(3), nil)
+		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15), "desc").Return(finishedOperations, int64(3), nil)
 
 		mux := runtime.NewServeMux()
 		err := api.RegisterOperationsServiceHandlerServer(context.Background(), mux, ProvideOperationsHandler(operationManager))
@@ -358,7 +358,7 @@ func TestListOperations(t *testing.T) {
 		mockCtrl := gomock.NewController(t)
 		operationManager := mock.NewMockOperationManager(mockCtrl)
 
-		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15)).Return(finishedOperations, int64(3), nil)
+		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15), "desc").Return(finishedOperations, int64(3), nil)
 
 		mux := runtime.NewServeMux()
 		err := api.RegisterOperationsServiceHandlerServer(context.Background(), mux, ProvideOperationsHandler(operationManager))
@@ -380,7 +380,7 @@ func TestListOperations(t *testing.T) {
 		mockCtrl := gomock.NewController(t)
 		operationManager := mock.NewMockOperationManager(mockCtrl)
 
-		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(10)).Return(finishedOperations, int64(13), nil)
+		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(10), "desc").Return(finishedOperations, int64(13), nil)
 
 		mux := runtime.NewServeMux()
 		err := api.RegisterOperationsServiceHandlerServer(context.Background(), mux, ProvideOperationsHandler(operationManager))
@@ -485,7 +485,7 @@ func TestListOperations(t *testing.T) {
 		mockCtrl := gomock.NewController(t)
 		operationManager := mock.NewMockOperationManager(mockCtrl)
 
-		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15)).Return(nil, int64(0), errors.NewErrUnexpected("error listing finished operations"))
+		operationManager.EXPECT().ListSchedulerFinishedOperations(gomock.Any(), schedulerName, int64(0), int64(15), "desc").Return(nil, int64(0), errors.NewErrUnexpected("error listing finished operations"))
 
 		mux := runtime.NewServeMux()
 		err := api.RegisterOperationsServiceHandlerServer(context.Background(), mux, ProvideOperationsHandler(operationManager))

internal/core/ports/mock/operation_ports_mock.go

@@ -52,7 +52,7 @@ type OperationManager interface {
 	// ListSchedulerActiveOperations returns a list of operations with active status for the given scheduler.
 	ListSchedulerActiveOperations(ctx context.Context, schedulerName string) ([]*operation.Operation, error)
 	// ListSchedulerFinishedOperations returns a list of operations with finished status for the given scheduler.
-	ListSchedulerFinishedOperations(ctx context.Context, schedulerName string, page, pageSize int64) ([]*operation.Operation, int64, error)
+	ListSchedulerFinishedOperations(ctx context.Context, schedulerName string, page, pageSize int64, order string) ([]*operation.Operation, int64, error)
 	// EnqueueOperationCancellationRequest enqueues a cancellation request for the given operation.
 	EnqueueOperationCancellationRequest(ctx context.Context, schedulerName, operationID string) error
 	// WatchOperationCancellationRequests starts an async process to watch for cancellation requests for the given operation and cancels it.
@@ -92,7 +92,7 @@ type OperationStorage interface {
 	// ListSchedulerActiveOperations list scheduler active operations.
 	ListSchedulerActiveOperations(ctx context.Context, schedulerName string) ([]*operation.Operation, error)
 	// ListSchedulerFinishedOperations list scheduler finished operations.
-	ListSchedulerFinishedOperations(ctx context.Context, schedulerName string, page, pageSize int64) ([]*operation.Operation, int64, error)
+	ListSchedulerFinishedOperations(ctx context.Context, schedulerName string, page, pageSize int64, order string) ([]*operation.Operation, int64, error)
 	// UpdateOperationStatus only updates the operation status for the given
 	// operation ID.
 	UpdateOperationStatus(ctx context.Context, schedulerName, operationID string, status operation.Status) error

internal/core/ports/operation_ports.go

@@ -187,8 +187,8 @@ func (om *OperationManager) ListSchedulerActiveOperations(ctx context.Context, s
 	return ops, nil
 }
 
-func (om *OperationManager) ListSchedulerFinishedOperations(ctx context.Context, schedulerName string, page, pageSize int64) (result []*operation.Operation, total int64, err error) {
-	result, total, err = om.Storage.ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize)
+func (om *OperationManager) ListSchedulerFinishedOperations(ctx context.Context, schedulerName string, page, pageSize int64, order string) (result []*operation.Operation, total int64, err error) {
+	result, total, err = om.Storage.ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize, order)
 	if err != nil {
 		return nil, -1, fmt.Errorf("failed to list finished operations for scheduler %s : %w", schedulerName, err)
 	}

internal/core/services/operations/operation_manager.go

@@ -641,8 +641,8 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 		page := int64(0)
 		pageSize := int64(10)
 
-		operationStorage.EXPECT().ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize).Return(operationsResult, int64(3), nil)
-		operations, total, err := opManager.ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize)
+		operationStorage.EXPECT().ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize, "").Return(operationsResult, int64(3), nil)
+		operations, total, err := opManager.ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize, "")
 		require.NoError(t, err)
 		assert.Equal(t, int64(3), total)
 		require.ElementsMatch(t, operationsResult, operations)
@@ -664,8 +664,8 @@ func TestListSchedulerFinishedOperations(t *testing.T) {
 		page := int64(0)
 		pageSize := int64(10)
 
-		operationStorage.EXPECT().ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize).Return(nil, int64(0), errors.New("some error"))
-		_, _, err := opManager.ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize)
+		operationStorage.EXPECT().ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize, "").Return(nil, int64(0), errors.New("some error"))
+		_, _, err := opManager.ListSchedulerFinishedOperations(ctx, schedulerName, page, pageSize, "")
 		require.ErrorContains(t, err, "failed to list finished operations for scheduler test-scheduler : some error")
 	})
 }