Chore: Refectory of shorturl service, move models into service (#61295)

Chore: refectory of shorturl service, move models into service

pkg/api/short_url.go

@@ -8,6 +8,7 @@ import (
 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
 	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/shorturls"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util"
 	"github.com/grafana/grafana/pkg/web"
@@ -17,7 +18,7 @@ import (
 func (hs *HTTPServer) createShortURL(c *models.ReqContext) response.Response {
 	cmd := dtos.CreateShortURLCmd{}
 	if err := web.Bind(c.Req, &cmd); err != nil {
-		return response.Err(models.ErrShortURLBadRequest.Errorf("bad request data: %w", err))
+		return response.Err(shorturls.ErrShortURLBadRequest.Errorf("bad request data: %w", err))
 	}
 	hs.log.Debug("Received request to create short URL", "path", cmd.Path)
 	shortURL, err := hs.ShortURLService.CreateShortURL(c.Req.Context(), c.SignedInUser, cmd.Path)
@@ -45,7 +46,7 @@ func (hs *HTTPServer) redirectFromShortURL(c *models.ReqContext) {
 
 	shortURL, err := hs.ShortURLService.GetShortURLByUID(c.Req.Context(), c.SignedInUser, shortURLUID)
 	if err != nil {
-		if models.ErrShortURLNotFound.Is(err) {
+		if shorturls.ErrShortURLNotFound.Is(err) {
 			hs.log.Debug("Not redirecting short URL since not found")
 			return
 		}

pkg/api/short_url_test.go

@@ -23,14 +23,14 @@ func TestShortURLAPIEndpoint(t *testing.T) {
 			Path: "d/TxKARsmGz/new-dashboard?orgId=1&from=1599389322894&to=1599410922894",
 		}
 
-		createResp := &models.ShortUrl{
+		createResp := &shorturls.ShortUrl{
 			Id:    1,
 			OrgId: testOrgID,
 			Uid:   "N1u6L4eGz",
 			Path:  cmd.Path,
 		}
 		service := &fakeShortURLService{
-			createShortURLFunc: func(ctx context.Context, user *user.SignedInUser, path string) (*models.ShortUrl, error) {
+			createShortURLFunc: func(ctx context.Context, user *user.SignedInUser, path string) (*shorturls.ShortUrl, error) {
 				return createResp, nil
 			},
 		}
@@ -77,14 +77,14 @@ func createShortURLScenario(t *testing.T, desc string, url string, routePattern
 }
 
 type fakeShortURLService struct {
-	createShortURLFunc func(ctx context.Context, user *user.SignedInUser, path string) (*models.ShortUrl, error)
+	createShortURLFunc func(ctx context.Context, user *user.SignedInUser, path string) (*shorturls.ShortUrl, error)
 }
 
-func (s *fakeShortURLService) GetShortURLByUID(ctx context.Context, user *user.SignedInUser, uid string) (*models.ShortUrl, error) {
+func (s *fakeShortURLService) GetShortURLByUID(ctx context.Context, user *user.SignedInUser, uid string) (*shorturls.ShortUrl, error) {
 	return nil, nil
 }
 
-func (s *fakeShortURLService) CreateShortURL(ctx context.Context, user *user.SignedInUser, path string) (*models.ShortUrl, error) {
+func (s *fakeShortURLService) CreateShortURL(ctx context.Context, user *user.SignedInUser, path string) (*shorturls.ShortUrl, error) {
 	if s.createShortURLFunc != nil {
 		return s.createShortURLFunc(ctx, user, path)
 	}
@@ -92,10 +92,10 @@ func (s *fakeShortURLService) CreateShortURL(ctx context.Context, user *user.Sig
 	return nil, nil
 }
 
-func (s *fakeShortURLService) UpdateLastSeenAt(ctx context.Context, shortURL *models.ShortUrl) error {
+func (s *fakeShortURLService) UpdateLastSeenAt(ctx context.Context, shortURL *shorturls.ShortUrl) error {
 	return nil
 }
 
-func (s *fakeShortURLService) DeleteStaleShortURLs(ctx context.Context, cmd *models.DeleteShortUrlCommand) error {
+func (s *fakeShortURLService) DeleteStaleShortURLs(ctx context.Context, cmd *shorturls.DeleteShortUrlCommand) error {
 	return nil
 }

pkg/cmd/grafana-cli/runner/wire.go

@@ -103,6 +103,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/serviceaccounts"
 	serviceaccountsmanager "github.com/grafana/grafana/pkg/services/serviceaccounts/manager"
 	"github.com/grafana/grafana/pkg/services/shorturls"
+	"github.com/grafana/grafana/pkg/services/shorturls/shorturlimpl"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/services/star/starimpl"
 	"github.com/grafana/grafana/pkg/services/store"
@@ -184,8 +185,8 @@ var wireSet = wire.NewSet(
 	wire.Bind(new(httpclient.Provider), new(*sdkhttpclient.Provider)),
 	serverlock.ProvideService,
 	cleanup.ProvideService,
-	shorturls.ProvideService,
+	shorturlimpl.ProvideService,
-	wire.Bind(new(shorturls.Service), new(*shorturls.ShortURLService)),
+	wire.Bind(new(shorturls.Service), new(*shorturlimpl.ShortURLService)),
 	queryhistory.ProvideService,
 	wire.Bind(new(queryhistory.Service), new(*queryhistory.QueryHistoryService)),
 	quotaimpl.ProvideService,

pkg/server/wire.go

@@ -114,6 +114,7 @@ import (
 	serviceaccountsmanager "github.com/grafana/grafana/pkg/services/serviceaccounts/manager"
 	serviceaccountsretriever "github.com/grafana/grafana/pkg/services/serviceaccounts/retriever"
 	"github.com/grafana/grafana/pkg/services/shorturls"
+	"github.com/grafana/grafana/pkg/services/shorturls/shorturlimpl"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
 	"github.com/grafana/grafana/pkg/services/star/starimpl"
@@ -204,8 +205,8 @@ var wireBasicSet = wire.NewSet(
 	annotationsimpl.ProvideCleanupService,
 	wire.Bind(new(annotations.Cleaner), new(*annotationsimpl.CleanupServiceImpl)),
 	cleanup.ProvideService,
-	shorturls.ProvideService,
+	shorturlimpl.ProvideService,
-	wire.Bind(new(shorturls.Service), new(*shorturls.ShortURLService)),
+	wire.Bind(new(shorturls.Service), new(*shorturlimpl.ShortURLService)),
 	queryhistory.ProvideService,
 	wire.Bind(new(queryhistory.Service), new(*queryhistory.QueryHistoryService)),
 	correlations.ProvideService,

pkg/services/cleanup/cleanup.go

@@ -16,7 +16,6 @@ import (
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/serverlock"
 	"github.com/grafana/grafana/pkg/infra/tracing"
-	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/annotations"
 	"github.com/grafana/grafana/pkg/services/dashboardsnapshots"
 	dashver "github.com/grafana/grafana/pkg/services/dashboardversion"
@@ -240,7 +239,7 @@ func (srv *CleanUpService) expireOldUserInvites(ctx context.Context) {
 
 func (srv *CleanUpService) deleteStaleShortURLs(ctx context.Context) {
 	logger := srv.log.FromContext(ctx)
-	cmd := models.DeleteShortUrlCommand{
+	cmd := shorturls.DeleteShortUrlCommand{
 		OlderThan: time.Now().Add(-time.Hour * 24 * 7),
 	}
 	if err := srv.ShortURLService.DeleteStaleShortURLs(ctx, &cmd); err != nil {

pkg/services/shorturls/models.go

@@ -1,4 +1,4 @@
-package models
+package shorturls
 
 import (
 	"time"

pkg/services/shorturls/short_url_service.go

@@ -1,109 +0,0 @@
-package shorturls
-
-import (
-	"context"
-	"path"
-	"strings"
-	"time"
-
-	"github.com/grafana/grafana/pkg/infra/db"
-	"github.com/grafana/grafana/pkg/models"
-	"github.com/grafana/grafana/pkg/services/user"
-	"github.com/grafana/grafana/pkg/util"
-)
-
-var getTime = time.Now
-
-func ProvideService(sqlStore db.DB) *ShortURLService {
-	return &ShortURLService{
-		SQLStore: sqlStore,
-	}
-}
-
-type Service interface {
-	GetShortURLByUID(ctx context.Context, user *user.SignedInUser, uid string) (*models.ShortUrl, error)
-	CreateShortURL(ctx context.Context, user *user.SignedInUser, path string) (*models.ShortUrl, error)
-	UpdateLastSeenAt(ctx context.Context, shortURL *models.ShortUrl) error
-	DeleteStaleShortURLs(ctx context.Context, cmd *models.DeleteShortUrlCommand) error
-}
-
-type ShortURLService struct {
-	SQLStore db.DB
-}
-
-func (s ShortURLService) GetShortURLByUID(ctx context.Context, user *user.SignedInUser, uid string) (*models.ShortUrl, error) {
-	var shortURL models.ShortUrl
-	err := s.SQLStore.WithDbSession(ctx, func(dbSession *db.Session) error {
-		exists, err := dbSession.Where("org_id=? AND uid=?", user.OrgID, uid).Get(&shortURL)
-		if err != nil {
-			return err
-		}
-		if !exists {
-			return models.ErrShortURLNotFound.Errorf("short URL not found")
-		}
-
-		return nil
-	})
-	if err != nil {
-		return nil, err
-	}
-
-	return &shortURL, nil
-}
-
-func (s ShortURLService) UpdateLastSeenAt(ctx context.Context, shortURL *models.ShortUrl) error {
-	shortURL.LastSeenAt = getTime().Unix()
-	return s.SQLStore.WithTransactionalDbSession(ctx, func(dbSession *db.Session) error {
-		_, err := dbSession.ID(shortURL.Id).Update(shortURL)
-		if err != nil {
-			return err
-		}
-
-		return nil
-	})
-}
-
-func (s ShortURLService) CreateShortURL(ctx context.Context, user *user.SignedInUser, relPath string) (*models.ShortUrl, error) {
-	relPath = strings.TrimSpace(relPath)
-
-	if path.IsAbs(relPath) {
-		return nil, models.ErrShortURLAbsolutePath.Errorf("expected relative path: %s", relPath)
-	}
-	if strings.Contains(relPath, "../") {
-		return nil, models.ErrShortURLInvalidPath.Errorf("path cannot contain '../': %s", relPath)
-	}
-
-	now := time.Now().Unix()
-	shortURL := models.ShortUrl{
-		OrgId:     user.OrgID,
-		Uid:       util.GenerateShortUID(),
-		Path:      relPath,
-		CreatedBy: user.UserID,
-		CreatedAt: now,
-	}
-
-	err := s.SQLStore.WithDbSession(ctx, func(session *db.Session) error {
-		_, err := session.Insert(&shortURL)
-		return err
-	})
-	if err != nil {
-		return nil, models.ErrShortURLInternal.Errorf("failed to insert shorturl: %w", err)
-	}
-
-	return &shortURL, nil
-}
-
-func (s ShortURLService) DeleteStaleShortURLs(ctx context.Context, cmd *models.DeleteShortUrlCommand) error {
-	return s.SQLStore.WithTransactionalDbSession(ctx, func(session *db.Session) error {
-		var rawSql = "DELETE FROM short_url WHERE created_at <= ? AND (last_seen_at IS NULL OR last_seen_at = 0)"
-
-		if result, err := session.Exec(rawSql, cmd.OlderThan.Unix()); err != nil {
-			return err
-		} else if cmd.NumDeleted, err = result.RowsAffected(); err != nil {
-			return err
-		}
-		return nil
-	})
-}
-
-var _ Service = &ShortURLService{}

pkg/services/shorturls/shorturl.go

@@ -0,0 +1,14 @@
+package shorturls
+
+import (
+	"context"
+
+	"github.com/grafana/grafana/pkg/services/user"
+)
+
+type Service interface {
+	GetShortURLByUID(ctx context.Context, user *user.SignedInUser, uid string) (*ShortUrl, error)
+	CreateShortURL(ctx context.Context, user *user.SignedInUser, path string) (*ShortUrl, error)
+	UpdateLastSeenAt(ctx context.Context, shortURL *ShortUrl) error
+	DeleteStaleShortURLs(ctx context.Context, cmd *DeleteShortUrlCommand) error
+}

pkg/services/shorturls/shorturlimpl/shorturl.go

@@ -0,0 +1,65 @@
+package shorturlimpl
+
+import (
+	"context"
+	"path"
+	"strings"
+	"time"
+
+	"github.com/grafana/grafana/pkg/infra/db"
+	"github.com/grafana/grafana/pkg/services/shorturls"
+	"github.com/grafana/grafana/pkg/services/user"
+	"github.com/grafana/grafana/pkg/util"
+)
+
+var getTime = time.Now
+
+type ShortURLService struct {
+	SQLStore store
+}
+
+func ProvideService(db db.DB) *ShortURLService {
+	return &ShortURLService{
+		SQLStore: &sqlStore{
+			db: db,
+		},
+	}
+}
+
+func (s ShortURLService) GetShortURLByUID(ctx context.Context, user *user.SignedInUser, uid string) (*shorturls.ShortUrl, error) {
+	return s.SQLStore.Get(ctx, user, uid)
+}
+
+func (s ShortURLService) UpdateLastSeenAt(ctx context.Context, shortURL *shorturls.ShortUrl) error {
+	return s.SQLStore.Update(ctx, shortURL)
+}
+
+func (s ShortURLService) CreateShortURL(ctx context.Context, user *user.SignedInUser, relPath string) (*shorturls.ShortUrl, error) {
+	relPath = strings.TrimSpace(relPath)
+
+	if path.IsAbs(relPath) {
+		return nil, shorturls.ErrShortURLAbsolutePath.Errorf("expected relative path: %s", relPath)
+	}
+	if strings.Contains(relPath, "../") {
+		return nil, shorturls.ErrShortURLInvalidPath.Errorf("path cannot contain '../': %s", relPath)
+	}
+
+	now := time.Now().Unix()
+	shortURL := shorturls.ShortUrl{
+		OrgId:     user.OrgID,
+		Uid:       util.GenerateShortUID(),
+		Path:      relPath,
+		CreatedBy: user.UserID,
+		CreatedAt: now,
+	}
+
+	if err := s.SQLStore.Insert(ctx, &shortURL); err != nil {
+		return nil, shorturls.ErrShortURLInternal.Errorf("failed to insert shorturl: %w", err)
+	}
+
+	return &shortURL, nil
+}
+
+func (s ShortURLService) DeleteStaleShortURLs(ctx context.Context, cmd *shorturls.DeleteShortUrlCommand) error {
+	return s.SQLStore.Delete(ctx, cmd)
+}

pkg/services/shorturls/shorturlimpl/shorturl_test.go

@@ -1,4 +1,4 @@
-package shorturls
+package shorturlimpl
 
 import (
 	"context"
@@ -8,18 +8,18 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/grafana/pkg/infra/db"
-	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/shorturls"
 	"github.com/grafana/grafana/pkg/services/user"
 )
 
 func TestShortURLService(t *testing.T) {
 	user := &user.SignedInUser{UserID: 1}
-	sqlStore := db.InitTestDB(t)
+	store := db.InitTestDB(t)
 
 	t.Run("User can create and read short URLs", func(t *testing.T) {
 		const refPath = "mock/path?test=true"
 
-		service := ShortURLService{SQLStore: sqlStore}
+		service := ShortURLService{SQLStore: &sqlStore{db: store}}
 
 		newShortURL, err := service.CreateShortURL(context.Background(), user, refPath)
 		require.NoError(t, err)
@@ -57,7 +57,7 @@ func TestShortURLService(t *testing.T) {
 			require.NotEmpty(t, staleShortURL.Uid)
 			require.Equal(t, int64(0), staleShortURL.LastSeenAt)
 
-			cmd := models.DeleteShortUrlCommand{OlderThan: time.Unix(staleShortURL.CreatedAt, 0)}
+			cmd := shorturls.DeleteShortUrlCommand{OlderThan: time.Unix(staleShortURL.CreatedAt, 0)}
 			err = service.DeleteStaleShortURLs(context.Background(), &cmd)
 			require.NoError(t, err)
 			require.Equal(t, int64(1), cmd.NumDeleted)
@@ -69,7 +69,7 @@ func TestShortURLService(t *testing.T) {
 			})
 
 			t.Run("and no action when no stale short urls exist", func(t *testing.T) {
-				cmd := models.DeleteShortUrlCommand{OlderThan: time.Unix(existingShortURL.CreatedAt, 0)}
+				cmd := shorturls.DeleteShortUrlCommand{OlderThan: time.Unix(existingShortURL.CreatedAt, 0)}
 				require.NoError(t, err)
 				require.Equal(t, int64(0), cmd.NumDeleted)
 			})
@@ -77,11 +77,11 @@ func TestShortURLService(t *testing.T) {
 	})
 
 	t.Run("User cannot look up nonexistent short URLs", func(t *testing.T) {
-		service := ShortURLService{SQLStore: sqlStore}
+		service := ShortURLService{SQLStore: &sqlStore{db: store}}
 
 		shortURL, err := service.GetShortURLByUID(context.Background(), user, "testnotfounduid")
 		require.Error(t, err)
-		require.True(t, models.ErrShortURLNotFound.Is(err))
+		require.True(t, shorturls.ErrShortURLNotFound.Is(err))
 		require.Nil(t, shortURL)
 	})
 }

pkg/services/shorturls/shorturlimpl/store.go

@@ -0,0 +1,71 @@
+package shorturlimpl
+
+import (
+	"context"
+
+	"github.com/grafana/grafana/pkg/infra/db"
+	"github.com/grafana/grafana/pkg/services/shorturls"
+	"github.com/grafana/grafana/pkg/services/user"
+)
+
+type store interface {
+	Get(ctx context.Context, user *user.SignedInUser, uid string) (*shorturls.ShortUrl, error)
+	Update(ctx context.Context, shortURL *shorturls.ShortUrl) error
+	Insert(ctx context.Context, shortURL *shorturls.ShortUrl) error
+	Delete(ctx context.Context, cmd *shorturls.DeleteShortUrlCommand) error
+}
+
+type sqlStore struct {
+	db db.DB
+}
+
+func (s sqlStore) Get(ctx context.Context, user *user.SignedInUser, uid string) (*shorturls.ShortUrl, error) {
+	var shortURL shorturls.ShortUrl
+	err := s.db.WithDbSession(ctx, func(dbSession *db.Session) error {
+		exists, err := dbSession.Where("org_id=? AND uid=?", user.OrgID, uid).Get(&shortURL)
+		if err != nil {
+			return err
+		}
+		if !exists {
+			return shorturls.ErrShortURLNotFound.Errorf("short URL not found")
+		}
+
+		return nil
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	return &shortURL, nil
+}
+
+func (s sqlStore) Update(ctx context.Context, shortURL *shorturls.ShortUrl) error {
+	shortURL.LastSeenAt = getTime().Unix()
+	return s.db.WithTransactionalDbSession(ctx, func(dbSession *db.Session) error {
+		_, err := dbSession.ID(shortURL.Id).Update(shortURL)
+		if err != nil {
+			return err
+		}
+		return nil
+	})
+}
+
+func (s sqlStore) Insert(ctx context.Context, shortURL *shorturls.ShortUrl) error {
+	return s.db.WithDbSession(ctx, func(session *db.Session) error {
+		_, err := session.Insert(shortURL)
+		return err
+	})
+}
+
+func (s sqlStore) Delete(ctx context.Context, cmd *shorturls.DeleteShortUrlCommand) error {
+	return s.db.WithTransactionalDbSession(ctx, func(session *db.Session) error {
+		var rawSql = "DELETE FROM short_url WHERE created_at <= ? AND (last_seen_at IS NULL OR last_seen_at = 0)"
+
+		if result, err := session.Exec(rawSql, cmd.OlderThan.Unix()); err != nil {
+			return err
+		} else if cmd.NumDeleted, err = result.RowsAffected(); err != nil {
+			return err
+		}
+		return nil
+	})
+}