From ebe67f9af8ae640925eb7180569108fe3c0ca20c Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Mon, 10 Jun 2024 12:54:37 +0200 Subject: [PATCH] enhancement: getting unique mountpoint only if the state transitions to accepted --- .../enhance-unique-share-mountpoint-name.md | 6 + .../usershareprovider/usershareprovider.go | 122 +++++++++--------- .../usershareprovider_test.go | 34 +++-- 3 files changed, 92 insertions(+), 70 deletions(-) create mode 100644 changelog/unreleased/enhance-unique-share-mountpoint-name.md diff --git a/changelog/unreleased/enhance-unique-share-mountpoint-name.md b/changelog/unreleased/enhance-unique-share-mountpoint-name.md new file mode 100644 index 0000000000..0e5b86edcf --- /dev/null +++ b/changelog/unreleased/enhance-unique-share-mountpoint-name.md @@ -0,0 +1,6 @@ +Enhancement: Unique share mountpoint name + +Accepting a received share with a mountpoint name that already exists will now append a unique suffix to the mountpoint name. + +https://github.com/cs3org/reva/pull/4714 +https://github.com/owncloud/ocis/issues/8961 diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index 44092cf9d1..c52caad51b 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -34,7 +34,6 @@ import ( "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "google.golang.org/grpc" - "google.golang.org/protobuf/types/known/fieldmaskpb" "github.com/cs3org/reva/v2/pkg/appctx" "github.com/cs3org/reva/v2/pkg/conversions" @@ -53,6 +52,7 @@ import ( const ( _fieldMaskPathMountPoint = "mount_point" _fieldMaskPathPermissions = "permissions" + _fieldMaskPathState = "state" ) func init() { @@ -530,76 +530,74 @@ func (s *service) UpdateReceivedShare(ctx context.Context, req *collaboration.Up }, nil } - gatewayClient, err := s.gatewaySelector.Next() - if err != nil { - return nil, err - } + isStateTransitionShareAccepted := slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathState) && req.GetShare().GetState() == collaboration.ShareState_SHARE_STATE_ACCEPTED + if isStateTransitionShareAccepted { + gatewayClient, err := s.gatewaySelector.Next() + if err != nil { + return nil, err + } - receivedShare, err := gatewayClient.GetReceivedShare(ctx, &collaboration.GetReceivedShareRequest{ - Ref: &collaboration.ShareReference{ - Spec: &collaboration.ShareReference_Id{ - Id: req.GetShare().GetShare().GetId(), + receivedShare, err := gatewayClient.GetReceivedShare(ctx, &collaboration.GetReceivedShareRequest{ + Ref: &collaboration.ShareReference{ + Spec: &collaboration.ShareReference_Id{ + Id: req.GetShare().GetShare().GetId(), + }, }, - }, - }) - switch { - case err != nil: - fallthrough - case receivedShare.GetStatus().GetCode() != rpc.Code_CODE_OK: - return &collaboration.UpdateReceivedShareResponse{ - Status: receivedShare.GetStatus(), - }, err - } - - resourceStat, err := gatewayClient.Stat(ctx, &provider.StatRequest{ - Ref: &provider.Reference{ - ResourceId: receivedShare.GetShare().GetShare().GetResourceId(), - }, - }) - switch { - case err != nil: - fallthrough - case resourceStat.GetStatus().GetCode() != rpc.Code_CODE_OK: - return &collaboration.UpdateReceivedShareResponse{ - Status: resourceStat.GetStatus(), - }, err - } - - // check if the update mask is nil and if so, initialize it - if req.GetUpdateMask() == nil { - req.UpdateMask = &fieldmaskpb.FieldMask{Paths: []string{}} - } - - // handle mount point related updates - { - // find a suitable mount point - var requestedMountpoint string + }) switch { - case slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) && req.GetShare().GetMountPoint().GetPath() != "": - requestedMountpoint = req.GetShare().GetMountPoint().GetPath() - case receivedShare.GetShare().GetMountPoint().GetPath() != "": - requestedMountpoint = receivedShare.GetShare().GetMountPoint().GetPath() - default: - requestedMountpoint = resourceStat.GetInfo().GetName() + case err != nil: + fallthrough + case receivedShare.GetStatus().GetCode() != rpc.Code_CODE_OK: + return &collaboration.UpdateReceivedShareResponse{ + Status: receivedShare.GetStatus(), + }, err } - // check if the requested mount point is available and if not, find a suitable one - availableMountpoint, err := GetAvailableMountpoint(ctx, gatewayClient, - resourceStat.GetInfo().GetId(), - requestedMountpoint, - ) - if err != nil { + resourceStat, err := gatewayClient.Stat(ctx, &provider.StatRequest{ + Ref: &provider.Reference{ + ResourceId: receivedShare.GetShare().GetShare().GetResourceId(), + }, + }) + switch { + case err != nil: + fallthrough + case resourceStat.GetStatus().GetCode() != rpc.Code_CODE_OK: return &collaboration.UpdateReceivedShareResponse{ - Status: status.NewInternal(ctx, err.Error()), - }, nil + Status: resourceStat.GetStatus(), + }, err } - if !slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) { - req.GetUpdateMask().Paths = append(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) - } + // handle mount point related updates + { + // find a suitable mount point + var requestedMountpoint string + switch { + case slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) && req.GetShare().GetMountPoint().GetPath() != "": + requestedMountpoint = req.GetShare().GetMountPoint().GetPath() + case receivedShare.GetShare().GetMountPoint().GetPath() != "": + requestedMountpoint = receivedShare.GetShare().GetMountPoint().GetPath() + default: + requestedMountpoint = resourceStat.GetInfo().GetName() + } - req.GetShare().MountPoint = &provider.Reference{ - Path: availableMountpoint, + // check if the requested mount point is available and if not, find a suitable one + availableMountpoint, err := GetAvailableMountpoint(ctx, gatewayClient, + resourceStat.GetInfo().GetId(), + requestedMountpoint, + ) + if err != nil { + return &collaboration.UpdateReceivedShareResponse{ + Status: status.NewInternal(ctx, err.Error()), + }, nil + } + + if !slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) { + req.GetUpdateMask().Paths = append(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) + } + + req.GetShare().MountPoint = &provider.Reference{ + Path: availableMountpoint, + } } } diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index 04b33732b1..ba4ccf8210 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -20,12 +20,22 @@ package usershareprovider_test import ( "context" + "path/filepath" + "regexp" + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" permissions "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1" rpcpb "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" collaborationpb "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" providerpb "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/pkg/errors" + "github.com/stretchr/testify/mock" + "google.golang.org/grpc" + "google.golang.org/protobuf/types/known/fieldmaskpb" + "github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider" "github.com/cs3org/reva/v2/pkg/conversions" ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" @@ -37,14 +47,6 @@ import ( "github.com/cs3org/reva/v2/pkg/share/mocks" "github.com/cs3org/reva/v2/pkg/utils" cs3mocks "github.com/cs3org/reva/v2/tests/cs3mocks/mocks" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "github.com/pkg/errors" - "github.com/stretchr/testify/mock" - "google.golang.org/grpc" - "google.golang.org/protobuf/types/known/fieldmaskpb" - "path/filepath" - "regexp" ) var _ = Describe("user share provider service", func() { @@ -211,7 +213,11 @@ var _ = Describe("user share provider service", func() { Entry( "requesting the share errors", &collaborationpb.UpdateReceivedShareRequest{ + UpdateMask: &fieldmaskpb.FieldMask{ + Paths: []string{"state"}, + }, Share: &collaborationpb.ReceivedShare{ + State: collaborationpb.ShareState_SHARE_STATE_ACCEPTED, Share: &collaborationpb.Share{ Id: &collaborationpb.ShareId{ OpaqueId: "1", @@ -225,7 +231,11 @@ var _ = Describe("user share provider service", func() { Entry( "requesting the share fails", &collaborationpb.UpdateReceivedShareRequest{ + UpdateMask: &fieldmaskpb.FieldMask{ + Paths: []string{"state"}, + }, Share: &collaborationpb.ReceivedShare{ + State: collaborationpb.ShareState_SHARE_STATE_ACCEPTED, Share: &collaborationpb.Share{ Id: &collaborationpb.ShareId{ OpaqueId: "1", @@ -280,7 +290,11 @@ var _ = Describe("user share provider service", func() { Entry( "stat the resource errors", &collaborationpb.UpdateReceivedShareRequest{ + UpdateMask: &fieldmaskpb.FieldMask{ + Paths: []string{"state"}, + }, Share: &collaborationpb.ReceivedShare{ + State: collaborationpb.ShareState_SHARE_STATE_ACCEPTED, Share: &collaborationpb.Share{ Id: &collaborationpb.ShareId{ OpaqueId: "1", @@ -294,7 +308,11 @@ var _ = Describe("user share provider service", func() { Entry( "stat the resource fails", &collaborationpb.UpdateReceivedShareRequest{ + UpdateMask: &fieldmaskpb.FieldMask{ + Paths: []string{"state"}, + }, Share: &collaborationpb.ReceivedShare{ + State: collaborationpb.ShareState_SHARE_STATE_ACCEPTED, Share: &collaborationpb.Share{ Id: &collaborationpb.ShareId{ OpaqueId: "1",