From 1a14a8c71a2686df14ea0e042e6b304a82469b9d Mon Sep 17 00:00:00 2001 From: Garrett Date: Tue, 7 Jun 2022 19:35:29 +0000 Subject: [PATCH 01/35] feat: add support for template in workspace filter --- coderd/database/databasefake/databasefake.go | 20 +++++++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 62 +++++++++++++++++++- coderd/database/queries/templates.sql | 9 +++ coderd/database/queries/workspaces.sql | 6 ++ coderd/workspaces.go | 58 ++++++++++-------- codersdk/workspaces.go | 7 ++- 7 files changed, 134 insertions(+), 29 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 78f85e71c597c..59c7e7f05be6a 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -761,6 +761,26 @@ func (q *fakeQuerier) UpdateTemplateMetaByID(_ context.Context, arg database.Upd return sql.ErrNoRows } +func (q *fakeQuerier) GetTemplatesByName(_ context.Context, arg database.GetTemplatesByNameParams) ([]database.Template, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + var templates []database.Template + for _, template := range q.templates { + if !strings.EqualFold(template.Name, arg.Name) { + continue + } + if template.Deleted != arg.Deleted { + continue + } + templates = append(templates, template) + } + if len(templates) > 0 { + return templates, nil + } + + return nil, sql.ErrNoRows +} + func (q *fakeQuerier) GetTemplateVersionsByTemplateID(_ context.Context, arg database.GetTemplateVersionsByTemplateIDParams) (version []database.TemplateVersion, err error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 59e8f4577ef8e..0ace0fb587a45 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -54,6 +54,7 @@ type querier interface { GetTemplateVersionByTemplateIDAndName(ctx context.Context, arg GetTemplateVersionByTemplateIDAndNameParams) (TemplateVersion, error) GetTemplateVersionsByTemplateID(ctx context.Context, arg GetTemplateVersionsByTemplateIDParams) ([]TemplateVersion, error) GetTemplatesByIDs(ctx context.Context, ids []uuid.UUID) ([]Template, error) + GetTemplatesByName(ctx context.Context, arg GetTemplatesByNameParams) ([]Template, error) GetTemplatesByOrganization(ctx context.Context, arg GetTemplatesByOrganizationParams) ([]Template, error) GetUserByEmailOrUsername(ctx context.Context, arg GetUserByEmailOrUsernameParams) (User, error) GetUserByID(ctx context.Context, id uuid.UUID) (User, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 06e5f61105269..e848cae37d737 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1713,6 +1713,56 @@ func (q *sqlQuerier) GetTemplatesByIDs(ctx context.Context, ids []uuid.UUID) ([] return items, nil } +const getTemplatesByName = `-- name: GetTemplatesByName :many +SELECT + id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, max_ttl, min_autostart_interval +FROM + templates +WHERE + deleted = $1 + AND LOWER("name") = LOWER($2) +` + +type GetTemplatesByNameParams struct { + Deleted bool `db:"deleted" json:"deleted"` + Name string `db:"name" json:"name"` +} + +func (q *sqlQuerier) GetTemplatesByName(ctx context.Context, arg GetTemplatesByNameParams) ([]Template, error) { + rows, err := q.db.QueryContext(ctx, getTemplatesByName, arg.Deleted, arg.Name) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Template + for rows.Next() { + var i Template + if err := rows.Scan( + &i.ID, + &i.CreatedAt, + &i.UpdatedAt, + &i.OrganizationID, + &i.Deleted, + &i.Name, + &i.Provisioner, + &i.ActiveVersionID, + &i.Description, + &i.MaxTtl, + &i.MinAutostartInterval, + ); 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 getTemplatesByOrganization = `-- name: GetTemplatesByOrganization :many SELECT id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, max_ttl, min_autostart_interval @@ -3707,10 +3757,16 @@ WHERE owner_id = $3 ELSE true END + -- Filter by template_id + AND CASE + WHEN $4 :: uuid != '00000000-00000000-00000000-00000000' THEN + template_id = $4 + ELSE true + END -- Filter by name, matching on substring AND CASE - WHEN $4 :: text != '' THEN - LOWER(name) LIKE '%' || LOWER($4) || '%' + WHEN $5 :: text != '' THEN + LOWER(name) LIKE '%' || LOWER($5) || '%' ELSE true END ` @@ -3719,6 +3775,7 @@ type GetWorkspacesWithFilterParams struct { Deleted bool `db:"deleted" json:"deleted"` OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` OwnerID uuid.UUID `db:"owner_id" json:"owner_id"` + TemplateID uuid.UUID `db:"template_id" json:"template_id"` Name string `db:"name" json:"name"` } @@ -3727,6 +3784,7 @@ func (q *sqlQuerier) GetWorkspacesWithFilter(ctx context.Context, arg GetWorkspa arg.Deleted, arg.OrganizationID, arg.OwnerID, + arg.TemplateID, arg.Name, ) if err != nil { diff --git a/coderd/database/queries/templates.sql b/coderd/database/queries/templates.sql index ffc0b9dabfb38..52564413f9980 100644 --- a/coderd/database/queries/templates.sql +++ b/coderd/database/queries/templates.sql @@ -28,6 +28,15 @@ WHERE LIMIT 1; +-- name: GetTemplatesByName :many +SELECT + * +FROM + templates +WHERE + deleted = @deleted + AND LOWER("name") = LOWER(@name); + -- name: GetTemplatesByOrganization :many SELECT * diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index 8c17c323b091d..4618fab2e5863 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -28,6 +28,12 @@ WHERE owner_id = @owner_id ELSE true END + -- Filter by template_id + AND CASE + WHEN @template_id :: uuid != '00000000-00000000-00000000-00000000' THEN + template_id = @template_id + ELSE true + END -- Filter by name, matching on substring AND CASE WHEN @name :: text != '' THEN diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 079f880ed44f3..8cd56614709d8 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -103,48 +103,56 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { // Optional filters with query params func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) + filter := database.GetWorkspacesWithFilterParams{Deleted: false} - // Empty strings mean no filter orgFilter := r.URL.Query().Get("organization_id") - ownerFilter := r.URL.Query().Get("owner") - nameFilter := r.URL.Query().Get("name") - - filter := database.GetWorkspacesWithFilterParams{Deleted: false} if orgFilter != "" { orgID, err := uuid.Parse(orgFilter) if err == nil { filter.OrganizationID = orgID } } + + ownerFilter := r.URL.Query().Get("owner") if ownerFilter == "me" { filter.OwnerID = apiKey.UserID } else if ownerFilter != "" { - userID, err := uuid.Parse(ownerFilter) - if err != nil { - // Maybe it's a username - user, err := api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{ - // Why not just accept 1 arg and use it for both in the sql? - Username: ownerFilter, - Email: ownerFilter, - }) - if err == nil { - filter.OwnerID = user.ID - } - } else { - filter.OwnerID = userID + user, err := api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{ + Username: ownerFilter, + }) + if err == nil { + filter.OwnerID = user.ID } } + + templateFilter := r.URL.Query().Get("template") + var templates []database.Template + if templateFilter != "" { + ts, err := api.Database.GetTemplatesByName(r.Context(), database.GetTemplatesByNameParams{ + Name: templateFilter, + }) + if err == nil { + templates = ts + } + } + + nameFilter := r.URL.Query().Get("name") if nameFilter != "" { filter.Name = nameFilter } - workspaces, err := api.Database.GetWorkspacesWithFilter(r.Context(), filter) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: "Internal error fetching workspaces.", - Detail: err.Error(), - }) - return + var workspaces []database.Workspace + for _, template := range templates { + filter.TemplateID = template.ID + ws, err := api.Database.GetWorkspacesWithFilter(r.Context(), filter) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: "Internal error fetching workspaces.", + Detail: err.Error(), + }) + return + } + workspaces = append(workspaces, ws...) } // Only return workspaces the user can read diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index 67db13a422459..23577195450be 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -218,9 +218,12 @@ func (c *Client) PutExtendWorkspace(ctx context.Context, id uuid.UUID, req PutEx type WorkspaceFilter struct { OrganizationID uuid.UUID `json:"organization_id,omitempty"` - // Owner can be a user_id (uuid), "me", or a username + // Owner can be "me" or a username Owner string `json:"owner,omitempty"` - Name string `json:"name,omitempty"` + // Template is a template name + Template string `json:"template,omitempty"` + // Name will return partial matches + Name string `json:"name,omitempty"` } // asRequestOption returns a function that can be used in (*Client).Request. From e3bd559735299766f4c1c5a4d543b15add4abc4f Mon Sep 17 00:00:00 2001 From: Garrett Date: Wed, 8 Jun 2022 23:28:50 +0000 Subject: [PATCH 02/35] get working --- coderd/database/databasefake/databasefake.go | 12 +++++++ coderd/database/queries.sql.go | 18 +++++------ coderd/database/queries/workspaces.sql | 6 ++-- coderd/workspaces.go | 34 +++++++++----------- coderd/workspaces_test.go | 24 ++++++++++++++ codersdk/workspaces.go | 3 ++ site/src/api/typesGenerated.ts | 3 +- 7 files changed, 68 insertions(+), 32 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 59c7e7f05be6a..6626aa9ec6323 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -333,6 +333,18 @@ func (q *fakeQuerier) GetWorkspacesWithFilter(_ context.Context, arg database.Ge if arg.Name != "" && !strings.Contains(workspace.Name, arg.Name) { continue } + if len(arg.TemplateIds) > 0 { + match := false + for _, id := range arg.TemplateIds { + if workspace.TemplateID == id { + match = true + break + } + } + if !match { + continue + } + } workspaces = append(workspaces, workspace) } diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index e848cae37d737..447102d8f8114 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3757,10 +3757,10 @@ WHERE owner_id = $3 ELSE true END - -- Filter by template_id + -- Filter by template_ids AND CASE - WHEN $4 :: uuid != '00000000-00000000-00000000-00000000' THEN - template_id = $4 + WHEN array_length($4 :: uuid[], 1) > 0 THEN + template_id = ANY($4) ELSE true END -- Filter by name, matching on substring @@ -3772,11 +3772,11 @@ WHERE ` type GetWorkspacesWithFilterParams struct { - Deleted bool `db:"deleted" json:"deleted"` - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - OwnerID uuid.UUID `db:"owner_id" json:"owner_id"` - TemplateID uuid.UUID `db:"template_id" json:"template_id"` - Name string `db:"name" json:"name"` + Deleted bool `db:"deleted" json:"deleted"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + OwnerID uuid.UUID `db:"owner_id" json:"owner_id"` + TemplateIds []uuid.UUID `db:"template_ids" json:"template_ids"` + Name string `db:"name" json:"name"` } func (q *sqlQuerier) GetWorkspacesWithFilter(ctx context.Context, arg GetWorkspacesWithFilterParams) ([]Workspace, error) { @@ -3784,7 +3784,7 @@ func (q *sqlQuerier) GetWorkspacesWithFilter(ctx context.Context, arg GetWorkspa arg.Deleted, arg.OrganizationID, arg.OwnerID, - arg.TemplateID, + pq.Array(arg.TemplateIds), arg.Name, ) if err != nil { diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index 4618fab2e5863..0d3151c8c46d3 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -28,10 +28,10 @@ WHERE owner_id = @owner_id ELSE true END - -- Filter by template_id + -- Filter by template_ids AND CASE - WHEN @template_id :: uuid != '00000000-00000000-00000000-00000000' THEN - template_id = @template_id + WHEN array_length(@template_ids :: uuid[], 1) > 0 THEN + template_id = ANY(@template_ids) ELSE true END -- Filter by name, matching on substring diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 8cd56614709d8..f4a233f6034a9 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -125,34 +125,30 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } } + nameFilter := r.URL.Query().Get("name") + if nameFilter != "" { + filter.Name = nameFilter + } + templateFilter := r.URL.Query().Get("template") - var templates []database.Template if templateFilter != "" { ts, err := api.Database.GetTemplatesByName(r.Context(), database.GetTemplatesByNameParams{ Name: templateFilter, }) if err == nil { - templates = ts + for _, t := range ts { + filter.TemplateIds = append(filter.TemplateIds, t.ID) + } } } - nameFilter := r.URL.Query().Get("name") - if nameFilter != "" { - filter.Name = nameFilter - } - - var workspaces []database.Workspace - for _, template := range templates { - filter.TemplateID = template.ID - ws, err := api.Database.GetWorkspacesWithFilter(r.Context(), filter) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: "Internal error fetching workspaces.", - Detail: err.Error(), - }) - return - } - workspaces = append(workspaces, ws...) + workspaces, err := api.Database.GetWorkspacesWithFilter(r.Context(), filter) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: "Internal error fetching workspaces.", + Detail: err.Error(), + }) + return } // Only return workspaces the user can read diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 73fed89c80c79..9253f8f2548f6 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -339,6 +339,30 @@ func TestWorkspaceFilter(t *testing.T) { require.NoError(t, err) require.Len(t, ws, 0) }) + t.Run("Template", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + template2 := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + _ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template2.ID) + + // empty + ws, err := client.Workspaces(context.Background(), codersdk.WorkspaceFilter{}) + require.NoError(t, err) + require.Len(t, ws, 2) + + // single template + ws, err = client.Workspaces(context.Background(), codersdk.WorkspaceFilter{ + Template: template.Name, + }) + require.NoError(t, err) + require.Len(t, ws, 1) + require.Equal(t, workspace.ID, ws[0].ID) + }) } func TestPostWorkspaceBuild(t *testing.T) { diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index 23577195450be..cd4dd3d001765 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -240,6 +240,9 @@ func (f WorkspaceFilter) asRequestOption() requestOption { if f.Name != "" { q.Set("name", f.Name) } + if f.Template != "" { + q.Set("template", f.Template) + } r.URL.RawQuery = q.Encode() } } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index f565f6f4e50ff..c08631373b9f3 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -463,7 +463,7 @@ export interface WorkspaceBuildsRequest extends Pagination { readonly WorkspaceID: string } -// From codersdk/workspaces.go:261:6 +// From codersdk/workspaces.go:264:6 export interface WorkspaceByOwnerAndNameParams { readonly include_deleted?: boolean } @@ -472,6 +472,7 @@ export interface WorkspaceByOwnerAndNameParams { export interface WorkspaceFilter { readonly organization_id?: string readonly owner?: string + readonly template?: string readonly name?: string } From 91d7d84ebb48709e8f1ee6cc6b067acf52835a9a Mon Sep 17 00:00:00 2001 From: Garrett Date: Wed, 8 Jun 2022 23:32:59 +0000 Subject: [PATCH 03/35] make gen --- site/src/api/typesGenerated.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index c08631373b9f3..e4c992b261bea 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -463,7 +463,7 @@ export interface WorkspaceBuildsRequest extends Pagination { readonly WorkspaceID: string } -// From codersdk/workspaces.go:264:6 +// From codersdk/workspaces.go:267:6 export interface WorkspaceByOwnerAndNameParams { readonly include_deleted?: boolean } From 2941b58b4b6c93f285f4a800b604b5f95a70df2d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 21:58:25 -0500 Subject: [PATCH 04/35] feat: Implement workspace search filter to support names --- coderd/database/queries.sql.go | 36 ++++++--- coderd/database/queries/workspaces.sql | 34 +++++--- coderd/httpapi/queryparams.go | 105 +++++++++++++++++++++++++ coderd/httpapi/search.go | 64 +++++++++++++++ coderd/workspaces.go | 72 ++++++++++------- 5 files changed, 264 insertions(+), 47 deletions(-) create mode 100644 coderd/httpapi/queryparams.go create mode 100644 coderd/httpapi/search.go diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 447102d8f8114..aba1959121472 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3744,7 +3744,7 @@ FROM workspaces WHERE -- Optionally include deleted workspaces - deleted = $1 + workspaces.deleted = $1 -- Filter by organization_id AND CASE WHEN $2 :: uuid != '00000000-00000000-00000000-00000000' THEN @@ -3753,20 +3753,34 @@ WHERE END -- Filter by owner_id AND CASE - WHEN $3 :: uuid != '00000000-00000000-00000000-00000000' THEN - owner_id = $3 - ELSE true + WHEN $3 :: uuid != '00000000-00000000-00000000-00000000' THEN + owner_id = $3 + ELSE true + END + -- Filter by owner_name + AND CASE + WHEN $4 :: text != '' THEN + owner_id = (SELECT id FROM users WHERE username = $4) + 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 $5 :: text != '' THEN + template_id = (SELECT id FROM templates WHERE name = $5) + ELSE true END -- Filter by template_ids AND CASE - WHEN array_length($4 :: uuid[], 1) > 0 THEN - template_id = ANY($4) - ELSE true + WHEN array_length($6 :: uuid[], 1) > 0 THEN + template_id = ANY($6) + ELSE true END -- Filter by name, matching on substring AND CASE - WHEN $5 :: text != '' THEN - LOWER(name) LIKE '%' || LOWER($5) || '%' + WHEN $7 :: text != '' THEN + LOWER(name) LIKE '%' || LOWER($7) || '%' ELSE true END ` @@ -3775,6 +3789,8 @@ type GetWorkspacesWithFilterParams struct { Deleted bool `db:"deleted" json:"deleted"` OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` OwnerID uuid.UUID `db:"owner_id" json:"owner_id"` + OwnerUsername string `db:"owner_username" json:"owner_username"` + TemplateName string `db:"template_name" json:"template_name"` TemplateIds []uuid.UUID `db:"template_ids" json:"template_ids"` Name string `db:"name" json:"name"` } @@ -3784,6 +3800,8 @@ func (q *sqlQuerier) GetWorkspacesWithFilter(ctx context.Context, arg GetWorkspa arg.Deleted, arg.OrganizationID, arg.OwnerID, + arg.OwnerUsername, + arg.TemplateName, pq.Array(arg.TemplateIds), arg.Name, ) diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index 0d3151c8c46d3..45c86f7afe7ba 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -15,7 +15,7 @@ FROM workspaces WHERE -- Optionally include deleted workspaces - deleted = @deleted + workspaces.deleted = @deleted -- Filter by organization_id AND CASE WHEN @organization_id :: uuid != '00000000-00000000-00000000-00000000' THEN @@ -24,21 +24,35 @@ WHERE END -- Filter by owner_id AND CASE - WHEN @owner_id :: uuid != '00000000-00000000-00000000-00000000' THEN - owner_id = @owner_id - ELSE true + WHEN @owner_id :: uuid != '00000000-00000000-00000000-00000000' THEN + owner_id = @owner_id + ELSE true + END + -- Filter by owner_name + AND CASE + WHEN @owner_username :: text != '' THEN + owner_id = (SELECT id FROM users WHERE username = @owner_username) + 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 @template_name :: text != '' THEN + template_id = (SELECT id FROM templates WHERE name = @template_name) + ELSE true END -- Filter by template_ids AND CASE - WHEN array_length(@template_ids :: uuid[], 1) > 0 THEN - template_id = ANY(@template_ids) - ELSE true + WHEN array_length(@template_ids :: uuid[], 1) > 0 THEN + template_id = ANY(@template_ids) + ELSE true END -- Filter by name, matching on substring AND CASE - WHEN @name :: text != '' THEN - LOWER(name) LIKE '%' || LOWER(@name) || '%' - ELSE true + WHEN @name :: text != '' THEN + LOWER(name) LIKE '%' || LOWER(@name) || '%' + ELSE true END ; diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go new file mode 100644 index 0000000000000..81a3d1a9d5fca --- /dev/null +++ b/coderd/httpapi/queryparams.go @@ -0,0 +1,105 @@ +package httpapi + +import ( + "fmt" + "net/http" + "strings" + + "github.com/google/uuid" + + "golang.org/x/xerrors" +) + +// QueryParamParser is a helper for parsing all query params and gathering all +// errors in 1 sweep. This means all invalid fields are returned at once, +// rather than only returning the first error +type QueryParamParser struct { + errors []Error +} + +func NewQueryParamParser() *QueryParamParser { + return &QueryParamParser{ + errors: []Error{}, + } +} + +// ValidationErrors is the set of errors to return via the API. If the length +// of this set is 0, there was no errors. +func (p QueryParamParser) ValidationErrors() []Error { + return p.errors +} + +func (p *QueryParamParser) ParseUUIDorMe(r *http.Request, def uuid.UUID, me uuid.UUID, queryParam string) uuid.UUID { + if r.URL.Query().Get(queryParam) == "me" { + return me + } + + v, err := parse(r, uuid.Parse, def, queryParam) + if err != nil { + p.errors = append(p.errors, Error{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q must be a valid uuid", queryParam), + }) + } + return v +} + +func (p *QueryParamParser) ParseUUID(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { + v, err := parse(r, uuid.Parse, def, queryParam) + if err != nil { + p.errors = append(p.errors, Error{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q must be a valid uuid", queryParam), + }) + } + return v +} + +func (p *QueryParamParser) ParseUUIDArray(r *http.Request, def []uuid.UUID, queryParam string) []uuid.UUID { + v, err := parse(r, func(v string) ([]uuid.UUID, error) { + var badValues []string + strs := strings.Split(v, ",") + ids := make([]uuid.UUID, 0, len(strs)) + for _, s := range strs { + id, err := uuid.Parse(s) + if err != nil { + badValues = append(badValues, v) + continue + } + ids = append(ids, id) + } + + if len(badValues) > 0 { + return nil, xerrors.Errorf("%s", strings.Join(badValues, ",")) + } + return ids, nil + }, def, queryParam) + if err != nil { + p.errors = append(p.errors, Error{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q has invalid uuids: %q", err.Error()), + }) + } + return v +} + +func (p *QueryParamParser) ParseString(r *http.Request, def string, queryParam string) string { + v, err := parse(r, func(v string) (string, error) { + return v, nil + }, def, queryParam) + if err != nil { + p.errors = append(p.errors, Error{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q must be a valid string", queryParam), + }) + } + return v +} + +func parse[T any](r *http.Request, parse func(v string) (T, error), def T, queryParam string) (T, error) { + if !r.URL.Query().Has(queryParam) { + return def, nil + } + str := r.URL.Query().Get(queryParam) + return parse(str) +} diff --git a/coderd/httpapi/search.go b/coderd/httpapi/search.go new file mode 100644 index 0000000000000..aafeda1bcd3e7 --- /dev/null +++ b/coderd/httpapi/search.go @@ -0,0 +1,64 @@ +package httpapi + +import ( + "strings" + + "golang.org/x/xerrors" +) + +// WorkspaceSearchQuery takes a query string and breaks it into it's query +// params as a set of key=value. +func WorkspaceSearchQuery(query string) (map[string]string, error) { + searchParams := make(map[string]string) + elements := queryElements(query) + for _, element := range elements { + parts := strings.Split(element, ":") + switch len(parts) { + case 1: + // No key:value pair. It is a workspace name, and maybe includes an owner + parts = strings.Split(element, "/") + switch len(parts) { + case 1: + searchParams["name"] = parts[0] + case 2: + searchParams["owner"] = parts[0] + searchParams["name"] = parts[1] + default: + return nil, xerrors.Errorf("Query element %q can only contain 1 '/'", element) + } + case 2: + searchParams[parts[0]] = parts[1] + default: + return nil, xerrors.Errorf("Query element %q can only contain 1 ':'", element) + } + } + + return searchParams, nil +} + +// queryElements takes a query string and splits it into the individual elements +// of the query. Each element is separated by a space. All quoted strings are +// kept as a single element. +func queryElements(query string) []string { + var parts []string + + quoted := false + var current strings.Builder + for _, c := range query { + switch c { + case '"': + quoted = !quoted + case ' ': + if quoted { + current.WriteRune(c) + } else { + parts = append(parts, current.String()) + current = strings.Builder{} + } + default: + current.WriteRune(c) + } + } + parts = append(parts, current.String()) + return parts +} diff --git a/coderd/workspaces.go b/coderd/workspaces.go index f4a233f6034a9..98f49cdba51df 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -103,43 +103,59 @@ func (api *API) workspace(rw http.ResponseWriter, r *http.Request) { // Optional filters with query params func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) - filter := database.GetWorkspacesWithFilterParams{Deleted: false} - orgFilter := r.URL.Query().Get("organization_id") - if orgFilter != "" { - orgID, err := uuid.Parse(orgFilter) - if err == nil { - filter.OrganizationID = orgID - } + queryStr := r.URL.Query().Get("q") + values, err := httpapi.WorkspaceSearchQuery(queryStr) + if err != nil { + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: "Invalid workspace search query", + Validations: []httpapi.Error{ + {Field: "q", Detail: err.Error()}, + }, + }) + return } - ownerFilter := r.URL.Query().Get("owner") - if ownerFilter == "me" { - filter.OwnerID = apiKey.UserID - } else if ownerFilter != "" { - user, err := api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{ - Username: ownerFilter, - }) - if err == nil { - filter.OwnerID = user.ID + // Set all the query params from the "q" field. + for k, v := range values { + // Do not allow overriding if the user also set query param fields + // outside the query string. + if r.URL.Query().Has(k) { + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: fmt.Sprintf("Workspace filter %q cannot be set twice. In query params %q and %q", k, k, "q"), + }) + return } + r.URL.Query().Set(k, v) } - nameFilter := r.URL.Query().Get("name") - if nameFilter != "" { - filter.Name = nameFilter + parser := httpapi.NewQueryParamParser() + filter := database.GetWorkspacesWithFilterParams{ + Deleted: false, + OrganizationID: parser.ParseUUID(r, uuid.Nil, "organization_id"), + OwnerID: parser.ParseUUIDorMe(r, uuid.Nil, apiKey.UserID, "owner_id"), + OwnerUsername: parser.ParseString(r, "", "owner"), + TemplateName: parser.ParseString(r, "", "template"), + TemplateIds: parser.ParseUUIDArray(r, []uuid.UUID{}, "template_ids"), + Name: parser.ParseString(r, "", "name"), } - - templateFilter := r.URL.Query().Get("template") - if templateFilter != "" { - ts, err := api.Database.GetTemplatesByName(r.Context(), database.GetTemplatesByNameParams{ - Name: templateFilter, + if len(parser.ValidationErrors()) > 0 { + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: fmt.Sprintf("Query parameters have invalid values"), + Validations: parser.ValidationErrors(), }) - if err == nil { - for _, t := range ts { - filter.TemplateIds = append(filter.TemplateIds, t.ID) - } + return + } + + if filter.OwnerUsername == "me" { + if !(filter.OwnerID == uuid.Nil || filter.OwnerID == apiKey.UserID) { + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: fmt.Sprintf("Cannot set both \"me\" in \"owner_name\" and use \"owner_id\""), + }) + return } + filter.OwnerID = apiKey.UserID + filter.OwnerUsername = "" } workspaces, err := api.Database.GetWorkspacesWithFilter(r.Context(), filter) From 70f430462461a2a725216e6954b774ccc7e8720a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 22:02:21 -0500 Subject: [PATCH 05/35] Use new query param parser for pagination fields --- coderd/httpapi/queryparams.go | 12 +++++++ coderd/pagination.go | 60 ++++++++--------------------------- coderd/workspaces.go | 2 +- 3 files changed, 26 insertions(+), 48 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 81a3d1a9d5fca..c33e9160d68c4 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -3,6 +3,7 @@ package httpapi import ( "fmt" "net/http" + "strconv" "strings" "github.com/google/uuid" @@ -29,6 +30,17 @@ func (p QueryParamParser) ValidationErrors() []Error { return p.errors } +func (p *QueryParamParser) ParseInteger(r *http.Request, def int, queryParam string) int { + v, err := parse(r, strconv.Atoi, def, queryParam) + if err != nil { + p.errors = append(p.errors, Error{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q must be a valid integer (%s)", queryParam, err.Error()), + }) + } + return v +} + func (p *QueryParamParser) ParseUUIDorMe(r *http.Request, def uuid.UUID, me uuid.UUID, queryParam string) uuid.UUID { if r.URL.Query().Get(queryParam) == "me" { return me diff --git a/coderd/pagination.go b/coderd/pagination.go index 1dc1a28886221..4cc9d5cfa1194 100644 --- a/coderd/pagination.go +++ b/coderd/pagination.go @@ -2,7 +2,6 @@ package coderd import ( "net/http" - "strconv" "github.com/google/uuid" @@ -13,53 +12,20 @@ import ( // parsePagination extracts pagination query params from the http request. // If an error is encountered, the error is written to w and ok is set to false. func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Pagination, ok bool) { - var ( - afterID = uuid.Nil - limit = -1 // Default to no limit and return all results. - offset = 0 - ) - - var err error - if s := r.URL.Query().Get("after_id"); s != "" { - afterID, err = uuid.Parse(r.URL.Query().Get("after_id")) - if err != nil { - httpapi.Write(w, http.StatusBadRequest, httpapi.Response{ - Message: "Query param 'after_id' must be a valid UUID.", - Validations: []httpapi.Error{ - {Field: "after_id", Detail: err.Error()}, - }, - }) - return p, false - } - } - if s := r.URL.Query().Get("limit"); s != "" { - limit, err = strconv.Atoi(s) - if err != nil { - httpapi.Write(w, http.StatusBadRequest, httpapi.Response{ - Message: "Query param 'limit' must be a valid integer.", - Validations: []httpapi.Error{ - {Field: "limit", Detail: err.Error()}, - }, - }) - return p, false - } + parser := httpapi.NewQueryParamParser() + params := codersdk.Pagination{ + AfterID: parser.ParseUUID(r, uuid.Nil, "after_id"), + // Limit default to "-1" which returns all results + Limit: parser.ParseInteger(r, -1, "limit"), + Offset: parser.ParseInteger(r, 0, "offset"), } - if s := r.URL.Query().Get("offset"); s != "" { - offset, err = strconv.Atoi(s) - if err != nil { - httpapi.Write(w, http.StatusBadRequest, httpapi.Response{ - Message: "Query param 'offset' must be a valid integer.", - Validations: []httpapi.Error{ - {Field: "offset", Detail: err.Error()}, - }, - }) - return p, false - } + if len(parser.ValidationErrors()) > 0 { + httpapi.Write(w, http.StatusBadRequest, httpapi.Response{ + Message: "Query parameters have invalid values.", + Validations: parser.ValidationErrors(), + }) + return params, false } - return codersdk.Pagination{ - AfterID: afterID, - Limit: limit, - Offset: offset, - }, true + return params, true } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 98f49cdba51df..54ed6fc055481 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -141,7 +141,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } if len(parser.ValidationErrors()) > 0 { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: fmt.Sprintf("Query parameters have invalid values"), + Message: "Query parameters have invalid values.", Validations: parser.ValidationErrors(), }) return From 0f7bb21f651d9b7fd70b555f207d6079c4a18b80 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 22:29:19 -0500 Subject: [PATCH 06/35] Fix fake db implementation --- coderd/database/databasefake/databasefake.go | 17 +++ coderd/httpapi/queryparams.go | 2 +- coderd/httpapi/search.go | 27 ++-- coderd/httpapi/search_test.go | 123 +++++++++++++++++++ coderd/workspaces.go | 6 +- coderd/workspaces_test.go | 19 +++ codersdk/workspaces.go | 5 + 7 files changed, 187 insertions(+), 12 deletions(-) create mode 100644 coderd/httpapi/search_test.go diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 6626aa9ec6323..d6fb70b72d61b 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -327,6 +327,23 @@ func (q *fakeQuerier) GetWorkspacesWithFilter(_ context.Context, arg database.Ge if arg.OwnerID != uuid.Nil && workspace.OwnerID != arg.OwnerID { continue } + if arg.OwnerUsername != "" { + owner, err := q.GetUserByID(context.Background(), workspace.OwnerID) + if err == nil && arg.OwnerUsername != owner.Username { + continue + } + } + if arg.TemplateName != "" { + templates, err := q.GetTemplatesByName(context.Background(), database.GetTemplatesByNameParams{ + Name: arg.TemplateName, + }) + // Add to later param + if err == nil { + for _, t := range templates { + arg.TemplateIds = append(arg.TemplateIds, t.ID) + } + } + } if !arg.Deleted && workspace.Deleted { continue } diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index c33e9160d68c4..68e4f8dbecd5d 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -89,7 +89,7 @@ func (p *QueryParamParser) ParseUUIDArray(r *http.Request, def []uuid.UUID, quer if err != nil { p.errors = append(p.errors, Error{ Field: queryParam, - Detail: fmt.Sprintf("Query param %q has invalid uuids: %q", err.Error()), + Detail: fmt.Sprintf("Query param %q has invalid uuids: %q", queryParam, err.Error()), }) } return v diff --git a/coderd/httpapi/search.go b/coderd/httpapi/search.go index aafeda1bcd3e7..f9eafb5721c95 100644 --- a/coderd/httpapi/search.go +++ b/coderd/httpapi/search.go @@ -6,17 +6,20 @@ import ( "golang.org/x/xerrors" ) -// WorkspaceSearchQuery takes a query string and breaks it into it's query -// params as a set of key=value. +// WorkspaceSearchQuery takes a query string and breaks it into it's queryparams +// as a set of key=value. func WorkspaceSearchQuery(query string) (map[string]string, error) { searchParams := make(map[string]string) - elements := queryElements(query) + if query == "" { + return searchParams, nil + } + elements := splitElements(query, ' ') for _, element := range elements { - parts := strings.Split(element, ":") + parts := splitElements(query, ':') switch len(parts) { case 1: // No key:value pair. It is a workspace name, and maybe includes an owner - parts = strings.Split(element, "/") + parts = splitElements(query, '/') switch len(parts) { case 1: searchParams["name"] = parts[0] @@ -36,10 +39,16 @@ func WorkspaceSearchQuery(query string) (map[string]string, error) { return searchParams, nil } -// queryElements takes a query string and splits it into the individual elements -// of the query. Each element is separated by a space. All quoted strings are +// splitElements 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. -func queryElements(query string) []string { +// +// Although all our names cannot have spaces, that is a validation error. +// We should still parse the quoted string as a single value so that validation +// can properly fail on the space. If we do not, a value of `template:"my name"` +// will search `template:"my name:name"`, which produces an empty list instead of +// an error. +func splitElements(query string, delimiter rune) []string { var parts []string quoted := false @@ -48,7 +57,7 @@ func queryElements(query string) []string { switch c { case '"': quoted = !quoted - case ' ': + case delimiter: if quoted { current.WriteRune(c) } else { diff --git a/coderd/httpapi/search_test.go b/coderd/httpapi/search_test.go new file mode 100644 index 0000000000000..c14e6288e83be --- /dev/null +++ b/coderd/httpapi/search_test.go @@ -0,0 +1,123 @@ +package httpapi_test + +import ( + "testing" + + "github.com/coder/coder/coderd/httpapi" + "github.com/stretchr/testify/require" +) + +func TestSearchWorkspace(t *testing.T) { + t.Parallel() + testCases := []struct { + Name string + Query string + Expected map[string]string + ExpectedErrorContains string + }{ + { + Name: "Empty", + Query: "", + Expected: map[string]string{}, + }, + { + Name: "Owner/Name", + Query: "Foo/Bar", + Expected: map[string]string{ + "owner": "Foo", + "name": "Bar", + }, + }, + { + Name: "Name", + Query: "workspace-name", + Expected: map[string]string{ + "name": "workspace-name", + }, + }, + { + Name: "Name+Param", + Query: "workspace-name template:docker", + Expected: map[string]string{ + "name": "workspace-name", + "template": "docker", + }, + }, + { + Name: "OnlyParams", + Query: "name:workspace-name template:docker owner:alice", + Expected: map[string]string{ + "owner": "alice", + "name": "workspace-name", + "template": "docker", + }, + }, + { + Name: "QuotedParam", + Query: `name:workspace-name template:"docker template" owner:alice`, + Expected: map[string]string{ + "owner": "alice", + "name": "workspace-name", + "template": "docker template", + }, + }, + { + Name: "QuotedKey", + Query: `"spaced key":"spaced value"`, + Expected: map[string]string{ + "spaced key": "spaced value", + }, + }, + { + // This will not return an error + Name: "ExtraKeys", + Query: `foo:bar`, + Expected: map[string]string{ + "foo": "bar", + }, + }, + { + // Quotes keep elements together + Name: "QuotedSpecial", + Query: `name:"workspace:name"`, + Expected: map[string]string{ + "name": "workspace:name", + }, + }, + { + Name: "QuotedMadness", + Query: `"key:is:wild/a/b/c":"foo:bar/baz/zoo:zonk"`, + Expected: map[string]string{ + "key:is:wild/a/b/c": "foo:bar/baz/zoo:zonk", + }, + }, + + // Failures + { + Name: "ExtraSlashes", + Query: `foo/bar/baz`, + ExpectedErrorContains: "can only contain 1 '/'", + }, + { + Name: "ExtraColon", + Query: `owner:name:extra`, + ExpectedErrorContains: "can only contain 1 ':'", + }, + } + + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + values, err := httpapi.WorkspaceSearchQuery(c.Query) + if c.ExpectedErrorContains != "" { + require.Error(t, err, "expected error") + require.ErrorContains(t, err, c.ExpectedErrorContains) + } else { + require.NoError(t, err, "expected no error") + require.Equal(t, c.Expected, values, "expected values") + } + + }) + } +} diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 54ed6fc055481..85c1aa5fa9828 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -117,17 +117,19 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } // Set all the query params from the "q" field. + q := r.URL.Query() for k, v := range values { // Do not allow overriding if the user also set query param fields // outside the query string. - if r.URL.Query().Has(k) { + if q.Has(k) { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ Message: fmt.Sprintf("Workspace filter %q cannot be set twice. In query params %q and %q", k, k, "q"), }) return } - r.URL.Query().Set(k, v) + q.Set(k, v) } + r.URL.RawQuery = q.Encode() parser := httpapi.NewQueryParamParser() filter := database.GetWorkspacesWithFilterParams{ diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 9253f8f2548f6..3b974f8791a4a 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -363,6 +363,25 @@ func TestWorkspaceFilter(t *testing.T) { require.Len(t, ws, 1) require.Equal(t, workspace.ID, ws[0].ID) }) + t.Run("FilterQuery", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + template2 := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + _ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template2.ID) + + // single workspace + ws, err := client.Workspaces(context.Background(), codersdk.WorkspaceFilter{ + FilterQuery: fmt.Sprintf("template:%s %s/%s", template.Name, workspace.OwnerName, workspace.Name), + }) + require.NoError(t, err) + require.Len(t, ws, 1) + require.Equal(t, workspace.ID, ws[0].ID) + }) } func TestPostWorkspaceBuild(t *testing.T) { diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index cd4dd3d001765..fb830bd903dc6 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -224,6 +224,8 @@ type WorkspaceFilter struct { Template string `json:"template,omitempty"` // Name will return partial matches Name string `json:"name,omitempty"` + // FilterQuery supports a raw filter query string + FilterQuery string `json:"q,omitempty"` } // asRequestOption returns a function that can be used in (*Client).Request. @@ -243,6 +245,9 @@ func (f WorkspaceFilter) asRequestOption() requestOption { if f.Template != "" { q.Set("template", f.Template) } + if f.FilterQuery != "" { + q.Set("q", f.FilterQuery) + } r.URL.RawQuery = q.Encode() } } From 5687dbe2c41b81feb8060be3eef7f34111a41db2 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 23:05:01 -0500 Subject: [PATCH 07/35] Add unit test for parsing query params --- coderd/httpapi/queryparams.go | 14 +- coderd/httpapi/queryparams_test.go | 203 +++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 11 deletions(-) create mode 100644 coderd/httpapi/queryparams_test.go diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 68e4f8dbecd5d..ee9841483e69b 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -45,15 +45,7 @@ func (p *QueryParamParser) ParseUUIDorMe(r *http.Request, def uuid.UUID, me uuid if r.URL.Query().Get(queryParam) == "me" { return me } - - v, err := parse(r, uuid.Parse, def, queryParam) - if err != nil { - p.errors = append(p.errors, Error{ - Field: queryParam, - Detail: fmt.Sprintf("Query param %q must be a valid uuid", queryParam), - }) - } - return v + return p.ParseUUID(r, def, queryParam) } func (p *QueryParamParser) ParseUUID(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { @@ -73,7 +65,7 @@ func (p *QueryParamParser) ParseUUIDArray(r *http.Request, def []uuid.UUID, quer strs := strings.Split(v, ",") ids := make([]uuid.UUID, 0, len(strs)) for _, s := range strs { - id, err := uuid.Parse(s) + id, err := uuid.Parse(strings.TrimSpace(s)) if err != nil { badValues = append(badValues, v) continue @@ -82,7 +74,7 @@ func (p *QueryParamParser) ParseUUIDArray(r *http.Request, def []uuid.UUID, quer } if len(badValues) > 0 { - return nil, xerrors.Errorf("%s", strings.Join(badValues, ",")) + return []uuid.UUID{}, xerrors.Errorf("%s", strings.Join(badValues, ",")) } return ids, nil }, def, queryParam) diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go new file mode 100644 index 0000000000000..ac76b14941dd0 --- /dev/null +++ b/coderd/httpapi/queryparams_test.go @@ -0,0 +1,203 @@ +package httpapi_test + +import ( + "fmt" + "net/http" + "net/url" + "testing" + + "github.com/coder/coder/coderd/httpapi" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +type queryParamTestCase[T any] struct { + QueryParam string + // No set does not set the query param, rather than setting the empty value + NoSet bool + Value string + Default T + Expected T + ExpectedErrorContains string + Parse func(r *http.Request, def T, queryParam string) T +} + +func TestParseQueryParams(t *testing.T) { + t.Parallel() + + t.Run("UUID", func(t *testing.T) { + t.Parallel() + me := uuid.New() + expParams := []queryParamTestCase[uuid.UUID]{ + { + QueryParam: "valid_id", + Value: "afe39fbf-0f52-4a62-b0cc-58670145d773", + Expected: uuid.MustParse("afe39fbf-0f52-4a62-b0cc-58670145d773"), + }, + { + QueryParam: "me", + Value: "me", + Expected: me, + }, + { + QueryParam: "invalid_id", + Value: "bogus", + ExpectedErrorContains: "must be a valid uuid", + }, + { + QueryParam: "long_id", + Value: "afe39fbf-0f52-4a62-b0cc-58670145d773-123", + ExpectedErrorContains: "must be a valid uuid", + }, + { + QueryParam: "no_value", + NoSet: true, + Default: uuid.MustParse("11111111-1111-1111-1111-111111111111"), + Expected: uuid.MustParse("11111111-1111-1111-1111-111111111111"), + }, + { + QueryParam: "empty", + Value: "", + ExpectedErrorContains: "must be a valid uuid", + }, + } + + parser := httpapi.NewQueryParamParser() + testQueryParams(t, expParams, parser, func(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { + return parser.ParseUUIDorMe(r, def, me, queryParam) + }) + }) + + t.Run("String", func(t *testing.T) { + t.Parallel() + expParams := []queryParamTestCase[string]{ + { + QueryParam: "valid_string", + Value: "random", + Expected: "random", + }, + { + QueryParam: "empty", + Value: "", + Expected: "", + }, + { + QueryParam: "no_value", + NoSet: true, + Default: "default", + Expected: "default", + }, + } + + parser := httpapi.NewQueryParamParser() + testQueryParams(t, expParams, parser, parser.ParseString) + }) + + t.Run("Integer", func(t *testing.T) { + t.Parallel() + expParams := []queryParamTestCase[int]{ + { + QueryParam: "valid_integer", + Value: "100", + Expected: 100, + }, + { + QueryParam: "empty", + Value: "", + Expected: 0, + ExpectedErrorContains: "must be a valid integer", + }, + { + QueryParam: "no_value", + NoSet: true, + Default: 5, + Expected: 5, + }, + { + QueryParam: "negative", + Value: "-10", + Expected: -10, + Default: 5, + }, + { + QueryParam: "invalid_integer", + Value: "bogus", + Expected: 0, + ExpectedErrorContains: "must be a valid integer", + }, + } + + parser := httpapi.NewQueryParamParser() + testQueryParams(t, expParams, parser, parser.ParseInteger) + }) + + t.Run("UUIDArray", func(t *testing.T) { + t.Parallel() + expParams := []queryParamTestCase[[]uuid.UUID]{ + { + QueryParam: "valid_ids_with_spaces", + Value: "6c8ef17d-5dd8-4b92-bac9-41944f90f237, 65fb05f3-12c8-4a0a-801f-40439cf9e681 , 01b94888-1eab-4bbf-aed0-dc7a8010da97", + Expected: []uuid.UUID{ + uuid.MustParse("6c8ef17d-5dd8-4b92-bac9-41944f90f237"), + uuid.MustParse("65fb05f3-12c8-4a0a-801f-40439cf9e681"), + uuid.MustParse("01b94888-1eab-4bbf-aed0-dc7a8010da97"), + }, + }, + { + QueryParam: "empty", + Value: "", + Expected: []uuid.UUID{}, + }, + { + QueryParam: "no_value", + NoSet: true, + Default: []uuid.UUID{}, + Expected: []uuid.UUID{}, + }, + { + QueryParam: "default", + NoSet: true, + Default: []uuid.UUID{uuid.Nil}, + Expected: []uuid.UUID{uuid.Nil}, + }, + { + QueryParam: "invalid_id_in_set", + Value: "6c8ef17d-5dd8-4b92-bac9-41944f90f237,bogus", + Expected: []uuid.UUID{}, + Default: []uuid.UUID{}, + ExpectedErrorContains: "bogus", + }, + } + + parser := httpapi.NewQueryParamParser() + testQueryParams(t, expParams, parser, parser.ParseUUIDArray) + }) +} + +func testQueryParams[T any](t *testing.T, testCases []queryParamTestCase[T], parser *httpapi.QueryParamParser, parse func(r *http.Request, def T, queryParam string) T) { + v := url.Values{} + for _, c := range testCases { + if c.NoSet { + continue + } + v.Set(c.QueryParam, c.Value) + } + r, err := http.NewRequest("GET", "https://example.com", nil) + require.NoError(t, err, "new request") + r.URL.RawQuery = v.Encode() + + for _, c := range testCases { + // !! Do not run these in parallel !! + t.Run(c.QueryParam, func(t *testing.T) { + v := parse(r, c.Default, c.QueryParam) + require.Equal(t, c.Expected, v, fmt.Sprintf("param=%q value=%q", c.QueryParam, c.Value)) + if c.ExpectedErrorContains != "" { + errors := parser.ValidationErrors() + require.True(t, len(errors) > 0, "error exist") + last := errors[len(errors)-1] + require.True(t, last.Field == c.QueryParam, fmt.Sprintf("query param %q did not fail", c.QueryParam)) + require.Contains(t, last.Detail, c.ExpectedErrorContains, "correct error") + } + }) + } +} From 18e52476872c483be80d47e7e63433907216531a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 23:17:52 -0500 Subject: [PATCH 08/35] Fix linting --- coderd/httpapi/queryparams_test.go | 3 ++- coderd/httpapi/search.go | 4 ++-- coderd/httpapi/search_test.go | 4 ++-- coderd/workspaces.go | 6 +++--- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index ac76b14941dd0..0bde9f6c09a31 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -6,9 +6,10 @@ import ( "net/url" "testing" - "github.com/coder/coder/coderd/httpapi" "github.com/google/uuid" "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/httpapi" ) type queryParamTestCase[T any] struct { diff --git a/coderd/httpapi/search.go b/coderd/httpapi/search.go index f9eafb5721c95..5cf0edd0e668a 100644 --- a/coderd/httpapi/search.go +++ b/coderd/httpapi/search.go @@ -59,13 +59,13 @@ func splitElements(query string, delimiter rune) []string { quoted = !quoted case delimiter: if quoted { - current.WriteRune(c) + _, _ = current.WriteRune(c) } else { parts = append(parts, current.String()) current = strings.Builder{} } default: - current.WriteRune(c) + _, _ = current.WriteRune(c) } } parts = append(parts, current.String()) diff --git a/coderd/httpapi/search_test.go b/coderd/httpapi/search_test.go index c14e6288e83be..70334fb358eb1 100644 --- a/coderd/httpapi/search_test.go +++ b/coderd/httpapi/search_test.go @@ -3,8 +3,9 @@ package httpapi_test import ( "testing" - "github.com/coder/coder/coderd/httpapi" "github.com/stretchr/testify/require" + + "github.com/coder/coder/coderd/httpapi" ) func TestSearchWorkspace(t *testing.T) { @@ -117,7 +118,6 @@ func TestSearchWorkspace(t *testing.T) { require.NoError(t, err, "expected no error") require.Equal(t, c.Expected, values, "expected values") } - }) } } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 85c1aa5fa9828..688cdc5cabe43 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -108,7 +108,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { values, err := httpapi.WorkspaceSearchQuery(queryStr) if err != nil { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: "Invalid workspace search query", + Message: "Invalid workspace search query.", Validations: []httpapi.Error{ {Field: "q", Detail: err.Error()}, }, @@ -123,7 +123,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { // outside the query string. if q.Has(k) { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: fmt.Sprintf("Workspace filter %q cannot be set twice. In query params %q and %q", k, k, "q"), + Message: fmt.Sprintf("Workspace filter %q cannot be set twice. In query params %q and %q.", k, k, "q"), }) return } @@ -152,7 +152,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { if filter.OwnerUsername == "me" { if !(filter.OwnerID == uuid.Nil || filter.OwnerID == apiKey.UserID) { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: fmt.Sprintf("Cannot set both \"me\" in \"owner_name\" and use \"owner_id\""), + Message: fmt.Sprintf("Cannot set both \"me\" in \"owner_name\" and use \"owner_id\"."), }) return } From 9a1b182a9e7624fc45e9e574814e494a6e157b1f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 23:27:03 -0500 Subject: [PATCH 09/35] Fix search --- coderd/httpapi/search.go | 14 ++++++++++---- coderd/httpapi/search_test.go | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/coderd/httpapi/search.go b/coderd/httpapi/search.go index 5cf0edd0e668a..0b72623ac9481 100644 --- a/coderd/httpapi/search.go +++ b/coderd/httpapi/search.go @@ -13,13 +13,16 @@ func WorkspaceSearchQuery(query string) (map[string]string, error) { if query == "" { return searchParams, nil } - elements := splitElements(query, ' ') + // Because we do this in 2 passes, we want to maintain quotes on the first + // pass.Further splitting occurs on the second pass and quotes will be + // dropped. + elements := splitElements(query, ' ', true) for _, element := range elements { - parts := splitElements(query, ':') + parts := splitElements(element, ':', false) switch len(parts) { case 1: // No key:value pair. It is a workspace name, and maybe includes an owner - parts = splitElements(query, '/') + parts = splitElements(element, '/', false) switch len(parts) { case 1: searchParams["name"] = parts[0] @@ -48,7 +51,7 @@ func WorkspaceSearchQuery(query string) (map[string]string, error) { // can properly fail on the space. If we do not, a value of `template:"my name"` // will search `template:"my name:name"`, which produces an empty list instead of // an error. -func splitElements(query string, delimiter rune) []string { +func splitElements(query string, delimiter rune, maintainQuotes bool) []string { var parts []string quoted := false @@ -56,6 +59,9 @@ func splitElements(query string, delimiter rune) []string { for _, c := range query { switch c { case '"': + if maintainQuotes { + _, _ = current.WriteRune(c) + } quoted = !quoted case delimiter: if quoted { diff --git a/coderd/httpapi/search_test.go b/coderd/httpapi/search_test.go index 70334fb358eb1..5b6e9b1d413a4 100644 --- a/coderd/httpapi/search_test.go +++ b/coderd/httpapi/search_test.go @@ -92,6 +92,21 @@ func TestSearchWorkspace(t *testing.T) { "key:is:wild/a/b/c": "foo:bar/baz/zoo:zonk", }, }, + { + Name: "QuotesName", + Query: `"foo/bar"`, + Expected: map[string]string{ + "name": "foo/bar", + }, + }, + { + Name: "QuotedOwner/Name", + Query: `"foo"/"bar"`, + Expected: map[string]string{ + "owner": "foo", + "name": "bar", + }, + }, // Failures { From fab5d8cf1f2ebf8ae32a04263691c98190c93307 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 23:32:02 -0500 Subject: [PATCH 10/35] Maintain old behavior --- coderd/httpapi/queryparams.go | 2 +- coderd/httpapi/queryparams_test.go | 14 +++++++------- coderd/pagination_internal_test.go | 11 ++++++----- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index ee9841483e69b..33a17dbd4a09c 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -101,7 +101,7 @@ func (p *QueryParamParser) ParseString(r *http.Request, def string, queryParam s } func parse[T any](r *http.Request, parse func(v string) (T, error), def T, queryParam string) (T, error) { - if !r.URL.Query().Has(queryParam) { + if !r.URL.Query().Has(queryParam) || r.URL.Query().Get(queryParam) == "" { return def, nil } str := r.URL.Query().Get(queryParam) diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index 0bde9f6c09a31..d7bbc61782000 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -57,9 +57,9 @@ func TestParseQueryParams(t *testing.T) { Expected: uuid.MustParse("11111111-1111-1111-1111-111111111111"), }, { - QueryParam: "empty", - Value: "", - ExpectedErrorContains: "must be a valid uuid", + QueryParam: "empty", + Value: "", + Expected: uuid.Nil, }, } @@ -103,10 +103,9 @@ func TestParseQueryParams(t *testing.T) { Expected: 100, }, { - QueryParam: "empty", - Value: "", - Expected: 0, - ExpectedErrorContains: "must be a valid integer", + QueryParam: "empty", + Value: "", + Expected: 0, }, { QueryParam: "no_value", @@ -147,6 +146,7 @@ func TestParseQueryParams(t *testing.T) { { QueryParam: "empty", Value: "", + Default: []uuid.UUID{}, Expected: []uuid.UUID{}, }, { diff --git a/coderd/pagination_internal_test.go b/coderd/pagination_internal_test.go index 97eba7275ea6f..5504ef267e165 100644 --- a/coderd/pagination_internal_test.go +++ b/coderd/pagination_internal_test.go @@ -14,6 +14,7 @@ import ( func TestPagination(t *testing.T) { t.Parallel() + const invalidValues = "Query parameters have invalid values" testCases := []struct { Name string @@ -27,27 +28,27 @@ func TestPagination(t *testing.T) { { Name: "BadAfterID", AfterID: "bogus", - ExpectedError: "Query param 'after_id' must be a valid UUID", + ExpectedError: invalidValues, }, { Name: "ShortAfterID", AfterID: "ff22a7b-bb6f-43d8-83e1-eefe0a1f5197", - ExpectedError: "Query param 'after_id' must be a valid UUID", + ExpectedError: invalidValues, }, { Name: "LongAfterID", AfterID: "cff22a7b-bb6f-43d8-83e1-eefe0a1f51972", - ExpectedError: "Query param 'after_id' must be a valid UUID", + ExpectedError: invalidValues, }, { Name: "BadLimit", Limit: "bogus", - ExpectedError: "Query param 'limit' must be a valid integer", + ExpectedError: invalidValues, }, { Name: "BadOffset", Offset: "bogus", - ExpectedError: "Query param 'offset' must be a valid integer", + ExpectedError: invalidValues, }, // Valid values From 15754c5bf6456bbc644dc8fd5d702bcc3a392268 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 9 Jun 2022 23:35:33 -0500 Subject: [PATCH 11/35] Linting --- coderd/httpapi/search.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/httpapi/search.go b/coderd/httpapi/search.go index 0b72623ac9481..c5ed0045e6138 100644 --- a/coderd/httpapi/search.go +++ b/coderd/httpapi/search.go @@ -51,6 +51,7 @@ func WorkspaceSearchQuery(query string) (map[string]string, error) { // can properly fail on the space. If we do not, a value of `template:"my name"` // will search `template:"my name:name"`, which produces an empty list instead of // an error. +// nolint:revive func splitElements(query string, delimiter rune, maintainQuotes bool) []string { var parts []string From d1c631935778e2cfce0998b1b79bd244d1486d4d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 10:17:56 -0500 Subject: [PATCH 12/35] Remove excessive calls, use filters on a single query --- coderd/database/databasefake/databasefake.go | 52 ++--- coderd/database/querier.go | 5 +- coderd/database/queries.sql.go | 192 ++++--------------- coderd/database/queries/templates.sql | 48 +++-- coderd/database/queries/workspaces.sql | 9 - coderd/templates.go | 6 +- coderd/workspaces.go | 4 +- 7 files changed, 103 insertions(+), 213 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 938de90698a3a..5ccc05864c724 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -334,8 +334,8 @@ func (q *fakeQuerier) GetWorkspacesWithFilter(_ context.Context, arg database.Ge } } if arg.TemplateName != "" { - templates, err := q.GetTemplatesByName(context.Background(), database.GetTemplatesByNameParams{ - Name: arg.TemplateName, + templates, err := q.GetTemplatesWithFilter(context.Background(), database.GetTemplatesWithFilterParams{ + ExactName: arg.TemplateName, }) // Add to later param if err == nil { @@ -799,17 +799,39 @@ func (q *fakeQuerier) UpdateTemplateMetaByID(_ context.Context, arg database.Upd return sql.ErrNoRows } -func (q *fakeQuerier) GetTemplatesByName(_ context.Context, arg database.GetTemplatesByNameParams) ([]database.Template, error) { +func (q *fakeQuerier) GetTemplatesWithFilter(_ context.Context, arg database.GetTemplatesWithFilterParams) ([]database.Template, error) { q.mutex.RLock() defer q.mutex.RUnlock() + var templates []database.Template for _, template := range q.templates { - if !strings.EqualFold(template.Name, arg.Name) { + if template.Deleted != arg.Deleted { continue } - if template.Deleted != arg.Deleted { + if arg.OrganizationID != uuid.Nil && template.OrganizationID != arg.OrganizationID { + continue + } + + if arg.Name != "" && !strings.Contains(template.Name, arg.Name) { + continue + } + + if arg.ExactName != "" && !strings.EqualFold(template.Name, arg.ExactName) { continue } + + if len(arg.Ids) > 0 { + match := false + for _, id := range arg.Ids { + if template.ID == id { + match = true + break + } + } + if !match { + continue + } + } templates = append(templates, template) } if len(templates) > 0 { @@ -956,26 +978,6 @@ func (q *fakeQuerier) GetParameterValueByScopeAndName(_ context.Context, arg dat return database.ParameterValue{}, sql.ErrNoRows } -func (q *fakeQuerier) GetTemplatesByOrganization(_ context.Context, arg database.GetTemplatesByOrganizationParams) ([]database.Template, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - - templates := make([]database.Template, 0) - for _, template := range q.templates { - if template.Deleted != arg.Deleted { - continue - } - if template.OrganizationID != arg.OrganizationID { - continue - } - templates = append(templates, template) - } - if len(templates) == 0 { - return nil, sql.ErrNoRows - } - return templates, nil -} - func (q *fakeQuerier) GetTemplatesByIDs(_ context.Context, ids []uuid.UUID) ([]database.Template, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 0ace0fb587a45..15f2ddab601ea 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -53,9 +53,7 @@ type querier interface { GetTemplateVersionByJobID(ctx context.Context, jobID uuid.UUID) (TemplateVersion, error) GetTemplateVersionByTemplateIDAndName(ctx context.Context, arg GetTemplateVersionByTemplateIDAndNameParams) (TemplateVersion, error) GetTemplateVersionsByTemplateID(ctx context.Context, arg GetTemplateVersionsByTemplateIDParams) ([]TemplateVersion, error) - GetTemplatesByIDs(ctx context.Context, ids []uuid.UUID) ([]Template, error) - GetTemplatesByName(ctx context.Context, arg GetTemplatesByNameParams) ([]Template, error) - GetTemplatesByOrganization(ctx context.Context, arg GetTemplatesByOrganizationParams) ([]Template, error) + GetTemplatesWithFilter(ctx context.Context, arg GetTemplatesWithFilterParams) ([]Template, error) GetUserByEmailOrUsername(ctx context.Context, arg GetUserByEmailOrUsernameParams) (User, error) GetUserByID(ctx context.Context, id uuid.UUID) (User, error) GetUserCount(ctx context.Context) (int64, error) @@ -79,7 +77,6 @@ type querier interface { GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.UUID) ([]WorkspaceResource, error) GetWorkspacesAutostart(ctx context.Context) ([]Workspace, error) GetWorkspacesByOrganizationIDs(ctx context.Context, arg GetWorkspacesByOrganizationIDsParams) ([]Workspace, error) - GetWorkspacesByTemplateID(ctx context.Context, arg GetWorkspacesByTemplateIDParams) ([]Workspace, error) GetWorkspacesWithFilter(ctx context.Context, arg GetWorkspacesWithFilterParams) ([]Workspace, error) InsertAPIKey(ctx context.Context, arg InsertAPIKeyParams) (APIKey, error) InsertAuditLog(ctx context.Context, arg InsertAuditLogParams) (AuditLog, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 9b68bbc5605e5..c42d39d35fa80 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1669,117 +1669,56 @@ func (q *sqlQuerier) GetTemplateByOrganizationAndName(ctx context.Context, arg G return i, err } -const getTemplatesByIDs = `-- name: GetTemplatesByIDs :many +const getTemplatesWithFilter = `-- name: GetTemplatesWithFilter :many SELECT id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, max_ttl, min_autostart_interval FROM templates WHERE - id = ANY($1 :: uuid [ ]) -` - -func (q *sqlQuerier) GetTemplatesByIDs(ctx context.Context, ids []uuid.UUID) ([]Template, error) { - rows, err := q.db.QueryContext(ctx, getTemplatesByIDs, pq.Array(ids)) - if err != nil { - return nil, err - } - defer rows.Close() - var items []Template - for rows.Next() { - var i Template - if err := rows.Scan( - &i.ID, - &i.CreatedAt, - &i.UpdatedAt, - &i.OrganizationID, - &i.Deleted, - &i.Name, - &i.Provisioner, - &i.ActiveVersionID, - &i.Description, - &i.MaxTtl, - &i.MinAutostartInterval, - ); 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 getTemplatesByName = `-- name: GetTemplatesByName :many -SELECT - id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, max_ttl, min_autostart_interval -FROM - templates -WHERE - deleted = $1 - AND LOWER("name") = LOWER($2) -` - -type GetTemplatesByNameParams struct { - Deleted bool `db:"deleted" json:"deleted"` - Name string `db:"name" json:"name"` -} - -func (q *sqlQuerier) GetTemplatesByName(ctx context.Context, arg GetTemplatesByNameParams) ([]Template, error) { - rows, err := q.db.QueryContext(ctx, getTemplatesByName, arg.Deleted, arg.Name) - if err != nil { - return nil, err - } - defer rows.Close() - var items []Template - for rows.Next() { - var i Template - if err := rows.Scan( - &i.ID, - &i.CreatedAt, - &i.UpdatedAt, - &i.OrganizationID, - &i.Deleted, - &i.Name, - &i.Provisioner, - &i.ActiveVersionID, - &i.Description, - &i.MaxTtl, - &i.MinAutostartInterval, - ); 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 getTemplatesByOrganization = `-- name: GetTemplatesByOrganization :many -SELECT - id, created_at, updated_at, organization_id, deleted, name, provisioner, active_version_id, description, max_ttl, min_autostart_interval -FROM - templates -WHERE - organization_id = $1 - AND deleted = $2 + -- Optionally include deleted templates + templates.deleted = $1 + -- Filter by organization_id + AND CASE + WHEN $2 :: uuid != '00000000-00000000-00000000-00000000' THEN + organization_id = $2 + ELSE true + END + -- Filter by name, matching on substring + AND CASE + WHEN $3 :: text != '' THEN + LOWER(name) LIKE '%' || LOWER($3) || '%' + ELSE true + END + -- Filter by exact name + AND CASE + WHEN $4 :: text != '' THEN + LOWER("name") = LOWER($4) + ELSE true + END + -- Filter by ids + AND CASE + WHEN array_length($5 :: uuid[], 1) > 0 THEN + id = ANY($5) + ELSE true + END ` -type GetTemplatesByOrganizationParams struct { - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - Deleted bool `db:"deleted" json:"deleted"` +type GetTemplatesWithFilterParams struct { + Deleted bool `db:"deleted" json:"deleted"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + Name string `db:"name" json:"name"` + ExactName string `db:"exact_name" json:"exact_name"` + Ids []uuid.UUID `db:"ids" json:"ids"` } -func (q *sqlQuerier) GetTemplatesByOrganization(ctx context.Context, arg GetTemplatesByOrganizationParams) ([]Template, error) { - rows, err := q.db.QueryContext(ctx, getTemplatesByOrganization, arg.OrganizationID, arg.Deleted) +func (q *sqlQuerier) GetTemplatesWithFilter(ctx context.Context, arg GetTemplatesWithFilterParams) ([]Template, error) { + rows, err := q.db.QueryContext(ctx, getTemplatesWithFilter, + arg.Deleted, + arg.OrganizationID, + arg.Name, + arg.ExactName, + pq.Array(arg.Ids), + ) if err != nil { return nil, err } @@ -3689,55 +3628,6 @@ func (q *sqlQuerier) GetWorkspacesByOrganizationIDs(ctx context.Context, arg Get return items, nil } -const getWorkspacesByTemplateID = `-- name: GetWorkspacesByTemplateID :many -SELECT - id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, ttl -FROM - workspaces -WHERE - template_id = $1 - AND deleted = $2 -` - -type GetWorkspacesByTemplateIDParams struct { - TemplateID uuid.UUID `db:"template_id" json:"template_id"` - Deleted bool `db:"deleted" json:"deleted"` -} - -func (q *sqlQuerier) GetWorkspacesByTemplateID(ctx context.Context, arg GetWorkspacesByTemplateIDParams) ([]Workspace, error) { - rows, err := q.db.QueryContext(ctx, getWorkspacesByTemplateID, arg.TemplateID, arg.Deleted) - if err != nil { - return nil, err - } - defer rows.Close() - var items []Workspace - for rows.Next() { - var i Workspace - if err := rows.Scan( - &i.ID, - &i.CreatedAt, - &i.UpdatedAt, - &i.OwnerID, - &i.OrganizationID, - &i.TemplateID, - &i.Deleted, - &i.Name, - &i.AutostartSchedule, - &i.Ttl, - ); 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 getWorkspacesWithFilter = `-- name: GetWorkspacesWithFilter :many SELECT id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, ttl diff --git a/coderd/database/queries/templates.sql b/coderd/database/queries/templates.sql index 52564413f9980..1f6e34e96415e 100644 --- a/coderd/database/queries/templates.sql +++ b/coderd/database/queries/templates.sql @@ -8,13 +8,39 @@ WHERE LIMIT 1; --- name: GetTemplatesByIDs :many +-- name: GetTemplatesWithFilter :many SELECT * FROM templates WHERE - id = ANY(@ids :: uuid [ ]); + -- Optionally include deleted templates + templates.deleted = @deleted + -- Filter by organization_id + AND CASE + WHEN @organization_id :: uuid != '00000000-00000000-00000000-00000000' THEN + organization_id = @organization_id + ELSE true + END + -- Filter by name, matching on substring + AND CASE + WHEN @name :: text != '' THEN + LOWER(name) LIKE '%' || LOWER(@name) || '%' + ELSE true + END + -- Filter by exact name + AND CASE + WHEN @exact_name :: text != '' THEN + LOWER("name") = LOWER(@exact_name) + ELSE true + END + -- Filter by ids + AND CASE + WHEN array_length(@ids :: uuid[], 1) > 0 THEN + id = ANY(@ids) + ELSE true + END +; -- name: GetTemplateByOrganizationAndName :one SELECT @@ -28,24 +54,6 @@ WHERE LIMIT 1; --- name: GetTemplatesByName :many -SELECT - * -FROM - templates -WHERE - deleted = @deleted - AND LOWER("name") = LOWER(@name); - --- name: GetTemplatesByOrganization :many -SELECT - * -FROM - templates -WHERE - organization_id = $1 - AND deleted = $2; - -- name: InsertTemplate :one INSERT INTO templates ( diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index 676c21fc431dd..db4d65e514a0e 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -73,15 +73,6 @@ AND (ttl IS NOT NULL AND ttl > 0) ); --- name: GetWorkspacesByTemplateID :many -SELECT - * -FROM - workspaces -WHERE - template_id = $1 - AND deleted = $2; - -- name: GetWorkspaceByOwnerIDAndName :one SELECT * diff --git a/coderd/templates.go b/coderd/templates.go index 386a360111960..e7dd818582c8c 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -58,8 +58,8 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) { return } - workspaces, err := api.Database.GetWorkspacesByTemplateID(r.Context(), database.GetWorkspacesByTemplateIDParams{ - TemplateID: template.ID, + workspaces, err := api.Database.GetWorkspacesWithFilter(r.Context(), database.GetWorkspacesWithFilterParams{ + TemplateIds: []uuid.UUID{template.ID}, }) if errors.Is(err, sql.ErrNoRows) { err = nil @@ -224,7 +224,7 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque func (api *API) templatesByOrganization(rw http.ResponseWriter, r *http.Request) { organization := httpmw.OrganizationParam(r) - templates, err := api.Database.GetTemplatesByOrganization(r.Context(), database.GetTemplatesByOrganizationParams{ + templates, err := api.Database.GetTemplatesWithFilter(r.Context(), database.GetTemplatesWithFilterParams{ OrganizationID: organization.ID, }) if errors.Is(err, sql.ErrNoRows) { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index f49d908120049..8e3cc2f48a87e 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -813,7 +813,9 @@ func convertWorkspaces(ctx context.Context, db database.Store, workspaces []data if err != nil { return nil, xerrors.Errorf("get workspace builds: %w", err) } - templates, err := db.GetTemplatesByIDs(ctx, templateIDs) + templates, err := db.GetTemplatesWithFilter(ctx, database.GetTemplatesWithFilterParams{ + Ids: templateIDs, + }) if errors.Is(err, sql.ErrNoRows) { err = nil } From e5ec365a0ce91761ae14ea485af99a74e5612410 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 10:20:08 -0500 Subject: [PATCH 13/35] Remove unused code --- coderd/database/databasefake/databasefake.go | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 5ccc05864c724..2c73208b943e1 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -368,26 +368,6 @@ func (q *fakeQuerier) GetWorkspacesWithFilter(_ context.Context, arg database.Ge return workspaces, nil } -func (q *fakeQuerier) GetWorkspacesByTemplateID(_ context.Context, arg database.GetWorkspacesByTemplateIDParams) ([]database.Workspace, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - - workspaces := make([]database.Workspace, 0) - for _, workspace := range q.workspaces { - if workspace.TemplateID.String() != arg.TemplateID.String() { - continue - } - if workspace.Deleted != arg.Deleted { - continue - } - workspaces = append(workspaces, workspace) - } - if len(workspaces) == 0 { - return nil, sql.ErrNoRows - } - return workspaces, nil -} - func (q *fakeQuerier) GetWorkspaceByID(_ context.Context, id uuid.UUID) (database.Workspace, error) { q.mutex.RLock() defer q.mutex.RUnlock() From 21683d25cc370d638659bb443bd3e4f2c49a6f96 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 10:34:52 -0500 Subject: [PATCH 14/35] Add unit test to keep fake db clean --- coderd/database/databasefake/databasefake.go | 19 ------ .../databasefake/databasefake_test.go | 59 +++++++++++++++++++ coderd/workspaces.go | 2 +- 3 files changed, 60 insertions(+), 20 deletions(-) create mode 100644 coderd/database/databasefake/databasefake_test.go diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 2c73208b943e1..e78c0870d26b5 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -958,25 +958,6 @@ func (q *fakeQuerier) GetParameterValueByScopeAndName(_ context.Context, arg dat return database.ParameterValue{}, sql.ErrNoRows } -func (q *fakeQuerier) GetTemplatesByIDs(_ context.Context, ids []uuid.UUID) ([]database.Template, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - - templates := make([]database.Template, 0) - for _, template := range q.templates { - for _, id := range ids { - if template.ID.String() != id.String() { - continue - } - templates = append(templates, template) - } - } - if len(templates) == 0 { - return nil, sql.ErrNoRows - } - return templates, nil -} - func (q *fakeQuerier) GetOrganizationMemberByUserID(_ context.Context, arg database.GetOrganizationMemberByUserIDParams) (database.OrganizationMember, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/databasefake/databasefake_test.go b/coderd/database/databasefake/databasefake_test.go new file mode 100644 index 0000000000000..40d1ae991b4eb --- /dev/null +++ b/coderd/database/databasefake/databasefake_test.go @@ -0,0 +1,59 @@ +package databasefake_test + +import ( + "fmt" + "reflect" + "testing" + + "github.com/coder/coder/coderd/database" + + "github.com/coder/coder/coderd/database/databasefake" +) + +// TestExactMethods will ensure the fake database does not hold onto excessive +// functions. The fake database is a manual implementation, so it is possible +// we forget to delete functions that we remove. This unit test just ensures +// we remove the extra methods. +func TestExactMethods(t *testing.T) { + // extraFakeMethods contains the extra allowed methods that are not a part + // of the database.Store interface. + extraFakeMethods := map[string]string{ + // Example + // "SortFakeLists": "Helper function used", + } + + fake := reflect.TypeOf(databasefake.New()) + fakeMethods := methods(fake) + + store := reflect.TypeOf((*database.Store)(nil)).Elem() + storeMethods := methods(store) + + // Store should be a subset + for k := range storeMethods { + _, ok := fakeMethods[k] + if !ok { + panic(fmt.Sprintf("This should never happen. FakeDB missing method %s, so doesn't fit the interface", k)) + } + delete(storeMethods, k) + delete(fakeMethods, k) + } + + for k := range fakeMethods { + _, ok := extraFakeMethods[k] + if ok { + continue + } + // If you are seeing this error, you have an extra function not required + // for the database.Store. If you still want to keep it, add it to + // 'extraFakeMethods' to allow it. + t.Errorf("Fake method '%s()' is excessive and not needed to fit interface, delete it", k) + } +} + +func methods(rt reflect.Type) map[string]bool { + methods := make(map[string]bool) + for i := 0; i < rt.NumMethod(); i++ { + methods[rt.Method(i).Name] = true + } + return methods +} diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 8e3cc2f48a87e..7f681351e3ea5 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -151,7 +151,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { if filter.OwnerUsername == "me" { if !(filter.OwnerID == uuid.Nil || filter.OwnerID == apiKey.UserID) { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: fmt.Sprintf("Cannot set both \"me\" in \"owner_name\" and use \"owner_id\"."), + Message: "Cannot set both \"me\" in \"owner_name\" and use \"owner_id\".", }) return } From fc483970852ca0509c51526352ea2fcee5b994b1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 10:41:28 -0500 Subject: [PATCH 15/35] Fix typo --- coderd/httpapi/search.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/httpapi/search.go b/coderd/httpapi/search.go index c5ed0045e6138..320ca0597b6fc 100644 --- a/coderd/httpapi/search.go +++ b/coderd/httpapi/search.go @@ -6,7 +6,7 @@ import ( "golang.org/x/xerrors" ) -// WorkspaceSearchQuery takes a query string and breaks it into it's queryparams +// WorkspaceSearchQuery takes a query string and breaks it into its queryparams // as a set of key=value. func WorkspaceSearchQuery(query string) (map[string]string, error) { searchParams := make(map[string]string) From 531016421a01766667212670f93b2fe8174cbc52 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 11:38:55 -0500 Subject: [PATCH 16/35] Drop like name search on template --- coderd/database/queries.sql.go | 16 ++++------------ coderd/database/queries/templates.sql | 6 ------ 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index c42d39d35fa80..d8d03ec898151 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1683,22 +1683,16 @@ WHERE organization_id = $2 ELSE true END - -- Filter by name, matching on substring - AND CASE - WHEN $3 :: text != '' THEN - LOWER(name) LIKE '%' || LOWER($3) || '%' - ELSE true - END -- Filter by exact name AND CASE - WHEN $4 :: text != '' THEN - LOWER("name") = LOWER($4) + WHEN $3 :: text != '' THEN + LOWER("name") = LOWER($3) ELSE true END -- Filter by ids AND CASE - WHEN array_length($5 :: uuid[], 1) > 0 THEN - id = ANY($5) + WHEN array_length($4 :: uuid[], 1) > 0 THEN + id = ANY($4) ELSE true END ` @@ -1706,7 +1700,6 @@ WHERE type GetTemplatesWithFilterParams struct { Deleted bool `db:"deleted" json:"deleted"` OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - Name string `db:"name" json:"name"` ExactName string `db:"exact_name" json:"exact_name"` Ids []uuid.UUID `db:"ids" json:"ids"` } @@ -1715,7 +1708,6 @@ func (q *sqlQuerier) GetTemplatesWithFilter(ctx context.Context, arg GetTemplate rows, err := q.db.QueryContext(ctx, getTemplatesWithFilter, arg.Deleted, arg.OrganizationID, - arg.Name, arg.ExactName, pq.Array(arg.Ids), ) diff --git a/coderd/database/queries/templates.sql b/coderd/database/queries/templates.sql index 1f6e34e96415e..df19a28606b35 100644 --- a/coderd/database/queries/templates.sql +++ b/coderd/database/queries/templates.sql @@ -22,12 +22,6 @@ WHERE organization_id = @organization_id ELSE true END - -- Filter by name, matching on substring - AND CASE - WHEN @name :: text != '' THEN - LOWER(name) LIKE '%' || LOWER(@name) || '%' - ELSE true - END -- Filter by exact name AND CASE WHEN @exact_name :: text != '' THEN From c64ed18a63a129030f36a4dfdc4c013ac4560021 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 11:42:49 -0500 Subject: [PATCH 17/35] Fix linting --- coderd/database/databasefake/databasefake.go | 4 ---- coderd/database/databasefake/databasefake_test.go | 2 ++ 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index e78c0870d26b5..3c3b33e03df77 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -792,10 +792,6 @@ func (q *fakeQuerier) GetTemplatesWithFilter(_ context.Context, arg database.Get continue } - if arg.Name != "" && !strings.Contains(template.Name, arg.Name) { - continue - } - if arg.ExactName != "" && !strings.EqualFold(template.Name, arg.ExactName) { continue } diff --git a/coderd/database/databasefake/databasefake_test.go b/coderd/database/databasefake/databasefake_test.go index 40d1ae991b4eb..6a3416b59c7ac 100644 --- a/coderd/database/databasefake/databasefake_test.go +++ b/coderd/database/databasefake/databasefake_test.go @@ -15,6 +15,8 @@ import ( // we forget to delete functions that we remove. This unit test just ensures // we remove the extra methods. func TestExactMethods(t *testing.T) { + t.Parallel() + // extraFakeMethods contains the extra allowed methods that are not a part // of the database.Store interface. extraFakeMethods := map[string]string{ From c6e3a579c7b5acf2d63a16f2d2b5db2393373850 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 11:57:56 -0500 Subject: [PATCH 18/35] Move WorkspaceSearchQuery to workspaces.go --- coderd/workspaces.go | 2 +- coderd/{httpapi/search.go => workspacesearch.go} | 2 +- coderd/{httpapi/search_test.go => workspacesearch_test.go} | 7 +++---- 3 files changed, 5 insertions(+), 6 deletions(-) rename coderd/{httpapi/search.go => workspacesearch.go} (99%) rename coderd/{httpapi/search_test.go => workspacesearch_test.go} (95%) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 7f681351e3ea5..66fd430356c56 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -104,7 +104,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) queryStr := r.URL.Query().Get("q") - values, err := httpapi.WorkspaceSearchQuery(queryStr) + values, err := WorkspaceSearchQuery(queryStr) if err != nil { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ Message: "Invalid workspace search query.", diff --git a/coderd/httpapi/search.go b/coderd/workspacesearch.go similarity index 99% rename from coderd/httpapi/search.go rename to coderd/workspacesearch.go index 320ca0597b6fc..e1a17612caf61 100644 --- a/coderd/httpapi/search.go +++ b/coderd/workspacesearch.go @@ -1,4 +1,4 @@ -package httpapi +package coderd import ( "strings" diff --git a/coderd/httpapi/search_test.go b/coderd/workspacesearch_test.go similarity index 95% rename from coderd/httpapi/search_test.go rename to coderd/workspacesearch_test.go index 5b6e9b1d413a4..d49d8424c2dab 100644 --- a/coderd/httpapi/search_test.go +++ b/coderd/workspacesearch_test.go @@ -1,11 +1,10 @@ -package httpapi_test +package coderd_test import ( "testing" + "github.com/coder/coder/coderd" "github.com/stretchr/testify/require" - - "github.com/coder/coder/coderd/httpapi" ) func TestSearchWorkspace(t *testing.T) { @@ -125,7 +124,7 @@ func TestSearchWorkspace(t *testing.T) { c := c t.Run(c.Name, func(t *testing.T) { t.Parallel() - values, err := httpapi.WorkspaceSearchQuery(c.Query) + values, err := coderd.WorkspaceSearchQuery(c.Query) if c.ExpectedErrorContains != "" { require.Error(t, err, "expected error") require.ErrorContains(t, err, c.ExpectedErrorContains) From 7821aa4fe655438da23d1ac991f164f59908ed5c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 12:02:53 -0500 Subject: [PATCH 19/35] Search all templates with name --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/workspaces.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index d8d03ec898151..a8027ef5d56df 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3651,7 +3651,7 @@ WHERE -- Use the organization filter to restrict to 1 org if needed. AND CASE WHEN $5 :: text != '' THEN - template_id = (SELECT id FROM templates WHERE name = $5) + template_id = ANY(SELECT id FROM templates WHERE name = $5) ELSE true END -- Filter by template_ids diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index db4d65e514a0e..cd957116c95c1 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -39,7 +39,7 @@ WHERE -- Use the organization filter to restrict to 1 org if needed. AND CASE WHEN @template_name :: text != '' THEN - template_id = (SELECT id FROM templates WHERE name = @template_name) + template_id = ANY(SELECT id FROM templates WHERE name = @template_name) ELSE true END -- Filter by template_ids From d3091f0ba9887de7f903792742382c8ab47f9580 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 12:52:50 -0500 Subject: [PATCH 20/35] Add more complex filter unit test --- coderd/workspaces.go | 21 +--- coderd/workspaces_test.go | 207 +++++++++++++++++++++++++++++++++++++- 2 files changed, 208 insertions(+), 20 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 66fd430356c56..41e633225132d 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -298,26 +298,13 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req return } - if organization.ID != template.OrganizationID { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: fmt.Sprintf("Template is not in organization %q.", organization.Name), - }) + if !api.Authorize(rw, r, rbac.ActionRead, template) { return } - _, err = api.Database.GetOrganizationMemberByUserID(r.Context(), database.GetOrganizationMemberByUserIDParams{ - OrganizationID: template.OrganizationID, - UserID: apiKey.UserID, - }) - if errors.Is(err, sql.ErrNoRows) { + + if organization.ID != template.OrganizationID { httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "You aren't allowed to access templates in that organization.", - }) - return - } - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: "Internal error fetching organization member.", - Detail: err.Error(), + Message: fmt.Sprintf("Template is not in organization %q.", organization.Name), }) return } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 4ac58edc3bb65..42e3718a1ee4d 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -4,18 +4,19 @@ import ( "context" "fmt" "net/http" + "strings" "testing" "time" - "github.com/coder/coder/coderd/rbac" - "github.com/coder/coder/coderd/util/ptr" - "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/autobuild/schedule" "github.com/coder/coder/coderd/coderdtest" + "github.com/coder/coder/coderd/rbac" + "github.com/coder/coder/coderd/util/ptr" "github.com/coder/coder/codersdk" + "github.com/coder/coder/cryptorand" "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" ) @@ -336,8 +337,200 @@ func TestWorkspaceByOwnerAndName(t *testing.T) { }) } +// TestWorkspaceFilter creates a set of workspaces, users, and organizations +// to run various filters against for testing. func TestWorkspaceFilter(t *testing.T) { t.Parallel() + type coderUser struct { + *codersdk.Client + User codersdk.User + Org codersdk.Organization + } + + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) + first := coderdtest.CreateFirstUser(t, client) + + users := make([]coderUser, 0) + for i := 0; i < 10; i++ { + userClient := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleAdmin()) + user, err := userClient.User(context.Background(), codersdk.Me) + require.NoError(t, err, "fetch me") + + org, err := userClient.CreateOrganization(context.Background(), codersdk.CreateOrganizationRequest{ + Name: user.Username + "-org", + }) + require.NoError(t, err, "create org") + + users = append(users, coderUser{ + Client: userClient, + User: user, + Org: org, + }) + } + + type madeWorkspace struct { + Owner codersdk.User + Workspace codersdk.Workspace + Template codersdk.Template + } + + availTemplates := make([]codersdk.Template, 0) + allWorkspaces := make([]madeWorkspace, 0) + + // Create some random workspaces + for i, user := range users { + version := coderdtest.CreateTemplateVersion(t, client, user.Org.ID, nil) + + // Create a template & workspace in the user's org + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + template := coderdtest.CreateTemplate(t, client, user.Org.ID, version.ID, func(request *codersdk.CreateTemplateRequest) { + // Have even templates share the same name for filter complexity. + if i%2 == 0 { + request.Name = "even-template" + } + }) + availTemplates = append(availTemplates, template) + workspace := coderdtest.CreateWorkspace(t, user.Client, template.OrganizationID, template.ID) + allWorkspaces = append(allWorkspaces, madeWorkspace{ + Workspace: workspace, + Template: template, + Owner: user.User, + }) + + // Make a workspace with a random template + idx, _ := cryptorand.Intn(len(availTemplates)) + randTemplate := availTemplates[idx] + randWorkspace := coderdtest.CreateWorkspace(t, user.Client, randTemplate.OrganizationID, randTemplate.ID) + allWorkspaces = append(allWorkspaces, madeWorkspace{ + Workspace: randWorkspace, + Template: randTemplate, + Owner: user.User, + }) + } + + // Make sure all workspaces are done. Do it after all are made + for i, w := range allWorkspaces { + latest := coderdtest.AwaitWorkspaceBuildJob(t, client, w.Workspace.LatestBuild.ID) + allWorkspaces[i].Workspace.LatestBuild = latest + } + + // --- Setup done --- + testCases := []struct { + Name string + Filter codersdk.WorkspaceFilter + // If FilterF is true, we include it in the expected results + FilterF func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool + }{ + { + Name: "All", + Filter: codersdk.WorkspaceFilter{}, + FilterF: func(_ codersdk.WorkspaceFilter, _ madeWorkspace) bool { + return true + }, + }, + { + Name: "Owner", + Filter: codersdk.WorkspaceFilter{ + Owner: users[must(cryptorand.Intn(len(users)))].User.Username, + }, + FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { + return workspace.Owner.Username == f.Owner + }, + }, + { + Name: "OrgID", + Filter: codersdk.WorkspaceFilter{ + OrganizationID: users[must(cryptorand.Intn(len(users)))].Org.ID, + }, + FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { + return workspace.Template.OrganizationID == f.OrganizationID + }, + }, + { + Name: "TemplateName", + Filter: codersdk.WorkspaceFilter{ + Template: "even-template", + }, + FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { + return workspace.Template.Name == f.Template + }, + }, + { + Name: "Template&Name", + Filter: codersdk.WorkspaceFilter{ + Template: "even-template", + // Use a common letter... one has to have this letter in it + Name: "a", + }, + FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { + return workspace.Template.Name == f.Template && strings.Contains(workspace.Workspace.Name, f.Name) + }, + }, + { + Name: "Q-Owner/Name", + Filter: codersdk.WorkspaceFilter{ + FilterQuery: allWorkspaces[5].Owner.Username + "/" + allWorkspaces[5].Workspace.Name, + }, + FilterF: func(_ codersdk.WorkspaceFilter, workspace madeWorkspace) bool { + return workspace.Workspace.ID == allWorkspaces[5].Workspace.ID + }, + }, + { + Name: "Org&Owner", + Filter: codersdk.WorkspaceFilter{ + OrganizationID: users[2].Org.ID, + Owner: users[2].User.Username, + }, + FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { + return workspace.Owner.Username == f.Owner && workspace.Template.OrganizationID == f.OrganizationID + }, + }, + { + Name: "Org&Owner", + Filter: codersdk.WorkspaceFilter{ + OrganizationID: users[2].Org.ID, + Owner: users[2].User.Username, + }, + FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { + return workspace.Owner.Username == f.Owner && workspace.Template.OrganizationID == f.OrganizationID + }, + }, + { + Name: "Many filters", + Filter: codersdk.WorkspaceFilter{ + OrganizationID: allWorkspaces[3].Template.OrganizationID, + Owner: allWorkspaces[3].Owner.Username, + Template: allWorkspaces[3].Template.Name, + Name: allWorkspaces[3].Workspace.Name, + }, + FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { + return workspace.Workspace.ID == allWorkspaces[3].Workspace.ID + }, + }, + } + + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + workspaces, err := client.Workspaces(context.Background(), c.Filter) + require.NoError(t, err, "fetch workspaces") + + exp := make([]codersdk.Workspace, 0) + for _, made := range allWorkspaces { + if c.FilterF(c.Filter, made) { + exp = append(exp, made.Workspace) + } + } + require.ElementsMatch(t, exp, workspaces, "expected workspaces returned") + }) + } +} + +// TestWorkspaceFilterManual runs some specific setups with basic checks. +func TestWorkspaceFilterManual(t *testing.T) { + t.Parallel() + t.Run("Name", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) @@ -910,3 +1103,11 @@ func mustLocation(t *testing.T, location string) *time.Location { return loc } + +func must[T any](s T, err error) T { + if err != nil { + panic(err) + } + + return s +} From ebcb8310ed86b5d061f0477c26364a5fd1cddb3d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 14:18:44 -0500 Subject: [PATCH 21/35] Fix unit test to not violate pg constraint --- coderd/workspaces_test.go | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 42e3718a1ee4d..c57777a39b82d 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -378,17 +378,12 @@ func TestWorkspaceFilter(t *testing.T) { allWorkspaces := make([]madeWorkspace, 0) // Create some random workspaces - for i, user := range users { + for _, user := range users { version := coderdtest.CreateTemplateVersion(t, client, user.Org.ID, nil) // Create a template & workspace in the user's org coderdtest.AwaitTemplateVersionJob(t, client, version.ID) - template := coderdtest.CreateTemplate(t, client, user.Org.ID, version.ID, func(request *codersdk.CreateTemplateRequest) { - // Have even templates share the same name for filter complexity. - if i%2 == 0 { - request.Name = "even-template" - } - }) + template := coderdtest.CreateTemplate(t, client, user.Org.ID, version.ID) availTemplates = append(availTemplates, template) workspace := coderdtest.CreateWorkspace(t, user.Client, template.OrganizationID, template.ID) allWorkspaces = append(allWorkspaces, madeWorkspace{ @@ -449,21 +444,20 @@ func TestWorkspaceFilter(t *testing.T) { { Name: "TemplateName", Filter: codersdk.WorkspaceFilter{ - Template: "even-template", + Template: allWorkspaces[5].Template.Name, }, FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { return workspace.Template.Name == f.Template }, }, { - Name: "Template&Name", + Name: "Name", Filter: codersdk.WorkspaceFilter{ - Template: "even-template", // Use a common letter... one has to have this letter in it Name: "a", }, FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { - return workspace.Template.Name == f.Template && strings.Contains(workspace.Workspace.Name, f.Name) + return strings.Contains(workspace.Workspace.Name, f.Name) }, }, { From 9368c481fa50b74ec5c9661ac39088bc388f62e5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 14:25:40 -0500 Subject: [PATCH 22/35] Drop owner_id from query params --- coderd/workspaces.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 41e633225132d..be890d029d07a 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -134,7 +134,6 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { filter := database.GetWorkspacesWithFilterParams{ Deleted: false, OrganizationID: parser.ParseUUID(r, uuid.Nil, "organization_id"), - OwnerID: parser.ParseUUIDorMe(r, uuid.Nil, apiKey.UserID, "owner_id"), OwnerUsername: parser.ParseString(r, "", "owner"), TemplateName: parser.ParseString(r, "", "template"), TemplateIds: parser.ParseUUIDArray(r, []uuid.UUID{}, "template_ids"), @@ -149,12 +148,6 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } if filter.OwnerUsername == "me" { - if !(filter.OwnerID == uuid.Nil || filter.OwnerID == apiKey.UserID) { - httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: "Cannot set both \"me\" in \"owner_name\" and use \"owner_id\".", - }) - return - } filter.OwnerID = apiKey.UserID filter.OwnerUsername = "" } From b5f570591f03e240277a7e134508ed1ab5c67c33 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 14:33:58 -0500 Subject: [PATCH 23/35] Remove unused code/params --- coderd/database/databasefake/databasefake.go | 22 ------ coderd/database/querier.go | 1 - coderd/database/queries.sql.go | 83 ++++---------------- coderd/database/queries/workspaces.sql | 9 --- coderd/workspaces.go | 10 +-- coderd/workspaces_test.go | 38 +-------- codersdk/workspaces.go | 4 - site/src/api/typesGenerated.ts | 1 - 8 files changed, 24 insertions(+), 144 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 3c3b33e03df77..d4c8522b1c1bb 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -321,9 +321,6 @@ func (q *fakeQuerier) GetWorkspacesWithFilter(_ context.Context, arg database.Ge workspaces := make([]database.Workspace, 0) for _, workspace := range q.workspaces { - if arg.OrganizationID != uuid.Nil && workspace.OrganizationID != arg.OrganizationID { - continue - } if arg.OwnerID != uuid.Nil && workspace.OwnerID != arg.OwnerID { continue } @@ -634,25 +631,6 @@ func (q *fakeQuerier) GetWorkspaceBuildByWorkspaceIDAndName(_ context.Context, a return database.WorkspaceBuild{}, sql.ErrNoRows } -func (q *fakeQuerier) GetWorkspacesByOrganizationIDs(_ context.Context, req database.GetWorkspacesByOrganizationIDsParams) ([]database.Workspace, error) { - q.mutex.RLock() - defer q.mutex.RUnlock() - - workspaces := make([]database.Workspace, 0) - for _, workspace := range q.workspaces { - for _, id := range req.Ids { - if workspace.OrganizationID != id { - continue - } - if workspace.Deleted != req.Deleted { - continue - } - workspaces = append(workspaces, workspace) - } - } - return workspaces, nil -} - func (q *fakeQuerier) GetOrganizations(_ context.Context) ([]database.Organization, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 15f2ddab601ea..4078a74576338 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -76,7 +76,6 @@ type querier interface { GetWorkspaceResourceByID(ctx context.Context, id uuid.UUID) (WorkspaceResource, error) GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.UUID) ([]WorkspaceResource, error) GetWorkspacesAutostart(ctx context.Context) ([]Workspace, error) - GetWorkspacesByOrganizationIDs(ctx context.Context, arg GetWorkspacesByOrganizationIDsParams) ([]Workspace, error) GetWorkspacesWithFilter(ctx context.Context, arg GetWorkspacesWithFilterParams) ([]Workspace, error) InsertAPIKey(ctx context.Context, arg InsertAPIKeyParams) (APIKey, error) InsertAuditLog(ctx context.Context, arg InsertAuditLogParams) (AuditLog, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a8027ef5d56df..46bea8c1704c8 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -3577,49 +3577,6 @@ func (q *sqlQuerier) GetWorkspacesAutostart(ctx context.Context) ([]Workspace, e return items, nil } -const getWorkspacesByOrganizationIDs = `-- name: GetWorkspacesByOrganizationIDs :many -SELECT id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, ttl FROM workspaces WHERE organization_id = ANY($1 :: uuid [ ]) AND deleted = $2 -` - -type GetWorkspacesByOrganizationIDsParams struct { - Ids []uuid.UUID `db:"ids" json:"ids"` - Deleted bool `db:"deleted" json:"deleted"` -} - -func (q *sqlQuerier) GetWorkspacesByOrganizationIDs(ctx context.Context, arg GetWorkspacesByOrganizationIDsParams) ([]Workspace, error) { - rows, err := q.db.QueryContext(ctx, getWorkspacesByOrganizationIDs, pq.Array(arg.Ids), arg.Deleted) - if err != nil { - return nil, err - } - defer rows.Close() - var items []Workspace - for rows.Next() { - var i Workspace - if err := rows.Scan( - &i.ID, - &i.CreatedAt, - &i.UpdatedAt, - &i.OwnerID, - &i.OrganizationID, - &i.TemplateID, - &i.Deleted, - &i.Name, - &i.AutostartSchedule, - &i.Ttl, - ); 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 getWorkspacesWithFilter = `-- name: GetWorkspacesWithFilter :many SELECT id, created_at, updated_at, owner_id, organization_id, template_id, deleted, name, autostart_schedule, ttl @@ -3628,60 +3585,52 @@ FROM WHERE -- Optionally include deleted workspaces workspaces.deleted = $1 - -- Filter by organization_id - AND CASE - WHEN $2 :: uuid != '00000000-00000000-00000000-00000000' THEN - organization_id = $2 - ELSE true - END -- Filter by owner_id AND CASE - WHEN $3 :: uuid != '00000000-00000000-00000000-00000000' THEN - owner_id = $3 + WHEN $2 :: uuid != '00000000-00000000-00000000-00000000' THEN + owner_id = $2 ELSE true END -- Filter by owner_name AND CASE - WHEN $4 :: text != '' THEN - owner_id = (SELECT id FROM users WHERE username = $4) + WHEN $3 :: text != '' THEN + owner_id = (SELECT id FROM users WHERE username = $3) 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 $5 :: text != '' THEN - template_id = ANY(SELECT id FROM templates WHERE name = $5) + WHEN $4 :: text != '' THEN + template_id = ANY(SELECT id FROM templates WHERE name = $4) ELSE true END -- Filter by template_ids AND CASE - WHEN array_length($6 :: uuid[], 1) > 0 THEN - template_id = ANY($6) + WHEN array_length($5 :: uuid[], 1) > 0 THEN + template_id = ANY($5) ELSE true END -- Filter by name, matching on substring AND CASE - WHEN $7 :: text != '' THEN - LOWER(name) LIKE '%' || LOWER($7) || '%' + WHEN $6 :: text != '' THEN + LOWER(name) LIKE '%' || LOWER($6) || '%' ELSE true END ` type GetWorkspacesWithFilterParams struct { - Deleted bool `db:"deleted" json:"deleted"` - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - OwnerID uuid.UUID `db:"owner_id" json:"owner_id"` - OwnerUsername string `db:"owner_username" json:"owner_username"` - TemplateName string `db:"template_name" json:"template_name"` - TemplateIds []uuid.UUID `db:"template_ids" json:"template_ids"` - Name string `db:"name" json:"name"` + Deleted bool `db:"deleted" json:"deleted"` + OwnerID uuid.UUID `db:"owner_id" json:"owner_id"` + OwnerUsername string `db:"owner_username" json:"owner_username"` + TemplateName string `db:"template_name" json:"template_name"` + TemplateIds []uuid.UUID `db:"template_ids" json:"template_ids"` + Name string `db:"name" json:"name"` } func (q *sqlQuerier) GetWorkspacesWithFilter(ctx context.Context, arg GetWorkspacesWithFilterParams) ([]Workspace, error) { rows, err := q.db.QueryContext(ctx, getWorkspacesWithFilter, arg.Deleted, - arg.OrganizationID, arg.OwnerID, arg.OwnerUsername, arg.TemplateName, diff --git a/coderd/database/queries/workspaces.sql b/coderd/database/queries/workspaces.sql index cd957116c95c1..dc68186fdc794 100644 --- a/coderd/database/queries/workspaces.sql +++ b/coderd/database/queries/workspaces.sql @@ -16,12 +16,6 @@ FROM WHERE -- Optionally include deleted workspaces workspaces.deleted = @deleted - -- Filter by organization_id - AND CASE - WHEN @organization_id :: uuid != '00000000-00000000-00000000-00000000' THEN - organization_id = @organization_id - ELSE true - END -- Filter by owner_id AND CASE WHEN @owner_id :: uuid != '00000000-00000000-00000000-00000000' THEN @@ -56,9 +50,6 @@ WHERE END ; --- name: GetWorkspacesByOrganizationIDs :many -SELECT * FROM workspaces WHERE organization_id = ANY(@ids :: uuid [ ]) AND deleted = @deleted; - -- name: GetWorkspacesAutostart :many SELECT * diff --git a/coderd/workspaces.go b/coderd/workspaces.go index be890d029d07a..fdc54e9b7948c 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -132,12 +132,10 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { parser := httpapi.NewQueryParamParser() filter := database.GetWorkspacesWithFilterParams{ - Deleted: false, - OrganizationID: parser.ParseUUID(r, uuid.Nil, "organization_id"), - OwnerUsername: parser.ParseString(r, "", "owner"), - TemplateName: parser.ParseString(r, "", "template"), - TemplateIds: parser.ParseUUIDArray(r, []uuid.UUID{}, "template_ids"), - Name: parser.ParseString(r, "", "name"), + Deleted: false, + OwnerUsername: parser.ParseString(r, "", "owner"), + TemplateName: parser.ParseString(r, "", "template"), + Name: parser.ParseString(r, "", "name"), } if len(parser.ValidationErrors()) > 0 { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index c57777a39b82d..59b3cd60f7242 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -432,15 +432,6 @@ func TestWorkspaceFilter(t *testing.T) { return workspace.Owner.Username == f.Owner }, }, - { - Name: "OrgID", - Filter: codersdk.WorkspaceFilter{ - OrganizationID: users[must(cryptorand.Intn(len(users)))].Org.ID, - }, - FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { - return workspace.Template.OrganizationID == f.OrganizationID - }, - }, { Name: "TemplateName", Filter: codersdk.WorkspaceFilter{ @@ -457,7 +448,7 @@ func TestWorkspaceFilter(t *testing.T) { Name: "a", }, FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { - return strings.Contains(workspace.Workspace.Name, f.Name) + return workspace.Template.Name == f.Template && strings.Contains(workspace.Workspace.Name, f.Name) }, }, { @@ -469,33 +460,12 @@ func TestWorkspaceFilter(t *testing.T) { return workspace.Workspace.ID == allWorkspaces[5].Workspace.ID }, }, - { - Name: "Org&Owner", - Filter: codersdk.WorkspaceFilter{ - OrganizationID: users[2].Org.ID, - Owner: users[2].User.Username, - }, - FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { - return workspace.Owner.Username == f.Owner && workspace.Template.OrganizationID == f.OrganizationID - }, - }, - { - Name: "Org&Owner", - Filter: codersdk.WorkspaceFilter{ - OrganizationID: users[2].Org.ID, - Owner: users[2].User.Username, - }, - FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { - return workspace.Owner.Username == f.Owner && workspace.Template.OrganizationID == f.OrganizationID - }, - }, { Name: "Many filters", Filter: codersdk.WorkspaceFilter{ - OrganizationID: allWorkspaces[3].Template.OrganizationID, - Owner: allWorkspaces[3].Owner.Username, - Template: allWorkspaces[3].Template.Name, - Name: allWorkspaces[3].Workspace.Name, + Owner: allWorkspaces[3].Owner.Username, + Template: allWorkspaces[3].Template.Name, + Name: allWorkspaces[3].Workspace.Name, }, FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { return workspace.Workspace.ID == allWorkspaces[3].Workspace.ID diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index 5c978e04febf2..009253e6933bd 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -217,7 +217,6 @@ func (c *Client) PutExtendWorkspace(ctx context.Context, id uuid.UUID, req PutEx } type WorkspaceFilter struct { - OrganizationID uuid.UUID `json:"organization_id,omitempty"` // Owner can be "me" or a username Owner string `json:"owner,omitempty"` // Template is a template name @@ -233,9 +232,6 @@ type WorkspaceFilter struct { func (f WorkspaceFilter) asRequestOption() requestOption { return func(r *http.Request) { q := r.URL.Query() - if f.OrganizationID != uuid.Nil { - q.Set("organization_id", f.OrganizationID.String()) - } if f.Owner != "" { q.Set("owner", f.Owner) } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 8ed4a20d8576c..e9767ec73f986 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -466,7 +466,6 @@ export interface WorkspaceBuildsRequest extends Pagination { // From codersdk/workspaces.go:219:6 export interface WorkspaceFilter { - readonly organization_id?: string readonly owner?: string readonly template?: string readonly name?: string From 4512a9b252079a4a0e08f51806be888db644eaa9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 14:53:33 -0500 Subject: [PATCH 24/35] Remove field from ts --- site/src/api/api.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 09c14645fbffb..66d851c2edde5 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -120,9 +120,6 @@ export const getWorkspacesURL = (filter?: TypesGen.WorkspaceFilter): string => { const basePath = "/api/v2/workspaces" const searchParams = new URLSearchParams() - if (filter?.organization_id) { - searchParams.append("organization_id", filter.organization_id) - } if (filter?.owner) { searchParams.append("owner", filter.owner) } From e9e913d032fbdd64de75680a51b25b2873ce404a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 15:21:23 -0500 Subject: [PATCH 25/35] Address PR comments --- coderd/httpapi/queryparams.go | 32 +++++++++++++----------------- coderd/httpapi/queryparams_test.go | 10 +++++----- coderd/pagination.go | 10 +++++----- coderd/workspaces.go | 13 ++++++------ 4 files changed, 31 insertions(+), 34 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 33a17dbd4a09c..aeae413fe074e 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -15,25 +15,21 @@ import ( // errors in 1 sweep. This means all invalid fields are returned at once, // rather than only returning the first error type QueryParamParser struct { - errors []Error + // Errors is the set of errors to return via the API. If the length + // of this set is 0, there are no errors!. + Errors []Error } func NewQueryParamParser() *QueryParamParser { return &QueryParamParser{ - errors: []Error{}, + Errors: []Error{}, } } -// ValidationErrors is the set of errors to return via the API. If the length -// of this set is 0, there was no errors. -func (p QueryParamParser) ValidationErrors() []Error { - return p.errors -} - -func (p *QueryParamParser) ParseInteger(r *http.Request, def int, queryParam string) int { +func (p *QueryParamParser) Integer(r *http.Request, def int, queryParam string) int { v, err := parse(r, strconv.Atoi, def, queryParam) if err != nil { - p.errors = append(p.errors, Error{ + p.Errors = append(p.Errors, Error{ Field: queryParam, Detail: fmt.Sprintf("Query param %q must be a valid integer (%s)", queryParam, err.Error()), }) @@ -41,17 +37,17 @@ func (p *QueryParamParser) ParseInteger(r *http.Request, def int, queryParam str return v } -func (p *QueryParamParser) ParseUUIDorMe(r *http.Request, def uuid.UUID, me uuid.UUID, queryParam string) uuid.UUID { +func (p *QueryParamParser) UUIDorMe(r *http.Request, def uuid.UUID, me uuid.UUID, queryParam string) uuid.UUID { if r.URL.Query().Get(queryParam) == "me" { return me } - return p.ParseUUID(r, def, queryParam) + return p.UUID(r, def, queryParam) } -func (p *QueryParamParser) ParseUUID(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { +func (p *QueryParamParser) UUID(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { v, err := parse(r, uuid.Parse, def, queryParam) if err != nil { - p.errors = append(p.errors, Error{ + p.Errors = append(p.Errors, Error{ Field: queryParam, Detail: fmt.Sprintf("Query param %q must be a valid uuid", queryParam), }) @@ -59,7 +55,7 @@ func (p *QueryParamParser) ParseUUID(r *http.Request, def uuid.UUID, queryParam return v } -func (p *QueryParamParser) ParseUUIDArray(r *http.Request, def []uuid.UUID, queryParam string) []uuid.UUID { +func (p *QueryParamParser) UUIDArray(r *http.Request, def []uuid.UUID, queryParam string) []uuid.UUID { v, err := parse(r, func(v string) ([]uuid.UUID, error) { var badValues []string strs := strings.Split(v, ",") @@ -79,7 +75,7 @@ func (p *QueryParamParser) ParseUUIDArray(r *http.Request, def []uuid.UUID, quer return ids, nil }, def, queryParam) if err != nil { - p.errors = append(p.errors, Error{ + p.Errors = append(p.Errors, Error{ Field: queryParam, Detail: fmt.Sprintf("Query param %q has invalid uuids: %q", queryParam, err.Error()), }) @@ -87,12 +83,12 @@ func (p *QueryParamParser) ParseUUIDArray(r *http.Request, def []uuid.UUID, quer return v } -func (p *QueryParamParser) ParseString(r *http.Request, def string, queryParam string) string { +func (p *QueryParamParser) String(r *http.Request, def string, queryParam string) string { v, err := parse(r, func(v string) (string, error) { return v, nil }, def, queryParam) if err != nil { - p.errors = append(p.errors, Error{ + p.Errors = append(p.Errors, Error{ Field: queryParam, Detail: fmt.Sprintf("Query param %q must be a valid string", queryParam), }) diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index d7bbc61782000..482d7df141a04 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -65,7 +65,7 @@ func TestParseQueryParams(t *testing.T) { parser := httpapi.NewQueryParamParser() testQueryParams(t, expParams, parser, func(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { - return parser.ParseUUIDorMe(r, def, me, queryParam) + return parser.UUIDorMe(r, def, me, queryParam) }) }) @@ -91,7 +91,7 @@ func TestParseQueryParams(t *testing.T) { } parser := httpapi.NewQueryParamParser() - testQueryParams(t, expParams, parser, parser.ParseString) + testQueryParams(t, expParams, parser, parser.String) }) t.Run("Integer", func(t *testing.T) { @@ -128,7 +128,7 @@ func TestParseQueryParams(t *testing.T) { } parser := httpapi.NewQueryParamParser() - testQueryParams(t, expParams, parser, parser.ParseInteger) + testQueryParams(t, expParams, parser, parser.Integer) }) t.Run("UUIDArray", func(t *testing.T) { @@ -171,7 +171,7 @@ func TestParseQueryParams(t *testing.T) { } parser := httpapi.NewQueryParamParser() - testQueryParams(t, expParams, parser, parser.ParseUUIDArray) + testQueryParams(t, expParams, parser, parser.UUIDArray) }) } @@ -193,7 +193,7 @@ func testQueryParams[T any](t *testing.T, testCases []queryParamTestCase[T], par v := parse(r, c.Default, c.QueryParam) require.Equal(t, c.Expected, v, fmt.Sprintf("param=%q value=%q", c.QueryParam, c.Value)) if c.ExpectedErrorContains != "" { - errors := parser.ValidationErrors() + errors := parser.Errors require.True(t, len(errors) > 0, "error exist") last := errors[len(errors)-1] require.True(t, last.Field == c.QueryParam, fmt.Sprintf("query param %q did not fail", c.QueryParam)) diff --git a/coderd/pagination.go b/coderd/pagination.go index 4cc9d5cfa1194..de84df69b89d4 100644 --- a/coderd/pagination.go +++ b/coderd/pagination.go @@ -14,15 +14,15 @@ import ( func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Pagination, ok bool) { parser := httpapi.NewQueryParamParser() params := codersdk.Pagination{ - AfterID: parser.ParseUUID(r, uuid.Nil, "after_id"), + AfterID: parser.UUID(r, uuid.Nil, "after_id"), // Limit default to "-1" which returns all results - Limit: parser.ParseInteger(r, -1, "limit"), - Offset: parser.ParseInteger(r, 0, "offset"), + Limit: parser.Integer(r, -1, "limit"), + Offset: parser.Integer(r, 0, "offset"), } - if len(parser.ValidationErrors()) > 0 { + if len(parser.Errors) > 0 { httpapi.Write(w, http.StatusBadRequest, httpapi.Response{ Message: "Query parameters have invalid values.", - Validations: parser.ValidationErrors(), + Validations: parser.Errors, }) return params, false } diff --git a/coderd/workspaces.go b/coderd/workspaces.go index fdc54e9b7948c..d149258d6e31e 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strconv" "time" @@ -116,7 +117,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } // Set all the query params from the "q" field. - q := r.URL.Query() + q := url.Values{} for k, v := range values { // Do not allow overriding if the user also set query param fields // outside the query string. @@ -133,14 +134,14 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { parser := httpapi.NewQueryParamParser() filter := database.GetWorkspacesWithFilterParams{ Deleted: false, - OwnerUsername: parser.ParseString(r, "", "owner"), - TemplateName: parser.ParseString(r, "", "template"), - Name: parser.ParseString(r, "", "name"), + OwnerUsername: parser.String(r, "", "owner"), + TemplateName: parser.String(r, "", "template"), + Name: parser.String(r, "", "name"), } - if len(parser.ValidationErrors()) > 0 { + if len(parser.Errors) > 0 { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ Message: "Query parameters have invalid values.", - Validations: parser.ValidationErrors(), + Validations: parser.Errors, }) return } From 2aac32b22a29c2562885915ae954e12884d1a9c4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 15:40:59 -0500 Subject: [PATCH 26/35] Fix js test --- coderd/workspaces.go | 3 +-- site/src/api/api.test.ts | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index d149258d6e31e..66fd318caa8cc 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "net/http" - "net/url" "strconv" "time" @@ -117,7 +116,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { } // Set all the query params from the "q" field. - q := url.Values{} + q := r.URL.Query() for k, v := range values { // Do not allow overriding if the user also set query param fields // outside the query string. diff --git a/site/src/api/api.test.ts b/site/src/api/api.test.ts index 083eb177fb6ac..394e3571228a6 100644 --- a/site/src/api/api.test.ts +++ b/site/src/api/api.test.ts @@ -118,10 +118,10 @@ describe("api.ts", () => { it.each<[TypesGen.WorkspaceFilter | undefined, string]>([ [undefined, "/api/v2/workspaces"], - [{ organization_id: "1", owner: "" }, "/api/v2/workspaces?organization_id=1"], - [{ organization_id: "", owner: "1" }, "/api/v2/workspaces?owner=1"], + [{ owner: "" }, "/api/v2/workspaces"], + [{ owner: "1" }, "/api/v2/workspaces?owner=1"], - [{ organization_id: "1", owner: "me" }, "/api/v2/workspaces?organization_id=1&owner=me"], + [{ owner: "me" }, "/api/v2/workspaces?owner=me"], ])(`getWorkspacesURL(%p) returns %p`, (filter, expected) => { expect(getWorkspacesURL(filter)).toBe(expected) }) From c8813cc43557197af1f4362ad8fb27812b278f79 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 10 Jun 2022 15:45:32 -0500 Subject: [PATCH 27/35] Fix unit test --- coderd/workspaces_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 59b3cd60f7242..d9324ce7ec82d 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -448,7 +448,7 @@ func TestWorkspaceFilter(t *testing.T) { Name: "a", }, FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { - return workspace.Template.Name == f.Template && strings.Contains(workspace.Workspace.Name, f.Name) + return strings.Contains(workspace.Workspace.Name, f.Name) }, }, { From ee3593500e82fcd2f9a7adfbd26fe4d3ee41410e Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 08:41:19 -0500 Subject: [PATCH 28/35] PR feedback - Unexport workspace search query function - move code into workspaces.go - use strings.SplitFields --- coderd/httpapi/queryparams.go | 2 +- coderd/httpapi/queryparams_test.go | 4 +- coderd/pagination.go | 4 +- coderd/workspaces.go | 66 ++++++++++++++- ...ch_test.go => workspaces_internal_test.go} | 5 +- coderd/workspaces_test.go | 2 +- coderd/workspacesearch.go | 80 ------------------- 7 files changed, 73 insertions(+), 90 deletions(-) rename coderd/{workspacesearch_test.go => workspaces_internal_test.go} (96%) delete mode 100644 coderd/workspacesearch.go diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index aeae413fe074e..8832829f97792 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -26,7 +26,7 @@ func NewQueryParamParser() *QueryParamParser { } } -func (p *QueryParamParser) Integer(r *http.Request, def int, queryParam string) int { +func (p *QueryParamParser) Int(r *http.Request, def int, queryParam string) int { v, err := parse(r, strconv.Atoi, def, queryParam) if err != nil { p.Errors = append(p.Errors, Error{ diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index 482d7df141a04..3c2f73606d800 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -94,7 +94,7 @@ func TestParseQueryParams(t *testing.T) { testQueryParams(t, expParams, parser, parser.String) }) - t.Run("Integer", func(t *testing.T) { + t.Run("Int", func(t *testing.T) { t.Parallel() expParams := []queryParamTestCase[int]{ { @@ -128,7 +128,7 @@ func TestParseQueryParams(t *testing.T) { } parser := httpapi.NewQueryParamParser() - testQueryParams(t, expParams, parser, parser.Integer) + testQueryParams(t, expParams, parser, parser.Int) }) t.Run("UUIDArray", func(t *testing.T) { diff --git a/coderd/pagination.go b/coderd/pagination.go index de84df69b89d4..c60013df05354 100644 --- a/coderd/pagination.go +++ b/coderd/pagination.go @@ -16,8 +16,8 @@ func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Paginat params := codersdk.Pagination{ AfterID: parser.UUID(r, uuid.Nil, "after_id"), // Limit default to "-1" which returns all results - Limit: parser.Integer(r, -1, "limit"), - Offset: parser.Integer(r, 0, "offset"), + Limit: parser.Int(r, -1, "limit"), + Offset: parser.Int(r, 0, "offset"), } if len(parser.Errors) > 0 { httpapi.Write(w, http.StatusBadRequest, httpapi.Response{ diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 66fd318caa8cc..de7b09f9c0c40 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -8,6 +8,7 @@ import ( "fmt" "net/http" "strconv" + "strings" "time" "github.com/go-chi/chi/v5" @@ -104,7 +105,7 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) queryStr := r.URL.Query().Get("q") - values, err := WorkspaceSearchQuery(queryStr) + values, err := workspaceSearchQuery(queryStr) if err != nil { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ Message: "Invalid workspace search query.", @@ -976,3 +977,66 @@ func validWorkspaceSchedule(s *string, min time.Duration) (sql.NullString, error String: *s, }, nil } + +// workspaceSearchQuery takes a query string and breaks it into its queryparams +// as a set of key=value. +func workspaceSearchQuery(query string) (map[string]string, error) { + searchParams := make(map[string]string) + if query == "" { + return searchParams, nil + } + // Because we do this in 2 passes, we want to maintain quotes on the first + // pass.Further splitting occurs on the second pass and quotes will be + // dropped. + elements := splitQueryParameterByDelimiter(query, ' ', true) + for _, element := range elements { + parts := splitQueryParameterByDelimiter(element, ':', false) + switch len(parts) { + case 1: + // No key:value pair. It is a workspace name, and maybe includes an owner + parts = splitQueryParameterByDelimiter(element, '/', false) + switch len(parts) { + case 1: + searchParams["name"] = parts[0] + case 2: + searchParams["owner"] = parts[0] + searchParams["name"] = parts[1] + default: + return nil, xerrors.Errorf("Query element %q can only contain 1 '/'", element) + } + case 2: + searchParams[parts[0]] = parts[1] + default: + return nil, xerrors.Errorf("Query element %q can only contain 1 ':'", element) + } + } + + return searchParams, 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. +// +// Although all our names cannot have spaces, that is a validation error. +// We should still parse the quoted string as a single value so that validation +// can properly fail on the space. If we do not, a value of `template:"my name"` +// will search `template:"my name:name"`, which produces an empty list instead of +// an error. +// nolint:revive +func splitQueryParameterByDelimiter(query string, delimiter rune, maintainQuotes bool) []string { + quoted := false + parts := strings.FieldsFunc(query, func(r rune) bool { + if r == '"' { + quoted = !quoted + } + return !quoted && r == delimiter + }) + if !maintainQuotes { + for i, part := range parts { + parts[i] = strings.Trim(part, "\"") + } + } + + return parts +} diff --git a/coderd/workspacesearch_test.go b/coderd/workspaces_internal_test.go similarity index 96% rename from coderd/workspacesearch_test.go rename to coderd/workspaces_internal_test.go index d49d8424c2dab..d3c659dd153a1 100644 --- a/coderd/workspacesearch_test.go +++ b/coderd/workspaces_internal_test.go @@ -1,9 +1,8 @@ -package coderd_test +package coderd import ( "testing" - "github.com/coder/coder/coderd" "github.com/stretchr/testify/require" ) @@ -124,7 +123,7 @@ func TestSearchWorkspace(t *testing.T) { c := c t.Run(c.Name, func(t *testing.T) { t.Parallel() - values, err := coderd.WorkspaceSearchQuery(c.Query) + values, err := workspaceSearchQuery(c.Query) if c.ExpectedErrorContains != "" { require.Error(t, err, "expected error") require.ErrorContains(t, err, c.ExpectedErrorContains) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index d9324ce7ec82d..3c2dc315ac1d2 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -426,7 +426,7 @@ func TestWorkspaceFilter(t *testing.T) { { Name: "Owner", Filter: codersdk.WorkspaceFilter{ - Owner: users[must(cryptorand.Intn(len(users)))].User.Username, + Owner: users[2].User.Username, }, FilterF: func(f codersdk.WorkspaceFilter, workspace madeWorkspace) bool { return workspace.Owner.Username == f.Owner diff --git a/coderd/workspacesearch.go b/coderd/workspacesearch.go deleted file mode 100644 index e1a17612caf61..0000000000000 --- a/coderd/workspacesearch.go +++ /dev/null @@ -1,80 +0,0 @@ -package coderd - -import ( - "strings" - - "golang.org/x/xerrors" -) - -// WorkspaceSearchQuery takes a query string and breaks it into its queryparams -// as a set of key=value. -func WorkspaceSearchQuery(query string) (map[string]string, error) { - searchParams := make(map[string]string) - if query == "" { - return searchParams, nil - } - // Because we do this in 2 passes, we want to maintain quotes on the first - // pass.Further splitting occurs on the second pass and quotes will be - // dropped. - elements := splitElements(query, ' ', true) - for _, element := range elements { - parts := splitElements(element, ':', false) - switch len(parts) { - case 1: - // No key:value pair. It is a workspace name, and maybe includes an owner - parts = splitElements(element, '/', false) - switch len(parts) { - case 1: - searchParams["name"] = parts[0] - case 2: - searchParams["owner"] = parts[0] - searchParams["name"] = parts[1] - default: - return nil, xerrors.Errorf("Query element %q can only contain 1 '/'", element) - } - case 2: - searchParams[parts[0]] = parts[1] - default: - return nil, xerrors.Errorf("Query element %q can only contain 1 ':'", element) - } - } - - return searchParams, nil -} - -// splitElements 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. -// -// Although all our names cannot have spaces, that is a validation error. -// We should still parse the quoted string as a single value so that validation -// can properly fail on the space. If we do not, a value of `template:"my name"` -// will search `template:"my name:name"`, which produces an empty list instead of -// an error. -// nolint:revive -func splitElements(query string, delimiter rune, maintainQuotes bool) []string { - var parts []string - - quoted := false - var current strings.Builder - for _, c := range query { - switch c { - case '"': - if maintainQuotes { - _, _ = current.WriteRune(c) - } - quoted = !quoted - case delimiter: - if quoted { - _, _ = current.WriteRune(c) - } else { - parts = append(parts, current.String()) - current = strings.Builder{} - } - default: - _, _ = current.WriteRune(c) - } - } - parts = append(parts, current.String()) - return parts -} From 4ad585e9804764925d6505116af9dfd38574c3c3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 08:43:51 -0500 Subject: [PATCH 29/35] Rename vague function name --- coderd/httpapi/queryparams.go | 12 ++++++------ coderd/httpapi/queryparams_test.go | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 8832829f97792..eca8d98f9f56b 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -27,7 +27,7 @@ func NewQueryParamParser() *QueryParamParser { } func (p *QueryParamParser) Int(r *http.Request, def int, queryParam string) int { - v, err := parse(r, strconv.Atoi, def, queryParam) + v, err := parseQueryParam(r, strconv.Atoi, def, queryParam) if err != nil { p.Errors = append(p.Errors, Error{ Field: queryParam, @@ -45,7 +45,7 @@ func (p *QueryParamParser) UUIDorMe(r *http.Request, def uuid.UUID, me uuid.UUID } func (p *QueryParamParser) UUID(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { - v, err := parse(r, uuid.Parse, def, queryParam) + v, err := parseQueryParam(r, uuid.Parse, def, queryParam) if err != nil { p.Errors = append(p.Errors, Error{ Field: queryParam, @@ -55,8 +55,8 @@ func (p *QueryParamParser) UUID(r *http.Request, def uuid.UUID, queryParam strin return v } -func (p *QueryParamParser) UUIDArray(r *http.Request, def []uuid.UUID, queryParam string) []uuid.UUID { - v, err := parse(r, func(v string) ([]uuid.UUID, error) { +func (p *QueryParamParser) UUIDs(r *http.Request, def []uuid.UUID, queryParam string) []uuid.UUID { + v, err := parseQueryParam(r, func(v string) ([]uuid.UUID, error) { var badValues []string strs := strings.Split(v, ",") ids := make([]uuid.UUID, 0, len(strs)) @@ -84,7 +84,7 @@ func (p *QueryParamParser) UUIDArray(r *http.Request, def []uuid.UUID, queryPara } func (p *QueryParamParser) String(r *http.Request, def string, queryParam string) string { - v, err := parse(r, func(v string) (string, error) { + v, err := parseQueryParam(r, func(v string) (string, error) { return v, nil }, def, queryParam) if err != nil { @@ -96,7 +96,7 @@ func (p *QueryParamParser) String(r *http.Request, def string, queryParam string return v } -func parse[T any](r *http.Request, parse func(v string) (T, error), def T, queryParam string) (T, error) { +func parseQueryParam[T any](r *http.Request, parse func(v string) (T, error), def T, queryParam string) (T, error) { if !r.URL.Query().Has(queryParam) || r.URL.Query().Get(queryParam) == "" { return def, nil } diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index 3c2f73606d800..f7f4f61b274c3 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -131,7 +131,7 @@ func TestParseQueryParams(t *testing.T) { testQueryParams(t, expParams, parser, parser.Int) }) - t.Run("UUIDArray", func(t *testing.T) { + t.Run("UUIDs", func(t *testing.T) { t.Parallel() expParams := []queryParamTestCase[[]uuid.UUID]{ { @@ -171,7 +171,7 @@ func TestParseQueryParams(t *testing.T) { } parser := httpapi.NewQueryParamParser() - testQueryParams(t, expParams, parser, parser.UUIDArray) + testQueryParams(t, expParams, parser, parser.UUIDs) }) } From 6251493531c487a952a935577fe93401ff668a9f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 09:07:10 -0500 Subject: [PATCH 30/35] WorkspaceSearchQuery now returns db filter --- coderd/httpapi/queryparams.go | 30 +++++------ coderd/httpapi/queryparams_test.go | 11 ++-- coderd/pagination.go | 7 +-- coderd/workspaces.go | 80 ++++++++++++------------------ coderd/workspaces_internal_test.go | 16 ++++-- coderd/workspaces_test.go | 8 --- codersdk/workspaces.go | 23 ++++++--- 7 files changed, 82 insertions(+), 93 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index eca8d98f9f56b..ea30480e16b4e 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -2,7 +2,7 @@ package httpapi import ( "fmt" - "net/http" + "net/url" "strconv" "strings" @@ -26,8 +26,8 @@ func NewQueryParamParser() *QueryParamParser { } } -func (p *QueryParamParser) Int(r *http.Request, def int, queryParam string) int { - v, err := parseQueryParam(r, strconv.Atoi, def, queryParam) +func (p *QueryParamParser) Int(vals url.Values, def int, queryParam string) int { + v, err := parseQueryParam(vals, strconv.Atoi, def, queryParam) if err != nil { p.Errors = append(p.Errors, Error{ Field: queryParam, @@ -37,15 +37,15 @@ func (p *QueryParamParser) Int(r *http.Request, def int, queryParam string) int return v } -func (p *QueryParamParser) UUIDorMe(r *http.Request, def uuid.UUID, me uuid.UUID, queryParam string) uuid.UUID { - if r.URL.Query().Get(queryParam) == "me" { +func (p *QueryParamParser) UUIDorMe(vals url.Values, def uuid.UUID, me uuid.UUID, queryParam string) uuid.UUID { + if vals.Get(queryParam) == "me" { return me } - return p.UUID(r, def, queryParam) + return p.UUID(vals, def, queryParam) } -func (p *QueryParamParser) UUID(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { - v, err := parseQueryParam(r, uuid.Parse, def, queryParam) +func (p *QueryParamParser) UUID(vals url.Values, def uuid.UUID, queryParam string) uuid.UUID { + v, err := parseQueryParam(vals, uuid.Parse, def, queryParam) if err != nil { p.Errors = append(p.Errors, Error{ Field: queryParam, @@ -55,8 +55,8 @@ func (p *QueryParamParser) UUID(r *http.Request, def uuid.UUID, queryParam strin return v } -func (p *QueryParamParser) UUIDs(r *http.Request, def []uuid.UUID, queryParam string) []uuid.UUID { - v, err := parseQueryParam(r, func(v string) ([]uuid.UUID, error) { +func (p *QueryParamParser) UUIDs(vals url.Values, def []uuid.UUID, queryParam string) []uuid.UUID { + v, err := parseQueryParam(vals, func(v string) ([]uuid.UUID, error) { var badValues []string strs := strings.Split(v, ",") ids := make([]uuid.UUID, 0, len(strs)) @@ -83,8 +83,8 @@ func (p *QueryParamParser) UUIDs(r *http.Request, def []uuid.UUID, queryParam st return v } -func (p *QueryParamParser) String(r *http.Request, def string, queryParam string) string { - v, err := parseQueryParam(r, func(v string) (string, error) { +func (p *QueryParamParser) String(vals url.Values, def string, queryParam string) string { + v, err := parseQueryParam(vals, func(v string) (string, error) { return v, nil }, def, queryParam) if err != nil { @@ -96,10 +96,10 @@ func (p *QueryParamParser) String(r *http.Request, def string, queryParam string return v } -func parseQueryParam[T any](r *http.Request, parse func(v string) (T, error), def T, queryParam string) (T, error) { - if !r.URL.Query().Has(queryParam) || r.URL.Query().Get(queryParam) == "" { +func parseQueryParam[T any](vals url.Values, parse func(v string) (T, error), def T, queryParam string) (T, error) { + if !vals.Has(queryParam) || vals.Get(queryParam) == "" { return def, nil } - str := r.URL.Query().Get(queryParam) + str := vals.Get(queryParam) return parse(str) } diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index f7f4f61b274c3..f4ff580b4dc22 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -64,8 +64,8 @@ func TestParseQueryParams(t *testing.T) { } parser := httpapi.NewQueryParamParser() - testQueryParams(t, expParams, parser, func(r *http.Request, def uuid.UUID, queryParam string) uuid.UUID { - return parser.UUIDorMe(r, def, me, queryParam) + testQueryParams(t, expParams, parser, func(vals url.Values, def uuid.UUID, queryParam string) uuid.UUID { + return parser.UUIDorMe(vals, def, me, queryParam) }) }) @@ -175,7 +175,7 @@ func TestParseQueryParams(t *testing.T) { }) } -func testQueryParams[T any](t *testing.T, testCases []queryParamTestCase[T], parser *httpapi.QueryParamParser, parse func(r *http.Request, def T, queryParam string) T) { +func testQueryParams[T any](t *testing.T, testCases []queryParamTestCase[T], parser *httpapi.QueryParamParser, parse func(vals url.Values, def T, queryParam string) T) { v := url.Values{} for _, c := range testCases { if c.NoSet { @@ -183,14 +183,11 @@ func testQueryParams[T any](t *testing.T, testCases []queryParamTestCase[T], par } v.Set(c.QueryParam, c.Value) } - r, err := http.NewRequest("GET", "https://example.com", nil) - require.NoError(t, err, "new request") - r.URL.RawQuery = v.Encode() for _, c := range testCases { // !! Do not run these in parallel !! t.Run(c.QueryParam, func(t *testing.T) { - v := parse(r, c.Default, c.QueryParam) + v := parse(v, c.Default, c.QueryParam) require.Equal(t, c.Expected, v, fmt.Sprintf("param=%q value=%q", c.QueryParam, c.Value)) if c.ExpectedErrorContains != "" { errors := parser.Errors diff --git a/coderd/pagination.go b/coderd/pagination.go index c60013df05354..07f7b0fe743db 100644 --- a/coderd/pagination.go +++ b/coderd/pagination.go @@ -12,12 +12,13 @@ import ( // parsePagination extracts pagination query params from the http request. // If an error is encountered, the error is written to w and ok is set to false. func parsePagination(w http.ResponseWriter, r *http.Request) (p codersdk.Pagination, ok bool) { + queryParams := r.URL.Query() parser := httpapi.NewQueryParamParser() params := codersdk.Pagination{ - AfterID: parser.UUID(r, uuid.Nil, "after_id"), + AfterID: parser.UUID(queryParams, uuid.Nil, "after_id"), // Limit default to "-1" which returns all results - Limit: parser.Int(r, -1, "limit"), - Offset: parser.Int(r, 0, "offset"), + Limit: parser.Int(queryParams, -1, "limit"), + Offset: parser.Int(queryParams, 0, "offset"), } if len(parser.Errors) > 0 { httpapi.Write(w, http.StatusBadRequest, httpapi.Response{ diff --git a/coderd/workspaces.go b/coderd/workspaces.go index de7b09f9c0c40..920adb85295da 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strconv" "strings" "time" @@ -105,43 +106,11 @@ func (api *API) workspaces(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) queryStr := r.URL.Query().Get("q") - values, err := workspaceSearchQuery(queryStr) - if err != nil { + filter, errs := workspaceSearchQuery(queryStr) + if len(errs) > 0 { httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: "Invalid workspace search query.", - Validations: []httpapi.Error{ - {Field: "q", Detail: err.Error()}, - }, - }) - return - } - - // Set all the query params from the "q" field. - q := r.URL.Query() - for k, v := range values { - // Do not allow overriding if the user also set query param fields - // outside the query string. - if q.Has(k) { - httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: fmt.Sprintf("Workspace filter %q cannot be set twice. In query params %q and %q.", k, k, "q"), - }) - return - } - q.Set(k, v) - } - r.URL.RawQuery = q.Encode() - - parser := httpapi.NewQueryParamParser() - filter := database.GetWorkspacesWithFilterParams{ - Deleted: false, - OwnerUsername: parser.String(r, "", "owner"), - TemplateName: parser.String(r, "", "template"), - Name: parser.String(r, "", "name"), - } - if len(parser.Errors) > 0 { - httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: "Query parameters have invalid values.", - Validations: parser.Errors, + Message: "Invalid workspace search query.", + Validations: errs, }) return } @@ -978,12 +947,13 @@ func validWorkspaceSchedule(s *string, min time.Duration) (sql.NullString, error }, nil } -// workspaceSearchQuery takes a query string and breaks it into its queryparams -// as a set of key=value. -func workspaceSearchQuery(query string) (map[string]string, error) { - searchParams := make(map[string]string) +// workspaceSearchQuery takes a query string and returns the workspace filter. +// It also can return the list of validation errors to return to the api. +func workspaceSearchQuery(query string) (database.GetWorkspacesWithFilterParams, []httpapi.Error) { + searchParams := make(url.Values) if query == "" { - return searchParams, nil + // No filter + return database.GetWorkspacesWithFilterParams{}, nil } // Because we do this in 2 passes, we want to maintain quotes on the first // pass.Further splitting occurs on the second pass and quotes will be @@ -997,21 +967,35 @@ func workspaceSearchQuery(query string) (map[string]string, error) { parts = splitQueryParameterByDelimiter(element, '/', false) switch len(parts) { case 1: - searchParams["name"] = parts[0] + searchParams.Set("name", parts[0]) case 2: - searchParams["owner"] = parts[0] - searchParams["name"] = parts[1] + searchParams.Set("owner", parts[0]) + searchParams.Set("name", parts[1]) default: - return nil, xerrors.Errorf("Query element %q can only contain 1 '/'", element) + return database.GetWorkspacesWithFilterParams{}, []httpapi.Error{ + {Field: "q", Detail: fmt.Sprintf("Query element %q can only contain 1 '/'", element)}, + } } case 2: - searchParams[parts[0]] = parts[1] + searchParams.Set(parts[0], parts[1]) default: - return nil, xerrors.Errorf("Query element %q can only contain 1 ':'", element) + return database.GetWorkspacesWithFilterParams{}, []httpapi.Error{ + {Field: "q", Detail: fmt.Sprintf("Query element %q can only contain 1 ':'", element)}, + } } } - return searchParams, nil + // Using the query param parser here just returns consistent errors with + // other parsing. + parser := httpapi.NewQueryParamParser() + filter := database.GetWorkspacesWithFilterParams{ + Deleted: false, + OwnerUsername: parser.String(searchParams, "", "owner"), + TemplateName: parser.String(searchParams, "", "template"), + Name: parser.String(searchParams, "", "name"), + } + + return filter, parser.Errors } // splitQueryParameterByDelimiter takes a query string and splits it into the individual elements diff --git a/coderd/workspaces_internal_test.go b/coderd/workspaces_internal_test.go index d3c659dd153a1..af00030f36195 100644 --- a/coderd/workspaces_internal_test.go +++ b/coderd/workspaces_internal_test.go @@ -1,8 +1,12 @@ package coderd import ( + "fmt" + "strings" "testing" + "github.com/coder/coder/coderd/httpapi" + "github.com/stretchr/testify/require" ) @@ -123,12 +127,16 @@ func TestSearchWorkspace(t *testing.T) { c := c t.Run(c.Name, func(t *testing.T) { t.Parallel() - values, err := workspaceSearchQuery(c.Query) + values, errs := workspaceSearchQuery(c.Query) if c.ExpectedErrorContains != "" { - require.Error(t, err, "expected error") - require.ErrorContains(t, err, c.ExpectedErrorContains) + require.True(t, len(errs) > 0, "expect some errors") + var s strings.Builder + for _, err := range errs { + _, _ = s.WriteString(fmt.Sprintf("%s: %s\n", err.Field, err.Detail)) + } + require.Contains(t, s.String(), c.ExpectedErrorContains) } else { - require.NoError(t, err, "expected no error") + require.Equal(t, []httpapi.Error{}, errs, "expected no error") require.Equal(t, c.Expected, values, "expected values") } }) diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 3c2dc315ac1d2..9e207362fe18d 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -1067,11 +1067,3 @@ func mustLocation(t *testing.T, location string) *time.Location { return loc } - -func must[T any](s T, err error) T { - if err != nil { - panic(err) - } - - return s -} diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index 009253e6933bd..f5139c8e5200a 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "strings" "time" "github.com/google/uuid" @@ -218,11 +219,11 @@ func (c *Client) PutExtendWorkspace(ctx context.Context, id uuid.UUID, req PutEx type WorkspaceFilter struct { // Owner can be "me" or a username - Owner string `json:"owner,omitempty"` + Owner string `json:"owner,omitempty" typescript:"-"` // Template is a template name - Template string `json:"template,omitempty"` + Template string `json:"template,omitempty" typescript:"-"` // Name will return partial matches - Name string `json:"name,omitempty"` + Name string `json:"name,omitempty" typescript:"-"` // FilterQuery supports a raw filter query string FilterQuery string `json:"q,omitempty"` } @@ -231,19 +232,25 @@ type WorkspaceFilter struct { // It modifies the request query parameters. func (f WorkspaceFilter) asRequestOption() requestOption { return func(r *http.Request) { - q := r.URL.Query() + var params []string + // Make sure all user input is quoted to ensure it's parsed as a single + // string. if f.Owner != "" { - q.Set("owner", f.Owner) + params = append(params, fmt.Sprintf("owner:%q", f.Owner)) } if f.Name != "" { - q.Set("name", f.Name) + params = append(params, fmt.Sprintf("name:%q", f.Name)) } if f.Template != "" { - q.Set("template", f.Template) + params = append(params, fmt.Sprintf("template:%q", f.Template)) } if f.FilterQuery != "" { - q.Set("q", f.FilterQuery) + // If custom stuff is added, just add it on here. + params = append(params, f.FilterQuery) } + + q := r.URL.Query() + q.Set("q", strings.Join(params, " ")) r.URL.RawQuery = q.Encode() } } From ac8dddd0a1860b307f6480ee6b094730c9f05581 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 09:34:08 -0500 Subject: [PATCH 31/35] Correct unit test and typescript --- coderd/workspaces_internal_test.go | 78 +++++++++++++++--------------- site/src/api/api.test.ts | 6 +-- site/src/api/api.ts | 7 +-- site/src/api/typesGenerated.ts | 19 +++----- site/src/util/workspace.test.ts | 12 ++--- site/src/util/workspace.ts | 37 +------------- 6 files changed, 60 insertions(+), 99 deletions(-) diff --git a/coderd/workspaces_internal_test.go b/coderd/workspaces_internal_test.go index af00030f36195..dc783b417ebfc 100644 --- a/coderd/workspaces_internal_test.go +++ b/coderd/workspaces_internal_test.go @@ -5,7 +5,7 @@ import ( "strings" "testing" - "github.com/coder/coder/coderd/httpapi" + "github.com/coder/coder/coderd/database" "github.com/stretchr/testify/require" ) @@ -15,98 +15,98 @@ func TestSearchWorkspace(t *testing.T) { testCases := []struct { Name string Query string - Expected map[string]string + Expected database.GetWorkspacesWithFilterParams ExpectedErrorContains string }{ { Name: "Empty", Query: "", - Expected: map[string]string{}, + Expected: database.GetWorkspacesWithFilterParams{}, }, { Name: "Owner/Name", Query: "Foo/Bar", - Expected: map[string]string{ - "owner": "Foo", - "name": "Bar", + Expected: database.GetWorkspacesWithFilterParams{ + OwnerUsername: "Foo", + Name: "Bar", }, }, { Name: "Name", Query: "workspace-name", - Expected: map[string]string{ - "name": "workspace-name", + Expected: database.GetWorkspacesWithFilterParams{ + Name: "workspace-name", }, }, { Name: "Name+Param", Query: "workspace-name template:docker", - Expected: map[string]string{ - "name": "workspace-name", - "template": "docker", + Expected: database.GetWorkspacesWithFilterParams{ + Name: "workspace-name", + TemplateName: "docker", }, }, { Name: "OnlyParams", Query: "name:workspace-name template:docker owner:alice", - Expected: map[string]string{ - "owner": "alice", - "name": "workspace-name", - "template": "docker", + Expected: database.GetWorkspacesWithFilterParams{ + Name: "workspace-name", + TemplateName: "docker", + OwnerUsername: "alice", }, }, { Name: "QuotedParam", Query: `name:workspace-name template:"docker template" owner:alice`, - Expected: map[string]string{ - "owner": "alice", - "name": "workspace-name", - "template": "docker template", + Expected: database.GetWorkspacesWithFilterParams{ + Name: "workspace-name", + TemplateName: "docker template", + OwnerUsername: "alice", }, }, { Name: "QuotedKey", - Query: `"spaced key":"spaced value"`, - Expected: map[string]string{ - "spaced key": "spaced value", + Query: `"name":baz "template":foo "owner":bar`, + Expected: database.GetWorkspacesWithFilterParams{ + Name: "baz", + TemplateName: "foo", + OwnerUsername: "bar", }, }, { // This will not return an error - Name: "ExtraKeys", - Query: `foo:bar`, - Expected: map[string]string{ - "foo": "bar", - }, + Name: "ExtraKeys", + Query: `foo:bar`, + Expected: database.GetWorkspacesWithFilterParams{}, }, { // Quotes keep elements together Name: "QuotedSpecial", Query: `name:"workspace:name"`, - Expected: map[string]string{ - "name": "workspace:name", + Expected: database.GetWorkspacesWithFilterParams{ + Name: "workspace:name", }, }, { Name: "QuotedMadness", - Query: `"key:is:wild/a/b/c":"foo:bar/baz/zoo:zonk"`, - Expected: map[string]string{ - "key:is:wild/a/b/c": "foo:bar/baz/zoo:zonk", + Query: `"name":"foo:bar:baz/baz/zoo:zonk"`, + Expected: database.GetWorkspacesWithFilterParams{ + Name: "foo:bar:baz/baz/zoo:zonk", }, }, { - Name: "QuotesName", + Name: "QuotedName", Query: `"foo/bar"`, - Expected: map[string]string{ - "name": "foo/bar", + Expected: database.GetWorkspacesWithFilterParams{ + Name: "foo/bar", }, }, { Name: "QuotedOwner/Name", Query: `"foo"/"bar"`, - Expected: map[string]string{ - "owner": "foo", - "name": "bar", + Expected: database.GetWorkspacesWithFilterParams{ + Name: "bar", + OwnerUsername: "foo", }, }, @@ -136,7 +136,7 @@ func TestSearchWorkspace(t *testing.T) { } require.Contains(t, s.String(), c.ExpectedErrorContains) } else { - require.Equal(t, []httpapi.Error{}, errs, "expected no error") + require.Len(t, errs, 0, "expected no error") require.Equal(t, c.Expected, values, "expected values") } }) diff --git a/site/src/api/api.test.ts b/site/src/api/api.test.ts index 394e3571228a6..3c608f3559004 100644 --- a/site/src/api/api.test.ts +++ b/site/src/api/api.test.ts @@ -118,10 +118,10 @@ describe("api.ts", () => { it.each<[TypesGen.WorkspaceFilter | undefined, string]>([ [undefined, "/api/v2/workspaces"], - [{ owner: "" }, "/api/v2/workspaces"], - [{ owner: "1" }, "/api/v2/workspaces?owner=1"], + [{ q:"" }, "/api/v2/workspaces"], + [{ q:"owner:1" }, "/api/v2/workspaces?q=owner:1"], - [{ owner: "me" }, "/api/v2/workspaces?owner=me"], + [{ q:"owner:me" }, "/api/v2/workspaces?q=owner:me"], ])(`getWorkspacesURL(%p) returns %p`, (filter, expected) => { expect(getWorkspacesURL(filter)).toBe(expected) }) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 66d851c2edde5..5df7f85bee217 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -120,11 +120,8 @@ export const getWorkspacesURL = (filter?: TypesGen.WorkspaceFilter): string => { const basePath = "/api/v2/workspaces" const searchParams = new URLSearchParams() - if (filter?.owner) { - searchParams.append("owner", filter.owner) - } - if (filter?.name) { - searchParams.append("name", filter.name) + if (filter?.q && filter.q != "") { + searchParams.append("q", filter.q) } const searchString = searchParams.toString() diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 8db2be74f052a..d63b9b46c3505 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -90,7 +90,7 @@ export interface CreateUserRequest { readonly organization_id: string } -// From codersdk/workspaces.go:34:6 +// From codersdk/workspaces.go:35:6 export interface CreateWorkspaceBuildRequest { readonly template_version_id?: string readonly transition: WorkspaceTransition @@ -223,7 +223,7 @@ export interface ProvisionerJobLog { readonly output: string } -// From codersdk/workspaces.go:201:6 +// From codersdk/workspaces.go:202:6 export interface PutExtendWorkspaceRequest { readonly deadline: string } @@ -311,12 +311,12 @@ export interface UpdateUserProfileRequest { readonly username: string } -// From codersdk/workspaces.go:160:6 +// From codersdk/workspaces.go:161:6 export interface UpdateWorkspaceAutostartRequest { readonly schedule?: string } -// From codersdk/workspaces.go:180:6 +// From codersdk/workspaces.go:181:6 export interface UpdateWorkspaceTTLRequest { readonly ttl_ms?: number } @@ -371,7 +371,7 @@ export interface UsersRequest extends Pagination { readonly status?: string } -// From codersdk/workspaces.go:18:6 +// From codersdk/workspaces.go:19:6 export interface Workspace { readonly id: string readonly created_at: string @@ -461,20 +461,17 @@ export interface WorkspaceBuild { readonly deadline: string } -// From codersdk/workspaces.go:83:6 +// From codersdk/workspaces.go:84:6 export interface WorkspaceBuildsRequest extends Pagination { readonly WorkspaceID: string } -// From codersdk/workspaces.go:219:6 +// From codersdk/workspaces.go:220:6 export interface WorkspaceFilter { - readonly owner?: string - readonly template?: string - readonly name?: string readonly q?: string } -// From codersdk/workspaces.go:41:6 +// From codersdk/workspaces.go:42:6 export interface WorkspaceOptions { readonly include_deleted?: boolean } diff --git a/site/src/util/workspace.test.ts b/site/src/util/workspace.test.ts index a7789dfbf8285..175789ad6bec2 100644 --- a/site/src/util/workspace.test.ts +++ b/site/src/util/workspace.test.ts @@ -105,12 +105,12 @@ describe("util > workspace", () => { it.each<[string | undefined, TypesGen.WorkspaceFilter]>([ [undefined, {}], ["", {}], - ["asdkfvjn", { name: "asdkfvjn" }], - ["owner:me", { owner: "me" }], - ["owner:me owner:me2", { owner: "me" }], - ["me/dev", { owner: "me", name: "dev" }], - ["me/", { owner: "me" }], - [" key:val owner:me ", { owner: "me" }], + ["asdkfvjn", { q: "asdkfvjn" }], + ["owner:me", { q: "owner:me" }], + ["owner:me owner:me2", { q: "owner:me owner:me2" }], + ["me/dev", { q: "me/dev" }], + ["me/", { q: "me/" }], + [" key:val owner:me ", { q: "key:val owner:me" }], ])(`query=%p, filter=%p`, (query, filter) => { expect(workspaceQueryToFilter(query)).toEqual(filter) }) diff --git a/site/src/util/workspace.ts b/site/src/util/workspace.ts index 94758aebd00d7..42c997f103f69 100644 --- a/site/src/util/workspace.ts +++ b/site/src/util/workspace.ts @@ -263,42 +263,9 @@ export const defaultWorkspaceExtension = (__startDate?: dayjs.Dayjs): TypesGen.P } export const workspaceQueryToFilter = (query?: string): TypesGen.WorkspaceFilter => { - const defaultFilter: TypesGen.WorkspaceFilter = {} const preparedQuery = query?.trim().replace(/ +/g, " ") - - if (!preparedQuery) { - return defaultFilter - } else { - const parts = preparedQuery.split(" ") - - for (const part of parts) { - if (part.includes(":")) { - const [key, val] = part.split(":") - if (key && val) { - if (key === "owner") { - return { - owner: val, - } - } - // skip invalid key pairs - continue - } - } - - if (part.includes("/")) { - const [username, name] = part.split("/") - return { - owner: username, - name: name === "" ? undefined : name, - } - } - - return { - name: part, - } - } - - return defaultFilter + return { + q: preparedQuery, } } From 92dcbc81a42703173e2382f2efe3d476147f211f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 09:48:01 -0500 Subject: [PATCH 32/35] fmt --- site/src/api/api.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/site/src/api/api.test.ts b/site/src/api/api.test.ts index 3c608f3559004..6e1566b114465 100644 --- a/site/src/api/api.test.ts +++ b/site/src/api/api.test.ts @@ -118,10 +118,10 @@ describe("api.ts", () => { it.each<[TypesGen.WorkspaceFilter | undefined, string]>([ [undefined, "/api/v2/workspaces"], - [{ q:"" }, "/api/v2/workspaces"], - [{ q:"owner:1" }, "/api/v2/workspaces?q=owner:1"], + [{ q: "" }, "/api/v2/workspaces"], + [{ q: "owner:1" }, "/api/v2/workspaces?q=owner:1"], - [{ q:"owner:me" }, "/api/v2/workspaces?q=owner:me"], + [{ q: "owner:me" }, "/api/v2/workspaces?q=owner:me"], ])(`getWorkspacesURL(%p) returns %p`, (filter, expected) => { expect(getWorkspacesURL(filter)).toBe(expected) }) From 0a51e0277cdff6017b9b1709b1032958953d8926 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 10:01:35 -0500 Subject: [PATCH 33/35] Use !== over != --- site/src/api/api.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 5df7f85bee217..e46dc2708748a 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -120,7 +120,7 @@ export const getWorkspacesURL = (filter?: TypesGen.WorkspaceFilter): string => { const basePath = "/api/v2/workspaces" const searchParams = new URLSearchParams() - if (filter?.q && filter.q != "") { + if (filter?.q && filter.q !== "") { searchParams.append("q", filter.q) } From 5a1272c175453fe5ad6ab499176e76d403b8952a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 10:11:46 -0500 Subject: [PATCH 34/35] Fix js unit test --- site/src/api/api.test.ts | 4 ++-- site/src/util/workspace.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/site/src/api/api.test.ts b/site/src/api/api.test.ts index 6e1566b114465..f7455a9910b32 100644 --- a/site/src/api/api.test.ts +++ b/site/src/api/api.test.ts @@ -119,9 +119,9 @@ describe("api.ts", () => { [undefined, "/api/v2/workspaces"], [{ q: "" }, "/api/v2/workspaces"], - [{ q: "owner:1" }, "/api/v2/workspaces?q=owner:1"], + [{ q: "owner:1" }, "/api/v2/workspaces?q=owner%3A1"], - [{ q: "owner:me" }, "/api/v2/workspaces?q=owner:me"], + [{ q: "owner:me" }, "/api/v2/workspaces?q=owner%3Ame"], ])(`getWorkspacesURL(%p) returns %p`, (filter, expected) => { expect(getWorkspacesURL(filter)).toBe(expected) }) diff --git a/site/src/util/workspace.test.ts b/site/src/util/workspace.test.ts index 175789ad6bec2..3df3ba65de186 100644 --- a/site/src/util/workspace.test.ts +++ b/site/src/util/workspace.test.ts @@ -104,7 +104,7 @@ describe("util > workspace", () => { describe("workspaceQueryToFilter", () => { it.each<[string | undefined, TypesGen.WorkspaceFilter]>([ [undefined, {}], - ["", {}], + ["", { q:"" }], ["asdkfvjn", { q: "asdkfvjn" }], ["owner:me", { q: "owner:me" }], ["owner:me owner:me2", { q: "owner:me owner:me2" }], From 5b5039b7331288058224f50051d5f66baf198930 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 13 Jun 2022 10:17:08 -0500 Subject: [PATCH 35/35] Js linting --- site/src/util/workspace.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/util/workspace.test.ts b/site/src/util/workspace.test.ts index 3df3ba65de186..cdfd833fe17a8 100644 --- a/site/src/util/workspace.test.ts +++ b/site/src/util/workspace.test.ts @@ -104,7 +104,7 @@ describe("util > workspace", () => { describe("workspaceQueryToFilter", () => { it.each<[string | undefined, TypesGen.WorkspaceFilter]>([ [undefined, {}], - ["", { q:"" }], + ["", { q: "" }], ["asdkfvjn", { q: "asdkfvjn" }], ["owner:me", { q: "owner:me" }], ["owner:me owner:me2", { q: "owner:me owner:me2" }],