Merge pull request #1090 from lsjostro/fix-link-regexp

connector/gitlab: Fix regexp in Link parser

connector/gitlab/gitlab.go

@@ -22,6 +22,11 @@ const (
 	scopeAPI  = "api"
 )
 
+var (
+	reNext = regexp.MustCompile("<([^>]+)>; rel=\"next\"")
+	reLast = regexp.MustCompile("<([^>]+)>; rel=\"last\"")
+)
+
 // Config holds configuration options for gilab logins.
 type Config struct {
 	BaseURL      string `json:"baseURL"`
@@ -236,9 +241,6 @@ func (c *gitlabConnector) groups(ctx context.Context, client *http.Client) ([]st
 
 	apiURL := c.baseURL + "/api/v4/groups"
 
-	reNext := regexp.MustCompile("<(.*)>; rel=\"next\"")
-	reLast := regexp.MustCompile("<(.*)>; rel=\"last\"")
-
 	groups := []string{}
 	var gitlabGroups []gitlabGroup
 	for {
@@ -272,22 +274,28 @@ func (c *gitlabConnector) groups(ctx context.Context, client *http.Client) ([]st
 
 		link := resp.Header.Get("Link")
 
-		if len(reLast.FindStringSubmatch(link)) > 1 {
-			lastPageURL := reLast.FindStringSubmatch(link)[1]
-
-			if apiURL == lastPageURL {
-				break
-			}
-		} else {
+		apiURL = nextURL(apiURL, link)
+		if apiURL == "" {
 			break
 		}
+	}
+	return groups, nil
+}
 
-		if len(reNext.FindStringSubmatch(link)) > 1 {
-			apiURL = reNext.FindStringSubmatch(link)[1]
-		} else {
-			break
+func nextURL(url string, link string) string {
+	if len(reLast.FindStringSubmatch(link)) > 1 {
+		lastPageURL := reLast.FindStringSubmatch(link)[1]
+
+		if url == lastPageURL {
+			return ""
 		}
+	} else {
+		return ""
+	}
 
+	if len(reNext.FindStringSubmatch(link)) > 1 {
+		return reNext.FindStringSubmatch(link)[1]
 	}
-	return groups, nil
+
+	return ""
 }

connector/gitlab/gitlab_test.go

+package gitlab
+
+import "testing"
+
+var nextURLTests = []struct {
+	link     string
+	expected string
+}{
+	{"<https://gitlab.com/api/v4/groups?page=2&per_page=20>; rel=\"next\", " +
+		"<https://gitlab.com/api/v4/groups?page=1&per_page=20>; rel=\"prev\"; pet=\"cat\", " +
+		"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"last\"",
+		"https://gitlab.com/api/v4/groups?page=2&per_page=20"},
+	{"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"next\", " +
+		"<https://gitlab.com/api/v4/groups?page=2&per_page=20>; rel=\"prev\"; pet=\"dog\", " +
+		"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"last\"",
+		"https://gitlab.com/api/v4/groups?page=3&per_page=20"},
+	{"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"prev\"; pet=\"bunny\", " +
+		"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"last\"",
+		""},
+}
+
+func TestNextURL(t *testing.T) {
+	apiURL := "https://gitlab.com/api/v4/groups"
+	for _, tt := range nextURLTests {
+		apiURL = nextURL(apiURL, tt.link)
+		if apiURL != tt.expected {
+			t.Errorf("Should have returned %s, got %s", tt.expected, apiURL)
+		}
+	}
+}