From d883404f505b5823e56e0c16184cc6e195b0cc96 Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Wed, 10 May 2023 16:20:16 +0300 Subject: [PATCH] Nested folders: Fix missing URL from folder responses (#68082) * Nested folders: Set URL in folder responses always * Apply suggestions from code review Co-authored-by: Arati R. <33031346+suntala@users.noreply.github.com> --- pkg/services/folder/folderimpl/sqlstore.go | 27 ++++++++++++++----- .../folder/folderimpl/sqlstore_test.go | 19 +++++++++++++ pkg/services/folder/model.go | 13 +++++++++ 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/pkg/services/folder/folderimpl/sqlstore.go b/pkg/services/folder/folderimpl/sqlstore.go index badf2fc272a..c1364681865 100644 --- a/pkg/services/folder/folderimpl/sqlstore.go +++ b/pkg/services/folder/folderimpl/sqlstore.go @@ -5,10 +5,9 @@ import ( "strings" "time" + "github.com/grafana/dskit/concurrency" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/infra/slugify" - "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/setting" @@ -74,7 +73,7 @@ func (ss *sqlStore) Create(ctx context.Context, cmd folder.CreateFolderCommand) } return nil }) - return foldr, err + return foldr.WithURL(), err } func (ss *sqlStore) Delete(ctx context.Context, uid string, orgID int64) error { @@ -159,7 +158,7 @@ func (ss *sqlStore) Update(ctx context.Context, cmd folder.UpdateFolderCommand) return nil }) - return foldr, err + return foldr.WithURL(), err } func (ss *sqlStore) Get(ctx context.Context, q folder.GetFolderQuery) (*folder.Folder, error) { @@ -185,8 +184,7 @@ func (ss *sqlStore) Get(ctx context.Context, q folder.GetFolderQuery) (*folder.F } return nil }) - foldr.URL = dashboards.GetFolderURL(foldr.UID, slugify.Slugify(foldr.Title)) - return foldr, err + return foldr.WithURL(), err } func (ss *sqlStore) GetParents(ctx context.Context, q folder.GetParentsQuery) ([]*folder.Folder, error) { @@ -218,6 +216,13 @@ func (ss *sqlStore) GetParents(ctx context.Context, q folder.GetParentsQuery) ([ }); err != nil { return nil, err } + + if err := concurrency.ForEachJob(ctx, len(folders), len(folders), func(ctx context.Context, idx int) error { + folders[idx].WithURL() + return nil + }); err != nil { + ss.log.Debug("failed to set URL to folders", "err", err) + } default: ss.log.Debug("recursive CTE subquery is not supported; it fallbacks to the iterative implementation") return ss.getParentsMySQL(ctx, q) @@ -257,6 +262,13 @@ func (ss *sqlStore) GetChildren(ctx context.Context, q folder.GetChildrenQuery) if err != nil { return folder.ErrDatabaseError.Errorf("failed to get folder children: %w", err) } + + if err := concurrency.ForEachJob(ctx, len(folders), len(folders), func(ctx context.Context, idx int) error { + folders[idx].WithURL() + return nil + }); err != nil { + ss.log.Debug("failed to set URL to folders", "err", err) + } return nil }) return folders, err @@ -281,7 +293,8 @@ func (ss *sqlStore) getParentsMySQL(ctx context.Context, cmd folder.GetParentsQu if !ok { break } - folders = append(folders, f) + + folders = append(folders, f.WithURL()) uid = f.ParentUID if len(folders) > folder.MaxNestedFolderDepth { return folder.ErrMaximumDepthReached.Errorf("failed to get parent folders iteratively") diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index b8f45eb85f1..0236ea92ad9 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/pkg/services/folder/folderimpl/sqlstore_test.go @@ -72,6 +72,7 @@ func TestIntegrationCreate(t *testing.T) { assert.NotEmpty(t, f.ID) assert.Equal(t, uid, f.UID) assert.Empty(t, f.ParentUID) + assert.NotEmpty(t, f.URL) ff, err := folderStore.Get(context.Background(), folder.GetFolderQuery{ UID: &f.UID, @@ -81,6 +82,7 @@ func TestIntegrationCreate(t *testing.T) { assert.Equal(t, folderTitle, ff.Title) assert.Equal(t, folderDsc, ff.Description) assert.Empty(t, ff.ParentUID) + assert.NotEmpty(t, ff.URL) assertAncestorUIDs(t, folderStore, f, []string{folder.GeneralFolderUID}) }) @@ -96,6 +98,7 @@ func TestIntegrationCreate(t *testing.T) { require.Equal(t, "parent", parent.Title) require.NotEmpty(t, parent.ID) assert.Equal(t, parentUID, parent.UID) + assert.NotEmpty(t, parent.URL) t.Cleanup(func() { err := folderStore.Delete(context.Background(), parent.UID, orgID) @@ -122,6 +125,7 @@ func TestIntegrationCreate(t *testing.T) { assert.NotEmpty(t, f.ID) assert.Equal(t, uid, f.UID) assert.Equal(t, parentUID, f.ParentUID) + assert.NotEmpty(t, f.URL) assertAncestorUIDs(t, folderStore, f, []string{folder.GeneralFolderUID, parent.UID}) assertChildrenUIDs(t, folderStore, parent, []string{f.UID}) @@ -134,6 +138,7 @@ func TestIntegrationCreate(t *testing.T) { assert.Equal(t, folderTitle, ff.Title) assert.Equal(t, folderDsc, ff.Description) assert.Equal(t, parentUID, ff.ParentUID) + assert.NotEmpty(t, ff.URL) }) } @@ -264,6 +269,7 @@ func TestIntegrationUpdate(t *testing.T) { assert.Equal(t, newTitle, updated.Title) assert.Equal(t, newDesc, updated.Description) assert.Equal(t, parent.UID, updated.ParentUID) + assert.NotEmpty(t, updated.URL) // assert.GreaterOrEqual(t, updated.Updated.UnixNano(), existingUpdated.UnixNano()) updated, err = folderStore.Get(context.Background(), folder.GetFolderQuery{ @@ -275,6 +281,7 @@ func TestIntegrationUpdate(t *testing.T) { assert.Equal(t, newDesc, updated.Description) // parent should not change assert.Equal(t, parent.UID, updated.ParentUID) + assert.NotEmpty(t, updated.URL) f = updated }) @@ -300,6 +307,7 @@ func TestIntegrationUpdate(t *testing.T) { assert.Equal(t, newUID, updated.UID) assert.Equal(t, existingTitle, updated.Title) assert.Equal(t, existingDesc, updated.Description) + assert.NotEmpty(t, updated.URL) }) t.Run("updating folder parent UID", func(t *testing.T) { @@ -374,6 +382,7 @@ func TestIntegrationUpdate(t *testing.T) { assert.Equal(t, existingTitle, updated.Title) assert.Equal(t, existingDesc, updated.Description) assert.Equal(t, existingUID, updated.UID) + assert.NotEmpty(t, updated.URL) }) } }) @@ -423,6 +432,7 @@ func TestIntegrationGet(t *testing.T) { //assert.Equal(t, folder.GeneralFolderUID, ff.ParentUID) assert.NotEmpty(t, ff.Created) assert.NotEmpty(t, ff.Updated) + assert.NotEmpty(t, ff.URL) }) t.Run("get folder by title should succeed", func(t *testing.T) { @@ -439,6 +449,7 @@ func TestIntegrationGet(t *testing.T) { //assert.Equal(t, folder.GeneralFolderUID, ff.ParentUID) assert.NotEmpty(t, ff.Created) assert.NotEmpty(t, ff.Updated) + assert.NotEmpty(t, ff.URL) }) t.Run("get folder by title should succeed", func(t *testing.T) { @@ -454,6 +465,7 @@ func TestIntegrationGet(t *testing.T) { //assert.Equal(t, folder.GeneralFolderUID, ff.ParentUID) assert.NotEmpty(t, ff.Created) assert.NotEmpty(t, ff.Updated) + assert.NotEmpty(t, ff.URL) }) } @@ -518,6 +530,7 @@ func TestIntegrationGetParents(t *testing.T) { require.NoError(t, err) parentUIDs := make([]string, 0) for _, p := range parents { + assert.NotEmpty(t, p.URL) parentUIDs = append(parentUIDs, p.UID) } require.Equal(t, []string{uid1}, parentUIDs) @@ -570,6 +583,7 @@ func TestIntegrationGetChildren(t *testing.T) { childrenUIDs := make([]string, 0, len(children)) for _, c := range children { + assert.NotEmpty(t, c.URL) childrenUIDs = append(childrenUIDs, c.UID) } @@ -586,6 +600,7 @@ func TestIntegrationGetChildren(t *testing.T) { childrenUIDs := make([]string, 0, len(children)) for _, c := range children { + assert.NotEmpty(t, c.URL) childrenUIDs = append(childrenUIDs, c.UID) } assert.Equal(t, []string{parent.UID}, childrenUIDs) @@ -618,6 +633,7 @@ func TestIntegrationGetChildren(t *testing.T) { childrenUIDs = make([]string, 0, len(children)) for _, c := range children { + assert.NotEmpty(t, c.URL) childrenUIDs = append(childrenUIDs, c.UID) } @@ -635,6 +651,7 @@ func TestIntegrationGetChildren(t *testing.T) { childrenUIDs = make([]string, 0, len(children)) for _, c := range children { + assert.NotEmpty(t, c.URL) childrenUIDs = append(childrenUIDs, c.UID) } @@ -652,6 +669,7 @@ func TestIntegrationGetChildren(t *testing.T) { childrenUIDs = make([]string, 0, len(children)) for _, c := range children { + assert.NotEmpty(t, c.URL) childrenUIDs = append(childrenUIDs, c.UID) } @@ -669,6 +687,7 @@ func TestIntegrationGetChildren(t *testing.T) { childrenUIDs = make([]string, 0, len(children)) for _, c := range children { + assert.NotEmpty(t, c.URL) childrenUIDs = append(childrenUIDs, c.UID) } diff --git a/pkg/services/folder/model.go b/pkg/services/folder/model.go index 91cda1d5533..21e021cd39d 100644 --- a/pkg/services/folder/model.go +++ b/pkg/services/folder/model.go @@ -1,9 +1,12 @@ package folder import ( + "fmt" "time" + "github.com/grafana/grafana/pkg/infra/slugify" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util/errutil" ) @@ -48,6 +51,16 @@ func (f *Folder) IsGeneral() bool { return f.ID == GeneralFolder.ID && f.Title == GeneralFolder.Title } +func (f *Folder) WithURL() *Folder { + if f == nil || f.URL != "" { + return f + } + + // copy of dashboards.GetFolderURL() + f.URL = fmt.Sprintf("%s/dashboards/f/%s/%s", setting.AppSubUrl, f.UID, slugify.Slugify(f.Title)) + return f +} + // NewFolder tales a title and returns a Folder with the Created and Updated // fields set to the current time. func NewFolder(title string, description string) *Folder {