API: Fix snapshot responses (#52998) (#53244)

* 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 5fec6cc4f5)

Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>

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)

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)
+}

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

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{
+var ErrBaseNotFound = errutil.NewBase(errutil.StatusNotFound, "dashboardsnapshots.not-found", errutil.WithPublicMessage("Snapshot not found"))
-	Reason:     "Dashboard snapshot not found",
-	StatusCode: 404,
-}

public/api-merged.json

@@ -8282,6 +8282,9 @@
           "200": {
             "$ref": "#/responses/getDashboardSnapshotResponse"
           },
+          "400": {
+            "$ref": "#/responses/badRequestError"
+          },
           "404": {
             "$ref": "#/responses/notFoundError"
           },

public/api-spec.json

@@ -7701,6 +7701,9 @@
           "200": {
             "$ref": "#/responses/getDashboardSnapshotResponse"
           },
+          "400": {
+            "$ref": "#/responses/badRequestError"
+          },
           "404": {
             "$ref": "#/responses/notFoundError"
           },