From e99761c5ca3f751be76a3994fc55472d4ca12741 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 13:21:53 -0500 Subject: [PATCH 1/2] chore: add organization search query to workspaces --- coderd/database/dbmem/dbmem.go | 6 +++ coderd/database/modelqueries.go | 1 + coderd/database/queries.sql.go | 62 +++++++++++---------- coderd/database/queries/workspaces.sql | 6 +++ coderd/searchquery/search.go | 74 +++++++++----------------- coderd/searchquery/search_test.go | 35 +++++++++++- coderd/workspaces.go | 2 +- 7 files changed, 108 insertions(+), 78 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 42fdd2b93f63e..de015a254c982 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -9964,6 +9964,12 @@ func (q *FakeQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg database. } } + if arg.OrganizationID != uuid.Nil { + if workspace.OrganizationID != arg.OrganizationID { + continue + } + } + if arg.OwnerUsername != "" { owner, err := q.getUserByIDNoLock(workspace.OwnerID) if err == nil && !strings.EqualFold(arg.OwnerUsername, owner.Username) { diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go index 83763ca55ec92..3dc894cda77bc 100644 --- a/coderd/database/modelqueries.go +++ b/coderd/database/modelqueries.go @@ -247,6 +247,7 @@ func (q *sqlQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg GetWorkspa arg.Deleted, arg.Status, arg.OwnerID, + arg.OrganizationID, pq.Array(arg.HasParam), arg.OwnerUsername, arg.TemplateName, diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 7a25b7f82533b..e681418e0a100 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -13708,9 +13708,15 @@ WHERE workspaces.owner_id = $5 ELSE true END + -- Filter by organization_id + AND CASE + WHEN $6 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + workspaces.organization_id = $6 + ELSE true + END -- Filter by build parameter -- @has_param will match any build that includes the parameter. - AND CASE WHEN array_length($6 :: text[], 1) > 0 THEN + AND CASE WHEN array_length($7 :: text[], 1) > 0 THEN EXISTS ( SELECT 1 @@ -13719,7 +13725,7 @@ WHERE WHERE workspace_build_parameters.workspace_build_id = latest_build.id AND -- ILIKE is case insensitive - workspace_build_parameters.name ILIKE ANY($6) + workspace_build_parameters.name ILIKE ANY($7) ) ELSE true END @@ -13744,40 +13750,40 @@ WHERE -- Filter by owner_name AND CASE - WHEN $7 :: text != '' THEN - workspaces.owner_id = (SELECT id FROM users WHERE lower(username) = lower($7) AND deleted = false) + WHEN $8 :: text != '' THEN + workspaces.owner_id = (SELECT id FROM users WHERE lower(username) = lower($8) AND deleted = false) ELSE true END -- Filter by template_name -- There can be more than 1 template with the same name across organizations. -- Use the organization filter to restrict to 1 org if needed. AND CASE - WHEN $8 :: text != '' THEN - workspaces.template_id = ANY(SELECT id FROM templates WHERE lower(name) = lower($8) AND deleted = false) + WHEN $9 :: text != '' THEN + workspaces.template_id = ANY(SELECT id FROM templates WHERE lower(name) = lower($9) AND deleted = false) ELSE true END -- Filter by template_ids AND CASE - WHEN array_length($9 :: uuid[], 1) > 0 THEN - workspaces.template_id = ANY($9) + WHEN array_length($10 :: uuid[], 1) > 0 THEN + workspaces.template_id = ANY($10) ELSE true END -- Filter by workspace_ids AND CASE - WHEN array_length($10 :: uuid[], 1) > 0 THEN - workspaces.id = ANY($10) + WHEN array_length($11 :: uuid[], 1) > 0 THEN + workspaces.id = ANY($11) ELSE true END -- Filter by name, matching on substring AND CASE - WHEN $11 :: text != '' THEN - workspaces.name ILIKE '%' || $11 || '%' + WHEN $12 :: text != '' THEN + workspaces.name ILIKE '%' || $12 || '%' ELSE true END -- Filter by agent status -- has-agent: is only applicable for workspaces in "start" transition. Stopped and deleted workspaces don't have agents. AND CASE - WHEN $12 :: text != '' THEN + WHEN $13 :: text != '' THEN ( SELECT COUNT(*) FROM @@ -13789,7 +13795,7 @@ WHERE WHERE workspace_resources.job_id = latest_build.provisioner_job_id AND latest_build.transition = 'start'::workspace_transition AND - $12 = ( + $13 = ( CASE WHEN workspace_agents.first_connected_at IS NULL THEN CASE @@ -13800,7 +13806,7 @@ WHERE END WHEN workspace_agents.disconnected_at > workspace_agents.last_connected_at THEN 'disconnected' - WHEN NOW() - workspace_agents.last_connected_at > INTERVAL '1 second' * $13 :: bigint THEN + WHEN NOW() - workspace_agents.last_connected_at > INTERVAL '1 second' * $14 :: bigint THEN 'disconnected' WHEN workspace_agents.last_connected_at IS NOT NULL THEN 'connected' @@ -13813,24 +13819,24 @@ WHERE END -- Filter by dormant workspaces. AND CASE - WHEN $14 :: boolean != 'false' THEN + WHEN $15 :: boolean != 'false' THEN dormant_at IS NOT NULL ELSE true END -- Filter by last_used AND CASE - WHEN $15 :: timestamp with time zone > '0001-01-01 00:00:00Z' THEN - workspaces.last_used_at <= $15 + WHEN $16 :: timestamp with time zone > '0001-01-01 00:00:00Z' THEN + workspaces.last_used_at <= $16 ELSE true END AND CASE - WHEN $16 :: timestamp with time zone > '0001-01-01 00:00:00Z' THEN - workspaces.last_used_at >= $16 + WHEN $17 :: timestamp with time zone > '0001-01-01 00:00:00Z' THEN + workspaces.last_used_at >= $17 ELSE true END AND CASE - WHEN $17 :: boolean IS NOT NULL THEN - (latest_build.template_version_id = template.active_version_id) = $17 :: boolean + WHEN $18 :: boolean IS NOT NULL THEN + (latest_build.template_version_id = template.active_version_id) = $18 :: boolean ELSE true END -- Authorize Filter clause will be injected below in GetAuthorizedWorkspaces @@ -13842,7 +13848,7 @@ WHERE filtered_workspaces fw ORDER BY -- To ensure that 'favorite' workspaces show up first in the list only for their owner. - CASE WHEN owner_id = $18 AND favorite THEN 0 ELSE 1 END ASC, + CASE WHEN owner_id = $19 AND favorite THEN 0 ELSE 1 END ASC, (latest_build_completed_at IS NOT NULL AND latest_build_canceled_at IS NULL AND latest_build_error IS NULL AND @@ -13851,11 +13857,11 @@ WHERE LOWER(name) ASC LIMIT CASE - WHEN $20 :: integer > 0 THEN - $20 + WHEN $21 :: integer > 0 THEN + $21 END OFFSET - $19 + $20 ), filtered_workspaces_order_with_summary AS ( SELECT fwo.id, fwo.created_at, fwo.updated_at, fwo.owner_id, fwo.organization_id, fwo.template_id, fwo.deleted, fwo.name, fwo.autostart_schedule, fwo.ttl, fwo.last_used_at, fwo.dormant_at, fwo.deleting_at, fwo.automatic_updates, fwo.favorite, fwo.template_name, fwo.template_version_id, fwo.template_version_name, fwo.username, fwo.latest_build_completed_at, fwo.latest_build_canceled_at, fwo.latest_build_error, fwo.latest_build_transition, fwo.latest_build_status @@ -13891,7 +13897,7 @@ WHERE 'start'::workspace_transition, -- latest_build_transition 'unknown'::provisioner_job_status -- latest_build_status WHERE - $21 :: boolean = true + $22 :: boolean = true ), total_count AS ( SELECT count(*) AS count @@ -13913,6 +13919,7 @@ type GetWorkspacesParams struct { Deleted bool `db:"deleted" json:"deleted"` Status string `db:"status" json:"status"` OwnerID uuid.UUID `db:"owner_id" json:"owner_id"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` HasParam []string `db:"has_param" json:"has_param"` OwnerUsername string `db:"owner_username" json:"owner_username"` TemplateName string `db:"template_name" json:"template_name"` @@ -13969,6 +13976,7 @@ func (q *sqlQuerier) GetWorkspaces(ctx context.Context, arg GetWorkspacesParams) arg.Deleted, arg.Status, arg.OwnerID, + arg.OrganizationID, pq.Array(arg.HasParam), arg.OwnerUsername, arg.TemplateName, diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index 9b36a99b8c396..42d7a5247f1b5 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -195,6 +195,12 @@ WHERE workspaces.owner_id = @owner_id ELSE true END + -- Filter by organization_id + AND CASE + WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN + workspaces.organization_id = @organization_id + ELSE true + END -- Filter by build parameter -- @has_param will match any build that includes the parameter. AND CASE WHEN array_length(@has_param :: text[], 1) > 0 THEN diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index 500eae2723336..707f32bfc7d32 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -39,6 +39,7 @@ func AuditLogs(ctx context.Context, db database.Store, query string) (database.G Email: parser.String(values, "", "email"), DateFrom: parser.Time(values, time.Time{}, "date_from", dateLayout), DateTo: parser.Time(values, time.Time{}, "date_to", dateLayout), + OrganizationID: parseOrganization(ctx, db, parser, values, "organization"), ResourceType: string(httpapi.ParseCustom(parser, values, "", "resource_type", httpapi.ParseEnum[database.ResourceType])), Action: string(httpapi.ParseCustom(parser, values, "", "action", httpapi.ParseEnum[database.AuditAction])), BuildReason: string(httpapi.ParseCustom(parser, values, "", "build_reason", httpapi.ParseEnum[database.BuildReason])), @@ -47,27 +48,6 @@ func AuditLogs(ctx context.Context, db database.Store, query string) (database.G filter.DateTo = filter.DateTo.Add(23*time.Hour + 59*time.Minute + 59*time.Second) } - // Convert the "organization" parameter to an organization uuid. This can require - // a database lookup. - organizationArg := parser.String(values, "", "organization") - if organizationArg != "" { - organizationID, err := uuid.Parse(organizationArg) - if err == nil { - filter.OrganizationID = organizationID - } else { - // Organization could be a name - organization, err := db.GetOrganizationByName(ctx, organizationArg) - if err != nil { - parser.Errors = append(parser.Errors, codersdk.ValidationError{ - Field: "organization", - Detail: fmt.Sprintf("Organization %q either does not exist, or you are unauthorized to view it", organizationArg), - }) - } else { - filter.OrganizationID = organization.ID - } - } - } - parser.ErrorExcessParams(values) return filter, parser.Errors } @@ -95,7 +75,7 @@ func Users(query string) (database.GetUsersParams, []codersdk.ValidationError) { return filter, parser.Errors } -func Workspaces(query string, page codersdk.Pagination, agentInactiveDisconnectTimeout time.Duration) (database.GetWorkspacesParams, []codersdk.ValidationError) { +func Workspaces(ctx context.Context, db database.Store, query string, page codersdk.Pagination, agentInactiveDisconnectTimeout time.Duration) (database.GetWorkspacesParams, []codersdk.ValidationError) { filter := database.GetWorkspacesParams{ AgentInactiveDisconnectTimeoutSeconds: int64(agentInactiveDisconnectTimeout.Seconds()), @@ -145,6 +125,7 @@ func Workspaces(query string, page codersdk.Pagination, agentInactiveDisconnectT // which will return all workspaces. Valid: values.Has("outdated"), } + filter.OrganizationID = parseOrganization(ctx, db, parser, values, "organization") type paramMatch struct { name string @@ -198,32 +179,12 @@ func Templates(ctx context.Context, db database.Store, query string) (database.G parser := httpapi.NewQueryParamParser() filter := database.GetTemplatesWithFilterParams{ - Deleted: parser.Boolean(values, false, "deleted"), - ExactName: parser.String(values, "", "exact_name"), - FuzzyName: parser.String(values, "", "name"), - IDs: parser.UUIDs(values, []uuid.UUID{}, "ids"), - Deprecated: parser.NullableBoolean(values, sql.NullBool{}, "deprecated"), - } - - // Convert the "organization" parameter to an organization uuid. This can require - // a database lookup. - organizationArg := parser.String(values, "", "organization") - if organizationArg != "" { - organizationID, err := uuid.Parse(organizationArg) - if err == nil { - filter.OrganizationID = organizationID - } else { - // Organization could be a name - organization, err := db.GetOrganizationByName(ctx, organizationArg) - if err != nil { - parser.Errors = append(parser.Errors, codersdk.ValidationError{ - Field: "organization", - Detail: fmt.Sprintf("Organization %q either does not exist, or you are unauthorized to view it", organizationArg), - }) - } else { - filter.OrganizationID = organization.ID - } - } + Deleted: parser.Boolean(values, false, "deleted"), + ExactName: parser.String(values, "", "exact_name"), + FuzzyName: parser.String(values, "", "name"), + IDs: parser.UUIDs(values, []uuid.UUID{}, "ids"), + Deprecated: parser.NullableBoolean(values, sql.NullBool{}, "deprecated"), + OrganizationID: parseOrganization(ctx, db, parser, values, "organization"), } parser.ErrorExcessParams(values) @@ -271,6 +232,23 @@ func searchTerms(query string, defaultKey func(term string, values url.Values) e return searchValues, nil } +func parseOrganization(ctx context.Context, db database.Store, parser *httpapi.QueryParamParser, vals url.Values, queryParam string) uuid.UUID { + return httpapi.ParseCustom(parser, vals, uuid.Nil, queryParam, func(v string) (uuid.UUID, error) { + if v == "" { + return uuid.Nil, nil + } + organizationID, err := uuid.Parse(v) + if err == nil { + return organizationID, nil + } + organization, err := db.GetOrganizationByName(ctx, v) + if err != nil { + return uuid.Nil, xerrors.Errorf("organization %q either does not exist, or you are unauthorized to view it", v) + } + return organization.ID, nil + }) +} + // splitQueryParameterByDelimiter takes a query string and splits it into the individual elements // of the query. Each element is separated by a delimiter. All quoted strings are // kept as a single element. diff --git a/coderd/searchquery/search_test.go b/coderd/searchquery/search_test.go index 98f7bed13bac2..675f98ebcba04 100644 --- a/coderd/searchquery/search_test.go +++ b/coderd/searchquery/search_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/coderd/searchquery" "github.com/coder/coder/v2/codersdk" @@ -25,6 +26,7 @@ func TestSearchWorkspace(t *testing.T) { Query string Expected database.GetWorkspacesParams ExpectedErrorContains string + Setup func(t *testing.T, db database.Store) }{ { Name: "Empty", @@ -195,6 +197,31 @@ func TestSearchWorkspace(t *testing.T) { ParamValues: []string{"bar"}, }, }, + { + Name: "Organization", + Query: `organization:4fe722f0-49bc-4a90-a3eb-4ac439bfce20`, + Setup: func(t *testing.T, db database.Store) { + dbgen.Organization(t, db, database.Organization{ + ID: uuid.MustParse("4fe722f0-49bc-4a90-a3eb-4ac439bfce20"), + }) + }, + Expected: database.GetWorkspacesParams{ + OrganizationID: uuid.MustParse("4fe722f0-49bc-4a90-a3eb-4ac439bfce20"), + }, + }, + { + Name: "OrganizationByName", + Query: `organization:foobar`, + Setup: func(t *testing.T, db database.Store) { + dbgen.Organization(t, db, database.Organization{ + ID: uuid.MustParse("08eb6715-02f8-45c5-b86d-03786fcfbb4e"), + Name: "foobar", + }) + }, + Expected: database.GetWorkspacesParams{ + OrganizationID: uuid.MustParse("08eb6715-02f8-45c5-b86d-03786fcfbb4e"), + }, + }, // Failures { @@ -243,7 +270,11 @@ func TestSearchWorkspace(t *testing.T) { c := c t.Run(c.Name, func(t *testing.T) { t.Parallel() - values, errs := searchquery.Workspaces(c.Query, codersdk.Pagination{}, 0) + db := dbmem.New() + if c.Setup != nil { + c.Setup(t, db) + } + values, errs := searchquery.Workspaces(context.Background(), db, c.Query, codersdk.Pagination{}, 0) if c.ExpectedErrorContains != "" { assert.True(t, len(errs) > 0, "expect some errors") var s strings.Builder @@ -270,7 +301,7 @@ func TestSearchWorkspace(t *testing.T) { query := `` timeout := 1337 * time.Second - values, errs := searchquery.Workspaces(query, codersdk.Pagination{}, timeout) + values, errs := searchquery.Workspaces(context.Background(), dbmem.New(), query, codersdk.Pagination{}, timeout) require.Empty(t, errs) require.Equal(t, int64(timeout.Seconds()), values.AgentInactiveDisconnectTimeoutSeconds) }) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 4215f32746a4b..62193b6d673f0 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -150,7 +150,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } queryStr := r.URL.Query().Get("q") - filter, errs := searchquery.Workspaces(queryStr, page, api.AgentInactiveDisconnectTimeout) + filter, errs := searchquery.Workspaces(ctx, api.Database, queryStr, page, api.AgentInactiveDisconnectTimeout) if len(errs) > 0 { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid workspace search query.", From 08a642fbe8042cea5693b04b7d5ee1550de99b3e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 28 Aug 2024 14:45:51 -0500 Subject: [PATCH 2/2] chore: add comment to use mock instead --- coderd/searchquery/search_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/searchquery/search_test.go b/coderd/searchquery/search_test.go index 675f98ebcba04..91d285afbd8ec 100644 --- a/coderd/searchquery/search_test.go +++ b/coderd/searchquery/search_test.go @@ -270,6 +270,7 @@ func TestSearchWorkspace(t *testing.T) { c := c t.Run(c.Name, func(t *testing.T) { t.Parallel() + // TODO: Replace this with the mock database. db := dbmem.New() if c.Setup != nil { c.Setup(t, db)