Skip to content

Commit

Permalink
fix: use non pointer Error
Browse files Browse the repository at this point in the history
  • Loading branch information
@fschade
fschade committed Apr 11, 2024
1 parent 32c8955 commit d42e295
Show file tree
Hide file tree
Showing 8 changed files with 102 additions and 55 deletions.
8 changes: 4 additions & 4 deletions services/graph/pkg/errorcode/cs3.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import (
// and a unified error handling within the application is necessary.
func FromCS3Status(status *cs3rpc.Status, inerr error, ignore ...cs3rpc.Code) error {
if inerr != nil {
return &Error{msg: inerr.Error(), errorCode: GeneralException}
return Error{msg: inerr.Error(), errorCode: GeneralException}
}

err := &Error{errorCode: GeneralException, msg: "unspecified error has occurred"}
err := Error{errorCode: GeneralException, msg: "unspecified error has occurred"}

if status != nil {
err.msg = status.GetMessage()
Expand All @@ -33,7 +33,7 @@ func FromCS3Status(status *cs3rpc.Status, inerr error, ignore ...cs3rpc.Code) er
case slices.Contains(ignore, status.GetCode()):
fallthrough
case code == cs3rpc.Code_CODE_OK:
err = nil
return nil
case code == cs3rpc.Code_CODE_NOT_FOUND:
err.errorCode = ItemNotFound
case code == cs3rpc.Code_CODE_PERMISSION_DENIED:
Expand Down Expand Up @@ -61,7 +61,7 @@ func FromCS3Status(status *cs3rpc.Status, inerr error, ignore ...cs3rpc.Code) er
return err
}

// FromStat transforms a *provider.StatResponse object and an error into an *Error.
// FromStat transforms a *provider.StatResponse object and an error into an Error.
//
// It takes a stat of type *provider.StatResponse, an error, and a variadic parameter of type cs3rpc.Code.
// It invokes the FromCS3Status function with the StatResponse Status and the ignore codes.
Expand Down
57 changes: 28 additions & 29 deletions services/graph/pkg/errorcode/cs3_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,45 +8,44 @@ import (
cs3rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"

"github.com/owncloud/ocis/v2/ocis-pkg/conversions"
"github.com/owncloud/ocis/v2/services/graph/pkg/errorcode"
)

func TestFromCS3Status(t *testing.T) {
var tests = []struct {
status *cs3rpc.Status
err error
ignore []cs3rpc.Code
result *errorcode.Error
status *cs3rpc.Status
err error
ignore []cs3rpc.Code
expected error
}{
{nil, nil, nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "unspecified error has occurred"))},
{nil, errors.New("test error"), nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "test error"))},
{nil, nil, nil, errorcode.New(errorcode.GeneralException, "unspecified error has occurred")},
{nil, errors.New("test error"), nil, errorcode.New(errorcode.GeneralException, "test error")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_OK}, nil, nil, nil},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_NOT_FOUND}, nil, []cs3rpc.Code{cs3rpc.Code_CODE_NOT_FOUND}, nil},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_PERMISSION_DENIED}, nil, []cs3rpc.Code{cs3rpc.Code_CODE_NOT_FOUND, cs3rpc.Code_CODE_PERMISSION_DENIED}, nil},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_NOT_FOUND, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.ItemNotFound, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_PERMISSION_DENIED, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.AccessDenied, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_UNAUTHENTICATED, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.Unauthenticated, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_INVALID_ARGUMENT, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.InvalidRequest, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_ALREADY_EXISTS, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.NameAlreadyExists, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_FAILED_PRECONDITION, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.InvalidRequest, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_UNIMPLEMENTED, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.NotSupported, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_INVALID, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_CANCELLED, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_UNKNOWN, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_RESOURCE_EXHAUSTED, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_ABORTED, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_OUT_OF_RANGE, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.InvalidRange, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_INTERNAL, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_UNAVAILABLE, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.ServiceNotAvailable, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_REDIRECTION, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_INSUFFICIENT_STORAGE, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.QuotaLimitReached, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_LOCKED, Message: "msg"}, nil, nil, conversions.ToPointer(errorcode.New(errorcode.ItemIsLocked, "msg"))},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_NOT_FOUND, Message: "msg"}, nil, nil, errorcode.New(errorcode.ItemNotFound, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_PERMISSION_DENIED, Message: "msg"}, nil, nil, errorcode.New(errorcode.AccessDenied, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_UNAUTHENTICATED, Message: "msg"}, nil, nil, errorcode.New(errorcode.Unauthenticated, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_INVALID_ARGUMENT, Message: "msg"}, nil, nil, errorcode.New(errorcode.InvalidRequest, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_ALREADY_EXISTS, Message: "msg"}, nil, nil, errorcode.New(errorcode.NameAlreadyExists, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_FAILED_PRECONDITION, Message: "msg"}, nil, nil, errorcode.New(errorcode.InvalidRequest, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_UNIMPLEMENTED, Message: "msg"}, nil, nil, errorcode.New(errorcode.NotSupported, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_INVALID, Message: "msg"}, nil, nil, errorcode.New(errorcode.GeneralException, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_CANCELLED, Message: "msg"}, nil, nil, errorcode.New(errorcode.GeneralException, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_UNKNOWN, Message: "msg"}, nil, nil, errorcode.New(errorcode.GeneralException, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_RESOURCE_EXHAUSTED, Message: "msg"}, nil, nil, errorcode.New(errorcode.GeneralException, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_ABORTED, Message: "msg"}, nil, nil, errorcode.New(errorcode.GeneralException, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_OUT_OF_RANGE, Message: "msg"}, nil, nil, errorcode.New(errorcode.InvalidRange, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_INTERNAL, Message: "msg"}, nil, nil, errorcode.New(errorcode.GeneralException, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_UNAVAILABLE, Message: "msg"}, nil, nil, errorcode.New(errorcode.ServiceNotAvailable, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_REDIRECTION, Message: "msg"}, nil, nil, errorcode.New(errorcode.GeneralException, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_INSUFFICIENT_STORAGE, Message: "msg"}, nil, nil, errorcode.New(errorcode.QuotaLimitReached, "msg")},
{&cs3rpc.Status{Code: cs3rpc.Code_CODE_LOCKED, Message: "msg"}, nil, nil, errorcode.New(errorcode.ItemIsLocked, "msg")},
}

