Skip to content

Commit cd6a193

Browse files
heiytorotavio
heiytor
authored and
otavio
committed
refactor!: replace direct query params with structured QueryOptions
- Replace individual paginator/filter/sorter parameters with structured QueryOptions in service methods - Introduce QueryOptions interface with fluent methods (InNamespace, Match, Sort, Paginate, WithDeviceStatus) - Update sessions and public keys service layer to accept *requests.ListSessions, *requests.ListPublicKeys instead of query.Paginator - Refactor MongoDB store to use QueryOptions pattern for building aggregation pipelines - Replace AggregateCount with CountAllMatchingDocuments that filters out pagination stages from count queries - Move filter parsing logic from queries package to internal package - Update all tests to use new QueryOptions API BREAKING CHANGE: Service method signatures changed from accepting individual query parameters to structured request objects. All store methods now use QueryOptions instead of direct parameters.
1 parent 539a8b3 commit cd6a193

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

46 files changed

+1175
-871
lines changed

‎api/routes/session.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"strconv"
66

77
"github.com/shellhub-io/shellhub/api/pkg/gateway"
8-
"github.com/shellhub-io/shellhub/pkg/api/query"
98
"github.com/shellhub-io/shellhub/pkg/api/requests"
109
"github.com/shellhub-io/shellhub/pkg/models"
1110
"github.com/shellhub-io/shellhub/pkg/websocket"
@@ -27,15 +26,20 @@ const (
2726
)
2827

