mirror of
https://github.com/coder/coder.git
synced 2026-11-01 04:47:17 +08:00
refactor(coderd): add related-data selection to workspace queries (#28302)
This is the first of three PRs implementing lite `codersdk.Workspace` responses ([GRU-82](https://linear.app/codercom/issue/GRU-82), [RFC](https://app.notion.com/p/coderhq/Lite-Workspace-API-Requests-3aad579be5928060b5e7cc65c539717f)). A fully populated workspace fans out into a large tree of DB queries (template, latest build, provisioner job + queue position, resources, metadata, agents, apps, statuses, scripts, log sources, template version). Most callers do not need most of that data, and one of these queries, `GetProvisionerJobsByIDsWithQueuePosition`, is consistently the top resource consumer in scale tests. This PR adds the internal plumbing to load only a selected subset of that tree. It introduces a `workspaceRelated` selection type and threads it through `workspaceData` and `workspaceBuildsData` down to the individual query call sites, skipping any query whose data was not selected. The selection is a pointer tree that mirrors the parent/child relationships between the objects: branch nodes are pointers (non-nil when selected) and leaf nodes are bools. Modeling it this way makes selecting a child without its parent unrepresentable, which matches how loading works (a parent must be queried to learn its children's identifiers), so there is no normalization step to forget. No behavior change: every existing caller passes a fully populated selection (`allWorkspaceRelated` / `allLatestBuildRelated`), so responses are identical. Wiring the HTTP `include_related` query parameter (Phase 2) and narrowing individual callers (Phase 3) come in follow-up PRs. Two notes on the current mapping: - `queue_position` is modeled as its own node, but only one job query exists and it always computes the ranking, so selecting `job` runs the full query today. Splitting off a cheaper queue-position-free job query is deferred (it needs a new SQL query + `make gen`). - The workspace-level `LatestAppStatus` is gated on the `latest_build.resources.agents.apps.statuses` node. It is queried directly by workspace ID, but a caller selecting it also pulls in the build chain as ancestors. Happy to decouple this if preferred. <details> <summary>Implementation plan</summary> # Lite Workspace API — Phase 1 Plan Add an internal configuration structure to the Coderd method that fetches a workspace with its related data (`workspaceData` /`workspaceBuildsData`), thread it down to the SQL query call sites, and skip ("squash") queries that are not requested. Server-internal only. No HTTP query-string parsing (Phase 2) and no `wsrelated` struct tags or field nil-ing (Phase 3). ## Scope - In: config struct mapping the RFC related-data hierarchy; threading through `workspaceData` and `workspaceBuildsData`; gating every related-data query; comprehensive unit tests with a mocked `database.Store` (gomock/dbmock). - Out: HTTP `include_related` parsing, codersdk changes, `wsrelated` tags, docs autogen, and changing `convertWorkspaces` skip-on-missing-data behavior. All existing callers pass an "include all" config, so Phase 1 is a behavior- preserving refactor. ## Design New file `coderd/wsrelateddata.go` with a pointer tree that mirrors the RFC hierarchy. Branch nodes are pointers (present when non-nil); leaf nodes are bools. Because a child cannot be expressed without its parent, the RFC "includes all ancestors" rule is a structural invariant of the type: an illegal selection is unrepresentable, so no normalization pass exists or is needed. ```go type workspaceRelated struct { Template bool LatestBuild *latestBuildRelated } type latestBuildRelated struct { Job *jobRelated Resources *resourcesRelated TemplateVersion bool } type jobRelated struct{ QueuePosition bool } type resourcesRelated struct { Metadata bool Agents *agentsRelated } type agentsRelated struct { Apps *appsRelated Scripts bool LogSources bool } type appsRelated struct{ Statuses bool } ``` Helpers: - `allWorkspaceRelated()` / `allLatestBuildRelated()` construct a fully populated tree. Passed by all current callers so behavior is unchanged. - Zero value (nil subtree) includes nothing but the workspace itself. - `(*latestBuildRelated).appStatuses()` is a nil-safe accessor for the deep `apps.statuses` leaf so call sites descend the tree without a guard chain. Rationale for the pointer tree over a flat struct plus `withAncestors()`: the invariant holds by construction (illegal state is unrepresentable) instead of relying on a normalization step a caller must remember. `workspaceBuildsData` takes the `*latestBuildRelated` subtree directly. ### Query gating `workspaceData(ctx, workspaces, cfg)`: - `GetTemplatesWithFilter` gated on `Template`. - `GetLatestWorkspaceBuildsByWorkspaceIDs` gated on `LatestBuild`; when off, skip `workspaceBuildsData` entirely (empty builds). - `GetLatestWorkspaceAppStatusesByWorkspaceIDs` (workspace-level LatestAppStatus) gated on `AppStatuses`. `workspaceBuildsData(ctx, builds, cfg)` gates each stage: - jobs (`GetProvisionerJobsByIDsWithQueuePosition`) + eligible provisioner daemons gated on `Job`. Only one job query exists and it always computes the queue position, so `QueuePosition` does not gate a separate query yet; it is modeled for hierarchy/Phase 2 completeness. A cheaper job query without the window-function ranking is a worthwhile follow-up but is out of scope for a simple Phase 1 (needs new SQL + sqlc gen). - resources gated on `Resources`; metadata on `Metadata`; agents on `Agents`; apps on `Apps`; app statuses on `AppStatuses`; scripts on `Scripts`; log sources on `LogSources`; template versions on `TemplateVersion`. Downstream `convertWorkspaceBuilds` already tolerates empty slices, so squashing queries yields zero/nil fields without extra work. ## Testing (comprehensive, mocked database.Store) Internal tests (`coderd/wsrelateddata_internal_test.go`) build `&API{Options: &Options{Database: dbmock.NewMockStore(ctrl)}}` and call `workspaceData` directly. gomock is strict, so setting EXPECT only on permitted queries proves the rest are squashed. Cases: include-all (every query runs); zero value (no related queries run); each single node (template only, latest_build only, job, resources, metadata, agents, apps, statuses, scripts, log_sources, template_version, workspace-level app statuses); the empty-resources short-circuit; and the constructors plus the nil-safe `appStatuses()` accessor at every depth. ## Steps 1. Add `coderd/wsrelateddata.go` (selection tree, constructors, `appStatuses`). 2. Thread the selection through `workspaceData` and `workspaceBuildsData`; gate queries by descending the tree. 3. Update the callers to pass `allWorkspaceRelated()` / `allLatestBuildRelated()`. 4. Add unit tests; run `make fmt`, `make lint`, targeted `go test`. ## Resolved decisions 1. Pointer tree over a flat struct + normalizer, so the ancestor rule is a structural invariant. 2. Cheaper queue-position-free job query deferred to a follow-up. </details> --- *Generated by Coder Agents on behalf of @spikecurtis.*
This commit is contained in:
+44
-35
@@ -96,7 +96,7 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
data, err := api.workspaceData(ctx, []database.Workspace{workspace})
|
||||
data, err := api.workspaceData(ctx, []database.Workspace{workspace}, allWorkspaceRelated())
|
||||
if err != nil {
|
||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||
Message: "Internal error fetching workspace resources.",
|
||||
@@ -224,7 +224,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
data, err := api.workspaceData(ctx, workspaces)
|
||||
data, err := api.workspaceData(ctx, workspaces, allWorkspaceRelated())
|
||||
if err != nil {
|
||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||
Message: "Internal error fetching workspace resources.",
|
||||
@@ -313,7 +313,7 @@ func (api *API) workspaceByOwnerAndName(rw http.ResponseWriter, r *http.Request)
|
||||
return
|
||||
}
|
||||
|
||||
data, err := api.workspaceData(ctx, []database.Workspace{workspace})
|
||||
data, err := api.workspaceData(ctx, []database.Workspace{workspace}, allWorkspaceRelated())
|
||||
if err != nil {
|
||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||
Message: "Internal error fetching workspace resources.",
|
||||
@@ -1562,7 +1562,7 @@ func (api *API) putWorkspaceDormant(rw http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
data, err := api.workspaceData(ctx, []database.Workspace{workspace})
|
||||
data, err := api.workspaceData(ctx, []database.Workspace{workspace}, allWorkspaceRelated())
|
||||
if err != nil {
|
||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||
Message: "Internal error fetching workspace resources.",
|
||||
@@ -2140,7 +2140,7 @@ func (api *API) watchWorkspace(
|
||||
return
|
||||
}
|
||||
|
||||
data, err := api.workspaceData(ctx, []database.Workspace{workspace})
|
||||
data, err := api.workspaceData(ctx, []database.Workspace{workspace}, allWorkspaceRelated())
|
||||
if err != nil {
|
||||
_ = sendEvent(codersdk.ServerSentEvent{
|
||||
Type: codersdk.ServerSentEventTypeError,
|
||||
@@ -2678,7 +2678,7 @@ func (api *API) allowWorkspaceSharing(ctx context.Context, rw http.ResponseWrite
|
||||
// does not have the correct perms to read a given template, the template will
|
||||
// not be returned.
|
||||
// So the caller must check the templates & users exist before using them.
|
||||
func (api *API) workspaceData(ctx context.Context, workspaces []database.Workspace) (workspaceData, error) {
|
||||
func (api *API) workspaceData(ctx context.Context, workspaces []database.Workspace, cfg workspaceRelated) (workspaceData, error) {
|
||||
workspaceIDs := make([]uuid.UUID, 0, len(workspaces))
|
||||
templateIDs := make([]uuid.UUID, 0, len(workspaces))
|
||||
for _, workspace := range workspaces {
|
||||
@@ -2692,41 +2692,50 @@ func (api *API) workspaceData(ctx context.Context, workspaces []database.Workspa
|
||||
appStatuses []database.WorkspaceAppStatus
|
||||
eg errgroup.Group
|
||||
)
|
||||
eg.Go(func() (err error) {
|
||||
templates, err = api.Database.GetTemplatesWithFilter(ctx, database.GetTemplatesWithFilterParams{
|
||||
IDs: templateIDs,
|
||||
if cfg.Template {
|
||||
eg.Go(func() (err error) {
|
||||
templates, err = api.Database.GetTemplatesWithFilter(ctx, database.GetTemplatesWithFilterParams{
|
||||
IDs: templateIDs,
|
||||
})
|
||||
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
||||
return xerrors.Errorf("get templates: %w", err)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
||||
return xerrors.Errorf("get templates: %w", err)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
eg.Go(func() (err error) {
|
||||
// This query must be run as system restricted to be efficient.
|
||||
// nolint:gocritic
|
||||
builds, err = api.Database.GetLatestWorkspaceBuildsByWorkspaceIDs(dbauthz.AsSystemRestricted(ctx), workspaceIDs)
|
||||
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
||||
return xerrors.Errorf("get workspace builds: %w", err)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
eg.Go(func() (err error) {
|
||||
// This query must be run as system restricted to be efficient.
|
||||
// nolint:gocritic
|
||||
appStatuses, err = api.Database.GetLatestWorkspaceAppStatusesByWorkspaceIDs(dbauthz.AsSystemRestricted(ctx), workspaceIDs)
|
||||
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
||||
return xerrors.Errorf("get workspace app statuses: %w", err)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
}
|
||||
if cfg.LatestBuild != nil {
|
||||
eg.Go(func() (err error) {
|
||||
// This query must be run as system restricted to be efficient.
|
||||
// nolint:gocritic
|
||||
builds, err = api.Database.GetLatestWorkspaceBuildsByWorkspaceIDs(dbauthz.AsSystemRestricted(ctx), workspaceIDs)
|
||||
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
||||
return xerrors.Errorf("get workspace builds: %w", err)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
}
|
||||
if cfg.LatestBuild.appStatuses() {
|
||||
eg.Go(func() (err error) {
|
||||
// This query must be run as system restricted to be efficient.
|
||||
// nolint:gocritic
|
||||
appStatuses, err = api.Database.GetLatestWorkspaceAppStatusesByWorkspaceIDs(dbauthz.AsSystemRestricted(ctx), workspaceIDs)
|
||||
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
||||
return xerrors.Errorf("get workspace app statuses: %w", err)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
}
|
||||
err := eg.Wait()
|
||||
if err != nil {
|
||||
return workspaceData{}, err
|
||||
}
|
||||
|
||||
data, err := api.workspaceBuildsData(ctx, builds)
|
||||
if err != nil {
|
||||
return workspaceData{}, xerrors.Errorf("get workspace builds data: %w", err)
|
||||
var data workspaceBuildsData
|
||||
if cfg.LatestBuild != nil {
|
||||
data, err = api.workspaceBuildsData(ctx, builds, *cfg.LatestBuild)
|
||||
if err != nil {
|
||||
return workspaceData{}, xerrors.Errorf("get workspace builds data: %w", err)
|
||||
}
|
||||
}
|
||||
|
||||
apiBuilds, err := api.convertWorkspaceBuilds(
|
||||
|
||||
Reference in New Issue
Block a user