Skip to content

Commit d9299ca

Browse files
rodrimaiamtojek
rodrimaia
and
mtojek
authored
feat: order workspaces by running first (#7656)
* wip * use updated sql * wip * Implement sorting in databasefake.go * More fixes * sql fmt --------- Co-authored-by: Marcin Tojek <marcin@coder.com>
1 parent 96a2e63 commit d9299ca

File tree

6 files changed

+134
-126
lines changed

6 files changed

+134
-126
lines changed

‎coderd/database/dbfake/databasefake.go

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"database/sql"
66
"encoding/json"
7+
"errors"
78
"fmt"
89
"reflect"
910
"regexp"
@@ -1329,6 +1330,63 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
13291330
workspaces = append(workspaces, workspace)
13301331
}
13311332

1333+
// Sort workspaces (ORDER BY)
1334+
isRunning := func(build database.WorkspaceBuild, job database.ProvisionerJob) bool {
1335+
return job.CompletedAt.Valid && !job.CanceledAt.Valid && !job.Error.Valid && build.Transition == database.WorkspaceTransitionStart
1336+
}
1337+
1338+
preloadedWorkspaceBuilds := map[uuid.UUID]database.WorkspaceBuild{}
1339+
preloadedProvisionerJobs := map[uuid.UUID]database.ProvisionerJob{}
1340+
preloadedUsers := map[uuid.UUID]database.User{}
1341+
1342+
for _, w := range workspaces {
1343+
build, err := q.getLatestWorkspaceBuildByWorkspaceIDNoLock(ctx, w.ID)
1344+
if err == nil {
1345+
preloadedWorkspaceBuilds[w.ID] = build
1346+
} else if !errors.Is(err, sql.ErrNoRows) {
1347+
return nil, xerrors.Errorf("get latest build: %w", err)
1348+
}
1349+
1350+
job, err := q.getProvisionerJobByIDNoLock(ctx, build.JobID)
1351+
if err == nil {
1352+
preloadedProvisionerJobs[w.ID] = job
1353+
} else if !errors.Is(err, sql.ErrNoRows) {
1354+
return nil, xerrors.Errorf("get provisioner job: %w", err)
1355+
}
1356+
1357+
user, err := q.getUserByIDNoLock(w.OwnerID)
1358+
if err == nil {
1359+
preloadedUsers[w.ID] = user
1360+
} else if !errors.Is(err, sql.ErrNoRows) {
1361+
return nil, xerrors.Errorf("get user: %w", err)
1362+
}
1363+
}
1364+
1365+
sort.Slice(workspaces, func(i, j int) bool {
1366+
w1 := workspaces[i]
1367+
w2 := workspaces[j]
1368+
1369+
// Order by: running first
1370+
w1IsRunning := isRunning(preloadedWorkspaceBuilds[w1.ID], preloadedProvisionerJobs[w1.ID])
1371+
w2IsRunning := isRunning(preloadedWorkspaceBuilds[w2.ID], preloadedProvisionerJobs[w2.ID])
1372+
1373+
if w1IsRunning && !w2IsRunning {
1374+
return true
1375+
}
1376+
1377+
if !w1IsRunning && w2IsRunning {
1378+
return false
1379+
}
1380+
1381+
// Order by: usernames
1382+
if w1.ID != w2.ID {
1383+
return sort.StringsAreSorted([]string{preloadedUsers[w1.ID].Username, preloadedUsers[w2.ID].Username})
1384+
}
1385+
1386+
// Order by: workspace names
1387+
return sort.StringsAreSorted([]string{w1.Name, w2.Name})
1388+
})
1389+
13321390
beforePageCount := len(workspaces)
13331391

