Bug 1723422 - properly handle pagination url (#182)

* Bug 1723422 - properly handle pagination url

* set path and query string properly
* fix getNextImageURL parsing of path url
* add unit test for getNextImageURL
* add unit test for NewClient
* change variable name per review comment
* add unit test for NewClient and NewRequest
* fix lint errors

registries/adapters/apiv2_adapter.go

@@ -230,6 +230,7 @@ func (r APIV2Adapter) getNextImages(url string) (*apiV2CatalogResponse, string,
 	}
 	log.Debug("Properly unmarshalled image response")
 
+	log.Debugf("link header [%s]", resp.Header.Get("Link"))
 	return &imageList, resp.Header.Get("Link"), nil
 }
 
@@ -240,6 +241,9 @@ func (r APIV2Adapter) getNextImageURL(link string) string {
 		return ""
 	}
 
+	// rudimentary Link header parser
+	// refer to the full RFC5988 spec if you need more functionality
+	// https://tools.ietf.org/html/rfc5988
 	res := strings.Split(link, ";")
 	if len(res[0]) == 0 {
 		log.Errorf("Invalid Link value")

registries/adapters/apiv2_adapter_test.go

@@ -17,6 +17,7 @@
 package adapters
 
 import (
+	"fmt"
 	"sort"
 	"testing"
 
@@ -187,3 +188,59 @@ func TestAPIV2FetchSpecs(t *testing.T) {
 		t.Fatal("Error: did not find 3 expected specs, only found: ", len(specs))
 	}
 }
+
+func TestGetNextImageURL(t *testing.T) {
+
+	serv := adaptertest.GetAPIV2Server(t, true)
+	defer serv.Close()
+	testConfig.URL = adaptertest.GetURL(t, serv)
+	apiv2a, err := NewAPIV2Adapter(testConfig)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	testCases := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "empty link",
+			input:    "",
+			expected: "",
+		},
+		{
+			name:     "no semicolon",
+			input:    "blah blah blah",
+			expected: fmt.Sprintf("%sblah blah blah", testConfig.URL.String()),
+		},
+		{
+			name:     "proper link header",
+			input:    "</v2/_catalog?last=ansibleplaybookbundle%2Fetherpad-apb&n=3>; rel=\"next\"",
+			expected: fmt.Sprintf("%s/v2/_catalog?last=ansibleplaybookbundle%%2Fetherpad-apb&n=3", testConfig.URL.String()),
+		},
+		{
+			name:     "starts with semicolon",
+			input:    "; rel=\"next\"",
+			expected: "",
+		},
+		{
+			name:     "spaces around the url",
+			input:    "      </v2/_catalog?last=ansibleplaybookbundle&n=3>     ; rel=\"next\"",
+			expected: fmt.Sprintf("%s/v2/_catalog?last=ansibleplaybookbundle&n=3", testConfig.URL.String()),
+		},
+		{
+			name:     "missing <> brackets",
+			input:    "/v2/_catalog?last=ansibleplaybookbundle&n=3; rel=\"next\"",
+			expected: fmt.Sprintf("%s/v2/_catalog?last=ansibleplaybookbundle&n=3", testConfig.URL.String()),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			output := apiv2a.getNextImageURL(tc.input)
+			ft.Equal(t, tc.expected, output)
+		})
+
+	}
+}

registries/adapters/oauth/client.go

