diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index 92a9d0c0501..9fefe7383e7 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -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, }) } diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index eb3d3ab0d2f..06e23d061e8 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -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{ diff --git a/pkg/services/folder/folderimpl/sqlstore.go b/pkg/services/folder/folderimpl/sqlstore.go index 7b5ab9518a6..d66c35894a0 100644 --- a/pkg/services/folder/folderimpl/sqlstore.go +++ b/pkg/services/folder/folderimpl/sqlstore.go @@ -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 +} diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index ee1104b078d..f66c4636eb0 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/pkg/services/folder/folderimpl/sqlstore_test.go @@ -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) diff --git a/pkg/services/folder/folderimpl/store.go b/pkg/services/folder/folderimpl/store.go index 9ba0933e4ab..902fddd7cfe 100644 --- a/pkg/services/folder/folderimpl/store.go +++ b/pkg/services/folder/folderimpl/store.go @@ -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) } diff --git a/pkg/services/folder/folderimpl/store_fake.go b/pkg/services/folder/folderimpl/store_fake.go index 8fa2a9c1beb..25c39096e20 100644 --- a/pkg/services/folder/folderimpl/store_fake.go +++ b/pkg/services/folder/folderimpl/store_fake.go @@ -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 } diff --git a/pkg/services/folder/model.go b/pkg/services/folder/model.go index 12143abef99..8dfc80e3b53 100644 --- a/pkg/services/folder/model.go +++ b/pkg/services/folder/model.go @@ -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"