From 29cb4b29eab6ee0a36ee4b567d9718747afb7564 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 21 Jul 2023 16:29:58 +0000 Subject: [PATCH 1/9] feat(coderd): add parameter insights to template insights Part of #8514 Refs #8109 --- coderd/apidoc/docs.go | 47 +++++++++++ coderd/apidoc/swagger.json | 47 +++++++++++ coderd/database/dbauthz/dbauthz.go | 8 ++ coderd/database/dbfake/dbfake.go | 9 +++ coderd/database/dbmetrics/dbmetrics.go | 7 ++ coderd/database/dbmock/dbmock.go | 15 ++++ coderd/database/querier.go | 5 ++ coderd/database/queries.sql.go | 102 +++++++++++++++++++++++ coderd/database/queries/insights.sql | 53 ++++++++++++ coderd/insights.go | 70 ++++++++++++++-- codersdk/insights.go | 30 +++---- docs/api/insights.md | 21 +++++ docs/api/schemas.md | 107 +++++++++++++++++++++++-- site/src/api/typesGenerated.ts | 16 ++++ 14 files changed, 505 insertions(+), 32 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index df9090b878d1e..1128900ff6b52 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -9319,6 +9319,12 @@ const docTemplate = `{ "type": "string", "format": "date-time" }, + "parameters_usage": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.TemplateParameterUsage" + } + }, "start_time": { "type": "string", "format": "date-time" @@ -9346,6 +9352,47 @@ const docTemplate = `{ } } }, + "codersdk.TemplateParameterUsage": { + "type": "object", + "properties": { + "display_name": { + "type": "string" + }, + "name": { + "type": "string" + }, + "options": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.TemplateVersionParameterOption" + } + }, + "template_ids": { + "type": "array", + "items": { + "type": "string", + "format": "uuid" + } + }, + "values": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.TemplateParameterValue" + } + } + } + }, + "codersdk.TemplateParameterValue": { + "type": "object", + "properties": { + "count": { + "type": "integer" + }, + "value": { + "type": "string" + } + } + }, "codersdk.TemplateRestartRequirement": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 768a8d7be1166..1c23ba2309a9d 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -8418,6 +8418,12 @@ "type": "string", "format": "date-time" }, + "parameters_usage": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.TemplateParameterUsage" + } + }, "start_time": { "type": "string", "format": "date-time" @@ -8445,6 +8451,47 @@ } } }, + "codersdk.TemplateParameterUsage": { + "type": "object", + "properties": { + "display_name": { + "type": "string" + }, + "name": { + "type": "string" + }, + "options": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.TemplateVersionParameterOption" + } + }, + "template_ids": { + "type": "array", + "items": { + "type": "string", + "format": "uuid" + } + }, + "values": { + "type": "array", + "items": { + "$ref": "#/definitions/codersdk.TemplateParameterValue" + } + } + } + }, + "codersdk.TemplateParameterValue": { + "type": "object", + "properties": { + "count": { + "type": "integer" + }, + "value": { + "type": "string" + } + } + }, "codersdk.TemplateRestartRequirement": { "type": "object", "properties": { diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index d606ff510f576..32e97bcae0286 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1189,6 +1189,14 @@ func (q *querier) GetTemplateInsights(ctx context.Context, arg database.GetTempl return q.db.GetTemplateInsights(ctx, arg) } +func (q *querier) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { + // FIXME: this should maybe be READ rbac.ResourceTemplate or it's own resource. + if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { + return nil, err + } + return q.db.GetTemplateParameterInsights(ctx, arg) +} + func (q *querier) GetTemplateVersionByID(ctx context.Context, tvid uuid.UUID) (database.TemplateVersion, error) { tv, err := q.db.GetTemplateVersionByID(ctx, tvid) if err != nil { diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index d2f71f7ea22f0..6b43073bc71b3 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2067,6 +2067,15 @@ func (q *FakeQuerier) GetTemplateInsights(_ context.Context, arg database.GetTem return result, nil } +func (q *FakeQuerier) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { + err := validateDatabaseType(arg) + if err != nil { + return nil, err + } + + panic("not implemented") +} + func (q *FakeQuerier) GetTemplateVersionByID(ctx context.Context, templateVersionID uuid.UUID) (database.TemplateVersion, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/dbmetrics/dbmetrics.go b/coderd/database/dbmetrics/dbmetrics.go index 5264e64779793..5631cbcfc9143 100644 --- a/coderd/database/dbmetrics/dbmetrics.go +++ b/coderd/database/dbmetrics/dbmetrics.go @@ -613,6 +613,13 @@ func (m metricsStore) GetTemplateInsights(ctx context.Context, arg database.GetT return r0, r1 } +func (m metricsStore) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { + start := time.Now() + r0, r1 := m.s.GetTemplateParameterInsights(ctx, arg) + m.queryLatencies.WithLabelValues("GetTemplateParameterInsights").Observe(time.Since(start).Seconds()) + return r0, r1 +} + func (m metricsStore) GetTemplateVersionByID(ctx context.Context, id uuid.UUID) (database.TemplateVersion, error) { start := time.Now() version, err := m.s.GetTemplateVersionByID(ctx, id) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index e9cdb1e4c9ec4..b5368795c9c97 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1241,6 +1241,21 @@ func (mr *MockStoreMockRecorder) GetTemplateInsights(arg0, arg1 interface{}) *go return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTemplateInsights", reflect.TypeOf((*MockStore)(nil).GetTemplateInsights), arg0, arg1) } +// GetTemplateParameterInsights mocks base method. +func (m *MockStore) GetTemplateParameterInsights(arg0 context.Context, arg1 database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetTemplateParameterInsights", arg0, arg1) + ret0, _ := ret[0].([]database.GetTemplateParameterInsightsRow) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetTemplateParameterInsights indicates an expected call of GetTemplateParameterInsights. +func (mr *MockStoreMockRecorder) GetTemplateParameterInsights(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetTemplateParameterInsights", reflect.TypeOf((*MockStore)(nil).GetTemplateParameterInsights), arg0, arg1) +} + // GetTemplateUserRoles mocks base method. func (m *MockStore) GetTemplateUserRoles(arg0 context.Context, arg1 uuid.UUID) ([]database.TemplateUser, error) { m.ctrl.T.Helper() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index d64090ea32c8f..224ebe06c507d 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -113,6 +113,11 @@ type sqlcQuerier interface { // GetTemplateInsights has a granularity of 5 minutes where if a session/app was // in use, we will add 5 minutes to the total usage for that session (per user). GetTemplateInsights(ctx context.Context, arg GetTemplateInsightsParams) (GetTemplateInsightsRow, error) + // GetTemplateParameterInsights does for each template in a given timeframe, + // look for the latest workspace build (for every workspace) that has been + // created in the timeframe and return the aggregate usage counts of parameter + // values. + GetTemplateParameterInsights(ctx context.Context, arg GetTemplateParameterInsightsParams) ([]GetTemplateParameterInsightsRow, error) GetTemplateVersionByID(ctx context.Context, id uuid.UUID) (TemplateVersion, error) GetTemplateVersionByJobID(ctx context.Context, jobID uuid.UUID) (TemplateVersion, error) GetTemplateVersionByTemplateIDAndName(ctx context.Context, arg GetTemplateVersionByTemplateIDAndNameParams) (TemplateVersion, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index e85603b1debdc..8c1c09be6509f 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1531,6 +1531,108 @@ func (q *sqlQuerier) GetTemplateInsights(ctx context.Context, arg GetTemplateIns return i, err } +const getTemplateParameterInsights = `-- name: GetTemplateParameterInsights :many +WITH latest_workspace_builds AS ( + SELECT + wb.id, + wbmax.template_id, + wb.template_version_id + FROM ( + SELECT + tv.template_id, wbmax.workspace_id, MAX(wbmax.build_number) as max_build_number + FROM workspace_builds wbmax + JOIN template_versions tv ON (tv.id = wbmax.template_version_id) + WHERE + wbmax.created_at >= $1::timestamptz + AND wbmax.created_at < $2::timestamptz + AND CASE WHEN COALESCE(array_length($3::uuid[], 1), 0) > 0 THEN tv.template_id = ANY($3::uuid[]) ELSE TRUE END + GROUP BY tv.template_id, wbmax.workspace_id + ) wbmax + JOIN workspace_builds wb ON ( + wb.workspace_id = wbmax.workspace_id + AND wb.build_number = wbmax.max_build_number + ) +), unique_template_params AS ( + SELECT + ROW_NUMBER() OVER () AS num, + array_agg(DISTINCT wb.template_id)::uuid[] AS template_ids, + array_agg(wb.id)::uuid[] AS workspace_build_ids, + tvp.name, + tvp.display_name, + tvp.description, + tvp.options + FROM latest_workspace_builds wb + JOIN template_version_parameters tvp ON (tvp.template_version_id = wb.template_version_id) + GROUP BY tvp.name, tvp.display_name, tvp.description, tvp.options +) + +SELECT + utp.num, + utp.template_ids, + utp.name, + utp.display_name, + utp.description, + utp.options, + wbp.value, + COUNT(wbp.value) AS count +FROM unique_template_params utp +JOIN workspace_build_parameters wbp ON (utp.workspace_build_ids @> ARRAY[wbp.workspace_build_id] AND utp.name = wbp.name) +GROUP BY utp.num, utp.name, utp.display_name, utp.description, utp.options, utp.template_ids, wbp.value +` + +type GetTemplateParameterInsightsParams struct { + StartTime time.Time `db:"start_time" json:"start_time"` + EndTime time.Time `db:"end_time" json:"end_time"` + TemplateIDs []uuid.UUID `db:"template_ids" json:"template_ids"` +} + +type GetTemplateParameterInsightsRow struct { + Num int64 `db:"num" json:"num"` + TemplateIDs []uuid.UUID `db:"template_ids" json:"template_ids"` + Name string `db:"name" json:"name"` + DisplayName string `db:"display_name" json:"display_name"` + Description string `db:"description" json:"description"` + Options json.RawMessage `db:"options" json:"options"` + Value string `db:"value" json:"value"` + Count int64 `db:"count" json:"count"` +} + +// GetTemplateParameterInsights does for each template in a given timeframe, +// look for the latest workspace build (for every workspace) that has been +// created in the timeframe and return the aggregate usage counts of parameter +// values. +func (q *sqlQuerier) GetTemplateParameterInsights(ctx context.Context, arg GetTemplateParameterInsightsParams) ([]GetTemplateParameterInsightsRow, error) { + rows, err := q.db.QueryContext(ctx, getTemplateParameterInsights, arg.StartTime, arg.EndTime, pq.Array(arg.TemplateIDs)) + if err != nil { + return nil, err + } + defer rows.Close() + var items []GetTemplateParameterInsightsRow + for rows.Next() { + var i GetTemplateParameterInsightsRow + if err := rows.Scan( + &i.Num, + pq.Array(&i.TemplateIDs), + &i.Name, + &i.DisplayName, + &i.Description, + &i.Options, + &i.Value, + &i.Count, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const getUserLatencyInsights = `-- name: GetUserLatencyInsights :many SELECT workspace_agent_stats.user_id, diff --git a/coderd/database/queries/insights.sql b/coderd/database/queries/insights.sql index c3dad57d2d673..1b4fde9dbf79f 100644 --- a/coderd/database/queries/insights.sql +++ b/coderd/database/queries/insights.sql @@ -103,3 +103,56 @@ SELECT COUNT(DISTINCT user_id) AS active_users FROM usage_by_day, unnest(template_ids) as template_id GROUP BY from_, to_; + + +-- name: GetTemplateParameterInsights :many +-- GetTemplateParameterInsights does for each template in a given timeframe, +-- look for the latest workspace build (for every workspace) that has been +-- created in the timeframe and return the aggregate usage counts of parameter +-- values. +WITH latest_workspace_builds AS ( + SELECT + wb.id, + wbmax.template_id, + wb.template_version_id + FROM ( + SELECT + tv.template_id, wbmax.workspace_id, MAX(wbmax.build_number) as max_build_number + FROM workspace_builds wbmax + JOIN template_versions tv ON (tv.id = wbmax.template_version_id) + WHERE + wbmax.created_at >= @start_time::timestamptz + AND wbmax.created_at < @end_time::timestamptz + AND CASE WHEN COALESCE(array_length(@template_ids::uuid[], 1), 0) > 0 THEN tv.template_id = ANY(@template_ids::uuid[]) ELSE TRUE END + GROUP BY tv.template_id, wbmax.workspace_id + ) wbmax + JOIN workspace_builds wb ON ( + wb.workspace_id = wbmax.workspace_id + AND wb.build_number = wbmax.max_build_number + ) +), unique_template_params AS ( + SELECT + ROW_NUMBER() OVER () AS num, + array_agg(DISTINCT wb.template_id)::uuid[] AS template_ids, + array_agg(wb.id)::uuid[] AS workspace_build_ids, + tvp.name, + tvp.display_name, + tvp.description, + tvp.options + FROM latest_workspace_builds wb + JOIN template_version_parameters tvp ON (tvp.template_version_id = wb.template_version_id) + GROUP BY tvp.name, tvp.display_name, tvp.description, tvp.options +) + +SELECT + utp.num, + utp.template_ids, + utp.name, + utp.display_name, + utp.description, + utp.options, + wbp.value, + COUNT(wbp.value) AS count +FROM unique_template_params utp +JOIN workspace_build_parameters wbp ON (utp.workspace_build_ids @> ARRAY[wbp.workspace_build_id] AND utp.name = wbp.name) +GROUP BY utp.num, utp.name, utp.display_name, utp.description, utp.options, utp.template_ids, wbp.value; diff --git a/coderd/insights.go b/coderd/insights.go index 3da60a13bfe84..b6f270894b9a0 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -2,6 +2,7 @@ package coderd import ( "context" + "encoding/json" "fmt" "net/http" "time" @@ -192,11 +193,11 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) { var dailyUsage []database.GetTemplateDailyInsightsRow // Use a transaction to ensure that we get consistent data between // the full and interval report. - err := api.Database.InTx(func(db database.Store) error { + err := api.Database.InTx(func(tx database.Store) error { var err error if interval != "" { - dailyUsage, err = db.GetTemplateDailyInsights(ctx, database.GetTemplateDailyInsightsParams{ + dailyUsage, err = tx.GetTemplateDailyInsights(ctx, database.GetTemplateDailyInsightsParams{ StartTime: startTime, EndTime: endTime, TemplateIDs: templateIDs, @@ -206,7 +207,7 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) { } } - usage, err = db.GetTemplateInsights(ctx, database.GetTemplateInsightsParams{ + usage, err = tx.GetTemplateInsights(ctx, database.GetTemplateInsightsParams{ StartTime: startTime, EndTime: endTime, TemplateIDs: templateIDs, @@ -225,13 +226,38 @@ func (api *API) insightsTemplates(rw http.ResponseWriter, r *http.Request) { return } + // Template parameter insights have no risk of inconsistency with the other + // insights, so we don't need to perform this in a transaction. + parameterRows, err := api.Database.GetTemplateParameterInsights(ctx, database.GetTemplateParameterInsightsParams{ + StartTime: startTime, + EndTime: endTime, + TemplateIDs: templateIDs, + }) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching template parameter insights.", + Detail: err.Error(), + }) + return + } + + parametersUsage, err := convertTemplateInsightsParameters(parameterRows) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error converting template parameter insights.", + Detail: err.Error(), + }) + return + } + resp := codersdk.TemplateInsightsResponse{ Report: codersdk.TemplateInsightsReport{ - StartTime: startTime, - EndTime: endTime, - TemplateIDs: usage.TemplateIDs, - ActiveUsers: usage.ActiveUsers, - AppsUsage: convertTemplateInsightsBuiltinApps(usage), + StartTime: startTime, + EndTime: endTime, + TemplateIDs: usage.TemplateIDs, + ActiveUsers: usage.ActiveUsers, + AppsUsage: convertTemplateInsightsBuiltinApps(usage), + ParametersUsage: parametersUsage, }, IntervalReports: []codersdk.TemplateInsightsIntervalReport{}, } @@ -286,6 +312,34 @@ func convertTemplateInsightsBuiltinApps(usage database.GetTemplateInsightsRow) [ } } +func convertTemplateInsightsParameters(parameterRows []database.GetTemplateParameterInsightsRow) ([]codersdk.TemplateParameterUsage, error) { + parametersByNum := make(map[int64]*codersdk.TemplateParameterUsage) + for _, param := range parameterRows { + if _, ok := parametersByNum[param.Num]; !ok { + var opts []codersdk.TemplateVersionParameterOption + err := json.Unmarshal(param.Options, &opts) + if err != nil { + return nil, xerrors.Errorf("unmarshal template parameter options: %w", err) + } + parametersByNum[param.Num] = &codersdk.TemplateParameterUsage{ + TemplateIDs: param.TemplateIDs, + Name: param.Name, + DisplayName: param.DisplayName, + Options: opts, + } + } + parametersByNum[param.Num].Values = append(parametersByNum[param.Num].Values, codersdk.TemplateParameterValue{ + Value: param.Value, + Count: param.Count, + }) + } + parametersUsage := []codersdk.TemplateParameterUsage{} + for _, param := range parametersByNum { + parametersUsage = append(parametersUsage, *param) + } + return parametersUsage, nil +} + // parseInsightsStartAndEndTime parses the start and end time query parameters // and returns the parsed values. The client provided timezone must be preserved // when parsing the time. Verification is performed so that the start and end diff --git a/codersdk/insights.go b/codersdk/insights.go index fb1c582c686c8..92f7eba196e6a 100644 --- a/codersdk/insights.go +++ b/codersdk/insights.go @@ -93,13 +93,12 @@ type TemplateInsightsResponse struct { // TemplateInsightsReport is the report from the template insights endpoint. type TemplateInsightsReport struct { - StartTime time.Time `json:"start_time" format:"date-time"` - EndTime time.Time `json:"end_time" format:"date-time"` - TemplateIDs []uuid.UUID `json:"template_ids" format:"uuid"` - ActiveUsers int64 `json:"active_users" example:"22"` - AppsUsage []TemplateAppUsage `json:"apps_usage"` - // TODO(mafredri): To be introduced in a future pull request. - // TemplateParametersUsage []TemplateParameterUsage `json:"parameters_usage"` + StartTime time.Time `json:"start_time" format:"date-time"` + EndTime time.Time `json:"end_time" format:"date-time"` + TemplateIDs []uuid.UUID `json:"template_ids" format:"uuid"` + ActiveUsers int64 `json:"active_users" example:"22"` + AppsUsage []TemplateAppUsage `json:"apps_usage"` + ParametersUsage []TemplateParameterUsage `json:"parameters_usage"` } // TemplateInsightsIntervalReport is the report from the template insights @@ -132,25 +131,22 @@ type TemplateAppUsage struct { Seconds int64 `json:"seconds" example:"80500"` } -// TODO(mafredri): To be introduced in a future pull request. -/* // TemplateParameterUsage shows the usage of a parameter for one or more // templates. type TemplateParameterUsage struct { - TemplateIDs []uuid.UUID `json:"template_ids" format:"uuid"` - DisplayName string `json:"display_name"` - Name string `json:"name"` - Values []TemplateParameterValue `json:"values"` + TemplateIDs []uuid.UUID `json:"template_ids" format:"uuid"` + DisplayName string `json:"display_name"` + Name string `json:"name"` + Options []TemplateVersionParameterOption `json:"options,omitempty"` + Values []TemplateParameterValue `json:"values"` } // TemplateParameterValue shows the usage of a parameter value for one or more // templates. type TemplateParameterValue struct { - Value *string `json:"value"` - Icon string `json:"icon"` - Count int64 `json:"count"` + Value string `json:"value"` + Count int64 `json:"count"` } -*/ type TemplateInsightsRequest struct { StartTime time.Time `json:"start_time" format:"date-time"` diff --git a/docs/api/insights.md b/docs/api/insights.md index a802916fa579c..56ffd14474952 100644 --- a/docs/api/insights.md +++ b/docs/api/insights.md @@ -78,6 +78,27 @@ curl -X GET http://coder-server:8080/api/v2/insights/templates \ } ], "end_time": "2019-08-24T14:15:22Z", + "parameters_usage": [ + { + "display_name": "string", + "name": "string", + "options": [ + { + "description": "string", + "icon": "string", + "name": "string", + "value": "string" + } + ], + "template_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"], + "values": [ + { + "count": 0, + "value": "string" + } + ] + } + ], "start_time": "2019-08-24T14:15:22Z", "template_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"] } diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 355e388393676..ee1d239797ee4 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -4208,6 +4208,27 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in } ], "end_time": "2019-08-24T14:15:22Z", + "parameters_usage": [ + { + "display_name": "string", + "name": "string", + "options": [ + { + "description": "string", + "icon": "string", + "name": "string", + "value": "string" + } + ], + "template_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"], + "values": [ + { + "count": 0, + "value": "string" + } + ] + } + ], "start_time": "2019-08-24T14:15:22Z", "template_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"] } @@ -4215,13 +4236,14 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in ### Properties -| Name | Type | Required | Restrictions | Description | -| -------------- | --------------------------------------------------------------- | -------- | ------------ | ----------- | -| `active_users` | integer | false | | | -| `apps_usage` | array of [codersdk.TemplateAppUsage](#codersdktemplateappusage) | false | | | -| `end_time` | string | false | | | -| `start_time` | string | false | | | -| `template_ids` | array of string | false | | | +| Name | Type | Required | Restrictions | Description | +| ------------------ | --------------------------------------------------------------------------- | -------- | ------------ | ----------- | +| `active_users` | integer | false | | | +| `apps_usage` | array of [codersdk.TemplateAppUsage](#codersdktemplateappusage) | false | | | +| `end_time` | string | false | | | +| `parameters_usage` | array of [codersdk.TemplateParameterUsage](#codersdktemplateparameterusage) | false | | | +| `start_time` | string | false | | | +| `template_ids` | array of string | false | | | ## codersdk.TemplateInsightsResponse @@ -4249,6 +4271,27 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in } ], "end_time": "2019-08-24T14:15:22Z", + "parameters_usage": [ + { + "display_name": "string", + "name": "string", + "options": [ + { + "description": "string", + "icon": "string", + "name": "string", + "value": "string" + } + ], + "template_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"], + "values": [ + { + "count": 0, + "value": "string" + } + ] + } + ], "start_time": "2019-08-24T14:15:22Z", "template_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"] } @@ -4262,6 +4305,56 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `interval_reports` | array of [codersdk.TemplateInsightsIntervalReport](#codersdktemplateinsightsintervalreport) | false | | | | `report` | [codersdk.TemplateInsightsReport](#codersdktemplateinsightsreport) | false | | | +## codersdk.TemplateParameterUsage + +```json +{ + "display_name": "string", + "name": "string", + "options": [ + { + "description": "string", + "icon": "string", + "name": "string", + "value": "string" + } + ], + "template_ids": ["497f6eca-6276-4993-bfeb-53cbbbba6f08"], + "values": [ + { + "count": 0, + "value": "string" + } + ] +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| -------------- | ------------------------------------------------------------------------------------------- | -------- | ------------ | ----------- | +| `display_name` | string | false | | | +| `name` | string | false | | | +| `options` | array of [codersdk.TemplateVersionParameterOption](#codersdktemplateversionparameteroption) | false | | | +| `template_ids` | array of string | false | | | +| `values` | array of [codersdk.TemplateParameterValue](#codersdktemplateparametervalue) | false | | | + +## codersdk.TemplateParameterValue + +```json +{ + "count": 0, + "value": "string" +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ------- | ------- | -------- | ------------ | ----------- | +| `count` | integer | false | | | +| `value` | string | false | | | + ## codersdk.TemplateRestartRequirement ```json diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 9f74ef107ddfa..b3bbfb616fb03 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -940,6 +940,7 @@ export interface TemplateInsightsReport { readonly template_ids: string[] readonly active_users: number readonly apps_usage: TemplateAppUsage[] + readonly parameters_usage: TemplateParameterUsage[] } // From codersdk/insights.go @@ -956,6 +957,21 @@ export interface TemplateInsightsResponse { readonly interval_reports: TemplateInsightsIntervalReport[] } +// From codersdk/insights.go +export interface TemplateParameterUsage { + readonly template_ids: string[] + readonly display_name: string + readonly name: string + readonly options?: TemplateVersionParameterOption[] + readonly values: TemplateParameterValue[] +} + +// From codersdk/insights.go +export interface TemplateParameterValue { + readonly value: string + readonly count: number +} + // From codersdk/templates.go export interface TemplateRestartRequirement { readonly days_of_week: string[] From 680da6eee5618f99e85ed49b5fdbad28e3824c2f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 21 Jul 2023 18:19:48 +0000 Subject: [PATCH 2/9] add fakedb implementation --- coderd/database/dbfake/dbfake.go | 87 +++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 6b43073bc71b3..83d8441285cf2 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2073,7 +2073,92 @@ func (q *FakeQuerier) GetTemplateParameterInsights(ctx context.Context, arg data return nil, err } - panic("not implemented") + q.mutex.RLock() + defer q.mutex.RUnlock() + + // WITH latest_workspace_builds ... + latestWorkspaceBuilds := make(map[uuid.UUID]database.WorkspaceBuild) + for _, wb := range q.workspaceBuilds { + if wb.CreatedAt.Before(arg.StartTime) || wb.CreatedAt.Equal(arg.EndTime) || wb.CreatedAt.After(arg.EndTime) { + continue + } + if latestWorkspaceBuilds[wb.WorkspaceID].BuildNumber < wb.BuildNumber { + latestWorkspaceBuilds[wb.WorkspaceID] = wb + } + } + if len(arg.TemplateIDs) > 0 { + for wsID := range latestWorkspaceBuilds { + ws, err := q.getWorkspaceByIDNoLock(ctx, wsID) + if err != nil { + return nil, err + } + if slices.Contains(arg.TemplateIDs, ws.TemplateID) { + delete(latestWorkspaceBuilds, wsID) + } + } + } + // WITH unique_template_params ... + num := int64(0) + uniqueTemplateParams := make(map[string]*database.GetTemplateParameterInsightsRow) + uniqueTemplateParamWorkspaceBuildIDs := make(map[string][]uuid.UUID) + for _, wb := range latestWorkspaceBuilds { + tv, err := q.getTemplateVersionByIDNoLock(ctx, wb.TemplateVersionID) + if err != nil { + return nil, err + } + for _, tvp := range q.templateVersionParameters { + if tvp.TemplateVersionID != tv.ID { + continue + } + key := fmt.Sprintf("%s:%s:%s:%s", tvp.Name, tvp.DisplayName, tvp.Description, tvp.Options) + if _, ok := uniqueTemplateParams[key]; !ok { + num++ + uniqueTemplateParams[key] = &database.GetTemplateParameterInsightsRow{ + Num: num, + Name: tvp.Name, + DisplayName: tvp.DisplayName, + Description: tvp.Description, + Options: tvp.Options, + } + } + uniqueTemplateParams[key].TemplateIDs = append(uniqueTemplateParams[key].TemplateIDs, tv.TemplateID.UUID) + uniqueTemplateParamWorkspaceBuildIDs[key] = append(uniqueTemplateParamWorkspaceBuildIDs[key], wb.ID) + } + } + // SELECT ... + counts := make(map[string]map[string]int64) + for key, utp := range uniqueTemplateParams { + for _, wbp := range q.workspaceBuildParameters { + if !slices.Contains(uniqueTemplateParamWorkspaceBuildIDs[key], wbp.WorkspaceBuildID) { + continue + } + if wbp.Name != utp.Name { + continue + } + if counts[key] == nil { + counts[key] = make(map[string]int64) + } + counts[key][wbp.Value]++ + } + } + + var rows []database.GetTemplateParameterInsightsRow + for key, utp := range uniqueTemplateParams { + for value, count := range counts[key] { + rows = append(rows, database.GetTemplateParameterInsightsRow{ + Num: utp.Num, + TemplateIDs: utp.TemplateIDs, + Name: utp.Name, + DisplayName: utp.DisplayName, + Description: utp.Description, + Options: utp.Options, + Value: value, + Count: count, + }) + } + } + + return rows, nil } func (q *FakeQuerier) GetTemplateVersionByID(ctx context.Context, templateVersionID uuid.UUID) (database.TemplateVersion, error) { From 05d7a2c8fa1d14fa75f46d8d431edaf83ea3a185 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 21 Jul 2023 18:26:35 +0000 Subject: [PATCH 3/9] add todo --- coderd/insights_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/insights_test.go b/coderd/insights_test.go index 613a2c07ec428..2fba03326abfb 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -341,6 +341,8 @@ func TestTemplateInsights(t *testing.T) { assert.WithinDuration(t, req.StartTime, resp.IntervalReports[0].StartTime, 0) assert.WithinDuration(t, req.EndTime, resp.IntervalReports[0].EndTime, 0) assert.Equal(t, resp.IntervalReports[0].ActiveUsers, int64(1), "want one active user in the interval report") + + // TODO(mafredri): Verify template parameter insights (first we need to generate them). } func TestTemplateInsights_BadRequest(t *testing.T) { From bf218e93d37157a31d44e1e92b2aaa78c267bd38 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 21 Jul 2023 18:35:45 +0000 Subject: [PATCH 4/9] uniq --- coderd/database/dbfake/dbfake.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 83d8441285cf2..a35def84e3e4b 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -562,6 +562,21 @@ func isNotNull(v interface{}) bool { // these methods remain unimplemented in the FakeQuerier. var ErrUnimplemented = xerrors.New("unimplemented") +func uniqueSortedUUIDs(uuids []uuid.UUID) []uuid.UUID { + set := make(map[uuid.UUID]struct{}) + for _, id := range uuids { + set[id] = struct{}{} + } + unique := make([]uuid.UUID, 0, len(set)) + for id := range set { + unique = append(unique, id) + } + slices.SortFunc(unique, func(a, b uuid.UUID) bool { + return a.String() < b.String() + }) + return unique +} + func (*FakeQuerier) AcquireLock(_ context.Context, _ int64) error { return xerrors.New("AcquireLock must only be called within a transaction") } @@ -2147,7 +2162,7 @@ func (q *FakeQuerier) GetTemplateParameterInsights(ctx context.Context, arg data for value, count := range counts[key] { rows = append(rows, database.GetTemplateParameterInsightsRow{ Num: utp.Num, - TemplateIDs: utp.TemplateIDs, + TemplateIDs: uniqueSortedUUIDs(utp.TemplateIDs), Name: utp.Name, DisplayName: utp.DisplayName, Description: utp.Description, From 05c91c77d29c0185d708ac348d4c80543b3b0052 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 1 Aug 2023 12:12:55 +0200 Subject: [PATCH 5/9] Fix: dbfake --- coderd/database/dbauthz/dbauthz.go | 17 ++++++++++++++--- coderd/database/dbfake/dbfake.go | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 5ebc1bb521c02..34e2c43079063 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1233,9 +1233,20 @@ func (q *querier) GetTemplateInsights(ctx context.Context, arg database.GetTempl } func (q *querier) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { - // FIXME: this should maybe be READ rbac.ResourceTemplate or it's own resource. - if err := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem); err != nil { - return nil, err + for _, templateID := range arg.TemplateIDs { + template, err := q.db.GetTemplateByID(ctx, templateID) + if err != nil { + return nil, err + } + + if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { + return nil, err + } + } + if len(arg.TemplateIDs) == 0 { + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { + return nil, err + } } return q.db.GetTemplateParameterInsights(ctx, arg) } diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index 348e242e53e31..c34c957173893 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -2147,7 +2147,7 @@ func (q *FakeQuerier) GetTemplateParameterInsights(ctx context.Context, arg data defer q.mutex.RUnlock() // WITH latest_workspace_builds ... - latestWorkspaceBuilds := make(map[uuid.UUID]database.WorkspaceBuild) + latestWorkspaceBuilds := make(map[uuid.UUID]database.WorkspaceBuildTable) for _, wb := range q.workspaceBuilds { if wb.CreatedAt.Before(arg.StartTime) || wb.CreatedAt.Equal(arg.EndTime) || wb.CreatedAt.After(arg.EndTime) { continue From 2e37d6a5fbff709afa1c5ac7509edd8abfd8654b Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Tue, 1 Aug 2023 13:05:24 +0200 Subject: [PATCH 6/9] fix: stories --- .../TemplateInsightsPage/TemplateInsightsPage.stories.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.stories.tsx b/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.stories.tsx index 995e8e83b799e..49678eef08375 100644 --- a/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.stories.tsx +++ b/site/src/pages/TemplatePage/TemplateInsightsPage/TemplateInsightsPage.stories.tsx @@ -26,6 +26,7 @@ export const Empty: Story = { start_time: "", template_ids: [], apps_usage: [], + parameters_usage: [], }, }, userLatency: { @@ -82,6 +83,7 @@ export const Loaded: Story = { seconds: 1020900, }, ], + parameters_usage: [], }, interval_reports: [ { From 6afb91421008d10c2e6018b62183cd94b4af0143 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Thu, 3 Aug 2023 11:12:41 +0200 Subject: [PATCH 7/9] Test parameter usage --- coderd/insights.go | 6 +++ coderd/insights_test.go | 94 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 94 insertions(+), 6 deletions(-) diff --git a/coderd/insights.go b/coderd/insights.go index 93903ff1a463c..b643303dd0df2 100644 --- a/coderd/insights.go +++ b/coderd/insights.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "sort" "time" "github.com/google/uuid" @@ -339,6 +340,11 @@ func convertTemplateInsightsParameters(parameterRows []database.GetTemplateParam for _, param := range parametersByNum { parametersUsage = append(parametersUsage, *param) } + + sort.Slice(parametersUsage, func(i, j int) bool { + return parametersUsage[i].Name < parametersUsage[j].Name + }) + return parametersUsage, nil } diff --git a/coderd/insights_test.go b/coderd/insights_test.go index 0925a871bfa79..2469cc0f4b362 100644 --- a/coderd/insights_test.go +++ b/coderd/insights_test.go @@ -20,6 +20,7 @@ import ( "github.com/coder/coder/codersdk" "github.com/coder/coder/codersdk/agentsdk" "github.com/coder/coder/provisioner/echo" + "github.com/coder/coder/provisionersdk/proto" "github.com/coder/coder/testutil" ) @@ -231,6 +232,32 @@ func TestUserLatencyInsights_BadRequest(t *testing.T) { func TestTemplateInsights(t *testing.T) { t.Parallel() + const ( + firstParameterName = "first_parameter" + firstParameterDisplayName = "First PARAMETER" + firstParameterType = "string" + firstParameterDescription = "This is first parameter" + firstParameterValue = "abc" + + secondParameterName = "second_parameter" + secondParameterDisplayName = "Second PARAMETER" + secondParameterType = "number" + secondParameterDescription = "This is second parameter" + secondParameterValue = "123" + + thirdParameterName = "third_parameter" + thirdParameterDisplayName = "Third PARAMETER" + thirdParameterType = "string" + thirdParameterDescription = "This is third parameter" + thirdParameterValue = "bbb" + thirdParameterOptionName1 = "This is AAA" + thirdParameterOptionValue1 = "aaa" + thirdParameterOptionName2 = "This is BBB" + thirdParameterOptionValue2 = "bbb" + thirdParameterOptionName3 = "This is CCC" + thirdParameterOptionValue3 = "ccc" + ) + logger := slogtest.Make(t, nil) opts := &coderdtest.Options{ IncludeProvisionerDaemon: true, @@ -241,15 +268,39 @@ func TestTemplateInsights(t *testing.T) { user := coderdtest.CreateFirstUser(t, client) authToken := uuid.NewString() version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, &echo.Responses{ - Parse: echo.ParseComplete, - ProvisionPlan: echo.ProvisionComplete, + Parse: echo.ParseComplete, + ProvisionPlan: []*proto.Provision_Response{ + { + Type: &proto.Provision_Response_Complete{ + Complete: &proto.Provision_Complete{ + Parameters: []*proto.RichParameter{ + {Name: firstParameterName, DisplayName: firstParameterDisplayName, Type: firstParameterType, Description: firstParameterDescription, Required: true}, + {Name: secondParameterName, DisplayName: secondParameterDisplayName, Type: secondParameterType, Description: secondParameterDescription, Required: true}, + {Name: thirdParameterName, DisplayName: thirdParameterDisplayName, Type: thirdParameterType, Description: thirdParameterDescription, Required: true, Options: []*proto.RichParameterOption{ + {Name: thirdParameterOptionName1, Value: thirdParameterOptionValue1}, + {Name: thirdParameterOptionName2, Value: thirdParameterOptionValue2}, + {Name: thirdParameterOptionName3, Value: thirdParameterOptionValue3}, + }}, + }, + }, + }, + }, + }, ProvisionApply: echo.ProvisionApplyWithAgent(authToken), }) template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) require.Empty(t, template.BuildTimeStats[codersdk.WorkspaceTransitionStart]) - coderdtest.AwaitTemplateVersionJob(t, client, version.ID) - workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + + buildParameters := []codersdk.WorkspaceBuildParameter{ + {Name: firstParameterName, Value: firstParameterValue}, + {Name: secondParameterName, Value: secondParameterValue}, + {Name: thirdParameterName, Value: thirdParameterValue}, + } + + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.RichParameterValues = buildParameters + }) coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) // Start an agent so that we can generate stats. @@ -346,12 +397,43 @@ func TestTemplateInsights(t *testing.T) { } } // The full timeframe is <= 24h, so the interval matches exactly. - assert.Len(t, resp.IntervalReports, 1, "want one interval report") + require.Len(t, resp.IntervalReports, 1, "want one interval report") assert.WithinDuration(t, req.StartTime, resp.IntervalReports[0].StartTime, 0) assert.WithinDuration(t, req.EndTime, resp.IntervalReports[0].EndTime, 0) assert.Equal(t, resp.IntervalReports[0].ActiveUsers, int64(1), "want one active user in the interval report") - // TODO(mafredri): Verify template parameter insights (first we need to generate them). + // The workspace uses 3 parameters + require.Len(t, resp.Report.ParametersUsage, 3) + assert.Equal(t, firstParameterName, resp.Report.ParametersUsage[0].Name) + assert.Equal(t, firstParameterDisplayName, resp.Report.ParametersUsage[0].DisplayName) + assert.Contains(t, resp.Report.ParametersUsage[0].Values, codersdk.TemplateParameterValue{ + Value: firstParameterValue, + Count: 1, + }) + assert.Contains(t, resp.Report.ParametersUsage[0].TemplateIDs, template.ID) + assert.Empty(t, resp.Report.ParametersUsage[0].Options) + + assert.Equal(t, secondParameterName, resp.Report.ParametersUsage[1].Name) + assert.Equal(t, secondParameterDisplayName, resp.Report.ParametersUsage[1].DisplayName) + assert.Contains(t, resp.Report.ParametersUsage[1].Values, codersdk.TemplateParameterValue{ + Value: secondParameterValue, + Count: 1, + }) + assert.Contains(t, resp.Report.ParametersUsage[1].TemplateIDs, template.ID) + assert.Empty(t, resp.Report.ParametersUsage[1].Options) + + assert.Equal(t, thirdParameterName, resp.Report.ParametersUsage[2].Name) + assert.Equal(t, thirdParameterDisplayName, resp.Report.ParametersUsage[2].DisplayName) + assert.Contains(t, resp.Report.ParametersUsage[2].Values, codersdk.TemplateParameterValue{ + Value: thirdParameterValue, + Count: 1, + }) + assert.Contains(t, resp.Report.ParametersUsage[2].TemplateIDs, template.ID) + assert.Equal(t, []codersdk.TemplateVersionParameterOption{ + {Name: thirdParameterOptionName1, Value: thirdParameterOptionValue1}, + {Name: thirdParameterOptionName2, Value: thirdParameterOptionValue2}, + {Name: thirdParameterOptionName3, Value: thirdParameterOptionValue3}, + }, resp.Report.ParametersUsage[2].Options) } func TestTemplateInsights_BadRequest(t *testing.T) { From 81146cda62e08c50d955e285836d18c59124ddc7 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Thu, 3 Aug 2023 13:40:24 +0200 Subject: [PATCH 8/9] try: Use GetAuthorizedTemplates --- coderd/database/dbauthz/dbauthz.go | 45 ++++++++++++++++-------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index ab865e2cc0f70..cadb672a05ce6 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1190,17 +1190,18 @@ func (q *querier) GetTemplateDAUs(ctx context.Context, arg database.GetTemplateD } func (q *querier) GetTemplateDailyInsights(ctx context.Context, arg database.GetTemplateDailyInsightsParams) ([]database.GetTemplateDailyInsightsRow, error) { - for _, templateID := range arg.TemplateIDs { - template, err := q.db.GetTemplateByID(ctx, templateID) + if len(arg.TemplateIDs) > 0 { + prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionUpdate, rbac.ResourceTemplate.Type) if err != nil { - return nil, err + return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err) } - - if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { + _, err = q.db.GetAuthorizedTemplates(ctx, database.GetTemplatesWithFilterParams{ + IDs: arg.TemplateIDs, + }, prep) + if err != nil { return nil, err } - } - if len(arg.TemplateIDs) == 0 { + } else { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { return nil, err } @@ -1209,17 +1210,18 @@ func (q *querier) GetTemplateDailyInsights(ctx context.Context, arg database.Get } func (q *querier) GetTemplateInsights(ctx context.Context, arg database.GetTemplateInsightsParams) (database.GetTemplateInsightsRow, error) { - for _, templateID := range arg.TemplateIDs { - template, err := q.db.GetTemplateByID(ctx, templateID) + if len(arg.TemplateIDs) > 0 { + prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionUpdate, rbac.ResourceTemplate.Type) if err != nil { - return database.GetTemplateInsightsRow{}, err + return database.GetTemplateInsightsRow{}, xerrors.Errorf("(dev error) prepare sql filter: %w", err) } - - if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { + _, err = q.db.GetAuthorizedTemplates(ctx, database.GetTemplatesWithFilterParams{ + IDs: arg.TemplateIDs, + }, prep) + if err != nil { return database.GetTemplateInsightsRow{}, err } - } - if len(arg.TemplateIDs) == 0 { + } else { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { return database.GetTemplateInsightsRow{}, err } @@ -1228,17 +1230,18 @@ func (q *querier) GetTemplateInsights(ctx context.Context, arg database.GetTempl } func (q *querier) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { - for _, templateID := range arg.TemplateIDs { - template, err := q.db.GetTemplateByID(ctx, templateID) + if len(arg.TemplateIDs) > 0 { + prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionUpdate, rbac.ResourceTemplate.Type) if err != nil { - return nil, err + return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err) } - - if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { + _, err = q.db.GetAuthorizedTemplates(ctx, database.GetTemplatesWithFilterParams{ + IDs: arg.TemplateIDs, + }, prep) + if err != nil { return nil, err } - } - if len(arg.TemplateIDs) == 0 { + } else { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { return nil, err } From 580826f9bbbe7c279a9534528967b5f52c118f53 Mon Sep 17 00:00:00 2001 From: Marcin Tojek Date: Thu, 3 Aug 2023 14:05:17 +0200 Subject: [PATCH 9/9] Revert "try: Use GetAuthorizedTemplates" This reverts commit 81146cda62e08c50d955e285836d18c59124ddc7. --- coderd/database/dbauthz/dbauthz.go | 45 ++++++++++++++---------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index cadb672a05ce6..ab865e2cc0f70 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1190,18 +1190,17 @@ func (q *querier) GetTemplateDAUs(ctx context.Context, arg database.GetTemplateD } func (q *querier) GetTemplateDailyInsights(ctx context.Context, arg database.GetTemplateDailyInsightsParams) ([]database.GetTemplateDailyInsightsRow, error) { - if len(arg.TemplateIDs) > 0 { - prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionUpdate, rbac.ResourceTemplate.Type) + for _, templateID := range arg.TemplateIDs { + template, err := q.db.GetTemplateByID(ctx, templateID) if err != nil { - return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err) + return nil, err } - _, err = q.db.GetAuthorizedTemplates(ctx, database.GetTemplatesWithFilterParams{ - IDs: arg.TemplateIDs, - }, prep) - if err != nil { + + if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { return nil, err } - } else { + } + if len(arg.TemplateIDs) == 0 { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { return nil, err } @@ -1210,18 +1209,17 @@ func (q *querier) GetTemplateDailyInsights(ctx context.Context, arg database.Get } func (q *querier) GetTemplateInsights(ctx context.Context, arg database.GetTemplateInsightsParams) (database.GetTemplateInsightsRow, error) { - if len(arg.TemplateIDs) > 0 { - prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionUpdate, rbac.ResourceTemplate.Type) + for _, templateID := range arg.TemplateIDs { + template, err := q.db.GetTemplateByID(ctx, templateID) if err != nil { - return database.GetTemplateInsightsRow{}, xerrors.Errorf("(dev error) prepare sql filter: %w", err) + return database.GetTemplateInsightsRow{}, err } - _, err = q.db.GetAuthorizedTemplates(ctx, database.GetTemplatesWithFilterParams{ - IDs: arg.TemplateIDs, - }, prep) - if err != nil { + + if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { return database.GetTemplateInsightsRow{}, err } - } else { + } + if len(arg.TemplateIDs) == 0 { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { return database.GetTemplateInsightsRow{}, err } @@ -1230,18 +1228,17 @@ func (q *querier) GetTemplateInsights(ctx context.Context, arg database.GetTempl } func (q *querier) GetTemplateParameterInsights(ctx context.Context, arg database.GetTemplateParameterInsightsParams) ([]database.GetTemplateParameterInsightsRow, error) { - if len(arg.TemplateIDs) > 0 { - prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionUpdate, rbac.ResourceTemplate.Type) + for _, templateID := range arg.TemplateIDs { + template, err := q.db.GetTemplateByID(ctx, templateID) if err != nil { - return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err) + return nil, err } - _, err = q.db.GetAuthorizedTemplates(ctx, database.GetTemplatesWithFilterParams{ - IDs: arg.TemplateIDs, - }, prep) - if err != nil { + + if err := q.authorizeContext(ctx, rbac.ActionUpdate, template); err != nil { return nil, err } - } else { + } + if len(arg.TemplateIDs) == 0 { if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceTemplate.All()); err != nil { return nil, err }