Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix the mount points naming #4546

Merged
merged 1 commit into from
Mar 13, 2024
Merged

fix the mount points naming #4546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/unreleased/fix-mount-points-naming.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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),
)
})
})
})
Loading