Skip to content

Commit b9983e4

Browse files
EmyrkEmyrk
authored
feat: Handle pagination cases where after_id does not exist (coder#1947)
* feat: Handle pagination cases where after_id does not exist Throw an error to the user in these cases - Templateversions - Workspacebuilds User pagination does not need it as suspended users still have rows in the database
1 parent 419dc6b commit b9983e4

8 files changed

Lines changed: 208 additions & 69 deletions

File tree

‎coderd/database/databasefake/databasefake.go‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -231,17 +231,19 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams
231231
users = tmp
232232
}
233233

234-
if len(params.Status) > 0 {
235-
usersFilteredByStatus := make([]database.User, 0, len(users))
236-
for i, user := range users {
237-
for _, status := range params.Status {
238-
if user.Status == status {
239-
usersFilteredByStatus = append(usersFilteredByStatus, users[i])
240-
}
234+
if len(params.Status) == 0 {
235+
params.Status = []database.UserStatus{database.UserStatusActive}
236+
}
237+
238+
usersFilteredByStatus := make([]database.User, 0, len(users))
239+
for i, user := range users {
240+
for _, status := range params.Status {
241+
if user.Status == status {
242+
usersFilteredByStatus = append(usersFilteredByStatus, users[i])
241243
}
242244
}
243-
users = usersFilteredByStatus
244245
}
246+
users = usersFilteredByStatus
245247

246248
if params.OffsetOpt > 0 {
247249
if int(params.OffsetOpt) > len(users)-1 {

‎coderd/organizations.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
5757
}
5858

5959
var organization database.Organization
60-
err = api.Database.InTx(func(db database.Store) error {
61-
organization, err = api.Database.InsertOrganization(r.Context(), database.InsertOrganizationParams{
60+
err = api.Database.InTx(func(store database.Store) error {
61+
organization, err = store.InsertOrganization(r.Context(), database.InsertOrganizationParams{
6262
ID: uuid.New(),
6363
Name: req.Name,
6464
CreatedAt: database.Now(),
@@ -67,7 +67,7 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
6767
if err != nil {
6868
return xerrors.Errorf("create organization: %w", err)
6969
}
70-
_, err = api.Database.InsertOrganizationMember(r.Context(), database.InsertOrganizationMemberParams{
70+
_, err = store.InsertOrganizationMember(r.Context(), database.InsertOrganizationMemberParams{
7171
OrganizationID: organization.ID,
7272
UserID: apiKey.UserID,
7373
CreatedAt: database.Now(),

‎coderd/templateversions.go‎

Lines changed: 66 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -385,51 +385,77 @@ func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Reque
385385
return
386386
}
387387

388-
apiVersion := []codersdk.TemplateVersion{}
389-
versions, err := api.Database.GetTemplateVersionsByTemplateID(r.Context(), database.GetTemplateVersionsByTemplateIDParams{
390-
TemplateID: template.ID,
391-
AfterID: paginationParams.AfterID,
392-
LimitOpt: int32(paginationParams.Limit),
393-
OffsetOpt: int32(paginationParams.Offset),
394-
})
395-
if errors.Is(err, sql.ErrNoRows) {
396-
httpapi.Write(rw, http.StatusOK, apiVersion)
397-
return
398-
}
399-
if err != nil {
400-
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
401-
Message: fmt.Sprintf("get template version: %s", err),
402-
})
403-
return
404-
}
405-
jobIDs := make([]uuid.UUID, 0, len(versions))
406-
for _, version := range versions {
407-
jobIDs = append(jobIDs, version.JobID)
408-
}
409-
jobs, err := api.Database.GetProvisionerJobsByIDs(r.Context(), jobIDs)
410-
if err != nil {
411-
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
412-
Message: fmt.Sprintf("get jobs: %s", err),
388+
var err error
389+
apiVersions := []codersdk.TemplateVersion{}
390+
err = api.Database.InTx(func(store database.Store) error {
391+
if paginationParams.AfterID != uuid.Nil {
392+
// See if the record exists first. If the record does not exist, the pagination
393+
// query will not work.
394+
_, err := store.GetTemplateVersionByID(r.Context(), paginationParams.AfterID)
395+
if err != nil && xerrors.Is(err, sql.ErrNoRows) {
396+
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
397+
Message: fmt.Sprintf("record at \"after_id\" (%q) does not exists", paginationParams.AfterID.String()),
398+
})
399+
return err
400+
} else if err != nil {
401+
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
402+
Message: fmt.Sprintf("get template version at after_id: %s", err),
403+
})
404+
return err
405+
}
406+
}
407+
408+
versions, err := store.GetTemplateVersionsByTemplateID(r.Context(), database.GetTemplateVersionsByTemplateIDParams{
409+
TemplateID: template.ID,
410+
AfterID: paginationParams.AfterID,
411+
LimitOpt: int32(paginationParams.Limit),
412+
OffsetOpt: int32(paginationParams.Offset),
413413
})
414-
return
415-
}
416-
jobByID := map[string]database.ProvisionerJob{}
417-
for _, job := range jobs {
418-
jobByID[job.ID.String()] = job
419-
}
414+
if errors.Is(err, sql.ErrNoRows) {
415+
httpapi.Write(rw, http.StatusOK, apiVersions)
416+
return err
417+
}
418+
if err != nil {
419+
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
420+
Message: fmt.Sprintf("get template version: %s", err),
421+
})
422+
return err
423+
}
420424

421-
for _, version := range versions {
422-
job, exists := jobByID[version.JobID.String()]
423-
if !exists {
425+
jobIDs := make([]uuid.UUID, 0, len(versions))
426+
for _, version := range versions {
427+
jobIDs = append(jobIDs, version.JobID)
428+
}
429+
jobs, err := store.GetProvisionerJobsByIDs(r.Context(), jobIDs)
430+
if err != nil {
424431
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
425-
Message: fmt.Sprintf("job %q doesn't exist for version %q", version.JobID, version.ID),
432+
Message: fmt.Sprintf("get jobs: %s", err),
426433
})
427-
return
434+
return err
428435
}
429-
apiVersion = append(apiVersion, convertTemplateVersion(version, convertProvisionerJob(job)))
436+
jobByID := map[string]database.ProvisionerJob{}
437+
for _, job := range jobs {
438+
jobByID[job.ID.String()] = job
439+
}
440+
441+
for _, version := range versions {
442+
job, exists := jobByID[version.JobID.String()]
443+
if !exists {
444+
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
445+
Message: fmt.Sprintf("job %q doesn't exist for version %q", version.JobID, version.ID),
446+
})
447+
return err
448+
}
449+
apiVersions = append(apiVersions, convertTemplateVersion(version, convertProvisionerJob(job)))
450+
}
451+
452+
return nil
453+
})
454+
if err != nil {
455+
return
430456
}
431457

432-
httpapi.Write(rw, http.StatusOK, apiVersion)
458+
httpapi.Write(rw, http.StatusOK, apiVersions)
433459
}
434460

435461
func (api *API) templateVersionByName(rw http.ResponseWriter, r *http.Request) {
@@ -582,7 +608,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
582608
}
583609
}
584610

585-
provisionerJob, err = api.Database.InsertProvisionerJob(r.Context(), database.InsertProvisionerJobParams{
611+
provisionerJob, err = db.InsertProvisionerJob(r.Context(), database.InsertProvisionerJobParams{
586612
ID: jobID,
587613
CreatedAt: database.Now(),
588614
UpdatedAt: database.Now(),
@@ -606,7 +632,7 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
606632
}
607633
}
608634

609-
templateVersion, err = api.Database.InsertTemplateVersion(r.Context(), database.InsertTemplateVersionParams{
635+
templateVersion, err = db.InsertTemplateVersion(r.Context(), database.InsertTemplateVersionParams{
610636
ID: uuid.New(),
611637
TemplateID: templateID,
612638
OrganizationID: organization.ID,

‎coderd/templateversions_test.go‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -694,9 +694,10 @@ func TestPaginatedTemplateVersions(t *testing.T) {
694694
pagination codersdk.Pagination
695695
}
696696
tests := []struct {
697-
name string
698-
args args
699-
want []codersdk.TemplateVersion
697+
name string
698+
args args
699+
want []codersdk.TemplateVersion
700+
expectedError string
700701
}{
701702
{
702703
name: "Single result",
@@ -728,6 +729,11 @@ func TestPaginatedTemplateVersions(t *testing.T) {
728729
args: args{ctx: ctx, pagination: codersdk.Pagination{Limit: 2, Offset: 10}},
729730
want: []codersdk.TemplateVersion{},
730731
},
732+
{
733+
name: "After_id does not exist",
734+
args: args{ctx: ctx, pagination: codersdk.Pagination{AfterID: uuid.New()}},
735+
expectedError: "does not exist",
736+
},
731737
}
732738
for _, tt := range tests {
733739
tt := tt
@@ -737,8 +743,13 @@ func TestPaginatedTemplateVersions(t *testing.T) {
737743
TemplateID: template.ID,
738744
Pagination: tt.args.pagination,
739745
})
740-
assert.NoError(t, err)
741-
assert.Equal(t, tt.want, got)
746+
if tt.expectedError != "" {
747+
require.Error(t, err)
748+
require.ErrorContains(t, err, tt.expectedError)
749+
} else {
750+
assert.NoError(t, err)
751+
assert.Equal(t, tt.want, got)
752+
}
742753
})
743754
}
744755
}

‎coderd/users_test.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -830,6 +830,50 @@ func TestWorkspacesByUser(t *testing.T) {
830830
})
831831
}
832832

833+
// TestSuspendedPagination is when the after_id is a suspended record.
834+
// The database query should still return the correct page, as the after_id
835+
// is in a subquery that finds the record regardless of its status.
836+
// This is mainly to confirm the db fake has the same behavior.
837+
func TestSuspendedPagination(t *testing.T) {
838+
t.Parallel()
839+
ctx := context.Background()
840+
client := coderdtest.New(t, &coderdtest.Options{APIRateLimit: -1})
841+
coderdtest.CreateFirstUser(t, client)
842+
me, err := client.User(context.Background(), codersdk.Me)
843+
require.NoError(t, err)
844+
orgID := me.OrganizationIDs[0]
845+
846+
total := 10
847+
users := make([]codersdk.User, 0, total)
848+
// Create users
849+
for i := 0; i < total; i++ {
850+
email := fmt.Sprintf("%d@coder.com", i)
851+
username := fmt.Sprintf("user%d", i)
852+
user, err := client.CreateUser(context.Background(), codersdk.CreateUserRequest{
853+
Email: email,
854+
Username: username,
855+
Password: "password",
856+
OrganizationID: orgID,
857+
})
858+
require.NoError(t, err)
859+
users = append(users, user)
860+
}
861+
sortUsers(users)
862+
deletedUser := users[2]
863+
expected := users[3:8]
864+
_, err = client.UpdateUserStatus(ctx, deletedUser.ID.String(), codersdk.UserStatusSuspended)
865+
require.NoError(t, err, "suspend user")
866+
867+
page, err := client.Users(ctx, codersdk.UsersRequest{
868+
Pagination: codersdk.Pagination{
869+
Limit: len(expected),
870+
AfterID: deletedUser.ID,
871+
},
872+
})
873+
require.NoError(t, err)
874+
require.Equal(t, expected, page, "expected page")
875+
}
876+
833877
// TestPaginatedUsers creates a list of users, then tries to paginate through
834878
// them using different page sizes.
835879
func TestPaginatedUsers(t *testing.T) {

‎coderd/workspacebuilds.go‎

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -51,22 +51,51 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) {
5151
if !ok {
5252
return
5353
}
54-
req := database.GetWorkspaceBuildByWorkspaceIDParams{
55-
WorkspaceID: workspace.ID,
56-
AfterID: paginationParams.AfterID,
57-
OffsetOpt: int32(paginationParams.Offset),
58-
LimitOpt: int32(paginationParams.Limit),
59-
}
60-
builds, err := api.Database.GetWorkspaceBuildByWorkspaceID(r.Context(), req)
61-
if xerrors.Is(err, sql.ErrNoRows) {
62-
err = nil
63-
}
54+
55+
var builds []database.WorkspaceBuild
56+
// Ensure all db calls happen in the same tx
57+
err := api.Database.InTx(func(store database.Store) error {
58+
var err error
59+
if paginationParams.AfterID != uuid.Nil {
60+
// See if the record exists first. If the record does not exist, the pagination
61+
// query will not work.
62+
_, err := store.GetWorkspaceBuildByID(r.Context(), paginationParams.AfterID)
63+
if err != nil && xerrors.Is(err, sql.ErrNoRows) {
64+
httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
65+
Message: fmt.Sprintf("record at \"after_id\" (%q) does not exist", paginationParams.AfterID.String()),
66+
})
67+
return err
68+
} else if err != nil {
69+
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
70+
Message: fmt.Sprintf("get workspace build at after_id: %s", err),
71+
})
72+
return err
73+
}
74+
}
75+
76+
req := database.GetWorkspaceBuildByWorkspaceIDParams{
77+
WorkspaceID: workspace.ID,
78+
AfterID: paginationParams.AfterID,
79+
OffsetOpt: int32(paginationParams.Offset),
80+
LimitOpt: int32(paginationParams.Limit),
81+
}
82+
builds, err = store.GetWorkspaceBuildByWorkspaceID(r.Context(), req)
83+
if xerrors.Is(err, sql.ErrNoRows) {
84+
err = nil
85+
}
86+
if err != nil {
87+
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
88+
Message: fmt.Sprintf("get workspace builds: %s", err),
89+
})
90+
return err
91+
}
92+
93+
return nil
94+
})
6495
if err != nil {
65-
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
66-
Message: fmt.Sprintf("get workspace builds: %s", err),
67-
})
6896
return
6997
}
98+
7099
jobIDs := make([]uuid.UUID, 0, len(builds))
71100
for _, version := range builds {
72101
jobIDs = append(jobIDs, version.JobID)

‎coderd/workspacebuilds_test.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77
"time"
88

9+
"github.com/google/uuid"
910
"github.com/stretchr/testify/require"
1011

1112
"github.com/coder/coder/coderd/coderdtest"
@@ -44,6 +45,30 @@ func TestWorkspaceBuilds(t *testing.T) {
4445
require.NoError(t, err)
4546
})
4647