13341392
if arg.Offset > 0 {

‎coderd/database/queries.sql.go

Lines changed: 11 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/workspaces.sql

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,11 @@ WHERE
7777
SELECT
7878
workspaces.*, COUNT(*) OVER () as count
7979
FROM
80-
workspaces
80+
workspaces
81+
JOIN
82+
users
83+
ON
84+
workspaces.owner_id = users.id
8185
LEFT JOIN LATERAL (
8286
SELECT
8387
workspace_builds.transition,
@@ -238,7 +242,12 @@ WHERE
238242
-- Authorize Filter clause will be injected below in GetAuthorizedWorkspaces
239243
-- @authorize_filter
240244
ORDER BY
241-
last_used_at DESC
245+
(latest_build.completed_at IS NOT NULL AND
246+
latest_build.canceled_at IS NULL AND
247+
latest_build.error IS NULL AND
248+
latest_build.transition = 'start'::workspace_transition) DESC,
249+
LOWER(users.username) ASC,
250+
LOWER(name) ASC
242251
LIMIT
243252
CASE
244253
WHEN @limit_ :: integer > 0 THEN

‎coderd/workspaces.go

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"errors"
88
"fmt"
99
"net/http"
10-
"sort"
1110
"strconv"
1211
"time"
1312

@@ -1015,36 +1014,9 @@ func convertWorkspaces(workspaces []database.Workspace, data workspaceData) ([]c
10151014
&owner,
10161015
))
10171016
}
1018-
1019-
sortWorkspaces(apiWorkspaces)
1020-
10211017
return apiWorkspaces, nil
10221018
}
10231019

1024-
func sortWorkspaces(workspaces []codersdk.Workspace) {
1025-
sort.Slice(workspaces, func(i, j int) bool {
1026-
iw := workspaces[i]
1027-
jw := workspaces[j]
1028-
1029-
if iw.LatestBuild.Status == codersdk.WorkspaceStatusRunning && jw.LatestBuild.Status != codersdk.WorkspaceStatusRunning {
1030-
return true
1031-
}
1032-
1033-
if jw.LatestBuild.Status == codersdk.WorkspaceStatusRunning && iw.LatestBuild.Status != codersdk.WorkspaceStatusRunning {
1034-
return false
1035-
}
1036-
1037-
if iw.OwnerID != jw.OwnerID {
1038-
return iw.OwnerName < jw.OwnerName
1039-
}
1040-
1041-
if jw.LastUsedAt.IsZero() && iw.LastUsedAt.IsZero() {
1042-
return iw.Name < jw.Name
1043-
}
1044-
return iw.LastUsedAt.After(jw.LastUsedAt)
1045-
})
1046-
}
1047-
10481020
func convertWorkspace(
10491021
workspace database.Workspace,
10501022
workspaceBuild codersdk.WorkspaceBuild,

‎coderd/workspaces_internal_test.go

Lines changed: 0 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"testing"
55
"time"
66

7-
"github.com/google/uuid"
87
"github.com/stretchr/testify/require"
98

109
"github.com/coder/coder/coderd/database"
@@ -81,96 +80,3 @@ func Test_calculateDeletingAt(t *testing.T) {
8180
})
8281
}
8382
}
84-
85-
func TestSortWorkspaces(t *testing.T) {
86-
// the correct sorting order is:
87-
// 1. first show workspaces that are currently running,
88-
// 2. then sort by user_name,
89-
// 3. then sort by last_used_at (descending),
90-
t.Parallel()
91-
92-
workspaceFactory := func(t *testing.T, name string, ownerID uuid.UUID, ownerName string, status codersdk.WorkspaceStatus, lastUsedAt time.Time) codersdk.Workspace {
93-
t.Helper()
94-
return codersdk.Workspace{
95-
ID: uuid.New(),
96-
OwnerID: ownerID,
97-
OwnerName: ownerName,
98-
LatestBuild: codersdk.WorkspaceBuild{
99-
Status: status,
100-
},
101-
Name: name,
102-
LastUsedAt: lastUsedAt,
103-
}
104-
}
105-
106-
userAuuid := uuid.New()
107-
108-
workspaceRunningUserA := workspaceFactory(t, "running-userA", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now())
109-
workspaceRunningUserB := workspaceFactory(t, "running-userB", uuid.New(), "userB", codersdk.WorkspaceStatusRunning, time.Now())
110-
workspacePendingUserC := workspaceFactory(t, "pending-userC", uuid.New(), "userC", codersdk.WorkspaceStatusPending, time.Now())
111-
workspaceRunningUserA2 := workspaceFactory(t, "running-userA2", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now().Add(time.Minute))
112-
workspaceRunningUserZ := workspaceFactory(t, "running-userZ", uuid.New(), "userZ", codersdk.WorkspaceStatusRunning, time.Now())
113-
workspaceRunningUserA3 := workspaceFactory(t, "running-userA3", userAuuid, "userA", codersdk.WorkspaceStatusRunning, time.Now().Add(time.Hour))
114-
115-
testCases := []struct {
116-
name string
117-
input []codersdk.Workspace
118-
expectedOrder []string
119-
}{
120-
{
121-
name: "Running workspaces should be first",
122-
input: []codersdk.Workspace{
123-
workspaceRunningUserB,
124-
workspacePendingUserC,
125-
workspaceRunningUserA,
126-
},
127-
expectedOrder: []string{
128-
"running-userA",
129-
"running-userB",
130-
"pending-userC",
131-
},
132-
},
133-
{
134-
name: "then sort by owner name",
135-
input: []codersdk.Workspace{
136-
workspaceRunningUserZ,
137-
workspaceRunningUserA,
138-
workspaceRunningUserB,
139-
},
140-
expectedOrder: []string{
141-
"running-userA",
142-
"running-userB",
143-
"running-userZ",
144-
},
145-
},
146-
{
147-
name: "then sort by last used at (recent first)",
148-
input: []codersdk.Workspace{
149-
workspaceRunningUserA,
150-
workspaceRunningUserA2,
151-
workspaceRunningUserA3,
152-
},
153-
expectedOrder: []string{
154-
"running-userA3",
155-
"running-userA2",
156-
"running-userA",
157-
},
158-
},
159-
}
160-
161-
for _, tc := range testCases {
162-
tc := tc
163-
t.Run(tc.name, func(t *testing.T) {
164-
t.Parallel()
165-
workspaces := tc.input
166-
sortWorkspaces(workspaces)
167-
168-
var resultNames []string
169-
for _, workspace := range workspaces {
170-
resultNames = append(resultNames, workspace.Name)
171-
}
172-
173-
require.Equal(t, tc.expectedOrder, resultNames, tc.name)
174-
})
175-
}
176-
}

