public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Nested folders: Refactor folder update (#60323)

Browse Source 
* Nested folders: Refactor folder update

* Apply suggestions from code review
This commit is contained in:
Sofia Papagiannaki
2022-12-20 15:00:33 +02:00
committed by GitHub
parent 5d4e35c3d5
commit 55b014974d
15 changed files with 267 additions and 148 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/folder/folderimpl/folder.go
+66 -35
View File
@@ -366,33 +366,33 @@ func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (
return f, nil
}
func (s *Service) Update(ctx context.Context, user *user.SignedInUser, orgID int64, existingUid string, cmd *models.UpdateFolderCommand) (*folder.Folder, error) {
foldr, err := s.legacyUpdate(ctx, user, orgID, existingUid, cmd)
func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) (*folder.Folder, error) {
if cmd.SignedInUser == nil {
return nil, folder.ErrBadRequest.Errorf("missing signed in user")
}
user := cmd.SignedInUser
foldr, err := s.legacyUpdate(ctx, cmd)
if err != nil {
return nil, err
}
if s.features.IsEnabled(featuremgmt.FlagNestedFolders) {
if cmd.Uid != "" {
if !util.IsValidShortUID(cmd.Uid) {
if cmd.NewUID != nil && *cmd.NewUID != "" {
if !util.IsValidShortUID(*cmd.NewUID) {
return nil, dashboards.ErrDashboardInvalidUid
} else if util.IsShortUIDTooLong(cmd.Uid) {
} else if util.IsShortUIDTooLong(*cmd.NewUID) {
return nil, dashboards.ErrDashboardUidTooLong
}
}
getFolder, err := s.store.Get(ctx, folder.GetFolderQuery{
UID: &existingUid,
OrgID: orgID,
})
if err != nil {
return nil, err
}
foldr, err := s.store.Update(ctx, folder.UpdateFolderCommand{
Folder: getFolder,
NewUID: &cmd.Uid,
NewTitle: &cmd.Title,
NewDescription: &cmd.Description,
UID: cmd.UID,
OrgID: cmd.OrgID,
NewUID: cmd.NewUID,
NewTitle: cmd.NewTitle,
NewDescription: cmd.NewDescription,
SignedInUser: user,
})
if err != nil {
return nil, err
@@ -402,10 +402,10 @@ func (s *Service) Update(ctx context.Context, user *user.SignedInUser, orgID int
return foldr, nil
}
func (s *Service) legacyUpdate(ctx context.Context, user *user.SignedInUser, orgID int64, existingUid string, cmd *models.UpdateFolderCommand) (*folder.Folder, error) {
func (s *Service) legacyUpdate(ctx context.Context, cmd *folder.UpdateFolderCommand) (*folder.Folder, error) {
logger := s.log.FromContext(ctx)
query := models.GetDashboardQuery{OrgId: orgID, Uid: existingUid}
query := models.GetDashboardQuery{OrgId: cmd.OrgID, Uid: cmd.UID}
_, err := s.dashboardStore.GetDashboard(ctx, &query)
if err != nil {
return nil, toFolderError(err)
@@ -418,12 +418,17 @@ func (s *Service) legacyUpdate(ctx context.Context, user *user.SignedInUser, org
return nil, dashboards.ErrFolderNotFound
}
cmd.UpdateDashboardModel(dashFolder, orgID, user.UserID)
if cmd.SignedInUser == nil {
return nil, folder.ErrBadRequest.Errorf("missing signed in user")
}
user := cmd.SignedInUser
prepareForUpdate(dashFolder, cmd.OrgID, cmd.SignedInUser.UserID, cmd)
dto := &dashboards.SaveDashboardDTO{
Dashboard: dashFolder,
OrgId: orgID,
User: user,
OrgId: cmd.OrgID,
User: cmd.SignedInUser,
Overwrite: cmd.Overwrite,
}
@@ -438,7 +443,7 @@ func (s *Service) legacyUpdate(ctx context.Context, user *user.SignedInUser, org
}
var foldr *folder.Folder
foldr, err = s.dashboardStore.GetFolderByID(ctx, orgID, dash.Id)
foldr, err = s.dashboardStore.GetFolderByID(ctx, cmd.OrgID, dash.Id)
if err != nil {
return nil, err
}
@@ -449,7 +454,7 @@ func (s *Service) legacyUpdate(ctx context.Context, user *user.SignedInUser, org
Title: foldr.Title,
ID: dash.Id,
UID: dash.Uid,
OrgID: orgID,
OrgID: cmd.OrgID,
}); err != nil {
logger.Error("failed to publish FolderTitleUpdated event", "folder", foldr.Title, "user", user.UserID, "error", err)
}
@@ -457,6 +462,32 @@ func (s *Service) legacyUpdate(ctx context.Context, user *user.SignedInUser, org
return foldr, nil
}
// prepareForUpdate updates an existing dashboard model from command into model for folder update
func prepareForUpdate(dashFolder *models.Dashboard, orgId int64, userId int64, cmd *folder.UpdateFolderCommand) {
dashFolder.OrgId = orgId
title := dashFolder.Title
if cmd.NewTitle != nil && *cmd.NewTitle != "" {
title = *cmd.NewTitle
}
dashFolder.Title = strings.TrimSpace(title)
dashFolder.Data.Set("title", dashFolder.Title)
if cmd.NewUID != nil && *cmd.NewUID != "" {
dashFolder.SetUid(*cmd.NewUID)
}
dashFolder.SetVersion(cmd.Version)
dashFolder.IsFolder = true
if userId == 0 {
userId = -1
}
dashFolder.UpdatedBy = userId
dashFolder.UpdateSlug()
}
func (s *Service) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderCommand) error {
logger := s.log.FromContext(ctx)
if cmd.SignedInUser == nil {
@@ -501,23 +532,20 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol
return nil, folder.ErrBadRequest.Errorf("missing signed in user")
}
foldr, err := s.Get(ctx, &folder.GetFolderQuery{
UID: &cmd.UID,
OrgID: cmd.OrgID,
SignedInUser: cmd.SignedInUser,
})
g, err := guardian.NewByUID(ctx, cmd.UID, cmd.OrgID, cmd.SignedInUser)
if err != nil {
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
if canSave, err := g.CanSave(); err != nil || !canSave {
if err != nil {
return nil, toFolderError(err)
}
return nil, dashboards.ErrFolderAccessDenied
}
// 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)
folderHeight, err := s.store.GetHeight(ctx, cmd.UID, cmd.OrgID, &cmd.NewParentUID)
if err != nil {
return nil, err
}
@@ -533,13 +561,16 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol
// if the current folder is already a parent of newparent, we should return error
for _, parent := range parents {
if parent.UID == foldr.UID {
if parent.UID == cmd.UID {
return nil, folder.ErrCircularReference
}
}
return s.store.Update(ctx, folder.UpdateFolderCommand{
Folder: foldr,
UID: cmd.UID,
OrgID: cmd.OrgID,
NewParentUID: &cmd.NewParentUID,
SignedInUser: cmd.SignedInUser,
})
}
pkg/services/folder/folderimpl/folder_test.go
+50 -11
View File
@@ -10,7 +10,6 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/xorcare/pointer"
"github.com/grafana/grafana/pkg/bus"
"github.com/grafana/grafana/pkg/infra/log"
@@ -96,9 +95,10 @@ func TestIntegrationFolderService(t *testing.T) {
require.Equal(t, err, dashboards.ErrFolderAccessDenied)
})
var zeroInt int64 = 0
t.Run("When get folder by id, with id = 0 should return default folder", func(t *testing.T) {
foldr, err := service.Get(context.Background(), &folder.GetFolderQuery{
ID: pointer.Int64(0),
ID: &zeroInt,
OrgID: orgID,
SignedInUser: usr,
})
@@ -126,15 +126,18 @@ func TestIntegrationFolderService(t *testing.T) {
require.Equal(t, err, dashboards.ErrFolderAccessDenied)
})
title := "Folder-TEST"
t.Run("When updating folder should return access denied error", func(t *testing.T) {
dashStore.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) {
folder := args.Get(1).(*models.GetDashboardQuery)
folder.Result = models.NewDashboard("dashboard-test")
folder.Result.IsFolder = true
}).Return(&models.Dashboard{}, nil)
_, err := service.Update(context.Background(), usr, orgID, folderUID, &models.UpdateFolderCommand{
Uid: folderUID,
Title: "Folder-TEST",
_, err := service.Update(context.Background(), &folder.UpdateFolderCommand{
UID: folderUID,
OrgID: orgID,
NewTitle: &title,
SignedInUser: usr,
})
require.Equal(t, err, dashboards.ErrFolderAccessDenied)
})
@@ -207,12 +210,15 @@ func TestIntegrationFolderService(t *testing.T) {
dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(dashboardFolder, nil)
dashStore.On("GetFolderByID", mock.Anything, orgID, dashboardFolder.Id).Return(f, nil)
req := &models.UpdateFolderCommand{
Uid: dashboardFolder.Uid,
Title: "TEST-Folder",
title := "TEST-Folder"
req := &folder.UpdateFolderCommand{
UID: dashboardFolder.Uid,
OrgID: orgID,
NewTitle: &title,
SignedInUser: usr,
}
reqResult, err := service.Update(context.Background(), usr, orgID, dashboardFolder.Uid, req)
reqResult, err := service.Update(context.Background(), req)
require.NoError(t, err)
require.Equal(t, f, reqResult)
})
@@ -393,7 +399,7 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) {
func TestNestedFolderService(t *testing.T) {
t.Run("with feature flag unset", func(t *testing.T) {
store := &FakeStore{}
store := NewFakeStore()
dashStore := dashboards.FakeDashboardStore{}
dashboardsvc := dashboards.FakeDashboardService{}
// nothing enabled yet
@@ -449,7 +455,7 @@ func TestNestedFolderService(t *testing.T) {
})
t.Run("with nested folder feature flag on", func(t *testing.T) {
store := &FakeStore{}
store := NewFakeStore()
dashStore := &dashboards.FakeDashboardStore{}
dashboardsvc := &dashboards.FakeDashboardService{}
// nothing enabled yet
@@ -583,6 +589,34 @@ func TestNestedFolderService(t *testing.T) {
})
})
t.Run("move, no view permission should fail", func(t *testing.T) {
// This test creates and deletes the dashboard, so needs some extra setup.
g := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanViewValue: false})
t.Cleanup(func() {
guardian.New = g
})
store.ExpectedError = nil
store.ExpectedFolder = &folder.Folder{UID: "myFolder", ParentUID: "newFolder"}
_, err := foldersvc.Move(context.Background(), &folder.MoveFolderCommand{UID: "myFolder", NewParentUID: "newFolder", OrgID: orgID, SignedInUser: usr})
require.Error(t, err, dashboards.ErrFolderAccessDenied)
})
t.Run("move, no save permission should fail", func(t *testing.T) {
// This test creates and deletes the dashboard, so needs some extra setup.
g := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: false, CanViewValue: true})
t.Cleanup(func() {
guardian.New = g
})
store.ExpectedError = nil
store.ExpectedFolder = &folder.Folder{UID: "myFolder", ParentUID: "newFolder"}
_, err := foldersvc.Move(context.Background(), &folder.MoveFolderCommand{UID: "myFolder", NewParentUID: "newFolder", OrgID: orgID, SignedInUser: usr})
require.Error(t, err, dashboards.ErrFolderAccessDenied)
})
t.Run("move, no error", func(t *testing.T) {
// This test creates and deletes the dashboard, so needs some extra setup.
g := guardian.New
@@ -593,6 +627,11 @@ func TestNestedFolderService(t *testing.T) {
store.ExpectedError = nil
store.ExpectedFolder = &folder.Folder{UID: "myFolder", ParentUID: "newFolder"}
store.ExpectedParentFolders = []*folder.Folder{
{UID: "newFolder", ParentUID: "newFolder"},
{UID: "newFolder2", ParentUID: "newFolder2"},
{UID: "newFolder3", ParentUID: "newFolder3"},
}
f, err := foldersvc.Move(context.Background(), &folder.MoveFolderCommand{UID: "myFolder", NewParentUID: "newFolder", OrgID: orgID, SignedInUser: usr})
require.NoError(t, err)
require.NotNil(t, f)
pkg/services/folder/folderimpl/sqlstore.go
+23 -14
View File
@@ -91,33 +91,34 @@ func (ss *sqlStore) Delete(ctx context.Context, uid string, orgID int64) error {
}
func (ss *sqlStore) Update(ctx context.Context, cmd folder.UpdateFolderCommand) (*folder.Folder, error) {
if cmd.Folder == nil {
return nil, folder.ErrBadRequest.Errorf("invalid update command: missing folder")
}
updated := time.Now()
uid := cmd.UID
cmd.Folder.Updated = time.Now()
existingUID := cmd.Folder.UID
var foldr *folder.Folder
err := ss.db.WithDbSession(ctx, func(sess *db.Session) error {
sql := strings.Builder{}
sql.Write([]byte("UPDATE folder SET "))
columnsToUpdate := []string{"updated = ?"}
args := []interface{}{cmd.Folder.Updated}
args := []interface{}{updated}
if cmd.NewDescription != nil {
columnsToUpdate = append(columnsToUpdate, "description = ?")
cmd.Folder.Description = *cmd.NewDescription
args = append(args, cmd.Folder.Description)
args = append(args, *cmd.NewDescription)
}
if cmd.NewTitle != nil {
columnsToUpdate = append(columnsToUpdate, "title = ?")
cmd.Folder.Title = *cmd.NewTitle
args = append(args, cmd.Folder.Title)
args = append(args, *cmd.NewTitle)
}
if cmd.NewUID != nil {
columnsToUpdate = append(columnsToUpdate, "uid = ?")
cmd.Folder.UID = *cmd.NewUID
args = append(args, cmd.Folder.UID)
uid = *cmd.NewUID
args = append(args, *cmd.NewUID)
}
if cmd.NewParentUID != nil {
columnsToUpdate = append(columnsToUpdate, "parent_uid = ?")
args = append(args, *cmd.NewParentUID)
}
if len(columnsToUpdate) == 0 {
@@ -126,7 +127,7 @@ func (ss *sqlStore) Update(ctx context.Context, cmd folder.UpdateFolderCommand)
sql.Write([]byte(strings.Join(columnsToUpdate, ", ")))
sql.Write([]byte(" WHERE uid = ? AND org_id = ?"))
args = append(args, existingUID, cmd.Folder.OrgID)
args = append(args, cmd.UID, cmd.OrgID)
args = append([]interface{}{sql.String()}, args...)
@@ -142,10 +143,18 @@ func (ss *sqlStore) Update(ctx context.Context, cmd folder.UpdateFolderCommand)
if affected == 0 {
return folder.ErrInternal.Errorf("no folders are updated")
}
foldr, err = ss.Get(ctx, folder.GetFolderQuery{
UID: &uid,
OrgID: cmd.OrgID,
})
if err != nil {
return err
}
return nil
})
return cmd.Folder, err
return foldr, err
}
func (ss *sqlStore) Get(ctx context.Context, q folder.GetFolderQuery) (*folder.Folder, error) {
pkg/services/folder/folderimpl/sqlstore_test.go
+15 -9
View File
@@ -9,7 +9,6 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/folder"
"github.com/grafana/grafana/pkg/services/org"
@@ -156,7 +155,7 @@ func TestIntegrationDelete(t *testing.T) {
})
*/
ancestorUIDs := CreateSubTree(t, folderStore, orgID, accesscontrol.GeneralFolderUID, folder.MaxNestedFolderDepth, "")
ancestorUIDs := CreateSubTree(t, folderStore, orgID, "", folder.MaxNestedFolderDepth, "")
require.Len(t, ancestorUIDs, folder.MaxNestedFolderDepth)
t.Cleanup(func() {
@@ -235,9 +234,7 @@ func TestIntegrationUpdate(t *testing.T) {
_, err = folderStore.Update(context.Background(), folder.UpdateFolderCommand{})
require.Error(t, err)
_, err = folderStore.Update(context.Background(), folder.UpdateFolderCommand{
Folder: &folder.Folder{},
})
_, err = folderStore.Update(context.Background(), folder.UpdateFolderCommand{})
require.Error(t, err)
})
@@ -246,7 +243,8 @@ func TestIntegrationUpdate(t *testing.T) {
newDesc := "new desc"
// existingUpdated := f.Updated
updated, err := folderStore.Update(context.Background(), folder.UpdateFolderCommand{
Folder: f,
UID: f.UID,
OrgID: f.OrgID,
NewTitle: &newTitle,
NewDescription: &newDesc,
})
@@ -264,6 +262,8 @@ func TestIntegrationUpdate(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, newTitle, updated.Title)
assert.Equal(t, newDesc, updated.Description)
f = updated
})
t.Run("updating folder UID should succeed", func(t *testing.T) {
@@ -271,7 +271,8 @@ func TestIntegrationUpdate(t *testing.T) {
existingTitle := f.Title
existingDesc := f.Description
updated, err := folderStore.Update(context.Background(), folder.UpdateFolderCommand{
Folder: f,
UID: f.UID,
OrgID: f.OrgID,
NewUID: &newUID,
})
require.NoError(t, err)
@@ -641,13 +642,16 @@ 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{parentUID}
ancestorUIDs := []string{}
if parentUID != "" {
ancestorUIDs = append(ancestorUIDs, parentUID)
}
for i := 0; i < depth; i++ {
title := fmt.Sprintf("%sfolder-%d", prefix, i)
cmd := folder.CreateFolderCommand{
Title: title,
OrgID: orgID,
ParentUID: ancestorUIDs[len(ancestorUIDs)-1],
ParentUID: parentUID,
UID: util.GenerateShortUID(),
}
f, err := store.Create(context.Background(), cmd)
@@ -668,6 +672,8 @@ func CreateSubTree(t *testing.T, store *sqlStore, orgID int64, parentUID string,
require.Equal(t, ancestorUIDs, parentUIDs)
ancestorUIDs = append(ancestorUIDs, f.UID)
parentUID = f.UID
}
return ancestorUIDs
pkg/services/folder/folderimpl/store_fake.go
+12 -12
View File
@@ -6,7 +6,7 @@ import (
"github.com/grafana/grafana/pkg/services/folder"
)
type FakeStore struct {
type fakeStore struct {
ExpectedChildFolders []*folder.Folder
ExpectedParentFolders []*folder.Folder
ExpectedFolder *folder.Folder
@@ -16,42 +16,42 @@ type FakeStore struct {
DeleteCalled bool
}
func NewFakeStore() *FakeStore {
return &FakeStore{}
func NewFakeStore() *fakeStore {
return &fakeStore{}
}
var _ store = (*FakeStore)(nil)
var _ store = (*fakeStore)(nil)
func (f *FakeStore) Create(ctx context.Context, cmd folder.CreateFolderCommand) (*folder.Folder, error) {
func (f *fakeStore) Create(ctx context.Context, cmd folder.CreateFolderCommand) (*folder.Folder, error) {
f.CreateCalled = true
return f.ExpectedFolder, f.ExpectedError
}
func (f *FakeStore) Delete(ctx context.Context, uid string, orgID int64) error {
func (f *fakeStore) Delete(ctx context.Context, uid string, orgID int64) error {
f.DeleteCalled = true
return f.ExpectedError
}
func (f *FakeStore) Update(ctx context.Context, cmd folder.UpdateFolderCommand) (*folder.Folder, error) {
func (f *fakeStore) Update(ctx context.Context, cmd folder.UpdateFolderCommand) (*folder.Folder, error) {
return f.ExpectedFolder, f.ExpectedError
}
func (f *FakeStore) Move(ctx context.Context, cmd folder.MoveFolderCommand) error {
func (f *fakeStore) Move(ctx context.Context, cmd folder.MoveFolderCommand) error {
return f.ExpectedError
}
func (f *FakeStore) Get(ctx context.Context, cmd folder.GetFolderQuery) (*folder.Folder, error) {
func (f *fakeStore) Get(ctx context.Context, cmd folder.GetFolderQuery) (*folder.Folder, error) {
return f.ExpectedFolder, f.ExpectedError
}
func (f *FakeStore) GetParents(ctx context.Context, cmd folder.GetParentsQuery) ([]*folder.Folder, error) {
func (f *fakeStore) GetParents(ctx context.Context, cmd folder.GetParentsQuery) ([]*folder.Folder, error) {
return f.ExpectedParentFolders, f.ExpectedError
}
func (f *FakeStore) GetChildren(ctx context.Context, cmd folder.GetChildrenQuery) ([]*folder.Folder, error) {
func (f *fakeStore) GetChildren(ctx context.Context, cmd folder.GetChildrenQuery) ([]*folder.Folder, error) {
return f.ExpectedChildFolders, f.ExpectedError
}
func (f *FakeStore) GetHeight(ctx context.Context, folderUID string, orgID int64, parentUID *string) (int, error) {
func (f *fakeStore) GetHeight(ctx context.Context, folderUID string, orgID int64, parentUID *string) (int, error) {
return f.ExpectedFolderHeight, f.ExpectedError
}