From fffec091c36ff6d8895537c3a404891e7f0e0502 Mon Sep 17 00:00:00 2001
From: Luke Bjerring <lukebjerring@users.noreply.github.com>
Date: Tue, 24 Apr 2018 09:50:05 -0700
Subject: [PATCH] Refactor code for repeated param, add tests for labels

---
 webapp/params.go      | 61 +++++++++++++++++--------------------------
 webapp/params_test.go | 34 ++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 37 deletions(-)

diff --git a/webapp/params.go b/webapp/params.go
index 6c0f087fb2d..97b576a469e 100644
--- a/webapp/params.go
+++ b/webapp/params.go
@@ -164,56 +164,43 @@ func ParseDiffFilterParams(r *http.Request) (param DiffFilterParam, err error) {
 	return param, nil
 }
 
-// ParsePathsParam returns a set list of test paths to include, or nil if no filter is provided (and all tests should be
-// included). It parses the 'paths' parameter, split on commas, and also checks for the (repeatable) 'path' params.
+// ParsePathsParam returns a set list of test paths to include, or nil if no
+// filter is provided (and all tests should be included). It parses the 'paths'
+// parameter, split on commas, and also checks for the (repeatable) 'path' params
 func ParsePathsParam(r *http.Request) (paths mapset.Set) {
-	pathParams := r.URL.Query()["path"]
-	pathsParam := r.URL.Query().Get("paths")
-	if len(pathParams) == 0 && pathsParam == "" {
-		return nil
-	}
-
-	paths = mapset.NewSet()
-	for _, path := range pathParams {
-		paths.Add(path)
-	}
-	if pathsParam != "" {
-		for _, path := range strings.Split(pathsParam, ",") {
-			paths.Add(path)
-		}
-	}
-	if paths.Contains("") {
-		paths.Remove("")
-	}
-	if paths.Cardinality() == 0 {
-		return nil
-	}
-	return paths
+	return ParseRepeatedParam(r, "path", "paths")
 }
 
 // ParseLabelsParam returns a set list of test-run labels to include, or nil if
 // no labels are provided.
 func ParseLabelsParam(r *http.Request) (labels mapset.Set) {
-	labelParams := r.URL.Query()["label"]
-	labelsParam := r.URL.Query().Get("labels")
-	if len(labelParams) == 0 && labelsParam == "" {
+	return ParseRepeatedParam(r, "label", "labels")
+}
+
+// ParseRepeatedParam parses a param that may be a plural name, with all values
+// comma-separated, or a repeated singular param.
+// e.g. ?label=foo&label=bar vs ?labels=foo,bar
+func ParseRepeatedParam(r *http.Request, singular string, plural string) (params mapset.Set) {
+	repeatedParam := r.URL.Query()[singular]
+	pluralParam := r.URL.Query().Get(plural)
+	if len(repeatedParam) == 0 && pluralParam == "" {
 		return nil
 	}
 
-	labels = mapset.NewSet()
-	for _, label := range labelParams {
-		labels.Add(label)
+	params = mapset.NewSet()
+	for _, label := range repeatedParam {
+		params.Add(label)
 	}
-	if labelsParam != "" {
-		for _, label := range strings.Split(labelsParam, ",") {
-			labels.Add(label)
+	if pluralParam != "" {
+		for _, label := range strings.Split(pluralParam, ",") {
+			params.Add(label)
 		}
 	}
-	if labels.Contains("") {
-		labels.Remove("")
+	if params.Contains("") {
+		params.Remove("")
 	}
-	if labels.Cardinality() == 0 {
+	if params.Cardinality() == 0 {
 		return nil
 	}
-	return labels
+	return params
 }
diff --git a/webapp/params_test.go b/webapp/params_test.go
index 180e88e1968..789f1365b2d 100644
--- a/webapp/params_test.go
+++ b/webapp/params_test.go
@@ -261,3 +261,37 @@ func TestParseDiffFilterParam_Invalid(t *testing.T) {
 	_, err := ParseDiffFilterParams(r)
 	assert.NotNil(t, err)
 }
+
+func TestParseLabelsParam_Missing(t *testing.T) {
+	r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs", nil)
+	labels := ParseLabelsParam(r)
+	assert.Nil(t, labels)
+}
+
+func TestParseLabelsParam_Empty(t *testing.T) {
+	r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs?label=", nil)
+	labels := ParseLabelsParam(r)
+	assert.Nil(t, labels)
+
+	r = httptest.NewRequest("GET", "http://wpt.fyi/api/runs?labels=", nil)
+	labels = ParseLabelsParam(r)
+	assert.Nil(t, labels)
+}
+
+func TestParseLabelsParam_Label_Duplicate(t *testing.T) {
+	r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs?label=unstable&label=unstable", nil)
+	labels := ParseLabelsParam(r)
+	assert.Equal(t, 1, labels.Cardinality())
+}
+
+func TestParseLabelsParam_Labels_Duplicate(t *testing.T) {
+	r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs?labels=unstable,unstable", nil)
+	labels := ParseLabelsParam(r)
+	assert.Equal(t, 1, labels.Cardinality())
+}
+
+func TestParseLabelsParam_LabelsAndLabel_Duplicate(t *testing.T) {
+	r := httptest.NewRequest("GET", "http://wpt.fyi/api/runs?labels=unstable&label=unstable", nil)
+	labels := ParseLabelsParam(r)
+	assert.Equal(t, 1, labels.Cardinality())
+}