From cdeb293f6d44ad57d41ba922fb01f21c6c8c2a19 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 2 Jul 2024 14:24:03 -0500 Subject: [PATCH 1/4] chore: add templates search query to a filter --- coderd/httpapi/queryparams.go | 50 ++++++++++++++++++++++++- coderd/httpapi/queryparams_test.go | 60 ++++++++++++++++++++++++++++++ coderd/searchquery/search.go | 47 +++++++++++++++++++++++ coderd/searchquery/search_test.go | 43 +++++++++++++++++++++ 4 files changed, 198 insertions(+), 2 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 77b58c8ae0589..af20d2beda1ba 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -1,6 +1,7 @@ package httpapi import ( + "database/sql" "errors" "fmt" "net/url" @@ -104,6 +105,27 @@ func (p *QueryParamParser) PositiveInt32(vals url.Values, def int32, queryParam return v } +// NullableBoolean will return a null sql value if no input is provided. +// SQLc still uses sql.NullBool rather than the generic type. So converting from +// the generic type is required. +func (p *QueryParamParser) NullableBoolean(vals url.Values, def sql.NullBool, queryParam string) sql.NullBool { + v, err := parseNullableQueryParam[bool](p, vals, strconv.ParseBool, sql.Null[bool]{ + V: def.Bool, + Valid: def.Valid, + }, queryParam) + if err != nil { + p.Errors = append(p.Errors, codersdk.ValidationError{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q must be a valid boolean: %s", queryParam, err.Error()), + }) + } + + return sql.NullBool{ + Bool: v.V, + Valid: v.Valid, + } +} + func (p *QueryParamParser) Boolean(vals url.Values, def bool, queryParam string) bool { v, err := parseQueryParam(p, vals, strconv.ParseBool, def, queryParam) if err != nil { @@ -294,9 +316,34 @@ func ParseCustomList[T any](parser *QueryParamParser, vals url.Values, def []T, return v } +func parseNullableQueryParam[T any](parser *QueryParamParser, vals url.Values, parse func(v string) (T, error), def sql.Null[T], queryParam string) (sql.Null[T], error) { + setParse := parseSingle(parser, parse, def.V, queryParam) + return parseQueryParamSet[sql.Null[T]](parser, vals, func(set []string) (sql.Null[T], error) { + if len(set) == 0 { + return sql.Null[T]{ + Valid: false, + }, nil + } + + value, err := setParse(set) + if err != nil { + return sql.Null[T]{}, err + } + return sql.Null[T]{ + V: value, + Valid: true, + }, nil + }, def, queryParam) +} + // parseQueryParam expects just 1 value set for the given query param. func parseQueryParam[T any](parser *QueryParamParser, vals url.Values, parse func(v string) (T, error), def T, queryParam string) (T, error) { - setParse := func(set []string) (T, error) { + setParse := parseSingle(parser, parse, def, queryParam) + return parseQueryParamSet(parser, vals, setParse, def, queryParam) +} + +func parseSingle[T any](parser *QueryParamParser, parse func(v string) (T, error), def T, queryParam string) func(set []string) (T, error) { + return func(set []string) (T, error) { if len(set) > 1 { // Set as a parser.Error rather than return an error. // Returned errors are errors from the passed in `parse` function, and @@ -311,7 +358,6 @@ func parseQueryParam[T any](parser *QueryParamParser, vals url.Values, parse fun } return parse(set[0]) } - return parseQueryParamSet(parser, vals, setParse, def, queryParam) } func parseQueryParamSet[T any](parser *QueryParamParser, vals url.Values, parse func(set []string) (T, error), def T, queryParam string) (T, error) { diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index 8e92b2b2676c5..16cf805534b05 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -1,6 +1,7 @@ package httpapi_test import ( + "database/sql" "fmt" "net/http" "net/url" @@ -220,6 +221,65 @@ func TestParseQueryParams(t *testing.T) { testQueryParams(t, expParams, parser, parser.Boolean) }) + t.Run("NullableBoolean", func(t *testing.T) { + t.Parallel() + expParams := []queryParamTestCase[sql.NullBool]{ + { + QueryParam: "valid_true", + Value: "true", + Expected: sql.NullBool{ + Bool: true, + Valid: true, + }, + }, + { + QueryParam: "no_value_true_def", + NoSet: true, + Default: sql.NullBool{ + Bool: true, + Valid: true, + }, + Expected: sql.NullBool{ + Bool: true, + Valid: true, + }, + }, + { + QueryParam: "no_value", + NoSet: true, + Expected: sql.NullBool{ + Bool: false, + Valid: false, + }, + }, + + { + QueryParam: "invalid_boolean", + Value: "yes", + Expected: sql.NullBool{ + Bool: false, + Valid: false, + }, + ExpectedErrorContains: "must be a valid boolean", + }, + { + QueryParam: "unexpected_list", + Values: []string{"true", "false"}, + ExpectedErrorContains: multipleValuesError, + // Expected value is a bit strange, but the error is raised + // in the parser, not as a parse failure. Maybe this should be + // fixed, but is how it is done atm. + Expected: sql.NullBool{ + Bool: false, + Valid: true, + }, + }, + } + + parser := httpapi.NewQueryParamParser() + testQueryParams(t, expParams, parser, parser.NullableBoolean) + }) + t.Run("Int", func(t *testing.T) { t.Parallel() expParams := []queryParamTestCase[int]{ diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index 98bdded5e98d2..fa3e23ab109a8 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -184,6 +184,53 @@ func Workspaces(query string, page codersdk.Pagination, agentInactiveDisconnectT return filter, parser.Errors } +func Templates(ctx context.Context, db database.Store, query string) (database.GetTemplatesWithFilterParams, []codersdk.ValidationError) { + // Always lowercase for all searches. + query = strings.ToLower(query) + values, errors := searchTerms(query, func(term string, values url.Values) error { + // Default to the template name + values.Add("name", term) + return nil + }) + if len(errors) > 0 { + return database.GetTemplatesWithFilterParams{}, errors + } + + const dateLayout = "2006-01-02" + parser := httpapi.NewQueryParamParser() + filter := database.GetTemplatesWithFilterParams{ + Deleted: parser.Boolean(values, false, "deleted"), + // TODO: Should name be a fuzzy search? + ExactName: parser.String(values, "", "name"), + IDs: parser.UUIDs(values, []uuid.UUID{}, "ids"), + Deprecated: parser.NullableBoolean(values, sql.NullBool{}, "deprecated"), + } + + // Convert the "organization" parameter to an organization uuid. This can require + // a database lookup. + organizationArg := parser.String(values, "", "organization") + if organizationArg != "" { + organizationID, err := uuid.Parse(organizationArg) + if err == nil { + filter.OrganizationID = organizationID + } else { + // Organization could be a name + organization, err := db.GetOrganizationByName(ctx, organizationArg) + if err != nil { + parser.Errors = append(parser.Errors, codersdk.ValidationError{ + Field: "organization", + Detail: fmt.Sprintf("Organization %q either does not exist, or you are unauthorized to view it", organizationArg), + }) + } else { + filter.OrganizationID = organization.ID + } + } + } + + parser.ErrorExcessParams(values) + return filter, parser.Errors +} + func searchTerms(query string, defaultKey func(term string, values url.Values) error) (url.Values, []codersdk.ValidationError) { searchValues := make(url.Values) diff --git a/coderd/searchquery/search_test.go b/coderd/searchquery/search_test.go index cbbeed0ee998e..536f0ead85170 100644 --- a/coderd/searchquery/search_test.go +++ b/coderd/searchquery/search_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -454,3 +455,45 @@ func TestSearchUsers(t *testing.T) { }) } } + +func TestSearchTemplates(t *testing.T) { + t.Parallel() + testCases := []struct { + Name string + Query string + Expected database.GetTemplatesWithFilterParams + ExpectedErrorContains string + }{ + { + Name: "Empty", + Query: "", + Expected: database.GetTemplatesWithFilterParams{}, + }, + } + + for _, c := range testCases { + c := c + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + // Do not use a real database, this is only used for an + // organization lookup. + db := dbmem.New() + values, errs := searchquery.Templates(context.Background(), db, c.Query) + if 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.Len(t, errs, 0, "expected no error") + if c.Expected.IDs == nil { + // Nil and length 0 are the same + c.Expected.IDs = []uuid.UUID{} + } + require.Equal(t, c.Expected, values, "expected values") + } + }) + } +} From ba9ee36a219391b991626c50a8185651dec5b803 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 Jul 2024 08:54:08 -0500 Subject: [PATCH 2/4] add search to /templates --- coderd/templates.go | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/coderd/templates.go b/coderd/templates.go index ffb45fd2e08e4..00401c209b0a2 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -21,6 +21,7 @@ import ( "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/schedule" + "github.com/coder/coder/v2/coderd/searchquery" "github.com/coder/coder/v2/coderd/telemetry" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/workspacestats" @@ -457,20 +458,12 @@ func (api *API) fetchTemplates(mutate func(r *http.Request, arg *database.GetTem return func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - p := httpapi.NewQueryParamParser() - values := r.URL.Query() - - deprecated := sql.NullBool{} - if values.Has("deprecated") { - deprecated = sql.NullBool{ - Bool: p.Boolean(values, false, "deprecated"), - Valid: true, - } - } - if len(p.Errors) > 0 { + queryStr := r.URL.Query().Get("q") + filter, errs := searchquery.Templates(ctx, api.Database, queryStr) + if len(errs) > 0 { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Invalid query params.", - Validations: p.Errors, + Message: "Invalid template search query.", + Validations: errs, }) return } @@ -484,9 +477,7 @@ func (api *API) fetchTemplates(mutate func(r *http.Request, arg *database.GetTem return } - args := database.GetTemplatesWithFilterParams{ - Deprecated: deprecated, - } + args := filter if mutate != nil { mutate(r, &args) } From ecb59d659313c6bca869dc8a11ddccc9ab002964 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 Jul 2024 08:58:26 -0500 Subject: [PATCH 3/4] update cli --- coderd/templates_test.go | 17 +++++++++++++---- codersdk/organizations.go | 25 ++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 9b4c813a263b0..612591120ec1a 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -420,7 +420,9 @@ func TestTemplatesByOrganization(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) - templates, err := client.TemplatesByOrganization(ctx, user.OrganizationID) + templates, err := client.Templates(ctx, codersdk.TemplateFilter{ + OrganizationID: user.OrganizationID, + }) require.NoError(t, err) require.Len(t, templates, 1) }) @@ -440,7 +442,7 @@ func TestTemplatesByOrganization(t *testing.T) { require.Len(t, templates, 2) // Listing all should match - templates, err = client.Templates(ctx) + templates, err = client.Templates(ctx, codersdk.TemplateFilter{}) require.NoError(t, err) require.Len(t, templates, 2) @@ -473,12 +475,19 @@ func TestTemplatesByOrganization(t *testing.T) { ctx := testutil.Context(t, testutil.WaitLong) // All 4 are viewable by the owner - templates, err := client.Templates(ctx) + templates, err := client.Templates(ctx, codersdk.TemplateFilter{}) require.NoError(t, err) require.Len(t, templates, 4) + // View a single organization from the owner + templates, err = client.Templates(ctx, codersdk.TemplateFilter{ + OrganizationID: owner.OrganizationID, + }) + require.NoError(t, err) + require.Len(t, templates, 2) + // Only 2 are viewable by the org user - templates, err = user.Templates(ctx) + templates, err = user.Templates(ctx, codersdk.TemplateFilter{}) require.NoError(t, err) require.Len(t, templates, 2) for _, tmpl := range templates { diff --git a/codersdk/organizations.go b/codersdk/organizations.go index e494018258e48..ecc87ccb983ce 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "strings" "time" "github.com/google/uuid" @@ -362,11 +363,33 @@ func (c *Client) TemplatesByOrganization(ctx context.Context, organizationID uui return templates, json.NewDecoder(res.Body).Decode(&templates) } +type TemplateFilter struct { + OrganizationID uuid.UUID +} + +// asRequestOption returns a function that can be used in (*Client).Request. +// It modifies the request query parameters. +func (f TemplateFilter) asRequestOption() RequestOption { + return func(r *http.Request) { + var params []string + // Make sure all user input is quoted to ensure it's parsed as a single + // string. + if f.OrganizationID != uuid.Nil { + params = append(params, fmt.Sprintf("organization:%q", f.OrganizationID.String())) + } + + q := r.URL.Query() + q.Set("q", strings.Join(params, " ")) + r.URL.RawQuery = q.Encode() + } +} + // Templates lists all viewable templates -func (c *Client) Templates(ctx context.Context) ([]Template, error) { +func (c *Client) Templates(ctx context.Context, filter TemplateFilter) ([]Template, error) { res, err := c.Request(ctx, http.MethodGet, "/api/v2/templates", nil, + filter.asRequestOption(), ) if err != nil { return nil, xerrors.Errorf("execute request: %w", err) From 42279298c5ba03f726260b96750c78d8a65031c4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 3 Jul 2024 10:23:52 -0500 Subject: [PATCH 4/4] linting --- coderd/searchquery/search.go | 1 - site/src/api/typesGenerated.ts | 5 +++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index fa3e23ab109a8..0744ec8482926 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -196,7 +196,6 @@ func Templates(ctx context.Context, db database.Store, query string) (database.G return database.GetTemplatesWithFilterParams{}, errors } - const dateLayout = "2006-01-02" parser := httpapi.NewQueryParamParser() filter := database.GetTemplatesWithFilterParams{ Deleted: parser.Boolean(values, false, "deleted"), diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index e878b25e1f452..c10b8d17fac62 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1165,6 +1165,11 @@ export interface TemplateExample { readonly markdown: string; } +// From codersdk/organizations.go +export interface TemplateFilter { + readonly OrganizationID: string; +} + // From codersdk/templates.go export interface TemplateGroup extends Group { readonly role: TemplateRole;