Skip to content

Commit 29a7313

Browse files
DanielleMaywoodDanielleMaywood
authored
refactor: untangle workspace creation from http logic (#19639)
Coder Tasks requires us to create a workspace, but we want to be able to return a `codersdk.Task` instead of a `codersdk.Workspace`. This requires untangling `createWorkspace` from directly writing to `http.ResponseWriter`.
1 parent 7365da1 commit 29a7313

File tree

3 files changed

+106
-57
lines changed

3 files changed

+106
-57
lines changed

‎coderd/aitasks.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/coder/coder/v2/coderd/audit"
1818
"github.com/coder/coder/v2/coderd/database"
1919
"github.com/coder/coder/v2/coderd/httpapi"
20+
"github.com/coder/coder/v2/coderd/httpapi/httperror"
2021
"github.com/coder/coder/v2/coderd/httpmw"
2122
"github.com/coder/coder/v2/coderd/rbac"
2223
"github.com/coder/coder/v2/coderd/rbac/policy"
@@ -154,8 +155,9 @@ func (api *API) tasksCreate(rw http.ResponseWriter, r *http.Request) {
154155
// This can be optimized. It exists as it is now for code simplicity.
155156
// The most common case is to create a workspace for 'Me'. Which does
156157
// not enter this code branch.
157-
template, ok := requestTemplate(ctx, rw, createReq, api.Database)
158-
if !ok {
158+
template, err := requestTemplate(ctx, createReq, api.Database)
159+
if err != nil {
160+
httperror.WriteResponseError(ctx, rw, err)
159161
return
160162
}
161163

@@ -188,7 +190,13 @@ func (api *API) tasksCreate(rw http.ResponseWriter, r *http.Request) {
188190
})
189191

190192
defer commitAudit()
191-
createWorkspace(ctx, aReq, apiKey.UserID, api, owner, createReq, rw, r)
193+
w, err := createWorkspace(ctx, aReq, apiKey.UserID, api, owner, createReq, r)
194+
if err != nil {
195+
httperror.WriteResponseError(ctx, rw, err)
196+
return
197+
}
198+
199+
httpapi.Write(ctx, rw, http.StatusCreated, w)
192200
}
193201

194202
// tasksFromWorkspaces converts a slice of API workspaces into tasks, fetching

‎coderd/httpapi/httperror/responserror.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
11
package httperror
22

33
import (
4+
"context"
45
"errors"
6+
"fmt"
7+
"net/http"
58

9+
"github.com/coder/coder/v2/coderd/httpapi"
610
"github.com/coder/coder/v2/codersdk"
711
)
812

@@ -17,3 +21,48 @@ func IsResponder(err error) (Responder, bool) {
1721
}
1822
return nil, false
1923
}
24+
25+
func NewResponseError(status int, resp codersdk.Response) error {
26+
return &responseError{
27+
status: status,
28+
response: resp,
29+
}
30+
}
31+
32+
func WriteResponseError(ctx context.Context, rw http.ResponseWriter, err error) {
33+
if responseErr, ok := IsResponder(err); ok {
34+
code, resp := responseErr.Response()
35+
36+
httpapi.Write(ctx, rw, code, resp)
37+
return
38+
}
39+
40+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
41+
Message: "Internal server error",
42+
Detail: err.Error(),
43+
})
44+
}
45+
46+
type responseError struct {
47+
status int
48+
response codersdk.Response
49+
}
50+
51+
var (
52+
_ error = (*responseError)(nil)
53+
_ Responder = (*responseError)(nil)
54+
)
55+
56+
func (e *responseError) Error() string {
57+
return fmt.Sprintf("%s: %s", e.response.Message, e.response.Detail)
58+
}
59+
60+
func (e *responseError) Status() int {
61+
return e.status
62+
}
63+
64+
func (e *responseError) Response() (int, codersdk.Response) {
65+
return e.status, e.response
66+
}
67+
68+
var ErrResourceNotFound = NewResponseError(http.StatusNotFound, httpapi.ResourceNotFoundResponse)

‎coderd/workspaces.go

Lines changed: 46 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,13 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
388388
AvatarURL: member.AvatarURL,
389389
}
390390

391-
createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, rw, r)
391+
w, err := createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, r)
392+
if err != nil {
393+
httperror.WriteResponseError(ctx, rw, err)
394+
return
395+
}
396+
397+
httpapi.Write(ctx, rw, http.StatusCreated, w)
392398
}
393399

394400
// Create a new workspace for the currently authenticated user.
@@ -442,8 +448,9 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) {
442448
// This can be optimized. It exists as it is now for code simplicity.
443449
// The most common case is to create a workspace for 'Me'. Which does
444450
// not enter this code branch.
445-
template, ok := requestTemplate(ctx, rw, req, api.Database)
446-
if !ok {
451+
template, err := requestTemplate(ctx, req, api.Database)
452+
if err != nil {
453+
httperror.WriteResponseError(ctx, rw, err)
447454
return
448455
}
449456

@@ -476,7 +483,14 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) {
476483
})
477484

