Skip to content

Commit

Permalink
fix: fix error to status code mapping (#110)
Browse files Browse the repository at this point in the history 
* fix: fix error to status code mapping for CreateAppeals

* chore: logs internal error in handler

* fix: fix error to status code mapping for UpdateApproval

* chore: remove unused errors

* test: add unit tests for ApprovalAction validation

* chore: fix typo in annotation

* fix: log internal error in ListUserRoles

* feat: log error message for invalid argument and failed preconditions errors
  • Loading branch information
@rahmatrhd
rahmatrhd authored Dec 6, 2023
1 parent fa845fb commit 16b3000
Show file tree
Hide file tree
Showing 20 changed files with 428 additions and 373 deletions.
12 changes: 6 additions & 6 deletions api/handler/v1beta1/activity.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ func (s *GRPCServer) GetActivity(ctx context.Context, req *guardianv1beta1.GetAc
if errors.Is(err, activity.ErrNotFound) {
return nil, status.Errorf(codes.NotFound, "activity not found")
}
return nil, status.Errorf(codes.Internal, "failed to get activity: %v", err)
return nil, s.internalError(ctx, "failed to get activity: %v", err)
}

activityProto, err := s.adapter.ToActivityProto(a)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse proto: %v", err)
return nil, s.internalError(ctx, "failed to parse proto: %v", err)
}

return &guardianv1beta1.GetActivityResponse{
Expand All @@ -48,14 +48,14 @@ func (s *GRPCServer) ListActivities(ctx context.Context, req *guardianv1beta1.Li

activities, err := s.activityService.Find(ctx, filter)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to list activities: %v", err)
return nil, s.internalError(ctx, "failed to list activities: %v", err)
}

activityProtos := []*guardianv1beta1.ProviderActivity{}
for _, a := range activities {
activityProto, err := s.adapter.ToActivityProto(a)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse proto: %v", err)
return nil, s.internalError(ctx, "failed to parse proto: %v", err)
}
activityProtos = append(activityProtos, activityProto)
}
Expand All @@ -82,14 +82,14 @@ func (s *GRPCServer) ImportActivities(ctx context.Context, req *guardianv1beta1.

activities, err := s.activityService.Import(ctx, filter)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to import activities: %v", err)
return nil, s.internalError(ctx, "failed to import activities: %v", err)
}

activityProtos := []*guardianv1beta1.ProviderActivity{}
for _, a := range activities {
activity, err := s.adapter.ToActivityProto(a)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse proto: %v", err)
return nil, s.internalError(ctx, "failed to parse proto: %v", err)
}

activityProtos = append(activityProtos, activity)
Expand Down
56 changes: 41 additions & 15 deletions api/handler/v1beta1/appeal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

