Skip to content

Commit 3b86b9d

Browse files
authored
fix(sessions): apply the offset query parameter (#1765)
## Bug `GET /api/v1/sessions` documents `offset` ("Number of sessions to skip for pagination"). It parses the value and echoes it back in the response's `offset` field, but never passes it on: `storage.SessionFilter` has no offset, and `GetRecentSessionsFiltered` always returns the first page. A client paging through sessions gets the newest page repeatedly while the response claims `offset: N`. ## Trigger With three or more stored sessions: ``` GET /api/v1/sessions?limit=1&offset=1 ``` - Expected: the second-newest session, with `offset: 1`. - Actual: the newest session, with `offset: 1`. ## Fix - Add `Offset` to `storage.SessionFilter`. - In `GetRecentSessionsFiltered`, skip that many sessions after the existing sort and filter, so `total` stays the filtered count. - Set `Offset` from the already-parsed query value in `handleGetSessions`. - An offset beyond the end returns an empty page. - Callers that don't set `Offset` (the clients hub, runtime) behave exactly as before. ## Test - `TestGetRecentSessionsFiltered_Offset` (storage): unfiltered `limit=2, offset=1` returns `[s-zed, s-api]`, and a filtered `client=cursor, offset=1` returns `[s-old-cursor]` with total 2. An offset past the end returns an empty page with the total intact. - `TestSessions_OffsetReachesStorage` (httpapi): `?limit=1&offset=2` reaches the controller as `SessionFilter.Offset == 2`. ## Commands run - `CGO_ENABLED=0 go test -count=1 ./internal/storage`: ok - `CGO_ENABLED=0 go test -count=1 ./internal/httpapi`: ok - `CGO_ENABLED=0 go test -count=1 -run Session ./internal/runtime`: ok - Storage behaviour reverted (the field kept so the test compiles): the storage test fails, returning `[s-cursor2 s-zed]` instead of `[s-zed s-api]`. - Handler line reverted: the httpapi test fails with `expected: 2, actual: 0`. - `gofmt -l` on the edited files: no output
1 parent 206cb09 commit 3b86b9d

4 files changed

Lines changed: 49 additions & 4 deletions

File tree

‎internal/httpapi/server.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6900,6 +6900,7 @@ func (s *Server) handleGetSessions(w http.ResponseWriter, r *http.Request) {
69006900
s.writeError(w, r, http.StatusBadRequest, err.Error())
69016901
return
69026902
}
6903+
sessionFilter.Offset = offset
69036904

69046905
// Get recent sessions from controller
69056906
sessions, total, err := s.controller.GetRecentSessions(sessionFilter)

‎internal/httpapi/sessions_scope_test.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,3 +111,15 @@ func TestSessionsScope_RowsCarryAttributionAndFiltersReachStorage(t *testing.T)
111111
assert.Contains(t, w.Body.String(), "client_name is not supported on this endpoint; filter by client")
112112
assert.Zero(t, ctrl.callCount, "the check runs before any storage read")
113113
}
114+
115+
func TestSessions_OffsetReachesStorage(t *testing.T) {
116+
ctrl := &scopedSessionController{mockSessionController{apiKey: "test-key", sessions: scopeTestSessions()}}
117+
srv := NewServer(ctrl, zap.NewNop().Sugar(), nil)
118+
req := httptest.NewRequest(http.MethodGet, "/api/v1/sessions?limit=1&offset=2", nil)
119+
req.Header.Set("X-API-Key", "test-key")
120+
w := httptest.NewRecorder()
121+
srv.ServeHTTP(w, req)
122+
123+
require.Equal(t, http.StatusOK, w.Code)
124+
assert.Equal(t, 2, ctrl.gotFilter.Offset, "offset query param must be passed to the session filter")
125+
}

‎internal/storage/manager.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1532,6 +1532,7 @@ type SessionRecord struct {
15321532
// TokenName are the credential at initialize.
15331533
type SessionFilter struct {
15341534
Limit int
1535+
Offset int // matching sessions to skip, applied after sort and filter
15351536
Status string
15361537
Profile string
15371538
ClientID string
@@ -1747,6 +1748,7 @@ func (m *Manager) GetRecentSessions(limit int, status string) ([]*SessionRecord,
17471748
// never truncated out by a page of newer non-matching ones.
17481749
func (m *Manager) GetRecentSessionsFiltered(f SessionFilter) ([]*SessionRecord, int, error) {
17491750
limit := f.Limit
1751+
offset := max(f.Offset, 0)
17501752
m.mu.RLock()
17511753
defer m.mu.RUnlock()
17521754

@@ -1792,17 +1794,18 @@ func (m *Manager) GetRecentSessionsFiltered(f SessionFilter) ([]*SessionRecord,
17921794
continue
17931795
}
17941796
total++
1795-
if len(sessions) < limit {
1797+
if total > offset && len(sessions) < limit {
17961798
sessions = append(sessions, session)
17971799
}
17981800
}
17991801
return sessions, total, nil
18001802
}
18011803

1802-
if limit < len(all) {
1803-
return all[:limit], len(all), nil
1804+
page := all[min(offset, len(all)):]
1805+
if limit < len(page) {
1806+
page = page[:limit]
18041807
}
1805-
return all, len(all), nil
1808+
return page, len(all), nil
18061809
}
18071810

18081811
// GetSessionByID retrieves a session by its ID
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package storage
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
func TestGetRecentSessionsFiltered_Offset(t *testing.T) {
11+
m, cleanup := setupTestStorageForActivity(t)
12+
defer cleanup()
13+
seedScopeSessions(t, m) // newest first: s-cursor2, s-zed, s-api, s-old-cursor
14+
15+
got, total, err := m.GetRecentSessionsFiltered(SessionFilter{Limit: 2, Offset: 1})
16+
require.NoError(t, err)
17+
assert.Equal(t, 4, total)
18+
assert.Equal(t, []string{"s-zed", "s-api"}, sessionIDs(got))
19+
20+
got, total, err = m.GetRecentSessionsFiltered(SessionFilter{Limit: 10, Offset: 1, ClientID: "cursor"})
21+
require.NoError(t, err)
22+
assert.Equal(t, 2, total, "total stays the filtered count")
23+
assert.Equal(t, []string{"s-old-cursor"}, sessionIDs(got))
24+
25+
got, total, err = m.GetRecentSessionsFiltered(SessionFilter{Limit: 10, Offset: 9})
26+
require.NoError(t, err)
27+
assert.Equal(t, 4, total)
28+
assert.Empty(t, got)
29+
}

0 commit comments

Comments
 (0)