fix: paging with load balancer going to endless loop

evilnerd · ghostsquad · commit 19d3fc0aecde · 2020-03-15T14:02:52.000-07:00
When used with a load balanced Jira, the SearchPages method would end up in an endless loop. This was caused by a bug where Jira would not handle the MaxResults=50 that is sent by defaul properly, thus retur- ning no issues. The SearchPages method didn't check for empty results and ended up in an endless loop. Fixed this by 1. Pre-escaping '&maxResults' to '&amp;MaxResults'. 2. Adding a check in SearchPages to see if the issues array is empty before going into the endless 'for'. Also fixed the appropriate tests. Fixes issue #260.
diff --git a/issue.go b/issue.go
@@ -965,7 +965,7 @@ func (s *IssueService) Search(jql string, options *SearchOptions) ([]Issue, *Res
 			u += fmt.Sprintf("&startAt=%d", options.StartAt)
 		}
 		if options.MaxResults != 0 {
-			u += fmt.Sprintf("&maxResults=%d", options.MaxResults)
+			u += fmt.Sprintf("&amp;maxResults=%d", options.MaxResults)
 		}
 		if options.Expand != "" {
 			u += fmt.Sprintf("&expand=%s", options.Expand)
@@ -1011,6 +1011,10 @@ func (s *IssueService) SearchPages(jql string, options *SearchOptions, f func(Is
 		return err
 	}
 
+	if len(issues) == 0 {
+		return nil
+	}
+
 	for {
 		for _, issue := range issues {
 			err = f(issue)
diff --git a/issue_test.go b/issue_test.go
@@ -3,14 +3,15 @@ package jira
 import (
 	"encoding/json"
 	"fmt"
-	"github.com/google/go-cmp/cmp"
 	"io"
 	"io/ioutil"
 	"net/http"
 	"reflect"
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"time"
 
 	"github.com/trivago/tgo/tcontainer"
@@ -596,7 +597,7 @@ func TestIssueService_Search(t *testing.T) {
 	defer teardown()
 	testMux.HandleFunc("/rest/api/2/search", func(w http.ResponseWriter, r *http.Request) {
 		testMethod(t, r, "GET")
-		testRequestURL(t, r, "/rest/api/2/search?jql=something&startAt=1&maxResults=40&expand=foo")
+		testRequestURL(t, r, "/rest/api/2/search?jql=something&startAt=1&amp;maxResults=40&expand=foo")
 		w.WriteHeader(http.StatusOK)
 		fmt.Fprint(w, `{"expand": "schema,names","startAt": 1,"maxResults": 40,"total": 6,"issues": [{"expand": "html","id": "10230","self": "https://kelpie9:8081/rest/api/2/issue/BULK-62","key": "BULK-62","fields": {"summary": "testing","timetracking": null,"issuetype": {"self": "https://kelpie9:8081/rest/api/2/issuetype/5","id": "5","description": "The sub-task of the issue","iconUrl": "https://kelpie9:8081/images/icons/issue_subtask.gif","name": "Sub-task","subtask": true},"customfield_10071": null}},{"expand": "html","id": "10004","self": "https://kelpie9:8081/rest/api/2/issue/BULK-47","key": "BULK-47","fields": {"summary": "Cheese v1 2.0 issue","timetracking": null,"issuetype": {"self": "https://kelpie9:8081/rest/api/2/issuetype/3","id": "3","description": "A task that needs to be done.","iconUrl": "https://kelpie9:8081/images/icons/task.gif","name": "Task","subtask": false}}}]}`)
 	})
@@ -657,15 +658,15 @@ func TestIssueService_SearchPages(t *testing.T) {
 	defer teardown()
 	testMux.HandleFunc("/rest/api/2/search", func(w http.ResponseWriter, r *http.Request) {
 		testMethod(t, r, "GET")
-		if r.URL.String() == "/rest/api/2/search?jql=something&startAt=1&maxResults=2&expand=foo&validateQuery=warn" {
+		if r.URL.String() == "/rest/api/2/search?jql=something&startAt=1&amp;maxResults=2&expand=foo&validateQuery=warn" {
 			w.WriteHeader(http.StatusOK)
 			fmt.Fprint(w, `{"expand": "schema,names","startAt": 1,"maxResults": 2,"total": 6,"issues": [{"expand": "html","id": "10230","self": "https://kelpie9:8081/rest/api/2/issue/BULK-62","key": "BULK-62","fields": {"summary": "testing","timetracking": null,"issuetype": {"self": "https://kelpie9:8081/rest/api/2/issuetype/5","id": "5","description": "The sub-task of the issue","iconUrl": "https://kelpie9:8081/images/icons/issue_subtask.gif","name": "Sub-task","subtask": true},"customfield_10071": null}},{"expand": "html","id": "10004","self": "https://kelpie9:8081/rest/api/2/issue/BULK-47","key": "BULK-47","fields": {"summary": "Cheese v1 2.0 issue","timetracking": null,"issuetype": {"self": "https://kelpie9:8081/rest/api/2/issuetype/3","id": "3","description": "A task that needs to be done.","iconUrl": "https://kelpie9:8081/images/icons/task.gif","name": "Task","subtask": false}}}]}`)
 			return
-		} else if r.URL.String() == "/rest/api/2/search?jql=something&startAt=3&maxResults=2&expand=foo&validateQuery=warn" {
+		} else if r.URL.String() == "/rest/api/2/search?jql=something&startAt=3&amp;maxResults=2&expand=foo&validateQuery=warn" {
 			w.WriteHeader(http.StatusOK)
 			fmt.Fprint(w, `{"expand": "schema,names","startAt": 3,"maxResults": 2,"total": 6,"issues": [{"expand": "html","id": "10230","self": "https://kelpie9:8081/rest/api/2/issue/BULK-62","key": "BULK-62","fields": {"summary": "testing","timetracking": null,"issuetype": {"self": "https://kelpie9:8081/rest/api/2/issuetype/5","id": "5","description": "The sub-task of the issue","iconUrl": "https://kelpie9:8081/images/icons/issue_subtask.gif","name": "Sub-task","subtask": true},"customfield_10071": null}},{"expand": "html","id": "10004","self": "https://kelpie9:8081/rest/api/2/issue/BULK-47","key": "BULK-47","fields": {"summary": "Cheese v1 2.0 issue","timetracking": null,"issuetype": {"self": "https://kelpie9:8081/rest/api/2/issuetype/3","id": "3","description": "A task that needs to be done.","iconUrl": "https://kelpie9:8081/images/icons/task.gif","name": "Task","subtask": false}}}]}`)
 			return
-		} else if r.URL.String() == "/rest/api/2/search?jql=something&startAt=5&maxResults=2&expand=foo&validateQuery=warn" {
+		} else if r.URL.String() == "/rest/api/2/search?jql=something&startAt=5&amp;maxResults=2&expand=foo&validateQuery=warn" {
 			w.WriteHeader(http.StatusOK)
 			fmt.Fprint(w, `{"expand": "schema,names","startAt": 5,"maxResults": 2,"total": 6,"issues": [{"expand": "html","id": "10230","self": "https://kelpie9:8081/rest/api/2/issue/BULK-62","key": "BULK-62","fields": {"summary": "testing","timetracking": null,"issuetype": {"self": "https://kelpie9:8081/rest/api/2/issuetype/5","id": "5","description": "The sub-task of the issue","iconUrl": "https://kelpie9:8081/images/icons/issue_subtask.gif","name": "Sub-task","subtask": true},"customfield_10071": null}}]}`)
 			return
@@ -690,6 +691,34 @@ func TestIssueService_SearchPages(t *testing.T) {
 	}
 }
 
+func TestIssueService_SearchPages_EmptyResult(t *testing.T) {
+	setup()
+	defer teardown()
+	testMux.HandleFunc("/rest/api/2/search", func(w http.ResponseWriter, r *http.Request) {
+		testMethod(t, r, "GET")
+		if r.URL.String() == "/rest/api/2/search?jql=something&startAt=1&amp;maxResults=50&expand=foo&validateQuery=warn" {
+			w.WriteHeader(http.StatusOK)
+			// This is what Jira outputs when the &maxResult= issue occurs. It used to cause SearchPages to go into an endless loop.
+			fmt.Fprint(w, `{"expand": "schema,names","startAt": 0,"maxResults": 0,"total": 6,"issues": []}`)
+			return
+		}
+
+		t.Errorf("Unexpected URL: %v", r.URL)
+	})
+
+	opt := &SearchOptions{StartAt: 1, MaxResults: 50, Expand: "foo", ValidateQuery: "warn"}
+	issues := make([]Issue, 0)
+	err := testClient.Issue.SearchPages("something", opt, func(issue Issue) error {
+		issues = append(issues, issue)
+		return nil
+	})
+
+	if err != nil {
+		t.Errorf("Error given: %s", err)
+	}
+
+}
+
 func TestIssueService_GetCustomFields(t *testing.T) {
 	setup()
 	defer teardown()

Original file line number	Diff line number	Diff line change
`@@ -965,7 +965,7 @@ func (s IssueService) Search(jql string, options SearchOptions) ([]Issue, *Res`
`965`	`965`	`u += fmt.Sprintf("&startAt=%d", options.StartAt)`
`966`	`966`	`}`
`967`	`967`	`if options.MaxResults != 0 {`
`968`		`- u += fmt.Sprintf("&maxResults=%d", options.MaxResults)`
	`968`	`+ u += fmt.Sprintf("&maxResults=%d", options.MaxResults)`
`969`	`969`	`}`
`970`	`970`	`if options.Expand != "" {`
`971`	`971`	`u += fmt.Sprintf("&expand=%s", options.Expand)`
`@@ -1011,6 +1011,10 @@ func (s IssueService) SearchPages(jql string, options SearchOptions, f func(Is`
`1011`	`1011`	`return err`
`1012`	`1012`	`}`
`1013`	`1013`
	`1014`	`+ if len(issues) == 0 {`
	`1015`	`+ return nil`
	`1016`	`+ }`
	`1017`	`+`
`1014`	`1018`	`for {`
`1015`	`1019`	`for _, issue := range issues {`
`1016`	`1020`	`err = f(issue)`