fix: send proper error code in case of bad request (#38)

- If the appeal id is empty or invalid in request, server should return 400 bad request instead of 500 internal server error
- fix and add test cases

api/handler/v1beta1/appeal.go

@@ -3,7 +3,6 @@ package v1beta1
 import (
 	"context"
 	"errors"
-
 	guardianv1beta1 "github.com/goto/guardian/api/proto/gotocompany/guardian/v1beta1"
 	"github.com/goto/guardian/core/appeal"
 	"github.com/goto/guardian/domain"
@@ -106,15 +105,20 @@ func (s *GRPCServer) CreateAppeal(ctx context.Context, req *guardianv1beta1.Crea
 
 func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppealRequest) (*guardianv1beta1.GetAppealResponse, error) {
 	id := req.GetId()
-	appeal, err := s.appealService.GetByID(ctx, id)
+
+	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, status.Errorf(codes.Internal, "failed to retrieve appeal: %v", err)
 	}
-	if appeal == nil {
+
+	if a == nil {
 		return nil, status.Errorf(codes.NotFound, "appeal not found: %v", id)
 	}
 
-	appealProto, err := s.adapter.ToAppealProto(appeal)
+	appealProto, err := s.adapter.ToAppealProto(a)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "failed to parse appeal: %v", err)
 	}
@@ -126,8 +130,13 @@ func (s *GRPCServer) GetAppeal(ctx context.Context, req *guardianv1beta1.GetAppe
 
 func (s *GRPCServer) CancelAppeal(ctx context.Context, req *guardianv1beta1.CancelAppealRequest) (*guardianv1beta1.CancelAppealResponse, error) {
 	id := req.GetId()
+
 	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())
+		}
+
 		switch err {
 		case appeal.ErrAppealNotFound:
 			return nil, status.Errorf(codes.NotFound, "appeal not found: %v", id)

api/handler/v1beta1/appeal_test.go

@@ -3,6 +3,7 @@ package v1beta1_test
 import (
 	"context"
 	"errors"
+	"github.com/google/uuid"
 	"time"
 
 	guardianv1beta1 "github.com/goto/guardian/api/proto/gotocompany/guardian/v1beta1"
@@ -557,7 +558,7 @@ func (s *GrpcHandlersSuite) TestGetAppeal() {
 		s.setup()
 		timeNow := time.Now()
 
-		expectedID := "test-appeal-id"
+		expectedID := uuid.New().String()
 		expectedAppeal := &domain.Appeal{
 			ID:         expectedID,
 			ResourceID: "test-resource-id",
@@ -655,7 +656,9 @@ func (s *GrpcHandlersSuite) TestGetAppeal() {
 		s.appealService.EXPECT().GetByID(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).
 			Return(nil, expectedError).Once()
 
-		req := &guardianv1beta1.GetAppealRequest{}
+		req := &guardianv1beta1.GetAppealRequest{
+			Id: uuid.New().String(),
+		}
 		res, err := s.grpcServer.GetAppeal(context.Background(), req)
 
 		s.Equal(codes.Internal, status.Code(err))
@@ -669,7 +672,9 @@ func (s *GrpcHandlersSuite) TestGetAppeal() {
 		s.appealService.EXPECT().GetByID(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).
 			Return(nil, nil).Once()
 
-		req := &guardianv1beta1.GetAppealRequest{}
+		req := &guardianv1beta1.GetAppealRequest{
+			Id: uuid.New().String(),
+		}
 		res, err := s.grpcServer.GetAppeal(context.Background(), req)
 
 		s.Equal(codes.NotFound, status.Code(err))
@@ -688,21 +693,24 @@ func (s *GrpcHandlersSuite) TestGetAppeal() {
 		s.appealService.EXPECT().GetByID(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).
 			Return(invalidAppeal, nil).Once()
 
-		req := &guardianv1beta1.GetAppealRequest{}
+		req := &guardianv1beta1.GetAppealRequest{
+			Id: uuid.New().String(),
+		}
 		res, err := s.grpcServer.GetAppeal(context.Background(), req)
 
 		s.Equal(codes.Internal, status.Code(err))
 		s.Nil(res)
 		s.appealService.AssertExpectations(s.T())
 	})
+
 }
 
 func (s *GrpcHandlersSuite) TestCancelAppeal() {
 	s.Run("should return appeal details on success", func() {
 		s.setup()
 		timeNow := time.Now()
 
-		expectedID := "test-appeal-id"
+		expectedID := uuid.New().String()
 		expectedAppeal := &domain.Appeal{
 			ID:         expectedID,
 			ResourceID: "test-resource-id",
@@ -838,7 +846,9 @@ func (s *GrpcHandlersSuite) TestCancelAppeal() {
 				s.appealService.EXPECT().Cancel(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).
 					Return(nil, tc.expectedError).Once()
 
-				req := &guardianv1beta1.CancelAppealRequest{}
+				req := &guardianv1beta1.CancelAppealRequest{
+					Id: uuid.New().String(),
+				}
 				res, err := s.grpcServer.CancelAppeal(context.Background(), req)
 
 				s.Equal(tc.expectedStatusCode, status.Code(err))
@@ -859,7 +869,9 @@ func (s *GrpcHandlersSuite) TestCancelAppeal() {
 		s.appealService.EXPECT().Cancel(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).
 			Return(invalidAppeal, nil).Once()
 
-		req := &guardianv1beta1.CancelAppealRequest{}
+		req := &guardianv1beta1.CancelAppealRequest{
+			Id: uuid.New().String(),
+		}
 		res, err := s.grpcServer.CancelAppeal(context.Background(), req)
 
 		s.Equal(codes.Internal, status.Code(err))

core/appeal/errors.go

@@ -1,6 +1,9 @@
 package appeal
 
-import "errors"
+import (
+	"errors"
+	"fmt"
+)
 
 var (
 	ErrAppealIDEmptyParam   = errors.New("appeal id is required")
@@ -50,3 +53,11 @@ var (
 	ErrApproverNotFound         = errors.New("approver not found")
 	ErrGrantNotFound            = errors.New("grant not found")
 )
+
+type InvalidError struct {
+	AppealID string
+}
+
+func (ie InvalidError) Error() string {
+	return fmt.Sprintf("invalid appeal id: %s", ie.AppealID)
+}

core/appeal/service.go

@@ -159,6 +159,10 @@ func (s *Service) GetByID(ctx context.Context, id string) (*domain.Appeal, error
 		return nil, ErrAppealIDEmptyParam
 	}
 
+	if !utils.IsValidUUID(id) {
+		return nil, InvalidError{AppealID: id}
+	}
+
 	return s.repo.GetByID(ctx, id)
 }
 
@@ -590,6 +594,14 @@ func (s *Service) Update(ctx context.Context, appeal *domain.Appeal) error {
 }
 
 func (s *Service) Cancel(ctx context.Context, id string) (*domain.Appeal, error) {
+	if id == "" {
+		return nil, ErrAppealIDEmptyParam
+	}
+
+	if !utils.IsValidUUID(id) {
+		return nil, InvalidError{AppealID: id}
+	}
+
 	appeal, err := s.GetByID(ctx, id)
 	if err != nil {
 		return nil, err

core/appeal/service_test.go

@@ -87,14 +87,15 @@ func (s *ServiceTestSuite) TestGetByID() {
 		expectedError := errors.New("repository error")
 		s.mockRepository.EXPECT().GetByID(mock.AnythingOfType("*context.emptyCtx"), mock.Anything).Return(nil, expectedError).Once()
 
-		actualResult, actualError := s.service.GetByID(context.Background(), "1")
+		id := uuid.New().String()
+		actualResult, actualError := s.service.GetByID(context.Background(), id)
 
 		s.Nil(actualResult)
 		s.EqualError(actualError, expectedError.Error())
 	})
 
 	s.Run("should return record on success", func() {
-		expectedID := "1"
+		expectedID := uuid.New().String()
 		expectedResult := &domain.Appeal{
 			ID: expectedID,
 		}
@@ -1430,6 +1431,7 @@ func (s *ServiceTestSuite) TestCreateAppeal__WithAdditionalAppeals() {
 
 func (s *ServiceTestSuite) TestUpdateApproval() {
 	timeNow := time.Now()
+	appealID := uuid.New().String()
 	appeal.TimeNow = func() time.Time {
 		return timeNow
 	}
@@ -1441,23 +1443,23 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 				Action:       "name",
 			},
 			{
-				AppealID: "1",
+				AppealID: appealID,
 				Actor:    "user@email.com",
 				Action:   "name",
 			},
 			{
-				AppealID:     "1",
+				AppealID:     appealID,
 				ApprovalName: "approval_1",
 				Actor:        "invalidemail",
 				Action:       "name",
 			},
 			{
-				AppealID:     "1",
+				AppealID:     appealID,
 				ApprovalName: "approval_1",
 				Action:       "name",
 			},
 			{
-				AppealID:     "1",
+				AppealID:     appealID,
 				ApprovalName: "approval_1",
 				Actor:        "user@email.com",
 			},
@@ -1472,7 +1474,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 	})
 
 	validApprovalActionParam := domain.ApprovalAction{
-		AppealID:     "1",
+		AppealID:     appealID,
 		ApprovalName: "approval_1",
 		Actor:        "user@email.com",
 		Action:       "approve",
@@ -1718,13 +1720,13 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 
 	s.Run("should terminate existing active grant if present", func() {
 		action := domain.ApprovalAction{
-			AppealID:     "1",
+			AppealID:     appealID,
 			ApprovalName: "test-approval-step",
 			Action:       "approve",
 			Actor:        "approver@example.com",
 		}
 		appealDetails := &domain.Appeal{
-			ID:         "1",
+			ID:         appealID,
 			AccountID:  "user@example.com",
 			ResourceID: "1",
 			Role:       "test-role",
@@ -1887,7 +1889,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 			{
 				name: "reject",
 				expectedApprovalAction: domain.ApprovalAction{
-					AppealID:     "1",
+					AppealID:     appealID,
 					ApprovalName: "approval_1",
 					Actor:        "user@email.com",
 					Action:       domain.AppealActionNameReject,
@@ -1962,7 +1964,7 @@ func (s *ServiceTestSuite) TestUpdateApproval() {
 			{
 				name: "reject in the middle step",
 				expectedApprovalAction: domain.ApprovalAction{
-					AppealID:     "1",
+					AppealID:     appealID,
 					ApprovalName: "approval_1",
 					Actor:        user,
 					Action:       domain.AppealActionNameReject,
@@ -2289,9 +2291,26 @@ func (s *ServiceTestSuite) TestGrantAccessToProvider() {
 	})
 }
 
-// func (s *ServiceTestSuite) TestCancel() {
-// 	s.Run("should return error from")
-// }
+func (s *ServiceTestSuite) TestCancel() {
+	s.Run("should return error if appeal id is empty", func() {
+		id := ""
+		expectedErr := appeal.ErrAppealIDEmptyParam
+
+		actualResult, actualErr := s.service.Cancel(context.Background(), id)
+		s.Nil(actualResult)
+		s.EqualError(actualErr, expectedErr.Error())
+	})
+
+	s.Run("should return error if appeal id is invalid", func() {
+		id := "abc"
+		expectedErr := appeal.InvalidError{AppealID: id}
+
+		actualResult, actualErr := s.service.Cancel(context.Background(), id)
+		s.Nil(actualResult)
+		s.EqualError(actualErr, expectedErr.Error())
+	})
+
+}
 
 func (s *ServiceTestSuite) TestAddApprover() {
 	s.Run("should return appeal on success", func() {

utils/utils.go

@@ -1,5 +1,7 @@
 package utils
 
+import "github.com/google/uuid"
+
 func IsInteger(val float64) bool {
 	return val == float64(int(val))
 }
@@ -19,3 +21,9 @@ func MapToSlice(m map[string]string) []string {
 	}
 	return s
 }
+
+// IsValidUUID returns true if uuid is valid
+func IsValidUUID(u string) bool {
+	_, err := uuid.Parse(u)
+	return err == nil
+}