From 77e37cfde62ac5bea027482063a62eb634228028 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 20 Mar 2024 11:15:31 -0500 Subject: [PATCH 1/3] chore: support multiple key:value search query params GitHub does this in their label search. Including this for an upcoming feature where the csv format is a bit unwieldy. --- coderd/httpapi/queryparams.go | 32 ++++++++++++++++++++----- coderd/httpapi/queryparams_test.go | 38 +++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index a6683620813ed..d20c9098957cd 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -248,13 +248,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 +287,18 @@ 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 { + return def, xerrors.Errorf("multiple values provided for the query param %q, only 1 is supported", queryParam) + } + 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 +314,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..8bd16e5756fff 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 @@ -354,6 +359,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 +403,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 { From eae808b7eaf6c3bca13f2f5e8fb83a8e93e132ff Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 20 Mar 2024 11:22:24 -0500 Subject: [PATCH 2/3] expand unit tests for list cases --- coderd/httpapi/queryparams.go | 16 ++++++++++----- coderd/httpapi/queryparams_test.go | 31 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index d20c9098957cd..58ec95eb762b2 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 } diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index 8bd16e5756fff..780fefaa964b4 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -32,6 +32,7 @@ type queryParamTestCase[T any] struct { func TestParseQueryParams(t *testing.T) { t.Parallel() + const multipleValuesError = "multiple values provided" t.Run("Enum", func(t *testing.T) { t.Parallel() @@ -64,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() @@ -156,6 +162,11 @@ func TestParseQueryParams(t *testing.T) { Default: "default", Expected: "default", }, + { + QueryParam: "unexpected_list", + Values: []string{"one", "two"}, + ExpectedErrorContains: multipleValuesError, + }, } parser := httpapi.NewQueryParamParser() @@ -198,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() @@ -235,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() @@ -279,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() @@ -316,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() From 3fe88c4331e27cc001ce458476133dfecccd55fa Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 20 Mar 2024 11:44:36 -0500 Subject: [PATCH 3/3] move requiring a single value further in the process where more context exists' --- coderd/httpapi/queryparams.go | 11 ++++++++++- coderd/httpapi/queryparams_test.go | 2 +- coderd/searchquery/search.go | 11 ----------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index 58ec95eb762b2..6d6f15925729f 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -297,7 +297,16 @@ func ParseCustomList[T any](parser *QueryParamParser, vals url.Values, def []T, 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 { - return def, xerrors.Errorf("multiple values provided for the query param %q, only 1 is supported", queryParam) + // 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]) } diff --git a/coderd/httpapi/queryparams_test.go b/coderd/httpapi/queryparams_test.go index 780fefaa964b4..3c4f938cce90c 100644 --- a/coderd/httpapi/queryparams_test.go +++ b/coderd/httpapi/queryparams_test.go @@ -32,7 +32,7 @@ type queryParamTestCase[T any] struct { func TestParseQueryParams(t *testing.T) { t.Parallel() - const multipleValuesError = "multiple values provided" + const multipleValuesError = "provided more than once" t.Run("Enum", func(t *testing.T) { t.Parallel() 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 }