From accb4dea55b4d423020125f1135929b5fa222e14 Mon Sep 17 00:00:00 2001 From: ying-jeanne <74549700+ying-jeanne@users.noreply.github.com> Date: Thu, 10 Nov 2022 09:42:32 +0100 Subject: [PATCH] [Nested Folder] Delete folder methode (#58444) * transfer DeleteFolder changes from larger PR * finish some thingies * add the simplest delete logics * some intermedia steps * fix tests * add test * fix some comments Co-authored-by: yangkb09 --- pkg/api/folder.go | 11 +- pkg/api/folder_test.go | 8 +- pkg/services/folder/folderimpl/folder.go | 59 ++++++---- pkg/services/folder/folderimpl/folder_test.go | 105 ++++++++++++++---- pkg/services/folder/folderimpl/sqlstore.go | 10 -- pkg/services/folder/folderimpl/store_fake.go | 2 + pkg/services/folder/foldertest/foldertest.go | 4 +- pkg/services/folder/model.go | 5 +- pkg/services/folder/service.go | 2 +- .../api/alerting/api_alertmanager_test.go | 3 +- 10 files changed, 139 insertions(+), 70 deletions(-) diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 0e5e1cf6746..1676127712b 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -2,7 +2,6 @@ package api import ( "errors" - "fmt" "net/http" "strconv" @@ -11,9 +10,9 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/libraryelements" - "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" ) @@ -175,16 +174,12 @@ func (hs *HTTPServer) DeleteFolder(c *models.ReqContext) response.Response { // } uid := web.Params(c.Req)[":uid"] - f, err := hs.folderService.DeleteFolder(c.Req.Context(), c.SignedInUser, c.OrgID, uid, c.QueryBool("forceDeleteRules")) + err = hs.folderService.DeleteFolder(c.Req.Context(), &folder.DeleteFolderCommand{UID: uid, OrgID: c.OrgID, ForceDeleteRules: c.QueryBool("forceDeleteRules")}) if err != nil { return apierrors.ToFolderErrorResponse(err) } - return response.JSON(http.StatusOK, util.DynMap{ - "title": f.Title, - "message": fmt.Sprintf("Folder %s deleted", f.Title), - "id": f.Id, - }) + return response.JSON(http.StatusOK, "") } func (hs *HTTPServer) toFolderDto(c *models.ReqContext, g guardian.DashboardGuardian, folder *models.Folder) dtos.Folder { diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index bc7ace258a9..2daafa0b274 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -308,7 +308,7 @@ type fakeFolderService struct { CreateFolderError error UpdateFolderResult *models.Folder UpdateFolderError error - DeleteFolderResult *models.Folder + DeleteFolderResult *folder.Folder DeleteFolderError error DeletedFolderUids []string } @@ -334,7 +334,7 @@ func (s *fakeFolderService) UpdateFolder(ctx context.Context, user *user.SignedI return s.UpdateFolderError } -func (s *fakeFolderService) DeleteFolder(ctx context.Context, user *user.SignedInUser, orgID int64, uid string, forceDeleteRules bool) (*models.Folder, error) { - s.DeletedFolderUids = append(s.DeletedFolderUids, uid) - return s.DeleteFolderResult, s.DeleteFolderError +func (s *fakeFolderService) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderCommand) error { + s.DeletedFolderUids = append(s.DeletedFolderUids, cmd.UID) + return s.DeleteFolderError } diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index f779dc7dba4..d0d19537dc9 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/events" + "github.com/grafana/grafana/pkg/infra/appcontext" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" @@ -232,7 +233,8 @@ func (s *Service) CreateFolder(ctx context.Context, user *user.SignedInUser, org // We'll log the error and also roll back the previously-created // (legacy) folder. s.log.Error("error saving folder to nested folder store", err) - _, err = s.DeleteFolder(ctx, user, orgID, createdFolder.Uid, true) + + err = s.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: createdFolder.Uid, OrgID: orgID, ForceDeleteRules: true}) if err != nil { s.log.Error("error deleting folder after failed save to nested folder store", err) } @@ -298,27 +300,37 @@ func (s *Service) UpdateFolder(ctx context.Context, user *user.SignedInUser, org return nil } -func (s *Service) DeleteFolder(ctx context.Context, user *user.SignedInUser, orgID int64, uid string, forceDeleteRules bool) (*models.Folder, error) { - dashFolder, err := s.dashboardStore.GetFolderByUID(ctx, orgID, uid) +func (s *Service) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderCommand) error { + if s.features.IsEnabled(featuremgmt.FlagNestedFolders) { + err := s.Delete(ctx, cmd) + if err != nil { + s.log.Error("the delete folder on folder table failed with err: ", err.Error()) + } + } + user, err := appcontext.User(ctx) if err != nil { - return nil, err + return err } - guard := guardian.New(ctx, dashFolder.Id, orgID, user) + dashFolder, err := s.dashboardStore.GetFolderByUID(ctx, cmd.OrgID, cmd.UID) + if err != nil { + return err + } + + guard := guardian.New(ctx, dashFolder.Id, cmd.OrgID, user) if canSave, err := guard.CanDelete(); err != nil || !canSave { if err != nil { - return nil, toFolderError(err) + return toFolderError(err) } - return nil, dashboards.ErrFolderAccessDenied + return dashboards.ErrFolderAccessDenied } - deleteCmd := models.DeleteDashboardCommand{OrgId: orgID, Id: dashFolder.Id, ForceDeleteFolderRules: forceDeleteRules} + deleteCmd := models.DeleteDashboardCommand{OrgId: cmd.OrgID, Id: dashFolder.Id, ForceDeleteFolderRules: cmd.ForceDeleteRules} if err := s.dashboardStore.DeleteDashboard(ctx, &deleteCmd); err != nil { - return nil, toFolderError(err) + return toFolderError(err) } - - return dashFolder, nil + return nil } func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (*folder.Folder, error) { @@ -354,23 +366,30 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol }) } -func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) (*folder.Folder, error) { - // check the flag, if old - do whatever did before - // for new only the store - // check if dashboard exists - - foldr, err := s.Get(ctx, &folder.GetFolderQuery{ +func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) error { + _, err := s.Get(ctx, &folder.GetFolderQuery{ UID: &cmd.UID, OrgID: cmd.OrgID, }) if err != nil { - return nil, err + return err + } + + folders, err := s.store.GetChildren(ctx, folder.GetTreeQuery{UID: cmd.UID, OrgID: cmd.OrgID}) + if err != nil { + return err + } + for _, f := range folders { + err := s.Delete(ctx, &folder.DeleteFolderCommand{UID: f.UID, OrgID: f.OrgID, ForceDeleteRules: cmd.ForceDeleteRules}) + if err != nil { + return err + } } err = s.store.Delete(ctx, cmd.UID, cmd.OrgID) if err != nil { - return nil, err + return err } - return foldr, nil + return nil } func (s *Service) Get(ctx context.Context, cmd *folder.GetFolderQuery) (*folder.Folder, error) { diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index f1b3d01db34..73e9256e784 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -28,7 +28,7 @@ import ( ) var orgID = int64(1) -var usr = &user.SignedInUser{UserID: 1} +var usr = &user.SignedInUser{UserID: 1, OrgID: orgID} func TestIntegrationProvideFolderService(t *testing.T) { if testing.Short() { @@ -79,12 +79,12 @@ func TestIntegrationFolderService(t *testing.T) { folderId := rand.Int63() folderUID := util.GenerateShortUID() - folder := models.NewFolder("Folder") - folder.Id = folderId - folder.Uid = folderUID + newFolder := models.NewFolder("Folder") + newFolder.Id = folderId + newFolder.Uid = folderUID - dashStore.On("GetFolderByID", mock.Anything, orgID, folderId).Return(folder, nil) - dashStore.On("GetFolderByUID", mock.Anything, orgID, folderUID).Return(folder, nil) + dashStore.On("GetFolderByID", mock.Anything, orgID, folderId).Return(newFolder, nil) + dashStore.On("GetFolderByUID", mock.Anything, orgID, folderUID).Return(newFolder, nil) t.Run("When get folder by id should return access denied error", func(t *testing.T) { _, err := service.GetFolderByID(context.Background(), usr, folderId, orgID) @@ -104,7 +104,7 @@ func TestIntegrationFolderService(t *testing.T) { t.Run("When creating folder should return access denied error", func(t *testing.T) { dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*models.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil).Times(2) - _, err := service.CreateFolder(context.Background(), usr, orgID, folder.Title, folderUID) + _, err := service.CreateFolder(context.Background(), usr, orgID, newFolder.Title, folderUID) require.Equal(t, err, dashboards.ErrFolderAccessDenied) }) @@ -122,7 +122,20 @@ func TestIntegrationFolderService(t *testing.T) { }) t.Run("When deleting folder by uid should return access denied error", func(t *testing.T) { - _, err := service.DeleteFolder(context.Background(), usr, orgID, folderUID, false) + ctx := context.Background() + ctx = appcontext.WithUser(ctx, usr) + + newFolder := models.NewFolder("Folder") + newFolder.Uid = folderUID + + dashStore.On("GetFolderByID", mock.Anything, orgID, folderId).Return(newFolder, nil) + dashStore.On("GetFolderByUID", mock.Anything, orgID, folderUID).Return(newFolder, nil) + + err := service.DeleteFolder(ctx, &folder.DeleteFolderCommand{ + UID: folderUID, + OrgID: orgID, + ForceDeleteRules: false, + }) require.Error(t, err) require.Equal(t, err, dashboards.ErrFolderAccessDenied) }) @@ -190,7 +203,13 @@ func TestIntegrationFolderService(t *testing.T) { }).Return(nil).Once() expectedForceDeleteRules := rand.Int63()%2 == 0 - _, err := service.DeleteFolder(context.Background(), usr, orgID, f.Uid, expectedForceDeleteRules) + ctx := context.Background() + ctx = appcontext.WithUser(ctx, usr) + err := service.DeleteFolder(ctx, &folder.DeleteFolderCommand{ + UID: f.Uid, + OrgID: orgID, + ForceDeleteRules: expectedForceDeleteRules, + }) require.NoError(t, err) require.NotNil(t, actualCmd) require.Equal(t, f.Id, actualCmd.Id) @@ -288,7 +307,7 @@ func TestFolderService(t *testing.T) { t.Run("delete folder", func(t *testing.T) { folderStore.ExpectedFolder = &folder.Folder{} - _, err := folderService.Delete(context.Background(), &folder.DeleteFolderCommand{}) + err := folderService.Delete(context.Background(), &folder.DeleteFolderCommand{}) require.NoError(t, err) }) @@ -334,7 +353,7 @@ func TestFolderService(t *testing.T) { }) } -func TestCreate_NestedFolders(t *testing.T) { +func TestNestedFolderService(t *testing.T) { t.Run("with feature flag unset", func(t *testing.T) { ctx := appcontext.WithUser(context.Background(), usr) store := &FakeStore{} @@ -354,17 +373,39 @@ func TestCreate_NestedFolders(t *testing.T) { features: features, } - // dashboard store & service commands that should be called. - dashboardsvc.On("BuildSaveDashboardCommand", - mock.Anything, mock.AnythingOfType("*dashboards.SaveDashboardDTO"), - mock.AnythingOfType("bool"), mock.AnythingOfType("bool")).Return(&models.SaveDashboardCommand{}, nil) - dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(&models.Dashboard{}, nil) - dashStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&models.Folder{}, nil) + t.Run("When create folder, no create in folder table done", func(t *testing.T) { + // dashboard store & service commands that should be called. + dashboardsvc.On("BuildSaveDashboardCommand", + mock.Anything, mock.AnythingOfType("*dashboards.SaveDashboardDTO"), + mock.AnythingOfType("bool"), mock.AnythingOfType("bool")).Return(&models.SaveDashboardCommand{}, nil) + dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("models.SaveDashboardCommand")).Return(&models.Dashboard{}, nil) + dashStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&models.Folder{}, nil) - _, err := foldersvc.CreateFolder(ctx, usr, orgID, "myFolder", "myFolder") - require.NoError(t, err) - // CreateFolder should not call the folder store create if the feature toggle is not enabled. - require.False(t, store.CreateCalled) + _, err := foldersvc.CreateFolder(ctx, usr, orgID, "myFolder", "myFolder") + require.NoError(t, err) + // CreateFolder should not call the folder store create if the feature toggle is not enabled. + require.False(t, store.CreateCalled) + }) + + t.Run("When delete folder, no delete in folder table done", func(t *testing.T) { + var actualCmd *models.DeleteDashboardCommand + dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { + actualCmd = args.Get(1).(*models.DeleteDashboardCommand) + }).Return(nil).Once() + dashStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&models.Folder{}, nil) + + g := guardian.New + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true}) + + err := foldersvc.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: "myFolder", OrgID: orgID}) + require.NoError(t, err) + require.NotNil(t, actualCmd) + + t.Cleanup(func() { + guardian.New = g + }) + require.False(t, store.DeleteCalled) + }) }) t.Run("with nested folder feature flag on", func(t *testing.T) { @@ -426,5 +467,27 @@ func TestCreate_NestedFolders(t *testing.T) { guardian.New = g }) }) + + 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) { + actualCmd = args.Get(1).(*models.DeleteDashboardCommand) + }).Return(nil).Once() + dashStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&models.Folder{}, nil) + + g := guardian.New + guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true}) + + store.ExpectedError = nil + + err := foldersvc.DeleteFolder(ctx, &folder.DeleteFolderCommand{UID: "myFolder", OrgID: orgID}) + require.NoError(t, err) + require.NotNil(t, actualCmd) + + t.Cleanup(func() { + guardian.New = g + }) + require.True(t, store.DeleteCalled) + }) }) } diff --git a/pkg/services/folder/folderimpl/sqlstore.go b/pkg/services/folder/folderimpl/sqlstore.go index 1a6351ab41a..4dbbafb0497 100644 --- a/pkg/services/folder/folderimpl/sqlstore.go +++ b/pkg/services/folder/folderimpl/sqlstore.go @@ -79,15 +79,6 @@ func (ss *sqlStore) Delete(ctx context.Context, uid string, orgID int64) error { if err != nil { return folder.ErrDatabaseError.Errorf("failed to delete folder: %w", err) } - /* - affected, err := res.RowsAffected() - if err != nil { - return folder.ErrDatabaseError.Errorf("failed to get affected rows: %w", err) - } - if affected == 0 { - return folder.ErrFolderNotFound.Errorf("folder not found uid:%s org_id:%d", uid, orgID) - } - */ return nil }) } @@ -161,7 +152,6 @@ func (ss *sqlStore) Get(ctx context.Context, q folder.GetFolderQuery) (*folder.F err := ss.db.WithDbSession(ctx, func(sess *db.Session) error { exists := false var err error - switch { case q.ID != nil: exists, err = sess.SQL("SELECT * FROM folder WHERE id = ?", q.ID).Get(foldr) diff --git a/pkg/services/folder/folderimpl/store_fake.go b/pkg/services/folder/folderimpl/store_fake.go index d18351437a6..8fa2a9c1beb 100644 --- a/pkg/services/folder/folderimpl/store_fake.go +++ b/pkg/services/folder/folderimpl/store_fake.go @@ -12,6 +12,7 @@ type FakeStore struct { ExpectedError error CreateCalled bool + DeleteCalled bool } func NewFakeStore() *FakeStore { @@ -26,6 +27,7 @@ func (f *FakeStore) Create(ctx context.Context, cmd folder.CreateFolderCommand) } func (f *FakeStore) Delete(ctx context.Context, uid string, orgID int64) error { + f.DeleteCalled = true return f.ExpectedError } diff --git a/pkg/services/folder/foldertest/foldertest.go b/pkg/services/folder/foldertest/foldertest.go index dedd9e1bf15..5783963c4f8 100644 --- a/pkg/services/folder/foldertest/foldertest.go +++ b/pkg/services/folder/foldertest/foldertest.go @@ -35,8 +35,8 @@ func (s *FakeService) UpdateFolder(ctx context.Context, user *user.SignedInUser, cmd.Result = s.ExpectedFolder return s.ExpectedError } -func (s *FakeService) DeleteFolder(ctx context.Context, user *user.SignedInUser, orgID int64, uid string, forceDeleteRules bool) (*models.Folder, error) { - return s.ExpectedFolder, s.ExpectedError +func (s *FakeService) DeleteFolder(ctx context.Context, cmd *folder.DeleteFolderCommand) error { + return s.ExpectedError } func (s *FakeService) MakeUserAdmin(ctx context.Context, orgID int64, userID, folderID int64, setViewAndEditPermissions bool) error { return s.ExpectedError diff --git a/pkg/services/folder/model.go b/pkg/services/folder/model.go index b8d6ed7a76a..eb1d68e3a11 100644 --- a/pkg/services/folder/model.go +++ b/pkg/services/folder/model.go @@ -78,8 +78,9 @@ type MoveFolderCommand struct { // DeleteFolderCommand captures the information required by the folder service // to delete a folder. type DeleteFolderCommand struct { - UID string `json:"uid" xorm:"uid"` - OrgID int64 `json:"orgId" xorm:"org_id"` + UID string `json:"uid" xorm:"uid"` + OrgID int64 `json:"orgId" xorm:"org_id"` + ForceDeleteRules bool `json:"forceDeleteRules"` } // GetFolderQuery is used for all folder Get requests. Only one of UID, ID, or diff --git a/pkg/services/folder/service.go b/pkg/services/folder/service.go index 1d18ea17890..9675cc51542 100644 --- a/pkg/services/folder/service.go +++ b/pkg/services/folder/service.go @@ -14,7 +14,7 @@ type Service interface { GetFolderByTitle(ctx context.Context, user *user.SignedInUser, orgID int64, title string) (*models.Folder, error) CreateFolder(ctx context.Context, user *user.SignedInUser, orgID int64, title, uid string) (*models.Folder, error) UpdateFolder(ctx context.Context, user *user.SignedInUser, orgID int64, existingUid string, cmd *models.UpdateFolderCommand) error - DeleteFolder(ctx context.Context, user *user.SignedInUser, orgID int64, uid string, forceDeleteRules bool) (*models.Folder, error) + DeleteFolder(ctx context.Context, cmd *DeleteFolderCommand) error MakeUserAdmin(ctx context.Context, orgID int64, userID, folderID int64, setViewAndEditPermissions bool) error } diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index aaf7afb0693..0be39d6e06f 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -799,10 +799,9 @@ func TestDeleteFolderWithRules(t *testing.T) { err := resp.Body.Close() require.NoError(t, err) }) - b, err := io.ReadAll(resp.Body) + _, err = io.ReadAll(resp.Body) require.NoError(t, err) require.Equal(t, 200, resp.StatusCode) - require.JSONEq(t, `{"id":1,"message":"Folder default deleted","title":"default"}`, string(b)) } // Finally, we ensure the rules were deleted.