From 4fb250a668d7241a098c4ca201ef61487387e84c Mon Sep 17 00:00:00 2001 From: Simone Gotti Date: Mon, 8 Apr 2019 12:29:25 +0200 Subject: [PATCH] configstore api: improve response handling * Always return a json message also on error. For internal errors return a generic "internal server error" message to not leak the real internal error to clients * Return 201 Created on resource creation * Return 204 No Content on resource deletion and other action with no json output --- internal/services/configstore/api/api.go | 44 ++++++++++++++++++- internal/services/configstore/api/org.go | 19 +++----- internal/services/configstore/api/project.go | 11 +++-- .../services/configstore/api/projectgroup.go | 16 ++----- .../services/configstore/api/remotesource.go | 19 +++----- internal/services/configstore/api/secret.go | 15 +++---- internal/services/configstore/api/user.go | 37 +++++++--------- internal/services/configstore/api/variable.go | 11 +++-- 8 files changed, 92 insertions(+), 80 deletions(-) diff --git a/internal/services/configstore/api/api.go b/internal/services/configstore/api/api.go index 1e65b03..97378c9 100644 --- a/internal/services/configstore/api/api.go +++ b/internal/services/configstore/api/api.go @@ -15,6 +15,7 @@ package api import ( + "encoding/json" "net/http" "net/url" @@ -24,12 +25,33 @@ import ( "github.com/sorintlab/agola/internal/util" ) +type ErrorResponse struct { + Message string `json:"message"` +} + +func ErrorResponseFromError(err error) *ErrorResponse { + if util.IsErrBadRequest(err) { + return &ErrorResponse{Message: err.Error()} + } + + // on generic error return an generic message to not leak the real error + return &ErrorResponse{Message: "internal server error"} +} + func httpError(w http.ResponseWriter, err error) bool { if err != nil { + response := ErrorResponseFromError(err) + resj, merr := json.Marshal(response) + if merr != nil { + w.WriteHeader(http.StatusInternalServerError) + return true + } if util.IsErrBadRequest(err) { - http.Error(w, err.Error(), http.StatusBadRequest) + w.WriteHeader(http.StatusBadRequest) + w.Write(resj) } else { - http.Error(w, "", http.StatusInternalServerError) + w.WriteHeader(http.StatusInternalServerError) + w.Write(resj) } return true } @@ -37,6 +59,24 @@ func httpError(w http.ResponseWriter, err error) bool { return false } +func httpResponse(w http.ResponseWriter, code int, res interface{}) error { + w.Header().Set("Content-Type", "application/json") + + if res != nil { + resj, err := json.Marshal(res) + if err != nil { + httpError(w, err) + return err + } + w.WriteHeader(code) + _, err = w.Write(resj) + return err + } + + w.WriteHeader(code) + return nil +} + func GetConfigTypeRef(r *http.Request) (types.ConfigType, string, error) { vars := mux.Vars(r) projectRef, err := url.PathUnescape(vars["projectref"]) diff --git a/internal/services/configstore/api/org.go b/internal/services/configstore/api/org.go index 783cfec..2786f37 100644 --- a/internal/services/configstore/api/org.go +++ b/internal/services/configstore/api/org.go @@ -58,10 +58,8 @@ func (h *OrgHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(org); err != nil { + if err := httpResponse(w, http.StatusOK, org); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -95,10 +93,8 @@ func (h *OrgByNameHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(org); err != nil { + if err := httpResponse(w, http.StatusOK, org); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -127,10 +123,8 @@ func (h *CreateOrgHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(org); err != nil { + if err := httpResponse(w, http.StatusCreated, org); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -154,6 +148,9 @@ func (h *DeleteOrgHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if httpError(w, err) { h.log.Errorf("err: %+v", err) } + if err := httpResponse(w, http.StatusNoContent, nil); err != nil { + h.log.Errorf("err: %+v", err) + } } const ( @@ -209,9 +206,7 @@ func (h *OrgsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(orgs); err != nil { + if err := httpResponse(w, http.StatusOK, orgs); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } diff --git a/internal/services/configstore/api/project.go b/internal/services/configstore/api/project.go index 5ebc943..d203dfa 100644 --- a/internal/services/configstore/api/project.go +++ b/internal/services/configstore/api/project.go @@ -74,10 +74,8 @@ func (h *ProjectHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(project); err != nil { + if err := httpResponse(w, http.StatusOK, project); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -107,10 +105,8 @@ func (h *CreateProjectHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } - if err := json.NewEncoder(w).Encode(project); err != nil { + if err := httpResponse(w, http.StatusCreated, project); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -137,6 +133,9 @@ func (h *DeleteProjectHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) if httpError(w, err) { h.log.Errorf("err: %+v", err) } + if err := httpResponse(w, http.StatusNoContent, nil); err != nil { + h.log.Errorf("err: %+v", err) + } } const ( diff --git a/internal/services/configstore/api/projectgroup.go b/internal/services/configstore/api/projectgroup.go index a1a13bb..6ea1c67 100644 --- a/internal/services/configstore/api/projectgroup.go +++ b/internal/services/configstore/api/projectgroup.go @@ -62,10 +62,8 @@ func (h *ProjectGroupHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } - if err := json.NewEncoder(w).Encode(projectGroup); err != nil { + if err := httpResponse(w, http.StatusOK, projectGroup); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -114,10 +112,8 @@ func (h *ProjectGroupProjectsHandler) ServeHTTP(w http.ResponseWriter, r *http.R return } - if err := json.NewEncoder(w).Encode(projects); err != nil { + if err := httpResponse(w, http.StatusOK, projects); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -166,10 +162,8 @@ func (h *ProjectGroupSubgroupsHandler) ServeHTTP(w http.ResponseWriter, r *http. return } - if err := json.NewEncoder(w).Encode(projectGroups); err != nil { + if err := httpResponse(w, http.StatusOK, projectGroups); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -199,9 +193,7 @@ func (h *CreateProjectGroupHandler) ServeHTTP(w http.ResponseWriter, r *http.Req return } - if err := json.NewEncoder(w).Encode(projectGroup); err != nil { + if err := httpResponse(w, http.StatusCreated, projectGroup); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } diff --git a/internal/services/configstore/api/remotesource.go b/internal/services/configstore/api/remotesource.go index a916bd8..5835b82 100644 --- a/internal/services/configstore/api/remotesource.go +++ b/internal/services/configstore/api/remotesource.go @@ -58,10 +58,8 @@ func (h *RemoteSourceHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } - if err := json.NewEncoder(w).Encode(remoteSource); err != nil { + if err := httpResponse(w, http.StatusOK, remoteSource); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -95,10 +93,8 @@ func (h *RemoteSourceByNameHandler) ServeHTTP(w http.ResponseWriter, r *http.Req return } - if err := json.NewEncoder(w).Encode(remoteSource); err != nil { + if err := httpResponse(w, http.StatusOK, remoteSource); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -128,10 +124,8 @@ func (h *CreateRemoteSourceHandler) ServeHTTP(w http.ResponseWriter, r *http.Req return } - if err := json.NewEncoder(w).Encode(remoteSource); err != nil { + if err := httpResponse(w, http.StatusCreated, remoteSource); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -154,6 +148,9 @@ func (h *DeleteRemoteSourceHandler) ServeHTTP(w http.ResponseWriter, r *http.Req if httpError(w, err) { h.log.Errorf("err: %+v", err) } + if err := httpResponse(w, http.StatusNoContent, nil); err != nil { + h.log.Errorf("err: %+v", err) + } } const ( @@ -204,9 +201,7 @@ func (h *RemoteSourcesHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } - if err := json.NewEncoder(w).Encode(remoteSources); err != nil { + if err := httpResponse(w, http.StatusOK, remoteSources); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } diff --git a/internal/services/configstore/api/secret.go b/internal/services/configstore/api/secret.go index e535c42..ed33008 100644 --- a/internal/services/configstore/api/secret.go +++ b/internal/services/configstore/api/secret.go @@ -57,10 +57,8 @@ func (h *SecretHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(secret); err != nil { + if err := httpResponse(w, http.StatusOK, secret); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -111,10 +109,8 @@ func (h *SecretsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(secrets); err != nil { + if err := httpResponse(w, http.StatusOK, secrets); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -152,10 +148,8 @@ func (h *CreateSecretHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } - if err := json.NewEncoder(w).Encode(secret); err != nil { + if err := httpResponse(w, http.StatusCreated, secret); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -183,4 +177,7 @@ func (h *DeleteSecretHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) if httpError(w, err) { h.log.Errorf("err: %+v", err) } + if err := httpResponse(w, http.StatusNoContent, nil); err != nil { + h.log.Errorf("err: %+v", err) + } } diff --git a/internal/services/configstore/api/user.go b/internal/services/configstore/api/user.go index c76e7c7..6907ca5 100644 --- a/internal/services/configstore/api/user.go +++ b/internal/services/configstore/api/user.go @@ -59,10 +59,8 @@ func (h *UserHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(user); err != nil { + if err := httpResponse(w, http.StatusOK, user); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -96,10 +94,8 @@ func (h *UserByNameHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(user); err != nil { + if err := httpResponse(w, http.StatusOK, user); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -148,10 +144,8 @@ func (h *CreateUserHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(user); err != nil { + if err := httpResponse(w, http.StatusCreated, user); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -174,6 +168,9 @@ func (h *DeleteUserHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { if httpError(w, err) { h.log.Errorf("err: %+v", err) } + if err := httpResponse(w, http.StatusNoContent, nil); err != nil { + h.log.Errorf("err: %+v", err) + } } const ( @@ -295,10 +292,8 @@ func (h *UsersHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } - if err := json.NewEncoder(w).Encode(users); err != nil { + if err := httpResponse(w, http.StatusOK, users); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -347,10 +342,8 @@ func (h *CreateUserLAHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } - if err := json.NewEncoder(w).Encode(user); err != nil { + if err := httpResponse(w, http.StatusCreated, user); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -373,6 +366,9 @@ func (h *DeleteUserLAHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) if httpError(w, err) { h.log.Errorf("err: %+v", err) } + if err := httpResponse(w, http.StatusNoContent, nil); err != nil { + h.log.Errorf("err: %+v", err) + } } type UpdateUserLARequest struct { @@ -420,10 +416,8 @@ func (h *UpdateUserLAHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) return } - if err := json.NewEncoder(w).Encode(user); err != nil { + if err := httpResponse(w, http.StatusOK, user); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -465,10 +459,8 @@ func (h *CreateUserTokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Reques resp := &CreateUserTokenResponse{ Token: token, } - if err := json.NewEncoder(w).Encode(resp); err != nil { + if err := httpResponse(w, http.StatusCreated, resp); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -491,4 +483,7 @@ func (h *DeleteUserTokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Reques if httpError(w, err) { h.log.Errorf("err: %+v", err) } + if err := httpResponse(w, http.StatusNoContent, nil); err != nil { + h.log.Errorf("err: %+v", err) + } } diff --git a/internal/services/configstore/api/variable.go b/internal/services/configstore/api/variable.go index 6ed0557..8c0b4a4 100644 --- a/internal/services/configstore/api/variable.go +++ b/internal/services/configstore/api/variable.go @@ -75,10 +75,8 @@ func (h *VariablesHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if err := json.NewEncoder(w).Encode(variables); err != nil { + if err := httpResponse(w, http.StatusOK, variables); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -118,10 +116,8 @@ func (h *CreateVariableHandler) ServeHTTP(w http.ResponseWriter, r *http.Request return } - if err := json.NewEncoder(w).Encode(variable); err != nil { + if err := httpResponse(w, http.StatusCreated, variable); err != nil { h.log.Errorf("err: %+v", err) - httpError(w, err) - return } } @@ -149,4 +145,7 @@ func (h *DeleteVariableHandler) ServeHTTP(w http.ResponseWriter, r *http.Request if httpError(w, err) { h.log.Errorf("err: %+v", err) } + if err := httpResponse(w, http.StatusNoContent, nil); err != nil { + h.log.Errorf("err: %+v", err) + } }