478485
defer commitAudit()
479-
createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, rw, r)
486+
487+
w, err := createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, r)
488+
if err != nil {
489+
httperror.WriteResponseError(ctx, rw, err)
490+
return
491+
}
492+
493+
httpapi.Write(ctx, rw, http.StatusCreated, w)
480494
}
481495

482496
type workspaceOwner struct {
@@ -492,12 +506,11 @@ func createWorkspace(
492506
api *API,
493507
owner workspaceOwner,
494508
req codersdk.CreateWorkspaceRequest,
495-
rw http.ResponseWriter,
496509
r *http.Request,
497-
) {
498-
template, ok := requestTemplate(ctx, rw, req, api.Database)
499-
if !ok {
500-
return
510+
) (codersdk.Workspace, error) {
511+
template, err := requestTemplate(ctx, req, api.Database)
512+
if err != nil {
513+
return codersdk.Workspace{}, err
501514
}
502515

503516
// This is a premature auth check to avoid doing unnecessary work if the user
@@ -506,14 +519,12 @@ func createWorkspace(
506519
rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(owner.ID.String())) {
507520
// If this check fails, return a proper unauthorized error to the user to indicate
508521
// what is going on.
509-
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
522+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusForbidden, codersdk.Response{
510523
Message: "Unauthorized to create workspace.",
511524
Detail: "You are unable to create a workspace in this organization. " +
512525
"It is possible to have access to the template, but not be able to create a workspace. " +
513526
"Please contact an administrator about your permissions if you feel this is an error.",
514-
Validations: nil,
515527
})
516-
return
517528
}
518529

519530
// Update audit log's organization
@@ -523,49 +534,42 @@ func createWorkspace(
523534
// would be wasted.
524535
if !api.Authorize(r, policy.ActionCreate,
525536
rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(owner.ID.String())) {
526-
httpapi.ResourceNotFound(rw)
527-
return
537+
return codersdk.Workspace{}, httperror.ErrResourceNotFound
528538
}
529539
// The user also needs permission to use the template. At this point they have
530540
// read perms, but not necessarily "use". This is also checked in `db.InsertWorkspace`.
531541
// Doing this up front can save some work below if the user doesn't have permission.
532542
if !api.Authorize(r, policy.ActionUse, template) {
533-
httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{
543+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusForbidden, codersdk.Response{
534544
Message: fmt.Sprintf("Unauthorized access to use the template %q.", template.Name),
535545
Detail: "Although you are able to view the template, you are unable to create a workspace using it. " +
536546
"Please contact an administrator about your permissions if you feel this is an error.",
537-
Validations: nil,
538547
})
539-
return
540548
}
541549

542550
templateAccessControl := (*(api.AccessControlStore.Load())).GetTemplateAccessControl(template)
543551
if templateAccessControl.IsDeprecated() {
544-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
552+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{
545553
Message: fmt.Sprintf("Template %q has been deprecated, and cannot be used to create a new workspace.", template.Name),
546554
// Pass the deprecated message to the user.
547-
Detail: templateAccessControl.Deprecated,
548-
Validations: nil,
555+
Detail: templateAccessControl.Deprecated,
549556
})
550-
return
551557
}
552558

553559
dbAutostartSchedule, err := validWorkspaceSchedule(req.AutostartSchedule)
554560
if err != nil {
555-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
561+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{
556562
Message: "Invalid Autostart Schedule.",
557563
Validations: []codersdk.ValidationError{{Field: "schedule", Detail: err.Error()}},
558564
})
559-
return
560565
}
561566

562567
templateSchedule, err := (*api.TemplateScheduleStore.Load()).Get(ctx, api.Database, template.ID)
563568
if err != nil {
564-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
569+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{
565570
Message: "Internal error fetching template schedule.",
566571
Detail: err.Error(),
567572
})
568-
return
569573
}
570574

571575
nextStartAt := sql.NullTime{}
@@ -578,23 +582,21 @@ func createWorkspace(
578582

579583
dbTTL, err := validWorkspaceTTLMillis(req.TTLMillis, templateSchedule.DefaultTTL)
580584
if err != nil {
581-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
585+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{
582586
Message: "Invalid Workspace Time to Shutdown.",
583587
Validations: []codersdk.ValidationError{{Field: "ttl_ms", Detail: err.Error()}},
584588
})
585-
return
586589
}
587590

588591
// back-compatibility: default to "never" if not included.
589592
dbAU := database.AutomaticUpdatesNever
590593
if req.AutomaticUpdates != "" {
591594
dbAU, err = validWorkspaceAutomaticUpdates(req.AutomaticUpdates)
592595
if err != nil {
593-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
596+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{
594597
Message: "Invalid Workspace Automatic Updates setting.",
595598
Validations: []codersdk.ValidationError{{Field: "automatic_updates", Detail: err.Error()}},
596599
})
597-
return
598600
}
599601
}
600602

