Inject http.client into httpclient.SendRequest

[Alex-CD]

This is a tiny change that refactors httpclient to:

1. Remove getHttpClient (which was unused).
2. Inject a *http.client into SendRequest, rather than having SendRequest self-initialise one.
3. Have usages of SendRequest initialise a http.client using GetHttpClient

In terms of logic, things are the same as they were before, with the exception of the request being initialized after the client, instead of before.

Now that SendRequest is decoupled from http.DefaultClient, it's a little easier to write unit tests for it (which I'd be happy to do)

I'd be super interested in your thoughts on the general theme of doing dependency injection like this.

[gregorriegler]

Hi @Alex-CD .
Thank you for your effort.
Please bear with me, but I would like the following approach.

I suggest that in order to distinguish between net/http.Client and our http.Client we need better naming.
I suggest to prefix the official http.Client with net: netHttpClient

In order to test our code we need our own wrapper of that netHttpClient, that implements an interface, that also has a Spy implementation.
Such as this code would do:

type Client interface {
	SendRequest(method string, url string, body []byte) error
}

type HttpClient struct {
	netHttpClient *http.Client
}

func CreateHttpClient(disableSSLVerification bool) HttpClient {
	return HttpClient{
		netHttpClient: GetNetHttpClient(disableSSLVerification),
	}
}

func GetNetHttpClient(disableSSLVerification bool) *http.Client {
	if disableSSLVerification {
		transCfg := &http.Transport{
			TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
		}
		return &http.Client{Transport: transCfg}
	}
	return http.DefaultClient
}

type HttpClientSpy struct {
	// used in unittests
	// records calls to http, but does not send them
}

func CreateHttpClientSpy() HttpClientSpy {
	return HttpClientSpy{
		// ...
	}
}

func (c HttpClient) SendRequest(requestMethod string, requestUrl string, requestBody []byte) error {
	say.Info(requestMethod + " " + requestUrl + " " + string(requestBody))
	// ...
}

We never ever use the returned responseBody, so I removed that from the return value.
When we run mob in a unittest, we would just use the Spy implementation.
The implementation of our own wrapper (this can be a spy, or the real one) needs to be either passed down the call stack, or stored somewhere.
I dont yet know whats the best option here. Would start with passing it down the call stack.

A Unittest would look something like this (pseudocode):

func TestBreakOf5(t *testing.T) {
	_, configuration := setup(t)
	httpClient = createHttpClientSpy()

	break(httpClient)

	httpClientSpy.assertCalledWith("GET", "url", "{the expected json}")

Since these are quite some changes, i propose we do it in small steps.

You would have to undo the parameter injection of netHttpClient.
We don't want any code other than our httpclient package, to depend on netHttpClient.
After you undid the injection, i would already merge.

If you like, you could also start preparing the interface and HttpClient Object.

What are your thoughts on this approach?

[Alex-CD]

I'm fully on board with this approach :)
I've pushed an update that removes the dependency injection and gets the initial HttpClient and Client interface.

[Alex-CD]

@gregorriegler I also see what you mean about the naming being confusing -- to that end I've changed some variable names in what I've added (example)

[gregorriegler]

nice, thank you !