diff --git a/coderd/database/dbfake/databasefake.go b/coderd/database/dbfake/databasefake.go index 41800dbcd9..c2e42f5c89 100644 --- a/coderd/database/dbfake/databasefake.go +++ b/coderd/database/dbfake/databasefake.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "encoding/json" + "errors" "fmt" "reflect" "regexp" @@ -1329,6 +1330,63 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database. workspaces = append(workspaces, workspace) } + // Sort workspaces (ORDER BY) + isRunning := func(build database.WorkspaceBuild, job database.ProvisionerJob) bool { + return job.CompletedAt.Valid && !job.CanceledAt.Valid && !job.Error.Valid && build.Transition == database.WorkspaceTransitionStart + } + + preloadedWorkspaceBuilds := map[uuid.UUID]database.WorkspaceBuild{} + preloadedProvisionerJobs := map[uuid.UUID]database.ProvisionerJob{} + preloadedUsers := map[uuid.UUID]database.User{} + + for _, w := range workspaces { + build, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, w.ID) + if err == nil { + preloadedWorkspaceBuilds[w.ID] = build + } else if !errors.Is(err, sql.ErrNoRows) { + return nil, xerrors.Errorf("get latest build: %w", err) + } + + job, err := q.getProvisionerJobByIDNoLock(ctx, build.JobID) + if err == nil { + preloadedProvisionerJobs[w.ID] = job + } else if !errors.Is(err, sql.ErrNoRows) { + return nil, xerrors.Errorf("get provisioner job: %w", err) + } + + user, err := q.getUserByIDNoLock(w.OwnerID) + if err == nil { + preloadedUsers[w.ID] = user + } else if !errors.Is(err, sql.ErrNoRows) { + return nil, xerrors.Errorf("get user: %w", err) + } + } + + sort.Slice(workspaces, func(i, j int) bool { + w1 := workspaces[i] + w2 := workspaces[j] + + // Order by: running first + w1IsRunning := isRunning(preloadedWorkspaceBuilds[w1.ID], preloadedProvisionerJobs[w1.ID]) + w2IsRunning := isRunning(preloadedWorkspaceBuilds[w2.ID], preloadedProvisionerJobs[w2.ID]) + + if w1IsRunning && !w2IsRunning { + return true + } + + if !w1IsRunning && w2IsRunning { + return false + } + + // Order by: usernames + if w1.ID != w2.ID { + return sort.StringsAreSorted([]string{preloadedUsers[w1.ID].Username, preloadedUsers[w2.ID].Username}) + } + + // Order by: workspace names + return sort.StringsAreSorted([]string{w1.Name, w2.Name}) + }) + beforePageCount := len(workspaces) if arg.Offset > 0 { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 9955deb439..de1a7231c3 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -8172,7 +8172,11 @@ const getWorkspaces = `-- name: GetWorkspaces :many SELECT workspaces.id, workspaces.created_at, workspaces.updated_at, workspaces.owner_id, workspaces.organization_id, workspaces.template_id, workspaces.deleted, workspaces.name, workspaces.autostart_schedule, workspaces.ttl, workspaces.last_used_at, COUNT(*) OVER () as count FROM - workspaces + workspaces +JOIN + users +ON + workspaces.owner_id = users.id LEFT JOIN LATERAL ( SELECT workspace_builds.transition, @@ -8333,7 +8337,12 @@ WHERE -- Authorize Filter clause will be injected below in GetAuthorizedWorkspaces -- @authorize_filter ORDER BY - last_used_at DESC + (latest_build.completed_at IS NOT NULL AND + latest_build.canceled_at IS NULL AND + latest_build.error IS NULL AND + latest_build.transition = 'start'::workspace_transition) DESC, + LOWER(users.username) ASC, + LOWER(name) ASC LIMIT CASE WHEN $11 :: integer > 0 THEN diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index 3702bc94e2..fda15b2a0f 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -77,7 +77,11 @@ WHERE SELECT workspaces.*, COUNT(*) OVER () as count FROM - workspaces + workspaces +JOIN + users +ON + workspaces.owner_id = users.id LEFT JOIN LATERAL ( SELECT workspace_builds.transition, @@ -238,7 +242,12 @@ WHERE -- Authorize Filter clause will be injected below in GetAuthorizedWorkspaces -- @authorize_filter ORDER BY - last_used_at DESC + (latest_build.completed_at IS NOT NULL AND + latest_build.canceled_at IS NULL AND + latest_build.error IS NULL AND + latest_build.transition = 'start'::workspace_transition) DESC, + LOWER(users.username) ASC, + LOWER(name) ASC LIMIT CASE WHEN @limit_ :: integer > 0 THEN diff --git a/coderd/workspaces.go b/coderd/workspaces.go index e303fa3150..627c431ae7 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "net/http" - "sort" "strconv" "time" @@ -1015,36 +1014,9 @@ func convertWorkspaces(workspaces []database.Workspace, data workspaceData) ([]c &owner, )) } - - sortWorkspaces(apiWorkspaces) - return apiWorkspaces, nil } -func sortWorkspaces(workspaces []codersdk.Workspace) { - sort.Slice(workspaces, func(i, j int) bool { - iw := workspaces[i] - jw := workspaces[j] - - if iw.LatestBuild.Status == codersdk.WorkspaceStatusRunning && jw.LatestBuild.Status != codersdk.WorkspaceStatusRunning { - return true - } - - if jw.LatestBuild.Status == codersdk.WorkspaceStatusRunning && iw.LatestBuild.Status != codersdk.WorkspaceStatusRunning { - return false - } - - if iw.OwnerID != jw.OwnerID { - return iw.OwnerName < jw.OwnerName - } - - if jw.LastUsedAt.IsZero() && iw.LastUsedAt.IsZero() { - return iw.Name < jw.Name - } - return iw.LastUsedAt.After(jw.LastUsedAt) - }) -} - func convertWorkspace( workspace database.Workspace, workspaceBuild codersdk.WorkspaceBuild, diff --git a/coderd/workspaces_internal_test.go b/coderd/workspaces_internal_test.go index 62a5091539..44c1699309 100644 --- a/coderd/workspaces_internal_test.go +++ b/coderd/workspaces_internal_test.go @@ -4,7 +4,6 @@ import ( "testing" "time" - "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/database" @@ -81,96 +80,3 @@ func Test_calculateDeletingAt(t *testing.T) { }) } } - -func TestSortWorkspaces(t *testing.T) { - // the correct sorting order is: - // 1. first show workspaces that are currently running, - // 2. then sort by user_name, - // 3. then sort by last_used_at (descending), - t.Parallel() - - workspaceFactory := func(t *testing.T, name string, ownerID uuid.UUID, ownerName string, status codersdk.WorkspaceStatus, lastUsedAt time.Time) codersdk.Workspace { - t.Helper() - return codersdk.Workspace{ - ID: uuid.New(), - OwnerID: ownerID, - OwnerName: ownerName, - LatestBuild: codersdk.WorkspaceBuild{ - Status: status, - }, - Name: name, - LastUsedAt: lastUsedAt, - } - } - - userAuuid := uuid.New() - - workspaceRunningUserA := workspaceFactory(t, "running-userA", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now()) - workspaceRunningUserB := workspaceFactory(t, "running-userB", uuid.New(), "userB", codersdk.WorkspaceStatusRunning, time.Now()) - workspacePendingUserC := workspaceFactory(t, "pending-userC", uuid.New(), "userC", codersdk.WorkspaceStatusPending, time.Now()) - workspaceRunningUserA2 := workspaceFactory(t, "running-userA2", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now().Add(time.Minute)) - workspaceRunningUserZ := workspaceFactory(t, "running-userZ", uuid.New(), "userZ", codersdk.WorkspaceStatusRunning, time.Now()) - workspaceRunningUserA3 := workspaceFactory(t, "running-userA3", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now().Add(time.Hour)) - - testCases := []struct { - name string - input []codersdk.Workspace - expectedOrder []string - }{ - { - name: "Running workspaces should be first", - input: []codersdk.Workspace{ - workspaceRunningUserB, - workspacePendingUserC, - workspaceRunningUserA, - }, - expectedOrder: []string{ - "running-userA", - "running-userB", - "pending-userC", - }, - }, - { - name: "then sort by owner name", - input: []codersdk.Workspace{ - workspaceRunningUserZ, - workspaceRunningUserA, - workspaceRunningUserB, - }, - expectedOrder: []string{ - "running-userA", - "running-userB", - "running-userZ", - }, - }, - { - name: "then sort by last used at (recent first)", - input: []codersdk.Workspace{ - workspaceRunningUserA, - workspaceRunningUserA2, - workspaceRunningUserA3, - }, - expectedOrder: []string{ - "running-userA3", - "running-userA2", - "running-userA", - }, - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - workspaces := tc.input - sortWorkspaces(workspaces) - - var resultNames []string - for _, workspace := range workspaces { - resultNames = append(resultNames, workspace.Name) - } - - require.Equal(t, tc.expectedOrder, resultNames, tc.name) - }) - } -} diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index f3005b8eb4..7218792ac8 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -198,6 +198,60 @@ func TestAdminViewAllWorkspaces(t *testing.T) { require.Equal(t, 0, len(memberViewWorkspaces.Workspaces), "member in other org should see 0 workspaces") } +func TestWorkspacesSortOrder(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + firstUser := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, firstUser.OrganizationID, version.ID) + + // c-workspace should be running + workspace1 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) { + ctr.Name = "c-workspace" + }) + coderdtest.AwaitWorkspaceBuildJob(t, client, workspace1.LatestBuild.ID) + + // b-workspace should be stopped + workspace2 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) { + ctr.Name = "b-workspace" + }) + coderdtest.AwaitWorkspaceBuildJob(t, client, workspace2.LatestBuild.ID) + + build2 := coderdtest.CreateWorkspaceBuild(t, client, workspace2, database.WorkspaceTransitionStop) + coderdtest.AwaitWorkspaceBuildJob(t, client, build2.ID) + + // a-workspace should be running + workspace3 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) { + ctr.Name = "a-workspace" + }) + coderdtest.AwaitWorkspaceBuildJob(t, client, workspace3.LatestBuild.ID) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + workspacesResponse, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{}) + require.NoError(t, err, "(first) fetch workspaces") + workspaces := workspacesResponse.Workspaces + + expected := []string{ + workspace3.Name, + workspace1.Name, + workspace2.Name, + } + + var actual []string + for _, w := range workspaces { + actual = append(actual, w.Name) + } + + // the correct sorting order is: + // 1. Running workspaces + // 2. Sort by usernames + // 3. Sort by workspace names + require.Equal(t, expected, actual) +} + func TestPostWorkspacesByOrganization(t *testing.T) { t.Parallel() t.Run("InvalidTemplate", func(t *testing.T) {