From 8b0dd4244b7cfc00a335ff401a612d392ff9b87e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 17 Apr 2019 13:07:50 +0200 Subject: [PATCH] Search: Fixes search limits and adds a page parameter (#16458) * Search: Fixes search limits and adds a page parameter This adds a page parameter to search api without adding any major breaking change. It does at an api validation error when trying to use a limit beyond 5000. This is a breaking change. We could remove this and have it only in the docs and describe that this is a limit that grafana will apply silently. Fixes #16049 * Fix: Corrected wrong array slice change * Docs: minor docs fix * Search: fixed folder tests * Fixed: Moved limit to correct inner query * Search: moving limit check and page check * Search: limit in handler is no longer needed --- .../http_api/folder_dashboard_search.md | 5 +-- pkg/api/api.go | 2 +- pkg/api/folder.go | 2 +- pkg/api/folder_test.go | 2 +- pkg/api/search.go | 15 +++++---- pkg/services/dashboards/folder_service.go | 8 ++--- pkg/services/search/handlers.go | 5 +-- pkg/services/search/models.go | 6 ++-- pkg/services/sqlstore/dashboard.go | 7 +--- pkg/services/sqlstore/dashboard_test.go | 29 +++++++++++++++++ pkg/services/sqlstore/search_builder.go | 32 ++++++++++++------- pkg/services/sqlstore/search_builder_test.go | 2 +- 12 files changed, 73 insertions(+), 42 deletions(-) diff --git a/docs/sources/http_api/folder_dashboard_search.md b/docs/sources/http_api/folder_dashboard_search.md index 73b5dd90b87..0a736a277b7 100644 --- a/docs/sources/http_api/folder_dashboard_search.md +++ b/docs/sources/http_api/folder_dashboard_search.md @@ -23,7 +23,8 @@ Query parameters: - **dashboardIds** – List of dashboard id's to search for - **folderIds** – List of folder id's to search in for dashboards - **starred** – Flag indicating if only starred Dashboards should be returned -- **limit** – Limit the number of returned results +- **limit** – Limit the number of returned results (max 5000) +- **page** – Use this parameter to access hits beyond limit. Numbering starts at 1. limit param acts as page size. **Example request for retrieving folders and dashboards of the general folder**: @@ -95,4 +96,4 @@ Content-Type: application/json "uri":"db/production-overview" // deprecated in Grafana v5.0 } ] -``` \ No newline at end of file +``` diff --git a/pkg/api/api.go b/pkg/api/api.go index 2313a5d1a3b..82f75014ca4 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -325,7 +325,7 @@ func (hs *HTTPServer) registerRoutes() { }) // Search - apiRoute.Get("/search/", Search) + apiRoute.Get("/search/", Wrap(Search)) // metrics apiRoute.Post("/tsdb/query", bind(dtos.MetricRequest{}), Wrap(hs.QueryMetrics)) diff --git a/pkg/api/folder.go b/pkg/api/folder.go index 0a9a2671071..802cb71c403 100644 --- a/pkg/api/folder.go +++ b/pkg/api/folder.go @@ -12,7 +12,7 @@ import ( func GetFolders(c *m.ReqContext) Response { s := dashboards.NewFolderService(c.OrgId, c.SignedInUser) - folders, err := s.GetFolders(c.QueryInt("limit")) + folders, err := s.GetFolders(c.QueryInt64("limit")) if err != nil { return toFolderError(err) diff --git a/pkg/api/folder_test.go b/pkg/api/folder_test.go index 5e7184ae0c9..f96e107b968 100644 --- a/pkg/api/folder_test.go +++ b/pkg/api/folder_test.go @@ -213,7 +213,7 @@ type fakeFolderService struct { DeletedFolderUids []string } -func (s *fakeFolderService) GetFolders(limit int) ([]*m.Folder, error) { +func (s *fakeFolderService) GetFolders(limit int64) ([]*m.Folder, error) { return s.GetFoldersResult, s.GetFoldersError } diff --git a/pkg/api/search.go b/pkg/api/search.go index 22709bf8a6d..ae6a49352eb 100644 --- a/pkg/api/search.go +++ b/pkg/api/search.go @@ -9,16 +9,17 @@ import ( "github.com/grafana/grafana/pkg/services/search" ) -func Search(c *m.ReqContext) { +func Search(c *m.ReqContext) Response { query := c.Query("query") tags := c.QueryStrings("tag") starred := c.Query("starred") - limit := c.QueryInt("limit") + limit := c.QueryInt64("limit") + page := c.QueryInt64("page") dashboardType := c.Query("type") permission := m.PERMISSION_VIEW - if limit == 0 { - limit = 1000 + if limit > 5000 { + return Error(422, "Limit is above maximum allowed (5000), use page parameter to access hits beyond limit", nil) } if c.Query("permission") == "Edit" { @@ -46,6 +47,7 @@ func Search(c *m.ReqContext) { Tags: tags, SignedInUser: c.SignedInUser, Limit: limit, + Page: page, IsStarred: starred == "true", OrgId: c.OrgId, DashboardIds: dbIDs, @@ -56,10 +58,9 @@ func Search(c *m.ReqContext) { err := bus.Dispatch(&searchQuery) if err != nil { - c.JsonApiErr(500, "Search failed", err) - return + return Error(500, "Search failed", err) } c.TimeRequest(metrics.M_Api_Dashboard_Search) - c.JSON(200, searchQuery.Result) + return JSON(200, searchQuery.Result) } diff --git a/pkg/services/dashboards/folder_service.go b/pkg/services/dashboards/folder_service.go index b521b0e5213..a3c88863e79 100644 --- a/pkg/services/dashboards/folder_service.go +++ b/pkg/services/dashboards/folder_service.go @@ -9,7 +9,7 @@ import ( // FolderService service for operating on folders type FolderService interface { - GetFolders(limit int) ([]*models.Folder, error) + GetFolders(limit int64) ([]*models.Folder, error) GetFolderByID(id int64) (*models.Folder, error) GetFolderByUID(uid string) (*models.Folder, error) CreateFolder(cmd *models.CreateFolderCommand) error @@ -25,11 +25,7 @@ var NewFolderService = func(orgId int64, user *models.SignedInUser) FolderServic } } -func (dr *dashboardServiceImpl) GetFolders(limit int) ([]*models.Folder, error) { - if limit == 0 { - limit = 1000 - } - +func (dr *dashboardServiceImpl) GetFolders(limit int64) ([]*models.Folder, error) { searchQuery := search.Query{ SignedInUser: dr.user, DashboardIds: make([]int64, 0), diff --git a/pkg/services/search/handlers.go b/pkg/services/search/handlers.go index 9d40697f489..0ea07176dc0 100644 --- a/pkg/services/search/handlers.go +++ b/pkg/services/search/handlers.go @@ -31,6 +31,7 @@ func (s *SearchService) searchHandler(query *Query) error { FolderIds: query.FolderIds, Tags: query.Tags, Limit: query.Limit, + Page: query.Page, Permission: query.Permission, } @@ -44,10 +45,6 @@ func (s *SearchService) searchHandler(query *Query) error { // sort main result array sort.Sort(hits) - if len(hits) > query.Limit { - hits = hits[0:query.Limit] - } - // sort tags for _, hit := range hits { sort.Strings(hit.Tags) diff --git a/pkg/services/search/models.go b/pkg/services/search/models.go index 475cb4a3777..454ff261732 100644 --- a/pkg/services/search/models.go +++ b/pkg/services/search/models.go @@ -48,7 +48,8 @@ type Query struct { Tags []string OrgId int64 SignedInUser *models.SignedInUser - Limit int + Limit int64 + Page int64 IsStarred bool Type string DashboardIds []int64 @@ -67,7 +68,8 @@ type FindPersistedDashboardsQuery struct { Type string FolderIds []int64 Tags []string - Limit int + Limit int64 + Page int64 Permission models.PermissionType Result HitList diff --git a/pkg/services/sqlstore/dashboard.go b/pkg/services/sqlstore/dashboard.go index 8e1f69d1f6a..5a14bbd0d86 100644 --- a/pkg/services/sqlstore/dashboard.go +++ b/pkg/services/sqlstore/dashboard.go @@ -197,12 +197,7 @@ type DashboardSearchProjection struct { } func findDashboards(query *search.FindPersistedDashboardsQuery) ([]DashboardSearchProjection, error) { - limit := query.Limit - if limit == 0 { - limit = 1000 - } - - sb := NewSearchBuilder(query.SignedInUser, limit, query.Permission). + sb := NewSearchBuilder(query.SignedInUser, query.Limit, query.Page, query.Permission). WithTags(query.Tags). WithDashboardIdsIn(query.DashboardIds) diff --git a/pkg/services/sqlstore/dashboard_test.go b/pkg/services/sqlstore/dashboard_test.go index 1a82d314de0..e13da51ddd0 100644 --- a/pkg/services/sqlstore/dashboard_test.go +++ b/pkg/services/sqlstore/dashboard_test.go @@ -259,6 +259,35 @@ func TestDashboardDataAccess(t *testing.T) { So(hit.FolderTitle, ShouldEqual, "") }) + Convey("Should be able to limit search", func() { + query := search.FindPersistedDashboardsQuery{ + OrgId: 1, + Limit: 1, + SignedInUser: &m.SignedInUser{OrgId: 1, OrgRole: m.ROLE_EDITOR}, + } + + err := SearchDashboards(&query) + So(err, ShouldBeNil) + + So(len(query.Result), ShouldEqual, 1) + So(query.Result[0].Title, ShouldEqual, "1 test dash folder") + }) + + Convey("Should be able to search beyond limit using paging", func() { + query := search.FindPersistedDashboardsQuery{ + OrgId: 1, + Limit: 1, + Page: 2, + SignedInUser: &m.SignedInUser{OrgId: 1, OrgRole: m.ROLE_EDITOR}, + } + + err := SearchDashboards(&query) + So(err, ShouldBeNil) + + So(len(query.Result), ShouldEqual, 1) + So(query.Result[0].Title, ShouldEqual, "test dash 23") + }) + Convey("Should be able to search for a dashboard folder's children", func() { query := search.FindPersistedDashboardsQuery{ OrgId: 1, diff --git a/pkg/services/sqlstore/search_builder.go b/pkg/services/sqlstore/search_builder.go index 7817c4635c9..112d5e229b9 100644 --- a/pkg/services/sqlstore/search_builder.go +++ b/pkg/services/sqlstore/search_builder.go @@ -11,7 +11,8 @@ type SearchBuilder struct { SqlBuilder tags []string isStarred bool - limit int + limit int64 + page int64 signedInUser *m.SignedInUser whereDashboardIdsIn []int64 whereTitle string @@ -21,10 +22,21 @@ type SearchBuilder struct { permission m.PermissionType } -func NewSearchBuilder(signedInUser *m.SignedInUser, limit int, permission m.PermissionType) *SearchBuilder { +func NewSearchBuilder(signedInUser *m.SignedInUser, limit int64, page int64, permission m.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, } @@ -88,12 +100,16 @@ func (sb *SearchBuilder) ToSql() (string, []interface{}) { sb.buildMainQuery() } + sb.sql.WriteString(` + ORDER BY dashboard.id ` + 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" + dialect.Limit(5000)) - + sb.sql.WriteString(" ORDER BY dashboard.title ASC") return sb.sql.String(), sb.params } @@ -133,12 +149,7 @@ func (sb *SearchBuilder) buildTagQuery() { sb.buildSearchWhereClause() // this ends the inner select (tag filtered part) - sb.sql.WriteString(` - GROUP BY dashboard.id HAVING COUNT(dashboard.id) >= ? - ORDER BY dashboard.id` + dialect.Limit(int64(sb.limit)) + `) as ids - INNER JOIN dashboard on ids.id = dashboard.id - `) - + sb.sql.WriteString(`GROUP BY dashboard.id HAVING COUNT(dashboard.id) >= ? `) sb.params = append(sb.params, len(sb.tags)) } @@ -152,7 +163,6 @@ func (sb *SearchBuilder) buildMainQuery() { sb.sql.WriteString(` WHERE `) sb.buildSearchWhereClause() - sb.sql.WriteString(` ORDER BY dashboard.title` + dialect.Limit(int64(sb.limit)) + `) as ids INNER JOIN dashboard on ids.id = dashboard.id `) } func (sb *SearchBuilder) buildSearchWhereClause() { diff --git a/pkg/services/sqlstore/search_builder_test.go b/pkg/services/sqlstore/search_builder_test.go index e7ec3eedac6..e39c4248b96 100644 --- a/pkg/services/sqlstore/search_builder_test.go +++ b/pkg/services/sqlstore/search_builder_test.go @@ -14,7 +14,7 @@ func TestSearchBuilder(t *testing.T) { UserId: 1, } - sb := NewSearchBuilder(signedInUser, 1000, m.PERMISSION_VIEW) + sb := NewSearchBuilder(signedInUser, 1000, 0, m.PERMISSION_VIEW) Convey("When building a normal search", func() { sql, params := sb.IsStarred().WithTitle("test").ToSql()