Skip to content

Commit

Permalink
Merge pull request #4546 from 2403905/issue-8471
Browse files Browse the repository at this point in the history 
fix the mount points naming
  • Loading branch information
@2403905
2403905 authored Mar 13, 2024
2 parents 04c0a31 + 2818a1e commit 352a246
Show file tree
Hide file tree
Showing 3 changed files with 161 additions and 35 deletions.
6 changes: 6 additions & 0 deletions changelog/unreleased/fix-mount-points-naming.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Bugfix: Fix the mount points naming

We fixed a bug that caused inconsistent naming when multiple users share the resource with same name to another user.

https://github.com/cs3org/reva/pull/4546
https://github.com/owncloud/ocis/issues/8471
77 changes: 42 additions & 35 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"net/http"
"path"
"path/filepath"
"sort"
"slices"
"strconv"
"strings"

Expand Down Expand Up @@ -128,55 +128,62 @@ func GetMountpointAndUnmountedShares(ctx context.Context, gwc gateway.GatewayAPI
}

// we need to sort the received shares by mount point in order to make things easier to evaluate.
mount := filepath.Clean(info.Name)
base := filepath.Clean(info.Name)
mount := base
existingMountpoint := ""
mountedShares := make([]*collaboration.ReceivedShare, 0, len(receivedShares))
mountedShares := make([]string, 0, len(receivedShares))
var pathExists bool

for _, s := range receivedShares {
if utils.ResourceIDEqual(s.Share.ResourceId, info.GetId()) {
if s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
// a share to the resource already exists and is mounted, remember the mount point
_, err := utils.GetResourceByID(ctx, s.Share.ResourceId, gwc)
if err == nil {
existingMountpoint = s.MountPoint.Path
}
} else {
// a share to the resource already exists but is not mounted, collect the unmounted share
unmountedShares = append(unmountedShares, s)
resourceIDEqual := utils.ResourceIDEqual(s.GetShare().GetResourceId(), info.GetId())

if resourceIDEqual && s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
// a share to the resource already exists and is mounted, remember the mount point
_, err := utils.GetResourceByID(ctx, s.GetShare().GetResourceId(), gwc)
if err == nil {
existingMountpoint = s.GetMountPoint().GetPath()
}
}

if resourceIDEqual && s.State != collaboration.ShareState_SHARE_STATE_ACCEPTED {
// a share to the resource already exists but is not mounted, collect the unmounted share
unmountedShares = append(unmountedShares, s)
}

if s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
mountedShares = append(mountedShares, s)
// collect all accepted mount points
mountedShares = append(mountedShares, s.GetMountPoint().GetPath())
if s.GetMountPoint().GetPath() == mount {
// does the shared resource still exist?
_, err := utils.GetResourceByID(ctx, s.GetShare().GetResourceId(), gwc)
if err == nil {
pathExists = true
}
// TODO we could delete shares here if the stat returns code NOT FOUND ... but listening for file deletes would be better
}
}
}

sort.Slice(mountedShares, func(i, j int) bool {
return mountedShares[i].MountPoint.Path > mountedShares[j].MountPoint.Path
})

if existingMountpoint != "" {
// we want to reuse the same mountpoint for all unmounted shares to the same resource
return existingMountpoint, unmountedShares, nil
}

