From 5fec6cc4f5b7dd4aa37399e93a77e6111d867f95 Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Wed, 3 Aug 2022 17:31:23 +0300 Subject: [PATCH] API: Fix snapshot responses (#52998) * API: Fix response status when snapshots are not found * API: Fix response status when snapshot key is empty * Apply suggestions from code review --- pkg/api/dashboard_snapshot.go | 9 +- pkg/api/dashboard_snapshot_test.go | 101 ++++++++++++++++++ .../dashboardsnapshots/database/database.go | 2 +- pkg/services/dashboardsnapshots/errors.go | 9 +- public/api-merged.json | 3 + public/api-spec.json | 3 + 6 files changed, 117 insertions(+), 10 deletions(-) diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go index 04e1f35c32f..0af30588d26 100644 --- a/pkg/api/dashboard_snapshot.go +++ b/pkg/api/dashboard_snapshot.go @@ -175,19 +175,20 @@ func (hs *HTTPServer) CreateDashboardSnapshot(c *models.ReqContext) response.Res // // Responses: // 200: getDashboardSnapshotResponse +// 400: badRequestError // 404: notFoundError // 500: internalServerError func (hs *HTTPServer) GetDashboardSnapshot(c *models.ReqContext) response.Response { key := web.Params(c.Req)[":key"] if len(key) == 0 { - return response.Error(404, "Snapshot not found", nil) + return response.Error(http.StatusBadRequest, "Empty snapshot key", nil) } query := &dashboardsnapshots.GetDashboardSnapshotQuery{Key: key} err := hs.dashboardsnapshotsService.GetDashboardSnapshot(c.Req.Context(), query) if err != nil { - return response.Error(500, "Failed to get dashboard snapshot", err) + return response.Err(err) } snapshot := query.Result @@ -265,7 +266,7 @@ func (hs *HTTPServer) DeleteDashboardSnapshotByDeleteKey(c *models.ReqContext) r query := &dashboardsnapshots.GetDashboardSnapshotQuery{DeleteKey: key} err := hs.dashboardsnapshotsService.GetDashboardSnapshot(c.Req.Context(), query) if err != nil { - return response.Error(500, "Failed to get dashboard snapshot", err) + return response.Err(err) } if query.Result.External { @@ -306,7 +307,7 @@ func (hs *HTTPServer) DeleteDashboardSnapshot(c *models.ReqContext) response.Res err := hs.dashboardsnapshotsService.GetDashboardSnapshot(c.Req.Context(), query) if err != nil { - return response.Error(500, "Failed to get dashboard snapshot", err) + return response.Err(err) } if query.Result == nil { return response.Error(404, "Failed to get dashboard snapshot", nil) diff --git a/pkg/api/dashboard_snapshot_test.go b/pkg/api/dashboard_snapshot_test.go index 8578fa2ed31..10f09f9d409 100644 --- a/pkg/api/dashboard_snapshot_test.go +++ b/pkg/api/dashboard_snapshot_test.go @@ -1,6 +1,7 @@ package api import ( + "errors" "fmt" "net/http" "net/http/httptest" @@ -223,3 +224,103 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) { }, sqlmock) }) } + +func TestGetDashboardSnapshotNotFound(t *testing.T) { + sqlmock := mockstore.NewSQLStoreMock() + sqlmock.ExpectedTeamsByUser = []*models.TeamDTO{} + + setUpSnapshotTest := func(t *testing.T) dashboardsnapshots.Service { + t.Helper() + + dashSnapSvc := dashboardsnapshots.NewMockService(t) + dashSnapSvc. + On("GetDashboardSnapshot", mock.Anything, mock.AnythingOfType("*dashboardsnapshots.GetDashboardSnapshotQuery")). + Run(func(args mock.Arguments) {}). + Return(dashboardsnapshots.ErrBaseNotFound.Errorf("")) + + return dashSnapSvc + } + + loggedInUserScenarioWithRole(t, + "GET /snapshots/{key} should return 404 when the snapshot does not exist", "GET", + "/api/snapshots/12345", "/api/snapshots/:key", models.ROLE_EDITOR, func(sc *scenarioContext) { + d := setUpSnapshotTest(t) + hs := &HTTPServer{dashboardsnapshotsService: d} + sc.handlerFunc = hs.GetDashboardSnapshot + sc.fakeReqWithParams("GET", sc.url, map[string]string{"key": "12345"}).exec() + + assert.Equal(t, http.StatusNotFound, sc.resp.Code) + }, sqlmock) + + loggedInUserScenarioWithRole(t, + "DELETE /snapshots/{key} should return 404 when the snapshot does not exist", "DELETE", + "/api/snapshots/12345", "/api/snapshots/:key", models.ROLE_EDITOR, func(sc *scenarioContext) { + d := setUpSnapshotTest(t) + hs := &HTTPServer{dashboardsnapshotsService: d} + sc.handlerFunc = hs.DeleteDashboardSnapshot + sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() + + assert.Equal(t, http.StatusNotFound, sc.resp.Code) + }, sqlmock) + + loggedInUserScenarioWithRole(t, + "GET /snapshots-delete/{deleteKey} should return 404 when the snapshot does not exist", "DELETE", + "/api/snapshots-delete/12345", "/api/snapshots-delete/:deleteKey", models.ROLE_EDITOR, func(sc *scenarioContext) { + d := setUpSnapshotTest(t) + hs := &HTTPServer{dashboardsnapshotsService: d} + sc.handlerFunc = hs.DeleteDashboardSnapshotByDeleteKey + sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"deleteKey": "12345"}).exec() + + assert.Equal(t, http.StatusNotFound, sc.resp.Code) + }, sqlmock) +} + +func TestGetDashboardSnapshotFailure(t *testing.T) { + sqlmock := mockstore.NewSQLStoreMock() + sqlmock.ExpectedTeamsByUser = []*models.TeamDTO{} + + setUpSnapshotTest := func(t *testing.T) dashboardsnapshots.Service { + t.Helper() + + dashSnapSvc := dashboardsnapshots.NewMockService(t) + dashSnapSvc. + On("GetDashboardSnapshot", mock.Anything, mock.AnythingOfType("*dashboardsnapshots.GetDashboardSnapshotQuery")). + Run(func(args mock.Arguments) {}). + Return(errors.New("something went wrong")) + + return dashSnapSvc + } + + loggedInUserScenarioWithRole(t, + "GET /snapshots/{key} should return 404 when the snapshot does not exist", "GET", + "/api/snapshots/12345", "/api/snapshots/:key", models.ROLE_EDITOR, func(sc *scenarioContext) { + d := setUpSnapshotTest(t) + hs := &HTTPServer{dashboardsnapshotsService: d} + sc.handlerFunc = hs.GetDashboardSnapshot + sc.fakeReqWithParams("GET", sc.url, map[string]string{"key": "12345"}).exec() + + assert.Equal(t, http.StatusInternalServerError, sc.resp.Code) + }, sqlmock) + + loggedInUserScenarioWithRole(t, + "DELETE /snapshots/{key} should return 404 when the snapshot does not exist", "DELETE", + "/api/snapshots/12345", "/api/snapshots/:key", models.ROLE_EDITOR, func(sc *scenarioContext) { + d := setUpSnapshotTest(t) + hs := &HTTPServer{dashboardsnapshotsService: d} + sc.handlerFunc = hs.DeleteDashboardSnapshot + sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"key": "12345"}).exec() + + assert.Equal(t, http.StatusInternalServerError, sc.resp.Code) + }, sqlmock) + + loggedInUserScenarioWithRole(t, + "GET /snapshots-delete/{deleteKey} should return 404 when the snapshot does not exist", "DELETE", + "/api/snapshots-delete/12345", "/api/snapshots-delete/:deleteKey", models.ROLE_EDITOR, func(sc *scenarioContext) { + d := setUpSnapshotTest(t) + hs := &HTTPServer{dashboardsnapshotsService: d} + sc.handlerFunc = hs.DeleteDashboardSnapshotByDeleteKey + sc.fakeReqWithParams("DELETE", sc.url, map[string]string{"deleteKey": "12345"}).exec() + + assert.Equal(t, http.StatusInternalServerError, sc.resp.Code) + }, sqlmock) +} diff --git a/pkg/services/dashboardsnapshots/database/database.go b/pkg/services/dashboardsnapshots/database/database.go index fc0b956ff9e..e4b00edf0db 100644 --- a/pkg/services/dashboardsnapshots/database/database.go +++ b/pkg/services/dashboardsnapshots/database/database.go @@ -91,7 +91,7 @@ func (d *DashboardSnapshotStore) GetDashboardSnapshot(ctx context.Context, query if err != nil { return err } else if !has { - return dashboardsnapshots.ErrDashboardSnapshotNotFound + return dashboardsnapshots.ErrBaseNotFound.Errorf("dashboard snapshot not found") } query.Result = &snapshot diff --git a/pkg/services/dashboardsnapshots/errors.go b/pkg/services/dashboardsnapshots/errors.go index 268532c833b..1a6e07c9d86 100644 --- a/pkg/services/dashboardsnapshots/errors.go +++ b/pkg/services/dashboardsnapshots/errors.go @@ -1,8 +1,7 @@ package dashboardsnapshots -import "github.com/grafana/grafana/pkg/services/dashboards" +import ( + "github.com/grafana/grafana/pkg/util/errutil" +) -var ErrDashboardSnapshotNotFound = dashboards.DashboardErr{ - Reason: "Dashboard snapshot not found", - StatusCode: 404, -} +var ErrBaseNotFound = errutil.NewBase(errutil.StatusNotFound, "dashboardsnapshots.not-found", errutil.WithPublicMessage("Snapshot not found")) diff --git a/public/api-merged.json b/public/api-merged.json index e7e44551f05..998232a2f05 100644 --- a/public/api-merged.json +++ b/public/api-merged.json @@ -8909,6 +8909,9 @@ "200": { "$ref": "#/responses/getDashboardSnapshotResponse" }, + "400": { + "$ref": "#/responses/badRequestError" + }, "404": { "$ref": "#/responses/notFoundError" }, diff --git a/public/api-spec.json b/public/api-spec.json index 2bd83958721..62049a252cd 100644 --- a/public/api-spec.json +++ b/public/api-spec.json @@ -8262,6 +8262,9 @@ "200": { "$ref": "#/responses/getDashboardSnapshotResponse" }, + "400": { + "$ref": "#/responses/badRequestError" + }, "404": { "$ref": "#/responses/notFoundError" },