From 39754ba2d61c7d681ba6af02186cca4b7df68d9a Mon Sep 17 00:00:00 2001 From: Tania Date: Tue, 21 Nov 2023 13:06:20 -0800 Subject: [PATCH] Nested Folders: Wrap create/update operations with transactions (#78000) * Nested Folders: Add transaction to create and update methods * Update tests * Make IncreaseVersionForAllRulesInNamespace synchronous * Resolve merge conflicts --- pkg/services/folder/folderimpl/folder.go | 131 +++++++++--------- pkg/services/folder/folderimpl/folder_test.go | 75 +++++----- pkg/services/ngalert/ngalert.go | 17 +-- 3 files changed, 108 insertions(+), 115 deletions(-) diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index 00a8a99281a..25f5c0f0bc1 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -416,43 +416,35 @@ func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) ( return nil, toFolderError(err) } - dash, err := s.dashboardStore.SaveDashboard(ctx, *saveDashboardCmd) - if err != nil { - return nil, toFolderError(err) - } + var nestedFolder *folder.Folder + var dash *dashboards.Dashboard + err = s.db.InTransaction(ctx, func(ctx context.Context) error { + if dash, err = s.dashboardStore.SaveDashboard(ctx, *saveDashboardCmd); err != nil { + return toFolderError(err) + } - var createdFolder *folder.Folder - createdFolder, err = s.dashboardFolderStore.GetFolderByID(ctx, cmd.OrgID, dash.ID) + cmd = &folder.CreateFolderCommand{ + // TODO: Today, if a UID isn't specified, the dashboard store + // generates a new UID. The new folder store will need to do this as + // well, but for now we take the UID from the newly created folder. + UID: dash.UID, + OrgID: cmd.OrgID, + Title: cmd.Title, + Description: cmd.Description, + ParentUID: cmd.ParentUID, + } + + if nestedFolder, err = s.nestedFolderCreate(ctx, cmd); err != nil { + logger.Error("error saving folder to nested folder store", "error", err) + return err + } + + return nil + }) if err != nil { return nil, err } - var nestedFolder *folder.Folder - cmd = &folder.CreateFolderCommand{ - // TODO: Today, if a UID isn't specified, the dashboard store - // generates a new UID. The new folder store will need to do this as - // well, but for now we take the UID from the newly created folder. - UID: dash.UID, - OrgID: cmd.OrgID, - Title: cmd.Title, - Description: cmd.Description, - ParentUID: cmd.ParentUID, - } - nestedFolder, err = s.nestedFolderCreate(ctx, cmd) - if err != nil { - // We'll log the error and also roll back the previously-created - // (legacy) folder. - logger.Error("error saving folder to nested folder store", "error", err) - // do not shallow create error if the legacy folder delete fails - if deleteErr := s.dashboardStore.DeleteDashboard(ctx, &dashboards.DeleteDashboardCommand{ - ID: createdFolder.ID, // nolint:staticcheck - OrgID: createdFolder.OrgID, - }); deleteErr != nil { - logger.Error("error deleting folder after failed save to nested folder store", "error", err) - } - return dashboards.FromDashboard(dash), err - } - f := dashboards.FromDashboard(dash) if nestedFolder != nil && nestedFolder.ParentUID != "" { f.ParentUID = nestedFolder.ParentUID @@ -468,25 +460,48 @@ func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) ( } user := cmd.SignedInUser - dashFolder, err := s.legacyUpdate(ctx, cmd) + var dashFolder, foldr *folder.Folder + var err error + err = s.db.InTransaction(ctx, func(ctx context.Context) error { + if dashFolder, err = s.legacyUpdate(ctx, cmd); err != nil { + return err + } + + if foldr, err = s.store.Update(ctx, folder.UpdateFolderCommand{ + UID: cmd.UID, + OrgID: cmd.OrgID, + NewTitle: cmd.NewTitle, + NewDescription: cmd.NewDescription, + SignedInUser: user, + }); err != nil { + return err + } + + if cmd.NewTitle != nil { + namespace, id := cmd.SignedInUser.GetNamespacedID() + + if err := s.bus.Publish(context.Background(), &events.FolderTitleUpdated{ + Timestamp: foldr.Updated, + Title: foldr.Title, + ID: dashFolder.ID, // nolint:staticcheck + UID: dashFolder.UID, + OrgID: cmd.OrgID, + }); err != nil { + logger.Error("failed to publish FolderTitleUpdated event", "folder", foldr.Title, "user", id, "namespace", namespace, "error", err) + return err + } + } + + return nil + }) + if err != nil { + logger.Error("folder update failed", "folderUID", cmd.UID, "error", err) return nil, err } - foldr, err := s.store.Update(ctx, folder.UpdateFolderCommand{ - UID: cmd.UID, - OrgID: cmd.OrgID, - NewTitle: cmd.NewTitle, - NewDescription: cmd.NewDescription, - SignedInUser: user, - }) - if err != nil { - if errors.Is(err, folder.ErrFolderNotFound) { - logger.Warn("attempt to update folder that does not exist in the folders table", "folderUID", cmd.UID) - return dashFolder, nil - } - - return nil, err + if !s.features.IsEnabled(ctx, featuremgmt.FlagNestedFolders) { + return dashFolder, nil } // always expose the dashboard store sequential ID @@ -511,8 +526,6 @@ func (s *Service) legacyUpdate(ctx context.Context, cmd *folder.UpdateFolderComm dashFolder.FolderUID = *cmd.NewParentUID } - currentTitle := dashFolder.Title - if !dashFolder.IsFolder { return nil, dashboards.ErrFolderNotFound } @@ -555,17 +568,6 @@ func (s *Service) legacyUpdate(ctx context.Context, cmd *folder.UpdateFolderComm return nil, err } - if currentTitle != foldr.Title { - if err := s.bus.Publish(ctx, &events.FolderTitleUpdated{ - Timestamp: foldr.Updated, - Title: foldr.Title, - ID: dash.ID, - UID: dash.UID, - OrgID: cmd.OrgID, - }); err != nil { - logger.Error("failed to publish FolderTitleUpdated event", "folder", foldr.Title, "user", id, "namespace", namespace, "error", err) - } - } return foldr, nil } @@ -630,10 +632,10 @@ func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) e return folder.ErrInternal.Errorf("failed to fetch subfolders from dashboard store: %w", err) } - for _, folder := range result { - dashFolder, ok := dashFolders[folder] + for _, foldr := range result { + dashFolder, ok := dashFolders[foldr] if !ok { - return err + return folder.ErrInternal.Errorf("folder does not exist in dashboard store") } if cmd.ForceDeleteRules { @@ -642,8 +644,7 @@ func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) e } } - err = s.legacyDelete(ctx, cmd, dashFolder) - if err != nil { + if err = s.legacyDelete(ctx, cmd, dashFolder); err != nil { return err } } diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index 59cedf0dac9..cd2e973e32d 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -191,7 +191,6 @@ func TestIntegrationFolderService(t *testing.T) { dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil) dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(dash, nil).Once() - folderStore.On("GetFolderByID", mock.Anything, orgID, dash.ID).Return(f, nil) actualFolder, err := service.Create(context.Background(), &folder.CreateFolderCommand{ OrgID: orgID, @@ -220,10 +219,21 @@ func TestIntegrationFolderService(t *testing.T) { dashboardFolder := dashboards.NewDashboardFolder("Folder") dashboardFolder.ID = rand.Int63() dashboardFolder.UID = util.GenerateShortUID() + dashboardFolder.OrgID = orgID f := dashboards.FromDashboard(dashboardFolder) + _, err := service.store.Create(context.Background(), folder.CreateFolderCommand{ + OrgID: orgID, + Title: dashboardFolder.Title, + UID: dashboardFolder.UID, + SignedInUser: usr, + }) + require.NoError(t, err) + dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil) dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(dashboardFolder, nil) + dashStore.On("GetDashboard", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardQuery")).Return(dashboardFolder, nil) + folderStore.On("GetFolderByID", mock.Anything, orgID, dashboardFolder.ID).Return(f, nil) title := "TEST-Folder" @@ -714,12 +724,12 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) { dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(&dashboards.Dashboard{}, nil) dashboardFolderStore := foldertest.NewFakeFolderStore(t) - dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) cfg := setting.NewCfg() folderService := &Service{ cfg: cfg, store: nestedFolderStore, + db: sqlstore.InitTestDB(t), dashboardStore: &dashStore, dashboardFolderStore: dashboardFolderStore, features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), @@ -744,21 +754,25 @@ func TestNestedFolderService(t *testing.T) { guardian.New = g }) + // dash is needed here because folderSvc.Create expects SaveDashboard to return it + dash := dashboards.NewDashboardFolder("myFolder") + dash.ID = rand.Int63() + dash.UID = "some_uid" + // dashboard store & service commands that should be called. dashStore := &dashboards.FakeDashboardStore{} dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil) - dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(&dashboards.Dashboard{}, nil) + dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(dash, nil) dashboardFolderStore := foldertest.NewFakeFolderStore(t) - dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) nestedFolderStore := NewFakeStore() - folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures(), acimpl.ProvideAccessControl(setting.NewCfg()), dbtest.NewFakeDB()) + folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures(), acimpl.ProvideAccessControl(setting.NewCfg()), sqlstore.InitTestDB(t)) _, err := folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ OrgID: orgID, - Title: "myFolder", - UID: "myFolder", + Title: dash.Title, + UID: dash.UID, SignedInUser: usr, }) require.NoError(t, err) @@ -774,21 +788,23 @@ func TestNestedFolderService(t *testing.T) { guardian.New = g }) + dash := dashboards.NewDashboardFolder("myFolder") + dash.ID = rand.Int63() + dash.UID = "some_uid" + // dashboard store commands that should be called. dashStore := &dashboards.FakeDashboardStore{} dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil) - dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(&dashboards.Dashboard{}, nil) + dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(dash, nil) dashboardFolderStore := foldertest.NewFakeFolderStore(t) - dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) - nestedFolderStore := NewFakeStore() - folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures("nestedFolders"), acimpl.ProvideAccessControl(setting.NewCfg()), dbtest.NewFakeDB()) + folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures("nestedFolders"), acimpl.ProvideAccessControl(setting.NewCfg()), sqlstore.InitTestDB(t)) _, err := folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ OrgID: orgID, - Title: "myFolder", - UID: "myFolder", + Title: dash.Title, + UID: dash.UID, SignedInUser: usr, }) require.NoError(t, err) @@ -840,17 +856,16 @@ func TestNestedFolderService(t *testing.T) { // dashboard store commands that should be called. dashStore := &dashboards.FakeDashboardStore{} dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil) - dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(&dashboards.Dashboard{}, nil) + dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(dash, nil) dashboardFolderStore := foldertest.NewFakeFolderStore(t) - dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) dashboardFolderStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&folder.Folder{}, nil) nestedFolderUser := &user.SignedInUser{UserID: 1, OrgID: orgID, Permissions: map[int64]map[string][]string{}} nestedFolderUser.Permissions[orgID] = map[string][]string{dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersProvider.GetResourceScopeUID("some_parent")}} nestedFolderStore := NewFakeStore() - folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures("nestedFolders"), acimpl.ProvideAccessControl(setting.NewCfg()), dbtest.NewFakeDB()) + folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures("nestedFolders"), acimpl.ProvideAccessControl(setting.NewCfg()), sqlstore.InitTestDB(t)) _, err := folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ OrgID: orgID, Title: dash.Title, @@ -875,13 +890,11 @@ func TestNestedFolderService(t *testing.T) { dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(&dashboards.Dashboard{UID: "newUID"}, nil) dashboardFolderStore := foldertest.NewFakeFolderStore(t) - dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) - nestedFolderStore := NewFakeStore() folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures("nestedFolders"), actest.FakeAccessControl{ ExpectedEvaluate: true, - }, dbtest.NewFakeDB()) + }, sqlstore.InitTestDB(t)) f, err := folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ OrgID: orgID, Title: "myFolder", @@ -909,13 +922,8 @@ func TestNestedFolderService(t *testing.T) { dashStore := &dashboards.FakeDashboardStore{} dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil) dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(dashboardFolder, nil) - var actualCmd *dashboards.DeleteDashboardCommand - dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { - actualCmd = args.Get(1).(*dashboards.DeleteDashboardCommand) - }).Return(nil).Once() dashboardFolderStore := foldertest.NewFakeFolderStore(t) - dashboardFolderStore.On("GetFolderByID", mock.Anything, orgID, dashboardFolder.ID).Return(f, nil) dashboardFolderStore.On("GetFolderByUID", mock.Anything, orgID, dashboardFolder.UID).Return(f, nil) nestedFolderStore := NewFakeStore() @@ -936,12 +944,11 @@ func TestNestedFolderService(t *testing.T) { folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures("nestedFolders"), actest.FakeAccessControl{ ExpectedEvaluate: true, - }, dbtest.NewFakeDB()) + }, sqlstore.InitTestDB(t)) _, err := folderSvc.Create(context.Background(), &cmd) require.Error(t, err, folder.ErrCircularReference) // CreateFolder should not call the folder store's create method. require.False(t, nestedFolderStore.CreateCalled) - require.NotNil(t, actualCmd) }) t.Run("create returns error from nested folder service", func(t *testing.T) { @@ -956,13 +963,8 @@ func TestNestedFolderService(t *testing.T) { dashStore := &dashboards.FakeDashboardStore{} dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil) dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(&dashboards.Dashboard{}, nil) - var actualCmd *dashboards.DeleteDashboardCommand - dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { - actualCmd = args.Get(1).(*dashboards.DeleteDashboardCommand) - }).Return(nil).Once() dashboardFolderStore := foldertest.NewFakeFolderStore(t) - dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) // return an error from the folder store nestedFolderStore := NewFakeStore() @@ -971,7 +973,7 @@ func TestNestedFolderService(t *testing.T) { // the service return success as long as the legacy create succeeds folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures("nestedFolders"), actest.FakeAccessControl{ ExpectedEvaluate: true, - }, dbtest.NewFakeDB()) + }, sqlstore.InitTestDB(t)) _, err := folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ OrgID: orgID, Title: "myFolder", @@ -982,7 +984,6 @@ func TestNestedFolderService(t *testing.T) { // CreateFolder should also call the folder store's create method. require.True(t, nestedFolderStore.CreateCalled) - require.NotNil(t, actualCmd) }) t.Run("move without the right permissions should fail", func(t *testing.T) { @@ -1141,13 +1142,8 @@ func TestNestedFolderService(t *testing.T) { dashStore := &dashboards.FakeDashboardStore{} dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil).Times(2) dashStore.On("SaveDashboard", mock.Anything, mock.AnythingOfType("dashboards.SaveDashboardCommand")).Return(&dashboards.Dashboard{}, nil) - var actualCmd *dashboards.DeleteDashboardCommand - dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { - actualCmd = args.Get(1).(*dashboards.DeleteDashboardCommand) - }).Return(nil).Once() dashboardFolderStore := foldertest.NewFakeFolderStore(t) - dashboardFolderStore.On("GetFolderByID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("int64")).Return(&folder.Folder{}, nil) dashboardFolderStore.On("GetFolderByUID", mock.Anything, mock.AnythingOfType("int64"), mock.AnythingOfType("string")).Return(&folder.Folder{}, nil) parents := make([]*folder.Folder, 0, folder.MaxNestedFolderDepth) @@ -1161,7 +1157,7 @@ func TestNestedFolderService(t *testing.T) { folderSvc := setup(t, dashStore, dashboardFolderStore, nestedFolderStore, featuremgmt.WithFeatures("nestedFolders"), actest.FakeAccessControl{ ExpectedEvaluate: true, - }, dbtest.NewFakeDB()) + }, sqlstore.InitTestDB(t)) _, err := folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ Title: "folder", OrgID: orgID, @@ -1170,7 +1166,6 @@ func TestNestedFolderService(t *testing.T) { SignedInUser: usr, }) assert.ErrorIs(t, err, folder.ErrMaximumDepthReached) - require.NotNil(t, actualCmd) }) t.Run("get default folder, no error", func(t *testing.T) { diff --git a/pkg/services/ngalert/ngalert.go b/pkg/services/ngalert/ngalert.go index cf9aac166fb..8694893793d 100644 --- a/pkg/services/ngalert/ngalert.go +++ b/pkg/services/ngalert/ngalert.go @@ -329,16 +329,13 @@ func (ng *AlertNG) init() error { func subscribeToFolderChanges(logger log.Logger, bus bus.Bus, dbStore api.RuleStore) { // if folder title is changed, we update all alert rules in that folder to make sure that all peers (in HA mode) will update folder title and // clean up the current state - bus.AddEventListener(func(ctx context.Context, e *events.FolderTitleUpdated) error { - // do not block the upstream execution - go func(evt *events.FolderTitleUpdated) { - logger.Info("Got folder title updated event. updating rules in the folder", "folderUID", evt.UID) - _, err := dbStore.IncreaseVersionForAllRulesInNamespace(ctx, evt.OrgID, evt.UID) - if err != nil { - logger.Error("Failed to update alert rules in the folder after its title was changed", "error", err, "folderUID", evt.UID, "folder", evt.Title) - return - } - }(e) + bus.AddEventListener(func(ctx context.Context, evt *events.FolderTitleUpdated) error { + logger.Info("Got folder title updated event. updating rules in the folder", "folderUID", evt.UID) + _, err := dbStore.IncreaseVersionForAllRulesInNamespace(ctx, evt.OrgID, evt.UID) + if err != nil { + logger.Error("Failed to update alert rules in the folder after its title was changed", "error", err, "folderUID", evt.UID, "folder", evt.Title) + return err + } return nil }) }