From 08205d64d1af8383932b11af100049d166922f4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20=C5=A0tibran=C3=BD?= Date: Thu, 24 Apr 2025 16:10:03 +0200 Subject: [PATCH] libraryelements: Spanner compability (#103881) * libraryelements: fix compatibility with Spanner * Log errors leading to status code 500. * Rename tests to make them integration tests. --- pkg/services/libraryelements/api.go | 22 ++++++++++--------- .../libraryelements_create_test.go | 2 +- .../libraryelements_delete_test.go | 5 +++-- .../libraryelements_get_all_test.go | 8 ++++--- .../libraryelements_get_test.go | 5 +++-- .../libraryelements_patch_test.go | 5 +++-- .../libraryelements/libraryelements_test.go | 5 +++-- pkg/services/libraryelements/writers.go | 9 +++++--- 8 files changed, 36 insertions(+), 25 deletions(-) diff --git a/pkg/services/libraryelements/api.go b/pkg/services/libraryelements/api.go index 09efc428c11..a26336e63c1 100644 --- a/pkg/services/libraryelements/api.go +++ b/pkg/services/libraryelements/api.go @@ -81,7 +81,7 @@ func (l *LibraryElementService) createHandler(c *contextmodel.ReqContext) respon element, err := l.createLibraryElement(c.Req.Context(), c.SignedInUser, cmd) if err != nil { - return toLibraryElementError(err, "Failed to create library element") + return l.toLibraryElementError(err, "Failed to create library element") } metrics.MFolderIDsServiceCount.WithLabelValues(metrics.LibraryElements).Inc() @@ -118,7 +118,7 @@ func (l *LibraryElementService) createHandler(c *contextmodel.ReqContext) respon func (l *LibraryElementService) deleteHandler(c *contextmodel.ReqContext) response.Response { id, err := l.deleteLibraryElement(c.Req.Context(), c.SignedInUser, web.Params(c.Req)[":uid"]) if err != nil { - return toLibraryElementError(err, "Failed to delete library element") + return l.toLibraryElementError(err, "Failed to delete library element") } return response.JSON(http.StatusOK, model.DeleteLibraryElementResponse{ @@ -148,7 +148,7 @@ func (l *LibraryElementService) getHandler(c *contextmodel.ReqContext) response. }, ) if err != nil { - return toLibraryElementError(err, "Failed to get library element") + return l.toLibraryElementError(err, "Failed to get library element") } if l.features.IsEnabled(ctx, featuremgmt.FlagLibraryPanelRBAC) { @@ -189,13 +189,13 @@ func (l *LibraryElementService) getAllHandler(c *contextmodel.ReqContext) respon } elementsResult, err := l.getAllLibraryElements(c.Req.Context(), c.SignedInUser, query) if err != nil { - return toLibraryElementError(err, "Failed to get library elements") + return l.toLibraryElementError(err, "Failed to get library elements") } if l.features.IsEnabled(c.Req.Context(), featuremgmt.FlagLibraryPanelRBAC) { filteredPanels, err := l.filterLibraryPanelsByPermission(c, elementsResult.Elements) if err != nil { - return toLibraryElementError(err, "Failed to evaluate permissions") + return l.toLibraryElementError(err, "Failed to evaluate permissions") } elementsResult.Elements = filteredPanels } @@ -241,7 +241,7 @@ func (l *LibraryElementService) patchHandler(c *contextmodel.ReqContext) respons element, err := l.patchLibraryElement(c.Req.Context(), c.SignedInUser, cmd, web.Params(c.Req)[":uid"]) if err != nil { - return toLibraryElementError(err, "Failed to update library element") + return l.toLibraryElementError(err, "Failed to update library element") } metrics.MFolderIDsServiceCount.WithLabelValues(metrics.LibraryElements).Inc() @@ -276,7 +276,7 @@ func (l *LibraryElementService) patchHandler(c *contextmodel.ReqContext) respons func (l *LibraryElementService) getConnectionsHandler(c *contextmodel.ReqContext) response.Response { connections, err := l.getConnections(c.Req.Context(), c.SignedInUser, web.Params(c.Req)[":uid"]) if err != nil { - return toLibraryElementError(err, "Failed to get connections") + return l.toLibraryElementError(err, "Failed to get connections") } return response.JSON(http.StatusOK, model.LibraryElementConnectionsResponse{Result: connections}) @@ -296,13 +296,13 @@ func (l *LibraryElementService) getConnectionsHandler(c *contextmodel.ReqContext func (l *LibraryElementService) getByNameHandler(c *contextmodel.ReqContext) response.Response { elements, err := l.getLibraryElementsByName(c.Req.Context(), c.SignedInUser, web.Params(c.Req)[":name"]) if err != nil { - return toLibraryElementError(err, "Failed to get library element") + return l.toLibraryElementError(err, "Failed to get library element") } if l.features.IsEnabled(c.Req.Context(), featuremgmt.FlagLibraryPanelRBAC) { filteredElements, err := l.filterLibraryPanelsByPermission(c, elements) if err != nil { - return toLibraryElementError(err, err.Error()) + return l.toLibraryElementError(err, err.Error()) } return response.JSON(http.StatusOK, model.LibraryElementArrayResponse{Result: filteredElements}) @@ -326,7 +326,7 @@ func (l *LibraryElementService) filterLibraryPanelsByPermission(c *contextmodel. return filteredPanels, nil } -func toLibraryElementError(err error, message string) response.Response { +func (l *LibraryElementService) toLibraryElementError(err error, message string) response.Response { if errors.Is(err, model.ErrLibraryElementAlreadyExists) { return response.Error(http.StatusBadRequest, model.ErrLibraryElementAlreadyExists.Error(), err) } @@ -354,6 +354,8 @@ func toLibraryElementError(err error, message string) response.Response { if errors.Is(err, model.ErrLibraryElementUIDTooLong) { return response.Error(http.StatusBadRequest, model.ErrLibraryElementUIDTooLong.Error(), err) } + // Log errors that cause internal server error status code. + l.log.Error(message, "error", err) return response.ErrOrFallback(http.StatusInternalServerError, message, err) } diff --git a/pkg/services/libraryelements/libraryelements_create_test.go b/pkg/services/libraryelements/libraryelements_create_test.go index 45e7a797b5b..5a66d170e83 100644 --- a/pkg/services/libraryelements/libraryelements_create_test.go +++ b/pkg/services/libraryelements/libraryelements_create_test.go @@ -11,7 +11,7 @@ import ( "github.com/grafana/grafana/pkg/util" ) -func TestCreateLibraryElement(t *testing.T) { +func TestIntegration_CreateLibraryElement(t *testing.T) { scenarioWithPanel(t, "When an admin tries to create a library panel that already exists, it should fail", func(t *testing.T, sc scenarioContext) { // nolint:staticcheck diff --git a/pkg/services/libraryelements/libraryelements_delete_test.go b/pkg/services/libraryelements/libraryelements_delete_test.go index 527cc1bd37d..5605313b4b6 100644 --- a/pkg/services/libraryelements/libraryelements_delete_test.go +++ b/pkg/services/libraryelements/libraryelements_delete_test.go @@ -4,15 +4,16 @@ import ( "encoding/json" "testing" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/libraryelements/model" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/web" - "github.com/stretchr/testify/require" ) -func TestDeleteLibraryElement(t *testing.T) { +func TestIntegration_DeleteLibraryElement(t *testing.T) { scenarioWithPanel(t, "When an admin tries to delete a library panel that does not exist, it should fail", func(t *testing.T, sc scenarioContext) { resp := sc.service.deleteHandler(sc.reqContext) diff --git a/pkg/services/libraryelements/libraryelements_get_all_test.go b/pkg/services/libraryelements/libraryelements_get_all_test.go index 501696c4791..5b57eac9de9 100644 --- a/pkg/services/libraryelements/libraryelements_get_all_test.go +++ b/pkg/services/libraryelements/libraryelements_get_all_test.go @@ -14,7 +14,7 @@ import ( "github.com/grafana/grafana/pkg/services/search/sort" ) -func TestGetAllLibraryElements(t *testing.T) { +func TestIntegration_GetAllLibraryElements(t *testing.T) { testScenario(t, "When an admin tries to get all library panels and none exists, it should return none", func(t *testing.T, sc scenarioContext) { resp := sc.service.getAllHandler(sc.reqContext) @@ -964,13 +964,14 @@ func TestGetAllLibraryElements(t *testing.T) { require.NoError(t, err) sc.reqContext.Req.Form.Add("perPage", "1") sc.reqContext.Req.Form.Add("page", "1") - sc.reqContext.Req.Form.Add("searchString", "description") + sc.reqContext.Req.Form.Add("searchString", "DeScRiPtIoN") // mixed case to test case-insensitive search. resp = sc.service.getAllHandler(sc.reqContext) require.Equal(t, 200, resp.Status()) var result libraryElementsSearch err = json.Unmarshal(resp.Body(), &result) require.NoError(t, err) + require.Equal(t, int64(1), result.Result.TotalCount) var expected = libraryElementsSearch{ Result: libraryElementsSearchResult{ TotalCount: 1, @@ -1039,13 +1040,14 @@ func TestGetAllLibraryElements(t *testing.T) { err := sc.reqContext.Req.ParseForm() require.NoError(t, err) - sc.reqContext.Req.Form.Add("searchString", "Library Panel") + sc.reqContext.Req.Form.Add("searchString", "library PANEL") // mixed-case to test case-insensitive search. resp = sc.service.getAllHandler(sc.reqContext) require.Equal(t, 200, resp.Status()) var result libraryElementsSearch err = json.Unmarshal(resp.Body(), &result) require.NoError(t, err) + require.Equal(t, int64(2), result.Result.TotalCount) var expected = libraryElementsSearch{ Result: libraryElementsSearchResult{ TotalCount: 2, diff --git a/pkg/services/libraryelements/libraryelements_get_test.go b/pkg/services/libraryelements/libraryelements_get_test.go index 5820e2bded7..42827e3f13a 100644 --- a/pkg/services/libraryelements/libraryelements_get_test.go +++ b/pkg/services/libraryelements/libraryelements_get_test.go @@ -5,6 +5,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/kinds/librarypanel" @@ -13,10 +15,9 @@ import ( "github.com/grafana/grafana/pkg/services/libraryelements/model" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/web" - "github.com/stretchr/testify/require" ) -func TestGetLibraryElement(t *testing.T) { +func TestIntegration_GetLibraryElement(t *testing.T) { scenarioWithPanel(t, "When an admin tries to get a library panel that does not exist, it should fail", func(t *testing.T, sc scenarioContext) { // by uid diff --git a/pkg/services/libraryelements/libraryelements_patch_test.go b/pkg/services/libraryelements/libraryelements_patch_test.go index 19cdb8c248a..3a7c962722f 100644 --- a/pkg/services/libraryelements/libraryelements_patch_test.go +++ b/pkg/services/libraryelements/libraryelements_patch_test.go @@ -8,11 +8,12 @@ import ( "github.com/grafana/grafana/pkg/util" "github.com/google/go-cmp/cmp" - "github.com/grafana/grafana/pkg/web" "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/web" ) -func TestPatchLibraryElement(t *testing.T) { +func TestIntegration_PatchLibraryElement(t *testing.T) { scenarioWithPanel(t, "When an admin tries to patch a library panel that does not exist, it should fail", func(t *testing.T, sc scenarioContext) { cmd := model.PatchLibraryElementCommand{Kind: int64(model.PanelElement), Version: 1} diff --git a/pkg/services/libraryelements/libraryelements_test.go b/pkg/services/libraryelements/libraryelements_test.go index 2d99b5edad1..faffe25225e 100644 --- a/pkg/services/libraryelements/libraryelements_test.go +++ b/pkg/services/libraryelements/libraryelements_test.go @@ -62,7 +62,7 @@ func TestMain(m *testing.M) { testsuite.Run(m) } -func TestDeleteLibraryPanelsInFolder(t *testing.T) { +func TestIntegration_DeleteLibraryPanelsInFolder(t *testing.T) { scenarioWithPanel(t, "When an admin tries to delete a folder that contains connected library elements, it should fail", func(t *testing.T, sc scenarioContext) { dashJSON := map[string]any{ @@ -137,7 +137,7 @@ func TestDeleteLibraryPanelsInFolder(t *testing.T) { }) } -func TestGetLibraryPanelConnections(t *testing.T) { +func TestIntegration_GetLibraryPanelConnections(t *testing.T) { scenarioWithPanel(t, "When an admin tries to get connections of library panel, it should succeed and return correct result", func(t *testing.T, sc scenarioContext) { dashJSON := map[string]any{ @@ -549,6 +549,7 @@ func testScenario(t *testing.T, desc string, fn func(t *testing.T, sc scenarioCo SQLStore: sqlStore, folderService: folderSvc, dashboardsService: dashService, + log: log.NewNopLogger(), } // deliberate difference between signed in user and user in db to make it crystal clear diff --git a/pkg/services/libraryelements/writers.go b/pkg/services/libraryelements/writers.go index 9629b96b66a..2c83ebbf4f6 100644 --- a/pkg/services/libraryelements/writers.go +++ b/pkg/services/libraryelements/writers.go @@ -60,8 +60,11 @@ func writeTypeFilterSQL(typeFilter []string, builder *db.SQLBuilder) { func writeSearchStringSQL(query model.SearchLibraryElementsQuery, sqlStore db.DB, builder *db.SQLBuilder) { if len(strings.TrimSpace(query.SearchString)) > 0 { - builder.Write(" AND (le.name "+sqlStore.GetDialect().LikeStr()+" ?", "%"+query.SearchString+"%") - builder.Write(" OR le.description "+sqlStore.GetDialect().LikeStr()+" ?)", "%"+query.SearchString+"%") + sql, param := sqlStore.GetDialect().LikeOperator("le.name", true, query.SearchString, true) + builder.Write(" AND ("+sql, param) + + sql, param = sqlStore.GetDialect().LikeOperator("le.description", true, query.SearchString, true) + builder.Write(" OR "+sql+")", param) } } @@ -147,7 +150,7 @@ func (f *FolderFilter) writeFolderFilterSQL(includeGeneral bool, builder *db.SQL if !includeGeneral && isGeneralFolder(folderID) { continue } - params = append(params, filter) + params = append(params, folderID) } if len(params) > 0 { sql.WriteString(` AND le.folder_id IN (?` + strings.Repeat(",?", len(params)-1) + ")")