From d1b385f442d9aca917aeac636b20bebc1820c83e Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 7 Oct 2021 16:21:30 +0200 Subject: [PATCH] Handle "not found" errors properly in user- and groupprovider Return a proper NOT_FOUND error instead of INTERNAL when a user or group is not found. --- .../unreleased/handle-user-group-not-found.md | 3 +++ .../services/groupprovider/groupprovider.go | 24 ++++++++++++------- .../services/userprovider/userprovider.go | 20 +++++++++------- tests/integration/grpc/userprovider_test.go | 6 ++--- 4 files changed, 34 insertions(+), 19 deletions(-) create mode 100644 changelog/unreleased/handle-user-group-not-found.md diff --git a/changelog/unreleased/handle-user-group-not-found.md b/changelog/unreleased/handle-user-group-not-found.md new file mode 100644 index 0000000000..5cf9f05dd0 --- /dev/null +++ b/changelog/unreleased/handle-user-group-not-found.md @@ -0,0 +1,3 @@ +Enhancement: Return a proper NOT_FOUND error when a user or group is not found + +https://github.com/cs3org/reva/pull/2282 diff --git a/internal/grpc/services/groupprovider/groupprovider.go b/internal/grpc/services/groupprovider/groupprovider.go index 1fda6e7d05..6fefa4cbe6 100644 --- a/internal/grpc/services/groupprovider/groupprovider.go +++ b/internal/grpc/services/groupprovider/groupprovider.go @@ -103,10 +103,14 @@ func (s *service) Register(ss *grpc.Server) { func (s *service) GetGroup(ctx context.Context, req *grouppb.GetGroupRequest) (*grouppb.GetGroupResponse, error) { group, err := s.groupmgr.GetGroup(ctx, req.GroupId) if err != nil { - err = errors.Wrap(err, "groupprovidersvc: error getting group") - return &grouppb.GetGroupResponse{ - Status: status.NewInternal(ctx, err, "error getting group"), - }, nil + res := &grouppb.GetGroupResponse{} + if _, ok := err.(errtypes.NotFound); ok { + res.Status = status.NewNotFound(ctx, "group not found") + } else { + err = errors.Wrap(err, "groupprovidersvc: error getting group") + res.Status = status.NewInternal(ctx, err, "error getting group") + } + return res, nil } return &grouppb.GetGroupResponse{ @@ -118,10 +122,14 @@ func (s *service) GetGroup(ctx context.Context, req *grouppb.GetGroupRequest) (* func (s *service) GetGroupByClaim(ctx context.Context, req *grouppb.GetGroupByClaimRequest) (*grouppb.GetGroupByClaimResponse, error) { group, err := s.groupmgr.GetGroupByClaim(ctx, req.Claim, req.Value) if err != nil { - err = errors.Wrap(err, "groupprovidersvc: error getting group by claim") - return &grouppb.GetGroupByClaimResponse{ - Status: status.NewInternal(ctx, err, "error getting group by claim"), - }, nil + res := &grouppb.GetGroupByClaimResponse{} + if _, ok := err.(errtypes.NotFound); ok { + res.Status = status.NewNotFound(ctx, fmt.Sprintf("group not found %s %s", req.Claim, req.Value)) + } else { + err = errors.Wrap(err, "groupprovidersvc: error getting group by claim") + res.Status = status.NewInternal(ctx, err, "error getting group by claim") + } + return res, nil } return &grouppb.GetGroupByClaimResponse{ diff --git a/internal/grpc/services/userprovider/userprovider.go b/internal/grpc/services/userprovider/userprovider.go index 66f91a1e1c..cd82bf3304 100644 --- a/internal/grpc/services/userprovider/userprovider.go +++ b/internal/grpc/services/userprovider/userprovider.go @@ -127,10 +127,12 @@ func (s *service) Register(ss *grpc.Server) { func (s *service) GetUser(ctx context.Context, req *userpb.GetUserRequest) (*userpb.GetUserResponse, error) { user, err := s.usermgr.GetUser(ctx, req.UserId) if err != nil { - // TODO(labkode): check for not found. - err = errors.Wrap(err, "userprovidersvc: error getting user") - res := &userpb.GetUserResponse{ - Status: status.NewInternal(ctx, err, "error getting user"), + res := &userpb.GetUserResponse{} + if _, ok := err.(errtypes.NotFound); ok { + res.Status = status.NewNotFound(ctx, "user not found") + } else { + err = errors.Wrap(err, "userprovidersvc: error getting user") + res.Status = status.NewInternal(ctx, err, "error getting user") } return res, nil } @@ -145,10 +147,12 @@ func (s *service) GetUser(ctx context.Context, req *userpb.GetUserRequest) (*use func (s *service) GetUserByClaim(ctx context.Context, req *userpb.GetUserByClaimRequest) (*userpb.GetUserByClaimResponse, error) { user, err := s.usermgr.GetUserByClaim(ctx, req.Claim, req.Value) if err != nil { - // TODO(labkode): check for not found. - err = errors.Wrap(err, "userprovidersvc: error getting user by claim") - res := &userpb.GetUserByClaimResponse{ - Status: status.NewInternal(ctx, err, "error getting user by claim"), + res := &userpb.GetUserByClaimResponse{} + if _, ok := err.(errtypes.NotFound); ok { + res.Status = status.NewNotFound(ctx, fmt.Sprintf("user not found %s %s", req.Claim, req.Value)) + } else { + err = errors.Wrap(err, "userprovidersvc: error getting user by claim") + res.Status = status.NewInternal(ctx, err, "error getting user by claim") } return res, nil } diff --git a/tests/integration/grpc/userprovider_test.go b/tests/integration/grpc/userprovider_test.go index c0252e457b..1de51e683f 100644 --- a/tests/integration/grpc/userprovider_test.go +++ b/tests/integration/grpc/userprovider_test.go @@ -134,7 +134,7 @@ var _ = Describe("user providers", func() { }, want: &userpb.GetUserResponse{ Status: &rpc.Status{ - Code: 15, + Code: 6, }, }, }, @@ -147,7 +147,7 @@ var _ = Describe("user providers", func() { }, want: &userpb.GetUserResponse{ Status: &rpc.Status{ - Code: 15, + Code: 6, }, }, }, @@ -160,7 +160,7 @@ var _ = Describe("user providers", func() { }, want: &userpb.GetUserResponse{ Status: &rpc.Status{ - Code: 15, + Code: 6, }, }, },