From dc52320fc9e0b1f801f77a326cbbcc234bb9dc99 Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Thu, 4 Aug 2022 19:21:39 +0300 Subject: [PATCH] [v8.5.x] API: Fix snapshot responses (#53301) * 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 (cherry picked from commit 5fec6cc4f5b7dd4aa37399e93a77e6111d867f95) --- pkg/api/dashboard_snapshot.go | 18 ++++++-- pkg/api/dashboard_snapshot_test.go | 69 ++++++++++++++++++++++++++++ pkg/api/docs/definitions/snapshot.go | 1 + public/api-merged.json | 3 ++ public/api-spec.json | 3 ++ 5 files changed, 90 insertions(+), 4 deletions(-) diff --git a/pkg/api/dashboard_snapshot.go b/pkg/api/dashboard_snapshot.go index 1d5c45cc5f7..378b67d4429 100644 --- a/pkg/api/dashboard_snapshot.go +++ b/pkg/api/dashboard_snapshot.go @@ -3,6 +3,7 @@ package api import ( "bytes" "encoding/json" + "errors" "fmt" "net/http" "time" @@ -152,14 +153,17 @@ func (hs *HTTPServer) CreateDashboardSnapshot(c *models.ReqContext) response.Res 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 := &models.GetDashboardSnapshotQuery{Key: key} err := hs.DashboardsnapshotsService.GetDashboardSnapshot(c.Req.Context(), query) if err != nil { - return response.Error(500, "Failed to get dashboard snapshot", err) + if errors.Is(err, models.ErrDashboardSnapshotNotFound) { + return response.Error(http.StatusNotFound, "Failed to find dashboard snapshot", err) + } + return response.Error(http.StatusInternalServerError, "Failed to get dashboard snapshot", err) } snapshot := query.Result @@ -226,7 +230,10 @@ func (hs *HTTPServer) DeleteDashboardSnapshotByDeleteKey(c *models.ReqContext) r query := &models.GetDashboardSnapshotQuery{DeleteKey: key} err := hs.DashboardsnapshotsService.GetDashboardSnapshot(c.Req.Context(), query) if err != nil { - return response.Error(500, "Failed to get dashboard snapshot", err) + if errors.Is(err, models.ErrDashboardSnapshotNotFound) { + return response.Error(http.StatusNotFound, "Failed to find dashboard snapshot", err) + } + return response.Error(http.StatusInternalServerError, "Failed to get dashboard snapshot", err) } if query.Result.External { @@ -259,7 +266,10 @@ 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) + if errors.Is(err, models.ErrDashboardSnapshotNotFound) { + return response.Error(http.StatusNotFound, "Failed to find dashboard snapshot", err) + } + return response.Error(http.StatusInternalServerError, "Failed to get dashboard snapshot", 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 d34d285c46d..4375f34259b 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" @@ -233,3 +234,71 @@ func TestDashboardSnapshotAPIEndpoint_singleSnapshot(t *testing.T) { }, sqlmock) }) } + +func TestGetDashboardSnapshotNotFound(t *testing.T) { + sqlmock := mockstore.NewSQLStoreMock() + sqlmock.ExpectedTeamsByUser = []*models.TeamDTO{} + sqlmock.ExpectedError = models.ErrDashboardSnapshotNotFound + hs := &HTTPServer{DashboardsnapshotsService: &dashboardsnapshots.Service{SQLStore: sqlmock}} + + 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) { + 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) { + 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) { + 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{} + sqlmock.ExpectedError = errors.New("something went wrong") + hs := &HTTPServer{DashboardsnapshotsService: &dashboardsnapshots.Service{SQLStore: sqlmock}} + + 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) { + 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) { + 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) { + 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/api/docs/definitions/snapshot.go b/pkg/api/docs/definitions/snapshot.go index 4926595a722..c1549ccefd9 100644 --- a/pkg/api/docs/definitions/snapshot.go +++ b/pkg/api/docs/definitions/snapshot.go @@ -30,6 +30,7 @@ import ( // // Responses: // 200: snapshotResponse +// 400: badRequestError // 404: notFoundError // 500: internalServerError diff --git a/public/api-merged.json b/public/api-merged.json index 6b93374de7c..a2d284e15f8 100644 --- a/public/api-merged.json +++ b/public/api-merged.json @@ -7897,6 +7897,9 @@ "200": { "$ref": "#/responses/snapshotResponse" }, + "400": { + "$ref": "#/responses/badRequestError" + }, "404": { "$ref": "#/responses/notFoundError" }, diff --git a/public/api-spec.json b/public/api-spec.json index ca093fc588c..0b620e9a4d6 100644 --- a/public/api-spec.json +++ b/public/api-spec.json @@ -6294,6 +6294,9 @@ "200": { "$ref": "#/responses/snapshotResponse" }, + "400": { + "$ref": "#/responses/badRequestError" + }, "404": { "$ref": "#/responses/notFoundError" },