From 6e9419ea80a790518df5ee6126d07e210bd25ff4 Mon Sep 17 00:00:00 2001 From: Kristin Laemmert Date: Tue, 27 Dec 2022 17:17:24 +0100 Subject: [PATCH] chore(dashboard version service): make method sigs more consistent (#60736) The DashboardVersion struct is the database object; the DashboardVersionDTO is the object that should be sent to the API layer. In the future I'd like to move DashboardVersion to dashverimpl and un-export it, but there are a few places that Insert directly into that table, not all of which are test fixtures, so that should wait until we clean up at least the DashboardService's use of it. --- pkg/api/dashboard_test.go | 10 ++--- pkg/services/dashboardversion/dashver.go | 2 +- .../dashboardversion/dashverimpl/dashver.go | 17 +++++-- .../dashverimpl/dashver_test.go | 11 ++--- .../dashverimpl/sqlx_store.go | 7 ++- .../dashboardversion/dashverimpl/store.go | 2 +- .../dashverimpl/xorm_store.go | 4 +- .../dashboardversion/dashvertest/fake.go | 6 +-- pkg/services/dashboardversion/model.go | 45 +++++++++++++------ 9 files changed, 67 insertions(+), 37 deletions(-) diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index 43b877f1e3c..baeaa8b222d 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -133,7 +133,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { fakeDash.FolderId = 1 fakeDash.HasACL = false fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake() - fakeDashboardVersionService.ExpectedDashboardVersion = &dashver.DashboardVersion{} + fakeDashboardVersionService.ExpectedDashboardVersion = &dashver.DashboardVersionDTO{} teamService := &teamtest.FakeService{} dashboardService := dashboards.NewFakeDashboardService(t) dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { @@ -241,7 +241,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { fakeDash.FolderId = 1 fakeDash.HasACL = true fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake() - fakeDashboardVersionService.ExpectedDashboardVersion = &dashver.DashboardVersion{} + fakeDashboardVersionService.ExpectedDashboardVersion = &dashver.DashboardVersionDTO{} teamService := &teamtest.FakeService{} dashboardService := dashboards.NewFakeDashboardService(t) dashboardService.On("GetDashboard", mock.Anything, mock.AnythingOfType("*models.GetDashboardQuery")).Run(func(args mock.Arguments) { @@ -787,7 +787,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { t.Run("Given two dashboards being compared", func(t *testing.T) { fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake() - fakeDashboardVersionService.ExpectedDashboardVersions = []*dashver.DashboardVersion{ + fakeDashboardVersionService.ExpectedDashboardVersions = []*dashver.DashboardVersionDTO{ { DashboardID: 1, Version: 1, @@ -874,7 +874,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { Version: 1, } fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake() - fakeDashboardVersionService.ExpectedDashboardVersions = []*dashver.DashboardVersion{ + fakeDashboardVersionService.ExpectedDashboardVersions = []*dashver.DashboardVersionDTO{ { DashboardID: 2, Version: 1, @@ -908,7 +908,7 @@ func TestDashboardAPIEndpoint(t *testing.T) { }).Return(nil, nil) fakeDashboardVersionService := dashvertest.NewDashboardVersionServiceFake() - fakeDashboardVersionService.ExpectedDashboardVersions = []*dashver.DashboardVersion{ + fakeDashboardVersionService.ExpectedDashboardVersions = []*dashver.DashboardVersionDTO{ { DashboardID: 2, Version: 1, diff --git a/pkg/services/dashboardversion/dashver.go b/pkg/services/dashboardversion/dashver.go index 59c893f3fda..8609e238d55 100644 --- a/pkg/services/dashboardversion/dashver.go +++ b/pkg/services/dashboardversion/dashver.go @@ -5,7 +5,7 @@ import ( ) type Service interface { - Get(context.Context, *GetDashboardVersionQuery) (*DashboardVersion, error) + Get(context.Context, *GetDashboardVersionQuery) (*DashboardVersionDTO, error) DeleteExpired(context.Context, *DeleteExpiredVersionsCommand) error List(context.Context, *ListDashboardVersionsQuery) ([]*DashboardVersionDTO, error) } diff --git a/pkg/services/dashboardversion/dashverimpl/dashver.go b/pkg/services/dashboardversion/dashverimpl/dashver.go index 82822a62ccf..569c0050a15 100644 --- a/pkg/services/dashboardversion/dashverimpl/dashver.go +++ b/pkg/services/dashboardversion/dashverimpl/dashver.go @@ -26,13 +26,15 @@ func ProvideService(db db.DB) dashver.Service { } } -func (s *Service) Get(ctx context.Context, query *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersion, error) { +func (s *Service) Get(ctx context.Context, query *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersionDTO, error) { version, err := s.store.Get(ctx, query) if err != nil { return nil, err } version.Data.Set("id", version.DashboardID) - return version, nil + + // FIXME: the next PR will add the dashboardService so we can grab the DashboardUID + return version.ToDTO(""), nil } func (s *Service) DeleteExpired(ctx context.Context, cmd *dashver.DeleteExpiredVersionsCommand) error { @@ -70,5 +72,14 @@ func (s *Service) List(ctx context.Context, query *dashver.ListDashboardVersions if query.Limit == 0 { query.Limit = 1000 } - return s.store.List(ctx, query) + dvs, err := s.store.List(ctx, query) + if err != nil { + return nil, err + } + 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("") + } + return dtos, nil } diff --git a/pkg/services/dashboardversion/dashverimpl/dashver_test.go b/pkg/services/dashboardversion/dashverimpl/dashver_test.go index 516c987a30a..480733bd56f 100644 --- a/pkg/services/dashboardversion/dashverimpl/dashver_test.go +++ b/pkg/services/dashboardversion/dashverimpl/dashver_test.go @@ -5,10 +5,11 @@ import ( "errors" "testing" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/components/simplejson" dashver "github.com/grafana/grafana/pkg/services/dashboardversion" "github.com/grafana/grafana/pkg/setting" - "github.com/stretchr/testify/require" ) func TestDashboardVersionService(t *testing.T) { @@ -23,7 +24,7 @@ func TestDashboardVersionService(t *testing.T) { dashboardVersionStore.ExpectedDashboardVersion = dashboard dashboardVersion, err := dashboardVersionService.Get(context.Background(), &dashver.GetDashboardVersionQuery{}) require.NoError(t, err) - require.Equal(t, dashboardVersion, dashboard) + require.Equal(t, dashboard.ToDTO(""), dashboardVersion) }) } @@ -59,7 +60,7 @@ func TestListDashboardVersions(t *testing.T) { t.Run("Get all versions for a given Dashboard ID", func(t *testing.T) { query := dashver.ListDashboardVersionsQuery{} - dashboardVersionStore.ExpectedListVersions = []*dashver.DashboardVersionDTO{{}} + dashboardVersionStore.ExpectedListVersions = []*dashver.DashboardVersion{{}} res, err := dashboardVersionService.List(context.Background(), &query) require.Nil(t, err) require.Equal(t, 1, len(res)) @@ -70,7 +71,7 @@ type FakeDashboardVersionStore struct { ExpectedDashboardVersion *dashver.DashboardVersion ExptectedDeletedVersions int64 ExpectedVersions []interface{} - ExpectedListVersions []*dashver.DashboardVersionDTO + ExpectedListVersions []*dashver.DashboardVersion ExpectedError error } @@ -90,6 +91,6 @@ func (f *FakeDashboardVersionStore) DeleteBatch(ctx context.Context, cmd *dashve return f.ExptectedDeletedVersions, f.ExpectedError } -func (f *FakeDashboardVersionStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) { +func (f *FakeDashboardVersionStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersion, error) { return f.ExpectedListVersions, f.ExpectedError } diff --git a/pkg/services/dashboardversion/dashverimpl/sqlx_store.go b/pkg/services/dashboardversion/dashverimpl/sqlx_store.go index 11bca594f16..b8a564bbc23 100644 --- a/pkg/services/dashboardversion/dashverimpl/sqlx_store.go +++ b/pkg/services/dashboardversion/dashverimpl/sqlx_store.go @@ -59,16 +59,15 @@ func (ss *sqlxStore) DeleteBatch(ctx context.Context, cmd *dashver.DeleteExpired return deleted, err } -func (ss *sqlxStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) { - var dashboardVersion []*dashver.DashboardVersionDTO +func (ss *sqlxStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersion, error) { + var dashboardVersion []*dashver.DashboardVersion qr := `SELECT dashboard_version.id, dashboard_version.dashboard_id, dashboard_version.parent_version, dashboard_version.restored_from, dashboard_version.version, dashboard_version.created, - dashboard_version.message, - "user".login as created_by_login + dashboard_version.message FROM dashboard_version LEFT JOIN "user" ON "user".id = dashboard_version.created_by LEFT JOIN dashboard ON dashboard.id = dashboard_version.dashboard_id diff --git a/pkg/services/dashboardversion/dashverimpl/store.go b/pkg/services/dashboardversion/dashverimpl/store.go index d378a5eb663..dfe2ea0cf4f 100644 --- a/pkg/services/dashboardversion/dashverimpl/store.go +++ b/pkg/services/dashboardversion/dashverimpl/store.go @@ -10,5 +10,5 @@ type store interface { Get(context.Context, *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersion, error) GetBatch(context.Context, *dashver.DeleteExpiredVersionsCommand, int, int) ([]interface{}, error) DeleteBatch(context.Context, *dashver.DeleteExpiredVersionsCommand, []interface{}) (int64, error) - List(context.Context, *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) + List(context.Context, *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersion, error) } diff --git a/pkg/services/dashboardversion/dashverimpl/xorm_store.go b/pkg/services/dashboardversion/dashverimpl/xorm_store.go index 58a4b3652a0..ac9b3e080f1 100644 --- a/pkg/services/dashboardversion/dashverimpl/xorm_store.go +++ b/pkg/services/dashboardversion/dashverimpl/xorm_store.go @@ -71,8 +71,8 @@ func (ss *sqlStore) DeleteBatch(ctx context.Context, cmd *dashver.DeleteExpiredV return deleted, err } -func (ss *sqlStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersionDTO, error) { - var dashboardVersion []*dashver.DashboardVersionDTO +func (ss *sqlStore) List(ctx context.Context, query *dashver.ListDashboardVersionsQuery) ([]*dashver.DashboardVersion, error) { + var dashboardVersion []*dashver.DashboardVersion err := ss.db.WithDbSession(ctx, func(sess *db.Session) error { err := sess.Table("dashboard_version"). Select(`dashboard_version.id, diff --git a/pkg/services/dashboardversion/dashvertest/fake.go b/pkg/services/dashboardversion/dashvertest/fake.go index c43076a1a9d..1d45c63aa3c 100644 --- a/pkg/services/dashboardversion/dashvertest/fake.go +++ b/pkg/services/dashboardversion/dashvertest/fake.go @@ -7,8 +7,8 @@ import ( ) type FakeDashboardVersionService struct { - ExpectedDashboardVersion *dashver.DashboardVersion - ExpectedDashboardVersions []*dashver.DashboardVersion + ExpectedDashboardVersion *dashver.DashboardVersionDTO + ExpectedDashboardVersions []*dashver.DashboardVersionDTO ExpectedListDashboarVersions []*dashver.DashboardVersionDTO counter int ExpectedError error @@ -18,7 +18,7 @@ func NewDashboardVersionServiceFake() *FakeDashboardVersionService { return &FakeDashboardVersionService{} } -func (f *FakeDashboardVersionService) Get(ctx context.Context, query *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersion, error) { +func (f *FakeDashboardVersionService) Get(ctx context.Context, query *dashver.GetDashboardVersionQuery) (*dashver.DashboardVersionDTO, error) { if len(f.ExpectedDashboardVersions) == 0 { return f.ExpectedDashboardVersion, f.ExpectedError } diff --git a/pkg/services/dashboardversion/model.go b/pkg/services/dashboardversion/model.go index 86c3756da47..b6a00918373 100644 --- a/pkg/services/dashboardversion/model.go +++ b/pkg/services/dashboardversion/model.go @@ -12,6 +12,10 @@ var ( ErrNoVersionsForDashboardID = errors.New("no dashboard versions found for the given DashboardId") ) +// DashboardVersion represents a dashboard version in the database. Ideally this +// will be moved into dashverimpl and unexported, but there are a few test +// fixtures that insert DashboardVersions directly into a database which must be +// refactored first. type DashboardVersion struct { ID int64 `json:"id" xorm:"pk autoincr 'id'" db:"id"` DashboardID int64 `json:"dashboardId" xorm:"dashboard_id" db:"dashboard_id"` @@ -26,6 +30,22 @@ type DashboardVersion struct { Data *simplejson.Json `json:"data" db:"data"` } +// ToDTO converts a DashboardVersion to a DashboardVersionDTO. +func (v *DashboardVersion) ToDTO(dashUid string) *DashboardVersionDTO { + return &DashboardVersionDTO{ + ID: v.ID, + DashboardID: v.DashboardID, + DashboardUID: dashUid, + ParentVersion: v.ParentVersion, + RestoredFrom: v.RestoredFrom, + Version: v.Version, + Created: v.Created, + CreatedBy: v.CreatedBy, + Message: v.Message, + Data: v.Data, + } +} + type GetDashboardVersionQuery struct { DashboardID int64 OrgID int64 @@ -45,22 +65,21 @@ type ListDashboardVersionsQuery struct { } type DashboardVersionDTO struct { - ID int64 `json:"id" xorm:"id" db:"id"` - DashboardID int64 `json:"dashboardId" xorm:"dashboard_id" db:"dashboard_id"` - DashboardUID string `json:"dashboardUid" xorm:"dashboard_uid" db:"dashboard_uid"` - ParentVersion int `json:"parentVersion" db:"parent_version"` - RestoredFrom int `json:"restoredFrom" db:"restored_from"` - Version int `json:"version" db:"version"` - Created time.Time `json:"created" db:"created"` - // Since we get created by with left join user table, this can be null technically, - // but in reality it will always be set, when database is not corrupted. - CreatedBy *string `json:"createdBy" db:"created_by_login"` - Message string `json:"message" db:"message"` + ID int64 `json:"id"` + DashboardID int64 `json:"dashboardId"` + DashboardUID string `json:"dashboardUid"` + ParentVersion int `json:"parentVersion"` + RestoredFrom int `json:"restoredFrom"` + Version int `json:"version"` + Created time.Time `json:"created"` + CreatedBy int64 `json:"createdBy"` + Message string `json:"message"` + Data *simplejson.Json `json:"data" db:"data"` } -// DashboardVersionMeta extends the dashboard version model with the names +// DashboardVersionMeta extends the DashboardVersionDTO with the names // associated with the UserIds, overriding the field with the same name from -// the DashboardVersion model. +// the DashboardVersionDTO model. type DashboardVersionMeta struct { ID int64 `json:"id"` DashboardID int64 `json:"dashboardId"`