diff --git a/api/api.go b/api/api.go index e1ab2ba19a54faa0a97a3243efc5f2b56cea87a3..500668bbcc87023e7f826c3737b5dd242c48c4b9 100644 --- a/api/api.go +++ b/api/api.go @@ -13,6 +13,7 @@ import ( "github.com/reviewboard/rb-gateway/api/tokens" "github.com/reviewboard/rb-gateway/config" + "github.com/reviewboard/rb-gateway/errors" "github.com/reviewboard/rb-gateway/repositories" "github.com/reviewboard/rb-gateway/repositories/hooks" ) @@ -27,6 +28,12 @@ type routingEntry struct { handler http.Handler } +func init() { + auth.NormalHeaders.UnauthContentType = "application/json" + auth.NormalHeaders.UnauthResponse = string(SerializeError(errors.New( + "Unauthorized").UserVisible())) +} + func addRoutes(router *mux.Router, routes []routingEntry) { for _, route := range routes { router.Path(route.path). @@ -74,6 +81,11 @@ func New(cfg *config.Config) (*API, error) { Methods("GET", "POST"). HandlerFunc(api.authenticator.Wrap(api.getSession)) + api.router.NotFoundHandler = http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + WriteErrorMsg(w, errors.New("Not found").UserVisible(), http.StatusNotFound) + }) + // The following routes all require token authorization. repoRouter := api.router.PathPrefix("/repos/{repo}").Subrouter() repoRouter.Use(api.withAuthorizationRequired) @@ -205,9 +217,13 @@ func (api *API) withRepository(next http.Handler) http.Handler { var exists bool if len(repoName) == 0 { - http.Error(w, "Repository not provided.", http.StatusBadRequest) + WriteError(w, + errors.New("Repository required.").UserVisible(), + http.StatusBadRequest) } else if repo, exists = api.config.Repositories[repoName]; !exists { - http.Error(w, "Repository not found.", http.StatusNotFound) + WriteError(w, + errors.New("Repository not found.").UserVisible(), + http.StatusNotFound) } else { ctx := context.WithValue(r.Context(), "repo", repo) next.ServeHTTP(w, r.WithContext(ctx)) @@ -218,7 +234,8 @@ func (api *API) withRepository(next http.Handler) http.Handler { func (api *API) withAuthorizationRequired(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if api.tokenStore.Get(r) == nil { - http.Error(w, "Authorization failed.", http.StatusUnauthorized) + WriteErrorMsg(w, errors.New("Authorization failed").UserVisible(), + http.StatusUnauthorized) } else { next.ServeHTTP(w, r) } diff --git a/api/errors.go b/api/errors.go new file mode 100644 index 0000000000000000000000000000000000000000..7c169106d2e6a11f87faaad51dcbffce24c67873 --- /dev/null +++ b/api/errors.go @@ -0,0 +1,48 @@ +package api + +import ( + "encoding/json" + "fmt" + "net/http" + + "github.com/reviewboard/rb-gateway/errors" +) + +type apiError struct { + Err errMsg `json:"err"` +} + +type errMsg struct { + Msg string `json:"msg"` +} + +// Write the given error to the http response and log it. +func WriteError(w http.ResponseWriter, err error, statusCode int) { + WriteErrorMsg(w, err, statusCode) + errors.LogError(err) +} + +// Write the given error message to the http response. +func WriteErrorMsg(w http.ResponseWriter, err error, statusCode int) { + w.WriteHeader(statusCode) + w.Header().Set("Content-Type", "application/json") + w.Write(SerializeError(err)) +} + +func SerializeError(err error) []byte { + var apiErr apiError + + if _, ok := err.(*errors.UserVisibleError); ok { + apiErr.Err.Msg = err.Error() + } else { + apiErr.Err.Msg = "An unknown error occurred." + } + + jsonRsp, jsonErr := json.Marshal(apiErr) + + if jsonErr != nil { + panic(fmt.Sprintf("Could not write error response %#v: %s", apiErr, jsonErr.Error())) + } + + return jsonRsp +} diff --git a/api/errors_test.go b/api/errors_test.go new file mode 100644 index 0000000000000000000000000000000000000000..57a717b72860cc961da131ee010bd081b49379a9 --- /dev/null +++ b/api/errors_test.go @@ -0,0 +1,81 @@ +package api_test + +import ( + "log" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/reviewboard/rb-gateway/api" + "github.com/reviewboard/rb-gateway/errors" +) + +const logPrefixLen = len("2006/01/02 15:04:05 ") + +func setupErrorTest(t *testing.T) (*httptest.ResponseRecorder, *strings.Builder) { + t.Helper() + + var buf strings.Builder + log.SetOutput(&buf) + + return httptest.NewRecorder(), &buf +} + +func TestWriteError(t *testing.T) { + assert := assert.New(t) + w, buf := setupErrorTest(t) + + api.WriteError(w, errors.New("Hidden from user"), http.StatusInternalServerError) + + assert.Equal("ERROR: Hidden from user\n", buf.String()[logPrefixLen:]) + assert.Equal(`{"err":{"msg":"An unknown error occurred."}}`, w.Body.String()) + assert.Equal("application/json", w.HeaderMap.Get("Content-Type")) + assert.Equal(http.StatusInternalServerError, w.Code) +} + +func TestWriteErrorWithCause(t *testing.T) { + assert := assert.New(t) + w, buf := setupErrorTest(t) + + cause := errors.New("Inner Error") + + api.WriteError(w, errors.NewWithCause(cause, "Complicated error"), http.StatusInternalServerError) + + assert.Equal("ERROR: Complicated error\n\tCAUSED BY: Inner Error\n", buf.String()[logPrefixLen:]) + assert.Equal(`{"err":{"msg":"An unknown error occurred."}}`, w.Body.String()) + assert.Equal("application/json", w.HeaderMap.Get("Content-Type")) + assert.Equal(http.StatusInternalServerError, w.Code) +} + +func TestWriteErrorChain(t *testing.T) { + assert := assert.New(t) + w, buf := setupErrorTest(t) + + err1 := errors.New("err1") + err2 := errors.NewWithCause(err1, "err2") + err3 := errors.NewWithCause(err2, "err3") + + api.WriteError(w, err3, http.StatusInternalServerError) + + assert.Equal("ERROR: err3\n\tCAUSED BY: err2\n\tCAUSED BY: err1\n", buf.String()[logPrefixLen:]) + assert.Equal(`{"err":{"msg":"An unknown error occurred."}}`, w.Body.String()) + assert.Equal("application/json", w.HeaderMap.Get("Content-Type")) + assert.Equal(http.StatusInternalServerError, w.Code) +} + +func TestWriteErrorUserVisible(t *testing.T) { + assert := assert.New(t) + w, buf := setupErrorTest(t) + + internal := errors.New("Internal") + + api.WriteError(w, errors.NewWithCause(internal, "User visible failure").UserVisible(), http.StatusBadRequest) + + assert.Equal("ERROR: User visible failure\n\tCAUSED BY: Internal\n", buf.String()[logPrefixLen:]) + assert.Equal(`{"err":{"msg":"User visible failure"}}`, w.Body.String()) + assert.Equal("application/json", w.HeaderMap.Get("Content-Type")) + assert.Equal(http.StatusBadRequest, w.Code) +} diff --git a/api/routes.go b/api/routes.go index e2d3024b4582450d61598383275812f71fa92b02..e31f3604012d69334fe2f1c8d4b7cddcb63cc0e4 100644 --- a/api/routes.go +++ b/api/routes.go @@ -4,12 +4,12 @@ import ( "bytes" "encoding/json" "fmt" - "log" "net/http" auth "github.com/abbot/go-http-auth" "github.com/gorilla/mux" + "github.com/reviewboard/rb-gateway/errors" "github.com/reviewboard/rb-gateway/repositories" "github.com/reviewboard/rb-gateway/repositories/hooks" ) @@ -21,8 +21,10 @@ func (api *API) getSession(w http.ResponseWriter, r *auth.AuthenticatedRequest) token, err := api.tokenStore.New() if err != nil { - log.Printf("Could not create session: %s", err.Error()) - http.Error(w, "Could not create session", http.StatusInternalServerError) + WriteError(w, + errors.NewWithCause(err, "Could not create session"). + UserVisible(), + http.StatusInternalServerError) } session := Session{ @@ -31,8 +33,10 @@ func (api *API) getSession(w http.ResponseWriter, r *auth.AuthenticatedRequest) json, err := json.Marshal(&session) if err != nil { - log.Printf("Could not serialize session: %s", err.Error()) - http.Error(w, "Could not create session", http.StatusInternalServerError) + WriteError(w, + errors.NewWithCause(err, "Could not create session"). + UserVisible(), + http.StatusInternalServerError) return } @@ -51,10 +55,14 @@ func (_ *API) getBranches(w http.ResponseWriter, r *http.Request) { var err error if branches, err = repo.GetBranches(); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + WriteError(w, + errors.NewWithCause(err, "Could not get branches"). + UserVisible(), + http.StatusInternalServerError) } else if response, err = json.Marshal(branches); err != nil { - log.Printf("Could not serialize branches: %s", err.Error()) - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, + errors.NewWithCause(err, "Could not serialize branches"), + http.StatusInternalServerError) } else { w.Header().Set("Content-Type", "application/json") w.Write(response) @@ -75,13 +83,18 @@ func (_ *API) getCommits(w http.ResponseWriter, r *http.Request) { var err error if len(branch) == 0 { - http.Error(w, "Branch not specified.", http.StatusBadRequest) + WriteError(w, + errors.New("Branch not specified."), + http.StatusBadRequest) } else if commits, err = repo.GetCommits(branch, start); err != nil { - http.Error(w, fmt.Sprintf("Could not get branches: %s", err.Error()), + WriteError(w, + errors.NewWithCause(err, "Could not get branches"). + UserVisible(), http.StatusBadRequest) } else if response, err = json.Marshal(commits); err != nil { - log.Printf("Could not serialize commits: %s", err.Error()) - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, + errors.NewWithCause(err, "Could not serialize commits"), + http.StatusInternalServerError) } else { w.Header().Set("Content-Type", "application/json") w.Write(response) @@ -101,14 +114,25 @@ func (_ *API) getCommit(w http.ResponseWriter, r *http.Request) { var err error if len(commitId) == 0 { - http.Error(w, "Commit ID not specified.", http.StatusBadRequest) + WriteError(w, + errors.New("Commit ID not specified.").UserVisible(), + http.StatusBadRequest) } else if commit, err = repo.GetCommit(commitId); err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) + WriteError(w, + errors.NewWithCause(err, "Could not get commit."). + UserVisible(), + http.StatusInternalServerError) } else if commit == nil { - http.Error(w, "Commit ID not found.", http.StatusNotFound) + WriteError(w, + errors.Newf("Commit ID \"%s\" not found", commitId). + UserVisible(), + http.StatusNotFound) } else if response, err = json.Marshal(*commit); err != nil { - log.Printf("Could not serialize commit \"%s\" in repo \"%s\": %s", commit.Id, repo.GetName(), err.Error()) - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, + errors.NewfWithCause(err, + "Could not serialize commit \"%s\" in repo \"%s\"", + commit.Id, repo.GetName()), + http.StatusInternalServerError) } else { w.Header().Set("Content-Type", "application/json") w.Write(response) @@ -126,9 +150,12 @@ func (_ *API) getFile(w http.ResponseWriter, r *http.Request) { var err error if len(objectId) == 0 { - http.Error(w, "File ID not specified.", http.StatusBadRequest) + WriteError(w, errors.New("File ID not specified.").UserVisible(), + http.StatusBadRequest) } else if contents, err = repo.GetFile(objectId); err != nil { - http.Error(w, fmt.Sprintf("Could not get file \"%s\": %s", objectId, err.Error()), + WriteError(w, errors.NewfWithCause(err, + "Could not get file \"%s\"", objectId). + UserVisible(), http.StatusNotFound) } else { w.Header().Set("Content-Type", "application/octet-stream") @@ -147,9 +174,13 @@ func (_ *API) getFileExists(w http.ResponseWriter, r *http.Request) { var err error if len(objectId) == 0 { - http.Error(w, "File ID not specified.", http.StatusBadRequest) + WriteError(w, errors.New("File ID not specified.").UserVisible(), + http.StatusBadRequest) } else if exists, err = repo.FileExists(objectId); err != nil { - http.Error(w, fmt.Sprintf("Could not find file \"%s\" in repo: %s", objectId, err.Error()), + WriteError(w, + errors.NewfWithCause(err, + "Could not fine file \"%s\" in repo", objectId). + UserVisible(), http.StatusBadRequest) } else if !exists { w.WriteHeader(http.StatusNotFound) @@ -172,13 +203,15 @@ func (_ *API) getFileByCommit(w http.ResponseWriter, r *http.Request) { var err error if len(commitId) == 0 { - http.Error(w, "Commit ID not specified.", http.StatusBadRequest) + WriteError(w, errors.New("Commit ID not specified."), + http.StatusBadRequest) } else if len(path) == 0 { - http.Error(w, "File path not specified.", http.StatusBadRequest) + WriteError(w, errors.New("File path not specified."), + http.StatusBadRequest) } else if contents, err = repo.GetFileByCommit(commitId, path); err != nil { - http.Error(w, - fmt.Sprintf("Could not get file \"%s\" at commit \"%s\": %s", - path, commitId, err.Error()), + WriteError(w, errors.NewfWithCause(err, + "Could not get file \"%s\" at commit \"%s\"", + path, commitId).UserVisible(), http.StatusNotFound) } else { w.Header().Set("Content-Type", "application/octet-stream") @@ -200,13 +233,15 @@ func (_ *API) getFileExistsByCommit(w http.ResponseWriter, r *http.Request) { var err error if len(commitId) == 0 { - http.Error(w, "Commit ID not specified.", http.StatusBadRequest) + WriteError(w, errors.New("Commit ID not specified").UserVisible(), + http.StatusBadRequest) } else if len(path) == 0 { - http.Error(w, "File path not specified.", http.StatusBadRequest) + WriteError(w, errors.New("File path not specified").UserVisible(), + http.StatusBadRequest) } else if exists, err = repo.FileExistsByCommit(commitId, path); err != nil { - http.Error(w, - fmt.Sprintf("Could not find file \"%s\" at commit \"%s\": %s", - path, commitId, err.Error()), + WriteError(w, errors.NewfWithCause(err, + "Could not find file \"%s\" at commit \"%s\"", + path, commitId).UserVisible(), http.StatusBadRequest) } else if !exists { w.WriteHeader(http.StatusNotFound) @@ -244,8 +279,9 @@ func (api *API) getHooks(w http.ResponseWriter, r *http.Request) { b, err := json.Marshal(hook) if err != nil { - log.Printf("Could not serialize hooks: %s", err.Error()) - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, + errors.NewWithCause(err, "Could not serialize hook"), + http.StatusInternalServerError) return } @@ -257,7 +293,6 @@ func (api *API) getHooks(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) w.Header().Set("Content-Type", "application/json") w.Write(buffer.Bytes()) - } func (api *API) createHook(w http.ResponseWriter, r *http.Request) { @@ -267,21 +302,22 @@ func (api *API) createHook(w http.ResponseWriter, r *http.Request) { var hook hooks.Webhook if err := json.NewDecoder(r.Body).Decode(&hook); err != nil { - http.Error(w, - fmt.Sprintf("Could not parse request body: %s", err.Error()), + WriteError(w, + errors.NewWithCause(err, "Could not parse request body"), http.StatusBadRequest) return } if api.hookStore[hook.Id] != nil { - http.Error(w, - fmt.Sprintf(`A webhook with ID "%s" already exists.`, hook.Id), + WriteError(w, + errors.Newf(`A webhook with ID "%s" already exists.`, hook.Id). + UserVisible(), http.StatusBadRequest) return } if err := hook.Validate(api.config.RepositorySet()); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + WriteError(w, err, http.StatusBadRequest) return } @@ -289,9 +325,9 @@ func (api *API) createHook(w http.ResponseWriter, r *http.Request) { if err := api.hookStore.Save(api.config.WebhookStorePath); err != nil { // If we cannot save the store, revert our state so that we stay // consistent with it. - log.Println("Could not save webhook store: ", err.Error()) delete(api.hookStore, hook.Id) - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, errors.NewWithCause(err, "Could not save webhook store"), + http.StatusInternalServerError) } else { w.WriteHeader(http.StatusCreated) } @@ -305,14 +341,16 @@ func (api *API) getHook(w http.ResponseWriter, r *http.Request) { var hook *hooks.Webhook if hook = api.hookStore[hookId]; hook == nil { - http.Error(w, "No such webhook", http.StatusNotFound) + WriteError(w, + errors.New("No such webhook").UserVisible(), + http.StatusNotFound) return } b, err := json.Marshal(hook) if err != nil { - log.Printf("Could not serialize hooks: %s", err.Error()) - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, errors.NewWithCause(err, "Could not serialize hooks"), + http.StatusInternalServerError) return } @@ -329,16 +367,17 @@ func (api *API) deleteHook(w http.ResponseWriter, r *http.Request) { var hook *hooks.Webhook if hook = api.hookStore[hookId]; hook == nil { - http.Error(w, "No such webhook", http.StatusNotFound) + WriteError(w, errors.New("No such webhook").UserVisible(), + http.StatusNotFound) } delete(api.hookStore, hookId) if err := api.hookStore.Save(api.config.WebhookStorePath); err != nil { // If we cannot save the store, revert our state so that we stay // consistent with it. - log.Println("Could not save webhook store: ", err.Error()) api.hookStore[hookId] = hook - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, errors.NewWithCause(err, "Could not save webhook store"), + http.StatusInternalServerError) } else { w.WriteHeader(http.StatusNoContent) } @@ -359,14 +398,18 @@ func (api *API) updateHook(w http.ResponseWriter, r *http.Request) { var parsedRequest struct { Id *string `json:"id"` - Url *string `json:"url",omitempty` - Secret *string `json:"secret",omitempty` + Url *string `json:"url"` + Secret *string `json:"secret"` Enabled *bool `json:"enabled"` Events []string `json:"events"` Repos []string `json:"repos"` } if err := json.NewDecoder(r.Body).Decode(&parsedRequest); err != nil { + WriteError(w, + errors.NewWithCause(err, "Could not parse request body").UserVisible(), + http.StatusBadRequest) + http.Error(w, fmt.Sprintf("Could not parse request body: %s", err.Error()), http.StatusBadRequest) @@ -383,7 +426,8 @@ func (api *API) updateHook(w http.ResponseWriter, r *http.Request) { } if parsedRequest.Id != nil { - http.Error(w, "Hook ID cannot be updated.", http.StatusBadRequest) + WriteError(w, errors.New("Hook ID cannot be updated.").UserVisible(), + http.StatusBadRequest) return } @@ -408,7 +452,7 @@ func (api *API) updateHook(w http.ResponseWriter, r *http.Request) { } if err := updatedHook.Validate(api.config.RepositorySet()); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) + WriteError(w, err, http.StatusBadRequest) return } @@ -417,12 +461,14 @@ func (api *API) updateHook(w http.ResponseWriter, r *http.Request) { // If we cannot save the store, revert our state so that we stay // consistent with it. api.hookStore[hook.Id] = hook - log.Println("Could not update hook store: ", err.Error()) - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, + errors.NewWithCause(err, "Could not update hook store"), + http.StatusInternalServerError) } else { var b []byte if b, err = json.MarshalIndent(updatedHook, "", " "); err != nil { - http.Error(w, "An unexpected error occurred.", http.StatusInternalServerError) + WriteError(w, errors.NewWithCause(err, "Could not serialize hook"), + http.StatusInternalServerError) return } diff --git a/api/routes_test.go b/api/routes_test.go index b8972c1e37ea35574897f879e2bb96a8d2982998..64c24e072a45a513090f8921b997952c3c7b101f 100644 --- a/api/routes_test.go +++ b/api/routes_test.go @@ -395,7 +395,7 @@ func TestCreateHookAPIValidate(t *testing.T) { }{ { hook: *testSetup.hooks["test-hook-1"], - errorMsg: "A webhook with ID \"test-hook-1\" already exists.\n", + errorMsg: `{"err":{"msg":"A webhook with ID \"test-hook-1\" already exists."}}`, }, { hook: hooks.Webhook{ @@ -406,7 +406,7 @@ func TestCreateHookAPIValidate(t *testing.T) { Events: []string{}, Repos: []string{"repo"}, }, - errorMsg: "Hook has no events.\n", + errorMsg: `{"err":{"msg":"Hook has no events."}}`, }, { hook: hooks.Webhook{ @@ -417,7 +417,7 @@ func TestCreateHookAPIValidate(t *testing.T) { Events: []string{"foo"}, Repos: []string{"repo"}, }, - errorMsg: "Invalid event: \"foo\".\n", + errorMsg: `{"err":{"msg":"Invalid event: \"foo\"."}}`, }, { hook: hooks.Webhook{ @@ -428,7 +428,7 @@ func TestCreateHookAPIValidate(t *testing.T) { Events: []string{events.PushEvent}, Repos: []string{}, }, - errorMsg: "Hook has no repositories.\n", + errorMsg: `{"err":{"msg":"Hook has no repositories."}}`, }, { hook: hooks.Webhook{ @@ -439,7 +439,7 @@ func TestCreateHookAPIValidate(t *testing.T) { Events: []string{events.PushEvent}, Repos: []string{"foo"}, }, - errorMsg: "Invalid repository: \"foo\".\n", + errorMsg: `{"err":{"msg":"Invalid repository: \"foo\"."}}`, }, { hook: hooks.Webhook{ @@ -450,7 +450,7 @@ func TestCreateHookAPIValidate(t *testing.T) { Events: []string{events.PushEvent}, Repos: []string{"repo"}, }, - errorMsg: "Invalid URL scheme \"ftp\": only HTTP and HTTPS are supported.\n", + errorMsg: `{"err":{"msg":"Invalid URL scheme \"ftp\": only HTTP and HTTPS are supported."}}`, }, { hook: hooks.Webhook{ @@ -461,7 +461,7 @@ func TestCreateHookAPIValidate(t *testing.T) { Events: []string{events.PushEvent}, Repos: []string{"repo"}, }, - errorMsg: "Secret is too short (1 bytes); secrets must be at least 20 bytes.\n", + errorMsg: `{"err":{"msg":"Secret is too short (1 bytes); secrets must be at least 20 bytes."}}`, }, } @@ -494,7 +494,7 @@ func TestUpdateHook(t *testing.T) { "id": "foo-bar", }, statusCode: 400, - errorMsg: "Hook ID cannot be updated.\n", + errorMsg: `{"err":{"msg":"Hook ID cannot be updated."}}`, }, { body: map[string]interface{}{ @@ -515,7 +515,7 @@ func TestUpdateHook(t *testing.T) { "secret": "abcd", }, statusCode: 400, - errorMsg: "Secret is too short (4 bytes); secrets must be at least 20 bytes.\n", + errorMsg: `{"err":{"msg":"Secret is too short (4 bytes); secrets must be at least 20 bytes."}}`, }, { body: map[string]interface{}{ @@ -550,28 +550,28 @@ func TestUpdateHook(t *testing.T) { "events": []string{events.PushEvent, "pull"}, }, statusCode: 400, - errorMsg: "Invalid event: \"pull\".\n", + errorMsg: `{"err":{"msg":"Invalid event: \"pull\"."}}`, }, { body: map[string]interface{}{ "events": []string{}, }, statusCode: 400, - errorMsg: "Hook has no events.\n", + errorMsg: `{"err":{"msg":"Hook has no events."}}`, }, { body: map[string]interface{}{ "repos": []string{"asdf"}, }, statusCode: 400, - errorMsg: "Invalid repository: \"asdf\".\n", + errorMsg: `{"err":{"msg":"Invalid repository: \"asdf\"."}}`, }, { body: map[string]interface{}{ "repos": []string{}, }, statusCode: 400, - errorMsg: "Hook has no repositories.\n", + errorMsg: `{"err":{"msg":"Hook has no repositories."}}`, }, } diff --git a/api/tokens/memory_store.go b/api/tokens/memory_store.go index 3a854c3793efbd96bc63aef8233ed1a6a9e3c3b6..057b4aaf45b5ab5840b542207758b374c96e024a 100644 --- a/api/tokens/memory_store.go +++ b/api/tokens/memory_store.go @@ -4,6 +4,8 @@ import ( "crypto/rand" "fmt" "net/http" + + "github.com/reviewboard/rb-gateway/errors" ) const ( @@ -55,7 +57,7 @@ func (store MemoryStore) New() (*string, error) { for i := 0; i < maxAttempts; i++ { if _, err := rand.Read(raw[:]); err != nil { - return nil, fmt.Errorf("Could not generate token: %s\n", err.Error()) + return nil, errors.NewWithCause(err, "Could not generate token").UserVisible() } token := fmt.Sprintf("%X", raw) @@ -65,7 +67,7 @@ func (store MemoryStore) New() (*string, error) { } } - return nil, fmt.Errorf("Could not generate token after %d attempts.\n", maxAttempts) + return nil, errors.Newf("Could not generate token after %d attempts.", maxAttempts).UserVisible() } // Return whether or not a token exists in the store. diff --git a/commands/serve.go b/commands/serve.go index 613a33203aba04c939d3bb259c31c67bac92f2be..de6cbcdba27c94f2acafe72c9102c5daa999bde6 100644 --- a/commands/serve.go +++ b/commands/serve.go @@ -8,6 +8,7 @@ import ( "github.com/reviewboard/rb-gateway/api" "github.com/reviewboard/rb-gateway/config" + "github.com/reviewboard/rb-gateway/errors" ) func Serve(configPath string) { @@ -78,7 +79,7 @@ func Serve(configPath string) { err = api.Shutdown(server) if err != nil { - log.Fatalf("An error occurred while shutting down the server: %s", err.Error()) + errors.LogError(errors.NewWithCause(err, "Could not shut down server")) } log.Println("Server shut down.") @@ -89,10 +90,10 @@ func Serve(configPath string) { if newCfg != nil { if newCfg.TokenStorePath == ":memory:" { - log.Println("Failed to reload configuration: cannot use memory store outside of tests.") + errors.LogError(errors.New("Failed to reload configuration: cannot use memory store outside of tests.")) log.Println("Configuration was not reloaded.") } else if err = api.SetConfig(newCfg); err != nil { - log.Printf("Failed to reload configuration: %s\n", err.Error()) + errors.LogError(errors.NewWithCause(err, "Failed to reload configurdation")) } else { log.Println("Configuration reloaded.") diff --git a/config/config.go b/config/config.go index eddbec5df0b653d8564fdfa428fe4244883ef0a4..539def1baf3769f837d6e2b62b0cc0a59f89ae73 100644 --- a/config/config.go +++ b/config/config.go @@ -2,12 +2,12 @@ package config import ( "encoding/json" - "fmt" "io/ioutil" "log" "path/filepath" "strings" + "github.com/reviewboard/rb-gateway/errors" "github.com/reviewboard/rb-gateway/repositories" ) @@ -147,7 +147,9 @@ func validate(cfgDir string, config *Config) (err error) { config.WebhookStorePath = resolvePath(cfgDir, config.WebhookStorePath) if len(missingFields) != 0 { - err = fmt.Errorf("Some required fields were missing from the configuration: %s.", strings.Join(missingFields, ",")) + err = errors.Newf( + "Some required fields were missing from the configuration: %s.", + strings.Join(missingFields, ",")) } return diff --git a/errors/errorchain.go b/errors/errorchain.go new file mode 100644 index 0000000000000000000000000000000000000000..1ab08dd5ebc7963098226ca22c5e91a3cdcd3578 --- /dev/null +++ b/errors/errorchain.go @@ -0,0 +1,41 @@ +package errors + +import ( + "fmt" + "log" + "strings" +) + +// An expanded error interface that supports causes and custom logging. +type ErrorChain interface { + error + + // Return the cause of the error. + Cause() error +} + +// Log an error message. +// +// If the error implements the ErrorChain interface, the entire chain will be +// logged. +func LogError(err error) { + var logMsg strings.Builder + + fmt.Fprintf(&logMsg, "ERROR: %s", err.Error()) + + if chain, ok := err.(ErrorChain); ok { + cause := chain.Cause() + + for cause != nil { + fmt.Fprintf(&logMsg, "\n\tCAUSED BY: %s", cause.Error()) + + if prevCause, ok := cause.(ErrorChain); ok { + cause = prevCause.Cause() + } else { + break + } + } + } + + log.Println(logMsg.String()) +} diff --git a/errors/uservisible.go b/errors/uservisible.go new file mode 100644 index 0000000000000000000000000000000000000000..b4287d43f39d65ac6c1eee64071decbfeeac19d9 --- /dev/null +++ b/errors/uservisible.go @@ -0,0 +1,70 @@ +package errors + +import "fmt" + +// A user-visible error message. +// +// This may be presented to an API consumer. +type UserVisibleError struct { + msg string + inner error +} + +// Return the error message for this error. +func (e *UserVisibleError) Error() string { + return e.msg +} + +// Return the cause (if any) for this error. +func (e *UserVisibleError) Cause() error { + return e.inner +} + +// An internal error message. +// +// This will not be presented to an API consumer. +type InternalError struct { + msg string + inner error +} + +// Return the error message for this error. +func (e *InternalError) Error() string { + return e.msg +} + +// Return the cause (if any) for this error. +func (e *InternalError) Cause() error { + return e.inner +} + +// Mark this error as user-visible. +func (e *InternalError) UserVisible() *UserVisibleError { + return &UserVisibleError{ + msg: e.msg, + inner: e.inner, + } +} + +// Create a new error. +func New(msg string) *InternalError { + return NewWithCause(nil, msg) +} + +// Create a new error with the given cause. +func NewWithCause(cause error, msg string) *InternalError { + return &InternalError{ + msg, + cause, + } +} + +// Create a new error with a format string. +func Newf(f string, vs ...interface{}) *InternalError { + return NewWithCause(nil, fmt.Sprintf(f, vs...)) +} + +// Create a new error with a cause using a format string. +func NewfWithCause(cause error, f string, vs ...interface{}) *InternalError { + return NewWithCause(cause, fmt.Sprintf(f, vs...)) +} diff --git a/repositories/git.go b/repositories/git.go index 97599202d5d865978cd160245156345a94710e4e..aced9fe206eeee324192c9b5604382f080eb7b3a 100644 --- a/repositories/git.go +++ b/repositories/git.go @@ -2,7 +2,6 @@ package repositories import ( "bytes" - "errors" "fmt" "io" "io/ioutil" @@ -14,6 +13,7 @@ import ( "gopkg.in/src-d/go-git.v4/plumbing" "gopkg.in/src-d/go-git.v4/plumbing/object" + "github.com/reviewboard/rb-gateway/errors" "github.com/reviewboard/rb-gateway/repositories/events" ) @@ -297,7 +297,7 @@ func (repo *GitRepository) GetCommit(commitId string) (*Commit, error) { } if commit.NumParents() == 0 { - return nil, errors.New("Commit has no parents.") + return nil, errors.New("Commit has no parents.").UserVisible() } parent, err := commit.Parent(0) @@ -339,7 +339,7 @@ func (repo *GitRepository) ParseEventPayload(event string, input io.Reader) (eve return repo.parsePushEvent(gitRepo, event, input) default: - return nil, fmt.Errorf(`Event "%s" unsupported by Git.`, event) + return nil, errors.Newf(`Event "%s" unsupported by Git.`, event) } } diff --git a/repositories/git_hooks.go b/repositories/git_hooks.go index a3e75467e041b69945bf11c206a1cc3d2b592024..c0942bac56f839571966c6a70510cdde9ef7cc41 100644 --- a/repositories/git_hooks.go +++ b/repositories/git_hooks.go @@ -9,6 +9,7 @@ import ( "github.com/kballard/go-shellquote" + "github.com/reviewboard/rb-gateway/errors" "github.com/reviewboard/rb-gateway/repositories/events" ) @@ -134,9 +135,8 @@ func (repo *GitRepository) installHook(hookDir string, hookData *gitHookData, fo log.Printf(`Restoring filesystem to original state for hook "%s"`, hookData.HookName) if err != nil { if err = os.Rename(renamedPath, dispatchPath); err != nil { - log.Println("Could not restore filesystem after error: ", err.Error()) + errors.LogError(errors.NewWithCause(err, "Could not restore filsystem after error")) } - } }() } diff --git a/repositories/hg.go b/repositories/hg.go index 2717eeede827d285bb7fc7d0d20e8fd4f3a82fb9..279fdbc770c90980994de0dafbd9418c0798281f 100644 --- a/repositories/hg.go +++ b/repositories/hg.go @@ -1,7 +1,6 @@ package repositories import ( - "errors" "fmt" "io" "os" @@ -12,6 +11,7 @@ import ( "github.com/go-ini/ini" "github.com/kballard/go-shellquote" + "github.com/reviewboard/rb-gateway/errors" "github.com/reviewboard/rb-gateway/repositories/events" ) @@ -348,7 +348,7 @@ func (repo *HgRepository) ParseEventPayload(event string, input io.Reader) (even return repo.parsePushEvent(first_node, last_node) default: - return nil, fmt.Errorf(`Event "%s" is unuspported by Hg.`, event) + return nil, errors.Newf(`Event "%s" is unsupported by Hg.`, event) } } diff --git a/repositories/hooks/webhook.go b/repositories/hooks/webhook.go index 1b381c9da380c15bb66317f8b65fc20982920111..dc596909b01aebf7839d6de10e347d01eae5ebdf 100644 --- a/repositories/hooks/webhook.go +++ b/repositories/hooks/webhook.go @@ -4,10 +4,9 @@ import ( "crypto/hmac" "crypto/sha1" "encoding/hex" - "errors" - "fmt" "net/url" + "github.com/reviewboard/rb-gateway/errors" "github.com/reviewboard/rb-gateway/repositories/events" ) @@ -42,38 +41,42 @@ func (hook Webhook) SignPayload(payload []byte) string { // Validate a hook. func (hook Webhook) Validate(repos map[string]struct{}) error { if len(hook.Events) == 0 { - return errors.New("Hook has no events.") + return errors.New("Hook has no events.").UserVisible() } else { for _, event := range hook.Events { if !events.IsValidEvent(event) { - return fmt.Errorf(`Invalid event: "%s".`, event) + return errors.Newf(`Invalid event: "%s".`, event). + UserVisible() } } } if len(hook.Repos) == 0 { - return errors.New("Hook has no repositories.") + return errors.New("Hook has no repositories.").UserVisible() } else { for _, repo := range hook.Repos { if _, ok := repos[repo]; !ok { - return fmt.Errorf(`Invalid repository: "%s".`, repo) + return errors.Newf(`Invalid repository: "%s".`, repo). + UserVisible() } } } url, err := url.Parse(hook.Url) if err != nil { - return fmt.Errorf("Invalid URL: %s", err.Error()) + return errors.Newf("Invalid URL: %s", err.Error()).UserVisible() } if url.Scheme != "http" && url.Scheme != "https" { - return fmt.Errorf(`Invalid URL scheme "%s": only HTTP and HTTPS are supported.`, - url.Scheme) + return errors.Newf( + `Invalid URL scheme "%s": only HTTP and HTTPS are supported.`, + url.Scheme).UserVisible() } if len(hook.Secret) < 20 { - return fmt.Errorf(`Secret is too short (%d bytes); secrets must be at least 20 bytes.`, - len(hook.Secret)) + return errors.Newf( + `Secret is too short (%d bytes); secrets must be at least 20 bytes.`, + len(hook.Secret)).UserVisible() } return nil diff --git a/repositories/invoke.go b/repositories/invoke.go index b80e0f1f04bfc790ebc791fed70de7d7a3e46379..e76a9618aa2bae86060d2f5ab519e94cb5ab00c6 100644 --- a/repositories/invoke.go +++ b/repositories/invoke.go @@ -2,11 +2,11 @@ package repositories import ( "bytes" - "fmt" "io/ioutil" "log" "net/http" + "github.com/reviewboard/rb-gateway/errors" "github.com/reviewboard/rb-gateway/repositories/events" "github.com/reviewboard/rb-gateway/repositories/hooks" ) @@ -20,7 +20,7 @@ func InvokeAllHooks( payload events.Payload, ) error { if !events.IsValidEvent(event) { - return fmt.Errorf(`Unknown event type "%s"`, event) + return errors.Newf(`Unknown event type "%s"`, event) } rawPayload, err := events.MarshalPayload(payload) @@ -31,15 +31,16 @@ func InvokeAllHooks( errs := store.ForEach(event, repository.GetName(), func(hook hooks.Webhook) error { err := invokeHook(client, event, repository, hook, rawPayload) if err != nil { - log.Printf(`Error ocurred while processing hook "%s" for URL "%s": %s`, - hook.Id, hook.Url, err.Error()) + errors.LogError(errors.NewfWithCause(err, + `Error ocurred while processing hook "%s" for URL "%s": %s`, + hook.Id, hook.Url, err.Error())) } return err }) if errs != nil { - return fmt.Errorf("%d errors occurred wihle processing webhooks", len(errs)) + return errors.Newf("%d errors occurred wihle processing webhooks", len(errs)) } return nil