@@ -81,7 +81,16 @@ func (c *Client) NewRequest(path string) (*http.Request, error) {
 	if err != nil {
 		return nil, err
 	}
-	req.URL.Path = path
+
+	// path might be a fully qualified URL and we only want the path
+	// and query bits
+	pathAsURL, err := url.Parse(path)
+	if err != nil {
+		return nil, err
+	}
+
+	req.URL.Path = pathAsURL.Path
+	req.URL.RawQuery = pathAsURL.Query().Encode()
 	if c.token != "" {
 		req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", c.token))
 	}

registries/adapters/oauth/client_test.go

@@ -17,9 +17,12 @@
 package oauth
 
 import (
+	"fmt"
 	"net/url"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 var headerCases = map[string]string{
@@ -96,22 +99,146 @@ func TestParseAuthTokenErrors(t *testing.T) {
 }
 
 func TestNewRequest(t *testing.T) {
-	u, _ := url.Parse("http://automationbroker.io")
-	c := NewClient("foo", "bar", false, u)
-	c.token = "letmein"
-	req, err := c.NewRequest("/v2/")
+	u, err := url.Parse("http://automationbroker.io")
 	if err != nil {
-		t.Error(err.Error())
-		return
+		t.Fatal("invalid url", err)
+	}
+	c := NewClient("foo", "bar", false, u)
+
+	testCases := []struct {
+		name        string
+		input       string
+		token       string
+		expectederr bool
+	}{
+		{
+			name:  "relative path",
+			input: "/v2/",
+			token: "letmein",
+		},
+		{
+			name:  "relative path without trailing slash",
+			input: "/v2",
+		},
+		{
+			name:  "relative path with multiple paths",
+			input: "/v2/foobar/baz",
+		},
+		{
+			name:  "relative path with hook",
+			input: "v2/_catalog/?n=5&last=mediawiki-apb",
+		},
+		{
+			name:  "fully qualified url with hook",
+			input: "https://example.com/v2/_catalog/?n=5&last=mediawiki-apb",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// set the token before calling NewRequest
+			c.token = tc.token
+
+			output, err := c.NewRequest(tc.input)
+			if tc.expectederr {
+				assert.Error(t, err)
+				assert.NotEmpty(t, err.Error())
+			} else if err != nil {
+				fmt.Println(err.Error())
+				t.Fatalf("unexpected error during test: %v\n", err)
+			}
+
+			assert.Equal(t, "application/json", output.Header.Get("Accept"))
+			if tc.token != "" {
+				assert.Equal(t, fmt.Sprintf("Bearer %s", tc.token), output.Header.Get("Authorization"))
+			} else {
+				assert.Equal(t, "", output.Header.Get("Authorization"))
+			}
+
+			expectedurl, err := url.Parse(tc.input)
+			if err != nil {
+				t.Fatalf("Invalid input url %s; %v\n", tc.input, err)
+			}
+
+			assert.Equal(t, c.url.Scheme, output.URL.Scheme)
+			assert.Equal(t, c.url.Host, output.URL.Host)
+			assert.Equal(t, expectedurl.Path, output.URL.Path)
+			assert.Equal(t, expectedurl.Query(), output.URL.Query())
+		})
 	}
-	accepth := req.Header.Get("Accept")
-	if accepth != "application/json" {
-		t.Errorf("incorrect or missing accept header: %s", accepth)
-		return
+}
+
+func TestNewClient(t *testing.T) {
+	testCases := []struct {
+		name       string
+		username   string
+		password   string
+		skipVerify bool
+		url        func() *url.URL
+	}{
+		{
+			name:       "fully qualified url",
+			username:   "foo",
+			password:   "bar",
+			skipVerify: false,
+			url: func() *url.URL {
+				daurl, err := url.Parse("http://automationbroker.io")
+				if err != nil {
+					t.Fatal(err)
+				}
+				return daurl
+			},
+		},
+		{
+			name:       "nil url",
+			username:   "foo",
+			password:   "bar",
+			skipVerify: false,
+			url: func() *url.URL {
+				return nil
+			},
+		},
+		{
+			name:       "empty username and password",
+			username:   "",
+			password:   "",
+			skipVerify: false,
+			url: func() *url.URL {
+				daurl, err := url.Parse("http://automationbroker.io")
+				if err != nil {
+					t.Fatal(err)
+				}
+				return daurl
+			},
+		},
+		{
+			name:       "skip verify true",
+			username:   "user",
+			password:   "pass",
+			skipVerify: true,
+			url: func() *url.URL {
+				daurl, err := url.Parse("http://automationbroker.io")
+				if err != nil {
+					t.Fatal(err)
+				}
+				return daurl
+			},
+		},
 	}
-	authh := req.Header.Get("Authorization")
-	if authh != "Bearer letmein" {
-		t.Errorf("incorrect or missing authorization header: %s", authh)
-		return
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fullURL := tc.url()
+
+			output := NewClient(tc.username, tc.password, tc.skipVerify, fullURL)
+
+			assert.NotNil(t, output)
+			assert.Equal(t, tc.username, output.user)
+			assert.Equal(t, tc.password, output.pass)
+			assert.Equal(t, fullURL, output.url)
+			assert.NotNil(t, output.client)
+			// token should always be empty after NewClient is called
+			assert.Equal(t, "", output.token)
+		})
 	}
 }