@@ -607,20 +609,18 @@ func createWorkspace(
607609
})
608610
if err == nil {
609611
// If the workspace already exists, don't allow creation.
610-
httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{
612+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusConflict, codersdk.Response{
611613
Message: fmt.Sprintf("Workspace %q already exists.", req.Name),
612614
Validations: []codersdk.ValidationError{{
613615
Field: "name",
614616
Detail: "This value is already in use and should be unique.",
615617
}},
616618
})
617-
return
618619
} else if !errors.Is(err, sql.ErrNoRows) {
619-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
620+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{
620621
Message: fmt.Sprintf("Internal error fetching workspace by name %q.", req.Name),
621622
Detail: err.Error(),
622623
})
623-
return
624624
}
625625

626626
var (
@@ -759,8 +759,7 @@ func createWorkspace(
759759
return err
760760
}, nil)
761761
if err != nil {
762-
httperror.WriteWorkspaceBuildError(ctx, rw, err)
763-
return
762+
return codersdk.Workspace{}, err
764763
}
765764

766765
err = provisionerjobs.PostJob(api.Pubsub, *provisionerJob)
@@ -809,11 +808,10 @@ func createWorkspace(
809808
provisionerDaemons,
810809
)
811810
if err != nil {
812-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
811+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{
813812
Message: "Internal error converting workspace build.",
814813
Detail: err.Error(),
815814
})
816-
return
817815
}
818816

819817
w, err := convertWorkspace(
@@ -825,40 +823,38 @@ func createWorkspace(
825823
codersdk.WorkspaceAppStatus{},
826824
)
827825
if err != nil {
828-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
826+
return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{
829827
Message: "Internal error converting workspace.",
830828
Detail: err.Error(),
831829
})
832-
return
833830
}
834-
httpapi.Write(ctx, rw, http.StatusCreated, w)
831+
832+
return w, nil
835833
}
836834

837-
func requestTemplate(ctx context.Context, rw http.ResponseWriter, req codersdk.CreateWorkspaceRequest, db database.Store) (database.Template, bool) {
835+
func requestTemplate(ctx context.Context, req codersdk.CreateWorkspaceRequest, db database.Store) (database.Template, error) {
838836
// If we were given a `TemplateVersionID`, we need to determine the `TemplateID` from it.
839837
templateID := req.TemplateID
840838

841839
if templateID == uuid.Nil {
842840
templateVersion, err := db.GetTemplateVersionByID(ctx, req.TemplateVersionID)
843841
if httpapi.Is404Error(err) {
844-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
842+
return database.Template{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{
845843
Message: fmt.Sprintf("Template version %q doesn't exist.", req.TemplateVersionID),
846844
Validations: []codersdk.ValidationError{{
847845
Field: "template_version_id",
848846
Detail: "template not found",
849847
}},
850848
})
851-
return database.Template{}, false
852849
}
853850
if err != nil {
854-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
851+
return database.Template{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{
855852
Message: "Internal error fetching template version.",
856853
Detail: err.Error(),
857854
})
858-
return database.Template{}, false
859855
}
860856
if templateVersion.Archived {
861-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
857+
return database.Template{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{
862858
Message: "Archived template versions cannot be used to make a workspace.",
863859
Validations: []codersdk.ValidationError{
864860
{
@@ -867,37 +863,33 @@ func requestTemplate(ctx context.Context, rw http.ResponseWriter, req codersdk.C
867863
},
868864
},
869865
})
870-
return database.Template{}, false
871866
}
872867

873868
templateID = templateVersion.TemplateID.UUID
874869
}
875870

876871
template, err := db.GetTemplateByID(ctx, templateID)
877872
if httpapi.Is404Error(err) {
878-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
873+
return database.Template{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{
879874
Message: fmt.Sprintf("Template %q doesn't exist.", templateID),
880875
Validations: []codersdk.ValidationError{{
881876
Field: "template_id",
882877
Detail: "template not found",
883878
}},
884879
})
885-
return database.Template{}, false
886880
}
887881
if err != nil {
888-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
882+
return database.Template{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{
889883
Message: "Internal error fetching template.",
890884
Detail: err.Error(),
891885
})
892-
return database.Template{}, false
893886
}
894887
if template.Deleted {
895-
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
888+
return database.Template{}, httperror.NewResponseError(http.StatusNotFound, codersdk.Response{
896889
Message: fmt.Sprintf("Template %q has been deleted!", template.Name),
897890
})
898-
return database.Template{}, false
899891
}
900-
return template, true
892+
return template, nil
901893
}
902894

903895
func claimPrebuild(

0 commit comments

Comments
 (0)