diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index 2800cc7099b..e86e1018d85 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -353,7 +353,7 @@ func (hs *HTTPServer) PostDashboard(c *models.ReqContext) response.Response { } if hs.Features.IsEnabled(featuremgmt.FlagValidateDashboardsOnSave) { - cm := hs.CoremodelStaticRegistry.Dashboard() + cm := hs.Coremodels.Dashboard() // Ideally, coremodel validation calls would be integrated into the web // framework. But this does the job for now. diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index 261cfc185b9..bf6d3ed5711 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -57,8 +57,8 @@ func TestGetHomeDashboard(t *testing.T) { SQLStore: mockstore.NewSQLStoreMock(), preferenceService: prefService, dashboardVersionService: dashboardVersionService, + Coremodels: registry.NewBase(), } - hs.CoremodelStaticRegistry, hs.CoremodelRegistry = setupDashboardCoremodel(t) tests := []struct { name string @@ -141,8 +141,8 @@ func TestDashboardAPIEndpoint(t *testing.T) { Features: featuremgmt.WithFeatures(), DashboardService: dashboardService, dashboardVersionService: fakeDashboardVersionService, + Coremodels: registry.NewBase(), } - hs.CoremodelStaticRegistry, hs.CoremodelRegistry = setupDashboardCoremodel(t) setUp := func() { viewerRole := models.ROLE_VIEWER @@ -262,8 +262,8 @@ func TestDashboardAPIEndpoint(t *testing.T) { DashboardService: dashboardService, dashboardVersionService: fakeDashboardVersionService, Features: featuremgmt.WithFeatures(), + Coremodels: registry.NewBase(), } - hs.CoremodelStaticRegistry, hs.CoremodelRegistry = setupDashboardCoremodel(t) setUp := func() { origCanEdit := setting.ViewersCanEdit @@ -903,8 +903,8 @@ func TestDashboardAPIEndpoint(t *testing.T) { AccessControl: accesscontrolmock.New(), DashboardService: dashboardService, Features: featuremgmt.WithFeatures(), + Coremodels: registry.NewBase(), } - hs.CoremodelStaticRegistry, hs.CoremodelRegistry = setupDashboardCoremodel(t) hs.callGetDashboard(sc) assert.Equal(t, 200, sc.resp.Code) @@ -958,8 +958,8 @@ func getDashboardShouldReturn200WithConfig(t *testing.T, sc *scenarioContext, pr ), DashboardService: dashboardService, Features: featuremgmt.WithFeatures(), + Coremodels: registry.NewBase(), } - hs.CoremodelStaticRegistry, hs.CoremodelRegistry = setupDashboardCoremodel(t) hs.callGetDashboard(sc) @@ -1024,8 +1024,8 @@ func postDashboardScenario(t *testing.T, desc string, url string, routePattern s DashboardService: dashboardService, folderService: folderService, Features: featuremgmt.WithFeatures(), + Coremodels: registry.NewBase(), } - hs.CoremodelStaticRegistry, hs.CoremodelRegistry = setupDashboardCoremodel(t) sc := setupScenarioContext(t, url) sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { @@ -1057,8 +1057,8 @@ func postDiffScenario(t *testing.T, desc string, url string, routePattern string SQLStore: sqlmock, dashboardVersionService: fakeDashboardVersionService, Features: featuremgmt.WithFeatures(), + Coremodels: registry.NewBase(), } - hs.CoremodelStaticRegistry, hs.CoremodelRegistry = setupDashboardCoremodel(t) sc := setupScenarioContext(t, url) sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response { @@ -1096,8 +1096,8 @@ func restoreDashboardVersionScenario(t *testing.T, desc string, url string, rout SQLStore: sqlStore, Features: featuremgmt.WithFeatures(), dashboardVersionService: fakeDashboardVersionService, + Coremodels: registry.NewBase(), } - hs.CoremodelStaticRegistry, hs.CoremodelRegistry = setupDashboardCoremodel(t) sc := setupScenarioContext(t, url) sc.sqlStore = sqlStore @@ -1121,16 +1121,6 @@ func restoreDashboardVersionScenario(t *testing.T, desc string, url string, rout }) } -func setupDashboardCoremodel(t *testing.T) (*registry.Static, *registry.Generic) { - // TODO abstract and generalize this further for wider reuse - t.Helper() - sreg, err := registry.ProvideStatic() - require.NoError(t, err) - greg, err := registry.ProvideGeneric() - require.NoError(t, err) - return sreg, greg -} - func (sc *scenarioContext) ToJSON() *simplejson.Json { result := simplejson.New() err := json.NewDecoder(sc.resp.Body).Decode(result) diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 86c09c037e4..e1014e1149c 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -171,10 +171,9 @@ type HTTPServer struct { dashboardVersionService dashver.Service PublicDashboardsApi *publicdashboardsApi.Api starService star.Service + Coremodels *registry.Base playlistService playlist.Service apiKeyService apikey.Service - CoremodelRegistry *registry.Generic - CoremodelStaticRegistry *registry.Static kvStore kvstore.KVStore secretsMigrator secrets.Migrator userService user.Service @@ -211,7 +210,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi avatarCacheServer *avatar.AvatarCacheServer, preferenceService pref.Service, teamsPermissionsService accesscontrol.TeamPermissionsService, folderPermissionsService accesscontrol.FolderPermissionsService, dashboardPermissionsService accesscontrol.DashboardPermissionsService, dashboardVersionService dashver.Service, - starService star.Service, csrfService csrf.Service, coremodelRegistry *registry.Generic, coremodelStaticRegistry *registry.Static, + starService star.Service, csrfService csrf.Service, coremodels *registry.Base, playlistService playlist.Service, apiKeyService apikey.Service, kvStore kvstore.KVStore, secretsMigrator secrets.Migrator, remoteSecretsCheck secretsKV.UseRemoteSecretsPluginCheck, publicDashboardsApi *publicdashboardsApi.Api, userService user.Service) (*HTTPServer, error) { web.Env = cfg.Env @@ -294,10 +293,9 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi dashboardPermissionsService: dashboardPermissionsService, dashboardVersionService: dashboardVersionService, starService: starService, + Coremodels: coremodels, playlistService: playlistService, apiKeyService: apiKeyService, - CoremodelRegistry: coremodelRegistry, - CoremodelStaticRegistry: coremodelStaticRegistry, kvStore: kvStore, PublicDashboardsApi: publicDashboardsApi, secretsMigrator: secretsMigrator, diff --git a/pkg/codegen/coremodel.go b/pkg/codegen/coremodel.go index 34e385586f1..0c6e6c7b32a 100644 --- a/pkg/codegen/coremodel.go +++ b/pkg/codegen/coremodel.go @@ -281,8 +281,8 @@ func (m modelReplacer) replacePrefix(str string) string { return str } -// GenerateCoremodelRegistry produces Go files that define a static registry -// with references to all the Go code that is expected to be generated from the +// GenerateCoremodelRegistry produces Go files that define a registry with +// references to all the Go code that is expected to be generated from the // provided lineages. func GenerateCoremodelRegistry(path string, ecl []*ExtractedLineage) (WriteDiffer, error) { var cml []tplVars @@ -409,6 +409,7 @@ var tmplRegistry = template.Must(template.New("registry").Parse(` package registry import ( + "fmt" "sync" "github.com/google/wire" @@ -419,25 +420,17 @@ import ( "github.com/grafana/thema" ) -// CoremodelSet contains all of the wire-style providers related to coremodels. -var CoremodelSet = wire.NewSet( - ProvideStatic, - ProvideGeneric, -) - -var ( - staticOnce sync.Once - defaultStatic *Static - defaultStaticErr error - - genericOnce sync.Once - defaultGeneric *Generic - defaultGenericErr error -) - -// Static is a registry that provides access to individual coremodels via -// explicit method calls, to aid with static analysis. -type Static struct { +// Base is a registry of coremodel.Interface. It provides two modes for accessing +// coremodels: individually via literal named methods, or as a slice returned from All(). +// +// Prefer the individual named methods for use cases where the particular coremodel(s) that +// are needed are known to the caller. For example, a dashboard linter can know that it +// specifically wants the dashboard coremodel. +// +// Prefer All() when performing operations generically across all coremodels. For example, +// a validation HTTP middleware for any coremodel-schematized object type. +type Base struct { + all []coremodel.Interface {{- range .Coremodels }} {{ .Name }} *{{ .Name }}.Coremodel{{end}} } @@ -451,51 +444,23 @@ var ( {{range .Coremodels }} // {{ .TitleName }} returns the {{ .Name }} coremodel. The return value is guaranteed to // implement coremodel.Interface. -func (s *Static) {{ .TitleName }}() *{{ .Name }}.Coremodel { +func (s *Base) {{ .TitleName }}() *{{ .Name }}.Coremodel { return s.{{ .Name }} } {{end}} -func provideStatic(lib *thema.Library) (*Static, error) { - if lib == nil { - staticOnce.Do(func() { - defaultStatic, defaultStaticErr = doProvideStatic(cuectx.ProvideThemaLibrary()) - }) - return defaultStatic, defaultStaticErr - } - - return doProvideStatic(*lib) -} - -func doProvideStatic(lib thema.Library) (*Static, error) { +func doProvideBase(lib thema.Library) *Base { var err error - reg := &Static{} + reg := &Base{} {{range .Coremodels }} reg.{{ .Name }}, err = {{ .Name }}.New(lib) if err != nil { - return nil, err + panic(fmt.Sprintf("error while initializing {{ .Name }} coremodel: %s", err)) } + reg.all = append(reg.all, reg.{{ .Name }}) {{end}} - return reg, nil -} - -func provideGeneric() (*Generic, error) { - ereg, err := provideStatic(nil) - if err != nil { - return nil, err - } - - genericOnce.Do(func() { - defaultGeneric, defaultGenericErr = doProvideGeneric(ereg) - }) - return defaultGeneric, defaultGenericErr -} - -func doProvideGeneric(ereg *Static) (*Generic, error) { - return NewRegistry({{ range .Coremodels }} - ereg.{{ .TitleName }}(),{{ end }} - ) + return reg } `)) diff --git a/pkg/framework/coremodel/gen.go b/pkg/framework/coremodel/gen.go index 513f46a81dd..07cec0c98a5 100644 --- a/pkg/framework/coremodel/gen.go +++ b/pkg/framework/coremodel/gen.go @@ -9,6 +9,7 @@ import ( "io/ioutil" "os" "path/filepath" + "sort" "strings" "cuelang.org/go/cue/cuecontext" @@ -59,6 +60,9 @@ func main() { lins = append(lins, lin) } } + sort.Slice(lins, func(i, j int) bool { + return lins[i].Lineage.Name() < lins[j].Lineage.Name() + }) wd := gcgen.NewWriteDiffer() for _, ls := range lins { diff --git a/pkg/framework/coremodel/registry/assignability_test.go b/pkg/framework/coremodel/registry/assignability_test.go index d96cb0f95ef..38f51f8a30c 100644 --- a/pkg/framework/coremodel/registry/assignability_test.go +++ b/pkg/framework/coremodel/registry/assignability_test.go @@ -8,12 +8,9 @@ import ( ) func TestSchemaAssignability(t *testing.T) { - reg, err := registry.ProvideGeneric() - if err != nil { - t.Fatal(err) - } + reg := registry.NewBase() - for _, cm := range reg.List() { + for _, cm := range reg.All() { tcm := cm t.Run(tcm.Lineage().Name(), func(t *testing.T) { err := thema.AssignableTo(tcm.CurrentSchema(), tcm.GoType()) diff --git a/pkg/framework/coremodel/registry/provide.go b/pkg/framework/coremodel/registry/provide.go index d8da7315005..a4b0db4b63f 100644 --- a/pkg/framework/coremodel/registry/provide.go +++ b/pkg/framework/coremodel/registry/provide.go @@ -1,40 +1,59 @@ package registry import ( + "sync" + + "github.com/google/wire" + "github.com/grafana/grafana/pkg/cuectx" + "github.com/grafana/grafana/pkg/framework/coremodel" "github.com/grafana/thema" ) -// ProvideStatic provides access to individual coremodels via explicit method calls. +// CoremodelSet contains all of the wire-style providers related to coremodels. +var CoremodelSet = wire.NewSet( + NewBase, +) + +// NewBase provides a registry of all coremodels, without any composition of +// plugin-defined schemas. // -// Prefer this to the ProvideGeneric type when your code works with known, -// specific coremodels(s), rather than generically across all of them. This -// allows standard Go static analysis tools to determine which code is depending -// on particular coremodels. -// -// This will use the default Grafana thema.Library, defined in pkg/cuectx, which -// will avoid duplicate parsing of Thema CUE schemas. If you need control over the -// thema.Library in use, use ProvideStaticWithLib instead. -func ProvideStatic() (*Static, error) { - return provideStatic(nil) +// The returned registry will use the default Grafana thema.Library, defined in +// pkg/cuectx. If you need control over the thema.Library used by the coremodel +// lineages, use NewBaseWithLib instead. +func NewBase() *Base { + return provideBase(nil) } -// ProvideStaticWithLib is the same as ProvideStatic, but -// allows control over the thema.Library used to initialize the underlying -// coremodels. +// NewBaseWithLib is the same as NewBase, but allows control over the +// thema.Library used to initialize the underlying coremodels. // -// Prefer ProvideStatic unless you absolutely need this control. -func ProvideStaticWithLib(lib thema.Library) (*Static, error) { - return provideStatic(&lib) +// Prefer NewBase unless you absolutely need this control. +func NewBaseWithLib(lib thema.Library) *Base { + return provideBase(&lib) } -// ProvideGeneric provides a simple Generic registry of all coremodels. -// -// Prefer this to the static ProvideStatic when your code needs to -// work with all coremodels generically, rather than specific coremodels. -func ProvideGeneric() (*Generic, error) { - return provideGeneric() +var ( + baseOnce sync.Once + defaultBase *Base +) + +func provideBase(lib *thema.Library) *Base { + if lib == nil { + baseOnce.Do(func() { + defaultBase = doProvideBase(cuectx.ProvideThemaLibrary()) + }) + return defaultBase + } + + return doProvideBase(*lib) } -// NOTE - no ProvideRegistryWithLib is defined because there are no anticipated -// cases where a caller would need to operate generically across all coremodels, -// and control the library they're initialized with. If that changes, add one. +// All returns a slice of all registered coremodels. +// +// Prefer this method when operating generically across all coremodels. +// +// The returned slice is sorted lexicographically by coremodel name. It should +// not be modified. +func (s *Base) All() []coremodel.Interface { + return s.all +} diff --git a/pkg/framework/coremodel/registry/registry.go b/pkg/framework/coremodel/registry/registry.go deleted file mode 100644 index 09c6a703233..00000000000 --- a/pkg/framework/coremodel/registry/registry.go +++ /dev/null @@ -1,80 +0,0 @@ -package registry - -import ( - "errors" - "fmt" - "sync" - - "github.com/grafana/grafana/pkg/framework/coremodel" - "github.com/grafana/thema" -) - -var ( - // ErrModelAlreadyRegistered is returned when trying to register duplicate model to Generic. - ErrModelAlreadyRegistered = errors.New("error registering duplicate model") -) - -// Generic is a registry of coremodel instances. It is intended for use in cases where -// generic operations limited to coremodel.Interface are being performed. -type Generic struct { - lock sync.RWMutex - models []coremodel.Interface - modelIdx map[string]coremodel.Interface -} - -// NewRegistry returns a new Generic with the provided coremodel instances. -func NewRegistry(models ...coremodel.Interface) (*Generic, error) { - r := &Generic{ - models: make([]coremodel.Interface, 0, len(models)), - modelIdx: make(map[string]coremodel.Interface, len(models)), - } - - if err := r.addModels(models); err != nil { - return nil, err - } - - return r, nil -} - -// Register adds coremodels to the Generic. -func (r *Generic) Register(models ...coremodel.Interface) error { - return r.addModels(models) -} - -// List returns all coremodels registered in this Generic. -func (r *Generic) List() []coremodel.Interface { - r.lock.RLock() - defer r.lock.RUnlock() - - return r.models -} - -func (r *Generic) addModels(models []coremodel.Interface) error { - r.lock.Lock() - defer r.lock.Unlock() - - // Update model index and return an error if trying to register a duplicate. - for _, m := range models { - k := m.Lineage().Name() - - // Ensure assignability first. TODO will this blow up for dashboards? - if err := thema.AssignableTo(m.CurrentSchema(), m.GoType()); err != nil { - return fmt.Errorf("%s schema version %v not assignable to provided Go type: %w", k, m.CurrentSchema().Version(), err) - } - - if _, ok := r.modelIdx[k]; ok { - return ErrModelAlreadyRegistered - } - - r.modelIdx[k] = m - } - - // Remake model list. - // TODO: this can be more performant (proper resizing, maybe single loop with index building, etc.). - r.models = r.models[:0] - for _, m := range r.modelIdx { - r.models = append(r.models, m) - } - - return nil -} diff --git a/pkg/framework/coremodel/registry/registry_gen.go b/pkg/framework/coremodel/registry/registry_gen.go index 6d06af5b179..cdca94666c1 100644 --- a/pkg/framework/coremodel/registry/registry_gen.go +++ b/pkg/framework/coremodel/registry/registry_gen.go @@ -6,35 +6,24 @@ package registry import ( - "sync" - - "github.com/google/wire" + "fmt" "github.com/grafana/grafana/pkg/coremodel/dashboard" - "github.com/grafana/grafana/pkg/cuectx" "github.com/grafana/grafana/pkg/framework/coremodel" "github.com/grafana/thema" ) -// CoremodelSet contains all of the wire-style providers related to coremodels. -var CoremodelSet = wire.NewSet( - ProvideStatic, - ProvideGeneric, -) - -var ( - staticOnce sync.Once - defaultStatic *Static - defaultStaticErr error - - genericOnce sync.Once - defaultGeneric *Generic - defaultGenericErr error -) - -// Static is a registry that provides access to individual coremodels via -// explicit method calls, to aid with static analysis. -type Static struct { +// Base is a registry of coremodel.Interface. It provides two modes for accessing +// coremodels: individually via literal named methods, or as a slice returned from All(). +// +// Prefer the individual named methods for use cases where the particular coremodel(s) that +// are needed are known to the caller. For example, a dashboard linter can know that it +// specifically wants the dashboard coremodel. +// +// Prefer All() when performing operations generically across all coremodels. For example, +// a validation HTTP middleware for any coremodel-schematized object type. +type Base struct { + all []coremodel.Interface dashboard *dashboard.Coremodel } @@ -45,47 +34,19 @@ var ( // Dashboard returns the dashboard coremodel. The return value is guaranteed to // implement coremodel.Interface. -func (s *Static) Dashboard() *dashboard.Coremodel { +func (s *Base) Dashboard() *dashboard.Coremodel { return s.dashboard } -func provideStatic(lib *thema.Library) (*Static, error) { - if lib == nil { - staticOnce.Do(func() { - defaultStatic, defaultStaticErr = doProvideStatic(cuectx.ProvideThemaLibrary()) - }) - return defaultStatic, defaultStaticErr - } - - return doProvideStatic(*lib) -} - -func doProvideStatic(lib thema.Library) (*Static, error) { +func doProvideBase(lib thema.Library) *Base { var err error - reg := &Static{} + reg := &Base{} reg.dashboard, err = dashboard.New(lib) if err != nil { - return nil, err + panic(fmt.Sprintf("error while initializing dashboard coremodel: %s", err)) } + reg.all = append(reg.all, reg.dashboard) - return reg, nil -} - -func provideGeneric() (*Generic, error) { - ereg, err := provideStatic(nil) - if err != nil { - return nil, err - } - - genericOnce.Do(func() { - defaultGeneric, defaultGenericErr = doProvideGeneric(ereg) - }) - return defaultGeneric, defaultGenericErr -} - -func doProvideGeneric(ereg *Static) (*Generic, error) { - return NewRegistry( - ereg.Dashboard(), - ) + return reg }