NestedFolders: Fix nested folder deletion #63572

ying-jeanne · 2023-02-22T13:38:15Z

What is this feature?

It's a fix for a bug relating to the deletion of nested folders as described in the linked issue.

Which issue(s) does this PR fix?:

Special notes for your reviewer:
I (@suntala) still need to try out the fix by reproducing the issue, but hopefully the new integration test (TestIntegrationDeleteNestedFolders) makes it clear that the fix works. This integration test has also been intended as a replacement for two test cases (1, 2).

Additionally, I'd be curious to know if anyone has suggestions for how to better test deletion when the feature flag is off (old way, new way). The new approach is to create a folder with the feature flag on so that it gets added to both the dashboard table and the folder one. Then we proceed with deletion and check that the entry in the folder table hasn't been removed.

When coming up with this approach, a corner case for this bug became apparent. Someone could theoretically turn the nested folder feature flag on, create some nested folders, turn the flag off and then delete the parent folder. This deletion wouldn't be complete. It would leave behind entries in the folder table. A fix for this would be to modify the legacyDelete method to check the folder table for nested folders when performing a deletion. I didn't implement this because hopefully people wouldn't use the nested folder flag in this way. However, please let me know if you think it would be valuable to do so.

I suggest not going through the commits individually because they aren't well structured and there was a change of approach halfway through.

papagian

I have tested and works as expected! Great work 🎉
I have left some minor suggestions.

How to better handle the corner case - deleting a parent folder (containing subfolders) with nested folders disabled - described by @suntala still an open question though. I'm tempted to suggest a common approach regardless of the feature flag as long as we discover nested folders.

papagian · 2023-03-03T11:34:31Z

pkg/kindsys/report.json

@@ -2008,7 +2008,7 @@
          "zipkindataquery",
          "zipkindatasourcecfg"
        ],
-        "count": 66
+        "count": 67


this change seems irrelevant; do we actually need it?

One of the pipeline steps was failing, the one called verify-gen-cue. There were instructions to run make gen-cue (if I remember correctly) and add the resulting changes to source control. I see that, in the meantime, someone else added the change. I'm going to go ahead and rebase on main to eliminate this change.

In the future, I think a better way of handling it would have been for me to create a separate PR for this issue and rebase this PR on top of it. Happy to hear if you have another suggestion though.

papagian · 2023-03-06T11:40:13Z

pkg/services/folder/folderimpl/sqlstore_test.go

+			_, err := serviceWithFlagOn.getFolderByUID(context.Background(), orgID, uid)
+			require.ErrorIs(t, err, dashboards.ErrFolderNotFound)


this assumes prior knowledge of the folder service getFolderByUID() internal implementation (that it searches in the dashboard store); it would be preferable to call the serviceWithFlagOn.dashboardFolderStore.GetFolderByUID() or directly folderStore.GetFolderByUID() (which is passed to the service)

We do require that the dashboard store be searched and not some other store. To make sure I understand you fully, it's not the prior knowledge of which store is being searched that is of concern here. Rather it is the knowledge of the internal implementation. Is that correct? When using serviceWithFlagOn.dashboardFolderStore.GetFolderByUID(), we don't require internal knowledge of which store is being searched because we know that dashboardFolderStore is dedicated to the dashboard store.

yes, here we want to test if there is no entry in the dashboard table but it requires knowledge of the folder service getFolderByUID() internal implementation to know that it checks the specific store and raise the specific error if no entry is not found there.

papagian · 2023-03-06T12:03:32Z

pkg/services/folder/folderimpl/folder.go

@@ -508,10 +520,11 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol
 	})
 }

