From 57f6e0855a3b7bf6f3214ccf7690e4e9ba346020 Mon Sep 17 00:00:00 2001
From: Jakub Bielecki <47531708+jabielecki@users.noreply.github.com>
Date: Wed, 19 Oct 2022 14:02:33 +0200
Subject: [PATCH 1/3] rm duplicated code

Do not duplicate code from packngo.go with the sole purpose
of passing a test through it. The test's goal is to warn about
bad changes to the real code - focus the test on that goal.

Signed-off-by: Jakub Bielecki <47531708+jabielecki@users.noreply.github.com>
---
 mocks_test.go            | 69 ----------------------------------------
 vlan_assignments_test.go | 25 ++++++---------
 2 files changed, 10 insertions(+), 84 deletions(-)

diff --git a/mocks_test.go b/mocks_test.go
index 0d04cb10..7a07bc55 100644
--- a/mocks_test.go
+++ b/mocks_test.go
@@ -2,11 +2,9 @@ package packngo
 
 import (
 	"bytes"
-	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/url"
 )
 
 // MockClient makes it simpler to test the Client
@@ -34,73 +32,6 @@ func (mc *MockClient) DoRequest(method, path string, body, v interface{}) (*Resp
 	return mc.fnDoRequest(method, path, body, v)
 }
 