// we have a list of shares, we want to iterate over all of them and check for name collisions
for i, ms := range mountedShares {
if ms.MountPoint.Path == mount {
// does the shared resource still exist?
_, err := utils.GetResourceByID(ctx, ms.Share.ResourceId, gwc)
if err == nil {
// The mount point really already exists, we need to insert a number into the filename
ext := filepath.Ext(mount)
name := strings.TrimSuffix(mount, ext)
// be smart about .tar.(gz|bz) files
if strings.HasSuffix(name, ".tar") {
name = strings.TrimSuffix(name, ".tar")
ext = ".tar" + ext
}

mount = fmt.Sprintf("%s (%s)%s", name, strconv.Itoa(i+1), ext)
// If the mount point really already exists, we need to insert a number into the filename
if pathExists {
// now we have a list of shares, we want to iterate over all of them and check for name collisions agents a mount points list
for i := 1; i <= len(mountedShares)+1; i++ {
ext := filepath.Ext(base)
name := strings.TrimSuffix(base, ext)
// be smart about .tar.(gz|bz) files
if strings.HasSuffix(name, ".tar") {
name = strings.TrimSuffix(name, ".tar")
ext = ".tar" + ext
}
mount = name + " (" + strconv.Itoa(i) + ")" + ext
if !slices.Contains(mountedShares, mount) {
return mount, unmountedShares, nil
}
// TODO we could delete shares here if the stat returns code NOT FOUND ... but listening for file deletes would be better
}
}
return mount, unmountedShares, nil
Expand Down
113 changes: 113 additions & 0 deletions internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocs/config"
Expand Down Expand Up @@ -341,5 +342,117 @@ var _ = Describe("The ocs API", func() {
client.AssertNumberOfCalls(GinkgoT(), "UpdateReceivedShare", 1)
})
})

Context("GetMountpointAndUnmountedShares ", func() {
storage := "storage-users-1"
userOneSpaceId := "first-user-id-0000-000000000000"
userTwoSpaceId := "second-user-id-0000-000000000000"
BeforeEach(func() {
client.On("ListReceivedShares", mock.Anything, mock.Anything, mock.Anything).Return(&collaboration.ListReceivedSharesResponse{
Status: status.NewOK(context.Background()),
Shares: []*collaboration.ReceivedShare{
{
Share: &collaboration.Share{
ResourceId: &provider.ResourceId{
StorageId: storage,
OpaqueId: "be098d47-4518-4a96-92e3-52e904b3958d",
SpaceId: userOneSpaceId,
},
},
State: 1,
},
{
Share: &collaboration.Share{
ResourceId: &provider.ResourceId{
StorageId: storage,
OpaqueId: "9284d5fc-da4c-448f-a999-797a2aa1376e",
SpaceId: userOneSpaceId,
},
},
State: 2,
MountPoint: &provider.Reference{
Path: "b.txt",
},
},
{
Share: &collaboration.Share{
ResourceId: &provider.ResourceId{
StorageId: storage,
OpaqueId: "3a83e157-f03b-4cd5-b64a-d5240c6e06b5",
SpaceId: userOneSpaceId,
},
},
State: 2,
MountPoint: &provider.Reference{
Path: "b (1).txt",
},
},
{
Share: &collaboration.Share{
ResourceId: &provider.ResourceId{
StorageId: storage,
OpaqueId: "9bed6929-6691-4f5d-ba5e-b2069fe508c7",
SpaceId: userTwoSpaceId,
},
},
State: 2,
MountPoint: &provider.Reference{
Path: "demo.tar.gz",
},
},
{
Share: &collaboration.Share{
ResourceId: &provider.ResourceId{
StorageId: storage,
OpaqueId: "f1a62fe5-6acb-469c-bbe8-d5206a13b3a1",
SpaceId: userOneSpaceId,
},
},
State: 2,
MountPoint: &provider.Reference{
Path: "a (1).txt",
},
},
},
}, nil)
})

DescribeTable("Resolve mountpoint name",
func(info *provider.ResourceInfo, expected string, unmountedLen int) {
// GetMountpointAndUnmountedShares check the error Stat response only
client.On("Stat", mock.Anything, mock.Anything).
Return(&provider.StatResponse{Status: &rpc.Status{Code: rpc.Code_CODE_OK},
Info: &provider.ResourceInfo{}}, nil)
fileName, unmounted, err := shares.GetMountpointAndUnmountedShares(ctx, client, info)
Expect(fileName).To(Equal(expected))
Expect(len(unmounted)).To(Equal(unmountedLen))
Expect(err).To(BeNil())
},
Entry("new mountpoint, name changed", &provider.ResourceInfo{
Name: "b.txt",
Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId},
}, "b (2).txt", 0),
Entry("new mountpoint, name changed", &provider.ResourceInfo{
Name: "a (1).txt",
Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId},
}, "a (1) (1).txt", 0),
Entry("new mountpoint, name is not collide", &provider.ResourceInfo{
Name: "file.txt",
Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId},
}, "file.txt", 0),
Entry("existing mountpoint", &provider.ResourceInfo{
Name: "b.txt",
Id: &provider.ResourceId{StorageId: storage, OpaqueId: "9284d5fc-da4c-448f-a999-797a2aa1376e", SpaceId: userOneSpaceId},
}, "b.txt", 0),
Entry("existing mountpoint tar.gz", &provider.ResourceInfo{
Name: "demo.tar.gz",
Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId},
}, "demo (1).tar.gz", 0),
Entry("unmountpoint", &provider.ResourceInfo{
Name: "b.txt",
Id: &provider.ResourceId{StorageId: storage, OpaqueId: "be098d47-4518-4a96-92e3-52e904b3958d", SpaceId: userOneSpaceId},
}, "b (2).txt", 1),
)
})
})
})

0 comments on commit 352a246

Please sign in to comment.