Skip to content

Commit fa8f50a

Browse files
EmyrkEmyrk
authored
fix: fix workspace status filter returning more statuses that requested (#7732)
1 parent b9e3226 commit fa8f50a

File tree

6 files changed

+347
-84
lines changed

6 files changed

+347
-84
lines changed

‎coderd/database/dbauthz/system_test.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package dbauthz_test
33
import (
44
"context"
55
"database/sql"
6+
"encoding/json"
67
"time"
78

89
"github.com/google/uuid"
@@ -240,7 +241,7 @@ func (s *MethodTestSuite) TestSystemFunctions() {
240241
j := dbgen.ProvisionerJob(s.T(), db, database.ProvisionerJob{
241242
StartedAt: sql.NullTime{Valid: false},
242243
})
243-
check.Args(database.AcquireProvisionerJobParams{Types: []database.ProvisionerType{j.Provisioner}}).
244+
check.Args(database.AcquireProvisionerJobParams{Types: []database.ProvisionerType{j.Provisioner}, Tags: must(json.Marshal(j.Tags))}).
244245
Asserts( /*rbac.ResourceSystem, rbac.ActionUpdate*/ )
245246
}))
246247
s.Run("UpdateProvisionerJobWithCompleteByID", s.Subtest(func(db database.Store, check *expects) {

‎coderd/database/dbfake/databasefake.go

+50-54
Original file line numberDiff line numberDiff line change
@@ -1168,82 +1168,68 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
11681168
return nil, xerrors.Errorf("get provisioner job: %w", err)
11691169
}
11701170

1171+
// This logic should match the logic in the workspace.sql file.
1172+
var statusMatch bool
11711173
switch database.WorkspaceStatus(arg.Status) {
11721174
case database.WorkspaceStatusPending:
1173-
if !job.StartedAt.Valid {
1174-
continue
1175-
}
1176-
1175+
statusMatch = isNull(job.StartedAt)
11771176
case database.WorkspaceStatusStarting:
1178-
if !job.StartedAt.Valid &&
1179-
!job.CanceledAt.Valid &&
1180-
job.CompletedAt.Valid &&
1181-
time.Since(job.UpdatedAt) > 30*time.Second ||
1182-
build.Transition != database.WorkspaceTransitionStart {
1183-
continue
1184-
}
1177+
statusMatch = isNotNull(job.StartedAt) &&
1178+
isNull(job.CanceledAt) &&
1179+
isNull(job.CompletedAt) &&
1180+
time.Since(job.UpdatedAt) < 30*time.Second &&
1181+
build.Transition == database.WorkspaceTransitionStart
11851182

11861183
case database.WorkspaceStatusRunning:
1187-
if !job.CompletedAt.Valid &&
1188-
job.CanceledAt.Valid &&
1189-
job.Error.Valid ||
1190-
build.Transition != database.WorkspaceTransitionStart {
1191-
continue
1192-
}
1184+
statusMatch = isNotNull(job.CompletedAt) &&
1185+
isNull(job.CanceledAt) &&
1186+
isNull(job.Error) &&
1187+
build.Transition == database.WorkspaceTransitionStart
11931188

11941189
case database.WorkspaceStatusStopping:
1195-
if !job.StartedAt.Valid &&
1196-
!job.CanceledAt.Valid &&
1197-
job.CompletedAt.Valid &&
1198-
time.Since(job.UpdatedAt) > 30*time.Second ||
1199-
build.Transition != database.WorkspaceTransitionStop {
1200-
continue
1201-
}
1190+
statusMatch = isNotNull(job.StartedAt) &&
1191+
isNull(job.CanceledAt) &&
1192+
isNull(job.CompletedAt) &&
1193+
time.Since(job.UpdatedAt) < 30*time.Second &&
1194+
build.Transition == database.WorkspaceTransitionStop
12021195

12031196
case database.WorkspaceStatusStopped:
1204-
if !job.CompletedAt.Valid &&
1205-
job.CanceledAt.Valid &&
1206-
job.Error.Valid ||
1207-
build.Transition != database.WorkspaceTransitionStop {
1208-
continue
1209-
}
1210-
1197+
statusMatch = isNotNull(job.CompletedAt) &&
1198+
isNull(job.CanceledAt) &&
1199+
isNull(job.Error) &&
1200+
build.Transition == database.WorkspaceTransitionStop
12111201
case database.WorkspaceStatusFailed:
1212-
if (!job.CanceledAt.Valid && !job.Error.Valid) ||
1213-
(!job.CompletedAt.Valid && !job.Error.Valid) {
1214-
continue
1215-
}
1202+
statusMatch = (isNotNull(job.CanceledAt) && isNotNull(job.Error)) ||
1203+
(isNotNull(job.CompletedAt) && isNotNull(job.Error))
12161204

12171205
case database.WorkspaceStatusCanceling:
1218-
if !job.CanceledAt.Valid && job.CompletedAt.Valid {
1219-
continue
1220-
}
1206+
statusMatch = isNotNull(job.CanceledAt) &&
1207+
isNull(job.CompletedAt)
12211208

12221209
case database.WorkspaceStatusCanceled:
1223-
if !job.CanceledAt.Valid && !job.CompletedAt.Valid {
1224-
continue
1225-
}
1210+
statusMatch = isNotNull(job.CanceledAt) &&
1211+
isNotNull(job.CompletedAt)
12261212

12271213
case database.WorkspaceStatusDeleted:
1228-
if !job.StartedAt.Valid &&
1229-
job.CanceledAt.Valid &&
1230-
!job.CompletedAt.Valid &&
1231-
time.Since(job.UpdatedAt) > 30*time.Second ||
1232-
build.Transition != database.WorkspaceTransitionDelete {
1233-
continue
1234-
}
1214+
statusMatch = isNotNull(job.StartedAt) &&
1215+
isNull(job.CanceledAt) &&
1216+
isNotNull(job.CompletedAt) &&
1217+
time.Since(job.UpdatedAt) < 30*time.Second &&
1218+
build.Transition == database.WorkspaceTransitionDelete &&
1219+
isNull(job.Error)
12351220

12361221
case database.WorkspaceStatusDeleting:
1237-
if !job.CompletedAt.Valid &&
1238-
job.CanceledAt.Valid &&
1239-
job.Error.Valid &&
1240-
build.Transition != database.WorkspaceTransitionDelete {
1241-
continue
1242-
}
1222+
statusMatch = isNull(job.CompletedAt) &&
1223+
isNull(job.CanceledAt) &&
1224+
isNull(job.Error) &&
1225+
build.Transition == database.WorkspaceTransitionDelete
12431226

12441227
default:
12451228
return nil, xerrors.Errorf("unknown workspace status in filter: %q", arg.Status)
12461229
}
1230+
if !statusMatch {
1231+
continue
1232+
}
12471233
}
12481234

12491235
if arg.HasAgent != "" {
@@ -5179,3 +5165,13 @@ func (q *fakeQuerier) UpdateWorkspaceProxyDeleted(_ context.Context, arg databas
51795165
}
51805166
return sql.ErrNoRows
51815167
}
5168+
5169+
// isNull is only used in dbfake, so reflect is ok. Use this to make the logic
5170+
// look more similar to the postgres.
5171+
func isNull(v interface{}) bool {
5172+
return !isNotNull(v)
5173+
}
5174+
5175+
func isNotNull(v interface{}) bool {
5176+
return reflect.ValueOf(v).FieldByName("Valid").Bool()
5177+
}

0 commit comments

Comments
 (0)