guardianv1beta1 "github.com/goto/guardian/api/proto/gotocompany/guardian/v1beta1"
"github.com/goto/guardian/core/appeal"
"github.com/goto/guardian/core/provider"
"github.com/goto/guardian/domain"
"golang.org/x/sync/errgroup"
"google.golang.org/grpc/codes"
Expand Down Expand Up @@ -96,21 +97,46 @@ func (s *GRPCServer) CreateAppeal(ctx context.Context, req *guardianv1beta1.Crea

appeals, err := s.adapter.FromCreateAppealProto(req, authenticatedUser)
if err != nil {
return nil, status.Errorf(codes.Internal, "cannot deserialize payload: %v", err)
return nil, s.internalError(ctx, "cannot deserialize payload: %v", err)
}

if err := s.appealService.Create(ctx, appeals); err != nil {
if errors.Is(err, appeal.ErrAppealDuplicate) {
return nil, status.Errorf(codes.AlreadyExists, "appeal already exists: %v", err)
switch {
case errors.Is(err, provider.ErrAppealValidationInvalidAccountType),
errors.Is(err, provider.ErrAppealValidationInvalidRole),
errors.Is(err, provider.ErrAppealValidationDurationNotSpecified),
errors.Is(err, provider.ErrAppealValidationEmptyDuration),
errors.Is(err, provider.ErrAppealValidationInvalidDurationValue),
errors.Is(err, provider.ErrAppealValidationMissingRequiredParameter),
errors.Is(err, provider.ErrAppealValidationMissingRequiredQuestion),
errors.Is(err, appeal.ErrDurationNotAllowed),
errors.Is(err, appeal.ErrCannotCreateAppealForOtherUser):
return nil, s.invalidArgument(ctx, err.Error())
case errors.Is(err, appeal.ErrAppealDuplicate):
s.logger.Error(ctx, err.Error())
return nil, status.Errorf(codes.AlreadyExists, err.Error())
case errors.Is(err, appeal.ErrResourceNotFound),
errors.Is(err, appeal.ErrResourceDeleted),
errors.Is(err, appeal.ErrProviderNotFound),
errors.Is(err, appeal.ErrPolicyNotFound),
errors.Is(err, appeal.ErrInvalidResourceType),
errors.Is(err, appeal.ErrAppealInvalidExtensionDuration),
errors.Is(err, appeal.ErrGrantNotEligibleForExtension),
errors.Is(err, domain.ErrFailedToGetApprovers),
errors.Is(err, domain.ErrApproversNotFound),
errors.Is(err, domain.ErrUnexpectedApproverType),
errors.Is(err, domain.ErrInvalidApproverValue):
return nil, s.failedPrecondition(ctx, err.Error())
default:
return nil, s.internalError(ctx, "failed to create appeal(s): %v", err)
}
return nil, status.Errorf(codes.Internal, "failed to create appeal: %v", err)
}

appealProtos := []*guardianv1beta1.Appeal{}
for _, appeal := range appeals {
appealProto, err := s.adapter.ToAppealProto(appeal)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err)
return nil, s.internalError(ctx, "failed to parse appeal: %v", err)
}
appealProtos = append(appealProtos, appealProto)
}
Expand All @@ -126,9 +152,9 @@ func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppe
a, err := s.appealService.GetByID(ctx, id)
if err != nil {
if errors.As(err, new(appeal.InvalidError)) || errors.Is(err, appeal.ErrAppealIDEmptyParam) {
return nil, status.Errorf(codes.InvalidArgument, err.Error())
return nil, s.invalidArgument(ctx, err.Error())
}
return nil, status.Errorf(codes.Internal, "failed to retrieve appeal: %v", err)
return nil, s.internalError(ctx, "failed to retrieve appeal: %v", err)
}

if a == nil {
Expand All @@ -137,7 +163,7 @@ func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppe

appealProto, err := s.adapter.ToAppealProto(a)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err)
return nil, s.internalError(ctx, "failed to parse appeal: %v", err)
}

