From 503c8cd8ef0b6decc12effb1e4962b26c88ca4f8 Mon Sep 17 00:00:00 2001 From: bergquist Date: Wed, 20 Jun 2018 16:55:42 +0200 Subject: [PATCH 01/14] tech: adds comments about route register code --- pkg/api/route_register.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/api/route_register.go b/pkg/api/route_register.go index 926de13c546..5b62e8585b7 100644 --- a/pkg/api/route_register.go +++ b/pkg/api/route_register.go @@ -14,15 +14,30 @@ type Router interface { // RouteRegister allows you to add routes and macaron.Handlers // that the web server should serve. type RouteRegister interface { + // Get adds a list of handlers to a given route with a GET HTTP verb Get(string, ...macaron.Handler) + + // Post adds a list of handlers to a given route with a POST HTTP verb Post(string, ...macaron.Handler) + + // Delete adds a list of handlers to a given route with a DELETE HTTP verb Delete(string, ...macaron.Handler) + + // Put adds a list of handlers to a given route with a PUT HTTP verb Put(string, ...macaron.Handler) + + // Patch adds a list of handlers to a given route with a PATCH HTTP verb Patch(string, ...macaron.Handler) + + // Any adds a list of handlers to a given route with any HTTP verb Any(string, ...macaron.Handler) + // Group allows you to pass a function that can add multiple routes + // with a shared prefix route. Group(string, func(RouteRegister), ...macaron.Handler) + // Register iterates over all routes added to the RouteRegister + // and add them to the `Router` pass as an parameter. Register(Router) *macaron.Router } From 97db9ece987218411169891300fdef6366fc6978 Mon Sep 17 00:00:00 2001 From: rozetko Date: Fri, 22 Jun 2018 16:17:02 +0300 Subject: [PATCH 02/14] Set $rootScope in DatasourceSrv --- public/app/features/plugins/datasource_srv.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/public/app/features/plugins/datasource_srv.ts b/public/app/features/plugins/datasource_srv.ts index aef43a4760b..19bdf599ce1 100644 --- a/public/app/features/plugins/datasource_srv.ts +++ b/public/app/features/plugins/datasource_srv.ts @@ -7,7 +7,7 @@ export class DatasourceSrv { datasources: any; /** @ngInject */ - constructor(private $q, private $injector, $rootScope, private templateSrv) { + constructor(private $q, private $injector, private $rootScope, private templateSrv) { this.init(); } @@ -61,7 +61,7 @@ export class DatasourceSrv { this.datasources[name] = instance; deferred.resolve(instance); }) - .catch(function(err) { + .catch(err => { this.$rootScope.appEvent('alert-error', [dsConfig.name + ' plugin failed', err.toString()]); }); From defb0396b74d2731ba9ea406a5b4e7522bbcb9b4 Mon Sep 17 00:00:00 2001 From: Austin Winstanley Date: Fri, 22 Jun 2018 21:15:36 -0500 Subject: [PATCH 03/14] Return a 404 when deleting a datasource through the API if it doesn't exist and add a test for it to confirm #12313 --- pkg/api/datasources.go | 3 +++ pkg/api/datasources_test.go | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/pkg/api/datasources.go b/pkg/api/datasources.go index 99677a93ee6..6ffefea991a 100644 --- a/pkg/api/datasources.go +++ b/pkg/api/datasources.go @@ -103,6 +103,9 @@ func DeleteDataSourceByName(c *m.ReqContext) Response { getCmd := &m.GetDataSourceByNameQuery{Name: name, OrgId: c.OrgId} if err := bus.Dispatch(getCmd); err != nil { + if err == m.ErrDataSourceNotFound { + return Error(404, "Data source not found", nil) + } return Error(500, "Failed to delete datasource", err) } diff --git a/pkg/api/datasources_test.go b/pkg/api/datasources_test.go index 490393727d6..6e52a27758b 100644 --- a/pkg/api/datasources_test.go +++ b/pkg/api/datasources_test.go @@ -46,5 +46,13 @@ func TestDataSourcesProxy(t *testing.T) { So(respJSON[3]["name"], ShouldEqual, "ZZZ") }) }) + + Convey("Should be able to save a data source", func() { + loggedInUserScenario("When calling DELETE on non-existing", "/api/datasources/name/12345", func(sc *scenarioContext) { + sc.handlerFunc = DeleteDataSourceByName + sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec() + So(sc.resp.Code, ShouldEqual, 404) + }) + }) }) } From e0ac31a28ac6a20997f4af332333b2af415e1886 Mon Sep 17 00:00:00 2001 From: bergquist Date: Mon, 25 Jun 2018 08:02:51 +0200 Subject: [PATCH 04/14] changelog: adds note about closing #12313 --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a00936090a4..178a8045028 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ * **Dataproxy**: Pass configured/auth headers to a Datasource [#10971](https://github.com/grafana/grafana/issues/10971), thx [@mrsiano](https://github.com/mrsiano) * **Cleanup**: Make temp file time to live configurable [#11607](https://github.com/grafana/grafana/issues/11607), thx [@xapon](https://github.com/xapon) +### Minor + +* **Api**: Delete nonexistent datasource should return 404 [#12313](https://github.com/grafana/grafana/issues/12313), thx [@AustinWinstanley](https://github.com/AustinWinstanley) + # 5.2.0 (unreleased) # 5.2.0-beta3 (2018-06-21) From 7797a66b584864c9288ce4e322eeab53bf2b6343 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 25 Jun 2018 13:17:33 +0200 Subject: [PATCH 05/14] changelog: add notes about closing #12383 [skip ci] --- CHANGELOG.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 178a8045028..923f76c5712 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,12 +3,16 @@ * **Dataproxy**: Pass configured/auth headers to a Datasource [#10971](https://github.com/grafana/grafana/issues/10971), thx [@mrsiano](https://github.com/mrsiano) * **Cleanup**: Make temp file time to live configurable [#11607](https://github.com/grafana/grafana/issues/11607), thx [@xapon](https://github.com/xapon) -### Minor +### Minor * **Api**: Delete nonexistent datasource should return 404 [#12313](https://github.com/grafana/grafana/issues/12313), thx [@AustinWinstanley](https://github.com/AustinWinstanley) # 5.2.0 (unreleased) +### Minor + +* **Plugins**: Handle errors correctly when loading datasource plugin [#12383](https://github.com/grafana/grafana/pull/12383) thx [@rozetko](https://github.com/rozetko) + # 5.2.0-beta3 (2018-06-21) ### Minor From 3bf12e4ff5cb3d6eead99a4528f0affae51e7810 Mon Sep 17 00:00:00 2001 From: PedroD Date: Mon, 25 Jun 2018 12:45:47 +0100 Subject: [PATCH 06/14] Fixing wrong /public path, relative to the webpack.dev script, that would avoid webpack from cleaning previous builds. (#12351) --- scripts/webpack/webpack.dev.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/webpack/webpack.dev.js b/scripts/webpack/webpack.dev.js index 7e43e8179ac..22df4fa3adf 100644 --- a/scripts/webpack/webpack.dev.js +++ b/scripts/webpack/webpack.dev.js @@ -65,7 +65,7 @@ module.exports = merge(common, { }, plugins: [ - new CleanWebpackPlugin('../public/build', { allowExternal: true }), + new CleanWebpackPlugin('../../public/build', { allowExternal: true }), extractSass, new HtmlWebpackPlugin({ filename: path.resolve(__dirname, '../../public/views/index.html'), From 762ee203fa99986ec9b0180a3342cbd4a33d8156 Mon Sep 17 00:00:00 2001 From: Tobias Skarhed Date: Mon, 25 Jun 2018 13:46:33 +0200 Subject: [PATCH 07/14] Fix 12248 --- public/app/core/components/search/search_results.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/public/app/core/components/search/search_results.ts b/public/app/core/components/search/search_results.ts index 273af224660..35ee1365e22 100644 --- a/public/app/core/components/search/search_results.ts +++ b/public/app/core/components/search/search_results.ts @@ -63,7 +63,8 @@ export class SearchResultsCtrl { } onItemClick(item) { - if (this.$location.path().indexOf(item.url) > -1) { + //Check if one string can be found in the other + if (this.$location.path().indexOf(item.url) > -1 || item.url.indexOf(this.$location.path()) > -1) { appEvents.emit('hide-dash-search'); } } From eb4d860fcbfcf88a5880b8babe18bb1c27f00669 Mon Sep 17 00:00:00 2001 From: Augustin Date: Mon, 25 Jun 2018 13:58:49 +0200 Subject: [PATCH 08/14] Light improve of massive delete annotation api (#12390) * fix delete annotations * fix self assignment * add right unit test using admin role --- pkg/api/annotations.go | 4 ++-- pkg/api/annotations_test.go | 47 +++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index 52eeb57dbb9..fe4658f3faf 100644 --- a/pkg/api/annotations.go +++ b/pkg/api/annotations.go @@ -37,7 +37,6 @@ func GetAnnotations(c *m.ReqContext) Response { if item.Email != "" { item.AvatarUrl = dtos.GetGravatarUrl(item.Email) } - item.Time = item.Time } return JSON(200, items) @@ -214,7 +213,8 @@ func DeleteAnnotations(c *m.ReqContext, cmd dtos.DeleteAnnotationsCmd) Response repo := annotations.GetRepository() err := repo.Delete(&annotations.DeleteParams{ - AlertId: cmd.PanelId, + Id: cmd.AnnotationId, + RegionId: cmd.RegionId, DashboardId: cmd.DashboardId, PanelId: cmd.PanelId, }) diff --git a/pkg/api/annotations_test.go b/pkg/api/annotations_test.go index 9fe96245b9b..e5f63ce022b 100644 --- a/pkg/api/annotations_test.go +++ b/pkg/api/annotations_test.go @@ -100,6 +100,11 @@ func TestAnnotationsApiEndpoint(t *testing.T) { Id: 1, } + deleteCmd := dtos.DeleteAnnotationsCmd{ + DashboardId: 1, + PanelId: 1, + } + viewerRole := m.ROLE_VIEWER editorRole := m.ROLE_EDITOR @@ -171,6 +176,25 @@ func TestAnnotationsApiEndpoint(t *testing.T) { }) }) }) + + Convey("When user is an Admin", func() { + role := m.ROLE_ADMIN + Convey("Should be able to do anything", func() { + postAnnotationScenario("When calling POST on", "/api/annotations", "/api/annotations", role, cmd, func(sc *scenarioContext) { + sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() + So(sc.resp.Code, ShouldEqual, 200) + }) + + putAnnotationScenario("When calling PUT on", "/api/annotations/1", "/api/annotations/:annotationId", role, updateCmd, func(sc *scenarioContext) { + sc.fakeReqWithParams("PUT", sc.url, map[string]string{}).exec() + So(sc.resp.Code, ShouldEqual, 200) + }) + deleteAnnotationsScenario("When calling POST on", "/api/annotations/mass-delete", "/api/annotations/mass-delete", role, deleteCmd, func(sc *scenarioContext) { + sc.fakeReqWithParams("POST", sc.url, map[string]string{}).exec() + So(sc.resp.Code, ShouldEqual, 200) + }) + }) + }) }) } @@ -239,3 +263,26 @@ func putAnnotationScenario(desc string, url string, routePattern string, role m. fn(sc) }) } + +func deleteAnnotationsScenario(desc string, url string, routePattern string, role m.RoleType, cmd dtos.DeleteAnnotationsCmd, fn scenarioFunc) { + Convey(desc+" "+url, func() { + defer bus.ClearBusHandlers() + + sc := setupScenarioContext(url) + sc.defaultHandler = wrap(func(c *m.ReqContext) Response { + sc.context = c + sc.context.UserId = TestUserID + sc.context.OrgId = TestOrgID + sc.context.OrgRole = role + + return DeleteAnnotations(c, cmd) + }) + + fakeAnnoRepo = &fakeAnnotationsRepo{} + annotations.SetRepository(fakeAnnoRepo) + + sc.m.Post(routePattern, sc.defaultHandler) + + fn(sc) + }) +} From ed1fec8e0ff62699c179db0795376e6c81ef5f87 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 25 Jun 2018 15:24:12 +0200 Subject: [PATCH 09/14] changelog: add notes about closing #12248 [skip ci] --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 923f76c5712..a49ba58ef89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ ### Minor * **Api**: Delete nonexistent datasource should return 404 [#12313](https://github.com/grafana/grafana/issues/12313), thx [@AustinWinstanley](https://github.com/AustinWinstanley) +* **Dashboard**: Fix selecting current dashboard from search should not reload dashboard [#12248](https://github.com/grafana/grafana/issues/12248) # 5.2.0 (unreleased) From 132df4553c491cc5b9801a812f8c6c3acd62f684 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Mon, 25 Jun 2018 16:02:34 +0200 Subject: [PATCH 10/14] fix: annnotation api & sql issue --- pkg/api/annotations.go | 5 ++++- pkg/services/annotations/annotations.go | 11 ++++++----- pkg/services/sqlstore/annotation.go | 19 ++++++++++--------- pkg/services/sqlstore/annotation_test.go | 2 +- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index fe4658f3faf..a44108537cb 100644 --- a/pkg/api/annotations.go +++ b/pkg/api/annotations.go @@ -213,6 +213,7 @@ func DeleteAnnotations(c *m.ReqContext, cmd dtos.DeleteAnnotationsCmd) Response repo := annotations.GetRepository() err := repo.Delete(&annotations.DeleteParams{ + OrgId: c.OrgId, Id: cmd.AnnotationId, RegionId: cmd.RegionId, DashboardId: cmd.DashboardId, @@ -235,7 +236,8 @@ func DeleteAnnotationByID(c *m.ReqContext) Response { } err := repo.Delete(&annotations.DeleteParams{ - Id: annotationID, + OrgId: c.OrgId, + Id: annotationID, }) if err != nil { @@ -254,6 +256,7 @@ func DeleteAnnotationRegion(c *m.ReqContext) Response { } err := repo.Delete(&annotations.DeleteParams{ + OrgId: c.OrgId, RegionId: regionID, }) diff --git a/pkg/services/annotations/annotations.go b/pkg/services/annotations/annotations.go index 5cebb3d2df9..9b490169d3b 100644 --- a/pkg/services/annotations/annotations.go +++ b/pkg/services/annotations/annotations.go @@ -35,11 +35,12 @@ type PostParams struct { } type DeleteParams struct { - Id int64 `json:"id"` - AlertId int64 `json:"alertId"` - DashboardId int64 `json:"dashboardId"` - PanelId int64 `json:"panelId"` - RegionId int64 `json:"regionId"` + OrgId int64 + Id int64 + AlertId int64 + DashboardId int64 + PanelId int64 + RegionId int64 } var repositoryInstance Repository diff --git a/pkg/services/sqlstore/annotation.go b/pkg/services/sqlstore/annotation.go index 52da7a99516..a65bc136554 100644 --- a/pkg/services/sqlstore/annotation.go +++ b/pkg/services/sqlstore/annotation.go @@ -238,18 +238,19 @@ func (r *SqlAnnotationRepo) Delete(params *annotations.DeleteParams) error { queryParams []interface{} ) + sqlog.Info("delete", "orgId", params.OrgId) if params.RegionId != 0 { - annoTagSql = "DELETE FROM annotation_tag WHERE annotation_id IN (SELECT id FROM annotation WHERE region_id = ?)" - sql = "DELETE FROM annotation WHERE region_id = ?" - queryParams = []interface{}{params.RegionId} + annoTagSql = "DELETE FROM annotation_tag WHERE annotation_id IN (SELECT id FROM annotation WHERE region_id = ? AND org_id = ?)" + sql = "DELETE FROM annotation WHERE region_id = ? AND org_id = ?" + queryParams = []interface{}{params.RegionId, params.OrgId} } else if params.Id != 0 { - annoTagSql = "DELETE FROM annotation_tag WHERE annotation_id IN (SELECT id FROM annotation WHERE id = ?)" - sql = "DELETE FROM annotation WHERE id = ?" - queryParams = []interface{}{params.Id} + annoTagSql = "DELETE FROM annotation_tag WHERE annotation_id IN (SELECT id FROM annotation WHERE id = ? AND org_id = ?)" + sql = "DELETE FROM annotation WHERE id = ? AND org_id = ?" + queryParams = []interface{}{params.Id, params.OrgId} } else { - annoTagSql = "DELETE FROM annotation_tag WHERE annotation_id IN (SELECT id FROM annotation WHERE dashboard_id = ? AND panel_id = ?)" - sql = "DELETE FROM annotation WHERE dashboard_id = ? AND panel_id = ?" - queryParams = []interface{}{params.DashboardId, params.PanelId} + annoTagSql = "DELETE FROM annotation_tag WHERE annotation_id IN (SELECT id FROM annotation WHERE dashboard_id = ? AND panel_id = ? AND org_id = ?)" + sql = "DELETE FROM annotation WHERE dashboard_id = ? AND panel_id = ? AND org_id = ?" + queryParams = []interface{}{params.DashboardId, params.PanelId, params.OrgId} } if _, err := sess.Exec(annoTagSql, queryParams...); err != nil { diff --git a/pkg/services/sqlstore/annotation_test.go b/pkg/services/sqlstore/annotation_test.go index 01a95c7db7b..c0d267f2578 100644 --- a/pkg/services/sqlstore/annotation_test.go +++ b/pkg/services/sqlstore/annotation_test.go @@ -268,7 +268,7 @@ func TestAnnotations(t *testing.T) { annotationId := items[0].Id - err = repo.Delete(&annotations.DeleteParams{Id: annotationId}) + err = repo.Delete(&annotations.DeleteParams{Id: annotationId, OrgId: 1}) So(err, ShouldBeNil) items, err = repo.Find(query) From f106de0efd7bae8d5f5f90fc51e68460961a5969 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Thu, 7 Jun 2018 15:15:13 +0200 Subject: [PATCH 11/14] enhance error message if phantomjs executable is not found if arm build, explain that phantomjs is not included by default in arm builds. If not explain that phantom js isn't installed correctly --- pkg/api/render.go | 11 +++++++++++ pkg/services/rendering/interface.go | 1 + pkg/services/rendering/phantomjs.go | 5 +++++ 3 files changed, 17 insertions(+) diff --git a/pkg/api/render.go b/pkg/api/render.go index f6f149980d6..b8ef6cc5cb6 100644 --- a/pkg/api/render.go +++ b/pkg/api/render.go @@ -3,7 +3,9 @@ package api import ( "fmt" "net/http" + "runtime" "strconv" + "strings" "time" m "github.com/grafana/grafana/pkg/models" @@ -55,6 +57,15 @@ func (hs *HTTPServer) RenderToPng(c *m.ReqContext) { return } + if err != nil && err == rendering.ErrPhantomJSNotInstalled { + if strings.HasPrefix(runtime.GOARCH, "arm") { + c.Handle(500, "Rendering failed - PhantomJS isn't included in arm build per default", err) + } else { + c.Handle(500, "Rendering failed - PhantomJS isn't installed correctly", err) + } + return + } + if err != nil { c.Handle(500, "Rendering failed.", err) return diff --git a/pkg/services/rendering/interface.go b/pkg/services/rendering/interface.go index 9498071f264..85c139cfc04 100644 --- a/pkg/services/rendering/interface.go +++ b/pkg/services/rendering/interface.go @@ -10,6 +10,7 @@ import ( var ErrTimeout = errors.New("Timeout error. You can set timeout in seconds with &timeout url parameter") var ErrNoRenderer = errors.New("No renderer plugin found nor is an external render server configured") +var ErrPhantomJSNotInstalled = errors.New("PhantomJS executable not found") type Opts struct { Width int diff --git a/pkg/services/rendering/phantomjs.go b/pkg/services/rendering/phantomjs.go index d4ceac0ed43..8e06b5fed9d 100644 --- a/pkg/services/rendering/phantomjs.go +++ b/pkg/services/rendering/phantomjs.go @@ -24,6 +24,11 @@ func (rs *RenderingService) renderViaPhantomJS(ctx context.Context, opts Opts) ( url := rs.getURL(opts.Path) binPath, _ := filepath.Abs(filepath.Join(rs.Cfg.PhantomDir, executable)) + if _, err := os.Stat(binPath); os.IsNotExist(err) { + rs.log.Error("executable not found", "executable", binPath) + return nil, ErrPhantomJSNotInstalled + } + scriptPath, _ := filepath.Abs(filepath.Join(rs.Cfg.PhantomDir, "render.js")) pngPath := rs.getFilePathForNewImage() From c23f9a1f377b66b983bbd6921837b2ac47e02f35 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 25 Jun 2018 16:32:32 +0200 Subject: [PATCH 12/14] changelog: add notes about closing #11868 [skip ci] --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a49ba58ef89..19daed035c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ ### Minor * **Plugins**: Handle errors correctly when loading datasource plugin [#12383](https://github.com/grafana/grafana/pull/12383) thx [@rozetko](https://github.com/rozetko) +* **Render**: Enhance error message if phantomjs executable is not found [#11868](https://github.com/grafana/grafana/issues/11868) # 5.2.0-beta3 (2018-06-21) From dbfafa1cb5d4740e30079d7c22e1a7c351fb6c03 Mon Sep 17 00:00:00 2001 From: bergquist Date: Mon, 25 Jun 2018 16:36:47 +0200 Subject: [PATCH 13/14] routing: allows routes to be added to existing groups this enables services to add routing to ex /api without causing conflicts. --- pkg/api/api.go | 53 ++++++++--------- pkg/api/http_server.go | 9 +-- pkg/api/{ => routing}/route_register.go | 26 ++++++++- pkg/api/{ => routing}/route_register_test.go | 60 +++++++++++++++++++- pkg/cmd/grafana-server/server.go | 7 ++- 5 files changed, 117 insertions(+), 38 deletions(-) rename pkg/api/{ => routing}/route_register.go (87%) rename pkg/api/{ => routing}/route_register_test.go (76%) diff --git a/pkg/api/api.go b/pkg/api/api.go index c205e7d3e2f..39dae56eb69 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -4,6 +4,7 @@ import ( "github.com/go-macaron/binding" "github.com/grafana/grafana/pkg/api/avatar" "github.com/grafana/grafana/pkg/api/dtos" + "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/middleware" m "github.com/grafana/grafana/pkg/models" ) @@ -117,10 +118,10 @@ func (hs *HTTPServer) registerRoutes() { r.Get("/api/login/ping", quota("session"), LoginAPIPing) // authed api - r.Group("/api", func(apiRoute RouteRegister) { + r.Group("/api", func(apiRoute routing.RouteRegister) { // user (signed in) - apiRoute.Group("/user", func(userRoute RouteRegister) { + apiRoute.Group("/user", func(userRoute routing.RouteRegister) { userRoute.Get("/", wrap(GetSignedInUser)) userRoute.Put("/", bind(m.UpdateUserCommand{}), wrap(UpdateSignedInUser)) userRoute.Post("/using/:id", wrap(UserSetUsingOrg)) @@ -140,7 +141,7 @@ func (hs *HTTPServer) registerRoutes() { }) // users (admin permission required) - apiRoute.Group("/users", func(usersRoute RouteRegister) { + apiRoute.Group("/users", func(usersRoute routing.RouteRegister) { usersRoute.Get("/", wrap(SearchUsers)) usersRoute.Get("/search", wrap(SearchUsersWithPaging)) usersRoute.Get("/:id", wrap(GetUserByID)) @@ -152,7 +153,7 @@ func (hs *HTTPServer) registerRoutes() { }, reqGrafanaAdmin) // team (admin permission required) - apiRoute.Group("/teams", func(teamsRoute RouteRegister) { + apiRoute.Group("/teams", func(teamsRoute routing.RouteRegister) { teamsRoute.Post("/", bind(m.CreateTeamCommand{}), wrap(CreateTeam)) teamsRoute.Put("/:teamId", bind(m.UpdateTeamCommand{}), wrap(UpdateTeam)) teamsRoute.Delete("/:teamId", wrap(DeleteTeamByID)) @@ -162,19 +163,19 @@ func (hs *HTTPServer) registerRoutes() { }, reqOrgAdmin) // team without requirement of user to be org admin - apiRoute.Group("/teams", func(teamsRoute RouteRegister) { + apiRoute.Group("/teams", func(teamsRoute routing.RouteRegister) { teamsRoute.Get("/:teamId", wrap(GetTeamByID)) teamsRoute.Get("/search", wrap(SearchTeams)) }) // org information available to all users. - apiRoute.Group("/org", func(orgRoute RouteRegister) { + apiRoute.Group("/org", func(orgRoute routing.RouteRegister) { orgRoute.Get("/", wrap(GetOrgCurrent)) orgRoute.Get("/quotas", wrap(GetOrgQuotas)) }) // current org - apiRoute.Group("/org", func(orgRoute RouteRegister) { + apiRoute.Group("/org", func(orgRoute routing.RouteRegister) { orgRoute.Put("/", bind(dtos.UpdateOrgForm{}), wrap(UpdateOrgCurrent)) orgRoute.Put("/address", bind(dtos.UpdateOrgAddressForm{}), wrap(UpdateOrgAddressCurrent)) orgRoute.Post("/users", quota("user"), bind(m.AddOrgUserCommand{}), wrap(AddOrgUserToCurrentOrg)) @@ -192,7 +193,7 @@ func (hs *HTTPServer) registerRoutes() { }, reqOrgAdmin) // current org without requirement of user to be org admin - apiRoute.Group("/org", func(orgRoute RouteRegister) { + apiRoute.Group("/org", func(orgRoute routing.RouteRegister) { orgRoute.Get("/users", wrap(GetOrgUsersForCurrentOrg)) }) @@ -203,7 +204,7 @@ func (hs *HTTPServer) registerRoutes() { apiRoute.Get("/orgs", reqGrafanaAdmin, wrap(SearchOrgs)) // orgs (admin routes) - apiRoute.Group("/orgs/:orgId", func(orgsRoute RouteRegister) { + apiRoute.Group("/orgs/:orgId", func(orgsRoute routing.RouteRegister) { orgsRoute.Get("/", wrap(GetOrgByID)) orgsRoute.Put("/", bind(dtos.UpdateOrgForm{}), wrap(UpdateOrg)) orgsRoute.Put("/address", bind(dtos.UpdateOrgAddressForm{}), wrap(UpdateOrgAddress)) @@ -217,24 +218,24 @@ func (hs *HTTPServer) registerRoutes() { }, reqGrafanaAdmin) // orgs (admin routes) - apiRoute.Group("/orgs/name/:name", func(orgsRoute RouteRegister) { + apiRoute.Group("/orgs/name/:name", func(orgsRoute routing.RouteRegister) { orgsRoute.Get("/", wrap(GetOrgByName)) }, reqGrafanaAdmin) // auth api keys - apiRoute.Group("/auth/keys", func(keysRoute RouteRegister) { + apiRoute.Group("/auth/keys", func(keysRoute routing.RouteRegister) { keysRoute.Get("/", wrap(GetAPIKeys)) keysRoute.Post("/", quota("api_key"), bind(m.AddApiKeyCommand{}), wrap(AddAPIKey)) keysRoute.Delete("/:id", wrap(DeleteAPIKey)) }, reqOrgAdmin) // Preferences - apiRoute.Group("/preferences", func(prefRoute RouteRegister) { + apiRoute.Group("/preferences", func(prefRoute routing.RouteRegister) { prefRoute.Post("/set-home-dash", bind(m.SavePreferencesCommand{}), wrap(SetHomeDashboard)) }) // Data sources - apiRoute.Group("/datasources", func(datasourceRoute RouteRegister) { + apiRoute.Group("/datasources", func(datasourceRoute routing.RouteRegister) { datasourceRoute.Get("/", wrap(GetDataSources)) datasourceRoute.Post("/", quota("data_source"), bind(m.AddDataSourceCommand{}), wrap(AddDataSource)) datasourceRoute.Put("/:id", bind(m.UpdateDataSourceCommand{}), wrap(UpdateDataSource)) @@ -250,7 +251,7 @@ func (hs *HTTPServer) registerRoutes() { apiRoute.Get("/plugins/:pluginId/settings", wrap(GetPluginSettingByID)) apiRoute.Get("/plugins/:pluginId/markdown/:name", wrap(GetPluginMarkdown)) - apiRoute.Group("/plugins", func(pluginRoute RouteRegister) { + apiRoute.Group("/plugins", func(pluginRoute routing.RouteRegister) { pluginRoute.Get("/:pluginId/dashboards/", wrap(GetPluginDashboards)) pluginRoute.Post("/:pluginId/settings", bind(m.UpdatePluginSettingCmd{}), wrap(UpdatePluginSetting)) }, reqOrgAdmin) @@ -260,17 +261,17 @@ func (hs *HTTPServer) registerRoutes() { apiRoute.Any("/datasources/proxy/:id", reqSignedIn, hs.ProxyDataSourceRequest) // Folders - apiRoute.Group("/folders", func(folderRoute RouteRegister) { + apiRoute.Group("/folders", func(folderRoute routing.RouteRegister) { folderRoute.Get("/", wrap(GetFolders)) folderRoute.Get("/id/:id", wrap(GetFolderByID)) folderRoute.Post("/", bind(m.CreateFolderCommand{}), wrap(CreateFolder)) - folderRoute.Group("/:uid", func(folderUidRoute RouteRegister) { + folderRoute.Group("/:uid", func(folderUidRoute routing.RouteRegister) { folderUidRoute.Get("/", wrap(GetFolderByUID)) folderUidRoute.Put("/", bind(m.UpdateFolderCommand{}), wrap(UpdateFolder)) folderUidRoute.Delete("/", wrap(DeleteFolder)) - folderUidRoute.Group("/permissions", func(folderPermissionRoute RouteRegister) { + folderUidRoute.Group("/permissions", func(folderPermissionRoute routing.RouteRegister) { folderPermissionRoute.Get("/", wrap(GetFolderPermissionList)) folderPermissionRoute.Post("/", bind(dtos.UpdateDashboardAclCommand{}), wrap(UpdateFolderPermissions)) }) @@ -278,7 +279,7 @@ func (hs *HTTPServer) registerRoutes() { }) // Dashboard - apiRoute.Group("/dashboards", func(dashboardRoute RouteRegister) { + apiRoute.Group("/dashboards", func(dashboardRoute routing.RouteRegister) { dashboardRoute.Get("/uid/:uid", wrap(GetDashboard)) dashboardRoute.Delete("/uid/:uid", wrap(DeleteDashboardByUID)) @@ -292,12 +293,12 @@ func (hs *HTTPServer) registerRoutes() { dashboardRoute.Get("/tags", GetDashboardTags) dashboardRoute.Post("/import", bind(dtos.ImportDashboardCommand{}), wrap(ImportDashboard)) - dashboardRoute.Group("/id/:dashboardId", func(dashIdRoute RouteRegister) { + dashboardRoute.Group("/id/:dashboardId", func(dashIdRoute routing.RouteRegister) { dashIdRoute.Get("/versions", wrap(GetDashboardVersions)) dashIdRoute.Get("/versions/:id", wrap(GetDashboardVersion)) dashIdRoute.Post("/restore", bind(dtos.RestoreDashboardVersionCommand{}), wrap(RestoreDashboardVersion)) - dashIdRoute.Group("/permissions", func(dashboardPermissionRoute RouteRegister) { + dashIdRoute.Group("/permissions", func(dashboardPermissionRoute routing.RouteRegister) { dashboardPermissionRoute.Get("/", wrap(GetDashboardPermissionList)) dashboardPermissionRoute.Post("/", bind(dtos.UpdateDashboardAclCommand{}), wrap(UpdateDashboardPermissions)) }) @@ -305,12 +306,12 @@ func (hs *HTTPServer) registerRoutes() { }) // Dashboard snapshots - apiRoute.Group("/dashboard/snapshots", func(dashboardRoute RouteRegister) { + apiRoute.Group("/dashboard/snapshots", func(dashboardRoute routing.RouteRegister) { dashboardRoute.Get("/", wrap(SearchDashboardSnapshots)) }) // Playlist - apiRoute.Group("/playlists", func(playlistRoute RouteRegister) { + apiRoute.Group("/playlists", func(playlistRoute routing.RouteRegister) { playlistRoute.Get("/", wrap(SearchPlaylists)) playlistRoute.Get("/:id", ValidateOrgPlaylist, wrap(GetPlaylist)) playlistRoute.Get("/:id/items", ValidateOrgPlaylist, wrap(GetPlaylistItems)) @@ -329,7 +330,7 @@ func (hs *HTTPServer) registerRoutes() { apiRoute.Get("/tsdb/testdata/gensql", reqGrafanaAdmin, wrap(GenerateSQLTestData)) apiRoute.Get("/tsdb/testdata/random-walk", wrap(GetTestDataRandomWalk)) - apiRoute.Group("/alerts", func(alertsRoute RouteRegister) { + apiRoute.Group("/alerts", func(alertsRoute routing.RouteRegister) { alertsRoute.Post("/test", bind(dtos.AlertTestCommand{}), wrap(AlertTest)) alertsRoute.Post("/:alertId/pause", reqEditorRole, bind(dtos.PauseAlertCommand{}), wrap(PauseAlert)) alertsRoute.Get("/:alertId", ValidateOrgAlert, wrap(GetAlert)) @@ -340,7 +341,7 @@ func (hs *HTTPServer) registerRoutes() { apiRoute.Get("/alert-notifications", wrap(GetAlertNotifications)) apiRoute.Get("/alert-notifiers", wrap(GetAlertNotifiers)) - apiRoute.Group("/alert-notifications", func(alertNotifications RouteRegister) { + apiRoute.Group("/alert-notifications", func(alertNotifications routing.RouteRegister) { alertNotifications.Post("/test", bind(dtos.NotificationTestCommand{}), wrap(NotificationTest)) alertNotifications.Post("/", bind(m.CreateAlertNotificationCommand{}), wrap(CreateAlertNotification)) alertNotifications.Put("/:notificationId", bind(m.UpdateAlertNotificationCommand{}), wrap(UpdateAlertNotification)) @@ -351,7 +352,7 @@ func (hs *HTTPServer) registerRoutes() { apiRoute.Get("/annotations", wrap(GetAnnotations)) apiRoute.Post("/annotations/mass-delete", reqOrgAdmin, bind(dtos.DeleteAnnotationsCmd{}), wrap(DeleteAnnotations)) - apiRoute.Group("/annotations", func(annotationsRoute RouteRegister) { + apiRoute.Group("/annotations", func(annotationsRoute routing.RouteRegister) { annotationsRoute.Post("/", bind(dtos.PostAnnotationsCmd{}), wrap(PostAnnotation)) annotationsRoute.Delete("/:annotationId", wrap(DeleteAnnotationByID)) annotationsRoute.Put("/:annotationId", bind(dtos.UpdateAnnotationsCmd{}), wrap(UpdateAnnotation)) @@ -365,7 +366,7 @@ func (hs *HTTPServer) registerRoutes() { }, reqSignedIn) // admin api - r.Group("/api/admin", func(adminRoute RouteRegister) { + r.Group("/api/admin", func(adminRoute routing.RouteRegister) { adminRoute.Get("/settings", AdminGetSettings) adminRoute.Post("/users", bind(dtos.AdminCreateUserForm{}), AdminCreateUser) adminRoute.Put("/users/:id/password", bind(dtos.AdminUpdateUserPasswordForm{}), AdminUpdateUserPassword) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 371c500a73e..e1a10fb468f 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -11,6 +11,7 @@ import ( "path" "time" + "github.com/grafana/grafana/pkg/api/routing" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" @@ -43,10 +44,10 @@ type HTTPServer struct { cache *gocache.Cache httpSrv *http.Server - RouteRegister RouteRegister `inject:""` - Bus bus.Bus `inject:""` - RenderService rendering.Service `inject:""` - Cfg *setting.Cfg `inject:""` + RouteRegister routing.RouteRegister `inject:""` + Bus bus.Bus `inject:""` + RenderService rendering.Service `inject:""` + Cfg *setting.Cfg `inject:""` } func (hs *HTTPServer) Init() error { diff --git a/pkg/api/route_register.go b/pkg/api/routing/route_register.go similarity index 87% rename from pkg/api/route_register.go rename to pkg/api/routing/route_register.go index 5b62e8585b7..e2ef316dd38 100644 --- a/pkg/api/route_register.go +++ b/pkg/api/routing/route_register.go @@ -1,9 +1,10 @@ -package api +package routing import ( "net/http" + "strings" - macaron "gopkg.in/macaron.v1" + "gopkg.in/macaron.v1" ) type Router interface { @@ -36,6 +37,9 @@ type RouteRegister interface { // with a shared prefix route. Group(string, func(RouteRegister), ...macaron.Handler) + // Insert adds more routes to an existing Group. + Insert(string, func(RouteRegister), ...macaron.Handler) + // Register iterates over all routes added to the RouteRegister // and add them to the `Router` pass as an parameter. Register(Router) *macaron.Router @@ -67,6 +71,24 @@ type routeRegister struct { groups []*routeRegister } +func (rr *routeRegister) Insert(pattern string, fn func(RouteRegister), handlers ...macaron.Handler) { + + //loop over all groups at current level + for _, g := range rr.groups { + + // apply routes if the prefix matches the pattern + if g.prefix == pattern { + g.Group("", fn) + break + } + + // go down one level if the prefix can be find in the pattern + if strings.HasPrefix(pattern, g.prefix) { + g.Insert(pattern, fn) + } + } +} + func (rr *routeRegister) Group(pattern string, fn func(rr RouteRegister), handlers ...macaron.Handler) { group := &routeRegister{ prefix: rr.prefix + pattern, diff --git a/pkg/api/route_register_test.go b/pkg/api/routing/route_register_test.go similarity index 76% rename from pkg/api/route_register_test.go rename to pkg/api/routing/route_register_test.go index 3b5d79599a8..35711f8a68a 100644 --- a/pkg/api/route_register_test.go +++ b/pkg/api/routing/route_register_test.go @@ -1,11 +1,11 @@ -package api +package routing import ( "net/http" "strconv" "testing" - macaron "gopkg.in/macaron.v1" + "gopkg.in/macaron.v1" ) type fakeRouter struct { @@ -33,7 +33,7 @@ func (fr *fakeRouter) Get(pattern string, handlers ...macaron.Handler) *macaron. } func emptyHandlers(n int) []macaron.Handler { - res := []macaron.Handler{} + var res []macaron.Handler for i := 1; n >= i; i++ { res = append(res, emptyHandler(strconv.Itoa(i))) } @@ -138,6 +138,60 @@ func TestRouteGroupedRegister(t *testing.T) { } } } +func TestRouteGroupInserting(t *testing.T) { + testTable := []route{ + {method: http.MethodGet, pattern: "/api/", handlers: emptyHandlers(1)}, + {method: http.MethodPost, pattern: "/api/group/endpoint", handlers: emptyHandlers(1)}, + + {method: http.MethodGet, pattern: "/api/group/inserted", handlers: emptyHandlers(1)}, + {method: http.MethodDelete, pattern: "/api/inserted-endpoint", handlers: emptyHandlers(1)}, + } + + // Setup + rr := NewRouteRegister() + + rr.Group("/api", func(api RouteRegister) { + api.Get("/", emptyHandler("1")) + + api.Group("/group", func(group RouteRegister) { + group.Post("/endpoint", emptyHandler("1")) + }) + }) + + rr.Insert("/api", func(api RouteRegister) { + api.Delete("/inserted-endpoint", emptyHandler("1")) + }) + + rr.Insert("/api/group", func(group RouteRegister) { + group.Get("/inserted", emptyHandler("1")) + }) + + fr := &fakeRouter{} + rr.Register(fr) + + // Validation + if len(fr.route) != len(testTable) { + t.Fatalf("want %v routes, got %v", len(testTable), len(fr.route)) + } + + for i := range testTable { + if testTable[i].method != fr.route[i].method { + t.Errorf("want %s got %v", testTable[i].method, fr.route[i].method) + } + + if testTable[i].pattern != fr.route[i].pattern { + t.Errorf("want %s got %v", testTable[i].pattern, fr.route[i].pattern) + } + + if len(testTable[i].handlers) != len(fr.route[i].handlers) { + t.Errorf("want %d handlers got %d handlers \ntestcase: %v\nroute: %v\n", + len(testTable[i].handlers), + len(fr.route[i].handlers), + testTable[i], + fr.route[i]) + } + } +} func TestNamedMiddlewareRouteRegister(t *testing.T) { testTable := []route{ diff --git a/pkg/cmd/grafana-server/server.go b/pkg/cmd/grafana-server/server.go index 4abdb3fb442..6444528f7f0 100644 --- a/pkg/cmd/grafana-server/server.go +++ b/pkg/cmd/grafana-server/server.go @@ -12,6 +12,7 @@ import ( "time" "github.com/facebookgo/inject" + "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/registry" @@ -61,8 +62,8 @@ type GrafanaServerImpl struct { shutdownReason string shutdownInProgress bool - RouteRegister api.RouteRegister `inject:""` - HttpServer *api.HTTPServer `inject:""` + RouteRegister routing.RouteRegister `inject:""` + HttpServer *api.HTTPServer `inject:""` } func (g *GrafanaServerImpl) Run() error { @@ -75,7 +76,7 @@ func (g *GrafanaServerImpl) Run() error { serviceGraph := inject.Graph{} serviceGraph.Provide(&inject.Object{Value: bus.GetBus()}) serviceGraph.Provide(&inject.Object{Value: g.cfg}) - serviceGraph.Provide(&inject.Object{Value: api.NewRouteRegister(middleware.RequestMetrics, middleware.RequestTracing)}) + serviceGraph.Provide(&inject.Object{Value: routing.NewRouteRegister(middleware.RequestMetrics, middleware.RequestTracing)}) // self registered services services := registry.GetServices() From 4a46dd886b5a02d0a9422943682632f88325be68 Mon Sep 17 00:00:00 2001 From: bergquist Date: Mon, 25 Jun 2018 16:58:49 +0200 Subject: [PATCH 14/14] routing: raise panic if duplicate routes are added --- pkg/api/routing/route_register.go | 6 ++++++ pkg/api/routing/route_register_test.go | 17 +++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/pkg/api/routing/route_register.go b/pkg/api/routing/route_register.go index e2ef316dd38..47531732564 100644 --- a/pkg/api/routing/route_register.go +++ b/pkg/api/routing/route_register.go @@ -129,6 +129,12 @@ func (rr *routeRegister) route(pattern, method string, handlers ...macaron.Handl h = append(h, rr.subfixHandlers...) h = append(h, handlers...) + for _, r := range rr.routes { + if r.pattern == rr.prefix+pattern && r.method == method { + panic("cannot add duplicate route") + } + } + rr.routes = append(rr.routes, route{ method: method, pattern: rr.prefix + pattern, diff --git a/pkg/api/routing/route_register_test.go b/pkg/api/routing/route_register_test.go index 35711f8a68a..62e8989ff92 100644 --- a/pkg/api/routing/route_register_test.go +++ b/pkg/api/routing/route_register_test.go @@ -193,6 +193,23 @@ func TestRouteGroupInserting(t *testing.T) { } } +func TestDuplicateRoutShouldPanic(t *testing.T) { + defer func() { + if recover() != "cannot add duplicate route" { + t.Errorf("Should cause panic if duplicate routes are added ") + } + }() + + rr := NewRouteRegister(func(name string) macaron.Handler { + return emptyHandler(name) + }) + + rr.Get("/api", emptyHandler("1")) + rr.Get("/api", emptyHandler("1")) + + fr := &fakeRouter{} + rr.Register(fr) +} func TestNamedMiddlewareRouteRegister(t *testing.T) { testTable := []route{ {method: "DELETE", pattern: "/admin", handlers: emptyHandlers(2)},