Skip to content

Commit

Permalink
Fix ListEvaluationHistory RPC faulty navigation to next page.
Browse files Browse the repository at this point in the history
The SQL statement underlying ListEvaluationHistory RPC has fixed
sorting. This does not create problems when navigating in the same
direction, but creates problem when changing direction from pages
further away from the first.

This change solves the issue by determining the sort direction
dynamically and inverting the records at the service layer
accordingly.

Fixes #3966
  • Loading branch information
blkt committed Jul 23, 2024
1 parent 9498645 commit f16ae49
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 2 deletions.
4 changes: 3 additions & 1 deletion database/query/eval_history.sql
Original file line number Diff line number Diff line change
Expand Up @@ -162,5 +162,7 @@ SELECT s.id::uuid AS evaluation_id,
OR s.evaluation_time BETWEEN sqlc.narg(fromts) AND sqlc.narg(tots))
-- implicit filter by project id
AND j.id = sqlc.arg(projectId)
ORDER BY s.evaluation_time DESC
ORDER BY
CASE WHEN sqlc.narg(next)::timestamp without time zone IS NULL THEN s.evaluation_time END ASC,
CASE WHEN sqlc.narg(prev)::timestamp without time zone IS NULL THEN s.evaluation_time END DESC
LIMIT sqlc.arg(size)::integer;
4 changes: 3 additions & 1 deletion internal/db/eval_history.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions internal/history/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"database/sql"
"errors"
"fmt"
"slices"

"github.com/google/uuid"

Expand Down Expand Up @@ -203,6 +204,10 @@ func (_ *evaluationHistoryService) ListEvaluationHistory(
return nil, errors.New("internal error")
}

if cursor != nil && cursor.Direction == Next {
slices.Reverse(rows)
}

result := &ListEvaluationHistoryResult{
Data: rows,
}
Expand Down
85 changes: 85 additions & 0 deletions internal/history/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,90 @@ func TestListEvaluationHistory(t *testing.T) {
},
},

// cursor flips on direction
{
name: "prev does not flip",
dbSetup: dbf.NewDBMock(
withListEvaluationHistory(nil, nil,
makeHistoryRow(
uuid1,
evaluatedAt1,
entityType,
remediation,
alert,
),
makeHistoryRow(
uuid2,
evaluatedAt2,
entityType,
remediation,
alert,
),
),
),
cursor: &ListEvaluationCursor{
Direction: Prev,
},
checkf: func(t *testing.T, rows *ListEvaluationHistoryResult) {
t.Helper()

require.NotNil(t, rows)
require.Len(t, rows.Data, 2)

// database order is maintained
item1 := rows.Data[0]
require.Equal(t, uuid1, item1.EvaluationID)
require.Equal(t, evaluatedAt1, item1.EvaluatedAt)
require.Equal(t, uuid1, item1.EntityID)

item2 := rows.Data[1]
require.Equal(t, uuid2, item2.EvaluationID)
require.Equal(t, evaluatedAt2, item2.EvaluatedAt)
require.Equal(t, uuid2, item2.EntityID)
},
},
{
name: "next does flip",
dbSetup: dbf.NewDBMock(
withListEvaluationHistory(nil, nil,
makeHistoryRow(
uuid2,
evaluatedAt2,
entityType,
remediation,
alert,
),
makeHistoryRow(
uuid1,
evaluatedAt1,
entityType,
remediation,
alert,
),
),
),
cursor: &ListEvaluationCursor{
Direction: Next,
},
checkf: func(t *testing.T, rows *ListEvaluationHistoryResult) {
t.Helper()

require.NotNil(t, rows)
require.Len(t, rows.Data, 2)

// database order is maintained
item1 := rows.Data[0]
require.Equal(t, uuid1, item1.EvaluationID)
require.Equal(t, evaluatedAt1, item1.EvaluatedAt)
require.Equal(t, uuid1, item1.EntityID)

item2 := rows.Data[1]
require.Equal(t, uuid2, item2.EvaluationID)
require.Equal(t, evaluatedAt2, item2.EvaluatedAt)
require.Equal(t, uuid2, item2.EntityID)
},
},

// cursor
{
name: "cursor next",
Expand Down Expand Up @@ -661,6 +745,7 @@ func TestListEvaluationHistory(t *testing.T) {
}
}

//nolint:unparam
func makeHistoryRow(
id uuid.UUID,
evaluatedAt time.Time,
Expand Down

0 comments on commit f16ae49

Please sign in to comment.