return &guardianv1beta1.GetAppealResponse{
Expand All @@ -151,7 +177,7 @@ func (s *GRPCServer) CancelAppeal(ctx context.Context, req *guardianv1beta1.Canc
a, err := s.appealService.Cancel(ctx, id)
if err != nil {
if errors.As(err, new(appeal.InvalidError)) || errors.Is(err, appeal.ErrAppealIDEmptyParam) {
return nil, status.Errorf(codes.InvalidArgument, err.Error())
return nil, s.invalidArgument(ctx, err.Error())
}

switch err {
Expand All @@ -161,15 +187,15 @@ func (s *GRPCServer) CancelAppeal(ctx context.Context, req *guardianv1beta1.Canc
appeal.ErrAppealStatusApproved,
appeal.ErrAppealStatusRejected,
appeal.ErrAppealStatusUnrecognized:
return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %v", err)
return nil, s.invalidArgument(ctx, "unable to process the request: %v", err)
default:
return nil, status.Errorf(codes.Internal, "failed to cancel appeal: %v", err)
return nil, s.internalError(ctx, "failed to cancel appeal: %v", err)
}
}

appealProto, err := s.adapter.ToAppealProto(a)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err)
return nil, s.internalError(ctx, "failed to parse appeal: %v", err)
}

return &guardianv1beta1.CancelAppealResponse{
Expand All @@ -185,15 +211,15 @@ func (s *GRPCServer) listAppeals(ctx context.Context, filters *domain.ListAppeal
eg.Go(func() error {
appealRecords, err := s.appealService.Find(ctx, filters)
if err != nil {
return status.Errorf(codes.Internal, "failed to get appeal list: %s", err)
return s.internalError(ctx, "failed to get appeal list: %s", err)
}
appeals = appealRecords
return nil
})
eg.Go(func() error {
totalRecord, err := s.appealService.GetAppealsTotalCount(ctx, filters)
if err != nil {
return status.Errorf(codes.Internal, "failed to get appeal total count: %s", err)
return s.internalError(ctx, "failed to get appeal total count: %s", err)
}
total = totalRecord
return nil
Expand All @@ -207,7 +233,7 @@ func (s *GRPCServer) listAppeals(ctx context.Context, filters *domain.ListAppeal
for _, a := range appeals {
appealProto, err := s.adapter.ToAppealProto(a)
if err != nil {
return nil, 0, status.Errorf(codes.Internal, "failed to parse appeal: %s", err)
return nil, 0, s.internalError(ctx, "failed to parse appeal: %s", err)
}
appealProtos = append(appealProtos, appealProto)
}
Expand Down
54 changes: 28 additions & 26 deletions api/handler/v1beta1/approval.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,30 +78,32 @@ func (s *GRPCServer) UpdateApproval(ctx context.Context, req *guardianv1beta1.Up
Reason: req.GetAction().GetReason(),
})
if err != nil {
switch err {
case appeal.ErrAppealStatusCanceled,
appeal.ErrAppealStatusApproved,
appeal.ErrAppealStatusRejected,
appeal.ErrAppealStatusUnrecognized,
appeal.ErrApprovalDependencyIsPending,
appeal.ErrApprovalStatusUnrecognized,
appeal.ErrApprovalStatusApproved,
appeal.ErrApprovalStatusRejected,
appeal.ErrApprovalStatusSkipped,
appeal.ErrActionInvalidValue:
return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %v", err)
case appeal.ErrActionForbidden:
switch {
case
errors.Is(err, appeal.ErrInvalidUpdateApprovalParameter),
errors.Is(err, appeal.ErrAppealIDEmptyParam),
errors.Is(err, appeal.ErrActionInvalidValue):
return nil, s.invalidArgument(ctx, err.Error())
case
errors.Is(err, appeal.ErrAppealNotEligibleForApproval),
errors.Is(err, appeal.ErrAppealStatusUnrecognized),
errors.Is(err, appeal.ErrApprovalNotEligibleForAction),
errors.Is(err, appeal.ErrApprovalStatusUnrecognized):
return nil, s.failedPrecondition(ctx, err.Error())
case errors.Is(err, appeal.ErrActionForbidden):
return nil, status.Error(codes.PermissionDenied, "permission denied")
case appeal.ErrApprovalNotFound:
return nil, status.Errorf(codes.NotFound, "approval not found: %v", id)
case
errors.Is(err, appeal.ErrAppealNotFound),
errors.Is(err, appeal.ErrApprovalNotFound):
return nil, status.Errorf(codes.NotFound, err.Error())
default:
return nil, status.Errorf(codes.Internal, "failed to update approval: %v", err)
return nil, s.internalError(ctx, "failed to update approval: %v", err)
}
}

appealProto, err := s.adapter.ToAppealProto(a)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err)
return nil, s.internalError(ctx, "failed to parse appeal: %v", err)
}

return &guardianv1beta1.UpdateApprovalResponse{
Expand All @@ -116,17 +118,17 @@ func (s *GRPCServer) AddApprover(ctx context.Context, req *guardianv1beta1.AddAp
errors.Is(err, appeal.ErrApprovalIDEmptyParam),
errors.Is(err, appeal.ErrApproverEmail),
errors.Is(err, appeal.ErrUnableToAddApprover):
return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %s", err)
return nil, s.invalidArgument(ctx, "unable to process the request: %s", err)
case errors.Is(err, appeal.ErrAppealNotFound),
errors.Is(err, appeal.ErrApprovalNotFound):
return nil, status.Errorf(codes.NotFound, "resource not found: %s", err)
case err != nil:
return nil, status.Errorf(codes.Internal, "failed to add approver: %s", err)
return nil, s.internalError(ctx, "failed to add approver: %s", err)
}

appealProto, err := s.adapter.ToAppealProto(a)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse appeal: %s", err)
return nil, s.internalError(ctx, "failed to parse appeal: %s", err)
}

return &guardianv1beta1.AddApproverResponse{
Expand All @@ -141,17 +143,17 @@ func (s *GRPCServer) DeleteApprover(ctx context.Context, req *guardianv1beta1.De
errors.Is(err, appeal.ErrApprovalIDEmptyParam),
errors.Is(err, appeal.ErrApproverEmail),
errors.Is(err, appeal.ErrUnableToDeleteApprover):
return nil, status.Errorf(codes.InvalidArgument, "unable to process the request: %s", err)
return nil, s.invalidArgument(ctx, "unable to process the request: %s", err)
case errors.Is(err, appeal.ErrAppealNotFound),
errors.Is(err, appeal.ErrApprovalNotFound):
return nil, status.Errorf(codes.NotFound, "resource not found: %s", err)
case err != nil:
return nil, status.Errorf(codes.Internal, "failed to delete approver: %s", err)
return nil, s.internalError(ctx, "failed to delete approver: %s", err)
}

appealProto, err := s.adapter.ToAppealProto(a)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to parse appeal: %s", err)
return nil, s.internalError(ctx, "failed to parse appeal: %s", err)
}

return &guardianv1beta1.DeleteApproverResponse{
Expand All @@ -167,7 +169,7 @@ func (s *GRPCServer) listApprovals(ctx context.Context, filters *domain.ListAppr
eg.Go(func() error {
approvalRecords, err := s.approvalService.ListApprovals(ctx, filters)
if err != nil {
return status.Errorf(codes.Internal, "failed to get approval list: %s", err)
return s.internalError(ctx, "failed to get approval list: %s", err)
}
approvals = approvalRecords
return nil
Expand All @@ -176,7 +178,7 @@ func (s *GRPCServer) listApprovals(ctx context.Context, filters *domain.ListAppr
eg.Go(func() error {
totalRecord, err := s.approvalService.GetApprovalsTotalCount(ctx, filters)
if err != nil {
return status.Errorf(codes.Internal, "failed to get approval list: %v", err)
return s.internalError(ctx, "failed to get approval list: %v", err)
}
total = totalRecord
return nil
Expand All @@ -190,7 +192,7 @@ func (s *GRPCServer) listApprovals(ctx context.Context, filters *domain.ListAppr
for _, a := range approvals {
approvalProto, err := s.adapter.ToApprovalProto(a)
if err != nil {
return nil, 0, status.Errorf(codes.Internal, "failed to parse approval: %v: %s", a.ID, err)
return nil, 0, s.internalError(ctx, "failed to parse approval: %v: %s", a.ID, err)
}
approvalProtos = append(approvalProtos, approvalProto)
}
Expand Down
39 changes: 7 additions & 32 deletions api/handler/v1beta1/approval_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,49 +493,24 @@ func (s *GrpcHandlersSuite) TestUpdateApproval() {
}{

{
"should return invalid error if appeal status already cancelled",
appeal.ErrAppealStatusCanceled,
codes.InvalidArgument,
},
{
"should return invalid error if appeal status already approved",
appeal.ErrAppealStatusApproved,
codes.InvalidArgument,
},
{
"should return invalid error if appeal status already rejected",
appeal.ErrAppealStatusRejected,
codes.InvalidArgument,
"should return invalid error if appeal status is not eligible for approval",
appeal.ErrAppealNotEligibleForApproval,
codes.FailedPrecondition,
},
{
"should return invalid error if appeal status unrecognized",
appeal.ErrAppealStatusUnrecognized,
codes.InvalidArgument,
codes.FailedPrecondition,
},
{
"should return invalid error if approval dependency is still pending",
appeal.ErrApprovalDependencyIsPending,
codes.InvalidArgument,
appeal.ErrApprovalNotEligibleForAction,
codes.FailedPrecondition,
},
{
"should return invalid error if approval status unrecognized",
appeal.ErrApprovalStatusUnrecognized,
codes.InvalidArgument,
},
{
"should return invalid error if approval status already approved",
appeal.ErrApprovalStatusApproved,
codes.InvalidArgument,
},
{
"should return invalid error if approval status already rejected",
appeal.ErrApprovalStatusRejected,
codes.InvalidArgument,
},
{
"should return invalid error if approval status already skipped",
appeal.ErrApprovalStatusSkipped,
codes.InvalidArgument,
codes.FailedPrecondition,
},
{
"should return invalid error if action name is invalid",
Expand Down
Loading

0 comments on commit 16b3000

Please sign in to comment.