coremodels: Combine static and generic registries (#53246)

* Stop generating non-dynamic registry code

* Remove generic, errors, s/static/base/

* Sort during codegen, not runtime

* Not a method call

* Precisiate a comment

* Remove generic registry, fix assignability test

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.

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)

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,

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
 }
 `))

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 {

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())

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
+}

pkg/framework/coremodel/registry/registry.go

@@ -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
-}

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
 }