48+
t.Run("PaginateNonExistentRow", func(t *testing.T) {
49+
t.Parallel()
50+
ctx := context.Background()
51+
52+
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})
53+
user := coderdtest.CreateFirstUser(t, client)
54+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
55+
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
56+
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
57+
workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
58+
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)
59+
60+
_, err := client.WorkspaceBuilds(ctx, codersdk.WorkspaceBuildsRequest{
61+
WorkspaceID: workspace.ID,
62+
Pagination: codersdk.Pagination{
63+
AfterID: uuid.New(),
64+
},
65+
})
66+
var apiError *codersdk.Error
67+
require.ErrorAs(t, err, &apiError)
68+
require.Equal(t, http.StatusBadRequest, apiError.StatusCode())
69+
require.Contains(t, apiError.Message, "does not exist")
70+
})
71+
4772
t.Run("PaginateLimitOffset", func(t *testing.T) {
4873
t.Parallel()
4974
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})

‎docker-compose.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ services:
1818
condition: service_healthy
1919
database:
2020
image: "postgres:14.2"
21+
ports:
22+
- "5432:5432"
2123
environment:
2224
POSTGRES_USER: ${POSTGRES_USER:-username} # The PostgreSQL user (useful to connect to the database)
2325
POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:-password} # The PostgreSQL password (useful to connect to the database)

0 commit comments

Comments
 (0)