‎coderd/workspaces_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,60 @@ func TestAdminViewAllWorkspaces(t *testing.T) {
198198
require.Equal(t, 0, len(memberViewWorkspaces.Workspaces), "member in other org should see 0 workspaces")
199199
}
200200

201+
func TestWorkspacesSortOrder(t *testing.T) {
202+
t.Parallel()
203+
204+
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
205+
firstUser := coderdtest.CreateFirstUser(t, client)
206+
version := coderdtest.CreateTemplateVersion(t, client, firstUser.OrganizationID, nil)
207+
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
208+
template := coderdtest.CreateTemplate(t, client, firstUser.OrganizationID, version.ID)
209+
210+
// c-workspace should be running
211+
workspace1 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) {
212+
ctr.Name = "c-workspace"
213+
})
214+
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace1.LatestBuild.ID)
215+
216+
// b-workspace should be stopped
217+
workspace2 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) {
218+
ctr.Name = "b-workspace"
219+
})
220+
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace2.LatestBuild.ID)
221+
222+
build2 := coderdtest.CreateWorkspaceBuild(t, client, workspace2, database.WorkspaceTransitionStop)
223+
coderdtest.AwaitWorkspaceBuildJob(t, client, build2.ID)
224+
225+
// a-workspace should be running
226+
workspace3 := coderdtest.CreateWorkspace(t, client, firstUser.OrganizationID, template.ID, func(ctr *codersdk.CreateWorkspaceRequest) {
227+
ctr.Name = "a-workspace"
228+
})
229+
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace3.LatestBuild.ID)
230+
231+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
232+
defer cancel()
233+
workspacesResponse, err := client.Workspaces(ctx, codersdk.WorkspaceFilter{})
234+
require.NoError(t, err, "(first) fetch workspaces")
235+
workspaces := workspacesResponse.Workspaces
236+
237+
expected := []string{
238+
workspace3.Name,
239+
workspace1.Name,
240+
workspace2.Name,
241+
}
242+
243+
var actual []string
244+
for _, w := range workspaces {
245+
actual = append(actual, w.Name)
246+
}
247+
248+
// the correct sorting order is:
249+
// 1. Running workspaces
250+
// 2. Sort by usernames
251+
// 3. Sort by workspace names
252+
require.Equal(t, expected, actual)
253+
}
254+
201255
func TestPostWorkspacesByOrganization(t *testing.T) {
202256
t.Parallel()
203257
t.Run("InvalidTemplate", func(t *testing.T) {

0 commit comments

Comments
 (0)