fix: access GetUserByID in database fake without lock to resolve race (#5909)

kylecarbs · web-flow · commit a911ddaa7b86 · 2023-01-29T16:03:29.000-06:00
See: https://github.com/coder/coder/actions/runs/4038615993/jobs/6942750837
diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go
@@ -556,6 +556,11 @@ func (q *fakeQuerier) GetUserByID(_ context.Context, id uuid.UUID) (database.Use
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
+	return q.getUserByIDNoLock(id)
+}
+
+// getUserByIDNoLock is used by other functions in the database fake.
+func (q *fakeQuerier) getUserByIDNoLock(id uuid.UUID) (database.User, error) {
 	for _, user := range q.users {
 		if user.ID == id {
 			return user, nil
@@ -891,7 +896,7 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.
 		}
 
 		if arg.OwnerUsername != "" {
-			owner, err := q.GetUserByID(ctx, workspace.OwnerID)
+			owner, err := q.getUserByIDNoLock(workspace.OwnerID)
 			if err == nil && !strings.EqualFold(arg.OwnerUsername, owner.Username) {
 				continue
 			}
@@ -2033,7 +2038,7 @@ func (q *fakeQuerier) GetTemplateUserRoles(_ context.Context, id uuid.UUID) ([]d
 
 	users := make([]database.TemplateUser, 0, len(template.UserACL))
 	for k, v := range template.UserACL {
-		user, err := q.GetUserByID(context.Background(), uuid.MustParse(k))
+		user, err := q.getUserByIDNoLock(uuid.MustParse(k))
 		if err != nil && xerrors.Is(err, sql.ErrNoRows) {
 			return nil, xerrors.Errorf("get user by ID: %w", err)
 		}
@@ -3593,7 +3598,7 @@ func (q *fakeQuerier) DeleteGitSSHKey(_ context.Context, userID uuid.UUID) error
 	return sql.ErrNoRows
 }
 
-func (q *fakeQuerier) GetAuditLogsOffset(ctx context.Context, arg database.GetAuditLogsOffsetParams) ([]database.GetAuditLogsOffsetRow, error) {
+func (q *fakeQuerier) GetAuditLogsOffset(_ context.Context, arg database.GetAuditLogsOffsetParams) ([]database.GetAuditLogsOffsetRow, error) {
 	if err := validateDatabaseType(arg); err != nil {
 		return nil, err
 	}
@@ -3619,13 +3624,13 @@ func (q *fakeQuerier) GetAuditLogsOffset(ctx context.Context, arg database.GetAu
 			continue
 		}
 		if arg.Username != "" {
-			user, err := q.GetUserByID(context.Background(), alog.UserID)
+			user, err := q.getUserByIDNoLock(alog.UserID)
 			if err == nil && !strings.EqualFold(arg.Username, user.Username) {
 				continue
 			}
 		}
 		if arg.Email != "" {
-			user, err := q.GetUserByID(context.Background(), alog.UserID)
+			user, err := q.getUserByIDNoLock(alog.UserID)
 			if err == nil && !strings.EqualFold(arg.Email, user.Email) {
 				continue
 			}
@@ -3647,7 +3652,7 @@ func (q *fakeQuerier) GetAuditLogsOffset(ctx context.Context, arg database.GetAu
 			}
 		}
 
-		user, err := q.GetUserByID(ctx, alog.UserID)
+		user, err := q.getUserByIDNoLock(alog.UserID)
 		userValid := err == nil
 
 		logs = append(logs, database.GetAuditLogsOffsetRow{

Original file line number	Diff line number	Diff line change
`@@ -556,6 +556,11 @@ func (q *fakeQuerier) GetUserByID(_ context.Context, id uuid.UUID) (database.Use`
`556`	`556`	`q.mutex.RLock()`
`557`	`557`	`defer q.mutex.RUnlock()`
`558`	`558`
	`559`	`+ return q.getUserByIDNoLock(id)`
	`560`	`+}`
	`561`	`+`
	`562`	`+// getUserByIDNoLock is used by other functions in the database fake.`
	`563`	`+func (q *fakeQuerier) getUserByIDNoLock(id uuid.UUID) (database.User, error) {`
`559`	`564`	`for _, user := range q.users {`
`560`	`565`	`if user.ID == id {`
`561`	`566`	`return user, nil`
`@@ -891,7 +896,7 @@ func (q *fakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database.`
`891`	`896`	`}`
`892`	`897`
`893`	`898`	`if arg.OwnerUsername != "" {`
`894`		`- owner, err := q.GetUserByID(ctx, workspace.OwnerID)`
	`899`	`+ owner, err := q.getUserByIDNoLock(workspace.OwnerID)`
`895`	`900`	`if err == nil && !strings.EqualFold(arg.OwnerUsername, owner.Username) {`
`896`	`901`	`continue`
`897`	`902`	`}`
`@@ -2033,7 +2038,7 @@ func (q *fakeQuerier) GetTemplateUserRoles(_ context.Context, id uuid.UUID) ([]d`
`2033`	`2038`
`2034`	`2039`	`users := make([]database.TemplateUser, 0, len(template.UserACL))`
`2035`	`2040`	`for k, v := range template.UserACL {`
`2036`		`- user, err := q.GetUserByID(context.Background(), uuid.MustParse(k))`
	`2041`	`+ user, err := q.getUserByIDNoLock(uuid.MustParse(k))`
`2037`	`2042`	`if err != nil && xerrors.Is(err, sql.ErrNoRows) {`
`2038`	`2043`	`return nil, xerrors.Errorf("get user by ID: %w", err)`
`2039`	`2044`	`}`
`@@ -3593,7 +3598,7 @@ func (q *fakeQuerier) DeleteGitSSHKey(_ context.Context, userID uuid.UUID) error`
`3593`	`3598`	`return sql.ErrNoRows`
`3594`	`3599`	`}`
`3595`	`3600`
`3596`		`-func (q *fakeQuerier) GetAuditLogsOffset(ctx context.Context, arg database.GetAuditLogsOffsetParams) ([]database.GetAuditLogsOffsetRow, error) {`
	`3601`	`+func (q *fakeQuerier) GetAuditLogsOffset(_ context.Context, arg database.GetAuditLogsOffsetParams) ([]database.GetAuditLogsOffsetRow, error) {`
`3597`	`3602`	`if err := validateDatabaseType(arg); err != nil {`
`3598`	`3603`	`return nil, err`
`3599`	`3604`	`}`
`@@ -3619,13 +3624,13 @@ func (q *fakeQuerier) GetAuditLogsOffset(ctx context.Context, arg database.GetAu`
`3619`	`3624`	`continue`
`3620`	`3625`	`}`
`3621`	`3626`	`if arg.Username != "" {`
`3622`		`- user, err := q.GetUserByID(context.Background(), alog.UserID)`
	`3627`	`+ user, err := q.getUserByIDNoLock(alog.UserID)`
`3623`	`3628`	`if err == nil && !strings.EqualFold(arg.Username, user.Username) {`
`3624`	`3629`	`continue`
`3625`	`3630`	`}`
`3626`	`3631`	`}`
`3627`	`3632`	`if arg.Email != "" {`
`3628`		`- user, err := q.GetUserByID(context.Background(), alog.UserID)`
	`3633`	`+ user, err := q.getUserByIDNoLock(alog.UserID)`
`3629`	`3634`	`if err == nil && !strings.EqualFold(arg.Email, user.Email) {`
`3630`	`3635`	`continue`
`3631`	`3636`	`}`
`@@ -3647,7 +3652,7 @@ func (q *fakeQuerier) GetAuditLogsOffset(ctx context.Context, arg database.GetAu`
`3647`	`3652`	`}`
`3648`	`3653`	`}`
`3649`	`3654`
`3650`		`- user, err := q.GetUserByID(ctx, alog.UserID)`
	`3655`	`+ user, err := q.getUserByIDNoLock(alog.UserID)`
`3651`	`3656`	`userValid := err == nil`
`3652`	`3657`
`3653`	`3658`	`logs = append(logs, database.GetAuditLogsOffsetRow{`