From 6d2a555866b354d66ca270f6bd6ee2b7e0d7673d Mon Sep 17 00:00:00 2001 From: bergquist Date: Tue, 30 Jan 2018 17:06:23 +0100 Subject: [PATCH 1/6] Drops unique index orgid_slug from dashboards. --- pkg/services/sqlstore/migrations/dashboard_mig.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/services/sqlstore/migrations/dashboard_mig.go b/pkg/services/sqlstore/migrations/dashboard_mig.go index 7da7c5a974f..edca733e174 100644 --- a/pkg/services/sqlstore/migrations/dashboard_mig.go +++ b/pkg/services/sqlstore/migrations/dashboard_mig.go @@ -163,4 +163,8 @@ func addDashboardMigration(mg *Migrator) { mg.AddMigration("Add unique index dashboard_org_id_uid", NewAddIndexMigration(dashboardV2, &Index{ Cols: []string{"org_id", "uid"}, Type: UniqueIndex, })) + + mg.AddMigration("Remove unique index org_id_slug", NewDropIndexMigration(dashboardV2, &Index{ + Cols: []string{"org_id", "slug"}, Type: UniqueIndex, + })) } From bb3183f6cdc8e92c98d02a079cb1a8f1af776e75 Mon Sep 17 00:00:00 2001 From: bergquist Date: Tue, 30 Jan 2018 18:57:33 +0100 Subject: [PATCH 2/6] removes uniqnes check on slug when saving dashboards --- pkg/api/dashboard.go | 2 +- pkg/models/dashboards.go | 3 ++- pkg/services/sqlstore/dashboard.go | 19 ++++++++++--------- pkg/services/sqlstore/dashboard_test.go | 16 ++++++++++++++++ .../sqlstore/dashboard_version_test.go | 5 ++--- tests/test-app/dashboards/connections.json | 1 + .../dashboards/connections_result.json | 1 + 7 files changed, 33 insertions(+), 14 deletions(-) diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index a1c9950c457..77d3d8882f5 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -214,7 +214,7 @@ func PostDashboard(c *middleware.Context, cmd m.SaveDashboardCommand) Response { } if err != nil { - if err == m.ErrDashboardWithSameNameExists { + if err == m.ErrDashboardWithSameUIDExists { return Json(412, util.DynMap{"status": "name-exists", "message": err.Error()}) } if err == m.ErrDashboardVersionMismatch { diff --git a/pkg/models/dashboards.go b/pkg/models/dashboards.go index 84ede04f226..cf7f6355537 100644 --- a/pkg/models/dashboards.go +++ b/pkg/models/dashboards.go @@ -16,7 +16,7 @@ import ( var ( ErrDashboardNotFound = errors.New("Dashboard not found") ErrDashboardSnapshotNotFound = errors.New("Dashboard snapshot not found") - ErrDashboardWithSameNameExists = errors.New("A dashboard with the same name already exists") + ErrDashboardWithSameUIDExists = errors.New("A dashboard with the same uid already exists") ErrDashboardVersionMismatch = errors.New("The dashboard has been changed by someone else") ErrDashboardTitleEmpty = errors.New("Dashboard title cannot be empty") ErrDashboardFolderCannotHaveParent = errors.New("A Dashboard Folder cannot be added to another folder") @@ -116,6 +116,7 @@ func NewDashboardFromJson(data *simplejson.Json) *Dashboard { dash.Uid = uid } else { dash.Uid = util.GenerateShortUid() + dash.Data.Set("uid", dash.Uid) } return dash diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index 84cfd1999c7..376e4e3c382 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -25,7 +25,7 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error { dash := cmd.GetDashboardModel() // try get existing dashboard - var existing, sameTitle m.Dashboard + var existing m.Dashboard if dash.Id > 0 { dashWithIdExists, err := sess.Where("id=? AND org_id=?", dash.Id, dash.OrgId).Get(&existing) @@ -51,19 +51,20 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error { } } - sameTitleExists, err := sess.Where("org_id=? AND slug=?", dash.OrgId, dash.Slug).Get(&sameTitle) + var sameUid m.Dashboard + sameUidExists, err := sess.Where("org_id=? AND uid=?", dash.OrgId, dash.Uid).Get(&sameUid) if err != nil { return err } - if sameTitleExists { - // another dashboard with same name - if dash.Id != sameTitle.Id { + if sameUidExists { + // another dashboard with same uid + if dash.Id != sameUid.Id { if cmd.Overwrite { - dash.Id = sameTitle.Id - dash.Version = sameTitle.Version + dash.Id = sameUid.Id + dash.Version = sameUid.Version } else { - return m.ErrDashboardWithSameNameExists + return m.ErrDashboardWithSameUIDExists } } } @@ -89,7 +90,7 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error { dash.Updated = cmd.UpdatedAt } - affectedRows, err = sess.MustCols("folder_id", "has_acl").Id(dash.Id).Update(dash) + affectedRows, err = sess.MustCols("folder_id", "has_acl").ID(dash.Id).Update(dash) } if err != nil { diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index 19e82614718..e8fb5332d32 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -207,6 +207,22 @@ func TestDashboardDataAccess(t *testing.T) { } err := SaveDashboard(&cmd) + So(err, ShouldBeNil) + }) + + Convey("Should not be able to save dashboard with same uid", func() { + cmd := m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "id": nil, + "title": "test dash 23", + "uid": "dsfalkjngailuedt", + }), + } + + err := SaveDashboard(&cmd) + So(err, ShouldBeNil) + err = SaveDashboard(&cmd) So(err, ShouldNotBeNil) }) diff --git a/pkg/services/sqlstore/dashboard_version_test.go b/pkg/services/sqlstore/dashboard_version_test.go index cbc33c8ad84..e20ac897b3d 100644 --- a/pkg/services/sqlstore/dashboard_version_test.go +++ b/pkg/services/sqlstore/dashboard_version_test.go @@ -12,7 +12,7 @@ import ( ) func updateTestDashboard(dashboard *m.Dashboard, data map[string]interface{}) { - data["title"] = dashboard.Title + data["uid"] = dashboard.Uid saveCmd := m.SaveDashboardCommand{ OrgId: dashboard.OrgId, @@ -44,12 +44,11 @@ func TestGetDashboardVersion(t *testing.T) { dashCmd := m.GetDashboardQuery{ OrgId: savedDash.OrgId, - Slug: savedDash.Slug, + Uid: savedDash.Uid, } err = GetDashboard(&dashCmd) So(err, ShouldBeNil) - dashCmd.Result.Data.Del("uid") eq := reflect.DeepEqual(dashCmd.Result.Data, query.Result.Data) So(eq, ShouldEqual, true) }) diff --git a/tests/test-app/dashboards/connections.json b/tests/test-app/dashboards/connections.json index cc189d2d113..042dc4baec5 100644 --- a/tests/test-app/dashboards/connections.json +++ b/tests/test-app/dashboards/connections.json @@ -7,6 +7,7 @@ } ], + "uid": "1MHHlVjzz", "title": "Nginx Connections", "revision": 25, "schemaVersion": 11, diff --git a/tests/test-app/dashboards/connections_result.json b/tests/test-app/dashboards/connections_result.json index 4bf0570ac1e..2d2bc8de238 100644 --- a/tests/test-app/dashboards/connections_result.json +++ b/tests/test-app/dashboards/connections_result.json @@ -16,5 +16,6 @@ ], "schemaVersion": 11, "title": "Nginx Connections", + "uid": "1MHHlVjzz", "version": 0 } From 7e9605259407d8db483c5482c96c3d65b07e8e38 Mon Sep 17 00:00:00 2001 From: bergquist Date: Wed, 31 Jan 2018 13:47:28 +0100 Subject: [PATCH 3/6] ensure dashboard title is unique in folder --- pkg/models/dashboards.go | 17 +- pkg/services/sqlstore/dashboard.go | 24 ++ .../sqlstore/dashboard_folder_test.go | 219 +++++++++++++ pkg/services/sqlstore/dashboard_test.go | 287 +++++------------- 4 files changed, 336 insertions(+), 211 deletions(-) create mode 100644 pkg/services/sqlstore/dashboard_folder_test.go diff --git a/pkg/models/dashboards.go b/pkg/models/dashboards.go index cf7f6355537..c4361eefd7d 100644 --- a/pkg/models/dashboards.go +++ b/pkg/models/dashboards.go @@ -14,14 +14,15 @@ import ( // Typed errors var ( - ErrDashboardNotFound = errors.New("Dashboard not found") - ErrDashboardSnapshotNotFound = errors.New("Dashboard snapshot not found") - ErrDashboardWithSameUIDExists = errors.New("A dashboard with the same uid already exists") - ErrDashboardVersionMismatch = errors.New("The dashboard has been changed by someone else") - ErrDashboardTitleEmpty = errors.New("Dashboard title cannot be empty") - ErrDashboardFolderCannotHaveParent = errors.New("A Dashboard Folder cannot be added to another folder") - ErrDashboardContainsInvalidAlertData = errors.New("Invalid alert data. Cannot save dashboard") - ErrDashboardFailedToUpdateAlertData = errors.New("Failed to save alert data") + ErrDashboardNotFound = errors.New("Dashboard not found") + ErrDashboardSnapshotNotFound = errors.New("Dashboard snapshot not found") + ErrDashboardWithSameUIDExists = errors.New("A dashboard with the same uid already exists") + ErrDashboardWithSameNameInFolderExists = errors.New("A dashboard with the same name in the folder already exists") + ErrDashboardVersionMismatch = errors.New("The dashboard has been changed by someone else") + ErrDashboardTitleEmpty = errors.New("Dashboard title cannot be empty") + ErrDashboardFolderCannotHaveParent = errors.New("A Dashboard Folder cannot be added to another folder") + ErrDashboardContainsInvalidAlertData = errors.New("Invalid alert data. Cannot save dashboard") + ErrDashboardFailedToUpdateAlertData = errors.New("Failed to save alert data") ) type UpdatePluginDashboardError struct { diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index 376e4e3c382..c378d70af10 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -20,6 +20,8 @@ func init() { bus.AddHandler("sql", GetDashboardsByPluginId) } + + func SaveDashboard(cmd *m.SaveDashboardCommand) error { return inTransaction(func(sess *DBSession) error { dash := cmd.GetDashboardModel() @@ -69,6 +71,11 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error { } } + err = guaranteeDashboardNameIsUniqueInFolder(sess, dash) + if err != nil { + return err + } + err = setHasAcl(sess, dash) if err != nil { return err @@ -140,6 +147,23 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error { }) } +func guaranteeDashboardNameIsUniqueInFolder(sess *DBSession, dash *m.Dashboard) error { + var sameNameInFolder m.Dashboard + sameNameInFolderExist, err := sess.Where("org_id=? AND title=? AND folder_id = ? AND uid <> ?", + dash.OrgId, dash.Title, dash.FolderId, dash.Uid). + Get(&sameNameInFolder) + + if err != nil { + return err + } + + if sameNameInFolderExist { + return m.ErrDashboardWithSameNameInFolderExists + } + + return nil +} + func setHasAcl(sess *DBSession, dash *m.Dashboard) error { // check if parent has acl if dash.FolderId > 0 { diff --git a/pkg/services/sqlstore/dashboard_folder_test.go b/pkg/services/sqlstore/dashboard_folder_test.go new file mode 100644 index 00000000000..6f880e7bb4c --- /dev/null +++ b/pkg/services/sqlstore/dashboard_folder_test.go @@ -0,0 +1,219 @@ +package sqlstore + +import ( + "testing" + + "github.com/go-xorm/xorm" + . "github.com/smartystreets/goconvey/convey" + + m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/search" +) + +func TestDashboardFolderDataAccess(t *testing.T) { + var x *xorm.Engine + + Convey("Testing DB", t, func() { + x = InitTestDB(t) + + Convey("Given one dashboard folder with two dashboards and one dashboard in the root folder", func() { + folder := insertTestDashboard("1 test dash folder", 1, 0, true, "prod", "webapp") + dashInRoot := insertTestDashboard("test dash 67", 1, 0, false, "prod", "webapp") + childDash := insertTestDashboard("test dash 23", 1, folder.Id, false, "prod", "webapp") + insertTestDashboard("test dash 45", 1, folder.Id, false, "prod") + + currentUser := createUser("viewer", "Viewer", false) + + Convey("and no acls are set", func() { + Convey("should return all dashboards", func() { + query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 2) + So(query.Result[0].Id, ShouldEqual, folder.Id) + So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) + }) + }) + + Convey("and acl is set for dashboard folder", func() { + var otherUser int64 = 999 + updateTestDashboardWithAcl(folder.Id, otherUser, m.PERMISSION_EDIT) + + Convey("should not return folder", func() { + query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 1) + So(query.Result[0].Id, ShouldEqual, dashInRoot.Id) + }) + + Convey("when the user is given permission", func() { + updateTestDashboardWithAcl(folder.Id, currentUser.Id, m.PERMISSION_EDIT) + + Convey("should be able to access folder", func() { + query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 2) + So(query.Result[0].Id, ShouldEqual, folder.Id) + So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) + }) + }) + + Convey("when the user is an admin", func() { + Convey("should be able to access folder", func() { + query := &search.FindPersistedDashboardsQuery{ + SignedInUser: &m.SignedInUser{ + UserId: currentUser.Id, + OrgId: 1, + OrgRole: m.ROLE_ADMIN, + }, + OrgId: 1, + DashboardIds: []int64{folder.Id, dashInRoot.Id}, + } + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 2) + So(query.Result[0].Id, ShouldEqual, folder.Id) + So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) + }) + }) + }) + + Convey("and acl is set for dashboard child and folder has all permissions removed", func() { + var otherUser int64 = 999 + aclId := updateTestDashboardWithAcl(folder.Id, otherUser, m.PERMISSION_EDIT) + removeAcl(aclId) + updateTestDashboardWithAcl(childDash.Id, otherUser, m.PERMISSION_EDIT) + + Convey("should not return folder or child", func() { + query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 1) + So(query.Result[0].Id, ShouldEqual, dashInRoot.Id) + }) + + Convey("when the user is given permission to child", func() { + updateTestDashboardWithAcl(childDash.Id, currentUser.Id, m.PERMISSION_EDIT) + + Convey("should be able to search for child dashboard but not folder", func() { + query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 2) + So(query.Result[0].Id, ShouldEqual, childDash.Id) + So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) + }) + }) + + Convey("when the user is an admin", func() { + Convey("should be able to search for child dash and folder", func() { + query := &search.FindPersistedDashboardsQuery{ + SignedInUser: &m.SignedInUser{ + UserId: currentUser.Id, + OrgId: 1, + OrgRole: m.ROLE_ADMIN, + }, + OrgId: 1, + DashboardIds: []int64{folder.Id, dashInRoot.Id, childDash.Id}, + } + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 3) + So(query.Result[0].Id, ShouldEqual, folder.Id) + So(query.Result[1].Id, ShouldEqual, childDash.Id) + So(query.Result[2].Id, ShouldEqual, dashInRoot.Id) + }) + }) + }) + }) + + Convey("Given two dashboard folders with one dashboard each and one dashboard in the root folder", func() { + folder1 := insertTestDashboard("1 test dash folder", 1, 0, true, "prod") + folder2 := insertTestDashboard("2 test dash folder", 1, 0, true, "prod") + dashInRoot := insertTestDashboard("test dash 67", 1, 0, false, "prod") + childDash1 := insertTestDashboard("child dash 1", 1, folder1.Id, false, "prod") + childDash2 := insertTestDashboard("child dash 2", 1, folder2.Id, false, "prod") + + currentUser := createUser("viewer", "Viewer", false) + var rootFolderId int64 = 0 + + Convey("and one folder is expanded, the other collapsed", func() { + Convey("should return dashboards in root and expanded folder", func() { + query := &search.FindPersistedDashboardsQuery{FolderIds: []int64{rootFolderId, folder1.Id}, SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1} + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 4) + So(query.Result[0].Id, ShouldEqual, folder1.Id) + So(query.Result[1].Id, ShouldEqual, folder2.Id) + So(query.Result[2].Id, ShouldEqual, childDash1.Id) + So(query.Result[3].Id, ShouldEqual, dashInRoot.Id) + }) + }) + + Convey("and acl is set for one dashboard folder", func() { + var otherUser int64 = 999 + updateTestDashboardWithAcl(folder1.Id, otherUser, m.PERMISSION_EDIT) + + Convey("and a dashboard is moved from folder without acl to the folder with an acl", func() { + movedDash := moveDashboard(1, childDash2.Data, folder1.Id) + So(movedDash.HasAcl, ShouldBeTrue) + + Convey("should not return folder with acl or its children", func() { + query := &search.FindPersistedDashboardsQuery{ + SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, + OrgId: 1, + DashboardIds: []int64{folder1.Id, childDash1.Id, childDash2.Id, dashInRoot.Id}, + } + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 1) + So(query.Result[0].Id, ShouldEqual, dashInRoot.Id) + }) + }) + + Convey("and a dashboard is moved from folder with acl to the folder without an acl", func() { + movedDash := moveDashboard(1, childDash1.Data, folder2.Id) + So(movedDash.HasAcl, ShouldBeFalse) + + Convey("should return folder without acl and its children", func() { + query := &search.FindPersistedDashboardsQuery{ + SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, + OrgId: 1, + DashboardIds: []int64{folder2.Id, childDash1.Id, childDash2.Id, dashInRoot.Id}, + } + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 4) + So(query.Result[0].Id, ShouldEqual, folder2.Id) + So(query.Result[1].Id, ShouldEqual, childDash1.Id) + So(query.Result[2].Id, ShouldEqual, childDash2.Id) + So(query.Result[3].Id, ShouldEqual, dashInRoot.Id) + }) + }) + + Convey("and a dashboard with an acl is moved to the folder without an acl", func() { + updateTestDashboardWithAcl(childDash1.Id, otherUser, m.PERMISSION_EDIT) + movedDash := moveDashboard(1, childDash1.Data, folder2.Id) + So(movedDash.HasAcl, ShouldBeTrue) + + Convey("should return folder without acl but not the dashboard with acl", func() { + query := &search.FindPersistedDashboardsQuery{ + SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, + OrgId: 1, + DashboardIds: []int64{folder2.Id, childDash1.Id, childDash2.Id, dashInRoot.Id}, + } + err := SearchDashboards(query) + So(err, ShouldBeNil) + So(len(query.Result), ShouldEqual, 3) + So(query.Result[0].Id, ShouldEqual, folder2.Id) + So(query.Result[1].Id, ShouldEqual, childDash2.Id) + So(query.Result[2].Id, ShouldEqual, dashInRoot.Id) + }) + }) + }) + }) + + }) +} diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index e8fb5332d32..6e77b44f8c1 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -196,18 +196,64 @@ func TestDashboardDataAccess(t *testing.T) { }) }) - Convey("Should not be able to save dashboard with same name", func() { - cmd := m.SaveDashboardCommand{ + Convey("Should be able to save dashboards with same name in different folders", func() { + firstSaveCmd := m.SaveDashboardCommand{ OrgId: 1, Dashboard: simplejson.NewFromAny(map[string]interface{}{ "id": nil, - "title": "test dash 23", + "title": "test dash folder and title", "tags": []interface{}{}, + "uid": "randomHash", }), + FolderId: 3, } - err := SaveDashboard(&cmd) + err := SaveDashboard(&firstSaveCmd) So(err, ShouldBeNil) + + secondSaveCmd := m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "id": nil, + "title": "test dash folder and title", + "tags": []interface{}{}, + "uid": "moreRandomHash", + }), + FolderId: 1, + } + + err = SaveDashboard(&secondSaveCmd) + So(err, ShouldBeNil) + }) + + Convey("Should not be able to save dashboard with same name in the same folder", func() { + firstSaveCmd := m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "id": nil, + "title": "test dash folder and title", + "tags": []interface{}{}, + "uid": "randomHash", + }), + FolderId: 3, + } + + err := SaveDashboard(&firstSaveCmd) + So(err, ShouldBeNil) + + secondSaveCmd := m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "id": nil, + "title": "test dash folder and title", + "tags": []interface{}{}, + "uid": "moreRandomHash", + }), + FolderId: 3, + } + + err = SaveDashboard(&secondSaveCmd) + So(err, ShouldEqual, m.ErrDashboardWithSameNameInFolderExists) }) Convey("Should not be able to save dashboard with same uid", func() { @@ -226,6 +272,40 @@ func TestDashboardDataAccess(t *testing.T) { So(err, ShouldNotBeNil) }) + Convey("Should be able to update dashboard with the same title and folder id", func() { + cmd := m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + //"id": 1, + "uid": "randomHash", + "title": "folderId", + "style": "light", + "tags": []interface{}{}, + }), + FolderId: 2, + } + + err := SaveDashboard(&cmd) + So(err, ShouldBeNil) + So(cmd.Result.FolderId, ShouldEqual, 2) + + cmd = m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "id": cmd.Result.Id, + "uid": "randomHash", + "title": "folderId", + "style": "dark", + "version": cmd.Result.Version, + "tags": []interface{}{}, + }), + FolderId: 2, + } + + err = SaveDashboard(&cmd) + So(err, ShouldBeNil) + }) + Convey("Should be able to update dashboard and remove folderId", func() { cmd := m.SaveDashboardCommand{ OrgId: 1, @@ -315,205 +395,6 @@ func TestDashboardDataAccess(t *testing.T) { }) }) - Convey("Given one dashboard folder with two dashboards and one dashboard in the root folder", func() { - folder := insertTestDashboard("1 test dash folder", 1, 0, true, "prod", "webapp") - dashInRoot := insertTestDashboard("test dash 67", 1, 0, false, "prod", "webapp") - childDash := insertTestDashboard("test dash 23", 1, folder.Id, false, "prod", "webapp") - insertTestDashboard("test dash 45", 1, folder.Id, false, "prod") - - currentUser := createUser("viewer", "Viewer", false) - - Convey("and no acls are set", func() { - Convey("should return all dashboards", func() { - query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 2) - So(query.Result[0].Id, ShouldEqual, folder.Id) - So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) - }) - }) - - Convey("and acl is set for dashboard folder", func() { - var otherUser int64 = 999 - updateTestDashboardWithAcl(folder.Id, otherUser, m.PERMISSION_EDIT) - - Convey("should not return folder", func() { - query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 1) - So(query.Result[0].Id, ShouldEqual, dashInRoot.Id) - }) - - Convey("when the user is given permission", func() { - updateTestDashboardWithAcl(folder.Id, currentUser.Id, m.PERMISSION_EDIT) - - Convey("should be able to access folder", func() { - query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, dashInRoot.Id}} - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 2) - So(query.Result[0].Id, ShouldEqual, folder.Id) - So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) - }) - }) - - Convey("when the user is an admin", func() { - Convey("should be able to access folder", func() { - query := &search.FindPersistedDashboardsQuery{ - SignedInUser: &m.SignedInUser{ - UserId: currentUser.Id, - OrgId: 1, - OrgRole: m.ROLE_ADMIN, - }, - OrgId: 1, - DashboardIds: []int64{folder.Id, dashInRoot.Id}, - } - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 2) - So(query.Result[0].Id, ShouldEqual, folder.Id) - So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) - }) - }) - }) - - Convey("and acl is set for dashboard child and folder has all permissions removed", func() { - var otherUser int64 = 999 - aclId := updateTestDashboardWithAcl(folder.Id, otherUser, m.PERMISSION_EDIT) - removeAcl(aclId) - updateTestDashboardWithAcl(childDash.Id, otherUser, m.PERMISSION_EDIT) - - Convey("should not return folder or child", func() { - query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}} - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 1) - So(query.Result[0].Id, ShouldEqual, dashInRoot.Id) - }) - - Convey("when the user is given permission to child", func() { - updateTestDashboardWithAcl(childDash.Id, currentUser.Id, m.PERMISSION_EDIT) - - Convey("should be able to search for child dashboard but not folder", func() { - query := &search.FindPersistedDashboardsQuery{SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1, DashboardIds: []int64{folder.Id, childDash.Id, dashInRoot.Id}} - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 2) - So(query.Result[0].Id, ShouldEqual, childDash.Id) - So(query.Result[1].Id, ShouldEqual, dashInRoot.Id) - }) - }) - - Convey("when the user is an admin", func() { - Convey("should be able to search for child dash and folder", func() { - query := &search.FindPersistedDashboardsQuery{ - SignedInUser: &m.SignedInUser{ - UserId: currentUser.Id, - OrgId: 1, - OrgRole: m.ROLE_ADMIN, - }, - OrgId: 1, - DashboardIds: []int64{folder.Id, dashInRoot.Id, childDash.Id}, - } - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 3) - So(query.Result[0].Id, ShouldEqual, folder.Id) - So(query.Result[1].Id, ShouldEqual, childDash.Id) - So(query.Result[2].Id, ShouldEqual, dashInRoot.Id) - }) - }) - }) - }) - - Convey("Given two dashboard folders with one dashboard each and one dashboard in the root folder", func() { - folder1 := insertTestDashboard("1 test dash folder", 1, 0, true, "prod") - folder2 := insertTestDashboard("2 test dash folder", 1, 0, true, "prod") - dashInRoot := insertTestDashboard("test dash 67", 1, 0, false, "prod") - childDash1 := insertTestDashboard("child dash 1", 1, folder1.Id, false, "prod") - childDash2 := insertTestDashboard("child dash 2", 1, folder2.Id, false, "prod") - - currentUser := createUser("viewer", "Viewer", false) - var rootFolderId int64 = 0 - - Convey("and one folder is expanded, the other collapsed", func() { - Convey("should return dashboards in root and expanded folder", func() { - query := &search.FindPersistedDashboardsQuery{FolderIds: []int64{rootFolderId, folder1.Id}, SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, OrgId: 1} - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 4) - So(query.Result[0].Id, ShouldEqual, folder1.Id) - So(query.Result[1].Id, ShouldEqual, folder2.Id) - So(query.Result[2].Id, ShouldEqual, childDash1.Id) - So(query.Result[3].Id, ShouldEqual, dashInRoot.Id) - }) - }) - - Convey("and acl is set for one dashboard folder", func() { - var otherUser int64 = 999 - updateTestDashboardWithAcl(folder1.Id, otherUser, m.PERMISSION_EDIT) - - Convey("and a dashboard is moved from folder without acl to the folder with an acl", func() { - movedDash := moveDashboard(1, childDash2.Data, folder1.Id) - So(movedDash.HasAcl, ShouldBeTrue) - - Convey("should not return folder with acl or its children", func() { - query := &search.FindPersistedDashboardsQuery{ - SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, - OrgId: 1, - DashboardIds: []int64{folder1.Id, childDash1.Id, childDash2.Id, dashInRoot.Id}, - } - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 1) - So(query.Result[0].Id, ShouldEqual, dashInRoot.Id) - }) - }) - - Convey("and a dashboard is moved from folder with acl to the folder without an acl", func() { - movedDash := moveDashboard(1, childDash1.Data, folder2.Id) - So(movedDash.HasAcl, ShouldBeFalse) - - Convey("should return folder without acl and its children", func() { - query := &search.FindPersistedDashboardsQuery{ - SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, - OrgId: 1, - DashboardIds: []int64{folder2.Id, childDash1.Id, childDash2.Id, dashInRoot.Id}, - } - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 4) - So(query.Result[0].Id, ShouldEqual, folder2.Id) - So(query.Result[1].Id, ShouldEqual, childDash1.Id) - So(query.Result[2].Id, ShouldEqual, childDash2.Id) - So(query.Result[3].Id, ShouldEqual, dashInRoot.Id) - }) - }) - - Convey("and a dashboard with an acl is moved to the folder without an acl", func() { - updateTestDashboardWithAcl(childDash1.Id, otherUser, m.PERMISSION_EDIT) - movedDash := moveDashboard(1, childDash1.Data, folder2.Id) - So(movedDash.HasAcl, ShouldBeTrue) - - Convey("should return folder without acl but not the dashboard with acl", func() { - query := &search.FindPersistedDashboardsQuery{ - SignedInUser: &m.SignedInUser{UserId: currentUser.Id, OrgId: 1}, - OrgId: 1, - DashboardIds: []int64{folder2.Id, childDash1.Id, childDash2.Id, dashInRoot.Id}, - } - err := SearchDashboards(query) - So(err, ShouldBeNil) - So(len(query.Result), ShouldEqual, 3) - So(query.Result[0].Id, ShouldEqual, folder2.Id) - So(query.Result[1].Id, ShouldEqual, childDash2.Id) - So(query.Result[2].Id, ShouldEqual, dashInRoot.Id) - }) - }) - }) - }) - Convey("Given a plugin with imported dashboards", func() { pluginId := "test-app" From 3da2ab61e0ccf63adb6c602af9e9abfed2cee18b Mon Sep 17 00:00:00 2001 From: bergquist Date: Wed, 31 Jan 2018 14:36:14 +0100 Subject: [PATCH 4/6] Verifies requirement of id in dashboards. --- pkg/services/sqlstore/dashboard_test.go | 34 ++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index 6e77b44f8c1..007ac4a01d5 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -276,7 +276,6 @@ func TestDashboardDataAccess(t *testing.T) { cmd := m.SaveDashboardCommand{ OrgId: 1, Dashboard: simplejson.NewFromAny(map[string]interface{}{ - //"id": 1, "uid": "randomHash", "title": "folderId", "style": "light", @@ -306,6 +305,39 @@ func TestDashboardDataAccess(t *testing.T) { So(err, ShouldBeNil) }) + Convey("Should not be able to update using just uid", func() { + cmd := m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "uid": savedDash.Uid, + "title": "folderId", + "version": savedDash.Version, + "tags": []interface{}{}, + }), + FolderId: savedDash.FolderId, + } + + err := SaveDashboard(&cmd) + So(err, ShouldEqual, m.ErrDashboardWithSameUIDExists) + }) + + Convey("Should be able to update using just uid with overwrite", func() { + cmd := m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "uid": savedDash.Uid, + "title": "folderId", + "version": savedDash.Version, + "tags": []interface{}{}, + }), + FolderId: savedDash.FolderId, + Overwrite: true, + } + + err := SaveDashboard(&cmd) + So(err, ShouldBeNil) + }) + Convey("Should be able to update dashboard and remove folderId", func() { cmd := m.SaveDashboardCommand{ OrgId: 1, From 16a1642831a521e7a5a57a3fe298f742c7e873da Mon Sep 17 00:00:00 2001 From: bergquist Date: Wed, 31 Jan 2018 15:15:15 +0100 Subject: [PATCH 5/6] gofmt... --- pkg/services/sqlstore/dashboard.go | 2 -- pkg/services/sqlstore/dashboard_test.go | 10 +++++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index c378d70af10..4c04fe1251a 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -20,8 +20,6 @@ func init() { bus.AddHandler("sql", GetDashboardsByPluginId) } - - func SaveDashboard(cmd *m.SaveDashboardCommand) error { return inTransaction(func(sess *DBSession) error { dash := cmd.GetDashboardModel() diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index 007ac4a01d5..87010849a77 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -276,10 +276,10 @@ func TestDashboardDataAccess(t *testing.T) { cmd := m.SaveDashboardCommand{ OrgId: 1, Dashboard: simplejson.NewFromAny(map[string]interface{}{ - "uid": "randomHash", - "title": "folderId", - "style": "light", - "tags": []interface{}{}, + "uid": "randomHash", + "title": "folderId", + "style": "light", + "tags": []interface{}{}, }), FolderId: 2, } @@ -330,7 +330,7 @@ func TestDashboardDataAccess(t *testing.T) { "version": savedDash.Version, "tags": []interface{}{}, }), - FolderId: savedDash.FolderId, + FolderId: savedDash.FolderId, Overwrite: true, } From 58cfb236250e0d5b3bac72c1d261abcfb5572435 Mon Sep 17 00:00:00 2001 From: bergquist Date: Wed, 31 Jan 2018 17:27:28 +0100 Subject: [PATCH 6/6] retry uid generation --- pkg/middleware/dashboard_redirect_test.go | 2 + pkg/models/dashboards.go | 6 +-- pkg/services/sqlstore/dashboard.go | 62 ++++++++++++++++------- pkg/services/sqlstore/dashboard_test.go | 28 +++++++++- 4 files changed, 74 insertions(+), 24 deletions(-) diff --git a/pkg/middleware/dashboard_redirect_test.go b/pkg/middleware/dashboard_redirect_test.go index bff4ee2253c..21fc12e5e84 100644 --- a/pkg/middleware/dashboard_redirect_test.go +++ b/pkg/middleware/dashboard_redirect_test.go @@ -6,6 +6,7 @@ import ( "github.com/grafana/grafana/pkg/bus" m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/util" . "github.com/smartystreets/goconvey/convey" ) @@ -19,6 +20,7 @@ func TestMiddlewareDashboardRedirect(t *testing.T) { fakeDash.Id = 1 fakeDash.FolderId = 1 fakeDash.HasAcl = false + fakeDash.Uid = util.GenerateShortUid() bus.AddHandler("test", func(query *m.GetDashboardQuery) error { query.Result = fakeDash diff --git a/pkg/models/dashboards.go b/pkg/models/dashboards.go index c4361eefd7d..e06bc0d1f0d 100644 --- a/pkg/models/dashboards.go +++ b/pkg/models/dashboards.go @@ -9,7 +9,6 @@ import ( "github.com/gosimple/slug" "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/setting" - "github.com/grafana/grafana/pkg/util" ) // Typed errors @@ -23,6 +22,7 @@ var ( ErrDashboardFolderCannotHaveParent = errors.New("A Dashboard Folder cannot be added to another folder") ErrDashboardContainsInvalidAlertData = errors.New("Invalid alert data. Cannot save dashboard") ErrDashboardFailedToUpdateAlertData = errors.New("Failed to save alert data") + ErrDashboardFailedGenerateUniqueUid = errors.New("Failed to generate unique dashboard uid.") ) type UpdatePluginDashboardError struct { @@ -66,7 +66,6 @@ type Dashboard struct { // NewDashboard creates a new dashboard func NewDashboard(title string) *Dashboard { dash := &Dashboard{} - dash.Uid = util.GenerateShortUid() dash.Data = simplejson.New() dash.Data.Set("title", title) dash.Title = title @@ -115,9 +114,6 @@ func NewDashboardFromJson(data *simplejson.Json) *Dashboard { if uid, err := dash.Data.Get("uid").String(); err == nil { dash.Uid = uid - } else { - dash.Uid = util.GenerateShortUid() - dash.Data.Set("uid", dash.Uid) } return dash diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index 4c04fe1251a..f93e7f402ae 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/metrics" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/search" + "github.com/grafana/grafana/pkg/util" ) func init() { @@ -20,6 +21,8 @@ func init() { bus.AddHandler("sql", GetDashboardsByPluginId) } +var generateNewUid func() string = util.GenerateShortUid + func SaveDashboard(cmd *m.SaveDashboardCommand) error { return inTransaction(func(sess *DBSession) error { dash := cmd.GetDashboardModel() @@ -27,7 +30,7 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error { // try get existing dashboard var existing m.Dashboard - if dash.Id > 0 { + if dash.Id != 0 { dashWithIdExists, err := sess.Where("id=? AND org_id=?", dash.Id, dash.OrgId).Get(&existing) if err != nil { return err @@ -49,27 +52,36 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error { if existing.PluginId != "" && cmd.Overwrite == false { return m.UpdatePluginDashboardError{PluginId: existing.PluginId} } - } + } else if dash.Uid != "" { + var sameUid m.Dashboard + sameUidExists, err := sess.Where("org_id=? AND uid=?", dash.OrgId, dash.Uid).Get(&sameUid) + if err != nil { + return err + } - var sameUid m.Dashboard - sameUidExists, err := sess.Where("org_id=? AND uid=?", dash.OrgId, dash.Uid).Get(&sameUid) - if err != nil { - return err - } - - if sameUidExists { - // another dashboard with same uid - if dash.Id != sameUid.Id { - if cmd.Overwrite { - dash.Id = sameUid.Id - dash.Version = sameUid.Version - } else { - return m.ErrDashboardWithSameUIDExists + if sameUidExists { + // another dashboard with same uid + if dash.Id != sameUid.Id { + if cmd.Overwrite { + dash.Id = sameUid.Id + dash.Version = sameUid.Version + } else { + return m.ErrDashboardWithSameUIDExists + } } } } - err = guaranteeDashboardNameIsUniqueInFolder(sess, dash) + if dash.Uid == "" { + uid, err := generateNewDashboardUid(sess, dash.OrgId) + if err != nil { + return err + } + dash.Uid = uid + dash.Data.Set("uid", uid) + } + + err := guaranteeDashboardNameIsUniqueInFolder(sess, dash) if err != nil { return err } @@ -144,6 +156,22 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error { return err }) } +func generateNewDashboardUid(sess *DBSession, orgId int64) (string, error) { + for i := 0; i < 3; i++ { + uid := generateNewUid() + + exists, err := sess.Where("org_id=? AND uid=?", orgId, uid).Get(&m.Dashboard{}) + if err != nil { + return "", err + } + + if !exists { + return uid, nil + } + } + + return "", m.ErrDashboardFailedGenerateUniqueUid +} func guaranteeDashboardNameIsUniqueInFolder(sess *DBSession, dash *m.Dashboard) error { var sameNameInFolder m.Dashboard diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index 87010849a77..6e240b9585e 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -5,12 +5,12 @@ import ( "testing" "github.com/go-xorm/xorm" - . "github.com/smartystreets/goconvey/convey" - "github.com/grafana/grafana/pkg/components/simplejson" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/search" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util" + . "github.com/smartystreets/goconvey/convey" ) func TestDashboardDataAccess(t *testing.T) { @@ -338,6 +338,30 @@ func TestDashboardDataAccess(t *testing.T) { So(err, ShouldBeNil) }) + Convey("Should retry generation of uid once if it fails.", func() { + timesCalled := 0 + generateNewUid = func() string { + timesCalled += 1 + if timesCalled <= 2 { + return savedDash.Uid + } else { + return util.GenerateShortUid() + } + } + cmd := m.SaveDashboardCommand{ + OrgId: 1, + Dashboard: simplejson.NewFromAny(map[string]interface{}{ + "title": "new dash 12334", + "tags": []interface{}{}, + }), + } + + err := SaveDashboard(&cmd) + So(err, ShouldBeNil) + + generateNewUid = util.GenerateShortUid + }) + Convey("Should be able to update dashboard and remove folderId", func() { cmd := m.SaveDashboardCommand{ OrgId: 1,