coder · Emyrk · Mar 21, 2024 · Mar 20, 2024 · Mar 20, 2024 · Mar 20, 2024
diff --git 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
@@ -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
@@ -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
 }