Merge branch 'refactor/artifact-try-make-request' into 'master'

refactor: remove host out of Artifact.TryMakeRequest

Closes #760

See merge request gitlab-org/gitlab-pages!787

6 files changed, 21 insertions, 21 deletions

diff --git a/internal/artifact/artifact.go b/internal/artifact/artifact.go
index dab1fb91..87ce1450 100644
--- a/internal/artifact/artifact.go
+++ b/internal/artifact/artifact.go
@@ -17,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httptransport"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/logging"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 )
 
 const (
@@ -61,11 +62,13 @@ func New(server string, timeoutSeconds int, pagesDomain string) *Artifact {
 // http.ResponseWriter, ultimately returning a bool that indicates if the
 // http.ResponseWriter has been written to in any capacity. Additional handler func
 // may be given which should return true if it did handle the response.
-func (a *Artifact) TryMakeRequest(host string, w http.ResponseWriter, r *http.Request, token string, additionalHandler func(*http.Response) bool) bool {
-	if a == nil || a.server == "" || host == "" {
+func (a *Artifact) TryMakeRequest(w http.ResponseWriter, r *http.Request, token string, additionalHandler func(*http.Response) bool) bool {
+	if a == nil || a.server == "" {
 		return false
 	}
 
+	host := request.GetHostWithoutPort(r)
+
 	reqURL, ok := a.BuildURL(host, r.URL.Path)
 	if !ok {
 		return false
diff --git a/internal/artifact/artifact_test.go b/internal/artifact/artifact_test.go
index b8c2771b..c015e44f 100644
--- a/internal/artifact/artifact_test.go
+++ b/internal/artifact/artifact_test.go
@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
-	"net/url"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -74,12 +73,12 @@ func TestTryMakeRequest(t *testing.T) {
 	for _, c := range cases {
 		t.Run(c.Description, func(t *testing.T) {
 			result := httptest.NewRecorder()
-			reqURL, err := url.Parse("/-/subgroup/project/-/jobs/1/artifacts" + c.Path)
+			url := "https://group.gitlab-example.io/-/subgroup/project/-/jobs/1/artifacts" + c.Path
+			r, err := http.NewRequest(http.MethodGet, url, nil)
 			require.NoError(t, err)
-			r := &http.Request{URL: reqURL}
 			art := artifact.New(testServer.URL, 1, "gitlab-example.io")
 
-			require.True(t, art.TryMakeRequest("group.gitlab-example.io", result, r, c.Token, func(resp *http.Response) bool { return false }))
+			require.True(t, art.TryMakeRequest(result, r, c.Token, func(resp *http.Response) bool { return false }))
 			require.Equal(t, c.Status, result.Code)
 			require.Equal(t, c.ContentType, result.Header().Get("Content-Type"))
 			require.Equal(t, c.Length, result.Header().Get("Content-Length"))
@@ -284,15 +283,16 @@ func TestContextCanceled(t *testing.T) {
 	t.Cleanup(testServer.Close)
 
 	result := httptest.NewRecorder()
-	reqURL, err := url.Parse("/-/subgroup/project/-/jobs/1/artifacts/200.html")
+	url := "https://group.gitlab-example.io/-/subgroup/project/-/jobs/1/artifacts/200.html"
+	r, err := http.NewRequest(http.MethodGet, url, nil)
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+
 	ctx, cancel := context.WithCancel(context.Background())
 	r = r.WithContext(ctx)
 	// cancel context explicitly
 	cancel()
 	art := artifact.New(testServer.URL, 1, "gitlab-example.io")
 
-	require.True(t, art.TryMakeRequest("group.gitlab-example.io", result, r, "", func(resp *http.Response) bool { return false }))
+	require.True(t, art.TryMakeRequest(result, r, "", func(resp *http.Response) bool { return false }))
 	require.Equal(t, http.StatusNotFound, result.Code)
 }
diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go
index c91491f0..80c771d5 100644
--- a/internal/handlers/handlers.go
+++ b/internal/handlers/handlers.go
@@ -5,7 +5,6 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/logging"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 )
 
 // Handlers take care of handling specific requests
@@ -63,10 +62,8 @@ func (a *Handlers) HandleArtifactRequest(w http.ResponseWriter, r *http.Request)
 		return true
 	}
 
-	host := request.GetHostWithoutPort(r)
-
 	// nolint: bodyclose // false positive
 	// a.checkIfLoginRequiredOrInvalidToken returns a response.Body, closing this body is responsibility
 	// of the TryMakeRequest implementation
-	return a.Artifact.TryMakeRequest(host, w, r, token, a.checkIfLoginRequiredOrInvalidToken(w, r, token))
+	return a.Artifact.TryMakeRequest(w, r, token, a.checkIfLoginRequiredOrInvalidToken(w, r, token))
 }
diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go
index aedc077c..99ba8ca9 100644
--- a/internal/handlers/handlers_test.go
+++ b/internal/handlers/handlers_test.go
@@ -18,7 +18,7 @@ func TestNotHandleArtifactRequestReturnsFalse(t *testing.T) {
 
 	mockArtifact := mock.NewMockArtifact(mockCtrl)
 	mockArtifact.EXPECT().
-		TryMakeRequest(gomock.Any(), gomock.Any(), gomock.Any(), "", gomock.Any()).
+		TryMakeRequest(gomock.Any(), gomock.Any(), "", gomock.Any()).
 		Return(false).
 		Times(1)
 
@@ -43,7 +43,7 @@ func TestHandleArtifactRequestedReturnsTrue(t *testing.T) {
 
 	mockArtifact := mock.NewMockArtifact(mockCtrl)
 	mockArtifact.EXPECT().
-		TryMakeRequest(gomock.Any(), gomock.Any(), gomock.Any(), "", gomock.Any()).
+		TryMakeRequest(gomock.Any(), gomock.Any(), "", gomock.Any()).
 		Return(true).
 		Times(1)
 
@@ -175,7 +175,7 @@ func TestHandleArtifactRequestButGetTokenFails(t *testing.T) {
 
 	mockArtifact := mock.NewMockArtifact(mockCtrl)
 	mockArtifact.EXPECT().
-		TryMakeRequest(gomock.Any(), gomock.Any(), gomock.Any(), "", gomock.Any()).
+		TryMakeRequest(gomock.Any(), gomock.Any(), "", gomock.Any()).
 		Times(0)
 
 	mockAuth := mock.NewMockAuth(mockCtrl)
diff --git a/internal/handlers/mock/handler_mock.go b/internal/handlers/mock/handler_mock.go
index 11548221..089b567c 100644
--- a/internal/handlers/mock/handler_mock.go
+++ b/internal/handlers/mock/handler_mock.go
@@ -35,17 +35,17 @@ func (m *MockArtifact) EXPECT() *MockArtifactMockRecorder {
 }
 
 // TryMakeRequest mocks base method.
-func (m *MockArtifact) TryMakeRequest(host string, w http.ResponseWriter, r *http.Request, token string, responseHandler func(*http.Response) bool) bool {
+func (m *MockArtifact) TryMakeRequest(w http.ResponseWriter, r *http.Request, token string, responseHandler func(*http.Response) bool) bool {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "TryMakeRequest", host, w, r, token, responseHandler)
+	ret := m.ctrl.Call(m, "TryMakeRequest", w, r, token, responseHandler)
 	ret0, _ := ret[0].(bool)
 	return ret0
 }
 
 // TryMakeRequest indicates an expected call of TryMakeRequest.
-func (mr *MockArtifactMockRecorder) TryMakeRequest(host, w, r, token, responseHandler interface{}) *gomock.Call {
+func (mr *MockArtifactMockRecorder) TryMakeRequest(w, r, token, responseHandler interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TryMakeRequest", reflect.TypeOf((*MockArtifact)(nil).TryMakeRequest), host, w, r, token, responseHandler)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TryMakeRequest", reflect.TypeOf((*MockArtifact)(nil).TryMakeRequest), w, r, token, responseHandler)
 }
 
 // MockAuth is a mock of Auth interface.
diff --git a/internal/interface.go b/internal/interface.go
index 3e82ee3a..2a77e569 100644
--- a/internal/interface.go
+++ b/internal/interface.go
@@ -6,7 +6,7 @@ import (
 
 // Artifact allows to handle artifact related requests
 type Artifact interface {
-	TryMakeRequest(host string, w http.ResponseWriter, r *http.Request, token string, responseHandler func(*http.Response) bool) bool
+	TryMakeRequest(w http.ResponseWriter, r *http.Request, token string, responseHandler func(*http.Response) bool) bool
 }
 
 // Auth handles the authentication logic