apitech
/
grafana
mirror of https://github.com/grafana/grafana
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

[Nested Folder] Block move operation that could introduce more than 8 level of depth,… (#59832)

Browse Source 
* block move operation that could introduce more than 8 level of depth, forbid circular reference

* move getHeight to store, mock store in service

* fix linter
pull/60009/head^2
ying-jeanne 3 years ago committed by GitHub
parent 9094153b30
commit 1131bac5da
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 185 additions and 48 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 29
      pkg/services/folder/folderimpl/folder.go
  2. 54
      pkg/services/folder/folderimpl/folder_test.go
  3. 24
      pkg/services/folder/folderimpl/sqlstore.go
  4. 100
      pkg/services/folder/folderimpl/sqlstore_test.go
  5. 4
      pkg/services/folder/folderimpl/store.go
  6. 21
      pkg/services/folder/folderimpl/store_fake.go
  7. 1
      pkg/services/folder/model.go

29
pkg/services/folder/folderimpl/folder.go
Unescape View File

@ -456,9 +456,36 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol
return nil, err
}
// if the new parent is the same as the current parent, we don't need to do anything
if foldr.ParentUID == cmd.NewParentUID {
return foldr, nil
}
// here we get the folder, we need to get the height of current folder
// and the depth of the new parent folder, the sum can't bypass 8
folderHeight, err := s.store.GetHeight(ctx, foldr.UID, cmd.OrgID, &cmd.NewParentUID)
if err != nil {
return nil, err
}
parents, err := s.GetParents(ctx, &folder.GetParentsQuery{UID: cmd.NewParentUID, OrgID: cmd.OrgID})
if err != nil {
return nil, err
}
// current folder height + current folder + parent folder + parent folder depth should be less than or equal 8
if folderHeight+len(parents)+2 > folder.MaxNestedFolderDepth {
return nil, folder.ErrMaximumDepthReached
}
// if the current folder is already a parent of newparent, we should return error
for _, parent := range parents {
if parent.UID == foldr.UID {
return nil, folder.ErrCircularReference
}
}
return s.store.Update(ctx, folder.UpdateFolderCommand{
Folder: foldr,
// NewParentUID: &cmd.NewParentUID,
})
}

54
pkg/services/folder/folderimpl/folder_test.go
Unescape View File

