From fa047c5435409613348e5fbf651bba869a3ff50d Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Mon, 16 Oct 2017 08:25:16 -0700 Subject: [PATCH] Refactor EventParser to not take reference to ctx --- server/events/event_parser.go | 27 ++++--- server/events/event_parser_test.go | 20 +++-- server/events/mocks/mock_event_parsing.go | 40 ++++++---- server/events_controller.go | 15 ++-- server/events_controller_test.go | 89 ++++++++++++++++++---- server/gh_request_validation.go | 2 + server/mocks/mock_gh_request_validation.go | 83 ++++++++++++++++++++ 7 files changed, 218 insertions(+), 58 deletions(-) create mode 100644 server/mocks/mock_gh_request_validation.go diff --git a/server/events/event_parser.go b/server/events/event_parser.go index 1982692cd..fe8fe5f03 100644 --- a/server/events/event_parser.go +++ b/server/events/event_parser.go @@ -21,7 +21,7 @@ type Command struct { type EventParsing interface { DetermineCommand(comment *github.IssueCommentEvent) (*Command, error) - ExtractCommentData(comment *github.IssueCommentEvent, ctx *CommandContext) error + ExtractCommentData(comment *github.IssueCommentEvent) (baseRepo models.Repo, user models.User, pull models.PullRequest, err error) ExtractPullData(pull *github.PullRequest) (models.PullRequest, models.Repo, error) ExtractRepoData(ghRepo *github.Repository) (models.Repo, error) } @@ -100,35 +100,38 @@ func (e *EventParser) DetermineCommand(comment *github.IssueCommentEvent) (*Comm return c, nil } -func (e *EventParser) ExtractCommentData(comment *github.IssueCommentEvent, ctx *CommandContext) error { - repo, err := e.ExtractRepoData(comment.Repo) +func (e *EventParser) ExtractCommentData(comment *github.IssueCommentEvent) (baseRepo models.Repo, user models.User, pull models.PullRequest, err error) { + baseRepo, err = e.ExtractRepoData(comment.Repo) if err != nil { - return err + return } pullNum := comment.Issue.GetNumber() if pullNum == 0 { - return errors.New("issue.number is null") + err = errors.New("issue.number is null") + return } pullCreator := comment.Issue.User.GetLogin() if pullCreator == "" { - return errors.New("issue.user.login is null") + err = errors.New("issue.user.login is null") + return } htmlURL := comment.Issue.GetHTMLURL() if htmlURL == "" { - return errors.New("issue.html_url is null") + err = errors.New("issue.html_url is null") + return } commentorUsername := comment.Comment.User.GetLogin() if commentorUsername == "" { - return errors.New("comment.user.login is null") + err = errors.New("comment.user.login is null") + return } - ctx.BaseRepo = repo - ctx.User = models.User{ + user = models.User{ Username: commentorUsername, } - ctx.Pull = models.PullRequest{ + pull = models.PullRequest{ Num: pullNum, } - return nil + return } func (e *EventParser) ExtractPullData(pull *github.PullRequest) (models.PullRequest, models.Repo, error) { diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index a4fd5bff5..65e7aba3a 100644 --- a/server/events/event_parser_test.go +++ b/server/events/event_parser_test.go @@ -158,35 +158,33 @@ func TestExtractCommentData(t *testing.T) { User: &github.User{Login: github.String("comment_user")}, }, } - ctx := events.CommandContext{} - testComment := deepcopy.Copy(comment).(github.IssueCommentEvent) testComment.Repo = nil - err := parser.ExtractCommentData(&testComment, &ctx) + _, _, _, err := parser.ExtractCommentData(&testComment) Equals(t, errors.New("repository.full_name is null"), err) testComment = deepcopy.Copy(comment).(github.IssueCommentEvent) testComment.Issue = nil - err = parser.ExtractCommentData(&testComment, &ctx) + _, _, _, err = parser.ExtractCommentData(&testComment) Equals(t, errors.New("issue.number is null"), err) testComment = deepcopy.Copy(comment).(github.IssueCommentEvent) testComment.Issue.User = nil - err = parser.ExtractCommentData(&testComment, &ctx) + _, _, _, err = parser.ExtractCommentData(&testComment) Equals(t, errors.New("issue.user.login is null"), err) testComment = deepcopy.Copy(comment).(github.IssueCommentEvent) testComment.Issue.HTMLURL = nil - err = parser.ExtractCommentData(&testComment, &ctx) + _, _, _, err = parser.ExtractCommentData(&testComment) Equals(t, errors.New("issue.html_url is null"), err) testComment = deepcopy.Copy(comment).(github.IssueCommentEvent) testComment.Comment.User.Login = nil - err = parser.ExtractCommentData(&testComment, &ctx) + _, _, _, err = parser.ExtractCommentData(&testComment) Equals(t, errors.New("comment.user.login is null"), err) // this should be successful - err = parser.ExtractCommentData(&comment, &ctx) + repo, user, pull, err := parser.ExtractCommentData(&comment) Ok(t, err) Equals(t, models.Repo{ Owner: *comment.Repo.Owner.Login, @@ -194,13 +192,13 @@ func TestExtractCommentData(t *testing.T) { CloneURL: "https://user:token@github.com/lkysow/atlantis-example.git", SanitizedCloneURL: *comment.Repo.CloneURL, Name: "repo", - }, ctx.BaseRepo) + }, repo) Equals(t, models.User{ Username: *comment.Comment.User.Login, - }, ctx.User) + }, user) Equals(t, models.PullRequest{ Num: *comment.Issue.Number, - }, ctx.Pull) + }, pull) } func TestExtractPullData(t *testing.T) { diff --git a/server/events/mocks/mock_event_parsing.go b/server/events/mocks/mock_event_parsing.go index 6d28d0fbd..a794e1c12 100644 --- a/server/events/mocks/mock_event_parsing.go +++ b/server/events/mocks/mock_event_parsing.go @@ -35,16 +35,28 @@ func (mock *MockEventParsing) DetermineCommand(comment *github.IssueCommentEvent return ret0, ret1 } -func (mock *MockEventParsing) ExtractCommentData(comment *github.IssueCommentEvent, ctx *events.CommandContext) error { - params := []pegomock.Param{comment, ctx} - result := pegomock.GetGenericMockFrom(mock).Invoke("ExtractCommentData", params, []reflect.Type{reflect.TypeOf((*error)(nil)).Elem()}) - var ret0 error +func (mock *MockEventParsing) ExtractCommentData(comment *github.IssueCommentEvent) (models.Repo, models.User, models.PullRequest, error) { + params := []pegomock.Param{comment} + result := pegomock.GetGenericMockFrom(mock).Invoke("ExtractCommentData", params, []reflect.Type{reflect.TypeOf((*models.Repo)(nil)).Elem(), reflect.TypeOf((*models.User)(nil)).Elem(), reflect.TypeOf((*models.PullRequest)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + var ret0 models.Repo + var ret1 models.User + var ret2 models.PullRequest + var ret3 error if len(result) != 0 { if result[0] != nil { - ret0 = result[0].(error) + ret0 = result[0].(models.Repo) + } + if result[1] != nil { + ret1 = result[1].(models.User) + } + if result[2] != nil { + ret2 = result[2].(models.PullRequest) + } + if result[3] != nil { + ret3 = result[3].(error) } } - return ret0 + return ret0, ret1, ret2, ret3 } func (mock *MockEventParsing) ExtractPullData(pull *github.PullRequest) (models.PullRequest, models.Repo, error) { @@ -128,8 +140,8 @@ func (c *EventParsing_DetermineCommand_OngoingVerification) GetAllCapturedArgume return } -func (verifier *VerifierEventParsing) ExtractCommentData(comment *github.IssueCommentEvent, ctx *events.CommandContext) *EventParsing_ExtractCommentData_OngoingVerification { - params := []pegomock.Param{comment, ctx} +func (verifier *VerifierEventParsing) ExtractCommentData(comment *github.IssueCommentEvent) *EventParsing_ExtractCommentData_OngoingVerification { + params := []pegomock.Param{comment} methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "ExtractCommentData", params) return &EventParsing_ExtractCommentData_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} } @@ -139,22 +151,18 @@ type EventParsing_ExtractCommentData_OngoingVerification struct { methodInvocations []pegomock.MethodInvocation } -func (c *EventParsing_ExtractCommentData_OngoingVerification) GetCapturedArguments() (*github.IssueCommentEvent, *events.CommandContext) { - comment, ctx := c.GetAllCapturedArguments() - return comment[len(comment)-1], ctx[len(ctx)-1] +func (c *EventParsing_ExtractCommentData_OngoingVerification) GetCapturedArguments() *github.IssueCommentEvent { + comment := c.GetAllCapturedArguments() + return comment[len(comment)-1] } -func (c *EventParsing_ExtractCommentData_OngoingVerification) GetAllCapturedArguments() (_param0 []*github.IssueCommentEvent, _param1 []*events.CommandContext) { +func (c *EventParsing_ExtractCommentData_OngoingVerification) GetAllCapturedArguments() (_param0 []*github.IssueCommentEvent) { params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) if len(params) > 0 { _param0 = make([]*github.IssueCommentEvent, len(params[0])) for u, param := range params[0] { _param0[u] = param.(*github.IssueCommentEvent) } - _param1 = make([]*events.CommandContext, len(params[1])) - for u, param := range params[1] { - _param1[u] = param.(*events.CommandContext) - } } return } diff --git a/server/events_controller.go b/server/events_controller.go index 567108653..44a275092 100644 --- a/server/events_controller.go +++ b/server/events_controller.go @@ -44,7 +44,16 @@ func (e *EventsController) HandleCommentEvent(w http.ResponseWriter, event *gith return } + baseRepo, user, pull, err := e.Parser.ExtractCommentData(event) + if err != nil { + e.respond(w, logging.Error, http.StatusBadRequest, "Failed parsing event: %v %s", err, githubReqID) + return + } ctx := &events.CommandContext{} + ctx.BaseRepo = baseRepo + ctx.User = user + ctx.Pull = pull + command, err := e.Parser.DetermineCommand(event) if err != nil { e.respond(w, logging.Debug, http.StatusOK, "Ignoring: %s %s", err, githubReqID) @@ -52,10 +61,6 @@ func (e *EventsController) HandleCommentEvent(w http.ResponseWriter, event *gith } ctx.Command = command - if err = e.Parser.ExtractCommentData(event, ctx); err != nil { - e.respond(w, logging.Error, http.StatusInternalServerError, "Failed parsing event: %v %s", err, githubReqID) - return - } // Respond with success and then actually execute the command asynchronously. // We use a goroutine so that this function returns and the connection is // closed. @@ -81,7 +86,7 @@ func (e *EventsController) HandlePullRequestEvent(w http.ResponseWriter, pullEve } if err := e.PullClosedExecutor.CleanUpPull(repo, pull); err != nil { - e.respond(w, logging.Error, http.StatusInternalServerError, "Error cleaning pull request: %s", err) + e.respond(w, logging.Error, http.StatusServiceUnavailable, "Error cleaning pull request: %s", err) return } e.Logger.Info("deleted locks and workspace for repo %s, pull %d", repo.FullName, pull.Num) diff --git a/server/events_controller_test.go b/server/events_controller_test.go index 387ab74d6..c998e2a3b 100644 --- a/server/events_controller_test.go +++ b/server/events_controller_test.go @@ -2,21 +2,82 @@ package server_test import ( "testing" - //. "github.com/hootsuite/atlantis/testing_util" - //. "github.com/petergtz/pegomock" - //"github.com/hootsuite/atlantis/server" - //"github.com/hootsuite/atlantis/server/logging" - //"net/http" - //"bytes" + . "github.com/hootsuite/atlantis/testing_util" + . "github.com/petergtz/pegomock" + "github.com/hootsuite/atlantis/server" + "github.com/hootsuite/atlantis/server/logging" + "net/http" + "bytes" + "github.com/hootsuite/atlantis/server/mocks" + "net/http/httptest" + "errors" ) func TestPost_InvalidSecret(t *testing.T) { - //t.Log("when the payload can't be validated against the github secret there is an error") - //RegisterMockTestingT(t) - //e := server.EventsController{ - // Logger: logging.NewNoopLogger(), - // GithubWebHookSecret: []byte("secret"), - //} - //req, err := http.NewRequest("GET", "http://localhost/event", bytes.NewBuffer(nil)) - //e.Post() + t.Log("when the payload can't be validated a 400 is returned") + RegisterMockTestingT(t) + v := mocks.NewMockGHRequestValidator() + secret := []byte("secret") + e := server.EventsController{ + Logger: logging.NewNoopLogger(), + GithubWebHookSecret: secret, + Validator: v, + } + req, err := http.NewRequest("GET", "http://localhost/event", bytes.NewBuffer(nil)) + Ok(t, err) + w := httptest.NewRecorder() + When(v.Validate(req, secret)).ThenReturn(nil, errors.New("err")) + e.Post(w, req) + + Equals(t, http.StatusBadRequest, w.Result().StatusCode) +} + +func TestPost_UnsupportedEvent(t *testing.T) { + t.Log("when the event type is unsupported we ignore it") + RegisterMockTestingT(t) +} + +func TestPost_CommentNotCreated(t *testing.T) { + t.Log("when the event is a comment but it's not a created event we ignore it") + RegisterMockTestingT(t) +} + +func TestPost_CommentInvalidComment(t *testing.T) { + t.Log("when the event is a comment without all expected data we return a 400") + RegisterMockTestingT(t) +} + +func TestPost_CommentInvalidCommand(t *testing.T) { + t.Log("when the event is a comment with an invalid command we ignore it") + RegisterMockTestingT(t) +} + +func TestPost_CommentSuccess(t *testing.T) { + t.Log("when the event is comment with a valid command we call the command handler") + RegisterMockTestingT(t) +} + +func TestPost_PullRequestNotClosed(t *testing.T) { + t.Log("when the event is pull reuqest but it's not a closed event we ignore it") + RegisterMockTestingT(t) +} + +func TestPost_PullRequestInvalid(t *testing.T) { + t.Log("when the event is pull reuqest with invalid data we return a 400") + RegisterMockTestingT(t) +} + +func TestPost_PullRequestInvalidRepo(t *testing.T) { + t.Log("when the event is pull reuqest with invalid repo data we return a 400") + RegisterMockTestingT(t) +} + +func TestPost_PullRequestErrCleaningPull(t *testing.T) { + t.Log("when the event is a pull request and we have an error calling CleanUpPull we return a 503") + RegisterMockTestingT(t) +} + +func TestPost_PullRequestSuccess(t *testing.T) { + t.Log("when the event is a pull request and everything works we return a 200") + RegisterMockTestingT(t) } diff --git a/server/gh_request_validation.go b/server/gh_request_validation.go index 056e82ce9..2376ad857 100644 --- a/server/gh_request_validation.go +++ b/server/gh_request_validation.go @@ -9,6 +9,8 @@ import ( "github.com/google/go-github/github" ) +//go:generate pegomock generate --use-experimental-model-gen --package mocks -o mocks/mock_gh_request_validation.go GHRequestValidator + // GHRequestValidator validates GitHub requests. type GHRequestValidator interface { // Validate returns the JSON payload of the request. diff --git a/server/mocks/mock_gh_request_validation.go b/server/mocks/mock_gh_request_validation.go new file mode 100644 index 000000000..76ba0ffe3 --- /dev/null +++ b/server/mocks/mock_gh_request_validation.go @@ -0,0 +1,83 @@ +// Automatically generated by pegomock. DO NOT EDIT! +// Source: github.com/hootsuite/atlantis/server (interfaces: GHRequestValidator) + +package mocks + +import ( + pegomock "github.com/petergtz/pegomock" + http "net/http" + "reflect" +) + +type MockGHRequestValidator struct { + fail func(message string, callerSkip ...int) +} + +func NewMockGHRequestValidator() *MockGHRequestValidator { + return &MockGHRequestValidator{fail: pegomock.GlobalFailHandler} +} + +func (mock *MockGHRequestValidator) Validate(r *http.Request, secret []byte) ([]byte, error) { + params := []pegomock.Param{r, secret} + result := pegomock.GetGenericMockFrom(mock).Invoke("Validate", params, []reflect.Type{reflect.TypeOf((*[]byte)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + var ret0 []byte + var ret1 error + if len(result) != 0 { + if result[0] != nil { + ret0 = result[0].([]byte) + } + if result[1] != nil { + ret1 = result[1].(error) + } + } + return ret0, ret1 +} + +func (mock *MockGHRequestValidator) VerifyWasCalledOnce() *VerifierGHRequestValidator { + return &VerifierGHRequestValidator{mock, pegomock.Times(1), nil} +} + +func (mock *MockGHRequestValidator) VerifyWasCalled(invocationCountMatcher pegomock.Matcher) *VerifierGHRequestValidator { + return &VerifierGHRequestValidator{mock, invocationCountMatcher, nil} +} + +func (mock *MockGHRequestValidator) VerifyWasCalledInOrder(invocationCountMatcher pegomock.Matcher, inOrderContext *pegomock.InOrderContext) *VerifierGHRequestValidator { + return &VerifierGHRequestValidator{mock, invocationCountMatcher, inOrderContext} +} + +type VerifierGHRequestValidator struct { + mock *MockGHRequestValidator + invocationCountMatcher pegomock.Matcher + inOrderContext *pegomock.InOrderContext +} + +func (verifier *VerifierGHRequestValidator) Validate(r *http.Request, secret []byte) *GHRequestValidator_Validate_OngoingVerification { + params := []pegomock.Param{r, secret} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "Validate", params) + return &GHRequestValidator_Validate_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} +} + +type GHRequestValidator_Validate_OngoingVerification struct { + mock *MockGHRequestValidator + methodInvocations []pegomock.MethodInvocation +} + +func (c *GHRequestValidator_Validate_OngoingVerification) GetCapturedArguments() (*http.Request, []byte) { + r, secret := c.GetAllCapturedArguments() + return r[len(r)-1], secret[len(secret)-1] +} + +func (c *GHRequestValidator_Validate_OngoingVerification) GetAllCapturedArguments() (_param0 []*http.Request, _param1 [][]byte) { + params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) + if len(params) > 0 { + _param0 = make([]*http.Request, len(params[0])) + for u, param := range params[0] { + _param0[u] = param.(*http.Request) + } + _param1 = make([][]byte, len(params[1])) + for u, param := range params[1] { + _param1[u] = param.([]byte) + } + } + return +}