From 830e8dc5fd50d68a2a7fb3e57a560d49f41085c5 Mon Sep 17 00:00:00 2001 From: Emil Tullstedt Date: Mon, 27 Apr 2020 14:16:03 +0200 Subject: [PATCH] Search: Replace search implementation (#23855) --- pkg/services/search/service.go | 42 ++-- pkg/services/search/sorting.go | 7 +- pkg/services/sqlstore/dashboard.go | 80 +++---- .../sqlstore/migrations/dashboard_mig.go | 5 + pkg/services/sqlstore/search_builder.go | 214 ------------------ pkg/services/sqlstore/search_builder_test.go | 39 ---- pkg/services/sqlstore/searchstore/builder.go | 29 ++- .../sqlstore/searchstore/search_test.go | 48 ++-- 8 files changed, 96 insertions(+), 368 deletions(-) delete mode 100644 pkg/services/sqlstore/search_builder.go delete mode 100644 pkg/services/sqlstore/search_builder_test.go diff --git a/pkg/services/search/service.go b/pkg/services/search/service.go index ff16bf73490..b550150bc3c 100644 --- a/pkg/services/search/service.go +++ b/pkg/services/search/service.go @@ -1,9 +1,10 @@ package search import ( + "sort" + "github.com/grafana/grafana/pkg/services/sqlstore/searchstore" "github.com/grafana/grafana/pkg/setting" - "sort" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" @@ -48,8 +49,7 @@ type FindPersistedDashboardsQuery struct { Page int64 Permission models.PermissionType - FeatureSearch2 bool - SortBy searchstore.FilterOrderBy + SortBy searchstore.FilterOrderBy Result HitList } @@ -72,29 +72,21 @@ func (s *SearchService) Init() error { } func (s *SearchService) searchHandler(query *Query) error { - sortOpt, exists := s.sortOptions[query.Sort] - if !exists { - sortOpt = sortAlphaAsc - } - - search2 := false - if s.Cfg != nil { - search2 = s.Cfg.FeatureToggles["search2"] - } - dashboardQuery := FindPersistedDashboardsQuery{ - Title: query.Title, - SignedInUser: query.SignedInUser, - IsStarred: query.IsStarred, - DashboardIds: query.DashboardIds, - Type: query.Type, - FolderIds: query.FolderIds, - Tags: query.Tags, - Limit: query.Limit, - Page: query.Page, - Permission: query.Permission, - FeatureSearch2: search2, - SortBy: sortOpt.Filter, + Title: query.Title, + SignedInUser: query.SignedInUser, + IsStarred: query.IsStarred, + DashboardIds: query.DashboardIds, + Type: query.Type, + FolderIds: query.FolderIds, + Tags: query.Tags, + Limit: query.Limit, + Page: query.Page, + Permission: query.Permission, + } + + if sortOpt, exists := s.sortOptions[query.Sort]; exists { + dashboardQuery.SortBy = sortOpt.Filter } if err := bus.Dispatch(&dashboardQuery); err != nil { diff --git a/pkg/services/search/sorting.go b/pkg/services/search/sorting.go index 5ab0c5ee510..7d8e193b730 100644 --- a/pkg/services/search/sorting.go +++ b/pkg/services/search/sorting.go @@ -1,20 +1,21 @@ package search import ( - "github.com/grafana/grafana/pkg/services/sqlstore/searchstore" "sort" + + "github.com/grafana/grafana/pkg/services/sqlstore/searchstore" ) var ( sortAlphaAsc = SortOption{ Name: "alpha-asc", - DisplayName: "A-Z", + DisplayName: "Alphabetically (A-Z)", Description: "Sort results in an alphabetically ascending order", Filter: searchstore.TitleSorter{}, } sortAlphaDesc = SortOption{ Name: "alpha-desc", - DisplayName: "Z-A", + DisplayName: "Alphabetically (Z-A)", Description: "Sort results in an alphabetically descending order", Filter: searchstore.TitleSorter{Descending: true}, } diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index a1b251ed700..78b19873a90 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -1,13 +1,12 @@ package sqlstore import ( + "strings" + "time" + "github.com/grafana/grafana/pkg/services/sqlstore/permissions" "github.com/grafana/grafana/pkg/services/sqlstore/searchstore" "github.com/prometheus/client_golang/prometheus" - "reflect" - "strconv" - "strings" - "time" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/metrics" @@ -217,11 +216,11 @@ type DashboardSearchProjection struct { } func findDashboards(query *search.FindPersistedDashboardsQuery) ([]DashboardSearchProjection, error) { - sb := NewSearchBuilder(query.SignedInUser, query.Limit, query.Page, query.Permission). - WithTags(query.Tags). - WithDashboardIdsIn(query.DashboardIds) + if query.SortBy == nil { + query.SortBy = searchstore.TitleSorter{} + } - sb2filters := []interface{}{ + filters := []interface{}{ query.SortBy, permissions.DashboardPermissionFilter{ OrgRole: query.SignedInUser.OrgRole, @@ -232,72 +231,55 @@ func findDashboards(query *search.FindPersistedDashboardsQuery) ([]DashboardSear }, } + if query.OrgId != 0 { + filters = append(filters, searchstore.OrgFilter{OrgId: query.OrgId}) + } else if query.SignedInUser.OrgId != 0 { + filters = append(filters, searchstore.OrgFilter{OrgId: query.SignedInUser.OrgId}) + } + if len(query.Tags) > 0 { - sb2filters = append(sb2filters, searchstore.TagsFilter{Tags: query.Tags}) + filters = append(filters, searchstore.TagsFilter{Tags: query.Tags}) } if len(query.DashboardIds) > 0 { - sb2filters = append(sb2filters, searchstore.DashboardFilter{IDs: query.DashboardIds}) + filters = append(filters, searchstore.DashboardFilter{IDs: query.DashboardIds}) } if query.IsStarred { - sb.IsStarred() - sb2filters = append(sb2filters, searchstore.StarredFilter{UserId: query.SignedInUser.UserId}) + filters = append(filters, searchstore.StarredFilter{UserId: query.SignedInUser.UserId}) } if len(query.Title) > 0 { - sb.WithTitle(query.Title) - sb2filters = append(sb2filters, searchstore.TitleFilter{Dialect: dialect, Title: query.Title}) + filters = append(filters, searchstore.TitleFilter{Dialect: dialect, Title: query.Title}) } if len(query.Type) > 0 { - sb.WithType(query.Type) - sb2filters = append(sb2filters, searchstore.TypeFilter{Dialect: dialect, Type: query.Type}) + filters = append(filters, searchstore.TypeFilter{Dialect: dialect, Type: query.Type}) } if len(query.FolderIds) > 0 { - sb.WithFolderIds(query.FolderIds) - sb2filters = append(sb2filters, searchstore.FolderFilter{IDs: query.FolderIds}) + filters = append(filters, searchstore.FolderFilter{IDs: query.FolderIds}) } var res []DashboardSearchProjection + sb := &searchstore.Builder{Dialect: dialect, Filters: filters} - sql, params := sb.ToSql() + limit := query.Limit + if limit < 1 { + limit = 1000 + } + + page := query.Page + if page < 1 { + page = 1 + } + + sql, params := sb.ToSql(limit, page) err := x.SQL(sql, params...).Find(&res) if err != nil { return nil, err } - if query.FeatureSearch2 { - var res2 []DashboardSearchProjection - sb := &searchstore.Builder{Dialect: dialect, Filters: sb2filters} - limit := query.Limit - if limit < 1 { - limit = 1000 - } - - page := query.Page - if page < 1 { - page = 1 - } - shadowSql, params := sb.ToSql(limit, page) - err = x.SQL(shadowSql, params...).Find(&res2) - - equal := reflect.DeepEqual(res2, res) - shadowSearchCounter.With(prometheus.Labels{ - "equal": strconv.FormatBool(equal), - "error": strconv.FormatBool(err != nil), - }).Inc() - sqlog.Debug( - "shadow search query result", - "err", err, - "equal", equal, - "shadowQuery", strings.Replace(strings.Replace(shadowSql, "\n", " ", -1), "\t", " ", -1), - "query", strings.Replace(strings.Replace(sql, "\n", " ", -1), "\t", " ", -1), - ) - return res2, nil - } - return res, nil } diff --git a/pkg/services/sqlstore/migrations/dashboard_mig.go b/pkg/services/sqlstore/migrations/dashboard_mig.go index b47261d06d7..9783958c7a1 100644 --- a/pkg/services/sqlstore/migrations/dashboard_mig.go +++ b/pkg/services/sqlstore/migrations/dashboard_mig.go @@ -215,4 +215,9 @@ func addDashboardMigration(mg *Migrator) { mg.AddMigration("Add check_sum column", NewAddColumnMigration(dashboardExtrasTableV2, &Column{ Name: "check_sum", Type: DB_NVarchar, Length: 32, Nullable: true, })) + mg.AddMigration("Add index for dashboard_title", NewAddIndexMigration(dashboardV2, &Index{ + Cols: []string{"title"}, + Type: IndexType, + })) + } diff --git a/pkg/services/sqlstore/search_builder.go b/pkg/services/sqlstore/search_builder.go deleted file mode 100644 index 4273400d757..00000000000 --- a/pkg/services/sqlstore/search_builder.go +++ /dev/null @@ -1,214 +0,0 @@ -package sqlstore - -import ( - "github.com/grafana/grafana/pkg/services/sqlstore/migrator" - "strings" - - "github.com/grafana/grafana/pkg/models" -) - -// SearchBuilder is a builder/object mother that builds a dashboard search query -type SearchBuilder struct { - SqlBuilder - - dialect migrator.Dialect - tags []string - isStarred bool - limit int64 - page int64 - signedInUser *models.SignedInUser - whereDashboardIdsIn []int64 - whereTitle string - whereTypeFolder bool - whereTypeDash bool - whereFolderIds []int64 - permission models.PermissionType -} - -func NewSearchBuilder(signedInUser *models.SignedInUser, limit int64, page int64, permission models.PermissionType) *SearchBuilder { - // Default to page 1 - if page < 1 { - page = 1 - } - - // default limit - if limit <= 0 { - limit = 1000 - } - - searchBuilder := &SearchBuilder{ - signedInUser: signedInUser, - limit: limit, - page: page, - permission: permission, - dialect: dialect, - } - - return searchBuilder -} - -func (sb *SearchBuilder) WithDialect(dialect migrator.Dialect) *SearchBuilder { - sb.dialect = dialect - return sb -} - -func (sb *SearchBuilder) WithTags(tags []string) *SearchBuilder { - if len(tags) > 0 { - sb.tags = tags - } - - return sb -} - -func (sb *SearchBuilder) IsStarred() *SearchBuilder { - sb.isStarred = true - - return sb -} - -func (sb *SearchBuilder) WithDashboardIdsIn(ids []int64) *SearchBuilder { - if len(ids) > 0 { - sb.whereDashboardIdsIn = ids - } - - return sb -} - -func (sb *SearchBuilder) WithTitle(title string) *SearchBuilder { - sb.whereTitle = title - - return sb -} - -func (sb *SearchBuilder) WithType(queryType string) *SearchBuilder { - if len(queryType) > 0 && queryType == "dash-folder" { - sb.whereTypeFolder = true - } - - if len(queryType) > 0 && queryType == "dash-db" { - sb.whereTypeDash = true - } - - return sb -} - -func (sb *SearchBuilder) WithFolderIds(folderIds []int64) *SearchBuilder { - sb.whereFolderIds = folderIds - return sb -} - -// ToSql builds the sql and returns it as a string, together with the params. -func (sb *SearchBuilder) ToSql() (string, []interface{}) { - sb.params = make([]interface{}, 0) - - sb.buildSelect() - - if len(sb.tags) > 0 { - sb.buildTagQuery() - } else { - sb.buildMainQuery() - } - - sb.sql.WriteString(` - ORDER BY dashboard.id ` + sb.dialect.LimitOffset(sb.limit, (sb.page-1)*sb.limit) + `) as ids - INNER JOIN dashboard on ids.id = dashboard.id - `) - - sb.sql.WriteString(` - LEFT OUTER JOIN dashboard folder on folder.id = dashboard.folder_id - LEFT OUTER JOIN dashboard_tag on dashboard.id = dashboard_tag.dashboard_id`) - - sb.sql.WriteString(" ORDER BY dashboard.title ASC") - return sb.sql.String(), sb.params -} - -func (sb *SearchBuilder) buildSelect() { - sb.sql.WriteString( - `SELECT - dashboard.id, - dashboard.uid, - dashboard.title, - dashboard.slug, - dashboard_tag.term, - dashboard.is_folder, - dashboard.folder_id, - folder.uid as folder_uid, - folder.slug as folder_slug, - folder.title as folder_title - FROM `) -} - -func (sb *SearchBuilder) buildTagQuery() { - sb.sql.WriteString( - `( - SELECT - dashboard.id FROM dashboard - LEFT OUTER JOIN dashboard_tag ON dashboard_tag.dashboard_id = dashboard.id - `) - - if sb.isStarred { - sb.sql.WriteString(" INNER JOIN star on star.dashboard_id = dashboard.id") - } - - sb.sql.WriteString(` WHERE dashboard_tag.term IN (?` + strings.Repeat(",?", len(sb.tags)-1) + `) AND `) - for _, tag := range sb.tags { - sb.params = append(sb.params, tag) - } - - sb.buildSearchWhereClause() - - // this ends the inner select (tag filtered part) - sb.sql.WriteString(` GROUP BY dashboard.id HAVING COUNT(dashboard.id) >= ? `) - sb.params = append(sb.params, len(sb.tags)) -} - -func (sb *SearchBuilder) buildMainQuery() { - sb.sql.WriteString(`( SELECT dashboard.id FROM dashboard `) - - if sb.isStarred { - sb.sql.WriteString(" INNER JOIN star on star.dashboard_id = dashboard.id") - } - - sb.sql.WriteString(` WHERE `) - sb.buildSearchWhereClause() - -} - -func (sb *SearchBuilder) buildSearchWhereClause() { - sb.sql.WriteString(` dashboard.org_id=?`) - sb.params = append(sb.params, sb.signedInUser.OrgId) - - if sb.isStarred { - sb.sql.WriteString(` AND star.user_id=?`) - sb.params = append(sb.params, sb.signedInUser.UserId) - } - - if len(sb.whereDashboardIdsIn) > 0 { - sb.sql.WriteString(` AND dashboard.id IN (?` + strings.Repeat(",?", len(sb.whereDashboardIdsIn)-1) + `)`) - for _, dashboardId := range sb.whereDashboardIdsIn { - sb.params = append(sb.params, dashboardId) - } - } - - sb.writeDashboardPermissionFilter(sb.signedInUser, sb.permission) - - if len(sb.whereTitle) > 0 { - sb.sql.WriteString(" AND dashboard.title " + sb.dialect.LikeStr() + " ?") - sb.params = append(sb.params, "%"+sb.whereTitle+"%") - } - - if sb.whereTypeFolder { - sb.sql.WriteString(" AND dashboard.is_folder = " + sb.dialect.BooleanStr(true)) - } - - if sb.whereTypeDash { - sb.sql.WriteString(" AND dashboard.is_folder = " + sb.dialect.BooleanStr(false)) - } - - if len(sb.whereFolderIds) > 0 { - sb.sql.WriteString(` AND dashboard.folder_id IN (?` + strings.Repeat(",?", len(sb.whereFolderIds)-1) + `) `) - for _, id := range sb.whereFolderIds { - sb.params = append(sb.params, id) - } - } -} diff --git a/pkg/services/sqlstore/search_builder_test.go b/pkg/services/sqlstore/search_builder_test.go deleted file mode 100644 index 7d199488f78..00000000000 --- a/pkg/services/sqlstore/search_builder_test.go +++ /dev/null @@ -1,39 +0,0 @@ -package sqlstore - -import ( - "github.com/grafana/grafana/pkg/services/sqlstore/migrator" - "testing" - - "github.com/grafana/grafana/pkg/models" - . "github.com/smartystreets/goconvey/convey" -) - -func TestSearchBuilder(t *testing.T) { - Convey("Testing building a search", t, func() { - if dialect == nil { - dialect = &migrator.Sqlite3{} - } - signedInUser := &models.SignedInUser{ - OrgId: 1, - UserId: 1, - } - - sb := NewSearchBuilder(signedInUser, 1000, 0, models.PERMISSION_VIEW) - - Convey("When building a normal search", func() { - sql, params := sb.IsStarred().WithTitle("test").ToSql() - So(sql, ShouldStartWith, "SELECT") - So(sql, ShouldContainSubstring, "INNER JOIN dashboard on ids.id = dashboard.id") - So(sql, ShouldContainSubstring, "ORDER BY dashboard.title ASC") - So(len(params), ShouldBeGreaterThan, 0) - }) - - Convey("When building a search with tag filter", func() { - sql, params := sb.WithTags([]string{"tag1", "tag2"}).ToSql() - So(sql, ShouldStartWith, "SELECT") - So(sql, ShouldContainSubstring, "LEFT OUTER JOIN dashboard_tag") - So(sql, ShouldContainSubstring, "ORDER BY dashboard.title ASC") - So(len(params), ShouldBeGreaterThan, 0) - }) - }) -} diff --git a/pkg/services/sqlstore/searchstore/builder.go b/pkg/services/sqlstore/searchstore/builder.go index 6c5872c18d1..b3c01f0a18c 100644 --- a/pkg/services/sqlstore/searchstore/builder.go +++ b/pkg/services/sqlstore/searchstore/builder.go @@ -3,8 +3,9 @@ package searchstore import ( "bytes" "fmt" - "github.com/grafana/grafana/pkg/services/sqlstore/migrator" "strings" + + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" ) // Builder defaults to returning a SQL query to get a list of all dashboards @@ -27,15 +28,17 @@ func (b *Builder) ToSql(limit, page int64) (string, []interface{}) { b.buildSelect() b.sql.WriteString("( ") - b.applyFilters() + orderQuery := b.applyFilters() b.sql.WriteString(b.Dialect.LimitOffset(limit, (page-1)*limit) + `) AS ids - INNER JOIN dashboard ON ids.id = dashboard.id - `) + INNER JOIN dashboard ON ids.id = dashboard.id`) + b.sql.WriteString("\n") - b.sql.WriteString(` - LEFT OUTER JOIN dashboard AS folder ON folder.id = dashboard.folder_id + b.sql.WriteString( + `LEFT OUTER JOIN dashboard AS folder ON folder.id = dashboard.folder_id LEFT OUTER JOIN dashboard_tag ON dashboard.id = dashboard_tag.dashboard_id`) + b.sql.WriteString("\n") + b.sql.WriteString(orderQuery) return b.sql.String(), b.params } @@ -56,8 +59,9 @@ func (b *Builder) buildSelect() { FROM `) } -func (b *Builder) applyFilters() { +func (b *Builder) applyFilters() (ordering string) { joins := []string{} + orderJoins := []string{} wheres := []string{} whereParams := []interface{}{} @@ -89,6 +93,9 @@ func (b *Builder) applyFilters() { } if f, ok := f.(FilterOrderBy); ok { + if f, ok := f.(FilterLeftJoin); ok { + orderJoins = append(orderJoins, fmt.Sprintf(" LEFT OUTER JOIN %s ", f.LeftJoin())) + } orders = append(orders, f.OrderBy()) } } @@ -107,6 +114,12 @@ func (b *Builder) applyFilters() { } if len(orders) > 0 { - b.sql.WriteString(fmt.Sprintf(" ORDER BY %s", strings.Join(orders, ", "))) + orderBy := fmt.Sprintf(" ORDER BY %s", strings.Join(orders, ", ")) + b.sql.WriteString(orderBy) + + order := strings.Join(orderJoins, "") + order += orderBy + return order } + return " ORDER BY dashboard.id" } diff --git a/pkg/services/sqlstore/searchstore/search_test.go b/pkg/services/sqlstore/searchstore/search_test.go index 8326ad31027..0a66b84cfa1 100644 --- a/pkg/services/sqlstore/searchstore/search_test.go +++ b/pkg/services/sqlstore/searchstore/search_test.go @@ -4,6 +4,9 @@ package searchstore_test import ( "context" "fmt" + "testing" + "time" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -12,8 +15,6 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore/searchstore" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "testing" - "time" ) var dialect migrator.Dialect @@ -47,25 +48,23 @@ func TestBuilder_EqualResults_Basic(t *testing.T) { Dialect: dialect, } - prevBuilder := sqlstore.NewSearchBuilder(user, limit, page, models.PERMISSION_EDIT) - prevBuilder.WithDialect(dialect) - - newRes := []sqlstore.DashboardSearchProjection{} + res := []sqlstore.DashboardSearchProjection{} err = db.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { sql, params := builder.ToSql(limit, page) - return sess.SQL(sql, params...).Find(&newRes) + return sess.SQL(sql, params...).Find(&res) }) require.NoError(t, err) - oldRes := []sqlstore.DashboardSearchProjection{} - err = db.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { - sql, params := prevBuilder.ToSql() - return sess.SQL(sql, params...).Find(&oldRes) - }) - require.NoError(t, err) - - assert.Len(t, newRes, 1) - assert.EqualValues(t, oldRes, newRes) + assert.Len(t, res, 1) + res[0].Uid = "" + assert.EqualValues(t, []sqlstore.DashboardSearchProjection{ + { + Id: 1, + Title: "A", + Slug: "a", + Term: "templated", + }, + }, res) } func TestBuilder_Pagination(t *testing.T) { @@ -143,25 +142,14 @@ func TestBuilder_Permissions(t *testing.T) { Dialect: dialect, } - prevBuilder := sqlstore.NewSearchBuilder(user, limit, page, level) - prevBuilder.WithDialect(dialect) - - newRes := []sqlstore.DashboardSearchProjection{} + res := []sqlstore.DashboardSearchProjection{} err = db.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { sql, params := builder.ToSql(limit, page) - return sess.SQL(sql, params...).Find(&newRes) + return sess.SQL(sql, params...).Find(&res) }) require.NoError(t, err) - oldRes := []sqlstore.DashboardSearchProjection{} - err = db.WithDbSession(context.Background(), func(sess *sqlstore.DBSession) error { - sql, params := prevBuilder.ToSql() - return sess.SQL(sql, params...).Find(&oldRes) - }) - require.NoError(t, err) - - assert.Len(t, newRes, 0) - assert.EqualValues(t, oldRes, newRes) + assert.Len(t, res, 0) } func setupTestEnvironment(t *testing.T) *sqlstore.SqlStore {