Classify errors; userError vs servicefault

- Branch protection not enabled = userError - No required status checks not enabled = userError - Cannot acquire lock to add review = TooManyRequestError Refactored errors into a separate package to avoid import cycle
marqeta · Jul 10, 2024 · 358f088 · 358f088
1 parent 7b3889e
commit 358f088
Show file tree

Hide file tree

Showing 17 changed files with 144 additions and 69 deletions.
diff --git a/errors.go → errors/errors.go b/errors.go → errors/errors.go
@@ -1,4 +1,4 @@
-package prbot
+package errors
 
 import (
 	"context"
@@ -28,6 +28,14 @@ func (e APIError) Render(_ http.ResponseWriter, r *http.Request) error {
 	return nil
 }
 
+func (e APIError) IsUserError() bool {
+	return e.StatusCode >= 400 && e.StatusCode < 500
+}
+
+func (e APIError) IsServiceError() bool {
+	return !e.IsUserError()
+}
+
 func RenderError(w http.ResponseWriter, r *http.Request, err error) {
 	var ae APIError
 	if errors.As(err, &ae) {

diff --git a/errors_test.go → errors/errors_test.go b/errors_test.go → errors/errors_test.go
@@ -1,4 +1,4 @@
-package prbot_test
+package errors_test
 
 import (
 	"bytes"
@@ -11,15 +11,17 @@ import (
 
 	"github.com/go-chi/chi/v5/middleware"
 	"github.com/google/go-github/v50/github"
-	prbot "github.com/marqeta/pr-bot"
+	pe "github.com/marqeta/pr-bot/errors"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestRenderError(t *testing.T) {
+	ctx := context.TODO()
+	ctx = context.WithValue(ctx, middleware.RequestIDKey, "requestID")
 
 	//nolint:goerr113
 	errRandom := errors.New("random")
-	apiError := prbot.APIError{
+	apiError := pe.APIError{
 		RequestID:  "requestID",
 		ErrorCode:  "code",
 		Message:    "msg",
@@ -29,23 +31,67 @@ func TestRenderError(t *testing.T) {
 	tests := []struct {
 		name    string
 		err     error
-		wantErr prbot.APIError
+		wantErr pe.APIError
 	}{
 		{
 			name: "Should render APIError",
 			err:  apiError,
-			wantErr: prbot.APIError{
+			wantErr: pe.APIError{
 				RequestID:  "requestID",
 				ErrorCode:  "code",
 				Message:    "msg",
 				StatusCode: http.StatusBadRequest,
 				ErrorText:  errRandom.Error(),
 			},
 		},
+		{
+			name: "Should render TooManyRequestError",
+			err:  pe.TooManyRequestError(ctx, "random", errRandom),
+			wantErr: pe.APIError{
+				RequestID:  "requestID",
+				ErrorCode:  "TooManyRequestsError",
+				Message:    "random",
+				StatusCode: http.StatusTooManyRequests,
+				ErrorText:  errRandom.Error(),
+			},
+		},
+		{
+			name: "Should render InValidRequestError",
+			err:  pe.InValidRequestError(ctx, "random", errRandom),
+			wantErr: pe.APIError{
+				RequestID:  "requestID",
+				ErrorCode:  "InValidRequestError",
+				Message:    "random",
+				StatusCode: http.StatusBadRequest,
+				ErrorText:  errRandom.Error(),
+			},
+		},
+		{
+			name: "Should render ServiceFault",
+			err:  pe.ServiceFault(ctx, "random", errRandom),
+			wantErr: pe.APIError{
+				RequestID:  "requestID",
+				ErrorCode:  "ServiceFault",
+				Message:    "random",
+				StatusCode: http.StatusInternalServerError,
+				ErrorText:  errRandom.Error(),
+			},
+		},
+		{
+			name: "Should render UserError",
+			err:  pe.UserError(ctx, "random", errRandom),
+			wantErr: pe.APIError{
+				RequestID:  "requestID",
+				ErrorCode:  "UserError",
+				Message:    "random",
+				StatusCode: http.StatusBadRequest,
+				ErrorText:  errRandom.Error(),
+			},
+		},
 		{
 			name: "Should render non APIError",
 			err:  errRandom,
-			wantErr: prbot.APIError{
+			wantErr: pe.APIError{
 				RequestID:  "requestID",
 				ErrorCode:  "Unknown",
 				StatusCode: http.StatusInternalServerError,
@@ -57,9 +103,9 @@ func TestRenderError(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			req, _ := NewRequest(t, tt.wantErr.RequestID)
 			rr := httptest.NewRecorder()
-			prbot.RenderError(rr, req, tt.err)
+			pe.RenderError(rr, req, tt.err)
 			assert.Equal(t, tt.wantErr.StatusCode, rr.Code)
-			var res prbot.APIError
+			var res pe.APIError
 			err := json.Unmarshal(rr.Body.Bytes(), &res)
 			assert.Nil(t, err, "error when unmarshalling response")
 			assert.Equal(t, tt.wantErr, res)

diff --git a/github/api.go b/github/api.go
@@ -4,7 +4,7 @@ import (
 	"context"
 
 	"github.com/google/go-github/v50/github"
-	prbot "github.com/marqeta/pr-bot"
+	pe "github.com/marqeta/pr-bot/errors"
 	"github.com/marqeta/pr-bot/id"
 	"github.com/shurcooL/githubv4"
 )
@@ -47,7 +47,7 @@ type API interface {
 	AddReview(ctx context.Context, id id.PR, summary, event string) error
 	EnableAutoMerge(ctx context.Context, id id.PR, method githubv4.PullRequestMergeMethod) error
 	IssueComment(ctx context.Context, id id.PR, comment string) error
-	IssueCommentForError(ctx context.Context, id id.PR, err prbot.APIError) error
+	IssueCommentForError(ctx context.Context, id id.PR, err pe.APIError) error
 	ListAllTopics(ctx context.Context, id id.PR) ([]string, error)
 	ListRequiredStatusChecks(ctx context.Context, id id.PR, branch string) ([]string, error)
 	ListFilesInRootDir(ctx context.Context, id id.PR, branch string) ([]string, error)

diff --git a/github/github.go b/github/github.go
@@ -8,7 +8,7 @@ import (
 
 	"github.com/go-chi/chi/v5/middleware"
 	"github.com/google/go-github/v50/github"
-	prbot "github.com/marqeta/pr-bot"
+	pe "github.com/marqeta/pr-bot/errors"
 	"github.com/marqeta/pr-bot/id"
 	"github.com/marqeta/pr-bot/metrics"
 	"github.com/shurcooL/githubv4"
@@ -59,7 +59,7 @@ func (gh *githubDao) ListReviews(ctx context.Context, id id.PR) ([]*github.PullR
 func (gh *githubDao) GetBranchProtection(ctx context.Context, id id.PR, branch string) (*github.Protection, error) {
 	b, resp, err := gh.v3.Repositories.GetBranchProtection(ctx, id.Owner, id.Repo, branch)
 	if err != nil {
-		return nil, err
+		return nil, classifyError(ctx, resp, fmt.Sprintf("error getting branch protection for PR %v", id.URL), err)
 	}
 	gh.emitTokenExpiration(ctx, resp)
 	return b, nil
@@ -84,7 +84,7 @@ func (gh *githubDao) ListFilesInRootDir(ctx context.Context, id id.PR, branch st
 		Ref: branch,
 	})
 	if err != nil {
-		return nil, err
+		return nil, classifyError(ctx, resp, fmt.Sprintf("error listing files on repo for PR %v", id.URL), err)
 	}
 	gh.emitTokenExpiration(ctx, resp)
 	filenames := make([]string, len(files))
@@ -101,7 +101,8 @@ func (gh *githubDao) ListFilesInRootDir(ctx context.Context, id id.PR, branch st
 func (gh *githubDao) ListRequiredStatusChecks(ctx context.Context, id id.PR, branch string) ([]string, error) {
 	checks, resp, err := gh.v3.Repositories.ListRequiredStatusChecksContexts(ctx, id.Owner, id.Repo, branch)
 	if err != nil {
-		return nil, err
+		return nil, classifyError(ctx, resp,
+			fmt.Sprintf("error listing required status checks on repo for PR %v", id.URL), err)
 	}
 	gh.emitTokenExpiration(ctx, resp)
 	return checks, nil
@@ -173,7 +174,7 @@ func (gh *githubDao) AddReview(ctx context.Context, id id.PR, summary, event str
 }
 
 // IssueCommentForError implements Dao
-func (gh *githubDao) IssueCommentForError(ctx context.Context, id id.PR, apiError prbot.APIError) error {
+func (gh *githubDao) IssueCommentForError(ctx context.Context, id id.PR, apiError pe.APIError) error {
 	b, err := json.MarshalIndent(apiError, "", "  ")
 	if err != nil {
 		return err
@@ -200,3 +201,20 @@ func (gh *githubDao) UI(id id.PR) string {
 	}
 	return fmt.Sprintf("https://%s/ui/eval/%s/pull/%d", gh.serverHost, id.RepoFullName, id.Number)
 }
+
+func classifyError(ctx context.Context, resp *github.Response, msg string, err error) error {
+	if resp == nil {
+		return err
+	}
+	if isClientError(resp) {
+		return pe.UserError(ctx, msg, err)
+	}
+	return pe.ServiceFault(ctx, msg, err)
+}
+
+func isClientError(resp *github.Response) bool {
+	if resp == nil {
+		return false
+	}
+	return resp.StatusCode >= 400 && resp.StatusCode < 500
+}
diff --git a/github/mock_api.go b/github/mock_api.go
diff --git a/pullrequest/dispatcher.go b/pullrequest/dispatcher.go
@@ -9,7 +9,7 @@ import (
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/go-chi/httplog"
 	"github.com/google/go-github/v50/github"
-	prbot "github.com/marqeta/pr-bot"
+	pe "github.com/marqeta/pr-bot/errors"
 	"github.com/marqeta/pr-bot/id"
 	"github.com/marqeta/pr-bot/metrics"
 )
@@ -126,5 +126,5 @@ func (d *dispatcher) Dispatch(ctx context.Context, _ string, eventName string, e
 }
 
 func parseError(ctx context.Context, err error) error {
-	return prbot.InValidRequestError(ctx, "error parsing webhook event", err)
+	return pe.InValidRequestError(ctx, "error parsing webhook event", err)
 }
diff --git a/pullrequest/dispatcher_test.go b/pullrequest/dispatcher_test.go
@@ -8,7 +8,7 @@ import (
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/google/go-github/v50/github"
-	prbot "github.com/marqeta/pr-bot"
+	pe "github.com/marqeta/pr-bot/errors"
 	"github.com/marqeta/pr-bot/id"
 	"github.com/marqeta/pr-bot/metrics"
 	"github.com/marqeta/pr-bot/pullrequest"
@@ -220,7 +220,7 @@ func Test_dispatcher_Dispatch(t *testing.T) {
 			setExpectations: func(_ id.PR, _ *github.PullRequestEvent,
 				_ *pullrequest.MockEventFilter, _ *pullrequest.MockEventHandler) {
 			},
-			wantErr: prbot.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrMismatchedEvent),
+			wantErr: pe.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrMismatchedEvent),
 		},
 		{
 			name: "Error when Event action is nil",
@@ -232,7 +232,7 @@ func Test_dispatcher_Dispatch(t *testing.T) {
 			setExpectations: func(_ id.PR, _ *github.PullRequestEvent,
 				_ *pullrequest.MockEventFilter, _ *pullrequest.MockEventHandler) {
 			},
-			wantErr: prbot.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrEventActionNotFound),
+			wantErr: pe.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrEventActionNotFound),
 		},
 		{
 			name: "Error when Event PR is nil",
@@ -247,7 +247,7 @@ func Test_dispatcher_Dispatch(t *testing.T) {
 			setExpectations: func(_ id.PR, _ *github.PullRequestEvent,
 				_ *pullrequest.MockEventFilter, _ *pullrequest.MockEventHandler) {
 			},
-			wantErr: prbot.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrPRNotFound),
+			wantErr: pe.InValidRequestError(ctx, "error parsing webhook event", pullrequest.ErrPRNotFound),
 		},
 		{
 			name: "Should return error from event handler",

diff --git a/pullrequest/event_filter.go b/pullrequest/event_filter.go
@@ -5,8 +5,8 @@ import (
 	"regexp"
 
 	"github.com/go-chi/httplog"
-	prbot "github.com/marqeta/pr-bot"
 	"github.com/marqeta/pr-bot/configstore"
+	pe "github.com/marqeta/pr-bot/errors"
 	gh "github.com/marqeta/pr-bot/github"
 	"github.com/marqeta/pr-bot/id"
 )
@@ -106,7 +106,7 @@ func (f *repoFilter) ShouldHandle(ctx context.Context, id id.PR) (bool, error) {
 func (f *repoFilter) hasIgnoreTopic(ctx context.Context, id id.PR, cfg *RepoFilterCfg) (bool, error) {
 	topics, err := f.dao.ListAllTopics(ctx, id)
 	if err != nil {
-		return true, prbot.ServiceFault(ctx, "error listing topics on repo", err)
+		return true, pe.ServiceFault(ctx, "error listing topics on repo", err)
 	}
 	for _, topic := range topics {
 		if cfg.IgnoreTopicsMap[topic] {

diff --git a/pullrequest/review/dedup_reviewer.go b/pullrequest/review/dedup_reviewer.go
@@ -29,7 +29,7 @@ func (d *DedupReviewer) Approve(ctx context.Context, id id.PR, body string, opts
 		return err
 	}
 	if d.checkForReview(reviews, types.Approve) {
-		oplog.Info().Msgf("PR already has a review of type %v or higher", types.Approve)
+		oplog.Info().Msgf("PR already has a review of type %v or higher %v", types.Approve, id.URL)
 		return nil
 	}
 	return d.delegate.Approve(ctx, id, body, opts)
@@ -44,7 +44,7 @@ func (d *DedupReviewer) Comment(ctx context.Context, id id.PR, body string) erro
 		return err
 	}
 	if d.checkForReview(reviews, types.Comment) {
-		oplog.Info().Msgf("PR already has a review of type %v or higher", types.Comment)
+		oplog.Info().Msgf("PR already has a review of type %v or higher %v", types.Comment, id.URL)
 		return nil
 	}
 	return d.delegate.Comment(ctx, id, body)
@@ -59,7 +59,7 @@ func (d *DedupReviewer) RequestChanges(ctx context.Context, id id.PR, body strin
 		return err
 	}
 	if d.checkForReview(reviews, types.RequestChanges) {
-		oplog.Info().Msgf("PR already has a review of type %v or higher", types.RequestChanges)
+		oplog.Info().Msgf("PR already has a review of type %v or higher %v", types.RequestChanges, id.URL)
 		return nil
 	}
 	return d.delegate.RequestChanges(ctx, id, body)