enhancement: getting unique mountpoint only if the state transitions …

…to accepted
cs3org · Jun 10, 2024 · ff87878 · ff87878
1 parent e8dc249
commit ff87878
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 70 deletions.
diff --git a/changelog/unreleased/enhance-unique-share-mountpoint-name.md 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
@@ -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
@@ -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",