diff --git a/pkg/server/wire.go b/pkg/server/wire.go index 17f2e479fd8..68cd6147429 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -75,7 +75,9 @@ import ( authinfodatabase "github.com/grafana/grafana/pkg/services/login/authinfoservice/database" "github.com/grafana/grafana/pkg/services/login/loginservice" "github.com/grafana/grafana/pkg/services/ngalert" + ngimage "github.com/grafana/grafana/pkg/services/ngalert/image" ngmetrics "github.com/grafana/grafana/pkg/services/ngalert/metrics" + ngstore "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/notifications" "github.com/grafana/grafana/pkg/services/oauthtoken" "github.com/grafana/grafana/pkg/services/org/orgimpl" @@ -211,6 +213,8 @@ var wireBasicSet = wire.NewSet( contexthandler.ProvideService, jwt.ProvideService, wire.Bind(new(models.JWTService), new(*jwt.AuthService)), + ngstore.ProvideDBStore, + ngimage.ProvideDeleteExpiredService, ngalert.ProvideService, librarypanels.ProvideService, wire.Bind(new(librarypanels.Service), new(*librarypanels.LibraryPanelService)), diff --git a/pkg/services/cleanup/cleanup.go b/pkg/services/cleanup/cleanup.go index a495c902bf3..768b4e3f99a 100644 --- a/pkg/services/cleanup/cleanup.go +++ b/pkg/services/cleanup/cleanup.go @@ -8,44 +8,46 @@ import ( "path" "time" - "github.com/grafana/grafana/pkg/services/dashboardsnapshots" - dashver "github.com/grafana/grafana/pkg/services/dashboardversion" - "github.com/grafana/grafana/pkg/services/queryhistory" - "github.com/grafana/grafana/pkg/services/shorturls" - "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/serverlock" "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" + "github.com/grafana/grafana/pkg/services/ngalert/image" + "github.com/grafana/grafana/pkg/services/queryhistory" + "github.com/grafana/grafana/pkg/services/shorturls" + "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" ) func ProvideService(cfg *setting.Cfg, serverLockService *serverlock.ServerLockService, - shortURLService shorturls.Service, store sqlstore.Store, queryHistoryService queryhistory.Service, - dashboardVersionService dashver.Service, dashSnapSvc dashboardsnapshots.Service) *CleanUpService { + shortURLService shorturls.Service, sqlstore *sqlstore.SQLStore, queryHistoryService queryhistory.Service, + dashboardVersionService dashver.Service, dashSnapSvc dashboardsnapshots.Service, deleteExpiredImageService *image.DeleteExpiredService) *CleanUpService { s := &CleanUpService{ - Cfg: cfg, - ServerLockService: serverLockService, - ShortURLService: shortURLService, - QueryHistoryService: queryHistoryService, - store: store, - log: log.New("cleanup"), - dashboardVersionService: dashboardVersionService, - dashboardSnapshotService: dashSnapSvc, + Cfg: cfg, + ServerLockService: serverLockService, + ShortURLService: shortURLService, + QueryHistoryService: queryHistoryService, + store: sqlstore, + log: log.New("cleanup"), + dashboardVersionService: dashboardVersionService, + dashboardSnapshotService: dashSnapSvc, + deleteExpiredImageService: deleteExpiredImageService, } return s } type CleanUpService struct { - log log.Logger - store sqlstore.Store - Cfg *setting.Cfg - ServerLockService *serverlock.ServerLockService - ShortURLService shorturls.Service - QueryHistoryService queryhistory.Service - dashboardVersionService dashver.Service - dashboardSnapshotService dashboardsnapshots.Service + log log.Logger + store sqlstore.Store + Cfg *setting.Cfg + ServerLockService *serverlock.ServerLockService + ShortURLService shorturls.Service + QueryHistoryService queryhistory.Service + dashboardVersionService dashver.Service + dashboardSnapshotService dashboardsnapshots.Service + deleteExpiredImageService *image.DeleteExpiredService } func (srv *CleanUpService) Run(ctx context.Context) error { @@ -61,6 +63,7 @@ func (srv *CleanUpService) Run(ctx context.Context) error { srv.cleanUpTmpFiles() srv.deleteExpiredSnapshots(ctx) srv.deleteExpiredDashboardVersions(ctx) + srv.deleteExpiredImages(ctx) srv.cleanUpOldAnnotations(ctxWithTimeout) srv.expireOldUserInvites(ctx) srv.deleteStaleShortURLs(ctx) @@ -156,6 +159,17 @@ func (srv *CleanUpService) deleteExpiredDashboardVersions(ctx context.Context) { } } +func (srv *CleanUpService) deleteExpiredImages(ctx context.Context) { + if !srv.Cfg.UnifiedAlerting.IsEnabled() { + return + } + if rowsAffected, err := srv.deleteExpiredImageService.DeleteExpired(ctx); err != nil { + srv.log.Error("Failed to delete expired images", "error", err.Error()) + } else { + srv.log.Debug("Deleted expired images", "rows affected", rowsAffected) + } +} + func (srv *CleanUpService) deleteOldLoginAttempts(ctx context.Context) { if srv.Cfg.DisableBruteForceLoginProtection { return diff --git a/pkg/services/ngalert/image/service.go b/pkg/services/ngalert/image/service.go index 0d5b656086a..1528367be42 100644 --- a/pkg/services/ngalert/image/service.go +++ b/pkg/services/ngalert/image/service.go @@ -32,6 +32,19 @@ var ( ErrNoPanel = errors.New("no panel") ) +// DeleteExpiredService is a service to delete expired images. +type DeleteExpiredService struct { + store store.ImageAdminStore +} + +func (s *DeleteExpiredService) DeleteExpired(ctx context.Context) (int64, error) { + return s.store.DeleteExpiredImages(ctx) +} + +func ProvideDeleteExpiredService(store *store.DBstore) *DeleteExpiredService { + return &DeleteExpiredService{store: store} +} + //go:generate mockgen -destination=mock.go -package=image github.com/grafana/grafana/pkg/services/ngalert/image ImageService type ImageService interface { // NewImage returns a new image for the alert instance. @@ -127,14 +140,16 @@ func (s *ScreenshotImageService) NewImage(ctx context.Context, r *ngmodels.Alert return &v, nil } +// NotAvailableImageService is a service that returns ErrScreenshotsUnavailable. type NotAvailableImageService struct{} -func (s *NotAvailableImageService) NewImage(ctx context.Context, r *ngmodels.AlertRule) (*ngmodels.Image, error) { +func (s *NotAvailableImageService) NewImage(_ context.Context, _ *ngmodels.AlertRule) (*ngmodels.Image, error) { return nil, screenshot.ErrScreenshotsUnavailable } +// NoopImageService is a no-op image service. type NoopImageService struct{} -func (s *NoopImageService) NewImage(ctx context.Context, r *ngmodels.AlertRule) (*ngmodels.Image, error) { +func (s *NoopImageService) NewImage(_ context.Context, _ *ngmodels.AlertRule) (*ngmodels.Image, error) { return &ngmodels.Image{}, nil } diff --git a/pkg/services/ngalert/models/image.go b/pkg/services/ngalert/models/image.go index 0610f6ca1c5..8c2d8cefd05 100644 --- a/pkg/services/ngalert/models/image.go +++ b/pkg/services/ngalert/models/image.go @@ -5,8 +5,10 @@ import ( "time" ) -// ErrImageNotFound is returned when the image does not exist. -var ErrImageNotFound = errors.New("image not found") +var ( + // ErrImageNotFound is returned when the image does not exist. + ErrImageNotFound = errors.New("image not found") +) type Image struct { ID int64 `xorm:"pk autoincr 'id'"` @@ -17,6 +19,27 @@ type Image struct { ExpiresAt time.Time `xorm:"expires_at"` } +// ExtendDuration extends the expiration time of the image. It can shorten +// the duration of the image if d is negative. +func (i *Image) ExtendDuration(d time.Duration) { + i.ExpiresAt = i.ExpiresAt.Add(d) +} + +// HasExpired returns true if the image has expired. +func (i *Image) HasExpired() bool { + return time.Now().After(i.ExpiresAt) +} + +// HasPath returns true if the image has a path on disk. +func (i *Image) HasPath() bool { + return i.Path != "" +} + +// HasURL returns true if the image has a URL. +func (i *Image) HasURL() bool { + return i.URL != "" +} + // A XORM interface that defines the used table for this struct. func (i *Image) TableName() string { return "alert_image" diff --git a/pkg/services/ngalert/models/image_test.go b/pkg/services/ngalert/models/image_test.go new file mode 100644 index 00000000000..5c8380cff04 --- /dev/null +++ b/pkg/services/ngalert/models/image_test.go @@ -0,0 +1,48 @@ +package models + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestImage_ExtendDuration(t *testing.T) { + var i Image + d := time.Now().Add(time.Minute) + i.ExpiresAt = d + // extend the duration for 1 minute + i.ExtendDuration(time.Minute) + assert.Equal(t, d.Add(time.Minute), i.ExpiresAt) + // can shorten the duration too + i.ExtendDuration(-time.Minute) + assert.Equal(t, d, i.ExpiresAt) +} + +func TestImage_HasExpired(t *testing.T) { + var i Image + i.ExpiresAt = time.Now().Add(time.Minute) + assert.False(t, i.HasExpired()) + i.ExpiresAt = time.Now() + assert.True(t, i.HasExpired()) + i.ExpiresAt = time.Now().Add(-time.Minute) + assert.True(t, i.HasExpired()) +} + +func TestImage_HasPath(t *testing.T) { + var i Image + assert.False(t, i.HasPath()) + i.Path = "/" + assert.True(t, i.HasPath()) + i.Path = "/tmp/image.png" + assert.True(t, i.HasPath()) +} + +func TestImage_HasURL(t *testing.T) { + var i Image + assert.False(t, i.HasURL()) + i.URL = "/" + assert.True(t, i.HasURL()) + i.URL = "https://example.com/image.png" + assert.True(t, i.HasURL()) +} diff --git a/pkg/services/ngalert/notifier/testing.go b/pkg/services/ngalert/notifier/testing.go index 3415569bd27..86001d4da3a 100644 --- a/pkg/services/ngalert/notifier/testing.go +++ b/pkg/services/ngalert/notifier/testing.go @@ -27,8 +27,8 @@ func (f *FakeConfigStore) GetImage(ctx context.Context, token string) (*models.I return nil, models.ErrImageNotFound } -func (f *FakeConfigStore) GetImages(ctx context.Context, tokens []string) ([]models.Image, error) { - return nil, models.ErrImageNotFound +func (f *FakeConfigStore) GetImages(ctx context.Context, tokens []string) ([]models.Image, []string, error) { + return nil, nil, models.ErrImageNotFound } func NewFakeConfigStore(t *testing.T, configs map[int64]*models.AlertConfiguration) FakeConfigStore { diff --git a/pkg/services/ngalert/store/database.go b/pkg/services/ngalert/store/database.go index a86e5168186..d3a1a37a5bc 100644 --- a/pkg/services/ngalert/store/database.go +++ b/pkg/services/ngalert/store/database.go @@ -36,3 +36,16 @@ type DBstore struct { AccessControl accesscontrol.AccessControl DashboardService dashboards.DashboardService } + +func ProvideDBStore( + cfg *setting.Cfg, sqlstore *sqlstore.SQLStore, folderService dashboards.FolderService, + access accesscontrol.AccessControl, dashboards dashboards.DashboardService) *DBstore { + return &DBstore{ + Cfg: cfg.UnifiedAlerting, + SQLStore: sqlstore, + Logger: log.New("dbstore"), + FolderService: folderService, + AccessControl: access, + DashboardService: dashboards, + } +} diff --git a/pkg/services/ngalert/store/image.go b/pkg/services/ngalert/store/image.go index b1702027dc4..a7b32c1cef9 100644 --- a/pkg/services/ngalert/store/image.go +++ b/pkg/services/ngalert/store/image.go @@ -11,84 +11,123 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore" ) +const ( + imageExpirationDuration = 24 * time.Hour +) + type ImageStore interface { - // GetImage returns the image with the token or ErrImageNotFound. + // GetImage returns the image with the token. It returns ErrImageNotFound + // if the image has expired or if an image with the token does not exist. GetImage(ctx context.Context, token string) (*models.Image, error) - // GetImages returns all images that match the tokens. If one or more - // tokens does not exist then it also returns ErrImageNotFound. - GetImages(ctx context.Context, tokens []string) ([]models.Image, error) + // GetImages returns all images that match the tokens. If one or more images + // have expired or do not exist then it also returns the unmatched tokens + // and an ErrImageNotFound error. + GetImages(ctx context.Context, tokens []string) ([]models.Image, []string, error) // SaveImage saves the image or returns an error. SaveImage(ctx context.Context, img *models.Image) error } -func (st DBstore) GetImage(ctx context.Context, token string) (*models.Image, error) { - var img models.Image - if err := st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { - exists, err := sess.Where("token = ?", token).Get(&img) - if err != nil { - return fmt.Errorf("failed to get image: %w", err) - } - if !exists { - return models.ErrImageNotFound - } - return nil - }); err != nil { - return nil, err - } - return &img, nil +type ImageAdminStore interface { + ImageStore + + // DeleteExpiredImages deletes expired images. It returns the number of deleted images + // or an error. + DeleteExpiredImages(context.Context) (int64, error) } -func (st DBstore) GetImages(ctx context.Context, tokens []string) ([]models.Image, error) { - var imgs []models.Image +func (st DBstore) GetImage(ctx context.Context, token string) (*models.Image, error) { + var image models.Image if err := st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { - return sess.In("token", tokens).Find(&imgs) + exists, err := sess.Where("token = ? AND expires_at > ?", token, TimeNow().UTC()).Get(&image) + if err != nil { + return fmt.Errorf("failed to get image: %w", err) + } else if !exists { + return models.ErrImageNotFound + } else { + return nil + } }); err != nil { return nil, err } - if len(imgs) < len(tokens) { - return imgs, models.ErrImageNotFound + return &image, nil +} + +func (st DBstore) GetImages(ctx context.Context, tokens []string) ([]models.Image, []string, error) { + var images []models.Image + if err := st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { + return sess.In("token", tokens).Where("expires_at > ?", TimeNow().UTC()).Find(&images) + }); err != nil { + return nil, nil, err } - return imgs, nil + if len(images) < len(tokens) { + return images, unmatchedTokens(tokens, images), models.ErrImageNotFound + } + return images, nil, nil } func (st DBstore) SaveImage(ctx context.Context, img *models.Image) error { return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { - // TODO: Is this a good idea? Do we actually want to automatically expire - // rows? See issue https://github.com/grafana/grafana/issues/49366 - img.ExpiresAt = TimeNow().Add(1 * time.Minute).UTC() - if img.ID == 0 { // xorm will fill this field on Insert. + if img.ID == 0 { + // If the ID is zero then this is a new image. It needs a token, a created timestamp + // and an expiration time. The expiration time of the image is derived from the created + // timestamp rather than the current time as it helps assert that the expiration time + // has the intended duration in tests. token, err := uuid.NewRandom() if err != nil { return fmt.Errorf("failed to create token: %w", err) } img.Token = token.String() img.CreatedAt = TimeNow().UTC() + img.ExpiresAt = img.CreatedAt.Add(imageExpirationDuration) if _, err := sess.Insert(img); err != nil { - return fmt.Errorf("failed to insert screenshot: %w", err) + return fmt.Errorf("failed to insert image: %w", err) } } else { - affected, err := sess.ID(img.ID).Update(img) - if err != nil { - return fmt.Errorf("failed to update screenshot: %v", err) + // Check if the image exists as some databases return 0 rows affected if + // no changes were made + if ok, err := sess.Where("id = ?", img.ID).ForUpdate().Exist(&models.Image{}); err != nil { + return fmt.Errorf("failed to check if image exists: %v", err) + } else if !ok { + return models.ErrImageNotFound } - if affected == 0 { - return fmt.Errorf("update statement had no effect") + + // Do not reset the expiration time as it can be extended with ExtendDuration + if _, err := sess.ID(img.ID).Update(img); err != nil { + return fmt.Errorf("failed to update image: %v", err) } } return nil }) } -//nolint:unused -func (st DBstore) DeleteExpiredImages(ctx context.Context) error { - return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { - n, err := sess.Where("expires_at < ?", TimeNow()).Delete(&models.Image{}) +func (st DBstore) DeleteExpiredImages(ctx context.Context) (int64, error) { + var n int64 + if err := st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + rows, err := sess.Where("expires_at < ?", TimeNow().UTC()).Delete(&models.Image{}) if err != nil { return fmt.Errorf("failed to delete expired images: %w", err) } - st.Logger.Info("deleted expired images", "n", n) - return err - }) + n = rows + return nil + }); err != nil { + return -1, err + } + return n, nil +} + +// unmatchedTokens returns the tokens that were not matched to an image. +func unmatchedTokens(tokens []string, images []models.Image) []string { + matched := make(map[string]struct{}) + for _, image := range images { + matched[image.Token] = struct{}{} + } + unmatched := make([]string, 0, len(tokens)) + for _, token := range tokens { + if _, ok := matched[token]; !ok { + unmatched = append(unmatched, token) + } + } + return unmatched } diff --git a/pkg/services/ngalert/store/image_test.go b/pkg/services/ngalert/store/image_test.go index 4834a038b52..71a8d0585d6 100644 --- a/pkg/services/ngalert/store/image_test.go +++ b/pkg/services/ngalert/store/image_test.go @@ -5,7 +5,6 @@ import ( "testing" "time" - "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -14,182 +13,175 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/tests" ) -func createTestImg(fakeUrl string, fakePath string) *models.Image { - return &models.Image{ - ID: 0, - Token: "", - Path: fakeUrl + "local", - URL: fakeUrl, - } -} - -func addID(img *models.Image, id int64) *models.Image { - img.ID = id - return img -} - -func addToken(img *models.Image) *models.Image { - token, err := uuid.NewRandom() - if err != nil { - panic("wat") - } - img.Token = token.String() - return img -} - func TestIntegrationSaveAndGetImage(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } - mockTimeNow() + + // our database schema uses second precision for timestamps + store.TimeNow = func() time.Time { + return time.Now().Truncate(time.Second) + } + ctx := context.Background() _, dbstore := tests.SetupTestEnv(t, baseIntervalSeconds) - // Here are some images to save. - imgs := []struct { - name string - img *models.Image - errors bool - }{ - { - "with file path", - createTestImg("", "path"), - false, - }, - { - "with URL", - createTestImg("url", ""), - false, - }, - { - "ID already set, should not change", - addToken(addID(createTestImg("Foo", ""), 123)), - true, - }, - } + // create an image with a path on disk + image1 := models.Image{Path: "example.png"} + require.NoError(t, dbstore.SaveImage(ctx, &image1)) + require.NotEqual(t, "", image1.Token) - for _, test := range imgs { - t.Run(test.name, func(t *testing.T) { - ctx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - err := dbstore.SaveImage(ctx, test.img) - if test.errors { - require.Error(t, err) - return - } + // image should not have expired + assert.False(t, image1.HasExpired()) + assert.Equal(t, image1.ExpiresAt, image1.CreatedAt.Add(24*time.Hour)) - require.NoError(t, err) - returned, err := dbstore.GetImage(ctx, test.img.Token) - assert.NoError(t, err, "Shouldn't error when getting the image") - assert.Equal(t, test.img, returned) + // should return the image with a path on disk + result1, err := dbstore.GetImage(ctx, image1.Token) + require.NoError(t, err) + assert.Equal(t, image1, *result1) - // Save again to test update path. - err = dbstore.SaveImage(ctx, test.img) - require.NoError(t, err, "Should have no error on second write") - returned, err = dbstore.GetImage(ctx, test.img.Token) - assert.NoError(t, err, "Shouldn't error when getting the image a second time") - assert.Equal(t, test.img, returned) - }) - } + // save the image a second time should not change the expiration time + ts := image1.ExpiresAt + require.NoError(t, dbstore.SaveImage(ctx, &image1)) + assert.Equal(t, image1.ExpiresAt, ts) + + // create an image with a URL + image2 := models.Image{URL: "https://example.com/example.png"} + require.NoError(t, dbstore.SaveImage(ctx, &image2)) + require.NotEqual(t, "", image2.Token) + + // image should not have expired + assert.False(t, image2.HasExpired()) + assert.Equal(t, image2.ExpiresAt, image2.CreatedAt.Add(24*time.Hour)) + + // should return the image with a URL + result2, err := dbstore.GetImage(ctx, image2.Token) + require.NoError(t, err) + assert.Equal(t, image2, *result2) + + // expired image should not be returned + image1.ExpiresAt = time.Now().Add(-time.Second) + require.NoError(t, dbstore.SaveImage(ctx, &image1)) + result1, err = dbstore.GetImage(ctx, image1.Token) + assert.EqualError(t, err, "image not found") + assert.Nil(t, result1) } func TestIntegrationGetImages(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } - mockTimeNow() - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() + + // our database schema uses second precision for timestamps + store.TimeNow = func() time.Time { + return time.Now().Truncate(time.Second) + } + + ctx := context.Background() _, dbstore := tests.SetupTestEnv(t, baseIntervalSeconds) - // create an image foo.png - img1 := models.Image{Path: "foo.png"} - require.NoError(t, dbstore.SaveImage(ctx, &img1)) + // create an image with a path on disk + image1 := models.Image{Path: "example.png"} + require.NoError(t, dbstore.SaveImage(ctx, &image1)) - // GetImages should return the first image - imgs, err := dbstore.GetImages(ctx, []string{img1.Token}) + // should return the first image + images, mismatched, err := dbstore.GetImages(ctx, []string{image1.Token}) require.NoError(t, err) - assert.Equal(t, []models.Image{img1}, imgs) + assert.Len(t, mismatched, 0) + assert.Equal(t, []models.Image{image1}, images) - // create another image bar.png - img2 := models.Image{Path: "bar.png"} - require.NoError(t, dbstore.SaveImage(ctx, &img2)) + // create an image with a URL + image2 := models.Image{Path: "https://example.com/example.png"} + require.NoError(t, dbstore.SaveImage(ctx, &image2)) - // GetImages should return both images - imgs, err = dbstore.GetImages(ctx, []string{img1.Token, img2.Token}) + // should return both images + images, mismatched, err = dbstore.GetImages(ctx, []string{image1.Token, image2.Token}) require.NoError(t, err) - assert.ElementsMatch(t, []models.Image{img1, img2}, imgs) + assert.Len(t, mismatched, 0) + assert.ElementsMatch(t, []models.Image{image1, image2}, images) - // GetImages should return the first image - imgs, err = dbstore.GetImages(ctx, []string{img1.Token}) + // should return the first image + images, mismatched, err = dbstore.GetImages(ctx, []string{image1.Token}) require.NoError(t, err) - assert.Equal(t, []models.Image{img1}, imgs) + assert.Len(t, mismatched, 0) + assert.Equal(t, []models.Image{image1}, images) - // GetImages should return the second image - imgs, err = dbstore.GetImages(ctx, []string{img2.Token}) + // should return the second image + images, mismatched, err = dbstore.GetImages(ctx, []string{image2.Token}) require.NoError(t, err) - assert.Equal(t, []models.Image{img2}, imgs) + assert.Len(t, mismatched, 0) + assert.Equal(t, []models.Image{image2}, images) - // GetImages should return the first image and an error - imgs, err = dbstore.GetImages(ctx, []string{img1.Token, "unknown"}) + // should return the first image and an error + images, mismatched, err = dbstore.GetImages(ctx, []string{image1.Token, "unknown"}) assert.EqualError(t, err, "image not found") - assert.Equal(t, []models.Image{img1}, imgs) + assert.Equal(t, []string{"unknown"}, mismatched) + assert.Equal(t, []models.Image{image1}, images) - // GetImages should return no images for no tokens - imgs, err = dbstore.GetImages(ctx, []string{}) + // should return no images for no tokens + images, mismatched, err = dbstore.GetImages(ctx, []string{}) require.NoError(t, err) - assert.Len(t, imgs, 0) + assert.Len(t, mismatched, 0) + assert.Len(t, images, 0) - // GetImages should return no images for nil tokens - imgs, err = dbstore.GetImages(ctx, nil) + // should return no images for nil tokens + images, mismatched, err = dbstore.GetImages(ctx, nil) require.NoError(t, err) - assert.Len(t, imgs, 0) + assert.Len(t, mismatched, 0) + assert.Len(t, images, 0) + + // expired image should not be returned + image1.ExpiresAt = time.Now().Add(-time.Second) + require.NoError(t, dbstore.SaveImage(ctx, &image1)) + images, mismatched, err = dbstore.GetImages(ctx, []string{image1.Token, image2.Token}) + assert.EqualError(t, err, "image not found") + assert.Equal(t, []string{image1.Token}, mismatched) + assert.Equal(t, []models.Image{image2}, images) } func TestIntegrationDeleteExpiredImages(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") } - mockTimeNow() - ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) - defer cancel() + + // our database schema uses second precision for timestamps + store.TimeNow = func() time.Time { + return time.Now().Truncate(time.Second) + } + + ctx := context.Background() _, dbstore := tests.SetupTestEnv(t, baseIntervalSeconds) - // Save two images. - imgs := []*models.Image{ - createTestImg("", ""), - createTestImg("", ""), - } + // create two images + image1 := models.Image{Path: "example.png"} + require.NoError(t, dbstore.SaveImage(ctx, &image1)) + image2 := models.Image{URL: "https://example.com/example.png"} + require.NoError(t, dbstore.SaveImage(ctx, &image2)) - for _, img := range imgs { - err := dbstore.SaveImage(ctx, img) - require.NoError(t, err) - } + s := dbstore.SQLStore.NewSession(ctx) + t.Cleanup(s.Close) - // Images are availabile - img, err := dbstore.GetImage(ctx, imgs[0].Token) + // should return both images + var result1, result2 models.Image + ok, err := s.Where("token = ?", image1.Token).Get(&result1) require.NoError(t, err) - require.NotNil(t, img) - - img, err = dbstore.GetImage(ctx, imgs[1].Token) + assert.True(t, ok) + ok, err = s.Where("token = ?", image2.Token).Get(&result2) require.NoError(t, err) - require.NotNil(t, img) + assert.True(t, ok) - // Wait until timeout. - for i := 0; i < 120; i++ { - store.TimeNow() - } - - // Call expired - err = dbstore.DeleteExpiredImages(ctx) + // should delete expired image + image1.ExpiresAt = time.Now().Add(-time.Second) + require.NoError(t, dbstore.SaveImage(ctx, &image1)) + n, err := dbstore.DeleteExpiredImages(ctx) require.NoError(t, err) + assert.Equal(t, int64(1), n) - // All images are gone. - img, err = dbstore.GetImage(ctx, imgs[0].Token) - require.Nil(t, img) - require.Error(t, err) - - img, err = dbstore.GetImage(ctx, imgs[1].Token) - require.Nil(t, img) - require.Error(t, err) + // should return just the second image + ok, err = s.Where("token = ?", image1.Token).Get(&result1) + require.NoError(t, err) + assert.False(t, ok) + ok, err = s.Where("token = ?", image2.Token).Get(&result2) + require.NoError(t, err) + assert.True(t, ok) } diff --git a/pkg/services/ngalert/store/instance_database_test.go b/pkg/services/ngalert/store/instance_database_test.go index a194aac0351..a0b1a117b72 100644 --- a/pkg/services/ngalert/store/instance_database_test.go +++ b/pkg/services/ngalert/store/instance_database_test.go @@ -3,27 +3,15 @@ package store_test import ( "context" "testing" - "time" - - "github.com/grafana/grafana/pkg/services/ngalert/models" - "github.com/grafana/grafana/pkg/services/ngalert/store" - "github.com/grafana/grafana/pkg/services/ngalert/tests" "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/services/ngalert/tests" ) const baseIntervalSeconds = 10 -// Every time this is called, time advances by 1 second. -func mockTimeNow() { - var timeSeed int64 - store.TimeNow = func() time.Time { - fakeNow := time.Unix(timeSeed, 0).UTC() - timeSeed++ - return fakeNow - } -} - func TestIntegrationAlertInstanceOperations(t *testing.T) { if testing.Short() { t.Skip("skipping integration test")