diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index a6683620813ed..6d6f15925729f 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -61,7 +61,7 @@ func (p *QueryParamParser) UInt(vals url.Values, def uint64, queryParam string) if err != nil { p.Errors = append(p.Errors, codersdk.ValidationError{ Field: queryParam, - Detail: fmt.Sprintf("Query param %q must be a valid positive integer (%s)", queryParam, err.Error()), + Detail: fmt.Sprintf("Query param %q must be a valid positive integer: %s", queryParam, err.Error()), }) return 0 } @@ -73,7 +73,7 @@ func (p *QueryParamParser) Int(vals url.Values, def int, queryParam string) int if err != nil { p.Errors = append(p.Errors, codersdk.ValidationError{ Field: queryParam, - Detail: fmt.Sprintf("Query param %q must be a valid integer (%s)", queryParam, err.Error()), + Detail: fmt.Sprintf("Query param %q must be a valid integer: %s", queryParam, err.Error()), }) } return v @@ -97,7 +97,7 @@ func (p *QueryParamParser) PositiveInt32(vals url.Values, def int32, queryParam if err != nil { p.Errors = append(p.Errors, codersdk.ValidationError{ Field: queryParam, - Detail: fmt.Sprintf("Query param %q must be a valid 32-bit positive integer (%s)", queryParam, err.Error()), + Detail: fmt.Sprintf("Query param %q must be a valid 32-bit positive integer: %s", queryParam, err.Error()), }) } return v @@ -108,7 +108,7 @@ func (p *QueryParamParser) Boolean(vals url.Values, def bool, queryParam string) 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()), + Detail: fmt.Sprintf("Query param %q must be a valid boolean: %s", queryParam, err.Error()), }) } return v @@ -203,9 +203,15 @@ func (p *QueryParamParser) timeWithMutate(vals url.Values, def time.Time, queryP } func (p *QueryParamParser) String(vals url.Values, def string, queryParam string) string { - v, _ := parseQueryParam(p, vals, func(v string) (string, error) { + v, err := parseQueryParam(p, vals, func(v string) (string, error) { return v, nil }, def, queryParam) + if err != nil { + p.Errors = append(p.Errors, codersdk.ValidationError{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q must be a valid string: %s", queryParam, err.Error()), + }) + } return v } @@ -248,13 +254,23 @@ func ParseCustom[T any](parser *QueryParamParser, vals url.Values, def T, queryP return v } -// ParseCustomList is a function that handles csv query params. +// ParseCustomList is a function that handles csv query params or multiple values +// for a query param. +// Csv is supported as it is a common way to pass multiple values in a query param. +// Multiple values is supported (key=value&key=value2) for feature parity with GitHub issue search. func ParseCustomList[T any](parser *QueryParamParser, vals url.Values, def []T, queryParam string, parseFunc func(v string) (T, error)) []T { - v, err := parseQueryParam(parser, vals, func(v string) ([]T, error) { - terms := strings.Split(v, ",") + v, err := parseQueryParamSet(parser, vals, func(set []string) ([]T, error) { + // Gather all terms. + allTerms := make([]string, 0, len(set)) + for _, s := range set { + // If a term is a csv, break it out into individual terms. + terms := strings.Split(s, ",") + allTerms = append(allTerms, terms...) + } + var badValues []string var output []T - for _, s := range terms { + for _, s := range allTerms { good, err := parseFunc(s) if err != nil { badValues = append(badValues, s) @@ -277,7 +293,27 @@ func ParseCustomList[T any](parser *QueryParamParser, vals url.Values, def []T, return v } +// 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) { + 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 + // imply the query param value had attempted to be parsed. + // By raising the error this way, we can also more easily control how it + // is presented to the user. A returned error is wrapped with more text. + parser.Errors = append(parser.Errors, codersdk.ValidationError{ + Field: queryParam, + Detail: fmt.Sprintf("Query param %q provided more than once, found %d times. Only provide 1 instance of this query param.", queryParam, len(set)), + }) + return def, nil + } + 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) { parser.addParsed(queryParam) // If the query param is required and not present, return an error. if parser.RequiredNotEmptyParams[queryParam] && (!vals.Has(queryParam) || vals.Get(queryParam) == "") { @@ -293,6 +329,5 @@ func parseQueryParam[T any](parser *QueryParamParser, vals url.Values, parse fun return def, nil } - str := vals.Get(queryParam) - return parse(str) + return parse(vals[queryParam]) } diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index 3f144fa19f861..3c4f938cce90c 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -17,8 +17,13 @@ import ( 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 + NoSet bool + // Value vs values is the difference between a single query param and multiple + // to the same key. + // -> key=value + Value string + // -> key=value1 key=value2 + Values []string Default T Expected T ExpectedErrorContains string @@ -27,6 +32,7 @@ type queryParamTestCase[T any] struct { func TestParseQueryParams(t *testing.T) { t.Parallel() + const multipleValuesError = "provided more than once" t.Run("Enum", func(t *testing.T) { t.Parallel() @@ -59,6 +65,11 @@ func TestParseQueryParams(t *testing.T) { Value: fmt.Sprintf("%s,%s", database.ResourceTypeWorkspace, database.ResourceTypeApiKey), Expected: []database.ResourceType{database.ResourceTypeWorkspace, database.ResourceTypeApiKey}, }, + { + QueryParam: "resource_type_as_list", + Values: []string{string(database.ResourceTypeWorkspace), string(database.ResourceTypeApiKey)}, + Expected: []database.ResourceType{database.ResourceTypeWorkspace, database.ResourceTypeApiKey}, + }, } parser := httpapi.NewQueryParamParser() @@ -151,6 +162,11 @@ func TestParseQueryParams(t *testing.T) { Default: "default", Expected: "default", }, + { + QueryParam: "unexpected_list", + Values: []string{"one", "two"}, + ExpectedErrorContains: multipleValuesError, + }, } parser := httpapi.NewQueryParamParser() @@ -193,6 +209,11 @@ func TestParseQueryParams(t *testing.T) { Expected: false, ExpectedErrorContains: "must be a valid boolean", }, + { + QueryParam: "unexpected_list", + Values: []string{"true", "false"}, + ExpectedErrorContains: multipleValuesError, + }, } parser := httpapi.NewQueryParamParser() @@ -230,6 +251,11 @@ func TestParseQueryParams(t *testing.T) { Expected: 0, ExpectedErrorContains: "must be a valid integer", }, + { + QueryParam: "unexpected_list", + Values: []string{"5", "10"}, + ExpectedErrorContains: multipleValuesError, + }, } parser := httpapi.NewQueryParamParser() @@ -274,6 +300,11 @@ func TestParseQueryParams(t *testing.T) { Expected: 0, ExpectedErrorContains: "must be a valid 32-bit positive integer", }, + { + QueryParam: "unexpected_list", + Values: []string{"5", "10"}, + ExpectedErrorContains: multipleValuesError, + }, } parser := httpapi.NewQueryParamParser() @@ -311,6 +342,11 @@ func TestParseQueryParams(t *testing.T) { Expected: 0, ExpectedErrorContains: "must be a valid positive integer", }, + { + QueryParam: "unexpected_list", + Values: []string{"5", "10"}, + ExpectedErrorContains: multipleValuesError, + }, } parser := httpapi.NewQueryParamParser() @@ -354,6 +390,23 @@ func TestParseQueryParams(t *testing.T) { Default: []uuid.UUID{}, ExpectedErrorContains: "bogus", }, + { + QueryParam: "multiple_keys", + Values: []string{"6c8ef17d-5dd8-4b92-bac9-41944f90f237", "65fb05f3-12c8-4a0a-801f-40439cf9e681"}, + Expected: []uuid.UUID{ + uuid.MustParse("6c8ef17d-5dd8-4b92-bac9-41944f90f237"), + uuid.MustParse("65fb05f3-12c8-4a0a-801f-40439cf9e681"), + }, + }, + { + QueryParam: "multiple_and_csv", + Values: []string{"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"), + }, + }, } parser := httpapi.NewQueryParamParser() @@ -381,7 +434,17 @@ func testQueryParams[T any](t *testing.T, testCases []queryParamTestCase[T], par if c.NoSet { continue } - v.Set(c.QueryParam, c.Value) + if len(c.Values) > 0 && c.Value != "" { + t.Errorf("test case %q has both value and values, choose one, not both!", c.QueryParam) + t.FailNow() + } + if c.Value != "" { + c.Values = append(c.Values, c.Value) + } + + for _, value := range c.Values { + v.Add(c.QueryParam, value) + } } for _, c := range testCases { diff --git a/coderd/searchquery/search.go b/coderd/searchquery/search.go index e1507c540fe85..7173c53387e70 100644 --- a/coderd/searchquery/search.go +++ b/coderd/searchquery/search.go @@ -163,17 +163,6 @@ func searchTerms(query string, defaultKey func(term string, values url.Values) e } } - for k := range searchValues { - if len(searchValues[k]) > 1 { - return nil, []codersdk.ValidationError{ - { - Field: "q", - Detail: fmt.Sprintf("Query parameter %q provided more than once, found %d times", k, len(searchValues[k])), - }, - } - } - } - return searchValues, nil }