From 4671a81535a9cf1d1516738bac9fa376be612322 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 29 Mar 2022 15:02:38 +0200 Subject: [PATCH] clean up cs3 share manager and the indexer --- pkg/publicshare/manager/cs3/cs3.go | 8 ++- pkg/share/manager/cs3/cs3.go | 78 +++++++++++----------- pkg/share/manager/cs3/cs3_test.go | 58 +++++++++++++--- pkg/storage/utils/indexer/indexer.go | 52 +++++++++------ pkg/storage/utils/indexer/indexer_test.go | 14 ++-- pkg/storage/utils/indexer/mocks/Indexer.go | 21 ++++-- 6 files changed, 147 insertions(+), 84 deletions(-) diff --git a/pkg/publicshare/manager/cs3/cs3.go b/pkg/publicshare/manager/cs3/cs3.go index 3c0331f284..28551a46b2 100644 --- a/pkg/publicshare/manager/cs3/cs3.go +++ b/pkg/publicshare/manager/cs3/cs3.go @@ -267,7 +267,9 @@ func (m *Manager) getWithPassword(ctx context.Context, ref *link.PublicShareRefe } func (m *Manager) getByID(ctx context.Context, id string) (*PublicShareWithPassword, error) { - tokens, err := m.indexer.FindBy(&link.PublicShare{}, "Id.OpaqueId", id) + tokens, err := m.indexer.FindBy(&link.PublicShare{}, + indexer.NewField("Id.OpaqueId", id), + ) if err != nil { return nil, err } @@ -298,7 +300,9 @@ func (m *Manager) ListPublicShares(ctx context.Context, u *user.User, filters [] return nil, err } - tokens, err := m.indexer.FindBy(&link.PublicShare{}, "Owner", userIDToIndex(u.Id)) + tokens, err := m.indexer.FindBy(&link.PublicShare{}, + indexer.NewField("Owner", userIDToIndex(u.Id)), + ) if err != nil { return nil, err } diff --git a/pkg/share/manager/cs3/cs3.go b/pkg/share/manager/cs3/cs3.go index e490da53a3..f1c09010d6 100644 --- a/pkg/share/manager/cs3/cs3.go +++ b/pkg/share/manager/cs3/cs3.go @@ -154,7 +154,7 @@ func (m *Manager) initialize() error { } err = m.indexer.AddIndex(&collaboration.Share{}, option.IndexByFunc{ Name: "ResourceId", - Func: indexResourceIdFunc, + Func: indexResourceIDFunc, }, "Id.OpaqueId", "shares", "non_unique", nil, true) if err != nil { return err @@ -252,74 +252,68 @@ func (m *Manager) ListShares(ctx context.Context, filters []*collaboration.Filte return nil, errtypes.UserRequired("error getting user from context") } - ownedShareIds, err := m.indexer.FindBy(&collaboration.Share{}, "OwnerId", userIDToIndex(user.GetId())) - if err != nil { - return nil, err - } - createdShareIds, err := m.indexer.FindBy(&collaboration.Share{}, "CreatorId", userIDToIndex(user.GetId())) + createdShareIds, err := m.indexer.FindBy(&collaboration.Share{}, + indexer.NewField("OwnerId", userIDToIndex(user.Id)), + indexer.NewField("CreatorId", userIDToIndex(user.Id)), + ) if err != nil { return nil, err } - mem := make(map[string]struct{}) + // We use shareMem as a temporary lookup store to check which shares were + // already added. This is to prevent duplicates. + shareMem := make(map[string]struct{}) result := []*collaboration.Share{} - for _, id := range ownedShareIds { - s, err := m.getShareByID(ctx, id) - if err != nil { - return nil, err - } - if share.MatchesFilters(s, filters) { - result = append(result, s) - mem[s.Id.OpaqueId] = struct{}{} - } - } for _, id := range createdShareIds { - if _, handled := mem[id]; handled { - // We don't want to add a share multiple times when we added it - // already. - continue - } s, err := m.getShareByID(ctx, id) if err != nil { return nil, err } if share.MatchesFilters(s, filters) { result = append(result, s) - mem[s.Id.OpaqueId] = struct{}{} + shareMem[s.Id.OpaqueId] = struct{}{} } } + // If a user requests to list shares which have not been created by them + // we have to explicitly fetch these shares and check if the user is + // allowed to list the shares. + // Only then can we add these shares to the result. grouped := share.GroupFiltersByType(filters) idFilter, ok := grouped[collaboration.Filter_TYPE_RESOURCE_ID] if !ok { return result, nil } - // shareIDsByResourceID contains the shareID as the key and the resourceID - // as the value. shareIDsByResourceID := make(map[string]*provider.ResourceId) for _, filter := range idFilter { resourceID := filter.GetResourceId() - ids, err := m.indexer.FindBy(&collaboration.Share{}, "ResourceId", resourceIDToIndex(resourceID)) + shareIDs, err := m.indexer.FindBy(&collaboration.Share{}, + indexer.NewField("ResourceId", resourceIDToIndex(resourceID)), + ) if err != nil { - return nil, err + continue } - - for _, id := range ids { - shareIDsByResourceID[id] = resourceID + for _, shareID := range shareIDs { + shareIDsByResourceID[shareID] = resourceID } } + // statMem is used as a local cache to prevent statting resources which + // already have been checked. statMem := make(map[string]struct{}) for shareID, resourceID := range shareIDsByResourceID { - if _, handled := mem[shareID]; handled { + if _, handled := shareMem[shareID]; handled { // We don't want to add a share multiple times when we added it // already. continue } if _, checked := statMem[resourceIDToIndex(resourceID)]; !checked { - sRes, err := m.gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: resourceID}}) + sReq := &provider.StatRequest{ + Ref: &provider.Reference{ResourceId: resourceID}, + } + sRes, err := m.gatewayClient.Stat(ctx, sReq) if err != nil { continue } @@ -338,7 +332,7 @@ func (m *Manager) ListShares(ctx context.Context, filters []*collaboration.Filte } if share.MatchesFilters(s, filters) { result = append(result, s) - mem[s.Id.OpaqueId] = struct{}{} + shareMem[s.Id.OpaqueId] = struct{}{} } } @@ -386,7 +380,9 @@ func (m *Manager) ListReceivedShares(ctx context.Context, filters []*collaborati if err != nil { return nil, err } - receivedIds, err := m.indexer.FindBy(&collaboration.Share{}, "GranteeId", ids) + receivedIds, err := m.indexer.FindBy(&collaboration.Share{}, + indexer.NewField("GranteeId", ids), + ) if err != nil { return nil, err } @@ -398,7 +394,9 @@ func (m *Manager) ListReceivedShares(ctx context.Context, filters []*collaborati if err != nil { return nil, err } - groupIds, err := m.indexer.FindBy(&collaboration.Share{}, "GranteeId", index) + groupIds, err := m.indexer.FindBy(&collaboration.Share{}, + indexer.NewField("GranteeId", index), + ) if err != nil { return nil, err } @@ -549,7 +547,9 @@ func (m *Manager) getShareByID(ctx context.Context, id string) (*collaboration.S } func (m *Manager) getShareByKey(ctx context.Context, key *collaboration.ShareKey) (*collaboration.Share, error) { - ownerIds, err := m.indexer.FindBy(&collaboration.Share{}, "OwnerId", userIDToIndex(key.Owner)) + ownerIds, err := m.indexer.FindBy(&collaboration.Share{}, + indexer.NewField("OwnerId", userIDToIndex(key.Owner)), + ) if err != nil { return nil, err } @@ -557,7 +557,9 @@ func (m *Manager) getShareByKey(ctx context.Context, key *collaboration.ShareKey if err != nil { return nil, err } - granteeIds, err := m.indexer.FindBy(&collaboration.Share{}, "GranteeId", granteeIndex) + granteeIds, err := m.indexer.FindBy(&collaboration.Share{}, + indexer.NewField("GranteeId", granteeIndex), + ) if err != nil { return nil, err } @@ -622,7 +624,7 @@ func indexGranteeFunc(v interface{}) (string, error) { return granteeToIndex(share.Grantee) } -func indexResourceIdFunc(v interface{}) (string, error) { +func indexResourceIDFunc(v interface{}) (string, error) { share, ok := v.(*collaboration.Share) if !ok { return "", fmt.Errorf("given entity is not a share") diff --git a/pkg/share/manager/cs3/cs3_test.go b/pkg/share/manager/cs3/cs3_test.go index f81e7322f5..67a9b5c311 100644 --- a/pkg/share/manager/cs3/cs3_test.go +++ b/pkg/share/manager/cs3/cs3_test.go @@ -140,7 +140,7 @@ var _ = Describe("Manager", func() { Describe("New", func() { JustBeforeEach(func() { - m, err := cs3.New(storage, indexer) + m, err := cs3.New(nil, storage, indexer) Expect(err).ToNot(HaveOccurred()) Expect(m).ToNot(BeNil()) }) @@ -163,7 +163,7 @@ var _ = Describe("Manager", func() { JustBeforeEach(func() { var err error - m, err = cs3.New(storage, indexer) + m, err = cs3.New(nil, storage, indexer) Expect(err).ToNot(HaveOccurred()) data, err := json.Marshal(share) Expect(err).ToNot(HaveOccurred()) @@ -277,9 +277,13 @@ var _ = Describe("Manager", func() { Context("when requesting the share by key", func() { It("returns NotFound", func() { - indexer.On("FindBy", mock.Anything, "OwnerId", url.QueryEscape(share.Owner.Idp+":"+share.Owner.OpaqueId)). + indexer.On("FindBy", mock.Anything, + indexerpkg.NewField("OwnerId", url.QueryEscape(share.Owner.Idp+":"+share.Owner.OpaqueId)), + ). Return([]string{share.Id.OpaqueId, share2.Id.OpaqueId}, nil) - indexer.On("FindBy", mock.Anything, "GranteeId", url.QueryEscape("user:"+grantee.Id.Idp+":"+grantee.Id.OpaqueId)). + indexer.On("FindBy", mock.Anything, + indexerpkg.NewField("GranteeId", url.QueryEscape("user:"+grantee.Id.Idp+":"+grantee.Id.OpaqueId)), + ). Return([]string{}, nil) returnedShare, err := m.GetShare(ctx, &collaboration.ShareReference{ Spec: &collaboration.ShareReference_Key{ @@ -294,9 +298,13 @@ var _ = Describe("Manager", func() { Expect(returnedShare).To(BeNil()) }) It("gets the share", func() { - indexer.On("FindBy", mock.Anything, "OwnerId", url.QueryEscape(share.Owner.Idp+":"+share.Owner.OpaqueId)). + indexer.On("FindBy", mock.Anything, + indexerpkg.NewField("OwnerId", url.QueryEscape(share.Owner.Idp+":"+share.Owner.OpaqueId)), + ). Return([]string{share.Id.OpaqueId, share2.Id.OpaqueId}, nil) - indexer.On("FindBy", mock.Anything, "GranteeId", url.QueryEscape("user:"+grantee.Id.Idp+":"+grantee.Id.OpaqueId)). + indexer.On("FindBy", mock.Anything, + indexerpkg.NewField("GranteeId", url.QueryEscape("user:"+grantee.Id.Idp+":"+grantee.Id.OpaqueId)), + ). Return([]string{share2.Id.OpaqueId}, nil) returnedShare, err := m.GetShare(ctx, &collaboration.ShareReference{ Spec: &collaboration.ShareReference_Key{ @@ -343,7 +351,19 @@ var _ = Describe("Manager", func() { Describe("ListShares", func() { JustBeforeEach(func() { - indexer.On("FindBy", mock.Anything, "OwnerId", mock.Anything).Return([]string{share.Id.OpaqueId, share2.Id.OpaqueId}, nil) + indexer.On("FindBy", mock.Anything, + mock.MatchedBy(func(input indexerpkg.Field) bool { + return input.Name == "OwnerId" + }), + mock.MatchedBy(func(input indexerpkg.Field) bool { + return input.Name == "CreatorId" + }), + ).Return([]string{share.Id.OpaqueId, share2.Id.OpaqueId}, nil) + indexer.On("FindBy", mock.Anything, + mock.MatchedBy(func(input indexerpkg.Field) bool { + return input.Name == "ResourceId" && input.Value == "!abcd" + }), + ).Return([]string{share.Id.OpaqueId}, nil) }) It("uses the index to get the owned shares", func() { shares, err := m.ListShares(ctx, []*collaboration.Filter{}) @@ -371,9 +391,17 @@ var _ = Describe("Manager", func() { Describe("ListReceivedShares", func() { Context("with a received user share", func() { BeforeEach(func() { - indexer.On("FindBy", mock.Anything, "GranteeId", granteeFn). + indexer.On("FindBy", mock.Anything, + mock.MatchedBy(func(input indexerpkg.Field) bool { + return input.Name == "GranteeId" && input.Value == granteeFn + }), + ). Return([]string{share2.Id.OpaqueId}, nil) - indexer.On("FindBy", mock.Anything, "GranteeId", mock.Anything). + indexer.On("FindBy", mock.Anything, + mock.MatchedBy(func(input indexerpkg.Field) bool { + return input.Name == "GranteeId" + }), + ). Return([]string{}, nil) }) @@ -393,9 +421,17 @@ var _ = Describe("Manager", func() { Context("with a received group share", func() { BeforeEach(func() { - indexer.On("FindBy", mock.Anything, "GranteeId", groupFn). + indexer.On("FindBy", mock.Anything, + mock.MatchedBy(func(input indexerpkg.Field) bool { + return input.Name == "GranteeId" && input.Value == groupFn + }), + ). Return([]string{share2.Id.OpaqueId}, nil) - indexer.On("FindBy", mock.Anything, "GranteeId", mock.Anything). + indexer.On("FindBy", mock.Anything, + mock.MatchedBy(func(input indexerpkg.Field) bool { + return input.Name == "GranteeId" + }), + ). Return([]string{}, nil) }) diff --git a/pkg/storage/utils/indexer/indexer.go b/pkg/storage/utils/indexer/indexer.go index 8afd7bef4f..1b42593d9a 100644 --- a/pkg/storage/utils/indexer/indexer.go +++ b/pkg/storage/utils/indexer/indexer.go @@ -42,10 +42,21 @@ import ( type Indexer interface { AddIndex(t interface{}, indexBy option.IndexBy, pkName, entityDirName, indexType string, bound *option.Bound, caseInsensitive bool) error Add(t interface{}) ([]IdxAddResult, error) - FindBy(t interface{}, field string, val string) ([]string, error) + FindBy(t interface{}, fields ...Field) ([]string, error) Delete(t interface{}) error } +// Field combines the name and value of an indexed field. +type Field struct { + Name string + Value string +} + +// NewField is a utility function to create a new Field. +func NewField(name, value string) Field { + return Field{Name: name, Value: value} +} + // StorageIndexer is the indexer implementation using metadata storage type StorageIndexer struct { storage metadata.Storage @@ -145,36 +156,38 @@ func (i *StorageIndexer) Add(t interface{}) ([]IdxAddResult, error) { return results, nil } -// FindBy finds a value on an index by field and value. -func (i *StorageIndexer) FindBy(t interface{}, findBy, val string) ([]string, error) { +// FindBy finds a value on an index by fields. +// If multiple fields are given then they are handled like an or condition. +func (i *StorageIndexer) FindBy(t interface{}, queryFields ...Field) ([]string, error) { typeName := getTypeFQN(t) i.mu.RLock(typeName) defer i.mu.RUnlock(typeName) - resultPaths := make([]string, 0) + resultPaths := make(map[string]struct{}) if fields, ok := i.indices[typeName]; ok { - for _, idx := range fields.IndicesByField[strcase.ToCamel(findBy)] { - idxVal := val - res, err := idx.Lookup(idxVal) - if err != nil { - if _, ok := err.(errtypes.IsNotFound); ok { - continue - } - + for _, field := range queryFields { + for _, idx := range fields.IndicesByField[strcase.ToCamel(field.Name)] { + res, err := idx.Lookup(field.Value) if err != nil { - return nil, err + if _, ok := err.(errtypes.IsNotFound); ok { + continue + } + + if err != nil { + return nil, err + } + } + for _, r := range res { + resultPaths[path.Base(r)] = struct{}{} } } - - resultPaths = append(resultPaths, res...) - } } result := make([]string, 0, len(resultPaths)) - for _, v := range resultPaths { - result = append(result, path.Base(v)) + for p := range resultPaths { + result = append(result, path.Base(p)) } return result, nil @@ -340,7 +353,8 @@ func (i *StorageIndexer) resolveTree(t interface{}, tree *queryTree, partials *[ return err } - r, err := i.FindBy(t, operand.field, operand.value) + field := Field{Name: operand.field, Value: operand.value} + r, err := i.FindBy(t, field) if err != nil { return err } diff --git a/pkg/storage/utils/indexer/indexer_test.go b/pkg/storage/utils/indexer/indexer_test.go index ed96b0b18a..87ffae77b0 100644 --- a/pkg/storage/utils/indexer/indexer_test.go +++ b/pkg/storage/utils/indexer/indexer_test.go @@ -43,7 +43,7 @@ func TestIndexer_Disk_FindByWithUniqueIndex(t *testing.T) { _, err = indexer.Add(u) assert.NoError(t, err) - res, err := indexer.FindBy(User{}, "UserName", "mikey") + res, err := indexer.FindBy(User{}, NewField("UserName", "mikey")) assert.NoError(t, err) t.Log(res) @@ -82,7 +82,7 @@ func TestIndexer_Disk_AddWithNonUniqueIndex(t *testing.T) { _, err = indexer.Add(pet2) assert.NoError(t, err) - res, err := indexer.FindBy(Pet{}, "Kind", "Hog") + res, err := indexer.FindBy(Pet{}, NewField("Kind", "Hog")) assert.NoError(t, err) t.Log(res) @@ -108,7 +108,7 @@ func TestIndexer_Disk_AddWithAutoincrementIndex(t *testing.T) { assert.Equal(t, "UID", res2[0].Field) assert.Equal(t, "6", path.Base(res2[0].Value)) - resFindBy, err := indexer.FindBy(User{}, "UID", "6") + resFindBy, err := indexer.FindBy(User{}, NewField("UID", "6")) assert.NoError(t, err) assert.Equal(t, "hijklmn-456", resFindBy[0]) t.Log(resFindBy) @@ -189,10 +189,10 @@ func TestIndexer_Disk_UpdateWithUniqueIndex(t *testing.T) { Email: "mikey@example.com", }) assert.NoError(t, err) - v, err1 := indexer.FindBy(&User{}, "UserName", "mikey-new") + v, err1 := indexer.FindBy(&User{}, NewField("UserName", "mikey-new")) assert.NoError(t, err1) assert.Len(t, v, 1) - v, err2 := indexer.FindBy(&User{}, "UserName", "mikey") + v, err2 := indexer.FindBy(&User{}, NewField("UserName", "mikey")) assert.NoError(t, err2) assert.Len(t, v, 0) @@ -206,10 +206,10 @@ func TestIndexer_Disk_UpdateWithUniqueIndex(t *testing.T) { Email: "mikey-new@example.com", }) assert.NoError(t, err1) - fbUserName, err2 := indexer.FindBy(&User{}, "UserName", "mikey-newest") + fbUserName, err2 := indexer.FindBy(&User{}, NewField("UserName", "mikey-newest")) assert.NoError(t, err2) assert.Len(t, fbUserName, 1) - fbEmail, err3 := indexer.FindBy(&User{}, "Email", "mikey-new@example.com") + fbEmail, err3 := indexer.FindBy(&User{}, NewField("Email", "mikey-new@example.com")) assert.NoError(t, err3) assert.Len(t, fbEmail, 1) diff --git a/pkg/storage/utils/indexer/mocks/Indexer.go b/pkg/storage/utils/indexer/mocks/Indexer.go index ca207fad23..12da53a201 100644 --- a/pkg/storage/utils/indexer/mocks/Indexer.go +++ b/pkg/storage/utils/indexer/mocks/Indexer.go @@ -83,13 +83,20 @@ func (_m *Indexer) Delete(t interface{}) error { return r0 } -// FindBy provides a mock function with given fields: t, field, val -func (_m *Indexer) FindBy(t interface{}, field string, val string) ([]string, error) { - ret := _m.Called(t, field, val) +// FindBy provides a mock function with given fields: t, fields +func (_m *Indexer) FindBy(t interface{}, fields ...indexer.Field) ([]string, error) { + _va := make([]interface{}, len(fields)) + for _i := range fields { + _va[_i] = fields[_i] + } + var _ca []interface{} + _ca = append(_ca, t) + _ca = append(_ca, _va...) + ret := _m.Called(_ca...) var r0 []string - if rf, ok := ret.Get(0).(func(interface{}, string, string) []string); ok { - r0 = rf(t, field, val) + if rf, ok := ret.Get(0).(func(interface{}, ...indexer.Field) []string); ok { + r0 = rf(t, fields...) } else { if ret.Get(0) != nil { r0 = ret.Get(0).([]string) @@ -97,8 +104,8 @@ func (_m *Indexer) FindBy(t interface{}, field string, val string) ([]string, er } var r1 error - if rf, ok := ret.Get(1).(func(interface{}, string, string) error); ok { - r1 = rf(t, field, val) + if rf, ok := ret.Get(1).(func(interface{}, ...indexer.Field) error); ok { + r1 = rf(t, fields...) } else { r1 = ret.Error(1) }