From 525fa03065fcc83297d0cb5db0717b182ccb2e76 Mon Sep 17 00:00:00 2001 From: justusbunsi Date: Mon, 11 Jul 2022 15:24:43 +0200 Subject: [PATCH] Centralize API response handling Currently, all API handler functions take care of response code and message on their own. This leads to a huge injection chain for HTTP related objects. This refactors the API to consistently return response code and message to the API main entrypoint where the response is created and sent. Now, SonarQube and Gitea will get a response at the very end of any bot action for one request. SonarQube has a timeout of 10 seconds, which may be reached due to network latency. We'll see. Signed-off-by: Steven Kriegler --- internal/api/gitea.go | 60 +++++++++++----------------------- internal/api/gitea_test.go | 16 +++++++-- internal/api/main.go | 21 ++++++++---- internal/api/main_test.go | 31 +++++++++++------- internal/api/sonarqube.go | 34 +++++-------------- internal/api/sonarqube_test.go | 9 ++++- 6 files changed, 85 insertions(+), 86 deletions(-) diff --git a/internal/api/gitea.go b/internal/api/gitea.go index bbb65cd..d9b6d66 100644 --- a/internal/api/gitea.go +++ b/internal/api/gitea.go @@ -1,8 +1,6 @@ package api import ( - "fmt" - "io" "io/ioutil" "log" "net/http" @@ -14,8 +12,8 @@ import ( ) type GiteaWebhookHandlerInferface interface { - HandleSynchronize(rw http.ResponseWriter, r *http.Request) - HandleComment(rw http.ResponseWriter, r *http.Request) + HandleSynchronize(r *http.Request) (int, string) + HandleComment(r *http.Request) (int, string) } type GiteaWebhookHandler struct { @@ -23,7 +21,7 @@ type GiteaWebhookHandler struct { sqSdk sqSdk.SonarQubeSdkInterface } -func (h *GiteaWebhookHandler) parseBody(rw http.ResponseWriter, r *http.Request) ([]byte, error) { +func (h *GiteaWebhookHandler) parseBody(r *http.Request) ([]byte, error) { if r.Body != nil { defer r.Body.Close() } @@ -32,82 +30,62 @@ func (h *GiteaWebhookHandler) parseBody(rw http.ResponseWriter, r *http.Request) if err != nil { log.Printf("Error reading request body %s", err.Error()) - rw.WriteHeader(http.StatusInternalServerError) - io.WriteString(rw, fmt.Sprintf(`{"message": "%s"}`, err.Error())) return nil, err } return raw, nil } -func (h *GiteaWebhookHandler) HandleSynchronize(rw http.ResponseWriter, r *http.Request) { - rw.Header().Set("Content-Type", "application/json") - - raw, err := h.parseBody(rw, r) +func (h *GiteaWebhookHandler) HandleSynchronize(r *http.Request) (int, string) { + raw, err := h.parseBody(r) if err != nil { - return + return http.StatusInternalServerError, err.Error() } ok, err := isValidWebhook(raw, settings.Gitea.Webhook.Secret, r.Header.Get("X-Gitea-Signature"), "Gitea") if !ok { log.Print(err.Error()) - rw.WriteHeader(http.StatusPreconditionFailed) - io.WriteString(rw, fmt.Sprint(`{"message": "Webhook validation failed. Request rejected."}`)) - return + return http.StatusPreconditionFailed, "Webhook validation failed. Request rejected." } w, ok := webhook.NewPullWebhook(raw) if !ok { - rw.WriteHeader(http.StatusUnprocessableEntity) - io.WriteString(rw, `{"message": "Error parsing POST body."}`) - return + return http.StatusUnprocessableEntity, "Error parsing POST body." } if err := w.Validate(); err != nil { - rw.WriteHeader(http.StatusOK) - io.WriteString(rw, fmt.Sprintf(`{"message": "%s"}`, err.Error())) - return + return http.StatusOK, err.Error() } - rw.WriteHeader(http.StatusOK) - io.WriteString(rw, `{"message": "Processing data. See bot logs for details."}`) - w.ProcessData(h.giteaSdk, h.sqSdk) + + return http.StatusOK, "Processing data. See bot logs for details." } -func (h *GiteaWebhookHandler) HandleComment(rw http.ResponseWriter, r *http.Request) { - rw.Header().Set("Content-Type", "application/json") - - raw, err := h.parseBody(rw, r) +func (h *GiteaWebhookHandler) HandleComment(r *http.Request) (int, string) { + raw, err := h.parseBody(r) if err != nil { - return + return http.StatusInternalServerError, err.Error() } ok, err := isValidWebhook(raw, settings.Gitea.Webhook.Secret, r.Header.Get("X-Gitea-Signature"), "Gitea") if !ok { log.Print(err.Error()) - rw.WriteHeader(http.StatusPreconditionFailed) - io.WriteString(rw, `{"message": "Webhook validation failed. Request rejected."}`) - return + return http.StatusPreconditionFailed, "Webhook validation failed. Request rejected." } w, ok := webhook.NewCommentWebhook(raw) if !ok { - rw.WriteHeader(http.StatusUnprocessableEntity) - io.WriteString(rw, `{"message": "Error parsing POST body."}`) - return + return http.StatusUnprocessableEntity, "Error parsing POST body." } if err := w.Validate(); err != nil { - rw.WriteHeader(http.StatusOK) - io.WriteString(rw, fmt.Sprintf(`{"message": "%s"}`, err.Error())) - return + return http.StatusOK, err.Error() } - rw.WriteHeader(http.StatusOK) - io.WriteString(rw, `{"message": "Processing data. See bot logs for details."}`) - w.ProcessData(h.giteaSdk, h.sqSdk) + + return http.StatusOK, "Processing data. See bot logs for details." } func NewGiteaWebhookHandler(g giteaSdk.GiteaSdkInterface, sq sqSdk.SonarQubeSdkInterface) GiteaWebhookHandlerInferface { diff --git a/internal/api/gitea_test.go b/internal/api/gitea_test.go index 74a407b..5eade86 100644 --- a/internal/api/gitea_test.go +++ b/internal/api/gitea_test.go @@ -2,6 +2,8 @@ package api import ( "bytes" + "fmt" + "io" "net/http" "net/http/httptest" "testing" @@ -20,7 +22,12 @@ func withValidGiteaCommentRequestData(t *testing.T, jsonBody []byte) (*http.Requ } rr := httptest.NewRecorder() - handler := http.HandlerFunc(webhookHandler.HandleComment) + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + status, response := webhookHandler.HandleComment(r) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + io.WriteString(w, fmt.Sprintf(`{"message": "%s"}`, response)) + }) return req, rr, handler } @@ -34,7 +41,12 @@ func withValidGiteaSynchronizeRequestData(t *testing.T, jsonBody []byte) (*http. } rr := httptest.NewRecorder() - handler := http.HandlerFunc(webhookHandler.HandleSynchronize) + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + status, response := webhookHandler.HandleSynchronize(r) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + io.WriteString(w, fmt.Sprintf(`{"message": "%s"}`, response)) + }) return req, rr, handler } diff --git a/internal/api/main.go b/internal/api/main.go index 7ae869f..1ff8c75 100644 --- a/internal/api/main.go +++ b/internal/api/main.go @@ -40,7 +40,10 @@ func (s *ApiServer) setup() { return } - s.sonarQubeWebhookHandler.Handle(c.Writer, c.Request) + status, response := s.sonarQubeWebhookHandler.Handle(c.Request) + c.JSON(status, gin.H{ + "message": response, + }) }).POST("/hooks/gitea", func(c *gin.Context) { h := validGiteaEndpointHeader{} @@ -49,16 +52,22 @@ func (s *ApiServer) setup() { return } + var status int + var response string + switch h.GiteaEvent { case "pull_request": - s.giteaWebhookHandler.HandleSynchronize(c.Writer, c.Request) + status, response = s.giteaWebhookHandler.HandleSynchronize(c.Request) case "issue_comment": - s.giteaWebhookHandler.HandleComment(c.Writer, c.Request) + status, response = s.giteaWebhookHandler.HandleComment(c.Request) default: - c.JSON(http.StatusOK, gin.H{ - "message": "ignore unknown event", - }) + status = http.StatusOK + response = "ignore unknown event" } + + c.JSON(status, gin.H{ + "message": response, + }) }) } diff --git a/internal/api/main_test.go b/internal/api/main_test.go index 046972a..9fad173 100644 --- a/internal/api/main_test.go +++ b/internal/api/main_test.go @@ -22,20 +22,23 @@ type SonarQubeHandlerMock struct { mock.Mock } -func (h *SonarQubeHandlerMock) Handle(rw http.ResponseWriter, r *http.Request) { - h.Called(rw, r) +func (h *SonarQubeHandlerMock) Handle(r *http.Request) (int, string) { + h.Called(r) + return http.StatusOK, "test-execution" } type GiteaHandlerMock struct { mock.Mock } -func (h *GiteaHandlerMock) HandleSynchronize(rw http.ResponseWriter, r *http.Request) { - h.Called(rw, r) +func (h *GiteaHandlerMock) HandleSynchronize(r *http.Request) (int, string) { + h.Called(r) + return http.StatusOK, "test-execution" } -func (h *GiteaHandlerMock) HandleComment(rw http.ResponseWriter, r *http.Request) { - h.Called(rw, r) +func (h *GiteaHandlerMock) HandleComment(r *http.Request) (int, string) { + h.Called(r) + return http.StatusOK, "test-execution" } type GiteaSdkMock struct { @@ -83,6 +86,8 @@ func (h *SQSdkMock) ComposeGiteaComment(data *sqSdk.CommentComposeData) (string, // SETUP: mute logs func TestMain(m *testing.M) { gin.SetMode(gin.TestMode) + gin.DefaultWriter = ioutil.Discard + gin.DefaultErrorWriter = ioutil.Discard log.SetOutput(ioutil.Discard) os.Exit(m.Run()) } @@ -113,7 +118,7 @@ func TestSonarQubeAPIRouteMissingProjectHeader(t *testing.T) { func TestSonarQubeAPIRouteProcessing(t *testing.T) { sonarQubeHandlerMock := new(SonarQubeHandlerMock) - sonarQubeHandlerMock.On("Handle", mock.Anything, mock.Anything).Return(nil) + sonarQubeHandlerMock.On("Handle", mock.IsType(&http.Request{})) router := New(new(GiteaHandlerMock), sonarQubeHandlerMock) @@ -124,6 +129,7 @@ func TestSonarQubeAPIRouteProcessing(t *testing.T) { assert.Equal(t, http.StatusOK, w.Code) sonarQubeHandlerMock.AssertNumberOfCalls(t, "Handle", 1) + sonarQubeHandlerMock.AssertExpectations(t) } func TestGiteaAPIRouteMissingEventHeader(t *testing.T) { @@ -139,7 +145,7 @@ func TestGiteaAPIRouteMissingEventHeader(t *testing.T) { func TestGiteaAPIRouteSynchronizeProcessing(t *testing.T) { giteaHandlerMock := new(GiteaHandlerMock) giteaHandlerMock.On("HandleSynchronize", mock.Anything, mock.Anything).Return(nil) - giteaHandlerMock.On("HandleComment", mock.Anything, mock.Anything).Return(nil) + giteaHandlerMock.On("HandleComment", mock.Anything, mock.Anything).Maybe() router := New(giteaHandlerMock, new(SonarQubeHandlerMock)) @@ -151,11 +157,12 @@ func TestGiteaAPIRouteSynchronizeProcessing(t *testing.T) { assert.Equal(t, http.StatusOK, w.Code) giteaHandlerMock.AssertNumberOfCalls(t, "HandleSynchronize", 1) giteaHandlerMock.AssertNumberOfCalls(t, "HandleComment", 0) + giteaHandlerMock.AssertExpectations(t) } func TestGiteaAPIRouteCommentProcessing(t *testing.T) { giteaHandlerMock := new(GiteaHandlerMock) - giteaHandlerMock.On("HandleSynchronize", mock.Anything, mock.Anything).Return(nil) + giteaHandlerMock.On("HandleSynchronize", mock.Anything, mock.Anything).Maybe() giteaHandlerMock.On("HandleComment", mock.Anything, mock.Anything).Return(nil) router := New(giteaHandlerMock, new(SonarQubeHandlerMock)) @@ -168,12 +175,13 @@ func TestGiteaAPIRouteCommentProcessing(t *testing.T) { assert.Equal(t, http.StatusOK, w.Code) giteaHandlerMock.AssertNumberOfCalls(t, "HandleSynchronize", 0) giteaHandlerMock.AssertNumberOfCalls(t, "HandleComment", 1) + giteaHandlerMock.AssertExpectations(t) } func TestGiteaAPIRouteUnknownEvent(t *testing.T) { giteaHandlerMock := new(GiteaHandlerMock) - giteaHandlerMock.On("HandleSynchronize", mock.Anything, mock.Anything).Return(nil) - giteaHandlerMock.On("HandleComment", mock.Anything, mock.Anything).Return(nil) + giteaHandlerMock.On("HandleSynchronize", mock.Anything, mock.Anything).Maybe() + giteaHandlerMock.On("HandleComment", mock.Anything, mock.Anything).Maybe() router := New(giteaHandlerMock, new(SonarQubeHandlerMock)) @@ -185,4 +193,5 @@ func TestGiteaAPIRouteUnknownEvent(t *testing.T) { assert.Equal(t, http.StatusOK, w.Code) giteaHandlerMock.AssertNumberOfCalls(t, "HandleSynchronize", 0) giteaHandlerMock.AssertNumberOfCalls(t, "HandleComment", 0) + giteaHandlerMock.AssertExpectations(t) } diff --git a/internal/api/sonarqube.go b/internal/api/sonarqube.go index 9ed8b92..3b5bbad 100644 --- a/internal/api/sonarqube.go +++ b/internal/api/sonarqube.go @@ -2,7 +2,6 @@ package api import ( "fmt" - "io" "io/ioutil" "log" "net/http" @@ -15,7 +14,7 @@ import ( ) type SonarQubeWebhookHandlerInferface interface { - Handle(rw http.ResponseWriter, r *http.Request) + Handle(r *http.Request) (int, string) } type SonarQubeWebhookHandler struct { @@ -56,17 +55,12 @@ func (h *SonarQubeWebhookHandler) processData(w *webhook.Webhook, repo settings. h.giteaSdk.PostComment(repo, w.PRIndex, comment) } -func (h *SonarQubeWebhookHandler) Handle(rw http.ResponseWriter, r *http.Request) { - rw.Header().Set("Content-Type", "application/json") - +func (h *SonarQubeWebhookHandler) Handle(r *http.Request) (int, string) { projectName := r.Header.Get("X-SonarQube-Project") found, pIdx := h.inProjectsMapping(settings.Projects, projectName) if !found { log.Printf("Received hook for project '%s' which is not configured. Request ignored.", projectName) - - rw.WriteHeader(http.StatusOK) - io.WriteString(rw, fmt.Sprintf(`{"message": "Project '%s' not in configured list. Request ignored."}`, projectName)) - return + return http.StatusOK, fmt.Sprintf("Project '%s' not in configured list. Request ignored.", projectName) } log.Printf("Received hook for project '%s'. Processing data.", projectName) @@ -78,38 +72,28 @@ func (h *SonarQubeWebhookHandler) Handle(rw http.ResponseWriter, r *http.Request raw, err := ioutil.ReadAll(r.Body) if err != nil { log.Printf("Error reading request body %s", err.Error()) - rw.WriteHeader(http.StatusInternalServerError) - io.WriteString(rw, fmt.Sprintf(`{"message": "%s"}`, err.Error())) - return + return http.StatusInternalServerError, err.Error() } ok, err := isValidWebhook(raw, settings.SonarQube.Webhook.Secret, r.Header.Get("X-Sonar-Webhook-HMAC-SHA256"), "SonarQube") if !ok { log.Print(err.Error()) - rw.WriteHeader(http.StatusPreconditionFailed) - io.WriteString(rw, `{"message": "Webhook validation failed. Request rejected."}`) - return + return http.StatusPreconditionFailed, "Webhook validation failed. Request rejected." } w, ok := webhook.New(raw) if !ok { - rw.WriteHeader(http.StatusUnprocessableEntity) - io.WriteString(rw, `{"message": "Error parsing POST body."}`) - return + return http.StatusUnprocessableEntity, "Error parsing POST body." } - // Send response to SonarQube at this point to ensure being within 10 seconds limit of webhook response timeout - rw.WriteHeader(http.StatusOK) - if strings.ToLower(w.Branch.Type) != "pull_request" { - io.WriteString(rw, `{"message": "Ignore Hook for non-PR analysis."}`) log.Println("Ignore Hook for non-PR analysis") - return + return http.StatusOK, "Ignore Hook for non-PR analysis." } - io.WriteString(rw, `{"message": "Processing data. See bot logs for details."}`) - h.processData(w, settings.Projects[pIdx].Gitea) + + return http.StatusOK, "Processing data. See bot logs for details." } func NewSonarQubeWebhookHandler(g giteaSdk.GiteaSdkInterface, sq sqSdk.SonarQubeSdkInterface) SonarQubeWebhookHandlerInferface { diff --git a/internal/api/sonarqube_test.go b/internal/api/sonarqube_test.go index d4efd3b..fbd7cee 100644 --- a/internal/api/sonarqube_test.go +++ b/internal/api/sonarqube_test.go @@ -2,6 +2,8 @@ package api import ( "bytes" + "fmt" + "io" "net/http" "net/http/httptest" "regexp" @@ -22,7 +24,12 @@ func withValidSonarQubeRequestData(t *testing.T, jsonBody []byte) (*http.Request req.Header.Set("X-SonarQube-Project", "pr-bot") rr := httptest.NewRecorder() - handler := http.HandlerFunc(webhookHandler.Handle) + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + status, response := webhookHandler.Handle(r) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + io.WriteString(w, fmt.Sprintf(`{"message": "%s"}`, response)) + }) return req, rr, handler }