public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Chore: SQL store split for annotations (#55089)

Browse Source 
* Chore: SQL store split for annotations

* Apply suggestion from code review
This commit is contained in:
Sofia Papagiannaki
2022-09-19 10:54:37 +03:00
committed by GitHub
parent 469f915b8c
commit 754eea20b3
44 changed files with 499 additions and 467 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/annotations.go
+15 -31
View File
@@ -63,9 +63,7 @@ func (hs *HTTPServer) GetAnnotations(c *models.ReqContext) response.Response {
}
}
repo := annotations.GetRepository()
items, err := repo.Find(c.Req.Context(), query)
items, err := hs.annotationsRepo.Find(c.Req.Context(), query)
if err != nil {
return response.Error(500, "Failed to get annotations", err)
}
@@ -135,8 +133,6 @@ func (hs *HTTPServer) PostAnnotation(c *models.ReqContext) response.Response {
return dashboardGuardianResponse(err)
}
repo := annotations.GetRepository()
if cmd.Text == "" {
err := &AnnotationError{"text field should not be empty"}
return response.Error(400, "Failed to save annotation", err)
@@ -154,7 +150,7 @@ func (hs *HTTPServer) PostAnnotation(c *models.ReqContext) response.Response {
Tags: cmd.Tags,
}
if err := repo.Save(&item); err != nil {
if err := hs.annotationsRepo.Save(c.Req.Context(), &item); err != nil {
if errors.Is(err, annotations.ErrTimerangeMissing) {
return response.Error(400, "Failed to save annotation", err)
}
@@ -194,8 +190,6 @@ func (hs *HTTPServer) PostGraphiteAnnotation(c *models.ReqContext) response.Resp
if err := web.Bind(c.Req, &cmd); err != nil {
return response.Error(http.StatusBadRequest, "bad request data", err)
}
repo := annotations.GetRepository()
if cmd.What == "" {
err := &AnnotationError{"what field should not be empty"}
return response.Error(400, "Failed to save Graphite annotation", err)
@@ -234,7 +228,7 @@ func (hs *HTTPServer) PostGraphiteAnnotation(c *models.ReqContext) response.Resp
Tags: tagsArray,
}
if err := repo.Save(&item); err != nil {
if err := hs.annotationsRepo.Save(context.Background(), &item); err != nil {
return response.Error(500, "Failed to save Graphite annotation", err)
}
@@ -267,9 +261,7 @@ func (hs *HTTPServer) UpdateAnnotation(c *models.ReqContext) response.Response {
return response.Error(http.StatusBadRequest, "annotationId is invalid", err)
}
repo := annotations.GetRepository()
annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.SignedInUser)
annotation, resp := findAnnotationByID(c.Req.Context(), hs.annotationsRepo, annotationID, c.SignedInUser)
if resp != nil {
return resp
}
@@ -288,7 +280,7 @@ func (hs *HTTPServer) UpdateAnnotation(c *models.ReqContext) response.Response {
Tags: cmd.Tags,
}
if err := repo.Update(c.Req.Context(), &item); err != nil {
if err := hs.annotationsRepo.Update(c.Req.Context(), &item); err != nil {
return response.Error(500, "Failed to update annotation", err)
}
@@ -319,9 +311,7 @@ func (hs *HTTPServer) PatchAnnotation(c *models.ReqContext) response.Response {
return response.Error(http.StatusBadRequest, "annotationId is invalid", err)
}
repo := annotations.GetRepository()
annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.SignedInUser)
annotation, resp := findAnnotationByID(c.Req.Context(), hs.annotationsRepo, annotationID, c.SignedInUser)
if resp != nil {
return resp
}
@@ -356,7 +346,7 @@ func (hs *HTTPServer) PatchAnnotation(c *models.ReqContext) response.Response {
existing.EpochEnd = cmd.TimeEnd
}
if err := repo.Update(c.Req.Context(), &existing); err != nil {
if err := hs.annotationsRepo.Update(c.Req.Context(), &existing); err != nil {
return response.Error(500, "Failed to update annotation", err)
}
@@ -391,7 +381,6 @@ func (hs *HTTPServer) MassDeleteAnnotations(c *models.ReqContext) response.Respo
return response.Error(http.StatusBadRequest, "bad request data", err)
}
repo := annotations.GetRepository()
var deleteParams *annotations.DeleteParams
// validations only for RBAC. A user can mass delete all annotations in a (dashboard + panel) or a specific annotation
@@ -400,7 +389,7 @@ func (hs *HTTPServer) MassDeleteAnnotations(c *models.ReqContext) response.Respo
var dashboardId int64
if cmd.AnnotationId != 0 {
annotation, respErr := findAnnotationByID(c.Req.Context(), repo, cmd.AnnotationId, c.SignedInUser)
annotation, respErr := findAnnotationByID(c.Req.Context(), hs.annotationsRepo, cmd.AnnotationId, c.SignedInUser)
if respErr != nil {
return respErr
}
@@ -431,7 +420,7 @@ func (hs *HTTPServer) MassDeleteAnnotations(c *models.ReqContext) response.Respo
}
}
err = repo.Delete(c.Req.Context(), deleteParams)
err = hs.annotationsRepo.Delete(c.Req.Context(), deleteParams)
if err != nil {
return response.Error(500, "Failed to delete annotations", err)
@@ -454,9 +443,7 @@ func (hs *HTTPServer) GetAnnotationByID(c *models.ReqContext) response.Response
return response.Error(http.StatusBadRequest, "annotationId is invalid", err)
}
repo := annotations.GetRepository()
annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.SignedInUser)
annotation, resp := findAnnotationByID(c.Req.Context(), hs.annotationsRepo, annotationID, c.SignedInUser)
if resp != nil {
return resp
}
@@ -485,9 +472,7 @@ func (hs *HTTPServer) DeleteAnnotationByID(c *models.ReqContext) response.Respon
return response.Error(http.StatusBadRequest, "annotationId is invalid", err)
}
repo := annotations.GetRepository()
annotation, resp := findAnnotationByID(c.Req.Context(), repo, annotationID, c.SignedInUser)
annotation, resp := findAnnotationByID(c.Req.Context(), hs.annotationsRepo, annotationID, c.SignedInUser)
if resp != nil {
return resp
}
@@ -496,7 +481,7 @@ func (hs *HTTPServer) DeleteAnnotationByID(c *models.ReqContext) response.Respon
return dashboardGuardianResponse(err)
}
err = repo.Delete(c.Req.Context(), &annotations.DeleteParams{
err = hs.annotationsRepo.Delete(c.Req.Context(), &annotations.DeleteParams{
OrgId: c.OrgID,
Id: annotationID,
})
@@ -563,8 +548,7 @@ func (hs *HTTPServer) GetAnnotationTags(c *models.ReqContext) response.Response
Limit: c.QueryInt64("limit"),
}
repo := annotations.GetRepository()
result, err := repo.FindTags(c.Req.Context(), query)
result, err := hs.annotationsRepo.FindTags(c.Req.Context(), query)
if err != nil {
return response.Error(500, "Failed to find annotation tags", err)
}
@@ -575,7 +559,7 @@ func (hs *HTTPServer) GetAnnotationTags(c *models.ReqContext) response.Response
// AnnotationTypeScopeResolver provides an ScopeAttributeResolver able to
// resolve annotation types. Scope "annotations:id:<id>" will be translated to "annotations:type:<type>,
// where <type> is the type of annotation with id <id>.
func AnnotationTypeScopeResolver() (string, accesscontrol.ScopeAttributeResolver) {
func AnnotationTypeScopeResolver(annotationsRepo annotations.Repository) (string, accesscontrol.ScopeAttributeResolver) {
prefix := accesscontrol.ScopeAnnotationsProvider.GetResourceScope("")
return prefix, accesscontrol.ScopeAttributeResolverFunc(func(ctx context.Context, orgID int64, initialScope string) ([]string, error) {
scopeParts := strings.Split(initialScope, ":")
@@ -601,7 +585,7 @@ func AnnotationTypeScopeResolver() (string, accesscontrol.ScopeAttributeResolver
},
}
annotation, resp := findAnnotationByID(ctx, annotations.GetRepository(), int64(annotationId), tempUser)
annotation, resp := findAnnotationByID(ctx, annotationsRepo, int64(annotationId), tempUser)
if resp != nil {
return nil, errors.New("could not resolve annotation type")
}
pkg/api/annotations_test.go
+10 -90
View File
@@ -17,6 +17,7 @@ import (
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/annotations"
"github.com/grafana/grafana/pkg/services/annotations/annotationstest"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/guardian"
"github.com/grafana/grafana/pkg/services/org"
@@ -73,8 +74,6 @@ func TestAnnotationsAPIEndpoint(t *testing.T) {
mock := mockstore.NewSQLStoreMock()
loggedInUserScenarioWithRole(t, "When calling DELETE on", "DELETE", "/api/annotations/1",
"/api/annotations/:annotationId", role, func(sc *scenarioContext) {
fakeAnnoRepo = NewFakeAnnotationsRepo()
annotations.SetRepository(fakeAnnoRepo)
sc.handlerFunc = hs.DeleteAnnotationByID
sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec()
assert.Equal(t, 403, sc.resp.Code)
@@ -103,8 +102,6 @@ func TestAnnotationsAPIEndpoint(t *testing.T) {
mock := mockstore.NewSQLStoreMock()
loggedInUserScenarioWithRole(t, "When calling DELETE on", "DELETE", "/api/annotations/1",
"/api/annotations/:annotationId", role, func(sc *scenarioContext) {
fakeAnnoRepo = NewFakeAnnotationsRepo()
annotations.SetRepository(fakeAnnoRepo)
sc.handlerFunc = hs.DeleteAnnotationByID
sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec()
assert.Equal(t, 200, sc.resp.Code)
@@ -178,8 +175,6 @@ func TestAnnotationsAPIEndpoint(t *testing.T) {
loggedInUserScenarioWithRole(t, "When calling DELETE on", "DELETE", "/api/annotations/1",
"/api/annotations/:annotationId", role, func(sc *scenarioContext) {
setUpACL()
fakeAnnoRepo = NewFakeAnnotationsRepo()
annotations.SetRepository(fakeAnnoRepo)
sc.handlerFunc = hs.DeleteAnnotationByID
sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec()
assert.Equal(t, 403, sc.resp.Code)
@@ -211,8 +206,6 @@ func TestAnnotationsAPIEndpoint(t *testing.T) {
loggedInUserScenarioWithRole(t, "When calling DELETE on", "DELETE", "/api/annotations/1",
"/api/annotations/:annotationId", role, func(sc *scenarioContext) {
setUpACL()
fakeAnnoRepo = NewFakeAnnotationsRepo()
annotations.SetRepository(fakeAnnoRepo)
sc.handlerFunc = hs.DeleteAnnotationByID
sc.fakeReqWithParams("DELETE", sc.url, map[string]string{}).exec()
assert.Equal(t, 200, sc.resp.Code)
@@ -286,59 +279,6 @@ func TestAnnotationsAPIEndpoint(t *testing.T) {
})
}
type fakeAnnotationsRepo struct {
annotations map[int64]annotations.Item
}
func NewFakeAnnotationsRepo() *fakeAnnotationsRepo {
return &fakeAnnotationsRepo{
annotations: map[int64]annotations.Item{},
}
}
func (repo *fakeAnnotationsRepo) Delete(_ context.Context, params *annotations.DeleteParams) error {
if params.Id != 0 {
delete(repo.annotations, params.Id)
} else {
for _, v := range repo.annotations {
if params.DashboardId == v.DashboardId && params.PanelId == v.PanelId {
delete(repo.annotations, v.Id)
}
}
}
return nil
}
func (repo *fakeAnnotationsRepo) Save(item *annotations.Item) error {
if item.Id == 0 {
item.Id = int64(len(repo.annotations) + 1)
}
repo.annotations[item.Id] = *item
return nil
}
func (repo *fakeAnnotationsRepo) Update(_ context.Context, item *annotations.Item) error {
return nil
}
func (repo *fakeAnnotationsRepo) Find(_ context.Context, query *annotations.ItemQuery) ([]*annotations.ItemDTO, error) {
if annotation, has := repo.annotations[query.AnnotationId]; has {
return []*annotations.ItemDTO{{Id: annotation.Id, DashboardId: annotation.DashboardId}}, nil
}
annotations := []*annotations.ItemDTO{{Id: 1, DashboardId: 0}}
return annotations, nil
}
func (repo *fakeAnnotationsRepo) FindTags(_ context.Context, query *annotations.TagsQuery) (annotations.FindTagsResult, error) {
result := annotations.FindTagsResult{
Tags: []*annotations.TagsDTO{},
}
return result, nil
}
func (repo *fakeAnnotationsRepo) LoadItems() {
}
var fakeAnnoRepo *fakeAnnotationsRepo
func postAnnotationScenario(t *testing.T, desc string, url string, routePattern string, role org.RoleType,
cmd dtos.PostAnnotationsCmd, store sqlstore.Store, dashSvc dashboards.DashboardService, fn scenarioFunc) {
t.Run(fmt.Sprintf("%s %s", desc, url), func(t *testing.T) {
@@ -358,9 +298,6 @@ func postAnnotationScenario(t *testing.T, desc string, url string, routePattern
return hs.PostAnnotation(c)
})
fakeAnnoRepo = NewFakeAnnotationsRepo()
annotations.SetRepository(fakeAnnoRepo)
sc.m.Post(routePattern, sc.defaultHandler)
fn(sc)
@@ -387,9 +324,6 @@ func putAnnotationScenario(t *testing.T, desc string, url string, routePattern s
return hs.UpdateAnnotation(c)
})
fakeAnnoRepo = NewFakeAnnotationsRepo()
annotations.SetRepository(fakeAnnoRepo)
sc.m.Put(routePattern, sc.defaultHandler)
fn(sc)
@@ -415,9 +349,6 @@ func patchAnnotationScenario(t *testing.T, desc string, url string, routePattern
return hs.PatchAnnotation(c)
})
fakeAnnoRepo = NewFakeAnnotationsRepo()
annotations.SetRepository(fakeAnnoRepo)
sc.m.Patch(routePattern, sc.defaultHandler)
fn(sc)
@@ -443,9 +374,6 @@ func deleteAnnotationsScenario(t *testing.T, desc string, url string, routePatte
return hs.MassDeleteAnnotations(c)
})
fakeAnnoRepo = NewFakeAnnotationsRepo()
annotations.SetRepository(fakeAnnoRepo)
sc.m.Post(routePattern, sc.defaultHandler)
fn(sc)
@@ -461,11 +389,8 @@ func TestAPI_Annotations_AccessControl(t *testing.T) {
dashboardAnnotation := &annotations.Item{Id: 1, DashboardId: 1}
organizationAnnotation := &annotations.Item{Id: 2, DashboardId: 0}
fakeAnnoRepo = NewFakeAnnotationsRepo()
_ = fakeAnnoRepo.Save(dashboardAnnotation)
_ = fakeAnnoRepo.Save(organizationAnnotation)
annotations.SetRepository(fakeAnnoRepo)
_ = sc.hs.annotationsRepo.Save(context.Background(), dashboardAnnotation)
_ = sc.hs.annotationsRepo.Save(context.Background(), organizationAnnotation)
postOrganizationCmd := dtos.PostAnnotationsCmd{
Time: 1000,
@@ -788,7 +713,7 @@ func TestAPI_Annotations_AccessControl(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
setUpRBACGuardian(t)
sc.acmock.
RegisterScopeAttributeResolver(AnnotationTypeScopeResolver())
RegisterScopeAttributeResolver(AnnotationTypeScopeResolver(sc.hs.annotationsRepo))
setAccessControlPermissions(sc.acmock, tt.args.permissions, sc.initCtx.OrgID)
r := callAPI(sc.server, tt.args.method, tt.args.url, tt.args.body, t)
@@ -835,13 +760,11 @@ func TestService_AnnotationTypeScopeResolver(t *testing.T) {
dashboardAnnotation := annotations.Item{Id: 1, DashboardId: 1}
organizationAnnotation := annotations.Item{Id: 2}
fakeAnnoRepo = NewFakeAnnotationsRepo()
_ = fakeAnnoRepo.Save(&dashboardAnnotation)
_ = fakeAnnoRepo.Save(&organizationAnnotation)
fakeAnnoRepo := annotationstest.NewFakeAnnotationsRepo()
_ = fakeAnnoRepo.Save(context.Background(), &dashboardAnnotation)
_ = fakeAnnoRepo.Save(context.Background(), &organizationAnnotation)
annotations.SetRepository(fakeAnnoRepo)
prefix, resolver := AnnotationTypeScopeResolver()
prefix, resolver := AnnotationTypeScopeResolver(fakeAnnoRepo)
require.Equal(t, "annotations:id:", prefix)
for _, tc := range testCases {
@@ -986,11 +909,8 @@ func TestAPI_MassDeleteAnnotations_AccessControl(t *testing.T) {
dashboardAnnotation := &annotations.Item{Id: 1, DashboardId: 1}
organizationAnnotation := &annotations.Item{Id: 2, DashboardId: 0}
fakeAnnoRepo = NewFakeAnnotationsRepo()
_ = fakeAnnoRepo.Save(dashboardAnnotation)
_ = fakeAnnoRepo.Save(organizationAnnotation)
annotations.SetRepository(fakeAnnoRepo)
_ = sc.hs.annotationsRepo.Save(context.Background(), dashboardAnnotation)
_ = sc.hs.annotationsRepo.Save(context.Background(), organizationAnnotation)
r := callAPI(sc.server, tt.args.method, tt.args.url, tt.args.body, t)
assert.Equalf(t, tt.want, r.Code, "Annotations API(%v)", tt.args.url)
pkg/api/common_test.go
+7 -4
View File
@@ -26,6 +26,7 @@ import (
"github.com/grafana/grafana/pkg/services/accesscontrol/acimpl"
accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/accesscontrol/ossaccesscontrol"
"github.com/grafana/grafana/pkg/services/annotations/annotationstest"
"github.com/grafana/grafana/pkg/services/auth"
"github.com/grafana/grafana/pkg/services/contexthandler"
"github.com/grafana/grafana/pkg/services/contexthandler/authproxy"
@@ -337,10 +338,11 @@ func setupSimpleHTTPServer(features *featuremgmt.FeatureManager) *HTTPServer {
cfg.IsFeatureToggleEnabled = features.IsEnabled
return &HTTPServer{
Cfg: cfg,
Features: features,
License: &licensing.OSSLicensingService{},
AccessControl: accesscontrolmock.New().WithDisabled(),
Cfg: cfg,
Features: features,
License: &licensing.OSSLicensingService{},
AccessControl: accesscontrolmock.New().WithDisabled(),
annotationsRepo: annotationstest.NewFakeAnnotationsRepo(),
}
}
@@ -408,6 +410,7 @@ func setupHTTPServerWithCfgDb(
),
preferenceService: preftest.NewPreferenceServiceFake(),
userService: userMock,
annotationsRepo: annotationstest.NewFakeAnnotationsRepo(),
}
for _, o := range options {
pkg/api/dashboard_test.go
+2 -1
View File
@@ -22,6 +22,7 @@ import (
"github.com/grafana/grafana/pkg/models"
accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/alerting"
"github.com/grafana/grafana/pkg/services/annotations/annotationstest"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/dashboards/database"
"github.com/grafana/grafana/pkg/services/dashboards/service"
@@ -109,7 +110,7 @@ func newTestLive(t *testing.T, store *sqlstore.SQLStore) *live.GrafanaLive {
nil,
&usagestats.UsageStatsMock{T: t},
nil,
features, accesscontrolmock.New(), &dashboards.FakeDashboardService{})
features, accesscontrolmock.New(), &dashboards.FakeDashboardService{}, annotationstest.NewFakeAnnotationsRepo())
require.NoError(t, err)
return gLive
}
pkg/api/http_server.go
+5 -2
View File
@@ -37,6 +37,7 @@ import (
"github.com/grafana/grafana/pkg/plugins/plugincontext"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/alerting"
"github.com/grafana/grafana/pkg/services/annotations"
"github.com/grafana/grafana/pkg/services/apikey"
"github.com/grafana/grafana/pkg/services/cleanup"
"github.com/grafana/grafana/pkg/services/comments"
@@ -189,6 +190,7 @@ type HTTPServer struct {
loginAttemptService loginAttempt.Service
orgService org.Service
accesscontrolService accesscontrol.Service
annotationsRepo annotations.Repository
}
type ServerOptions struct {
@@ -227,7 +229,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
secretsMigrator secrets.Migrator, secretsPluginManager plugins.SecretsPluginManager, secretsService secrets.Service,
secretsPluginMigrator spm.SecretMigrationProvider, secretsStore secretsKV.SecretsKVStore,
publicDashboardsApi *publicdashboardsApi.Api, userService user.Service, tempUserService tempUser.Service, loginAttemptService loginAttempt.Service, orgService org.Service,
accesscontrolService accesscontrol.Service, dashboardThumbsService dashboardThumbs.Service,
accesscontrolService accesscontrol.Service, dashboardThumbsService dashboardThumbs.Service, annotationRepo annotations.Repository,
) (*HTTPServer, error) {
web.Env = cfg.Env
m := web.New()
@@ -323,6 +325,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
loginAttemptService: loginAttemptService,
orgService: orgService,
accesscontrolService: accesscontrolService,
annotationsRepo: annotationRepo,
}
if hs.Listener != nil {
hs.log.Debug("Using provided listener")
@@ -330,7 +333,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
hs.registerRoutes()
// Register access control scope resolver for annotations
hs.AccessControl.RegisterScopeAttributeResolver(AnnotationTypeScopeResolver())
hs.AccessControl.RegisterScopeAttributeResolver(AnnotationTypeScopeResolver(hs.annotationsRepo))
if err := hs.declareFixedRoles(); err != nil {
return nil, err