Query url parameters should not escape + or : (#22)

* pre-escape + and : in url query parameters (closes #18)

* add test to ensure + does not cause a failed query

* update changelog

Author: opalmer

CHANGELOG.md

@@ -13,6 +13,10 @@ first. For more complete details see
   instead of a hard coded value when comparing response codes.
 * Updated AccountInfo.AccountID to be omitted of empty (such as when 
   used in ApprovalInfo).
+* + and : in url parameters for queries are no longer escaped. This was
+  causing `400 Bad Request` to be returned when the + symbol was
+  included as part of the query. To match behavior with Gerrit's search
+  handling, the : symbol was also excluded.
 * Fixed documentation for NewClient and moved fmt.Errorf call from
   inside the function to a `ErrNoInstanceGiven` variable so it's
   easier to compare against.

changes_test.go

@@ -31,3 +31,30 @@ func ExampleChangesService_QueryChanges() {
 	// Output:
 	// Project: platform/art -> ART: Change return types of field access entrypoints -> https://android-review.googlesource.com/249244
 }
+
+// Prior to fixing #18 this test would fail.
+func ExampleChangesService_QueryChangesWithSymbols() {
+	instance := "https://android-review.googlesource.com/"
+	client, err := gerrit.NewClient(instance, nil)
+	if err != nil {
+		panic(err)
+	}
+
+	opt := &gerrit.QueryChangeOptions{}
+	opt.Query = []string{
+		"change:249244+status:merged",
+	}
+	opt.Limit = 2
+	opt.AdditionalFields = []string{"LABELS"}
+	changes, _, err := client.Changes.QueryChanges(opt)
+	if err != nil {
+		panic(err)
+	}
+
+	for _, change := range *changes {
+		fmt.Printf("Project: %s -> %s -> %s%d\n", change.Project, change.Subject, instance, change.Number)
+	}
+
+	// Output:
+	// Project: platform/art -> ART: Change return types of field access entrypoints -> https://android-review.googlesource.com/249244
+}

gerrit.go

@@ -433,6 +433,14 @@ func CheckResponse(r *http.Response) error {
 	return err
 }
 
+// queryParameterReplacements are values in a url, specifically the query
+// portion of the url, which should not be escaped before being sent to
+// Gerrit. Note, Gerrit itself does not escape these values when using the
+// search box so we shouldn't escape them either.
+var queryParameterReplacements = map[string]string{
+	"+": "GOGERRIT_URL_PLACEHOLDER_PLUS",
+	":": "GOGERRIT_URL_PLACEHOLDER_COLON"}
+
 // addOptions adds the parameters in opt as URL query parameters to s.
 // opt must be a struct whose fields may contain "url" tags.
 func addOptions(s string, opt interface{}) (string, error) {
@@ -451,7 +459,37 @@ func addOptions(s string, opt interface{}) (string, error) {
 		return s, err
 	}
 
-	u.RawQuery = qs.Encode()
+	// If the url contained one or more query parameters (q) then we need
+	// to do some escaping on these values before Encode() is called.  By
+	// doing so we're ensuring that : and + don't get encoded which means
+	// they'll be passed along to Gerrit as raw ascii. Without this Gerrit
+	// could return 400 Bad Request depending on the query parameters. For
+	// more complete information see this issue on GitHub:
+	//   https://github.com/andygrunwald/go-gerrit/issues/18
+	_, hasQuery := qs["q"]
+	if hasQuery {
+		values := []string{}
+		for _, value := range qs["q"] {
+			for key, replacement := range queryParameterReplacements {
+				value = strings.Replace(value, key, replacement, -1)
+			}
+			values = append(values, value)
+		}
+
+		qs.Del("q")
+		for _, value := range values {
+			qs.Add("q", value)
+		}
+	}
+	encoded := qs.Encode()
+
+	if hasQuery {
+		for key, replacement := range queryParameterReplacements {
+			encoded = strings.Replace(encoded, replacement, key, -1)
+		}
+	}
+
+	u.RawQuery = encoded
 	return u.String(), nil
 }