From adc0cbf17652351e1a6880e92f7839f8ec5820a7 Mon Sep 17 00:00:00 2001 From: ying-jeanne <74549700+ying-jeanne@users.noreply.github.com> Date: Tue, 22 Mar 2022 12:20:57 +0100 Subject: [PATCH] remove global variable in annotation (#46746) * remove global varaible in annotation * remove todo * replace intransaction with withdbtransaction * fix typo --- pkg/api/annotations.go | 18 +- pkg/api/annotations_test.go | 4 +- pkg/services/annotations/annotations.go | 4 +- .../comments/commentmodel/permissions.go | 4 +- pkg/services/ngalert/store/testing.go | 4 +- pkg/services/sqlstore/annotation.go | 229 +++++++++--------- pkg/services/sqlstore/annotation_test.go | 44 ++-- pkg/services/sqlstore/sqlstore.go | 2 +- 8 files changed, 157 insertions(+), 152 deletions(-) diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index c64be7a3075..4f0d354a1b0 100644 --- a/pkg/api/annotations.go +++ b/pkg/api/annotations.go @@ -35,7 +35,7 @@ func (hs *HTTPServer) GetAnnotations(c *models.ReqContext) response.Response { repo := annotations.GetRepository() - items, err := repo.Find(query) + items, err := repo.Find(c.Req.Context(), query) if err != nil { return response.Error(500, "Failed to get annotations", err) } @@ -192,7 +192,7 @@ func (hs *HTTPServer) UpdateAnnotation(c *models.ReqContext) response.Response { repo := annotations.GetRepository() - annotation, resp := findAnnotationByID(repo, annotationID, c.OrgId) + annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.OrgId) if resp != nil { return resp } @@ -220,7 +220,7 @@ func (hs *HTTPServer) UpdateAnnotation(c *models.ReqContext) response.Response { Tags: cmd.Tags, } - if err := repo.Update(&item); err != nil { + if err := repo.Update(c.Req.Context(), &item); err != nil { return response.Error(500, "Failed to update annotation", err) } @@ -239,7 +239,7 @@ func (hs *HTTPServer) PatchAnnotation(c *models.ReqContext) response.Response { repo := annotations.GetRepository() - annotation, resp := findAnnotationByID(repo, annotationID, c.OrgId) + annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.OrgId) if resp != nil { return resp } @@ -282,7 +282,7 @@ func (hs *HTTPServer) PatchAnnotation(c *models.ReqContext) response.Response { existing.EpochEnd = cmd.TimeEnd } - if err := repo.Update(&existing); err != nil { + if err := repo.Update(c.Req.Context(), &existing); err != nil { return response.Error(500, "Failed to update annotation", err) } @@ -318,7 +318,7 @@ func (hs *HTTPServer) DeleteAnnotationByID(c *models.ReqContext) response.Respon repo := annotations.GetRepository() - annotation, resp := findAnnotationByID(repo, annotationID, c.OrgId) + annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.OrgId) if resp != nil { return resp } @@ -360,8 +360,8 @@ func canSaveOrganizationAnnotation(c *models.ReqContext) bool { return c.SignedInUser.HasRole(models.ROLE_EDITOR) } -func findAnnotationByID(repo annotations.Repository, annotationID int64, orgID int64) (*annotations.ItemDTO, response.Response) { - items, err := repo.Find(&annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgID}) +func findAnnotationByID(ctx context.Context, repo annotations.Repository, annotationID int64, orgID int64) (*annotations.ItemDTO, response.Response) { + items, err := repo.Find(ctx, &annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgID}) if err != nil { return nil, response.Error(500, "Failed to find annotation", err) @@ -406,7 +406,7 @@ func AnnotationTypeScopeResolver() (string, accesscontrol.AttributeScopeResolveF return "", accesscontrol.ErrInvalidScope } - annotation, resp := findAnnotationByID(annotations.GetRepository(), int64(annotationId), orgID) + annotation, resp := findAnnotationByID(ctx, annotations.GetRepository(), int64(annotationId), orgID) if resp != nil { return "", err } diff --git a/pkg/api/annotations_test.go b/pkg/api/annotations_test.go index 02cb0b530af..6abe17e379c 100644 --- a/pkg/api/annotations_test.go +++ b/pkg/api/annotations_test.go @@ -248,10 +248,10 @@ func (repo *fakeAnnotationsRepo) Save(item *annotations.Item) error { item.Id = 1 return nil } -func (repo *fakeAnnotationsRepo) Update(item *annotations.Item) error { +func (repo *fakeAnnotationsRepo) Update(_ context.Context, item *annotations.Item) error { return nil } -func (repo *fakeAnnotationsRepo) Find(query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) { +func (repo *fakeAnnotationsRepo) Find(_ context.Context, query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) { if annotation, has := repo.annotations[query.AnnotationId]; has { return []*annotations.ItemDTO{&annotation}, nil } diff --git a/pkg/services/annotations/annotations.go b/pkg/services/annotations/annotations.go index 6ba1c714e4f..9798e4a42b6 100644 --- a/pkg/services/annotations/annotations.go +++ b/pkg/services/annotations/annotations.go @@ -14,8 +14,8 @@ var ( type Repository interface { Save(item *Item) error - Update(item *Item) error - Find(query *ItemQuery) ([]*ItemDTO, error) + Update(ctx context.Context, item *Item) error + Find(ctx context.Context, query *ItemQuery) ([]*ItemDTO, error) Delete(params *DeleteParams) error FindTags(query *TagsQuery) (FindTagsResult, error) } diff --git a/pkg/services/comments/commentmodel/permissions.go b/pkg/services/comments/commentmodel/permissions.go index c35f927538c..bdfed834f44 100644 --- a/pkg/services/comments/commentmodel/permissions.go +++ b/pkg/services/comments/commentmodel/permissions.go @@ -62,7 +62,7 @@ func (c *PermissionChecker) CheckReadPermissions(ctx context.Context, orgId int6 if err != nil { return false, nil } - items, err := repo.Find(&annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgId}) + items, err := repo.Find(ctx, &annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgId}) if err != nil || len(items) != 1 { return false, nil } @@ -109,7 +109,7 @@ func (c *PermissionChecker) CheckWritePermissions(ctx context.Context, orgId int if err != nil { return false, nil } - items, err := repo.Find(&annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgId}) + items, err := repo.Find(ctx, &annotations.ItemQuery{AnnotationId: annotationID, OrgId: orgId}) if err != nil || len(items) != 1 { return false, nil } diff --git a/pkg/services/ngalert/store/testing.go b/pkg/services/ngalert/store/testing.go index 46f33565cfc..8d54fe57e58 100644 --- a/pkg/services/ngalert/store/testing.go +++ b/pkg/services/ngalert/store/testing.go @@ -505,11 +505,11 @@ func (repo *FakeAnnotationsRepo) Save(item *annotations.Item) error { return nil } -func (repo *FakeAnnotationsRepo) Update(item *annotations.Item) error { +func (repo *FakeAnnotationsRepo) Update(_ context.Context, item *annotations.Item) error { return nil } -func (repo *FakeAnnotationsRepo) Find(query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) { +func (repo *FakeAnnotationsRepo) Find(_ context.Context, query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) { annotations := []*annotations.ItemDTO{{Id: 1}} return annotations, nil } diff --git a/pkg/services/sqlstore/annotation.go b/pkg/services/sqlstore/annotation.go index 7efa1aca788..2dd380adcda 100644 --- a/pkg/services/sqlstore/annotation.go +++ b/pkg/services/sqlstore/annotation.go @@ -2,6 +2,7 @@ package sqlstore import ( "bytes" + "context" "errors" "fmt" "strings" @@ -29,6 +30,7 @@ func validateTimeRange(item *annotations.Item) error { } type SQLAnnotationRepo struct { + sql *SQLStore } func (r *SQLAnnotationRepo) Save(item *annotations.Item) error { @@ -64,8 +66,8 @@ func (r *SQLAnnotationRepo) Save(item *annotations.Item) error { }) } -func (r *SQLAnnotationRepo) Update(item *annotations.Item) error { - return inTransaction(func(sess *DBSession) error { +func (r *SQLAnnotationRepo) Update(ctx context.Context, item *annotations.Item) error { + return r.sql.WithTransactionalDbSession(ctx, func(sess *DBSession) error { var ( isExist bool err error @@ -117,121 +119,124 @@ func (r *SQLAnnotationRepo) Update(item *annotations.Item) error { }) } -func (r *SQLAnnotationRepo) Find(query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) { +func (r *SQLAnnotationRepo) Find(ctx context.Context, query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) { var sql bytes.Buffer params := make([]interface{}, 0) - - sql.WriteString(` - SELECT - annotation.id, - annotation.epoch as time, - annotation.epoch_end as time_end, - annotation.dashboard_id, - annotation.panel_id, - annotation.new_state, - annotation.prev_state, - annotation.alert_id, - annotation.text, - annotation.tags, - annotation.data, - annotation.created, - annotation.updated, - usr.email, - usr.login, - alert.name as alert_name - FROM annotation - LEFT OUTER JOIN ` + dialect.Quote("user") + ` as usr on usr.id = annotation.user_id - LEFT OUTER JOIN alert on alert.id = annotation.alert_id - INNER JOIN ( - SELECT a.id from annotation a - `) - - sql.WriteString(`WHERE a.org_id = ?`) - params = append(params, query.OrgId) - - if query.AnnotationId != 0 { - // fmt.Print("annotation query") - sql.WriteString(` AND a.id = ?`) - params = append(params, query.AnnotationId) - } - - if query.AlertId != 0 { - sql.WriteString(` AND a.alert_id = ?`) - params = append(params, query.AlertId) - } - - if query.DashboardId != 0 { - sql.WriteString(` AND a.dashboard_id = ?`) - params = append(params, query.DashboardId) - } - - if query.PanelId != 0 { - sql.WriteString(` AND a.panel_id = ?`) - params = append(params, query.PanelId) - } - - if query.UserId != 0 { - sql.WriteString(` AND a.user_id = ?`) - params = append(params, query.UserId) - } - - if query.From > 0 && query.To > 0 { - sql.WriteString(` AND a.epoch <= ? AND a.epoch_end >= ?`) - params = append(params, query.To, query.From) - } - - if query.Type == "alert" { - sql.WriteString(` AND a.alert_id > 0`) - } else if query.Type == "annotation" { - sql.WriteString(` AND a.alert_id = 0`) - } - - if len(query.Tags) > 0 { - keyValueFilters := []string{} - - tags := models.ParseTagPairs(query.Tags) - for _, tag := range tags { - if tag.Value == "" { - keyValueFilters = append(keyValueFilters, "(tag."+dialect.Quote("key")+" = ?)") - params = append(params, tag.Key) - } else { - keyValueFilters = append(keyValueFilters, "(tag."+dialect.Quote("key")+" = ? AND tag."+dialect.Quote("value")+" = ?)") - params = append(params, tag.Key, tag.Value) - } - } - - if len(tags) > 0 { - tagsSubQuery := fmt.Sprintf(` - SELECT SUM(1) FROM annotation_tag at - INNER JOIN tag on tag.id = at.tag_id - WHERE at.annotation_id = a.id - AND ( - %s - ) - `, strings.Join(keyValueFilters, " OR ")) - - if query.MatchAny { - sql.WriteString(fmt.Sprintf(" AND (%s) > 0 ", tagsSubQuery)) - } else { - sql.WriteString(fmt.Sprintf(" AND (%s) = %d ", tagsSubQuery, len(tags))) - } - } - } - - if query.Limit == 0 { - query.Limit = 100 - } - - // order of ORDER BY arguments match the order of a sql index for performance - sql.WriteString(" ORDER BY a.org_id, a.epoch_end DESC, a.epoch DESC" + dialect.Limit(query.Limit) + " ) dt on dt.id = annotation.id") - items := make([]*annotations.ItemDTO, 0) + err := r.sql.WithDbSession(ctx, func(sess *DBSession) error { + sql.WriteString(` + SELECT + annotation.id, + annotation.epoch as time, + annotation.epoch_end as time_end, + annotation.dashboard_id, + annotation.panel_id, + annotation.new_state, + annotation.prev_state, + annotation.alert_id, + annotation.text, + annotation.tags, + annotation.data, + annotation.created, + annotation.updated, + usr.email, + usr.login, + alert.name as alert_name + FROM annotation + LEFT OUTER JOIN ` + dialect.Quote("user") + ` as usr on usr.id = annotation.user_id + LEFT OUTER JOIN alert on alert.id = annotation.alert_id + INNER JOIN ( + SELECT a.id from annotation a + `) - if err := x.SQL(sql.String(), params...).Find(&items); err != nil { - return nil, err - } + sql.WriteString(`WHERE a.org_id = ?`) + params = append(params, query.OrgId) - return items, nil + if query.AnnotationId != 0 { + // fmt.Print("annotation query") + sql.WriteString(` AND a.id = ?`) + params = append(params, query.AnnotationId) + } + + if query.AlertId != 0 { + sql.WriteString(` AND a.alert_id = ?`) + params = append(params, query.AlertId) + } + + if query.DashboardId != 0 { + sql.WriteString(` AND a.dashboard_id = ?`) + params = append(params, query.DashboardId) + } + + if query.PanelId != 0 { + sql.WriteString(` AND a.panel_id = ?`) + params = append(params, query.PanelId) + } + + if query.UserId != 0 { + sql.WriteString(` AND a.user_id = ?`) + params = append(params, query.UserId) + } + + if query.From > 0 && query.To > 0 { + sql.WriteString(` AND a.epoch <= ? AND a.epoch_end >= ?`) + params = append(params, query.To, query.From) + } + + if query.Type == "alert" { + sql.WriteString(` AND a.alert_id > 0`) + } else if query.Type == "annotation" { + sql.WriteString(` AND a.alert_id = 0`) + } + + if len(query.Tags) > 0 { + keyValueFilters := []string{} + + tags := models.ParseTagPairs(query.Tags) + for _, tag := range tags { + if tag.Value == "" { + keyValueFilters = append(keyValueFilters, "(tag."+dialect.Quote("key")+" = ?)") + params = append(params, tag.Key) + } else { + keyValueFilters = append(keyValueFilters, "(tag."+dialect.Quote("key")+" = ? AND tag."+dialect.Quote("value")+" = ?)") + params = append(params, tag.Key, tag.Value) + } + } + + if len(tags) > 0 { + tagsSubQuery := fmt.Sprintf(` + SELECT SUM(1) FROM annotation_tag at + INNER JOIN tag on tag.id = at.tag_id + WHERE at.annotation_id = a.id + AND ( + %s + ) + `, strings.Join(keyValueFilters, " OR ")) + + if query.MatchAny { + sql.WriteString(fmt.Sprintf(" AND (%s) > 0 ", tagsSubQuery)) + } else { + sql.WriteString(fmt.Sprintf(" AND (%s) = %d ", tagsSubQuery, len(tags))) + } + } + } + + if query.Limit == 0 { + query.Limit = 100 + } + + // order of ORDER BY arguments match the order of a sql index for performance + sql.WriteString(" ORDER BY a.org_id, a.epoch_end DESC, a.epoch DESC" + dialect.Limit(query.Limit) + " ) dt on dt.id = annotation.id") + + if err := sess.SQL(sql.String(), params...).Find(&items); err != nil { + items = nil + return err + } + return nil + }, + ) + + return items, err } func (r *SQLAnnotationRepo) Delete(params *annotations.DeleteParams) error { diff --git a/pkg/services/sqlstore/annotation_test.go b/pkg/services/sqlstore/annotation_test.go index 20be79ca2d9..6de716258d4 100644 --- a/pkg/services/sqlstore/annotation_test.go +++ b/pkg/services/sqlstore/annotation_test.go @@ -4,10 +4,10 @@ package sqlstore import ( - "testing" - + "context" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "testing" "github.com/grafana/grafana/pkg/services/annotations" ) @@ -15,8 +15,9 @@ import ( func TestAnnotations(t *testing.T) { mockTimeNow() defer resetTimeNow() - InitTestDB(t) + repo := SQLAnnotationRepo{} + repo.sql = InitTestDB(t) t.Run("Testing annotation create, read, update and delete", func(t *testing.T) { t.Cleanup(func() { @@ -79,9 +80,8 @@ func TestAnnotations(t *testing.T) { err = repo.Save(globalAnnotation2) require.NoError(t, err) assert.Greater(t, globalAnnotation2.Id, int64(0)) - t.Run("Can query for annotation by dashboard id", func(t *testing.T) { - items, err := repo.Find(&annotations.ItemQuery{ + items, err := repo.Find(context.Background(), &annotations.ItemQuery{ OrgId: 1, DashboardId: 1, From: 0, @@ -99,7 +99,7 @@ func TestAnnotations(t *testing.T) { }) t.Run("Can query for annotation by id", func(t *testing.T) { - items, err := repo.Find(&annotations.ItemQuery{ + items, err := repo.Find(context.Background(), &annotations.ItemQuery{ OrgId: 1, AnnotationId: annotation2.Id, }) @@ -109,7 +109,7 @@ func TestAnnotations(t *testing.T) { }) t.Run("Should not find any when item is outside time range", func(t *testing.T) { - items, err := repo.Find(&annotations.ItemQuery{ + items, err := repo.Find(context.Background(), &annotations.ItemQuery{ OrgId: 1, DashboardId: 1, From: 12, @@ -120,7 +120,7 @@ func TestAnnotations(t *testing.T) { }) t.Run("Should not find one when tag filter does not match", func(t *testing.T) { - items, err := repo.Find(&annotations.ItemQuery{ + items, err := repo.Find(context.Background(), &annotations.ItemQuery{ OrgId: 1, DashboardId: 1, From: 1, @@ -132,7 +132,7 @@ func TestAnnotations(t *testing.T) { }) t.Run("Should not find one when type filter does not match", func(t *testing.T) { - items, err := repo.Find(&annotations.ItemQuery{ + items, err := repo.Find(context.Background(), &annotations.ItemQuery{ OrgId: 1, DashboardId: 1, From: 1, @@ -144,7 +144,7 @@ func TestAnnotations(t *testing.T) { }) t.Run("Should find one when all tag filters does match", func(t *testing.T) { - items, err := repo.Find(&annotations.ItemQuery{ + items, err := repo.Find(context.Background(), &annotations.ItemQuery{ OrgId: 1, DashboardId: 1, From: 1, @@ -156,7 +156,7 @@ func TestAnnotations(t *testing.T) { }) t.Run("Should find two annotations using partial match", func(t *testing.T) { - items, err := repo.Find(&annotations.ItemQuery{ + items, err := repo.Find(context.Background(), &annotations.ItemQuery{ OrgId: 1, From: 1, To: 25, @@ -168,7 +168,7 @@ func TestAnnotations(t *testing.T) { }) t.Run("Should find one when all key value tag filters does match", func(t *testing.T) { - items, err := repo.Find(&annotations.ItemQuery{ + items, err := repo.Find(context.Background(), &annotations.ItemQuery{ OrgId: 1, DashboardId: 1, From: 1, @@ -186,11 +186,11 @@ func TestAnnotations(t *testing.T) { From: 0, To: 15, } - items, err := repo.Find(query) + items, err := repo.Find(context.Background(), query) require.NoError(t, err) annotationId := items[0].Id - err = repo.Update(&annotations.Item{ + err = repo.Update(context.Background(), &annotations.Item{ Id: annotationId, OrgId: 1, Text: "something new", @@ -198,7 +198,7 @@ func TestAnnotations(t *testing.T) { }) require.NoError(t, err) - items, err = repo.Find(query) + items, err = repo.Find(context.Background(), query) require.NoError(t, err) assert.Equal(t, annotationId, items[0].Id) @@ -213,11 +213,11 @@ func TestAnnotations(t *testing.T) { From: 0, To: 15, } - items, err := repo.Find(query) + items, err := repo.Find(context.Background(), query) require.NoError(t, err) annotationId := items[0].Id - err = repo.Update(&annotations.Item{ + err = repo.Update(context.Background(), &annotations.Item{ Id: annotationId, OrgId: 1, Text: "something new", @@ -225,7 +225,7 @@ func TestAnnotations(t *testing.T) { }) require.NoError(t, err) - items, err = repo.Find(query) + items, err = repo.Find(context.Background(), query) require.NoError(t, err) assert.Equal(t, annotationId, items[0].Id) @@ -241,14 +241,14 @@ func TestAnnotations(t *testing.T) { From: 0, To: 15, } - items, err := repo.Find(query) + items, err := repo.Find(context.Background(), query) require.NoError(t, err) annotationId := items[0].Id err = repo.Delete(&annotations.DeleteParams{Id: annotationId, OrgId: 1}) require.NoError(t, err) - items, err = repo.Find(query) + items, err = repo.Find(context.Background(), query) require.NoError(t, err) assert.Empty(t, items) }) @@ -270,7 +270,7 @@ func TestAnnotations(t *testing.T) { OrgId: 1, AnnotationId: annotation3.Id, } - items, err := repo.Find(query) + items, err := repo.Find(context.Background(), query) require.NoError(t, err) dashboardId := items[0].DashboardId @@ -278,7 +278,7 @@ func TestAnnotations(t *testing.T) { err = repo.Delete(&annotations.DeleteParams{DashboardId: dashboardId, PanelId: panelId, OrgId: 1}) require.NoError(t, err) - items, err = repo.Find(query) + items, err = repo.Find(context.Background(), query) require.NoError(t, err) assert.Empty(t, items) }) diff --git a/pkg/services/sqlstore/sqlstore.go b/pkg/services/sqlstore/sqlstore.go index 0c50d28161c..e1881e8c4c3 100644 --- a/pkg/services/sqlstore/sqlstore.go +++ b/pkg/services/sqlstore/sqlstore.go @@ -109,7 +109,7 @@ func newSQLStore(cfg *setting.Cfg, cacheService *localcache.CacheService, b bus. dialect = ss.Dialect // Init repo instances - annotations.SetRepository(&SQLAnnotationRepo{}) + annotations.SetRepository(&SQLAnnotationRepo{sql: ss}) annotations.SetAnnotationCleaner(&AnnotationCleanupService{batchSize: ss.Cfg.AnnotationCleanupJobBatchSize, log: log.New("annotationcleaner")}) ss.Bus.SetTransactionManager(ss)