@ -347,7 +347,7 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) {
})
t.Run("get children folder", func(t *testing.T) {
folderStore.ExpectedFolders = []*folder.Folder{
folderStore.ExpectedChildFolders = []*folder.Folder{
{
UID: "test",
},
@ -517,6 +517,56 @@ func TestNestedFolderService(t *testing.T) {
require.NotNil(t, f)
})
t.Run("move when parentUID in the current subtree returns error from nested folder service", func(t *testing.T) {
g := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true, CanViewValue: true})
t.Cleanup(func() {
guardian.New = g
})
store.ExpectedFolder = &folder.Folder{UID: "myFolder", ParentUID: "newFolder"}
store.ExpectedError = folder.ErrCircularReference
f, err := foldersvc.Move(context.Background(), &folder.MoveFolderCommand{UID: "myFolder", NewParentUID: "newFolder", OrgID: orgID, SignedInUser: usr})
require.Error(t, err, folder.ErrCircularReference)
require.Nil(t, f)
store.ExpectedChildFolders = []*folder.Folder{}
})
t.Run("move when new parentUID depth + subTree height bypassed maximum depth returns error", func(t *testing.T) {
g := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true, CanViewValue: true})
t.Cleanup(func() {
guardian.New = g
})
store.ExpectedError = nil
store.ExpectedFolder = &folder.Folder{UID: "myFolder", ParentUID: "newFolder"}
store.ExpectedParentFolders = []*folder.Folder{
{UID: "newFolder", ParentUID: "newFolder"},
{UID: "newFolder2", ParentUID: "newFolder2"},
}
store.ExpectedFolderHeight = 5
f, err := foldersvc.Move(context.Background(), &folder.MoveFolderCommand{UID: "myFolder", NewParentUID: "newFolder2", OrgID: orgID, SignedInUser: usr})
require.Error(t, err, folder.ErrMaximumDepthReached)
require.Nil(t, f)
})
t.Run("move when parentUID in the current subtree returns error from nested folder service", func(t *testing.T) {
g := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true, CanViewValue: true})
t.Cleanup(func() {
guardian.New = g
})
store.ExpectedError = nil
store.ExpectedFolder = &folder.Folder{UID: "myFolder", ParentUID: "newFolder"}
store.ExpectedParentFolders = []*folder.Folder{{UID: "myFolder", ParentUID: "12345"}, {UID: "12345", ParentUID: ""}}
f, err := foldersvc.Move(context.Background(), &folder.MoveFolderCommand{UID: "myFolder", NewParentUID: "newFolder2", OrgID: orgID, SignedInUser: usr})
require.Error(t, err, folder.ErrCircularReference)
require.Nil(t, f)
store.ExpectedChildFolders = []*folder.Folder{}
})
t.Run("delete with success", func(t *testing.T) {
var actualCmd *models.DeleteDashboardCommand
dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
@ -561,7 +611,7 @@ func TestNestedFolderService(t *testing.T) {
for i := 0; i < folder.MaxNestedFolderDepth; i++ {
parents = append(parents, &folder.Folder{UID: fmt.Sprintf("folder%d", i)})
}
store.ExpectedFolders = parents
store.ExpectedParentFolders = parents
store.ExpectedError = nil
_, err := foldersvc.Create(context.Background(), &folder.CreateFolderCommand{

24
pkg/services/folder/folderimpl/sqlstore.go
Unescape View File

@ -264,3 +264,27 @@ func (ss *sqlStore) getParentsMySQL(ctx context.Context, cmd folder.GetParentsQu
})
return util.Reverse(folders), err
}
func (ss *sqlStore) GetHeight(ctx context.Context, foldrUID string, orgID int64, parentUID *string) (int, error) {
height := -1
queue := []string{foldrUID}
for len(queue) > 0 {
length := len(queue)
height++
for i := 0; i < length; i++ {
ele := queue[0]
queue = queue[1:]
if parentUID != nil && *parentUID == ele {
return 0, folder.ErrCircularReference
}
folders, err := ss.GetChildren(ctx, folder.GetTreeQuery{UID: ele, OrgID: orgID})
if err != nil {
return 0, err
}
for _, f := range folders {
queue = append(queue, f.UID)
}
}
}
return height, nil
}

100
pkg/services/folder/folderimpl/sqlstore_test.go
Unescape View File

@ -19,6 +19,9 @@ import (
"github.com/grafana/grafana/pkg/util"
)
var folderTitle string = "folder1"
var folderDsc string = "folder desc"
func TestIntegrationCreate(t *testing.T) {
if testing.Short() {
t.Skip("skipping integration test")
@ -32,8 +35,8 @@ func TestIntegrationCreate(t *testing.T) {
t.Run("creating a folder without providing a UID should fail", func(t *testing.T) {
_, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: "folder1",
Description: "folder desc",
Title: folderTitle,
Description: folderDsc,
OrgID: orgID,
})
require.Error(t, err)
@ -41,10 +44,10 @@ func TestIntegrationCreate(t *testing.T) {
t.Run("creating a folder with unknown parent should fail", func(t *testing.T) {
_, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: "folder1",
Title: folderTitle,
OrgID: orgID,
ParentUID: "unknown",
Description: "folder desc",
Description: folderDsc,
UID: util.GenerateShortUID(),
})
require.Error(t, err)
@ -53,8 +56,8 @@ func TestIntegrationCreate(t *testing.T) {
t.Run("creating a folder without providing a parent should default to the empty parent folder", func(t *testing.T) {
uid := util.GenerateShortUID()
f, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: "folder1",
Description: "folder desc",
Title: folderTitle,
Description: folderDsc,
OrgID: orgID,
UID: uid,
})
@ -65,8 +68,8 @@ func TestIntegrationCreate(t *testing.T) {
require.NoError(t, err)
})
assert.Equal(t, "folder1", f.Title)
assert.Equal(t, "folder desc", f.Description)
assert.Equal(t, folderTitle, f.Title)
assert.Equal(t, folderDsc, f.Description)
assert.NotEmpty(t, f.ID)
assert.Equal(t, uid, f.UID)
assert.Empty(t, f.ParentUID)
@ -76,8 +79,8 @@ func TestIntegrationCreate(t *testing.T) {
OrgID: orgID,
})
assert.NoError(t, err)
assert.Equal(t, "folder1", ff.Title)
assert.Equal(t, "folder desc", ff.Description)
assert.Equal(t, folderTitle, ff.Title)
assert.Equal(t, folderDsc, ff.Description)
assert.Empty(t, ff.ParentUID)
assertAncestorUIDs(t, folderStore, f, []string{folder.GeneralFolderUID})
@ -103,10 +106,10 @@ func TestIntegrationCreate(t *testing.T) {
uid := util.GenerateShortUID()
f, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: "folder1",
Title: folderTitle,
OrgID: orgID,
ParentUID: parent.UID,
Description: "folder desc",
Description: folderDsc,
UID: uid,
})
require.NoError(t, err)
@ -115,8 +118,8 @@ func TestIntegrationCreate(t *testing.T) {
require.NoError(t, err)
})
assert.Equal(t, "folder1", f.Title)
assert.Equal(t, "folder desc", f.Description)
assert.Equal(t, folderTitle, f.Title)
assert.Equal(t, folderDsc, f.Description)
assert.NotEmpty(t, f.ID)
assert.Equal(t, uid, f.UID)
assert.Equal(t, parentUID, f.ParentUID)
@ -129,8 +132,8 @@ func TestIntegrationCreate(t *testing.T) {
OrgID: f.OrgID,
})
assert.NoError(t, err)
assert.Equal(t, "folder1", ff.Title)
assert.Equal(t, "folder desc", ff.Description)
assert.Equal(t, folderTitle, ff.Title)
assert.Equal(t, folderDsc, ff.Description)
assert.Equal(t, parentUID, ff.ParentUID)
})
}
@ -195,11 +198,9 @@ func TestIntegrationUpdate(t *testing.T) {
orgID := CreateOrg(t, db)
// create folder
origTitle := "folder1"
origDesc := "folder desc"
f, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: origTitle,
Description: origDesc,
Title: folderTitle,
Description: folderDsc,
OrgID: orgID,
UID: util.GenerateShortUID(),
})
@ -300,12 +301,10 @@ func TestIntegrationGet(t *testing.T) {
orgID := CreateOrg(t, db)
// create folder
title1 := "folder1"
desc1 := "folder desc"
uid1 := util.GenerateShortUID()
f, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: title1,
Description: desc1,
Title: folderTitle,
Description: folderDsc,
OrgID: orgID,
UID: uid1,
})
@ -381,12 +380,10 @@ func TestIntegrationGetParents(t *testing.T) {
orgID := CreateOrg(t, db)
// create folder
title1 := "folder1"
desc1 := "folder desc"
uid1 := util.GenerateShortUID()
f, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: title1,
Description: desc1,
Title: folderTitle,
Description: folderDsc,
OrgID: orgID,
UID: uid1,
})
@ -450,12 +447,10 @@ func TestIntegrationGetChildren(t *testing.T) {
orgID := CreateOrg(t, db)
// create folder
title1 := "folder1"
desc1 := "folder desc"
uid1 := util.GenerateShortUID()
parent, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: title1,
Description: desc1,
Title: folderTitle,
Description: folderDsc,
OrgID: orgID,
UID: uid1,
})
@ -565,6 +560,40 @@ func TestIntegrationGetChildren(t *testing.T) {
})
}
func TestIntegrationGetHeight(t *testing.T) {
if testing.Short() {
t.Skip("skipping integration test")
}
t.Skip("skipping until folder migration is merged")
db := sqlstore.InitTestDB(t)
folderStore := ProvideStore(db, db.Cfg, &featuremgmt.FeatureManager{})
orgID := CreateOrg(t, db)
// create folder
uid1 := util.GenerateShortUID()
parent, err := folderStore.Create(context.Background(), folder.CreateFolderCommand{
Title: folderTitle,
Description: folderDsc,
OrgID: orgID,
UID: uid1,
})
require.NoError(t, err)
subTree := CreateSubTree(t, folderStore, orgID, parent.UID, 4, "sub")
t.Run("should successfully get height", func(t *testing.T) {
height, err := folderStore.GetHeight(context.Background(), parent.UID, orgID, nil)
require.NoError(t, err)
require.Equal(t, 4, height)
})
t.Run("should failed when the parent folder exist in the subtree", func(t *testing.T) {
_, err = folderStore.GetHeight(context.Background(), parent.UID, orgID, &subTree[0])
require.Error(t, err, folder.ErrCircularReference)
})
}
func CreateOrg(t *testing.T, db *sqlstore.SQLStore) int64 {
t.Helper()
@ -583,18 +612,15 @@ func CreateOrg(t *testing.T, db *sqlstore.SQLStore) int64 {
func CreateSubTree(t *testing.T, store *sqlStore, orgID int64, parentUID string, depth int, prefix string) []string {
t.Helper()
ancestorUIDs := []string{}
ancestorUIDs := []string{parentUID}
for i := 0; i < depth; i++ {
title := fmt.Sprintf("%sfolder-%d", prefix, i)
cmd := folder.CreateFolderCommand{
Title: title,
OrgID: orgID,
ParentUID: parentUID,
ParentUID: ancestorUIDs[len(ancestorUIDs)-1],
UID: util.GenerateShortUID(),
}
if len(ancestorUIDs) > 0 {
cmd.ParentUID = ancestorUIDs[len(ancestorUIDs)-1]
}
f, err := store.Create(context.Background(), cmd)
require.NoError(t, err)
require.Equal(t, title, f.Title)

4
pkg/services/folder/folderimpl/store.go
Unescape View File

@ -27,4 +27,8 @@ type store interface {
// GetChildren returns the set of immediate children folders (depth=1) of the
// given folder.
GetChildren(ctx context.Context, cmd folder.GetTreeQuery) ([]*folder.Folder, error)
// GetHeight returns the height of the folder tree. When parentUID is set, the function would
// verify in the meanwhile that parentUID is not present in the subtree of the folder with the given UID.
GetHeight(ctx context.Context, foldrUID string, orgID int64, parentUID *string) (int, error)
}

21
pkg/services/folder/folderimpl/store_fake.go
Unescape View File

@ -7,12 +7,13 @@ import (
)
type FakeStore struct {
ExpectedFolders []*folder.Folder
ExpectedFolder *folder.Folder
ExpectedError error
CreateCalled bool
DeleteCalled bool
ExpectedChildFolders []*folder.Folder
ExpectedParentFolders []*folder.Folder
ExpectedFolder *folder.Folder
ExpectedError error
ExpectedFolderHeight int
CreateCalled bool
DeleteCalled bool
}
func NewFakeStore() *FakeStore {
@ -44,9 +45,13 @@ func (f *FakeStore) Get(ctx context.Context, cmd folder.GetFolderQuery) (*folder
}
func (f *FakeStore) GetParents(ctx context.Context, cmd folder.GetParentsQuery) ([]*folder.Folder, error) {
return f.ExpectedFolders, f.ExpectedError
return f.ExpectedParentFolders, f.ExpectedError
}
func (f *FakeStore) GetChildren(ctx context.Context, cmd folder.GetTreeQuery) ([]*folder.Folder, error) {
return f.ExpectedFolders, f.ExpectedError
return f.ExpectedChildFolders, f.ExpectedError
}
func (f *FakeStore) GetHeight(ctx context.Context, folderUID string, orgID int64, parentUID *string) (int, error) {
return f.ExpectedFolderHeight, f.ExpectedError
}

1
pkg/services/folder/model.go
Unescape View File

@ -14,6 +14,7 @@ var ErrBadRequest = errutil.NewBase(errutil.StatusBadRequest, "folder.bad-reques
var ErrDatabaseError = errutil.NewBase(errutil.StatusInternal, "folder.database-error")
var ErrInternal = errutil.NewBase(errutil.StatusInternal, "folder.internal")
var ErrFolderTooDeep = errutil.NewBase(errutil.StatusInternal, "folder.too-deep")
var ErrCircularReference = errutil.NewBase(errutil.StatusBadRequest, "folder.circular-reference", errutil.WithPublicMessage("Circular reference detected"))
const (
GeneralFolderUID = "general"

Write Preview
Loading…
Cancel
Save