From db8592fa9339d9d2eec0c3ae31d557f353e39c44 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Tue, 10 Oct 2023 16:55:28 +0400 Subject: [PATCH] chore: refactor workspace conversion to accept ownerName (#10171) Refactors workspace conversion to accept the ownerName, rather than a slice of users, since all it does is search the slice for the owner and use the username. This is in preparation for a fix to `postWorkspacesByOrganization()` that will remove the need to pass the user object. Also avoids panicing if the required user is not in the slice, since `findUser` could return nil in the old code, which would then get dereferenced for the username. --- coderd/users.go | 10 +++--- coderd/workspaceagents.go | 12 ++++---- coderd/workspacebuilds.go | 51 ++++++++++++++++++++---------- coderd/workspaces.go | 65 ++++++++++++++++++++++++++------------- 4 files changed, 90 insertions(+), 48 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 7633290a80..9fa71c0563 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1177,13 +1177,13 @@ func userOrganizationIDs(ctx context.Context, api *API, user database.User) ([]u return member.OrganizationIDs, nil } -func findUser(id uuid.UUID, users []database.User) *database.User { - for _, u := range users { - if u.ID == id { - return &u +func usernameWithID(id uuid.UUID, users []database.User) (string, bool) { + for _, user := range users { + if id == user.ID { + return user.Username, true } } - return nil + return "", false } func convertAPIKey(k database.APIKey) codersdk.APIKey { diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index ce5d0de0f3..44f047bb3a 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -121,7 +121,7 @@ func (api *API) workspaceAgent(rw http.ResponseWriter, r *http.Request) { } apiAgent, err := convertWorkspaceAgent( - api.DERPMap(), *api.TailnetCoordinator.Load(), workspaceAgent, convertApps(dbApps, workspaceAgent, owner, workspace), convertScripts(scripts), convertLogSources(logSources), api.AgentInactiveDisconnectTimeout, + api.DERPMap(), *api.TailnetCoordinator.Load(), workspaceAgent, convertApps(dbApps, workspaceAgent, owner.Username, workspace), convertScripts(scripts), convertLogSources(logSources), api.AgentInactiveDisconnectTimeout, api.DeploymentValues.AgentFallbackTroubleshootingURL.String(), ) if err != nil { @@ -235,7 +235,7 @@ func (api *API) workspaceAgentManifest(rw http.ResponseWriter, r *http.Request) httpapi.Write(ctx, rw, http.StatusOK, agentsdk.Manifest{ AgentID: apiAgent.ID, - Apps: convertApps(dbApps, workspaceAgent, owner, workspace), + Apps: convertApps(dbApps, workspaceAgent, owner.Username, workspace), Scripts: convertScripts(scripts), DERPMap: api.DERPMap(), DERPForceWebSockets: api.DeploymentValues.DERP.Config.ForceWebSockets.Value(), @@ -1404,14 +1404,14 @@ func (api *API) workspaceAgentClientCoordinate(rw http.ResponseWriter, r *http.R // convertProvisionedApps converts applications that are in the middle of provisioning process. // It means that they may not have an agent or workspace assigned (dry-run job). func convertProvisionedApps(dbApps []database.WorkspaceApp) []codersdk.WorkspaceApp { - return convertApps(dbApps, database.WorkspaceAgent{}, database.User{}, database.Workspace{}) + return convertApps(dbApps, database.WorkspaceAgent{}, "", database.Workspace{}) } -func convertApps(dbApps []database.WorkspaceApp, agent database.WorkspaceAgent, owner database.User, workspace database.Workspace) []codersdk.WorkspaceApp { +func convertApps(dbApps []database.WorkspaceApp, agent database.WorkspaceAgent, ownerName string, workspace database.Workspace) []codersdk.WorkspaceApp { apps := make([]codersdk.WorkspaceApp, 0) for _, dbApp := range dbApps { var subdomainName string - if dbApp.Subdomain && agent.Name != "" && owner.Username != "" && workspace.Name != "" { + if dbApp.Subdomain && agent.Name != "" && ownerName != "" && workspace.Name != "" { appSlug := dbApp.Slug if appSlug == "" { appSlug = dbApp.DisplayName @@ -1420,7 +1420,7 @@ func convertApps(dbApps []database.WorkspaceApp, agent database.WorkspaceAgent, AppSlugOrPort: appSlug, AgentName: agent.Name, WorkspaceName: workspace.Name, - Username: owner.Username, + Username: ownerName, }.String() } diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 998bf37ce1..16326f9945 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -68,12 +68,20 @@ func (api *API) workspaceBuild(rw http.ResponseWriter, r *http.Request) { }) return } + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error converting workspace build.", + Detail: "owner not found for workspace", + }) + return + } apiBuild, err := api.convertWorkspaceBuild( workspaceBuild, workspace, data.jobs[0], - data.users, + ownerName, data.resources, data.metadata, data.agents, @@ -274,12 +282,20 @@ func (api *API) workspaceBuildByBuildNumber(rw http.ResponseWriter, r *http.Requ }) return } + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error converting workspace build.", + Detail: "owner not found for workspace", + }) + return + } apiBuild, err := api.convertWorkspaceBuild( workspaceBuild, workspace, data.jobs[0], - data.users, + ownerName, data.resources, data.metadata, data.agents, @@ -398,6 +414,14 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { }) return } + ownerName, exists := usernameWithID(workspace.OwnerID, users) + if !exists { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error converting workspace build.", + Detail: "owner not found for workspace", + }) + return + } apiBuild, err := api.convertWorkspaceBuild( *workspaceBuild, @@ -406,7 +430,7 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { ProvisionerJob: *provisionerJob, QueuePosition: 0, }, - users, + ownerName, []database.WorkspaceResource{}, []database.WorkspaceResourceMetadatum{}, []database.WorkspaceAgent{}, @@ -807,12 +831,16 @@ func (api *API) convertWorkspaceBuilds( if !exists { return nil, xerrors.New("template version not found") } + ownerName, exists := usernameWithID(workspace.OwnerID, users) + if !exists { + return nil, xerrors.Errorf("owner not found for workspace: %q", workspace.Name) + } apiBuild, err := api.convertWorkspaceBuild( build, workspace, job, - users, + ownerName, workspaceResources, resourceMetadata, resourceAgents, @@ -835,7 +863,7 @@ func (api *API) convertWorkspaceBuild( build database.WorkspaceBuild, workspace database.Workspace, job database.GetProvisionerJobsByIDsWithQueuePositionRow, - users []database.User, + ownerName string, workspaceResources []database.WorkspaceResource, resourceMetadata []database.WorkspaceResourceMetadatum, resourceAgents []database.WorkspaceAgent, @@ -844,10 +872,6 @@ func (api *API) convertWorkspaceBuild( agentLogSources []database.WorkspaceAgentLogSource, templateVersion database.TemplateVersion, ) (codersdk.WorkspaceBuild, error) { - userByID := map[uuid.UUID]database.User{} - for _, user := range users { - userByID[user.ID] = user - } resourcesByJobID := map[uuid.UUID][]database.WorkspaceResource{} for _, resource := range workspaceResources { resourcesByJobID[resource.JobID] = append(resourcesByJobID[resource.JobID], resource) @@ -873,11 +897,6 @@ func (api *API) convertWorkspaceBuild( logSourcesByAgentID[logSource.WorkspaceAgentID] = append(logSourcesByAgentID[logSource.WorkspaceAgentID], logSource) } - owner, exists := userByID[workspace.OwnerID] - if !exists { - return codersdk.WorkspaceBuild{}, xerrors.Errorf("owner not found for workspace: %q", workspace.Name) - } - resources := resourcesByJobID[job.ProvisionerJob.ID] apiResources := make([]codersdk.WorkspaceResource, 0) for _, resource := range resources { @@ -888,7 +907,7 @@ func (api *API) convertWorkspaceBuild( scripts := scriptsByAgentID[agent.ID] logSources := logSourcesByAgentID[agent.ID] apiAgent, err := convertWorkspaceAgent( - api.DERPMap(), *api.TailnetCoordinator.Load(), agent, convertApps(apps, agent, owner, workspace), convertScripts(scripts), convertLogSources(logSources), api.AgentInactiveDisconnectTimeout, + api.DERPMap(), *api.TailnetCoordinator.Load(), agent, convertApps(apps, agent, ownerName, workspace), convertScripts(scripts), convertLogSources(logSources), api.AgentInactiveDisconnectTimeout, api.DeploymentValues.AgentFallbackTroubleshootingURL.String(), ) if err != nil { @@ -906,7 +925,7 @@ func (api *API) convertWorkspaceBuild( CreatedAt: build.CreatedAt, UpdatedAt: build.UpdatedAt, WorkspaceOwnerID: workspace.OwnerID, - WorkspaceOwnerName: owner.Username, + WorkspaceOwnerName: ownerName, WorkspaceID: build.WorkspaceID, WorkspaceName: workspace.Name, TemplateVersionID: build.TemplateVersionID, diff --git a/coderd/workspaces.go b/coderd/workspaces.go index a95bf2afe7..b4f971fe97 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -92,12 +92,19 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { httpapi.Forbidden(rw) return } - + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: "unable to find workspace owner's username", + }) + return + } httpapi.Write(ctx, rw, http.StatusOK, convertWorkspace( workspace, data.builds[0], data.templates[0], - findUser(workspace.OwnerID, data.users), + ownerName, )) } @@ -274,12 +281,19 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request) httpapi.ResourceNotFound(rw) return } - + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: "unable to find workspace owner's username", + }) + return + } httpapi.Write(ctx, rw, http.StatusOK, convertWorkspace( workspace, data.builds[0], data.templates[0], - findUser(workspace.OwnerID, data.users), + ownerName, )) } @@ -529,21 +543,11 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req } aReq.New = workspace - initiator, err := api.Database.GetUserByID(ctx, workspaceBuild.InitiatorID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching user.", - Detail: err.Error(), - }) - return - } - api.Telemetry.Report(&telemetry.Snapshot{ Workspaces: []telemetry.Workspace{telemetry.ConvertWorkspace(workspace)}, WorkspaceBuilds: []telemetry.WorkspaceBuild{telemetry.ConvertWorkspaceBuild(*workspaceBuild)}, }) - users := []database.User{user, initiator} apiBuild, err := api.convertWorkspaceBuild( *workspaceBuild, workspace, @@ -551,7 +555,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req ProvisionerJob: *provisionerJob, QueuePosition: 0, }, - users, + user.Username, []database.WorkspaceResource{}, []database.WorkspaceResourceMetadatum{}, []database.WorkspaceAgent{}, @@ -572,7 +576,7 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req workspace, apiBuild, template, - findUser(user.ID, users), + user.Username, )) } @@ -885,6 +889,14 @@ func (api *API) putWorkspaceDormant(rw http.ResponseWriter, r *http.Request) { }) return } + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: "unable to find workspace owner's username", + }) + return + } if len(data.templates) == 0 { httpapi.Forbidden(rw) @@ -896,7 +908,7 @@ func (api *API) putWorkspaceDormant(rw http.ResponseWriter, r *http.Request) { workspace, data.builds[0], data.templates[0], - findUser(workspace.OwnerID, data.users), + ownerName, )) } @@ -1111,13 +1123,24 @@ func (api *API) watchWorkspace(rw http.ResponseWriter, r *http.Request) { return } + ownerName, ok := usernameWithID(workspace.OwnerID, data.users) + if !ok { + _ = sendEvent(ctx, codersdk.ServerSentEvent{ + Type: codersdk.ServerSentEventTypeError, + Data: codersdk.Response{ + Message: "Internal error fetching workspace resources.", + Detail: "unable to find workspace owner's username", + }, + }) + return + } _ = sendEvent(ctx, codersdk.ServerSentEvent{ Type: codersdk.ServerSentEventTypeData, Data: convertWorkspace( workspace, data.builds[0], data.templates[0], - findUser(workspace.OwnerID, data.users), + ownerName, ), }) } @@ -1267,7 +1290,7 @@ func convertWorkspaces(workspaces []database.Workspace, data workspaceData) ([]c workspace, build, template, - &owner, + owner.Username, )) } return apiWorkspaces, nil @@ -1277,7 +1300,7 @@ func convertWorkspace( workspace database.Workspace, workspaceBuild codersdk.WorkspaceBuild, template database.Template, - owner *database.User, + ownerName string, ) codersdk.Workspace { var autostartSchedule *string if workspace.AutostartSchedule.Valid { @@ -1310,7 +1333,7 @@ func convertWorkspace( CreatedAt: workspace.CreatedAt, UpdatedAt: workspace.UpdatedAt, OwnerID: workspace.OwnerID, - OwnerName: owner.Username, + OwnerName: ownerName, OrganizationID: workspace.OrganizationID, TemplateID: workspace.TemplateID, LatestBuild: workspaceBuild,