Short URL: Cleanup unvisited/stale short URLs (#28867)

* cleanup stale short urls

* refactor test case names

* service injection

* fix query

* add docs

* remove comma

docs/sources/explore/index.md

@@ -53,7 +53,7 @@ You can close the newly created query by clicking on the Close Split button.
 
 
 > Share shortened link is only available in Grafana 7.3 and above.
 > Share shortened link is only available in Grafana 7.3 and above.
 
 
-The Share shortened link capability allows you to create smaller and simpler URLs of the format /goto/:uid instead of using longer URLs containing complex query parameters. You can create a shortened link by clicking on the **Share** option in Explore toolbar.
+The Share shortened link capability allows you to create smaller and simpler URLs of the format /goto/:uid instead of using longer URLs containing complex query parameters. You can create a shortened link by clicking on the **Share** option in Explore toolbar. Please note that any shortened links that are never used will be automatically deleted after 7 days.
 
 
 ## Query history
 ## Query history
 
 

pkg/models/shorturl.go

@@ -2,6 +2,7 @@ package models
 
 
 import (
 import (
 	"errors"
 	"errors"
+	"time"
 )
 )
 
 
 var (
 var (
@@ -17,3 +18,9 @@ type ShortUrl struct {
 	CreatedAt  int64
 	CreatedAt  int64
 	LastSeenAt int64
 	LastSeenAt int64
 }
 }
+
+type DeleteShortUrlCommand struct {
+	OlderThan time.Time
+
+	NumDeleted int64
+}

pkg/services/cleanup/cleanup.go

@@ -7,6 +7,8 @@ import (
 	"path"
 	"path"
 	"time"
 	"time"
 
 
+	"github.com/grafana/grafana/pkg/services/shorturls"
+
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/serverlock"
 	"github.com/grafana/grafana/pkg/infra/serverlock"
@@ -20,6 +22,7 @@ type CleanUpService struct {
 	log               log.Logger
 	log               log.Logger
 	Cfg               *setting.Cfg                  `inject:""`
 	Cfg               *setting.Cfg                  `inject:""`
 	ServerLockService *serverlock.ServerLockService `inject:""`
 	ServerLockService *serverlock.ServerLockService `inject:""`
+	ShortURLService   *shorturls.ShortURLService    `inject:""`
 }
 }
 
 
 func init() {
 func init() {
@@ -46,6 +49,7 @@ func (srv *CleanUpService) Run(ctx context.Context) error {
 			srv.deleteExpiredDashboardVersions()
 			srv.deleteExpiredDashboardVersions()
 			srv.cleanUpOldAnnotations(ctxWithTimeout)
 			srv.cleanUpOldAnnotations(ctxWithTimeout)
 			srv.expireOldUserInvites()
 			srv.expireOldUserInvites()
+			srv.deleteStaleShortURLs()
 			err := srv.ServerLockService.LockAndExecute(ctx, "delete old login attempts",
 			err := srv.ServerLockService.LockAndExecute(ctx, "delete old login attempts",
 				time.Minute*10, func() {
 				time.Minute*10, func() {
 					srv.deleteOldLoginAttempts()
 					srv.deleteOldLoginAttempts()
@@ -151,3 +155,14 @@ func (srv *CleanUpService) expireOldUserInvites() {
 		srv.log.Debug("Expired user invites", "rows affected", cmd.NumExpired)
 		srv.log.Debug("Expired user invites", "rows affected", cmd.NumExpired)
 	}
 	}
 }
 }
+
+func (srv *CleanUpService) deleteStaleShortURLs() {
+	cmd := models.DeleteShortUrlCommand{
+		OlderThan: time.Now().Add(-time.Hour * 24 * 7),
+	}
+	if err := srv.ShortURLService.DeleteStaleShortURLs(context.Background(), &cmd); err != nil {
+		srv.log.Error("Problem deleting stale short urls", "error", err.Error())
+	} else {
+		srv.log.Debug("Deleted short urls", "rows affected", cmd.NumDeleted)
+	}
+}

pkg/services/shorturls/short_url_service.go

@@ -76,3 +76,16 @@ func (s ShortURLService) CreateShortURL(ctx context.Context, user *models.Signed
 
 
 	return &shortURL, nil
 	return &shortURL, nil
 }
 }
+
+func (s ShortURLService) DeleteStaleShortURLs(ctx context.Context, cmd *models.DeleteShortUrlCommand) error {
+	return s.SQLStore.WithTransactionalDbSession(ctx, func(session *sqlstore.DBSession) 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
+	})
+}

pkg/services/shorturls/short_url_service_test.go

@@ -47,6 +47,31 @@ func TestShortURLService(t *testing.T) {
 			require.NoError(t, err)
 			require.NoError(t, err)
 			require.Equal(t, expectedTime.Unix(), updatedShortURL.LastSeenAt)
 			require.Equal(t, expectedTime.Unix(), updatedShortURL.LastSeenAt)
 		})
 		})
+
+		t.Run("and stale short urls can be deleted", func(t *testing.T) {
+			staleShortURL, err := service.CreateShortURL(context.Background(), user, refPath)
+			require.NoError(t, err)
+			require.NotNil(t, staleShortURL)
+			require.NotEmpty(t, staleShortURL.Uid)
+			require.Equal(t, int64(0), staleShortURL.LastSeenAt)
+
+			cmd := models.DeleteShortUrlCommand{OlderThan: time.Unix(staleShortURL.CreatedAt, 0)}
+			err = service.DeleteStaleShortURLs(context.Background(), &cmd)
+			require.NoError(t, err)
+			require.Equal(t, int64(1), cmd.NumDeleted)
+
+			t.Run("and previously accessed short urls will still exist", func(t *testing.T) {
+				updatedShortURL, err := service.GetShortURLByUID(context.Background(), user, existingShortURL.Uid)
+				require.NoError(t, err)
+				require.NotNil(t, updatedShortURL)
+			})
+
+			t.Run("and no action when no stale short urls exist", func(t *testing.T) {
+				cmd := models.DeleteShortUrlCommand{OlderThan: time.Unix(existingShortURL.CreatedAt, 0)}
+				require.NoError(t, err)
+				require.Equal(t, int64(0), cmd.NumDeleted)
+			})
+		})
 	})
 	})
 
 
 	t.Run("User cannot look up nonexistent short URLs", func(t *testing.T) {
 	t.Run("User cannot look up nonexistent short URLs", func(t *testing.T) {

pkg/services/sqlstore/temp_user_test.go

@@ -4,6 +4,7 @@ package sqlstore
 
 
 import (
 import (
 	"testing"
 	"testing"
+	"time"
 
 
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/models"
 	. "github.com/smartystreets/goconvey/convey"
 	. "github.com/smartystreets/goconvey/convey"
@@ -68,16 +69,18 @@ func TestTempUserCommandsAndQueries(t *testing.T) {
 			})
 			})
 
 
 			Convey("Should be able expire temp user", func() {
 			Convey("Should be able expire temp user", func() {
-				cmd2 := models.ExpireTempUsersCommand{OlderThan: timeNow()}
+				createdAt := time.Unix(cmd.Result.Created, 0)
+				cmd2 := models.ExpireTempUsersCommand{OlderThan: createdAt.Add(1 * time.Second)}
 				err := ExpireOldUserInvites(&cmd2)
 				err := ExpireOldUserInvites(&cmd2)
 				So(err, ShouldBeNil)
 				So(err, ShouldBeNil)
-				So(cmd2.NumExpired, ShouldEqual, 1)
+				So(cmd2.NumExpired, ShouldEqual, int64(1))
 
 
 				Convey("Should do nothing when no temp users to expire", func() {
 				Convey("Should do nothing when no temp users to expire", func() {
-					cmd2 = models.ExpireTempUsersCommand{OlderThan: timeNow()}
+					createdAt := time.Unix(cmd.Result.Created, 0)
+					cmd2 := models.ExpireTempUsersCommand{OlderThan: createdAt.Add(1 * time.Second)}
 					err := ExpireOldUserInvites(&cmd2)
 					err := ExpireOldUserInvites(&cmd2)
 					So(err, ShouldBeNil)
 					So(err, ShouldBeNil)
-					So(cmd2.NumExpired, ShouldEqual, 0)
+					So(cmd2.NumExpired, ShouldEqual, int64(0))
 				})
 				})
 			})
 			})
 		})
 		})