2928
func (h *Handler) GetSessionList(c gateway.Context) error {
30-
paginator := query.NewPaginator()
31-
if err := c.Bind(paginator); err != nil {
29+
req := new(requests.ListSessions)
30+
31+
if err := c.Bind(req); err != nil {
32+
return err
33+
}
34+
35+
if err := c.Validate(req); err != nil {
3236
return err
3337
}
3438

3539
// TODO: normalize is not required when request is privileged
36-
paginator.Normalize()
40+
req.Paginator.Normalize()
3741

38-
sessions, count, err := h.service.ListSessions(c.Ctx(), *paginator)
42+
sessions, count, err := h.service.ListSessions(c.Ctx(), req)
3943
if err != nil {
4044
return err
4145
}

‎api/routes/session_test.go‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -35,17 +35,19 @@ func TestGetSessionList(t *testing.T) {
3535
cases := []struct {
3636
description string
3737
paginator query.Paginator
38-
requiredMocks func(paginator query.Paginator)
38+
headers map[string]string
39+
requiredMocks func()
3940
expected Expected
4041
}{
4142
{
4243
description: "fails when try to searching a session list of a existing session",
43-
paginator: query.Paginator{
44-
Page: 1,
45-
PerPage: 10,
46-
},
47-
requiredMocks: func(paginator query.Paginator) {
48-
mock.On("ListSessions", gomock.Anything, paginator).Return(nil, 0, svc.ErrNotFound).Once()
44+
paginator: query.Paginator{Page: 1, PerPage: 10},
45+
headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"},
46+
requiredMocks: func() {
47+
mock.
48+
On("ListSessions", gomock.Anything, &requests.ListSessions{Paginator: query.Paginator{Page: 1, PerPage: 10}, TenantID: "00000000-0000-4000-0000-000000000000"}).
49+
Return(nil, 0, svc.ErrNotFound).
50+
Once()
4951
},
5052
expected: Expected{
5153
expectedSession: nil,
@@ -54,13 +56,13 @@ func TestGetSessionList(t *testing.T) {
5456
},
5557
{
5658
description: "success when try to searching a session list of a existing session",
57-
paginator: query.Paginator{
58-
Page: 1,
59-
PerPage: 10,
60-
},
61-
requiredMocks: func(paginator query.Paginator) {
62-
ss := []models.Session{}
63-
mock.On("ListSessions", gomock.Anything, paginator).Return(ss, 1, nil).Once()
59+
paginator: query.Paginator{Page: 1, PerPage: 10},
60+
headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"},
61+
requiredMocks: func() {
62+
mock.
63+
On("ListSessions", gomock.Anything, &requests.ListSessions{Paginator: query.Paginator{Page: 1, PerPage: 10}, TenantID: "00000000-0000-4000-0000-000000000000"}).
64+
Return([]models.Session{}, 1, nil).
65+
Once()
6466
},
6567
expected: Expected{
6668
expectedSession: []models.Session{},
@@ -71,7 +73,7 @@ func TestGetSessionList(t *testing.T) {
7173

7274
for _, tc := range cases {
7375
t.Run(tc.description, func(t *testing.T) {
74-
tc.requiredMocks(tc.paginator)
76+
tc.requiredMocks()
7577

7678
jsonData, err := json.Marshal(tc.paginator)
7779
if err != nil {
@@ -81,6 +83,10 @@ func TestGetSessionList(t *testing.T) {
8183
req := httptest.NewRequest(http.MethodGet, "/api/sessions", strings.NewReader(string(jsonData)))
8284
req.Header.Set("Content-Type", "application/json")
8385
req.Header.Set("X-Role", authorizer.RoleOwner.String())
86+
for k, v := range tc.headers {
87+
req.Header.Set(k, v)
88+
}
89+
8490
rec := httptest.NewRecorder()
8591

8692
e := NewRouter(mock)

‎api/routes/sshkeys.go‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77

88
"github.com/shellhub-io/shellhub/api/pkg/gateway"
99
"github.com/shellhub-io/shellhub/api/store"
10-
"github.com/shellhub-io/shellhub/pkg/api/query"
1110
"github.com/shellhub-io/shellhub/pkg/api/requests"
1211
"github.com/shellhub-io/shellhub/pkg/models"
1312
)
@@ -30,15 +29,20 @@ const (
3029
)
3130

3231
func (h *Handler) GetPublicKeys(c gateway.Context) error {
33-
paginator := query.NewPaginator()
34-
if err := c.Bind(paginator); err != nil {
32+
req := new(requests.ListPublicKeys)
33+
34+
if err := c.Bind(req); err != nil {
35+
return err
36+
}
37+
38+
if err := c.Validate(req); err != nil {
3539
return err
3640
}
3741

3842
// TODO: normalize is not required when request is privileged
39-
paginator.Normalize()
43+
req.Paginator.Normalize()
4044

41-
list, count, err := h.service.ListPublicKeys(c.Ctx(), *paginator)
45+
list, count, err := h.service.ListPublicKeys(c.Ctx(), req)
4246
if err != nil {
4347
return err
4448
}

‎api/routes/sshkeys_test.go‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,17 +29,19 @@ func TestGetPublicKeys(t *testing.T) {
2929
cases := []struct {
3030
description string
3131
paginator query.Paginator
32-
requiredMocks func(query query.Paginator)
32+
headers map[string]string
33+
requiredMocks func()
3334
expected Expected
3435
}{
3536
{
3637
description: "success when try to list a publics keys exists",
37-
paginator: query.Paginator{
38-
Page: 1,
39-
PerPage: 10,
40-
},
41-
requiredMocks: func(query query.Paginator) {
42-
mock.On("ListPublicKeys", gomock.Anything, query).Return([]models.PublicKey{}, 1, nil)
38+
paginator: query.Paginator{Page: 1, PerPage: 10},
39+
headers: map[string]string{"X-Tenant-ID": "00000000-0000-4000-0000-000000000000"},
40+
requiredMocks: func() {
41+
mock.
42+
On("ListPublicKeys", gomock.Anything, &requests.ListPublicKeys{Paginator: query.Paginator{Page: 1, PerPage: 10}, TenantID: "00000000-0000-4000-0000-000000000000"}).
43+
Return([]models.PublicKey{}, 1, nil).
44+
Once()
4345
},
4446
expected: Expected{
4547
expectedSession: []models.PublicKey{},
@@ -50,7 +52,7 @@ func TestGetPublicKeys(t *testing.T) {
5052

5153
for _, tc := range cases {
5254
t.Run(tc.description, func(t *testing.T) {
53-
tc.requiredMocks(tc.paginator)
55+
tc.requiredMocks()
5456

5557
jsonData, err := json.Marshal(tc.paginator)
5658
if err != nil {
@@ -60,6 +62,10 @@ func TestGetPublicKeys(t *testing.T) {
6062
req := httptest.NewRequest(http.MethodGet, "/api/sshkeys/public-keys", strings.NewReader(string(jsonData)))
6163
req.Header.Set("Content-Type", "application/json")
6264
req.Header.Set("X-Role", authorizer.RoleOwner.String())
65+
for k, v := range tc.headers {
66+
req.Header.Set(k, v)
67+
}
68+
6369
rec := httptest.NewRecorder()
6470

6571
e := NewRouter(mock)

‎api/services/api-key.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,12 @@ func (s *service) CreateAPIKey(ctx context.Context, req *requests.CreateAPIKey)
9494
}
9595

9696
func (s *service) ListAPIKeys(ctx context.Context, req *requests.ListAPIKey) ([]models.APIKey, int, error) {
97-
return s.store.APIKeyList(ctx, req.TenantID, req.Paginator, req.Sorter)
97+
return s.store.APIKeyList(
98+
ctx,
99+
s.store.Options().InNamespace(req.TenantID),
100+
s.store.Options().Sort(&req.Sorter),
101+
s.store.Options().Paginate(&req.Paginator),
102+
)
98103
}
99104

100105
func (s *service) UpdateAPIKey(ctx context.Context, req *requests.UpdateAPIKey) error {

‎api/services/api-key_test.go‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/shellhub-io/shellhub/pkg/models"
1919
"github.com/shellhub-io/shellhub/pkg/uuid"
2020
uuidmock "github.com/shellhub-io/shellhub/pkg/uuid/mocks"
21+
"github.com/stretchr/testify/mock"
2122
"github.com/stretchr/testify/require"
2223
)
2324

@@ -404,6 +405,8 @@ func TestListAPIKey(t *testing.T) {
404405
}
405406

406407
storeMock := new(storemock.Store)
408+
queryOptionsMock := new(storemock.QueryOptions)
409+
storeMock.On("Options").Return(queryOptionsMock)
407410

408411
cases := []struct {
409412
description string
@@ -420,8 +423,20 @@ func TestListAPIKey(t *testing.T) {
420423
Sorter: query.Sorter{By: "expires_in", Order: query.OrderAsc},
421424
},
422425
requiredMocks: func(ctx context.Context) {
426+
queryOptionsMock.
427+
On("InNamespace", "00000000-0000-4000-0000-000000000000").
428+
Return(nil).
429+
Once()
430+
queryOptionsMock.
431+
On("Sort", &query.Sorter{By: "expires_in", Order: query.OrderAsc}).
432+
Return(nil).
433+
Once()
434+
queryOptionsMock.
435+
On("Paginate", &query.Paginator{Page: 1, PerPage: 10}).
436+
Return(nil).
437+
Once()
423438
storeMock.
424-
On("APIKeyList", ctx, "00000000-0000-4000-0000-000000000000", query.Paginator{Page: 1, PerPage: 10}, query.Sorter{By: "expires_in", Order: query.OrderAsc}).
439+
On("APIKeyList", ctx, mock.AnythingOfType("store.QueryOption"), mock.AnythingOfType("store.QueryOption"), mock.AnythingOfType("store.QueryOption")).
425440
Return(nil, 0, errors.New("error")).
426441
Once()
427442
},
@@ -439,8 +454,20 @@ func TestListAPIKey(t *testing.T) {
439454
Sorter: query.Sorter{By: "expires_in", Order: query.OrderAsc},
440455
},
441456
requiredMocks: func(ctx context.Context) {
457+
queryOptionsMock.
458+
On("InNamespace", "00000000-0000-4000-0000-000000000000").
459+
Return(nil).
460+
Once()
461+
queryOptionsMock.
462+
On("Sort", &query.Sorter{By: "expires_in", Order: query.OrderAsc}).
463+
Return(nil).
464+
Once()
465+
queryOptionsMock.
466+
On("Paginate", &query.Paginator{Page: 1, PerPage: 10}).
467+
Return(nil).
468+
Once()
442469
storeMock.
443-
On("APIKeyList", ctx, "00000000-0000-4000-0000-000000000000", query.Paginator{Page: 1, PerPage: 10}, query.Sorter{By: "expires_in", Order: query.OrderAsc}).
470+
On("APIKeyList", ctx, mock.AnythingOfType("store.QueryOption"), mock.AnythingOfType("store.QueryOption"), mock.AnythingOfType("store.QueryOption")).
444471
Return(
445472
[]models.APIKey{
446473
{

‎api/services/device.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,20 @@ type DeviceService interface {
5757
}
5858

5959
func (s *service) ListDevices(ctx context.Context, req *requests.DeviceList) ([]models.Device, int, error) {
60+
opts := []store.QueryOption{}
61+
62+
if req.DeviceStatus != "" {
63+
opts = append(opts, s.store.Options().WithDeviceStatus(req.DeviceStatus))
64+
}
65+
66+
if req.TenantID != "" {
67+
opts = append(opts, s.store.Options().InNamespace(req.TenantID))
68+
}
69+
70+
opts = append(opts, s.store.Options().Match(&req.Filters), s.store.Options().Sort(&req.Sorter), s.store.Options().Paginate(&req.Paginator))
71+
6072
if req.DeviceStatus == models.DeviceStatusRemoved {
61-
return s.store.DeviceList(ctx, req.DeviceStatus, req.Paginator, req.Filters, req.Sorter, store.DeviceAcceptableFromRemoved)
73+
return s.store.DeviceList(ctx, store.DeviceAcceptableFromRemoved, opts...)
6274
}
6375

6476
if req.TenantID != "" {
@@ -71,19 +83,19 @@ func (s *service) ListDevices(ctx context.Context, req *requests.DeviceList) ([]
7183
switch {
7284
case envs.IsCloud():
7385
if ns.HasLimitDevicesReached() {
74-
return s.store.DeviceList(ctx, req.DeviceStatus, req.Paginator, req.Filters, req.Sorter, store.DeviceAcceptableFromRemoved)
86+
return s.store.DeviceList(ctx, store.DeviceAcceptableFromRemoved, opts...)
7587
}
7688
case envs.IsEnterprise():
7789
fallthrough
7890
case envs.IsCommunity():
7991
if ns.HasMaxDevicesReached() {
80-
return s.store.DeviceList(ctx, req.DeviceStatus, req.Paginator, req.Filters, req.Sorter, store.DeviceAcceptableAsFalse)
92+
return s.store.DeviceList(ctx, store.DeviceAcceptableAsFalse, opts...)
8193
}
8294
}
8395
}
8496
}
8597

86-
return s.store.DeviceList(ctx, req.DeviceStatus, req.Paginator, req.Filters, req.Sorter, store.DeviceAcceptableIfNotAccepted)
98+
return s.store.DeviceList(ctx, store.DeviceAcceptableIfNotAccepted, opts...)
8799
}
88100

89101
func (s *service) GetDevice(ctx context.Context, uid models.UID) (*models.Device, error) {

0 commit comments

Comments
 (0)