for _, test := range tests {
if output := errorcode.FromCS3Status(test.status, test.err, test.ignore...); !reflect.DeepEqual(output, test.result) {
t.Error("Test Failed: {} expected, received: {}", test.result, output)
if got := errorcode.FromCS3Status(test.status, test.err, test.ignore...); !reflect.DeepEqual(got, test.expected) {
t.Error("Test Failed: {} expected, received: {}", test.expected, got)
}
}
}
Expand All @@ -55,9 +54,9 @@ func TestFromStat(t *testing.T) {
var tests = []struct {
stat *provider.StatResponse
err error
result *errorcode.Error
result error
}{
{nil, errors.New("some error"), conversions.ToPointer(errorcode.New(errorcode.GeneralException, "some error"))},
{nil, errors.New("some error"), errorcode.New(errorcode.GeneralException, "some error")},
{&provider.StatResponse{Status: &cs3rpc.Status{Code: cs3rpc.Code_CODE_OK}}, nil, nil},
}

Expand Down
5 changes: 3 additions & 2 deletions services/graph/pkg/errorcode/errorcode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ func RenderError(w http.ResponseWriter, r *http.Request, err error) {
var errcode Error
if errors.As(err, &errcode) {
errcode.Render(w, r)
} else {
GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
return
}

GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
}
45 changes: 45 additions & 0 deletions services/graph/pkg/errorcode/errorcode_test.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package errorcode_test

import (
"net/http"
"net/http/httptest"
"testing"

"github.com/stretchr/testify/require"

"github.com/owncloud/ocis/v2/services/graph/pkg/errorcode"
)

type customErr struct{}

func (customErr) Error() string {
return "some error"
}

func TestRenderError(t *testing.T) {
t.Parallel()

t.Run("errorcode.Error value error", func(t *testing.T) {
r := httptest.NewRequest("GET", "/", nil)
w := httptest.NewRecorder()
err := errorcode.New(errorcode.ItemNotFound, "test error")
errorcode.RenderError(w, r, err)
require.Equal(t, http.StatusNotFound, w.Code)
})

t.Run("errorcode.Error zero value error", func(t *testing.T) {
r := httptest.NewRequest("GET", "/", nil)
w := httptest.NewRecorder()
var err errorcode.Error
errorcode.RenderError(w, r, err)
require.Equal(t, http.StatusForbidden, w.Code)
})

t.Run("custom error", func(t *testing.T) {
r := httptest.NewRequest("GET", "/", nil)
w := httptest.NewRecorder()
var err customErr
errorcode.RenderError(w, r, err)
require.Equal(t, http.StatusInternalServerError, w.Code)
})
}
8 changes: 4 additions & 4 deletions services/graph/pkg/service/v0/api_driveitem_permissions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
libregraph "github.com/owncloud/libre-graph-api-go"
"github.com/stretchr/testify/mock"
"github.com/tidwall/gjson"
"google.golang.org/grpc"

"github.com/owncloud/ocis/v2/ocis-pkg/log"
"github.com/owncloud/ocis/v2/services/graph/mocks"
"github.com/owncloud/ocis/v2/services/graph/pkg/config/defaults"
Expand All @@ -35,9 +39,6 @@ import (
"github.com/owncloud/ocis/v2/services/graph/pkg/linktype"
svc "github.com/owncloud/ocis/v2/services/graph/pkg/service/v0"
"github.com/owncloud/ocis/v2/services/graph/pkg/unifiedrole"
"github.com/stretchr/testify/mock"
"github.com/tidwall/gjson"
"google.golang.org/grpc"
)

var _ = Describe("DriveItemPermissionsService", func() {
Expand Down Expand Up @@ -649,7 +650,6 @@ var _ = Describe("DriveItemPermissionsService", func() {
},
},
}

})
It("fails when no share is found", func() {
getShareMockResponse.Share = nil
Expand Down
3 changes: 2 additions & 1 deletion services/graph/pkg/service/v0/api_drives_drive_item_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/cs3org/reva/v2/pkg/rgrpc/status"
"github.com/cs3org/reva/v2/pkg/storagespace"
cs3mocks "github.com/cs3org/reva/v2/tests/cs3mocks/mocks"

"github.com/owncloud/ocis/v2/ocis-pkg/log"
"github.com/owncloud/ocis/v2/services/graph/mocks"
"github.com/owncloud/ocis/v2/services/graph/pkg/errorcode"
Expand Down Expand Up @@ -377,7 +378,7 @@ var _ = Describe("DrivesDriveItemService", func() {
Return(&collaborationv1beta1.GetReceivedShareResponse{}, errors.New("listing shares failed"))

err := drivesDriveItemService.UnmountShare(context.Background(), storageprovider.ResourceId{})
Expect(err).To(MatchError(&expectedError))
Expect(err).To(MatchError(expectedError))
})

It("uses the correct filters to get the shares", func() {
Expand Down
3 changes: 2 additions & 1 deletion services/graph/pkg/service/v0/base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ import (
"github.com/cs3org/reva/v2/pkg/storagespace"
"github.com/cs3org/reva/v2/pkg/utils"
libregraph "github.com/owncloud/libre-graph-api-go"
"google.golang.org/protobuf/types/known/fieldmaskpb"

"github.com/owncloud/ocis/v2/ocis-pkg/log"
"github.com/owncloud/ocis/v2/services/graph/pkg/config"
"github.com/owncloud/ocis/v2/services/graph/pkg/errorcode"
"github.com/owncloud/ocis/v2/services/graph/pkg/identity"
"github.com/owncloud/ocis/v2/services/graph/pkg/linktype"
"github.com/owncloud/ocis/v2/services/graph/pkg/unifiedrole"
"google.golang.org/protobuf/types/known/fieldmaskpb"
)

// BaseGraphService implements a couple of helper functions that are
Expand Down
28 changes: 14 additions & 14 deletions services/graph/pkg/service/v0/drives.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
revactx "github.com/cs3org/reva/v2/pkg/ctx"
"github.com/cs3org/reva/v2/pkg/storagespace"
"github.com/cs3org/reva/v2/pkg/utils"
"github.com/owncloud/ocis/v2/ocis-pkg/conversions"

"github.com/owncloud/ocis/v2/ocis-pkg/l10n"
"github.com/owncloud/ocis/v2/ocis-pkg/service/grpc"
v0 "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0"
Expand Down Expand Up @@ -78,7 +78,7 @@ func (g Graph) GetDrives(version APIVersion) http.HandlerFunc {
func (g Graph) GetDrivesV1(w http.ResponseWriter, r *http.Request) {
spaces, errCode := g.getDrives(r, false, APIVersion_1)
if errCode != nil {
errCode.Render(w, r)
errorcode.RenderError(w, r, errCode)
return
}

Expand All @@ -99,7 +99,7 @@ func (g Graph) GetDrivesV1(w http.ResponseWriter, r *http.Request) {
func (g Graph) GetDrivesV1Beta1(w http.ResponseWriter, r *http.Request) {
spaces, errCode := g.getDrives(r, false, APIVersion_1_Beta_1)
if errCode != nil {
errCode.Render(w, r)
errorcode.RenderError(w, r, errCode)
return
}

Expand Down Expand Up @@ -133,7 +133,7 @@ func (g Graph) GetAllDrives(version APIVersion) http.HandlerFunc {
func (g Graph) GetAllDrivesV1(w http.ResponseWriter, r *http.Request) {
spaces, errCode := g.getDrives(r, true, APIVersion_1)
if errCode != nil {
errCode.Render(w, r)
errorcode.RenderError(w, r, errCode)
return
}

Expand All @@ -153,7 +153,7 @@ func (g Graph) GetAllDrivesV1(w http.ResponseWriter, r *http.Request) {
func (g Graph) GetAllDrivesV1Beta1(w http.ResponseWriter, r *http.Request) {
drives, errCode := g.getDrives(r, true, APIVersion_1_Beta_1)
if errCode != nil {
errCode.Render(w, r)
errorcode.RenderError(w, r, errCode)
return
}

Expand All @@ -168,7 +168,7 @@ func (g Graph) GetAllDrivesV1Beta1(w http.ResponseWriter, r *http.Request) {
}

// getDrives implements the Service interface.
func (g Graph) getDrives(r *http.Request, unrestricted bool, apiVersion APIVersion) ([]*libregraph.Drive, *errorcode.Error) {
func (g Graph) getDrives(r *http.Request, unrestricted bool, apiVersion APIVersion) ([]*libregraph.Drive, error) {
logger := g.logger.SubloggerWithRequestID(r.Context())
logger.Info().
Interface("query", r.URL.Query()).
Expand All @@ -179,20 +179,20 @@ func (g Graph) getDrives(r *http.Request, unrestricted bool, apiVersion APIVersi
odataReq, err := godata.ParseRequest(r.Context(), sanitizedPath, r.URL.Query())
if err != nil {
logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get drives: query error")
return nil, conversions.ToPointer(errorcode.New(errorcode.InvalidRequest, err.Error()))
return nil, errorcode.New(errorcode.InvalidRequest, err.Error())
}
ctx := r.Context()

filters, err := generateCs3Filters(odataReq)
if err != nil {
logger.Debug().Err(err).Interface("query", r.URL.Query()).Msg("could not get drives: error parsing filters")
return nil, conversions.ToPointer(errorcode.New(errorcode.NotSupported, err.Error()))
return nil, errorcode.New(errorcode.NotSupported, err.Error())
}
if !unrestricted {
user, ok := revactx.ContextGetUser(r.Context())
if !ok {
logger.Debug().Msg("could not create drive: invalid user")
return nil, conversions.ToPointer(errorcode.New(errorcode.AccessDenied, "invalid user"))
return nil, errorcode.New(errorcode.AccessDenied, "invalid user")
}
filters = append(filters, &storageprovider.ListStorageSpacesRequest_Filter{
Type: storageprovider.ListStorageSpacesRequest_Filter_TYPE_USER,
Expand All @@ -210,32 +210,32 @@ func (g Graph) getDrives(r *http.Request, unrestricted bool, apiVersion APIVersi
switch {
case err != nil:
logger.Error().Err(err).Msg("could not get drives: transport error")
return nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, err.Error()))
return nil, errorcode.New(errorcode.GeneralException, err.Error())
case res.Status.Code != cs3rpc.Code_CODE_OK:
if res.Status.Code == cs3rpc.Code_CODE_NOT_FOUND {
// ok, empty return
return nil, nil
}
logger.Debug().Str("message", res.GetStatus().GetMessage()).Msg("could not get drives: grpc error")
return nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, res.Status.Message))
return nil, errorcode.New(errorcode.GeneralException, res.Status.Message)
}

webDavBaseURL, err := g.getWebDavBaseURL()
if err != nil {
logger.Error().Err(err).Str("url", webDavBaseURL.String()).Msg("could not get drives: error parsing url")
return nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, err.Error()))
return nil, errorcode.New(errorcode.GeneralException, err.Error())
}

spaces, err := g.formatDrives(ctx, webDavBaseURL, res.StorageSpaces, apiVersion)
if err != nil {
logger.Debug().Err(err).Msg("could not get drives: error parsing grpc response")
return nil, conversions.ToPointer(errorcode.New(errorcode.GeneralException, err.Error()))
return nil, errorcode.New(errorcode.GeneralException, err.Error())
}

spaces, err = sortSpaces(odataReq, spaces)
if err != nil {
logger.Debug().Err(err).Msg("could not get drives: error sorting the spaces list according to query")
return nil, conversions.ToPointer(errorcode.New(errorcode.InvalidRequest, err.Error()))
return nil, errorcode.New(errorcode.InvalidRequest, err.Error())
}

return spaces, nil
Expand Down

0 comments on commit d42e295

Please sign in to comment.