From fc73bc1161c2801a027ad76a7022408d845a73df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Fri, 10 Sep 2021 11:22:13 +0200 Subject: [PATCH] LibraryElements: Enables creating library elements with specific UID (#39019) * LibraryPanels: Enables create/update library panels with specific UID * Chore: added check for uid length after PR comments * Refactor: creates IsShortUIDTooLong function * Refactor: adds UID to PATCH endpoint * Refactor: clarifies the patch code * Refactor: changes after PR comments --- pkg/services/dashboards/dashboard_service.go | 2 +- pkg/services/libraryelements/api.go | 6 ++ pkg/services/libraryelements/database.go | 30 +++++++- .../libraryelements_create_test.go | 71 +++++++++++++++++++ .../libraryelements_patch_test.go | 66 +++++++++++++++++ pkg/services/libraryelements/models.go | 10 ++- .../sqlstore/migrations/libraryelements.go | 4 ++ pkg/util/shortid_generator.go | 5 ++ pkg/util/shortid_generator_test.go | 36 +++++++++- 9 files changed, 224 insertions(+), 6 deletions(-) diff --git a/pkg/services/dashboards/dashboard_service.go b/pkg/services/dashboards/dashboard_service.go index d3e3a7aa3d2..2049082f667 100644 --- a/pkg/services/dashboards/dashboard_service.go +++ b/pkg/services/dashboards/dashboard_service.go @@ -103,7 +103,7 @@ func (dr *dashboardServiceImpl) buildSaveDashboardCommand(dto *SaveDashboardDTO, if !util.IsValidShortUID(dash.Uid) { return nil, models.ErrDashboardInvalidUid - } else if len(dash.Uid) > 40 { + } else if util.IsShortUIDTooLong(dash.Uid) { return nil, models.ErrDashboardUidTooLong } diff --git a/pkg/services/libraryelements/api.go b/pkg/services/libraryelements/api.go index 983ad3d6694..4df43e3760f 100644 --- a/pkg/services/libraryelements/api.go +++ b/pkg/services/libraryelements/api.go @@ -125,5 +125,11 @@ func toLibraryElementError(err error, message string) response.Response { if errors.Is(err, errLibraryElementHasConnections) { return response.Error(403, errLibraryElementHasConnections.Error(), err) } + if errors.Is(err, errLibraryElementInvalidUID) { + return response.Error(400, errLibraryElementInvalidUID.Error(), err) + } + if errors.Is(err, errLibraryElementUIDTooLong) { + return response.Error(400, errLibraryElementUIDTooLong.Error(), err) + } return response.Error(500, message, err) } diff --git a/pkg/services/libraryelements/database.go b/pkg/services/libraryelements/database.go index 3fa51f77d85..ddc5ed18342 100644 --- a/pkg/services/libraryelements/database.go +++ b/pkg/services/libraryelements/database.go @@ -2,6 +2,7 @@ package libraryelements import ( "encoding/json" + "errors" "fmt" "strings" "time" @@ -92,10 +93,20 @@ func (l *LibraryElementService) createLibraryElement(c *models.ReqContext, cmd C if err := l.requireSupportedElementKind(cmd.Kind); err != nil { return LibraryElementDTO{}, err } + createUID := cmd.UID + if len(createUID) == 0 { + createUID = util.GenerateShortUID() + } else { + if !util.IsValidShortUID(createUID) { + return LibraryElementDTO{}, errLibraryElementInvalidUID + } else if util.IsShortUIDTooLong(createUID) { + return LibraryElementDTO{}, errLibraryElementUIDTooLong + } + } element := LibraryElement{ OrgID: c.SignedInUser.OrgId, FolderID: cmd.FolderID, - UID: util.GenerateShortUID(), + UID: createUID, Name: cmd.Name, Model: cmd.Model, Version: 1, @@ -434,12 +445,27 @@ func (l *LibraryElementService) patchLibraryElement(c *models.ReqContext, cmd pa if elementInDB.Version != cmd.Version { return errLibraryElementVersionMismatch } + updateUID := cmd.UID + if len(updateUID) == 0 { + updateUID = uid + } else if updateUID != uid { + if !util.IsValidShortUID(updateUID) { + return errLibraryElementInvalidUID + } else if util.IsShortUIDTooLong(updateUID) { + return errLibraryElementUIDTooLong + } + + _, err := getLibraryElement(l.SQLStore.Dialect, session, updateUID, c.SignedInUser.OrgId) + if !errors.Is(err, errLibraryElementNotFound) { + return errLibraryElementAlreadyExists + } + } var libraryElement = LibraryElement{ ID: elementInDB.ID, OrgID: c.SignedInUser.OrgId, FolderID: cmd.FolderID, - UID: uid, + UID: updateUID, Name: cmd.Name, Kind: elementInDB.Kind, Type: elementInDB.Type, diff --git a/pkg/services/libraryelements/libraryelements_create_test.go b/pkg/services/libraryelements/libraryelements_create_test.go index 080905e5b25..01f72f35963 100644 --- a/pkg/services/libraryelements/libraryelements_create_test.go +++ b/pkg/services/libraryelements/libraryelements_create_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/util" ) func TestCreateLibraryElement(t *testing.T) { @@ -59,6 +60,76 @@ func TestCreateLibraryElement(t *testing.T) { } }) + testScenario(t, "When an admin tries to create a library panel that does not exists using an nonexistent UID, it should succeed", + func(t *testing.T, sc scenarioContext) { + command := getCreatePanelCommand(sc.folder.Id, "Nonexistent UID") + command.UID = util.GenerateShortUID() + resp := sc.service.createHandler(sc.reqContext, command) + var result = validateAndUnMarshalResponse(t, resp) + var expected = libraryElementResult{ + Result: libraryElement{ + ID: 1, + OrgID: 1, + FolderID: 1, + UID: command.UID, + Name: "Nonexistent UID", + Kind: int64(models.PanelElement), + Type: "text", + Description: "A description", + Model: map[string]interface{}{ + "datasource": "${DS_GDEV-TESTDATA}", + "description": "A description", + "id": float64(1), + "title": "Text - Library Panel", + "type": "text", + }, + Version: 1, + Meta: LibraryElementDTOMeta{ + ConnectedDashboards: 0, + Created: result.Result.Meta.Created, + Updated: result.Result.Meta.Updated, + CreatedBy: LibraryElementDTOMetaUser{ + ID: 1, + Name: "signed_in_user", + AvatarURL: "/avatar/37524e1eb8b3e32850b57db0a19af93b", + }, + UpdatedBy: LibraryElementDTOMetaUser{ + ID: 1, + Name: "signed_in_user", + AvatarURL: "/avatar/37524e1eb8b3e32850b57db0a19af93b", + }, + }, + }, + } + if diff := cmp.Diff(expected, result, getCompareOptions()...); diff != "" { + t.Fatalf("Result mismatch (-want +got):\n%s", diff) + } + }) + + scenarioWithPanel(t, "When an admin tries to create a library panel that does not exists using an existent UID, it should fail", + func(t *testing.T, sc scenarioContext) { + command := getCreatePanelCommand(sc.folder.Id, "Existing UID") + command.UID = sc.initialResult.Result.UID + resp := sc.service.createHandler(sc.reqContext, command) + require.Equal(t, 400, resp.Status()) + }) + + scenarioWithPanel(t, "When an admin tries to create a library panel that does not exists using an invalid UID, it should fail", + func(t *testing.T, sc scenarioContext) { + command := getCreatePanelCommand(sc.folder.Id, "Invalid UID") + command.UID = "Testing an invalid UID" + resp := sc.service.createHandler(sc.reqContext, command) + require.Equal(t, 400, resp.Status()) + }) + + scenarioWithPanel(t, "When an admin tries to create a library panel that does not exists using an UID that is too long, it should fail", + func(t *testing.T, sc scenarioContext) { + command := getCreatePanelCommand(sc.folder.Id, "Invalid UID") + command.UID = "j6T00KRZzj6T00KRZzj6T00KRZzj6T00KRZzj6T00K" + resp := sc.service.createHandler(sc.reqContext, command) + require.Equal(t, 400, resp.Status()) + }) + testScenario(t, "When an admin tries to create a library panel where name and panel title differ, it should not update panel title", func(t *testing.T, sc scenarioContext) { command := getCreatePanelCommand(1, "Library Panel Name") diff --git a/pkg/services/libraryelements/libraryelements_patch_test.go b/pkg/services/libraryelements/libraryelements_patch_test.go index 4b2c3d58eda..b644e1caa23 100644 --- a/pkg/services/libraryelements/libraryelements_patch_test.go +++ b/pkg/services/libraryelements/libraryelements_patch_test.go @@ -3,6 +3,8 @@ package libraryelements import ( "testing" + "github.com/grafana/grafana/pkg/util" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" @@ -122,6 +124,70 @@ func TestPatchLibraryElement(t *testing.T) { } }) + scenarioWithPanel(t, "When an admin tries to patch a library panel with a nonexistent UID, it should change UID successfully and return correct result", + func(t *testing.T, sc scenarioContext) { + cmd := patchLibraryElementCommand{ + FolderID: -1, + UID: util.GenerateShortUID(), + Kind: int64(models.PanelElement), + Version: 1, + } + sc.reqContext.ReplaceAllParams(map[string]string{":uid": sc.initialResult.Result.UID}) + resp := sc.service.patchHandler(sc.reqContext, cmd) + var result = validateAndUnMarshalResponse(t, resp) + sc.initialResult.Result.UID = cmd.UID + sc.initialResult.Result.Meta.CreatedBy.Name = userInDbName + sc.initialResult.Result.Meta.CreatedBy.AvatarURL = userInDbAvatar + sc.initialResult.Result.Model["title"] = "Text - Library Panel" + sc.initialResult.Result.Version = 2 + if diff := cmp.Diff(sc.initialResult.Result, result.Result, getCompareOptions()...); diff != "" { + t.Fatalf("Result mismatch (-want +got):\n%s", diff) + } + }) + + scenarioWithPanel(t, "When an admin tries to patch a library panel with an invalid UID, it should fail", + func(t *testing.T, sc scenarioContext) { + cmd := patchLibraryElementCommand{ + FolderID: -1, + UID: "Testing an invalid UID", + Kind: int64(models.PanelElement), + Version: 1, + } + sc.reqContext.ReplaceAllParams(map[string]string{":uid": sc.initialResult.Result.UID}) + resp := sc.service.patchHandler(sc.reqContext, cmd) + require.Equal(t, 400, resp.Status()) + }) + + scenarioWithPanel(t, "When an admin tries to patch a library panel with an UID that is too long, it should fail", + func(t *testing.T, sc scenarioContext) { + cmd := patchLibraryElementCommand{ + FolderID: -1, + UID: "j6T00KRZzj6T00KRZzj6T00KRZzj6T00KRZzj6T00K", + Kind: int64(models.PanelElement), + Version: 1, + } + sc.reqContext.ReplaceAllParams(map[string]string{":uid": sc.initialResult.Result.UID}) + resp := sc.service.patchHandler(sc.reqContext, cmd) + require.Equal(t, 400, resp.Status()) + }) + + scenarioWithPanel(t, "When an admin tries to patch a library panel with an existing UID, it should fail", + func(t *testing.T, sc scenarioContext) { + command := getCreatePanelCommand(sc.folder.Id, "Existing UID") + command.UID = util.GenerateShortUID() + resp := sc.service.createHandler(sc.reqContext, command) + require.Equal(t, 200, resp.Status()) + cmd := patchLibraryElementCommand{ + FolderID: -1, + UID: command.UID, + Kind: int64(models.PanelElement), + Version: 1, + } + sc.reqContext.ReplaceAllParams(map[string]string{":uid": sc.initialResult.Result.UID}) + resp = sc.service.patchHandler(sc.reqContext, cmd) + require.Equal(t, 400, resp.Status()) + }) + scenarioWithPanel(t, "When an admin tries to patch a library panel with model only, it should change model successfully, sync type and description fields and return correct result", func(t *testing.T, sc scenarioContext) { cmd := patchLibraryElementCommand{ diff --git a/pkg/services/libraryelements/models.go b/pkg/services/libraryelements/models.go index 6cecb54991a..711e6616ec9 100644 --- a/pkg/services/libraryelements/models.go +++ b/pkg/services/libraryelements/models.go @@ -136,7 +136,7 @@ type LibraryElementConnectionDTO struct { var ( // errLibraryElementAlreadyExists is an error for when the user tries to add a library element that already exists. - errLibraryElementAlreadyExists = errors.New("library element with that name already exists") + errLibraryElementAlreadyExists = errors.New("library element with that name or UID already exists") // errLibraryElementNotFound is an error for when a library element can't be found. errLibraryElementNotFound = errors.New("library element could not be found") // errLibraryElementDashboardNotFound is an error for when a library element connection can't be found. @@ -147,8 +147,12 @@ var ( errLibraryElementVersionMismatch = errors.New("the library element has been changed by someone else") // errLibraryElementUnSupportedElementKind is an error for when the kind is unsupported. errLibraryElementUnSupportedElementKind = errors.New("the element kind is not supported") - // ErrFolderHasConnectedLibraryElements is an error for when an user deletes a folder that contains connected library elements. + // ErrFolderHasConnectedLibraryElements is an error for when a user deletes a folder that contains connected library elements. ErrFolderHasConnectedLibraryElements = errors.New("folder contains library elements that are linked in use") + // errLibraryElementInvalidUID is an error for when the uid of a library element is invalid + errLibraryElementInvalidUID = errors.New("uid contains illegal characters") + // errLibraryElementUIDTooLong is an error for when the uid of a library element is invalid + errLibraryElementUIDTooLong = errors.New("uid too long, max 40 characters") ) // Commands @@ -159,6 +163,7 @@ type CreateLibraryElementCommand struct { Name string `json:"name"` Model json.RawMessage `json:"model"` Kind int64 `json:"kind" binding:"Required"` + UID string `json:"uid"` } // patchLibraryElementCommand is the command for patching a LibraryElement @@ -168,6 +173,7 @@ type patchLibraryElementCommand struct { Model json.RawMessage `json:"model"` Kind int64 `json:"kind" binding:"Required"` Version int64 `json:"version" binding:"Required"` + UID string `json:"uid"` } // searchLibraryElementsQuery is the query used for searching for Elements diff --git a/pkg/services/sqlstore/migrations/libraryelements.go b/pkg/services/sqlstore/migrations/libraryelements.go index 099e204fc1e..a60d6a72403 100644 --- a/pkg/services/sqlstore/migrations/libraryelements.go +++ b/pkg/services/sqlstore/migrations/libraryelements.go @@ -50,4 +50,8 @@ func addLibraryElementsMigrations(mg *migrator.Migrator) { mg.AddMigration("create "+models.LibraryElementConnectionTableName+" table v1", migrator.NewAddTableMigration(libraryElementConnectionV1)) mg.AddMigration("add index "+models.LibraryElementConnectionTableName+" element_id-kind-connection_id", migrator.NewAddIndexMigration(libraryElementConnectionV1, libraryElementConnectionV1.Indices[0])) + + mg.AddMigration("add unique index library_element org_id_uid", migrator.NewAddIndexMigration(libraryElementsV1, &migrator.Index{ + Cols: []string{"org_id", "uid"}, Type: migrator.UniqueIndex, + })) } diff --git a/pkg/util/shortid_generator.go b/pkg/util/shortid_generator.go index c035e088944..767ea1a9f3e 100644 --- a/pkg/util/shortid_generator.go +++ b/pkg/util/shortid_generator.go @@ -20,6 +20,11 @@ func IsValidShortUID(uid string) bool { return validUIDPattern(uid) } +// IsShortUIDTooLong checks if short unique identifier is too long +func IsShortUIDTooLong(uid string) bool { + return len(uid) > 40 +} + // GenerateShortUID generates a short unique identifier. func GenerateShortUID() string { return shortid.MustGenerate() diff --git a/pkg/util/shortid_generator_test.go b/pkg/util/shortid_generator_test.go index 94055f0bcfd..2e0fc0cc6d7 100644 --- a/pkg/util/shortid_generator_test.go +++ b/pkg/util/shortid_generator_test.go @@ -1,6 +1,10 @@ package util -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/require" +) func TestAllowedCharMatchesUidPattern(t *testing.T) { for _, c := range allowedChars { @@ -9,3 +13,33 @@ func TestAllowedCharMatchesUidPattern(t *testing.T) { } } } + +func TestIsShortUIDTooLong(t *testing.T) { + var tests = []struct { + name string + uid string + expected bool + }{ + { + name: "when the length of uid is longer than 40 chars then IsShortUIDTooLong should return true", + uid: allowedChars, + expected: true, + }, + { + name: "when the length of uid is equal too 40 chars then IsShortUIDTooLong should return false", + uid: "0123456789012345678901234567890123456789", + expected: false, + }, + { + name: "when the length of uid is shorter than 40 chars then IsShortUIDTooLong should return false", + uid: "012345678901234567890123456789012345678", + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.expected, IsShortUIDTooLong(tt.uid)) + }) + } +}