-/* deadcode, for now
-func mockDoRequestWithHeader(doFn func(req *http.Request, v interface{}) (*Response, error), newRequestFn func(method, path string, body interface{}) (*http.Request, error)) func(string, map[string]string, string, interface{}, interface{}) (*Response, error) {
-	return func(method string, headers map[string]string, path string, body, v interface{}) (*Response, error) {
-		req, err := newRequestFn(method, path, body)
-		for k, v := range headers {
-			req.Header.Add(k, v)
-		}
-
-		if err != nil {
-			return nil, err
-		}
-		return doFn(req, v)
-	}
-}
-*/
-
-func mockNewRequest() func(string, string, interface{}) (*http.Request, error) {
-	baseURL := &url.URL{}
-	apiKey, consumerToken, userAgent := "", "", ""
-	return func(method, path string, body interface{}) (*http.Request, error) {
-		// relative path to append to the endpoint url, no leading slash please
-		if path[0] == '/' {
-			path = path[1:]
-		}
-		rel, err := url.Parse(path)
-		if err != nil {
-			return nil, err
-		}
-
-		u := baseURL.ResolveReference(rel)
-
-		// json encode the request body, if any
-		buf := new(bytes.Buffer)
-		if body != nil {
-			err := json.NewEncoder(buf).Encode(body)
-			if err != nil {
-				return nil, err
-			}
-		}
-
-		req, err := http.NewRequest(method, u.String(), buf)
-		if err != nil {
-			return nil, err
-		}
-
-		req.Close = true
-
-		req.Header.Add("X-Auth-Token", apiKey)
-		req.Header.Add("X-Consumer-Token", consumerToken)
-
-		req.Header.Add("Content-Type", mediaType)
-		req.Header.Add("Accept", mediaType)
-		req.Header.Add("User-Agent", userAgent)
-		return req, nil
-	}
-}
-
-func mockDoRequest(newRequestFn func(string, string, interface{}) (*http.Request, error), doFn func(*http.Request, interface{}) (*Response, error)) func(method, path string, body, v interface{}) (*Response, error) {
-	return func(method, path string, body, v interface{}) (*Response, error) {
-		req, err := newRequestFn(method, path, body)
-		if err != nil {
-			return nil, err
-		}
-		return doFn(req, v)
-	}
-}
-
 // DoRequestWithHeader uses the mock DoRequestWithHeader function
 func (mc *MockClient) DoRequestWithHeader(method string, headers map[string]string, path string, body, v interface{}) (*Response, error) {
 	return mc.fnDoRequestWithHeader(method, headers, path, body, v)
diff --git a/vlan_assignments_test.go b/vlan_assignments_test.go
index 64c600a3..ff62b56d 100644
--- a/vlan_assignments_test.go
+++ b/vlan_assignments_test.go
@@ -3,7 +3,6 @@ package packngo
 import (
 	"encoding/json"
 	"fmt"
-	"net/http"
 	"path"
 	"strconv"
 	"strings"
@@ -52,30 +51,26 @@ func TestVLANAssignmentServiceOp_Get(t *testing.T) {
 		{
 			name: "Simple",
 			fields: fields{
-				client: (func() *MockClient {
+				client: &MockClient{
+					fnDoRequest: func(method, pathURL string, body, v interface{}) (*Response, error) {
+						raw := mockAssignedPortBody(testAssignmentId, testPortId, testVnId)
 
-					raw := mockAssignedPortBody(
-						testAssignmentId, testPortId, testVnId)
-					mockNR := mockNewRequest()
-					mockDo := func(req *http.Request, obj interface{}) (*Response, error) {
 						// baseURL is not needed here
 						expectedPath := path.Join(portBasePath, testPortId, portVLANAssignmentsPath, testAssignmentId)
-						if expectedPath != req.URL.Path {
+						if expectedPath != pathURL {
 							return nil, fmt.Errorf("wrong url")
 						}
-						if err := json.NewDecoder(strings.NewReader(raw)).Decode(obj); err != nil {
+						if err := json.NewDecoder(strings.NewReader(raw)).Decode(v); err != nil {
 							return nil, err
 						}
 
-						return mockResponse(200, raw, req), nil
-					}
-
-					return &MockClient{
-						fnDoRequest: mockDoRequest(mockNR, mockDo),
-					}
-				})(),
+						return mockResponse(200, raw, nil), nil
+					},
+				},
 			},
 			args: args{portID: testPortId, assignmentID: testAssignmentId},
+			// TODO: This is testing the code residing inside (json.Decoder).Decode(), which is already exhaustively tested.
+			// What needs testing is the code residing inside VLANAssignmentServiceOp.Get().
 			want: &VLANAssignment{
 				ID:             testAssignmentId,
 				CreatedAt:      Timestamp{Time: func() time.Time { t, _ := time.Parse(time.RFC3339, "2021-05-28T16:02:33Z"); return t }()},

From c8f9431cba8c3b0934a4a0abe671da480b5f6aea Mon Sep 17 00:00:00 2001
From: Jakub Bielecki <47531708+jabielecki@users.noreply.github.com>
Date: Wed, 19 Oct 2022 12:41:47 +0200
Subject: [PATCH 2/3] add ClientOpt options

Add the ClientOpt, just classic functional options:
https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

The immediate need is to provide means for callers to insert custom
headers into every request. For example, if callers need a third header
in addition to the typical X-Auth-Key and X-Consumer-Token pair, they
can now use WithHeader.

The existing possibility of customizing headers per every request
with DoRequestWithHeaders has the problem that for an existing code base
using packngo.DoRequest heavily, it becomes hard to add a header
to every request without a massive code modification.

I modified the constructor NewClient in a backward-compatible way to
use the functional options. The small difference in logic is that if the
builtin default base API URL turns out to be non-parseable:

  - now NewClient will return: nil Client, appropriate error
  - previously NewClient returned: nil Client, nil error

Also the order of reading env variables changed, which means that
from now on majority of the constructor code can benefit from
Client.debug (`$PACKNGO_DEBUG`). Previously it was known quite late
into the construction.

Signed-off-by: Jakub Bielecki <47531708+jabielecki@users.noreply.github.com>
---
 clientopt.go | 54 ++++++++++++++++++++++++++++++++++++++++++++
 packngo.go   | 64 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 101 insertions(+), 17 deletions(-)
 create mode 100644 clientopt.go

diff --git a/clientopt.go b/clientopt.go
new file mode 100644
index 00000000..c6c09db0
--- /dev/null
+++ b/clientopt.go
@@ -0,0 +1,54 @@
+package packngo
+
+import (
+	"net/http"
+	"net/url"
+)
+
+// ClientOpt is an option usable as an argument to NewClient constructor.
+type ClientOpt func(*Client) error
+
+// WithAuth configures Client with a specific consumerToken and apiKey for subsequent HTTP requests.
+func WithAuth(consumerToken string, apiKey string) ClientOpt {
+	return func(c *Client) error {
+		c.ConsumerToken = consumerToken
+		c.APIKey = apiKey
+
+		return nil
+	}
+}
+
+// WithHTTPClient configures Client to use a specific httpClient for subsequent HTTP requests.
+func WithHTTPClient(httpClient *http.Client) ClientOpt {
+	return func(c *Client) error {
+		c.client = httpClient
+
+		return nil
+	}
+}
+
+// WithBaseURL configures Client to use a nonstandard API URL, e.g. for mocking the remote API.
+func WithBaseURL(apiBaseURL string) ClientOpt {
+	return func(c *Client) error {
+		u, err := url.Parse(apiBaseURL)
+		if err != nil {
+			return err
+		}
+
+		c.BaseURL = u
+
+		return nil
+	}
+}
+
+// WithHeader configures Client to use the given HTTP header set.
+// The headers X-Auth-Token, X-Consumer-Token, User-Agent will be ignored even if provided in the set.
+func WithHeader(header http.Header) ClientOpt {
+	return func(c *Client) error {
+		for k, v := range header {
+			c.header[k] = v
+		}
+
+		return nil
+	}
+}
diff --git a/packngo.go b/packngo.go
index 0932ab86..920d9af0 100644
--- a/packngo.go
+++ b/packngo.go
@@ -90,6 +90,7 @@ type Client struct {
 	UserAgent     string
 	ConsumerToken string
 	APIKey        string
+	header        http.Header
 
 	RateLimit Rate
 
@@ -176,12 +177,11 @@ func (c *Client) NewRequest(method, path string, body interface{}) (*http.Reques
 
 	req.Close = true
 
-	req.Header.Add("X-Auth-Token", c.APIKey)
-	req.Header.Add("X-Consumer-Token", c.ConsumerToken)
+	req.Header = c.header.Clone()
+	req.Header.Set("X-Auth-Token", c.APIKey)
+	req.Header.Set("X-Consumer-Token", c.ConsumerToken)
+	req.Header.Set("User-Agent", c.UserAgent)
 
-	req.Header.Add("Content-Type", mediaType)
-	req.Header.Add("Accept", mediaType)
-	req.Header.Add("User-Agent", c.UserAgent)
 	return req, nil
 }
 
@@ -333,16 +333,6 @@ func (c *Client) DoRequestWithHeader(method string, headers map[string]string, p
 	return c.Do(req, v)
 }
 
-// NewClient initializes and returns a Client
-func NewClient() (*Client, error) {
-	apiToken := os.Getenv(authTokenEnvVar)
-	if apiToken == "" {
-		return nil, fmt.Errorf("you must export %s", authTokenEnvVar)
-	}
-	c := NewClientWithAuth("packngo lib", apiToken, nil)
-	return c, nil
-}
-
 // NewClientWithAuth initializes and returns a Client, use this to get an API Client to operate on
 // N.B.: Equinix Metal's API certificate requires Go 1.5+ to successfully parse. If you are using
 // an older version of Go, pass in a custom http.Client with a custom TLS configuration
@@ -359,12 +349,37 @@ func NewClientWithBaseURL(consumerToken string, apiKey string, httpClient *http.
 		httpClient = http.DefaultClient
 	}
 
-	u, err := url.Parse(apiBaseURL)
+	return NewClient(WithAuth(consumerToken, apiKey), WithHTTPClient(httpClient), WithBaseURL(apiBaseURL))
+}
+
+// NewClient initializes and returns a Client. The opts are functions such as WithAuth,
+// WithHTTPClient, etc.
+//
+// An example:
+//
+//	c, err := NewClient()
+//
+// An alternative example, which avoids reading PACKET_AUTH_TOKEN environment variable:
+//
+//	c, err := NewClient(WithAuth("packngo lib", packetAuthToken))
+func NewClient(opts ...ClientOpt) (*Client, error) {
+	// set defaults, then let caller override them
+	c := &Client{
+		client:        http.DefaultClient,
+		UserAgent:     UserAgent,
+		ConsumerToken: "packngo lib",
+		header:        http.Header{},
+	}
+
+	c.header.Set("Content-Type", mediaType)
+	c.header.Set("Accept", mediaType)
+
+	var err error
+	c.BaseURL, err = url.Parse(baseURL)
 	if err != nil {
 		return nil, err
 	}
 
-	c := &Client{client: httpClient, BaseURL: u, UserAgent: UserAgent, ConsumerToken: consumerToken, APIKey: apiKey}
 	c.APIKeys = &APIKeyServiceOp{client: c}
 	c.BGPConfig = &BGPConfigServiceOp{client: c}
 	c.BGPSessions = &BGPSessionServiceOp{client: c}
@@ -402,6 +417,21 @@ func NewClientWithBaseURL(consumerToken string, apiKey string, httpClient *http.
 	c.VLANAssignments = &VLANAssignmentServiceOp{client: c}
 	c.debug = os.Getenv(debugEnvVar) != ""
 
+	for _, fn := range opts {
+		err := fn(c)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	if c.APIKey == "" {
+		c.APIKey = os.Getenv(authTokenEnvVar)
+	}
+
+	if c.APIKey == "" {
+		return nil, fmt.Errorf("you must export %s", authTokenEnvVar)
+	}
+
 	return c, nil
 }
 

From b386c0b8deb8ad80721785855a1b42ab008c0d88 Mon Sep 17 00:00:00 2001
From: Jakub Bielecki <47531708+jabielecki@users.noreply.github.com>
Date: Fri, 28 Oct 2022 12:02:06 +0200
Subject: [PATCH 3/3] add private field Client.apiKeySet

Keep backward compatibility for users who emitted an empty X-Auth-Token
(verbatim `X-Auth-Token: ` and end of line). Could happen in tests
and whatnot.

Signed-off-by: Jakub Bielecki <47531708+jabielecki@users.noreply.github.com>
---
 clientopt.go |  1 +
 packngo.go   | 11 +++++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/clientopt.go b/clientopt.go
index c6c09db0..11096ab6 100644
--- a/clientopt.go
+++ b/clientopt.go
@@ -13,6 +13,7 @@ func WithAuth(consumerToken string, apiKey string) ClientOpt {
 	return func(c *Client) error {
 		c.ConsumerToken = consumerToken
 		c.APIKey = apiKey
+		c.apiKeySet = true
 
 		return nil
 	}
diff --git a/packngo.go b/packngo.go
index 920d9af0..25c9c331 100644
--- a/packngo.go
+++ b/packngo.go
@@ -90,6 +90,7 @@ type Client struct {
 	UserAgent     string
 	ConsumerToken string
 	APIKey        string
+	apiKeySet     bool
 	header        http.Header
 
 	RateLimit Rate
@@ -424,12 +425,14 @@ func NewClient(opts ...ClientOpt) (*Client, error) {
 		}
 	}
 
-	if c.APIKey == "" {
+	if !c.apiKeySet {
 		c.APIKey = os.Getenv(authTokenEnvVar)
-	}
 
-	if c.APIKey == "" {
-		return nil, fmt.Errorf("you must export %s", authTokenEnvVar)
+		if c.APIKey == "" {
+			return nil, fmt.Errorf("you must export %s", authTokenEnvVar)
+		}
+
+		c.apiKeySet = true
 	}
 
 	return c, nil