Repository: inbucket/inbucket
Pull request #463 (2024-01-23 10:24:22)

[Rest Client] Use options for client.New
The rest client does not currently allow a user to pass options, such as a custom transport. This PR extends the `client.New` function to allow for custom options to be passed

coveralls: ## Pull Request Test Coverage Report for [Build 7624706559](https://coveralls.io/builds/65238856)


* **0** of **0**   changed or added relevant lines in **0** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.006%**) to **55.337%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/65238856/badge)](https://coveralls.io/builds/65238856) |
| :-- | --: |
| Change from base [Build 7358487704](https://coveralls.io/builds/64801076): |  0.006% |
| Covered Lines: | 2639 |
| Relevant Lines: | 4769 |

---
##### 💛  - [Coveralls](https://coveralls.io)

jhillyerd: The problem with this implementation is that the `ClientOptions` struct becomes part of our public API, limiting how we can change it in the future.  The [Uber](https://github.com/uber-go/guide/blob/master/style.md#functional-options) method is definitely more cumbersome to implement, but allows us to keep it private.
corey-aloia: Thanks for the link @jhillyerd 

Actually, I can just make ClientOpts small and then it is not exposed, and still works. It looks like ubers argument is:
```
that there's a method of implementing this pattern with closures but we believe that the pattern above provides more flexibility for authors and is easier to debug and test for users. In particular, it allows options to be compared against each other in tests and mocks, versus closures where this is impossible. Further, it lets options implement other interfaces, including fmt.Stringer which allows for user-readable string representations of the options.
```
Im not sure that 
I can change from the pattern with closure to the one you and uber use with apply 👍 
jhillyerd: Looks good, but apologies, I should have mentioned this before:  What do you think about dropping the Client name prefix from most of the types/funcs you are introducing?  I know the existing client.Client type has repetition in it, but I think we could use client.Option interface and client.WithOptTransport instead.
corey-aloia: > Looks good, but apologies, I should have mentioned this before: What do you think about dropping the Client name prefix from most of the types/funcs you are introducing? I know the existing client.Client type has repetition in it, but I think we could use client.Option interface and client.WithOptTransport instead.

Good point.... client.client* is repetitive, I also thought about changing but didnt for some reason 😄 .... will adjust 👍 

Patch:
diff --git a/pkg/rest/client/apiv1_client.go b/pkg/rest/client/apiv1_client.go
index ce8990a..5a50c56 100644
--- a/pkg/rest/client/apiv1_client.go
+++ b/pkg/rest/client/apiv1_client.go
@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
-	"time"
 
 	"github.com/inbucket/inbucket/v3/pkg/rest/model"
 )
@@ -18,15 +17,22 @@ type Client struct {
 
 // New creates a new v1 REST API client given the base URL of an Inbucket server, ex:
 // "http://localhost:9000"
-func New(baseURL string) (*Client, error) {
+func New(baseURL string, opts ...Option) (*Client, error) {
 	parsedURL, err := url.Parse(baseURL)
 	if err != nil {
 		return nil, err
 	}
+
+	mergedOpts := getDefaultOptions()
+	for _, opt := range opts {
+		opt.apply(mergedOpts)
+	}
+
 	c := &Client{
 		restClient{
 			client: &http.Client{
-				Timeout: 30 * time.Second,
+				Timeout:   mergedOpts.timeout,
+				Transport: mergedOpts.transport,
 			},
 			baseURL: parsedURL,
 		},
diff --git a/pkg/rest/client/apiv1_client_opts.go b/pkg/rest/client/apiv1_client_opts.go
new file mode 100644
index 0000000..49fd67c
--- /dev/null
+++ b/pkg/rest/client/apiv1_client_opts.go
@@ -0,0 +1,38 @@
+package client
+
+import (
+	"net/http"
+	"time"
+)
+
+// options is a struct that holds the options for the rest client
+type options struct {
+	transport http.RoundTripper
+	timeout   time.Duration
+}
+
+type Option interface {
+	apply(*options)
+}
+
+func getDefaultOptions() *options {
+	return &options{
+		timeout: 30 * time.Second,
+	}
+}
+
+type transportOption struct {
+	transport http.RoundTripper
+}
+
+func (t transportOption) apply(opts *options) {
+	opts.transport = t.transport
+}
+
+// WithOptTransport sets the transport for the rest client.
+// Transport specifies the mechanism by which individual
+// HTTP requests are made.
+// If nil, http.DefaultTransport is used.
+func WithOptTransport(transport http.RoundTripper) Option {
+	return transportOption{transport}
+}


Test patch:
diff --git a/pkg/rest/client/apiv1_client_test.go b/pkg/rest/client/apiv1_client_test.go
index 22eb2f5..f7f84e2 100644
--- a/pkg/rest/client/apiv1_client_test.go
+++ b/pkg/rest/client/apiv1_client_test.go
@@ -1,6 +1,8 @@
 package client_test
 
 import (
+	"bytes"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -210,6 +212,33 @@ func TestClientV1GetMessageSource(t *testing.T) {
 	}
 }
 
+func TestClientV1WithCustomTransport(t *testing.T) {
+	// Call setup, passing a custom roundtripper and make sure it was used during the request.
+	mockRoundTripper := &mockRoundTripper{ResponseBody: "Custom Transport"}
+	c, router, teardown := setup(client.WithOptTransport(mockRoundTripper))
+
+	defer teardown()
+
+	router.Path("/api/v1/mailbox/testbox/20170107T224128-0000/source").Methods("GET").
+		Handler(&jsonHandler{json: `message source`})
+
+	// Method under test.
+	source, err := c.GetMessageSource("testbox", "20170107T224128-0000")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	want := mockRoundTripper.ResponseBody
+	got := source.String()
+	if got != want {
+		t.Errorf("Source got %q, want %q", got, want)
+	}
+
+	if mockRoundTripper.CallCount != 1 {
+		t.Errorf("RoundTripper called %v times, want 1", mockRoundTripper.CallCount)
+	}
+}
+
 func TestClientV1DeleteMessage(t *testing.T) {
 	// Setup.
 	c, router, teardown := setup()
@@ -337,11 +366,24 @@ func TestClientV1MessageHeader(t *testing.T) {
 	}
 }
 
+type mockRoundTripper struct {
+	ResponseBody string
+	CallCount    int
+}
+
+func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	m.CallCount++
+	return &http.Response{
+		StatusCode: http.StatusOK,
+		Body:       io.NopCloser(bytes.NewBufferString(m.ResponseBody)),
+	}, nil
+}
+
 // setup returns a client, router and server for API testing.
-func setup() (c *client.Client, router *mux.Router, teardown func()) {
+func setup(opts ...client.Option) (c *client.Client, router *mux.Router, teardown func()) {
 	router = mux.NewRouter()
 	server := httptest.NewServer(router)
-	c, err := client.New(server.URL)
+	c, err := client.New(server.URL, opts...)
 	if err != nil {
 		panic(err)
 	}