From 77775d548acdc274fe5192ba685b9078ef3fbdc4 Mon Sep 17 00:00:00 2001 From: Arati R <33031346+suntala@users.noreply.github.com> Date: Tue, 30 Jan 2024 14:15:12 +0100 Subject: [PATCH] Chore: Add some tests for the Create method (#81364) * Add test for create method Co-authored-by: Tania B <10127682+undef1nd@users.noreply.github.com> * Change structure of entity package to break import cycle * Update wire file --------- Co-authored-by: Tania B <10127682+undef1nd@users.noreply.github.com> --- pkg/server/wire.go | 5 +- pkg/services/grafana-apiserver/service.go | 4 +- pkg/services/store/entity/db/dbimpl/dbimpl.go | 155 ++++++++++++++++++ .../{ => db}/migrations/entity_store_mig.go | 0 .../entity/{ => db}/migrations/migrator.go | 4 +- pkg/services/store/entity/db/service.go | 152 +---------------- pkg/services/store/entity/server/service.go | 4 +- .../entity/sqlstash/sql_storage_server.go | 15 +- .../sqlstash/sql_storage_server_test.go | 132 +++++++++++++++ 9 files changed, 304 insertions(+), 167 deletions(-) create mode 100644 pkg/services/store/entity/db/dbimpl/dbimpl.go rename pkg/services/store/entity/{ => db}/migrations/entity_store_mig.go (100%) rename pkg/services/store/entity/{ => db}/migrations/migrator.go (91%) create mode 100644 pkg/services/store/entity/sqlstash/sql_storage_server_test.go diff --git a/pkg/server/wire.go b/pkg/server/wire.go index f64b19ff8a8..65458a41d2f 100644 --- a/pkg/server/wire.go +++ b/pkg/server/wire.go @@ -138,6 +138,7 @@ import ( "github.com/grafana/grafana/pkg/services/stats/statsimpl" "github.com/grafana/grafana/pkg/services/store" entityDB "github.com/grafana/grafana/pkg/services/store/entity/db" + "github.com/grafana/grafana/pkg/services/store/entity/db/dbimpl" "github.com/grafana/grafana/pkg/services/store/entity/sqlstash" "github.com/grafana/grafana/pkg/services/store/resolver" "github.com/grafana/grafana/pkg/services/store/sanitizer" @@ -344,8 +345,8 @@ var wireBasicSet = wire.NewSet( grpcserver.ProvideHealthService, grpcserver.ProvideReflectionService, interceptors.ProvideAuthenticator, - entityDB.ProvideEntityDB, - wire.Bind(new(sqlstash.EntityDB), new(*entityDB.EntityDB)), + dbimpl.ProvideEntityDB, + wire.Bind(new(entityDB.EntityDBInterface), new(*dbimpl.EntityDB)), sqlstash.ProvideSQLEntityServer, resolver.ProvideEntityReferenceResolver, teamimpl.ProvideService, diff --git a/pkg/services/grafana-apiserver/service.go b/pkg/services/grafana-apiserver/service.go index c23006132d5..30519427505 100644 --- a/pkg/services/grafana-apiserver/service.go +++ b/pkg/services/grafana-apiserver/service.go @@ -43,7 +43,7 @@ import ( entitystorage "github.com/grafana/grafana/pkg/services/grafana-apiserver/storage/entity" filestorage "github.com/grafana/grafana/pkg/services/grafana-apiserver/storage/file" "github.com/grafana/grafana/pkg/services/store/entity" - entityDB "github.com/grafana/grafana/pkg/services/store/entity/db" + "github.com/grafana/grafana/pkg/services/store/entity/db/dbimpl" "github.com/grafana/grafana/pkg/services/store/entity/sqlstash" "github.com/grafana/grafana/pkg/setting" ) @@ -280,7 +280,7 @@ func (s *service) start(ctx context.Context) error { return fmt.Errorf("unified storage requires the unifiedStorage feature flag (and app_mode = development)") } - eDB, err := entityDB.ProvideEntityDB(s.db, s.cfg, s.features) + eDB, err := dbimpl.ProvideEntityDB(s.db, s.cfg, s.features) if err != nil { return err } diff --git a/pkg/services/store/entity/db/dbimpl/dbimpl.go b/pkg/services/store/entity/db/dbimpl/dbimpl.go new file mode 100644 index 00000000000..ff623fe999e --- /dev/null +++ b/pkg/services/store/entity/db/dbimpl/dbimpl.go @@ -0,0 +1,155 @@ +package dbimpl + +import ( + "fmt" + "strings" + "time" + + "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/sqlstore/session" + entitydb "github.com/grafana/grafana/pkg/services/store/entity/db" + "github.com/grafana/grafana/pkg/services/store/entity/db/migrations" + "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util" + "github.com/jmoiron/sqlx" + "xorm.io/xorm" +) + +var _ entitydb.EntityDBInterface = (*EntityDB)(nil) + +func ProvideEntityDB(db db.DB, cfg *setting.Cfg, features featuremgmt.FeatureToggles) (*EntityDB, error) { + return &EntityDB{ + db: db, + cfg: cfg, + features: features, + log: log.New("entity-db"), + }, nil +} + +type EntityDB struct { + db db.DB + features featuremgmt.FeatureToggles + engine *xorm.Engine + cfg *setting.Cfg + log log.Logger +} + +func (db *EntityDB) Init() error { + _, err := db.GetEngine() + return err +} + +func (db *EntityDB) GetEngine() (*xorm.Engine, error) { + if db.engine != nil { + return db.engine, nil + } + + var engine *xorm.Engine + var err error + + cfgSection := db.cfg.SectionWithEnvOverrides("entity_api") + dbType := cfgSection.Key("db_type").MustString("") + + // if explicit connection settings are provided, use them + if dbType != "" { + dbHost := cfgSection.Key("db_host").MustString("") + dbName := cfgSection.Key("db_name").MustString("") + dbUser := cfgSection.Key("db_user").MustString("") + dbPass := cfgSection.Key("db_pass").MustString("") + + if dbType == "postgres" { + // TODO: support all postgres connection options + dbSslMode := cfgSection.Key("db_sslmode").MustString("disable") + + addr, err := util.SplitHostPortDefault(dbHost, "127.0.0.1", "5432") + if err != nil { + return nil, fmt.Errorf("invalid host specifier '%s': %w", dbHost, err) + } + + connectionString := fmt.Sprintf( + "user=%s password=%s host=%s port=%s dbname=%s sslmode=%s", // sslcert=%s sslkey=%s sslrootcert=%s", + dbUser, dbPass, addr.Host, addr.Port, dbName, dbSslMode, // ss.dbCfg.ClientCertPath, ss.dbCfg.ClientKeyPath, ss.dbCfg.CaCertPath + ) + + engine, err = xorm.NewEngine("postgres", connectionString) + if err != nil { + return nil, err + } + + // FIXME: this config option is cockroachdb-specific, it's not supported by postgres + _, err = engine.Exec("SET SESSION enable_experimental_alter_column_type_general=true") + if err != nil { + db.log.Error("error connecting to postgres", "msg", err.Error()) + // FIXME: return nil, err + } + } else if dbType == "mysql" { + // TODO: support all mysql connection options + protocol := "tcp" + if strings.HasPrefix(dbHost, "/") { + protocol = "unix" + } + + connectionString := fmt.Sprintf("%s:%s@%s(%s)/%s?collation=utf8mb4_unicode_ci&allowNativePasswords=true&clientFoundRows=true", + dbUser, dbPass, protocol, dbHost, dbName) + + engine, err = xorm.NewEngine("mysql", connectionString) + if err != nil { + return nil, err + } + + engine.SetMaxOpenConns(0) + engine.SetMaxIdleConns(2) + engine.SetConnMaxLifetime(time.Second * time.Duration(14400)) + + _, err = engine.Exec("SELECT 1") + if err != nil { + return nil, err + } + } else { + // TODO: sqlite support + return nil, fmt.Errorf("invalid db type specified: %s", dbType) + } + + // configure sql logging + debugSQL := cfgSection.Key("log_queries").MustBool(false) + if !debugSQL { + engine.SetLogger(&xorm.DiscardLogger{}) + } else { + // add stack to database calls to be able to see what repository initiated queries. Top 7 items from the stack as they are likely in the xorm library. + // engine.SetLogger(sqlstore.NewXormLogger(log.LvlInfo, log.WithSuffix(log.New("sqlstore.xorm"), log.CallerContextKey, log.StackCaller(log.DefaultCallerDepth)))) + engine.ShowSQL(true) + engine.ShowExecTime(true) + } + // otherwise, try to use the grafana db connection + } else { + if db.db == nil { + return nil, fmt.Errorf("no db connection provided") + } + + engine = db.db.GetEngine() + } + + db.engine = engine + + if err := migrations.MigrateEntityStore(db, db.features); err != nil { + db.engine = nil + return nil, err + } + + return db.engine, nil +} + +func (db *EntityDB) GetSession() (*session.SessionDB, error) { + engine, err := db.GetEngine() + if err != nil { + return nil, err + } + + return session.GetSession(sqlx.NewDb(engine.DB().DB, engine.DriverName())), nil +} + +func (db *EntityDB) GetCfg() *setting.Cfg { + return db.cfg +} diff --git a/pkg/services/store/entity/migrations/entity_store_mig.go b/pkg/services/store/entity/db/migrations/entity_store_mig.go similarity index 100% rename from pkg/services/store/entity/migrations/entity_store_mig.go rename to pkg/services/store/entity/db/migrations/entity_store_mig.go diff --git a/pkg/services/store/entity/migrations/migrator.go b/pkg/services/store/entity/db/migrations/migrator.go similarity index 91% rename from pkg/services/store/entity/migrations/migrator.go rename to pkg/services/store/entity/db/migrations/migrator.go index edb1964a2ae..ecfac6fa22b 100644 --- a/pkg/services/store/entity/migrations/migrator.go +++ b/pkg/services/store/entity/db/migrations/migrator.go @@ -7,10 +7,10 @@ import ( "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" "github.com/grafana/grafana/pkg/services/sqlstore/session" - "github.com/grafana/grafana/pkg/services/store/entity/sqlstash" + "github.com/grafana/grafana/pkg/services/store/entity/db" ) -func MigrateEntityStore(db sqlstash.EntityDB, features featuremgmt.FeatureToggles) error { +func MigrateEntityStore(db db.EntityDBInterface, features featuremgmt.FeatureToggles) error { // Skip if feature flag is not enabled if !features.IsEnabledGlobally(featuremgmt.FlagUnifiedStorage) { return nil diff --git a/pkg/services/store/entity/db/service.go b/pkg/services/store/entity/db/service.go index 17d4a74dc0a..7a5414dacd6 100755 --- a/pkg/services/store/entity/db/service.go +++ b/pkg/services/store/entity/db/service.go @@ -1,158 +1,16 @@ package db import ( - "fmt" - "strings" - "time" - - "github.com/jmoiron/sqlx" "xorm.io/xorm" - "github.com/grafana/grafana/pkg/infra/db" - "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/services/featuremgmt" - // "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/session" - "github.com/grafana/grafana/pkg/services/store/entity/migrations" - "github.com/grafana/grafana/pkg/services/store/entity/sqlstash" "github.com/grafana/grafana/pkg/setting" - "github.com/grafana/grafana/pkg/util" ) -var _ sqlstash.EntityDB = (*EntityDB)(nil) - -func ProvideEntityDB(db db.DB, cfg *setting.Cfg, features featuremgmt.FeatureToggles) (*EntityDB, error) { - return &EntityDB{ - db: db, - cfg: cfg, - features: features, - log: log.New("entity-db"), - }, nil -} - -type EntityDB struct { - db db.DB - features featuremgmt.FeatureToggles - engine *xorm.Engine - cfg *setting.Cfg - log log.Logger -} - -func (db *EntityDB) Init() error { - _, err := db.GetEngine() - return err -} - -func (db *EntityDB) GetEngine() (*xorm.Engine, error) { - if db.engine != nil { - return db.engine, nil - } - - var engine *xorm.Engine - var err error - - cfgSection := db.cfg.SectionWithEnvOverrides("entity_api") - dbType := cfgSection.Key("db_type").MustString("") - - // if explicit connection settings are provided, use them - if dbType != "" { - dbHost := cfgSection.Key("db_host").MustString("") - dbName := cfgSection.Key("db_name").MustString("") - dbUser := cfgSection.Key("db_user").MustString("") - dbPass := cfgSection.Key("db_pass").MustString("") - - if dbType == "postgres" { - // TODO: support all postgres connection options - dbSslMode := cfgSection.Key("db_sslmode").MustString("disable") - - addr, err := util.SplitHostPortDefault(dbHost, "127.0.0.1", "5432") - if err != nil { - return nil, fmt.Errorf("invalid host specifier '%s': %w", dbHost, err) - } - - connectionString := fmt.Sprintf( - "user=%s password=%s host=%s port=%s dbname=%s sslmode=%s", // sslcert=%s sslkey=%s sslrootcert=%s", - dbUser, dbPass, addr.Host, addr.Port, dbName, dbSslMode, // ss.dbCfg.ClientCertPath, ss.dbCfg.ClientKeyPath, ss.dbCfg.CaCertPath - ) - - engine, err = xorm.NewEngine("postgres", connectionString) - if err != nil { - return nil, err - } - - // FIXME: this config option is cockroachdb-specific, it's not supported by postgres - _, err = engine.Exec("SET SESSION enable_experimental_alter_column_type_general=true") - if err != nil { - db.log.Error("error connecting to postgres", "msg", err.Error()) - // FIXME: return nil, err - } - } else if dbType == "mysql" { - // TODO: support all mysql connection options - protocol := "tcp" - if strings.HasPrefix(dbHost, "/") { - protocol = "unix" - } - - connectionString := fmt.Sprintf("%s:%s@%s(%s)/%s?collation=utf8mb4_unicode_ci&allowNativePasswords=true&clientFoundRows=true", - dbUser, dbPass, protocol, dbHost, dbName) - - engine, err = xorm.NewEngine("mysql", connectionString) - if err != nil { - return nil, err - } - - engine.SetMaxOpenConns(0) - engine.SetMaxIdleConns(2) - engine.SetConnMaxLifetime(time.Second * time.Duration(14400)) - - _, err = engine.Exec("SELECT 1") - if err != nil { - return nil, err - } - } else { - // TODO: sqlite support - return nil, fmt.Errorf("invalid db type specified: %s", dbType) - } - - // configure sql logging - debugSQL := cfgSection.Key("log_queries").MustBool(false) - if !debugSQL { - engine.SetLogger(&xorm.DiscardLogger{}) - } else { - // add stack to database calls to be able to see what repository initiated queries. Top 7 items from the stack as they are likely in the xorm library. - // engine.SetLogger(sqlstore.NewXormLogger(log.LvlInfo, log.WithSuffix(log.New("sqlstore.xorm"), log.CallerContextKey, log.StackCaller(log.DefaultCallerDepth)))) - engine.ShowSQL(true) - engine.ShowExecTime(true) - } - // otherwise, try to use the grafana db connection - } else { - if db.db == nil { - return nil, fmt.Errorf("no db connection provided") - } - - engine = db.db.GetEngine() - } - - db.engine = engine - - if err := migrations.MigrateEntityStore(db, db.features); err != nil { - db.engine = nil - return nil, err - } - - return db.engine, nil -} - -func (db *EntityDB) GetSession() (*session.SessionDB, error) { - engine, err := db.GetEngine() - if err != nil { - return nil, err - } - - return session.GetSession(sqlx.NewDb(engine.DB().DB, engine.DriverName())), nil -} - -func (db *EntityDB) GetCfg() *setting.Cfg { - return db.cfg +type EntityDBInterface interface { + Init() error + GetSession() (*session.SessionDB, error) + GetEngine() (*xorm.Engine, error) + GetCfg() *setting.Cfg } diff --git a/pkg/services/store/entity/server/service.go b/pkg/services/store/entity/server/service.go index 10a62adddfb..c5ae0b429f9 100644 --- a/pkg/services/store/entity/server/service.go +++ b/pkg/services/store/entity/server/service.go @@ -18,7 +18,7 @@ import ( "github.com/grafana/grafana/pkg/services/grpcserver" "github.com/grafana/grafana/pkg/services/grpcserver/interceptors" "github.com/grafana/grafana/pkg/services/store/entity" - entityDB "github.com/grafana/grafana/pkg/services/store/entity/db" + "github.com/grafana/grafana/pkg/services/store/entity/db/dbimpl" "github.com/grafana/grafana/pkg/services/store/entity/sqlstash" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" @@ -147,7 +147,7 @@ func (s *service) start(ctx context.Context) error { // TODO: use wire // TODO: support using grafana db connection? - eDB, err := entityDB.ProvideEntityDB(nil, s.cfg, s.features) + eDB, err := dbimpl.ProvideEntityDB(nil, s.cfg, s.features) if err != nil { return err } diff --git a/pkg/services/store/entity/sqlstash/sql_storage_server.go b/pkg/services/store/entity/sqlstash/sql_storage_server.go index 19804b9ea24..be233187f61 100644 --- a/pkg/services/store/entity/sqlstash/sql_storage_server.go +++ b/pkg/services/store/entity/sqlstash/sql_storage_server.go @@ -10,8 +10,6 @@ import ( "strings" "time" - "xorm.io/xorm" - "github.com/bwmarrin/snowflake" "github.com/google/uuid" @@ -22,20 +20,13 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore/session" "github.com/grafana/grafana/pkg/services/store" "github.com/grafana/grafana/pkg/services/store/entity" - "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/services/store/entity/db" ) -type EntityDB interface { - Init() error - GetSession() (*session.SessionDB, error) - GetEngine() (*xorm.Engine, error) - GetCfg() *setting.Cfg -} - // Make sure we implement both store + admin var _ entity.EntityStoreServer = &sqlEntityServer{} -func ProvideSQLEntityServer(db EntityDB /*, cfg *setting.Cfg */) (entity.EntityStoreServer, error) { +func ProvideSQLEntityServer(db db.EntityDBInterface /*, cfg *setting.Cfg */) (entity.EntityStoreServer, error) { snode, err := snowflake.NewNode(rand.Int63n(1024)) if err != nil { return nil, err @@ -52,7 +43,7 @@ func ProvideSQLEntityServer(db EntityDB /*, cfg *setting.Cfg */) (entity.EntityS type sqlEntityServer struct { log log.Logger - db EntityDB // needed to keep xorm engine in scope + db db.EntityDBInterface // needed to keep xorm engine in scope sess *session.SessionDB dialect migrator.Dialect snowflake *snowflake.Node diff --git a/pkg/services/store/entity/sqlstash/sql_storage_server_test.go b/pkg/services/store/entity/sqlstash/sql_storage_server_test.go new file mode 100644 index 00000000000..430731131b3 --- /dev/null +++ b/pkg/services/store/entity/sqlstash/sql_storage_server_test.go @@ -0,0 +1,132 @@ +package sqlstash + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/store/entity" + "github.com/grafana/grafana/pkg/services/store/entity/db/dbimpl" + "github.com/grafana/grafana/pkg/setting" +) + +func TestCreate(t *testing.T) { + s := setUpTestServer(t) + + tests := []struct { + name string + ent *entity.Entity + errIsExpected bool + statusIsExpected bool + }{ + { + "request with key and entity creator", + &entity.Entity{ + Group: "playlist.grafana.app", + Resource: "playlists", + Namespace: "default", + Name: "set-minimum-uid", + Key: "/playlist.grafana.app/playlists/default/set-minimum-uid", + CreatedBy: "set-minimum-creator", + }, + false, + true, + }, + { + "request with no entity creator", + &entity.Entity{ + Key: "/playlist.grafana.app/playlists/default/set-only-key", + }, + true, + false, + }, + { + "request with no key", + &entity.Entity{ + CreatedBy: "entity-creator", + }, + true, + true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req := entity.CreateEntityRequest{ + Entity: &entity.Entity{ + Key: tc.ent.Key, + CreatedBy: tc.ent.CreatedBy, + }, + } + resp, err := s.Create(context.Background(), &req) + + if tc.errIsExpected { + require.Error(t, err) + + if tc.statusIsExpected { + require.Equal(t, entity.CreateEntityResponse_ERROR, resp.Status) + } + + return + } + + require.Nil(t, err) + require.Equal(t, entity.CreateEntityResponse_CREATED, resp.Status) + require.NotNil(t, resp) + require.Nil(t, resp.Error) + + read, err := s.Read(context.Background(), &entity.ReadEntityRequest{ + Key: tc.ent.Key, + }) + require.NoError(t, err) + require.NotNil(t, read) + + require.Greater(t, len(read.Guid), 0) + require.Greater(t, read.ResourceVersion, int64(0)) + + expectedETag := createContentsHash(tc.ent.Body, tc.ent.Meta, tc.ent.Status) + require.Equal(t, expectedETag, read.ETag) + require.Equal(t, tc.ent.Origin, read.Origin) + require.Equal(t, tc.ent.Group, read.Group) + require.Equal(t, tc.ent.Resource, read.Resource) + require.Equal(t, tc.ent.Namespace, read.Namespace) + require.Equal(t, tc.ent.Name, read.Name) + require.Equal(t, tc.ent.Subresource, read.Subresource) + require.Equal(t, tc.ent.GroupVersion, read.GroupVersion) + require.Equal(t, tc.ent.Key, read.Key) + require.Equal(t, tc.ent.Folder, read.Folder) + require.Equal(t, tc.ent.Meta, read.Meta) + require.Equal(t, tc.ent.Body, read.Body) + require.Equal(t, tc.ent.Status, read.Status) + require.Equal(t, tc.ent.Title, read.Title) + require.Equal(t, tc.ent.Size, read.Size) + require.Equal(t, tc.ent.CreatedAt, read.CreatedAt) + require.Equal(t, tc.ent.CreatedBy, read.CreatedBy) + require.Equal(t, tc.ent.UpdatedAt, read.UpdatedAt) + require.Equal(t, tc.ent.UpdatedBy, read.UpdatedBy) + require.Equal(t, tc.ent.Description, read.Description) + require.Equal(t, tc.ent.Slug, read.Slug) + require.Equal(t, tc.ent.Message, read.Message) + require.Equal(t, tc.ent.Labels, read.Labels) + require.Equal(t, tc.ent.Fields, read.Fields) + require.Equal(t, tc.ent.Errors, read.Errors) + }) + } +} + +func setUpTestServer(t *testing.T) entity.EntityStoreServer { + sqlStore := db.InitTestDB(t) + + entityDB, err := dbimpl.ProvideEntityDB( + sqlStore, + setting.NewCfg(), + featuremgmt.WithFeatures(featuremgmt.FlagUnifiedStorage)) + require.NoError(t, err) + + s, err := ProvideSQLEntityServer(entityDB) + require.NoError(t, err) + return s +}