Refactor: Add Payload Validators + Middleware In Go Code (#366)

refactor: Refactors the Go code to introduce a middleware pattern, and adds more robust logging options.
harrisoncramer · Sep 12, 2024 · 9f963d5 · 9f963d5
1 parent d4faec4
commit 9f963d5
Show file tree

Hide file tree

Showing 57 changed files with 1,444 additions and 1,249 deletions.
diff --git a/cmd/app/approve.go b/cmd/app/approve.go
@@ -17,14 +17,7 @@ type mergeRequestApproverService struct {
 }
 
 /* approveHandler approves a merge request. */
-func (a mergeRequestApproverService) handler(w http.ResponseWriter, r *http.Request) {
-	w.Header().Set("Content-Type", "application/json")
-	if r.Method != http.MethodPost {
-		w.Header().Set("Access-Control-Allow-Methods", http.MethodPost)
-		handleError(w, InvalidRequestError{}, "Expected POST", http.StatusMethodNotAllowed)
-		return
-	}
-
+func (a mergeRequestApproverService) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	_, res, err := a.client.ApproveMergeRequest(a.projectInfo.ProjectId, a.projectInfo.MergeId, nil, nil)
 
 	if err != nil {
@@ -33,15 +26,12 @@ func (a mergeRequestApproverService) handler(w http.ResponseWriter, r *http.Requ
 	}
 
 	if res.StatusCode >= 300 {
-		handleError(w, GenericError{endpoint: "/mr/approve"}, "Could not approve merge request", res.StatusCode)
+		handleError(w, GenericError{r.URL.Path}, "Could not approve merge request", res.StatusCode)
 		return
 	}
 
 	w.WriteHeader(http.StatusOK)
-	response := SuccessResponse{
-		Message: "Approved MR",
-		Status:  http.StatusOK,
-	}
+	response := SuccessResponse{Message: "Approved MR"}
 
 	err = json.NewEncoder(w).Encode(response)
 	if err != nil {

diff --git a/cmd/app/approve_test.go b/cmd/app/approve_test.go
@@ -23,33 +23,36 @@ func TestApproveHandler(t *testing.T) {
 	t.Run("Approves merge request", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/mr/approve", nil)
 		client := fakeApproverClient{}
-		svc := mergeRequestApproverService{testProjectData, client}
+		svc := middleware(
+			mergeRequestApproverService{testProjectData, client},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withMethodCheck(http.MethodPost),
+		)
 		data := getSuccessData(t, svc, request)
 		assert(t, data.Message, "Approved MR")
-		assert(t, data.Status, http.StatusOK)
-	})
-
-	t.Run("Disallows non-POST method", func(t *testing.T) {
-		request := makeRequest(t, http.MethodGet, "/mr/approve", nil)
-		client := fakeApproverClient{}
-		svc := mergeRequestApproverService{testProjectData, client}
-		data := getFailData(t, svc, request)
-		checkBadMethod(t, data, http.MethodPost)
 	})
 
 	t.Run("Handles errors from Gitlab client", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/mr/approve", nil)
 		client := fakeApproverClient{testBase{errFromGitlab: true}}
-		svc := mergeRequestApproverService{testProjectData, client}
-		data := getFailData(t, svc, request)
+		svc := middleware(
+			mergeRequestApproverService{testProjectData, client},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withMethodCheck(http.MethodPost),
+		)
+		data, _ := getFailData(t, svc, request)
 		checkErrorFromGitlab(t, data, "Could not approve merge request")
 	})
 
 	t.Run("Handles non-200s from Gitlab client", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/mr/approve", nil)
 		client := fakeApproverClient{testBase{status: http.StatusSeeOther}}
-		svc := mergeRequestApproverService{testProjectData, client}
-		data := getFailData(t, svc, request)
+		svc := middleware(
+			mergeRequestApproverService{testProjectData, client},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withMethodCheck(http.MethodPost),
+		)
+		data, _ := getFailData(t, svc, request)
 		checkNon200(t, data, "Could not approve merge request", "/mr/approve")
 	})
 }
diff --git a/cmd/app/assignee.go b/cmd/app/assignee.go
@@ -2,52 +2,33 @@ package app
 
 import (
 	"encoding/json"
-	"io"
+	"errors"
 	"net/http"
 
 	"github.com/xanzy/go-gitlab"
 )
 
 type AssigneeUpdateRequest struct {
-	Ids []int `json:"ids"`
+	Ids []int `json:"ids" validate:"required"`
 }
 
 type AssigneeUpdateResponse struct {
 	SuccessResponse
 	Assignees []*gitlab.BasicUser `json:"assignees"`
 }
 
-type AssigneesRequestResponse struct {
-	SuccessResponse
-	Assignees []int `json:"assignees"`
-}
-
 type assigneesService struct {
 	data
 	client MergeRequestUpdater
 }
 
 /* assigneesHandler adds or removes assignees from a merge request. */
-func (a assigneesService) handler(w http.ResponseWriter, r *http.Request) {
-	w.Header().Set("Content-Type", "application/json")
-	if r.Method != http.MethodPut {
-		w.Header().Set("Access-Control-Allow-Methods", http.MethodPut)
-		handleError(w, InvalidRequestError{}, "Expected PUT", http.StatusMethodNotAllowed)
-		return
-	}
+func (a assigneesService) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
-	body, err := io.ReadAll(r.Body)
-	if err != nil {
-		handleError(w, err, "Could not read request body", http.StatusBadRequest)
-		return
-	}
+	assigneeUpdateRequest, ok := r.Context().Value(payload("payload")).(*AssigneeUpdateRequest)
 
-	defer r.Body.Close()
-	var assigneeUpdateRequest AssigneeUpdateRequest
-	err = json.Unmarshal(body, &assigneeUpdateRequest)
-
-	if err != nil {
-		handleError(w, err, "Could not read JSON from request", http.StatusBadRequest)
+	if !ok {
+		handleError(w, errors.New("Could not get payload from context"), "Bad payload", http.StatusInternalServerError)
 		return
 	}
 
@@ -61,17 +42,14 @@ func (a assigneesService) handler(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if res.StatusCode >= 300 {
-		handleError(w, GenericError{endpoint: "/mr/assignee"}, "Could not modify merge request assignees", res.StatusCode)
+		handleError(w, GenericError{r.URL.Path}, "Could not modify merge request assignees", res.StatusCode)
 		return
 	}
 
 	w.WriteHeader(http.StatusOK)
 	response := AssigneeUpdateResponse{
-		SuccessResponse: SuccessResponse{
-			Message: "Assignees updated",
-			Status:  http.StatusOK,
-		},
-		Assignees: mr.Assignees,
+		SuccessResponse: SuccessResponse{Message: "Assignees updated"},
+		Assignees:       mr.Assignees,
 	}
 
 	err = json.NewEncoder(w).Encode(response)

diff --git a/cmd/app/assignee_test.go b/cmd/app/assignee_test.go
@@ -24,34 +24,39 @@ func TestAssigneeHandler(t *testing.T) {
 
 	t.Run("Updates assignees", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPut, "/mr/assignee", updatePayload)
-		client := fakeAssigneeClient{}
-		svc := assigneesService{testProjectData, client}
+		svc := middleware(
+			assigneesService{testProjectData, fakeAssigneeClient{}},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{http.MethodPut: &AssigneeUpdateRequest{}}),
+			withMethodCheck(http.MethodPut),
+		)
 		data := getSuccessData(t, svc, request)
 		assert(t, data.Message, "Assignees updated")
-		assert(t, data.Status, http.StatusOK)
-	})
-
-	t.Run("Disallows non-PUT method", func(t *testing.T) {
-		request := makeRequest(t, http.MethodGet, "/mr/assignee", nil)
-		client := fakeAssigneeClient{}
-		svc := assigneesService{testProjectData, client}
-		data := getFailData(t, svc, request)
-		checkBadMethod(t, data, http.MethodPut)
 	})
 
 	t.Run("Handles errors from Gitlab client", func(t *testing.T) {
-		request := makeRequest(t, http.MethodPut, "/mr/approve", updatePayload)
+		request := makeRequest(t, http.MethodPut, "/mr/assignee", updatePayload)
 		client := fakeAssigneeClient{testBase{errFromGitlab: true}}
-		svc := assigneesService{testProjectData, client}
-		data := getFailData(t, svc, request)
+		svc := middleware(
+			assigneesService{testProjectData, client},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{http.MethodPut: &AssigneeUpdateRequest{}}),
+			withMethodCheck(http.MethodPut),
+		)
+		data, _ := getFailData(t, svc, request)
 		checkErrorFromGitlab(t, data, "Could not modify merge request assignees")
 	})
 
 	t.Run("Handles non-200s from Gitlab client", func(t *testing.T) {
-		request := makeRequest(t, http.MethodPut, "/mr/approve", updatePayload)
+		request := makeRequest(t, http.MethodPut, "/mr/assignee", updatePayload)
 		client := fakeAssigneeClient{testBase{status: http.StatusSeeOther}}
-		svc := assigneesService{testProjectData, client}
-		data := getFailData(t, svc, request)
+		svc := middleware(
+			assigneesService{testProjectData, client},
+			withMr(testProjectData, fakeMergeRequestLister{}),
+			withPayloadValidation(methodToPayload{http.MethodPut: &AssigneeUpdateRequest{}}),
+			withMethodCheck(http.MethodPut),
+		)
+		data, _ := getFailData(t, svc, request)
 		checkNon200(t, data, "Could not modify merge request assignees", "/mr/assignee")
 	})
 }
