feat: Handle pagination cases where after_id does not exist

Emyrk · Emyrk · commit d1bc9a8027a0 · 2022-06-01T09:52:24.000-05:00
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
diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go
@@ -231,17 +231,19 @@ func (q *fakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams
 		users = tmp
 	}
 
-	if len(params.Status) > 0 {
-		usersFilteredByStatus := make([]database.User, 0, len(users))
-		for i, user := range users {
-			for _, status := range params.Status {
-				if user.Status == status {
-					usersFilteredByStatus = append(usersFilteredByStatus, users[i])
-				}
+	if len(params.Status) == 0 {
+		params.Status = []database.UserStatus{database.UserStatusActive}
+	}
+
+	usersFilteredByStatus := make([]database.User, 0, len(users))
+	for i, user := range users {
+		for _, status := range params.Status {
+			if user.Status == status {
+				usersFilteredByStatus = append(usersFilteredByStatus, users[i])
 			}
 		}
-		users = usersFilteredByStatus
 	}
+	users = usersFilteredByStatus
 
 	if params.OffsetOpt > 0 {
 		if int(params.OffsetOpt) > len(users)-1 {
diff --git a/coderd/templateversions.go b/coderd/templateversions.go
@@ -174,6 +174,23 @@ func (api *API) templateVersionsByTemplate(rw http.ResponseWriter, r *http.Reque
 		return
 	}
 
+	if paginationParams.AfterID != uuid.Nil {
+		// See if the record exists first. If the record does not exist, the pagination
+		// query will not work.
+		_, err := api.Database.GetTemplateVersionByID(r.Context(), paginationParams.AfterID)
+		if err != nil && xerrors.Is(err, sql.ErrNoRows) {
+			httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
+				Message: fmt.Sprintf("record at \"after_id\" (%q) does not exists", paginationParams.AfterID.String()),
+			})
+			return
+		} else if err != nil {
+			httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
+				Message: fmt.Sprintf("get template version at after_id: %s", err),
+			})
+			return
+		}
+	}
+
 	apiVersion := []codersdk.TemplateVersion{}
 	versions, err := api.Database.GetTemplateVersionsByTemplateID(r.Context(), database.GetTemplateVersionsByTemplateIDParams{
 		TemplateID: template.ID,
diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go
@@ -491,9 +491,10 @@ func TestPaginatedTemplateVersions(t *testing.T) {
 		pagination codersdk.Pagination
 	}
 	tests := []struct {
-		name string
-		args args
-		want []codersdk.TemplateVersion
+		name          string
+		args          args
+		want          []codersdk.TemplateVersion
+		expectedError string
 	}{
 		{
 			name: "Single result",
@@ -525,6 +526,11 @@ func TestPaginatedTemplateVersions(t *testing.T) {
 			args: args{ctx: ctx, pagination: codersdk.Pagination{Limit: 2, Offset: 10}},
 			want: []codersdk.TemplateVersion{},
 		},
+		{
+			name:          "After_id does not exist",
+			args:          args{ctx: ctx, pagination: codersdk.Pagination{AfterID: uuid.New()}},
+			expectedError: "does not exist",
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -534,8 +540,13 @@ func TestPaginatedTemplateVersions(t *testing.T) {
 				TemplateID: template.ID,
 				Pagination: tt.args.pagination,
 			})
-			assert.NoError(t, err)
-			assert.Equal(t, tt.want, got)
+			if tt.expectedError != "" {
+				require.Error(t, err)
+				require.ErrorContains(t, err, tt.expectedError)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tt.want, got)
+			}
 		})
 	}
 }
diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -735,6 +735,50 @@ func TestWorkspacesByUser(t *testing.T) {
 	})
 }
 
+// TestSuspendedPagination is when the after_id is a suspended record.
+// The database query should still return the correct page, as the after_id
+// is in a subquery that finds the record regardless of its status.
+// This is mainly to confirm the db fake has the same behavior.
+func TestSuspendedPagination(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	client := coderdtest.New(t, &coderdtest.Options{APIRateLimit: -1})
+	coderdtest.CreateFirstUser(t, client)
+	me, err := client.User(context.Background(), codersdk.Me)
+	require.NoError(t, err)
+	orgID := me.OrganizationIDs[0]
+
+	total := 10
+	users := make([]codersdk.User, 0, total)
+	// Create users
+	for i := 0; i < total; i++ {
+		email := fmt.Sprintf("%d@coder.com", i)
+		username := fmt.Sprintf("user%d", i)
+		user, err := client.CreateUser(context.Background(), codersdk.CreateUserRequest{
+			Email:          email,
+			Username:       username,
+			Password:       "password",
+			OrganizationID: orgID,
+		})
+		require.NoError(t, err)
+		users = append(users, user)
+	}
+	sortUsers(users)
+	deletedUser := users[2]
+	expected := users[3:8]
+	_, err = client.UpdateUserStatus(ctx, deletedUser.ID.String(), codersdk.UserStatusSuspended)
+	require.NoError(t, err, "suspend user")
+
+	page, err := client.Users(ctx, codersdk.UsersRequest{
+		Pagination: codersdk.Pagination{
+			Limit:   len(expected),
+			AfterID: deletedUser.ID,
+		},
+	})
+	require.NoError(t, err)
+	require.Equal(t, expected, page, "expected page")
+}
+
 // TestPaginatedUsers creates a list of users, then tries to paginate through
 // them using different page sizes.
 func TestPaginatedUsers(t *testing.T) {
diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go
@@ -57,6 +57,24 @@ func (api *API) workspaceBuilds(rw http.ResponseWriter, r *http.Request) {
 	if !ok {
 		return
 	}
+
+	if paginationParams.AfterID != uuid.Nil {
+		// See if the record exists first. If the record does not exist, the pagination
+		// query will not work.
+		_, err := api.Database.GetWorkspaceBuildByID(r.Context(), paginationParams.AfterID)
+		if err != nil && xerrors.Is(err, sql.ErrNoRows) {
+			httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{
+				Message: fmt.Sprintf("record at \"after_id\" (%q) does not exist", paginationParams.AfterID.String()),
+			})
+			return
+		} else if err != nil {
+			httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
+				Message: fmt.Sprintf("get workspace build at after_id: %s", err),
+			})
+			return
+		}
+	}
+
 	req := database.GetWorkspaceBuildByWorkspaceIDParams{
 		WorkspaceID: workspace.ID,
 		AfterID:     paginationParams.AfterID,
diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go
@@ -6,6 +6,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/uuid"
+
 	"github.com/stretchr/testify/require"
 
 	"github.com/coder/coder/coderd/coderdtest"
@@ -44,6 +46,30 @@ func TestWorkspaceBuilds(t *testing.T) {
 		require.NoError(t, err)
 	})
 
+	t.Run("PaginateNonExistentRow", func(t *testing.T) {
+		t.Parallel()
+		ctx := context.Background()
+
+		client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})
+		user := coderdtest.CreateFirstUser(t, client)
+		version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
+		template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)
+		coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
+		workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
+		coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)
+
+		_, err := client.WorkspaceBuilds(ctx, codersdk.WorkspaceBuildsRequest{
+			WorkspaceID: workspace.ID,
+			Pagination: codersdk.Pagination{
+				AfterID: uuid.New(),
+			},
+		})
+		var apiError *codersdk.Error
+		require.ErrorAs(t, err, &apiError)
+		require.Equal(t, http.StatusBadRequest, apiError.StatusCode())
+		require.Contains(t, apiError.Message, "does not exist")
+	})
+
 	t.Run("PaginateLimitOffset", func(t *testing.T) {
 		t.Parallel()
 		client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})
