From 61c5b4a25e943b4ebef41532b8e5b24d205770fc Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Mon, 27 Jan 2025 16:39:18 -0700 Subject: [PATCH] Library elements: remove usage of dashboard table on get (#99619) --- pkg/services/libraryelements/database.go | 32 +++++++++----- .../libraryelements_get_test.go | 34 ++++++++++++++ .../libraryelements/libraryelements_test.go | 44 ++++++++++++++++++- 3 files changed, 98 insertions(+), 12 deletions(-) diff --git a/pkg/services/libraryelements/database.go b/pkg/services/libraryelements/database.go index 057a7e3783c..8b23f58cf5e 100644 --- a/pkg/services/libraryelements/database.go +++ b/pkg/services/libraryelements/database.go @@ -79,15 +79,12 @@ func syncFieldsWithModel(libraryElement *model.LibraryElement) error { return nil } -func GetLibraryElement(dialect migrator.Dialect, session *db.Session, uid string, orgID int64) (model.LibraryElementWithMeta, error) { +func (l *LibraryElementService) GetLibraryElement(c context.Context, signedInUser identity.Requester, session *db.Session, uid string) (model.LibraryElementWithMeta, error) { elements := make([]model.LibraryElementWithMeta, 0) sql := selectLibraryElementDTOWithMeta + - ", coalesce(dashboard.title, 'General') AS folder_name" + - ", coalesce(dashboard.uid, '') AS folder_uid" + - getFromLibraryElementDTOWithMeta(dialect) + - " LEFT JOIN dashboard AS dashboard ON dashboard.id = le.folder_id" + + getFromLibraryElementDTOWithMeta(l.SQLStore.GetDialect()) + " WHERE le.uid=? AND le.org_id=?" - sess := session.SQL(sql, uid, orgID) + sess := session.SQL(sql, uid, signedInUser.GetOrgID()) err := sess.Find(&elements) if err != nil { return model.LibraryElementWithMeta{}, err @@ -99,6 +96,19 @@ func GetLibraryElement(dialect migrator.Dialect, session *db.Session, uid string return model.LibraryElementWithMeta{}, fmt.Errorf("found %d elements, while expecting at most one", len(elements)) } + // get the folder title + f, err := l.folderService.Get(c, &folder.GetFolderQuery{ + OrgID: elements[0].OrgID, + UID: &elements[0].FolderUID, + SignedInUser: signedInUser, + }) + if err == nil { + elements[0].FolderName = f.Title + } else { + // default to General if we cannot find the folder + elements[0].FolderName = "General" + } + return elements[0], nil } @@ -220,7 +230,7 @@ func (l *LibraryElementService) createLibraryElement(c context.Context, signedIn func (l *LibraryElementService) deleteLibraryElement(c context.Context, signedInUser identity.Requester, uid string) (int64, error) { var elementID int64 err := l.SQLStore.WithTransactionalDbSession(c, func(session *db.Session) error { - element, err := GetLibraryElement(l.SQLStore.GetDialect(), session, uid, signedInUser.GetOrgID()) + element, err := l.GetLibraryElement(c, signedInUser, session, uid) if err != nil { return err } @@ -593,7 +603,7 @@ func (l *LibraryElementService) patchLibraryElement(c context.Context, signedInU return model.LibraryElementDTO{}, err } err := l.SQLStore.WithTransactionalDbSession(c, func(session *db.Session) error { - elementInDB, err := GetLibraryElement(l.SQLStore.GetDialect(), session, uid, signedInUser.GetOrgID()) + elementInDB, err := l.GetLibraryElement(c, signedInUser, session, uid) if err != nil { return err } @@ -610,7 +620,7 @@ func (l *LibraryElementService) patchLibraryElement(c context.Context, signedInU return model.ErrLibraryElementUIDTooLong } - _, err := GetLibraryElement(l.SQLStore.GetDialect(), session, updateUID, signedInUser.GetOrgID()) + _, err := l.GetLibraryElement(c, signedInUser, session, updateUID) if !errors.Is(err, model.ErrLibraryElementNotFound) { return model.ErrLibraryElementAlreadyExists } @@ -705,7 +715,7 @@ func (l *LibraryElementService) getConnections(c context.Context, signedInUser i } err = l.SQLStore.WithDbSession(c, func(session *db.Session) error { - element, err := GetLibraryElement(l.SQLStore.GetDialect(), session, uid, signedInUser.GetOrgID()) + element, err := l.GetLibraryElement(c, signedInUser, session, uid) if err != nil { return err } @@ -832,7 +842,7 @@ func (l *LibraryElementService) connectElementsToDashboardID(c context.Context, return err } for _, elementUID := range elementUIDs { - element, err := GetLibraryElement(l.SQLStore.GetDialect(), session, elementUID, signedInUser.GetOrgID()) + element, err := l.GetLibraryElement(c, signedInUser, session, elementUID) if err != nil { return err } diff --git a/pkg/services/libraryelements/libraryelements_get_test.go b/pkg/services/libraryelements/libraryelements_get_test.go index b441505639b..96f64f4c35f 100644 --- a/pkg/services/libraryelements/libraryelements_get_test.go +++ b/pkg/services/libraryelements/libraryelements_get_test.go @@ -1,12 +1,15 @@ package libraryelements import ( + "encoding/json" "testing" "github.com/google/go-cmp/cmp" "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/kinds/librarypanel" "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/libraryelements/model" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/web" @@ -91,6 +94,37 @@ func TestGetLibraryElement(t *testing.T) { } }) + scenarioWithPanel(t, "When an admin tries to get a library panel that exists, but the original folder does not, it should succeed and return correct result", + func(t *testing.T, sc scenarioContext) { + b, err := json.Marshal(map[string]string{"test": "test"}) + require.NoError(t, err) + newFolder := createFolder(t, sc, "NewFolder", nil) + sc.reqContext.SignedInUser.Permissions[sc.reqContext.OrgID][dashboards.ActionFoldersRead] = []string{dashboards.ScopeFoldersAll} + sc.reqContext.SignedInUser.Permissions[sc.reqContext.OrgID][dashboards.ActionFoldersDelete] = []string{dashboards.ScopeFoldersAll} + result, err := sc.service.createLibraryElement(sc.reqContext.Req.Context(), sc.reqContext.SignedInUser, model.CreateLibraryElementCommand{ + FolderID: newFolder.ID, // nolint:staticcheck + FolderUID: &newFolder.UID, + Name: "Testing Library Panel With Deleted Folder", + Kind: 1, + Model: b, + UID: "panel-with-deleted-folder", + }) + require.NoError(t, err) + err = sc.service.folderService.Delete(sc.reqContext.Req.Context(), &folder.DeleteFolderCommand{ + UID: newFolder.UID, + OrgID: sc.reqContext.OrgID, + SignedInUser: sc.reqContext.SignedInUser, + }) + require.NoError(t, err) + err = sc.sqlStore.WithDbSession(sc.reqContext.Req.Context(), func(session *db.Session) error { + elem, err := sc.service.GetLibraryElement(sc.reqContext.Req.Context(), sc.reqContext.SignedInUser, session, result.UID) + require.NoError(t, err) + require.Equal(t, elem.FolderName, "General") + return nil + }) + require.NoError(t, err) + }) + scenarioWithPanel(t, "When an admin tries to get a connected library panel, it should succeed and return correct connected dashboards", func(t *testing.T, sc scenarioContext) { dashJSON := map[string]any{ diff --git a/pkg/services/libraryelements/libraryelements_test.go b/pkg/services/libraryelements/libraryelements_test.go index fd7bb559a12..7ea77dd22ff 100644 --- a/pkg/services/libraryelements/libraryelements_test.go +++ b/pkg/services/libraryelements/libraryelements_test.go @@ -32,10 +32,13 @@ import ( "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/folder/folderimpl" + "github.com/grafana/grafana/pkg/services/folder/foldertest" "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/libraryelements/model" + ngstore "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/org/orgimpl" + "github.com/grafana/grafana/pkg/services/publicdashboards" "github.com/grafana/grafana/pkg/services/quota/quotatest" "github.com/grafana/grafana/pkg/services/supportbundles/supportbundlestest" "github.com/grafana/grafana/pkg/services/tag/tagimpl" @@ -198,6 +201,39 @@ func TestGetLibraryPanelConnections(t *testing.T) { t.Fatalf("Result mismatch (-want +got):\n%s", diff) } }) + + scenarioWithPanel(t, "When an admin tries to create a connection with an element that exists, but the original folder does not, it should still succeed", + func(t *testing.T, sc scenarioContext) { + b, err := json.Marshal(map[string]string{"test": "test"}) + require.NoError(t, err) + newFolder := createFolder(t, sc, "NewFolder", nil) + sc.reqContext.SignedInUser.Permissions[sc.reqContext.OrgID][dashboards.ActionFoldersRead] = []string{dashboards.ScopeFoldersAll} + sc.reqContext.SignedInUser.Permissions[sc.reqContext.OrgID][dashboards.ActionFoldersDelete] = []string{dashboards.ScopeFoldersAll} + _, err = sc.service.createLibraryElement(sc.reqContext.Req.Context(), sc.reqContext.SignedInUser, model.CreateLibraryElementCommand{ + FolderID: newFolder.ID, // nolint:staticcheck + FolderUID: &newFolder.UID, + Name: "Testing Library Panel With Deleted Folder", + Kind: 1, + Model: b, + UID: "panel-with-deleted-folder", + }) + require.NoError(t, err) + err = sc.service.folderService.Delete(sc.reqContext.Req.Context(), &folder.DeleteFolderCommand{ + UID: newFolder.UID, + OrgID: sc.reqContext.OrgID, + SignedInUser: sc.reqContext.SignedInUser, + }) + require.NoError(t, err) + + dash := dashboards.Dashboard{ + Title: "Testing create element", + Data: simplejson.NewFromAny(map[string]any{}), + } + // nolint:staticcheck + dashInDB := createDashboard(t, sc.sqlStore, sc.user, &dash, sc.folder.ID, sc.folder.UID) + err = sc.service.ConnectElementsToDashboard(sc.reqContext.Req.Context(), sc.reqContext.SignedInUser, []string{sc.initialResult.Result.UID}, dashInDB.ID) + require.NoError(t, err) + }) } type libraryElement struct { @@ -477,9 +513,15 @@ func testScenario(t *testing.T, desc string, fn func(t *testing.T, sc scenarioCo dashboardPermissions := acmock.NewMockedPermissionsService() folderStore := folderimpl.ProvideDashboardFolderStore(sqlStore) fStore := folderimpl.ProvideStore(sqlStore) + publicDash := &publicdashboards.FakePublicDashboardServiceWrapper{} + publicDash.On("DeleteByDashboardUIDs", mock.Anything, mock.Anything, mock.Anything).Return(nil) folderSvc := folderimpl.ProvideService( fStore, ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, - nil, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil, cfg, nil, tracing.InitializeTracerForTest()) + nil, sqlStore, features, supportbundlestest.NewFakeBundleService(), publicDash, cfg, nil, tracing.InitializeTracerForTest()) + alertStore, err := ngstore.ProvideDBStore(cfg, features, sqlStore, &foldertest.FakeService{}, &dashboards.FakeDashboardService{}, ac, bus.ProvideBus(tracing.InitializeTracerForTest())) + require.NoError(t, err) + err = folderSvc.RegisterService(alertStore) + require.NoError(t, err) dashService, dashSvcErr := dashboardservice.ProvideDashboardServiceImpl( cfg, dashboardStore, folderStore, features, folderPermissions, ac,