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

Nested folders: Write to folders table even if the feature toggle is off (#77788)

Browse Source 
* Update folders table even if the feature toggle is off

* Fix failing test

* Apply review feedback

* Revert test changes
This commit is contained in:
Tania
2023-11-10 13:03:00 +01:00
committed by GitHub
parent cb711660f1
commit 6b4337a842
3 changed files with 42 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/folder/folderimpl/folder.go
+34 -35
View File
@@ -424,31 +424,29 @@ func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (
}
var nestedFolder *folder.Folder
if s.features.IsEnabled(featuremgmt.FlagNestedFolders) {
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,
OrgID: createdFolder.OrgID,
}); deleteErr != nil {
logger.Error("error deleting folder after failed save to nested folder store", "error", err)
}
return dashboards.FromDashboard(dash), err
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,
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)
@@ -459,6 +457,8 @@ func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) (
}
func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) (*folder.Folder, error) {
logger := s.log.FromContext(ctx)
if cmd.SignedInUser == nil {
return nil, folder.ErrBadRequest.Errorf("missing signed in user")
}
@@ -469,10 +469,6 @@ func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) (
return nil, err
}
if !s.features.IsEnabled(featuremgmt.FlagNestedFolders) {
return dashFolder, nil
}
foldr, err := s.store.Update(ctx, folder.UpdateFolderCommand{
UID: cmd.UID,
OrgID: cmd.OrgID,
@@ -481,6 +477,11 @@ func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) (
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
}
@@ -611,15 +612,13 @@ func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) e
result := []string{cmd.UID}
err = s.db.InTransaction(ctx, func(ctx context.Context) error {
if s.features.IsEnabled(featuremgmt.FlagNestedFolders) {
subfolders, err := s.nestedFolderDelete(ctx, cmd)
subfolders, err := s.nestedFolderDelete(ctx, cmd)
if err != nil {
logger.Error("the delete folder on folder table failed with err: ", "error", err)
return err
}
result = append(result, subfolders...)
if err != nil {
logger.Error("the delete folder on folder table failed with err: ", "error", err)
return err
}
result = append(result, subfolders...)
dashFolders, err := s.dashboardFolderStore.GetFolders(ctx, cmd.OrgID, result)
if err != nil {
pkg/services/folder/folderimpl/folder_test.go
+7 -5
View File
@@ -178,12 +178,13 @@ func TestIntegrationFolderService(t *testing.T) {
t.Run("Given user has permission to save", func(t *testing.T) {
origNewGuardian := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true})
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true, CanViewValue: true})
service.features = featuremgmt.WithFeatures()
t.Run("When creating folder should not return access denied error", func(t *testing.T) {
dash := dashboards.NewDashboardFolder("Test-Folder")
dash.ID = rand.Int63()
dash.UID = util.GenerateShortUID()
f := dashboards.FromDashboard(dash)
dashStore.On("ValidateDashboardBeforeSave", mock.Anything, mock.AnythingOfType("*dashboards.Dashboard"), mock.AnythingOfType("bool")).Return(true, nil)
@@ -193,7 +194,7 @@ func TestIntegrationFolderService(t *testing.T) {
actualFolder, err := service.Create(context.Background(), &folder.CreateFolderCommand{
OrgID: orgID,
Title: dash.Title,
UID: "someuid",
UID: dash.UID,
SignedInUser: usr,
})
require.NoError(t, err)
@@ -241,6 +242,7 @@ func TestIntegrationFolderService(t *testing.T) {
f.ID = rand.Int63()
f.UID = util.GenerateShortUID()
folderStore.On("GetFolders", mock.Anything, orgID, []string{f.UID}).Return(map[string]*folder.Folder{f.UID: f}, nil)
folderStore.On("GetFolderByUID", mock.Anything, orgID, f.UID).Return(f, nil)
var actualCmd *dashboards.DeleteDashboardCommand
dashStore.On("DeleteDashboard", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
@@ -582,6 +584,7 @@ func TestIntegrationNestedFolderService(t *testing.T) {
depth: 1,
forceDelete: true,
dashboardErr: dashboards.ErrFolderNotFound,
folderErr: folder.ErrFolderNotFound,
libPanelParentErr: model.ErrLibraryElementNotFound,
desc: "With nested folder feature flag off and force deletion of rules",
},
@@ -723,7 +726,7 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) {
func TestNestedFolderService(t *testing.T) {
t.Run("with feature flag unset", func(t *testing.T) {
t.Run("When create folder, no create in folder table done", func(t *testing.T) {
t.Run("Should create a folder in both dashboard and folders tables", func(t *testing.T) {
g := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanSaveValue: true})
t.Cleanup(func() {
@@ -748,8 +751,7 @@ func TestNestedFolderService(t *testing.T) {
SignedInUser: usr,
})
require.NoError(t, err)
// CreateFolder should not call the folder store create if the feature toggle is not enabled.
require.False(t, nestedFolderStore.CreateCalled)
require.True(t, nestedFolderStore.CreateCalled)
})
})
pkg/services/folder/folderimpl/sqlstore.go
+1 -1
View File
@@ -140,7 +140,7 @@ func (ss *sqlStore) Update(ctx context.Context, cmd folder.UpdateFolderCommand)
return folder.ErrInternal.Errorf("failed to get affected row: %w", err)
}
if affected == 0 {
return folder.ErrInternal.Errorf("no folders are updated")
return folder.ErrInternal.Errorf("no folders are updated: %w", folder.ErrFolderNotFound)
}
foldr, err = ss.Get(ctx, folder.GetFolderQuery{