-func (s *Service) nestedFolderDelete(ctx context.Context, cmd *folder.DeleteFolderCommand) error {


nit; it would be nice to have a doc string describing what the function returns

suntala · 2023-03-07T08:47:27Z

Hi @papagian, thanks a lot for the review. All your comments make sense to me and I learned some things along the way. I have made the suggested changes.

I'm tempted to suggest a common approach regardless of the feature flag as long as we discover nested folders.

This does seem like the way to go for me as well. I propose tackling it as a separate PR to not make this one bigger. WDYT?

papagian

I have some additional - minor - suggestions

papagian · 2023-03-07T09:16:23Z

pkg/services/folder/folderimpl/folder.go

-func (s *Service) nestedFolderDelete(ctx context.Context, cmd *folder.DeleteFolderCommand) error {
+// nestedFolderDelete inspects the folder referenced by the cmd argument, deletes all the entries for
+// its descendant folders (folders which are nested within it either directly or indirectly) from
+// the folder store and returns the UIDs for all its descendants.


nit

Suggested change

// the folder store and returns the UIDs for all its descendants.

// the folder store and returns the UIDs for all its direct descendants.

As far as I can tell, because nestedFolderDelete is recursive, it returns the UIDs of all its descendants, both direct and indirect. We're going progressively deeper into the nested folders and appending the UIDs of all the subfolders we find:

grafana/pkg/services/folder/folderimpl/folder.go

Lines 546 to 554 in 9a023c3

for _, f := range folders {

result = append(result, f.UID)

logger.Info("deleting subfolder", "org_id", f.OrgID, "uid", f.UID)

subfolders, err := s.nestedFolderDelete(ctx, &folder.DeleteFolderCommand{UID: f.UID, OrgID: f.OrgID, ForceDeleteRules: cmd.ForceDeleteRules, SignedInUser: cmd.SignedInUser})

if err != nil {

logger.Error("failed deleting subfolder", "org_id", f.OrgID, "uid", f.UID, "error", err)

return result, err

}

result = append(result, subfolders...)

Please let me know if I'm missing something though.

you are right, I had in mind only the first iteration

papagian · 2023-03-07T10:30:09Z

pkg/services/folder/folderimpl/sqlstore_test.go

@@ -183,6 +196,113 @@ func TestIntegrationDelete(t *testing.T) {
 	})
 }

+func TestIntegrationDeleteNestedFolders(t *testing.T) {


This file contains only tests for the nested folder store. Therefore I would suggest to move this to the folder service tests (since we test the folder service Delete() behaviour).

papagian · 2023-03-07T10:33:28Z

pkg/services/folder/folderimpl/sqlstore_test.go

@@ -632,30 +756,35 @@ func CreateOrg(t *testing.T, db *sqlstore.SQLStore) int64 {
 	return orgID
 }

-func CreateSubTree(t *testing.T, store *sqlStore, orgID int64, parentUID string, depth int, prefix string) []string {
+func CreateSubTree(t *testing.T, store *sqlStore, service *Service, depth int, prefix string, cmd folder.CreateFolderCommand) []string {


This function has become over-complicated: I would suggest leave this one as as is and introduce a similar that uses the service instead (or the dashboard folder store).

suntala · 2023-03-08T15:03:58Z

Update: we're going to hold off on merging this for one week (source).

amy-super · 2023-03-09T16:00:48Z

@suntala I talked to the User Essentials squad about this and we are all of the mind that we shouldn't do much work to support cases that will come up when users toggle nested folders on/off. Instead, we'll shift our efforts to documenting a recommendation for users to take a backup before playing around with the toggle and document that they might end up with orphaned items that are hidden from the UI if they turn the toggle off after created a nested structure.

Co-authored-by: ying-jeanne <ying-jeanne@users.noreply.github.com> Co-authored-by: jeanne0731 <jeanne0731@users.noreply.github.com>

suntala · 2023-03-14T15:49:44Z

@papagian I made the change you suggested regarding naming in 8e8c21b. I think all the suggested changes have been made. Thanks for the reviews! Do you consider the PR ready to merge?

papagian · 2023-03-14T15:52:02Z

@papagian I made the change you suggested regarding naming 8e8c21b. I think all the changes suggested have been made. Thanks for the reviews! Do you consider the PR ready to merge?

yes, go ahead merging it!

grafanabot added the area/backend label Feb 22, 2023

suntala force-pushed the bugfixingdelete branch 5 times, most recently from ec85cec to 41fb115 Compare March 2, 2023 17:12

suntala marked this pull request as ready for review March 2, 2023 17:48

suntala requested review from a team as code owners March 2, 2023 17:48

suntala requested review from sakjur, papagian and mildwonkey and removed request for a team March 2, 2023 17:48

suntala changed the title ~~bug fixing delete~~ Fix nested folder deletion Mar 2, 2023

suntala added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Mar 2, 2023

suntala added this to the 9.5.0 milestone Mar 2, 2023

suntala changed the title ~~Fix nested folder deletion~~ NestedFolders: Fix nested folder deletion Mar 2, 2023

papagian reviewed Mar 6, 2023

View reviewed changes

suntala force-pushed the bugfixingdelete branch 2 times, most recently from 7156f4c to 9a023c3 Compare March 7, 2023 08:54

papagian reviewed Mar 7, 2023

View reviewed changes

suntala force-pushed the bugfixingdelete branch 2 times, most recently from 33258a6 to ee574dd Compare March 8, 2023 08:25

ying-jeanne and others added 3 commits March 10, 2023 08:37

bug fixing delete

136e5f6

changes during the call

6ac9600

Fix some tests

4bcf901

suntala and others added 12 commits March 10, 2023 08:37

Fix nested folder service tests

60c7a4e

Partial nested folder deletion integration test

d84b6fd

Part 2 nested folder deletion integration test

6a9fa9d

Co-authored-by: ying-jeanne <ying-jeanne@users.noreply.github.com> Co-authored-by: jeanne0731 <jeanne0731@users.noreply.github.com>

Fix TestIntegrationNestedFolderDelete

2b84c82

Update CreateSubTree function

54aa123

Fix verify-gen-cue CI step

7ef2429

Add test for nested deletion with feature flag off

dc3e8ad

Add test cleanup

35bf108

Remove stray comments

08f1cf3

Address review comments

d9f7427

Move TestIntegrationDeleteNestedFolders

cbe48b4

Add a separate method for creating legacy subtrees

ede16ae

suntala force-pushed the bugfixingdelete branch from ee574dd to ede16ae Compare March 10, 2023 07:37

Rename CreateLegacySubtree -> CreateSubtreeInStore

8e8c21b

suntala requested a review from papagian March 14, 2023 15:48

papagian approved these changes Mar 14, 2023

View reviewed changes

suntala added type/bug add to changelog and removed no-changelog Skip including change in changelog/release notes labels Mar 15, 2023

suntala merged commit 6974f43 into main Mar 15, 2023
10 checks passed

suntala deleted the bugfixingdelete branch March 15, 2023 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NestedFolders: Fix nested folder deletion #63572

NestedFolders: Fix nested folder deletion #63572

ying-jeanne commented Feb 22, 2023 •

edited by suntala

papagian left a comment

papagian Mar 3, 2023

suntala Mar 7, 2023

papagian Mar 6, 2023

suntala Mar 7, 2023

papagian Mar 8, 2023

papagian Mar 6, 2023

suntala commented Mar 7, 2023 •

edited

papagian left a comment

papagian Mar 7, 2023

suntala Mar 8, 2023

papagian Mar 8, 2023

papagian Mar 7, 2023

papagian Mar 7, 2023

suntala commented Mar 8, 2023

amy-super commented Mar 9, 2023

suntala commented Mar 14, 2023 •

edited

papagian commented Mar 14, 2023

		_, err := serviceWithFlagOn.getFolderByUID(context.Background(), orgID, uid)
		require.ErrorIs(t, err, dashboards.ErrFolderNotFound)

	// the folder store and returns the UIDs for all its descendants.
	// the folder store and returns the UIDs for all its direct descendants.

	for _, f := range folders {
	result = append(result, f.UID)
	logger.Info("deleting subfolder", "org_id", f.OrgID, "uid", f.UID)
	subfolders, err := s.nestedFolderDelete(ctx, &folder.DeleteFolderCommand{UID: f.UID, OrgID: f.OrgID, ForceDeleteRules: cmd.ForceDeleteRules, SignedInUser: cmd.SignedInUser})
	if err != nil {
	logger.Error("failed deleting subfolder", "org_id", f.OrgID, "uid", f.UID, "error", err)
	return result, err
	}
	result = append(result, subfolders...)

NestedFolders: Fix nested folder deletion #63572

NestedFolders: Fix nested folder deletion #63572

Conversation

ying-jeanne commented Feb 22, 2023 • edited by suntala

papagian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suntala commented Mar 7, 2023 • edited

papagian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suntala commented Mar 8, 2023

amy-super commented Mar 9, 2023

suntala commented Mar 14, 2023 • edited

papagian commented Mar 14, 2023

ying-jeanne commented Feb 22, 2023 •

edited by suntala

suntala commented Mar 7, 2023 •

edited

suntala commented Mar 14, 2023 •

edited