From 183f9317c5db8088d9e8ddb41a1f8053ddb876b4 Mon Sep 17 00:00:00 2001 From: Timur Olzhabayev Date: Mon, 27 Feb 2023 23:01:48 +0100 Subject: [PATCH] =?UTF-8?q?fix(dashboard=20version=20service):=20add=20Das?= =?UTF-8?q?hboardUID=20to=20query=20and=20respons=E2=80=A6=20(#63821)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix(dashboard version service): add DashboardUID to query and responses (#60800) * fix(dashboard version service): add DashboardUID to query and responses The DashboardUID was not populated in the response from Get and ListDashboardVersions. This adds the DashboardUID to the Get query (it was already in List) and populated the DashboardUID in the returned DashboardVersionDTOs. (cherry picked from commit 42be0e106feab07361c3f4ee665a7067880ed674) Co-authored-by: Kristin Laemmert --- pkg/api/dashboard.go | 11 ++- .../dashboards/dashboard_service_mock.go | 3 +- pkg/services/dashboards/store_mock.go | 3 +- .../dashboardversion/dashverimpl/dashver.go | 92 ++++++++++++++++-- .../dashverimpl/dashver_test.go | 93 +++++++++++++++++-- .../dashverimpl/store_test.go | 4 +- pkg/services/dashboardversion/model.go | 9 +- 7 files changed, 187 insertions(+), 28 deletions(-) diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index ce2ca9d31aa..bd4f5d79a46 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -739,9 +739,10 @@ func (hs *HTTPServer) GetDashboardVersion(c *contextmodel.ReqContext) response.R version, _ := strconv.ParseInt(web.Params(c.Req)[":id"], 10, 32) query := dashver.GetDashboardVersionQuery{ - OrgID: c.OrgID, - DashboardID: dash.ID, - Version: int(version), + OrgID: c.OrgID, + DashboardID: dash.ID, + DashboardUID: dash.UID, + Version: int(version), } res, err := hs.dashboardVersionService.Get(c.Req.Context(), &query) @@ -757,7 +758,7 @@ func (hs *HTTPServer) GetDashboardVersion(c *contextmodel.ReqContext) response.R dashVersionMeta := &dashver.DashboardVersionMeta{ ID: res.ID, DashboardID: res.DashboardID, - DashboardUID: dashUID, + DashboardUID: dash.UID, Data: res.Data, ParentVersion: res.ParentVersion, RestoredFrom: res.RestoredFrom, @@ -989,7 +990,7 @@ func (hs *HTTPServer) RestoreDashboardVersion(c *contextmodel.ReqContext) respon return dashboardGuardianResponse(err) } - versionQuery := dashver.GetDashboardVersionQuery{DashboardID: dashID, Version: apiCmd.Version, OrgID: c.OrgID} + versionQuery := dashver.GetDashboardVersionQuery{DashboardID: dashID, DashboardUID: dash.UID, Version: apiCmd.Version, OrgID: c.OrgID} version, err := hs.dashboardVersionService.Get(c.Req.Context(), &versionQuery) if err != nil { return response.Error(404, "Dashboard version not found", nil) diff --git a/pkg/services/dashboards/dashboard_service_mock.go b/pkg/services/dashboards/dashboard_service_mock.go index a59d811dbbe..3da367b22be 100644 --- a/pkg/services/dashboards/dashboard_service_mock.go +++ b/pkg/services/dashboards/dashboard_service_mock.go @@ -5,9 +5,8 @@ package dashboards import ( context "context" - mock "github.com/stretchr/testify/mock" - folder "github.com/grafana/grafana/pkg/services/folder" + mock "github.com/stretchr/testify/mock" ) // FakeDashboardService is an autogenerated mock type for the DashboardService type diff --git a/pkg/services/dashboards/store_mock.go b/pkg/services/dashboards/store_mock.go index 4b2b5a9bded..f32268a86ce 100644 --- a/pkg/services/dashboards/store_mock.go +++ b/pkg/services/dashboards/store_mock.go @@ -5,10 +5,11 @@ package dashboards import ( context "context" + folder "github.com/grafana/grafana/pkg/services/folder" mock "github.com/stretchr/testify/mock" models "github.com/grafana/grafana/pkg/services/alerting/models" - folder "github.com/grafana/grafana/pkg/services/folder" + quota "github.com/grafana/grafana/pkg/services/quota" ) diff --git a/pkg/services/dashboardversion/dashverimpl/dashver.go b/pkg/services/dashboardversion/dashverimpl/dashver.go index 569c0050a15..f8b090560ba 100644 --- a/pkg/services/dashboardversion/dashverimpl/dashver.go +++ b/pkg/services/dashboardversion/dashverimpl/dashver.go @@ -2,8 +2,11 @@ package dashverimpl import ( "context" + "errors" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/dashboards" dashver "github.com/grafana/grafana/pkg/services/dashboardversion" "github.com/grafana/grafana/pkg/setting" ) @@ -14,27 +17,49 @@ const ( ) type Service struct { - store store + store store + dashSvc dashboards.DashboardService + log log.Logger } -func ProvideService(db db.DB) dashver.Service { +func ProvideService(db db.DB, dashboardService dashboards.DashboardService) dashver.Service { return &Service{ store: &sqlStore{ db: db, dialect: db.GetDialect(), }, + dashSvc: dashboardService, + log: log.New("dashboard-version"), } } func (s *Service) Get(ctx context.Context, query *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersionDTO, error) { + // Get the DashboardUID if not populated + if query.DashboardUID == "" { + u, err := s.getDashUIDMaybeEmpty(ctx, query.DashboardID) + if err != nil { + return nil, err + } + query.DashboardUID = u + } + + // The store methods require the dashboard ID (uid is not in the dashboard + // versions table, at time of this writing), so get the DashboardID if it + // was not populated. + if query.DashboardID == 0 { + id, err := s.getDashIDMaybeEmpty(ctx, query.DashboardUID) + if err != nil { + return nil, err + } + query.DashboardID = id + } + version, err := s.store.Get(ctx, query) if err != nil { return nil, err } version.Data.Set("id", version.DashboardID) - - // FIXME: the next PR will add the dashboardService so we can grab the DashboardUID - return version.ToDTO(""), nil + return version.ToDTO(query.DashboardUID), nil } func (s *Service) DeleteExpired(ctx context.Context, cmd *dashver.DeleteExpiredVersionsCommand) error { @@ -69,6 +94,25 @@ func (s *Service) DeleteExpired(ctx context.Context, cmd *dashver.DeleteExpiredV // List all dashboard versions for the given dashboard ID. func (s *Service) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) { + // Get the DashboardUID if not populated + if query.DashboardUID == "" { + u, err := s.getDashUIDMaybeEmpty(ctx, query.DashboardID) + if err != nil { + return nil, err + } + query.DashboardUID = u + } + + // The store methods require the dashboard ID (uid is not in the dashboard + // versions table, at time of this writing), so get the DashboardID if it + // was not populated. + if query.DashboardID == 0 { + id, err := s.getDashIDMaybeEmpty(ctx, query.DashboardUID) + if err != nil { + return nil, err + } + query.DashboardID = id + } if query.Limit == 0 { query.Limit = 1000 } @@ -78,8 +122,42 @@ func (s *Service) List(ctx context.Context, query *dashver.ListDashboardVersions } dtos := make([]*dashver.DashboardVersionDTO, len(dvs)) for i, v := range dvs { - // FIXME: the next PR will add the dashboardService so we can grab the DashboardUID - dtos[i] = v.ToDTO("") + dtos[i] = v.ToDTO(query.DashboardUID) } return dtos, nil } + +// getDashUIDMaybeEmpty is a helper function which takes a dashboardID and +// returns the UID. If the dashboard is not found, it will return an empty +// string. +func (s *Service) getDashUIDMaybeEmpty(ctx context.Context, id int64) (string, error) { + q := dashboards.GetDashboardRefByIDQuery{ID: id} + result, err := s.dashSvc.GetDashboardUIDByID(ctx, &q) + if err != nil { + if errors.Is(err, dashboards.ErrDashboardNotFound) { + s.log.Debug("dashboard not found") + return "", nil + } else { + s.log.Error("error getting dashboard", err) + return "", err + } + } + return result.UID, nil +} + +// getDashIDMaybeEmpty is a helper function which takes a dashboardUID and +// returns the ID. If the dashboard is not found, it will return -1. +func (s *Service) getDashIDMaybeEmpty(ctx context.Context, uid string) (int64, error) { + q := dashboards.GetDashboardQuery{UID: uid} + result, err := s.dashSvc.GetDashboard(ctx, &q) + if err != nil { + if errors.Is(err, dashboards.ErrDashboardNotFound) { + s.log.Debug("dashboard not found") + return -1, nil + } else { + s.log.Error("error getting dashboard", err) + return -1, err + } + } + return result.ID, nil +} diff --git a/pkg/services/dashboardversion/dashverimpl/dashver_test.go b/pkg/services/dashboardversion/dashverimpl/dashver_test.go index 480733bd56f..4cac843787a 100644 --- a/pkg/services/dashboardversion/dashverimpl/dashver_test.go +++ b/pkg/services/dashboardversion/dashverimpl/dashver_test.go @@ -5,16 +5,20 @@ import ( "errors" "testing" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/dashboards" dashver "github.com/grafana/grafana/pkg/services/dashboardversion" "github.com/grafana/grafana/pkg/setting" ) func TestDashboardVersionService(t *testing.T) { dashboardVersionStore := newDashboardVersionStoreFake() - dashboardVersionService := Service{store: dashboardVersionStore} + dashboardService := dashboards.NewFakeDashboardService(t) + dashboardVersionService := Service{store: dashboardVersionStore, dashSvc: dashboardService} t.Run("Get dashboard version", func(t *testing.T) { dashboard := &dashver.DashboardVersion{ @@ -22,9 +26,11 @@ func TestDashboardVersionService(t *testing.T) { Data: &simplejson.Json{}, } dashboardVersionStore.ExpectedDashboardVersion = dashboard + dashboardService.On("GetDashboardUIDByID", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardRefByIDQuery")).Return(&dashboards.DashboardRef{UID: "uid"}, nil) + dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardQuery")).Return(&dashboards.Dashboard{ID: 42}, nil) dashboardVersion, err := dashboardVersionService.Get(context.Background(), &dashver.GetDashboardVersionQuery{}) require.NoError(t, err) - require.Equal(t, dashboard.ToDTO(""), dashboardVersion) + require.Equal(t, dashboard.ToDTO("uid"), dashboardVersion) }) } @@ -33,7 +39,8 @@ func TestDeleteExpiredVersions(t *testing.T) { setting.DashboardVersionsToKeep = versionsToKeep dashboardVersionStore := newDashboardVersionStoreFake() - dashboardVersionService := Service{store: dashboardVersionStore} + dashboardService := dashboards.NewFakeDashboardService(t) + dashboardVersionService := Service{store: dashboardVersionStore, dashSvc: dashboardService} t.Run("Don't delete anything if there are no expired versions", func(t *testing.T) { err := dashboardVersionService.DeleteExpired(context.Background(), &dashver.DeleteExpiredVersionsCommand{DeletedRows: 4}) @@ -55,15 +62,85 @@ func TestDeleteExpiredVersions(t *testing.T) { } func TestListDashboardVersions(t *testing.T) { - dashboardVersionStore := newDashboardVersionStoreFake() - dashboardVersionService := Service{store: dashboardVersionStore} + t.Run("List all versions for a given Dashboard ID", func(t *testing.T) { + dashboardVersionStore := newDashboardVersionStoreFake() + dashboardService := dashboards.NewFakeDashboardService(t) + dashboardVersionService := Service{store: dashboardVersionStore, dashSvc: dashboardService} + dashboardVersionStore.ExpectedListVersions = []*dashver.DashboardVersion{ + {ID: 1, DashboardID: 42}, + } + dashboardService.On("GetDashboardUIDByID", mock.Anything, + mock.AnythingOfType("*dashboards.GetDashboardRefByIDQuery")). + Return(&dashboards.DashboardRef{UID: "uid"}, nil) - t.Run("Get all versions for a given Dashboard ID", func(t *testing.T) { - query := dashver.ListDashboardVersionsQuery{} - dashboardVersionStore.ExpectedListVersions = []*dashver.DashboardVersion{{}} + query := dashver.ListDashboardVersionsQuery{DashboardID: 42} res, err := dashboardVersionService.List(context.Background(), &query) require.Nil(t, err) require.Equal(t, 1, len(res)) + // validate that the UID was populated + require.EqualValues(t, []*dashver.DashboardVersionDTO{{ID: 1, DashboardID: 42, DashboardUID: "uid"}}, res) + }) + + t.Run("List all versions for a non-existent DashboardID", func(t *testing.T) { + dashboardVersionStore := newDashboardVersionStoreFake() + dashboardService := dashboards.NewFakeDashboardService(t) + dashboardVersionService := Service{store: dashboardVersionStore, dashSvc: dashboardService, log: log.NewNopLogger()} + dashboardVersionStore.ExpectedListVersions = []*dashver.DashboardVersion{ + {ID: 1, DashboardID: 42}, + } + dashboardService.On("GetDashboardUIDByID", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardRefByIDQuery")). + Return(nil, dashboards.ErrDashboardNotFound).Once() + + query := dashver.ListDashboardVersionsQuery{DashboardID: 42} + res, err := dashboardVersionService.List(context.Background(), &query) + require.Nil(t, err) + require.Equal(t, 1, len(res)) + // The DashboardID remains populated with the given value, even though the dash was not found + require.EqualValues(t, []*dashver.DashboardVersionDTO{{ID: 1, DashboardID: 42}}, res) + }) + + t.Run("List all versions for a given DashboardUID", func(t *testing.T) { + dashboardVersionStore := newDashboardVersionStoreFake() + dashboardService := dashboards.NewFakeDashboardService(t) + dashboardVersionService := Service{store: dashboardVersionStore, dashSvc: dashboardService, log: log.NewNopLogger()} + dashboardVersionStore.ExpectedListVersions = []*dashver.DashboardVersion{{DashboardID: 42, ID: 1}} + dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardQuery")). + Return(&dashboards.Dashboard{ID: 42}, nil) + + query := dashver.ListDashboardVersionsQuery{DashboardUID: "uid"} + res, err := dashboardVersionService.List(context.Background(), &query) + require.Nil(t, err) + require.Equal(t, 1, len(res)) + // validate that the dashboardID was populated from the GetDashboard method call. + require.EqualValues(t, []*dashver.DashboardVersionDTO{{ID: 1, DashboardID: 42, DashboardUID: "uid"}}, res) + }) + + t.Run("List all versions for a given non-existent DashboardUID", func(t *testing.T) { + dashboardVersionStore := newDashboardVersionStoreFake() + dashboardService := dashboards.NewFakeDashboardService(t) + dashboardVersionService := Service{store: dashboardVersionStore, dashSvc: dashboardService, log: log.NewNopLogger()} + dashboardVersionStore.ExpectedListVersions = []*dashver.DashboardVersion{{DashboardID: 42, ID: 1}} + dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*dashboards.GetDashboardQuery")). + Return(nil, dashboards.ErrDashboardNotFound) + + query := dashver.ListDashboardVersionsQuery{DashboardUID: "uid"} + res, err := dashboardVersionService.List(context.Background(), &query) + require.Nil(t, err) + require.Equal(t, 1, len(res)) + // validate that the dashboardUID & ID are populated, even though the dash was not found + require.EqualValues(t, []*dashver.DashboardVersionDTO{{ID: 1, DashboardID: 42, DashboardUID: "uid"}}, res) + }) + + t.Run("List Dashboard versions - error from store", func(t *testing.T) { + dashboardVersionStore := newDashboardVersionStoreFake() + dashboardService := dashboards.NewFakeDashboardService(t) + dashboardVersionService := Service{store: dashboardVersionStore, dashSvc: dashboardService, log: log.NewNopLogger()} + dashboardVersionStore.ExpectedError = dashver.ErrDashboardVersionNotFound + + query := dashver.ListDashboardVersionsQuery{DashboardID: 42, DashboardUID: "42"} + res, err := dashboardVersionService.List(context.Background(), &query) + require.Nil(t, res) + require.ErrorIs(t, err, dashver.ErrDashboardVersionNotFound) }) } diff --git a/pkg/services/dashboardversion/dashverimpl/store_test.go b/pkg/services/dashboardversion/dashverimpl/store_test.go index 25255828dd8..753a6654809 100644 --- a/pkg/services/dashboardversion/dashverimpl/store_test.go +++ b/pkg/services/dashboardversion/dashverimpl/store_test.go @@ -34,8 +34,8 @@ func testIntegrationGetDashboardVersion(t *testing.T, fn getStore) { res, err := dashVerStore.Get(context.Background(), &query) require.Nil(t, err) - assert.Equal(t, query.DashboardID, savedDash.ID) - assert.Equal(t, query.Version, savedDash.Version) + assert.Equal(t, savedDash.ID, res.ID) + assert.Equal(t, savedDash.Version, res.Version) assert.Equal(t, createdById, res.CreatedBy) dashCmd := &dashboards.Dashboard{ diff --git a/pkg/services/dashboardversion/model.go b/pkg/services/dashboardversion/model.go index b6a00918373..35cb8facc48 100644 --- a/pkg/services/dashboardversion/model.go +++ b/pkg/services/dashboardversion/model.go @@ -46,10 +46,13 @@ func (v *DashboardVersion) ToDTO(dashUid string) *DashboardVersionDTO { } } +// GetDashboardVersionQuery is used to Get a dashboard version. Only one of +// DashboardID and DashboardUID are required. type GetDashboardVersionQuery struct { - DashboardID int64 - OrgID int64 - Version int + DashboardID int64 + DashboardUID string + OrgID int64 + Version int } type DeleteExpiredVersionsCommand struct {