Skip to content

Commit

Permalink
*: Improve error handling
Browse files Browse the repository at this point in the history 
* Unify all custom error kinds to a DetailedError that will wrap an
  error and can have different Kinds and optional code and message.
* The http handlers will use the first DetailedError available in the
  error chain and generate a json response body containing the code and
  the user message. The wrapped error is internal and is not sent in the
  response.
  If no detailed error is available in the chain a generic internal
  server error will be returned.
* Add a RemoteError type that will be created from remote services calls
  (runservice, configstore). It's similar to the DetailedError but a
  different type to not propagate to the caller response and it'll not
  contain any wrapped error.
* Gateway: when we call a remote service, by default, we'll create a
  DetailedError using the RemoteError Kind (omitting the code and the
  message that usually must not be propagated).
  This is done for all the remote service calls as a starting point, in
  future, if this default behavior is not the right one for a specific
  remote service call, a new detailed error with a different kind and/or
  augmented with the calling service error codes and user messages could
  be created.
  • Loading branch information
@sgotti
sgotti committed Feb 24, 2022
1 parent a199749 commit efc3874
Show file tree
Hide file tree
Showing 63 changed files with 1,131 additions and 1,366 deletions.
4 changes: 2 additions & 2 deletions internal/datamanager/data.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ func (d *DataManager) Read(dataType, id string) (io.Reader, error) {
var matchingDataFileID string
// get the matching data file for the action entry ID
if len(curFiles[dataType]) == 0 {
return nil, util.NewErrNotExist(errors.Errorf("datatype %q doesn't exists", dataType))
return nil, util.NewDetailedError(util.ErrNotExist, errors.Errorf("datatype %q doesn't exists", dataType))
}

matchingDataFileID = curFiles[dataType][0].ID
Expand All @@ -526,7 +526,7 @@ func (d *DataManager) Read(dataType, id string) (io.Reader, error) {

pos, ok := dataFileIndex.Index[id]
if !ok {
return nil, util.NewErrNotExist(errors.Errorf("datatype %q, id %q doesn't exists", dataType, id))
return nil, util.NewDetailedError(util.ErrNotExist, errors.Errorf("datatype %q, id %q doesn't exists", dataType, id))
}

dataf, err := d.ost.ReadObject(d.DataFilePath(dataType, matchingDataFileID))
Expand Down
8 changes: 4 additions & 4 deletions internal/datamanager/datamanager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -559,8 +559,8 @@ func TestReadObject(t *testing.T) {

// should not exists
_, _, err = dm.ReadObject("datatype01", "object1", nil)
if !util.IsNotExist(err) {
t.Fatalf("expected err %v, got: %v", &util.ErrNotExist{}, err)
if !util.DetailedErrorIs(err, util.ErrNotExist) {
t.Fatalf("expected detailed error with kind %s, got: %v", util.ErrNotExist, err)
}
// should exist
_, _, err = dm.ReadObject("datatype01", "object19", nil)
Expand All @@ -583,8 +583,8 @@ func TestReadObject(t *testing.T) {

// should not exists
_, _, err = dm.ReadObject("datatype01", "object1", nil)
if !util.IsNotExist(err) {
t.Fatalf("expected err %v, got: %v", &util.ErrNotExist{}, err)
if !util.DetailedErrorIs(err, util.ErrNotExist) {
t.Fatalf("expected detailed error with kind %s, got: %v", util.ErrNotExist, err)
}
// should exist
_, _, err = dm.ReadObject("datatype01", "object19", nil)
Expand Down
2 changes: 1 addition & 1 deletion internal/datamanager/wal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func (d *DataManager) ReadObject(dataType, id string, cgNames []string) (io.Read
}
}
}
return nil, nil, util.NewErrNotExist(errors.Errorf("no datatype %q, id %q in wal %s", dataType, id, walseq))
return nil, nil, util.NewDetailedError(util.ErrNotExist, errors.Errorf("no datatype %q, id %q in wal %s", dataType, id, walseq))
}

f, err := d.Read(dataType, id)
Expand Down
18 changes: 3 additions & 15 deletions internal/gitsources/agolagit/agolagit.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (

gitsource "agola.io/agola/internal/gitsources"
"agola.io/agola/internal/services/types"
errors "golang.org/x/xerrors"
"agola.io/agola/internal/util"
)

var (
Expand Down Expand Up @@ -96,20 +96,8 @@ func (c *Client) getResponse(method, path string, query url.Values, header http.
return nil, err
}

if resp.StatusCode/100 != 2 {
defer resp.Body.Close()
data, err := ioutil.ReadAll(resp.Body)
if err != nil {
return nil, err
}

if len(data) <= 1 {
return resp, errors.New(resp.Status)
}

// TODO(sgotti) use a json error response

return resp, errors.New(string(data))
if err := util.ErrFromRemote(resp); err != nil {
return resp, err
}

return resp, nil
Expand Down
8 changes: 4 additions & 4 deletions internal/objectstorage/objectstorage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ func (e *ErrNotExist) Error() string {
return e.err.Error()
}

func (*ErrNotExist) Is(err error) bool {
_, ok := err.(*ErrNotExist)
return ok
func (e *ErrNotExist) Unwrap() error {
return e.err
}

func IsNotExist(err error) bool {
return errors.Is(err, &ErrNotExist{})
var e *ErrNotExist
return errors.As(err, &e)
}

type ReadSeekCloser interface {
Expand Down
6 changes: 3 additions & 3 deletions internal/services/configstore/action/maintenance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ func (h *ActionHandler) MaintenanceMode(ctx context.Context, enable bool) error
}

if enable && len(resp.Kvs) > 0 {
return util.NewErrBadRequest(errors.Errorf("maintenance mode already enabled"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("maintenance mode already enabled"))
}
if !enable && len(resp.Kvs) == 0 {
return util.NewErrBadRequest(errors.Errorf("maintenance mode already disabled"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("maintenance mode already disabled"))
}

if enable {
Expand Down Expand Up @@ -67,7 +67,7 @@ func (h *ActionHandler) Export(ctx context.Context, w io.Writer) error {

func (h *ActionHandler) Import(ctx context.Context, r io.Reader) error {
if !h.maintenanceMode {
return util.NewErrBadRequest(errors.Errorf("not in maintenance mode"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("not in maintenance mode"))
}
return h.dm.Import(ctx, r)
}
26 changes: 13 additions & 13 deletions internal/services/configstore/action/org.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (h *ActionHandler) GetOrgMembers(ctx context.Context, orgRef string) ([]*Or
return err
}
if org == nil {
return util.NewErrNotExist(errors.Errorf("org %q doesn't exist", orgRef))
return util.NewDetailedError(util.ErrNotExist, errors.Errorf("org %q doesn't exist", orgRef))
}

orgUsers, err = h.readDB.GetOrgUsers(tx, org.ID)
Expand All @@ -71,13 +71,13 @@ func (h *ActionHandler) GetOrgMembers(ctx context.Context, orgRef string) ([]*Or

func (h *ActionHandler) CreateOrg(ctx context.Context, org *types.Organization) (*types.Organization, error) {
if org.Name == "" {
return nil, util.NewErrBadRequest(errors.Errorf("organization name required"))
return nil, util.NewDetailedError(util.ErrBadRequest, errors.Errorf("organization name required"))
}
if !util.ValidateName(org.Name) {
return nil, util.NewErrBadRequest(errors.Errorf("invalid organization name %q", org.Name))
return nil, util.NewDetailedError(util.ErrBadRequest, errors.Errorf("invalid organization name %q", org.Name))
}
if !types.IsValidVisibility(org.Visibility) {
return nil, util.NewErrBadRequest(errors.Errorf("invalid organization visibility"))
return nil, util.NewDetailedError(util.ErrBadRequest, errors.Errorf("invalid organization visibility"))
}

var cgt *datamanager.ChangeGroupsUpdateToken
Expand All @@ -98,7 +98,7 @@ func (h *ActionHandler) CreateOrg(ctx context.Context, org *types.Organization)
return err
}
if o != nil {
return util.NewErrBadRequest(errors.Errorf("org %q already exists", o.Name))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("org %q already exists", o.Name))
}

if org.CreatorUserID != "" {
Expand All @@ -107,7 +107,7 @@ func (h *ActionHandler) CreateOrg(ctx context.Context, org *types.Organization)
return err
}
if user == nil {
return util.NewErrBadRequest(errors.Errorf("creator user %q doesn't exist", org.CreatorUserID))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("creator user %q doesn't exist", org.CreatorUserID))
}
}

Expand Down Expand Up @@ -190,7 +190,7 @@ func (h *ActionHandler) DeleteOrg(ctx context.Context, orgRef string) error {
return err
}
if org == nil {
return util.NewErrBadRequest(errors.Errorf("org %q doesn't exist", orgRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("org %q doesn't exist", orgRef))
}

// changegroup is the org id
Expand Down Expand Up @@ -223,7 +223,7 @@ func (h *ActionHandler) DeleteOrg(ctx context.Context, orgRef string) error {
// TODO(sgotti) handle invitation when implemented
func (h *ActionHandler) AddOrgMember(ctx context.Context, orgRef, userRef string, role types.MemberRole) (*types.OrganizationMember, error) {
if !types.IsValidMemberRole(role) {
return nil, util.NewErrBadRequest(errors.Errorf("invalid role %q", role))
return nil, util.NewDetailedError(util.ErrBadRequest, errors.Errorf("invalid role %q", role))
}

var org *types.Organization
Expand All @@ -240,15 +240,15 @@ func (h *ActionHandler) AddOrgMember(ctx context.Context, orgRef, userRef string
return err
}
if org == nil {
return util.NewErrBadRequest(errors.Errorf("org %q doesn't exists", orgRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("org %q doesn't exists", orgRef))
}
// check existing user
user, err = h.readDB.GetUser(tx, userRef)
if err != nil {
return err
}
if user == nil {
return util.NewErrBadRequest(errors.Errorf("user %q doesn't exists", userRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("user %q doesn't exists", userRef))
}

// fetch org member if it already exist
Expand Down Expand Up @@ -316,15 +316,15 @@ func (h *ActionHandler) RemoveOrgMember(ctx context.Context, orgRef, userRef str
return err
}
if org == nil {
return util.NewErrBadRequest(errors.Errorf("org %q doesn't exists", orgRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("org %q doesn't exists", orgRef))
}
// check existing user
user, err = h.readDB.GetUser(tx, userRef)
if err != nil {
return err
}
if user == nil {
return util.NewErrBadRequest(errors.Errorf("user %q doesn't exists", userRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("user %q doesn't exists", userRef))
}

// check that org member exists
Expand All @@ -333,7 +333,7 @@ func (h *ActionHandler) RemoveOrgMember(ctx context.Context, orgRef, userRef str
return err
}
if orgmember == nil {
return util.NewErrBadRequest(errors.Errorf("orgmember for org %q, user %q doesn't exists", orgRef, userRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("orgmember for org %q, user %q doesn't exists", orgRef, userRef))
}

cgNames := []string{util.EncodeSha256Hex(fmt.Sprintf("orgmember-%s-%s", org.ID, user.ID))}
Expand Down
50 changes: 25 additions & 25 deletions internal/services/configstore/action/project.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,35 +30,35 @@ import (

func (h *ActionHandler) ValidateProject(ctx context.Context, project *types.Project) error {
if project.Name == "" {
return util.NewErrBadRequest(errors.Errorf("project name required"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project name required"))
}
if !util.ValidateName(project.Name) {
return util.NewErrBadRequest(errors.Errorf("invalid project name %q", project.Name))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("invalid project name %q", project.Name))
}
if project.Parent.ID == "" {
return util.NewErrBadRequest(errors.Errorf("project parent id required"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project parent id required"))
}
if project.Parent.Type != types.ConfigTypeProjectGroup {
return util.NewErrBadRequest(errors.Errorf("invalid project parent type %q", project.Parent.Type))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("invalid project parent type %q", project.Parent.Type))
}
if !types.IsValidVisibility(project.Visibility) {
return util.NewErrBadRequest(errors.Errorf("invalid project visibility"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("invalid project visibility"))
}
if !types.IsValidRemoteRepositoryConfigType(project.RemoteRepositoryConfigType) {
return util.NewErrBadRequest(errors.Errorf("invalid project remote repository config type %q", project.RemoteRepositoryConfigType))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("invalid project remote repository config type %q", project.RemoteRepositoryConfigType))
}
if project.RemoteRepositoryConfigType == types.RemoteRepositoryConfigTypeRemoteSource {
if project.RemoteSourceID == "" {
return util.NewErrBadRequest(errors.Errorf("empty remote source id"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("empty remote source id"))
}
if project.LinkedAccountID == "" {
return util.NewErrBadRequest(errors.Errorf("empty linked account id"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("empty linked account id"))
}
if project.RepositoryID == "" {
return util.NewErrBadRequest(errors.Errorf("empty remote repository id"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("empty remote repository id"))
}
if project.RepositoryPath == "" {
return util.NewErrBadRequest(errors.Errorf("empty remote repository path"))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("empty remote repository path"))
}
}
return nil
Expand All @@ -76,7 +76,7 @@ func (h *ActionHandler) GetProject(ctx context.Context, projectRef string) (*typ
}

if project == nil {
return nil, util.NewErrNotExist(errors.Errorf("project %q doesn't exist", projectRef))
return nil, util.NewDetailedError(util.ErrNotExist, errors.Errorf("project %q doesn't exist", projectRef))
}

return project, nil
Expand All @@ -97,7 +97,7 @@ func (h *ActionHandler) CreateProject(ctx context.Context, project *types.Projec
return err
}
if group == nil {
return util.NewErrBadRequest(errors.Errorf("project group with id %q doesn't exist", project.Parent.ID))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project group with id %q doesn't exist", project.Parent.ID))
}
project.Parent.ID = group.ID

Expand All @@ -121,7 +121,7 @@ func (h *ActionHandler) CreateProject(ctx context.Context, project *types.Projec
return err
}
if p != nil {
return util.NewErrBadRequest(errors.Errorf("project with name %q, path %q already exists", p.Name, pp))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project with name %q, path %q already exists", p.Name, pp))
}

if project.RemoteRepositoryConfigType == types.RemoteRepositoryConfigTypeRemoteSource {
Expand All @@ -131,14 +131,14 @@ func (h *ActionHandler) CreateProject(ctx context.Context, project *types.Projec
return errors.Errorf("failed to get user with linked account id %q: %w", project.LinkedAccountID, err)
}
if user == nil {
return util.NewErrBadRequest(errors.Errorf("user for linked account %q doesn't exist", project.LinkedAccountID))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("user for linked account %q doesn't exist", project.LinkedAccountID))
}
la, ok := user.LinkedAccounts[project.LinkedAccountID]
if !ok {
return util.NewErrBadRequest(errors.Errorf("linked account id %q for user %q doesn't exist", project.LinkedAccountID, user.Name))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("linked account id %q for user %q doesn't exist", project.LinkedAccountID, user.Name))
}
if la.RemoteSourceID != project.RemoteSourceID {
return util.NewErrBadRequest(errors.Errorf("linked account id %q remote source %q different than project remote source %q", project.LinkedAccountID, la.RemoteSourceID, project.RemoteSourceID))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("linked account id %q remote source %q different than project remote source %q", project.LinkedAccountID, la.RemoteSourceID, project.RemoteSourceID))
}
}

Expand Down Expand Up @@ -193,11 +193,11 @@ func (h *ActionHandler) UpdateProject(ctx context.Context, req *UpdateProjectReq
return err
}
if p == nil {
return util.NewErrBadRequest(errors.Errorf("project with ref %q doesn't exist", req.ProjectRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project with ref %q doesn't exist", req.ProjectRef))
}
// check that the project.ID matches
if p.ID != req.Project.ID {
return util.NewErrBadRequest(errors.Errorf("project with ref %q has a different id", req.ProjectRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project with ref %q has a different id", req.ProjectRef))
}

// check parent project group exists
Expand All @@ -206,7 +206,7 @@ func (h *ActionHandler) UpdateProject(ctx context.Context, req *UpdateProjectReq
return err
}
if group == nil {
return util.NewErrBadRequest(errors.Errorf("project group with id %q doesn't exist", req.Project.Parent.ID))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project group with id %q doesn't exist", req.Project.Parent.ID))
}
req.Project.Parent.ID = group.ID

Expand All @@ -223,7 +223,7 @@ func (h *ActionHandler) UpdateProject(ctx context.Context, req *UpdateProjectReq
return err
}
if ap != nil {
return util.NewErrBadRequest(errors.Errorf("project with name %q, path %q already exists", req.Project.Name, pp))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project with name %q, path %q already exists", req.Project.Name, pp))
}
}

Expand All @@ -239,7 +239,7 @@ func (h *ActionHandler) UpdateProject(ctx context.Context, req *UpdateProjectReq
return err
}
if curGroup == nil {
return util.NewErrBadRequest(errors.Errorf("project group with id %q doesn't exist", p.Parent.ID))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project group with id %q doesn't exist", p.Parent.ID))
}
curGroupPath, err := h.readDB.GetProjectGroupPath(tx, curGroup)
if err != nil {
Expand All @@ -262,14 +262,14 @@ func (h *ActionHandler) UpdateProject(ctx context.Context, req *UpdateProjectReq
return errors.Errorf("failed to get user with linked account id %q: %w", req.Project.LinkedAccountID, err)
}
if user == nil {
return util.NewErrBadRequest(errors.Errorf("user for linked account %q doesn't exist", req.Project.LinkedAccountID))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("user for linked account %q doesn't exist", req.Project.LinkedAccountID))
}
la, ok := user.LinkedAccounts[req.Project.LinkedAccountID]
if !ok {
return util.NewErrBadRequest(errors.Errorf("linked account id %q for user %q doesn't exist", req.Project.LinkedAccountID, user.Name))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("linked account id %q for user %q doesn't exist", req.Project.LinkedAccountID, user.Name))
}
if la.RemoteSourceID != req.Project.RemoteSourceID {
return util.NewErrBadRequest(errors.Errorf("linked account id %q remote source %q different than project remote source %q", req.Project.LinkedAccountID, la.RemoteSourceID, req.Project.RemoteSourceID))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("linked account id %q remote source %q different than project remote source %q", req.Project.LinkedAccountID, la.RemoteSourceID, req.Project.RemoteSourceID))
}
}

Expand Down Expand Up @@ -311,7 +311,7 @@ func (h *ActionHandler) DeleteProject(ctx context.Context, projectRef string) er
return err
}
if project == nil {
return util.NewErrBadRequest(errors.Errorf("project %q doesn't exist", projectRef))
return util.NewDetailedError(util.ErrBadRequest, errors.Errorf("project %q doesn't exist", projectRef))
}

// changegroup is the project id.
Expand Down
Loading

0 comments on commit efc3874

Please sign in to comment.