Merge pull request #17 from hellofresh/patch/fix-crash

PT-3844 Fix crash

Author: startnow65

pkg/zappr/client.go

@@ -144,6 +144,12 @@ func (c *clientImpl) doRequest(req *http.Request) (int, *zapprErrorResponse, err
 	zapprErrorResponse := &zapprErrorResponse{}
 	resp, err := c.slingClient.Do(req, nil, zapprErrorResponse)
 
+	if resp == nil {
+		// Even though 0 does not seem like a valid http response code
+		// The status would be ignored by any calling code anyway since a non-nil error is returned
+		return 0, nil, errwrap.Wrap(ErrZapprServerError, err)
+	}
+
 	if resp.StatusCode == http.StatusUnauthorized {
 		return resp.StatusCode, nil, ErrZapprUnauthorized
 	}
@@ -153,7 +159,7 @@ func (c *clientImpl) doRequest(req *http.Request) (int, *zapprErrorResponse, err
 			return resp.StatusCode, zapprErrorResponse, errwrap.Wrap(errors.New(zapprErrorResponse.Detail), ErrZapprServerError)
 		}
 
-		return resp.StatusCode, nil, ErrZapprServerError
+		return resp.StatusCode, nil, errwrap.Wrap(ErrZapprServerError, err)
 	}
 
 	return resp.StatusCode, nil, err

pkg/zappr/client_test.go

@@ -32,7 +32,7 @@ func TestAuthWithGithubToken(t *testing.T) {
 
 	client.Enable(1)
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }
 
@@ -58,7 +58,7 @@ func TestAuthWithZapprToken(t *testing.T) {
 
 	client.Enable(1)
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }
 
@@ -94,7 +94,7 @@ func TestArbitraryErrorWithJSONErrorBodyFromZappr(t *testing.T) {
 	// Assert error detail retured by zappr is in returned error
 	assert.True(t, errwrap.Contains(err, "Strange error message"), err)
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }
 
@@ -122,7 +122,7 @@ func TestArbitraryErrorWithNonJSONErrorBodyFromZappr(t *testing.T) {
 	// Assert "unknown zappr" error was returned
 	assert.True(t, errwrap.Contains(err, ErrZapprServerError.Error()))
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }
 
@@ -149,7 +149,7 @@ func TestEnable(t *testing.T) {
 	// Assert no errors were received
 	assert.Nil(t, err)
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }
 
@@ -182,7 +182,7 @@ func TestEnable_AlreadyExist(t *testing.T) {
 	// Assert "already enabled" error was returned
 	assert.Equal(t, ErrZapprAlreadyEnabled, err)
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }
 
@@ -209,7 +209,7 @@ func TestDisable(t *testing.T) {
 	// Assert no errors were received
 	assert.Nil(t, err)
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }
 
@@ -242,7 +242,7 @@ func TestDisable_RepoDeletedFromGithub(t *testing.T) {
 	// Assert "already not enabled" error was returned
 	assert.Equal(t, ErrZapprAlreadyNotEnabled, err)
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }
 
@@ -275,6 +275,23 @@ func TestDisable_RepoDeletedFromGithub_AndNotOnZappr(t *testing.T) {
 	// Assert "already not enabled" error was returned
 	assert.Equal(t, ErrZapprAlreadyNotEnabled, err)
 
-	// Assert expected calls we made to Zappr
+	// Assert expected calls were made to Zappr
+	zapprMock.AssertExpectations(t)
+}
+
+func TestProblematicRequest(t *testing.T) {
+	// Get the Zappr Client, Mock Handler and Test Server
+	client, zapprMock, testServer := NewMockAndHandlerNilResponse()
+
+	// Start the test server and stop it when done
+	testServer.Start()
+	defer testServer.Close()
+
+	err := client.Enable(1)
+
+	// Assert "unknown zappr" error was returned
+	assert.True(t, errwrap.Contains(err, ErrZapprServerError.Error()))
+
+	// Assert expected calls were made to Zappr
 	zapprMock.AssertExpectations(t)
 }

pkg/zappr/zappr_mock.go

@@ -1,6 +1,7 @@
 package zappr
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -46,13 +47,33 @@ func newMockAndHandler() (*http.Client, *test.MockHandler, *test.Server) {
 	return httpClient, mockHandler, mockServer
 }
 
+func newMockAndHandlerWithNilResponseTransport() (*http.Client, *test.MockHandler, *test.Server) {
+	mockHandler := &test.MockHandler{}
+	mockServer := test.NewUnstartedServer(mockHandler)
+
+	httpClient := &http.Client{
+		Transport: &NilResponseTransport{},
+	}
+
+	return httpClient, mockHandler, mockServer
+}
+
 // NewMockAndHandler returns a Zappr Client that uses Github Token, Mockhandler, and Server. The client proxies
 // requests to the server and handlers can be registered on the mux to handle
 // requests. The caller must close the test server.
 func NewMockAndHandler() (Client, *test.MockHandler, *test.Server) {
 	return NewMockAndHandlerWithGithubToken("1234567890")
 }
 
+// NewMockAndHandlerNilResponse returns a Zappr Client that uses Github Token, Mockhandler, and Server.
+// The internal http client always returns a nil response object.
+func NewMockAndHandlerNilResponse() (Client, *test.MockHandler, *test.Server) {
+	httpClient, mockHandler, mockServer := newMockAndHandlerWithNilResponseTransport()
+	client := NewWithGithubToken("https://fake.zappr/", "1234567890", httpClient)
+
+	return client, mockHandler, mockServer
+}
+
 // NewMockAndHandlerWithGithubToken returns a Zappr Client that uses Github Token, Mockhandler, and Server. The client proxies
 // requests to the server and handlers can be registered on the mux to handle
 // requests. The caller must close the test server.
@@ -88,3 +109,14 @@ func (t *RewriteTransport) RoundTrip(req *http.Request) (*http.Response, error)
 	}
 	return t.Transport.RoundTrip(req)
 }
+
+// NilResponseTransport always returns a nil response object
+type NilResponseTransport struct {
+	Transport http.RoundTripper
+}
+
+// RoundTrip rewrites the request scheme to http and calls through to the
+// composed RoundTripper or if it is nil, to the http.DefaultTransport.
+func (t *NilResponseTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	return nil, errors.New("some error")
+}