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.

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,
+		OrgID:        c.OrgID,
-		DashboardID: dash.ID,
+		DashboardID:  dash.ID,
-		Version:     int(version),
+		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)

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

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"
 )
 

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)
-
+	return version.ToDTO(query.DashboardUID), 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 {
@@ -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(query.DashboardUID)
-		dtos[i] = v.ToDTO("")
 	}
 	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
+}

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()
+	t.Run("List all versions for a given Dashboard ID", func(t *testing.T) {
-	dashboardVersionService := Service{store: dashboardVersionStore}
+		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{DashboardID: 42}
-		query := dashver.ListDashboardVersionsQuery{}
-		dashboardVersionStore.ExpectedListVersions = []*dashver.DashboardVersion{{}}
 		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)
 	})
 }
 

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, savedDash.ID, res.ID)
-		assert.Equal(t, query.Version, savedDash.Version)
+		assert.Equal(t, savedDash.Version, res.Version)
 		assert.Equal(t, createdById, res.CreatedBy)
 
 		dashCmd := &dashboards.Dashboard{

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
+	DashboardID  int64
-	OrgID       int64
+	DashboardUID string
-	Version     int
+	OrgID        int64
+	Version      int
 }
 
 type DeleteExpiredVersionsCommand struct {