diff --git a/cmd/app/attachment.go b/cmd/app/attachment.go
@@ -16,8 +16,8 @@ type FileReader interface {
 }
 
 type AttachmentRequest struct {
-	FilePath string `json:"file_path"`
-	FileName string `json:"file_name"`
+	FilePath string `json:"file_path" validate:"required"`
+	FileName string `json:"file_name" validate:"required"`
 }
 
 type AttachmentResponse struct {
@@ -58,55 +58,31 @@ type attachmentService struct {
 }
 
 /* attachmentHandler uploads an attachment (file, image, etc) to Gitlab and returns metadata about the upload. */
-func (a attachmentService) handler(w http.ResponseWriter, r *http.Request) {
-	w.Header().Set("Content-Type", "application/json")
-	if r.Method != http.MethodPost {
-		w.Header().Set("Access-Control-Allow-Methods", http.MethodPost)
-		handleError(w, InvalidRequestError{}, "Expected POST", http.StatusMethodNotAllowed)
-		return
-	}
-
-	var attachmentRequest AttachmentRequest
-
-	body, err := io.ReadAll(r.Body)
-	if err != nil {
-		handleError(w, err, "Could not read request body", http.StatusBadRequest)
-		return
-	}
-
-	defer r.Body.Close()
-
-	err = json.Unmarshal(body, &attachmentRequest)
-	if err != nil {
-		handleError(w, err, "Could not unmarshal JSON", http.StatusBadRequest)
-		return
-	}
+func (a attachmentService) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	payload := r.Context().Value(payload("payload")).(*AttachmentRequest)
 
-	file, err := a.fileReader.ReadFile(attachmentRequest.FilePath)
+	file, err := a.fileReader.ReadFile(payload.FilePath)
 	if err != nil || file == nil {
-		handleError(w, err, fmt.Sprintf("Could not read %s file", attachmentRequest.FileName), http.StatusInternalServerError)
+		handleError(w, err, fmt.Sprintf("Could not read %s file", payload.FileName), http.StatusInternalServerError)
 		return
 	}
 
-	projectFile, res, err := a.client.UploadFile(a.projectInfo.ProjectId, file, attachmentRequest.FileName)
+	projectFile, res, err := a.client.UploadFile(a.projectInfo.ProjectId, file, payload.FileName)
 	if err != nil {
-		handleError(w, err, fmt.Sprintf("Could not upload %s to Gitlab", attachmentRequest.FileName), http.StatusInternalServerError)
+		handleError(w, err, fmt.Sprintf("Could not upload %s to Gitlab", payload.FileName), http.StatusInternalServerError)
 		return
 	}
 
 	if res.StatusCode >= 300 {
-		handleError(w, GenericError{endpoint: "/attachment"}, fmt.Sprintf("Could not upload %s to Gitlab", attachmentRequest.FileName), res.StatusCode)
+		handleError(w, GenericError{r.URL.Path}, fmt.Sprintf("Could not upload %s to Gitlab", payload.FileName), res.StatusCode)
 		return
 	}
 
 	response := AttachmentResponse{
-		SuccessResponse: SuccessResponse{
-			Status:  http.StatusOK,
-			Message: "File uploaded successfully",
-		},
-		Markdown: projectFile.Markdown,
-		Alt:      projectFile.Alt,
-		Url:      projectFile.URL,
+		SuccessResponse: SuccessResponse{Message: "File uploaded successfully"},
+		Markdown:        projectFile.Markdown,
+		Alt:             projectFile.Alt,
+		Url:             projectFile.URL,
 	}
 
 	err = json.NewEncoder(w).Encode(response)

diff --git a/cmd/app/attachment_test.go b/cmd/app/attachment_test.go
@@ -36,29 +36,34 @@ func TestAttachmentHandler(t *testing.T) {
 
 	t.Run("Returns 200-status response after upload", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/attachment", attachmentTestRequestData)
-		svc := attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{}}
+		svc := middleware(
+			attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{}},
+			withPayloadValidation(methodToPayload{http.MethodPost: &AttachmentRequest{}}),
+			withMethodCheck(http.MethodPost),
+		)
 		data := getSuccessData(t, svc, request)
-		assert(t, data.Status, http.StatusOK)
 		assert(t, data.Message, "File uploaded successfully")
 	})
 
