Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit7a3d49f

Browse files
Emyrkkylecarbs
authored andcommitted
feat: Handle pagination cases where after_id does not exist (#1947)
* feat: Handle pagination cases where after_id does not existThrow an error to the user in these cases- Templateversions- WorkspacebuildsUser pagination does not need it as suspended users stillhave rows in the database
1 parent0507530 commit7a3d49f

File tree

8 files changed

+208
-69
lines changed

8 files changed

+208
-69
lines changed

‎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-
iflen(params.Status)>0 {
235-
usersFilteredByStatus:=make([]database.User,0,len(users))
236-
fori,user:=rangeusers {
237-
for_,status:=rangeparams.Status {
238-
ifuser.Status==status {
239-
usersFilteredByStatus=append(usersFilteredByStatus,users[i])
240-
}
234+
iflen(params.Status)==0 {
235+
params.Status= []database.UserStatus{database.UserStatusActive}
236+
}
237+
238+
usersFilteredByStatus:=make([]database.User,0,len(users))
239+
fori,user:=rangeusers {
240+
for_,status:=rangeparams.Status {
241+
ifuser.Status==status {
242+
usersFilteredByStatus=append(usersFilteredByStatus,users[i])
241243
}
242244
}
243-
users=usersFilteredByStatus
244245
}
246+
users=usersFilteredByStatus
245247

246248
ifparams.OffsetOpt>0 {
247249
ifint(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
varorganization 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
iferr!=nil {
6868
returnxerrors.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-
iferrors.Is(err,sql.ErrNoRows) {
396-
httpapi.Write(rw,http.StatusOK,apiVersion)
397-
return
398-
}
399-
iferr!=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:=rangeversions {
407-
jobIDs=append(jobIDs,version.JobID)
408-
}
409-
jobs,err:=api.Database.GetProvisionerJobsByIDs(r.Context(),jobIDs)
410-
iferr!=nil {
411-
httpapi.Write(rw,http.StatusInternalServerError, httpapi.Response{
412-
Message:fmt.Sprintf("get jobs: %s",err),
388+
varerrerror
389+
apiVersions:=[]codersdk.TemplateVersion{}
390+
err=api.Database.InTx(func(store database.Store)error {
391+
ifpaginationParams.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+
iferr!=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+
returnerr
400+
}elseiferr!=nil{
401+
httpapi.Write(rw,http.StatusInternalServerError, httpapi.Response{
402+
Message:fmt.Sprintf("get template version at after_id: %s",err),
403+
})
404+
returnerr
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:=rangejobs {
418-
jobByID[job.ID.String()]=job
419-
}
414+
iferrors.Is(err,sql.ErrNoRows) {
415+
httpapi.Write(rw,http.StatusOK,apiVersions)
416+
returnerr
417+
}
418+
iferr!=nil {
419+
httpapi.Write(rw,http.StatusInternalServerError, httpapi.Response{
420+
Message:fmt.Sprintf("get template version: %s",err),
421+
})
422+
returnerr
423+
}
420424

421-
for_,version:=rangeversions {
422-
job,exists:=jobByID[version.JobID.String()]
423-
if!exists {
425+
jobIDs:=make([]uuid.UUID,0,len(versions))
426+
for_,version:=rangeversions {
427+
jobIDs=append(jobIDs,version.JobID)
428+
}
429+
jobs,err:=store.GetProvisionerJobsByIDs(r.Context(),jobIDs)
430+
iferr!=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+
returnerr
428435
}
429-
apiVersion=append(apiVersion,convertTemplateVersion(version,convertProvisionerJob(job)))
436+
jobByID:=map[string]database.ProvisionerJob{}
437+
for_,job:=rangejobs {
438+
jobByID[job.ID.String()]=job
439+
}
440+
441+
for_,version:=rangeversions {
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+
returnerr
448+
}
449+
apiVersions=append(apiVersions,convertTemplateVersion(version,convertProvisionerJob(job)))
450+
}
451+
452+
returnnil
453+
})
454+
iferr!=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-
namestring
698-
argsargs
699-
want []codersdk.TemplateVersion
697+
namestring
698+
argsargs
699+
want []codersdk.TemplateVersion
700+
expectedErrorstring
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:=rangetests {
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+
iftt.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+
funcTestSuspendedPagination(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+
fori:=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
funcTestPaginatedUsers(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-
ifxerrors.Is(err,sql.ErrNoRows) {
62-
err=nil
63-
}
54+
55+
varbuilds []database.WorkspaceBuild
56+
// Ensure all db calls happen in the same tx
57+
err:=api.Database.InTx(func(store database.Store)error {
58+
varerrerror
59+
ifpaginationParams.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+
iferr!=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+
returnerr
68+
}elseiferr!=nil {
69+
httpapi.Write(rw,http.StatusInternalServerError, httpapi.Response{
70+
Message:fmt.Sprintf("get workspace build at after_id: %s",err),
71+
})
72+
returnerr
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+
ifxerrors.Is(err,sql.ErrNoRows) {
84+
err=nil
85+
}
86+
iferr!=nil {
87+
httpapi.Write(rw,http.StatusInternalServerError, httpapi.Response{
88+
Message:fmt.Sprintf("get workspace builds: %s",err),
89+
})
90+
returnerr
91+
}
92+
93+
returnnil
94+
})
6495
iferr!=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:=rangebuilds {
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+
varapiError*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)
[8]ページ先頭
©2009-2025 Movatter.jp