-	t.Run("Disallows non-POST method", func(t *testing.T) {
-		request := makeRequest(t, http.MethodGet, "/attachment", nil)
-		svc := attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{}}
-		data := getFailData(t, svc, request)
-		checkBadMethod(t, data, http.MethodPost)
-	})
 	t.Run("Handles errors from Gitlab client", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/attachment", attachmentTestRequestData)
-		svc := attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{testBase{errFromGitlab: true}}}
-		data := getFailData(t, svc, request)
+		svc := middleware(
+			attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{testBase{errFromGitlab: true}}},
+			withPayloadValidation(methodToPayload{http.MethodPost: &AttachmentRequest{}}),
+			withMethodCheck(http.MethodPost),
+		)
+		data, _ := getFailData(t, svc, request)
 		checkErrorFromGitlab(t, data, "Could not upload some_file_name to Gitlab")
 	})
 
 	t.Run("Handles non-200s from Gitlab client", func(t *testing.T) {
 		request := makeRequest(t, http.MethodPost, "/attachment", attachmentTestRequestData)
-		svc := attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{testBase{status: http.StatusSeeOther}}}
-		data := getFailData(t, svc, request)
+		svc := middleware(
+			attachmentService{testProjectData, fakeFileReader{}, fakeFileUploaderClient{testBase{status: http.StatusSeeOther}}},
+			withPayloadValidation(methodToPayload{http.MethodPost: &AttachmentRequest{}}),
+			withMethodCheck(http.MethodPost),
+		)
+		data, _ := getFailData(t, svc, request)
 		checkNon200(t, data, "Could not upload some_file_name to Gitlab", "/attachment")
 	})
 }